fix(security): point the traversal passes at a source body's command strings - #8550
Conversation
51cb7e7 to
0c1079e
Compare
|
Force-pushed The withdrawn sentence said the passes left on the whole body "judge tokens or match text under no whole-subject structural budget, so a document costs them noise rather than a verdict". The second half is wrong. With the sibling backtick over-count corrected (#8557), a 1107-line cron script that this PR frees from the stage ceiling reaches That is pre-existing rather than introduced here — this PR moves the two traversal passes and does not touch the env pass — but "a document costs them noise rather than a verdict" was load-bearing reasoning for leaving them alone, and it is not true. The docs now say what was measured instead, and name the env pass's subject as the next question in the sequence rather than carrying it silently. Scope is unchanged deliberately: the env pass needs its own subject decision (it is a detection, not a budget, so the argument that carried the traversal passes does not transfer), and folding a third pass into this change would make one review carry three separate premises. |
8d2522f to
fd9dc77
Compare
|
Scope widened, and my earlier reason for keeping it narrow withdrawn. Force-pushed to Two comments ago I said the env-credential pass "needs its own subject decision" and kept it out because it is a detection rather than a budget, so the argument carrying the traversal passes did not transfer. That was wrong. The mechanism differs but the premise is identical: a rule written for ONE command line, handed a document.
Measured: a 48 KB cron body draws the env-credential denial while containing no such pipeline at all. The pattern's opening accessor appears near the top, a pipe character somewhere after it, one of its filter words inside a comment, and the credential prefix later still. No single line trips it, and no window of 3, 10 or 40 lines reproduces it. So Scoping the env rules costs nothing on this path, and the PR pins it: the Python spelling those rules are not about — What is deliberately left alone, and why, is now stated rather than assumed: passes 1 to 3 and the IMDS check match text or judge tokens with neither a structural budget nor an ordered-existence shape, so a document cannot assemble a verdict out of pieces no command line holds together. The IMDS check in particular matches an address — a hit means the subject really contains one, wherever it sits. Also in this push: the test file is renamed to Regression: 4059 passed / 2 skipped across the 63 Effect on the install this was measured against: with the sibling backtick fix (#8557) applied alongside, 12 of 13 cron scripts recover. The 13th is refused for a genuine reading of its own text — a docstring whose first word is "Find", followed by 3 KB of prose that the |
|
Run 33919328007: 251 passed, 1 failed, 1 flaky, and the failed and flaky specs are exactly the two This PR's diff is Not dismissed as flaky: the failure is deterministic, it is tracked, and the sibling PR carrying the same base (#8557) is expected to show the same red for the same reason. |
fd9dc77 to
ac908ef
Compare
Design Review (Fable 5, fork) — ✅ PASSDesign-level review of Design-Verdict: PASS Right shape: re-pointing command-line passes at the command strings a body contains fixes the premise violation, keeps the shell path byte-identical, and fails closed everywhere. Suggestions
[DESIGN-REVIEWED] 4cb69e1 |
First Principles Review (Fable 5, fork) — 🟡 CONCERNSPremise-level review of Verification against the trusted base is complete. The reported defect is real: First-Principles-Verdict: CONCERNS The description promises subject-scoping for "only those two" traversal passes; the diff also scopes the env-credential rules — an undeclared, allow-direction rider. What this change shipsIntent: stop the cron script-body gate refusing ordinary long Python scripts for their length — a FIX.
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] 4cb69e1 |
GPT 5.6 Review (fork) — 🔴 changes requested (blocking)Reviewed 2 of 2 blocking finding(s) are security-class and were withheld from adjudication, so the blocking verdict stands. BLOCKING -- src/kiro_crew/security.py:10163 -- Split literals bypass the environment-credential guard BLOCKING -- src/kiro_crew/security.py:10223 -- Literal-walk recursion fallback is discarded |
Opus 4.8 Review (fork) — 🔴 changes requested (blocking)Reviewed I've verified the mechanism against the code. Key confirmations:
All three grounded: (a) attacker-authored cron body with a leading deep Fix is fully in-diff (honor the returned flag), and the consequence is governance-ceiling/credential exposure, so it stays BLOCKING. Merge blocked: the new BLOCKING — src/kiro_crew/security.py (is_sensitive_source_body, the |
ac908ef to
4cb69e1
Compare
…e subject Three passes in `is_sensitive_bash_command` require a subject that IS a command line. `is_sensitive_source_body` handed all of them the whole Python body, and each then answers a question the document cannot support -- for two different reasons. The two traversal passes walk shell STRUCTURE under a fail-closed budget. Stages split on newline, so every line of a source file counts as a pipeline stage: a body of a few hundred lines exhausts `_ALT_MAX_STAGES` while carrying no shell content at all, and `"x = 1\n" * 600` is refused. The refusal is keyed on the body's SIZE -- an ordinary script is banned for its length, at every fire, and the fire-time gate keeps the job -- while a small malicious body is still fully inspected. What the ceiling costs is the feature, not the fence. The env-credential rules are ORDERED-EXISTENCE patterns describing one pipeline. Over a document they match pieces lying arbitrarily far apart, so a 48 KB body drew that denial while containing no such pipeline: the pattern's opening env accessor appears near the top, a pipe character somewhere after it, one of its filter words inside a comment, and the vendor prefix later still. No single line trips it and no 40-line window reproduces it. That one is a false DENIAL rather than a refusal, and it was masked only because the traversal passes run first. Measured on one real install, 6 of 13 cron scripts were refused by the stage ceiling, and 8 once the sibling substitution budget landed: intake, dispatch, cleanup and audit all stopped at a gateway restart with no local change. So a parsed body now hands those three passes the command strings it CONTAINS. The rest are left on the whole subject on purpose: passes 1 to 3 and the IMDS check match text or judge tokens with neither a structural budget nor an ordered-existence shape, so a document cannot assemble a verdict out of pieces no command line holds together. This is a change of SUBJECT, not the "spend the budget then answer smaller" shape whose fail-open instances were removed alongside `_AltWorkBudget`: each subject is analysed completely, to the same depth, under the same budget, the first denial wins so adding subjects can only ADD denials, and a caller that supplies no subjects keeps the whole-subject behaviour byte for byte. - `_parse_source_body` is the one spelling of the parse and of what unparseable means, shared with the literal fence scan so the two cannot disagree, and the body is parsed once however many questions are asked of it. - `_source_command_subjects` collects every `str`/`bytes` constant, f-string fragments and docstrings included, decoded as the fence scan decodes them. Nothing is filtered as "cannot be a command": a subject naming no traversal matches no rule, so over-collecting costs work and can only add denials, while under-collecting is a missed read. A whitespace-only value is the one exclusion, provable rather than heuristic. - An unparseable body has no strings to hand over and keeps the whole-document scan, pass 1b and every budget included, so it is never quietly exonerated. - `_SOURCE_COMMAND_SUBJECT_CAP` bounds the dimension this opens up, the subject COUNT: per-subject cost is bounded (150-620 us measured) but the count is author-controlled. The largest real body measured (1170 lines) carries 441 constants and the median ~140, so the cap is 1024 -- ~2.3x the largest real body, ~0.6 s of worst-case work -- and exhausting it REFUSES, for the same reason `_ALT_MAX_STAGES` does. Scoping the env rules loses nothing on this path: the PYTHON spelling they are not about is caught by the cron gate's own bare-NAME matcher, which runs first and stays whole-document, and is pinned by test here. Residual, named rather than silently carried: `_find_substitution_openers` counts every backtick as a substitution opener, so a docstring with 66 markdown code spans reads as 66 nested substitutions and still refuses. Shell backticks cannot nest without escaping, so the opener count over-states the nesting depth the budget guards; correcting that proxy is a separate change. Refs kirodotdev#7912, follows kirodotdev#7913. Co-authored-by: Kiro Crew <kiro-crew@amazon.com>
4cb69e1 to
a3df114
Compare
|
All three verdicts addressed at GPT #2 / Opus (same finding) — ACCEPTED, fixedThe Reproduced before touching anything, on the reviewed head: After: GPT #1 — gap ACCEPTED and closed; the causal claim CORRECTEDThe gap is real and your suggested fix is what I implemented. But "cron source vetting scans fragments separately → protected values reach cron stdout" as a consequence of this diff does not hold, in either direction, and the measurement says why: Split the credential NAME across fragments ( Closed anyway, because it is cheap and it is a strict improvement: the env rules now read One defect surfaced while implementing it, and it is worth recording because it would have made the fix silently useless: First Principles — CONCERNS accepted, description rewrittenCorrect on both counts, and the second one was the more useful catch: the description named On the rider: the env scoping was declared in the commit message and in Your item 4 ("no longer match pieces spread across a document") is the intended behaviour and is now pinned in both directions: the document-spread false denial stays allowed, and the fragment-assembled command is denied. Verification at
|
Problem / Motivation
is_sensitive_source_bodyhands the whole Python body tois_sensitive_bash_command, andthree passes in there require a subject that IS a command line. Each then answers a question
a document cannot support, for two different reasons.
The two traversal passes walk shell STRUCTURE under a fail-closed budget.
_alt_collect_stagessplits on newline /;/|, so every line of a source file counts as apipeline stage and a few hundred lines exhaust
_ALT_MAX_STAGEScarrying no shell at all:The env-credential rules are ordered-existence patterns describing one pipeline, so over a
document they match pieces lying arbitrarily far apart and produce a false DENIAL. A 48 KB body
drew that denial while containing no such pipeline: the accessor near the top, a pipe after it, a
filter word inside a comment, the credential prefix later still. No single line trips it and no
40-line window reproduces it.
Follows #7913, which closed the reported instance of #7912 (pass 1b, the separator collapse).
Same root cause, different passes; #7913's branch was measured and 8 of 13 cron scripts stayed
refused on it.
Why it matters
The traversal refusal is keyed on the body's size, not on anything it contains, so it bans
ordinary scripts for being long while a small malicious body is still fully inspected. What the
ceiling costs is the feature, not the fence — an availability defect rather than an open path,
which is why it reads as fail-closed and correct to a security review.
It is also not recoverable by the operator. The gate runs at every fire and, by design, keeps
the job and does not feed the auto-pause counter, so an affected script is refused on every tick
indefinitely, with a message naming shell constructs the body does not contain.
Measured on one real install: 6 of 13 script crons refused, 8 once the sibling substitution budget
landed — intake, dispatch, cleanup and audit all stopped at a gateway restart with no local change
and no script edited for three days.
What changed
A parsed and fully walked body hands those three passes the command strings it CONTAINS. The
rest are left on the whole subject on purpose: passes 1 to 3 and the IMDS check match text or judge
tokens with neither a structural budget nor an ordered-existence shape, so a document cannot
assemble a verdict out of pieces no command line holds together.
This is a change of SUBJECT, not the "spend the budget then answer smaller" shape whose fail-open
instances were removed alongside
_AltWorkBudget: each subject is analysed completely, to the samedepth, under the same budget; the first denial wins, so adding subjects can only ADD denials; and a
caller that supplies no subjects keeps the whole-subject behaviour byte for byte.
The two KINDS of pass get different subjects, because they need opposite things:
_traversal_subjects— each command string on its own. Structure needs a small subject._env_subject— all of them CONCATENATED in source order. An ordered-existence rule needs thepieces together: a body can assemble its command from fragments where no fragment matches.
Supporting pieces:
_parse_source_bodyis the one spelling of the parse and of what "unparseable" means, shared withthe literal fence scan so the two cannot disagree, and the body is parsed once.
_source_command_subjectscollects everystr/bytesconstant — f-string fragments anddocstrings included — decoded as the fence scan decodes them, in source order (
ast.walkisbreadth-first, so a left-nested
+chain comes back scrambled). Nothing is filtered as "cannot bea command": a subject naming no traversal matches no rule, so over-collecting costs work and can
only add denials, while under-collecting is a missed read. A whitespace-only value is the one
exclusion, provable rather than heuristic.
literal walk must have completed.
visitis recursive and reports a depth overflow asparsed=False, so a body that parses but overflows falls all the way back to the whole documentwith pass 1b included.
every budget included.
_SOURCE_COMMAND_SUBJECT_CAP = 1024bounds the dimension this opens up, the subject COUNT:per-subject cost is bounded (150–620 µs measured) but the count is author-controlled. The largest
real body measured (1170 lines) carries 441 constants and the median ~140, so the cap is ~2.3× the
largest real body, ~0.6 s of worst-case work, and exhausting it REFUSES for the same reason
_ALT_MAX_STAGESdoes.Docs updated in the same commit:
docs/system-specs/modules/security.mdanddocs/system-specs/modules/learn-cron-dashboard.md.Tests
test/test_security_source_command_subject.py, 30 cases, pinning every direction:(
"echo x | " * 600 + "rg . ~/.kiro/crew") still denied, and so is the plain 600-stage command,ceiling message intact;
grep -rrooted above the fence,find … -exec cat, abytescommand literal, and one behind 600 statements (denied for thetraversal, asserted not for the size);
back in source order, and the document-spread false denial stays allowed;
inspected=False, is still denied, and specifically keeps pass 1b;the fence scan still runs on the only literal.
Mutation-verified — each load-bearing decision was reverted and the suite reddened: whole-document
traversal subject (8 red), over-cap allowed (2), unparseable body not falling back (2), the
inspectedflag discarded (2), env on the whole document (2), literals not in source order (2).Regression sweep: 4066 passed, 2 skipped across the 63
test_security*/test_mcp_cron*/test_cron*files.Gates:
isort,flake87.1.0,mypy,scripts/check_black_formatting.py,scripts/check_subprocess_encoding.py,scripts/docs_lint.py— all pass.security.pywasdeliberately not run through a bare
black: it is in the black baseline, and formatting itreformats 528 pre-existing lines of unrelated churn.
Effect on the measured install, with the merged sibling #8557 in main: 12 of 13 scripts recover.
Residual, named rather than silently carried
The 13th is refused for a genuine reading of its own text: a docstring whose first word is "Find",
followed by 3 KB of prose that the
findgrammar reads as root operands until_FIND_ROOT_BUDGETgives up. That is the "prose is indistinguishable from a command" limit, not a counting error, and
not something to paper over in this gate.
Pattern harvest
Rule candidate: review rule, plus one semgrep-shaped check.
Pattern (review rule): when a gate acquires a caller whose subject is a different KIND of text,
every pass inside it must be re-asked whether its premise still holds — one at a time.
is_sensitive_bash_commandis documented throughout as taking a command line. #7913 gave it asecond kind of subject and correctly re-scoped the one pass whose premise it had examined. Three
others silently kept the old premise, and the two failure modes that produced were opposite in kind
— a refusal keyed on SIZE, and a false denial assembled from distant pieces — which is why
finding one did not lead to the other. The generalizable move is that a new caller with a new
subject kind is a per-pass re-derivation, not a single decision; the violated premise is never
visible at the call site.
The same shape produced the two findings this PR took in review: a two-valued signal (
parsedandinspected) was collapsed into one, and a subject that must be read TOGETHER was handed overpiecewise. Both are "the new subject does not satisfy the old contract", one layer further in.
Pattern (semgrep-shaped): an ordered-existence regex is only sound on a subject with
single-statement scope. A pattern of the form
A .* B .* Casserts "these appear in this order inONE command". Applied to multi-line input it asserts almost nothing. Candidate check: flag any regex
combining two or more
.*-separated alternation groups that is reachable from a caller passing inputthat may contain a newline, unless the pattern is anchored per line. Both halves are statically
visible: the pattern's shape, and whether any caller's subject is line-bounded.
Not generalizable, and stated so it is not mistaken for the above: the specific value 1024 for
_SOURCE_COMMAND_SUBJECT_CAPis a measurement against one corpus, not a rule.