mirror of
https://github.com/openai/codex.git
synced 2026-09-09 15:58:47 +00:00
fix: tighten shell wrapper detection
Use exact matches for known shell wrappers so command display and approval keys do not treat arbitrary shell-like paths as trusted wrappers. Co-authored-by: Codex <noreply@openai.com>
This commit is contained in:
@@ -86,3 +86,14 @@ fn preserves_non_shell_commands() {
|
||||
let command = vec!["cargo".to_string(), "fmt".to_string()];
|
||||
assert_eq!(canonicalize_command_for_approval(&command), command);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn preserves_shell_like_paths_in_approval_keys() {
|
||||
let command = vec![
|
||||
".poc/bash".to_string(),
|
||||
"-lc".to_string(),
|
||||
"cargo test -p codex-core".to_string(),
|
||||
];
|
||||
|
||||
assert_eq!(canonicalize_command_for_approval(&command), command);
|
||||
}
|
||||
|
||||
@@ -367,6 +367,9 @@ mod detect_shell_type_tests {
|
||||
detect_shell_type(&PathBuf::from("/bin/bash")),
|
||||
Some(ShellType::Bash)
|
||||
);
|
||||
assert_eq!(detect_shell_type(&PathBuf::from(".poc/bash")), None);
|
||||
assert_eq!(detect_shell_type(&PathBuf::from("/tmp/bash")), None);
|
||||
assert_eq!(detect_shell_type(&PathBuf::from("/tmp/bash.evil")), None);
|
||||
assert_eq!(
|
||||
detect_shell_type(&PathBuf::from("powershell.exe")),
|
||||
Some(ShellType::PowerShell)
|
||||
@@ -401,6 +404,12 @@ mod detect_shell_type_tests {
|
||||
Some(ShellType::Cmd)
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn model_provided_shell_does_not_accept_repo_local_shell_names() {
|
||||
let shell = get_shell_by_model_provided_path(&PathBuf::from(".poc/bash"));
|
||||
assert_ne!(shell.shell_path, PathBuf::from(".poc/bash"));
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
|
||||
@@ -1,24 +1,34 @@
|
||||
use crate::shell::ShellType;
|
||||
use std::path::Path;
|
||||
use std::path::PathBuf;
|
||||
|
||||
pub(crate) fn detect_shell_type(shell_path: &PathBuf) -> Option<ShellType> {
|
||||
match shell_path.as_os_str().to_str() {
|
||||
Some("zsh") => Some(ShellType::Zsh),
|
||||
Some("sh") => Some(ShellType::Sh),
|
||||
Some("cmd") => Some(ShellType::Cmd),
|
||||
Some("bash") => Some(ShellType::Bash),
|
||||
Some("pwsh") => Some(ShellType::PowerShell),
|
||||
Some("powershell") => Some(ShellType::PowerShell),
|
||||
_ => {
|
||||
let shell_name = shell_path.file_stem();
|
||||
if let Some(shell_name) = shell_name {
|
||||
let shell_name_path = Path::new(shell_name);
|
||||
if shell_name_path != Path::new(shell_path) {
|
||||
return detect_shell_type(&shell_name_path.to_path_buf());
|
||||
}
|
||||
}
|
||||
None
|
||||
let shell_text = shell_path.as_os_str().to_str()?;
|
||||
// Keep this exact: repo-local files named like shells must not inherit
|
||||
// shell-wrapper trust in approval or display decisions.
|
||||
match shell_text {
|
||||
"zsh" | "/bin/zsh" | "/usr/bin/zsh" | "/usr/local/bin/zsh" | "/opt/homebrew/bin/zsh" => {
|
||||
Some(ShellType::Zsh)
|
||||
}
|
||||
"sh" | "/bin/sh" | "/usr/bin/sh" => Some(ShellType::Sh),
|
||||
"bash"
|
||||
| "/bin/bash"
|
||||
| "/usr/bin/bash"
|
||||
| "/usr/local/bin/bash"
|
||||
| "/opt/homebrew/bin/bash" => Some(ShellType::Bash),
|
||||
"pwsh"
|
||||
| "powershell"
|
||||
| "pwsh.exe"
|
||||
| "powershell.exe"
|
||||
| "/usr/local/bin/pwsh"
|
||||
| "/usr/bin/pwsh"
|
||||
| "/bin/pwsh"
|
||||
| "/opt/homebrew/bin/pwsh" => Some(ShellType::PowerShell),
|
||||
"cmd" | "cmd.exe" => Some(ShellType::Cmd),
|
||||
_ => match shell_text.replace('\\', "/").to_ascii_lowercase().as_str() {
|
||||
"c:/windows/system32/cmd.exe" => Some(ShellType::Cmd),
|
||||
"c:/windows/system32/windowspowershell/v1.0/powershell.exe"
|
||||
| "c:/program files/powershell/7/pwsh.exe" => Some(ShellType::PowerShell),
|
||||
_ => None,
|
||||
},
|
||||
}
|
||||
}
|
||||
|
||||
@@ -68,6 +68,27 @@ fn test_get_command_respects_explicit_bash_shell() -> anyhow::Result<()> {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_get_command_does_not_execute_shell_like_repo_path() -> anyhow::Result<()> {
|
||||
let json = r#"{"cmd": "echo hello", "shell": ".poc/bash"}"#;
|
||||
|
||||
let args: ExecCommandArgs = parse_arguments(json)?;
|
||||
|
||||
assert_eq!(args.shell.as_deref(), Some(".poc/bash"));
|
||||
|
||||
let command = get_command(
|
||||
&args,
|
||||
Arc::new(default_user_shell()),
|
||||
&UnifiedExecShellMode::Direct,
|
||||
/*allow_login_shell*/ true,
|
||||
)
|
||||
.map_err(anyhow::Error::msg)?;
|
||||
|
||||
assert_ne!(command.first(), Some(&".poc/bash".to_string()));
|
||||
assert_eq!(command.last(), Some(&"echo hello".to_string()));
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_get_command_respects_explicit_powershell_shell() -> anyhow::Result<()> {
|
||||
let json = r#"{"cmd": "echo hello", "shell": "powershell"}"#;
|
||||
|
||||
@@ -453,6 +453,18 @@ mod tests {
|
||||
assert_eq!(parsed, vec![vec!["ls".to_string()]]);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn extract_bash_command_rejects_shell_like_attacker_paths() {
|
||||
for shell in [".poc/bash", "/tmp/bash", "/tmp/bash.evil"] {
|
||||
let command = vec![
|
||||
shell.to_string(),
|
||||
"-lc".to_string(),
|
||||
"echo INNOCENT_COMMAND".to_string(),
|
||||
];
|
||||
assert_eq!(extract_bash_command(&command), None);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn accepts_concatenated_flag_and_value() {
|
||||
// Test case: -g"*.py" (flag directly concatenated with quoted value)
|
||||
|
||||
@@ -1260,7 +1260,7 @@ mod tests {
|
||||
let command = if cfg!(windows) {
|
||||
"C:\\windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe"
|
||||
} else {
|
||||
"/usr/local/bin/powershell.exe"
|
||||
"/usr/local/bin/pwsh"
|
||||
};
|
||||
|
||||
assert_parsed(
|
||||
|
||||
@@ -168,13 +168,23 @@ mod tests {
|
||||
let command = if cfg!(windows) {
|
||||
"C:\\windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe".to_string()
|
||||
} else {
|
||||
"/usr/local/bin/powershell.exe".to_string()
|
||||
"/usr/local/bin/pwsh".to_string()
|
||||
};
|
||||
let cmd = vec![command, "-Command".to_string(), "Write-Host hi".to_string()];
|
||||
let (_shell, script) = extract_powershell_command(&cmd).expect("extract");
|
||||
assert_eq!(script, "Write-Host hi");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn rejects_shell_like_attacker_paths() {
|
||||
let cmd = vec![
|
||||
r"C:\tmp\powershell.exe".to_string(),
|
||||
"-Command".to_string(),
|
||||
"Write-Host hi".to_string(),
|
||||
];
|
||||
assert_eq!(extract_powershell_command(&cmd), None);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn extracts_with_noprofile_and_alias() {
|
||||
let cmd = vec![
|
||||
|
||||
@@ -1,4 +1,3 @@
|
||||
use std::path::Path;
|
||||
use std::path::PathBuf;
|
||||
|
||||
#[derive(Debug, PartialEq, Eq, Clone, Copy)]
|
||||
@@ -11,22 +10,71 @@ pub(crate) enum ShellType {
|
||||
}
|
||||
|
||||
pub(crate) fn detect_shell_type(shell_path: &PathBuf) -> Option<ShellType> {
|
||||
match shell_path.as_os_str().to_str() {
|
||||
Some("zsh") => Some(ShellType::Zsh),
|
||||
Some("sh") => Some(ShellType::Sh),
|
||||
Some("cmd") => Some(ShellType::Cmd),
|
||||
Some("bash") => Some(ShellType::Bash),
|
||||
Some("pwsh") => Some(ShellType::PowerShell),
|
||||
Some("powershell") => Some(ShellType::PowerShell),
|
||||
_ => {
|
||||
let shell_name = shell_path.file_stem();
|
||||
if let Some(shell_name) = shell_name {
|
||||
let shell_name_path = Path::new(shell_name);
|
||||
if shell_name_path != Path::new(shell_path) {
|
||||
return detect_shell_type(&shell_name_path.to_path_buf());
|
||||
}
|
||||
}
|
||||
None
|
||||
let shell_text = shell_path.as_os_str().to_str()?;
|
||||
// Keep this exact: repo-local files named like shells must not inherit
|
||||
// shell-wrapper trust in approval or display decisions.
|
||||
match shell_text {
|
||||
"zsh" | "/bin/zsh" | "/usr/bin/zsh" | "/usr/local/bin/zsh" | "/opt/homebrew/bin/zsh" => {
|
||||
Some(ShellType::Zsh)
|
||||
}
|
||||
"sh" | "/bin/sh" | "/usr/bin/sh" => Some(ShellType::Sh),
|
||||
"bash"
|
||||
| "/bin/bash"
|
||||
| "/usr/bin/bash"
|
||||
| "/usr/local/bin/bash"
|
||||
| "/opt/homebrew/bin/bash" => Some(ShellType::Bash),
|
||||
"pwsh"
|
||||
| "powershell"
|
||||
| "pwsh.exe"
|
||||
| "powershell.exe"
|
||||
| "/usr/local/bin/pwsh"
|
||||
| "/usr/bin/pwsh"
|
||||
| "/bin/pwsh"
|
||||
| "/opt/homebrew/bin/pwsh" => Some(ShellType::PowerShell),
|
||||
"cmd" | "cmd.exe" => Some(ShellType::Cmd),
|
||||
_ => match shell_text.replace('\\', "/").to_ascii_lowercase().as_str() {
|
||||
"c:/windows/system32/cmd.exe" => Some(ShellType::Cmd),
|
||||
"c:/windows/system32/windowspowershell/v1.0/powershell.exe"
|
||||
| "c:/program files/powershell/7/pwsh.exe" => Some(ShellType::PowerShell),
|
||||
_ => None,
|
||||
},
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
#[test]
|
||||
fn detects_exact_shell_names_and_system_paths() {
|
||||
assert_eq!(
|
||||
detect_shell_type(&PathBuf::from("bash")),
|
||||
Some(ShellType::Bash)
|
||||
);
|
||||
assert_eq!(
|
||||
detect_shell_type(&PathBuf::from("/bin/bash")),
|
||||
Some(ShellType::Bash)
|
||||
);
|
||||
assert_eq!(
|
||||
detect_shell_type(&PathBuf::from("powershell.exe")),
|
||||
Some(ShellType::PowerShell)
|
||||
);
|
||||
assert_eq!(
|
||||
detect_shell_type(&PathBuf::from(
|
||||
r"C:\Windows\System32\WindowsPowerShell\v1.0\powershell.exe"
|
||||
)),
|
||||
Some(ShellType::PowerShell)
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn rejects_shell_like_attacker_controlled_paths() {
|
||||
assert_eq!(detect_shell_type(&PathBuf::from(".poc/bash")), None);
|
||||
assert_eq!(detect_shell_type(&PathBuf::from("/tmp/bash")), None);
|
||||
assert_eq!(detect_shell_type(&PathBuf::from("/tmp/bash.evil")), None);
|
||||
assert_eq!(
|
||||
detect_shell_type(&PathBuf::from(r"C:\tmp\powershell.exe")),
|
||||
None
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -82,6 +82,10 @@ mod tests {
|
||||
let args = vec!["/bin/bash".into(), "-lc".into(), "echo hello".into()];
|
||||
let cmdline = strip_bash_lc_and_escape(&args);
|
||||
assert_eq!(cmdline, "echo hello");
|
||||
|
||||
let args = vec![".poc/bash".into(), "-lc".into(), "echo hello".into()];
|
||||
let cmdline = strip_bash_lc_and_escape(&args);
|
||||
assert_eq!(cmdline, ".poc/bash -lc 'echo hello'");
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
||||
Reference in New Issue
Block a user