From 78f87987dfce8737cd99714b0af36a990c69e304 Mon Sep 17 00:00:00 2001 From: viyatb-oai Date: Tue, 3 Feb 2026 13:34:04 -0800 Subject: [PATCH] fix(exec-server): prevent basename policy escalation for local binaries --- codex-rs/exec-server/src/posix.rs | 147 +++++++++++++++++- .../src/posix/mcp_escalation_policy.rs | 9 +- 2 files changed, 151 insertions(+), 5 deletions(-) diff --git a/codex-rs/exec-server/src/posix.rs b/codex-rs/exec-server/src/posix.rs index 7f9ce569c6..a50622ac6e 100644 --- a/codex-rs/exec-server/src/posix.rs +++ b/codex-rs/exec-server/src/posix.rs @@ -173,12 +173,16 @@ pub async fn main_execve_wrapper() -> anyhow::Result<()> { /// /// `file` is the absolute, canonical path to the executable to run, i.e. the first arg to exec. /// `argv` is the argv, including the program name (`argv[0]`). +/// `workdir` is the shell working directory used to detect workdir-local binaries. pub(crate) fn evaluate_exec_policy( policy: &Policy, file: &Path, argv: &[String], + workdir: &Path, preserve_program_paths: bool, ) -> Result { + let preserve_program_paths = + should_preserve_program_paths(file, argv, workdir, preserve_program_paths); let program_name = format_program_name(file, preserve_program_paths).ok_or_else(|| { McpError::internal_error( format!("failed to format program name for `{}`", file.display()), @@ -190,7 +194,10 @@ pub(crate) fn evaluate_exec_policy( .chain(argv.iter().skip(1).cloned()) .collect(); let evaluation = policy.check(&command, &|cmd| { - if command_might_be_dangerous(cmd) { + // Keep dangerous-command heuristics stable even when policy evaluation + // needs a full program path. + let normalized_for_heuristics = normalize_program_name_for_heuristics(cmd); + if command_might_be_dangerous(&normalized_for_heuristics) { Decision::Prompt } else { Decision::Allow @@ -220,6 +227,61 @@ pub(crate) fn evaluate_exec_policy( }) } +/// Decide whether execpolicy matching should use full program paths. +/// +/// Security rules: +/// - Always preserve paths for path-qualified invocations (for example `./git`). +/// - Preserve paths for binaries resolved under `workdir` to avoid basename +/// authorization confusion for repo-local executables. +/// - Otherwise keep basename matching for compatibility with existing rules. +fn should_preserve_program_paths( + file: &Path, + argv: &[String], + workdir: &Path, + preserve_program_paths: bool, +) -> bool { + if preserve_program_paths { + return true; + } + + // Path-qualified invocations (for example `./git` or `/tmp/git`) must not + // be reduced to basenames when evaluating escalation policy. + if let Some(argv0) = argv.first() + && Path::new(argv0).components().count() > 1 + { + return true; + } + + // Protect against PATH-based hijacks that resolve to workdir-local binaries + // while preserving basename matching for normal system binaries. + // + // Skip this for root (`/`) workdirs because every absolute path would + // otherwise match. + workdir.parent().is_some() && file.starts_with(workdir) +} + +/// Normalize command argv for dangerous-command heuristics. +/// +/// Policy matching may intentionally preserve full paths, but dangerous-command +/// detection is keyed off command basenames (for example `rm` and `git`). +/// This helper keeps heuristics behavior stable by converting `argv[0]` to a +/// basename while leaving the remaining args unchanged. +fn normalize_program_name_for_heuristics(command: &[String]) -> Vec { + let Some((cmd0, args)) = command.split_first() else { + return Vec::new(); + }; + + let normalized_cmd0 = Path::new(cmd0) + .file_name() + .and_then(|osstr| osstr.to_str()) + .unwrap_or(cmd0) + .to_string(); + + std::iter::once(normalized_cmd0) + .chain(args.iter().cloned()) + .collect() +} + fn format_program_name(path: &Path, preserve_program_paths: bool) -> Option { if preserve_program_paths { path.to_str().map(str::to_string) @@ -263,9 +325,11 @@ mod tests { fn evaluate_exec_policy_uses_heuristics_for_dangerous_commands() { let policy = Policy::empty(); let file = Path::new("/bin/rm"); + let workdir = Path::new("/tmp"); let argv = vec!["rm".to_string(), "-rf".to_string(), "/".to_string()]; - let outcome = evaluate_exec_policy(&policy, file, &argv, false).expect("policy evaluation"); + let outcome = + evaluate_exec_policy(&policy, file, &argv, workdir, false).expect("policy evaluation"); assert_eq!( outcome, @@ -288,13 +352,15 @@ mod tests { ) .expect("policy rule should be added"); let file = Path::new("/usr/local/bin/custom-cmd"); + let workdir = Path::new("/tmp"); let argv = vec![ "/usr/local/bin/custom-cmd".to_string(), "--flag".to_string(), "value".to_string(), ]; - let outcome = evaluate_exec_policy(&policy, file, &argv, true).expect("policy evaluation"); + let outcome = + evaluate_exec_policy(&policy, file, &argv, workdir, true).expect("policy evaluation"); assert_eq!( outcome, @@ -303,4 +369,79 @@ mod tests { } ); } + + #[test] + fn evaluate_exec_policy_does_not_escalate_basename_rule_when_paths_preserved() { + let mut policy = Policy::empty(); + policy + .add_prefix_rule(&["git".to_string(), "status".to_string()], Decision::Allow) + .expect("policy rule should be added"); + let file = Path::new("/tmp/repo/git"); + let workdir = Path::new("/tmp/repo"); + let argv = vec!["git".to_string(), "status".to_string()]; + + let outcome = + evaluate_exec_policy(&policy, file, &argv, workdir, false).expect("policy evaluation"); + + assert_eq!( + outcome, + ExecPolicyOutcome::Allow { + sandbox_permissions: SandboxPermissions::UseDefault + } + ); + } + + #[test] + fn evaluate_exec_policy_path_preservation_still_prompts_for_dangerous_commands() { + let policy = Policy::empty(); + let file = Path::new("/tmp/repo/rm"); + let workdir = Path::new("/tmp/repo"); + let argv = vec!["./rm".to_string(), "-rf".to_string(), "/".to_string()]; + + let outcome = + evaluate_exec_policy(&policy, file, &argv, workdir, false).expect("policy evaluation"); + + assert_eq!( + outcome, + ExecPolicyOutcome::Prompt { + sandbox_permissions: SandboxPermissions::UseDefault + } + ); + } + + #[test] + fn evaluate_exec_policy_default_git_runs_in_sandbox() { + let policy = Policy::empty(); + let file = Path::new("/usr/bin/git"); + let workdir = Path::new("/tmp/repo"); + let argv = vec!["git".to_string(), "status".to_string()]; + + let outcome = + evaluate_exec_policy(&policy, file, &argv, workdir, false).expect("policy evaluation"); + + assert_eq!( + outcome, + ExecPolicyOutcome::Allow { + sandbox_permissions: SandboxPermissions::UseDefault + } + ); + } + + #[test] + fn evaluate_exec_policy_default_dot_git_runs_in_sandbox() { + let policy = Policy::empty(); + let file = Path::new("/tmp/repo/git"); + let workdir = Path::new("/tmp/repo"); + let argv = vec!["./git".to_string(), "status".to_string()]; + + let outcome = + evaluate_exec_policy(&policy, file, &argv, workdir, false).expect("policy evaluation"); + + assert_eq!( + outcome, + ExecPolicyOutcome::Allow { + sandbox_permissions: SandboxPermissions::UseDefault + } + ); + } } diff --git a/codex-rs/exec-server/src/posix/mcp_escalation_policy.rs b/codex-rs/exec-server/src/posix/mcp_escalation_policy.rs index 6d0c1bb338..2e49aab428 100644 --- a/codex-rs/exec-server/src/posix/mcp_escalation_policy.rs +++ b/codex-rs/exec-server/src/posix/mcp_escalation_policy.rs @@ -105,8 +105,13 @@ impl EscalationPolicy for McpEscalationPolicy { workdir: &Path, ) -> Result { let policy = self.policy.read().await; - let outcome = - crate::posix::evaluate_exec_policy(&policy, file, argv, self.preserve_program_paths)?; + let outcome = crate::posix::evaluate_exec_policy( + &policy, + file, + argv, + workdir, + self.preserve_program_paths, + )?; let action = match outcome { ExecPolicyOutcome::Allow { sandbox_permissions,