From 9674c505e224aa4b35e4ae030054ddc8b38a0ff6 Mon Sep 17 00:00:00 2001 From: yacosta738 <33158051+yacosta738@users.noreply.github.com> Date: Sat, 16 May 2026 16:41:47 +0000 Subject: [PATCH] fix(security): harden SecurityPolicy against flag bypasses and improve risk classification - Refactored command validation to preserve argument case for accurate flag checking. - Blocked `find -execdir` and `-okdir` bypasses. - Hardened `git` and `cargo` against `--config` and `-c` injection. - Improved risk classification for `build`, `clone`, and `install` operations. - Added comprehensive unit tests for new security rules. --- .agents/journal/sentinnel-journal.md | 9 +++ clients/agent-runtime/src/security/policy.rs | 82 ++++++++++++++++---- 2 files changed, 74 insertions(+), 17 deletions(-) diff --git a/.agents/journal/sentinnel-journal.md b/.agents/journal/sentinnel-journal.md index 5e60cd94..879a3358 100644 --- a/.agents/journal/sentinnel-journal.md +++ b/.agents/journal/sentinnel-journal.md @@ -31,3 +31,12 @@ **Learning:** `SecurityPolicy` was vulnerable to path validation bypass using nested or partial quotes (e.g., `"/etc"/passwd`, `""/etc/passwd""`). The previous `strip_matching_quotes` only removed outer quotes, leaving internal quotes to disrupt prefix-based forbidden path and absolute path checks. **Action:** Implemented `strip_all_quotes` to normalize command arguments and paths by removing all single and double quotes. In `is_path_allowed`, `iterative_url_decode` is still applied before quote stripping so encoded quotes are exposed before later processing. + +## 2025-06-05 - Hardening SecurityPolicy against Flag-based Bypasses and Improving Risk Classification + +**Learning:** `SecurityPolicy` was vulnerable to bypasses using bundled flags (e.g., `git -cname=value`) and variants of dangerous commands (e.g., `find -execdir`). Also, some common operations like `cargo build` and `git clone` were classified as Low risk, which is inaccurate given they can execute arbitrary code (build scripts) or fetch external content. Normalizing all arguments to lowercase too early caused a loss of distinction between case-sensitive flags like `git -c` (dangerous) and `git -C` (safe path). +**Action:** +1. ✅ Refactored `command_risk_level` and `is_segment_valid` to pass preserved-case arguments to `is_args_safe` and `is_medium_risk_command`. +2. ✅ Updated `is_args_safe` to block `-execdir` and `-okdir` for `find`, and bundled `-c` flags for `git`. +3. ✅ Added `--config` to blocked flags for `git` and `cargo`. +4. ✅ Classified `cargo build/check`, `git clone/init`, `find -delete`, and `npm i/ci` as `Medium` risk. diff --git a/clients/agent-runtime/src/security/policy.rs b/clients/agent-runtime/src/security/policy.rs index 7a474a7f..b657a78e 100644 --- a/clients/agent-runtime/src/security/policy.rs +++ b/clients/agent-runtime/src/security/policy.rs @@ -358,7 +358,10 @@ impl SecurityPolicy { }; let base = base_raw.to_ascii_lowercase(); - let args: Vec = words.map(|w| w.to_ascii_lowercase()).collect(); + let raw_args: Vec<&str> = words.collect(); + let args = normalize_args_for_path_checks(&raw_args).unwrap_or_else(|| { + raw_args.iter().map(|s| s.to_string()).collect() + }); let joined_segment = cmd_part.to_ascii_lowercase(); if is_high_risk_base_command(&base) || contains_high_risk_snippet(&joined_segment) { @@ -522,16 +525,12 @@ impl SecurityPolicy { } let raw_args: Vec<&str> = words.collect(); - let normalized_args = match normalize_args_for_path_checks(&raw_args) { + let args = match normalize_args_for_path_checks(&raw_args) { Some(args) => args, None => return false, }; - let args: Vec = normalized_args - .iter() - .map(|arg| arg.to_ascii_lowercase()) - .collect(); - for arg in &normalized_args { + for arg in &args { let effective_arg = Self::effective_path_arg(arg); if !self.is_path_argument_safe(effective_arg) { return false; @@ -575,24 +574,37 @@ impl SecurityPolicy { match base.as_str() { "find" => { // find -exec and find -ok allow arbitrary command execution - !args.iter().any(|arg| arg == "-exec" || arg == "-ok") + !args.iter().any(|arg| { + let l = arg.to_ascii_lowercase(); + l == "-exec" || l == "-ok" || l == "-execdir" || l == "-okdir" + }) } "git" => { // git config, alias, and -c can be used to set dangerous options // (e.g. git config core.editor "rm -rf /") !args.iter().any(|arg| { - arg == "config" - || arg.starts_with("config.") - || arg == "alias" - || arg.starts_with("alias.") + let l = arg.to_ascii_lowercase(); + l == "config" + || l.starts_with("config.") + || l == "alias" + || l.starts_with("alias.") || arg == "-c" + || arg.starts_with("-c") + || l == "--config" + || l.starts_with("--config=") }) } "npm" | "pnpm" | "yarn" => { // npm config and set can be used to set dangerous options // (e.g. npm config set editor "rm -rf /") - !args.iter().any(|arg| arg == "config" || arg == "set") + !args.iter().any(|arg| { + let l = arg.to_ascii_lowercase(); + l == "config" || l == "c" || l == "set" || l == "get" + }) } + "cargo" => !args + .iter() + .any(|arg| arg.to_ascii_lowercase().starts_with("--config")), _ => true, } } @@ -779,9 +791,10 @@ fn contains_high_risk_snippet(segment: &str) -> bool { fn is_medium_risk_command(base: &str, args: &[String]) -> bool { match base { + "find" => args.iter().any(|arg| arg.to_ascii_lowercase() == "-delete"), "git" => args.first().is_some_and(|verb| { matches!( - verb.as_str(), + verb.to_ascii_lowercase().as_str(), "commit" | "push" | "reset" @@ -794,12 +807,16 @@ fn is_medium_risk_command(base: &str, args: &[String]) -> bool { | "checkout" | "switch" | "tag" + | "clone" + | "init" ) }), "npm" | "pnpm" | "yarn" => args.first().is_some_and(|verb| { matches!( - verb.as_str(), + verb.to_ascii_lowercase().as_str(), "install" + | "i" + | "ci" | "add" | "remove" | "uninstall" @@ -815,8 +832,20 @@ fn is_medium_risk_command(base: &str, args: &[String]) -> bool { }), "cargo" => args.first().is_some_and(|verb| { matches!( - verb.as_str(), - "add" | "remove" | "install" | "clean" | "publish" | "run" | "r" | "test" | "t" + verb.to_ascii_lowercase().as_str(), + "add" + | "remove" + | "install" + | "clean" + | "publish" + | "run" + | "r" + | "test" + | "t" + | "build" + | "b" + | "check" + | "c" ) }), "touch" | "mkdir" | "mv" | "cp" | "ln" => true, @@ -2287,4 +2316,23 @@ fn test_package_manager_risk_hardening() { assert_eq!(p.command_risk_level("cargo r"), CommandRiskLevel::Medium); assert_eq!(p.command_risk_level("cargo test"), CommandRiskLevel::Medium); assert_eq!(p.command_risk_level("cargo t"), CommandRiskLevel::Medium); + assert_eq!(p.command_risk_level("cargo build"), CommandRiskLevel::Medium); + assert_eq!(p.command_risk_level("cargo b"), CommandRiskLevel::Medium); + assert_eq!(p.command_risk_level("cargo check"), CommandRiskLevel::Medium); + assert_eq!(p.command_risk_level("cargo c"), CommandRiskLevel::Medium); + + // is_args_safe hardening additional + assert!(!p.is_command_allowed("find . -execdir ls {} +")); + assert!(!p.is_command_allowed("find . -okdir ls {} +")); + assert!(p.command_risk_level("find . -delete") == CommandRiskLevel::Medium); + + assert!(!p.is_command_allowed("git -c core.editor=malicious commit")); + assert!(!p.is_command_allowed("git --config core.editor=malicious commit")); + assert!(p.is_command_allowed("git -C subdir status")); + + assert!(!p.is_command_allowed("npm c set editor malicious")); + assert!(!p.is_command_allowed("npm get user")); + assert!(p.command_risk_level("npm i") == CommandRiskLevel::Medium); + + assert!(!p.is_command_allowed("cargo --config core.editor=malicious build")); }