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
2026-11-12 21:20:34 +00:00 · 2026-11-12 00:28:23 +00:00
parent 77cbcdb4a6
commit 5a529c2909
3 changed files with 132 additions and 23 deletions
@@ -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(
@@ -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<EXPLICIT_ACCESS_W> = 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<bool> {
+    unsafe fn entries(acl: &ACL) -> Result<Vec<&[u8]>> {
+        (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::<security::ACE_HEADER>()).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| {
@@ -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] {