From f691537472d75c494fab9a6737c5290b588646c5 Mon Sep 17 00:00:00 2001 From: Mahdi Salmanzade Date: Tue, 2 Jun 2026 07:25:43 +0400 Subject: [PATCH] fix(security): stop leaking the vault master password via process argv (#879) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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//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//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 | 17 +++- tests/test_vault_password_not_in_argv.py | 100 +++++++++++++++++++++++ 2 files changed, 115 insertions(+), 2 deletions(-) create mode 100644 tests/test_vault_password_not_in_argv.py diff --git a/routes/vault_routes.py b/routes/vault_routes.py index e41c92f..17a635d 100644 --- a/routes/vault_routes.py +++ b/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//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. + 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//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]}"} diff --git a/tests/test_vault_password_not_in_argv.py b/tests/test_vault_password_not_in_argv.py new file mode 100644 index 0000000..df3d50d --- /dev/null +++ b/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//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)