From 3f29c6799a692a7cee57f5af18cd027cfd8a0fbf Mon Sep 17 00:00:00 2001 From: Felipe Coury Date: Mon, 20 Jul 2026 18:23:01 +0000 Subject: [PATCH] Ignore inherited ACEs when refreshing Windows write roots (#34392) ## Why An inherited `FILE_DELETE_CHILD` grant can make a write root look stale, but `SET_ACCESS` cannot replace an ACE inherited from an ancestor. Treating that grant as explicit causes unnecessary ACL refresh attempts that cannot converge. ## What changed - Add explicit-ACE filtering to the Windows DACL permission checks. - Refresh a write root only when `FILE_DELETE_CHILD` is present in an explicit allow ACE, while retaining effective-permission checks for required rights. - Ignore inherited stale rights when deciding whether `SET_ACCESS` must repair an allow ACE. ## Testing Add a Windows regression test covering a write root that inherits `FILE_DELETE_CHILD`, verifying that repeated refresh checks leave its explicit write ACE unchanged. GitOrigin-RevId: d0df9429efcf299da3ff3c1bce92942684803293 (cherry picked from commit bd92b056ddd91bd7c2ecfea3d8773f7eb5a879a6) --- codex-rs/windows-sandbox-rs/src/acl.rs | 64 ++++++++++++++++++- .../src/bin/setup_main/win.rs | 58 +++++++++++++++-- codex-rs/windows-sandbox-rs/src/lib.rs | 2 + 3 files changed, 116 insertions(+), 8 deletions(-) diff --git a/codex-rs/windows-sandbox-rs/src/acl.rs b/codex-rs/windows-sandbox-rs/src/acl.rs index 4b859b047f..073452d354 100644 --- a/codex-rs/windows-sandbox-rs/src/acl.rs +++ b/codex-rs/windows-sandbox-rs/src/acl.rs @@ -49,6 +49,7 @@ 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 INHERITED_ACE: u8 = 0x10; const ACCESS_ALLOWED_ACE_TYPE: u8 = 0; const ACCESS_DENIED_ACE_TYPE: u8 = 1; const GENERIC_READ_MASK: u32 = 0x8000_0000; @@ -103,6 +104,28 @@ pub unsafe fn dacl_mask_allows( psids: &[*mut c_void], desired_mask: u32, require_all_bits: bool, +) -> bool { + dacl_mask_allows_with_scope( + p_dacl, + psids, + desired_mask, + require_all_bits, + AceScope::Effective, + ) +} + +#[derive(Clone, Copy)] +enum AceScope { + Effective, + Explicit, +} + +unsafe fn dacl_mask_allows_with_scope( + p_dacl: *mut ACL, + psids: &[*mut c_void], + desired_mask: u32, + require_all_bits: bool, + scope: AceScope, ) -> bool { if p_dacl.is_null() { return false; @@ -135,6 +158,11 @@ pub unsafe fn dacl_mask_allows( if (hdr.AceFlags & INHERIT_ONLY_ACE) != 0 { continue; } + // SET_ACCESS cannot replace an ACE inherited from an ancestor, so it cannot make + // an explicit-only repair converge when that inherited ACE contains stale rights. + if matches!(scope, AceScope::Explicit) && (hdr.AceFlags & INHERITED_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; @@ -166,10 +194,41 @@ pub fn path_mask_allows( psids: &[*mut c_void], desired_mask: u32, require_all_bits: bool, +) -> Result { + path_mask_allows_with_scope( + path, + psids, + desired_mask, + require_all_bits, + AceScope::Effective, + ) +} + +/// Returns whether an explicit allow ACE for one of the provided SIDs grants any bit in `desired_mask`. +pub fn path_mask_has_explicit_allow_ace( + path: &Path, + psids: &[*mut c_void], + desired_mask: u32, +) -> Result { + path_mask_allows_with_scope( + path, + psids, + desired_mask, + /*require_all_bits*/ false, + AceScope::Explicit, + ) +} + +fn path_mask_allows_with_scope( + path: &Path, + psids: &[*mut c_void], + desired_mask: u32, + require_all_bits: bool, + scope: AceScope, ) -> Result { unsafe { let (p_dacl, sd) = fetch_dacl_handle(path)?; - let has = dacl_mask_allows(p_dacl, psids, desired_mask, require_all_bits); + let has = dacl_mask_allows_with_scope(p_dacl, psids, desired_mask, require_all_bits, scope); if !sd.is_null() { LocalFree(sd as HLOCAL); } @@ -318,11 +377,12 @@ unsafe fn ensure_allow_mask_aces_with_inheritance_impl( let mut entries: Vec = Vec::new(); for sid in sids { if dacl_mask_allows(p_dacl, &[*sid], allow_mask, /*require_all_bits*/ true) - && !dacl_mask_allows( + && !dacl_mask_allows_with_scope( p_dacl, &[*sid], disallow_mask, /*require_all_bits*/ false, + AceScope::Explicit, ) { continue; diff --git a/codex-rs/windows-sandbox-rs/src/bin/setup_main/win.rs b/codex-rs/windows-sandbox-rs/src/bin/setup_main/win.rs index efe9f6af9e..3904785afa 100644 --- a/codex-rs/windows-sandbox-rs/src/bin/setup_main/win.rs +++ b/codex-rs/windows-sandbox-rs/src/bin/setup_main/win.rs @@ -22,6 +22,7 @@ use codex_windows_sandbox::is_command_cwd_root; use codex_windows_sandbox::log_note; use codex_windows_sandbox::log_writer; use codex_windows_sandbox::path_mask_allows; +use codex_windows_sandbox::path_mask_has_explicit_allow_ace; use codex_windows_sandbox::sandbox_bin_dir; use codex_windows_sandbox::sandbox_dir; use codex_windows_sandbox::sandbox_secrets_dir; @@ -169,12 +170,7 @@ fn write_root_needs_refresh(root: &Path, psid: *mut c_void) -> Result { )? { return Ok(true); } - path_mask_allows( - root, - &[psid], - FILE_DELETE_CHILD, - /*require_all_bits*/ false, - ) + path_mask_has_explicit_allow_ace(root, &[psid], FILE_DELETE_CHILD) } fn spawn_read_acl_helper(payload: &Payload, _log: &mut dyn Write) -> Result<()> { @@ -1060,6 +1056,7 @@ mod tests { use codex_windows_sandbox::ensure_allow_mask_aces; use codex_windows_sandbox::ensure_allow_write_aces; use codex_windows_sandbox::load_or_create_cap_sids; + use codex_windows_sandbox::path_mask_allows; use codex_windows_sandbox::workspace_write_cap_sid_for_root; use pretty_assertions::assert_eq; use serde_json::json; @@ -1144,6 +1141,55 @@ mod tests { ); } + #[test] + fn write_root_refresh_ignores_inherited_delete_child_grant() { + let temp = tempfile::tempdir().expect("tempdir"); + let codex_home = temp.path().join("codex-home"); + let parent = temp.path().join("parent"); + let workspace = parent.join("workspace"); + fs::create_dir_all(&codex_home).expect("create codex home"); + fs::create_dir_all(&workspace).expect("create workspace"); + + let sid = workspace_write_cap_sid_for_root(&codex_home, &workspace, &workspace) + .expect("workspace sid"); + let psid = unsafe { convert_string_sid_to_sid(&sid).expect("convert workspace sid") }; + let seeded_explicit = + unsafe { ensure_allow_mask_aces(&workspace, &[psid], WRITE_ROOT_ALLOW_MASK) } + .expect("seed explicit write ACE"); + let seeded_parent = unsafe { + ensure_allow_mask_aces(&parent, &[psid], WRITE_ROOT_ALLOW_MASK | FILE_DELETE_CHILD) + } + .expect("seed inherited stale write ACE"); + let has_inherited_delete_child = path_mask_allows( + &workspace, + &[psid], + FILE_DELETE_CHILD, + /*require_all_bits*/ false, + ) + .expect("check inherited stale write ACE"); + let needs_refresh = + write_root_needs_refresh(&workspace, psid).expect("check inherited stale write ACE"); + let first_refresh = unsafe { ensure_allow_write_aces(&workspace, &[psid]) } + .expect("first inherited write ACE refresh"); + let second_refresh = unsafe { ensure_allow_write_aces(&workspace, &[psid]) } + .expect("second inherited write ACE refresh"); + unsafe { + LocalFree(psid as HLOCAL); + } + + assert_eq!( + ( + seeded_explicit, + seeded_parent, + has_inherited_delete_child, + needs_refresh, + first_refresh, + second_refresh, + ), + (true, true, true, false, false, false) + ); + } + #[test] fn deny_path_under_active_root_uses_only_matching_root_sid() { let temp = tempfile::tempdir().expect("tempdir"); diff --git a/codex-rs/windows-sandbox-rs/src/lib.rs b/codex-rs/windows-sandbox-rs/src/lib.rs index c2e1dd4b74..40ae46b617 100644 --- a/codex-rs/windows-sandbox-rs/src/lib.rs +++ b/codex-rs/windows-sandbox-rs/src/lib.rs @@ -148,6 +148,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_has_explicit_allow_ace; +#[cfg(target_os = "windows")] pub use audit::apply_world_writable_scan_and_denies_for_permissions; #[cfg(target_os = "windows")] pub use cap::load_or_create_cap_sids;