From 7447e678deba206126aa1aebd2bc7bada9f66ad6 Mon Sep 17 00:00:00 2001 From: yacosta738 <33158051+yacosta738@users.noreply.github.com> Date: Sat, 27 Jun 2026 17:28:52 +0000 Subject: [PATCH] fix(security): harden command validation and risk classification - Refactor argument processing to preserve case for flag validation - Block dangerous configuration flags in git and package managers - Improve subcommand detection to catch risk-carrying verbs after global flags - Prevent false positives on filenames by limiting subcommand scanning - Expand risk classification for git, cargo, and npm toolsets --- .github/workflows/dependabot-auto-merge.yml | 2 +- .github/workflows/semantic-pull-request.yml | 2 +- clients/agent-runtime/src/security/policy.rs | 85 ++++++++++++++------ 3 files changed, 64 insertions(+), 25 deletions(-) diff --git a/.github/workflows/dependabot-auto-merge.yml b/.github/workflows/dependabot-auto-merge.yml index 1c7b5216..396f3268 100644 --- a/.github/workflows/dependabot-auto-merge.yml +++ b/.github/workflows/dependabot-auto-merge.yml @@ -9,4 +9,4 @@ jobs: target: squash approve: true secrets: - token: ${{ secrets.GITHUB_TOKEN }} \ No newline at end of file + token: ${{ secrets.GITHUB_TOKEN }} diff --git a/.github/workflows/semantic-pull-request.yml b/.github/workflows/semantic-pull-request.yml index 8171578f..f5f2bbe6 100644 --- a/.github/workflows/semantic-pull-request.yml +++ b/.github/workflows/semantic-pull-request.yml @@ -4,4 +4,4 @@ on: types: [opened, edited, synchronize] jobs: main: - uses: dallay/common-actions/.github/workflows/semantic-pull-request.yml@v2.0.0 \ No newline at end of file + uses: dallay/common-actions/.github/workflows/semantic-pull-request.yml@v2.0.0 diff --git a/clients/agent-runtime/src/security/policy.rs b/clients/agent-runtime/src/security/policy.rs index 7a474a7f..e3a67886 100644 --- a/clients/agent-runtime/src/security/policy.rs +++ b/clients/agent-runtime/src/security/policy.rs @@ -358,7 +358,7 @@ impl SecurityPolicy { }; let base = base_raw.to_ascii_lowercase(); - let args: Vec = words.map(|w| w.to_ascii_lowercase()).collect(); + let args: Vec = words.map(|w| w.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 +522,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,23 +571,35 @@ 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 low = arg.to_ascii_lowercase(); + low == "-exec" || low.starts_with("-exec") || low == "-ok" || low.starts_with("-ok") + }) } "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.") - || arg == "-c" + // git config, alias, and -c can be used to set dangerous options + let low = arg.to_ascii_lowercase(); + low == "config" + || low.starts_with("config.") + || low == "alias" + || low.starts_with("alias.") + // Case-sensitive block for -c to allow -C + || arg.starts_with("-c") + || arg.starts_with("--exec-path") }) } - "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") + "npm" | "pnpm" | "yarn" | "cargo" => { + !args.iter().any(|arg| { + let low = arg.to_ascii_lowercase(); + low == "config" + || low == "set" + || low == "--config" + || low.starts_with("--config=") + // Case-sensitive block for -c + || arg.starts_with("-c") + || arg.starts_with("--exec-path") + }) } _ => true, } @@ -778,8 +786,16 @@ fn contains_high_risk_snippet(segment: &str) -> bool { } fn is_medium_risk_command(base: &str, args: &[String]) -> bool { + // Only check the first few non-flag arguments for subcommands to avoid false positives on filenames + let verbs: Vec = args + .iter() + .filter(|a| !a.starts_with('-')) + .take(2) + .map(|a| a.to_ascii_lowercase()) + .collect(); + match base { - "git" => args.first().is_some_and(|verb| { + "git" => verbs.iter().any(|verb| { matches!( verb.as_str(), "commit" @@ -794,9 +810,13 @@ fn is_medium_risk_command(base: &str, args: &[String]) -> bool { | "checkout" | "switch" | "tag" + | "clone" + | "pull" + | "fetch" + | "init" ) }), - "npm" | "pnpm" | "yarn" => args.first().is_some_and(|verb| { + "npm" | "pnpm" | "yarn" => verbs.iter().any(|verb| { matches!( verb.as_str(), "install" @@ -811,14 +831,33 @@ fn is_medium_risk_command(base: &str, args: &[String]) -> bool { | "t" | "it" | "cit" + | "ci" + | "init" + | "link" ) }), - "cargo" => args.first().is_some_and(|verb| { + "cargo" => verbs.iter().any(|verb| { matches!( verb.as_str(), - "add" | "remove" | "install" | "clean" | "publish" | "run" | "r" | "test" | "t" + "add" + | "remove" + | "install" + | "clean" + | "publish" + | "run" + | "r" + | "test" + | "t" + | "build" + | "b" + | "check" + | "c" + | "update" + | "init" + | "new" ) }), + "find" => args.iter().any(|arg| arg.to_ascii_lowercase() == "-delete"), "touch" | "mkdir" | "mv" | "cp" | "ln" => true, _ => false, }