diff --git a/routes/vault_routes.py b/routes/vault_routes.py index b04ba33..c6258bb 100644 --- a/routes/vault_routes.py +++ b/routes/vault_routes.py @@ -83,10 +83,9 @@ async def _run_bw(args: list, session: str = None, input_text: str = None, if session: env["BW_SESSION"] = session # Secrets must never be passed as argv — process arguments are world-readable - # via `ps` / `/proc//cmdline` to any local user. Hand the master password - # to `bw` through the environment instead (paired with `--passwordenv - # BW_PASSWORD` in args); /proc//environ is readable only by the process - # owner. This mirrors how BW_SESSION is already passed above. + # via `ps` / `/proc//cmdline` to any local user. Keep --passwordenv + # support for bw commands that need it; unlock/login callers should prefer + # stdin so the master password is not left in the child environment either. if bw_password is not None: env["BW_PASSWORD"] = bw_password bw_path = _find_bw() @@ -184,13 +183,12 @@ def setup_vault_routes(): async def unlock(req: VaultUnlockRequest, request: Request): """Unlock the vault and save the session key.""" require_admin(request) - # Pass the master password via the environment (--passwordenv), NOT as - # an argv element — argv is visible to every local user through `ps` / - # /proc//cmdline. (The sibling /login handler already keeps the - # password off argv by feeding it on stdin.) + # Pass the master password on stdin, not argv. argv is visible through + # `ps` / /proc//cmdline; stdin also avoids leaving the secret in + # the child process environment. stdout, stderr, rc = await _run_bw( - ["unlock", "--passwordenv", "BW_PASSWORD", "--raw"], - bw_password=req.master_password, + ["unlock", "--raw"], + input_text=req.master_password + "\n", ) if rc != 0: return {"ok": False, "error": f"Unlock failed: {stderr[:300]}"} diff --git a/src/tool_implementations.py b/src/tool_implementations.py index 3ad8d58..3847cf8 100644 --- a/src/tool_implementations.py +++ b/src/tool_implementations.py @@ -4094,7 +4094,9 @@ async def do_vault_unlock(content: str, owner: Optional[str] = None) -> Dict: if not master_password: return {"error": "master_password is required", "exit_code": 1} - stdout, stderr, rc = await _run_bw(["unlock", master_password, "--raw"]) + # Do not pass the master password as an argv element. Local process lists + # can expose argv to other users; stdin keeps the secret out of `ps`. + stdout, stderr, rc = await _run_bw(["unlock", "--raw"], input_text=master_password + "\n") if rc != 0: return {"error": f"Unlock failed: {stderr[:300]}", "exit_code": 1} diff --git a/tests/test_vault_password_not_in_argv.py b/tests/test_vault_password_not_in_argv.py index 2e05ed7..32267a9 100644 --- a/tests/test_vault_password_not_in_argv.py +++ b/tests/test_vault_password_not_in_argv.py @@ -4,8 +4,8 @@ ``asyncio.create_subprocess_exec(bw_path, *args)`` — every element of ``args`` becomes a process argument, which is world-readable through ``ps`` / ``/proc//cmdline``. The master password therefore must be handed to ``bw`` -via the environment (``--passwordenv BW_PASSWORD``), exactly like the existing -``BW_SESSION`` env passing, and never as a positional argv element. +out-of-band (stdin or ``--passwordenv BW_PASSWORD``), and never as a positional +argv element. The /unlock route previously did ``_run_bw(["unlock", req.master_password, "--raw"])`` — leaking the Bitwarden master password (which decrypts the whole @@ -68,7 +68,7 @@ def _patch_exec(monkeypatch): @pytest.mark.asyncio -async def test_run_bw_password_goes_to_env_not_argv(monkeypatch): +async def test_run_bw_passwordenv_does_not_put_password_in_argv(monkeypatch): captured = _patch_exec(monkeypatch) secret = "correct horse battery staple" await vr._run_bw(["unlock", "--passwordenv", "BW_PASSWORD", "--raw"], @@ -87,18 +87,26 @@ async def test_run_bw_without_password_does_not_set_env(monkeypatch): assert "BW_PASSWORD" not in captured["env"] -def test_unlock_handler_uses_passwordenv_not_argv(): +def test_unlock_handler_feeds_password_on_stdin_not_argv(): """Source-level guard: the /unlock route must feed the master password via - --passwordenv / bw_password=, never as a bare positional argv element.""" + stdin, never as a bare positional argv element.""" src = vr.__file__ with open(src, encoding="utf-8") as fh: text = fh.read() # The old, vulnerable call shape must be gone. assert 'req.master_password, "--raw"' not in text assert "[\"unlock\", req.master_password" not in text - # And the secure shape must be present. - assert "--passwordenv" in text - assert re.search(r"bw_password\s*=\s*req\.master_password", text) + # And the safer stdin shape must be present. + assert "[\"unlock\", \"--raw\"]" in text + assert re.search(r'input_text\s*=\s*req\.master_password\s*\+\s*"\\n"', text) + + +def test_tool_vault_unlock_feeds_password_on_stdin_not_argv(): + text = open("src/tool_implementations.py", encoding="utf-8").read() + + assert '["unlock", master_password, "--raw"]' not in text + assert '_run_bw(["unlock", master_password' not in text + assert re.search(r'input_text\s*=\s*master_password\s*\+\s*"\\n"', text) def test_load_config_ignores_non_object_json(tmp_path, monkeypatch):