wrapper-guard: judge command position on prefixes and on what a shell will execute
ci/woodpecker/pr/ci Pipeline failed
ci/woodpecker/pr/ci Pipeline failed
Round-three review found two more absence-driven allows, both in the skeleton introduced by round two, and fixing them exposed a third the reviewer had not reached yet. 1. A word in front of a command does not displace the command. `env VAR=v curl`, `command curl`, `timeout 10 curl` and `/usr/bin/curl` were all real writes at execution position that a bare-name match could not see. The `env` form is the one that matters: it is what an agent reaches for to keep a credential out of the global environment, so the careful spelling was the invisible one. 2. Quoted data stops being data when a shell is about to execute it, and the first version knew only `bash -c`, `sh <<` and `eval`. It did not know the pipe, which is the form people actually use: `printf ... | sh`, `cat <<EOF | sh`, `sh -s <<EOF` each made a live call vanish from the skeleton while still running. 3. Found while testing the fix: that decision was made for the WHOLE command, so a single unrelated `docker run ... sh -c 'echo hi'` promoted every other quoted span on every other line to code. It blocked its own author for the second time in a day. A shell on one line does not execute a string on another line, and over-blocking is not the safe direction — a guard that blocks ordinary work gets switched off, and a guard that is off permits everything. The skeleton is now built per line, and a heredoc body is code only when the line that opened it fed a shell. All seven reviewer repros are pinned as fixtures, each with a counter-fixture in the allowed direction: `echo timeout 10 curl ...` is not a call, a pipe to `wc` is not execution, an unrelated shell on another line changes nothing. Fixtures 40/40, and a sweep of 18 ordinary commands blocks none of them. Gates: sanitization, resident budget, test enumeration, tools-index (self-test 4/4, git suite 100%), prettier.
This commit is contained in:
@@ -74,6 +74,27 @@ FIXTURES="$TMP/fixtures.tsv"
|
||||
printf '0\t{"tool_input":{"command":"cat >> notes.md <<EOF\\nwe ran: curl -d@b https://git.example.invalid/api/v1/repos/a/b/issues\\nEOF"}}\ta heredoc body is data, not code\n'
|
||||
# ...but quotes stop being data the moment something executes them.
|
||||
printf '2\t{"tool_input":{"command":"bash -c \\"curl -d@b https://git.example.invalid/api/v1/repos/a/b/pulls/1/merge\\""}}\tbash -c makes the quoted text code again\n'
|
||||
# --- round three. The skeleton was right about position and wrong about which
|
||||
# words hold it. A word in front of a command does not displace the command:
|
||||
# each of these four is a real write that the bare-name match could not see.
|
||||
printf '2\t{"tool_input":{"command":"env GITEA_TOKEN=$T curl -d@b https://git.example.invalid/api/v1/repos/a/b/issues/1/comments"}}\tenv VAR=... in front of the client is still the client\n'
|
||||
printf '2\t{"tool_input":{"command":"command curl -d@b https://git.example.invalid/api/v1/repos/a/b/issues/1/comments"}}\tcommand in front of the client is still the client\n'
|
||||
printf '2\t{"tool_input":{"command":"timeout 10 curl -d@b https://git.example.invalid/api/v1/repos/a/b/issues/1/comments"}}\ttimeout N in front of the client is still the client\n'
|
||||
printf '2\t{"tool_input":{"command":"/usr/bin/curl -d@b https://git.example.invalid/api/v1/repos/a/b/issues/1/comments"}}\tan absolute path to the client is still the client\n'
|
||||
# ...and the prefix list must stay a list of prefixes. `echo` is not one.
|
||||
printf '0\t{"tool_input":{"command":"echo timeout 10 curl -d@b https://git.example.invalid/api/v1/repos/a/b/issues/1/comments >> notes.md"}}\tnaming a command after echo is not running it\n'
|
||||
# A shell standing between quoted data and execution makes that data code,
|
||||
# and the pipe is the form agents actually use. Filing it as data allowed the
|
||||
# call to vanish from the skeleton while still running.
|
||||
printf '2\t{"tool_input":{"command":"printf '"'"'%%s\\\\n'"'"' '"'"'curl -d@b https://git.example.invalid/api/v1/repos/a/b/issues/1/comments'"'"' | sh"}}\tquoted code piped to a shell is code\n'
|
||||
printf '2\t{"tool_input":{"command":"cat <<EOF | sh\\ncurl -d@b https://git.example.invalid/api/v1/repos/a/b/issues/1/comments\\nEOF"}}\ta heredoc piped to a shell is code\n'
|
||||
printf '2\t{"tool_input":{"command":"sh -s <<EOF\\ncurl -d@b https://git.example.invalid/api/v1/repos/a/b/issues/1/comments\\nEOF"}}\tsh -s reads its script from the heredoc\n'
|
||||
# ...but a pipe to anything that is not a shell leaves the data as data.
|
||||
printf '0\t{"tool_input":{"command":"grep -R \\"curl -d@b https://git.example.invalid/api/v1/repos/a/b/issues/1/comments\\" docs/ | wc -l"}}\tpiping an example to wc is not executing it\n'
|
||||
# ...and a shell on ONE line does not execute a string on another. Switching
|
||||
# the whole command into code because it contains an unrelated `sh -c` is how
|
||||
# this blocked its author a second time.
|
||||
printf '0\t{"tool_input":{"command":"docker run --rm alpine sh -c '"'"'echo hi'"'"'\\necho \\"example: curl -d@b https://git.example.invalid/api/v1/repos/a/b/issues/1/comments\\" >> notes.md"}}\tan unrelated shell on another line does not promote quoted prose to code\n'
|
||||
} > "$FIXTURES"
|
||||
|
||||
fail=0 n=0
|
||||
|
||||
@@ -106,23 +106,50 @@ fi
|
||||
# skeleton would be blind to them.
|
||||
#
|
||||
# The exception is the reason quotes are data at all: if something is about to
|
||||
# EXECUTE the quoted text (`bash -c`, `sh <<EOF`, `eval`), then the quotes hold
|
||||
# code and the skeleton is the full text again.
|
||||
if printf '%s' "$CMD" | grep -Eq '(^|[[:space:]])((ba|z)?sh[[:space:]]+-c|(ba|z)?sh[[:space:]]*<<|eval[[:space:]])'; then
|
||||
# Quotes become command separators rather than disappearing: in `bash -c
|
||||
# "curl ..."` the client IS at command position, just one interpreter down.
|
||||
SKEL="$(printf '%s' "$CMD" | tr "\"'" ';;')"
|
||||
else
|
||||
# Heredoc bodies first (line-oriented), then quoted spans (span-oriented).
|
||||
SKEL="$(printf '%s' "$CMD" | awk '
|
||||
{ if (hd != "") { if ($0 == hd) hd=""; next }
|
||||
if (match($0, /<<-?[[:space:]]*'"'"'?"?[A-Za-z_][A-Za-z0-9_]*/)) {
|
||||
t = substr($0, RSTART, RLENGTH); sub(/^<<-?[[:space:]]*['"'"'"]?/, "", t); hd = t
|
||||
}
|
||||
print }' | sed "s/'[^']*'//g; s/\"[^\"]*\"//g")"
|
||||
fi
|
||||
# EXECUTE the quoted text, then the quotes hold code and the skeleton is the
|
||||
# full text again. The first version of this list named only `bash -c`, `sh <<`
|
||||
# and `eval`, and review immediately produced the spellings it did not know:
|
||||
# printf '%s\n' 'curl -d@b .../comments' | sh
|
||||
# cat <<EOF | sh
|
||||
# sh -s <<EOF
|
||||
# Each one executes; each one had its client filed away as data. So the test is
|
||||
# not "which invocation form did I think of" but "does a shell stand between
|
||||
# this data and execution" — including the pipe, which is the common form.
|
||||
SHELL_EXECUTES_DATA='(^|[[:space:]|;&(])((ba|z)?sh|dash)([[:space:]]+-[a-z]*[cs]([[:space:]]|$)|[[:space:]]*<<)'
|
||||
SHELL_EXECUTES_DATA="$SHELL_EXECUTES_DATA"'|(^|[[:space:]|;&(])eval([[:space:]]|$)'
|
||||
SHELL_EXECUTES_DATA="$SHELL_EXECUTES_DATA"'|\|[[:space:]]*((ba|z)?sh|dash)([[:space:]]|$)'
|
||||
|
||||
CLIENT_AT_CMD_POS='(^|[;&|(){}]|`|\$\()[[:space:]]*([A-Za-z_][A-Za-z0-9_]*=[^[:space:]]*[[:space:]]+)*(curl|wget|httpie|http)([[:space:]]|$)'
|
||||
# That decision is made PER LINE, not for the whole command. The first version
|
||||
# switched globally, and it blocked its author again within the hour: a script
|
||||
# whose only shell invocation was an unrelated `docker run ... sh -c 'echo hi'`
|
||||
# had every OTHER quoted span on every other line promoted to code with it.
|
||||
# A shell on one line does not execute a string on another, and a guard that
|
||||
# says otherwise is back to blocking ordinary work.
|
||||
SKEL="$(printf '%s' "$CMD" | awk -v inv="$SHELL_EXECUTES_DATA" '
|
||||
function ascode(s) { gsub(/["\047]/, ";", s); return s } # quotes separate
|
||||
function asdata(s) { gsub(/\047[^\047]*\047/, "", s); gsub(/"[^"]*"/, "", s); return s }
|
||||
{
|
||||
# Inside a heredoc: the body is code only if the line that OPENED it fed a
|
||||
# shell (`cat <<EOF | sh`, `sh -s <<EOF`). Otherwise it is a document.
|
||||
if (hd != "") { if ($0 == hd) { hd=""; next }
|
||||
if (hdcode) print ascode($0); next }
|
||||
if (match($0, /<<-?[[:space:]]*\047?"?[A-Za-z_][A-Za-z0-9_]*/)) {
|
||||
t = substr($0, RSTART, RLENGTH); sub(/^<<-?[[:space:]]*[\047"]?/, "", t)
|
||||
hd = t; hdcode = ($0 ~ inv)
|
||||
}
|
||||
print ($0 ~ inv) ? ascode($0) : asdata($0)
|
||||
}')"
|
||||
|
||||
# Command position is not "the first word is literally `curl`". A word can sit
|
||||
# in front of a command without displacing it, and review found four ordinary
|
||||
# ones hiding a real write: `env GITEA_TOKEN=$T curl`, `command curl`,
|
||||
# `timeout 10 curl`, `/usr/bin/curl`. The `env` form matters most, because it is
|
||||
# precisely what an agent reaches for to keep a credential out of the global
|
||||
# environment — the careful spelling was the invisible one.
|
||||
CMD_PREFIX='([A-Za-z_][A-Za-z0-9_]*=[^[:space:]]*|env|command|builtin|exec|nohup|setsid|stdbuf|nice|ionice|sudo|doas|xargs|time|timeout|[0-9]+[smhd]?|-[^[:space:]]+)[[:space:]]+'
|
||||
# A path in front of the client is still the client.
|
||||
CLIENT='([^[:space:]]*/)?(curl|wget|httpie|http)'
|
||||
CLIENT_AT_CMD_POS='(^|[;&|(){}]|`|\$\()[[:space:]]*('"$CMD_PREFIX"')*'"$CLIENT"'([[:space:]]|$)'
|
||||
|
||||
if printf '%s' "$SKEL" | grep -Eq "$CLIENT_AT_CMD_POS" \
|
||||
&& printf '%s' "$CMD" | grep -Eq 'https?://'; then
|
||||
|
||||
Reference in New Issue
Block a user