Keep Bitwarden unlock password off argv (#1311)

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/<pid>/cmdline` to any local user. Hand the master password
-    # to `bw` through the environment instead (paired with `--passwordenv
-    # BW_PASSWORD` in args); /proc/<pid>/environ is readable only by the process
-    # owner. This mirrors how BW_SESSION is already passed above.
+    # via `ps` / `/proc/<pid>/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/<pid>/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/<pid>/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]}"}

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}
 

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/<pid>/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):