fix(shell): re-check command wrappers and path-prefixed names against the block list#3131
Open
warmjademe wants to merge 1 commit into
Open
fix(shell): re-check command wrappers and path-prefixed names against the block list#3131warmjademe wants to merge 1 commit into
warmjademe wants to merge 1 commit into
Conversation
… the block list CommandsBlocker/ArgumentsBlocker matched only args[0] verbatim, so the bannedCommands block list (the hard control that survives skip_permission_requests / crush run auto-approve) was defeated by a single wrapper or a leading path: nohup curl, env curl, env -S 'curl ...', timeout 5 nc, setsid scp, /usr/bin/curl, ./curl all bypassed it. The same holds for dangerous-exec entries (sudo, systemctl) behind a wrapper. blockHandler now re-applies the same block funcs to each layer, peeling one exec wrapper at a time (the recursive re-dispatch pattern scriptDispatchHandler already uses for scripts), and matches the block list against filepath.Base of the command name. A closed recognition table of exec wrappers (nohup, setsid, nice, ionice, stdbuf, chrt, taskset, runuser, setpriv, flock, timeout, env, xargs) is peeled; env -S / --split-string values are re-tokenized. The protected set (bannedCommands) is unchanged; a missing wrapper fails open to current behavior. This hardens a best-effort guardrail; it is not a sandbox. Interpreter forms (sh -c, python3 -c), command substitution, find -exec, busybox/toybox applets, and novel wrappers remain out of scope.
Contributor
|
All contributors have signed the CLA ✍️ ✅ |
Author
|
I have read the Contributor License Agreement (CLA) and hereby sign the CLA. |
1 similar comment
Author
|
I have read the Contributor License Agreement (CLA) and hereby sign the CLA. |
Author
|
recheck |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Crush blocks a set of dangerous leaf commands at the shell exec boundary
(
internal/agent/tools/bash.go::bannedCommands→internal/shellCommandsBlocker/ArgumentsBlocker, installed viablockFuncs()). This isthe hard control that still applies when permission prompts are skipped
(
permissions.skip_permission_requests, the non-interactivecrush runauto-approve path). Its largest category is network/download tools (
curl,wget,nc,scp,ssh, ...) plussudo/su, package managers, andmount/systemctl/crontab.blockHandler(internal/shell/run.go),CommandsBlockerandArgumentsBlocker(internal/shell/shell.go) only ever inspectedargs[0],verbatim. Two one-token bypasses follow directly:
Exec wrapper prefix. The interpreter invokes the exec handler once per
command with that command's argv. A banned command placed behind a wrapper
binary that is not itself banned slips straight through and is exec'd as
a child process out of the interpreter's reach:
The same hole defeats
nohup sudo rm -rf /,env systemctl stop ...,timeout 60 apt install ...— so it covers both the network-egress entriesand the dangerous-execution entries of
bannedCommands.Absolute / relative path. A path prefix changes
args[0]so thebasename match misses entirely:
All of the above were confirmed executing against the unpatched fork (negative
control:
curlreaches DNS resolution,apt installreaches its package step,etc. — real non-security exit codes, not the security error).
Fix
Two changes, both contained to the existing exec-handler choke point. No entry
is added to or removed from
bannedCommands.blockHandlerre-dispatches the inner command through the same blockfuncs (
internal/shell/run.go). Whenargs[0]is a recognized execwrapper,
unwrapCommand(newinternal/shell/wrappers.go) peels exactly onewrapper layer — the wrapper token, its own option flags and their values,
envNAME=valueassignments,env -S/--split-string(re-tokenized onwhitespace), and a leading value positional (
timeout'sDURATION,flock's lockfile) — and the handler loops, re-running the sameblockFuncsagainst each peeled layer. This is the same recursivere-dispatch idea
scriptDispatchHandleralready uses for path-prefixedscripts ("so deny rules apply recursively"): the protection is the existing
block list applied to the argv that actually runs, not a parallel list
of "dangerous behind a wrapper" commands. Nesting such as
nohup env timeout 5 curl ...is handled by the loop.commandWrappersis a recognition table of leaf binaries whose documentedpurpose is to exec a command argument:
nohup,setsid,nice,ionice,stdbuf,chrt,taskset,runuser,setpriv,flock,timeout,env,xargs. This is a closed set of wrappers, not a denylist ofdangerous commands; the thing it protects (
bannedCommands) is unchanged.CommandsBlocker/ArgumentsBlockermatch on the command basename(
internal/shell/shell.go), so/usr/bin/curl,./curl, and/usr/bin/apt installare matched by the same entry as the bare name. A sharedbaseNamehelper normalizes the path (including Windows-style separators, since the
bash tool runs POSIX emulation on every platform).
The diagnostic names the command that actually runs, by basename:
"curl",not
"nohup"or"/usr/bin/curl". Behavior for non-wrapped, non-banned,bare-name commands is unchanged.
Scope and known limits (honest)
This hardens an existing best-effort guardrail; it is not a sandbox. A
command-name block list cannot stop a process that takes its command as data,
and the following bypasses remain open with or without this change:
Interpreters / subshells:
sh -c 'curl ...',bash -c ...,python3 -c ...,perl -e ....sh/bashare not inbannedCommands, sothese bypass the block list today. They are not exec wrappers in the
unwrapCommandsense (they consume the command as an opaque string, not as atrailing argv that can be peeled), so they are deliberately out of scope.
-cstring forms of otherwise-handled binaries:env -cdoes not exist,but
flock /tmp/l -c 'curl ...'andxargs sh -c '...'route throughshand fall in the interpreter class above.
Building the command name at runtime:
$(printf cur)l https://x, writinga script to disk then executing it, base64-decoding into a shell.
Wrappers outside the recognition table: any leaf binary that execs a
command but is not in
commandWrappers(e.g. an unusualscript -qc,watch,parallelform) —watch/parallel/script -clargely route theirpayload through a shell, so they reduce to the interpreter class; genuinely
novel exec wrappers would need an entry added.
Applet multiplexers:
busybox <applet>/toybox <applet>(andbusybox sh -c ...). The applet name is the inner command but the multiplexer binaryis what is invoked; the applet is not a trailing argv that
unwrapCommandpeels, and
busybox/toyboxare not incommandWrappers. Same residualclass as a novel exec wrapper; not covered by this patch.
find -exec:find . -exec curl {} \;runs the banned command throughfind's ownexec, not through the shell exec handler that the block listhooks;
findis not a recognized wrapper. Not covered.safeCommandsprefix match insafe.go: the permission-prompt layertreats a command as safe by prefix, so
nohup curl ...still skips theconfirmation prompt (the prompt is advisory). This is unchanged by this
patch — but the block layer hardened here is the hard control and now
catches
nohup curl, so the advisory gap does not translate into anunblocked egress. Prompt = advisory; block = hard control.
A note on the test corpus:
rmappears in the test block list(
realBlockFuncs) only to illustrate the mechanism on a dangerous-executionentry. Crush's real
bannedCommandsissudo/su/doasand similar; it doesnot contain
rm. This patch does not addrmto any list and makes noclaim of protecting
rm.The patch closes the trivial wrapper-prefix and path-prefix bypasses — the
forms an LLM or a naive prompt injection reaches for first — and keeps the
diagnostic accurate. The real egress / exec boundary remains the OS sandbox /
network namespace; no claim is made that egress or dangerous execution is now
prevented, only that the existing block list is no longer defeated by a
one-token wrapper or a leading slash.
Tests
internal/shell/wrappers_test.go:TestBaseName— basename normalization, including a Windows-style path.TestUnwrapCommand— 30-case table for the one-layer peel: options,value-flags, combined shorts,
--,envassignments,env -S/--split-string(separate and inline value),timeout/flock/taskset/chrtleading positional (DURATION / lockfile / CPU mask / priority),path-prefixed wrapper recognition, and bare-wrapper (
envalone)returning
ok=false.TestCommandBlocking_WrapperAndPathBypassClosed— 24 cases driven through thereal
Shell.Execpath: every wrapper-smuggled / path-prefixed banned commandis now blocked, with the inner command named. Covers egress (curl/wget/nc/scp
via nohup/env/env -S/nice/timeout/setsid/stdbuf/xargs/flock/runuser/taskset/
chrt, plus a
nested
nohup env timeout 5 curl) and dangerous execution (env sudo rm -rf,nohup systemctl stop,timeout 5 rm -rf,nohup apt install,timeout 60 npm install -g), plus absolute/relative path behind a wrapper.TestCommandsBlocker_PathPrefixed/TestArgumentsBlocker_PathPrefixed—deterministic blocker-level assertions for
/usr/bin/curl,./curl,/bin/sudo,/usr/bin/apt install, with negatives (/usr/bin/echo,mycurl,apt list).TestCommandBlocking_LegitWrappedCommandsAllowed— 24-case false-positivecorpus: wrapping a NON-banned command (
nohup ./myserver,timeout 30 echo,nice -n 10 echo,env FOO=bar echo,env -i PATH=... echo,env -S 'A=b echo',setsid echo,stdbuf -oL echo,flock /tmp/l echo,echo x | xargs echo,npm install lodash,env -u curl make(curl is the-u value),
flock /var/lock/at make(at is the lockfile),timeout -s SIGTERM 30 echo, ...) is NOT blocked.The existing
internal/shellsuite (command_block_test.go, etc.) andinternal/agent/toolssuite stay green.Build / verification (golang:1.26)
go build -buildvcs=false ./...→ 0 errors.go vet -buildvcs=false ./internal/shell→ clean.go test -buildvcs=false -count=1 ./internal/shell ./internal/agent/tools→both
ok.nohup curl,/usr/bin/curl,env -S 'curl ...',timeout 5 nc,nohup sudo rm -rf,env curl,./curl,timeout 60 apt install,nohup systemctl stopall executed(not blocked) — bug reproduced before the fix.
git apply --checkagainst pristine v0.76.0 → RC 0.