SECURITY: close RIPGREP_CONFIG_PATH RCE (#32) + ship Security dashboard subcommand - #33
Merged
Merged
Conversation
#32) Two changes, one PR — both surfaced by tonight's adversarial security review of the new grep passthrough. ## P1 RCE fix (issue #32) Confirmed end-to-end this evening: `RIPGREP_CONFIG_PATH=/path/to/conf contextcrawler grep needle file.txt` → arbitrary code execution via rg's `--pre <script>` per-file preprocessor mechanism. **No CLI args required** — the attack vector is env-only. Reachable through BOTH the canonical grep filter (`src/cmds/system/grep_cmd.rs`) and the new format-flag passthrough (`src/main.rs::run_grep_format_passthrough`). Empirically verified all 4 attack paths (env-via-canonical, env-via-bypass, CLI-via-canonical, control direct-rg) fired the preprocessor pre-fix, and the first 3 are blocked post-fix. Two-layer defense in `src/core/utils.rs`: 1. **`secure_rg_command(name)`** — wraps `resolved_command()` with `env_remove("RIPGREP_CONFIG_PATH")` + `env_remove("RIPGREP_CONFIG_FILE")`. Strips the hijack vector from the inherited env before spawn. Both call sites that invoke rg/grep migrated to it. 2. **`check_forbidden_rg_args(&[…])`** — rejects `--pre`, `--pre-glob`, `--search-zip`, `-z` (long-form and `=value` form). Returns an `Err` with a user-facing message pointing at the escape hatch `contextcrawler proxy rg ...`. Both call sites gate on this before spawning. 6 unit tests cover the deny-list (each short/long form, both `=value` and space-separated, normal args still pass, error message mentions escape hatch + issue ref). ## `contextcrawler security` dashboard The CONTEXTCRAWLER.md template has long advertised `contextcrawler security` as "Tirith defense-in-depth gate (if installed)" — but no such subcommand existed. The invocation fell through `run_fallback` to macOS's `/usr/bin/security` (keychain tool). Users had no documented way to inspect Tirith status or see recent gate decisions. `src/hooks/tirith_gate.rs::run_security_dashboard()` ships the actual feature. Two modes: - **Default (human-readable)**: tirith binary path resolution, gate enabled/disabled state (with `CONTEXTCRAWLER_TIRITH_DISABLED` / `_REQUIRED` env-var awareness), downgrade log path + existence, and the last 10 downgrade events. - **`--json`**: machine-readable, suitable for piping into jq. The dashboard also includes a scope note: "tirith inspects COMMAND STRINGS only — env-var-driven attacks (e.g. RIPGREP_CONFIG_PATH hijack, see issue #32) are blocked separately inside the spawning code path via env_remove + arg deny-list." Makes the layered defense visible to operators. `Commands::Security { all, json }` added to the enum + dispatcher. `security` added to `RTK_META_COMMANDS` so flag-only invocations show the proper clap error instead of falling through to `/usr/bin/security`. ## Verified - `cargo test --bin contextcrawler` — 2033 pass, 0 fail (+6 new `secure_rg_tests`) - Empirical PoC re-run post-fix: all 3 in-binary attack paths blocked (env-canonical, env-bypass, CLI-canonical); direct rg control case still pwns (by design — that's rg, not us) - `contextcrawler security` produces real dashboard (your existing Tirith blocks for homograph URLs + private-network SSRF surface correctly from the downgrades.jsonl) Closes #32 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-Authored-By: Background security-review agent (codex+claude) <noreply@anthropic.com>
Three findings raised by codex review on the initial draft, all valid:
1. **[P1] Bundle bypass of `-z`.** The deny-list only compared whole
arg strings, so `-cz` / `-rzL` / `-Lzn` slipped through and reached
rg, where `z` enables --search-zip — the exact thing the deny-list
was meant to block. Same regression family as the bypass we just
shipped the original fix for; same risk profile too (RCE-adjacent
for archives with traversal).
Fix: extend `check_forbidden_rg_args` to detect short-flag bundles
(single `-`, all-alphabetic body) and reject any bundle containing
`z`. Mirrors the bundle-detection logic in main.rs's
`grep_format_flag_present` so the scanner agrees with how rg/grep
actually parses short-flag groups.
New regression test `rejects_z_inside_bundled_short_flags` covers
`-cz`, `-rz`, `-rzL`, `-Lzn`. Empirical: `-cz` now exits with the
deny error, the preprocessor doesn't fire.
2. **[P2] Line-oriented downgrade-log parsing breaks on multi-line
records.** The dashboard split the JSONL file with `content.lines()`.
Today's `log_downgrade` writes single-line JSON so this works, but
any future change to pretty-print would silently corrupt the
`--json` dashboard output for those records.
Fix: `read_recent_downgrades` now does brace-balanced + string-aware
scanning to find each top-level `{...}` region, validates by
round-tripping through `serde_json`, and re-serialises to canonical
compact form. Multi-line / pretty-printed records get coalesced
into single logical entries before they reach the dashboard.
3. **[P3] Hand-rolled JSON escaping for the `--json` envelope.** The
previous `json_escape_inner` only handled `\` and `"`, so a path
containing tab / newline / control chars would emit invalid JSON.
Fix: build the entire envelope as `serde_json::json!({...})` and
render with `to_string_pretty`. serde_json handles all escaping
correctly. Removed the hand-rolled helper.
Verified empirically:
- `-cz` bundle → deny error, no exec
- `-cn` (legitimate count + line-numbers) → still works
- `contextcrawler security --json` output parses cleanly via Python's
`json.load` with all 10 records intact
Tests: 2034 bin pass (+7 secure_rg_tests incl. the bundle case).
Co-Authored-By: Codex review <noreply@openai.com>
This was referenced May 18, 2026
Closed
Merged
noogalabs
pushed a commit
to noogalabs/contextcrawler
that referenced
this pull request
Jun 4, 2026
…hehoff#39) Ships the generic counterpart to secure_rg_command (PR thehoff#33) so future per-tool wrapped CLIs can opt in via a declarative ToolPolicy instead of copy-pasting the env-strip + arg-deny scaffolding. - UNIVERSAL_ENV_STRIP covers loader hijacks (LD_PRELOAD, DYLD_*), pager/editor (EDITOR, PAGER, LESS, ...), per-lang loader injection (PERL5OPT, LUA_INIT, ...), and shell metaprogramming (BASH_ENV, PROMPT_COMMAND, IFS, ...). Dynamic BASH_FUNC_* and DYLD_* are stripped by walking std::env::vars(). - ToolPolicy struct + secure_command_with_policy() and check_args_with_policy() mirror the rg helpers' semantics, including the codex-P1 short-bundle deny that catches '-cz'-style bypasses. - New docs/security/zero-trust-wrapped-cli.md documents the threat model (LLM mistakes + hostile inherited env, not local-shell attacker), the defense-in-depth layers, and the new-tool checklist. - 6 new unit tests in policy_registry_tests; full bin suite still 2040 passing. This PR deliberately does NOT refactor secure_rg_command or the per-tool helpers from in-flight PRs thehoff#34-thehoff#38 to use the new primitive yet -- that refactor is a follow-up so this lands cleanly while those PRs are also in flight. Closes thehoff#39 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
noogalabs
pushed a commit
to noogalabs/contextcrawler
that referenced
this pull request
Jun 4, 2026
Follow-up to PR thehoff#33's rg/grep hardening — close the equivalent class of env-var-driven and arg-driven RCE vectors against the `git` wrapper. ## What's added Two new helpers in `src/core/utils.rs`, mirroring `secure_rg_command` / `check_forbidden_rg_args` exactly in shape and style: 1. **`secure_git_command()`** — wraps `resolved_command("git")` with `env_remove` for every git env var that lets an attacker steer the child into exec'ing an arbitrary program: - exec sinks: GIT_EXTERNAL_DIFF, GIT_SSH, GIT_SSH_COMMAND, GIT_PROXY_COMMAND, GIT_PAGER, GIT_EDITOR, GIT_SEQUENCE_EDITOR, GIT_ASKPASS, SSH_ASKPASS - config-file overrides: GIT_CONFIG, GIT_CONFIG_GLOBAL, GIT_CONFIG_SYSTEM - env-driven `-c key=val` injection: GIT_CONFIG_COUNT plus GIT_CONFIG_KEY_0..63 / GIT_CONFIG_VALUE_0..63 (covers GIT_CONFIG_COUNT values up to 64) - path/helper redirection: GIT_TEMPLATE_DIR, GIT_EXEC_PATH, GIT_HOOKS_PATH 2. **`check_forbidden_git_args(&[…])`** — rejects: - `--upload-pack` / `--receive-pack` / `--exec-path` in both `--flag value` and `--flag=value` shapes - `-c key=val` (and the rarer `-c=key=val` single-arg shape) where the key matches any of these case-insensitive prefixes: `diff.external`, `core.editor`, `core.pager`, `core.sshCommand`, `core.fsmonitor`, `core.gitProxy`, `core.hooksPath`, `protocol.`, `uploadpack.packObjectsHook`, `safe.directory` Error message format matches `rg_deny_message` — names the offending flag, references issue thehoff#35, points at the escape hatch `contextcrawler proxy git ...`. ## Wire-in sites - `src/cmds/git/git.rs::git_cmd` — every git invocation through the filtered passthrough (the single spawn point that fans out to status / diff / log / show / commit / push / pull / branch / fetch / stash / worktree / add) now uses `secure_git_command`. - `src/main.rs::Commands::Git` — every `--config-override` (-c) entry passed via the CLI is validated through `check_forbidden_git_args` before being forwarded into git's global args. Rejection prints the deny message to stderr and returns exit code 2. - `src/hooks/permissions.rs::find_project_root` — the `git rev-parse --show-toplevel` fallback (runs inside Claude Code's PreToolUse hook) switched to `secure_git_command` for defense in depth. The `gh_cmd.rs`, `glab_cmd.rs`, `gt_cmd.rs`, and `diff_cmd.rs` files spawn `gh`/`glab`/`gt` — they do not invoke `git` directly, so no changes were needed there. ## Tests 13 new unit tests in `src/core/utils.rs::secure_git_tests`: - `secure_git_command_strips_all_listed_env_vars` introspects the Command via `get_envs()` and asserts every named var (plus the indexed `GIT_CONFIG_KEY_0..63`/`VALUE_0..63` pairs) has been removed - one test per denied flag class, covering both `--flag value` and `--flag=value` shapes - case-insensitive key match test for `core.sshCommand` vs `core.sshcommand` - benign-args allowlist test (`status`, `log -3 --oneline`, `diff HEAD~1`, `-c user.email=…`, `-c color.ui=always` all pass) - error-message format test (mentions escape hatch + issue ref) 6 new integration tests in `tests/git_hardening.rs`, each spawning the built binary against a real throwaway git repo with a marker-script attack armed. Critically, each env-var test first runs raw `git` with the same env to confirm the PoC actually fires (so a passing test proves the hardening, not just that git was busy with something else): - GIT_EXTERNAL_DIFF=<script> ./contextcrawler git diff - GIT_CONFIG_COUNT=1 GIT_CONFIG_KEY_0=diff.external GIT_CONFIG_VALUE_0=<script> ./contextcrawler git diff - GIT_CONFIG_GLOBAL=<evil.gitconfig> ./contextcrawler git diff - contextcrawler git -c diff.external=… diff exits non-zero with the deny message - contextcrawler git status / git log -3 --oneline still succeed All three env-var PoC sanity checks confirm the attack vector is real on this platform (raw git did exec the marker script), and all three post-hardening assertions confirm the wrapper neutralizes it. ## Empirical verification Pre-fix raw-git runs: GIT_EXTERNAL_DIFF=… git diff → marker touched (RCE) GIT_CONFIG_COUNT=1 KEY_0=… VALUE_0=… git diff → marker touched (RCE) GIT_CONFIG_GLOBAL=… git diff → marker touched (RCE) Post-fix contextcrawler runs, same env, same args: ./contextcrawler git diff (×3) → marker NOT touched Plus: ./contextcrawler git -c diff.external=… diff → exit 2 + deny error ## Test counts cargo test --bin contextcrawler: 2034 → 2047 (+13 unit tests) cargo test --test git_hardening: 0 → 6 (+6 integration tests) One pre-existing test (`test_git_cmd_c_locale_sets_stable_env`) was updated to skip env entries that are now intentionally removed via `env_remove` — it was unwrapping `Option<&OsStr>` with `expect` and new None entries (from the hardening's strip list) tripped it. The test still asserts the LC_ALL=C entry is present; coverage of the strip behaviour lives in the new `secure_git_command_strips_all_listed_env_vars` test. Closes thehoff#35 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Summary
Two changes, both surfaced by tonight's adversarial security review of the new grep passthrough.
1. P1 RCE fix (issue #32)
Confirmed end-to-end this evening:
RIPGREP_CONFIG_PATH=/path/to/conf contextcrawler grep needle file.txt→ arbitrary code execution via rg's--preper-file preprocessorTwo-layer defense in
src/core/utils.rs:secure_rg_command()— stripsRIPGREP_CONFIG_PATH/RIPGREP_CONFIG_FILEfrom inherited env before subprocess spawncheck_forbidden_rg_args()— rejects--pre,--pre-glob,--search-zip,-z(and bundled short forms like-czper codex P1 catch). Helpful error points at escape hatchcontextcrawler proxy rg ...2.
contextcrawler securitydashboardThe CONTEXTCRAWLER.md template has long advertised this subcommand, but no implementation existed — invocations fell through to macOS
/usr/bin/security. Now shipped:--alland--jsonmodessecure_rg_command(makes the layered defense visible to operators)Reviewed by Codex
-zvia-cz/-rzL→ fixed with short-letter bundle scannerserde_json::json!()envelopeVerified
cargo test --bin contextcrawler— 2034 pass, 0 fail (+7 newsecure_rg_tests)-czrejected; normal-cnstill works--jsondashboard parses cleanly viapython3 -m json.toolwith all records intactCloses #32