From 9d25b6f1e6e2e50100d226a1b7cc0afa61e46126 Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Thu, 4 Jun 2026 10:49:11 +0000 Subject: [PATCH] =?UTF-8?q?=F0=9F=9B=A1=EF=B8=8F=20Sentinel:=20[CRITICAL]?= =?UTF-8?q?=20Fix=20command=20injection=20bypass=20in=20validation=20comma?= =?UTF-8?q?nd=20allowlist?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 🚨 Severity: CRITICAL 💡 Vulnerability: Command allowlist bypass using string starts_with 🎯 Impact: Arbitrary command execution 🔧 Fix: Implemented strict parsing and exact argument matching ✅ Verification: Ran cargo test and validated against malicious commands Co-authored-by: mudcube <101564+mudcube@users.noreply.github.com> --- .jules/sentinel.md | 5 ++ .../services/filesystem/file_service/utils.rs | 51 +++++++++++++------ 2 files changed, 41 insertions(+), 15 deletions(-) diff --git a/.jules/sentinel.md b/.jules/sentinel.md index 798ec7de6..98fb5adbc 100644 --- a/.jules/sentinel.md +++ b/.jules/sentinel.md @@ -2,3 +2,8 @@ **Vulnerability:** The `OperationQueue` worker in `mill-server` executed file operations (create, write, delete, rename) using raw paths from the operation object without validating they were within the project root. **Learning:** Background workers that process serialized operations are a common bypass for security checks enforced at the API layer. The API layer might validate the request, but if the worker is "dumb" and blindly executes the queued operation, an internal attacker or a buggy component can exploit it. **Prevention:** Validation must happen at the *execution point* (in the worker), not just at the ingestion point. We introduced `validate_path` in the worker loop to enforce project root containment using `canonicalize` (handling non-existent files correctly). + +## 2025-06-04 - Argument Injection in Command Allowlists +**Vulnerability:** The `run_validation` command allowlist in `mill-services` verified if user-provided commands were safe by checking if the command string started with allowed prefixes using `.starts_with()`. This allowed bypassing the restriction by appending malicious strings without spaces (e.g., `cargo test-malicious`). +**Learning:** Using simple string operations like `.starts_with()` for validating commands against a prefix allowlist is insufficient and prone to argument injection bypasses. The underlying shell or command runner parses arguments based on spaces and quotes, not arbitrary character positions. +**Prevention:** Always parse the command string into its component parts (program and arguments) *before* validating against an allowlist. Ensure exact matching of the program name and each individual allowed argument. diff --git a/crates/mill-services/src/services/filesystem/file_service/utils.rs b/crates/mill-services/src/services/filesystem/file_service/utils.rs index daeefdeb7..1611a3e2d 100644 --- a/crates/mill-services/src/services/filesystem/file_service/utils.rs +++ b/crates/mill-services/src/services/filesystem/file_service/utils.rs @@ -56,21 +56,6 @@ impl FileService { "make check", ]; - let is_safe = safe_prefixes - .iter() - .any(|prefix| self.validation_config.command.trim().starts_with(prefix)); - - if !is_safe { - error!( - command = %self.validation_config.command, - "Validation command blocked by security policy. Command must start with a known safe prefix (e.g., 'cargo', 'npm', 'go', 'make')." - ); - return Some(json!({ - "validation_status": "error", - "validation_error": format!("Security Error: Command '{}' is not in the allowed list.", self.validation_config.command) - })); - } - // Run validation command in the project root // SECURITY: Parse command string to avoid shell injection let (program, args) = match parse_command_line(&self.validation_config.command) { @@ -83,6 +68,42 @@ impl FileService { } }; + // SECURITY: Validate the parsed command against the allowlist. + // We parse both the target command and the allowed prefixes, then ensure + // exact matching of the program name and each prefix argument. + // This prevents argument injection bypasses (e.g., "cargo test-malicious") + // that could occur if we merely checked if the command string started + // with the prefix using `.starts_with()`. + let is_safe = safe_prefixes.iter().any(|prefix| { + if let Some((safe_prog, safe_args)) = parse_command_line(prefix) { + if program != safe_prog { + return false; + } + if args.len() < safe_args.len() { + return false; + } + for (i, safe_arg) in safe_args.iter().enumerate() { + if args[i] != *safe_arg { + return false; + } + } + true + } else { + false + } + }); + + if !is_safe { + error!( + command = %self.validation_config.command, + "Validation command blocked by security policy. Command must start with a known safe prefix (e.g., 'cargo', 'npm', 'go', 'make')." + ); + return Some(json!({ + "validation_status": "error", + "validation_error": format!("Security Error: Command '{}' is not in the allowed list.", self.validation_config.command) + })); + } + let output = match Command::new(&program) .args(&args) .current_dir(&self.project_root)