fix(security): re-join the path a find traversal factors apart - #7298
Conversation
Design Review (Fable 5) — ✅ PASSDesign-level review of Design-Verdict: PASS A real, deterministic keystone-floor bypass closed by a fail-closed, additive-only pass; alternatives measured, residual class honestly descoped to filed issues. [DESIGN-REVIEWED] 258bea5 |
GPT 5.6 Review — ✅ human override acceptedHuman judgment by @chenmingwei23 overrides the GPT 5.6 finding for This comment is updated in place on each push. The model was not re-run because an authorized human decision supersedes it. False positive or not applicable? A repository writer can comment: |
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of All claims verified. Composing the review. First-Principles-Verdict: CONCERNS Every item closes a measured bypass of the reported defect, but the change ships the repo's third curated secret-filename vocabulary, with the two siblings named and left to diverge. What this change shipsIntent: stop a prompt-injected agent reading fenced secrets by making
More granular differences exist (BSD pre-root flags, glued operators, redirect ordering); all serve items 1–2. No new public surface, config key, or flag; existing machinery ( Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] 258bea5 |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsI've analyzed the sole candidate against the code. Candidate 1 — over-budget numeric brace sequence degrading to a literal. The divergence is real at the code level: But the finding needs (c) an observable wrong outcome, and that cannot be established:
So (c) resolves to "if any expansion resolves onto a fenced path" — a case the diff's own semantics make unconstructable, exactly the "could/might" the falsification bar rejects. The candidate's own confidence line concedes it could not build a concrete exploit. The item is a documented-contract divergence with no reachable wrong ALLOW; it does not survive. No other finding grounds to the (a)/(b)/(c) bar within the changed lines. No findings. [OPUS-REVIEWED] 258bea5 Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
Round 1 disposition -- all findings acted on, none overriddenEvery finding reproduced before it was touched. The probe results below come from a fake Opus 4.8 -- 3 findings, all REAL, all fixed
Fixed wider than prescribed, because the prescription (reduce the regex's fixed leaf) would have left the glob spelling open.
First Principles (CONCERNS) + Design Review -- the same ask, doneBoth rounds asked for the residuals statement to name sibling traversal tools so the pass cannot be read as closing the traversal class. Agreed, and it is the failure mode #7034 itself warns about. The block comment now names
On the second First Principles concern,
GPT 5.6 -- review incomplete on the previous headNo verdict was produced for Verification on the new head116 cases in |
UX Review (Fable 5) — ⏭️ skippedRevision |
b43f92a to
be815c2
Compare
Round 2 -- the undeclared frontend change is not in this PRFirst Principles, "a full frontend change (#7272's content) rides in the diff undeclared, apparently from a stale base" -- the stale-base half of that reading is right, and the conclusion that follows from it is not. #7272's content was never in this PR's diff. GitHub's own file list for the PR, at the head that was reviewed: Two files. No The mechanism is the one That also explains the UX round, which reviewed an icon change and passed it: there is no icon change here to pass. Both lanes were reading #7272. Fixed at the source rather than argued: the branch is rebased onto current main ( One disclosure on the push itself: the local commit-message privacy hook refused the rebase because upstream commit |
Round 2 disposition -- the BLOCKING finding was real, and it had a second halfOpus, Two of the three filter families were stripped and Fixed at the read site rather than by adding the third strip at the call site. The second half, which the finding did not name and which matters more. The reason a corrupt pattern was a PERMIT rather than a denial is that the matchers were collected with a filter that DROPPED whatever would not compile, while All three were allowed before this round and are denied now. An opaque pattern is read as The boundary is pinned in both directions: fail-closed applies to UNCOMPILABLE, never to merely unmatched. Verification127 cases in No |
9ca5932 to
22acd92
Compare
22acd92 to
bd08e22
Compare
Round 3 disposition -- the DoS is real, and the reproducible hang is in the path the finding did not nameOpus, Screening for catastrophic shapes was considered and rejected rather than skipped. This module already carries the argument against it -- enumerating dangerous punctuation does not terminate against an untrusted string -- and a hostile pattern author is exactly that case. The half the finding did not name, which is the one that actually hangs. The prescribed fix scopes to
Globs are bounded rather than refused, because they can be: adjacent One property makes the cap sufficient rather than a guess: the SUBJECTS are not agent input. They are fence basenames and credential-store entry names, all short. Only the pattern is hostile. The cost, stated plainlyA delivering Verification137 cases in
No |
Round 16: both lanes found the same crash in my own code, and I nearly dismissed itHead Fixed: this pass let
|
Round 17: Opus clean again; GPT's one finding is now filed as #8150Head GPT:
|
Round 18: the capture strip was defeating the opaque-filter mechanismHead Fixed: a computed filter lost its opaque reading to my own stripThe reported command denies, so the finding looked unreproducible at first: But that is the fenced root deciding on its own -- clause 1 answers before the filter matters. The filter only DECIDES when the root is not itself fenced, and there the finding is exact: The mechanism for a computed filter was never missing: a pattern carrying an unresolved substitution sets This is the same shape as the pre-filter removed in round 10 and the escaped pattern in round 14: the capability existed and a text manipulation of my own defeated it. Not the descoped computed-operand class, which is about values this pass cannot compute at all. The strip is now bounded by balance rather than applied unconditionally: a trailing Not patched: three already answered
A note on the harnessThe mutation run came back 25/26 with one ANCHOR-MISS rather than a survivor: round 14's mutation targeted the exact line this round rewrote, so it could no longer be applied. That is a harness failure, not a passing test -- an anchor that stops matching silently reduces coverage while still reporting a number. Retargeted at the escape substitution, which is what that mutation is actually about. Verification1537 passed / 1 skipped. 27 mutations, 27 killed, no anchor misses -- including one that restores the unconditional strip. flake8, isort, mypy and the baselined black gate clean. Scope against the merge base: 1233 + 1580 insertions, 0 deletions, 2 files, 1 commit. Nothing merged, approved or overridden. |
Round 19: BSD pre-root options hid the root; the other four are already answeredHead Fixed: an unrecognised pre-root option ended the roots run
An option outside the set ends the roots walk, so the traversal was read as rooted at Two things worth separating. The
Benign traversals through the same flags stay allowed ( Not patched: four already answered
Verification1548 passed / 1 skipped. 29 mutations, 29 killed, no anchor misses -- two new ones stop recognising the bundle and stop collecting Nothing merged, approved or overridden. |
Round 19 follow-up: my two new tests were wrong, and the failure proved the fix worksHead The path in the denial reason is the whole explanation: the fixture places the crew home under So the failure was the new option handling working -- Both assertions now root at The lesson is about the assertion, not the flag: a directory is only outside the fence relative to where the fence is, and the fence's location is environment-dependent. Picking a "clearly benign" absolute path is an assumption about the test environment, and Nothing else changed: the BSD pre-root option set, the bundles, and Verification1548 passed / 1 skipped, and the corrected assertions re-verified under a planted fence rather than only on this machine. 29 mutations, 29 killed, no anchor misses. flake8, isort, mypy and the baselined black gate clean. Scope against the merge base: 1268 + 1633 insertions, 0 deletions, 2 files, 1 commit. Nothing merged, approved or overridden. |
Round 20: a wall-clock assertion of mine was measuring main's cost, not this pass'sHead That is seconds, not milliseconds -- the gate took 10.25 s against a 10 s budget. So not a threshold shaved too fine, but a real cost. The question was whose. AttributionThe test bounded two shapes. Measured separately, with a tree lacking this pass as the control: The wide shape is the slow one, and it is identical with and without this pass. My pass contributes 10 ms of walk and then refuses the command at the 64-opener budget; the remaining 2.4 s is the other passes' quadratic behaviour on wide substitutions, which this change neither causes nor can guarantee. A slow Windows runner turns 2.4 s into more than 10 s, and the assertion failed on a cost that was never this PR's. So the wall clock was the defect: a timing bound over What replaced itDeterministic assertions about the invariants the test is actually for:
The wide input is now sized just past the budget rather than pathologically wide, since the invariant is the comparison and extra width only re-imports the other passes' cost into this test's runtime. And a false premise in the old test, which the rewrite exposedAsserting a verdict on the 40-layer input failed, which is correct: naive single-quote wrapping does not nest in shell. Verification1548 passed / 1 skipped. 29 mutations, 29 killed, no anchor misses. flake8, isort, mypy and the baselined black gate clean. Scope against the merge base: 1268 + 1646 insertions, 0 deletions, 2 files, 1 commit. Nothing merged, approved or overridden. |
Round 21: the closable set is now emptyHead
No patch, per the descope. Both need a value this pass cannot compute from the text, which is the distinction the boundary rests on. Worth recording the trajectory, because it is the argument that the boundary is holding rather than merely being invoked. GPT's finding count across the last four settled heads went 5 -> 4 -> 5 -> 2, and every finding that was closable got closed:
Three of those six were defects in my own additions rather than in the original design, which is the honest shape of this PR's later rounds. What remains is one decision, unchanged since it was raised: the two findings above are the descoped class, so the lane cannot go green without either the polarity inversion in #8074 (its own change, with the false-positive surface priced) or an override. The override is @chenmingwei23's -- asked at 06:05Z and still open -- and I will not press it. Verification unchanged from the last push: 1548 passed / 1 skipped, 29/29 mutations killed with no anchor misses, flake8 / isort / mypy / black gate clean, 1268 + 1646 insertions with 0 deletions across 2 files in 1 commit. Nothing merged, approved or overridden. |
|
There is no advisory and no package. The log names no GHSA, no severity and no dependency. Both PRs fail on one line, byte-identical:
So the gate did not find a vulnerability. Neither PR introduced it, and neither PR's own dependencies are involved. #7199 touches zero lines of
The lane passed on this PR at 15:09:22Z with 1028 packages, and has failed on every run after Classification: main-owned, and a tooling budget rather than a dependency. Not a new advisory on a main dependency either — that was the natural hypothesis and the log rules it out. Two consequences worth being explicit about, because both differ from the obvious action:
What actually fixes it has to land on I have not changed anything in #7199 for this and will not. |
Open PR relationship auditThis is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion. Relationship findings
No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit. |
Round 24: two real findings, recovered from a lane that reported no verdictHead 1. Brace-expanded roots evaded the fence -- fixed, whole class
This is not the globbed-root case parked in #8074, and the distinction is the whole reason it is in scope. A glob cannot be enumerated without asking the filesystem what exists. Brace expansion is finite and decided by the text alone -- same family as the quoting, backslash-escape and capture-strip gaps closed in earlier rounds. So the set closes rather than being sampled. Measured before the fix, all six spellings allowed where the plain root denied:
2. Root resolution was unbounded -- fixed, and the premise measured firstEach root costs a filesystem resolution per candidate form, and nothing upstream bounds how many roots a
One measurement corrected itself here and is worth recording. With 500 roots all naming the fenced directory the branch still read 726 ms after the fix, which looked like the bound failing. It was not: main costs 706.9 ms on that same command and the branch 738.5 ms, so the residual is main's own scan of a long line, and the branch's marginal cost is +31.6 ms. The first probe simply had no control for that case. 3.
|
Round 25: two more real findings, and a scope discipline that changed the fixHead 1.
|
| spelling | before |
|---|---|
F=fi; F+=nd; $F |
ALLOW |
F=fi; F+=nd; ${F} |
ALLOW |
F=; F+=find; $F (empty initial value) |
ALLOW |
F=f; F+=i; F+=nd; $F (repeated append) |
ALLOW |
F=fi; F+=n; ${F}d (append then split use) |
ALLOW |
F=fi; F+='nd'; $F (quoted tail) |
ALLOW |
F=fi ; F+=nd ; $F (spaced operators) |
ALLOW |
An append tail is a LITERAL, so what the shell runs is decided by the text -- the same reason quoting, backslash escaping and brace expansion were in scope, and the same reason $(printf find) is not.
2. accessTokens.json -- added, after both premises were measured
The Azure CLI's bearer-token cache. This vocabulary requires two things before a name goes in, and both were checked rather than assumed:
- A direct read already denies.
cat <fenced>/.azure/accessTokens.jsondenies on this branch and on main. So naming it makes the traversal agree with the direct read instead of becoming stricter than it -- the test that sentauthorized_keysandknown_hoststo bash gate: a spelling-based find recognizer cannot close computed operands; invert the polarity #8074 instead. - The file is itself a credential -- it holds access and refresh tokens.
Stored case-folded, so the accessTokens.json the CLI actually writes is covered.
The fix I wrote first was the wrong shape
Worth recording, because the better-looking change was the wrong one for this PR. Main already solves this exact problem on the path-tracking side with _SHELL_ASSIGN_RE = ...([A-Za-z_][A-Za-z0-9_]*)(\+?)=(.*), carrying the append form as an optional group -- and its docstring describes the same failure mode. So the tidy fix was to give _LOCAL_ASSIGN_RE the same optional group.
That worked and all 1816 tests passed, but the numstat told on it: 13 deletions, every one of them group-renumbering churn across three call sites that had nothing to do with this bug (the alias resolver and the payload scan both read group(2) as the value). This PR has been insertions-only for 24 rounds, and widening its blast radius into unrelated passes to save a regex is a bad trade on a change that is already large and hard to land.
So the append branch REUSES _SHELL_ASSIGN_RE instead of redefining anything -- one spelling of the rule in the module, matched only where the assignment pattern rejects the token. Same behaviour, 0 deletions, nothing else touched.
Verification
1816 passed / 1 skipped in test_security.py, plus 619 passed in test_denied_commands_security.py -- run because the append branch reuses a pattern shared with the alias and kill-by-name passes, and a regex shared across passes cannot be verified from one file. 36/36 mutations killed, zero anchor misses, up from 33: three added here, two of which had to be re-anchored after the rework above, which is exactly the failure the anchor check exists to catch. Scope 1472 + 1780 insertions, 0 deletions, 2 files, 1 commit. flake8 / isort / mypy / black gate clean.
A false-positive guard is pinned alongside the fix: appends that never spell a traversal (F=he; F+=llo), an append spelling a DIFFERENT program (P=fin; P+=ger), and an append used as an argument rather than as the program word all still allow.
Nothing merged, approved or overridden.
|
Correcting my own note above, because following it would leave people stuck: the fix has landed on
Why my PR is still red, and why re-running cannot fix it. I re-ran the audit on #7199 at 11:30Z, five hours after that commit landed. It failed with the old message,
Practical consequence, which reverses half of what I wrote earlier: I said neither a re-run nor a rebase clears this. The re-run half is now doubly confirmed and I can name the mechanism. The other half was wrong — any new push clears it, because that recomputes the merge ref against So the remaining red on a PR that has not pushed since ~04:34Z is a stale artifact of a workflow that no longer exists in this form, not a live signal. Nobody needs to re-run anything; the next push retires it. I am leaving #7199's red in place rather than pushing a no-op commit to refresh CI. Thanks for the retry-and-budget work — worth noting your first suggestion turned out to be the right one for the electron-sized case, and the release-only move handles the large lockfile that a retry could not. |
The bash gate asks one question of every token: does this resolve to a fenced
path? A `find` traversal answers no by construction, because it factors the path
in two -- the directory in one argument, the leaf in another -- and produces the
path itself at runtime:
find ~/.kiro/crew -name '.env' -exec cat {} +
find ~ -name credentials | xargs cat
A new pass re-joins the halves. It judges only a traversal that DELIVERS its
matches -- an action primary, a pipe, a redirect, or a substitution that captures
the output -- and denies it where the fence supplies the missing half: the root is
a credential directory, a filter matches a basename the fence declares, or a
filter resolves to a file that is really sitting in a fenced store.
The inert primaries are an allow-list, so a primary nobody enumerated denies
rather than permits -- the polarity _TRUST_ROOT_READ_LISTERS already documents.
Nothing here knows the child command, which is why xargs needs no flag grammar of
its own.
WHICH TEXT to judge is its own question, and answering it by inspecting the
command's characters was wrong twice. Capture was read off the two opener
characters glued to the program word's token, so `cat $( find ... )` was allowed
where `cat $(find ... )` was denied -- the same read, one space apart. And a
`bash -c '<traversal>'` payload was never re-tokenized at all, so the traversal
was invisible, even though the argv floor already descends into payloads when
they carry a plain fenced path. Both are one defect: the pass judged the
command's text rather than the things the shell runs. `_find_traversal_views`
enumerates those instead -- the outer line, every substitution body, every nested
payload -- so capture stops being a spelling to detect and becomes a property of
how a view was DERIVED, and no depth of wrapping is a special case. It reuses the
module's own view idiom and its termination discipline: no depth cap, because
whatever number is chosen one more level defeats it; a view is a proper substring
of its parent and a visited set stops sibling wrappers re-walking the same text.
The same reframing settles the program word. A shell expands `f?nd` against the
filesystem before running it, which no substring test on the command text can
see, so the word is matched through the pattern machinery instead -- and a word
the wildcard bound refuses is read AS a match, so the bound widens the pass
rather than opening a hole in it. `-files0-from` is the mirror image on the other
operand: find reads its roots from a file or from stdin, so the command names no
root and a parse reading only operands defaulted to `.`. It is read as an
unknowable root, which lets the filter still decide. And a program word or root
held in a variable the SAME command assigns is resolved through
`_resolve_local_assignments`, which the plain-path passes already consumed -- so
`D=<fenced>; cat $D/.env` was denied while `D=<fenced>; find "$D" -type f` was
allowed. That asymmetry was again machinery this pass had not been wired into.
Resolving it also SEPARATES two cases an unresolved-root reading conflated: an
assigned root is judged as the real path, while a genuinely unassigned `$SRC`
stays unknowable and is left to the filter.
A filter is agent-supplied text and this gate is synchronous, so evaluating one is
a denial-of-service surface before it is a correctness question. A `-regex`
pattern is never compiled and never run; a glob is bounded instead, by collapsing
adjacent `.*` runs and capping what remains. A filter or root carrying an
expansion the command never assigned is read as opaque. All of these fail closed
by widening the traversal rather than dropping the filter.
Whether a traversal RESOLVES a credential path is decided from the NAME it asks
for, with no filesystem access. Two earlier revisions asked the store instead and
each way of asking was its own defect: a direct-child `os.path.exists` could not
see `~/.ssh/archive/id_rsa` while denying the same name at the top, and an
`os.listdir` per store to match a glob measured 110 listdir calls and 14.4ms on
one traversal, against 0 and 0.7ms for an ordinary fenced read. Those two point in
opposite directions -- probe deeper, stop enumerating -- so neither is reachable by
extending the probe. Deciding from the name removes the depth question and the
enumeration together, and drops a host-dependence that was never a feature, where
the same command was allowed or denied according to whether a store happened to
exist yet. It is strictly wider than a probe, which is the fail-closed direction,
and what bounds it is a conservative predicate listing known credential leaves and
suffixes, nothing pattern-like -- the companion to `DENIED_ROOT_PARTS`, which
answers about a directory rather than a name. It is private: it has one consumer,
and a public spelling would exist only for a migration this change does not do.
A curated list does mean an OMISSION allows, which is the one decision here whose
polarity is not "anything unenumerated denies"; the alternative -- classifying
pure stores and denying any filter over one -- denies `find ~ -name '*.py'`
whenever `.ssh` exists. Neither is dominant, so the trade is recorded, not settled. Every filter spelling of one read answers alike -- `-name id_rsa`, `-name 'id_*'`,
`-path '*/id_rsa'` and `-path '*/id_*'` -- an invariant an earlier round
established and which each revision here had to be re-checked against. A `-path`
pattern is matched WHOLE against a synthetic fenced path rather than by its leaf,
because its earlier segments constrain what it can name: a trailing wildcard
matches any leaf yet `*/node_modules/*` can never name a store entry.
A filter bounds the delivery only where find will run the delivery after it and only
where it holds, so POSITION and the boolean operators decide the bound rather than
mere presence. `find ~ -exec cat {} + -name '*.py'` cats every file under the home
directory, and a disjunction, a negation or a comma each detach the filter from the
delivery; all of them were read as if the filter narrowed it. Establishing which
filters really bind would need find's own precedence grammar, so the traversal is
read as UNFILTERED instead -- no grammar, and it fails closed. An explicit `-a`
binds exactly as the implicit one does, and a `-prune -o ... -print` idiom is
untouched because it only lists.
There is no cheap pre-filter on the pass. A substring test for the program name has
to survive every rewriting the shell performs between the text and the program it
runs, and the one that stood here modelled quote removal and globs but not
parameter expansion, so `F=fin; ${F}d <fenced>` returned before the pass ran --
while the pass it guarded resolves exactly that spelling. Any pre-filter cheap
enough to be worth having is one spelling away from being the hole, so the pass
runs unconditionally and decides from the resolved program word. Measured at
45-190 us against the 136-844 us the surrounding checks already spend.
A redirect placed BEFORE the operands is delivery, not a root. Redirect spellings
carrying an `&` (`&>`, `&>>`, `>&`) also match the control-operator break, which
ran first in the primaries loop while the roots loop ordered the two the other way
-- so the pass disagreed with itself about the same redirect. Ordering the tests
consistently covers the operator set rather than one spelling. The roots loop
collected `>` and its target as traversal roots, so `find >out <fenced> -type f`
was allowed while `find <fenced> -type f >out` denied -- the same write with the
redirect moved to the front. Nothing there is expanded; the tokens were simply
classified in the wrong role, which is why it is a parse-order defect rather than
another spelling of the computed-operand class.
The `-path` probe is matched in both separator spellings, because the pattern uses
find's forward slashes while the join uses the platform's -- on Windows the clause
was inert. The separator is a parameter rather than read from `os.sep`, so the
behaviour is reachable from a test on either platform: taken from the global it was
unfalsifiable off Windows, and a mutation deleting the second spelling survived
every local test.
The tests interpolate every temp path into a command as POSIX. A backslash is an
escape to `shlex`, so a Windows temp root lost every separator and the gate parsed
a directory the test had not created -- 27 tests on the Windows shard, red from the
first revision. `find /` is likewise a POSIX-only literal, and the absolute-root case is computed from
the ANCHOR of the home the fence is anchored on -- deriving it from the working
directory's drive aimed the traversal at one drive while the fence sat on another.
The widened readings for an unknowable root are gated on there being a filter left
to reject them, because widening both at once denied an ordinary
`find "$TMPDIR" -type f -delete` outright.
The residuals are named where someone changing this code will read them: a pattern
held in a variable, a shell loop, a root reached by a preceding `cd` rather than
named (the working-directory emulation this module deliberately does not rest
security on), a leaf fenced only by LOCATION whose own name carries no credential
signal (`known_hosts`), an unknowable root with no filter left to decide, and the
sibling traversal tools (`fd`, `locate`, `rg --files`, `du -a`, `grep -r`) filed as
their own issue. Two leaf modules carry their own copy of the credential-filename
list and both already import `DENIED_ROOT_PARTS` from here, so they can be pointed
at the new constant; that migration is left out deliberately, because it moves the
behaviour of two unrelated surfaces.
SCOPE. This pass recognises a traversal by its SPELLING, and that is the whole of what
it claims. An operand the shell COMPUTES is deliberately NOT covered:
$(printf find) ~/.kiro/crew -type f -exec cat {} +
Knowing that runs `find` means knowing what `printf` writes, which is not a property of
the command text. The same holds for a glob-bearing root (`~/.kir*/crew`), brace
expansion, a root supplied from outside the command line (`-files0-from -`), a genuinely
unassigned expansion, and a root reached by a preceding `cd`. Six review rounds measured
17 such spellings, and each one closed revealed others, because the set of programs whose
output is `find` is not enumerable by a pattern over the text. Issue #8074 carries the
argument and the fix -- invert the polarity, require literal operands, fail closed on
computed ones -- which is a behaviour change on a security gate with its own
false-positive surface to price, so it is not folded in here.
What this pass DOES resolve is what the text alone determines: a same-command assignment
(`F=find; $F`, and the split `F=fin; ${F}d`), a glob-expanded program word (`f?nd`), a
quote splice, a nested `-c` payload, and the body of a substitution -- each of which turns
a computed spelling back into a literal one.
The credential-name vocabulary is a documented boundary too, not a gap awaiting entries:
it is the one clause whose polarity lets an OMISSION allow, and a private key the user
named themselves (`github_work`) is outside any curated list. Both ways to invert it were
measured and both cost real false positives; the choice is tracked in #8074.
Fixes #7034
Grounds for the
|
|
/ai-review override gpt 258bea5: Both blocking findings are the computed-operand and name-vocabulary-polarity class that this PR formally excludes and #8074 tracks with an existence proof, and the second contradicts a false-positive withdrawal Opus required on the same code path; grounds recorded in full at issuecomment-5541252977. |
Human judgment recorded@chenmingwei23 marked the gpt AI finding as false positive, not applicable, or explicitly accepted for
This decision applies only to this commit. A new push requires a new judgment. |
1. What is the problem?
The bash gate asks one question of every token: does this resolve to a fenced path? A
findtraversal answers no by CONSTRUCTION. It factors the path in two -- the directory is one argument, the leaf is another -- and produces the path itself at runtime, so the fenced path appears nowhere in the command text.Measured on
main:That last line is the tell the issue points at:
~/.sshis fenced as a DIRECTORY, so the command names it directly and the existing matcher sees it. A leaf fenced by FILENAME is invisible, because the name appears only as a-nameargument -- syntactically unrelated to the directory beside it.Neighbourhood probing found the same hole with no
-nameat all (find ~/.kiro/crew -type f -exec cat {} +reads every keystone leaf), and through every other way a traversal hands a match onward:-execdir,-ok,-okdir,-delete,-fprint/-fls/-fprintf, a pipe into anyxargsspelling, a command or process substitution, a plain redirect.2. Why this issue matters to the user
The keystone floor exists so a prompt-injected agent cannot read or author its own authorizations.
.env,token_signing.key,.local_secretandsecurity_policy.jsonare all fenced by filename, so all of them are reachable this way, and~/.aws/credentialsis reachable too since.awsis fenced but the traversal starts at~.Unlike a race against a transient temp, this needs no timing and no guessing: the command is deterministic, the file is the permanent one, and the protected spelling and the unprotected one differ by moving the filename one argument to the left.
It was raised by the GPT review lane on #7024 and deliberately NOT fixed there, because a fix scoped to that PR's artifacts would have guarded the transient copy while leaving the permanent secret open to the identical command -- worse than the honest gap, because it reads as covered.
3. How our fix solves it
Re-join what
findfactored apart. A new pass (_check_find_traversal_reaches_fence) reads the traversal's roots and its name/path filters and asks whether the SET it names intersects the fence. Three bounds keep that from becoming an over-block, and the issue's own two candidate shapes are each rejected for the reason it gives:Delivery. A
findthat only LISTS is left alone, so the newly-denied set is exactly the traversals that hand a match to a command. The inert primaries are an ALLOW-list and anything else denies -- the polarity_TRUST_ROOT_READ_LISTERSalready documents, because enumerating the delivering primaries would fail OPEN on the one nobody thought of. This needs no knowledge of the CHILD command, which is the issue's objection to gating the sink:cat,base64, a script andxargswith any of-0/-I/-n/-Pare all the same to it, because the gate denies NAMING a fenced path whatever is then done with it. That is also why there is no executor list here at all.Framing. WHICH text to judge is its own question, and answering it by inspecting the command's characters was wrong twice. Capture was read off the two opener characters glued to the program word's token, so
cat $( find ... )was allowed wherecat $(find ... )was denied -- the same read, one space apart. And abash -c '<traversal>'payload was never re-tokenized, so the traversal was invisible even though the argv floor already descends into payloads carrying a plain fenced path. Both are one defect: the pass judged the command's TEXT rather than the things the shell runs._find_traversal_viewsenumerates those instead -- the outer line, every substitution body, every nested payload -- so capture stops being a spelling to detect and becomes a property of how a view was DERIVED, and no depth of wrapping is a special case. It reuses this module's own view idiom (_self_token_framesjoins exactly these two extractors) and its termination discipline: no depth cap, because whatever number is chosen one more level defeats it; a view is a proper substring of its parent, and a visited set stops sibling wrappers re-walking the same text.Certainty. The join is denied where the FENCE supplies the missing half, and left alone where the traversal merely might wander into one:
find ~/.kiro/crew -type f -exec cat {} +~/.kiro/crew/$Fshape_sensitive_under_unresolved_varalready deniesfind ~ -name '.env' -exec cat {} +*, which matches every entry under the rootfind ~ -name credentials -exec cat {} +.aws/.sshis fenced, so a request for one such name resolves a path rather than searchingThe third clause is the one that needed care, and it is why the issue's other shape -- treat any root that CONTAINS a fence as the signal, via
path_contains_sensitive-- is not what shipped:find ~names the home directory, which contains everything, so that reading refuses unqualified traversals outright. Instead the clause asks what the FILTER names._find_filter_names_a_credential_leaf(security.py:9168) compares the filter against a fixed vocabulary of leaf names that carry credentials wherever they sit --_CREDENTIAL_LEAF_NAMES, 16 entries atsecurity.py:7523, and_CREDENTIAL_LEAF_SUFFIXES, 7 entries atsecurity.py:7543, both private. It performs no filesystem access at all: no stat, no directory listing. A literal name is tested directly; a glob is matched against that same vocabulary (each leaf name, plus one synthetic bearer per suffix), which is what makes-name 'id_*'answer like-name id_rsaand-name '*.py'match nothing.Two earlier revisions asked the filesystem instead, and each way of asking was its own defect. An existence check joined onto the store could only ever see a store's DIRECT children, so a key one level down was read while the same name at the top denied. Matching a glob against a store's real entries cost 110 directory listings and 14.4ms on a single traversal, against 0 listings and 0.7ms for an ordinary fenced read, on a gate that is synchronous and in-process. Those two point in OPPOSITE directions -- probe deeper, stop enumerating -- so neither is reachable by extending the probe. Deciding from the name removes the depth question and the enumeration together, and drops a host-dependence that was never a feature, where the same command was allowed or denied according to whether a store happened to exist yet.
The cost is that the clause is strictly WIDER than a probe in one direction and NARROWER in another, and both are stated in the residual list below. Wider: a credential-looking name is refused whether or not that store exists on the machine. Narrower: a leaf whose own basename carries no credential signal is not reached from an ancestor root. What bounds the width is that the vocabulary is conservative -- known credential leaves and suffixes, nothing pattern-like -- because a broad rule reintroduces exactly the false positives it exists to avoid: the fenced tree includes OPERATIONAL directories that hold ordinary readable files (
~/.kirocrew/runholds transient sandbox*.pywrappers,~/.local/share/kiro-cliholdstui.js, several fenced directories hold a*.json). That is the entire difference between denyingfind ~ -name credentials -exec cat {} +and leavingfind ~ -type d -name __pycache__ -exec rm -rf {} +alone -- no credential name is asked for, so the clause says no.-path/-wholename/-ipathmatches the whole path, so the pattern carries the fenced segments itself; dropping its wildcard SEGMENTS leaves something the ordinary path gate can answer (*/.aws/credentialsreduces to.aws/credentials, while*/node_modules/*reduces tonode_modules).gfindis parsed alongsidefind: macOS installs GNU findutils under that name, and the two share the grammar this pass reads.A filter is agent-supplied text, and this gate is synchronous and in-process, so evaluating one is a denial-of-service surface before it is a correctness question. CPython's
rehas no timeout, and this module already records a watchdog-crossing hang from admitting a run into a pattern. Two rules bound it, and both fail CLOSED by widening the traversal rather than dropping the filter:-regex/-iregexpattern is never compiled and never run. Screening for catastrophic shapes was rejected rather than skipped: this module already argues that enumerating dangerous constructs cannot terminate against an untrusted string, and a hostile pattern author is exactly that case._redos_proneexists here but was written to catch accidents in the repository's own deny patterns, which is a far weaker claim than defending against a crafted one. The cost is an over-block on a rare flag, bounded by the root -- see section 5._glob_to_regexmaps*and a brace group to.*, so-name '{a}{a}{a}{a}{a}{a}{a}{a}{a}{a}{a}{a}{a}{a}b'compiled to fourteen adjacent.*and hung the gate outright -- measured as still running after 12 seconds, now 0.031s. Adjacent runs are collapsed, which is semantics-preserving (.*.*names exactly what.*names, so that pattern and*x40 both become a single.*and still match), and literal-separated runs, which cannot be collapsed, are capped at 8. Over the cap the matcher is refused, which the caller reads as opaque, so the bound can never become a bypass.What makes the cap sufficient rather than a guess is that the SUBJECTS are not agent input: they are fence basenames and credential-store entry names, all short. Only the pattern is hostile.
The change can only ADD denials. It is a new pass returning a reason or
None, appended after the existing ones; no existing pass is touched. The one deliberate over-trigger is on record as a test rather than left to be found:find ~ -name '*.json' -exec cat {} +is refused, because*.jsoncoverssecurity_policy.jsonandconfig.json, which the fence declares by name -- the glob carve-out is about names the fence does NOT declare, not about widening one it does.The spellings that reach the pass were found by running it rather than by reading it. The cheap
"find" not in commandbail-out was itself a bypass twice over:fi''ndreaches shlex asfind, so quote and escape characters are stripped first, and a shell expandsf?ndagainst the filesystem, which no substring test on the text can see -- so a glob metacharacter now defeats the bail too, and the program word is matched through the same bounded matcher the filters use (_find_program_word_names_find). A word the wildcard bound refuses is read AS a match, so the bound widens the pass rather than opening a hole in it. A process substitution arrives with the redirect glued to the program word (<(find); a control operator glued to the program word (ls|find,true;find,true&&find) arrived as ONE shlex token whose basename matched no program name, so the whole pass was skipped -- the gate defeated by deleting one space. A captured substitution's closing paren arrives glued to the pattern ($(find ~ -regex '.*/id_rsa$')), which is why the strip lives where patterns are READ rather than per-list at the call site. And the brace-group glob hung the gate outright.-files0-fromis the mirror image on the other operand: GNU find reads its roots from a file, or from stdin for-, so the command names no root at all and a parse reading only operands defaulted to.while the traversal walked a fenced root it never saw. It is read as an unknowable root -- the same treatment a root carrying an unassigned expansion gets -- so the filter still decides andfind -files0-from list.txt -name '*.o' -deleteis untouched. Closed by construction: it is the only way find takes a root that is not an operand.Delivery is likewise read from the invocation's own token span rather than the command line, because reading the whole line denied a listing whenever an unrelated LATER command carried a pipe (
find ~/.kiro/crew -type f; cat notes | less).Two shapes were fixed wider than the review that found them prescribed, both because the prescription would have left a sibling open: the store question replaced a literal-only probe that denied
-name id_rsawhile allowing-name 'id_*'for the identical read, and the wildcard bound covers the glob path, where the reproducible hang actually was, rather than only the-regexpath that was reported.What this covers, and what it deliberately does not. This pass recognises a traversal by its SPELLING, and that is the whole of the claim. An operand the shell COMPUTES is out of scope:
$(printf find) ~/.kiro/crew -type f -exec cat {} +is allowed, because knowing it runsfindmeans knowing whatprintfwrites, which is not a property of the command text. The same holds for a glob-bearing root (~/.kir*/crew), brace expansion (f{i,}nd), a root supplied from outside the command line (-files0-from -with no filter left to judge), a genuinely unassigned expansion, and a root reached by a precedingcdrather than named. Review measured 17 such spellings across six rounds, and closing one revealed others every time -- the set of programs whose output isfindis not enumerable by a pattern over the text, so a spelling-based recognizer does not terminate on this class. #8074 carries that argument and the fix (invert the polarity: require literal operands, fail closed on computed ones), which is a behaviour change on a security gate with its own false-positive surface to price and so is not folded in here. This is a descope, not a to-do list.What the pass DOES resolve is what the text alone determines, and those cases are tested: a same-command assignment (
F=find; $F, and the splitF=fin; ${F}d), a glob-expanded program word (f?nd), a quote splice (fi''nd), a nested-cpayload, and the body of a substitution -- each turns a computed spelling back into a literal one. A test pins BOTH sides of that line, so the description cannot drift from the behaviour.Two further boundaries, both properties of the code rather than oversights. First, the credential-name vocabulary is the one clause whose polarity lets an OMISSION allow:
find ~ -name known_hosts | xargs cat,-name 'pubring*'and-name trustdb.gpgare allowed because neither the vocabulary nor the fence's declared entry names cover them (measured), and so is a private key the user named themselves (find ~ -name github_work). No addition to the list closes the latter -- the name space is the user's -- so this is a boundary, and the choice between the two measured ways to invert it is tracked in #8074. A name the fence DECLARES is still caught by the clause above it, which is why-name configand-name config.jsondo deny (.kube/config,.docker/config.jsonare declared entries). Second, in the other direction, a credential-looking filter is denied whether or not the store exists on this machine:find ~ -name id_rsa -exec cat {} +is refused on a home with no.sshat all. That is the fail-closed direction and it is deliberate, but it is a behaviour change worth naming rather than discovering. Outside the pass entirely: any OTHER program that factors a root and a name the same way --fd/fdfind(-x/-X),locate/plocate,rg --files,du -a-- andgrep -r <dir>, which needs no second command at all because the reader IS the traversal. Filed as #7309. The block comment says all of this where someone changing the code will read it.4. What tests we did
TestFindTraversalReachesFenceintest/test_security.py, 197 cases:The five commands from the issue, each a read of a permanent secret. The fifth reaches
~/.aws/credentials, so it runs in a fake home with the store on disk rather than depending on the runner's own dotfiles.The carrier grammar, 19 forms:
-execwith;and+,-execdir,-ok,-okdir,-delete,-fprint,-fls,-fprintf,-exec sh -c, fourxargsspellings including-0and-I{}, awhile readloop, both substitution spellings, a process substitution, and a plain redirect. Plus an unknown primary, which must deny.Traversal spellings:
-L/-H/-P,-D tree,-O3, multiple roots,/usr/bin/find,fi''nd,$HOMEand${HOME}, depth flags, a parenthesised-ogroup, and afindafter&&.Zero false positives, 23 cases: project-rooted traversals for every delivery form, absolute non-home roots, home-rooted GLOB searches, the crew home's own non-secret subtrees (
workspace,workspace/memory,skills-- which agents read constantly), listing-only forms, and commands that are not traversals. All 23 pass onmaintoo, so none is a behaviour change.The boundary of the name clause, in both directions:
credentialsandid_rsadeny by vocabulary and*.pemby suffix, whilepackage.json,__pycache__,tsconfig.json, a*.pyglob and every listing form stay allowed. A separate test pins the narrow side as a named residual --known_hostsis fenced by WHERE it sits rather than by what it is called, so it is allowed from an ancestor root whilecat ~/.ssh/known_hostsandfind ~/.ssh -type f -exec cat {} +both still deny. Another pins the wide side: the same request is refused on a home with no store on disk. And one counts filesystem calls through the clause and asserts zero, because "it is fast now" is not a property and zero is.A 61-command benign corpus and a 400-command carrier cross-product (4 root spellings x 10 filters x 10 delivery forms): 0 false positives, 0 misses.
The three round-1 findings, in both directions: six filter spellings of one store read all deny while two regex spellings of an ordinary read stay allowed; five glued-operator prefixes plus the glued-pipe-after case deny; four sequencing shapes stay allowed with
| lessas the control that must deny.The pattern-evaluation bounds, in both directions: three adjacent-wildcard globs asserted to collapse to exactly one
.*and still match, three literal-separated ones asserted refused, six ordinary globs asserted still compiling, the regex-widens clause with its root-bounded counterpart, and a wall-clock assertion over four previously-hanging commands with a deliberately enormous margin (measured ~30ms, asserted under 10s, so it cannot flake on a loaded box while still catching a return to super-polynomial matching).Mutation-verified: with
security.pyreverted to main's version, 148 of the 197 cases fail -- direct evidence the bypasses were real -- while all 49 no-regression controls pass on both sides. Separately, each mechanism this revision adds was verified by breaking it on purpose: 15 targeted mutations, 15 killed (drop nested payloads from the view set; drop substitution bodies; make a body not carry capture; make a payload not inherit its wrapper's capture; revert the program word to an exact match; revert the bail-out to a substring test; make the wildcard cap fail OPEN; disable-files0-from; trust the defaulted.; always and never hypothesise; revert the store probe to literals; accept any store entry; widen the predicate to a substring rule; let the unreadable-storeOSErrorescape). One mutation SURVIVED on the first run and exposed a test that did not bite -- an all-wildcard program word collapses to a single.*and so never exercised the refusal path -- which is why that assertion now uses a literal-separated pattern the cap genuinely refuses. The individual review findings were verified the same way but by probe: each was reproduced as a measured ALLOW (or a measured false DENY, or a measured hang) before the change and flipped after, with the controls unmoved.Targeted runs, never the full suite:
test_security.py+test_governance_self_protection.py+test_denied_commands_security.py-- 1985 passed, 1 skipped.test_hooks.py+test_connections_tool_aliases.py+test_app_sources_write_protection.py-- 416 passed. Also green earlier:test_computer_use_api.py,test_computer_use_enable_state.py,test_config_loader.py,test_aws_consent.py,test_snapshot_redaction_optout_ceiling.py,test_mcp_cron_security.py. flake8, isort and mypy clean; the baselined black gate passes in scope, and the added code is black-clean even thoughsecurity.pyis baselined.5. Any other suggestions on the work
The computed-operand class is descoped to bash gate: a spelling-based find recognizer cannot close computed operands; invert the polarity #8074, and that is the honest shape of this change. Six review rounds on this PR kept surfacing new spellings of one class (6 -> 10 -> 13 -> 17 distinct spellings on the record). Every finding that turned out to be closable was the parser assigning a literal token to the wrong ROLE -- a leading redirect read as a root, a filter read as bounding a delivery it does not bound,
&>read as a control operator, a pre-filter that missed a spelling the pass could already resolve -- and each of those is fixed here. Every finding that stayed open needed a value the text does not carry. Continuing to patch spellings would have kept producing revisions without converging, so the class is now recorded with its existence proof rather than half-covered. bash gate: a spelling-based find recognizer cannot close computed operands; invert the polarity #8074 also carries the credential-name polarity decision, with the price of each option measured (the narrower option flips 12 of 12 ordinary literal-name searches to DENY).The sibling traversal tools are the honest remaining gap, and are now filed.
fd -x catis the same deterministic single-command form with a different grammar (positional regex pattern,-x/-Xexec),locate | xargshas no root operand at all, andgrep -r <dir>needs no sink because the reader and the traversal are one command. Bash gate misses a fenced path reached through fd, locate, rg --files or grep -r #7309 carries all of them, with the note that a producer allow-list is the wrong direction -- the module's comments already argue that enumerating programs fails open, so a half-done list would read as covered. The two reusable halves of this pass are already tool-independent, which is what that issue points at.A keystone publish artifact named literally from outside the crew home is not covered.
find ~ -name 'tmpAB12CD34.tmp' -exec cat {} +needs the mkstemp name, which is the timing race fix(security): fence a keystone leaf's atomic-write temp and lock sibling #7024 describes; rooted at the crew home it is denied by the first clause. Recorded rather than papered over.The
-regexover-block is the one deliberate cost of not evaluating agent patterns. A DELIVERING-regextraversal over a root that contains a fence is refused whatever the pattern says,find ~ -regex '.*[.]py$' -exec grep -l foo {} +included. It is bounded by the root --find ~/Repos -regex ... -exec wc -l {} +andfind /tmp -regex ... -deleteare untouched, and a listing is untouched wherever it is rooted -- and both directions are pinned by tests. If the flag turns out to be common enough for that to bite, the fix is a linear matcher for a reduced subset of the regex grammar, not a screen for dangerous shapes.The credential-filename predicate is deliberately conservative, and deliberately not yet shared. It lists known credential names and suffixes only; a broad rule (any name containing
secret) would reintroduce exactly the false positives it exists to avoid, and a test pins the three families that must stay allowed. Two leaf modules carry their own copy of this list --PROJECT_SECRET_NAMESin the design-tweak preview server and_EXCLUDE_NAMESin the cloud source packer -- and both already importDENIED_ROOT_PARTSfromsecurity.py, so they can be pointed at the new constant the same way. That migration is left out on purpose: it would move the behaviour of two unrelated surfaces (static-file serving, tarball packing) inside a security fix.An unreadable credential store is not a hole, and the reason is simpler than it was. Nothing on this revision lists a store, so a directory the process cannot read cannot reach the verdict at all -- there is no fallback to get wrong. An earlier revision needed an explicit
OSErrorguard; the test that injectsPermissionErroris kept because a future revision reaching for the filesystem again would need it back.The depth flags are read but not modelled.
-maxdepth 1narrows what a traversal visits, and this pass ignores that, so it over-approximates the visited set. That is the fail-closed direction and cheap; modelling depth would only ever remove denials.Pattern harvest
Rule candidate: review-prompt
Pattern: a path-string fence judged only against the tokens a command NAMES, while the shell can compute the path at runtime.
This is the third member of one class, and all three were found the same way -- probe the gate with a command that reaches the fenced file without spelling it.
cd ~/.kiro/crew && cat .envremoves the ANCHOR (closed by the working-directory tracker); the Windowscdspelling of that same removal is the sibling comment on #7034 and is still open;find -execremoves the path ENTIRELY. Each arrived as its own bug report. The shared property is that the gate's question -- "does a token resolve to a fenced path?" -- is not the shell's question, which is "what file will be opened?".So the harvest is a check to run when a fence is added or widened: ask which commands can reach the fenced path WITHOUT naming it -- by moving the anchor (
cd,pushd,Set-Location), by generating the leaf (find -name, a glob, a variable), or by handing a produced path to a second program (xargs, a substitution, awhile readloop). The three known members are all one of those three moves.Deliberately NOT a semgrep or lint rule: the defect is a missing case in a semantic model, not a code shape a matcher can see -- there is no syntax common to
cd,-execand$Fto match on. The module already documents the class in three places (bare_protected_path's comment,_sensitive_under_unresolved_var, and now this pass), which is itself the evidence that what was missing is the prompt-level question rather than a pattern.Fixes #7034