From bb4ea552bdf4f72085c94fa53674cbeb7c8efde7 Mon Sep 17 00:00:00 2001 From: David Wiesen Date: Fri, 17 Apr 2026 12:06:31 -0700 Subject: [PATCH] Repair inherited ACLs for existing Windows sandbox files --- codex-rs/windows-sandbox-rs/src/acl.rs | 46 +++++++++++++++++++ codex-rs/windows-sandbox-rs/src/lib.rs | 2 + .../windows-sandbox-rs/src/setup_main_win.rs | 15 +++++- 3 files changed, 62 insertions(+), 1 deletion(-) diff --git a/codex-rs/windows-sandbox-rs/src/acl.rs b/codex-rs/windows-sandbox-rs/src/acl.rs index f351dba190..db78f28fee 100644 --- a/codex-rs/windows-sandbox-rs/src/acl.rs +++ b/codex-rs/windows-sandbox-rs/src/acl.rs @@ -11,10 +11,14 @@ use windows_sys::Win32::Foundation::INVALID_HANDLE_VALUE; 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::Authorization::ProgressInvokeNever; use windows_sys::Win32::Security::Authorization::SetEntriesInAclW; use windows_sys::Win32::Security::Authorization::SetNamedSecurityInfoW; use windows_sys::Win32::Security::Authorization::SetSecurityInfo; +use windows_sys::Win32::Security::Authorization::TREE_SEC_INFO_RESET_KEEP_EXPLICIT; +use windows_sys::Win32::Security::Authorization::TreeSetNamedSecurityInfoW; use windows_sys::Win32::Security::Authorization::EXPLICIT_ACCESS_W; +use windows_sys::Win32::Security::Authorization::SE_FILE_OBJECT; use windows_sys::Win32::Security::Authorization::TRUSTEE_IS_SID; use windows_sys::Win32::Security::Authorization::TRUSTEE_IS_UNKNOWN; use windows_sys::Win32::Security::Authorization::TRUSTEE_W; @@ -383,6 +387,48 @@ pub unsafe fn ensure_allow_write_aces(path: &Path, sids: &[*mut c_void]) -> Resu ensure_allow_mask_aces(path, sids, WRITE_ALLOW_MASK) } +/// Ensure the directory and its existing descendants inherit write-capable ACEs for the provided +/// SIDs while preserving explicit child ACEs that were already present. +/// +/// # Safety +/// Caller must pass valid SID pointers and an existing path; free the returned security +/// descriptor with `LocalFree`. +pub unsafe fn ensure_allow_write_aces_recursively( + path: &Path, + sids: &[*mut c_void], +) -> Result { + let added = ensure_allow_write_aces(path, sids)?; + if !path.is_dir() { + return Ok(added); + } + + let (p_dacl, p_sd) = fetch_dacl_handle(path)?; + let code = TreeSetNamedSecurityInfoW( + to_wide(path).as_ptr(), + SE_FILE_OBJECT, + DACL_SECURITY_INFORMATION, + std::ptr::null_mut(), + std::ptr::null_mut(), + p_dacl, + std::ptr::null(), + TREE_SEC_INFO_RESET_KEEP_EXPLICIT, + None, + ProgressInvokeNever, + std::ptr::null(), + ); + if !p_sd.is_null() { + LocalFree(p_sd as HLOCAL); + } + if code != ERROR_SUCCESS { + return Err(anyhow!( + "TreeSetNamedSecurityInfoW failed for {}: {}", + path.display(), + code + )); + } + Ok(added) +} + /// Adds an allow ACE granting read/write/execute to the given SID on the target path. /// /// # Safety diff --git a/codex-rs/windows-sandbox-rs/src/lib.rs b/codex-rs/windows-sandbox-rs/src/lib.rs index 90176b08ea..30c4ae7c0c 100644 --- a/codex-rs/windows-sandbox-rs/src/lib.rs +++ b/codex-rs/windows-sandbox-rs/src/lib.rs @@ -58,6 +58,8 @@ pub use acl::ensure_allow_mask_aces_with_inheritance; #[cfg(target_os = "windows")] pub use acl::ensure_allow_write_aces; #[cfg(target_os = "windows")] +pub use acl::ensure_allow_write_aces_recursively; +#[cfg(target_os = "windows")] pub use acl::fetch_dacl_handle; #[cfg(target_os = "windows")] pub use acl::path_mask_allows; 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..82b1100c4d 100644 --- a/codex-rs/windows-sandbox-rs/src/setup_main_win.rs +++ b/codex-rs/windows-sandbox-rs/src/setup_main_win.rs @@ -16,6 +16,7 @@ use codex_windows_sandbox::canonicalize_path; use codex_windows_sandbox::convert_string_sid_to_sid; use codex_windows_sandbox::ensure_allow_mask_aces_with_inheritance; use codex_windows_sandbox::ensure_allow_write_aces; +use codex_windows_sandbox::ensure_allow_write_aces_recursively; use codex_windows_sandbox::extract_setup_failure; use codex_windows_sandbox::hide_newly_created_users; use codex_windows_sandbox::is_command_cwd_root; @@ -705,6 +706,8 @@ fn run_setup_full(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<( root.display() ), )?; + } + if need_grant || root.is_dir() { grant_tasks.push(root.clone()); } } @@ -712,6 +715,7 @@ fn run_setup_full(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<( let (tx, rx) = mpsc::channel::<(PathBuf, Result)>(); std::thread::scope(|scope| { for root in grant_tasks { + let recurse_existing_descendants = root.is_dir(); let is_command_cwd = is_command_cwd_root(&root, &canonical_command_cwd); let sid_strings = if is_command_cwd { vec![sandbox_group_sid_str.clone(), workspace_sid_str.clone()] @@ -731,7 +735,16 @@ fn run_setup_full(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<( } } - let res = unsafe { ensure_allow_write_aces(&root, &psids) }; + // Existing descendants may have missed inherited ACEs from earlier setups, + // so directory write roots need a recursive DACL refresh in addition to the + // top-level grant. + let res = unsafe { + if recurse_existing_descendants { + ensure_allow_write_aces_recursively(&root, &psids) + } else { + ensure_allow_write_aces(&root, &psids) + } + }; for psid in psids { unsafe {