From 38c70e2df1f22b8b9f429e1dcd70f76789abb16e Mon Sep 17 00:00:00 2001 From: Chris Bookholt Date: Fri, 3 Jul 2026 15:34:28 -0700 Subject: [PATCH] fix(core): harden model-selected shell policy identity --- codex-rs/core/src/exec_policy.rs | 5 +- .../core/src/exec_policy_powershell_tests.rs | 57 ++++++ codex-rs/shell-command/src/shell_detect.rs | 183 ++++++++++++++++-- 3 files changed, 232 insertions(+), 13 deletions(-) diff --git a/codex-rs/core/src/exec_policy.rs b/codex-rs/core/src/exec_policy.rs index 4dda333295..fc7095a9a5 100644 --- a/codex-rs/core/src/exec_policy.rs +++ b/codex-rs/core/src/exec_policy.rs @@ -824,7 +824,10 @@ impl ExecPolicyManager { return requirement; } Some(powershell_policy::PreparedPowerShell::Parsed(parsed)) => { - if let Some(outer_argv) = parsed.untrusted_outer_argv() { + let composed_outer_argv = parsed + .untrusted_outer_argv() + .or_else(|| provenance.requires_outer_policy().then_some(command)); + if let Some(outer_argv) = composed_outer_argv { return create_untrusted_powershell_approval_requirement( exec_policy.as_ref(), outer_argv, diff --git a/codex-rs/core/src/exec_policy_powershell_tests.rs b/codex-rs/core/src/exec_policy_powershell_tests.rs index be79163ff2..993a2f1465 100644 --- a/codex-rs/core/src/exec_policy_powershell_tests.rs +++ b/codex-rs/core/src/exec_policy_powershell_tests.rs @@ -692,6 +692,63 @@ fn untrusted_wrapper_identity_uses_exact_outer_and_restrictive_basename_rules() } } +#[tokio::test] +async fn local_model_resolved_trusted_powershell_composes_exact_outer_and_inner() { + let outer = powershell_command("Get-Location"); + let inner = vec_str(&["Get-Location"]); + let full_outer_allow = prefix_rule_for(&outer, "allow"); + let short_outer_allow = prefix_rule_for(&outer[..1], "allow"); + let inner_allow = prefix_rule_for(&inner, "allow"); + + pretty_assertions::assert_eq!( + requirement_with_provenance( + Some(&format!("{full_outer_allow}\n{inner_allow}")), + &outer, + AskForApproval::OnRequest, + PermissionProfile::workspace_write(), + SandboxPermissions::UseDefault, + ShellApprovalProvenance::local_model_resolved(), + ) + .await, + untrusted_skip(/*bypass_sandbox*/ true), + "exact outer and explicit inner authority should authorize the parsed composition", + ); + + pretty_assertions::assert_eq!( + requirement_with_provenance( + Some(&format!("{short_outer_allow}\n{inner_allow}")), + &outer, + AskForApproval::OnRequest, + PermissionProfile::workspace_write(), + SandboxPermissions::UseDefault, + ShellApprovalProvenance::local_model_resolved(), + ) + .await, + untrusted_skip(/*bypass_sandbox*/ false), + "partial outer authority may proceed only inside the managed sandbox", + ); + + let inner_forbidden = prefix_rule_for(&inner, "forbidden"); + pretty_assertions::assert_eq!( + requirement_with_provenance( + Some(&format!("{full_outer_allow}\n{inner_forbidden}")), + &outer, + AskForApproval::OnRequest, + PermissionProfile::workspace_write(), + SandboxPermissions::UseDefault, + ShellApprovalProvenance::local_model_resolved(), + ) + .await, + ExecApprovalRequirement::Forbidden { + reason: format!( + "`{}` rejected: policy forbids commands starting with `Get-Location`", + render_shlex_command(&outer) + ), + }, + "an inner forbidden rule must dominate exact outer authority", + ); +} + #[tokio::test] async fn untrusted_parsed_results_ignore_requested_amendments() { let command = absolute_untrusted_powershell_command("echo allowed"); diff --git a/codex-rs/shell-command/src/shell_detect.rs b/codex-rs/shell-command/src/shell_detect.rs index 4237302169..e011d7347b 100644 --- a/codex-rs/shell-command/src/shell_detect.rs +++ b/codex-rs/shell-command/src/shell_detect.rs @@ -42,6 +42,7 @@ pub enum ModelShellResolveError { UnsupportedWindowsLaunch(PathBuf), UnsupportedWindowsPathNamespace(PathBuf), UnresolvedWindowsRelativePath(PathBuf), + ParentDirectoryComponent(PathBuf), NonUtf8ResolvedPath(PathBuf), RelativeWorkingDirectory(PathBuf), } @@ -91,6 +92,11 @@ impl std::fmt::Display for ModelShellResolveError { "model-provided Windows shell path `{}` is drive-relative and cannot be resolved against the selected working directory", path.display() ), + Self::ParentDirectoryComponent(path) => write!( + formatter, + "model-provided shell resolution refuses parent-directory component in `{}`", + path.display() + ), Self::NonUtf8ResolvedPath(path) => write!( formatter, "resolved model-provided shell path `{}` is not representable in command argv", @@ -410,12 +416,18 @@ pub fn resolve_model_provided_shell_in( let resolved_path = { let _ = path_ext; if is_bare_name { - std::env::split_paths(search_path) - .map(|directory| resolve_relative_path(&directory, cwd).join(shell_path)) - .find(|candidate| validate_model_shell_path(candidate).is_ok()) + let mut resolved = None; + for directory in std::env::split_paths(search_path) { + let candidate = resolve_relative_path(&directory, cwd)?.join(shell_path); + if validate_model_shell_path(&candidate).is_ok() { + resolved = Some(candidate); + break; + } + } + resolved .ok_or_else(|| ModelShellResolveError::MissingBareName(shell_path.to_path_buf()))? } else { - resolve_relative_path(shell_path, cwd) + resolve_relative_path(shell_path, cwd)? } }; @@ -429,16 +441,22 @@ pub fn resolve_model_provided_shell_in( }) } -fn resolve_relative_path(path: &Path, cwd: &Path) -> PathBuf { +fn resolve_relative_path(path: &Path, cwd: &Path) -> Result { let absolute = if path.is_absolute() { path.to_path_buf() } else { cwd.join(path) }; - absolute + if absolute + .components() + .any(|component| matches!(component, std::path::Component::ParentDir)) + { + return Err(ModelShellResolveError::ParentDirectoryComponent(absolute)); + } + Ok(absolute .components() .filter(|component| !matches!(component, std::path::Component::CurDir)) - .collect() + .collect()) } #[cfg(any(windows, test))] @@ -467,10 +485,8 @@ fn resolve_model_shell_path_windows( )); } let mut first_specific_error = None; - for candidate in candidate_names - .into_iter() - .map(|candidate| resolve_relative_path(&candidate, cwd)) - { + for candidate in candidate_names { + let candidate = resolve_relative_path(&candidate, cwd)?; match validate_model_shell_path(&candidate) { Ok(()) => return Ok(candidate), Err(ModelShellResolveError::MissingPath(_)) => {} @@ -550,7 +566,7 @@ fn resolve_windows_path_against_cwd( validate_windows_local_path_namespace(cwd)?; } - Ok(resolve_relative_path(path, cwd)) + resolve_relative_path(path, cwd) } #[cfg(any(windows, test))] @@ -794,6 +810,93 @@ mod tests { } } + #[cfg(unix)] + #[test] + fn model_shell_resolver_rejects_parent_components_in_shell_and_cwd() { + let temp_dir = tempfile::tempdir().expect("temp dir"); + let cwd = temp_dir.path().join("workspace"); + let outside = temp_dir.path().join("outside"); + let tools = cwd.join("tools"); + std::fs::create_dir_all(&outside).expect("create outside"); + std::fs::create_dir_all(&tools).expect("create tools"); + write_executable(&outside.join("bash")); + write_executable(&tools.join("bash")); + + for (requested, selected_cwd, rejected) in [ + ( + PathBuf::from("../outside/bash"), + cwd.clone(), + cwd.join("../outside/bash"), + ), + ( + cwd.join("../outside/bash"), + cwd.clone(), + cwd.join("../outside/bash"), + ), + ( + PathBuf::from("tools/bash"), + cwd.join("../workspace"), + cwd.join("../workspace/tools/bash"), + ), + ] { + assert_eq!( + resolve_model_provided_shell_in( + &requested, + OsStr::new(""), + /*path_ext*/ None, + &selected_cwd, + ), + Err(ModelShellResolveError::ParentDirectoryComponent(rejected)), + "parent components must be rejected for {requested:?} against {selected_cwd:?}", + ); + } + } + + #[cfg(unix)] + #[test] + fn model_shell_resolver_rejects_parent_path_entries_in_lookup_order() { + let temp_dir = tempfile::tempdir().expect("temp dir"); + let cwd = temp_dir.path().join("workspace"); + let valid_bin = temp_dir.path().join("valid-bin"); + let outside = temp_dir.path().join("outside"); + std::fs::create_dir(&cwd).expect("create cwd"); + std::fs::create_dir(&valid_bin).expect("create valid bin"); + std::fs::create_dir(&outside).expect("create outside"); + let valid_shell = valid_bin.join("bash"); + write_executable(&valid_shell); + write_executable(&outside.join("bash")); + + let parent_first = std::env::join_paths([Path::new("../outside"), valid_bin.as_path()]) + .expect("join PATH"); + assert_eq!( + resolve_model_provided_shell_in( + Path::new("bash"), + &parent_first, + /*path_ext*/ None, + &cwd, + ), + Err(ModelShellResolveError::ParentDirectoryComponent( + cwd.join("../outside") + )), + ); + + let valid_first = std::env::join_paths([valid_bin.as_path(), Path::new("../outside")]) + .expect("join PATH"); + assert_eq!( + resolve_model_provided_shell_in( + Path::new("bash"), + &valid_first, + /*path_ext*/ None, + &cwd, + ), + Ok(DetectedShell { + shell_type: ShellType::Bash, + shell_path: valid_shell, + }), + "an earlier valid PATH candidate wins before an unused parent-containing entry", + ); + } + #[cfg(unix)] #[test] fn model_shell_resolver_rejects_unsupported_missing_and_non_launchable_inputs() { @@ -1070,6 +1173,62 @@ mod tests { ); } + #[cfg(windows)] + #[test] + fn windows_model_shell_resolver_rejects_parent_components_in_lookup_order() { + let temp_dir = tempfile::tempdir().expect("temp dir"); + let bin = temp_dir.path().join("bin"); + let tools = temp_dir.path().join("tools"); + std::fs::create_dir(&bin).expect("create bin"); + std::fs::create_dir(&tools).expect("create tools"); + let shell = bin.join("powershell.EXE"); + std::fs::write(&shell, "fixture").expect("write fake executable"); + + let requested = PathBuf::from(r"tools\..\bin\powershell.exe"); + assert_eq!( + resolve_model_provided_shell_in( + &requested, + OsStr::new(""), + Some(OsStr::new(".EXE")), + temp_dir.path(), + ), + Err(ModelShellResolveError::ParentDirectoryComponent( + temp_dir.path().join(&requested) + )), + ); + + let parent_entry = PathBuf::from(r"tools\.."); + let parent_first = + std::env::join_paths([parent_entry.as_path(), bin.as_path()]).expect("join PATH"); + assert_eq!( + resolve_model_provided_shell_in( + Path::new("powershell"), + &parent_first, + Some(OsStr::new(".EXE")), + temp_dir.path(), + ), + Err(ModelShellResolveError::ParentDirectoryComponent( + temp_dir.path().join(&parent_entry) + )), + ); + + let valid_first = + std::env::join_paths([bin.as_path(), parent_entry.as_path()]).expect("join PATH"); + assert_eq!( + resolve_model_provided_shell_in( + Path::new("powershell"), + &valid_first, + Some(OsStr::new(".EXE")), + temp_dir.path(), + ), + Ok(DetectedShell { + shell_type: ShellType::PowerShell, + shell_path: shell, + }), + "an earlier valid PATH candidate wins before an unused parent-containing entry", + ); + } + #[cfg(windows)] #[test] fn windows_model_shell_resolver_checks_unsafe_cwd_only_when_resolution_uses_it() {