From 9c6229ce249e4bbd86a22463be73ac2986db6324 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Sat, 27 Jun 2026 19:44:55 -0700 Subject: [PATCH] fix(security): centralize credential-safe subprocess env (#29157) Subprocesses spawned outside the terminal/execute_code path (agent-browser, copilot ACP, dep-ensure, lazy_deps uv install, TUI Node host, cli.exec) inherited the operator's full credential environment via os.environ.copy(). The terminal path was already scrubbed by _HERMES_PROVIDER_ENV_BLOCKLIST (#1002/#1264/#32314); these spawn sites bypassed it. Adds hermes_subprocess_env(inherit_credentials=) in tools/environments/local.py reusing the existing dynamic blocklist as the single source of truth: - Tier 1 (_ALWAYS_STRIP_KEYS): gateway bot tokens, GitHub auth, infra secrets -- stripped even for credential-inheriting children. - Tier 2 (_HERMES_PROVIDER_ENV_BLOCKLIST): provider/tool keys -- stripped unless inherit_credentials=True. The opt-in is grep-able for audit. Browser worker keeps a _BROWSER_PASSTHROUGH_KEYS allowlist (BROWSERBASE/ FIRECRAWL) re-added after the strip. Model-driving children (ACP, TUI Node host, cli.exec) use inherit_credentials=True so they still get provider keys while losing Tier-1 secrets. Installers (dep-ensure, lazy_deps) inherit nothing sensitive. cua_backend already routed through _sanitize_subprocess_env on main -- left as-is. Gateway adapter utility spawns (gh pr comment, ffmpeg) are left inheriting env: gh needs GH_TOKEN by design, ffmpeg is a trusted system binary -- no untrusted-dependency exposure. This is defense-in-depth (personal-assistant trust model: same-user spawns), making the existing scrub policy uniform across the spawn surface; the main real payoff is shrinking the blast radius if a transitive npm dep in agent-browser is compromised. Reconstructed on current main from the design in #31959 (Tranquil-Flow); also credits #39003 (rodboev), #37843 (coygeek), #35769 (egilewski). Co-authored-by: Tranquil-Flow Co-authored-by: rodboev Co-authored-by: egilewski --- agent/copilot_acp_client.py | 6 +- hermes_cli/dep_ensure.py | 4 +- tests/tools/test_hermes_subprocess_env.py | 151 ++++++++++++++++++++++ tools/browser_tool.py | 27 +++- tools/environments/local.py | 94 ++++++++++++++ tools/lazy_deps.py | 4 +- tui_gateway/server.py | 9 +- 7 files changed, 288 insertions(+), 7 deletions(-) create mode 100644 tests/tools/test_hermes_subprocess_env.py diff --git a/agent/copilot_acp_client.py b/agent/copilot_acp_client.py index e3c03938a..b6e301bd8 100644 --- a/agent/copilot_acp_client.py +++ b/agent/copilot_acp_client.py @@ -23,6 +23,7 @@ from typing import Any from agent.file_safety import get_read_block_error, is_write_denied from agent.redact import redact_sensitive_text +from tools.environments.local import hermes_subprocess_env ACP_MARKER_BASE_URL = "acp://copilot" _DEFAULT_TIMEOUT_SECONDS = 900.0 @@ -94,7 +95,10 @@ def _resolve_home_dir() -> str: def _build_subprocess_env() -> dict[str, str]: - env = os.environ.copy() + # Copilot ACP is a model-driving CLI executor: it legitimately needs LLM + # provider credentials. Route through the central helper so Tier-1 secrets + # (gateway bot tokens, GitHub auth, infra) are still stripped (#29157). + env = hermes_subprocess_env(inherit_credentials=True) home = _resolve_home_dir() env["HOME"] = home from hermes_constants import apply_subprocess_home_env diff --git a/hermes_cli/dep_ensure.py b/hermes_cli/dep_ensure.py index 6f0bc9506..eacb34168 100644 --- a/hermes_cli/dep_ensure.py +++ b/hermes_cli/dep_ensure.py @@ -23,6 +23,7 @@ import sys from pathlib import Path from hermes_constants import agent_browser_runnable +from tools.environments.local import hermes_subprocess_env _IS_WINDOWS = platform.system() == "Windows" @@ -148,7 +149,8 @@ def ensure_dependency( else: cmd = ["bash", str(script), "--ensure", dep] - run_env = {**os.environ, "IS_INTERACTIVE": "false"} + run_env = hermes_subprocess_env(inherit_credentials=False) + run_env["IS_INTERACTIVE"] = "false" result = subprocess.run( cmd, env=run_env, diff --git a/tests/tools/test_hermes_subprocess_env.py b/tests/tools/test_hermes_subprocess_env.py new file mode 100644 index 000000000..92f629e39 --- /dev/null +++ b/tests/tools/test_hermes_subprocess_env.py @@ -0,0 +1,151 @@ +"""Tests for hermes_subprocess_env() — the centralized credential-safe env +builder for the non-terminal subprocess spawn surface. + +Covers GHSA-m4m8-xjp4-5rmm / issue #29157: subprocesses spawned by the +gateway/browser/ACP/installer paths must not blindly inherit the operator's +full credential environment. Two tiers: + + * Tier 1 (_ALWAYS_STRIP_KEYS): gateway bot tokens, GitHub auth, infra + secrets — stripped even when inherit_credentials=True. + * Tier 2 (_HERMES_PROVIDER_ENV_BLOCKLIST): LLM provider/tool keys — stripped + unless the caller opts into inherit_credentials=True. +""" + +import os +from unittest.mock import patch + +from tools.environments.local import ( + hermes_subprocess_env, + _ALWAYS_STRIP_KEYS, + _HERMES_PROVIDER_ENV_FORCE_PREFIX, +) + + +_TIER1_SAMPLE = { + "GH_TOKEN": "ghp_secret", + "TELEGRAM_BOT_TOKEN": "bot-token", + "SLACK_APP_TOKEN": "xapp-secret", + "MODAL_TOKEN_SECRET": "modal-secret", + "HERMES_DASHBOARD_SESSION_TOKEN": "dash-secret", +} + +_PROVIDER_SAMPLE = { + "OPENAI_API_KEY": "sk-fake", + "ANTHROPIC_API_KEY": "ant-fake", + "OPENROUTER_API_KEY": "or-fake", +} + +_SAFE_SAMPLE = { + "PATH": "/usr/bin:/bin", + "HOME": "/home/user", + "USER": "testuser", + "MY_APP_VAR": "keep-me", +} + + +def _build(extra=None, *, inherit_credentials=False): + env = dict(_SAFE_SAMPLE) + if extra: + env.update(extra) + with patch.dict(os.environ, env, clear=True): + return hermes_subprocess_env(inherit_credentials=inherit_credentials) + + +class TestStripByDefault: + def test_provider_keys_stripped_by_default(self): + result = _build(_PROVIDER_SAMPLE) + for var in _PROVIDER_SAMPLE: + assert var not in result, f"{var} leaked with inherit_credentials=False" + + def test_tier1_secrets_stripped_by_default(self): + result = _build(_TIER1_SAMPLE) + for var in _TIER1_SAMPLE: + assert var not in result, f"{var} leaked (Tier-1) with inherit_credentials=False" + + def test_safe_vars_preserved(self): + result = _build() + assert result["HOME"] == "/home/user" + assert result["USER"] == "testuser" + assert "PATH" in result + assert result["MY_APP_VAR"] == "keep-me" + + def test_force_prefix_hints_stripped(self): + result = _build({f"{_HERMES_PROVIDER_ENV_FORCE_PREFIX}OPENAI_API_KEY": "sk-x"}) + assert f"{_HERMES_PROVIDER_ENV_FORCE_PREFIX}OPENAI_API_KEY" not in result + assert "OPENAI_API_KEY" not in result + + def test_pythonutf8_set(self): + result = _build() + assert result.get("PYTHONUTF8") == "1" + + +class TestInheritCredentials: + def test_provider_keys_preserved_when_inheriting(self): + result = _build(_PROVIDER_SAMPLE, inherit_credentials=True) + for var, val in _PROVIDER_SAMPLE.items(): + assert result.get(var) == val, f"{var} should survive inherit_credentials=True" + + def test_tier1_secrets_stripped_even_when_inheriting(self): + """The whole point of Tier 1: gateway/GitHub/infra secrets never reach + a child, even a model-driving CLI that legitimately needs provider keys.""" + result = _build({**_PROVIDER_SAMPLE, **_TIER1_SAMPLE}, inherit_credentials=True) + for var in _TIER1_SAMPLE: + assert var not in result, ( + f"{var} (Tier-1) must be stripped even with inherit_credentials=True" + ) + # ...while provider keys survive. + for var in _PROVIDER_SAMPLE: + assert var in result + + def test_pythonutf8_set_when_inheriting(self): + assert _build(inherit_credentials=True).get("PYTHONUTF8") == "1" + + +class TestTierInvariants: + def test_tier1_always_stripped_both_paths(self): + """Behavioral invariant: every Tier-1 key is stripped on BOTH the + default path and the inherit_credentials=True path. This is what + guarantees no gap, regardless of whether the key also happens to be + in the provider blocklist.""" + sample = {k: f"secret-{k}" for k in _ALWAYS_STRIP_KEYS} + for inherit in (False, True): + result = _build(sample, inherit_credentials=inherit) + leaked = {k for k in _ALWAYS_STRIP_KEYS if k in result} + assert not leaked, ( + f"Tier-1 keys leaked with inherit_credentials={inherit}: {sorted(leaked)}" + ) + + def test_tier1_covers_gateway_bot_token(self): + assert "TELEGRAM_BOT_TOKEN" in _ALWAYS_STRIP_KEYS + + def test_tier1_covers_github_auth(self): + assert {"GH_TOKEN", "GITHUB_TOKEN"} <= _ALWAYS_STRIP_KEYS + + def test_tier1_covers_infra_secrets(self): + assert {"MODAL_TOKEN_ID", "MODAL_TOKEN_SECRET", "DAYTONA_API_KEY"} <= _ALWAYS_STRIP_KEYS + + +class TestBrowserPassthroughPattern: + def test_browser_keys_recoverable_after_strip(self): + """Browser tool pattern: strip everything, then re-add the browser + backend keys agent-browser actually needs.""" + from tools.browser_tool import _BROWSER_PASSTHROUGH_KEYS + + leaked = { + "BROWSERBASE_API_KEY": "bb-key", + "BROWSERBASE_PROJECT_ID": "bb-proj", + "FIRECRAWL_API_KEY": "fc-key", + "ANTHROPIC_API_KEY": "ant-should-go", + "TELEGRAM_BOT_TOKEN": "bot-should-go", + } + with patch.dict(os.environ, {**_SAFE_SAMPLE, **leaked}, clear=True): + env = hermes_subprocess_env(inherit_credentials=False) + for key in _BROWSER_PASSTHROUGH_KEYS: + if key in os.environ: + env[key] = os.environ[key] + + assert env["BROWSERBASE_API_KEY"] == "bb-key" + assert env["FIRECRAWL_API_KEY"] == "fc-key" + # Provider + gateway secrets must NOT come back. + assert "ANTHROPIC_API_KEY" not in env + assert "TELEGRAM_BOT_TOKEN" not in env diff --git a/tools/browser_tool.py b/tools/browser_tool.py index d679997d3..c27b46b24 100644 --- a/tools/browser_tool.py +++ b/tools/browser_tool.py @@ -69,6 +69,22 @@ from hermes_constants import agent_browser_runnable, get_hermes_home from utils import env_int, is_truthy_value from hermes_cli.config import DEFAULT_CONFIG, cfg_get from hermes_cli._subprocess_compat import windows_hide_flags +from tools.environments.local import hermes_subprocess_env + +# Browser-specific tool keys passed through to the agent-browser subprocess +# AFTER credential stripping. agent-browser is a Node process loading npm +# deps; handing it the full operator keyring (#29157 / GHSA-m4m8-xjp4-5rmm) +# means a compromised transitive dependency could read every Hermes secret +# straight out of process.env. Strip by default, then re-add only the +# browser-backend keys the worker legitimately needs. +_BROWSER_PASSTHROUGH_KEYS: tuple[str, ...] = ( + "BROWSERBASE_API_KEY", + "BROWSERBASE_PROJECT_ID", + "BROWSER_USE_API_KEY", + "FIRECRAWL_API_KEY", + "FIRECRAWL_API_URL", + "FIRECRAWL_BROWSER_TTL", +) try: from tools.website_policy import check_website_access @@ -872,7 +888,11 @@ def _run_chrome_fallback_command( task_socket_dir = os.path.join(_socket_safe_tmpdir(), f"agent-browser-{tmp_session}") os.makedirs(task_socket_dir, mode=0o700, exist_ok=True) - browser_env = {**os.environ, "AGENT_BROWSER_SOCKET_DIR": task_socket_dir} + browser_env = hermes_subprocess_env(inherit_credentials=False) + for _key in _BROWSER_PASSTHROUGH_KEYS: + if _key in os.environ: + browser_env[_key] = os.environ[_key] + browser_env["AGENT_BROWSER_SOCKET_DIR"] = task_socket_dir browser_env["PATH"] = _merge_browser_path(browser_env.get("PATH", "")) if "AGENT_BROWSER_IDLE_TIMEOUT_MS" not in browser_env: @@ -2125,7 +2145,10 @@ def _run_browser_command( logger.debug("browser cmd=%s task=%s socket_dir=%s (%d chars)", command, task_id, task_socket_dir, len(task_socket_dir)) - browser_env = {**os.environ} + browser_env = hermes_subprocess_env(inherit_credentials=False) + for _key in _BROWSER_PASSTHROUGH_KEYS: + if _key in os.environ: + browser_env[_key] = os.environ[_key] # Ensure subprocesses inherit the same browser-specific PATH fallbacks # used during CLI discovery. diff --git a/tools/environments/local.py b/tools/environments/local.py index 71ba4e97f..3e968d37e 100644 --- a/tools/environments/local.py +++ b/tools/environments/local.py @@ -250,6 +250,100 @@ def _sanitize_subprocess_env(base_env: dict | None, extra_env: dict | None = Non return sanitized +# Tier-1 secrets: stripped from EVERY spawned subprocess unconditionally — +# even when the caller opts into credential inheritance for a model-driving +# CLI (claude / codex / gemini). These are not LLM provider credentials; no +# legitimate child Hermes spawns needs them, and they are the highest-value +# secrets to keep out of a compromised dependency's reach (gateway bot tokens, +# GitHub auth, remote-compute tokens, dashboard session secret). The set is a +# narrow subset of _HERMES_PROVIDER_ENV_BLOCKLIST; provider keys are handled by +# the conditional Tier-2 strip in hermes_subprocess_env(). +_ALWAYS_STRIP_KEYS: frozenset[str] = frozenset({ + # GitHub auth + "GH_TOKEN", + "GITHUB_TOKEN", + "GITHUB_APP_ID", + "GITHUB_APP_PRIVATE_KEY_PATH", + "GITHUB_APP_INSTALLATION_ID", + # Gateway / messaging bot tokens and access control + "TELEGRAM_BOT_TOKEN", + "DISCORD_BOT_TOKEN", + "SLACK_BOT_TOKEN", + "SLACK_APP_TOKEN", + "SLACK_SIGNING_SECRET", + "GATEWAY_ALLOWED_USERS", + "GATEWAY_ALLOW_ALL_USERS", + "HASS_TOKEN", + "EMAIL_PASSWORD", + "HERMES_DASHBOARD_SESSION_TOKEN", + # Remote-compute / infrastructure secrets + "MODAL_TOKEN_ID", + "MODAL_TOKEN_SECRET", + "DAYTONA_API_KEY", +}) + + +def hermes_subprocess_env(*, inherit_credentials: bool = False) -> dict[str, str]: + """Build a sanitized environment dict for a spawned subprocess. + + Centralized helper for the **non-terminal** spawn surface (browser, + ACP/CLI executors, computer-use driver, dep-ensure, TUI Node host, + detached gateway). Use this instead of copying ``os.environ`` directly + so strip-by-default is the uniform policy across every spawn site, with a + single source of truth (``_HERMES_PROVIDER_ENV_BLOCKLIST``). The terminal + / execute_code path keeps using :func:`_sanitize_subprocess_env`, which is + skill-aware (``env_passthrough``); this helper is for spawns that have no + skill-passthrough concept. + + Two-tier stripping: + + * **Tier 1 (always):** ``_ALWAYS_STRIP_KEYS`` — gateway bot tokens, GitHub + auth, and remote-compute secrets are removed regardless of + ``inherit_credentials``. No child Hermes spawns legitimately needs them. + * **Tier 2 (conditional):** the rest of ``_HERMES_PROVIDER_ENV_BLOCKLIST`` + (LLM provider API keys, tool secrets) is removed unless the caller passes + ``inherit_credentials=True``. + + Pass ``inherit_credentials=True`` **only** when the child legitimately + needs LLM provider credentials — a user-blessed ``claude`` / ``codex`` / + ``gemini`` CLI executor, or the TUI Node host that makes model calls. The + flag is grep-able for audit: ``grep -rn 'inherit_credentials=True'`` lists + every spawn site that still receives provider credentials. + + Callers that need a *specific* non-provider secret (e.g. the browser worker + needs ``BROWSERBASE_API_KEY`` / ``FIRECRAWL_API_KEY``) should call with + ``inherit_credentials=False`` and copy just those keys back from + ``os.environ`` into the returned dict. + """ + env = os.environ.copy() + + # Tier 1 — always strip. + for key in _ALWAYS_STRIP_KEYS: + env.pop(key, None) + # Internal routing hints must never reach a child. + for key in list(env): + if key.startswith(_HERMES_PROVIDER_ENV_FORCE_PREFIX): + env.pop(key, None) + + if not inherit_credentials: + # Tier 2 — strip provider/tool credentials unless explicitly inherited. + for key in _HERMES_PROVIDER_ENV_BLOCKLIST: + env.pop(key, None) + + # Windows UTF-8 safety for spawned processes (#31420). + env.setdefault("PYTHONUTF8", "1") + + _inject_context_hermes_home(env) + from hermes_constants import apply_subprocess_home_env + apply_subprocess_home_env(env) + + # Active-venv markers must not clobber another project's environment. + for _marker in _ACTIVE_VENV_MARKER_VARS: + env.pop(_marker, None) + + return env + + def _find_bash() -> str: """Find bash for command execution.""" if not _IS_WINDOWS: diff --git a/tools/lazy_deps.py b/tools/lazy_deps.py index cec730dcb..2e74f8391 100644 --- a/tools/lazy_deps.py +++ b/tools/lazy_deps.py @@ -613,7 +613,9 @@ def _venv_pip_install(specs: tuple[str, ...], *, timeout: int = 300) -> _Install try: venv_root = Path(sys.executable).parent.parent - uv_env = {**os.environ, "VIRTUAL_ENV": str(venv_root)} + from tools.environments.local import hermes_subprocess_env + uv_env = hermes_subprocess_env(inherit_credentials=False) + uv_env["VIRTUAL_ENV"] = str(venv_root) # Tier 1: uv (preferred — fast, doesn't need pip in the venv) uv_bin = shutil.which("uv") diff --git a/tui_gateway/server.py b/tui_gateway/server.py index 1bb293972..45abc47b7 100644 --- a/tui_gateway/server.py +++ b/tui_gateway/server.py @@ -25,6 +25,7 @@ from hermes_constants import ( ) from hermes_cli.env_loader import load_hermes_dotenv from utils import is_truthy_value +from tools.environments.local import hermes_subprocess_env from tui_gateway import git_probe from tui_gateway.transport import ( StdioTransport, @@ -284,7 +285,9 @@ class _SlashWorker: text=True, bufsize=1, cwd=os.getcwd(), - env=os.environ.copy(), + # slash_worker runs the Hermes agent → needs provider credentials. + # Tier-1 secrets (gateway/GitHub/infra) are still stripped (#29157). + env=hermes_subprocess_env(inherit_credentials=True), ) threading.Thread(target=self._drain_stdout, daemon=True).start() threading.Thread(target=self._drain_stderr, daemon=True).start() @@ -11131,7 +11134,9 @@ def _(rid, params: dict) -> dict: text=True, timeout=min(int(params.get("timeout", 240)), 600), cwd=os.getcwd(), - env=os.environ.copy(), + # cli.exec runs `python -m hermes_cli.main` (can drive the agent) → + # needs provider credentials. Tier-1 secrets still stripped (#29157). + env=hermes_subprocess_env(inherit_credentials=True), stdin=subprocess.DEVNULL, ) parts = [r.stdout or "", r.stderr or ""]