fix(security): close find -exec / write side-doors in shell command whitelist (CWE-184)

[kovtcharov-amd]

run_shell_command is documented as restricted to read-only, informational commands and gated behind confirmation. Before this change, that promise was breakable: the validator only checked the first token against the whitelist, so whitelisted commands with write/exec/delete predicates ran prohibited operations that a direct call blocks. A researcher demonstrated find … -exec touch {} + creating a file that direct touch cannot (CWE-184). Investigation found the same class of hole in find -delete, find -fprint*, sort -o (incl. -oFILE, -ro, and --output abbreviations like --out), and uniq INPUT OUTPUT — the researcher's one-line "block find -exec" fix would have left those open. After this change, all of these are rejected while legitimate read-only forms (-print/-printf/-ls/-name, plain sort/uniq) still work.

The audit-workflow hardening and the PSIRT/CVSS triage skill that came out of this finding are in a separate PR (#2752).

Test plan

• PYTHONPATH=src python -m pytest tests/unit/test_shell_guardrails.py — 63 passed (adds find exec/execdir/ok/okdir/delete/fprint*, sort -o/-oFILE/-ro/--out/--output=, uniq-output guards, plus read-only forms staying allowed)
• black --check clean on changed files
• Researcher's PoC no longer confirms the bypass against the patched source

[github-actions]

Verdict: Approve.

This closes a real whitelist bypass (CWE-184): find, sort, and uniq were allowed as "read-only" but each has a side-door — find -exec/-delete, sort -o, uniq out.txt — that writes files or runs arbitrary binaries not on the whitelist. The new per-command guards block those actions while leaving the genuine read-only forms working, and the tests cover the tricky spellings (attached -oFILE, bundled -ro, GNU long-option abbreviations, case tricks). Nicely scoped: two files, no drive-by changes.

The guards deliberately err toward over-blocking, which is the right direction for a security check. Only nit is a slightly inaccurate comment; nothing blocking.

Real-world evidence

Strong — the evidence bundle exercises the real run_shell_command tool path (pulled from the tool registry, actual subprocess.run), not just the validate() test helper:

• find … -exec touch …/pwned.txt {} + → blocked, and pwned.txt was confirmed not created.
• find … -delete → blocked, and the canary file survived.
• sort -o … and uniq in out → blocked with the read-only-policy error.
• Read-only forms (find -print, sort file, uniq file) still succeed; spot-regression on ls/cat/grep | sort/git status all pass.
• tests/unit/test_shell_guardrails.py: 63 passed; lint clean.

Agent-UI screenshot correctly marked N/A (this tool has no UI panel; no agent turn runs on the no-inference runner). Evidence matches the surface this PR changes and supports the Approve.

🔍 Technical details

Strengths

• Correct layering: the new elif branches sit inside _validate_command, so they're applied per-segment in the pipeline path (shell_tools.py:672-680) too — a … | sort -o x bypass is covered, not just top-level commands.
• The -exec … + case is the one that genuinely needs this guard: the ;-terminated form is already caught by DANGEROUS_SHELL_OPERATORS, but + is not, so the find branch is real defense, not redundant.
• uniq operand counter correctly skips value-consuming flags (-f/-s/-w + long forms) before deciding a second operand is an output file (shell_tools.py:411-434).
• Tests assert the effect (file not created / not deleted), not just the error dict — matches GAIA's "verify the call is valid, not just invoked" guidance.

🟢 Minor — sort guard comment is inaccurate (shell_tools.py:386-387)
The comment says "'o' is the only sort short-flag letter, so any pure short cluster containing it is the output flag." sort actually has many short flags (-b -d -f -g -i -k -m -n -r -s -t -u -z …); the reason the heuristic is safe is that it over-blocks, not that -o is the only letter. A cluster like -to (field separator o) would be blocked as output — harmless over-block, but the stated rationale is wrong and could mislead a future maintainer. Consider:

        # Special handling for sort - -o/--output writes to a file. Cover every
        # spelling: -o FILE, -oFILE, -o=FILE, bundled short clusters (-ro), and
        # every GNU long-option abbreviation of --output (--o, --out, --output=).
        # Any short cluster containing 'o' is treated as -o; this over-blocks a
        # few exotic clusters (e.g. -t with separator 'o'), which is acceptable
        # for a read-only security guard.

🟢 Follow-up (out of scope, non-blocking) — other allowed commands have write side-doors. sysctl -w key=val and date -s both mutate state and are on ALLOWED_COMMANDS, though both need root so the risk is lower than the find/sort/uniq cases this PR fixes. Worth a tracking note for a later sweep; no need to expand this PR.

[kovtcharov-amd]

Good catch on the sort comment — it was wrong, and I've fixed it in 734706c. sort --help lists 16 short flags (-b -d -f -g -i -h -n -r -c -k -m -o -s -t -u -z), so "'o' is the only short-flag letter" was simply false. Confirmed your -to example too: sort -to file is legitimate read-only usage (field separator o) that this guard rejects. The behaviour stays as-is — over-blocking is the right bias for a security check — but the comment now states the real reason instead of a wrong one, so nobody "corrects" the heuristic later on a false premise.

On the follow-up: sysctl -w and date -s are real write side-doors on ALLOWED_COMMANDS, and you're right that they're lower risk (both need root) and out of scope here. Filing a tracking issue for a full sweep of the whitelist rather than expanding this PR.

[github-actions]

Verdict: Request changes (small, one-line fixes — the core hardening is solid)

This PR closes the find -exec / sort -o / uniq out.txt write-and-exec side-doors in the shell whitelist (CWE-184), with thorough tests and real tool-call evidence showing the bypasses are blocked before any subprocess runs. Good, high-value work.

Two residual holes in the same class remain, though — both let a write slip through the very guard this PR adds:

• find still allows -fprint0. The block set covers -fprint, -fprintf, and -fls, but -fprint0 is the same "write results to a file" action. find . -fprint0 /tmp/x writes a file and isn't blocked. One-word fix: add it to the set.
• sort misses the bundled+attached form -ro/tmp/x. -o, -ro (separate), -oFILE, and --output are all caught, but when a short cluster carries an attached value (-ro/tmp/x = -r -o /tmp/x) the isalpha() check fails on the / and the output write slips through.

Since the whole point of the PR is to close this bypass class, worth folding both in before merge. Suggestions below.

Real-world evidence

Strong and matched to the surface. evidence-bundle.md exercised the registered run_shell_command @tool (pulled from _TOOL_REGISTRY, invoked as the agent loop would) against real subprocesses, not just the internal helper. All three bypass primitives were rejected and — verified on disk — no target file was created:

--- find -exec bypass attempt ---
"error": "find action '-exec' is not allowed: ..."
canary_find exists after blocked call: False
--- sort -o bypass attempt ---
"error": "sort -o/--output writes to a file, ..."
canary_sort.txt exists after blocked call: False
--- uniq output-file bypass attempt ---
"error": "uniq with an output file is not allowed: ..."
canary_uniq_out.txt exists after blocked call: False

Legit read-only find -name / sort / uniq still work; a spot-regression pass confirmed sibling allow/deny (grep, rm, sort -k) is unchanged. pytest tests/unit/test_shell_guardrails.py → 63 passed. Screenshot/live-LLM path is correctly N/A here — this guard fires inside the agent tool loop (needs inference), deferred to the strix-halo lane. The verdict rests on static review of the two gaps above plus this evidence.

🔍 Technical details

🟡 find guard omits -fprint0 (src/gaia/agents/tools/shell_tools.py:17) — GNU find's -fprint0 FILE writes (null-separated) to FILE, exactly like the -fprint/-fprintf/-fls actions already blocked. find . -fprint0 /tmp/canary passes the guard. Add it (and note the existing test class covers -fprint/-fprintf/-fls but not -fprint0):

    "-delete",
    "-fprint",
    "-fprint0",
    "-fprintf",
    "-fls",
}

🟡 sort guard misses bundled+attached -o value (src/gaia/agents/tools/shell_tools.py:399-402) — body.isalpha() returns False once the attached value adds a /, so -ro/tmp/x (= -r -o /tmp/x) is not flagged, while -ro (separate token) is. Checking whether o appears in the leading short-flag run closes this and is consistent with the file's documented "over-block exotic clusters like -to" tradeoff:

                    body = flag[1:]
                    # An 'o' in the leading short-flag run means -o consumes the
                    # rest of the cluster as its value (-o, -oFILE, -ro, -ro/tmp/x).
                    lead = ""
                    for ch in body:
                        if ch.isalpha():
                            lead += ch
                        else:
                            break
                    if "o" in lead:
                        is_output = True

This keeps every existing test green (-r -u → allowed, -ro → blocked, -k1,1 → allowed) and adds coverage for -ro/tmp/x. Worth a matching test in TestSortOutputGuard and one for -fprint0 in TestFindActionGuards.

Strengths

• Per-segment validation means the new guards apply to each pipeline stage (shell_tools.py:673-681), so ... | find … -exec is covered, not just a leading find.
• Evidence asserts the effect (no file on disk), not just the error message — the right way to prove a security guard.
• The sort/uniq flag-parsing correctly handles attached (-oFILE), =-joined (--output=), and value-consuming (uniq -f 2) forms, with tests for each.

[kovtcharov-amd]

Both gaps confirmed and fixed in d9398f5 — you were right on both, and both were genuinely reachable.

Verified each is a real write primitive before fixing (not just a theoretical flag):

• find /tmp -maxdepth 0 -fprint0 /tmp/canary → file created on disk. Added -fprint0 to DANGEROUS_FIND_ACTIONS.
• sort -ro/tmp/out.txt in.txt → file written. The isalpha() check failed on the /, exactly as you described.

For the sort cluster I went slightly broader than a special-case: match the leading run of letters as the flag cluster and treat anything after it as an attached value, so -o, -oFILE, -ro, -ro/tmp/x, and -o=FILE all resolve the same way rather than each needing its own branch. Regression-checked 15 legitimate forms (-n -rn -k1,1 -t: -c -S1G --reverse --key=1, plain sort) — all still allowed, no new false positives.

Two tests added (test_find_fprint0_blocked, test_sort_output_bundled_attached_blocked); 65 passing, lint clean.