From 5a529c2909eb7f169b1f1fb2ed4893718504ffba Mon Sep 17 00:00:00 2001 From: zm-oai Date: Thu, 12 Nov 2026 00:12:55 +0000 Subject: [PATCH] Preserve file permissions during repeated Windows sandbox setup (#60943) ## Why Reapplying directory ACLs during registered sandbox setup can change permissions on existing files moved into the sandbox directory. ## What changed During `ProvisionOnly` setup for a registered runtime, verify both persistent sandbox directories' ACLs before writing them. Skip both writes when the explicit permissions match the expected policy, allowing additional inherited entries. If verification fails or either policy differs, apply the directory policies as before. ## Testing Add a regression test that repeats registered setup and verifies that a moved file remains unwritable by the sandbox group while a newly created file inherits write access. GitOrigin-RevId: fb199001fc554903064618b86562b2ff0d0384c5 --- codex-rs/windows-sandbox-rs/src/acl.rs | 8 ++ .../src/setup_provisioning.rs | 110 ++++++++++++++---- .../src/setup_provisioning/acl_tests.rs | 37 +++++- 3 files changed, 132 insertions(+), 23 deletions(-) diff --git a/codex-rs/windows-sandbox-rs/src/acl.rs b/codex-rs/windows-sandbox-rs/src/acl.rs index e3f2ecd810..5275100bce 100644 --- a/codex-rs/windows-sandbox-rs/src/acl.rs +++ b/codex-rs/windows-sandbox-rs/src/acl.rs @@ -109,6 +109,14 @@ pub unsafe fn fetch_dacl_handle(path: &Path) -> Result<(*mut ACL, *mut c_void)> .custom_flags(FILE_FLAG_BACKUP_SEMANTICS) .open(path) .with_context(|| format!("open ACL target {}", path.display()))?; + unsafe { fetch_dacl_from_handle(&handle, path) } +} + +/// The returned descriptor owns the DACL and must be freed with LocalFree. +pub(crate) unsafe fn fetch_dacl_from_handle( + handle: &impl AsRawHandle, + path: &Path, +) -> Result<(*mut ACL, *mut c_void)> { let mut p_sd: *mut c_void = std::ptr::null_mut(); let mut p_dacl: *mut ACL = std::ptr::null_mut(); let code = GetSecurityInfo( diff --git a/codex-rs/windows-sandbox-rs/src/setup_provisioning.rs b/codex-rs/windows-sandbox-rs/src/setup_provisioning.rs index ba93bfac8e..c4e86584cb 100644 --- a/codex-rs/windows-sandbox-rs/src/setup_provisioning.rs +++ b/codex-rs/windows-sandbox-rs/src/setup_provisioning.rs @@ -61,10 +61,14 @@ use std::path::PathBuf; use std::process::Command; use std::process::Stdio; use std::sync::mpsc; +use windows::Win32::Foundation::BOOL; +use windows::Win32::Foundation::HLOCAL as LocalAllocHandle; +use windows::core::Owned; use windows_sys::Win32::Foundation::GetLastError; use windows_sys::Win32::Foundation::HLOCAL; use windows_sys::Win32::Foundation::LocalFree; use windows_sys::Win32::NetworkManagement::NetManagement::UF_ACCOUNTDISABLE; +use windows_sys::Win32::Security as security; use windows_sys::Win32::Security::ACL; use windows_sys::Win32::Security::Authorization::ConvertStringSidToSidW; use windows_sys::Win32::Security::Authorization::EXPLICIT_ACCESS_W; @@ -157,9 +161,10 @@ impl SetupMode { } #[derive(Clone, Copy)] -enum DaclInheritance { - Inherited, - Protected, +enum DirectoryAcl { + VerifyInherited, + ApplyInherited, + ApplyProtected, } fn log_line(log: &mut dyn Write, msg: &str) -> Result<()> { @@ -349,7 +354,7 @@ fn lock_sandbox_dir( sandbox_group_access_mode: i32, sandbox_group_mask: u32, real_user_mask: u32, - dacl_inheritance: DaclInheritance, + policy: DirectoryAcl, setup_mode: SetupMode, ) -> Result<()> { // ProvisionOnly accepts another user's CODEX_HOME; keep its ACL mutation @@ -359,6 +364,14 @@ fn lock_sandbox_dir( std::fs::create_dir_all(dir)?; None } + SetupMode::ProvisionOnly if matches!(policy, DirectoryAcl::VerifyInherited) => { + Some(open_directory_no_reparse( + dir, + READ_CONTROL, + FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, + DirectoryOpenDisposition::OpenExisting, + )?) + } SetupMode::ProvisionOnly => Some(open_directory_no_reparse( dir, // SetSecurityInfo can reject a WRITE_DAC-only directory handle. @@ -371,10 +384,12 @@ fn lock_sandbox_dir( let admins_sid = resolve_sid("Administrators")?; let real_sid = resolve_sid(real_user)?; let modify = FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE | DELETE; - let (security_information, system_access) = match dacl_inheritance { - DaclInheritance::Inherited => (DACL_SECURITY_INFORMATION, modify), + let (security_information, system_access) = match policy { + DirectoryAcl::VerifyInherited | DirectoryAcl::ApplyInherited => { + (DACL_SECURITY_INFORMATION, modify) + } // The SYSTEM helper must be able to reapply the bin policy to existing children. - DaclInheritance::Protected => ( + DirectoryAcl::ApplyProtected => ( DACL_SECURITY_INFORMATION | PROTECTED_DACL_SECURITY_INFORMATION, modify | WRITE_DAC, ), @@ -391,7 +406,7 @@ fn lock_sandbox_dir( ]; unsafe { let mut eas: Vec = Vec::new(); - let mut sids: Vec<*mut c_void> = Vec::new(); + let mut sids = Vec::new(); for (sid_bytes, mask, access_mode) in entries.iter().map(|(s, m, a)| (s, *m, *a)) { let sid_str = string_from_sid_bytes(sid_bytes).map_err(anyhow::Error::msg)?; let sid_w = to_wide(OsStr::new(&sid_str)); @@ -402,7 +417,7 @@ fn lock_sandbox_dir( GetLastError() )); } - sids.push(psid); + sids.push(Owned::new(LocalAllocHandle(psid))); eas.push(EXPLICIT_ACCESS_W { grfAccessPermissions: mask, grfAccessMode: access_mode, @@ -428,6 +443,17 @@ fn lock_sandbox_dir( "SetEntriesInAclW sandbox dir failed: {set}", )); } + let _owned_acl = Owned::new(LocalAllocHandle(new_dacl.cast())); + if matches!(policy, DirectoryAcl::VerifyInherited) { + let handle = directory + .as_ref() + .context("verification requires a service directory handle")?; + anyhow::ensure!( + metadata_policy_matches(dir, handle, &*new_dacl)?, + "metadata directory permissions differ" + ); + return Ok(()); + } let (res, api) = match directory.as_ref() { Some(directory) => ( SetSecurityInfo( @@ -460,18 +486,44 @@ fn lock_sandbox_dir( if res != 0 { return Err(anyhow::anyhow!("{api} sandbox dir failed: {res}")); } - if !new_dacl.is_null() { - LocalFree(new_dacl as HLOCAL); - } - for sid in sids { - if !sid.is_null() { - LocalFree(sid as HLOCAL); - } - } } Ok(()) } +// Compare complete ACEs so extra explicit permissions or a changed mask still require an update. +fn metadata_policy_matches( + path: &Path, + directory: &impl AsRawHandle, + expected: &ACL, +) -> Result { + unsafe fn entries(acl: &ACL) -> Result> { + (0..u32::from(acl.AceCount)) + .map(|index| { + let mut ace = std::ptr::null_mut(); + unsafe { + BOOL(security::GetAce(acl, index, &mut ace)).ok()?; + } + let size = unsafe { (*ace.cast::()).AceSize }; + Ok(unsafe { std::slice::from_raw_parts(ace.cast(), usize::from(size)) }) + }) + .collect() + } + unsafe { + let (current, descriptor) = crate::acl::fetch_dacl_from_handle(directory, path)?; + let _descriptor = Owned::new(LocalAllocHandle(descriptor)); + let Some(current) = current.as_ref() else { + return Ok(false); + }; + let current = entries(current)?; + let explicit = entries(expected)?; + // Windows puts inherited rules after the directory's explicit policy. + Ok(current.starts_with(&explicit) + && current[explicit.len()..] + .iter() + .all(|ace| ace[1] & security::INHERITED_ACE as u8 != 0)) + } +} + pub fn main() -> Result<()> { let mut setup_mode = None; let ret = real_main(&mut setup_mode); @@ -816,7 +868,11 @@ fn configure_offline_sandbox_network( Ok(()) } -fn lock_persistent_sandbox_dirs(payload: &Payload, sandbox_group_sid: &[u8]) -> Result<()> { +fn persistent_directory_policy( + payload: &Payload, + sandbox_group_sid: &[u8], + policy: DirectoryAcl, +) -> Result<()> { lock_sandbox_dir( &sandbox_dir(&payload.codex_home), &payload.real_user, @@ -824,7 +880,7 @@ fn lock_persistent_sandbox_dirs(payload: &Payload, sandbox_group_sid: &[u8]) -> GRANT_ACCESS, FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE | DELETE, FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE, - DaclInheritance::Inherited, + policy, payload.mode, ) .map_err(|err| { @@ -843,7 +899,7 @@ fn lock_persistent_sandbox_dirs(payload: &Payload, sandbox_group_sid: &[u8]) -> DENY_ACCESS, FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE | DELETE, FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE, - DaclInheritance::Inherited, + policy, payload.mode, ) .map_err(|err| { @@ -855,6 +911,18 @@ fn lock_persistent_sandbox_dirs(payload: &Payload, sandbox_group_sid: &[u8]) -> ), )) })?; + Ok(()) +} + +fn lock_persistent_sandbox_dirs(payload: &Payload, sandbox_group_sid: &[u8]) -> Result<()> { + // Only a complete match can skip both writes. On a mismatch the original write checks run in order. + let configured = payload.mode == SetupMode::ProvisionOnly + && payload.runtime == SetupRuntime::Registered + && persistent_directory_policy(payload, sandbox_group_sid, DirectoryAcl::VerifyInherited) + .is_ok(); + if !configured { + persistent_directory_policy(payload, sandbox_group_sid, DirectoryAcl::ApplyInherited)?; + } let legacy_users = sandbox_dir(&payload.codex_home).join("sandbox_users.json"); if legacy_users.exists() { let _ = std::fs::remove_file(&legacy_users); @@ -874,7 +942,7 @@ fn lock_sandbox_bin_dir(payload: &Payload, sandbox_group_sid: &[u8]) -> Result<( GRANT_ACCESS, FILE_GENERIC_READ | FILE_GENERIC_EXECUTE, FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE | DELETE | WRITE_DAC, - DaclInheritance::Protected, + DirectoryAcl::ApplyProtected, payload.mode, ) .map_err(|err| { diff --git a/codex-rs/windows-sandbox-rs/src/setup_provisioning/acl_tests.rs b/codex-rs/windows-sandbox-rs/src/setup_provisioning/acl_tests.rs index dbc0c90004..aca29e68da 100644 --- a/codex-rs/windows-sandbox-rs/src/setup_provisioning/acl_tests.rs +++ b/codex-rs/windows-sandbox-rs/src/setup_provisioning/acl_tests.rs @@ -1,5 +1,5 @@ use super::DELETE; -use super::DaclInheritance; +use super::DirectoryAcl; use super::FILE_GENERIC_EXECUTE; use super::FILE_GENERIC_READ; use super::FILE_GENERIC_WRITE; @@ -77,7 +77,7 @@ fn provision_only_locks_plain_directory_via_handle() { GRANT_ACCESS, FILE_GENERIC_READ | FILE_GENERIC_EXECUTE, FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE | DELETE, - DaclInheritance::Protected, + DirectoryAcl::ApplyProtected, SetupMode::ProvisionOnly, ) .expect("lock sandbox bin via no-reparse handle"); @@ -85,6 +85,39 @@ fn provision_only_locks_plain_directory_via_handle() { assert!(sandbox_bin.is_dir()); } +#[test] +fn repeated_registered_setup_keeps_existing_file_permissions() -> anyhow::Result<()> { + let home = tempfile::tempdir()?; + let mut payload: Payload = serde_json::from_value(super::tests::payload_json())?; + payload.codex_home = home.path().to_owned(); + payload.real_user = crate::runtime_ownership::current_setup_user()?; + payload.mode = SetupMode::ProvisionOnly; + payload.runtime = super::SetupRuntime::Registered; + let group = crate::winutil::sid_bytes_from_string("S-1-5-21-42-43-44-1000")?; + let configure = || super::lock_persistent_sandbox_dirs(&payload, &group); + configure()?; + + let existing = home.path().join("existing"); + fs::write(&existing, b"test")?; + let moved = home.path().join(".sandbox").join("existing"); + fs::rename(existing, &moved)?; + let can_write = |file: &std::path::Path| { + path_mask_allows( + file, + &[group.as_ptr().cast_mut().cast()], + windows_sys::Win32::Storage::FileSystem::FILE_WRITE_DATA, + /*require_all_bits*/ true, + ) + }; + assert!(!can_write(&moved)?); + configure()?; + assert!(!can_write(&moved)?); + let new_file = home.path().join(".sandbox").join("new"); + fs::write(&new_file, b"test")?; + assert!(can_write(&new_file)?); + Ok(()) +} + #[test] fn lock_sandbox_dir_blocks_inherited_write_for_runner_files() { for setup_mode in [SetupMode::Full, SetupMode::ProvisionOnly] {