fix(security): stop leaking the vault master password via process argv (#879)

The /api/vault/unlock handler ran `bw` as
`_run_bw(["unlock", req.master_password, "--raw"])`. _run_bw launches it with
`asyncio.create_subprocess_exec(bw_path, *args)`, so the master password became
a process argument — readable by any local user through `ps` and
`/proc/<pid>/cmdline` for the lifetime of the unlock subprocess. The Bitwarden
master password decrypts the entire vault, so this is a serious credential
exposure on any multi-user / shared host (CWE-214).

The sibling /login handler already avoids this by feeding the password on
stdin; unlock was the outlier. Hand the password to `bw` through the
environment instead (`--passwordenv BW_PASSWORD`), mirroring how BW_SESSION is
already passed — `/proc/<pid>/environ` is readable only by the process owner,
not other local users. Add regression tests pinning that the secret reaches
the subprocess env and never appears in argv.

routes/vault_routes.py

@@ -75,11 +75,19 @@ def _save_config(cfg: dict):
     safe_chmod(str(VAULT_FILE), 0o600)
 
 
-async def _run_bw(args: list, session: str = None, input_text: str = None) -> tuple:
+async def _run_bw(args: list, session: str = None, input_text: str = None,
+                  bw_password: str = None) -> tuple:
     env = {}
     env.update(os.environ)
     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.
+    if bw_password is not None:
+        env["BW_PASSWORD"] = bw_password
     bw_path = _find_bw()
     try:
         proc = await asyncio.create_subprocess_exec(
@@ -175,8 +183,13 @@ 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.)
         stdout, stderr, rc = await _run_bw(
-            ["unlock", req.master_password, "--raw"],
+            ["unlock", "--passwordenv", "BW_PASSWORD", "--raw"],
+            bw_password=req.master_password,
         )
         if rc != 0:
             return {"ok": False, "error": f"Unlock failed: {stderr[:300]}"}

tests/test_vault_password_not_in_argv.py

@@ -0,0 +1,100 @@
+"""Pin the vault master-password handling so it never regresses into argv.
+
+`routes.vault_routes._run_bw` launches the Bitwarden CLI with
+``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.
+
+The /unlock route previously did ``_run_bw(["unlock", req.master_password,
+"--raw"])`` — leaking the Bitwarden master password (which decrypts the whole
+vault) to any local user for the lifetime of the unlock subprocess.
+"""
+
+import os
+import re
+import sys
+import types
+from unittest.mock import MagicMock
+
+import pytest
+
+sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
+
+# Importing routes.vault_routes pulls in core.middleware → core/__init__ →
+# session_manager, which explodes under the conftest stubs. Stub the heavy
+# imports the module needs so we can reach the self-contained _run_bw helper.
+if "core.database" not in sys.modules:
+    _db = types.ModuleType("core.database")
+    for _n in ("SessionLocal", "ChatMessage", "Session", "Document"):
+        setattr(_db, _n, MagicMock())
+    sys.modules["core.database"] = _db
+if "core.middleware" not in sys.modules:
+    _mw = types.ModuleType("core.middleware")
+    _mw.require_admin = MagicMock()
+    sys.modules["core.middleware"] = _mw
+if "core.platform_compat" not in sys.modules:
+    _pc = types.ModuleType("core.platform_compat")
+    _pc.IS_WINDOWS = False
+    _pc.safe_chmod = MagicMock()
+    _pc.which_tool = MagicMock(return_value="bw")
+    sys.modules["core.platform_compat"] = _pc
+
+import routes.vault_routes as vr  # noqa: E402
+
+
+class _FakeProc:
+    def __init__(self, stdout=b"session-key", stderr=b"", rc=0):
+        self._out, self._err, self.returncode = stdout, stderr, rc
+
+    async def communicate(self, input=None):
+        return self._out, self._err
+
+
+def _patch_exec(monkeypatch):
+    """Capture the argv + env handed to create_subprocess_exec."""
+    captured = {}
+
+    async def _fake_exec(*argv, env=None, **kwargs):
+        captured["argv"] = list(argv)
+        captured["env"] = env or {}
+        return _FakeProc()
+
+    monkeypatch.setattr(vr, "_find_bw", lambda: "bw")
+    monkeypatch.setattr(vr.asyncio, "create_subprocess_exec", _fake_exec)
+    return captured
+
+
+@pytest.mark.asyncio
+async def test_run_bw_password_goes_to_env_not_argv(monkeypatch):
+    captured = _patch_exec(monkeypatch)
+    secret = "correct horse battery staple"
+    await vr._run_bw(["unlock", "--passwordenv", "BW_PASSWORD", "--raw"],
+                     bw_password=secret)
+    # The secret must reach bw through the environment...
+    assert captured["env"].get("BW_PASSWORD") == secret
+    # ...and must NOT appear anywhere in the argv (which `ps` exposes).
+    assert secret not in captured["argv"]
+    assert all(secret not in str(a) for a in captured["argv"])
+
+
+@pytest.mark.asyncio
+async def test_run_bw_without_password_does_not_set_env(monkeypatch):
+    captured = _patch_exec(monkeypatch)
+    await vr._run_bw(["lock"])
+    assert "BW_PASSWORD" not in captured["env"]
+
+
+def test_unlock_handler_uses_passwordenv_not_argv():
+    """Source-level guard: the /unlock route must feed the master password via
+    --passwordenv / bw_password=, 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)