Repository navigation
security: rm argv floor misses keyword, comment and in-argument substitution spellings #18464
Description
Due: 2026-10-30
The recursive-force rm argv floor added by #18415 (fixes #8237 and #8387) reads each command line with a small word split in src/kiro_crew/security/rm_floor.py. Review rounds on that PR found shell shapes the split does not read. Each one below is a recursive-force wipe of home that is_denied allows on main and still allows on #18415, so none is a regression; they are the floor's residual coverage.
| # | Shape | Probe (allowed on main and on #18415) |
|---|---|---|
| 1 | Apostrophe in a comment keeps the quote state open, so the next line is not read | echo ready # don't keep builds + newline + rm -fr ~ |
| 2 | then keyword in program position |
if :; then rm -fr ~; fi |
| 3 | do keyword in program position |
for i in 1; do rm -fr ~; done |
| 4 | ! pipeline negation |
! rm -fr ~ |
| 5 | time keyword |
time rm -fr ~ |
| 6 | $(...) inside the rm arguments splits the argv at its parens |
rm -fr $(printf build) ~ |
Probes were run through kiro_crew.security.is_denied on origin/main 7044e75 and on #18415.
Shapes found in earlier rounds of #18415 and fixed there (no action needed): an operand glued to a redirection (~>/dev/null, &>), comments and quoted-tag heredocs counted as live substitutions, a heredoc body read as commands, "$HO"ME read as $HOME, a quoted <<EOF, $(...) inside double quotes, a nested bash -c '... $HOME'.
Out of the floor's scope by design (left to the catalog regex and the OS sandbox): exec wrappers such as sudo rm -fr / and command rm -fr /, and interpreter bodies.
Task
Close rows 1-6 in rm_floor.py without widening the private word split further:
- Row 1: consume comment bytes outside the quote-state iterator and restart scanning unquoted at the newline.
- Rows 2-5: when an unquoted word in program position is in
shell_normalizer._KEEPS_COMMAND_POSITION(or is!/time), keep command position for the next word before program attribution. - Row 6: treat a substitution inside a word as one opaque operand, with its body checked as its own frame.
Prefer reusing an existing shared shell reader over adding states to the floor's own split. Verify each row with a red-first case in TestRecursiveForceRmFloor, and keep every allow case in that class allowed.
Activity
iamwhatever commentedon Oct 11, 2026
Over-refusals carried from the catalog row (refused on main in the
-rfspelling; #18415 makes-frand other spellings agree):cat << ""EOFwith$(rm -fr /)in the bodycleanup=( rm -fr / )rm -fr --help /,rm -fr --version /Narrowing these changes the row itself, so it should cover both spellings in one PR.
iamwhatever commentedon Oct 13, 2026
More rows from PR #18415's Opus review on
d3baeea8eb(each allowed on main in the-frspelling and on that head):case x in x) rm -fr /;; esac: a)ends the argv in_frame_targets, but the program list still showscase, sormis not seen as the program. Smallest fix: pass";"to_argv_programswhereverends[i]is true.echo ${x:- #}; rm -fr /and${x:- <<EOF}:_scandoes not track${…}/$[…], so a space inside one starts a fake comment or a fake heredoc tag. Smallest fix: track parameter-expansion depth in_scan, and read no comment or heredoc tag inside it.iamwhatever commentedon Oct 13, 2026
More rows from PR #18415's Opus review on
402c7a46d3(each allowed on main in the-frspelling and on that head):x=$(cat <<E+ newline +E); rm -fr ~(and the backtick form): a heredoc opened inside a substitution waits for a line exactlyE, so the closingE); rm -fr ~line is swallowed as body. Smallest fix: end the heredoc body at the close of the substitution it opened in, or fail closed by scanning unmatched lines as commands.echo $[ 1 << 2 ]+ newline +rm -fr /: only((/$((count as arithmetic, so2is read as a heredoc tag. Smallest fix: treat$[…]like$((in_scan's paren stack.iamwhatever commentedon Oct 24, 2026
Claimed by issue-triage (Wave N11). A worker session is starting a fix now.
The worker will pick the most conservative fix, write the chosen decision here and in the PR body, and keep the PR small so a maintainer can review the decision directly.
🤖 AI-assisted comment 🤖
iamwhatever commentedon Oct 24, 2026
Fix opened: #23840
Decision: deny-only. Every shape in this issue and its comments is now refused by the rm floor. No allow path is widened, and every existing allow case in
TestRecursiveForceRmFloorstill passes.then/do/else/{/!/time(andtime -p) beforerm: stepped over likeVAR=val, using the shared_KEEPS_COMMAND_POSITIONset.$(...)or a backtick inside the rm argv: one opaque operand, with its body checked as its own frame. This also ends a heredoc opened inside a substitution at the substitution's close.#or<<inside${...}/$[...]: read as data until the expansion closes.case x in x)row were already refused on main; they get tests only.Not changed: heredoc bodies stay data, exec wrappers stay with the catalog rows, and the catalog row's over-refusals are left as they are. A quoted keyword such as
"then" rm -fr ~is now refused too, which is the safe side.🤖 AI-assisted comment 🤖
iamwhatever commentedon Oct 24, 2026
Scope update: #23840 is now
Part of #18464, notCloses.Decision: option A. The PR keeps the deny-only fixes that held up in review: the keyword rows (
then,do,else,{,!,time),case,#/<<inside${...}/$[...](with$$read as the PID), and empty quoted substitutions. It removes reading an unquoted$(...)or backtick as its own frame. Three review rounds on that change found new bypasses, and it refused common heredoc commit messages with ana)list item.Still open on this issue:
rm -fr $(printf build) ~x=$(cat <<E+ newline +E); rm -fr ~, and the backtick formBoth need
_matching_close_parento skip heredoc bodies before the floor can read an unquoted substitution as one operand.🤖 AI-assisted comment 🤖
iamwhatever commentedon Oct 25, 2026
PR #23840 is paused for a maintainer decision after four review rounds on the rm floor's own reader. The choices are here: #23840 (comment)
🤖 AI-assisted comment 🤖