From add870a4bf226d87e0201b4e28ee6a159a49aed1 Mon Sep 17 00:00:00 2001 From: johnl-oai Date: Wed, 2 Sep 2026 18:40:47 +0000 Subject: [PATCH] Harden Windows sandbox provisioning file handling (#42342) ## Why Windows sandbox provisioning performs privileged work beneath a requested Codex home. Provisioning paths and output entries must remain bound to the files and directories that were validated, without following reparse points or writing through caller-controlled hard links. ## What changed - Add reusable no-reparse directory opens, path validation, directory guards, and handle-relative atomic file replacement. - Use fresh or retained handles for provisioning logs, sandbox credentials, setup markers, and error reports in service provisioning mode. - Add service-side validation for framed provisioning requests, proxy settings, fixed-drive Codex homes, and pinned sandbox directories. ## Testing Add Windows tests covering junction and unsafe-path rejection, hard-link-safe output replacement, retained output handles, request framing and normalization, and ancestor pinning. GitOrigin-RevId: 2ec4f0d15404c3387df1414bb0c1915cf452ecf2 --- codex-rs/Cargo.lock | 1 + .../src/bin/setup_main/win.rs | 73 +++++- .../src/bin/setup_main/win/no_reparse_dir.rs | 154 ----------- .../setup_main/win/no_reparse_dir_tests.rs | 64 ----- .../src/bin/setup_main/win/sandbox_users.rs | 54 +++- codex-rs/windows-sandbox-rs/src/file_write.rs | 149 +++++++++++ .../src/file_write_tests.rs | 70 +++++ codex-rs/windows-sandbox-rs/src/lib.rs | 16 ++ codex-rs/windows-sandbox-rs/src/logging.rs | 35 +++ .../windows-sandbox-rs/src/no_reparse_dir.rs | 245 ++++++++++++++++++ .../src/no_reparse_dir_tests.rs | 120 +++++++++ codex-rs/windows-sandbox-service/Cargo.toml | 1 + codex-rs/windows-sandbox-service/src/ipc.rs | 17 ++ .../windows-sandbox-service/src/ipc/home.rs | 158 +++++++++++ .../src/ipc/request.rs | 71 +++++ .../windows-sandbox-service/src/ipc_tests.rs | 225 ++++++++++++++++ 16 files changed, 1210 insertions(+), 243 deletions(-) delete mode 100644 codex-rs/windows-sandbox-rs/src/bin/setup_main/win/no_reparse_dir.rs delete mode 100644 codex-rs/windows-sandbox-rs/src/bin/setup_main/win/no_reparse_dir_tests.rs create mode 100644 codex-rs/windows-sandbox-rs/src/file_write.rs create mode 100644 codex-rs/windows-sandbox-rs/src/file_write_tests.rs create mode 100644 codex-rs/windows-sandbox-rs/src/no_reparse_dir.rs create mode 100644 codex-rs/windows-sandbox-rs/src/no_reparse_dir_tests.rs create mode 100644 codex-rs/windows-sandbox-service/src/ipc/home.rs create mode 100644 codex-rs/windows-sandbox-service/src/ipc/request.rs create mode 100644 codex-rs/windows-sandbox-service/src/ipc_tests.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index b5fc6347b4..dba3abd7a1 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -4977,6 +4977,7 @@ name = "codex-windows-sandbox-service" version = "0.0.0" dependencies = [ "anyhow", + "codex-windows-sandbox", "windows-sys 0.52.0", ] diff --git a/codex-rs/windows-sandbox-rs/src/bin/setup_main/win.rs b/codex-rs/windows-sandbox-rs/src/bin/setup_main/win.rs index bc8bdb506f..5baa3d676a 100644 --- a/codex-rs/windows-sandbox-rs/src/bin/setup_main/win.rs +++ b/codex-rs/windows-sandbox-rs/src/bin/setup_main/win.rs @@ -1,5 +1,4 @@ mod firewall; -mod no_reparse_dir; mod read_acl_mutex; use anyhow::Context; @@ -7,6 +6,7 @@ use anyhow::Result; use base64::Engine; use base64::engine::general_purpose::STANDARD as BASE64; use codex_otel::StatsigMetricsSettings; +use codex_windows_sandbox::DirectoryOpenDisposition; use codex_windows_sandbox::SETUP_VERSION; use codex_windows_sandbox::SetupErrorCode; use codex_windows_sandbox::SetupErrorReport; @@ -20,17 +20,21 @@ use codex_windows_sandbox::hide_newly_created_users; use codex_windows_sandbox::install_wfp_filters; use codex_windows_sandbox::log_note; use codex_windows_sandbox::log_writer; +use codex_windows_sandbox::open_directory_no_reparse; use codex_windows_sandbox::path_mask_allows; use codex_windows_sandbox::path_write_aces_need_refresh; use codex_windows_sandbox::resolve_sid; use codex_windows_sandbox::sandbox_bin_dir; use codex_windows_sandbox::sandbox_dir; use codex_windows_sandbox::sandbox_secrets_dir; +use codex_windows_sandbox::setup_error_path; +use codex_windows_sandbox::setup_log_writer; use codex_windows_sandbox::string_from_sid_bytes; use codex_windows_sandbox::sync_persistent_deny_read_acls; use codex_windows_sandbox::to_wide; use codex_windows_sandbox::workspace_write_cap_sid_for_root; use codex_windows_sandbox::workspace_write_root_overlaps_path; +use codex_windows_sandbox::write_file_atomically; use codex_windows_sandbox::write_setup_error_report; use serde::Deserialize; use serde::Serialize; @@ -66,6 +70,11 @@ use windows_sys::Win32::Storage::FileSystem::DELETE; use windows_sys::Win32::Storage::FileSystem::FILE_GENERIC_EXECUTE; use windows_sys::Win32::Storage::FileSystem::FILE_GENERIC_READ; use windows_sys::Win32::Storage::FileSystem::FILE_GENERIC_WRITE; +use windows_sys::Win32::Storage::FileSystem::FILE_SHARE_DELETE; +use windows_sys::Win32::Storage::FileSystem::FILE_SHARE_READ; +use windows_sys::Win32::Storage::FileSystem::FILE_SHARE_WRITE; +use windows_sys::Win32::Storage::FileSystem::READ_CONTROL; +use windows_sys::Win32::Storage::FileSystem::WRITE_DAC; const DENY_ACCESS: i32 = 3; #[cfg(test)] @@ -74,7 +83,6 @@ const WRITE_ROOT_ALLOW_MASK: u32 = mod sandbox_users; mod setup_runtime_bin; -use no_reparse_dir::open_or_create_no_reparse; use read_acl_mutex::acquire_read_acl_mutex; use read_acl_mutex::read_acl_mutex_exists; use sandbox_users::commit_setup_marker; @@ -320,7 +328,13 @@ fn lock_sandbox_dir( std::fs::create_dir_all(dir)?; None } - SetupMode::ProvisionOnly => Some(open_or_create_no_reparse(dir)?), + SetupMode::ProvisionOnly => Some(open_directory_no_reparse( + dir, + // SetSecurityInfo can reject a WRITE_DAC-only directory handle. + READ_CONTROL | WRITE_DAC, + FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, + DirectoryOpenDisposition::OpenOrCreate, + )?), }; let system_sid = resolve_sid("SYSTEM")?; let admins_sid = resolve_sid("Administrators")?; @@ -433,13 +447,16 @@ fn lock_sandbox_dir( } pub fn main() -> Result<()> { - let ret = real_main(); + let mut setup_mode = None; + let ret = real_main(&mut setup_mode); if let Err(e) = &ret { // Best-effort: log unexpected top-level errors. if let Ok(codex_home) = std::env::var("CODEX_HOME") { let sbx_dir = sandbox_dir(Path::new(&codex_home)); let _ = std::fs::create_dir_all(&sbx_dir); - if let Some(mut f) = log_writer(&sbx_dir) { + // An unparsed payload must not enable writes to an existing log. + let mode = setup_mode.unwrap_or(SetupMode::ProvisionOnly); + if let Ok(mut f) = open_setup_log(&sbx_dir, mode) { let _ = writeln!( f, "[{}] top-level error: {}", @@ -452,7 +469,18 @@ pub fn main() -> Result<()> { ret } -fn real_main() -> Result<()> { +fn open_setup_log(sbx_dir: &Path, mode: SetupMode) -> Result> { + match mode { + SetupMode::ProvisionOnly => Ok(Box::new(setup_log_writer(sbx_dir)?)), + SetupMode::Full | SetupMode::InteractiveProvision | SetupMode::ReadAclsOnly => { + log_writer(sbx_dir) + .map(|log| Box::new(log) as Box) + .context("open daily sandbox log") + } + } +} + +fn real_main(setup_mode: &mut Option) -> Result<()> { let mut args = std::env::args().collect::>(); if args.len() != 2 { return Err(anyhow::Error::new(SetupFailure::new( @@ -473,6 +501,7 @@ fn real_main() -> Result<()> { format!("failed to parse payload json: {err}"), )) })?; + *setup_mode = Some(payload.mode); if payload.version != SETUP_VERSION { return Err(anyhow::Error::new(SetupFailure::new( SetupErrorCode::HelperRequestArgsFailed, @@ -489,10 +518,10 @@ fn real_main() -> Result<()> { format!("failed to create sandbox dir {}: {err}", sbx_dir.display()), )) })?; - let mut log = log_writer(&sbx_dir).ok_or_else(|| { + let mut log = open_setup_log(&sbx_dir, payload.mode).map_err(|err| { anyhow::Error::new(SetupFailure::new( SetupErrorCode::HelperLogFailed, - format!("open log in {} failed", sbx_dir.display()), + format!("open log in {} failed: {err}", sbx_dir.display()), )) })?; let result = run_setup(&payload, &mut log, &sbx_dir); @@ -508,7 +537,17 @@ fn real_main() -> Result<()> { code: failure.code, message: failure.message, }; - if let Err(write_err) = write_setup_error_report(&payload.codex_home, &report) { + let write_report = match payload.mode { + SetupMode::ProvisionOnly => serde_json::to_vec_pretty(&report) + .map_err(anyhow::Error::from) + .and_then(|json| { + write_file_atomically(&setup_error_path(&payload.codex_home), &json) + }), + SetupMode::Full | SetupMode::InteractiveProvision | SetupMode::ReadAclsOnly => { + write_setup_error_report(&payload.codex_home, &report) + } + }; + if let Err(write_err) = write_report { let _ = log_line( &mut log, &format!("setup error report write failed: {write_err}"), @@ -524,9 +563,15 @@ fn real_main() -> Result<()> { fn run_setup(payload: &Payload, log: &mut dyn Write, sbx_dir: &Path) -> Result<()> { let writes_setup_marker = !payload.refresh_only && payload.mode != SetupMode::ReadAclsOnly; - if writes_setup_marker { - prepare_setup_marker(&payload.codex_home, &payload.real_user)?; - } + let marker = if writes_setup_marker { + Some(prepare_setup_marker( + &payload.codex_home, + &payload.real_user, + payload.mode, + )?) + } else { + None + }; match payload.mode { SetupMode::ReadAclsOnly => run_read_acl_only(payload, log), SetupMode::InteractiveProvision | SetupMode::ProvisionOnly => { @@ -534,8 +579,9 @@ fn run_setup(payload: &Payload, log: &mut dyn Write, sbx_dir: &Path) -> Result<( } SetupMode::Full => run_setup_full(payload, log, sbx_dir), }?; - if writes_setup_marker { + if let Some(marker) = marker { commit_setup_marker( + marker, &payload.codex_home, &payload.offline_username, &payload.online_username, @@ -619,6 +665,7 @@ fn provision_and_hide_sandbox_users( &payload.offline_username, &payload.online_username, log, + payload.mode, ); if let Err(err) = provision_result { if extract_setup_failure(&err).is_some() { diff --git a/codex-rs/windows-sandbox-rs/src/bin/setup_main/win/no_reparse_dir.rs b/codex-rs/windows-sandbox-rs/src/bin/setup_main/win/no_reparse_dir.rs deleted file mode 100644 index 93d983f12f..0000000000 --- a/codex-rs/windows-sandbox-rs/src/bin/setup_main/win/no_reparse_dir.rs +++ /dev/null @@ -1,154 +0,0 @@ -use anyhow::Context; -use anyhow::Result; -use anyhow::bail; -use anyhow::ensure; -use std::ffi::OsStr; -use std::ffi::c_void; -use std::mem::size_of; -use std::os::windows::ffi::OsStrExt; -use std::os::windows::io::FromRawHandle; -use std::os::windows::io::OwnedHandle; -use std::path::Component; -use std::path::Path; -use std::path::Prefix; -use std::ptr; -use windows_sys::Win32::Foundation::HANDLE; -use windows_sys::Win32::Foundation::INVALID_HANDLE_VALUE; -use windows_sys::Win32::Foundation::NTSTATUS; -use windows_sys::Win32::Foundation::RtlNtStatusToDosError; -use windows_sys::Win32::Foundation::UNICODE_STRING; -use windows_sys::Win32::Storage::FileSystem::FILE_ATTRIBUTE_NORMAL; -use windows_sys::Win32::Storage::FileSystem::FILE_SHARE_DELETE; -use windows_sys::Win32::Storage::FileSystem::FILE_SHARE_READ; -use windows_sys::Win32::Storage::FileSystem::FILE_SHARE_WRITE; -use windows_sys::Win32::Storage::FileSystem::READ_CONTROL; -use windows_sys::Win32::Storage::FileSystem::WRITE_DAC; -use windows_sys::Win32::System::IO::IO_STATUS_BLOCK; -use windows_sys::Win32::System::IO::IO_STATUS_BLOCK_0; -use windows_sys::Win32::System::Kernel::OBJ_CASE_INSENSITIVE; -use windows_sys::Win32::System::Kernel::OBJ_DONT_REPARSE; - -const FILE_OPEN_IF: u32 = 3; -const FILE_DIRECTORY_FILE: u32 = 1; -const STATUS_REPARSE_POINT_ENCOUNTERED: NTSTATUS = 0xC000_050B_u32 as i32; - -#[repr(C)] -struct ObjectAttributes { - length: u32, - root_directory: HANDLE, - object_name: *const UNICODE_STRING, - attributes: u32, - security_descriptor: *const c_void, - security_quality_of_service: *const c_void, -} - -#[link(name = "ntdll")] -unsafe extern "system" { - fn NtCreateFile( - file_handle: *mut HANDLE, - desired_access: u32, - object_attributes: *const ObjectAttributes, - io_status_block: *mut IO_STATUS_BLOCK, - allocation_size: *const i64, - file_attributes: u32, - share_access: u32, - create_disposition: u32, - create_options: u32, - ea_buffer: *const c_void, - ea_length: u32, - ) -> NTSTATUS; -} - -/// Opens or creates the final directory without following a reparse point in -/// any path component. -/// -/// Parent directories must already exist; only the final directory is created. -/// -/// The returned handle must remain open through any security mutation so the -/// mutation stays bound to the directory that passed this validation. -pub(super) fn open_or_create_no_reparse(path: &Path) -> Result { - ensure!( - path.is_absolute(), - "sandbox ACL path must be absolute: {}", - path.display() - ); - let source_offset = match path.components().next() { - Some(Component::Prefix(prefix)) if matches!(prefix.kind(), Prefix::Disk(_)) => 0, - Some(Component::Prefix(prefix)) if matches!(prefix.kind(), Prefix::VerbatimDisk(_)) => 4, - _ => bail!( - "sandbox ACL path must have a local disk prefix: {}", - path.display() - ), - }; - // NtCreateFile expects an NT namespace path. Convert only the local-disk - // spellings accepted by provisioning, without canonicalizing and following - // the reparse points that OBJ_DONT_REPARSE must reject. - let source: Vec = path.as_os_str().encode_wide().collect(); - let mut nt_path: Vec = OsStr::new("\\??\\").encode_wide().collect(); - for unit in source.into_iter().skip(source_offset) { - nt_path.push(if unit == b'/' as u16 { - b'\\' as u16 - } else { - unit - }); - } - nt_path.push(0); - let name_length = u16::try_from((nt_path.len() - 1) * size_of::()) - .context("sandbox ACL path is too long")?; - let maximum_length = - u16::try_from(nt_path.len() * size_of::()).context("sandbox ACL path is too long")?; - let object_name = UNICODE_STRING { - Length: name_length, - MaximumLength: maximum_length, - Buffer: nt_path.as_mut_ptr(), - }; - let object_attributes = ObjectAttributes { - length: size_of::() as u32, - root_directory: 0, - object_name: &object_name, - attributes: OBJ_CASE_INSENSITIVE as u32 | OBJ_DONT_REPARSE as u32, - security_descriptor: ptr::null(), - security_quality_of_service: ptr::null(), - }; - let mut io_status_block = IO_STATUS_BLOCK { - Anonymous: IO_STATUS_BLOCK_0 { Status: 0 }, - Information: 0, - }; - let mut handle = 0; - let status = unsafe { - // SetSecurityInfo can reject a WRITE_DAC-only directory handle. - NtCreateFile( - &mut handle, - READ_CONTROL | WRITE_DAC, - &object_attributes, - &mut io_status_block, - ptr::null(), - FILE_ATTRIBUTE_NORMAL, - FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, - FILE_OPEN_IF, - FILE_DIRECTORY_FILE, - ptr::null(), - /*ea_length*/ 0, - ) - }; - if status < 0 { - if status == STATUS_REPARSE_POINT_ENCOUNTERED { - bail!( - "sandbox ACL path contains a reparse point: {}", - path.display() - ); - } - let error = unsafe { RtlNtStatusToDosError(status) }; - return Err(std::io::Error::from_raw_os_error(error as i32)) - .with_context(|| format!("open sandbox ACL directory {}", path.display())); - } - ensure!( - handle != 0 && handle != INVALID_HANDLE_VALUE, - "NtCreateFile returned an invalid sandbox ACL directory handle" - ); - Ok(unsafe { OwnedHandle::from_raw_handle(handle as *mut c_void) }) -} - -#[cfg(test)] -#[path = "no_reparse_dir_tests.rs"] -mod tests; diff --git a/codex-rs/windows-sandbox-rs/src/bin/setup_main/win/no_reparse_dir_tests.rs b/codex-rs/windows-sandbox-rs/src/bin/setup_main/win/no_reparse_dir_tests.rs deleted file mode 100644 index 4d8f987f51..0000000000 --- a/codex-rs/windows-sandbox-rs/src/bin/setup_main/win/no_reparse_dir_tests.rs +++ /dev/null @@ -1,64 +0,0 @@ -use std::fs; -use std::path::Path; -use std::process::Command; - -use anyhow::Result; - -use super::open_or_create_no_reparse; - -fn create_directory_junction(target: &Path, alias: &Path) -> Result<()> { - let output = Command::new("cmd") - .args(["/C", "mklink", "/J"]) - .arg(alias) - .arg(target) - .output()?; - if !output.status.success() { - anyhow::bail!( - "mklink /J failed: stdout={} stderr={}", - String::from_utf8_lossy(&output.stdout).trim(), - String::from_utf8_lossy(&output.stderr).trim() - ); - } - Ok(()) -} - -#[test] -fn creates_and_opens_plain_directory_leaf() -> Result<()> { - let temporary = tempfile::tempdir()?; - let directory = temporary.path().join(".sandbox-bin"); - - drop(open_or_create_no_reparse(&directory)?); - let _handle = open_or_create_no_reparse(&directory)?; - - assert!(directory.is_dir()); - Ok(()) -} - -#[test] -fn rejects_final_directory_junction() -> Result<()> { - let temporary = tempfile::tempdir()?; - let target = temporary.path().join("target"); - let alias = temporary.path().join(".sandbox-bin"); - fs::create_dir(&target)?; - create_directory_junction(&target, &alias)?; - - let _ = - open_or_create_no_reparse(&alias).expect_err("final directory junction must be rejected"); - fs::remove_dir(&alias)?; - Ok(()) -} - -#[test] -fn rejects_ancestor_directory_junction() -> Result<()> { - let temporary = tempfile::tempdir()?; - let target_home = temporary.path().join("target-home"); - let alias_home = temporary.path().join("linked-home"); - fs::create_dir(&target_home)?; - fs::create_dir(target_home.join(".sandbox-bin"))?; - create_directory_junction(&target_home, &alias_home)?; - - let _ = open_or_create_no_reparse(&alias_home.join(".sandbox-bin")) - .expect_err("ancestor directory junction must be rejected"); - fs::remove_dir(&alias_home)?; - Ok(()) -} diff --git a/codex-rs/windows-sandbox-rs/src/bin/setup_main/win/sandbox_users.rs b/codex-rs/windows-sandbox-rs/src/bin/setup_main/win/sandbox_users.rs index da13115314..f3bc3f4a74 100644 --- a/codex-rs/windows-sandbox-rs/src/bin/setup_main/win/sandbox_users.rs +++ b/codex-rs/windows-sandbox-rs/src/bin/setup_main/win/sandbox_users.rs @@ -7,10 +7,11 @@ use rand::rngs::SmallRng; use serde::Serialize; use std::ffi::OsStr; use std::ffi::c_void; +use std::fs::File; use std::io::Write; +use std::os::windows::io::FromRawHandle; use std::path::Path; use std::path::PathBuf; -use windows_sys::Win32::Foundation::CloseHandle; use windows_sys::Win32::Foundation::ERROR_INSUFFICIENT_BUFFER; use windows_sys::Win32::Foundation::GENERIC_WRITE; use windows_sys::Win32::Foundation::GetLastError; @@ -48,6 +49,9 @@ use codex_windows_sandbox::sandbox_dir; use codex_windows_sandbox::sandbox_secrets_dir; use codex_windows_sandbox::string_from_sid_bytes; use codex_windows_sandbox::to_wide; +use codex_windows_sandbox::write_file_atomically; + +use super::SetupMode; const SID_USERS: &str = "S-1-5-32-545"; @@ -55,11 +59,12 @@ pub fn resolve_sandbox_users_group_sid() -> Result> { resolve_sid(SANDBOX_USERS_GROUP) } -pub fn provision_sandbox_users( +pub(super) fn provision_sandbox_users( codex_home: &Path, offline_username: &str, online_username: &str, log: &mut dyn Write, + mode: SetupMode, ) -> Result<()> { if let Err(err) = ensure_sandbox_users_group() { let message = format!("failed to create local group {SANDBOX_USERS_GROUP}: {err}"); @@ -83,6 +88,7 @@ pub fn provision_sandbox_users( &offline_password, online_username, &online_password, + mode, )?; Ok(()) } @@ -295,6 +301,7 @@ fn write_secrets( offline_pwd: &str, online_user: &str, online_pwd: &str, + mode: SetupMode, ) -> Result<()> { let secrets_dir = sandbox_secrets_dir(codex_home); std::fs::create_dir_all(&secrets_dir).map_err(|err| { @@ -336,7 +343,13 @@ fn write_secrets( format!("serialize sandbox users failed: {err}"), )) })?; - std::fs::write(&users_path, users_json).map_err(|err| { + let write_result = match mode { + SetupMode::ProvisionOnly => write_file_atomically(&users_path, &users_json), + SetupMode::Full | SetupMode::InteractiveProvision | SetupMode::ReadAclsOnly => { + std::fs::write(&users_path, users_json).map_err(anyhow::Error::from) + } + }; + write_result.map_err(|err| { anyhow::Error::new(SetupFailure::new( SetupErrorCode::HelperUsersFileWriteFailed, format!( @@ -348,11 +361,19 @@ fn write_secrets( Ok(()) } -// Create the final marker path with its protected ACL before provisioning begins. The empty file -// intentionally fails readiness checks while setup is in progress, and sandbox users cannot read, -// modify, or replace it. Once every setup step succeeds, `commit_setup_marker` writes the valid -// marker contents without changing the file's ACL. -pub(super) fn prepare_setup_marker(codex_home: &Path, real_user: &str) -> Result<()> { +/// Service provisioning retains the marker's exclusive handle; legacy setup reopens it at commit. +pub(super) enum PreparedSetupMarker { + Retained(File), + Reopen, +} + +// Create the final marker with its protected ACL. The empty file intentionally fails readiness +// checks until setup succeeds. Only service provisioning pins it for the entire operation. +pub(super) fn prepare_setup_marker( + codex_home: &Path, + real_user: &str, + mode: SetupMode, +) -> Result { let marker_path = sandbox_dir(codex_home).join("setup_marker.json"); match std::fs::remove_file(&marker_path) { Ok(()) => {} @@ -429,13 +450,18 @@ pub(super) fn prepare_setup_marker(codex_home: &Path, real_user: &str) -> Result ), ))); } - unsafe { - CloseHandle(marker_handle); + let file = unsafe { File::from_raw_handle(marker_handle as *mut c_void) }; + match mode { + SetupMode::ProvisionOnly => Ok(PreparedSetupMarker::Retained(file)), + SetupMode::Full | SetupMode::InteractiveProvision | SetupMode::ReadAclsOnly => { + drop(file); + Ok(PreparedSetupMarker::Reopen) + } } - Ok(()) } pub(super) fn commit_setup_marker( + file: PreparedSetupMarker, codex_home: &Path, offline_user: &str, online_user: &str, @@ -459,7 +485,11 @@ pub(super) fn commit_setup_marker( format!("serialize setup marker failed: {err}"), )) })?; - std::fs::write(&marker_path, marker_json).map_err(|err| { + let write_result = match file { + PreparedSetupMarker::Retained(mut file) => file.write_all(&marker_json), + PreparedSetupMarker::Reopen => std::fs::write(&marker_path, marker_json), + }; + write_result.map_err(|err| { anyhow::Error::new(SetupFailure::new( SetupErrorCode::HelperSetupMarkerWriteFailed, format!( diff --git a/codex-rs/windows-sandbox-rs/src/file_write.rs b/codex-rs/windows-sandbox-rs/src/file_write.rs new file mode 100644 index 0000000000..e8a2932bcf --- /dev/null +++ b/codex-rs/windows-sandbox-rs/src/file_write.rs @@ -0,0 +1,149 @@ +//! Writes helper outputs through fresh handles, replacing directory entries +//! without opening caller-supplied destination files or copying their ACLs. + +use anyhow::Context; +use anyhow::Result; +use rand::RngCore; +use std::ffi::OsStr; +use std::ffi::c_void; +use std::fs::File; +use std::io::Write; +use std::mem::offset_of; +use std::os::windows::ffi::OsStrExt; +use std::os::windows::io::AsRawHandle; +use std::os::windows::io::OwnedHandle; +use std::path::Path; +use windows_sys::Win32::Foundation::HANDLE; +use windows_sys::Win32::Foundation::NTSTATUS; +use windows_sys::Win32::Foundation::RtlNtStatusToDosError; +use windows_sys::Win32::Storage::FileSystem::DELETE; +use windows_sys::Win32::Storage::FileSystem::FILE_DISPOSITION_INFO; +use windows_sys::Win32::Storage::FileSystem::FILE_READ_ATTRIBUTES; +use windows_sys::Win32::Storage::FileSystem::FILE_RENAME_INFO; +use windows_sys::Win32::Storage::FileSystem::FILE_SHARE_READ; +use windows_sys::Win32::Storage::FileSystem::FILE_SHARE_WRITE; +use windows_sys::Win32::Storage::FileSystem::FILE_TRAVERSE; +use windows_sys::Win32::Storage::FileSystem::FILE_WRITE_DATA; +use windows_sys::Win32::Storage::FileSystem::FileDispositionInfo; +use windows_sys::Win32::Storage::FileSystem::SYNCHRONIZE; +use windows_sys::Win32::Storage::FileSystem::SetFileInformationByHandle; +use windows_sys::Win32::System::IO::IO_STATUS_BLOCK; +use windows_sys::Win32::System::IO::IO_STATUS_BLOCK_0; + +use crate::no_reparse_dir::DirectoryOpenDisposition; +use crate::no_reparse_dir::open_directory_no_reparse; +use crate::no_reparse_dir::open_no_reparse; +use crate::no_reparse_dir::validate_local_directory_path; + +const FILE_CREATE: u32 = 2; +const FILE_SYNCHRONOUS_IO_NONALERT: u32 = 0x20; +const FILE_NON_DIRECTORY_FILE: u32 = 0x40; +const FILE_RENAME_INFORMATION: i32 = 10; + +#[link(name = "ntdll")] +unsafe extern "system" { + fn NtSetInformationFile( + file_handle: HANDLE, + io_status_block: *mut IO_STATUS_BLOCK, + file_information: *const c_void, + length: u32, + file_information_class: i32, + ) -> NTSTATUS; +} + +/// Replaces an output with a fresh file that inherits its parent directory's ACL. +/// The destination is never opened for writing, even if replacement fails. +pub fn write_file_atomically(path: &Path, contents: &[u8]) -> Result<()> { + validate_local_directory_path(path)?; + let parent = path + .parent() + .context("output must have a parent directory")?; + let name: Vec = path + .file_name() + .context("output must have a file name")? + .encode_wide() + .collect(); + let (mut file, directory) = create_temporary_file(parent, ".tmp")?; + let result: Result<()> = (|| { + file.write_all(contents)?; + + // FILE_RENAME_INFO ends in a variable-length UTF-16 name. Allocate + // enough aligned storage for both the fixed header and that name. + let size = size_of::() + size_of_val(name.as_slice()); + let mut buffer = vec![0_usize; size.div_ceil(size_of::())]; + let info = buffer.as_mut_ptr().cast::(); + let mut io_status = IO_STATUS_BLOCK { + Anonymous: IO_STATUS_BLOCK_0 { Status: 0 }, + Information: 0, + }; + unsafe { + (*info).Anonymous.ReplaceIfExists = 1; + (*info).RootDirectory = directory.as_raw_handle() as HANDLE; + (*info).FileNameLength = u32::try_from(size_of_val(name.as_slice()))?; + let filename = buffer + .as_mut_ptr() + .cast::() + .add(offset_of!(FILE_RENAME_INFO, FileName)) + .cast::(); + std::ptr::copy_nonoverlapping(name.as_ptr(), filename, name.len()); + // The Win32 wrapper rejects RootDirectory on some Windows versions. + // The native rename accepts the pinned parent and has the same layout. + let status = NtSetInformationFile( + file.as_raw_handle() as HANDLE, + &mut io_status, + info.cast(), + u32::try_from(size)?, + FILE_RENAME_INFORMATION, + ); + if status < 0 { + return Err( + std::io::Error::from_raw_os_error(RtlNtStatusToDosError(status) as i32).into(), + ); + } + } + Ok(()) + })(); + if result.is_err() { + // Clean up only the file we created, not a pathname the caller could + // have replaced. Preserve the original write/rename error. + let disposition = FILE_DISPOSITION_INFO { DeleteFile: 1 }; + unsafe { + SetFileInformationByHandle( + file.as_raw_handle() as HANDLE, + FileDispositionInfo, + (&raw const disposition).cast(), + size_of::() as u32, + ); + } + } + result.with_context(|| format!("replace output {}", path.display())) +} + +pub(crate) fn create_temporary_file(parent: &Path, suffix: &str) -> Result<(File, OwnedHandle)> { + let directory = open_directory_no_reparse( + parent, + FILE_TRAVERSE | FILE_READ_ATTRIBUTES, + FILE_SHARE_READ | FILE_SHARE_WRITE, + DirectoryOpenDisposition::OpenExisting, + )?; + let mut random = [0_u8; 8]; + rand::rngs::OsRng + .try_fill_bytes(&mut random) + .context("generate sandbox output name")?; + let id = u64::from_le_bytes(random); + let name = format!("sandbox-{id:016x}{suffix}"); + let mut name: Vec = OsStr::new(&name).encode_wide().chain(Some(0)).collect(); + let file = open_no_reparse( + directory.as_raw_handle() as HANDLE, + &mut name, + FILE_WRITE_DATA | DELETE | SYNCHRONIZE, + FILE_SHARE_READ, + FILE_CREATE, + FILE_NON_DIRECTORY_FILE | FILE_SYNCHRONOUS_IO_NONALERT, + )?; + Ok((File::from(file), directory)) +} + +#[cfg(test)] +#[path = "file_write_tests.rs"] +mod tests; diff --git a/codex-rs/windows-sandbox-rs/src/file_write_tests.rs b/codex-rs/windows-sandbox-rs/src/file_write_tests.rs new file mode 100644 index 0000000000..6711c8050c --- /dev/null +++ b/codex-rs/windows-sandbox-rs/src/file_write_tests.rs @@ -0,0 +1,70 @@ +//! Checks fresh output handles and safe replacement of caller-controlled entries. + +use super::create_temporary_file; +use super::write_file_atomically; +use anyhow::Result; +use pretty_assertions::assert_eq; +use std::fs; +use std::fs::OpenOptions; +use std::io::Write; +use std::os::windows::fs::OpenOptionsExt; +use windows_sys::Win32::Storage::FileSystem::FILE_SHARE_READ; +use windows_sys::Win32::Storage::FileSystem::FILE_SHARE_WRITE; + +#[test] +fn replaces_hard_link_without_writing_to_its_target() -> Result<()> { + let temp = tempfile::tempdir()?; + let victim = temp.path().join("victim"); + let output = temp.path().join("sandbox_users.json"); + fs::write(&victim, b"unchanged")?; + fs::hard_link(&victim, &output)?; + + write_file_atomically(&output, b"new contents")?; + + assert_eq!(fs::read(&victim)?, b"unchanged"); + assert_eq!(fs::read(&output)?, b"new contents"); + assert_eq!(fs::read_dir(temp.path())?.count(), 2); + Ok(()) +} + +#[test] +fn failed_replacement_does_not_truncate_target_or_leave_temporary_file() -> Result<()> { + let temp = tempfile::tempdir()?; + let victim = temp.path().join("victim"); + let output = temp.path().join("setup_error.json"); + fs::write(&victim, b"unchanged")?; + fs::hard_link(&victim, &output)?; + let _held = OpenOptions::new() + .read(true) + .share_mode(FILE_SHARE_READ | FILE_SHARE_WRITE) + .open(&output)?; + + assert!(write_file_atomically(&output, b"new contents").is_err()); + + assert_eq!(fs::read(&victim)?, b"unchanged"); + assert_eq!(fs::read(&output)?, b"unchanged"); + assert_eq!(fs::read_dir(temp.path())?.count(), 2); + Ok(()) +} + +#[test] +fn fresh_output_cannot_be_written_or_replaced_while_held() -> Result<()> { + let temp = tempfile::tempdir()?; + let (mut file, _directory) = create_temporary_file(temp.path(), ".log")?; + let path = fs::read_dir(temp.path())?.next().unwrap()?.path(); + + assert!( + OpenOptions::new() + .write(true) + .truncate(true) + .open(&path) + .is_err() + ); + assert!(fs::remove_file(&path).is_err()); + assert!(fs::rename(&path, temp.path().join("moved")).is_err()); + file.write_all(b"log contents")?; + drop(file); + + assert_eq!(fs::read(path)?, b"log contents"); + Ok(()) +} diff --git a/codex-rs/windows-sandbox-rs/src/lib.rs b/codex-rs/windows-sandbox-rs/src/lib.rs index e48063aa79..d4c23b2dab 100644 --- a/codex-rs/windows-sandbox-rs/src/lib.rs +++ b/codex-rs/windows-sandbox-rs/src/lib.rs @@ -69,6 +69,8 @@ mod dpapi; #[cfg(target_os = "windows")] mod env; #[cfg(target_os = "windows")] +mod file_write; +#[cfg(target_os = "windows")] mod framed_io; #[cfg(target_os = "windows")] mod helper_materialization; @@ -79,6 +81,8 @@ mod identity; #[cfg(target_os = "windows")] mod logging; #[cfg(target_os = "windows")] +mod no_reparse_dir; +#[cfg(target_os = "windows")] mod path_normalization; #[cfg(target_os = "windows")] mod process; @@ -195,6 +199,8 @@ pub use elevated_impl::ElevatedSandboxProfileCaptureRequest; #[cfg(target_os = "windows")] pub use elevated_impl::run_windows_sandbox_capture_for_permission_profile as run_windows_sandbox_capture_for_permission_profile_elevated; #[cfg(target_os = "windows")] +pub use file_write::write_file_atomically; +#[cfg(target_os = "windows")] pub use helper_materialization::resolve_current_exe_for_launch; #[cfg(target_os = "windows")] pub use helper_materialization::resolve_exe_for_launch; @@ -249,6 +255,16 @@ pub use logging::log_note; #[cfg(target_os = "windows")] pub use logging::log_writer; #[cfg(target_os = "windows")] +pub use logging::setup_log_writer; +#[cfg(target_os = "windows")] +pub use no_reparse_dir::DirectoryOpenDisposition; +#[cfg(target_os = "windows")] +pub use no_reparse_dir::create_directory_guard; +#[cfg(target_os = "windows")] +pub use no_reparse_dir::open_directory_no_reparse; +#[cfg(target_os = "windows")] +pub use no_reparse_dir::validate_local_directory_path; +#[cfg(target_os = "windows")] pub use path_normalization::canonicalize_path; #[cfg(target_os = "windows")] pub use process::ConsoleMode; diff --git a/codex-rs/windows-sandbox-rs/src/logging.rs b/codex-rs/windows-sandbox-rs/src/logging.rs index 7108a1de36..34527182cd 100644 --- a/codex-rs/windows-sandbox-rs/src/logging.rs +++ b/codex-rs/windows-sandbox-rs/src/logging.rs @@ -1,6 +1,11 @@ +use anyhow::Context; +use anyhow::Result; +use std::fs::File; use std::io::Write; +use std::os::windows::io::OwnedHandle; use std::path::Path; use std::path::PathBuf; +use std::sync::Mutex; use std::sync::OnceLock; use codex_utils_string::take_bytes_at_char_boundary; @@ -12,6 +17,10 @@ pub const LOG_FILE_PREFIX: &str = "sandbox"; pub const LOG_FILE_SUFFIX: &str = "log"; pub const MAX_LOG_FILES: usize = 90; +// Service provisioning never appends to a caller-controlled existing log entry. +// An initialized but empty slot also prevents falling back after an open failure. +static SETUP_LOG: OnceLock>> = OnceLock::new(); + fn exe_label() -> &'static str { static LABEL: OnceLock = OnceLock::new(); LABEL.get_or_init(|| { @@ -61,7 +70,33 @@ pub fn log_writer(base_dir: &Path) -> Option { .ok() } +/// Opens one fresh log for service provisioning and retains its parent directory. +/// Subsequent setup diagnostics share this handle instead of reopening a pathname. +pub fn setup_log_writer(base_dir: &Path) -> Result { + let mut log = SETUP_LOG + .get_or_init(|| Mutex::new(None)) + .lock() + .map_err(|_| anyhow::anyhow!("setup log lock poisoned"))?; + match log.as_ref() { + Some((file, _parent)) => file.try_clone().context("clone setup log"), + None => { + let (file, parent) = crate::file_write::create_temporary_file(base_dir, ".log")?; + let writer = file.try_clone().context("clone setup log")?; + *log = Some((file, parent)); + Ok(writer) + } + } +} + fn append_line(line: &str, base_dir: Option<&Path>) { + if let Some(log) = SETUP_LOG.get() { + if let Ok(mut log) = log.lock() + && let Some((file, _parent)) = log.as_mut() + { + let _ = writeln!(file, "{line}"); + } + return; + } if let Some(dir) = base_dir && let Some(mut f) = log_writer(dir) { diff --git a/codex-rs/windows-sandbox-rs/src/no_reparse_dir.rs b/codex-rs/windows-sandbox-rs/src/no_reparse_dir.rs new file mode 100644 index 0000000000..da8cb2de0e --- /dev/null +++ b/codex-rs/windows-sandbox-rs/src/no_reparse_dir.rs @@ -0,0 +1,245 @@ +//! Opens local directories without traversing reparse points. Callers choose +//! access, sharing, and creation behavior and retain handles for their operation. +//! Handle-relative guard files keep pinned directories from becoming reparses. + +use anyhow::Context; +use anyhow::Result; +use anyhow::bail; +use anyhow::ensure; +use rand::RngCore; +use std::ffi::OsStr; +use std::ffi::c_void; +use std::mem::size_of; +use std::mem::size_of_val; +use std::os::windows::ffi::OsStrExt; +use std::os::windows::io::AsRawHandle; +use std::os::windows::io::BorrowedHandle; +use std::os::windows::io::FromRawHandle; +use std::os::windows::io::OwnedHandle; +use std::path::Component; +use std::path::Path; +use std::path::Prefix; +use std::ptr; +use windows_sys::Win32::Foundation::HANDLE; +use windows_sys::Win32::Foundation::INVALID_HANDLE_VALUE; +use windows_sys::Win32::Foundation::NTSTATUS; +use windows_sys::Win32::Foundation::RtlNtStatusToDosError; +use windows_sys::Win32::Foundation::UNICODE_STRING; +use windows_sys::Win32::Storage::FileSystem::DELETE; +use windows_sys::Win32::Storage::FileSystem::FILE_ATTRIBUTE_NORMAL; +use windows_sys::Win32::Storage::FileSystem::FILE_READ_DATA; +use windows_sys::Win32::Storage::FileSystem::FILE_SHARE_READ; +use windows_sys::Win32::Storage::FileSystem::FILE_SHARE_WRITE; +use windows_sys::Win32::System::IO::IO_STATUS_BLOCK; +use windows_sys::Win32::System::IO::IO_STATUS_BLOCK_0; +use windows_sys::Win32::System::Kernel::OBJ_CASE_INSENSITIVE; +use windows_sys::Win32::System::Kernel::OBJ_DONT_REPARSE; + +const FILE_OPEN: u32 = 1; +const FILE_CREATE: u32 = 2; +const FILE_OPEN_IF: u32 = 3; +const FILE_DIRECTORY_FILE: u32 = 1; +const FILE_NON_DIRECTORY_FILE: u32 = 0x40; +const FILE_DELETE_ON_CLOSE: u32 = 0x1000; +const STATUS_REPARSE_POINT_ENCOUNTERED: NTSTATUS = 0xC000_050B_u32 as i32; + +#[repr(C)] +struct ObjectAttributes { + length: u32, + root_directory: HANDLE, + object_name: *const UNICODE_STRING, + attributes: u32, + security_descriptor: *const c_void, + security_quality_of_service: *const c_void, +} + +#[link(name = "ntdll")] +unsafe extern "system" { + fn NtCreateFile( + file_handle: *mut HANDLE, + desired_access: u32, + object_attributes: *const ObjectAttributes, + io_status_block: *mut IO_STATUS_BLOCK, + allocation_size: *const i64, + file_attributes: u32, + share_access: u32, + create_disposition: u32, + create_options: u32, + ea_buffer: *const c_void, + ea_length: u32, + ) -> NTSTATUS; +} + +/// Whether a no-reparse open may create the final directory. +#[derive(Clone, Copy, Debug)] +pub enum DirectoryOpenDisposition { + OpenExisting, + OpenOrCreate, +} + +/// Validates an absolute local-drive directory path without accessing the filesystem. +/// +/// Relative components and alternate data streams are not directory targets. +pub fn validate_local_directory_path(path: &Path) -> Result<()> { + ensure!( + path.is_absolute(), + "directory path must be absolute: {}", + path.display() + ); + let mut components = path.components(); + match components.next() { + Some(Component::Prefix(prefix)) + if matches!(prefix.kind(), Prefix::Disk(_) | Prefix::VerbatimDisk(_)) => {} + _ => bail!( + "directory path must have a local disk prefix: {}", + path.display() + ), + } + for component in components { + match component { + Component::RootDir => {} + Component::Normal(name) if !name.to_string_lossy().contains(':') => {} + Component::Normal(_) => bail!("directory path cannot contain alternate data streams"), + Component::ParentDir | Component::CurDir | Component::Prefix(_) => { + bail!("directory path cannot contain relative or nested prefix components"); + } + } + } + Ok(()) +} + +/// Opens a directory without following a reparse point in any path component. +/// Parent directories must exist even when creating the final directory. +/// +/// Retain the handle through handle-based security mutations. To also prevent +/// renaming or deleting the directory, omit `FILE_SHARE_DELETE` and request +/// access that participates in sharing checks, such as `FILE_TRAVERSE`. +pub fn open_directory_no_reparse( + path: &Path, + desired_access: u32, + share_access: u32, + disposition: DirectoryOpenDisposition, +) -> Result { + validate_local_directory_path(path)?; + let source_offset = if matches!( + path.components().next(), + Some(Component::Prefix(prefix)) if matches!(prefix.kind(), Prefix::VerbatimDisk(_)) + ) { + 4 + } else { + 0 + }; + // NtCreateFile expects an NT namespace path. Convert only the local-disk + // spellings accepted by provisioning, without canonicalizing and following + // the reparse points that OBJ_DONT_REPARSE must reject. + let source: Vec = path.as_os_str().encode_wide().collect(); + let mut nt_path: Vec = OsStr::new("\\??\\").encode_wide().collect(); + for unit in source.into_iter().skip(source_offset) { + nt_path.push(if unit == b'/' as u16 { + b'\\' as u16 + } else { + unit + }); + } + nt_path.push(0); + open_no_reparse( + /*root_directory*/ 0, + &mut nt_path, + desired_access, + share_access, + match disposition { + DirectoryOpenDisposition::OpenExisting => FILE_OPEN, + DirectoryOpenDisposition::OpenOrCreate => FILE_OPEN_IF, + }, + FILE_DIRECTORY_FILE, + ) + .with_context(|| format!("open directory {}", path.display())) +} + +/// Keeps a directory nonempty, preventing in-place reparse conversion while the +/// returned handle is held. The fresh file cannot be renamed or deleted by other +/// opens and uses delete-on-close cleanup after the guard and its duplicates close. +/// +/// Creation is relative to the pinned directory, never to its mutable pathname. +/// The caller must still validate that pathname after installing the guard and +/// retain its directory pins alongside the guard throughout privileged work. +pub fn create_directory_guard(directory: BorrowedHandle<'_>) -> Result { + let mut random = [0_u8; 8]; + rand::rngs::OsRng + .try_fill_bytes(&mut random) + .context("generate sandbox directory guard name")?; + let guard_id = u64::from_le_bytes(random); + let name = format!(".codex-provisioning-{guard_id:016x}.guard"); + let mut name: Vec = OsStr::new(&name).encode_wide().chain(Some(0)).collect(); + open_no_reparse( + directory.as_raw_handle() as HANDLE, + &mut name, + FILE_READ_DATA | DELETE, + FILE_SHARE_READ | FILE_SHARE_WRITE, + FILE_CREATE, + FILE_NON_DIRECTORY_FILE | FILE_DELETE_ON_CLOSE, + ) + .context("create sandbox directory guard") +} + +pub(crate) fn open_no_reparse( + root_directory: HANDLE, + name: &mut [u16], + desired_access: u32, + share_access: u32, + disposition: u32, + create_options: u32, +) -> Result { + let name_length = u16::try_from((name.len() - 1) * size_of::()) + .context("no-reparse path is too long")?; + let maximum_length = u16::try_from(size_of_val(name)).context("no-reparse path is too long")?; + let object_name = UNICODE_STRING { + Length: name_length, + MaximumLength: maximum_length, + Buffer: name.as_mut_ptr(), + }; + let object_attributes = ObjectAttributes { + length: size_of::() as u32, + root_directory, + object_name: &object_name, + attributes: OBJ_CASE_INSENSITIVE as u32 | OBJ_DONT_REPARSE as u32, + security_descriptor: ptr::null(), + security_quality_of_service: ptr::null(), + }; + let mut io_status_block = IO_STATUS_BLOCK { + Anonymous: IO_STATUS_BLOCK_0 { Status: 0 }, + Information: 0, + }; + let mut handle = 0; + let status = unsafe { + NtCreateFile( + &mut handle, + desired_access, + &object_attributes, + &mut io_status_block, + ptr::null(), + FILE_ATTRIBUTE_NORMAL, + share_access, + disposition, + create_options, + ptr::null(), + /*ea_length*/ 0, + ) + }; + if status < 0 { + if status == STATUS_REPARSE_POINT_ENCOUNTERED { + bail!("path contains a reparse point"); + } + let error = unsafe { RtlNtStatusToDosError(status) }; + return Err(std::io::Error::from_raw_os_error(error as i32)).context("NtCreateFile"); + } + ensure!( + handle != 0 && handle != INVALID_HANDLE_VALUE, + "NtCreateFile returned an invalid handle" + ); + Ok(unsafe { OwnedHandle::from_raw_handle(handle as *mut c_void) }) +} + +#[cfg(test)] +#[path = "no_reparse_dir_tests.rs"] +mod tests; diff --git a/codex-rs/windows-sandbox-rs/src/no_reparse_dir_tests.rs b/codex-rs/windows-sandbox-rs/src/no_reparse_dir_tests.rs new file mode 100644 index 0000000000..829552e577 --- /dev/null +++ b/codex-rs/windows-sandbox-rs/src/no_reparse_dir_tests.rs @@ -0,0 +1,120 @@ +use std::fs; +use std::os::windows::io::OwnedHandle; +use std::path::Path; +use std::process::Command; +use windows_sys::Win32::Storage::FileSystem::FILE_SHARE_DELETE; +use windows_sys::Win32::Storage::FileSystem::FILE_SHARE_READ; +use windows_sys::Win32::Storage::FileSystem::FILE_SHARE_WRITE; +use windows_sys::Win32::Storage::FileSystem::READ_CONTROL; +use windows_sys::Win32::Storage::FileSystem::WRITE_DAC; + +use anyhow::Result; + +use super::DirectoryOpenDisposition; +use super::open_directory_no_reparse; +use super::validate_local_directory_path; + +fn open_for_acl(path: &Path, disposition: DirectoryOpenDisposition) -> Result { + open_directory_no_reparse( + path, + READ_CONTROL | WRITE_DAC, + FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, + disposition, + ) +} + +fn create_directory_junction(target: &Path, alias: &Path) -> Result<()> { + let output = Command::new("cmd") + .args(["/C", "mklink", "/J"]) + .arg(alias) + .arg(target) + .output()?; + if !output.status.success() { + anyhow::bail!( + "mklink /J failed: stdout={} stderr={}", + String::from_utf8_lossy(&output.stdout).trim(), + String::from_utf8_lossy(&output.stderr).trim() + ); + } + Ok(()) +} + +#[test] +fn creates_and_opens_plain_directory_leaf() -> Result<()> { + let temporary = tempfile::tempdir()?; + let directory = temporary.path().join(".sandbox-bin"); + + drop(open_for_acl( + &directory, + DirectoryOpenDisposition::OpenOrCreate, + )?); + let _handle = open_for_acl(&directory, DirectoryOpenDisposition::OpenExisting)?; + + assert!(directory.is_dir()); + Ok(()) +} + +#[test] +fn rejects_final_directory_junction() -> Result<()> { + let temporary = tempfile::tempdir()?; + let target = temporary.path().join("target"); + let alias = temporary.path().join(".sandbox-bin"); + fs::create_dir(&target)?; + create_directory_junction(&target, &alias)?; + + let _ = open_for_acl(&alias, DirectoryOpenDisposition::OpenOrCreate) + .expect_err("final directory junction must be rejected"); + fs::remove_dir(&alias)?; + Ok(()) +} + +#[test] +fn rejects_ancestor_directory_junction() -> Result<()> { + let temporary = tempfile::tempdir()?; + let target_home = temporary.path().join("target-home"); + let alias_home = temporary.path().join("linked-home"); + fs::create_dir(&target_home)?; + fs::create_dir(target_home.join(".sandbox-bin"))?; + create_directory_junction(&target_home, &alias_home)?; + + let _ = open_for_acl( + &alias_home.join(".sandbox-bin"), + DirectoryOpenDisposition::OpenOrCreate, + ) + .expect_err("ancestor directory junction must be rejected"); + fs::remove_dir(&alias_home)?; + Ok(()) +} + +#[test] +fn open_existing_does_not_create_missing_directory() -> Result<()> { + let temporary = tempfile::tempdir()?; + let directory = temporary.path().join("missing"); + assert!(open_for_acl(&directory, DirectoryOpenDisposition::OpenExisting).is_err()); + assert!(!directory.exists()); + Ok(()) +} + +#[test] +fn local_directory_path_accepts_drive_and_verbatim_drive_paths() { + assert!(validate_local_directory_path(Path::new(r"C:\Users\alice\.codex")).is_ok()); + assert!(validate_local_directory_path(Path::new(r"\\?\D:\Codex Data\home")).is_ok()); +} + +#[test] +fn local_directory_path_rejects_relative_network_parent_and_stream_paths() { + for path in [ + r"relative\.codex", + r"C:relative\.codex", + r"\\server\share\.codex", + r"\\?\UNC\server\share\.codex", + r"\\.\C:\Users\alice", + r"C:\Users\alice\..\other", + r"C:\Users\alice\.codex:stream", + ] { + assert!( + validate_local_directory_path(Path::new(path)).is_err(), + "unexpectedly accepted {path}" + ); + } +} diff --git a/codex-rs/windows-sandbox-service/Cargo.toml b/codex-rs/windows-sandbox-service/Cargo.toml index 080378a67f..97632ad5b2 100644 --- a/codex-rs/windows-sandbox-service/Cargo.toml +++ b/codex-rs/windows-sandbox-service/Cargo.toml @@ -18,6 +18,7 @@ workspace = true [dependencies] anyhow = { workspace = true } +codex-windows-sandbox = { workspace = true } [target.'cfg(windows)'.dependencies.windows-sys] version = "0.52" diff --git a/codex-rs/windows-sandbox-service/src/ipc.rs b/codex-rs/windows-sandbox-service/src/ipc.rs index f3bc65363a..70831447d4 100644 --- a/codex-rs/windows-sandbox-service/src/ipc.rs +++ b/codex-rs/windows-sandbox-service/src/ipc.rs @@ -1,10 +1,23 @@ //! Disabled provisioning IPC until authenticated request handling is installed. +// Keep these private until the authenticated transport is connected. +#[allow(dead_code)] +mod home; +#[allow(dead_code)] +mod request; + use anyhow::Result; +#[cfg(test)] +use home::pin_existing_ancestors; +#[cfg(test)] +use request::ProvisioningRequest; +#[cfg(test)] +use request::validate_request; use std::sync::Arc; use std::sync::atomic::AtomicBool; pub(crate) const PIPE_NAME: &str = r"\\.\pipe\OpenAI.CodexSandbox"; +const MAX_REQUEST_BYTES: usize = 4096; pub(crate) fn run( _shutdown: Arc, @@ -14,3 +27,7 @@ pub(crate) fn run( } pub(crate) fn wake() {} + +#[cfg(test)] +#[path = "ipc_tests.rs"] +mod tests; diff --git a/codex-rs/windows-sandbox-service/src/ipc/home.rs b/codex-rs/windows-sandbox-service/src/ipc/home.rs new file mode 100644 index 0000000000..e8a0c3f295 --- /dev/null +++ b/codex-rs/windows-sandbox-service/src/ipc/home.rs @@ -0,0 +1,158 @@ +//! Validates and pins local Codex home and sandbox directories, keeping sandbox +//! leaves nonempty so they cannot become junctions during provisioning. + +use anyhow::Context; +use anyhow::Result; +use anyhow::bail; +use codex_windows_sandbox::DirectoryOpenDisposition; +use codex_windows_sandbox::create_directory_guard; +use codex_windows_sandbox::open_directory_no_reparse; +use codex_windows_sandbox::to_wide; +use codex_windows_sandbox::validate_local_directory_path; +use std::os::windows::fs::MetadataExt; +use std::os::windows::io::BorrowedHandle; +use std::os::windows::io::IntoRawHandle; +use std::path::Component; +use std::path::Path; +use std::path::PathBuf; +use windows_sys::Win32::Foundation as foundation; +use windows_sys::Win32::Foundation::HANDLE; +use windows_sys::Win32::Storage::FileSystem as filesystem; + +const DRIVE_FIXED: u32 = 3; + +pub(super) struct OwnedHandle(pub(super) HANDLE); + +impl Drop for OwnedHandle { + fn drop(&mut self) { + if self.0 != 0 && self.0 != foundation::INVALID_HANDLE_VALUE { + unsafe { foundation::CloseHandle(self.0) }; + } + } +} + +pub(super) fn prepare_codex_home(requested: &Path) -> Result<(PathBuf, Vec)> { + let mut handles = Vec::new(); + validate_local_directory_path(requested)?; + let requested_root = requested + .ancestors() + .last() + .context("find the root of the requested Codex home")?; + if unsafe { filesystem::GetDriveTypeW(to_wide(requested_root.as_os_str()).as_ptr()) } + != DRIVE_FIXED + { + bail!("Codex home must be located on a fixed local drive"); + } + let parent = requested + .parent() + .context("Codex home must have an existing parent directory")?; + pin_existing_ancestors(parent, &mut handles)?; + handles.push(pin_directory( + requested, + filesystem::FILE_READ_ATTRIBUTES, + DirectoryOpenDisposition::OpenOrCreate, + )?); + let home = requested + .canonicalize() + .with_context(|| format!("canonicalize Codex home {}", requested.display()))?; + validate_local_directory_path(&home)?; + let root = home + .ancestors() + .last() + .context("find the root of the requested Codex home")?; + if unsafe { filesystem::GetDriveTypeW(to_wide(root.as_os_str()).as_ptr()) } != DRIVE_FIXED { + bail!("Codex home must be located on a fixed local drive"); + } + if home != requested { + pin_existing_ancestors(&home, &mut handles)?; + } + for (index, directory) in [ + home.as_path(), + &home.join(".sandbox"), + &home.join(".sandbox-secrets"), + &home.join(".sandbox-bin"), + ] + .into_iter() + .enumerate() + { + let mut access = filesystem::FILE_READ_ATTRIBUTES + | filesystem::FILE_ADD_FILE + | filesystem::FILE_ADD_SUBDIRECTORY; + if index != 0 { + access |= filesystem::WRITE_DAC; + } + let handle = pin_directory(directory, access, DirectoryOpenDisposition::OpenOrCreate) + .with_context(|| { + if index == 0 { + format!( + "requesting user must be permitted to write Codex home {}", + directory.display() + ) + } else { + format!( + "requesting user must be permitted to write and modify sandbox directory ACL {}", + directory.display() + ) + } + })?; + if index != 0 { + // The handle is owned here and remains live until the guard is installed. + let directory_handle = unsafe { BorrowedHandle::borrow_raw(handle.0 as _) }; + let guard = create_directory_guard(directory_handle)?; + // Reject conversion before guard creation; after creation the retained + // file prevents this directory from becoming empty and being converted. + drop(pin_directory( + directory, + filesystem::FILE_READ_ATTRIBUTES, + DirectoryOpenDisposition::OpenExisting, + )?); + handles.push(OwnedHandle(guard.into_raw_handle() as HANDLE)); + } + handles.push(handle); + if index == 0 { + continue; + } + for entry in std::fs::read_dir(directory)? { + let entry = entry?; + let metadata = entry.path().symlink_metadata()?; + if metadata.file_attributes() & filesystem::FILE_ATTRIBUTE_REPARSE_POINT != 0 { + bail!("refusing reparse point {}", entry.path().display()); + } + } + } + + // Pinned parent handles prevent directory replacement, but existing helper file + // opens remain pathname-based; hardlink/leaf-file races need a follow-up redesign. + Ok((home, handles)) +} + +pub(super) fn pin_existing_ancestors(path: &Path, handles: &mut Vec) -> Result<()> { + let mut current = PathBuf::new(); + for component in path.components() { + current.push(component.as_os_str()); + if !matches!(component, Component::Prefix(_)) && current.is_absolute() { + handles.push(pin_directory( + ¤t, + filesystem::FILE_READ_ATTRIBUTES, + DirectoryOpenDisposition::OpenExisting, + )?); + } + } + Ok(()) +} + +fn pin_directory( + path: &Path, + access: u32, + disposition: DirectoryOpenDisposition, +) -> Result { + let handle = open_directory_no_reparse( + path, + // Metadata-only access does not enforce no-delete sharing. + access | filesystem::FILE_TRAVERSE, + filesystem::FILE_SHARE_READ | filesystem::FILE_SHARE_WRITE, + disposition, + ) + .with_context(|| format!("pin {}", path.display()))?; + Ok(OwnedHandle(handle.into_raw_handle() as HANDLE)) +} diff --git a/codex-rs/windows-sandbox-service/src/ipc/request.rs b/codex-rs/windows-sandbox-service/src/ipc/request.rs new file mode 100644 index 0000000000..d5fd7eac92 --- /dev/null +++ b/codex-rs/windows-sandbox-service/src/ipc/request.rs @@ -0,0 +1,71 @@ +//! Validates framed provisioning requests and normalizes proxy settings. + +use anyhow::Context; +use anyhow::Result; +use anyhow::bail; +use codex_windows_sandbox::PROVISIONING_PROTOCOL_VERSION; +use codex_windows_sandbox::ProvisioningMessage; +use codex_windows_sandbox::WindowsSandboxProvisioningSettings; +use codex_windows_sandbox::WindowsSandboxProxyListeners; +use codex_windows_sandbox::read_provisioning_frame; +use std::path::PathBuf; + +use super::MAX_REQUEST_BYTES; + +#[derive(Debug, Eq, PartialEq)] +pub(super) struct ProvisioningRequest { + pub(super) codex_home: PathBuf, + pub(super) listeners: WindowsSandboxProxyListeners, + pub(super) settings: WindowsSandboxProvisioningSettings, +} + +pub(super) fn validate_request(request: &[u8]) -> Result { + if request.len() > MAX_REQUEST_BYTES { + bail!("provisioning request exceeds size limit"); + } + let mut reader = request; + let frame = read_provisioning_frame(&mut reader) + .context("invalid framed provisioning request")? + .context("provisioning client sent an empty request")?; + if !reader.is_empty() { + bail!("provisioning requests must contain exactly one IPC frame"); + } + if frame.version != PROVISIONING_PROTOCOL_VERSION { + bail!( + "unsupported provisioning request version: {}", + frame.version + ); + } + let ProvisioningMessage::ProvisionSandboxRequest { payload: request } = frame.message else { + bail!("expected a sandbox provisioning request"); + }; + if request.codex_home.is_empty() || request.codex_home.contains(['\0', '\r', '\n']) { + bail!("Codex home is empty or contains an invalid control character"); + } + let mut settings = request.settings; + let mut listeners = request.listeners; + for ports in [ + &mut settings.proxy_ports, + &mut listeners.http_ports, + &mut listeners.socks_ports, + ] { + if ports.contains(&0) { + bail!("provisioning request includes an invalid proxy port"); + } + ports.sort_unstable(); + ports.dedup(); + } + if listeners + .http_ports + .iter() + .chain(&listeners.socks_ports) + .any(|port| !settings.proxy_ports.contains(port)) + { + bail!("provisioning listener is absent from the proxy settings"); + } + Ok(ProvisioningRequest { + codex_home: PathBuf::from(request.codex_home), + listeners, + settings, + }) +} diff --git a/codex-rs/windows-sandbox-service/src/ipc_tests.rs b/codex-rs/windows-sandbox-service/src/ipc_tests.rs new file mode 100644 index 0000000000..9b353475f2 --- /dev/null +++ b/codex-rs/windows-sandbox-service/src/ipc_tests.rs @@ -0,0 +1,225 @@ +use super::ProvisioningRequest; +use super::pin_existing_ancestors; +use super::validate_request; +use codex_windows_sandbox::FramedProvisioningMessage; +use codex_windows_sandbox::PROVISIONING_PROTOCOL_VERSION; +use codex_windows_sandbox::ProvisioningMessage; +use codex_windows_sandbox::SandboxProvisioningRequest; +use codex_windows_sandbox::SandboxProvisioningResponse; +use codex_windows_sandbox::WindowsSandboxProvisioningSettings; +use codex_windows_sandbox::WindowsSandboxProxyListeners; +use codex_windows_sandbox::write_provisioning_frame; +use std::path::PathBuf; + +fn framed_request(request: SandboxProvisioningRequest) -> Vec { + let mut frame = Vec::new(); + write_provisioning_frame( + &mut frame, + &FramedProvisioningMessage { + version: PROVISIONING_PROTOCOL_VERSION, + message: ProvisioningMessage::ProvisionSandboxRequest { payload: request }, + }, + ) + .unwrap(); + frame +} + +#[test] +fn provisioning_request_preserves_home_spaces_and_unicode() { + let request = framed_request(SandboxProvisioningRequest { + codex_home: "D:\\Codex Homes\\Jos\u{00e9}\\.codex".to_string(), + settings: WindowsSandboxProvisioningSettings::default(), + listeners: WindowsSandboxProxyListeners::default(), + }); + assert_eq!( + validate_request(&request).unwrap(), + ProvisioningRequest { + codex_home: PathBuf::from("D:\\Codex Homes\\Jos\u{00e9}\\.codex"), + listeners: WindowsSandboxProxyListeners::default(), + settings: WindowsSandboxProvisioningSettings::default(), + } + ); +} + +#[test] +fn structured_provisioning_request_carries_normalized_proxy_settings() { + for (http_port, socks_port, proxy_ports) in + [(8081, 3128, vec![3128, 8081]), (8081, 8081, vec![8081])] + { + let request = framed_request(SandboxProvisioningRequest { + codex_home: "D:\\Codex Homes\\Jos\u{00e9}\\.codex".to_string(), + settings: WindowsSandboxProvisioningSettings { + proxy_ports: vec![http_port, socks_port, http_port], + allow_local_binding: true, + }, + listeners: WindowsSandboxProxyListeners { + http_ports: vec![http_port, http_port], + socks_ports: vec![socks_port], + }, + }); + assert_eq!( + validate_request(&request).unwrap(), + ProvisioningRequest { + codex_home: PathBuf::from("D:\\Codex Homes\\Jos\u{00e9}\\.codex"), + listeners: WindowsSandboxProxyListeners { + http_ports: vec![http_port], + socks_ports: vec![socks_port], + }, + settings: WindowsSandboxProvisioningSettings { + proxy_ports, + allow_local_binding: true, + }, + }, + ); + } +} + +#[test] +fn structured_provisioning_request_accepts_independent_and_additional_proxy_ports() { + for (http_ports, socks_ports, proxy_ports) in [ + (vec![3128], vec![], vec![3128]), + (vec![], vec![1080], vec![1080]), + (vec![3128, 8080], vec![1080], vec![1080, 1082, 3128, 8080]), + ] { + let settings = WindowsSandboxProvisioningSettings { + proxy_ports, + allow_local_binding: false, + }; + let listeners = WindowsSandboxProxyListeners { + http_ports, + socks_ports, + }; + let request = framed_request(SandboxProvisioningRequest { + codex_home: r"C:\Users\alice\.codex".to_string(), + settings: settings.clone(), + listeners: listeners.clone(), + }); + assert_eq!( + validate_request(&request).unwrap(), + ProvisioningRequest { + codex_home: PathBuf::from(r"C:\Users\alice\.codex"), + settings, + listeners, + } + ); + } +} + +#[test] +fn structured_provisioning_request_accepts_disabled_listeners() { + let request = framed_request(SandboxProvisioningRequest { + codex_home: r"C:\Users\alice\.codex".to_string(), + settings: WindowsSandboxProvisioningSettings::default(), + listeners: WindowsSandboxProxyListeners::default(), + }); + assert_eq!( + validate_request(&request).unwrap(), + ProvisioningRequest { + codex_home: PathBuf::from(r"C:\Users\alice\.codex"), + listeners: WindowsSandboxProxyListeners::default(), + settings: WindowsSandboxProvisioningSettings::default(), + } + ); +} + +#[test] +fn structured_provisioning_request_requires_exact_version_fields_and_framing() { + let valid = SandboxProvisioningRequest { + codex_home: r"C:\Users\alice\.codex".to_string(), + settings: WindowsSandboxProvisioningSettings::default(), + listeners: WindowsSandboxProxyListeners::default(), + }; + let mut invalid_version = Vec::new(); + write_provisioning_frame( + &mut invalid_version, + &FramedProvisioningMessage { + version: PROVISIONING_PROTOCOL_VERSION + 1, + message: ProvisioningMessage::ProvisionSandboxRequest { + payload: valid.clone(), + }, + }, + ) + .unwrap(); + assert!(validate_request(&invalid_version).is_err()); + + let mut unexpected_message = Vec::new(); + write_provisioning_frame( + &mut unexpected_message, + &FramedProvisioningMessage { + version: PROVISIONING_PROTOCOL_VERSION, + message: ProvisioningMessage::ProvisionSandboxResponse { + payload: SandboxProvisioningResponse::Ok, + }, + }, + ) + .unwrap(); + assert!(validate_request(&unexpected_message).is_err()); + + let valid = framed_request(valid); + assert!(validate_request(&valid[..valid.len() - 1]).is_err()); + let mut duplicate = valid.clone(); + duplicate.extend_from_slice(&valid); + assert!(validate_request(&duplicate).is_err()); +} + +#[test] +fn structured_provisioning_request_rejects_invalid_or_inconsistent_ports() { + for (proxy_ports, http_ports, socks_ports) in [ + (vec![0], vec![], vec![]), + (vec![3128], vec![0], vec![]), + (vec![3128], vec![3128], vec![0]), + (vec![3128], vec![3128], vec![8081]), + ] { + let request = framed_request(SandboxProvisioningRequest { + codex_home: r"C:\Users\alice\.codex".to_string(), + settings: WindowsSandboxProvisioningSettings { + proxy_ports, + allow_local_binding: false, + }, + listeners: WindowsSandboxProxyListeners { + http_ports, + socks_ports, + }, + }); + assert!(validate_request(&request).is_err()); + } +} + +#[test] +fn provisioning_request_rejects_empty_control_characters_and_invalid_utf8() { + for home in ["", "C:\\safe\0evil", "C:\\safe\rmore", "C:\\safe\nmore"] { + let request = framed_request(SandboxProvisioningRequest { + codex_home: home.to_string(), + settings: WindowsSandboxProvisioningSettings::default(), + listeners: WindowsSandboxProxyListeners::default(), + }); + assert!(validate_request(&request).is_err()); + } + + let mut invalid_utf8 = framed_request(SandboxProvisioningRequest { + codex_home: r"C:\Users\alice\.codex".to_string(), + settings: WindowsSandboxProvisioningSettings::default(), + listeners: WindowsSandboxProxyListeners::default(), + }); + invalid_utf8[std::mem::size_of::()] = 0xff; + assert!(validate_request(&invalid_utf8).is_err()); +} + +#[test] +fn pin_existing_ancestors_accepts_drive_and_verbatim_drive_roots() { + let executable = std::env::current_exe().unwrap().canonicalize().unwrap(); + let verbatim_root = executable.ancestors().last().unwrap(); + let ordinary_root = PathBuf::from( + verbatim_root + .to_str() + .unwrap() + .strip_prefix(r"\\?\") + .unwrap(), + ); + + for root in [ordinary_root.as_path(), verbatim_root] { + let mut handles = Vec::new(); + pin_existing_ancestors(root, &mut handles).unwrap(); + assert_eq!(handles.len(), 1); + } +}