diff --git a/codex-rs/git-utils/src/errors.rs b/codex-rs/git-utils/src/errors.rs index 574cfc311a..04274135ec 100644 --- a/codex-rs/git-utils/src/errors.rs +++ b/codex-rs/git-utils/src/errors.rs @@ -50,6 +50,8 @@ pub(crate) enum GitReadError { InvalidRepositoryMetadata { path: PathBuf, reason: String }, #[error("{path:?} is not a Git repository")] NotRepository { path: PathBuf }, + #[error("invalid Git configuration environment: {reason}")] + InvalidConfigEnvironment { reason: String }, } impl GitReadError { @@ -59,7 +61,9 @@ impl GitReadError { Self::UnprovenPrimaryAuthority { .. } | Self::UnsafeRepositoryMetadata { .. } => { std::io::ErrorKind::PermissionDenied } - Self::InvalidRepositoryMetadata { .. } => std::io::ErrorKind::InvalidData, + Self::InvalidRepositoryMetadata { .. } | Self::InvalidConfigEnvironment { .. } => { + std::io::ErrorKind::InvalidData + } } } diff --git a/codex-rs/git-utils/src/git_command.rs b/codex-rs/git-utils/src/git_command.rs index 924ed32385..3cac863d21 100644 --- a/codex-rs/git-utils/src/git_command.rs +++ b/codex-rs/git-utils/src/git_command.rs @@ -5,6 +5,7 @@ use std::path::PathBuf; use std::process::Command; use crate::errors::GitReadError; +use crate::git_config_environment::GitConfigEnvironmentSnapshot; #[cfg(test)] use crate::git_executable::git_executable_name; use crate::git_executable::harden_git_launch_environment; @@ -30,6 +31,7 @@ pub(crate) struct GitRunner { argv0: PathBuf, safe_path: std::ffi::OsString, authority: RepositoryAuthority, + config_environment: GitConfigEnvironmentSnapshot, } /// A Git command that can only be spawned through [`GitRunner::output`], @@ -84,6 +86,7 @@ impl GitRunner { command.current_dir(parent); } harden_git_launch_environment(&mut command, &self.safe_path); + self.config_environment.apply_to(&mut command); GitCommand { inner: command } } @@ -117,12 +120,18 @@ impl GitRunner { ) -> Result { authority.ensure_primary_authority()?; let selected = select_git_executable(&authority, search_path)?; + let config_environment = GitConfigEnvironmentSnapshot::capture().map_err(|error| { + GitReadError::InvalidConfigEnvironment { + reason: error.to_string(), + } + })?; Ok(Self { executable: selected.executable, #[cfg(any(unix, test))] argv0: selected.argv0, safe_path: selected.safe_path, authority, + config_environment, }) } } diff --git a/codex-rs/git-utils/src/git_command_tests.rs b/codex-rs/git-utils/src/git_command_tests.rs index 2679f744fc..868ad6c097 100644 --- a/codex-rs/git-utils/src/git_command_tests.rs +++ b/codex-rs/git-utils/src/git_command_tests.rs @@ -52,6 +52,95 @@ fn run_git_stdout(cwd: &Path, args: &[&str]) -> String { .to_string() } +#[test] +fn runner_binds_config_environment_across_ambient_mutation() { + const CHILD: &str = "CODEX_GIT_CONFIG_BINDING_CHILD"; + if std::env::var_os(CHILD).is_none() { + let fixture = tempfile::tempdir().expect("fixture"); + let repo = fixture.path().join("repo"); + let safe_home = fixture.path().join("safe-home"); + let malicious_home = repo.join("malicious-home"); + std::fs::create_dir_all(&repo).expect("repo"); + std::fs::create_dir_all(&safe_home).expect("safe home"); + std::fs::create_dir_all(&malicious_home).expect("malicious home"); + run_git(&repo, &["init", "-q"]); + std::fs::write( + safe_home.join(".gitconfig"), + "[codex]\n\tbound = safe-home\n", + ) + .expect("safe home config"); + let malicious_global = repo.join("malicious.gitconfig"); + std::fs::write(&malicious_global, "[codex]\n\tbound = malicious\n") + .expect("malicious global config"); + std::fs::write( + malicious_home.join(".gitconfig"), + "[codex]\n\tbound = malicious-home\n", + ) + .expect("malicious home config"); + + let mut command = Command::new(std::env::current_exe().expect("test executable")); + isolate_git_command_environment(&mut command); + let output = command + .arg("git_command::tests::runner_binds_config_environment_across_ambient_mutation") + .arg("--exact") + .arg("--nocapture") + .env(CHILD, "1") + .env("RUST_TEST_THREADS", "1") + .env("CODEX_GIT_CONFIG_BINDING_ROOT", &repo) + .env("CODEX_GIT_CONFIG_BINDING_MALICIOUS", &malicious_global) + .env("CODEX_GIT_CONFIG_BINDING_MALICIOUS_HOME", &malicious_home) + .env("HOME", &safe_home) + .env("GIT_CONFIG_NOSYSTEM", "1") + .env("GIT_CONFIG_COUNT", "1") + .env("GIT_CONFIG_KEY_0", "codex.count") + .env("GIT_CONFIG_VALUE_0", "safe-count") + .env_remove("GIT_CONFIG_GLOBAL") + .env_remove("GIT_CONFIG_SYSTEM") + .env_remove("GIT_CONFIG_PARAMETERS") + .output() + .expect("isolated config binding test"); + assert!( + output.status.success(), + "isolated config binding test failed:\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + return; + } + + let root = PathBuf::from(std::env::var_os("CODEX_GIT_CONFIG_BINDING_ROOT").expect("root")); + let malicious = + std::env::var_os("CODEX_GIT_CONFIG_BINDING_MALICIOUS").expect("malicious config"); + let malicious_home = + std::env::var_os("CODEX_GIT_CONFIG_BINDING_MALICIOUS_HOME").expect("malicious home"); + let runner = GitRunner::for_cwd_io(&root).expect("runner with captured config environment"); + + // SAFETY: the parent starts an isolated test process that runs only this + // exact test with one harness thread. No other application thread reads or + // writes these variables, and the process exits after the assertions. + unsafe { + std::env::set_var("GIT_CONFIG_GLOBAL", malicious); + std::env::set_var("HOME", malicious_home); + std::env::set_var("GIT_CONFIG_VALUE_0", "malicious-count"); + std::env::set_var( + "GIT_CONFIG_PARAMETERS", + "'codex.count'='malicious-parameter'", + ); + } + + for (key, expected) in [("codex.bound", "safe-home"), ("codex.count", "safe-count")] { + let mut command = runner.command_for_cwd(&root).expect("bound command"); + command.args(["config", "--get", key]); + let output = runner.output(command).expect("bound config read"); + assert!( + output.status.success(), + "{key}: {}", + String::from_utf8_lossy(&output.stderr) + ); + assert_eq!(String::from_utf8_lossy(&output.stdout).trim(), expected); + } +} + fn commit_all(cwd: &Path, message: &str) { run_git( cwd, @@ -396,6 +485,12 @@ fn git_read_error_io_kind_table_is_exhaustive() { }, io::ErrorKind::InvalidData, ), + ( + GitReadError::InvalidConfigEnvironment { + reason: "malformed GIT_CONFIG_COUNT".to_string(), + }, + io::ErrorKind::InvalidData, + ), ] { assert_eq!(error.io_kind(), expected, "{error}"); assert_eq!(error.into_io_error().kind(), expected); diff --git a/codex-rs/git-utils/src/git_config_environment.rs b/codex-rs/git-utils/src/git_config_environment.rs new file mode 100644 index 0000000000..43d80a3afd --- /dev/null +++ b/codex-rs/git-utils/src/git_config_environment.rs @@ -0,0 +1,136 @@ +use std::collections::BTreeMap; +use std::ffi::OsStr; +use std::ffi::OsString; +use std::io; +use std::process::Command; + +const FIXED_CONFIG_ENVIRONMENT_KEYS: &[&str] = &[ + "GIT_CONFIG_GLOBAL", + "GIT_CONFIG_SYSTEM", + "GIT_CONFIG_NOSYSTEM", + "GIT_CONFIG_COUNT", + "GIT_CONFIG_PARAMETERS", + "HOME", + "XDG_CONFIG_HOME", + #[cfg(windows)] + "APPDATA", + #[cfg(windows)] + "PROGRAMDATA", + #[cfg(windows)] + "USERPROFILE", + #[cfg(windows)] + "HOMEDRIVE", + #[cfg(windows)] + "HOMEPATH", +]; + +// Git config command entries are an untrusted process input. Keep capture +// bounded rather than allocating attacker-selected amounts of memory before +// Git gets a chance to reject an unreasonable count. +const MAX_CONFIG_ENVIRONMENT_ENTRIES: usize = 1024; + +/// Exact config-relevant process environment bound to one +/// [`GitRunner`](crate::git_command::GitRunner). +/// +/// Every ordinary Git child receives these captured values (including explicit +/// removals for variables that were absent). Commands that intentionally +/// isolate a config source may override them after command construction. +pub(crate) struct GitConfigEnvironmentSnapshot { + entries: Box<[(OsString, Option)]>, +} + +impl std::fmt::Debug for GitConfigEnvironmentSnapshot { + fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + formatter + .debug_struct("GitConfigEnvironmentSnapshot") + .field("captured_entries", &self.entries.len()) + .finish() + } +} + +impl GitConfigEnvironmentSnapshot { + pub(crate) fn capture() -> io::Result { + let environment = std::env::vars_os().collect::>(); + Self::capture_from(|name| captured_environment_value(&environment, name)) + } + + fn capture_from(mut value_for: impl FnMut(&OsStr) -> Option) -> io::Result { + let mut entries = FIXED_CONFIG_ENVIRONMENT_KEYS + .iter() + .map(|name| (OsString::from(name), value_for(OsStr::new(name)))) + .collect::>(); + + if let Some(raw_count) = value_for(OsStr::new("GIT_CONFIG_COUNT")) { + let count = raw_count + .to_str() + .ok_or_else(|| invalid_environment("non-UTF-8 GIT_CONFIG_COUNT"))? + .parse::() + .map_err(|_| invalid_environment("invalid GIT_CONFIG_COUNT"))?; + if count > MAX_CONFIG_ENVIRONMENT_ENTRIES { + return Err(invalid_environment( + "GIT_CONFIG_COUNT exceeds the supported safety bound", + )); + } + for index in 0..count { + for prefix in ["GIT_CONFIG_KEY_", "GIT_CONFIG_VALUE_"] { + let name = OsString::from(format!("{prefix}{index}")); + let value = value_for(&name); + entries.push((name, value)); + } + } + } + + Ok(Self { + entries: entries.into_boxed_slice(), + }) + } + + pub(crate) fn apply_to(&self, command: &mut Command) { + for (name, value) in &self.entries { + match value { + Some(value) => { + command.env(name, value); + } + None => { + command.env_remove(name); + } + } + } + } + + pub(crate) fn value(&self, name: &str) -> Option<&OsStr> { + self.entries + .iter() + .find_map(|(candidate, value)| (candidate == name).then_some(value.as_deref())) + .flatten() + } +} + +#[cfg(not(windows))] +fn captured_environment_value( + environment: &BTreeMap, + name: &OsStr, +) -> Option { + environment.get(name).cloned() +} + +#[cfg(windows)] +fn captured_environment_value( + environment: &BTreeMap, + name: &OsStr, +) -> Option { + environment.iter().find_map(|(candidate, value)| { + candidate + .to_string_lossy() + .eq_ignore_ascii_case(&name.to_string_lossy()) + .then(|| value.clone()) + }) +} + +fn invalid_environment(message: &str) -> io::Error { + io::Error::new(io::ErrorKind::InvalidData, message) +} + +#[cfg(test)] +#[path = "git_config_environment_tests.rs"] +mod tests; diff --git a/codex-rs/git-utils/src/git_config_environment_tests.rs b/codex-rs/git-utils/src/git_config_environment_tests.rs new file mode 100644 index 0000000000..3cdec9a790 --- /dev/null +++ b/codex-rs/git-utils/src/git_config_environment_tests.rs @@ -0,0 +1,126 @@ +use super::*; +use pretty_assertions::assert_eq; + +#[test] +fn snapshot_binds_present_empty_and_absent_fixed_and_indexed_values() { + let values = BTreeMap::from([ + (OsString::from("GIT_CONFIG_GLOBAL"), OsString::from("")), + ( + OsString::from("GIT_CONFIG_PARAMETERS"), + OsString::from("'safe.parameter'='present'"), + ), + (OsString::from("GIT_CONFIG_NOSYSTEM"), OsString::from("1")), + (OsString::from("HOME"), OsString::from("/safe/home")), + ( + OsString::from("XDG_CONFIG_HOME"), + OsString::from("/safe/xdg"), + ), + (OsString::from("GIT_CONFIG_COUNT"), OsString::from("2")), + ( + OsString::from("GIT_CONFIG_KEY_0"), + OsString::from("safe.one"), + ), + ( + OsString::from("GIT_CONFIG_VALUE_0"), + OsString::from("present"), + ), + ( + OsString::from("GIT_CONFIG_KEY_1"), + OsString::from("safe.two"), + ), + ]); + let snapshot = GitConfigEnvironmentSnapshot::capture_from(|name| values.get(name).cloned()) + .expect("capture environment"); + + assert_eq!(snapshot.value("GIT_CONFIG_GLOBAL"), Some(OsStr::new(""))); + assert_eq!(snapshot.value("HOME"), Some(OsStr::new("/safe/home"))); + assert_eq!(snapshot.value("GIT_CONFIG_SYSTEM"), None); + assert_eq!(snapshot.value("GIT_CONFIG_NOSYSTEM"), Some(OsStr::new("1"))); + assert_eq!( + snapshot.value("GIT_CONFIG_PARAMETERS"), + Some(OsStr::new("'safe.parameter'='present'")) + ); + assert_eq!( + snapshot.value("GIT_CONFIG_KEY_0"), + Some(OsStr::new("safe.one")) + ); + assert_eq!(snapshot.value("GIT_CONFIG_VALUE_1"), None); + + let mut command = Command::new("git"); + command + .env("GIT_CONFIG_SYSTEM", "/later/system") + .env("GIT_CONFIG_VALUE_1", "later"); + snapshot.apply_to(&mut command); + let child_environment = command + .get_envs() + .map(|(name, value)| (name.to_owned(), value.map(OsStr::to_owned))) + .collect::>(); + assert_eq!( + child_environment.get(OsStr::new("GIT_CONFIG_SYSTEM")), + Some(&None) + ); + assert_eq!( + child_environment.get(OsStr::new("GIT_CONFIG_VALUE_1")), + Some(&None) + ); + assert_eq!( + child_environment.get(OsStr::new("HOME")), + Some(&Some(OsString::from("/safe/home"))) + ); +} + +#[test] +fn snapshot_rejects_malformed_or_unbounded_command_config_count() { + for count in [OsString::from("invalid"), OsString::from("1025")] { + let error = GitConfigEnvironmentSnapshot::capture_from(|name| { + (name == "GIT_CONFIG_COUNT").then(|| count.clone()) + }) + .expect_err("reject count"); + assert_eq!(error.kind(), io::ErrorKind::InvalidData); + } +} + +#[cfg(unix)] +#[test] +fn snapshot_preserves_non_utf8_config_path_bytes() { + use std::os::unix::ffi::OsStringExt; + + let raw = OsString::from_vec(vec![b'/', b't', b'm', b'p', b'/', 0xff]); + let snapshot = GitConfigEnvironmentSnapshot::capture_from(|name| { + (name == "GIT_CONFIG_GLOBAL").then(|| raw.clone()) + }) + .expect("capture non-UTF-8 path"); + assert_eq!(snapshot.value("GIT_CONFIG_GLOBAL"), Some(raw.as_os_str())); +} + +#[cfg(windows)] +#[test] +fn snapshot_matches_mixed_case_windows_environment_names() { + let environment = BTreeMap::from([ + (OsString::from("git_config_count"), OsString::from("1")), + ( + OsString::from("Git_Config_Key_0"), + OsString::from("safe.key"), + ), + ( + OsString::from("gIt_CoNfIg_VaLuE_0"), + OsString::from("safe-value"), + ), + (OsString::from("Home"), OsString::from(r"C:\safe-home")), + ]); + let snapshot = GitConfigEnvironmentSnapshot::capture_from(|name| { + captured_environment_value(&environment, name) + }) + .expect("capture mixed-case Windows environment"); + + assert_eq!(snapshot.value("GIT_CONFIG_COUNT"), Some(OsStr::new("1"))); + assert_eq!( + snapshot.value("GIT_CONFIG_KEY_0"), + Some(OsStr::new("safe.key")) + ); + assert_eq!( + snapshot.value("GIT_CONFIG_VALUE_0"), + Some(OsStr::new("safe-value")) + ); + assert_eq!(snapshot.value("HOME"), Some(OsStr::new(r"C:\safe-home"))); +} diff --git a/codex-rs/git-utils/src/lib.rs b/codex-rs/git-utils/src/lib.rs index 9309328e0b..e4dca00166 100644 --- a/codex-rs/git-utils/src/lib.rs +++ b/codex-rs/git-utils/src/lib.rs @@ -6,6 +6,7 @@ mod errors; mod fsmonitor; mod git_command; mod git_config; +mod git_config_environment; mod git_executable; mod info; mod local_only;