Repair Windows sandbox access to existing runtime children (#46241)

## Why A runtime directory can have read/execute access while existing files and subdirectories lack inherited permissions. Checking only the runtime roots leaves those children inaccessible to sandbox users. ## What changed - Walk the Codex app runtime tree and add missing read/execute grants for the sandbox group, validating access after changes. - Use `GRANT_ACCESS` to preserve existing grants and skip additions that could override deny entries, including inherited denials for other trustees. - Skip reparse points and trees with reparse-point ancestors, tolerate disappearing paths, and report repair failures through setup logging and refresh errors. ## Testing Add Windows regression tests for repairing children beneath an already-readable root, repeated repairs, preserving read/write denials and other trustees' inherited file denials, avoiding write or ACL-management grants, and leaving junction targets untouched. GitOrigin-RevId: 93d55dd9fca2b24eac157ae9be505982d07dd9b9
2026-11-06 20:20:34 +00:00 · 2026-09-17 15:44:11 +00:00
parent 32b54cffdd
commit 2833985d88
3 changed files with 370 additions and 12 deletions
@@ -21,9 +21,12 @@ use windows_sys::Win32::Security::ACE_HEADER;
 use windows_sys::Win32::Security::ACL;
 use windows_sys::Win32::Security::ACL_SIZE_INFORMATION;
 use windows_sys::Win32::Security::AclSizeInformation;
+use windows_sys::Win32::Security::Authorization::ACCESS_MODE;
 use windows_sys::Win32::Security::Authorization::EXPLICIT_ACCESS_W;
+use windows_sys::Win32::Security::Authorization::GRANT_ACCESS;
 use windows_sys::Win32::Security::Authorization::GetNamedSecurityInfoW;
 use windows_sys::Win32::Security::Authorization::GetSecurityInfo;
+use windows_sys::Win32::Security::Authorization::SET_ACCESS;
 use windows_sys::Win32::Security::Authorization::SetEntriesInAclW;
 use windows_sys::Win32::Security::Authorization::SetNamedSecurityInfoW;
 use windows_sys::Win32::Security::Authorization::SetSecurityInfo;
@@ -68,6 +71,8 @@ const ACCESS_ALLOWED_ACE_TYPE: u8 = 0;
 const ACCESS_DENIED_ACE_TYPE: u8 = 1;
 const GENERIC_READ_MASK: u32 = 0x8000_0000;
 const GENERIC_WRITE_MASK: u32 = 0x4000_0000;
+const GENERIC_EXECUTE_MASK: u32 = 0x2000_0000;
+const GENERIC_ALL_MASK: u32 = 0x1000_0000;
 const DENY_ACCESS: i32 = 3;
 // TrustedInstaller is a deterministic service SID, not a machine-local account SID.
 const TRUSTED_INSTALLER_SID: &str =
@@ -444,6 +449,19 @@ pub unsafe fn dacl_has_write_deny_for_sid(p_dacl: *mut ACL, psid: *mut c_void) -
 }

 pub unsafe fn dacl_has_read_deny_for_sid(p_dacl: *mut ACL, psid: *mut c_void) -> bool {
+    dacl_has_deny_mask(
+        p_dacl,
+        DenyAceScope::EffectiveForSid(psid),
+        FILE_GENERIC_READ | GENERIC_READ_MASK,
+    )
+}
+
+enum DenyAceScope {
+    EffectiveForSid(*mut c_void),
+    Any,
+}
+
+unsafe fn dacl_has_deny_mask(p_dacl: *mut ACL, scope: DenyAceScope, deny_mask: u32) -> bool {
    if p_dacl.is_null() {
        return false;
    }
@@ -457,7 +475,6 @@ pub unsafe fn dacl_has_read_deny_for_sid(p_dacl: *mut ACL, psid: *mut c_void) ->
    if ok == 0 {
        return false;
    }
-    let deny_read_mask = FILE_GENERIC_READ | GENERIC_READ_MASK;
    for i in 0..info.AceCount {
        let mut p_ace: *mut c_void = std::ptr::null_mut();
        if GetAce(p_dacl as *const ACL, i, &mut p_ace) == 0 {
@@ -467,14 +484,14 @@ pub unsafe fn dacl_has_read_deny_for_sid(p_dacl: *mut ACL, psid: *mut c_void) ->
        if hdr.AceType != ACCESS_DENIED_ACE_TYPE {
            continue; // ACCESS_DENIED_ACE_TYPE
        }
-        if (hdr.AceFlags & INHERIT_ONLY_ACE) != 0 {
+        let ace = &*(p_ace as *const ACCESS_DENIED_ACE);
+        if let DenyAceScope::EffectiveForSid(psid) = scope
+            && ((hdr.AceFlags & INHERIT_ONLY_ACE) != 0
+                || EqualSid(std::ptr::addr_of!(ace.SidStart) as *mut c_void, psid) == 0)
+        {
            continue;
        }
-        let ace = &*(p_ace as *const ACCESS_DENIED_ACE);
-        let base = p_ace as usize;
-        let sid_ptr =
-            (base + std::mem::size_of::<ACE_HEADER>() + std::mem::size_of::<u32>()) as *mut c_void;
-        if EqualSid(sid_ptr, psid) != 0 && (ace.Mask & deny_read_mask) != 0 {
+        if (ace.Mask & deny_mask) != 0 {
            return true;
        }
    }
@@ -522,17 +539,33 @@ unsafe fn ensure_allow_mask_aces_with_inheritance_impl(
    sids: &[*mut c_void],
    allow_mask: u32,
    disallow_mask: u32,
+    access_mode: ACCESS_MODE,
    inheritance: u32,
 ) -> Result<bool> {
    let (p_dacl, p_sd) = fetch_dacl_handle(path)?;
    let mut entries: Vec<EXPLICIT_ACCESS_W> = Vec::new();
    for sid in sids {
+        // A new allow can outrank another trustee's inherited deny, including a
+        // file-only deny on children. Without the complete token, preserve every deny.
+        if access_mode == GRANT_ACCESS
+            && dacl_has_deny_mask(
+                p_dacl,
+                DenyAceScope::Any,
+                allow_mask
+                    | GENERIC_READ_MASK
+                    | GENERIC_WRITE_MASK
+                    | GENERIC_EXECUTE_MASK
+                    | GENERIC_ALL_MASK,
+            )
+        {
+            continue;
+        }
        if !dacl_allow_mask_needs_refresh(p_dacl, *sid, allow_mask, disallow_mask) {
            continue;
        }
        entries.push(EXPLICIT_ACCESS_W {
            grfAccessPermissions: allow_mask,
-            grfAccessMode: 2, // SET_ACCESS
+            grfAccessMode: access_mode,
            grfInheritance: inheritance,
            Trustee: TRUSTEE_W {
                pMultipleTrustee: std::ptr::null_mut(),
@@ -605,6 +638,27 @@ pub unsafe fn ensure_allow_mask_aces_with_inheritance(
        sids,
        allow_mask,
        /*disallow_mask*/ 0,
+        SET_ACCESS,
+        inheritance,
+    )
+}
+
+/// Ensure read/execute grants without replacing existing entries or overriding denies.
+/// Returns true if any ACE was added.
+///
+/// # Safety
+/// Caller must pass valid SID pointers and an existing path.
+pub(crate) unsafe fn grant_read_execute_aces(
+    path: &Path,
+    sids: &[*mut c_void],
+    inheritance: u32,
+) -> Result<bool> {
+    ensure_allow_mask_aces_with_inheritance_impl(
+        path,
+        sids,
+        FILE_GENERIC_READ | FILE_GENERIC_EXECUTE,
+        /*disallow_mask*/ 0,
+        GRANT_ACCESS,
        inheritance,
    )
 }
@@ -638,6 +692,7 @@ pub unsafe fn ensure_allow_write_aces(path: &Path, sids: &[*mut c_void]) -> Resu
        sids,
        WRITE_ALLOW_MASK,
        FILE_DELETE_CHILD,
+        SET_ACCESS,
        CONTAINER_INHERIT_ACE | OBJECT_INHERIT_ACE,
    )
 }
@@ -1,11 +1,14 @@
 use std::ffi::c_void;
 use std::io::Write;
 use std::os::windows::fs::MetadataExt as _;
+use std::path::Path;
 use std::path::PathBuf;

-use crate::ensure_allow_mask_aces_with_inheritance;
+use crate::acl::grant_read_execute_aces;
 use crate::path_mask_allows;
+use anyhow::Context;
 use anyhow::Result;
+use anyhow::ensure;
 use windows_sys::Win32::Security::CONTAINER_INHERIT_ACE;
 use windows_sys::Win32::Security::OBJECT_INHERIT_ACE;
 use windows_sys::Win32::Storage::FileSystem::FILE_ATTRIBUTE_REPARSE_POINT;
@@ -22,8 +25,9 @@ pub(super) fn ensure_codex_app_runtime_paths_readable(
    log: &mut dyn Write,
 ) -> Result<()> {
    let read_execute_mask = FILE_GENERIC_READ | FILE_GENERIC_EXECUTE;
+    let local_app_data = local_app_data_root();
    let runtime_paths = runtime_paths(
-        local_app_data_root(),
+        local_app_data.clone(),
        std::env::var_os("USERPROFILE").map(PathBuf::from),
    );

@@ -68,10 +72,9 @@ pub(super) fn ensure_codex_app_runtime_paths_readable(
            ),
        )?;
        let result = unsafe {
-            ensure_allow_mask_aces_with_inheritance(
+            grant_read_execute_aces(
                &runtime_path,
                &[sandbox_group_psid],
-                read_execute_mask,
                OBJECT_INHERIT_ACE | CONTAINER_INHERIT_ACE,
            )
        };
@@ -89,6 +92,93 @@ pub(super) fn ensure_codex_app_runtime_paths_readable(
            )?;
        }
    }
+    if let Some(local_app_data) = local_app_data {
+        let runtime_root = local_app_data.join("OpenAI").join("Codex").join("runtimes");
+        if let Err(err) = ensure_runtime_tree_readable(&runtime_root, sandbox_group_psid) {
+            let message = format!("runtime read/execute validation failed: {err:#}");
+            super::log_line(log, &message)?;
+            refresh_errors.push(message);
+        }
+    }
+    Ok(())
+}
+
+fn ensure_runtime_tree_readable(
+    runtime_root: &Path,
+    sandbox_group_psid: *mut c_void,
+) -> Result<()> {
+    for ancestor in runtime_root.ancestors() {
+        let metadata = match std::fs::symlink_metadata(ancestor) {
+            Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Ok(()),
+            result => result
+                .with_context(|| format!("inspect runtime ancestor {}", ancestor.display()))?,
+        };
+        if !metadata.is_dir() || metadata.file_attributes() & FILE_ATTRIBUTE_REPARSE_POINT != 0 {
+            return Ok(());
+        }
+    }
+
+    let read_execute_mask = FILE_GENERIC_READ | FILE_GENERIC_EXECUTE;
+    let mut paths = vec![runtime_root.to_path_buf()];
+    while let Some(path) = paths.pop() {
+        let metadata = match std::fs::symlink_metadata(&path) {
+            Err(err) if err.kind() == std::io::ErrorKind::NotFound => continue,
+            result => result.with_context(|| format!("inspect runtime path {}", path.display()))?,
+        };
+        if metadata.file_attributes() & FILE_ATTRIBUTE_REPARSE_POINT != 0 {
+            continue;
+        }
+
+        // A readable parent does not prove that existing runtime children inherited its ACL.
+        let access_result = unsafe {
+            grant_read_execute_aces(
+                &path,
+                &[sandbox_group_psid],
+                if metadata.is_dir() {
+                    OBJECT_INHERIT_ACE | CONTAINER_INHERIT_ACE
+                } else {
+                    0
+                },
+            )
+        }
+        .and_then(|changed| {
+            if changed {
+                ensure!(
+                    path_mask_allows(
+                        &path,
+                        &[sandbox_group_psid],
+                        read_execute_mask,
+                        /*require_all_bits*/ true,
+                    )?,
+                    "runtime read/execute access still missing"
+                );
+            }
+            Ok(())
+        });
+        if let Err(err) = access_result {
+            if std::fs::symlink_metadata(&path)
+                .is_err_and(|err| err.kind() == std::io::ErrorKind::NotFound)
+            {
+                continue;
+            }
+            return Err(err).with_context(|| {
+                format!("validate runtime read/execute access on {}", path.display())
+            });
+        }
+        if metadata.is_dir() {
+            let entries = match std::fs::read_dir(&path) {
+                Err(err) if err.kind() == std::io::ErrorKind::NotFound => continue,
+                result => result
+                    .with_context(|| format!("enumerate runtime directory {}", path.display()))?,
+            };
+            for entry in entries {
+                match entry {
+                    Err(err) if err.kind() == std::io::ErrorKind::NotFound => continue,
+                    result => paths.push(result?.path()),
+                }
+            }
+        }
+    }
    Ok(())
 }

@@ -1,6 +1,219 @@
+use super::ensure_runtime_tree_readable;
 use super::runtime_paths;
+use crate::LocalSid;
+use crate::add_deny_read_ace;
+use crate::add_deny_write_ace;
+use crate::ensure_allow_mask_aces_with_inheritance;
+use crate::path_mask_allows;
 use pretty_assertions::assert_eq;
+use std::fs;
 use std::path::PathBuf;
+use windows_sys::Win32::Foundation::HLOCAL;
+use windows_sys::Win32::Foundation::LocalFree;
+use windows_sys::Win32::Security::Authorization::ConvertStringSecurityDescriptorToSecurityDescriptorW;
+use windows_sys::Win32::Security::Authorization::SDDL_REVISION_1;
+use windows_sys::Win32::Security::CheckTokenMembership;
+use windows_sys::Win32::Security::DACL_SECURITY_INFORMATION;
+use windows_sys::Win32::Security::PROTECTED_DACL_SECURITY_INFORMATION;
+use windows_sys::Win32::Security::SetFileSecurityW;
+use windows_sys::Win32::Storage::FileSystem::DELETE;
+use windows_sys::Win32::Storage::FileSystem::FILE_APPEND_DATA;
+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_WRITE_DATA;
+use windows_sys::Win32::Storage::FileSystem::WRITE_DAC;
+
+#[test]
+fn runtime_repair_preserves_other_trustees_inherited_file_denial() {
+    for (denied_trustee, granted_sid) in [("WD", "S-1-5-11"), ("S-1-5-32-545", "S-1-1-0")] {
+        let ancestor = tempfile::tempdir().expect("runtime ancestor");
+        let granted_sid = LocalSid::from_string(granted_sid).expect("grant SID");
+        let mut member = 0;
+        assert_ne!(
+            unsafe {
+                CheckTokenMembership(/*tokenhandle*/ 0, granted_sid.as_ptr(), &mut member)
+            },
+            0,
+        );
+        assert_ne!(member, 0, "the real read must exercise the added grant");
+
+        unsafe {
+            let mut descriptor = std::ptr::null_mut();
+            assert_ne!(
+                ConvertStringSecurityDescriptorToSecurityDescriptorW(
+                    crate::winutil::to_wide(format!(
+                        "D:P(D;OIIO;0x1;;;{denied_trustee})(A;OICI;FA;;;OW)"
+                    ))
+                    .as_ptr(),
+                    SDDL_REVISION_1,
+                    &mut descriptor,
+                    std::ptr::null_mut(),
+                ),
+                0,
+            );
+            let set = SetFileSecurityW(
+                crate::winutil::to_wide(ancestor.path()).as_ptr(),
+                DACL_SECURITY_INFORMATION | PROTECTED_DACL_SECURITY_INFORMATION,
+                descriptor,
+            );
+            LocalFree(descriptor as HLOCAL);
+            assert_ne!(set, 0, "install an inheritable file-data denial");
+        }
+        let runtime = ancestor.path().join("runtimes");
+        fs::create_dir(&runtime).expect("runtime directory");
+        let module = runtime.join("private.js");
+        fs::write(&module, b"private runtime content").expect("runtime file");
+        for repair in [false, true] {
+            if repair {
+                ensure_runtime_tree_readable(&runtime, granted_sid.as_ptr())
+                    .expect("repair must preserve inherited denies");
+            }
+            assert_eq!(
+                fs::read(&module)
+                    .expect_err("file-data denial must remain effective")
+                    .kind(),
+                std::io::ErrorKind::PermissionDenied,
+            );
+        }
+    }
+}
+
+#[test]
+fn runtime_repair_does_not_follow_directory_junctions() {
+    let runtime = tempfile::tempdir().expect("runtime directory");
+    let external = tempfile::tempdir().expect("external directory");
+    let external_file = external.path().join("private.js");
+    fs::write(&external_file, b"external content").expect("external file");
+    let alias = runtime.path().join("linked-runtime");
+    let output = std::process::Command::new("cmd")
+        .args(["/C", "mklink", "/J"])
+        .arg(&alias)
+        .arg(external.path())
+        .output()
+        .expect("create directory junction");
+    assert!(output.status.success(), "mklink failed: {output:?}");
+    let sandbox_sid = LocalSid::from_string("S-1-5-21-10-20-30-40").expect("test SID");
+    for root in [runtime.path(), alias.as_path()] {
+        ensure_runtime_tree_readable(root, sandbox_sid.as_ptr()).expect("repair runtimes");
+        for path in [external.path(), external_file.as_path()] {
+            assert!(
+                !path_mask_allows(
+                    path,
+                    &[sandbox_sid.as_ptr()],
+                    FILE_GENERIC_READ | FILE_GENERIC_EXECUTE,
+                    /*require_all_bits*/ false,
+                )
+                .expect("external ACL must remain untouched")
+            );
+        }
+    }
+    fs::remove_dir(&alias).expect("remove junction");
+}
+
+#[test]
+fn repairs_children_when_runtime_root_already_has_read_execute_access() {
+    let runtime = tempfile::tempdir().expect("runtime directory");
+    let node = runtime.path().join("bin/node.exe");
+    let module = runtime.path().join("sky/dist/project/index.js");
+    for path in [&node, &module] {
+        fs::create_dir_all(path.parent().expect("parent")).expect("runtime subdirectory");
+        fs::write(path, b"runtime content").expect("runtime file");
+    }
+    let sandbox_sid = LocalSid::from_string("S-1-5-21-10-20-30-40").expect("test SID");
+    let read_execute_mask = FILE_GENERIC_READ | FILE_GENERIC_EXECUTE;
+    unsafe {
+        ensure_allow_mask_aces_with_inheritance(
+            runtime.path(),
+            &[sandbox_sid.as_ptr()],
+            read_execute_mask,
+            /*inheritance*/ 0,
+        )
+    }
+    .expect("grant root-only read/execute access");
+    assert!(
+        path_mask_allows(
+            runtime.path(),
+            &[sandbox_sid.as_ptr()],
+            read_execute_mask,
+            /*require_all_bits*/ true,
+        )
+        .expect("root access")
+    );
+    for path in [&node, &module] {
+        assert!(
+            !path_mask_allows(
+                path,
+                &[sandbox_sid.as_ptr()],
+                read_execute_mask,
+                /*require_all_bits*/ true,
+            )
+            .expect("child access before repair")
+        );
+    }
+
+    let denied_directory = runtime.path().join("denied");
+    fs::create_dir(&denied_directory).expect("denied directory");
+    assert!(
+        unsafe { add_deny_read_ace(&denied_directory, sandbox_sid.as_ptr()) }
+            .expect("deny directory reads")
+    );
+    let denied_child = denied_directory.join("private.js");
+    fs::write(&denied_child, b"private runtime content").expect("denied child");
+    let write_denied_file = runtime.path().join("read-only.js");
+    fs::write(&write_denied_file, b"read-only runtime content").expect("write-denied file");
+    assert!(
+        unsafe { add_deny_write_ace(&write_denied_file, sandbox_sid.as_ptr()) }
+            .expect("deny runtime writes")
+    );
+
+    for _ in 0..2 {
+        ensure_runtime_tree_readable(runtime.path(), sandbox_sid.as_ptr())
+            .expect("repair runtimes");
+        for path in [&denied_directory, &denied_child] {
+            assert!(
+                !unsafe { add_deny_read_ace(path, sandbox_sid.as_ptr()) }
+                    .expect("read denial must survive repair")
+            );
+            assert!(
+                !path_mask_allows(
+                    path,
+                    &[sandbox_sid.as_ptr()],
+                    read_execute_mask,
+                    /*require_all_bits*/ false,
+                )
+                .expect("repair must not add an allow ahead of inherited denial")
+            );
+        }
+        assert!(
+            !unsafe { add_deny_write_ace(&write_denied_file, sandbox_sid.as_ptr()) }
+                .expect("write denial must survive repair")
+        );
+        for path in [
+            node.as_path(),
+            module.as_path(),
+            module.parent().expect("module directory"),
+        ] {
+            assert!(
+                path_mask_allows(
+                    path,
+                    &[sandbox_sid.as_ptr()],
+                    read_execute_mask,
+                    /*require_all_bits*/ true,
+                )
+                .expect("child access after repair")
+            );
+            assert!(
+                !path_mask_allows(
+                    path,
+                    &[sandbox_sid.as_ptr()],
+                    FILE_WRITE_DATA | FILE_APPEND_DATA | DELETE | WRITE_DAC,
+                    /*require_all_bits*/ false,
+                )
+                .expect("repair must not grant write or ACL management")
+            );
+        }
+    }
+}

 #[test]
 fn runtime_paths_include_desktop_parent_and_primary_runtime_roots() {