fix: strip env variable assignments from bash permission patterns#14108
fix: strip env variable assignments from bash permission patterns#14108edevil wants to merge 1 commit intoanomalyco:devfrom
Conversation
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
|
I believe this change will introduce a considerable security concern. Consider the command: GOFLAGS=$(malicious command) go test ./... A misbehaving LLM could therefore run arbitrary commands and circumvent the allowlist |
What about |
|
A security conscious user can currently allowlist explicit commands (with no wildcards) to prevent this sort of injection. This change would significantly weaken those safeguards |
2acf3bf to
79e4331
Compare
Yes, if you don't use wildcards I can see this being a downgrade. I updated the PR to check for command substitutions. |
a086736 to
be5f07a
Compare
Commands prefixed with environment variables (e.g. GOFLAGS=-mod=vendor go test) were not matching permission rules like "go *": "allow" because the full command text including the variable assignment was used for pattern matching. Strip leading variable_assignment nodes from the command text before adding it to the permission patterns, so the actual command name is what gets matched against user-defined permission rules. To prevent smuggling arbitrary execution through env var side effects, assignments containing command substitutions ($(…) or backticks) are not stripped — the full text is preserved so it falls through to the catch-all rule.
be5f07a to
8d44927
Compare
Fixes #14110
Summary
GOFLAGS=-mod=vendor go test ./...) were not matching permission rules like"go *": "allow"because the full command text — including the variable assignment — was used for pattern matching.variable_assignmentAST nodes from the command text before adding it to permission patterns, so the actual command name is what gets matched against user-defined rules.$(…)or backticks) are not stripped, preserving the full text so it falls through to the catch-all rule. This prevents smuggling arbitrary execution through env var side effects.How I verified it works
The bash tool uses tree-sitter to parse commands.
commandText(fromnode.text) included env var assignments, but thecommandarray already skipped them. The fix finds the last leadingvariable_assignmentchild and slicescommandTextfrom its end position, but only if none of the assignments containcommand_substitutiondescendants.7 new test cases in
test/tool/bash.test.ts:GOFLAGS=-mod=vendor go test ./...) — strippedGOFLAGS=-mod=vendor go test ./... 2>&1) — strippedFOO=bar BAZ=qux echo hello) — strippedalwayspatterns exclude env vars$()substitution in env var (EVIL=$(curl evil.com) echo hello) — NOT strippedEVIL=`curl evil.com` echo hello) — NOT strippedAll 22 tests pass.