mirror of
https://github.com/openai/codex.git
synced 2026-11-09 20:50:33 +00:00
Harden PowerShell parser trust
Only run the PowerShell AST parser on Windows through trusted system install paths, and skip Windows PowerShell safelisting entirely on non-Windows hosts. Add regression coverage for workspace-controlled pwsh paths so parser classification cannot execute attacker-controlled binaries before sandboxing.
This commit is contained in:
@@ -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"])));
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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<Vec<Vec<String>>> {
|
||||
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<String> {
|
||||
#[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<Vec<String>>),
|
||||
@@ -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::*;
|
||||
|
||||
@@ -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<Vec<
|
||||
/// Tokenizes an inline PowerShell script and delegates to the command splitter.
|
||||
/// Examples of when this is called: pwsh.exe -Command '<script>' or pwsh.exe -Command:<script>
|
||||
fn parse_powershell_script(executable: &str, script: &str) -> Option<Vec<Vec<String>>> {
|
||||
if let PowershellParseOutcome::Commands(commands) =
|
||||
parse_with_powershell_ast(executable, script)
|
||||
{
|
||||
Some(commands)
|
||||
} else {
|
||||
None
|
||||
}
|
||||
try_parse_powershell_ast_commands(executable, script)
|
||||
}
|
||||
|
||||
/// Returns true when the executable name is one of the supported PowerShell binaries.
|
||||
@@ -228,6 +221,8 @@ mod tests {
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::string::ToString;
|
||||
|
||||
const POWERSHELL_EXE: &str = r"C:\Windows\System32\WindowsPowerShell\v1.0\powershell.exe";
|
||||
|
||||
/// Converts a slice of string literals into owned `String`s for the tests.
|
||||
fn vec_str(args: &[&str]) -> Vec<String> {
|
||||
args.iter().map(ToString::to_string).collect()
|
||||
@@ -236,21 +231,21 @@ mod tests {
|
||||
#[test]
|
||||
fn recognizes_safe_powershell_wrappers() {
|
||||
assert!(is_safe_command_windows(&vec_str(&[
|
||||
"powershell.exe",
|
||||
POWERSHELL_EXE,
|
||||
"-NoLogo",
|
||||
"-Command",
|
||||
"Get-ChildItem -Path .",
|
||||
])));
|
||||
|
||||
assert!(is_safe_command_windows(&vec_str(&[
|
||||
"powershell.exe",
|
||||
POWERSHELL_EXE,
|
||||
"-NoProfile",
|
||||
"-Command",
|
||||
"git status",
|
||||
])));
|
||||
|
||||
assert!(is_safe_command_windows(&vec_str(&[
|
||||
"powershell.exe",
|
||||
POWERSHELL_EXE,
|
||||
"Get-Content",
|
||||
"Cargo.toml",
|
||||
])));
|
||||
@@ -283,7 +278,7 @@ mod tests {
|
||||
}
|
||||
|
||||
assert!(is_safe_command_windows(&vec_str(&[
|
||||
r"C:\Windows\System32\WindowsPowerShell\v1.0\powershell.exe",
|
||||
POWERSHELL_EXE,
|
||||
"-Command",
|
||||
"Get-Content Cargo.toml",
|
||||
])));
|
||||
@@ -381,7 +376,7 @@ mod tests {
|
||||
(
|
||||
script,
|
||||
is_safe_command_windows(&[
|
||||
"powershell.exe".to_string(),
|
||||
POWERSHELL_EXE.to_string(),
|
||||
"-NoProfile".to_string(),
|
||||
"-Command".to_string(),
|
||||
script.to_string(),
|
||||
@@ -405,97 +400,97 @@ mod tests {
|
||||
#[test]
|
||||
fn rejects_powershell_commands_with_side_effects() {
|
||||
assert!(!is_safe_command_windows(&vec_str(&[
|
||||
"powershell.exe",
|
||||
POWERSHELL_EXE,
|
||||
"-NoLogo",
|
||||
"-Command",
|
||||
"Remove-Item foo.txt",
|
||||
])));
|
||||
|
||||
assert!(!is_safe_command_windows(&vec_str(&[
|
||||
"powershell.exe",
|
||||
POWERSHELL_EXE,
|
||||
"-NoProfile",
|
||||
"-Command",
|
||||
"rg --pre cat",
|
||||
])));
|
||||
|
||||
assert!(!is_safe_command_windows(&vec_str(&[
|
||||
"powershell.exe",
|
||||
POWERSHELL_EXE,
|
||||
"-Command",
|
||||
"Set-Content foo.txt 'hello'",
|
||||
])));
|
||||
|
||||
// Redirections are blocked
|
||||
assert!(!is_safe_command_windows(&vec_str(&[
|
||||
"powershell.exe",
|
||||
POWERSHELL_EXE,
|
||||
"-Command",
|
||||
"echo hi > out.txt",
|
||||
])));
|
||||
assert!(!is_safe_command_windows(&vec_str(&[
|
||||
"powershell.exe",
|
||||
POWERSHELL_EXE,
|
||||
"-Command",
|
||||
"Get-Content x | Out-File y",
|
||||
])));
|
||||
assert!(!is_safe_command_windows(&vec_str(&[
|
||||
"powershell.exe",
|
||||
POWERSHELL_EXE,
|
||||
"-Command",
|
||||
"Write-Output foo 2> err.txt",
|
||||
])));
|
||||
|
||||
// Call operator is blocked
|
||||
assert!(!is_safe_command_windows(&vec_str(&[
|
||||
"powershell.exe",
|
||||
POWERSHELL_EXE,
|
||||
"-Command",
|
||||
"& Remove-Item foo",
|
||||
])));
|
||||
|
||||
// Chained safe + unsafe must fail
|
||||
assert!(!is_safe_command_windows(&vec_str(&[
|
||||
"powershell.exe",
|
||||
POWERSHELL_EXE,
|
||||
"-Command",
|
||||
"Get-ChildItem; Remove-Item foo",
|
||||
])));
|
||||
// Nested unsafe cmdlet inside safe command must fail
|
||||
assert!(!is_safe_command_windows(&vec_str(&[
|
||||
"powershell.exe",
|
||||
POWERSHELL_EXE,
|
||||
"-Command",
|
||||
"Write-Output (Set-Content foo6.txt 'abc')",
|
||||
])));
|
||||
// Additional nested unsafe cmdlet examples must fail
|
||||
assert!(!is_safe_command_windows(&vec_str(&[
|
||||
"powershell.exe",
|
||||
POWERSHELL_EXE,
|
||||
"-Command",
|
||||
"Write-Host (Remove-Item foo.txt)",
|
||||
])));
|
||||
assert!(!is_safe_command_windows(&vec_str(&[
|
||||
"powershell.exe",
|
||||
POWERSHELL_EXE,
|
||||
"-Command",
|
||||
"Get-Content (New-Item bar.txt)",
|
||||
])));
|
||||
|
||||
// Unsafe @ expansion.
|
||||
assert!(!is_safe_command_windows(&vec_str(&[
|
||||
"powershell.exe",
|
||||
POWERSHELL_EXE,
|
||||
"-Command",
|
||||
"ls @(calc.exe)"
|
||||
])));
|
||||
|
||||
// Unsupported constructs that the AST parser refuses (no fallback to manual splitting).
|
||||
assert!(!is_safe_command_windows(&vec_str(&[
|
||||
"powershell.exe",
|
||||
POWERSHELL_EXE,
|
||||
"-Command",
|
||||
"ls && pwd"
|
||||
])));
|
||||
|
||||
// Sub-expressions are rejected even if they contain otherwise safe commands.
|
||||
assert!(!is_safe_command_windows(&vec_str(&[
|
||||
"powershell.exe",
|
||||
POWERSHELL_EXE,
|
||||
"-Command",
|
||||
"Write-Output $(Get-Content foo)"
|
||||
])));
|
||||
|
||||
// Empty words from the parser (e.g. '') are rejected.
|
||||
assert!(!is_safe_command_windows(&vec_str(&[
|
||||
"powershell.exe",
|
||||
POWERSHELL_EXE,
|
||||
"-Command",
|
||||
"''"
|
||||
])));
|
||||
@@ -504,13 +499,13 @@ mod tests {
|
||||
#[test]
|
||||
fn accepts_constant_expression_arguments() {
|
||||
assert!(is_safe_command_windows(&vec_str(&[
|
||||
"powershell.exe",
|
||||
POWERSHELL_EXE,
|
||||
"-Command",
|
||||
"Get-Content 'foo bar'"
|
||||
])));
|
||||
|
||||
assert!(is_safe_command_windows(&vec_str(&[
|
||||
"powershell.exe",
|
||||
POWERSHELL_EXE,
|
||||
"-Command",
|
||||
"Get-Content \"foo bar\""
|
||||
])));
|
||||
@@ -519,13 +514,13 @@ mod tests {
|
||||
#[test]
|
||||
fn rejects_dynamic_arguments() {
|
||||
assert!(!is_safe_command_windows(&vec_str(&[
|
||||
"powershell.exe",
|
||||
POWERSHELL_EXE,
|
||||
"-Command",
|
||||
"Get-Content $foo"
|
||||
])));
|
||||
|
||||
assert!(!is_safe_command_windows(&vec_str(&[
|
||||
"powershell.exe",
|
||||
POWERSHELL_EXE,
|
||||
"-Command",
|
||||
"Write-Output \"foo $bar\""
|
||||
])));
|
||||
@@ -539,12 +534,9 @@ mod tests {
|
||||
|
||||
let chain = "pwd && ls";
|
||||
assert!(
|
||||
!is_safe_command_windows(&vec_str(&[
|
||||
"powershell.exe",
|
||||
"-NoProfile",
|
||||
"-Command",
|
||||
chain,
|
||||
])),
|
||||
!is_safe_command_windows(&vec_str(
|
||||
&[POWERSHELL_EXE, "-NoProfile", "-Command", chain,]
|
||||
)),
|
||||
"`{chain}` is not recognized by powershell.exe"
|
||||
);
|
||||
|
||||
|
||||
@@ -155,6 +155,10 @@ mod tests {
|
||||
#[cfg(windows)]
|
||||
use super::parse_powershell_command_into_plain_commands;
|
||||
|
||||
#[cfg(windows)]
|
||||
const WINDOWS_POWERSHELL_EXE: &str =
|
||||
r"C:\Windows\System32\WindowsPowerShell\v1.0\powershell.exe";
|
||||
|
||||
#[test]
|
||||
fn extracts_basic_powershell_command() {
|
||||
let cmd = vec![
|
||||
@@ -206,7 +210,7 @@ mod tests {
|
||||
#[test]
|
||||
fn parses_plain_powershell_commands() {
|
||||
let commands = parse_powershell_command_into_plain_commands(&[
|
||||
"powershell.exe".to_string(),
|
||||
WINDOWS_POWERSHELL_EXE.to_string(),
|
||||
"-NoProfile".to_string(),
|
||||
"-Command".to_string(),
|
||||
"echo hi".to_string(),
|
||||
@@ -220,7 +224,7 @@ mod tests {
|
||||
#[test]
|
||||
fn parses_multiple_plain_powershell_commands() {
|
||||
let commands = parse_powershell_command_into_plain_commands(&[
|
||||
"powershell.exe".to_string(),
|
||||
WINDOWS_POWERSHELL_EXE.to_string(),
|
||||
"-NoProfile".to_string(),
|
||||
"-Command".to_string(),
|
||||
"Write-Output foo | Measure-Object".to_string(),
|
||||
|
||||
Reference in New Issue
Block a user