fix(bash): classify commands by their arguments, not their name - #495
Conversation
The shell classifier decided a command's effects from the binary name alone,
so any invocation of a name on the reader list was treated as read-only
regardless of what it actually did.
Two consequences, one live and one latent.
Live: the dangerous-command guard on the Bash tool matched its blocklist
against command names, so wrapping or quoting hid the real command. `'rm'
-rf /`, `env rm file` and `nohup rm file` were all accepted, in every mode.
Only a separate raw substring match on "rm -rf" caught the `-rf` spellings,
which is why this looked like it worked.
Latent: `git push`, `git reset --hard`, `env rm -rf /`, `awk
'BEGIN{system(...)}'`, `sort -o`, `less` (shell escape), `ip link set` and
`dig` all classified as read-only. Nothing shipped consumed that yet —
`validate_read_only` and the sandbox decision helper had no production
callers — but Auto mode now does, and plan mode is meant to.
Classification is now argument-aware for the names where arguments decide:
git (subcommand allowlist, global-option and program-invoking-option
refusal), env (recurses into the wrapped command), awk (system/redirect/
getline), find (-exec/-delete/…), sort (-o), uniq and hostname (positional
means write), ip and ifconfig (configure verbs), date (-s), ss (-K), rg
(--pre), and the pagers (always mutating — they spawn shells). Names are
unquoted before matching, so `-'delete'` and `-dele\te` no longer slip past
an option denylist. Anything undecidable classifies as mutating.
Redirection detection moves into the classifier, so the sandbox decision
helper is covered too — it previously returned "run freely" for
`echo evil > ~/.bashrc`.
Plan mode now enforces the read-only profile on the live Bash path rather
than relying on a check that nothing called.
Auto mode's per-segment double-check now classifies the invocation instead
of the binary name, which is strictly stronger: it distinguishes `git
status` from `git push` where a name lookup cleared both.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66b1083778
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| fn args_contain(args: &[String], names: &[&str]) -> bool { | ||
| args.iter().any(|a| names.contains(&opt_name(a))) | ||
| } |
There was a problem hiding this comment.
When Auto or plan mode evaluates git grep -O/tmp/pager pattern, opt_name leaves the attached short-option token intact, so it does not match -O and the invocation is classified read-only; Auto's secondary option check has the same exact-or-= limitation. The local git grep -h describes -O as opening matching files in a pager, and an execution probe confirmed this attached form runs the supplied executable, allowing arbitrary writes without a prompt in Auto and arbitrary execution in plan mode. Handle attached values for short program-invoking options rather than matching only the standalone spelling.
AGENTS.md reference: AGENTS.md:L93-L98
Useful? React with 👍 / 👎.
| if let Some(&next) = chars.peek() | ||
| && next == '&' | ||
| { | ||
| // `>&` (file descriptor duplication, e.g. `2>&1`). | ||
| // Not a write to a path. |
There was a problem hiding this comment.
When Bash is reachable in plan mode, echo owned >& plan-output creates or truncates plan-output, but this branch skips every >& sequence as though it were an FD duplication. Only forms such as 2>&1 duplicate descriptors; Bash also treats >&word as redirecting stdout and stderr to a file, so this command retains only ReadOnly effects and passes the newly live plan-mode gate. Check whether the operand is an FD before suppressing the write effect.
AGENTS.md reference: AGENTS.md:L93-L98
Useful? React with 👍 / 👎.
| let idx = tokens[1..] | ||
| .iter() | ||
| .position(|tok| !tok.starts_with('-') && !is_env_assignment(tok))?; |
There was a problem hiding this comment.
For wrappers with separated option operands, this selects the operand rather than the wrapped command: env -u HOME rm /tmp/victim resolves to HOME. env --help confirms that -u consumes a variable name, so the added AST guard never adds rm; the older raw checks also miss a non-recursive rm whose pipeline head is env, allowing the deletion through the destructive-command validation. Parse each supported wrapper's options and their operands before identifying the executable.
AGENTS.md reference: AGENTS.md:L100-L102
Useful? React with 👍 / 👎.
`find_destructive` matched its patterns against the raw command text, so re-spelling a command without changing what it runs walked past it. Verified end to end through `BashTool::validate_input`: chmod 777 /etc -> rejected ch\\mod 777 /etc -> ACCEPTED git push --force -> rejected 'git' push --force -> ACCEPTED Each invocation is now also scanned in normalized form — its tokens with shell quoting and backslash escapes removed, joined by single spaces. The pipeline-head check unquotes and de-paths too, so `/bin/rm` and `'rm'` are both recognised as `rm`. Both scans are additive: nothing the raw scan caught stops being caught. This is the same evasion class fixed in #495 for the name-based classifier. That fix unquoted at comparison time; this scan is a substring match over raw text and was never covered by it. Also pins a pre-existing limitation that this change does not address: the scan cannot distinguish a command from a string that merely mentions one, so `grep -r 'rm -rf' docs/` is refused. That is over-blocking, and separating arguments from commands is a design change of its own — the test documents the behaviour so a later fix has something to flip.
1 participant
Summary
The shell classifier decided a command's effects from the binary name alone. Any invocation of a name on the reader list counted as read-only regardless of what it actually did. This has one live consequence and one latent one — I want to be precise about which is which.
Live today: the dangerous-command guard is bypassable
BashTool::validate_input→check_parsed_securitymatches its blocklist against command names, so wrapping or quoting hides the real command. Driving the real validation path:This applies in every mode. The substring fallback is what made it look healthy.
Latent: the read-only classification was wrong
git push,git commit,git reset --hard,git clean -fd,env rm -rf /,awk 'BEGIN{system("…")}',sort -o /tmp/pwned,less(!cmdspawns a shell),ip link set eth0 downanddigall classified as read-only.Nothing shipped consumed that yet —
validate_read_onlyandsandbox_decision::decidehad no production callers, and plan mode's actual gate blocks Bash wholesale at the executor. So this was not an exploitable plan-mode escape. But Auto mode (#494) consumes the classifier now, and plan mode is supposed to use this check, so it was a loaded gun.What changed
Classification is argument-aware for the names where arguments decide:
gitpush/pull/fetch/clone→ Mutating+Network; unsafe globals (-c,-C,--git-dir,--exec-path) and program-invoking options (--ext-diff,--pager,--textconv,--upload-pack,-O) → Mutating;branch/tagread-only only with pure listing flagsenvawk/gawk/mawksystem(,close(,print >,|getline,-f progfile→ Mutatingless/more/pgfind-exec,-execdir,-ok,-delete,-fprintf,-fls→ Mutatingsort-o/--output→ Mutatinguniq,hostnameip,ifconfigdig/host/nslookupdate -s,ss -K,rg --pre,top(non-batch)Names and options are unquoted before matching, so
-'delete'and-dele\teno longer slip past a denylist. Anything undecidable classifies as Mutating.Also:
classify— the sandbox decision helper previously returned "run freely" forecho evil > ~/.bashrc.git statusandgit pushidentically; the invocation check separates them.Verification
git status/diff/log/show/blame,find . -name '*.rs',awk '{print $1}',sort f,du -sh * | sort -h, …) — a fix that made plan mode safe but useless would be its own failure.test_detect_quoted_dangerous_commandasserted only that parsing "at least works"); it now asserts the violation is reported. No test was weakened.clippy --all-targets -- -D warningsandfmt --checkclean. Full suite green except the 3 pre-existingbwrap_*tests that fail on this host withsetting up uid map: Permission denied.Not fixed, flagging
RIPGREP_CONFIG_PATHcan inject--preintorgfrom a config file; env-var-driven config for other tools has the same shape. Out of scope for a classifier — worth its own issue.