From 6178e9f4eed8d99f4fc550add939d58c7bed6206 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Mon, 7 Sep 2026 14:20:56 -0700 Subject: [PATCH] fix(approvals): honor GNU env split escapes and argv0 operands --- evals/approval_deny_dispatch.py | 28 ++++++++++ tests/tools/test_approval_deny_rules.py | 34 ++++++++++++ tools/approval_detection.py | 71 ++++++++++++++++++++++--- website/docs/user-guide/security.md | 4 +- 4 files changed, 128 insertions(+), 9 deletions(-) diff --git a/evals/approval_deny_dispatch.py b/evals/approval_deny_dispatch.py index 470964c623..91d31d6483 100644 --- a/evals/approval_deny_dispatch.py +++ b/evals/approval_deny_dispatch.py @@ -42,11 +42,39 @@ with tempfile.TemporaryDirectory(prefix="hermes-deny-dispatch-") as directory: "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")] + cases += [ + r"env -S 'printf\_SAFE'", r"env -S 'printf\_SAFE\c ignored'", + "env -S 'printf SAFE # ignored'", "env -S 'printf # ignored' SAFE", + "env -a marker printf SAFE", "env --argv0 marker printf SAFE", + "env --argv0=marker printf SAFE", "env -amarker printf SAFE", + r"env -a marker -S 'printf\_SAFE'", + "env -S '\"printf\"\\_SAFE'", "env -S \"'printf' SAFE\"", + r"env -S 'printf\_\_SAFE'", r"env -S 'printf\c ignored' SAFE", + "env -a sudo printf BENIGN", "env --argv0 sudo printf BENIGN", + "env -S 'printf %s \"sudo\\_-n\\_id\"'", + r"env -S 'printf BENIGN\c bash -c sudo'", + "env -S 'printf BENIGN # bash -c sudo'", + r"env -S 'printf %s \${IGNORED}'", + ] rows = [] for command in cases: result = registry.dispatch("terminal", {"command": command, "workdir": directory, "timeout": 10}) if isinstance(result, str): result = json.loads(result) rows.append({"command": command, "result": result}) + # Optional assertions keep the same harness usable for before/after receipts. + expected_outputs = {5: 'sudo -n id -u', 6: 'sudo;-n;id;', 9: 'ok', + 10: 'sudo -n id', 11: 'BENIGN', 27: 'BENIGN', 28: 'BENIGN', + 29: 'sudo -n id', 30: 'BENIGN', 31: 'BENIGN', 32: '${IGNORED}'} + if '--verify' in sys.argv: + for index, row in enumerate(rows): + result = row['result'] + if index in expected_outputs: + assert result.get('exit_code') == 0, row + assert result.get('output', '').strip() == expected_outputs[index], row + else: + assert result.get('status') == 'blocked', row + assert 'user-defined deny rule' in result.get('error', ''), row + assert not result.get('output'), row print(json.dumps({"source": str(source), "config": approval_context._get_approval_config(), "rows": rows}, indent=2)) diff --git a/tests/tools/test_approval_deny_rules.py b/tests/tools/test_approval_deny_rules.py index df06f1b771..85f7794e4e 100644 --- a/tests/tools/test_approval_deny_rules.py +++ b/tests/tools/test_approval_deny_rules.py @@ -6,6 +6,7 @@ making it the user-editable counterpart to the code-shipped hardline floor. """ import os +import shlex import pytest @@ -88,6 +89,13 @@ def test_deny_follows_executable_identity(deny_config, clean_env, monkeypatch): "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", + r"env -S 'printf\_SAFE'", r"env -S 'printf\_SAFE\c ignored'", + "env -S 'printf SAFE # ignored'", "env -S 'printf # ignored' SAFE", + "env -a marker printf SAFE", "env --argv0 marker printf SAFE", + "env --argv0=marker printf SAFE", "env -amarker printf SAFE", + r"env -a marker -S 'printf\_SAFE'", + "env -S '\"printf\"\\_SAFE'", "env -S \"'printf' SAFE\"", + r"env -S 'printf\_\_SAFE'", r"env -S 'printf\c ignored' SAFE", "2>/tmp/log FOO=bar /usr/bin/sudo -n id -u", "if true; then /usr/bin/sudo -n id -u; fi", ] @@ -95,6 +103,7 @@ def test_deny_follows_executable_identity(deny_config, clean_env, monkeypatch): deny_config(["sudo *", "printf SAFE"], mode=mode) monkeypatch.setattr(mod, "_YOLO_MODE_FROZEN", yolo) for command in commands: + assert approval_floors._match_user_deny_rule(command), command for guard in (mod.check_dangerous_command, mod.check_all_command_guards): result = guard(command, "local") assert result.get("user_deny") is True, (mode, yolo, command, result) @@ -112,6 +121,12 @@ def test_deny_projection_preserves_data_and_path_rules(deny_config): 'printf "%s" "$(printf safe) sudo -n id -u"', "command -v sudo", "command -V sudo", "command -pv sudo", "env -u sudo printf ok", "exec -a sudo printf ok", + "env -a sudo printf ok", "env --argv0 sudo printf ok", + r"env -S 'printf %s\_sudo\_-n\_id'", + "env -S 'printf %s \"sudo\\_-n\\_id\"'", + r"env -S 'printf ok\c bash -c sudo'", + "env -S 'printf ok # bash -c sudo'", + r"env -S 'printf %s \${IGNORED}'", "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'", @@ -131,6 +146,25 @@ def test_deny_projection_preserves_data_and_path_rules(deny_config): deny_config(['printf "a b"']) assert approval_floors._match_user_deny_rule('env printf "a b"') assert approval_floors._match_user_deny_rule('env printf "a b"') is None + # The argv projection must retain GNU escapes as data, not shell syntax. + from tools.approval_detection import _split_env_string + + for literal, expected in ( + (r'a\_b', ['a', 'b']), (r'"a\_b"', ['a b']), + (r"'a\_b'", [r'a\_b']), (r'a\cb ignored', ['a']), + ('a # ignored', ['a']), ('a#b', ['a#b']), (r'\#a', ['#a']), + (r'\${NAME}', ['${NAME}']), ("'${NAME}'", ['${NAME}']), + (r"'a\'b'", ["a'b"]), (r"'a\\b'", [r'a\b']), + (r'a\"b', ['a"b']), ('"" a', ['', 'a']), + *((rf'a\{key}b', ['a' + value + 'b']) + for key, value in {'f': '\f', 'n': '\n', 'r': '\r', 't': '\t', 'v': '\v'}.items()), + ): + assert _split_env_string(literal) == expected, literal + command = 'env -S ' + shlex.quote('printf %s ' + literal) + deny_config(['sudo *']) + assert approval_floors._match_user_deny_rule(command) is None, command + for unresolved in ('${NAME}', '"${NAME}"', r'"a\cb"', r'a\qb', "'unclosed"): + assert _split_env_string(unresolved) is None class TestDenyBeatsYolo: diff --git a/tools/approval_detection.py b/tools/approval_detection.py index ba8d530e48..6b8d777706 100644 --- a/tools/approval_detection.py +++ b/tools/approval_detection.py @@ -506,7 +506,8 @@ _SUDO_OPTIONS_WITH_ARG = {"-c", "--close-from", "-g", "--group", "-h", "--host", # data, not executable positions; option spelling remains case-sensitive. _COMMAND_WRAPPER_OPTIONS_WITH_ARG = { "chroot": {"--groups", "--userspec"}, - "sudo": _SUDO_OPTIONS_WITH_ARG, "env": {"-C", "--chdir", "-S", "--split-string", "-u", "--unset"}, + "sudo": _SUDO_OPTIONS_WITH_ARG, + "env": {"-a", "--argv0", "-C", "--chdir", "-S", "--split-string", "-u", "--unset"}, "exec": {"-a"}, "nice": {"-n", "--adjustment"}, "time": {"-f", "--format", "-o", "--output"}, "timeout": {"-k", "--kill-after", "-s", "--signal"}, @@ -1181,6 +1182,64 @@ def _shell_command_segment(command: str, start: int) -> str: return command[start:end].strip() +def _split_env_string(payload: str) -> list[str] | None: + r"""Project GNU env -S literal argv, not POSIX shell words. + + Dynamic ${NAME} expansion is deliberately not evaluated: the execution + backend's environment need not be this process's environment. + """ + escapes = {"f": "\f", "n": "\n", "r": "\r", "t": "\t", "v": "\v", + "#": "#", "$": "$", "\"": "\"", "'": "'", "\\": "\\"} + args, word = [], [] + quote, started, index = None, False, 0 + while index < len(payload): + char = payload[index] + index += 1 + if char == "\\": + if index == len(payload): + return None + escaped = payload[index] + if quote == "'" and escaped not in ("'", "\\"): + word.append(char) + started = True + continue + index += 1 + if escaped == "c": + if quote: + return None + break + if escaped == "_" and quote is None: + if started: + args.append("".join(word)) + word, started = [], False + continue + if escaped not in escapes and escaped != "_": + return None + word.append(" " if escaped == "_" else escapes[escaped]) + started = True + continue + if char in ("'", '"') and (quote is None or char == quote): + quote = char if quote is None else None + started = True + continue + if quote is None and char in " \t\n\r\v\f": + if started: + args.append("".join(word)) + word, started = [], False + continue + if quote is None and char == "#" and not started: + break + if char == "$" and quote != "'": + return None + word.append(char) + started = True + if quote: + return None + if started: + args.append("".join(word)) + return args + + def _env_split_payload(tokens: list[str]) -> str | None: index = 1 while index < len(tokens): @@ -1194,12 +1253,10 @@ def _env_split_payload(tokens: list[str]) -> str | None: 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 + args = _split_env_string(payload) + # Protect literal separators when reusing command-position detection; + # only a real shell -c carrier may turn these argv bytes into code. + return shlex.join(args + tokens[index + 1:]) if args is not None else 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 9fcad7eb2f..f809dcb09d 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 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. +- 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 executable-and-argument strings in `env -S` / `--split-string` use GNU quoting and escapes (including `\_` word boundaries and `\c` termination), with the remaining command arguments appended; shell punctuation inside those arguments stays data unless an actual shell `-c` consumes it. `env -a` / `--argv0` values are arguments, not executable names. Shell and GNU split-string 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. @@ -166,7 +166,7 @@ Details: Like the rest of the approval config, changes take effect immediately (the config cache is mtime-keyed) — no session restart needed. :::note Threat model -Deny rules are a shell-command policy, not a complete shell interpreter or an OS capability sandbox. Normalization does not resolve arbitrary variables, aliases, functions, renamed binaries, scripts, interpreter programs, or every shell/launcher grammar (for example, case-pattern syntax). Do not use a basename deny rule as a guarantee that a capability cannot be reached by other means. For containment, use OS permissions and an isolated backend with appropriately restricted mounts, credentials, and network access. This matching behavior does not change the configured approval mode or the empty-deny-list default. +Deny rules are a shell-command policy, not a complete shell interpreter or an OS capability sandbox. Normalization does not resolve arbitrary variables (including GNU `env -S` `${NAME}` expansion), aliases, functions, renamed binaries, scripts, interpreter programs, or every shell/launcher grammar (for example, case-pattern syntax, clustered launcher options, or options embedded inside an `env -S` string). Do not use a basename deny rule as a guarantee that a capability cannot be reached by other means. For containment, use OS permissions and an isolated backend with appropriately restricted mounts, credentials, and network access. This matching behavior does not change the configured approval mode or the empty-deny-list default. ::: ### Approval Timeout