Elevated Sandbox 1 (#7788)

- updating helpers, refactoring some functions that will be used in the
elevated sandbox
- better logging
- better and faster handling of ACL checks/writes
- No functional change—legacy restricted-token sandbox
remains the only path.

codex-rs/Cargo.lock

@@ -1727,6 +1727,7 @@ name = "codex-windows-sandbox"
 version = "0.0.0"
 dependencies = [
  "anyhow",
+ "chrono",
  "codex-protocol",
  "dirs-next",
  "dunce",

codex-rs/windows-sandbox-rs/Cargo.toml

@@ -10,6 +10,7 @@ path = "src/lib.rs"
 
 [dependencies]
 anyhow = "1.0"
+chrono = { version = "0.4.42", default-features = false, features = ["clock", "std"] }
 dunce = "1.0"
 serde = { version = "1.0", features = ["derive"] }
 serde_json = "1.0"

codex-rs/windows-sandbox-rs/src/acl.rs

@@ -1,4 +1,4 @@
-use crate::winutil::to_wide;
+use crate::winutil::to_wide;
 use anyhow::anyhow;
 use anyhow::Result;
 use std::ffi::c_void;
@@ -9,6 +9,7 @@ use windows_sys::Win32::Foundation::ERROR_SUCCESS;
 use windows_sys::Win32::Foundation::HLOCAL;
 use windows_sys::Win32::Foundation::INVALID_HANDLE_VALUE;
 use windows_sys::Win32::Security::AclSizeInformation;
+use windows_sys::Win32::Security::Authorization::GetEffectiveRightsFromAclW;
 use windows_sys::Win32::Security::Authorization::GetNamedSecurityInfoW;
 use windows_sys::Win32::Security::Authorization::GetSecurityInfo;
 use windows_sys::Win32::Security::Authorization::SetEntriesInAclW;
@@ -21,28 +22,148 @@ use windows_sys::Win32::Security::Authorization::TRUSTEE_W;
 use windows_sys::Win32::Security::EqualSid;
 use windows_sys::Win32::Security::GetAce;
 use windows_sys::Win32::Security::GetAclInformation;
+use windows_sys::Win32::Security::MapGenericMask;
 use windows_sys::Win32::Security::ACCESS_ALLOWED_ACE;
 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::DACL_SECURITY_INFORMATION;
+use windows_sys::Win32::Security::GENERIC_MAPPING;
 use windows_sys::Win32::Storage::FileSystem::CreateFileW;
+use windows_sys::Win32::Storage::FileSystem::FILE_ALL_ACCESS;
+use windows_sys::Win32::Storage::FileSystem::FILE_APPEND_DATA;
 use windows_sys::Win32::Storage::FileSystem::FILE_ATTRIBUTE_NORMAL;
+use windows_sys::Win32::Storage::FileSystem::FILE_FLAG_BACKUP_SEMANTICS;
 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_GENERIC_WRITE;
-use windows_sys::Win32::Storage::FileSystem::FILE_APPEND_DATA;
+use windows_sys::Win32::Storage::FileSystem::FILE_SHARE_DELETE;
+use windows_sys::Win32::Storage::FileSystem::FILE_SHARE_READ;
+use windows_sys::Win32::Storage::FileSystem::FILE_SHARE_WRITE;
 use windows_sys::Win32::Storage::FileSystem::FILE_WRITE_ATTRIBUTES;
 use windows_sys::Win32::Storage::FileSystem::FILE_WRITE_DATA;
 use windows_sys::Win32::Storage::FileSystem::FILE_WRITE_EA;
-use windows_sys::Win32::Storage::FileSystem::FILE_SHARE_READ;
-use windows_sys::Win32::Storage::FileSystem::FILE_SHARE_WRITE;
 use windows_sys::Win32::Storage::FileSystem::OPEN_EXISTING;
+use windows_sys::Win32::Storage::FileSystem::READ_CONTROL;
 const SE_KERNEL_OBJECT: u32 = 6;
 const INHERIT_ONLY_ACE: u8 = 0x08;
 const GENERIC_WRITE_MASK: u32 = 0x4000_0000;
 const DENY_ACCESS: i32 = 3;
 
+/// Fetch DACL via handle-based query; caller must LocalFree the returned SD.
+pub unsafe fn fetch_dacl_handle(path: &Path) -> Result<(*mut ACL, *mut c_void)> {
+    let wpath = to_wide(path);
+    let h = CreateFileW(
+        wpath.as_ptr(),
+        READ_CONTROL,
+        FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
+        std::ptr::null_mut(),
+        OPEN_EXISTING,
+        FILE_FLAG_BACKUP_SEMANTICS,
+        0,
+    );
+    if h == INVALID_HANDLE_VALUE {
+        return Err(anyhow!("CreateFileW failed for {}", path.display()));
+    }
+    let mut p_sd: *mut c_void = std::ptr::null_mut();
+    let mut p_dacl: *mut ACL = std::ptr::null_mut();
+    let code = GetSecurityInfo(
+        h,
+        1, // SE_FILE_OBJECT
+        DACL_SECURITY_INFORMATION,
+        std::ptr::null_mut(),
+        std::ptr::null_mut(),
+        &mut p_dacl,
+        std::ptr::null_mut(),
+        &mut p_sd,
+    );
+    CloseHandle(h);
+    if code != ERROR_SUCCESS {
+        return Err(anyhow!(
+            "GetSecurityInfo failed for {}: {}",
+            path.display(),
+            code
+        ));
+    }
+    Ok((p_dacl, p_sd))
+}
+
+/// Fast mask-based check: does any ACE for provided SIDs grant at least one desired bit? Skips inherit-only.
+pub unsafe fn dacl_quick_mask_allows(
+    p_dacl: *mut ACL,
+    psids: &[*mut c_void],
+    desired_mask: u32,
+) -> bool {
+    if p_dacl.is_null() {
+        return false;
+    }
+    let mut info: ACL_SIZE_INFORMATION = std::mem::zeroed();
+    let ok = GetAclInformation(
+        p_dacl as *const ACL,
+        &mut info as *mut _ as *mut c_void,
+        std::mem::size_of::<ACL_SIZE_INFORMATION>() as u32,
+        AclSizeInformation,
+    );
+    if ok == 0 {
+        return false;
+    }
+    let mapping = GENERIC_MAPPING {
+        GenericRead: FILE_GENERIC_READ,
+        GenericWrite: FILE_GENERIC_WRITE,
+        GenericExecute: FILE_GENERIC_EXECUTE,
+        GenericAll: FILE_ALL_ACCESS,
+    };
+    for i in 0..(info.AceCount as usize) {
+        let mut p_ace: *mut c_void = std::ptr::null_mut();
+        if GetAce(p_dacl as *const ACL, i as u32, &mut p_ace) == 0 {
+            continue;
+        }
+        let hdr = &*(p_ace as *const ACE_HEADER);
+        if hdr.AceType != 0 {
+            continue; // not ACCESS_ALLOWED
+        }
+        if (hdr.AceFlags & INHERIT_ONLY_ACE) != 0 {
+            continue;
+        }
+        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;
+        let mut matched = false;
+        for sid in psids {
+            if EqualSid(sid_ptr, *sid) != 0 {
+                matched = true;
+                break;
+            }
+        }
+        if !matched {
+            continue;
+        }
+        let ace = &*(p_ace as *const ACCESS_ALLOWED_ACE);
+        let mut mask = ace.Mask;
+        MapGenericMask(&mut mask, &mapping);
+        if (mask & desired_mask) != 0 {
+            return true;
+        }
+    }
+    false
+}
+
+/// Path-based wrapper around the quick mask check (single DACL fetch).
+pub fn path_quick_mask_allows(
+    path: &Path,
+    psids: &[*mut c_void],
+    desired_mask: u32,
+) -> Result<bool> {
+    unsafe {
+        let (p_dacl, sd) = fetch_dacl_handle(path)?;
+        let has = dacl_quick_mask_allows(p_dacl, psids, desired_mask);
+        if !sd.is_null() {
+            LocalFree(sd as HLOCAL);
+        }
+        Ok(has)
+    }
+}
+
 pub unsafe fn dacl_has_write_allow_for_sid(p_dacl: *mut ACL, psid: *mut c_void) -> bool {
     if p_dacl.is_null() {
         return false;
@@ -131,6 +252,44 @@ pub unsafe fn dacl_has_write_deny_for_sid(p_dacl: *mut ACL, psid: *mut c_void) -
 // This accounts for deny ACEs and ordering; falls back to a conservative per-ACE scan if the API fails.
 #[allow(dead_code)]
 pub unsafe fn dacl_effective_allows_write(p_dacl: *mut ACL, psid: *mut c_void) -> bool {
+    if p_dacl.is_null() {
+        return false;
+    }
+    let trustee = TRUSTEE_W {
+        pMultipleTrustee: std::ptr::null_mut(),
+        MultipleTrusteeOperation: 0,
+        TrusteeForm: TRUSTEE_IS_SID,
+        TrusteeType: TRUSTEE_IS_UNKNOWN,
+        ptstrName: psid as *mut u16,
+    };
+    let mut access: u32 = 0;
+    let ok = GetEffectiveRightsFromAclW(p_dacl, &trustee, &mut access);
+    if ok == ERROR_SUCCESS {
+        // Map generic bits to avoid “missing” write when generic permissions are present.
+        let mut mapped_access = access;
+        if (access & GENERIC_WRITE_MASK) != 0 {
+            mapped_access |= FILE_GENERIC_WRITE | FILE_WRITE_DATA | FILE_APPEND_DATA;
+        }
+        if (access & READ_CONTROL) != 0 {
+            mapped_access |= FILE_GENERIC_READ;
+        }
+        let write_bits = FILE_GENERIC_WRITE
+            | FILE_WRITE_DATA
+            | FILE_APPEND_DATA
+            | FILE_WRITE_EA
+            | FILE_WRITE_ATTRIBUTES;
+        return (mapped_access & write_bits) != 0;
+    }
+    // Fallback: simple allow ACE scan (already ignores inherit-only)
+    dacl_has_write_allow_for_sid(p_dacl, psid)
+}
+
+#[allow(dead_code)]
+pub unsafe fn dacl_effective_allows_mask(
+    p_dacl: *mut ACL,
+    psid: *mut c_void,
+    desired_mask: u32,
+) -> bool {
     if p_dacl.is_null() {
         return false;
     }
@@ -148,18 +307,100 @@ pub unsafe fn dacl_effective_allows_write(p_dacl: *mut ACL, psid: *mut c_void) -
     };
     let mut access: u32 = 0;
     let ok = GetEffectiveRightsFromAclW(p_dacl, &trustee, &mut access);
-    if ok != 0 {
+    if ok == ERROR_SUCCESS {
-        // Check for generic or specific write bits
+        // Map generic bits to avoid “missing” when generic permissions are present.
-        let write_bits = FILE_GENERIC_WRITE
+        let mut mapped_access = access;
-            | windows_sys::Win32::Storage::FileSystem::FILE_WRITE_DATA
+        if (access & GENERIC_WRITE_MASK) != 0 {
-            | windows_sys::Win32::Storage::FileSystem::FILE_APPEND_DATA
+            mapped_access |= FILE_GENERIC_WRITE | FILE_WRITE_DATA | FILE_APPEND_DATA;
-            | windows_sys::Win32::Storage::FileSystem::FILE_WRITE_EA
+        }
-            | windows_sys::Win32::Storage::FileSystem::FILE_WRITE_ATTRIBUTES;
+        if (access & READ_CONTROL) != 0 {
-        return (access & write_bits) != 0;
+            mapped_access |= FILE_GENERIC_READ;
+        }
+        return (mapped_access & desired_mask) == desired_mask;
     }
-    // Fallback: simple allow ACE scan (already ignores inherit-only)
+    // Fallbacks on error: if write bits are requested, reuse the write helper; otherwise fail closed.
-    dacl_has_write_allow_for_sid(p_dacl, psid)
+    if (desired_mask & FILE_GENERIC_WRITE) != 0 {
+        return dacl_effective_allows_write(p_dacl, psid);
+    }
+    false
 }
+
+#[allow(dead_code)]
+const WRITE_ALLOW_MASK: u32 = FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE;
+
+/// Ensure all provided SIDs have a write-capable allow ACE on the path.
+/// Returns true if any ACE was added.
+#[allow(dead_code)]
+pub unsafe fn ensure_allow_write_aces(path: &Path, sids: &[*mut c_void]) -> Result<bool> {
+    let (p_dacl, p_sd) = fetch_dacl_handle(path)?;
+    let mut entries: Vec<EXPLICIT_ACCESS_W> = Vec::new();
+    for sid in sids {
+        if dacl_quick_mask_allows(p_dacl, &[*sid], WRITE_ALLOW_MASK) {
+            continue;
+        }
+        entries.push(EXPLICIT_ACCESS_W {
+            grfAccessPermissions: WRITE_ALLOW_MASK,
+            grfAccessMode: 2, // SET_ACCESS
+            grfInheritance: CONTAINER_INHERIT_ACE | OBJECT_INHERIT_ACE,
+            Trustee: TRUSTEE_W {
+                pMultipleTrustee: std::ptr::null_mut(),
+                MultipleTrusteeOperation: 0,
+                TrusteeForm: TRUSTEE_IS_SID,
+                TrusteeType: TRUSTEE_IS_UNKNOWN,
+                ptstrName: *sid as *mut u16,
+            },
+        });
+    }
+    let mut added = false;
+    if !entries.is_empty() {
+        let mut p_new_dacl: *mut ACL = std::ptr::null_mut();
+        let code2 = SetEntriesInAclW(
+            entries.len() as u32,
+            entries.as_ptr(),
+            p_dacl,
+            &mut p_new_dacl,
+        );
+        if code2 == ERROR_SUCCESS {
+            let code3 = SetNamedSecurityInfoW(
+                to_wide(path).as_ptr() as *mut u16,
+                1,
+                DACL_SECURITY_INFORMATION,
+                std::ptr::null_mut(),
+                std::ptr::null_mut(),
+                p_new_dacl,
+                std::ptr::null_mut(),
+            );
+            if code3 == ERROR_SUCCESS {
+                added = true;
+            } else {
+                if !p_new_dacl.is_null() {
+                    LocalFree(p_new_dacl as HLOCAL);
+                }
+                if !p_sd.is_null() {
+                    LocalFree(p_sd as HLOCAL);
+                }
+                return Err(anyhow!("SetNamedSecurityInfoW failed: {}", code3));
+            }
+            if !p_new_dacl.is_null() {
+                LocalFree(p_new_dacl as HLOCAL);
+            }
+        } else {
+            if !p_sd.is_null() {
+                LocalFree(p_sd as HLOCAL);
+            }
+            return Err(anyhow!("SetEntriesInAclW failed: {}", code2));
+        }
+    }
+    if !p_sd.is_null() {
+        LocalFree(p_sd as HLOCAL);
+    }
+    Ok(added)
+}
+
+/// Adds an allow ACE granting read/write/execute to the given SID on the target path.
+///
+/// # Safety
+/// Caller must ensure `psid` points to a valid SID and `path` refers to an existing file or directory.
 pub unsafe fn add_allow_ace(path: &Path, psid: *mut c_void) -> Result<bool> {
     let mut p_sd: *mut c_void = std::ptr::null_mut();
     let mut p_dacl: *mut ACL = std::ptr::null_mut();
@@ -176,39 +417,44 @@ pub unsafe fn add_allow_ace(path: &Path, psid: *mut c_void) -> Result<bool> {
     if code != ERROR_SUCCESS {
         return Err(anyhow!("GetNamedSecurityInfoW failed: {}", code));
     }
+    // Already has write? Skip costly DACL rewrite.
+    if dacl_has_write_allow_for_sid(p_dacl, psid) {
+        if !p_sd.is_null() {
+            LocalFree(p_sd as HLOCAL);
+        }
+        return Ok(false);
+    }
     let mut added = false;
-    if !dacl_has_write_allow_for_sid(p_dacl, psid) {
+    // Always ensure write is present: if an allow ACE exists without write, add one with write+RX.
-        let trustee = TRUSTEE_W {
+    let trustee = TRUSTEE_W {
-            pMultipleTrustee: std::ptr::null_mut(),
+        pMultipleTrustee: std::ptr::null_mut(),
-            MultipleTrusteeOperation: 0,
+        MultipleTrusteeOperation: 0,
-            TrusteeForm: TRUSTEE_IS_SID,
+        TrusteeForm: TRUSTEE_IS_SID,
-            TrusteeType: TRUSTEE_IS_UNKNOWN,
+        TrusteeType: TRUSTEE_IS_UNKNOWN,
-            ptstrName: psid as *mut u16,
+        ptstrName: psid as *mut u16,
-        };
+    };
-        let mut explicit: EXPLICIT_ACCESS_W = std::mem::zeroed();
+    let mut explicit: EXPLICIT_ACCESS_W = std::mem::zeroed();
-        explicit.grfAccessPermissions =
+    explicit.grfAccessPermissions = FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE;
-            FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE;
+    explicit.grfAccessMode = 2; // SET_ACCESS
-        explicit.grfAccessMode = 2; // SET_ACCESS
+    explicit.grfInheritance = CONTAINER_INHERIT_ACE | OBJECT_INHERIT_ACE;
-        explicit.grfInheritance = CONTAINER_INHERIT_ACE | OBJECT_INHERIT_ACE;
+    explicit.Trustee = trustee;
-        explicit.Trustee = trustee;
+    let mut p_new_dacl: *mut ACL = std::ptr::null_mut();
-        let mut p_new_dacl: *mut ACL = std::ptr::null_mut();
+    let code2 = SetEntriesInAclW(1, &explicit, p_dacl, &mut p_new_dacl);
-        let code2 = SetEntriesInAclW(1, &explicit, p_dacl, &mut p_new_dacl);
+    if code2 == ERROR_SUCCESS {
-        if code2 == ERROR_SUCCESS {
+        let code3 = SetNamedSecurityInfoW(
-            let code3 = SetNamedSecurityInfoW(
+            to_wide(path).as_ptr() as *mut u16,
-                to_wide(path).as_ptr() as *mut u16,
+            1,
-                1,
+            DACL_SECURITY_INFORMATION,
-                DACL_SECURITY_INFORMATION,
+            std::ptr::null_mut(),
-                std::ptr::null_mut(),
+            std::ptr::null_mut(),
-                std::ptr::null_mut(),
+            p_new_dacl,
-                p_new_dacl,
+            std::ptr::null_mut(),
-                std::ptr::null_mut(),
+        );
-            );
+        if code3 == ERROR_SUCCESS {
-            if code3 == ERROR_SUCCESS {
+            added = !dacl_has_write_allow_for_sid(p_dacl, psid);
-                added = true;
+        }
-            }
+        if !p_new_dacl.is_null() {
-            if !p_new_dacl.is_null() {
+            LocalFree(p_new_dacl as HLOCAL);
-                LocalFree(p_new_dacl as HLOCAL);
-            }
         }
     }
     if !p_sd.is_null() {
@@ -217,6 +463,10 @@ pub unsafe fn add_allow_ace(path: &Path, psid: *mut c_void) -> Result<bool> {
     Ok(added)
 }
 
+/// Adds a deny ACE to prevent write/append/delete for the given SID on the target path.
+///
+/// # Safety
+/// Caller must ensure `psid` points to a valid SID and `path` refers to an existing file or directory.
 pub unsafe fn add_deny_write_ace(path: &Path, psid: *mut c_void) -> Result<bool> {
     let mut p_sd: *mut c_void = std::ptr::null_mut();
     let mut p_dacl: *mut ACL = std::ptr::null_mut();
@@ -330,6 +580,10 @@ pub unsafe fn revoke_ace(path: &Path, psid: *mut c_void) {
     }
 }
 
+/// Grants RX to the null device for the given SID to support stdout/stderr redirection.
+///
+/// # Safety
+/// Caller must ensure `psid` is a valid SID pointer.
 pub unsafe fn allow_null_device(psid: *mut c_void) {
     let desired = 0x00020000 | 0x00040000; // READ_CONTROL | WRITE_DAC
     let h = CreateFileW(

codex-rs/windows-sandbox-rs/src/audit.rs

@@ -1,12 +1,12 @@
 use crate::acl::add_deny_write_ace;
+use crate::acl::path_quick_mask_allows;
 use crate::cap::cap_sid_file;
 use crate::cap::load_or_create_cap_sids;
-use crate::logging::log_note;
+use crate::logging::{debug_log, log_note};
 use crate::policy::SandboxPolicy;
 use crate::token::convert_string_sid_to_sid;
 use crate::token::world_sid;
 use anyhow::anyhow;
-use crate::winutil::to_wide;
 use anyhow::Result;
 use std::collections::HashSet;
 use std::ffi::c_void;
@@ -14,38 +14,10 @@ use std::path::Path;
 use std::path::PathBuf;
 use std::time::Duration;
 use std::time::Instant;
-use windows_sys::Win32::Foundation::CloseHandle;
-use windows_sys::Win32::Foundation::ERROR_SUCCESS;
-use windows_sys::Win32::Foundation::HLOCAL;
-use windows_sys::Win32::Foundation::INVALID_HANDLE_VALUE;
-use windows_sys::Win32::Foundation::LocalFree;
-use windows_sys::Win32::Security::ACCESS_ALLOWED_ACE;
-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::GetNamedSecurityInfoW;
-use windows_sys::Win32::Security::Authorization::GetSecurityInfo;
-use windows_sys::Win32::Security::DACL_SECURITY_INFORMATION;
-use windows_sys::Win32::Security::EqualSid;
-use windows_sys::Win32::Security::GetAce;
-use windows_sys::Win32::Security::GetAclInformation;
-use windows_sys::Win32::Security::MapGenericMask;
-use windows_sys::Win32::Security::GENERIC_MAPPING;
-use windows_sys::Win32::Storage::FileSystem::CreateFileW;
-use windows_sys::Win32::Storage::FileSystem::FILE_ALL_ACCESS;
 use windows_sys::Win32::Storage::FileSystem::FILE_APPEND_DATA;
-use windows_sys::Win32::Storage::FileSystem::FILE_FLAG_BACKUP_SEMANTICS;
-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_GENERIC_WRITE;
-use windows_sys::Win32::Storage::FileSystem::FILE_SHARE_DELETE;
-use windows_sys::Win32::Storage::FileSystem::FILE_SHARE_READ;
-use windows_sys::Win32::Storage::FileSystem::FILE_SHARE_WRITE;
 use windows_sys::Win32::Storage::FileSystem::FILE_WRITE_ATTRIBUTES;
 use windows_sys::Win32::Storage::FileSystem::FILE_WRITE_DATA;
 use windows_sys::Win32::Storage::FileSystem::FILE_WRITE_EA;
-use windows_sys::Win32::Storage::FileSystem::OPEN_EXISTING;
 
 // Preflight scan limits
 const MAX_ITEMS_PER_DIR: i32 = 1000;
@@ -109,79 +81,10 @@ fn gather_candidates(cwd: &Path, env: &std::collections::HashMap<String, String>
 }
 
 unsafe fn path_has_world_write_allow(path: &Path) -> Result<bool> {
-    // Prefer handle-based query (often faster than name-based), fallback to name-based on error
-    let mut p_sd: *mut c_void = std::ptr::null_mut();
-    let mut p_dacl: *mut ACL = std::ptr::null_mut();
-
-    let mut try_named = false;
-    let wpath = to_wide(path);
-    let h = CreateFileW(
-        wpath.as_ptr(),
-        0x00020000, // READ_CONTROL
-        FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
-        std::ptr::null_mut(),
-        OPEN_EXISTING,
-        FILE_FLAG_BACKUP_SEMANTICS,
-        0,
-    );
-    if h == INVALID_HANDLE_VALUE {
-        try_named = true;
-    } else {
-        let code = GetSecurityInfo(
-            h,
-            1, // SE_FILE_OBJECT
-            DACL_SECURITY_INFORMATION,
-            std::ptr::null_mut(),
-            std::ptr::null_mut(),
-            &mut p_dacl,
-            std::ptr::null_mut(),
-            &mut p_sd,
-        );
-        CloseHandle(h);
-        if code != ERROR_SUCCESS {
-            try_named = true;
-            if !p_sd.is_null() {
-                LocalFree(p_sd as HLOCAL);
-                p_sd = std::ptr::null_mut();
-                p_dacl = std::ptr::null_mut();
-            }
-        }
-    }
-
-    if try_named {
-        let code = GetNamedSecurityInfoW(
-            wpath.as_ptr(),
-            1,
-            DACL_SECURITY_INFORMATION,
-            std::ptr::null_mut(),
-            std::ptr::null_mut(),
-            &mut p_dacl,
-            std::ptr::null_mut(),
-            &mut p_sd,
-        );
-        if code != ERROR_SUCCESS {
-            if !p_sd.is_null() {
-                LocalFree(p_sd as HLOCAL);
-            }
-            return Ok(false);
-        }
-    }
-
     let mut world = world_sid()?;
     let psid_world = world.as_mut_ptr() as *mut c_void;
-    // Very fast mask-based check for world-writable grants (includes GENERIC_*).
+    let write_mask = FILE_WRITE_DATA | FILE_APPEND_DATA | FILE_WRITE_EA | FILE_WRITE_ATTRIBUTES;
-    if !dacl_quick_world_write_mask_allows(p_dacl, psid_world) {
+    path_quick_mask_allows(path, &[psid_world], write_mask)
-        if !p_sd.is_null() {
-            LocalFree(p_sd as HLOCAL);
-        }
-        return Ok(false);
-    }
-    // Quick detector flagged a write grant for Everyone: treat as writable.
-    let has = true;
-    if !p_sd.is_null() {
-        LocalFree(p_sd as HLOCAL);
-    }
-    Ok(has)
 }
 
 pub fn audit_everyone_writable(
@@ -193,6 +96,21 @@ pub fn audit_everyone_writable(
     let mut flagged: Vec<PathBuf> = Vec::new();
     let mut seen: HashSet<String> = HashSet::new();
     let mut checked = 0usize;
+    let check_world_writable = |path: &Path| -> bool {
+        match unsafe { path_has_world_write_allow(path) } {
+            Ok(has) => has,
+            Err(err) => {
+                debug_log(
+                    &format!(
+                        "AUDIT: treating unreadable ACL as not world-writable: {} ({err})",
+                        path.display()
+                    ),
+                    logs_base_dir,
+                );
+                false
+            }
+        }
+    };
     // Fast path: check CWD immediate children first so workspace issues are caught early.
     if let Ok(read) = std::fs::read_dir(cwd) {
         for ent in read.flatten().take(MAX_ITEMS_PER_DIR as usize) {
@@ -210,7 +128,7 @@ pub fn audit_everyone_writable(
             }
             let p = ent.path();
             checked += 1;
-            let has = unsafe { path_has_world_write_allow(&p)? };
+            let has = check_world_writable(&p);
             if has {
                 let key = normalize_path_key(&p);
                 if seen.insert(key) {
@@ -228,7 +146,7 @@ pub fn audit_everyone_writable(
             break;
         }
         checked += 1;
-        let has_root = unsafe { path_has_world_write_allow(&root)? };
+        let has_root = check_world_writable(&root);
         if has_root {
             let key = normalize_path_key(&root);
             if seen.insert(key) {
@@ -260,7 +178,7 @@ pub fn audit_everyone_writable(
                 }
                 if ft.is_dir() {
                     checked += 1;
-                    let has_child = unsafe { path_has_world_write_allow(&p)? };
+                    let has_child = check_world_writable(&p);
                     if has_child {
                         let key = normalize_path_key(&p);
                         if seen.insert(key) {
@@ -384,57 +302,3 @@ pub fn apply_capability_denies_for_world_writable(
     }
     Ok(())
 }
-// Fast mask-based check: does the DACL contain any ACCESS_ALLOWED ACE for
-// Everyone that grants write after generic bits are expanded? Skips inherit-only
-// ACEs (do not apply to the current object).
-unsafe fn dacl_quick_world_write_mask_allows(p_dacl: *mut ACL, psid_world: *mut c_void) -> bool {
-    if p_dacl.is_null() {
-        return false;
-    }
-    const INHERIT_ONLY_ACE: u8 = 0x08;
-    let mut info: ACL_SIZE_INFORMATION = std::mem::zeroed();
-    let ok = GetAclInformation(
-        p_dacl as *const ACL,
-        &mut info as *mut _ as *mut c_void,
-        std::mem::size_of::<ACL_SIZE_INFORMATION>() as u32,
-        AclSizeInformation,
-    );
-    if ok == 0 {
-        return false;
-    }
-    let mapping = GENERIC_MAPPING {
-        GenericRead: FILE_GENERIC_READ,
-        GenericWrite: FILE_GENERIC_WRITE,
-        GenericExecute: FILE_GENERIC_EXECUTE,
-        GenericAll: FILE_ALL_ACCESS,
-    };
-    for i in 0..(info.AceCount as usize) {
-        let mut p_ace: *mut c_void = std::ptr::null_mut();
-        if GetAce(p_dacl as *const ACL, i as u32, &mut p_ace) == 0 {
-            continue;
-        }
-        let hdr = &*(p_ace as *const ACE_HEADER);
-        if hdr.AceType != 0 {
-            // ACCESS_ALLOWED_ACE_TYPE
-            continue;
-        }
-        if (hdr.AceFlags & INHERIT_ONLY_ACE) != 0 {
-            continue;
-        }
-        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; // skip header + mask
-        if EqualSid(sid_ptr, psid_world) == 0 {
-            continue;
-        }
-        let ace = &*(p_ace as *const ACCESS_ALLOWED_ACE);
-        let mut mask = ace.Mask;
-        // Expand generic bits to concrete file rights before checking for write.
-        MapGenericMask(&mut mask, &mapping);
-        let write_mask = FILE_WRITE_DATA | FILE_APPEND_DATA | FILE_WRITE_EA | FILE_WRITE_ATTRIBUTES;
-        if (mask & write_mask) != 0 {
-            return true;
-        }
-    }
-    false
-}

codex-rs/windows-sandbox-rs/src/env.rs

@@ -1,11 +1,10 @@
-use anyhow::Result;
+use anyhow::{anyhow, Result};
+use dirs_next::home_dir;
 use std::collections::HashMap;
 use std::env;
-use std::fs::File;
+use std::fs::{self, File};
-use std::fs::{self};
 use std::io::Write;
-use std::path::Path;
+use std::path::{Path, PathBuf};
-use std::path::PathBuf;
 
 pub fn normalize_null_device_env(env_map: &mut HashMap<String, String>) {
     let keys: Vec<String> = env_map.keys().cloned().collect();
@@ -29,6 +28,21 @@ pub fn ensure_non_interactive_pager(env_map: &mut HashMap<String, String>) {
     env_map.entry("LESS".into()).or_insert_with(|| "".into());
 }
 
+// Keep PATH and PATHEXT stable for callers that rely on inheriting the parent process env.
+#[allow(dead_code)]
+pub fn inherit_path_env(env_map: &mut HashMap<String, String>) {
+    if !env_map.contains_key("PATH") {
+        if let Ok(path) = env::var("PATH") {
+            env_map.insert("PATH".into(), path);
+        }
+    }
+    if !env_map.contains_key("PATHEXT") {
+        if let Ok(pathext) = env::var("PATHEXT") {
+            env_map.insert("PATHEXT".into(), pathext);
+        }
+    }
+}
+
 fn prepend_path(env_map: &mut HashMap<String, String>, prefix: &str) {
     let existing = env_map
         .get("PATH")
@@ -64,7 +78,7 @@ fn reorder_pathext_for_stubs(env_map: &mut HashMap<String, String>) {
         .map(|s| s.to_string())
         .collect();
     let exts_norm: Vec<String> = exts.iter().map(|e| e.to_ascii_uppercase()).collect();
-    let want = [".BAT", ".CMD"]; // move to front if present
+    let want = [".BAT", ".CMD"];
     let mut front: Vec<String> = Vec::new();
     for w in want {
         if let Some(idx) = exts_norm.iter().position(|e| e == w) {
@@ -90,7 +104,7 @@ fn ensure_denybin(tools: &[&str], denybin_dir: Option<&Path>) -> Result<PathBuf>
     let base = match denybin_dir {
         Some(p) => p.to_path_buf(),
         None => {
-            let home = dirs_next::home_dir().ok_or_else(|| anyhow::anyhow!("no home dir"))?;
+            let home = home_dir().ok_or_else(|| anyhow!("no home dir"))?;
             home.join(".sbx-denybin")
         }
     };
@@ -146,16 +160,12 @@ pub fn apply_no_network_to_env(env_map: &mut HashMap<String, String>) -> Result<
         .entry("GIT_ALLOW_PROTOCOLS".into())
         .or_insert_with(|| "".into());
 
-    // Block interactive network tools that bypass HTTP(S) proxy settings, but
-    // allow curl/wget to run so commands like `curl --version` still succeed.
-    // Network access is disabled via proxy envs above.
     let base = ensure_denybin(&["ssh", "scp"], None)?;
-    // Clean up any stale stubs from previous runs so real curl/wget can run.
     for tool in ["curl", "wget"] {
         for ext in [".bat", ".cmd"] {
             let p = base.join(format!("{}{}", tool, ext));
             if p.exists() {
-                let _ = std::fs::remove_file(&p);
+                let _ = fs::remove_file(&p);
             }
         }
     }

codex-rs/windows-sandbox-rs/src/logging.rs

@@ -2,9 +2,20 @@ use std::fs::OpenOptions;
 use std::io::Write;
 use std::path::Path;
 use std::path::PathBuf;
+use std::sync::OnceLock;
 
 const LOG_COMMAND_PREVIEW_LIMIT: usize = 200;
-pub const LOG_FILE_NAME: &str = "sandbox_commands.rust.log";
+pub const LOG_FILE_NAME: &str = "sandbox.log";
+
+fn exe_label() -> &'static str {
+    static LABEL: OnceLock<String> = OnceLock::new();
+    LABEL.get_or_init(|| {
+        std::env::current_exe()
+            .ok()
+            .and_then(|p| p.file_name().map(|n| n.to_string_lossy().to_string()))
+            .unwrap_or_else(|| "proc".to_string())
+    })
+}
 
 fn preview(command: &[String]) -> String {
     let joined = command.join(" ");
@@ -35,17 +46,17 @@ fn append_line(line: &str, base_dir: Option<&Path>) {
 
 pub fn log_start(command: &[String], base_dir: Option<&Path>) {
     let p = preview(command);
-    append_line(&format!("START: {p}"), base_dir);
+    log_note(&format!("START: {p}"), base_dir);
 }
 
 pub fn log_success(command: &[String], base_dir: Option<&Path>) {
     let p = preview(command);
-    append_line(&format!("SUCCESS: {p}"), base_dir);
+    log_note(&format!("SUCCESS: {p}"), base_dir);
 }
 
 pub fn log_failure(command: &[String], detail: &str, base_dir: Option<&Path>) {
     let p = preview(command);
-    append_line(&format!("FAILURE: {p} ({detail})"), base_dir);
+    log_note(&format!("FAILURE: {p} ({detail})"), base_dir);
 }
 
 // Debug logging helper. Emits only when SBX_DEBUG=1 to avoid noisy logs.
@@ -56,7 +67,8 @@ pub fn debug_log(msg: &str, base_dir: Option<&Path>) {
     }
 }
 
-// Unconditional note logging to sandbox_commands.rust.log
+// Unconditional note logging to sandbox.log
 pub fn log_note(msg: &str, base_dir: Option<&Path>) {
-    append_line(msg, base_dir);
+    let ts = chrono::Local::now().format("%Y-%m-%d %H:%M:%S%.3f");
+    append_line(&format!("[{ts} {}] {}", exe_label(), msg), base_dir);
 }

codex-rs/windows-sandbox-rs/src/process.rs

@@ -79,6 +79,7 @@ fn quote_arg(a: &str) -> String {
     out.push('"');
     out
 }
+#[allow(dead_code)]
 unsafe fn ensure_inheritable_stdio(si: &mut STARTUPINFOW) -> Result<()> {
     for kind in [STD_INPUT_HANDLE, STD_OUTPUT_HANDLE, STD_ERROR_HANDLE] {
         let h = GetStdHandle(kind);
@@ -96,12 +97,16 @@ unsafe fn ensure_inheritable_stdio(si: &mut STARTUPINFOW) -> Result<()> {
     Ok(())
 }
 
+/// # Safety
+/// Caller must provide a valid primary token handle (`h_token`) with appropriate access,
+/// and the `argv`, `cwd`, and `env_map` must remain valid for the duration of the call.
 pub unsafe fn create_process_as_user(
     h_token: HANDLE,
     argv: &[String],
     cwd: &Path,
     env_map: &HashMap<String, String>,
     logs_base_dir: Option<&Path>,
+    stdio: Option<(HANDLE, HANDLE, HANDLE)>,
 ) -> Result<(PROCESS_INFORMATION, STARTUPINFOW)> {
     let cmdline_str = argv
         .iter()
@@ -117,19 +122,41 @@ pub unsafe fn create_process_as_user(
     // Point explicitly at the interactive desktop.
     let desktop = to_wide("Winsta0\\Default");
     si.lpDesktop = desktop.as_ptr() as *mut u16;
-    ensure_inheritable_stdio(&mut si)?;
     let mut pi: PROCESS_INFORMATION = std::mem::zeroed();
+    // Ensure handles are inheritable when custom stdio is supplied.
+    let inherit_handles = match stdio {
+        Some((stdin_h, stdout_h, stderr_h)) => {
+            si.dwFlags |= STARTF_USESTDHANDLES;
+            si.hStdInput = stdin_h;
+            si.hStdOutput = stdout_h;
+            si.hStdError = stderr_h;
+            for h in [stdin_h, stdout_h, stderr_h] {
+                if SetHandleInformation(h, HANDLE_FLAG_INHERIT, HANDLE_FLAG_INHERIT) == 0 {
+                    return Err(anyhow!(
+                        "SetHandleInformation failed for stdio handle: {}",
+                        GetLastError()
+                    ));
+                }
+            }
+            true
+        }
+        None => {
+            ensure_inheritable_stdio(&mut si)?;
+            true
+        }
+    };
+
     let ok = CreateProcessAsUserW(
         h_token,
         std::ptr::null(),
         cmdline.as_mut_ptr(),
         std::ptr::null_mut(),
         std::ptr::null_mut(),
-        1,
+        inherit_handles as i32,
         CREATE_UNICODE_ENVIRONMENT,
         env_block.as_ptr() as *mut c_void,
         to_wide(cwd).as_ptr(),
-        &si,
+        &mut si,
         &mut pi,
     );
     if ok == 0 {
@@ -149,6 +176,9 @@ pub unsafe fn create_process_as_user(
     Ok((pi, si))
 }
 
+/// # Safety
+/// Caller must provide valid process information handles.
+#[allow(dead_code)]
 pub unsafe fn wait_process_and_exitcode(pi: &PROCESS_INFORMATION) -> Result<i32> {
     let res = WaitForSingleObject(pi.hProcess, INFINITE);
     if res != 0 {
@@ -161,6 +191,9 @@ pub unsafe fn wait_process_and_exitcode(pi: &PROCESS_INFORMATION) -> Result<i32>
     Ok(code as i32)
 }
 
+/// # Safety
+/// Caller must close the returned job handle.
+#[allow(dead_code)]
 pub unsafe fn create_job_kill_on_close() -> Result<HANDLE> {
     let h = CreateJobObjectW(std::ptr::null_mut(), std::ptr::null());
     if h == 0 {
@@ -183,6 +216,9 @@ pub unsafe fn create_job_kill_on_close() -> Result<HANDLE> {
     Ok(h)
 }
 
+/// # Safety
+/// Caller must pass valid handles for a job object and a process.
+#[allow(dead_code)]
 pub unsafe fn assign_to_job(h_job: HANDLE, h_process: HANDLE) -> Result<()> {
     if AssignProcessToJobObject(h_job, h_process) == 0 {
         return Err(anyhow!(

codex-rs/windows-sandbox-rs/src/token.rs

@@ -24,6 +24,7 @@ use windows_sys::Win32::Security::TOKEN_DUPLICATE;
 use windows_sys::Win32::Security::TOKEN_PRIVILEGES;
 use windows_sys::Win32::Security::TOKEN_QUERY;
 use windows_sys::Win32::System::Threading::GetCurrentProcess;
+use windows_sys::Win32::System::Threading::OpenProcessToken;
 
 const DISABLE_MAX_PRIVILEGE: u32 = 0x01;
 const LUA_TOKEN: u32 = 0x04;
@@ -52,6 +53,8 @@ pub unsafe fn world_sid() -> Result<Vec<u8>> {
     Ok(buf)
 }
 
+/// # Safety
+/// Caller is responsible for freeing the returned SID with `LocalFree`.
 pub unsafe fn convert_string_sid_to_sid(s: &str) -> Option<*mut c_void> {
     #[link(name = "advapi32")]
     extern "system" {
@@ -66,6 +69,9 @@ pub unsafe fn convert_string_sid_to_sid(s: &str) -> Option<*mut c_void> {
     }
 }
 
+/// # Safety
+/// Caller must close the returned token handle.
+#[allow(dead_code)]
 pub unsafe fn get_current_token_for_restriction() -> Result<HANDLE> {
     let desired = TOKEN_DUPLICATE
         | TOKEN_QUERY
@@ -197,13 +203,55 @@ unsafe fn enable_single_privilege(h_token: HANDLE, name: &str) -> Result<()> {
     Ok(())
 }
 
-// removed unused create_write_restricted_token_strict
+/// # Safety
+/// Opens the current process token and adjusts privileges; caller should ensure this is needed in the current context.
+#[allow(dead_code)]
+pub unsafe fn enable_privilege_on_current(name: &str) -> Result<()> {
+    let mut h: HANDLE = 0;
+    let ok = OpenProcessToken(
+        GetCurrentProcess(),
+        TOKEN_ADJUST_PRIVILEGES | TOKEN_QUERY,
+        &mut h,
+    );
+    if ok == 0 {
+        return Err(anyhow!("OpenProcessToken failed: {}", GetLastError()));
+    }
+    let res = enable_single_privilege(h, name);
+    CloseHandle(h);
+    res
+}
 
+/// # Safety
+/// Caller must close the returned token handle.
+#[allow(dead_code)]
 pub unsafe fn create_workspace_write_token_with_cap(
     psid_capability: *mut c_void,
 ) -> Result<(HANDLE, *mut c_void)> {
     let base = get_current_token_for_restriction()?;
-    let mut logon_sid_bytes = get_logon_sid_bytes(base)?;
+    let res = create_workspace_write_token_with_cap_from(base, psid_capability);
+    CloseHandle(base);
+    res
+}
+
+/// # Safety
+/// Caller must close the returned token handle.
+#[allow(dead_code)]
+pub unsafe fn create_readonly_token_with_cap(
+    psid_capability: *mut c_void,
+) -> Result<(HANDLE, *mut c_void)> {
+    let base = get_current_token_for_restriction()?;
+    let res = create_readonly_token_with_cap_from(base, psid_capability);
+    CloseHandle(base);
+    res
+}
+
+/// # Safety
+/// Caller must close the returned token handle; base_token must be a valid primary token.
+pub unsafe fn create_workspace_write_token_with_cap_from(
+    base_token: HANDLE,
+    psid_capability: *mut c_void,
+) -> Result<(HANDLE, *mut c_void)> {
+    let mut logon_sid_bytes = get_logon_sid_bytes(base_token)?;
     let psid_logon = logon_sid_bytes.as_mut_ptr() as *mut c_void;
     let mut everyone = world_sid()?;
     let psid_everyone = everyone.as_mut_ptr() as *mut c_void;
@@ -218,7 +266,7 @@ pub unsafe fn create_workspace_write_token_with_cap(
     let mut new_token: HANDLE = 0;
     let flags = DISABLE_MAX_PRIVILEGE | LUA_TOKEN | WRITE_RESTRICTED;
     let ok = CreateRestrictedToken(
-        base,
+        base_token,
         flags,
         0,
         std::ptr::null(),
@@ -235,11 +283,13 @@ pub unsafe fn create_workspace_write_token_with_cap(
     Ok((new_token, psid_capability))
 }
 
-pub unsafe fn create_readonly_token_with_cap(
+/// # Safety
+/// Caller must close the returned token handle; base_token must be a valid primary token.
+pub unsafe fn create_readonly_token_with_cap_from(
+    base_token: HANDLE,
     psid_capability: *mut c_void,
 ) -> Result<(HANDLE, *mut c_void)> {
-    let base = get_current_token_for_restriction()?;
+    let mut logon_sid_bytes = get_logon_sid_bytes(base_token)?;
-    let mut logon_sid_bytes = get_logon_sid_bytes(base)?;
     let psid_logon = logon_sid_bytes.as_mut_ptr() as *mut c_void;
     let mut everyone = world_sid()?;
     let psid_everyone = everyone.as_mut_ptr() as *mut c_void;
@@ -254,7 +304,7 @@ pub unsafe fn create_readonly_token_with_cap(
     let mut new_token: HANDLE = 0;
     let flags = DISABLE_MAX_PRIVILEGE | LUA_TOKEN | WRITE_RESTRICTED;
     let ok = CreateRestrictedToken(
-        base,
+        base_token,
         flags,
         0,
         std::ptr::null(),

codex-rs/windows-sandbox-rs/src/winutil.rs

@@ -6,6 +6,7 @@ use windows_sys::Win32::System::Diagnostics::Debug::FormatMessageW;
 use windows_sys::Win32::System::Diagnostics::Debug::FORMAT_MESSAGE_ALLOCATE_BUFFER;
 use windows_sys::Win32::System::Diagnostics::Debug::FORMAT_MESSAGE_FROM_SYSTEM;
 use windows_sys::Win32::System::Diagnostics::Debug::FORMAT_MESSAGE_IGNORE_INSERTS;
+use windows_sys::Win32::Security::Authorization::ConvertSidToStringSidW;
 
 pub fn to_wide<S: AsRef<OsStr>>(s: S) -> Vec<u16> {
     let mut v: Vec<u16> = s.as_ref().encode_wide().collect();
@@ -41,3 +42,22 @@ pub fn format_last_error(err: i32) -> String {
         s
     }
 }
+
+#[allow(dead_code)]
+pub fn string_from_sid_bytes(sid: &[u8]) -> Result<String, String> {
+    unsafe {
+        let mut str_ptr: *mut u16 = std::ptr::null_mut();
+        let ok = ConvertSidToStringSidW(sid.as_ptr() as *mut std::ffi::c_void, &mut str_ptr);
+        if ok == 0 || str_ptr.is_null() {
+            return Err(format!("ConvertSidToStringSidW failed: {}", std::io::Error::last_os_error()));
+        }
+        let mut len = 0;
+        while *str_ptr.add(len) != 0 {
+            len += 1;
+        }
+        let slice = std::slice::from_raw_parts(str_ptr, len);
+        let out = String::from_utf16_lossy(slice);
+        let _ = LocalFree(str_ptr as HLOCAL);
+        Ok(out)
+    }
+}