Repair legacy Windows sandbox bin permissions for SYSTEM (#59745)

## Why

Legacy `.sandbox-bin` ACLs grant `SYSTEM` Modify access without `WRITE_DAC`, preventing the provisioning service from reapplying the bin policy.

## What changed

- Grant `SYSTEM` inheritable `WRITE_DAC` access when protecting the sandbox bin so it can update permissions on the directory and its children.
- Before legacy provisioning, repair the old ACL through an authenticated owner handle when `SYSTEM` is denied access. Upgrade only the expected SYSTEM entry in a protected, allow-only ACL, reject unexpected permissions, and verify that `SYSTEM` can reopen the directory with `WRITE_DAC`.

## Testing

Extend ACL coverage to check `SYSTEM` permissions on the bin and existing and newly created runner files. Add an ignored Windows integration test requiring LocalSystem and user-owned legacy homes to cover successful repair, repeated repair, and rejection of an explicit SYSTEM deny entry.

GitOrigin-RevId: 2303cdf5182947c2d044c79ac7027a16f0373c7d

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(

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,

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::<std::io::Error>()
+                .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")

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()

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::<io::Error>()
+                .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::<security::ACL>();
+    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::<security::ACE_HEADER>() };
+        ensure!(
+            header.AceType == 0 && header.AceSize >= 16,
+            "unsupported legacy sandbox bin ACE"
+        );
+        let allow = unsafe { &mut *ace.cast::<security::ACCESS_ALLOWED_ACE>() };
+        let sid = std::ptr::addr_of!(allow.SidStart).cast::<u8>();
+        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(())
+}

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;
 

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<Option<(Vec<u32>, u16)>> {
+pub(crate) fn read_acl(handle: &OwnedHandle) -> Result<Option<(Vec<u32>, 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<Option<(Vec<u32>, 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<Option<(Vec<u32>, 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<Option<(Vec<u32>, 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::<u8>(), buffer.as_mut_ptr().cast(), length);
@@ -279,14 +279,14 @@ fn insert_metadata_ace(acl: &mut Vec<u32>, 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(())
 }