From 50617d1c750bf86e167f63e80c0483688fe4435b Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Mon, 7 Sep 2026 13:38:18 -0700 Subject: [PATCH] fix(approvals): preserve env argv and shell comment boundaries --- evals/approval_deny_dispatch.py | 12 ++++++++++-- tests/tools/test_approval_deny_rules.py | 18 ++++++++++++++++-- tools/approval_detection.py | 23 ++++++++++++++++++----- website/docs/user-guide/security.md | 2 +- 4 files changed, 45 insertions(+), 10 deletions(-) diff --git a/evals/approval_deny_dispatch.py b/evals/approval_deny_dispatch.py index 1206052e73..470964c623 100644 --- a/evals/approval_deny_dispatch.py +++ b/evals/approval_deny_dispatch.py @@ -18,11 +18,14 @@ with tempfile.TemporaryDirectory(prefix="hermes-deny-dispatch-") as directory: os.environ.update(PATH=directory + ":/usr/bin:/bin", HOME=directory, HERMES_HOME=directory, HERMES_INTERACTIVE="1", TERMINAL_ENV="local") (home / "config.yaml").write_text( - 'approvals:\n mode: "off"\n deny: ["sudo *"]\n' + 'approvals:\n mode: "off"\n deny: ["sudo *", "printf SAFE", "git status"]\n' 'security:\n tirith_enabled: false\n', encoding="utf-8") executable = home / "sudo" executable.write_text('#!/bin/sh\nprintf "OWNED_EXECUTABLE_REACHED\\n"\n', encoding="utf-8") executable.chmod(0o700) + # Prefer GNU env when the host's default env implements different -S syntax. + if Path("/usr/bin/gnuenv").exists(): + (home / "env").symlink_to("/usr/bin/gnuenv") sys.path.insert(0, str(source)) from tools import terminal_tool # noqa: F401 from tools.registry import registry @@ -33,7 +36,12 @@ with tempfile.TemporaryDirectory(prefix="hermes-deny-dispatch-") as directory: "command " + binary + " -n id -u", "true && " + binary + " -n id -u", "bash -c " + shlex.quote(binary + " -n id -u"), - "printf '%s\\n' 'sudo -n id -u'"] + "printf '%s\\n' 'sudo -n id -u'", + "env -S 'printf %s; sudo -n id'", "env -S printf SAFE", + "env -S git status", "echo ok # ; bash -c " + shlex.quote(binary + " -n id"), + "env -S 'printf %s' 'sudo -n id'", "env -S printf BENIGN", + "env -S " + shlex.quote("bash -c " + shlex.quote(binary + " -n id")), + "echo ok # ignored\n bash -c " + shlex.quote(binary + " -n id")] rows = [] for command in cases: result = registry.dispatch("terminal", {"command": command, "workdir": directory, "timeout": 10}) diff --git a/tests/tools/test_approval_deny_rules.py b/tests/tools/test_approval_deny_rules.py index 30656a9a4c..df06f1b771 100644 --- a/tests/tools/test_approval_deny_rules.py +++ b/tests/tools/test_approval_deny_rules.py @@ -82,11 +82,17 @@ def test_deny_follows_executable_identity(deny_config, clean_env, monkeypatch): 'echo "$(/usr/bin/sudo -n id -u)"', "bash -lc 'env -i /usr/bin/sudo -n id -u'", "env -S '/usr/bin/sudo -n id -u'", + "env -S /usr/bin/sudo -n id -u", + "env --split-string=/usr/bin/sudo -n id -u", + "env -S \"bash -c '/usr/bin/sudo -n id -u'\"", + "echo ok # ignored\n bash -c '/usr/bin/sudo -n id -u'", + "printf SAFE", "env -S printf SAFE", "env -Sprintf SAFE", + "env --split-string=printf SAFE", "env -S 'printf' SAFE", "2>/tmp/log FOO=bar /usr/bin/sudo -n id -u", "if true; then /usr/bin/sudo -n id -u; fi", ] for mode, yolo in (("manual", False), ("off", False), ("manual", True)): - deny_config(["sudo *"], mode=mode) + deny_config(["sudo *", "printf SAFE"], mode=mode) monkeypatch.setattr(mod, "_YOLO_MODE_FROZEN", yolo) for command in commands: for guard in (mod.check_dangerous_command, mod.check_all_command_guards): @@ -108,10 +114,18 @@ def test_deny_projection_preserves_data_and_path_rules(deny_config): "env -u sudo printf ok", "exec -a sudo printf ok", "ionice --pid sudo", "chrt --pid sudo", "taskset --pid 1 sudo", "echo 'first\nsudo -n id -u'", "echo ok # ; sudo -n id -u", + "env -S 'printf %s; sudo -n id'", + "env -S 'printf %s' 'sudo -n id'", + "env -S 'printf %s' '$(sudo -n id)'", + "echo ok # ; bash -c '/usr/bin/sudo -n id'", + "echo ok # unmatched '\n printf ok", + "printf '%s' '# ; bash -c sudo'", "git log --grep='git status'", r'printf "%s" "a\"; sudo -n id -u"', ): assert approval_floors._match_user_deny_rule(command) is None, command - for command in ("env git status; echo ok", "(git status)", "git\tstatus # comment"): + for command in ("env git status; echo ok", "(git status)", "git\tstatus # comment", + "env -S git status", "env -S 'git' status", "env -Sgit status", + "env --split-string=git status"): assert approval_floors._match_user_deny_rule(command) == "git status", command assert approval_floors._match_user_deny_rule('env git st""atus') == "git status" deny_config(['printf "a b"']) diff --git a/tools/approval_detection.py b/tools/approval_detection.py index bcc7d6faff..ba8d530e48 100644 --- a/tools/approval_detection.py +++ b/tools/approval_detection.py @@ -772,11 +772,11 @@ def _shell_segment_tokens(segment: str, start: int) -> list[str] | None: def _iter_top_level_shell_segments(command: str): """Yield top-level command segments in one left-to-right pass.""" start = 0 - for kind, i, _, quote in _scan_shell(command): - if kind == "char" and quote is None and command[i] in ";&|\n": + for kind, i, j, quote in _scan_shell(command, comments=True): + if kind == "comment" or (kind == "char" and quote is None and command[i] in ";&|\n"): if start < i: yield command[start:i] - start = i + 1 + start = j if start < len(command): yield command[start:] @@ -1149,6 +1149,10 @@ def _iter_shell_command_word_spans(command: str): continue if wrapper and options and deobfuscated.startswith("-"): option = deobfuscated.split("=", 1)[0] + if wrapper == "env" and (option == "--split-string" or deobfuscated.startswith("-S")): + # The split string and remaining argv form ONE command, handled + # by _env_split_payload; the suffix is not a new executable. + break queries = _COMMAND_WRAPPER_NON_EXECUTING_OPTIONS.get(wrapper, set()) if option in queries or (wrapper == "command" and not option.startswith("--") and set(option[1:]) & {"v", "V"}): @@ -1185,8 +1189,17 @@ def _env_split_payload(tokens: list[str]) -> str | None: return None option, equals, value = token.partition("=") if option == "--split-string" or token.startswith("-S"): - payload = value if equals else token[2:] if token.startswith("-S") else "" - return payload or (tokens[index + 1] if index + 1 < len(tokens) else None) + attached = equals if option == "--split-string" else len(token) > 2 + if not attached: + index += 1 + payload = (value if option == "--split-string" else token[2:]) if attached else ( + tokens[index] if index < len(tokens) else "") + try: + # env splits argv, not shell syntax: protect literal separators and + # expansions when feeding the existing command-position scanner. + return shlex.join(shlex.split(payload) + tokens[index + 1:]) + except ValueError: + return None index += 2 if not equals and option in _COMMAND_WRAPPER_OPTIONS_WITH_ARG["env"] else 1 return None diff --git a/website/docs/user-guide/security.md b/website/docs/user-guide/security.md index e3f0ae9390..9fcad7eb2f 100644 --- a/website/docs/user-guide/security.md +++ b/website/docs/user-guide/security.md @@ -157,7 +157,7 @@ Details: - Patterns are [fnmatch](https://docs.python.org/3/library/fnmatch.html) globs (`*`, `?`, `[...]`) matched **case-insensitively** against the whole command text and individual executable-command candidates. `git push --force*` matches `git push --force origin main` but not `git push origin main`. - Matching runs over the same normalized/deobfuscated command variants the dangerous-pattern detector uses, so simple quoting tricks (`git pu""sh --force`) don't slip past a rule. - Executable candidates retain the literal path and also match its basename: `sudo *` covers `/usr/bin/sudo -n id` and `./sudo -n id`. A path-specific rule such as `/usr/bin/sudo *` does **not** become a rule for every binary named `sudo`. -- Quote-aware parsing exposes commands after assignments, leading redirections, `;`, `&&`, `||`, pipelines, groups, command substitutions, and ordinary `if`/`then`/`else`/`do` transitions. Supported launchers include `sudo`, `env`, `command`, `exec`, `nohup`, `setsid`, `time`, `nice`, `timeout`, `stdbuf`, `ionice`, `chrt`, `taskset`, and `chroot`. Known option operands are skipped; `command -v`/`-V` lookups are not executions. Shell `-c` payloads and literal `env -S` / `--split-string` payloads are inspected recursively. +- Quote-aware parsing exposes commands after assignments, leading redirections, `;`, `&&`, `||`, pipelines, groups, command substitutions, and ordinary `if`/`then`/`else`/`do` transitions. Supported launchers include `sudo`, `env`, `command`, `exec`, `nohup`, `setsid`, `time`, `nice`, `timeout`, `stdbuf`, `ionice`, `chrt`, `taskset`, and `chroot`. Known option operands are skipped; `command -v`/`-V` lookups are not executions. Shell `-c` payloads are inspected recursively. Literal `env -S` / `--split-string` strings are split into arguments with the remaining command arguments appended; shell punctuation inside those arguments stays data unless an actual shell `-c` consumes it. Shell comments do not introduce executable candidates. - In the additional executable candidates, whitespace **between** words is collapsed, but quoted argument content and argument paths are retained. An exact rule such as `git status` therefore also matches `env git\tstatus; echo done` (where `\t` represents a tab). Quoted mentions such as `echo 'sudo -n id'` are not promoted to commands. Existing whole-input globs such as `*sudo*` still intentionally match mentions anywhere. - **YAML quoting:** always quote patterns. A bare leading `*` is a YAML alias and fails to parse; `{`, `!`, and `: ` have their own YAML meanings. Single quotes are safest for shell-ish content. - User-defined deny rules apply to all terminal backends, including isolated containers, before any backend-specific approval shortcut.