diff --git a/codex-rs/shell-command/src/command_safety/is_dangerous_command.rs b/codex-rs/shell-command/src/command_safety/is_dangerous_command.rs index 9e4bfe16a..3aa31c80f 100644 --- a/codex-rs/shell-command/src/command_safety/is_dangerous_command.rs +++ b/codex-rs/shell-command/src/command_safety/is_dangerous_command.rs @@ -53,6 +53,29 @@ fn is_git_global_option_with_inline_value(arg: &str) -> bool { ) || ((arg.starts_with("-C") || arg.starts_with("-c")) && arg.len() > 2) } +/// Git global options that can redirect config, repository, or helper lookup +/// and therefore must never be auto-approved as "safe". +pub(crate) fn git_global_option_requires_prompt(arg: &str) -> bool { + matches!( + arg, + "-c" | "--config-env" + | "--exec-path" + | "--git-dir" + | "--namespace" + | "--super-prefix" + | "--work-tree" + ) || matches!( + arg, + s if (s.starts_with("-c") && s.len() > 2) + || s.starts_with("--config-env=") + || s.starts_with("--exec-path=") + || s.starts_with("--git-dir=") + || s.starts_with("--namespace=") + || s.starts_with("--super-prefix=") + || s.starts_with("--work-tree=") + ) +} + pub(crate) fn executable_name_lookup_key(raw: &str) -> Option { #[cfg(windows)] { 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 b7349f220..2906fef27 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; +use crate::command_safety::is_dangerous_command::git_global_option_requires_prompt; use crate::command_safety::windows_safe_commands::is_safe_command_windows; pub fn is_known_safe_command(command: &[String]) -> bool { @@ -134,10 +135,10 @@ fn is_safe_to_call_with_exec(command: &[String]) -> bool { // Git Some("git") => { - // Global config overrides like `-c core.pager=...` can force git - // to execute arbitrary external commands. With no sandboxing, we - // should always prompt in those cases. - if git_has_config_override_global_option(command) { + // Global options that redirect config, repository, or helper + // lookup can make otherwise read-only git commands execute + // attacker-controlled code, so they must never be auto-approved. + if git_has_unsafe_global_option(command) { return false; } @@ -208,12 +209,12 @@ fn git_branch_is_read_only(branch_args: &[String]) -> bool { saw_read_only_flag } -fn git_has_config_override_global_option(command: &[String]) -> bool { - command.iter().map(String::as_str).any(|arg| { - matches!(arg, "-c" | "--config-env") - || (arg.starts_with("-c") && arg.len() > 2) - || arg.starts_with("--config-env=") - }) +fn git_has_unsafe_global_option(command: &[String]) -> bool { + command + .iter() + .skip(1) + .map(String::as_str) + .any(git_global_option_requires_prompt) } fn git_subcommand_args_are_read_only(args: &[String]) -> bool { @@ -356,7 +357,7 @@ mod tests { } #[test] - fn git_output_and_config_override_flags_are_not_safe() { + fn git_output_flags_are_not_safe() { assert!(!is_known_safe_command(&vec_str(&[ "git", "log", @@ -376,6 +377,10 @@ mod tests { "--output=/tmp/git-show-out-test", "HEAD", ]))); + } + + #[test] + fn git_global_override_flags_are_not_safe() { assert!(!is_known_safe_command(&vec_str(&[ "git", "-c", @@ -389,6 +394,32 @@ mod tests { "-ccore.pager=cat", "status", ]))); + + for args in [ + vec_str(&["git", "--config-env", "core.pager=PAGER", "show", "HEAD"]), + vec_str(&["git", "--config-env=core.pager=PAGER", "show", "HEAD"]), + vec_str(&["git", "--git-dir", ".evil-git", "diff", "HEAD~1..HEAD"]), + vec_str(&["git", "--git-dir=.evil-git", "diff", "HEAD~1..HEAD"]), + vec_str(&["git", "--work-tree", ".", "status"]), + vec_str(&["git", "--work-tree=.", "status"]), + vec_str(&["git", "--exec-path", ".git/helpers", "show", "HEAD"]), + vec_str(&["git", "--exec-path=.git/helpers", "show", "HEAD"]), + vec_str(&["git", "--namespace", "attacker", "show", "HEAD"]), + vec_str(&["git", "--namespace=attacker", "show", "HEAD"]), + vec_str(&["git", "--super-prefix", "attacker/", "show", "HEAD"]), + vec_str(&["git", "--super-prefix=attacker/", "show", "HEAD"]), + ] { + assert!( + !is_known_safe_command(&args), + "expected {args:?} to require approval due to unsafe git global option", + ); + } + + assert!(!is_known_safe_command(&vec_str(&[ + "bash", + "-lc", + "git --git-dir=.evil-git diff HEAD~1..HEAD", + ]))); } #[test] 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 ac479a4d2..aa7ff9681 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,3 +1,4 @@ +use crate::command_safety::is_dangerous_command::git_global_option_requires_prompt; use base64::Engine; use base64::engine::general_purpose::STANDARD as BASE64_STANDARD; use serde::Deserialize; @@ -305,33 +306,17 @@ fn is_safe_ripgrep(words: &[String]) -> bool { fn is_safe_git_command(words: &[String]) -> bool { const SAFE_SUBCOMMANDS: &[&str] = &["status", "log", "show", "diff", "cat-file"]; - let mut iter = words.iter().skip(1); - while let Some(arg) = iter.next() { + for arg in words.iter().skip(1) { let arg_lc = arg.to_ascii_lowercase(); if arg.starts_with('-') { - if arg.eq_ignore_ascii_case("-c") || arg.eq_ignore_ascii_case("--config") { - if iter.next().is_none() { - // Examples rejected here: "pwsh -Command 'git -c'" and "pwsh -Command 'git --config'". - return false; - } - continue; - } - - if arg_lc.starts_with("-c=") + if git_global_option_requires_prompt(&arg_lc) + || arg.eq_ignore_ascii_case("--config") || arg_lc.starts_with("--config=") - || arg_lc.starts_with("--git-dir=") - || arg_lc.starts_with("--work-tree=") { - continue; - } - - if arg.eq_ignore_ascii_case("--git-dir") || arg.eq_ignore_ascii_case("--work-tree") { - if iter.next().is_none() { - // Examples rejected here: "pwsh -Command 'git --git-dir'" and "pwsh -Command 'git --work-tree'". - return false; - } - continue; + // Examples rejected here: "pwsh -Command 'git --git-dir=.evil-git diff'" and + // "pwsh -Command 'git -c core.pager=cat show HEAD:foo.rs'". + return false; } continue; @@ -435,14 +420,6 @@ mod tests { "Get-Content foo.rs | Select-Object -Skip 200".to_string() ])); - assert!(is_safe_command_windows(&[ - pwsh.clone(), - "-NoLogo".to_string(), - "-NoProfile".to_string(), - "-Command".to_string(), - "git -c core.pager=cat show HEAD:foo.rs".to_string() - ])); - assert!(is_safe_command_windows(&[ pwsh.clone(), "-Command".to_string(), @@ -462,6 +439,41 @@ mod tests { ])); } + #[test] + fn rejects_git_global_override_options() { + let Some(pwsh) = try_find_pwsh_executable_blocking() else { + return; + }; + + let pwsh: String = pwsh.as_path().to_str().unwrap().into(); + for script in [ + "git -c core.pager=cat show HEAD:foo.rs", + "git --config-env core.pager=PAGER show HEAD:foo.rs", + "git --config-env=core.pager=PAGER show HEAD:foo.rs", + "git --git-dir .evil-git diff HEAD~1..HEAD", + "git --git-dir=.evil-git diff HEAD~1..HEAD", + "git --work-tree . status", + "git --work-tree=. status", + "git --exec-path .git/helpers show HEAD:foo.rs", + "git --exec-path=.git/helpers show HEAD:foo.rs", + "git --namespace attacker show HEAD:foo.rs", + "git --namespace=attacker show HEAD:foo.rs", + "git --super-prefix attacker/ show HEAD:foo.rs", + "git --super-prefix=attacker/ show HEAD:foo.rs", + ] { + assert!( + !is_safe_command_windows(&[ + pwsh.clone(), + "-NoLogo".to_string(), + "-NoProfile".to_string(), + "-Command".to_string(), + script.to_string(), + ]), + "expected {script:?} to require approval due to unsafe git global option", + ); + } + } + #[test] fn rejects_powershell_commands_with_side_effects() { assert!(!is_safe_command_windows(&vec_str(&[