diff --git a/Cargo.lock b/Cargo.lock index ed8564f3..67f83fe9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -50,7 +50,7 @@ checksum = "9330f8b2ff13f34540b44e946ef35111825727b38d33286ef986142615121801" [[package]] name = "clawband" -version = "3.22.1" +version = "3.22.3" dependencies = [ "libc", "regex", diff --git a/Cargo.toml b/Cargo.toml index 3ba96063..8e2ff41e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "clawband" -version = "3.22.1" +version = "3.22.3" edition = "2021" description = "Claude Code PreToolUse hook that guards against destructive shell commands and unsafe file-write content" diff --git a/src/main.rs b/src/main.rs index bc4e4b16..3e6f89ea 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1115,6 +1115,71 @@ fn builtin_deny() -> Vec { // ─── Built-in ask patterns ──────────────────────────────────────────────────── +/// git argument injection — classic RCE vector: `--upload-pack=` / +/// `--receive-pack=` passed to git clone/fetch/push cause git to +/// invoke instead of the expected git-upload-pack/git-receive-pack +/// helper (the mechanism behind several historical git CVEs involving +/// attacker-controlled remote "paths"/URLs); `--exec-path=` is the +/// same class of risk — it changes where git looks for its own core +/// helper binaries, so pointing it at an attacker-writable directory can +/// substitute a trojanned binary for any subsequent git operation. +/// +/// Three independent patterns rather than one combined regex: `|` has +/// the lowest precedence of any regex operator, so `A|B|C` in a single +/// pattern is three fully independent top-level alternatives across the +/// whole string, not "A, followed by (B or C)" — a real-world bug +/// reported from a hand-rolled variant of this pattern that additionally +/// tried to scope it behind a `(-c|b)+` prefix: since Pattern::builtin() +/// wraps every pattern in `(?i)`, that `-c` branch case-insensitively +/// matched `-C` too (a completely unrelated git flag — "run as if git +/// was started in " vs `-c`'s "pass a config override"), causing +/// `git -C worktree list | grep verify` to false-positive. None +/// of the three real dangerous flags need any such prefix to be +/// dangerous, so it's dropped entirely here — `git ... --upload-pack` +/// is dangerous regardless of what other flags (if any) precede it. +/// +/// `[^|;&]*` (not `.*`) between `git` and the flag: `.*` is greedy and +/// unconstrained, so it can bridge across `|`/`;`/`&` and match a flag +/// that belongs to a completely different command later in the same +/// segment — e.g. `git log --oneline | grep --upload-pack` false- +/// positived under `.*` because split_segments() deliberately does not +/// split on bare `|` (pipe-to-interpreter detection elsewhere needs +/// the whole pipeline in one segment) and `check_command`/is_allowed +/// pipe-stage checks don't apply to ask/deny matching. Restricting the +/// filler to "not a compound/pipe separator" keeps the pattern scoped +/// to a single command while still matching arbitrary intervening git +/// flags (`git -c foo.bar=baz clone --upload-pack=x` still matches). +/// Found via third-party review of PR #307. +fn git_argument_injection_ask_patterns() -> &'static [(&'static str, &'static str)] { + &[ + ("git --upload-pack", r"\bgit\b[^|;&]*--upload-pack\b"), + // `--exec` is an undocumented synonym for `--receive-pack` on + // `git push` (see git-push(1)): `git push --exec=` runs + // instead of git-receive-pack, identical RCE shape to + // `--receive-pack=`. Folded into this pattern rather than a + // separate one since it's the exact same vulnerability class. + // + // No `\b` immediately before the alternation: the preceding filler + // usually ends right before a `-` (e.g. `push ` then `--exec`), and + // `-` is itself a non-word character, so a space-to-dash transition + // is NOT a `\b` boundary — a leading `\b` there silently prevented + // the whole pattern from ever matching. `--exec` requires a literal + // `=` (not just a trailing `\b`) so it doesn't also match the + // unrelated `--exec-path` flag (`--exec-path` is caught by its own + // dedicated pattern below). + ( + "git --receive-pack", + r"\bgit\b[^|;&]*(?:--receive-pack\b|--exec=)", + ), + // Bare `git --exec-path` (no `=`) only prints the current + // exec-path setting and exits — read-only, per git(1). Only the + // `--exec-path=` form actually changes where git looks for its + // core helper binaries, so the pattern requires the `=` to avoid + // asking about a harmless introspection command. + ("git --exec-path", r"\bgit\b[^|;&]*--exec-path="), + ] +} + fn builtin_ask() -> Vec { let specs: &[(&str, &str)] = &[ // eval — executes arbitrary strings; subshell-only idioms like @@ -1428,7 +1493,11 @@ fn builtin_ask() -> Vec { // Pattern: redirect (> or >>) followed immediately by $( or backtick. ("redirect to subshell path", r">\s*(?:\$\(|\x60)"), ]; - specs.iter().map(|(l, p)| Pattern::builtin(l, p)).collect() + specs + .iter() + .chain(git_argument_injection_ask_patterns()) + .map(|(l, p)| Pattern::builtin(l, p)) + .collect() } // ─── Built-in protected-ask patterns ───────────────────────────────────────── @@ -2315,7 +2384,28 @@ fn split_segments(cmd: &str) -> Vec { // the command into phantom segments (issue #108). let s = mask_quoted_separators(&s); - let splitter = Regex::new(r"[ \t]*(\|\||&&|;|\n)[ \t]*").unwrap(); + // Mask a bare `&` immediately preceding a `}` (optionally with whitespace + // between them) so treating bare `&` as a segment delimiter (added for + // the third-opinion PR #307 fix, see below) does not shred the fork-bomb + // deny pattern's structural match: the canonical fork bomb + // `:(){ :|:& };:` needs `{ :|:& }` to stay in one piece for + // `[\w:]+\(\)\s*\{[^}]*\|[^}]*[\w:]+\s*&` to see the whole `{...}&` shape. + // `cmd & }` (backgrounding the last command in a `{ ...; }` group) is the + // same shell construct either way, so not splitting there is also + // semantically reasonable, not just a narrow carve-out for this one + // pattern. + let mask_bg_before_brace = Regex::new(r"&(\s*\})").unwrap(); + let s = mask_bg_before_brace.replace_all(&s, "\x04$1"); + + // `&&` must be listed before the bare `&` alternative so the regex + // engine's leftmost-alternative-wins semantics consume both `&` + // characters as one `&&` delimiter rather than matching a single `&`, + // leaving a second `&` to be matched (and an empty segment produced) + // immediately after. Bare `&` (the shell background operator) is a + // delimiter in its own right: `git status & git clone --upload-pack=...` + // must not be treated as one unsplit segment (issue: `&` bypass of the + // per-segment is_allowed check, found via third-party review of PR #307). + let splitter = Regex::new(r"[ \t]*(\|\||&&|&|;|\n)[ \t]*").unwrap(); let s = splitter.replace_all(&s, SEP); s.split(SEP) @@ -2333,6 +2423,38 @@ fn split_segments(cmd: &str) -> Vec { .collect() } +/// Split a segment (as returned by `split_segments`) into pipe stages, for +/// the `is_allowed` check in `check_command` only — this is NOT used for +/// ask/deny matching, which intentionally sees the whole unsplit segment. +/// +/// A `|` inside single/double quotes, or escaped with a backslash, is not a +/// stage boundary. `||` has already been consumed by `split_segments` before +/// a segment ever reaches this function, so any bare `|` found here is a +/// genuine single pipe. +fn split_pipe_stages(segment: &str) -> Vec<&str> { + let bytes = segment.as_bytes(); + let mut stages = Vec::new(); + let mut start = 0; + let mut in_single = false; + let mut in_double = false; + let mut i = 0; + while i < bytes.len() { + match bytes[i] { + b'\'' if !in_double => in_single = !in_single, + b'"' if !in_single => in_double = !in_double, + b'\\' if i + 1 < bytes.len() => i += 1, + b'|' if !in_single && !in_double => { + stages.push(&segment[start..i]); + start = i + 1; + } + _ => {} + } + i += 1; + } + stages.push(&segment[start..]); + stages +} + // ─── Comment stripping (issue #128) ────────────────────────────────────────── // A `#` that appears outside of any quoted string and is preceded by whitespace // (or is at position 0) begins a shell comment. Everything from that `#` to the @@ -6536,6 +6658,39 @@ fn check_encoded_payload<'a>( scan_decoded_content(&decoded, deny_pats, ask_pats) } +/// A segment is "allowed" (suppresses the ASK tier only — DENY always fires) +/// when any of its forms matches an allow pattern. +/// +/// A segment containing a bare `|` needs a stricter check: `split_segments` +/// deliberately does NOT split on `|` (see its doc comment — pipe-to- +/// interpreter needs to stay in one segment for ask/deny matching), so a +/// pipeline like `git status | git clone --upload-pack=...` reaches this +/// point as a single segment. Matching the whole segment against an allow +/// pattern anchored only at `^` (e.g. "git read-only") lets that anchor +/// match the safe first stage and silently wave through whatever dangerous +/// command follows the pipe — found via second-opinion review on PR #307 +/// (2026-09-26): `git status | git clone --upload-pack=...` was reaching +/// ALLOW instead of the intended ASK. Requiring every pipe stage to +/// independently match an allow pattern closes this without touching how +/// `|` is handled anywhere else (ask/deny still match against the full, +/// unsplit segment). Known-safe wrapper pipes (RTK's `git -C ...`, +/// sqz's trailing `| sqz compress ...`, and the inline `| python3 -c "..."` / +/// `| node -m ...` forms) are all already stripped upstream +/// (`strip_rtk`/`strip_sqz`/`strip_safe_pipes`) before `check_command` ever +/// sees the command, so they never reach this branch. +fn segment_is_allowed(segment: &str, forms: &[&str], allow_pats: &[Pattern]) -> bool { + if segment.contains('|') { + split_pipe_stages(segment).iter().all(|stage| { + let stage = stage.trim(); + !stage.is_empty() && allow_pats.iter().any(|p| p.matches(stage)) + }) + } else { + forms + .iter() + .any(|f| allow_pats.iter().any(|p| p.matches(f))) + } +} + // ─── Core check logic ──────────────────────────────────────────────────────── // Returns Some(("deny"|"ask", reason)) or None for pass. // Does NOT perform script-file scanning (requires filesystem) or subshell checks. @@ -6583,10 +6738,8 @@ fn check_command<'a>( let forms: &[&str] = &forms_vec; // allow_pats suppress the ASK tier only — DENY tier always fires. - // A segment is "allowed" when any of its forms matches an allow pattern. - let is_allowed = forms - .iter() - .any(|f| allow_pats.iter().any(|p| p.matches(f))); + // See `segment_is_allowed`'s doc comment for the pipe-stage subtlety. + let is_allowed = segment_is_allowed(segment, forms, allow_pats); // ── Deny tier (always runs, allow cannot suppress) ──────────────────── @@ -8055,6 +8208,198 @@ mod tests { assert_eq!(decision("git branch --delete main"), None); } + // ── git argument injection (--upload-pack/--receive-pack/--exec-path) ────── + + #[test] + fn git_upload_pack_asks() { + assert_eq!( + decision("git clone --upload-pack='touch pwned' https://example.com/repo.git"), + Some("ask".into()) + ); + } + + #[test] + fn git_receive_pack_asks() { + assert_eq!( + decision("git push --receive-pack='touch pwned' origin main"), + Some("ask".into()) + ); + } + + #[test] + fn git_exec_path_asks() { + assert_eq!( + decision("git --exec-path=/tmp/evil status"), + Some("ask".into()) + ); + } + + #[test] + fn git_dash_capital_c_worktree_list_passes() { + // Regression: a hand-rolled variant of this pattern scoped the dangerous + // flags behind a `(-c|b)+` prefix, which — combined with the (?i) every + // built-in pattern gets wrapped in — case-insensitively matched `-C` + // (an unrelated "run as if started in " flag) too, false-positiving + // on this exact benign command. The fix drops that prefix entirely. + assert_eq!( + decision("git -C /some/path worktree list 2>&1 | grep verify"), + None + ); + } + + #[test] + fn git_dash_lowercase_c_config_passes() { + // `-c key=value` (git's own per-invocation config override) must also + // not be caught — neither -c nor -C have anything to do with the real + // danger here (the long-form --upload-pack/--receive-pack/--exec-path + // flags), which is exactly why the fix drops the prefix rather than + // trying to special-case -c vs -C. + assert_eq!(decision("git -c user.name=test status"), None); + } + + #[test] + fn git_upload_pack_pipe_bypass_asks() { + // Second-opinion review finding (PR #307, 2026-09-26): the pre-existing + // "git read-only" allow pattern (`^git\s+(log|diff|status|...)\b`, no `$` + // anchor) matched the start of the WHOLE piped segment — since + // split_segments() deliberately does not split on bare `|` — letting a + // read-only prefix wave through whatever dangerous command follows the + // pipe. `;`/`&&` correctly isolate the two halves; only `|` leaked. + assert_eq!( + decision("git status | git clone --upload-pack='touch pwned' https://evil.com/x.git"), + Some("ask".into()) + ); + } + + #[test] + fn git_receive_pack_pipe_bypass_asks() { + assert_eq!( + decision("git log | git push --receive-pack='touch pwned' origin main"), + Some("ask".into()) + ); + } + + #[test] + fn git_upload_pack_semicolon_still_asks() { + // Regression: confirms `;` chaining (unaffected by this fix) still + // correctly isolates and asks — only the `|` form was ever broken. + assert_eq!( + decision("git status ; git clone --upload-pack='touch pwned' https://evil.com/x.git"), + Some("ask".into()) + ); + } + + #[test] + fn git_log_pipe_grep_still_passes() { + // A genuinely benign pipeline (no ask pattern matches either stage) + // must not regress into an ask just because the stricter pipe-stage + // check no longer blanket-allows it. + assert_eq!(decision("git log | grep foo"), None); + } + + // ── third-opinion review of PR #307 (Gemini/Antigravity) ─────────────────── + + #[test] + fn bare_ampersand_background_is_a_segment_delimiter() { + // Finding #1 (highest priority, exploit repro included in the review): + // split_segments() split on `&&` but not bare `&` (the shell background + // operator). Combined with the per-segment `is_allowed` check, a segment + // like `git status & git clone --upload-pack=...` never got split, so + // the unanchored "git read-only" allow pattern matching the leading + // `git status` waved the whole unsplit segment through as allow — + // completely skipping the ask tier for the dangerous second half. + assert_eq!( + decision("git status & git clone --upload-pack='touch pwned' https://evil.com/x.git"), + Some("ask".into()) + ); + } + + #[test] + fn double_ampersand_still_splits_as_one_delimiter() { + // Regression guard for the #1 fix: adding a bare `&` alternative to the + // splitter regex must not break `&&` matching as a single two-char + // delimiter (e.g. producing a spurious empty segment between the two + // `&` characters). `git status && git log` must still behave exactly + // as it did before — both stages read-only, so overall pass (no ask). + assert_eq!(decision("git status && git log"), None); + } + + #[test] + fn segments_split_correctly_on_bare_ampersand() { + let segs = split_segments("echo one & echo two"); + assert_eq!(segs, vec!["echo one".to_string(), "echo two".to_string()]); + } + + #[test] + fn segments_split_correctly_on_double_ampersand_no_empty_segment() { + let segs = split_segments("echo one && echo two"); + assert_eq!(segs, vec!["echo one".to_string(), "echo two".to_string()]); + } + + #[test] + fn git_upload_pack_pipe_grep_false_positive_passes() { + // Finding #2: `.*` in the ask patterns is unconstrained and can bridge + // across a `|` inside a single pipeline segment (split_segments() + // deliberately does not split on bare `|`). `grep --upload-pack` here + // is an unrelated grep flag/arg, not a dangerous git invocation — must + // not ask. + assert_eq!(decision("git log --oneline | grep --upload-pack"), None); + assert_eq!(decision("git log --oneline | grep --receive-pack"), None); + assert_eq!(decision("git log --oneline | grep --exec-path"), None); + } + + #[test] + fn git_upload_pack_still_matches_with_intervening_flags() { + // Regression guard for #2: the fix (`.*` -> `[^|;&]*`) must not break + // the original PR #307 intent — arbitrary git flags between `git` and + // the dangerous long-form flag must still be matched. + assert_eq!( + decision("git -c foo.bar=baz clone --upload-pack=x https://evil.com/x.git"), + Some("ask".into()) + ); + assert_eq!( + decision("git -c foo.bar=baz push --receive-pack=x origin main"), + Some("ask".into()) + ); + } + + #[test] + fn git_exec_path_bare_form_passes() { + // Finding #3: bare `git --exec-path` (no `=`) only prints the + // current setting and exits (git(1)) — read-only, must not ask. + assert_eq!(decision("git --exec-path"), None); + assert_eq!(decision("git --exec-path status"), None); + } + + #[test] + fn git_exec_path_with_value_still_asks() { + // Regression guard for #3: `--exec-path=` actually changes where + // git looks for its core helper binaries and must still ask. + assert_eq!( + decision("git --exec-path=/tmp/evil status"), + Some("ask".into()) + ); + } + + #[test] + fn git_push_exec_synonym_asks() { + // Finding #4: `--exec=` is an undocumented synonym + // for `--receive-pack=` on `git push` (git-push(1)) — + // same RCE shape, previously uncaught entirely. + assert_eq!( + decision("git push --exec='touch pwned' origin main"), + Some("ask".into()) + ); + } + + #[test] + fn git_push_exec_synonym_does_not_false_positive_on_exec_path() { + // Regression guard: the `--exec=` pattern must not also match the + // unrelated `--exec-path` flag (which is handled, and correctly + // scoped, by its own dedicated pattern). + assert_eq!(decision("git --exec-path"), None); + } + #[test] fn docker_rm_force_asks() { assert_eq!(decision("docker rm -f mycontainer"), Some("ask".into())); diff --git a/tests/cli.rs b/tests/cli.rs index 40f306c3..a87f2b32 100644 --- a/tests/cli.rs +++ b/tests/cli.rs @@ -4296,6 +4296,210 @@ fn e2e_git_branch_delete_only_passes() { ); } +// ── git argument injection (--upload-pack/--receive-pack/--exec-path) ────────── + +#[test] +fn e2e_git_upload_pack_asks() { + let out = run( + &bash("git clone --upload-pack='touch pwned' https://example.com/repo.git"), + &[], + ); + assert_eq!( + decision(&out), + Some("ask"), + "git --upload-pack must trigger ask: {out}" + ); +} + +#[test] +fn e2e_git_receive_pack_asks() { + let out = run( + &bash("git push --receive-pack='touch pwned' origin main"), + &[], + ); + assert_eq!( + decision(&out), + Some("ask"), + "git --receive-pack must trigger ask: {out}" + ); +} + +#[test] +fn e2e_git_exec_path_asks() { + let out = run(&bash("git --exec-path=/tmp/evil status"), &[]); + assert_eq!( + decision(&out), + Some("ask"), + "git --exec-path must trigger ask: {out}" + ); +} + +#[test] +fn e2e_git_dash_capital_c_worktree_list_passes() { + // Regression: reported false-positive from a hand-rolled pattern that + // scoped these flags behind a `(-c|b)+` prefix, which case-insensitively + // (every builtin pattern is wrapped in (?i)) matched -C too. + let out = run( + &bash("git -C /some/path worktree list 2>&1 | grep verify"), + &[], + ); + assert_eq!( + decision(&out), + None, + "git -C ... worktree list must not be blocked: {out}" + ); +} + +#[test] +fn e2e_git_dash_lowercase_c_config_passes() { + let out = run(&bash("git -c user.name=test status"), &[]); + assert_eq!( + decision(&out), + None, + "git -c key=value config override must not be blocked: {out}" + ); +} + +#[test] +fn e2e_git_upload_pack_pipe_bypass_asks() { + // Second-opinion review finding (PR #307, 2026-09-26): a read-only prefix + // before a `|` waved through whatever dangerous command followed it, + // since split_segments() doesn't split on bare `|`. + let out = run( + &bash("git status | git clone --upload-pack='touch pwned' https://evil.com/x.git"), + &[], + ); + assert_eq!( + decision(&out), + Some("ask"), + "git status | git clone --upload-pack=... must still ask: {out}" + ); +} + +#[test] +fn e2e_git_receive_pack_pipe_bypass_asks() { + let out = run( + &bash("git log | git push --receive-pack='touch pwned' origin main"), + &[], + ); + assert_eq!( + decision(&out), + Some("ask"), + "git log | git push --receive-pack=... must still ask: {out}" + ); +} + +#[test] +fn e2e_git_log_pipe_grep_still_passes() { + // Explicitly "allow" (not silent/None) is correct here: this hits a + // separate, pre-existing full-command allow.patterns match (only reached + // when check_command's deny/ask scan found nothing) that isn't part of + // this fix — it's benign since it only ever activates once nothing + // dangerous was already found. The unit-level `git_log_pipe_grep_still_passes` + // test in main.rs exercises check_command directly and correctly sees None. + let out = run(&bash("git log | grep foo"), &[]); + assert_eq!( + decision(&out), + Some("allow"), + "a genuinely benign pipeline must not regress into ask: {out}" + ); +} + +// ── third-opinion review of PR #307 (Gemini/Antigravity) ─────────────────── + +#[test] +fn e2e_bare_ampersand_background_is_a_segment_delimiter() { + // Finding #1 (exploit repro from the review): a single `&` (background + // operator) was not a split_segments() delimiter, so the unanchored + // "git read-only" allow pattern matching the leading `git status` waved + // the whole unsplit segment through as allow, skipping the ask tier + // entirely for the dangerous `git clone --upload-pack=...` half. + let out = run( + &bash("git status & git clone --upload-pack='touch pwned' https://evil.com/x.git"), + &[], + ); + assert_eq!( + decision(&out), + Some("ask"), + "bare & must not bypass the ask tier: {out}" + ); +} + +#[test] +fn e2e_double_ampersand_still_splits_correctly() { + // Regression guard: adding bare `&` as a delimiter must not break `&&` + // matching as one two-char delimiter (no spurious empty segment). + let out = run(&bash("git status && git log"), &[]); + assert_eq!( + decision(&out), + Some("allow"), + "git status && git log must still pass through: {out}" + ); +} + +#[test] +fn e2e_git_upload_pack_pipe_grep_false_positive_passes() { + // Finding #2: unconstrained `.*` in the ask patterns bridged across `|`, + // so `grep --upload-pack` (an unrelated grep arg) false-positived. + let out = run(&bash("git log --oneline | grep --upload-pack"), &[]); + assert_ne!( + decision(&out), + Some("ask"), + "grep --upload-pack must not trigger the git argument-injection ask: {out}" + ); +} + +#[test] +fn e2e_git_upload_pack_still_matches_with_intervening_flags() { + // Regression guard for #2: arbitrary git flags between `git` and the + // dangerous long-form flag must still be matched after `.*` -> `[^|;&]*`. + let out = run( + &bash("git -c foo.bar=baz clone --upload-pack=x https://evil.com/x.git"), + &[], + ); + assert_eq!( + decision(&out), + Some("ask"), + "git --upload-pack must still ask with intervening flags: {out}" + ); +} + +#[test] +fn e2e_git_exec_path_bare_form_passes() { + // Finding #3: bare `git --exec-path` (no `=`) only prints the + // current setting and exits — read-only, must not ask. + let out = run(&bash("git --exec-path"), &[]); + assert_ne!( + decision(&out), + Some("ask"), + "bare git --exec-path (no value) must not ask: {out}" + ); +} + +#[test] +fn e2e_git_exec_path_with_value_still_asks() { + // Regression guard for #3: `--exec-path=` must still ask. + let out = run(&bash("git --exec-path=/tmp/evil status"), &[]); + assert_eq!( + decision(&out), + Some("ask"), + "git --exec-path= must still ask: {out}" + ); +} + +#[test] +fn e2e_git_push_exec_synonym_asks() { + // Finding #4: `--exec=` is an undocumented synonym for + // `--receive-pack=` on `git push` — same RCE shape, previously + // uncaught entirely. + let out = run(&bash("git push --exec='touch pwned' origin main"), &[]); + assert_eq!( + decision(&out), + Some("ask"), + "git push --exec= must ask like --receive-pack=: {out}" + ); +} + #[test] fn e2e_pnpm_dlx_asks() { let out = run(&bash("pnpm dlx create-react-app ."), &[]);