mirror of
https://github.com/openai/codex.git
synced 2026-04-24 14:45:27 +00:00
[codex] Block unsafe git global options from safe allowlist (#15796)
## Summary - block git global options that can redirect config, repository, or helper lookup from being auto-approved as safe - share the unsafe global-option predicate across the Unix and Windows git safety checks - add regression coverage for inline and split forms, including `bash -lc` and PowerShell wrappers ## Root cause The Unix safe-command gate only rejected `-c` and `--config-env`, even though the shared git parser already knew how to skip additional pre-subcommand globals such as `--git-dir`, `--work-tree`, `--exec-path`, `--namespace`, and `--super-prefix`. That let those arguments slip through safe-command classification on otherwise read-only git invocations and bypass approval. The Windows-specific safe-command path had the same trust-boundary gap for git global options.
This commit is contained in:
@@ -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<String> {
|
||||
#[cfg(windows)]
|
||||
{
|
||||
|
||||
@@ -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]
|
||||
|
||||
@@ -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(&[
|
||||
|
||||
Reference in New Issue
Block a user