diff --git a/codex-rs/git-utils/src/git_command.rs b/codex-rs/git-utils/src/git_command.rs index 5edb011f3c..4491959515 100644 --- a/codex-rs/git-utils/src/git_command.rs +++ b/codex-rs/git-utils/src/git_command.rs @@ -1,7 +1,11 @@ use std::ffi::OsStr; use std::io; +#[cfg(windows)] +use std::path::Component; use std::path::Path; use std::path::PathBuf; +#[cfg(windows)] +use std::path::Prefix; use std::process::Command; use crate::errors::GitReadError; @@ -48,6 +52,13 @@ impl GitRunner { if !directory.is_absolute() { continue; } + // Check the PATH spelling before appending `git`. On Windows, + // PathBuf::push resolves `..` when its base uses a verbatim prefix, + // which would otherwise erase repository traversal before the + // first containment check. + if search_directory_is_untrusted(&directory, untrusted) { + continue; + } let candidate = directory.join(git_executable_name()); if path_is_untrusted(&candidate, untrusted) { continue; @@ -196,6 +207,73 @@ fn path_is_untrusted(path: &Path, locations: &UntrustedGitLocations) -> bool { .any(|common_dir| path_is_in_worktree_for_common_dir(path, common_dir)) } +fn search_directory_is_untrusted(directory: &Path, locations: &UntrustedGitLocations) -> bool { + #[cfg(windows)] + if windows_path_requires_fail_closed(directory) + || windows_path_has_untrusted_canonical_ancestor(directory, locations) + { + return true; + } + path_is_untrusted(directory, locations) +} + +#[cfg(windows)] +fn windows_path_requires_fail_closed(path: &Path) -> bool { + let mut components = path.components(); + let supported_namespace = match components.next() { + Some(Component::Prefix(prefix)) => match prefix.kind() { + Prefix::Disk(_) + | Prefix::VerbatimDisk(_) + | Prefix::UNC(_, _) + | Prefix::VerbatimUNC(_, _) => true, + Prefix::DeviceNS(device) => windows_device_namespace_is_filesystem(device), + Prefix::Verbatim(namespace) => namespace + .to_str() + .is_some_and(|namespace| namespace.eq_ignore_ascii_case("UNC")), + }, + _ => false, + }; + !supported_namespace || components.any(|component| matches!(component, Component::ParentDir)) +} + +#[cfg(windows)] +fn windows_device_namespace_is_filesystem(device: &OsStr) -> bool { + let bytes = device.as_encoded_bytes(); + bytes.eq_ignore_ascii_case(b"UNC") + || matches!(bytes, [drive, b':'] if drive.is_ascii_alphabetic()) +} + +#[cfg(windows)] +fn windows_path_has_untrusted_canonical_ancestor( + path: &Path, + locations: &UntrustedGitLocations, +) -> bool { + let Ok(canonical_path) = std::fs::canonicalize(path) else { + return true; + }; + if path_is_untrusted(&canonical_path, locations) { + return true; + } + for ancestor in path.ancestors().skip(1) { + let canonical_ancestor = match std::fs::canonicalize(ancestor) { + Ok(canonical_ancestor) => canonical_ancestor, + Err(error) + if matches!( + error.kind(), + io::ErrorKind::NotFound | io::ErrorKind::InvalidInput + ) => + { + continue; + } + Err(_) => return true, + }; + if path_is_untrusted(&canonical_ancestor, locations) { + return true; + } + } + false +} + fn path_is_in_worktree_for_common_dir(path: &Path, expected_common_dir: &Path) -> bool { let path = if path.is_dir() { path diff --git a/codex-rs/git-utils/src/git_command_tests.rs b/codex-rs/git-utils/src/git_command_tests.rs index 77807ba8d4..5ea99191d6 100644 --- a/codex-rs/git-utils/src/git_command_tests.rs +++ b/codex-rs/git-utils/src/git_command_tests.rs @@ -61,6 +61,22 @@ fn write_git_candidate(directory: &Path) { std::fs::write(candidate, b"git fixture").expect("write executable fixture"); } +#[cfg(windows)] +fn create_junction(path: &Path, target: &Path) { + let output = Command::new("cmd.exe") + .args(["/D", "/C", "mklink", "/J"]) + .arg(path) + .arg(target) + .output() + .expect("create junction"); + let stdout = String::from_utf8_lossy(&output.stdout); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + output.status.success(), + "mklink failed: stdout={stdout} stderr={stderr}" + ); +} + fn locations_for_root(root: &Path) -> UntrustedGitLocations { let mut roots = vec![root.to_path_buf()]; push_unique( @@ -73,6 +89,16 @@ fn locations_for_root(root: &Path) -> UntrustedGitLocations { } } +fn raw_parent_traversal(root: &Path, sibling: &str) -> PathBuf { + let separator = std::path::MAIN_SEPARATOR.to_string(); + let mut path = root.as_os_str().to_os_string(); + path.push(&separator); + path.push(".."); + path.push(&separator); + path.push(sibling); + path.into() +} + fn path_text(path: &Path) -> &str { path.to_str().expect("UTF-8 fixture path") } @@ -335,8 +361,31 @@ fn resolver_rejects_parent_traversal_spelled_through_repository() { let locations = locations_for_root(&repo); for root in &locations.roots { - let traversing_path = root.join("../trusted-bin"); - let search_path = std::env::join_paths([traversing_path]).expect("PATH"); + // Append without PathBuf::push: it resolves `..` when `root` has a + // verbatim Windows prefix, before the resolver can inspect the PATH + // spelling. + let traversing_path = raw_parent_traversal(root, "trusted-bin"); + let search_path = std::env::join_paths([&traversing_path]).expect("PATH"); + let split_paths = std::env::split_paths(&search_path).collect::>(); + assert_eq!(split_paths, vec![traversing_path.clone()]); + assert!( + search_directory_is_untrusted(&split_paths[0], &locations), + "raw PATH traversal was not rejected from {root:?}" + ); + + #[cfg(windows)] + if matches!( + root.components().next(), + Some(Component::Prefix(prefix)) if prefix.kind().is_verbatim() + ) { + assert_eq!( + split_paths[0].join(git_executable_name()), + std::fs::canonicalize(&trusted_bin) + .expect("canonical trusted bin") + .join(git_executable_name()) + ); + } + assert!( matches!( GitRunner::from_search_path(&locations, &search_path), @@ -347,6 +396,95 @@ fn resolver_rejects_parent_traversal_spelled_through_repository() { } } +#[cfg(windows)] +#[test] +fn resolver_rejects_parent_traversal_across_windows_namespaces() { + let traversing = [ + r"C:\Repo\..\outside", + r"\\?\C:\Repo\..\outside", + r"\\Server\Share\Repo\..\outside", + r"\\?\UNC\Server\Share\Repo\..\outside", + r"\\?\unc\Server\Share\Repo\..\outside", + r"\\.\C:\Repo\..\outside", + r"\\.\UNC\Server\Share\Repo\..\outside", + r"\\?\C:\RÉPO\..\outside", + ]; + for path in traversing { + assert!( + windows_path_requires_fail_closed(Path::new(path)), + "parent traversal was accepted: {path:?}" + ); + } + + let normalized_external = [ + r"C:\outside", + r"\\?\C:\outside", + r"\\Server\Share\outside", + r"\\?\UNC\Server\Share\outside", + r"\\?\unc\Server\Share\outside", + r"\\.\C:\outside", + r"\\.\UNC\Server\Share\outside", + ]; + for path in normalized_external { + assert!( + !windows_path_requires_fail_closed(Path::new(path)), + "normalized filesystem path was rejected: {path:?}" + ); + } +} + +#[cfg(windows)] +#[test] +fn resolver_rejects_unicode_case_alias_through_repository_junction() { + let fixture = tempfile::tempdir().expect("fixture"); + let repo = fixture.path().join("Répo"); + let outside = fixture.path().join("outside"); + let junction = repo.join("git-bin"); + std::fs::create_dir_all(&repo).expect("create repository"); + write_git_candidate(&outside); + create_junction(&junction, &outside); + + let case_alias = fixture.path().join("RÉPO").join("git-bin"); + let verbatim_case_alias = PathBuf::from(format!(r"\\?\{}", case_alias.display())); + assert_eq!( + std::fs::canonicalize(&verbatim_case_alias).expect("canonical alias"), + std::fs::canonicalize(&outside).expect("canonical outside") + ); + + let locations = locations_for_root(&repo); + assert!( + !path_is_untrusted(&verbatim_case_alias, &locations), + "fixture must exercise the Unicode alias before canonical ancestry" + ); + assert!(search_directory_is_untrusted( + &verbatim_case_alias, + &locations + )); + let search_path = std::env::join_paths([verbatim_case_alias]).expect("PATH"); + assert!(matches!( + GitRunner::from_search_path(&locations, &search_path), + Err(GitReadError::NoTrustedGit) + )); +} + +#[cfg(windows)] +#[test] +fn resolver_fails_closed_for_unsupported_windows_device_namespaces() { + let unsupported = [ + r"\\?\GLOBALROOT\Device\HarddiskVolumeShadowCopy1\git.exe", + r"\\?\Volume{11111111-1111-1111-1111-111111111111}\git.exe", + r"\\.\PhysicalDrive0", + r"\\.\pipe\codex-git", + ]; + + for path in unsupported { + assert!( + windows_path_requires_fail_closed(Path::new(path)), + "unsupported namespace was trusted: {path:?}" + ); + } +} + #[cfg(windows)] #[test] fn resolver_selects_native_git_exe_only() {