From e405191a28bed64f61679c1f6ee2ce17b04743be Mon Sep 17 00:00:00 2001 From: David Wiesen Date: Tue, 28 Apr 2026 13:05:57 -0700 Subject: [PATCH] Avoid redundant Windows sandbox ACL rewrites --- codex-rs/windows-sandbox-rs/src/acl.rs | 86 +++++++++++++++++++ codex-rs/windows-sandbox-rs/src/lib.rs | 2 + .../windows-sandbox-rs/src/setup_main_win.rs | 52 +++++------ 3 files changed, 114 insertions(+), 26 deletions(-) diff --git a/codex-rs/windows-sandbox-rs/src/acl.rs b/codex-rs/windows-sandbox-rs/src/acl.rs index f351dba190..6bd2641173 100644 --- a/codex-rs/windows-sandbox-rs/src/acl.rs +++ b/codex-rs/windows-sandbox-rs/src/acl.rs @@ -173,6 +173,92 @@ pub fn path_mask_allows( } } +/// Aggregate allow check across all matching ACEs for the provided SIDs. +/// +/// This is closer to Windows effective-access evaluation than `dacl_mask_allows` because +/// a token can satisfy a requested mask through multiple allow ACEs across different groups +/// or capability SIDs. +pub unsafe fn dacl_mask_allows_aggregate( + p_dacl: *mut ACL, + psids: &[*mut c_void], + desired_mask: u32, + require_all_bits: bool, +) -> 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::() 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, + }; + let mut granted_mask = 0u32; + 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::() + std::mem::size_of::()) 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); + granted_mask |= mask; + if (require_all_bits && (granted_mask & desired_mask) == desired_mask) + || (!require_all_bits && (granted_mask & desired_mask) != 0) + { + return true; + } + } + false +} + +/// Path-based wrapper around the aggregate mask check (single DACL fetch). +pub fn path_mask_allows_aggregate( + path: &Path, + psids: &[*mut c_void], + desired_mask: u32, + require_all_bits: bool, +) -> Result { + unsafe { + let (p_dacl, sd) = fetch_dacl_handle(path)?; + let has = dacl_mask_allows_aggregate(p_dacl, psids, desired_mask, require_all_bits); + 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; diff --git a/codex-rs/windows-sandbox-rs/src/lib.rs b/codex-rs/windows-sandbox-rs/src/lib.rs index 8110c3237d..009ed91dff 100644 --- a/codex-rs/windows-sandbox-rs/src/lib.rs +++ b/codex-rs/windows-sandbox-rs/src/lib.rs @@ -89,6 +89,8 @@ pub use acl::fetch_dacl_handle; #[cfg(target_os = "windows")] pub use acl::path_mask_allows; #[cfg(target_os = "windows")] +pub use acl::path_mask_allows_aggregate; +#[cfg(target_os = "windows")] pub use audit::apply_world_writable_scan_and_denies; #[cfg(target_os = "windows")] pub use cap::load_or_create_cap_sids; diff --git a/codex-rs/windows-sandbox-rs/src/setup_main_win.rs b/codex-rs/windows-sandbox-rs/src/setup_main_win.rs index 78c0a8be8e..18cf5f8462 100644 --- a/codex-rs/windows-sandbox-rs/src/setup_main_win.rs +++ b/codex-rs/windows-sandbox-rs/src/setup_main_win.rs @@ -22,6 +22,7 @@ use codex_windows_sandbox::is_command_cwd_root; use codex_windows_sandbox::load_or_create_cap_sids; use codex_windows_sandbox::log_note; use codex_windows_sandbox::path_mask_allows; +use codex_windows_sandbox::path_mask_allows_aggregate; use codex_windows_sandbox::sandbox_bin_dir; use codex_windows_sandbox::sandbox_dir; use codex_windows_sandbox::sandbox_secrets_dir; @@ -669,33 +670,32 @@ fn run_setup_full(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<( } else { cap_psid }; - for (label, psid) in [ - ("sandbox_group", sandbox_group_psid), - (cap_label, cap_psid_for_root), - ] { - let has = - match path_mask_allows(root, &[psid], write_mask, /*require_all_bits*/ true) { - Ok(h) => h, - Err(e) => { - refresh_errors.push(format!( - "write mask check failed on {} for {label}: {}", - root.display(), - e - )); - log_line( - log, - &format!( - "write mask check failed on {} for {label}: {}; continuing", - root.display(), - e - ), - )?; - false - } - }; - if !has { - need_grant = true; + let has_effective_write = match path_mask_allows_aggregate( + root, + &[sandbox_group_psid, cap_psid_for_root], + write_mask, + /*require_all_bits*/ true, + ) { + Ok(h) => h, + Err(e) => { + refresh_errors.push(format!( + "write mask check failed on {} for sandbox_group+{cap_label}: {}", + root.display(), + e + )); + log_line( + log, + &format!( + "write mask check failed on {} for sandbox_group+{cap_label}: {}; continuing", + root.display(), + e + ), + )?; + false } + }; + if !has_effective_write { + need_grant = true; } if need_grant { log_line(