diff --git a/codex-rs/shell-command/src/command_safety/is_safe_command.rs b/codex-rs/shell-command/src/command_safety/is_safe_command.rs index b35144a12b..5f20d34de5 100644 --- a/codex-rs/shell-command/src/command_safety/is_safe_command.rs +++ b/codex-rs/shell-command/src/command_safety/is_safe_command.rs @@ -4,6 +4,7 @@ use crate::command_safety::is_dangerous_command::executable_name_lookup_key; // may appear before it (e.g., `-C`, `-c`, `--git-dir`). // Implemented in `is_dangerous_command` and shared here. use crate::command_safety::is_dangerous_command::find_git_subcommand; +#[cfg(windows)] use crate::command_safety::windows_safe_commands::is_safe_command_windows; #[cfg(windows)] use crate::command_safety::windows_safe_commands::is_safe_powershell_words as is_safe_powershell_words_windows; @@ -20,8 +21,11 @@ pub fn is_known_safe_command(command: &[String]) -> bool { }) .collect(); - if is_safe_command_windows(&command) { - return true; + #[cfg(windows)] + { + if is_safe_command_windows(&command) { + return true; + } } if is_safe_to_call_with_exec(&command) { @@ -338,6 +342,38 @@ mod tests { args.iter().map(ToString::to_string).collect() } + #[cfg(not(windows))] + #[test] + fn powershell_like_paths_are_not_probed_on_non_windows() { + use std::os::unix::fs::PermissionsExt; + + let test_dir = std::env::temp_dir().join(format!( + "codex-powershell-safe-command-{}", + std::process::id() + )); + let fake_pwsh = test_dir.join("pwsh"); + let marker = test_dir.join("marker"); + std::fs::create_dir_all(&test_dir).unwrap(); + std::fs::write( + &fake_pwsh, + format!("#!/bin/sh\nprintf spawned > '{}'\n", marker.display()), + ) + .unwrap(); + let mut permissions = std::fs::metadata(&fake_pwsh).unwrap().permissions(); + permissions.set_mode(0o755); + std::fs::set_permissions(&fake_pwsh, permissions).unwrap(); + + let command = vec![ + fake_pwsh.to_string_lossy().to_string(), + "-Command".to_string(), + "Get-Location".to_string(), + ]; + assert!(!is_known_safe_command(&command)); + assert!(!marker.exists()); + + let _ = std::fs::remove_dir_all(test_dir); + } + #[test] fn known_safe_examples() { assert!(is_safe_to_call_with_exec(&vec_str(&["ls"]))); diff --git a/codex-rs/shell-command/src/command_safety/mod.rs b/codex-rs/shell-command/src/command_safety/mod.rs index 2040364e5b..60bed2effc 100644 --- a/codex-rs/shell-command/src/command_safety/mod.rs +++ b/codex-rs/shell-command/src/command_safety/mod.rs @@ -2,5 +2,6 @@ mod powershell_parser; pub mod is_dangerous_command; pub mod is_safe_command; +#[cfg(windows)] pub(crate) mod windows_safe_commands; pub(crate) use powershell_parser::try_parse_powershell_ast_commands; diff --git a/codex-rs/shell-command/src/command_safety/powershell_parser.rs b/codex-rs/shell-command/src/command_safety/powershell_parser.rs index 580f177f9f..58bb86f003 100644 --- a/codex-rs/shell-command/src/command_safety/powershell_parser.rs +++ b/codex-rs/shell-command/src/command_safety/powershell_parser.rs @@ -7,6 +7,8 @@ use std::io::BufRead; use std::io::BufReader; use std::io::ErrorKind; use std::io::Write; +#[cfg(windows)] +use std::path::Path; use std::process::Child; use std::process::ChildStdin; use std::process::ChildStdout; @@ -17,6 +19,10 @@ use std::sync::Mutex; use std::sync::PoisonError; const POWERSHELL_PARSER_SCRIPT: &str = include_str!("powershell_parser.ps1"); +#[cfg(windows)] +const WINDOWS_POWERSHELL_EXE: &str = r"C:\Windows\System32\WindowsPowerShell\v1.0\powershell.exe"; +#[cfg(windows)] +const WINDOWS_PWSH_EXE: &str = r"C:\Program Files\PowerShell\7\pwsh.exe"; /// Cache one long-lived parser process per executable path so repeated safety checks reuse /// PowerShell startup work while still consulting the real parser every time. @@ -38,12 +44,66 @@ pub(crate) fn try_parse_powershell_ast_commands( executable: &str, script: &str, ) -> Option>> { - match parse_with_powershell_ast(executable, script) { + let parser_executable = trusted_powershell_parser_executable(executable)?; + match parse_with_powershell_ast(parser_executable.as_str(), script) { PowershellParseOutcome::Commands(commands) => Some(commands), PowershellParseOutcome::Unsupported | PowershellParseOutcome::Failed => None, } } +/// Resolve the PowerShell binary that is safe to launch as the host-side parser. +/// +/// The command being classified may name `pwsh`/`powershell.exe`, but that argv[0] +/// is not itself trusted input. In particular, paths such as `./pwsh` or +/// `C:\Temp\pwsh.exe` can point at workspace-controlled executables, while bare +/// names such as `pwsh.exe` are resolved through search paths that can include +/// the current directory. The parser subprocess runs before sandboxed execution, +/// so only known system install locations are eligible here. Unknown locations +/// fail closed. +pub(super) fn trusted_powershell_parser_executable(executable: &str) -> Option { + #[cfg(not(windows))] + { + let _ = executable; + None + } + + #[cfg(windows)] + { + let executable_path = Path::new(executable); + let executable_name = executable_path + .file_name() + .and_then(|osstr| osstr.to_str()) + .unwrap_or(executable) + .to_ascii_lowercase(); + let parser_path = match executable_name.as_str() { + "powershell" | "powershell.exe" => WINDOWS_POWERSHELL_EXE, + "pwsh" | "pwsh.exe" => WINDOWS_PWSH_EXE, + _ => return None, + }; + + let has_path_component = executable_path + .parent() + .is_some_and(|parent| !parent.as_os_str().is_empty()); + if !has_path_component { + return None; + } + + { + let normalized_executable = executable.trim_start_matches("\\\\?\\").replace('/', "\\"); + let normalized_parser = parser_path.trim_start_matches("\\\\?\\").replace('/', "\\"); + if !normalized_executable.eq_ignore_ascii_case(&normalized_parser) { + return None; + } + } + + if Path::new(parser_path).is_file() { + Some(parser_path.to_string()) + } else { + None + } + } +} + #[derive(Debug, PartialEq, Eq)] pub(super) enum PowershellParseOutcome { Commands(Vec>), @@ -264,6 +324,46 @@ fn kill_child(child: &mut Child) { let _ = child.wait(); } +#[cfg(test)] +mod trust_tests { + use super::*; + use pretty_assertions::assert_eq; + + #[test] + fn rejects_untrusted_powershell_names() { + for executable in [ + "pwsh", + "powershell.exe", + "./pwsh", + ".\\pwsh.exe", + r"C:\Temp\pwsh.exe", + "/tmp/pwsh", + ] { + assert_eq!( + trusted_powershell_parser_executable(executable), + None, + "{executable:?} must not be launched as a parser process", + ); + } + } + + #[cfg(not(windows))] + #[test] + fn does_not_resolve_powershell_parsers_on_non_windows() { + assert_eq!(trusted_powershell_parser_executable("pwsh"), None); + assert_eq!(trusted_powershell_parser_executable("powershell.exe"), None); + } + + #[cfg(windows)] + #[test] + fn resolves_system_windows_powershell_parser() { + assert_eq!( + trusted_powershell_parser_executable(WINDOWS_POWERSHELL_EXE), + Some(WINDOWS_POWERSHELL_EXE.to_string()), + ); + } +} + #[cfg(all(test, windows))] mod tests { use super::*; diff --git a/codex-rs/shell-command/src/command_safety/windows_safe_commands.rs b/codex-rs/shell-command/src/command_safety/windows_safe_commands.rs index 8ef3f8e8f9..1226728617 100644 --- a/codex-rs/shell-command/src/command_safety/windows_safe_commands.rs +++ b/codex-rs/shell-command/src/command_safety/windows_safe_commands.rs @@ -1,6 +1,5 @@ use crate::command_safety::is_safe_command::is_safe_git_command; -use crate::command_safety::powershell_parser::PowershellParseOutcome; -use crate::command_safety::powershell_parser::parse_with_powershell_ast; +use crate::command_safety::powershell_parser::try_parse_powershell_ast_commands; use std::path::Path; /// On Windows, we conservatively allow only clearly read-only PowerShell invocations @@ -94,13 +93,7 @@ fn parse_powershell_invocation(executable: &str, args: &[String]) -> Option' or pwsh.exe -Command: