diff --git a/codex-rs/windows-sandbox-rs/src/setup_provisioning.rs b/codex-rs/windows-sandbox-rs/src/setup_provisioning.rs index d4557688e9..ba93bfac8e 100644 --- a/codex-rs/windows-sandbox-rs/src/setup_provisioning.rs +++ b/codex-rs/windows-sandbox-rs/src/setup_provisioning.rs @@ -370,22 +370,23 @@ fn lock_sandbox_dir( let system_sid = resolve_sid("SYSTEM")?; 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), + // The SYSTEM helper must be able to reapply the bin policy to existing children. + DaclInheritance::Protected => ( + DACL_SECURITY_INFORMATION | PROTECTED_DACL_SECURITY_INFORMATION, + modify | WRITE_DAC, + ), + }; let entries = [ ( sandbox_group_sid.to_vec(), sandbox_group_mask, sandbox_group_access_mode, ), - ( - system_sid, - FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE | DELETE, - GRANT_ACCESS, - ), - ( - admins_sid, - FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE | DELETE, - GRANT_ACCESS, - ), + (system_sid, system_access, GRANT_ACCESS), + (admins_sid, modify, GRANT_ACCESS), (real_sid, real_user_mask, GRANT_ACCESS), ]; unsafe { @@ -427,12 +428,6 @@ fn lock_sandbox_dir( "SetEntriesInAclW sandbox dir failed: {set}", )); } - let security_information = match dacl_inheritance { - DaclInheritance::Inherited => DACL_SECURITY_INFORMATION, - DaclInheritance::Protected => { - DACL_SECURITY_INFORMATION | PROTECTED_DACL_SECURITY_INFORMATION - } - }; let (res, api) = match directory.as_ref() { Some(directory) => ( SetSecurityInfo( 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 876d60501b..dbc0c90004 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 @@ -158,10 +158,21 @@ fn lock_sandbox_dir_blocks_inherited_write_for_runner_files() { ) .expect("sandbox cannot change the bin DACL") ); + let system_sid = resolve_sid("SYSTEM").expect("resolve SYSTEM SID"); + let system_psid = sid_bytes_to_psid(&system_sid).expect("convert SYSTEM SID"); let new_runner = sandbox_bin.join("new-runner.exe"); fs::write(&new_runner, b"new").expect("create new runner"); for path in [&sandbox_bin, &existing_runner, &new_runner] { + assert!( + path_mask_allows( + path, + &[system_psid], + WRITE_DAC, + /*require_all_bits*/ true + ) + .expect("SYSTEM can reapply the bin policy") + ); assert!( !path_mask_allows( path, diff --git a/codex-rs/windows-sandbox-service/src/ipc_tests.rs b/codex-rs/windows-sandbox-service/src/ipc_tests.rs index 14a7dbdc5d..23ce2e8390 100644 --- a/codex-rs/windows-sandbox-service/src/ipc_tests.rs +++ b/codex-rs/windows-sandbox-service/src/ipc_tests.rs @@ -48,6 +48,60 @@ struct NonOwningPipeHandle(foundation::HANDLE); // threads. This wrapper does not own the handle or extend its lifetime. unsafe impl Send for NonOwningPipeHandle {} +// Run as LocalSystem on a disposable Windows VM. The interactive user seeds both +// homes with the legacy DACL, with a SYSTEM WRITE_DAC deny on the second bin. +#[test] +#[ignore = "requires SYSTEM, an interactive owner, and two user-owned legacy sandbox homes"] +fn repair_legacy_bin_as_its_owner() -> anyhow::Result<()> { + use std::os::windows::io::AsHandle; + use windows_sys::Win32::System::RemoteDesktop::WTSQueryUserToken; + + let session = std::env::var("CODEX_TEST_OWNER_SESSION")?.parse()?; + let mut token = std::ptr::null_mut(); + if unsafe { WTSQueryUserToken(session, &mut token) } == 0 { + return Err(std::io::Error::last_os_error().into()); + } + // SAFETY: WTSQueryUserToken transferred this interactive user's token. + let owner = unsafe { OwnedHandle::from_raw_handle(token) }; + for (variable, should_repair) in [ + ("CODEX_TEST_LEGACY_HOME", true), + ("CODEX_TEST_DENIED_HOME", false), + ] { + let home = std::path::PathBuf::from(std::env::var(variable)?); + let (home, _pins) = crate::package_lifecycle::with_owner_impersonation(token, || { + prepare_codex_home( + &home, + SetupRuntime::Legacy, + DirectoryOpenDisposition::OpenExisting, + ) + })?; + let open = || { + super::pin_directory( + &home.join(".sandbox-bin"), + filesystem::READ_CONTROL | filesystem::WRITE_DAC, + DirectoryOpenDisposition::OpenExisting, + ) + }; + let denied = open().expect_err("the seeded bin must refuse SYSTEM ACL writes"); + assert_eq!( + denied + .downcast_ref::() + .and_then(std::io::Error::raw_os_error), + Some(5) + ); + let repair = || crate::provisioning::legacy_bin::prepare(&home, owner.as_handle()); + if should_repair { + repair()?; + open()?; + repair()?; + } else { + assert!(repair().is_err()); + assert!(open().is_err()); + } + } + Ok(()) +} + #[test] fn provisioning_error_response_preserves_causes_within_message_limits() { let error = anyhow::anyhow!("Windows error\t5\n") diff --git a/codex-rs/windows-sandbox-service/src/provisioning.rs b/codex-rs/windows-sandbox-service/src/provisioning.rs index bc99a9e362..b803ef083e 100644 --- a/codex-rs/windows-sandbox-service/src/provisioning.rs +++ b/codex-rs/windows-sandbox-service/src/provisioning.rs @@ -1,5 +1,6 @@ //! Provisions an authenticated client's sandbox through its selected setup path. +pub(crate) mod legacy_bin; mod registered; use std::os::windows::fs::MetadataExt; @@ -113,6 +114,7 @@ pub(crate) fn run( helper.display() ); } + legacy_bin::prepare(&identity.codex_home, identity.token.as_handle())?; let retained_handles = identity .directory_handles .iter() diff --git a/codex-rs/windows-sandbox-service/src/provisioning/legacy_bin.rs b/codex-rs/windows-sandbox-service/src/provisioning/legacy_bin.rs new file mode 100644 index 0000000000..4f3d2df3f6 --- /dev/null +++ b/codex-rs/windows-sandbox-service/src/provisioning/legacy_bin.rs @@ -0,0 +1,87 @@ +//! Upgrade the protected ACL written by the old legacy helper before SYSTEM reuses the bin. +use std::io; +use std::os::windows::io::AsRawHandle; +use std::os::windows::io::BorrowedHandle; +use std::path::Path; + +use anyhow::Context; +use anyhow::Result; +use anyhow::ensure; +use codex_windows_sandbox::DirectoryOpenDisposition; +use windows::Win32::Foundation::BOOL; +use windows_sys::Win32::Foundation::ERROR_ACCESS_DENIED; +use windows_sys::Win32::Security as security; +use windows_sys::Win32::Storage::FileSystem as fs; + +/// Admission has pinned the authenticated user's no-reparse path through the helper call. +pub(crate) fn prepare(home: &Path, owner: BorrowedHandle<'_>) -> Result<()> { + let bin = home.join(".sandbox-bin"); + let open = || { + codex_windows_sandbox::open_directory_no_reparse( + &bin, + fs::READ_CONTROL | fs::WRITE_DAC, + fs::FILE_SHARE_READ | fs::FILE_SHARE_WRITE | fs::FILE_SHARE_DELETE, + DirectoryOpenDisposition::OpenExisting, + ) + }; + match open() { + Ok(_) => return Ok(()), + Err(error) + if error + .downcast_ref::() + .is_some_and(|cause| cause.raw_os_error() == Some(ERROR_ACCESS_DENIED as i32)) => {} + Err(error) => return Err(error), + } + let directory = + crate::package_lifecycle::with_owner_impersonation(owner.as_raw_handle(), open)?; + if let Some((mut acl, control)) = crate::registered_runtime::read_acl(&directory)? { + upgrade_legacy_system_entry(&mut acl, control)?; + crate::registered_runtime::write_acl(&directory, &mut acl, control)?; + } + open() + .map(drop) + .context("verify SYSTEM can protect the repaired sandbox bin") +} + +// The old helper wrote a protected, allow-only ACL with one inheritable SYSTEM Modify entry. +// Alter only that entry in the in-memory ACL. A rejected edit is never written back. +fn upgrade_legacy_system_entry(acl: &mut [u32], control: u16) -> Result<()> { + ensure!( + control & security::SE_DACL_PROTECTED != 0, + "expected a protected legacy sandbox bin ACL" + ); + let system = codex_windows_sandbox::resolve_sid("SYSTEM")?; + let mut matched = false; + let dacl = acl.as_mut_ptr().cast::(); + for index in 0..u32::from(unsafe { (*dacl).AceCount }) { + let mut ace = std::ptr::null_mut(); + unsafe { BOOL(security::GetAce(dacl, index, &mut ace)).ok() } + .context("read sandbox bin ACE")?; + let header = unsafe { &*ace.cast::() }; + ensure!( + header.AceType == 0 && header.AceSize >= 16, + "unsupported legacy sandbox bin ACE" + ); + let allow = unsafe { &mut *ace.cast::() }; + let sid = std::ptr::addr_of!(allow.SidStart).cast::(); + let sid_len = usize::from(allow.Header.AceSize) + - std::mem::offset_of!(security::ACCESS_ALLOWED_ACE, SidStart); + if unsafe { std::slice::from_raw_parts(sid, sid_len) } == system { + let legacy_mask = fs::FILE_GENERIC_READ + | fs::FILE_GENERIC_WRITE + | fs::FILE_GENERIC_EXECUTE + | fs::DELETE; + ensure!( + !matched + && allow.Mask == legacy_mask + && u32::from(allow.Header.AceFlags) + == (security::OBJECT_INHERIT_ACE | security::CONTAINER_INHERIT_ACE), + "unexpected legacy SYSTEM permissions" + ); + allow.Mask |= fs::WRITE_DAC; + matched = true; + } + } + ensure!(matched, "missing legacy SYSTEM allow entry"); + Ok(()) +} diff --git a/codex-rs/windows-sandbox-service/src/registered_runtime.rs b/codex-rs/windows-sandbox-service/src/registered_runtime.rs index abf1468d51..5bb8cfdb7f 100644 --- a/codex-rs/windows-sandbox-service/src/registered_runtime.rs +++ b/codex-rs/windows-sandbox-service/src/registered_runtime.rs @@ -39,6 +39,8 @@ use crate::package_lifecycle::with_owner_impersonation; mod known_folder; mod metadata; +pub(crate) use metadata::read_acl; +pub(crate) use metadata::write_acl; mod registration_result; mod removal; diff --git a/codex-rs/windows-sandbox-service/src/registered_runtime/metadata.rs b/codex-rs/windows-sandbox-service/src/registered_runtime/metadata.rs index 48cd4e66d0..dd899e4d59 100644 --- a/codex-rs/windows-sandbox-service/src/registered_runtime/metadata.rs +++ b/codex-rs/windows-sandbox-service/src/registered_runtime/metadata.rs @@ -140,7 +140,7 @@ fn edit_root_acl( Ok(()) } -fn read_acl(handle: &OwnedHandle) -> Result, u16)>> { +pub(crate) fn read_acl(handle: &OwnedHandle) -> Result, u16)>> { let mut acl = ptr::null_mut(); let mut descriptor = ptr::null_mut(); let status = unsafe { @@ -156,7 +156,7 @@ fn read_acl(handle: &OwnedHandle) -> Result, u16)>> { ) }; if status != 0 { - return Err(io::Error::from_raw_os_error(status as i32)).context("read AppData DACL"); + return Err(io::Error::from_raw_os_error(status as i32)).context("read directory DACL"); } let result = (|| { if acl.is_null() { @@ -164,7 +164,7 @@ fn read_acl(handle: &OwnedHandle) -> Result, u16)>> { } ensure!( unsafe { security::IsValidAcl(acl) } != 0, - "invalid AppData DACL" + "invalid directory DACL" ); let length = unsafe { (*acl).AclSize } as usize; let mut control = 0; @@ -172,7 +172,7 @@ fn read_acl(handle: &OwnedHandle) -> Result, u16)>> { let queried = unsafe { security::GetSecurityDescriptorControl(descriptor, &mut control, &mut revision) }; - BOOL(queried).ok().context("read AppData DACL control")?; + BOOL(queried).ok().context("read directory DACL control")?; let mut buffer = vec![0u32; length.div_ceil(4)]; unsafe { ptr::copy_nonoverlapping(acl.cast::(), buffer.as_mut_ptr().cast(), length); @@ -279,14 +279,14 @@ fn insert_metadata_ace(acl: &mut Vec, sid: &LocalSid) -> Result<()> { Ok(()) } -fn write_acl(handle: &OwnedHandle, acl: &mut [u32], control: u16) -> Result<()> { +pub(crate) fn write_acl(handle: &OwnedHandle, acl: &mut [u32], control: u16) -> Result<()> { let mut descriptor: security::SECURITY_DESCRIPTOR = unsafe { std::mem::zeroed() }; let preserved = security::SE_DACL_PROTECTED | security::SE_DACL_AUTO_INHERITED; let raw_descriptor = (&mut descriptor as *mut security::SECURITY_DESCRIPTOR).cast(); unsafe { BOOL(security::InitializeSecurityDescriptor(raw_descriptor, 1)) .ok() - .context("initialize AppData security descriptor")?; + .context("initialize directory security descriptor")?; BOOL(security::SetSecurityDescriptorDacl( raw_descriptor, 1, @@ -294,14 +294,14 @@ fn write_acl(handle: &OwnedHandle, acl: &mut [u32], control: u16) -> Result<()> i32::from(control & security::SE_DACL_DEFAULTED != 0), )) .ok() - .context("set AppData DACL")?; + .context("set directory DACL")?; BOOL(security::SetSecurityDescriptorControl( raw_descriptor, preserved, control & preserved, )) .ok() - .context("preserve AppData DACL control")?; + .context("preserve directory DACL control")?; } // SetSecurityInfo recursively reapplies existing inheritable ACEs. The native // object setter changes only this retained, owner-authorized directory handle. @@ -316,7 +316,7 @@ fn write_acl(handle: &OwnedHandle, acl: &mut [u32], control: u16) -> Result<()> return Err(io::Error::from_raw_os_error( unsafe { RtlNtStatusToDosError(status) } as i32, )) - .context("write AppData metadata DACL"); + .context("write directory DACL"); } Ok(()) }