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 bca44139cd..af5a1eb14c 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 @@ -621,12 +621,10 @@ mod tests { return; } - let Some(powershell) = crate::powershell::try_find_pwsh_executable_blocking() - .or_else(crate::powershell::try_find_powershell_executable_blocking) - else { + let powershell = r"C:\Windows\System32\WindowsPowerShell\v1.0\powershell.exe"; + if !std::path::Path::new(powershell).is_file() { return; - }; - let powershell = powershell.as_path().to_str().unwrap(); + } assert!(is_known_safe_command(&vec_str(&[ powershell, 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 00b18af6f0..92e124437e 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(any(test, 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(any(test, windows))] +const WINDOWS_POWERSHELL_EXE: &str = r"C:\Windows\System32\WindowsPowerShell\v1.0\powershell.exe"; +#[cfg(any(test, 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. @@ -24,7 +30,7 @@ const POWERSHELL_PARSER_SCRIPT: &str = include_str!("powershell_parser.ps1"); /// We keep the cache behind one mutex because each child process speaks a simple /// request/response protocol over a single stdin/stdout pair, so callers targeting the same /// executable must serialize access anyway. -pub(super) fn parse_with_powershell_ast(executable: &str, script: &str) -> PowershellParseOutcome { +fn parse_with_powershell_ast(executable: &str, script: &str) -> PowershellParseOutcome { static PARSER_PROCESSES: LazyLock>> = LazyLock::new(|| Mutex::new(HashMap::new())); @@ -38,12 +44,49 @@ 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, script) { PowershellParseOutcome::Commands(commands) => Some(commands), PowershellParseOutcome::Unsupported | PowershellParseOutcome::Failed => None, } } +/// Selects the host-side parser only when the command itself names that same trusted binary. +/// +/// The parser runs before the command approval and sandbox boundaries. Bare executable names and +/// arbitrary paths are therefore not eligible: their eventual resolution can depend on the +/// workspace, current directory, or `PATH`. Returning a fixed path also ensures the parser spawn +/// never receives the command's attacker-controlled `argv[0]`. +fn trusted_powershell_parser_executable(executable: &str) -> Option<&'static str> { + #[cfg(windows)] + { + resolve_trusted_powershell_parser_executable(executable, Path::is_file) + } + + #[cfg(not(windows))] + { + let _ = executable; + None + } +} + +#[cfg(any(test, windows))] +fn resolve_trusted_powershell_parser_executable( + executable: &str, + is_file: impl Fn(&Path) -> bool, +) -> Option<&'static str> { + let normalized_executable = executable + .strip_prefix(r"\\?\") + .unwrap_or(executable) + .replace('/', "\\"); + + let trusted_path = [WINDOWS_POWERSHELL_EXE, WINDOWS_PWSH_EXE] + .into_iter() + .find(|candidate| normalized_executable.eq_ignore_ascii_case(candidate))?; + + is_file(Path::new(trusted_path)).then_some(trusted_path) +} + #[derive(Debug, PartialEq, Eq)] pub(super) enum PowershellParseOutcome { Commands(Vec>), @@ -264,6 +307,10 @@ fn kill_child(child: &mut Child) { let _ = child.wait(); } +#[cfg(test)] +#[path = "powershell_parser_trust_tests.rs"] +mod trust_tests; + #[cfg(all(test, windows))] mod tests { use super::*; diff --git a/codex-rs/shell-command/src/command_safety/powershell_parser_trust_tests.rs b/codex-rs/shell-command/src/command_safety/powershell_parser_trust_tests.rs new file mode 100644 index 0000000000..2210c908f3 --- /dev/null +++ b/codex-rs/shell-command/src/command_safety/powershell_parser_trust_tests.rs @@ -0,0 +1,84 @@ +use super::*; +use pretty_assertions::assert_eq; + +fn resolve_existing(executable: &str) -> Option<&'static str> { + resolve_trusted_powershell_parser_executable(executable, |_| true) +} + +#[test] +fn accepts_only_fixed_trusted_powershell_paths() { + assert_eq!( + resolve_existing(WINDOWS_POWERSHELL_EXE), + Some(WINDOWS_POWERSHELL_EXE), + ); + assert_eq!( + resolve_existing(r"c:/WINDOWS/System32/WindowsPowerShell/v1.0/POWERSHELL.EXE"), + Some(WINDOWS_POWERSHELL_EXE), + ); + assert_eq!( + resolve_existing(r"\\?\C:\Windows\System32\WindowsPowerShell\v1.0\powershell.exe"), + Some(WINDOWS_POWERSHELL_EXE), + ); + assert_eq!(resolve_existing(WINDOWS_PWSH_EXE), Some(WINDOWS_PWSH_EXE)); + assert_eq!( + resolve_existing(r"c:/program files/powershell/7/PWSH.EXE"), + Some(WINDOWS_PWSH_EXE), + ); +} + +#[test] +fn rejects_search_path_and_workspace_controlled_variants() { + for executable in [ + "powershell", + "powershell.exe", + "pwsh", + "pwsh.exe", + r".\powershell.exe", + r".\pwsh.exe", + "./powershell.exe", + "./pwsh.exe", + r"tools\powershell.exe", + r"tools\pwsh.exe", + r"C:\workspace\powershell.exe", + r"C:\workspace\pwsh.exe", + r"\\server\share\pwsh.exe", + ] { + assert_eq!( + resolve_existing(executable), + None, + "{executable:?} must not be launched as a parser", + ); + } +} + +#[test] +fn rejects_command_wrappers_and_path_aliases() { + for executable in [ + r"C:\Windows\System32\WindowsPowerShell\v1.0\powershell.cmd", + r"C:\Program Files\PowerShell\7\pwsh.cmd", + r"C:\Windows\System32\WindowsPowerShell\v1.0\powershell", + r"C:\Program Files\PowerShell\7\pwsh", + r"C:\Windows\System32\WindowsPowerShell\v1.0\..\powershell.exe", + r"\\.\C:\Windows\System32\WindowsPowerShell\v1.0\powershell.exe", + r"C:\Windows\System32\WindowsPowerShell\v1.0\powershell.exe.cmd", + r"C:\Program Files\PowerShell\7\pwsh.exe.cmd", + ] { + assert_eq!( + resolve_existing(executable), + None, + "{executable:?} must fail closed", + ); + } +} + +#[test] +fn fails_closed_when_the_trusted_install_is_missing() { + assert_eq!( + resolve_trusted_powershell_parser_executable(WINDOWS_POWERSHELL_EXE, |_| false), + None, + ); + assert_eq!( + resolve_trusted_powershell_parser_executable(WINDOWS_PWSH_EXE, |_| false), + None, + ); +} 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 df2025253b..514c3daac0 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: