fix(security): check bash operands verb-independently in the normalizer - #2603
Conversation
Design Review (Fable 5) — ✅ PASSAdvisory design-level review of Reviewing the diff, PR description, and the normalizer implementation is done; here is the design review. Design-Verdict: PASS Consolidating both surfaces onto the one normalizing checker is the right root-cause shape; residual gaps are honestly documented, spec and tests land in the same commit. Watch
[DESIGN-REVIEWED] 3e69af8 |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsThe diff is a security-hardening change to the bash-command sensitive-path normalizer. Let me verify the key behaviors against the code I've read. The change:
Verifications:
No correctness, security-regression, or lifecycle defect on the changed lines. The change strictly tightens the gate and matches its stated purpose. No findings. [OPUS-REVIEWED] 3e69af8 Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
GPT 5.6 Review — ✅ human override acceptedHuman judgment by @iamwhatever 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: |
78db92e to
eb13cd3
Compare
eb13cd3 to
a30ba6b
Compare
Remove the verb-allowlist gate from _check_sensitive_via_normalizer() so all path-like operands are checked regardless of verb, closing the write-side gap where dot-segment spellings bypassed normalization for non-read verbs. Additionally: - Split key=value operands (of=, --output=) and check the value as a path - Strip attached shell redirection operators (>~/path, >>~/path, 2>~/path, <~/path) that shlex keeps as a single token, exposing the path portion for checking; heredoc (<<) is excluded (delimiter word, not a path) 64 regression tests pin every spelling x write-shape combination plus the output and input redirection bypasses. Original work by Akash Vishwakarma (@joyboy5477). Closes #1638
a30ba6b to
3e69af8
Compare
|
/ai-review override gpt 3e69af8: GPT infra failure (review incomplete, no verdict produced). Both prior genuine findings (output redirection bypass, input redirection bypass) were fixed in this commit. Opus 4.8 passed clean. |
Human judgment recorded@iamwhatever 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. |
…er (kirodotdev#2603) Remove the verb-allowlist gate from _check_sensitive_via_normalizer() so all path-like operands are checked regardless of verb, closing the write-side gap where dot-segment spellings bypassed normalization for non-read verbs. Additionally: - Split key=value operands (of=, --output=) and check the value as a path - Strip attached shell redirection operators (>~/path, >>~/path, 2>~/path, <~/path) that shlex keeps as a single token, exposing the path portion for checking; heredoc (<<) is excluded (delimiter word, not a path) 64 regression tests pin every spelling x write-shape combination plus the output and input redirection bypasses. Original work by Akash Vishwakarma (@joyboy5477). Closes kirodotdev#1638 Co-authored-by: Akash Vishwakarma <vishwaka@usc.edu>
Cherry-pick of the fix from #2320 (by @joyboy5477) onto an org-owned branch, with an additional fix for attached-redirection bypass found by GPT 5.6 review.
Problem
_check_sensitive_via_normalizer()only ran for commands containing a recognized read verb, so write verbs with dot-segment spellings (./,../,//,$HOME) bypassed path normalization entirely. Additionally, shell redirections attached without a space (>~/path,>>~/path,2>~/path) were kept as a single token byshlex.split, making the path portion invisible to the checker.Why it matters
The normalizer is the only layer that can decide path equivalence. Gating it on read verbs left the keystone fence open for every write verb on a default install — e.g.
echo x > ~/.kiro/crew/./live_target.jsonwas unchecked (code execution in the gateway's identity), whilecat ~/.kiro/crew/./live_target.jsonwas blocked.Fix (symptoms → root cause → change)
has_relevant_verbearly-return gate; the verb allowlist is now used only to skip the command name itselfkey=valueoperand splitting —of=,--output=carry their path throughis_sensitive_path()_REDIR_PREFIX_REstrips leading>,>>,N>,N>>prefixes so the path portion is exposed for checkingTests
test_governance_self_protection.py+test_security.pyAttribution
Original work by @joyboy5477 in #2320. The redirection-stripping fix addresses the GPT 5.6 finding on the cherry-picked code.
Closes #1638