From 50f6b4245b37302a241c5fca380b705a43f6a0f3 Mon Sep 17 00:00:00 2001 From: Felipe Coury Date: Sat, 4 Jul 2026 21:47:24 -0300 Subject: [PATCH 1/3] fix(windows-sandbox): grant delete rights to writable roots --- codex-rs/windows-sandbox-rs/src/spawn_prep.rs | 3 +- .../src/unified_exec/tests.rs | 102 ++++++++++++++++++ 2 files changed, 104 insertions(+), 1 deletion(-) diff --git a/codex-rs/windows-sandbox-rs/src/spawn_prep.rs b/codex-rs/windows-sandbox-rs/src/spawn_prep.rs index 382a6a453174..9479c0b8d79b 100644 --- a/codex-rs/windows-sandbox-rs/src/spawn_prep.rs +++ b/codex-rs/windows-sandbox-rs/src/spawn_prep.rs @@ -1,6 +1,7 @@ use crate::acl::add_allow_ace; use crate::acl::add_deny_write_ace; use crate::acl::allow_null_device; +use crate::acl::ensure_allow_write_aces; use crate::allow::AllowDenyPaths; use crate::allow::compute_allow_paths_for_permissions; use crate::cap::load_or_create_cap_sids; @@ -294,7 +295,7 @@ pub(crate) fn apply_legacy_session_acl_rules( let Some(root_sid) = matching_root_capability(p, acl_sids.write_root_sids) else { continue; }; - let _ = add_allow_ace(p, root_sid.sid.as_ptr()); + let _ = ensure_allow_write_aces(p, &[root_sid.sid.as_ptr()]); } } for p in &deny { diff --git a/codex-rs/windows-sandbox-rs/src/unified_exec/tests.rs b/codex-rs/windows-sandbox-rs/src/unified_exec/tests.rs index 96f5c39e9233..461e1daf2f1b 100644 --- a/codex-rs/windows-sandbox-rs/src/unified_exec/tests.rs +++ b/codex-rs/windows-sandbox-rs/src/unified_exec/tests.rs @@ -454,6 +454,108 @@ fn legacy_capture_powershell_emits_output() { ); } +#[test] +fn legacy_workspace_write_delete_is_limited_to_writable_roots() { + let _guard = legacy_process_test_guard(); + let runtime = current_thread_runtime(); + runtime.block_on(async move { + // Keep writable roots out of USERPROFILE exclusions such as AppData. + let test_root = TempDir::new_in(sandbox_cwd()).expect("create legacy delete test root"); + let codex_home = sandbox_home("legacy-delete-writable-roots"); + let workspace = test_root.path().join("workspace"); + let temp_root = test_root.path().join("temp"); + let tmp_root = test_root.path().join("tmp"); + let outside_root = test_root.path().join("outside"); + for directory in [&workspace, &temp_root, &tmp_root, &outside_root] { + fs::create_dir_all(directory).expect("create legacy delete test directory"); + } + + let workspace_file = workspace.join("workspace-delete.txt"); + let temp_file = temp_root.join("temp-delete.txt"); + let tmp_file = tmp_root.join("tmp-delete.txt"); + let outside_file = outside_root.join("outside-delete.txt"); + fs::write(&workspace_file, "workspace").expect("seed workspace file"); + fs::write(&temp_file, "temp").expect("seed TEMP file"); + fs::write(&tmp_file, "tmp").expect("seed TMP file"); + fs::write(&outside_file, "outside").expect("seed outside file"); + + let script = workspace.join("delete-fixtures.cmd"); + fs::write( + &script, + concat!( + "@echo off\r\n", + "del /f /q \"%WORKSPACE_DELETE%\"\r\n", + "del /f /q \"%TEMP_DELETE%\"\r\n", + "del /f /q \"%TMP_DELETE%\"\r\n", + "del /f /q \"%OUTSIDE_DELETE%\"\r\n", + "exit /b 0\r\n", + ), + ) + .expect("write delete script"); + + let env_map = HashMap::from([ + ("TEMP".to_string(), temp_root.to_string_lossy().into_owned()), + ("TMP".to_string(), tmp_root.to_string_lossy().into_owned()), + ( + "WORKSPACE_DELETE".to_string(), + workspace_file.to_string_lossy().into_owned(), + ), + ( + "TEMP_DELETE".to_string(), + temp_file.to_string_lossy().into_owned(), + ), + ( + "TMP_DELETE".to_string(), + tmp_file.to_string_lossy().into_owned(), + ), + ( + "OUTSIDE_DELETE".to_string(), + outside_file.to_string_lossy().into_owned(), + ), + ]); + + let permission_profile = PermissionProfile::workspace_write(); + let spawned = spawn_windows_sandbox_session_legacy( + &permission_profile, + workspace_roots_for(workspace.as_path()).as_slice(), + codex_home.path(), + vec![ + "C:\\Windows\\System32\\cmd.exe".to_string(), + "/d".to_string(), + "/c".to_string(), + script.display().to_string(), + ], + workspace.as_path(), + env_map, + /*timeout_ms*/ Some(5_000), + &[], + &[], + /*tty*/ false, + /*stdin_open*/ false, + /*use_private_desktop*/ true, + ) + .await + .expect("spawn legacy delete session"); + let (stdout, exit_code) = + collect_stdout_and_exit(spawned, codex_home.path(), Duration::from_secs(/*secs*/ 10)) + .await; + let stdout = String::from_utf8_lossy(&stdout); + + assert_eq!( + ( + exit_code, + workspace_file.exists(), + temp_file.exists(), + tmp_file.exists(), + fs::read_to_string(&outside_file).ok(), + ), + (0, false, false, false, Some("outside".to_string())), + "stdout={stdout:?}\n{}", + sandbox_log(codex_home.path()) + ); + }); +} + #[test] fn legacy_capture_cancellation_is_not_reported_as_timeout() { let Some(pwsh) = pwsh_path() else { From 620bb0e9f55ee13430dfb10e2c0ee07938544fdb Mon Sep 17 00:00:00 2001 From: Felipe Coury Date: Tue, 7 Jul 2026 14:12:47 -0300 Subject: [PATCH 2/3] fix(windows-sandbox): preserve deny-write carveouts --- codex-rs/windows-sandbox-rs/src/acl.rs | 31 ++++++++++++++++--- .../src/bin/setup_main/win.rs | 3 +- .../src/unified_exec/tests.rs | 10 +++++- 3 files changed, 37 insertions(+), 7 deletions(-) diff --git a/codex-rs/windows-sandbox-rs/src/acl.rs b/codex-rs/windows-sandbox-rs/src/acl.rs index a368371dc4ea..4b859b047f25 100644 --- a/codex-rs/windows-sandbox-rs/src/acl.rs +++ b/codex-rs/windows-sandbox-rs/src/acl.rs @@ -301,19 +301,30 @@ pub unsafe fn dacl_has_read_deny_for_sid(p_dacl: *mut ACL, psid: *mut c_void) -> false } +// Grant DELETE on each inheriting descendant instead of FILE_DELETE_CHILD on +// its parent. A parent delete-child grant would bypass a direct deny-write ACE +// on protected children such as `.git` or an explicit read-only subpath. const WRITE_ALLOW_MASK: u32 = - FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE | DELETE | FILE_DELETE_CHILD; + FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE | DELETE; unsafe fn ensure_allow_mask_aces_with_inheritance_impl( path: &Path, sids: &[*mut c_void], allow_mask: u32, + disallow_mask: u32, inheritance: u32, ) -> Result { let (p_dacl, p_sd) = fetch_dacl_handle(path)?; let mut entries: Vec = Vec::new(); for sid in sids { - if dacl_mask_allows(p_dacl, &[*sid], allow_mask, /*require_all_bits*/ true) { + if dacl_mask_allows(p_dacl, &[*sid], allow_mask, /*require_all_bits*/ true) + && !dacl_mask_allows( + p_dacl, + &[*sid], + disallow_mask, + /*require_all_bits*/ false, + ) + { continue; } entries.push(EXPLICIT_ACCESS_W { @@ -386,7 +397,13 @@ pub unsafe fn ensure_allow_mask_aces_with_inheritance( allow_mask: u32, inheritance: u32, ) -> Result { - ensure_allow_mask_aces_with_inheritance_impl(path, sids, allow_mask, inheritance) + ensure_allow_mask_aces_with_inheritance_impl( + path, + sids, + allow_mask, + /*disallow_mask*/ 0, + inheritance, + ) } /// Ensure all provided SIDs have an allow ACE with the requested mask on the path. @@ -413,7 +430,13 @@ pub unsafe fn ensure_allow_mask_aces( /// # 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(path: &Path, sids: &[*mut c_void]) -> Result { - ensure_allow_mask_aces(path, sids, WRITE_ALLOW_MASK) + ensure_allow_mask_aces_with_inheritance_impl( + path, + sids, + WRITE_ALLOW_MASK, + FILE_DELETE_CHILD, + CONTAINER_INHERIT_ACE | OBJECT_INHERIT_ACE, + ) } /// Adds an allow ACE granting read/write/execute to the given SID on the target path. 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 e1146987bf61..560dccb2dbff 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 @@ -826,8 +826,7 @@ fn run_setup_full(payload: &Payload, log: &mut dyn Write, sbx_dir: &Path) -> Res )?; } - let write_mask = - FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE | DELETE | FILE_DELETE_CHILD; + let write_mask = FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE | DELETE; let mut grant_tasks: Vec<(PathBuf, String)> = Vec::new(); let mut seen_deny_paths: HashSet = HashSet::new(); diff --git a/codex-rs/windows-sandbox-rs/src/unified_exec/tests.rs b/codex-rs/windows-sandbox-rs/src/unified_exec/tests.rs index 461e1daf2f1b..12159ec9912d 100644 --- a/codex-rs/windows-sandbox-rs/src/unified_exec/tests.rs +++ b/codex-rs/windows-sandbox-rs/src/unified_exec/tests.rs @@ -469,6 +469,8 @@ fn legacy_workspace_write_delete_is_limited_to_writable_roots() { for directory in [&workspace, &temp_root, &tmp_root, &outside_root] { fs::create_dir_all(directory).expect("create legacy delete test directory"); } + let protected_git_dir = workspace.join(".git"); + fs::create_dir(&protected_git_dir).expect("create protected .git directory"); let workspace_file = workspace.join("workspace-delete.txt"); let temp_file = temp_root.join("temp-delete.txt"); @@ -488,6 +490,7 @@ fn legacy_workspace_write_delete_is_limited_to_writable_roots() { "del /f /q \"%TEMP_DELETE%\"\r\n", "del /f /q \"%TMP_DELETE%\"\r\n", "del /f /q \"%OUTSIDE_DELETE%\"\r\n", + "rmdir \"%PROTECTED_GIT_DIR%\"\r\n", "exit /b 0\r\n", ), ) @@ -512,6 +515,10 @@ fn legacy_workspace_write_delete_is_limited_to_writable_roots() { "OUTSIDE_DELETE".to_string(), outside_file.to_string_lossy().into_owned(), ), + ( + "PROTECTED_GIT_DIR".to_string(), + protected_git_dir.to_string_lossy().into_owned(), + ), ]); let permission_profile = PermissionProfile::workspace_write(); @@ -548,8 +555,9 @@ fn legacy_workspace_write_delete_is_limited_to_writable_roots() { temp_file.exists(), tmp_file.exists(), fs::read_to_string(&outside_file).ok(), + protected_git_dir.is_dir(), ), - (0, false, false, false, Some("outside".to_string())), + (0, false, false, false, Some("outside".to_string()), true), "stdout={stdout:?}\n{}", sandbox_log(codex_home.path()) ); From 767540eec35cc88dafc34d87fcec7e58087413fb Mon Sep 17 00:00:00 2001 From: Felipe Coury Date: Tue, 7 Jul 2026 14:26:36 -0300 Subject: [PATCH 3/3] fix(windows-sandbox): refresh stale delete-child grants --- .../src/bin/setup_main/win.rs | 95 +++++++++++++++---- 1 file changed, 75 insertions(+), 20 deletions(-) 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 560dccb2dbff..efe9f6af9e53 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 @@ -65,6 +65,8 @@ use windows_sys::Win32::Storage::FileSystem::FILE_GENERIC_READ; use windows_sys::Win32::Storage::FileSystem::FILE_GENERIC_WRITE; const DENY_ACCESS: i32 = 3; +const WRITE_ROOT_ALLOW_MASK: u32 = + FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE | DELETE; mod sandbox_users; mod setup_runtime_bin; @@ -158,6 +160,23 @@ fn workspace_write_cap_sids_for_path( Ok(sid_strs) } +fn write_root_needs_refresh(root: &Path, psid: *mut c_void) -> Result { + if !path_mask_allows( + root, + &[psid], + WRITE_ROOT_ALLOW_MASK, + /*require_all_bits*/ true, + )? { + return Ok(true); + } + path_mask_allows( + root, + &[psid], + FILE_DELETE_CHILD, + /*require_all_bits*/ false, + ) +} + fn spawn_read_acl_helper(payload: &Payload, _log: &mut dyn Write) -> Result<()> { let mut read_payload = payload.clone(); read_payload.mode = SetupMode::ReadAclsOnly; @@ -826,7 +845,6 @@ fn run_setup_full(payload: &Payload, log: &mut dyn Write, sbx_dir: &Path) -> Res )?; } - let write_mask = FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE | DELETE; let mut grant_tasks: Vec<(PathBuf, String)> = Vec::new(); let mut seen_deny_paths: HashSet = HashSet::new(); @@ -861,27 +879,26 @@ fn run_setup_full(payload: &Payload, log: &mut dyn Write, sbx_dir: &Path) -> Res ("sandbox_group", sandbox_group_psid), (cap_label, root_cap_psid), ] { - 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}: {}", + let needs_refresh = match write_root_needs_refresh(root, psid) { + Ok(needs_refresh) => needs_refresh, + Err(e) => { + refresh_errors.push(format!( + "write ACE check failed on {} for {label}: {}", + root.display(), + e + )); + log_line( + log, + &format!( + "write ACE check failed on {} for {label}: {}; continuing", root.display(), e - )); - log_line( - log, - &format!( - "write mask check failed on {} for {label}: {}; continuing", - root.display(), - e - ), - )?; - false - } - }; - if !has { + ), + )?; + true + } + }; + if needs_refresh { need_grant = true; } } @@ -1035,13 +1052,21 @@ fn run_setup_full(payload: &Payload, log: &mut dyn Write, sbx_dir: &Path) -> Res mod tests { use super::Payload; use super::SETUP_VERSION; + use super::WRITE_ROOT_ALLOW_MASK; + use super::convert_string_sid_to_sid; use super::workspace_write_cap_sids_for_path; + use super::write_root_needs_refresh; use codex_otel::StatsigMetricsSettings; + 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::workspace_write_cap_sid_for_root; use pretty_assertions::assert_eq; use serde_json::json; use std::fs; + use windows_sys::Win32::Foundation::HLOCAL; + use windows_sys::Win32::Foundation::LocalFree; + use windows_sys::Win32::Storage::FileSystem::FILE_DELETE_CHILD; fn payload_json() -> serde_json::Value { json!({ @@ -1089,6 +1114,36 @@ mod tests { ); } + #[test] + fn write_root_refresh_replaces_stale_delete_child_grant() { + let temp = tempfile::tempdir().expect("tempdir"); + let codex_home = temp.path().join("codex-home"); + let workspace = temp.path().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 stale_write_mask = WRITE_ROOT_ALLOW_MASK | FILE_DELETE_CHILD; + let seeded = unsafe { ensure_allow_mask_aces(&workspace, &[psid], stale_write_mask) } + .expect("seed stale write ACE"); + let needs_refresh_before = + write_root_needs_refresh(&workspace, psid).expect("check stale write ACE"); + let replaced = unsafe { ensure_allow_write_aces(&workspace, &[psid]) } + .expect("replace stale write ACE"); + let needs_refresh_after = + write_root_needs_refresh(&workspace, psid).expect("check refreshed write ACE"); + unsafe { + LocalFree(psid as HLOCAL); + } + + assert_eq!( + (seeded, needs_refresh_before, replaced, needs_refresh_after), + (true, true, true, false) + ); + } + #[test] fn deny_path_under_active_root_uses_only_matching_root_sid() { let temp = tempfile::tempdir().expect("tempdir");