From ddc2b11508694bf922520f8091db89cc5a8cae5c Mon Sep 17 00:00:00 2001 From: Daniel Hoffman Date: Fri, 22 May 2026 09:51:23 +1000 Subject: [PATCH 1/3] docs(audit): end-to-end 4-agent review findings (2026-05-22) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third independent review pass — security core, command filters, analytics/discovery, runtime-data mining. 5 top-tier findings (2 P0 operational, 3 security CRITICAL), 13 important, plus data-grounded feature ideas. Measured blended savings 53.4%; 33 startup crashes in 7 days. Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/audits/2026-05-22-e2e-review.md | 60 ++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) create mode 100644 docs/audits/2026-05-22-e2e-review.md diff --git a/docs/audits/2026-05-22-e2e-review.md b/docs/audits/2026-05-22-e2e-review.md new file mode 100644 index 0000000000..b3cfb8c4c8 --- /dev/null +++ b/docs/audits/2026-05-22-e2e-review.md @@ -0,0 +1,60 @@ +# ContextCrawler — End-to-End Review (2026-05-22) + +Third independent review pass, four parallel agents by app scope: security core, +command filters, analytics/discovery/parser, and runtime-data mining. Prior +audit passes: #100 (Codex), #111 (Claude). Verdict: the security *plumbing* is +solid; the gaps are a soft centre in permission matching and one missed +ecosystem. The bigger story is operational — the binary crashes daily and +headline token savings are oversold. + +## Top tier — fix first + +| ID | Severity | Finding | +|---|---|---| +| P0-crash | operational | 33 SIGABRT crashes in 7 days, daily, on the current binary. ~30–100ms process lifetime ⇒ Rust panic in the startup path (an `unwrap()`/`expect()` against the no-unwrap rule). Symbolicate, reproduce with `RUST_BACKTRACE=1`, fix. Suspect: `core/config.rs`, `core/tracking.rs` (DB open), `hooks/` init. | +| P0-grep | operational | `grep` filter's clap parser rejects standard flags (`-q -r -E -i -c -A -B`) → 1,255 parse-failures, 62% of all 2,016. Delivered savings only 10.3% on 17.4M tokens — worst filter in the suite. | +| SEC-C1 | CRITICAL | `dotnet` has no argv deny-list checker on any of its 4 run paths (`dotnet_cmd.rs:34,78,119`). MSBuild `-p:CustomBefore*Targets=` / `CustomAfter*` and `dotnet test --runsettings ` are unguarded RCE flags — the same class #111 closed for `go -toolexec`. Fix: add `check_forbidden_dotnet_args`, wire into all 4 paths. | +| SEC-C2 | CRITICAL | Command substitution bypasses the permission/gate stack. `discover/lexer.rs` tokenises `` ` ``/`(`/`)` but `split_on_operators` never decomposes a `$(...)`, backtick, or `<(...)`/`>(...)` payload into its own segment, so `echo $(rm -rf /tmp/x)` evades a `Bash(rm -rf)` deny rule. Fix: lexer surfaces substitution payloads as first-class segments, or `check_command` fails closed on unparsed substitution. | +| SEC-C3 | CRITICAL | Permission rules are byte-prefix matched (`permissions.rs:224-251`). `git push --force` (double space), quoting, or `--force=` evade a deny rule. Fix: tokenise both command and pattern via `shell_split`, compare token sequences. | + +## Important + +| ID | Area | Finding | +|---|---|---| +| SEC-I1 | integrity | `integrity::runtime_check` only validates the legacy `rtk-rewrite.sh` hook; the modern `settings.json` binary-command registration has zero tamper detection. The 27KB SHA-256 subsystem protects a near-dead path. | +| SEC-I2 | supply-chain | No aggregate wall-clock budget or package-count cap on `supply_chain_gate::check`; 64MB-per-response cap. 20 packages × hostile registry ≈ 8-minute hook stall. | +| SEC-I3 | supply-chain | `parse_iso8601` Z-suffix fallback can misparse a near-RFC3339 string; a far-past misparse skips the cooldown silently. Reject >1-day-future timestamps; treat ambiguous parses as `Unavailable`. | +| SEC-I4 | tirith | `read_recent_downgrades` reads the whole `downgrades.jsonl` uncapped, O(n²) brace scan. Cap the read; split on newlines (the writer emits single-line JSON). | +| CMD-I1 | ruby | `rake` has no argv checker; `rake -f /tmp/evil.rake` runs arbitrary Ruby. rspec/rubocop siblings are covered. Add `check_forbidden_rake_args` (`-r`/`--require`, `-R`, `-I`, `-f`/`--rakefile`). | +| CMD-I2 | js | Node `--config`/`-c` (eslint/vitest/prettier) loads an executed JS config module — not in `NODE_FORBIDDEN_FLAGS_ALWAYS`. Consider a path-shape deny. | +| AN-I1 | analytics | `gain` "By Command" Avg% (`tracking.rs:796`) is still an unweighted mean of per-invocation percentages — inconsistent with the volume-weighted headline. | +| AN-I2 | analytics | `gain quota` divides lifetime saved tokens by a hardcoded *monthly* quota (`ESTIMATED_PRO_MONTHLY`) — mixed time horizons, can read >100%. | +| AN-I3 | discover | `output_len` uses byte length as a token proxy; multibyte output overstates ~3×. Use `chars().count()`. | +| AN-I4 | analytics | `ccusage` `--since 20250101` is a stale hardcoded literal (~16-month window and growing). Compute `now − 90d`. | +| READ-I1 | filters | `read` filter delivers 13.7% savings; 1,209 of 1,653 runs returned literal 0%; 15.5M-token sink — second-largest. Effectively not filtering. | +| GIT-I1 | filters | `git log` filter delivers 1.9% over 790 runs vs the 80% documented claim — not engaging on common flag variants. | +| PERF-I1 | core | `cleanup_old` runs `VACUUM` inline in the `record()` write hot path; rewrites the whole 6.7MB DB on the first prune of each day. Use incremental auto-vacuum or schedule it. | + +## Runtime data — measured reality + +- **Blended token savings: 53.4%** (61.4M saved / 74.3M input, 14,789 commands, window 2026-04-25 → 05-21). Below the advertised 60–90%. +- Delivering: `cargo` 66.5% (25.4M input — largest sink, working), `find` 73.9%, `ls` 72.2%. +- Underperforming: `read` 13.7%, `grep` 10.3%, `git` 32.2% (`git log` 1.9%), `curl` 2.6% (77/90 runs zero), `gh` 24.4%, `pytest` 13.2%. +- Supply-chain gate: 5,279 records, 99.5% no-op; logs 5,252 `skip` lines → 11.5MB JSONL noise. `RecentRelease` 3-day cooldown false-positives on mainstream packages (`requests`, `@types/node`); `UnvettableSource` false-positives on running an already-installed local plugin. `KnownVulnerability` blocks (pinned vulnerable `requests`/`urllib3`) are correct and valuable. +- Tirith gate: 48 downgrades, no clear false positives. The homoglyph catch is genuinely good — blocked a Cyrillic-`і` `github.com` lookalike, 5 rules, 24ms. Keep it. +- `err` wrapper writes 564 junk `parse_failures` rows (`cmd_ok2_*`, error `"err"`) — test DB likely leaking into the production DB. + +## Feature / uplift ideas + +- `gain --weak-filters` — rank tools by `input_tokens × (1 − savings)` ("where tokens leak"); `gain --failures` — render the ranked weakness map from `parse_failures`. Data is already collected. +- Missing filters with real volume: `ps` (1.7M tokens passthrough), `rsync`, `du`, `log show`. Route `rg` → existing grep filter (trivial). +- `learn` could auto-generate TOML `match_output`/`replace` filter suggestions from frequent correction rules — close the detection→DSL loop. +- Stop logging supply-chain `skip` records; cache `ccusage` (slow npm subprocess) with a TTL. +- Token-aware permission matching + substitution-aware lexer decomposition (SEC-C2/C3) close an entire evasion class. +- Integrity for the binary-command era — hash/verify the `settings.json` hook registration. + +## Method + +Four read-only agents, app-scoped, run in parallel; findings consolidated here. +No code changes were made by the review. Severity is the reviewer's assessment; +"CRITICAL" = exploitable or correctness-breaking, "P0" = operational urgency. From 2a4968dc220c979762fc9cd33ac3a01cfd8e2e7c Mon Sep 17 00:00:00 2001 From: Daniel Hoffman Date: Fri, 22 May 2026 11:46:12 +1000 Subject: [PATCH 2/3] =?UTF-8?q?fix(dotnet):=20add=20forbidden-arg=20checke?= =?UTF-8?q?r=20=E2=80=94=20block=20MSBuild=20Custom*Targets=20/=20runsetti?= =?UTF-8?q?ngs=20RCE=20(SEC-C1)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 4.7 (1M context) --- src/cmds/dotnet/dotnet_cmd.rs | 28 ++++- src/core/utils.rs | 223 ++++++++++++++++++++++++++++++++++ 2 files changed, 250 insertions(+), 1 deletion(-) diff --git a/src/cmds/dotnet/dotnet_cmd.rs b/src/cmds/dotnet/dotnet_cmd.rs index 5c87b3f0ab..ffe8053070 100644 --- a/src/cmds/dotnet/dotnet_cmd.rs +++ b/src/cmds/dotnet/dotnet_cmd.rs @@ -3,7 +3,7 @@ use crate::binlog; use crate::core::stream::exec_capture; use crate::core::tracking; -use crate::core::utils::{secure_dotnet_command, truncate}; +use crate::core::utils::{check_forbidden_dotnet_args, secure_dotnet_command, truncate}; use crate::dotnet_format_report; use crate::dotnet_trx; use anyhow::{Context, Result}; @@ -32,6 +32,15 @@ pub fn run_restore(args: &[String], verbose: u8) -> Result { } pub fn run_format(args: &[String], verbose: u8) -> Result { + // `secure_dotnet_command` strips dangerous *env vars*, but dotnet/MSBuild + // also exposes RCE-grade *CLI flags* (`-p:CustomBefore*Targets=`, + // `--runsettings`) that env-stripping does not touch. Reject those before + // any arg reaches the child process — fail closed (SEC-C1). + if let Err(msg) = check_forbidden_dotnet_args(args) { + eprintln!("{}", msg); + return Ok(2); + } + let timer = tracking::TimedExecution::start(); let (report_path, cleanup_report_path) = resolve_format_report_path(args); // `secure_dotnet_command` strips DOTNET_STARTUP_HOOKS / @@ -80,6 +89,15 @@ pub fn run_passthrough(args: &[OsString], verbose: u8) -> Result { anyhow::bail!("dotnet: no subcommand specified"); } + // Reject RCE-grade dotnet/MSBuild CLI flags (`-p:CustomBefore*Targets`, + // `--runsettings`) that `secure_dotnet_command`'s env-strip cannot defend. + // Fail closed before any arg reaches the child process (SEC-C1). + let arg_strs: Vec = args.iter().map(|a| a.to_string_lossy().into_owned()).collect(); + if let Err(msg) = check_forbidden_dotnet_args(&arg_strs) { + eprintln!("{}", msg); + return Ok(2); + } + let timer = tracking::TimedExecution::start(); let subcommand = args[0].to_string_lossy().to_string(); @@ -117,6 +135,14 @@ pub fn run_passthrough(args: &[OsString], verbose: u8) -> Result { } fn run_dotnet_with_binlog(subcommand: &str, args: &[String], verbose: u8) -> Result { + // Reject RCE-grade dotnet/MSBuild CLI flags (`-p:CustomBefore*Targets`, + // `--runsettings`) that `secure_dotnet_command`'s env-strip cannot defend. + // Fail closed before any arg reaches the child process (SEC-C1). + if let Err(msg) = check_forbidden_dotnet_args(args) { + eprintln!("{}", msg); + return Ok(2); + } + let timer = tracking::TimedExecution::start(); let binlog_path = build_binlog_path(subcommand); let should_expect_binlog = subcommand != "test" || has_binlog_arg(args); diff --git a/src/core/utils.rs b/src/core/utils.rs index 45dd49fe0e..35ebce988c 100644 --- a/src/core/utils.rs +++ b/src/core/utils.rs @@ -3302,6 +3302,112 @@ pub fn check_forbidden_pip_args>(args: &[S]) -> Result<(), String> Ok(()) } +/// MSBuild property *prefixes* that, when assigned a path, inject an +/// attacker-controlled `.targets` / `.props` file into the build. MSBuild +/// evaluates these as imports during every build, and a `` +/// task inside such a file is arbitrary code execution. The match is on a +/// lowercased property name and is a `starts_with`, so the whole family is +/// covered: `CustomBeforeMicrosoftCommonTargets`, +/// `CustomAfterMicrosoftCommonTargets`, +/// `CustomBeforeMicrosoftCommonProps`, `CustomAfterMicrosoftCommonProps`, +/// `CustomBeforeDirectoryBuildTargets`, `CustomAfterDirectoryBuildTargets`, +/// `CustomBeforeDirectoryBuildProps`, `CustomAfterDirectoryBuildProps`, +/// and any future `CustomBefore*` / `CustomAfter*` variant. MSBuild property +/// names are case-insensitive, so the comparison must be too. +const FORBIDDEN_MSBUILD_PROPERTY_PREFIXES: &[&str] = &["custombefore", "customafter"]; + +/// Flag spellings that introduce an MSBuild property assignment. dotnet / +/// MSBuild treat `-p:` and `/p:` as equivalent, and also accept the long +/// `--property:` / `-property:` forms. All carry `NAME=VALUE` glued to the +/// flag (`-p:Name=Value`). +const MSBUILD_PROPERTY_FLAG_PREFIXES: &[&str] = &["-p:", "/p:", "--property:", "-property:"]; + +/// `dotnet test` accepts a `.runsettings` file via `--runsettings` or the +/// `-s` short form. A `.runsettings` file can declare `TestAdaptersPaths` / +/// data collectors that load an attacker-controlled assembly into the test +/// host — RCE. This is a CLI flag, not an env var, so `secure_dotnet_command` +/// does NOT defend it. +const DOTNET_RUNSETTINGS_FLAGS_EXACT: &[&str] = &["--runsettings", "-s"]; + +/// Returns `Some(name)` with the property name if `arg` is an MSBuild +/// property assignment (`-p:NAME=VALUE`, `/p:NAME=VALUE`, +/// `--property:NAME=VALUE`, `-property:NAME=VALUE`). The name is the text +/// between the flag prefix and the first `=`, with surrounding whitespace +/// trimmed (MSBuild tolerates `-p: Name =Value`). +fn msbuild_property_name(arg: &str) -> Option<&str> { + for prefix in MSBUILD_PROPERTY_FLAG_PREFIXES { + if let Some(rest) = arg.strip_prefix(prefix) { + let name = rest.split('=').next().unwrap_or(rest).trim(); + return Some(name); + } + } + None +} + +/// dotnet / MSBuild expose RCE-grade *CLI flags* that env-stripping +/// (`secure_dotnet_command`, issue #36) does NOT touch: +/// +/// * `-p:CustomBeforeMicrosoftCommonTargets=` (and the `CustomAfter*`, +/// `*DirectoryBuildTargets`, and `*Props` variants) injects an attacker- +/// controlled `.targets` / `.props` file evaluated during the build; an +/// `` task in it is arbitrary code execution. The `/p:` +/// syntax is equivalent to `-p:`; `--property:` / `-property:` likewise. +/// * `dotnet test --runsettings ` / `-s ` loads a `.runsettings` +/// file that can declare `TestAdaptersPaths` / data collectors which load +/// an attacker assembly into the test host. +/// +/// This is the same class as #111 (`go -toolexec`) and #34 (`cargo +/// --config target.*.runner`). Legitimate properties such as +/// `-p:Configuration=Release` are NOT rejected — only the +/// `Custom(Before|After)*` family and runsettings. +pub fn check_forbidden_dotnet_args>(args: &[S]) -> Result<(), String> { + let mut i = 0; + while i < args.len() { + let raw = args[i].as_ref(); + + // MSBuild property assignment → reject only the Custom*Targets / + // Custom*Props family. The property name match is case-insensitive + // (MSBuild property names are case-insensitive). + if let Some(name) = msbuild_property_name(raw) { + let name_lower = name.to_ascii_lowercase(); + if FORBIDDEN_MSBUILD_PROPERTY_PREFIXES + .iter() + .any(|p| name_lower.starts_with(p)) + { + return Err(pyrbjvm_deny_message_with_issue( + "dotnet", + raw, + "MSBuild Custom(Before|After)* property imports an attacker \ + .targets/.props file evaluated during the build (Exec task = RCE)", + "#36", + )); + } + } + + // `--runsettings ` / `-s ` (separate-arg form) and the + // attached `--runsettings=` / `-s:` / `-s=` forms. + // A bare `-s` / `--runsettings` with no following value is harmless, + // but we reject it too: there is no benign use of it under + // contextcrawler, and the escape hatch is `contextcrawler proxy`. + let is_runsettings = DOTNET_RUNSETTINGS_FLAGS_EXACT.contains(&raw) + || raw.starts_with("--runsettings=") + || raw.starts_with("-s:") + || raw.starts_with("-s="); + if is_runsettings { + return Err(pyrbjvm_deny_message_with_issue( + "dotnet", + raw, + "--runsettings / -s loads a .runsettings file that can declare \ + TestAdaptersPaths / data collectors loading an attacker assembly", + "#36", + )); + } + + i += 1; + } + Ok(()) +} + #[cfg(test)] mod secure_pyrbjvmdotnet_tests { @@ -3725,6 +3831,123 @@ mod secure_pyrbjvmdotnet_tests { assert!(check_forbidden_pip_args(&["list", "--format=json"]).is_ok()); } + // ── dotnet deny (SEC-C1) ───────────────────────────────────────── + + #[test] + fn dotnet_rejects_custom_before_after_targets_props() { + // CustomBefore/After*Targets — the canonical MSBuild import RCE. + assert!(check_forbidden_dotnet_args(&[ + "build", + "-p:CustomBeforeMicrosoftCommonTargets=/tmp/evil.targets" + ]) + .is_err()); + assert!(check_forbidden_dotnet_args(&[ + "build", + "-p:CustomAfterMicrosoftCommonTargets=/tmp/evil.targets" + ]) + .is_err()); + assert!(check_forbidden_dotnet_args(&[ + "build", + "-p:CustomBeforeDirectoryBuildTargets=/tmp/evil.targets" + ]) + .is_err()); + assert!(check_forbidden_dotnet_args(&[ + "build", + "-p:CustomAfterDirectoryBuildTargets=/tmp/evil.targets" + ]) + .is_err()); + // The Custom*Props variants. + assert!(check_forbidden_dotnet_args(&[ + "build", + "-p:CustomBeforeMicrosoftCommonProps=/tmp/evil.props" + ]) + .is_err()); + assert!(check_forbidden_dotnet_args(&[ + "build", + "-p:CustomAfterDirectoryBuildProps=/tmp/evil.props" + ]) + .is_err()); + } + + #[test] + fn dotnet_rejects_slash_p_and_long_property_forms() { + // `/p:` is equivalent to `-p:`. + assert!(check_forbidden_dotnet_args(&[ + "build", + "/p:CustomBeforeMicrosoftCommonTargets=/tmp/evil.targets" + ]) + .is_err()); + // `--property:` / `-property:` long forms. + assert!(check_forbidden_dotnet_args(&[ + "build", + "--property:CustomAfterMicrosoftCommonTargets=/tmp/evil.targets" + ]) + .is_err()); + assert!(check_forbidden_dotnet_args(&[ + "build", + "-property:CustomBeforeDirectoryBuildTargets=/tmp/evil.targets" + ]) + .is_err()); + } + + #[test] + fn dotnet_property_name_match_is_case_insensitive() { + // MSBuild property names are case-insensitive — a lowercased or + // mixed-case spelling must still be caught. + assert!(check_forbidden_dotnet_args(&[ + "build", + "-p:custombeforemicrosoftcommontargets=/tmp/evil.targets" + ]) + .is_err()); + assert!(check_forbidden_dotnet_args(&[ + "build", + "-p:CuStOmAfTeRmicrosoftcommontargets=/tmp/evil.targets" + ]) + .is_err()); + } + + #[test] + fn dotnet_rejects_runsettings() { + assert!( + check_forbidden_dotnet_args(&["test", "--runsettings", "/tmp/evil.runsettings"]) + .is_err() + ); + assert!( + check_forbidden_dotnet_args(&["test", "--runsettings=/tmp/evil.runsettings"]).is_err() + ); + assert!(check_forbidden_dotnet_args(&["test", "-s", "/tmp/evil.runsettings"]).is_err()); + assert!(check_forbidden_dotnet_args(&["test", "-s:/tmp/evil.runsettings"]).is_err()); + } + + #[test] + fn dotnet_allows_legitimate_properties_and_args() { + // Legitimate `-p:` / `/p:` properties must NOT be rejected. + assert!(check_forbidden_dotnet_args(&["build", "-p:Configuration=Release"]).is_ok()); + assert!(check_forbidden_dotnet_args(&["build", "/p:Configuration=Debug"]).is_ok()); + assert!(check_forbidden_dotnet_args(&[ + "build", + "-p:TreatWarningsAsErrors=true" + ]) + .is_ok()); + assert!(check_forbidden_dotnet_args(&["test", "--filter", "Category=Unit"]).is_ok()); + assert!(check_forbidden_dotnet_args(&["build"]).is_ok()); + assert!(check_forbidden_dotnet_args(&["build", "MyApp.csproj"]).is_ok()); + // A property that merely *contains* "custom" but is not the + // Custom(Before|After)* family is fine. + assert!(check_forbidden_dotnet_args(&["build", "-p:MyCustomProp=value"]).is_ok()); + } + + #[test] + fn dotnet_deny_message_mentions_escape_hatch() { + let e = check_forbidden_dotnet_args(&[ + "build", + "-p:CustomBeforeMicrosoftCommonTargets=/tmp/evil.targets", + ]) + .unwrap_err(); + assert!(e.contains("contextcrawler proxy dotnet")); + assert!(e.contains("-p:CustomBeforeMicrosoftCommonTargets")); + } + // ── error message ──────────────────────────────────────────────── #[test] From 4d19bd4b4edcdba4c4063994e2c08dcf909313ce Mon Sep 17 00:00:00 2001 From: Daniel Hoffman Date: Fri, 22 May 2026 11:55:13 +1000 Subject: [PATCH 3/3] fix(dotnet): close semicolon-batch + case-fold + @response-file checker bypasses --- src/core/utils.rs | 151 +++++++++++++++++++++++++++++++++++++++------- 1 file changed, 128 insertions(+), 23 deletions(-) diff --git a/src/core/utils.rs b/src/core/utils.rs index 35ebce988c..dbec453b94 100644 --- a/src/core/utils.rs +++ b/src/core/utils.rs @@ -3329,16 +3329,31 @@ const MSBUILD_PROPERTY_FLAG_PREFIXES: &[&str] = &["-p:", "/p:", "--property:", " /// does NOT defend it. const DOTNET_RUNSETTINGS_FLAGS_EXACT: &[&str] = &["--runsettings", "-s"]; -/// Returns `Some(name)` with the property name if `arg` is an MSBuild +/// Returns `Some(names)` with every property name if `arg` is an MSBuild /// property assignment (`-p:NAME=VALUE`, `/p:NAME=VALUE`, -/// `--property:NAME=VALUE`, `-property:NAME=VALUE`). The name is the text -/// between the flag prefix and the first `=`, with surrounding whitespace -/// trimmed (MSBuild tolerates `-p: Name =Value`). -fn msbuild_property_name(arg: &str) -> Option<&str> { +/// `--property:NAME=VALUE`, `-property:NAME=VALUE`). +/// +/// MSBuild accepts multiple properties batched into one flag, semicolon- +/// delimited (`-p:A=1;B=2`), so the remainder after the flag prefix is split +/// on `;` and the name of every `NAME=VALUE` pair is returned. Each name is +/// the text before its first `=`, with surrounding whitespace trimmed +/// (MSBuild tolerates `-p: Name =Value`). +/// +/// The flag-prefix match is case-insensitive: MSBuild compares switch names +/// case-insensitively, so `-P:`, `/P:`, `--PROPERTY:` are all valid. +fn msbuild_property_names(arg: &str) -> Option> { for prefix in MSBUILD_PROPERTY_FLAG_PREFIXES { - if let Some(rest) = arg.strip_prefix(prefix) { - let name = rest.split('=').next().unwrap_or(rest).trim(); - return Some(name); + // Case-insensitive prefix match: lowercase only the leading span of + // `arg` that is the same length as `prefix`, then compare. + if arg.len() >= prefix.len() + && arg[..prefix.len()].eq_ignore_ascii_case(prefix) + { + let rest = &arg[prefix.len()..]; + let names = rest + .split(';') + .map(|pair| pair.split('=').next().unwrap_or(pair).trim()) + .collect(); + return Some(names); } } None @@ -3365,22 +3380,41 @@ pub fn check_forbidden_dotnet_args>(args: &[S]) -> Result<(), Stri while i < args.len() { let raw = args[i].as_ref(); + // `@response-file` → MSBuild expands the file's contents into flags + // before parsing, so a `.rsp` containing `-p:CustomBefore...` would + // bypass this checker entirely. There is no benign response-file use + // under the contextcrawler wrapper; the escape hatch is + // `contextcrawler proxy dotnet`. + if raw.starts_with('@') { + return Err(pyrbjvm_deny_message_with_issue( + "dotnet", + raw, + "@response-file expands to arbitrary MSBuild flags bypassing \ + the arg checker", + "#36", + )); + } + // MSBuild property assignment → reject only the Custom*Targets / - // Custom*Props family. The property name match is case-insensitive - // (MSBuild property names are case-insensitive). - if let Some(name) = msbuild_property_name(raw) { - let name_lower = name.to_ascii_lowercase(); - if FORBIDDEN_MSBUILD_PROPERTY_PREFIXES - .iter() - .any(|p| name_lower.starts_with(p)) - { - return Err(pyrbjvm_deny_message_with_issue( - "dotnet", - raw, - "MSBuild Custom(Before|After)* property imports an attacker \ - .targets/.props file evaluated during the build (Exec task = RCE)", - "#36", - )); + // Custom*Props family. Both the flag-prefix match and the property + // name match are case-insensitive (MSBuild switch names and property + // names are both case-insensitive). Multiple properties batched into + // one flag (`-p:A=1;B=2`) are all checked, not just the first. + if let Some(names) = msbuild_property_names(raw) { + for name in names { + let name_lower = name.to_ascii_lowercase(); + if FORBIDDEN_MSBUILD_PROPERTY_PREFIXES + .iter() + .any(|p| name_lower.starts_with(p)) + { + return Err(pyrbjvm_deny_message_with_issue( + "dotnet", + raw, + "MSBuild Custom(Before|After)* property imports an attacker \ + .targets/.props file evaluated during the build (Exec task = RCE)", + "#36", + )); + } } } @@ -3937,6 +3971,77 @@ mod secure_pyrbjvmdotnet_tests { assert!(check_forbidden_dotnet_args(&["build", "-p:MyCustomProp=value"]).is_ok()); } + #[test] + fn dotnet_rejects_semicolon_batched_custom_property() { + // MSBuild accepts multiple properties in one `-p:` arg, semicolon- + // delimited. The forbidden property may be in ANY position, not just + // the first — every pair's name must be checked. + assert!(check_forbidden_dotnet_args(&[ + "build", + "-p:Configuration=Release;CustomBeforeMicrosoftCommonTargets=/tmp/evil.targets" + ]) + .is_err()); + assert!(check_forbidden_dotnet_args(&[ + "build", + "-p:A=1;B=2;CustomAfterMicrosoftCommonTargets=/x" + ]) + .is_err()); + // Forbidden property first, benign property after. + assert!(check_forbidden_dotnet_args(&[ + "build", + "-p:CustomBeforeMicrosoftCommonTargets=/x;OutputPath=bin" + ]) + .is_err()); + } + + #[test] + fn dotnet_flag_prefix_match_is_case_insensitive() { + // MSBuild compares switch names case-insensitively — `-P:`, `/P:`, + // `--PROPERTY:` are all valid and must not bypass the checker. + assert!(check_forbidden_dotnet_args(&[ + "build", + "-P:CustomBeforeMicrosoftCommonTargets=/tmp/evil.targets" + ]) + .is_err()); + assert!(check_forbidden_dotnet_args(&[ + "build", + "/P:CustomBeforeMicrosoftCommonTargets=/tmp/evil.targets" + ]) + .is_err()); + assert!(check_forbidden_dotnet_args(&[ + "build", + "--PROPERTY:CustomAfterMicrosoftCommonTargets=/tmp/evil.targets" + ]) + .is_err()); + } + + #[test] + fn dotnet_rejects_response_file() { + // `@file` is expanded by MSBuild into flags before parsing, so a + // `.rsp` file could smuggle `-p:CustomBefore...` past the checker. + let e = check_forbidden_dotnet_args(&["build", "@/tmp/evil.rsp"]).unwrap_err(); + assert!(e.contains("contextcrawler proxy dotnet")); + assert!(e.contains("@response-file")); + } + + #[test] + fn dotnet_rejects_whitespace_padded_property_name() { + // MSBuild tolerates `-p: Name =Value`; the trimmed name must still + // be matched against the forbidden family. + assert!(check_forbidden_dotnet_args(&[ + "build", + "-p: CustomBeforeMicrosoftCommonTargets =/tmp/evil.targets" + ]) + .is_err()); + } + + #[test] + fn dotnet_allows_semicolon_batched_benign_properties() { + // A semicolon-batched `-p:` with no Custom(Before|After)* member is + // legitimate and must NOT be rejected. + assert!(check_forbidden_dotnet_args(&["build", "-p:A=1;B=2"]).is_ok()); + } + #[test] fn dotnet_deny_message_mentions_escape_hatch() { let e = check_forbidden_dotnet_args(&[