From f6234d00c5d59450adea1d7edd30ad3859375c79 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Wed, 2 Sep 2026 10:14:42 -0700 Subject: [PATCH] =?UTF-8?q?fix(security):=20close=20GitSpawn=20RCE=20class?= =?UTF-8?q?=20=E2=80=94=20malicious=20repo=20.git/config=20no=20longer=20e?= =?UTF-8?q?xecutes=20on=20context=20gathering=20(GHSA-7x36-8jrh-v4pw)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Hermes gathers workspace context by running git against the session directory automatically — the coding-workspace snapshot, gateway project-tree build, /diff, @diff|@staged context refs, goal-gate fingerprint, and -w startup worktree add — before any prompt, tool call, approval, or trust gate. Those probes ran the system git without stripping the repository's own config, so a repo delivered as files with its .git directory intact (a shared zip, sync folder, or USB stick; git clone never transfers .git/config) could set an execution-sink git setting and get arbitrary host code execution as the user with nothing on screen. - core.fsmonitor / core.hooksPath / pager / editor / credential helper: neutralized by routing every automatic probe through noninteractive_git_env(), which pins those keys to inert values via GIT_CONFIG_* and ignores global/system config. bounded_git_probe (the reported sink, coding_context._git + tui_gateway.git_probe) now defaults to that env; worktree-add, working_diff, web_git, context_references, goals, and subagent_worktree route through it too. - Attribute-scoped [diff "x"] command=/textconv= drivers: the attacker names the driver in .gitattributes, so GIT_CONFIG_KEY overrides can't enumerate them. Added harden_git_argv(), which inserts --no-ext-diff --no-textconv on diff-rendering subcommands (diff/show/ log/blame) only — status et al reject the flags. Both flags required (verified empirically; each alone leaves the other live). Builds on the noninteractive_git_env config-scrubbing from the gemini-cli #28792 port. Real-git E2E regression suite arms a malicious repo and asserts every automatic path neutralizes fsmonitor, hooks, external-diff, and textconv; a baseline test proves the repo is armed. --- agent/context_references.py | 10 +- cli.py | 6 + hermes_cli/_subprocess_compat.py | 73 ++++++- hermes_cli/goals.py | 4 + hermes_cli/web_git.py | 4 +- .../test_gitspawn_config_injection.py | 200 ++++++++++++++++++ tools/subagent_worktree.py | 15 +- tools/working_diff.py | 14 +- 8 files changed, 317 insertions(+), 9 deletions(-) create mode 100644 tests/security/test_gitspawn_config_injection.py diff --git a/agent/context_references.py b/agent/context_references.py index dd36e67672..cadb044163 100644 --- a/agent/context_references.py +++ b/agent/context_references.py @@ -12,7 +12,12 @@ from pathlib import Path from typing import Awaitable, Callable from agent.model_metadata import estimate_tokens_rough -from hermes_cli._subprocess_compat import IS_WINDOWS, windows_hide_flags +from hermes_cli._subprocess_compat import ( + IS_WINDOWS, + harden_git_argv, + noninteractive_git_env, + windows_hide_flags, +) from hermes_cli.sizefmt import format_bytes from abc import ABC, abstractmethod @@ -425,12 +430,13 @@ def _expand_git_reference( _popen_kwargs = {"creationflags": windows_hide_flags()} if IS_WINDOWS else {} try: result = subprocess.run( - ["git", *args], + ["git", *harden_git_argv(args)], cwd=cwd, capture_output=True, text=True, encoding='utf-8', errors='replace', timeout=30, stdin=subprocess.DEVNULL, + env=noninteractive_git_env(), **_popen_kwargs, ) except subprocess.TimeoutExpired: diff --git a/cli.py b/cli.py index ab2f2d3d7f..20c160931f 100644 --- a/cli.py +++ b/cli.py @@ -1837,6 +1837,10 @@ def _setup_worktree(repo_root: str = None, sync_base: bool = True, """ import subprocess + from hermes_cli._subprocess_compat import ( + noninteractive_git_env as _noninteractive_git_env, + ) + repo_root = repo_root or _git_repo_root() if not repo_root: _cprint("\033[31m✗ --worktree requires being inside a git repository.\033[0m") @@ -1910,6 +1914,7 @@ def _setup_worktree(repo_root: str = None, sync_base: bool = True, result = subprocess.run( ["git", *_wt_add_cfg, "worktree", "add", str(wt_path), "-b", branch_name, base_ref], capture_output=True, text=True, encoding="utf-8", errors="replace", timeout=120, cwd=repo_root, + stdin=subprocess.DEVNULL, env=_noninteractive_git_env(), ) if result.returncode != 0: # If branching from the resolved remote ref failed for any reason @@ -1925,6 +1930,7 @@ def _setup_worktree(repo_root: str = None, sync_base: bool = True, result = subprocess.run( ["git", "worktree", "add", str(wt_path), "-b", branch_name, base_ref], capture_output=True, text=True, encoding="utf-8", errors="replace", timeout=120, cwd=repo_root, + stdin=subprocess.DEVNULL, env=_noninteractive_git_env(), ) if result.returncode != 0: _cleanup_failed_worktree_add(repo_root, wt_path, branch_name) diff --git a/hermes_cli/_subprocess_compat.py b/hermes_cli/_subprocess_compat.py index ca8effcb26..12350d294c 100644 --- a/hermes_cli/_subprocess_compat.py +++ b/hermes_cli/_subprocess_compat.py @@ -47,9 +47,64 @@ __all__ = [ "bounded_git_probe", "bounded_probe_run", "noninteractive_git_env", + "NO_DRIVER_DIFF_FLAGS", "pid_is_hermes", ] +# Flags that neutralize *attribute-scoped* diff drivers on any diff-rendering +# git command (``diff``, ``log -p``, ``show``, ``blame``). A malicious repo can +# name a driver in ``.gitattributes`` (``* diff=evil``) and point it at an +# arbitrary program via ``[diff "evil"] command=/textconv=`` in ``.git/config``. +# Because the attacker chooses the driver name, ``GIT_CONFIG_KEY`` overrides in +# ``noninteractive_git_env`` cannot enumerate and disable it — only these +# command-line flags do. ``--no-ext-diff`` kills ``command=``; ``--no-textconv`` +# kills ``textconv=``. Both are required (verified empirically: each alone +# leaves the other live). Smudge/clean filters are neutralized by the env +# layer's ``core.hooksPath`` + running against the index without checkout. +NO_DRIVER_DIFF_FLAGS = ("--no-ext-diff", "--no-textconv") + +# Subcommands that render diffs and therefore invoke ``.gitattributes``-scoped +# diff/textconv drivers. Only these accept ``NO_DRIVER_DIFF_FLAGS`` — ``status`` +# and friends reject the flags (``unknown option``), so the helper must gate on +# this set rather than blanket-prepending. +_DIFF_RENDERING_SUBCOMMANDS = frozenset({"diff", "show", "log", "blame"}) + + +def harden_git_argv(args: Sequence[str]) -> list[str]: + """Return a copy of subcommand-first git *args* with diff-driver flags + inserted for diff-rendering subcommands. + + *args* is the argument list WITHOUT the leading ``"git"`` (e.g. + ``["diff", "HEAD"]`` or ``["-c", "core.quotePath=false", "diff", ...]``). + The first non-option token is treated as the subcommand; if it is one of + :data:`_DIFF_RENDERING_SUBCOMMANDS`, :data:`NO_DRIVER_DIFF_FLAGS` is + inserted immediately after it. Non-diff subcommands are returned unchanged. + + Pair with :func:`noninteractive_git_env`: the env layer disables + fsmonitor/hooks/pager/editor/credential sinks, this closes the one class + (attacker-named attribute drivers) env overrides cannot reach. + """ + out = list(args) + # Options that consume the FOLLOWING token as their value, so that value is + # never mistaken for the subcommand (``-C diff`` is a path; ``-c diff=x`` is + # a config pair — neither is the diff subcommand). + _value_opts = {"-C", "-c", "--git-dir", "--work-tree", "--namespace", "--exec-path"} + i = 0 + while i < len(out): + tok = out[i] + if tok in _value_opts: + i += 2 + continue + if tok.startswith("-"): + i += 1 + continue + if tok in _DIFF_RENDERING_SUBCOMMANDS: + return out[: i + 1] + list(NO_DRIVER_DIFF_FLAGS) + out[i + 1 :] + # First non-option token is the subcommand; if it isn't a diff renderer + # there is nothing to harden. + return out + return out + IS_WINDOWS = sys.platform == "win32" @@ -610,6 +665,7 @@ def bounded_probe_run( *, timeout: float, errors: str = "replace", + env: "Mapping[str, str] | None" = None, ) -> "subprocess.CompletedProcess[str] | None": """Deadlock-safe ``subprocess.run(argv, capture_output=True, timeout=...)`` for fail-open probe call sites. Returns a ``CompletedProcess`` when the @@ -647,6 +703,7 @@ def bounded_probe_run( text=True, encoding="utf-8", errors=errors, + env=dict(env) if env is not None else None, **_popen_kwargs, ) except Exception: @@ -674,6 +731,20 @@ def bounded_git_probe(argv: Sequence[str], *, timeout: float) -> str: ``subprocess.run(["git", ...], timeout=...)`` at fail-open probe call sites (``tui_gateway.git_probe.run_git``, ``agent.coding_context._git``). + **Security (GHSA-7x36-8jrh-v4pw):** these probes run automatically against + whatever directory the session sits in — the coding-workspace snapshot and + the gateway project-tree build fire ``git status`` / ``git branch`` before + any tool call, approval, or trust prompt. An index refresh executes the + repository-configured ``core.fsmonitor`` program, and other config keys + (hooks, pager, editor, credential helper) are execution sinks too. A repo + delivered as files with its ``.git`` directory intact (a shared zip, sync + folder, or USB stick — ``git clone`` never transfers ``.git/config``) would + otherwise get host code execution as the user. Every probe now runs under + :func:`noninteractive_git_env`, which pins those keys to inert values via + ``GIT_CONFIG_*`` and ignores global/system config. Diff-rendering callers + additionally pass :data:`NO_DRIVER_DIFF_FLAGS` (attribute-scoped drivers + can't be disabled through env overrides). + Why not ``subprocess.run``: on Windows, ``run()``'s post-timeout cleanup calls an *unbounded* ``communicate()`` after killing git. Killing the PATH-resolved launcher can leave a suspended descendant ``git.exe`` holding @@ -698,7 +769,7 @@ def bounded_git_probe(argv: Sequence[str], *, timeout: float) -> str: openai/codex#36793). ``process_group`` only changes which group the child belongs to; it does not detach the terminal or alter the fast path. """ - result = bounded_probe_run(argv, timeout=timeout) + result = bounded_probe_run(argv, timeout=timeout, env=noninteractive_git_env()) if result is None or result.returncode != 0: return "" return (result.stdout or "").strip() diff --git a/hermes_cli/goals.py b/hermes_cli/goals.py index 74acc22c80..d159adbb65 100644 --- a/hermes_cli/goals.py +++ b/hermes_cli/goals.py @@ -42,6 +42,8 @@ from dataclasses import dataclass, field, asdict from datetime import datetime, timezone from typing import Any, Dict, List, Optional, Tuple +from hermes_cli._subprocess_compat import noninteractive_git_env + logger = logging.getLogger(__name__) @@ -494,6 +496,7 @@ def workspace_fingerprint(cwd: Optional[str] = None) -> str: ["git", "rev-parse", "HEAD"], capture_output=True, text=True, encoding="utf-8", errors="replace", timeout=10, cwd=workdir, + stdin=subprocess.DEVNULL, env=noninteractive_git_env(), ) if head.returncode != 0: return "" @@ -501,6 +504,7 @@ def workspace_fingerprint(cwd: Optional[str] = None) -> str: ["git", "status", "--porcelain"], capture_output=True, text=True, encoding="utf-8", errors="replace", timeout=30, cwd=workdir, + stdin=subprocess.DEVNULL, env=noninteractive_git_env(), ) if status.returncode != 0: return "" diff --git a/hermes_cli/web_git.py b/hermes_cli/web_git.py index 3ea3c77c02..f06f736d49 100644 --- a/hermes_cli/web_git.py +++ b/hermes_cli/web_git.py @@ -19,7 +19,7 @@ import shutil import subprocess from pathlib import Path -from hermes_cli._subprocess_compat import noninteractive_git_env +from hermes_cli._subprocess_compat import harden_git_argv, noninteractive_git_env _GIT_TIMEOUT = 30 _GH_TIMEOUT = 30 @@ -42,7 +42,7 @@ def _git(cwd: str, args: list[str], *, timeout: int = _GIT_TIMEOUT) -> tuple[int the real auth error in the toast instead.""" try: proc = subprocess.run( - ["git", *args], + ["git", *harden_git_argv(args)], cwd=cwd, capture_output=True, text=True, encoding='utf-8', errors='replace', diff --git a/tests/security/test_gitspawn_config_injection.py b/tests/security/test_gitspawn_config_injection.py new file mode 100644 index 0000000000..bd212f9e3c --- /dev/null +++ b/tests/security/test_gitspawn_config_injection.py @@ -0,0 +1,200 @@ +"""GitSpawn / GHSA-7x36-8jrh-v4pw regression suite. + +A repository delivered as files (zip, sync folder, USB) can carry a +``.git/config`` that names a command in an execution-sink git setting — +``core.fsmonitor``, ``core.hooksPath`` hooks, or an attribute-scoped +``[diff "x"] command=/textconv=`` driver. Hermes gathers workspace context by +running git against the session directory automatically, before any prompt, +approval, or trust gate, so an unhardened probe would execute that command on +the host as the user. + +These tests build a real malicious repo and assert that every automatic +context-gathering git path Hermes runs neutralizes every sink. They use a real +``git`` and skip if it is unavailable. +""" + +from __future__ import annotations + +import os +import shutil +import subprocess +from pathlib import Path + +import pytest + +from hermes_cli._subprocess_compat import ( + NO_DRIVER_DIFF_FLAGS, + harden_git_argv, + noninteractive_git_env, +) + +_HAS_GIT = shutil.which("git") is not None +pytestmark = pytest.mark.skipif(not _HAS_GIT, reason="git not installed") + + +# --------------------------------------------------------------------------- +# 1. harden_git_argv unit contract +# --------------------------------------------------------------------------- + + +class TestHardenGitArgv: + def test_diff_gets_flags_after_subcommand(self): + assert harden_git_argv(["diff", "HEAD"]) == [ + "diff", *NO_DRIVER_DIFF_FLAGS, "HEAD", + ] + + def test_show_log_blame_are_hardened(self): + for sub in ("show", "log", "blame"): + out = harden_git_argv([sub, "x"]) + assert out[0] == sub + assert out[1:3] == list(NO_DRIVER_DIFF_FLAGS) + + def test_status_is_not_touched(self): + # status rejects --no-ext-diff (`unknown option`), so it must pass through. + assert harden_git_argv(["status", "--porcelain=2", "--branch"]) == [ + "status", "--porcelain=2", "--branch", + ] + + def test_worktree_and_other_subcommands_untouched(self): + assert harden_git_argv(["worktree", "add", "x"]) == ["worktree", "add", "x"] + assert harden_git_argv(["rev-parse", "HEAD"]) == ["rev-parse", "HEAD"] + + def test_global_options_are_skipped_when_finding_subcommand(self): + out = harden_git_argv(["-C", "/repo", "diff", "HEAD"]) + assert out == ["-C", "/repo", "diff", *NO_DRIVER_DIFF_FLAGS, "HEAD"] + + def test_dash_c_value_is_not_mistaken_for_subcommand(self): + # ``-C diff`` is a path; the real subcommand is status → no flags. + assert harden_git_argv(["-C", "diff", "status"]) == ["-C", "diff", "status"] + # ``-c diff=x`` is a config pair; the real subcommand is status. + assert harden_git_argv(["-c", "diff=x", "status"]) == ["-c", "diff=x", "status"] + + def test_config_pair_before_diff_still_hardens(self): + out = harden_git_argv(["-c", "core.quotePath=false", "diff", "--numstat"]) + assert out == [ + "-c", "core.quotePath=false", "diff", *NO_DRIVER_DIFF_FLAGS, "--numstat", + ] + + +# --------------------------------------------------------------------------- +# 2. Real-git E2E: every automatic path neutralizes every sink +# --------------------------------------------------------------------------- + + +def _make_malicious_repo(tmp: Path) -> tuple[Path, Path]: + """Build a repo whose .git/config arms fsmonitor, a checkout hook, and an + attribute-scoped external-diff + textconv driver. Returns (repo, marker_stem): + a fired sink leaves ``.`` on disk.""" + repo = tmp / "poc" + clean = { + **os.environ, + "GIT_CONFIG_GLOBAL": os.devnull, + "GIT_CONFIG_SYSTEM": os.devnull, + "GIT_CONFIG_NOSYSTEM": "1", + } + subprocess.run(["git", "init", "-q", str(repo)], check=True, env=clean) + (repo / "README").write_text("hi\n") + ident = ["-c", "user.email=a@b", "-c", "user.name=a"] + subprocess.run(["git", "-C", str(repo), *ident, "add", "."], check=True, env=clean) + subprocess.run(["git", "-C", str(repo), *ident, "commit", "-qm", "init"], check=True, env=clean) + + marker = tmp / "MARKER" + hooks = repo / "evil-hooks" + hooks.mkdir() + hook = hooks / "post-checkout" + hook.write_text(f"#!/bin/sh\ntouch {marker}.hook\n") + hook.chmod(0o755) + with (repo / ".git" / "config").open("a") as f: + f.write(f'[core]\n\tfsmonitor = "touch {marker}.fsmonitor"\n\thooksPath = {hooks}\n') + f.write(f'[diff "evil"]\n\tcommand = "touch {marker}.extdiff"\n') + f.write(f'\ttextconv = "sh -c \'touch {marker}.textconv; cat\'"\n') + (repo / ".gitattributes").write_text("* diff=evil\n") + (repo / "README").write_text("changed\n") # dirty working tree so diffs run + return repo, marker + + +def _fired(marker: Path) -> list[str]: + out = [] + for sink in ("fsmonitor", "hook", "extdiff", "textconv"): + p = Path(f"{marker}.{sink}") + if p.exists(): + out.append(sink) + p.unlink() + return out + + +@pytest.fixture() +def malicious_repo(tmp_path): + repo, marker = _make_malicious_repo(tmp_path) + yield repo, marker + + +def test_baseline_unhardened_git_fires_sinks(malicious_repo): + """Sanity: without hardening the payload actually fires — proves the repo + is armed and the test can detect a regression.""" + repo, marker = malicious_repo + subprocess.run(["git", "-C", str(repo), "diff", "HEAD"], capture_output=True) + fired = _fired(marker) + assert "fsmonitor" in fired and "extdiff" in fired, fired + + +def test_coding_workspace_snapshot_is_safe(malicious_repo): + import agent.coding_context as cc + repo, marker = malicious_repo + cc.build_coding_workspace_block(cwd=repo) + assert _fired(marker) == [] + + +def test_gateway_git_probe_is_safe(malicious_repo): + from tui_gateway import git_probe + repo, marker = malicious_repo + git_probe.branch(str(repo)) + git_probe.run_git(str(repo), "status", "--porcelain") + assert _fired(marker) == [] + + +def test_working_diff_is_safe(malicious_repo): + from tools.working_diff import collect_working_diff + repo, marker = malicious_repo + collect_working_diff(str(repo), "working") + assert _fired(marker) == [] + + +def test_goals_fingerprint_is_safe(malicious_repo): + from hermes_cli.goals import workspace_fingerprint + repo, marker = malicious_repo + workspace_fingerprint(str(repo)) + assert _fired(marker) == [] + + +def test_web_git_diff_is_safe(malicious_repo): + from hermes_cli import web_git + repo, marker = malicious_repo + web_git._git(str(repo), ["status", "--porcelain=v2", "-z"]) + web_git._git_out(str(repo), ["diff", "HEAD"]) + assert _fired(marker) == [] + + +def test_context_reference_diff_is_safe(malicious_repo): + from agent import context_references as cr + repo, marker = malicious_repo + ref = type("R", (), {"raw": "@diff"})() + cr._expand_git_reference(ref, repo, ["diff", "HEAD"], "git diff") + assert _fired(marker) == [] + + +def test_subagent_worktree_add_is_safe(malicious_repo, tmp_path): + from tools import subagent_worktree as sw + repo, marker = malicious_repo + sw._run_git(["worktree", "add", str(tmp_path / "wt1"), "-b", "safe1"], str(repo)) + assert _fired(marker) == [] + + +def test_noninteractive_env_pins_fsmonitor_and_hooks(): + env = noninteractive_git_env({}) + values = { + env[f"GIT_CONFIG_KEY_{i}"]: env[f"GIT_CONFIG_VALUE_{i}"] + for i in range(int(env["GIT_CONFIG_COUNT"])) + } + assert values["core.fsmonitor"] == "false" + assert values["core.hooksPath"] == os.devnull diff --git a/tools/subagent_worktree.py b/tools/subagent_worktree.py index b09b1a55be..54611295f0 100644 --- a/tools/subagent_worktree.py +++ b/tools/subagent_worktree.py @@ -44,6 +44,8 @@ import uuid from pathlib import Path from typing import Any, Dict, Optional +from hermes_cli._subprocess_compat import harden_git_argv, noninteractive_git_env + logger = logging.getLogger(__name__) _GIT_TIMEOUT = 30 @@ -52,15 +54,24 @@ _BRANCH_NAMESPACE = "hermes-subagent" def _run_git(args, cwd: str, timeout: int = _GIT_TIMEOUT): - """Run a git command, capturing output. Never raises on non-zero exit.""" + """Run a git command, capturing output. Never raises on non-zero exit. + + Runs under :func:`noninteractive_git_env` (GHSA-7x36-8jrh-v4pw): worktree + isolation runs automatically for delegated subagents against whatever repo + the parent sits in, and ``worktree add`` runs checkout hooks. Disabling the + fsmonitor/hooks/pager/credential config sinks keeps a malicious ``.git/config`` + from executing on the host. + """ return subprocess.run( - ["git", *args], + ["git", *harden_git_argv(args)], cwd=cwd, capture_output=True, text=True, encoding="utf-8", errors="replace", timeout=timeout, + stdin=subprocess.DEVNULL, + env=noninteractive_git_env(), ) diff --git a/tools/working_diff.py b/tools/working_diff.py index a5df51d4f6..876bc47339 100644 --- a/tools/working_diff.py +++ b/tools/working_diff.py @@ -24,6 +24,8 @@ import shutil import subprocess from typing import Dict, List +from hermes_cli._subprocess_compat import harden_git_argv, noninteractive_git_env + _GIT_TIMEOUT = 15 _MAX_UNTRACKED_FILES = 50 # sanity cap so a node_modules explosion can't hang us @@ -31,11 +33,19 @@ VALID_MODES = ("working", "staged", "all") def _run(args: List[str], cwd: str, timeout: int = _GIT_TIMEOUT): - """Run git, returning (returncode, stdout). Never raises on git failure.""" + """Run git, returning (returncode, stdout). Never raises on git failure. + + Hardened against a malicious repo's ``.git/config`` (GHSA-7x36-8jrh-v4pw): + ``noninteractive_git_env`` disables fsmonitor/hooks/pager/editor/credential + sinks, and ``harden_git_argv`` appends ``--no-ext-diff --no-textconv`` to + the diff-rendering subcommands so attribute-scoped diff/textconv drivers + can't execute either. + """ proc = subprocess.run( - ["git", "-c", "core.quotePath=false", *args], + ["git", "-c", "core.quotePath=false", *harden_git_argv(args)], cwd=cwd, capture_output=True, text=True, timeout=timeout, encoding="utf-8", errors="replace", + stdin=subprocess.DEVNULL, env=noninteractive_git_env(), ) return proc.returncode, proc.stdout