From 25ecd0c2e49788585528e9aebc46075269ccb7ca Mon Sep 17 00:00:00 2001 From: iceweasel-oai Date: Wed, 17 Dec 2025 15:27:52 -0800 Subject: [PATCH] speed and reliability improvements for setting reads ACLs (#8216) - Batch read ACL creation for online/offline sandbox user - creates a new ACL helper process that is long-lived and runs in the background - uses a mutex so that only one helper process is running at a time. --- codex-rs/windows-sandbox-rs/src/acl.rs | 22 +- codex-rs/windows-sandbox-rs/src/lib.rs | 2 + .../windows-sandbox-rs/src/read_acl_mutex.rs | 62 +++ .../windows-sandbox-rs/src/setup_main_win.rs | 451 +++++++++++------- 4 files changed, 360 insertions(+), 177 deletions(-) create mode 100644 codex-rs/windows-sandbox-rs/src/read_acl_mutex.rs diff --git a/codex-rs/windows-sandbox-rs/src/acl.rs b/codex-rs/windows-sandbox-rs/src/acl.rs index f84c53689..eee826d66 100644 --- a/codex-rs/windows-sandbox-rs/src/acl.rs +++ b/codex-rs/windows-sandbox-rs/src/acl.rs @@ -342,21 +342,25 @@ const WRITE_ALLOW_MASK: u32 = FILE_GENERIC_READ | DELETE | FILE_DELETE_CHILD; -/// Ensure all provided SIDs have a write-capable allow ACE on the path. +/// Ensure all provided SIDs have an allow ACE with the requested mask on the path. /// Returns true if any ACE was added. /// /// # Safety /// Caller must pass valid SID pointers and an existing path; free the returned security descriptor with `LocalFree`. #[allow(dead_code)] -pub unsafe fn ensure_allow_write_aces(path: &Path, sids: &[*mut c_void]) -> Result { +pub unsafe fn ensure_allow_mask_aces( + path: &Path, + sids: &[*mut c_void], + allow_mask: 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], WRITE_ALLOW_MASK, true) { + if dacl_mask_allows(p_dacl, &[*sid], allow_mask, true) { continue; } entries.push(EXPLICIT_ACCESS_W { - grfAccessPermissions: WRITE_ALLOW_MASK, + grfAccessPermissions: allow_mask, grfAccessMode: 2, // SET_ACCESS grfInheritance: CONTAINER_INHERIT_ACE | OBJECT_INHERIT_ACE, Trustee: TRUSTEE_W { @@ -414,6 +418,16 @@ pub unsafe fn ensure_allow_write_aces(path: &Path, sids: &[*mut c_void]) -> Resu Ok(added) } +/// Ensure all provided SIDs have a write-capable allow ACE on the path. +/// Returns true if any ACE was added. +/// +/// # Safety +/// Caller must pass valid SID pointers and an existing path; free the returned security descriptor with `LocalFree`. +#[allow(dead_code)] +pub unsafe fn ensure_allow_write_aces(path: &Path, sids: &[*mut c_void]) -> Result { + ensure_allow_mask_aces(path, sids, WRITE_ALLOW_MASK) +} + /// 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 700075d7a..7373b7ad4 100644 --- a/codex-rs/windows-sandbox-rs/src/lib.rs +++ b/codex-rs/windows-sandbox-rs/src/lib.rs @@ -18,6 +18,8 @@ mod elevated_impl; #[cfg(target_os = "windows")] pub use acl::allow_null_device; #[cfg(target_os = "windows")] +pub use acl::ensure_allow_mask_aces; +#[cfg(target_os = "windows")] pub use acl::ensure_allow_write_aces; #[cfg(target_os = "windows")] pub use acl::fetch_dacl_handle; diff --git a/codex-rs/windows-sandbox-rs/src/read_acl_mutex.rs b/codex-rs/windows-sandbox-rs/src/read_acl_mutex.rs new file mode 100644 index 000000000..0b3435fec --- /dev/null +++ b/codex-rs/windows-sandbox-rs/src/read_acl_mutex.rs @@ -0,0 +1,62 @@ +use anyhow::Result; +use std::ffi::OsStr; +use windows_sys::Win32::Foundation::CloseHandle; +use windows_sys::Win32::Foundation::GetLastError; +use windows_sys::Win32::Foundation::ERROR_ALREADY_EXISTS; +use windows_sys::Win32::Foundation::ERROR_FILE_NOT_FOUND; +use windows_sys::Win32::Foundation::HANDLE; +use windows_sys::Win32::System::Threading::CreateMutexW; +use windows_sys::Win32::System::Threading::OpenMutexW; +use windows_sys::Win32::System::Threading::ReleaseMutex; +use windows_sys::Win32::System::Threading::MUTEX_ALL_ACCESS; + +use super::to_wide; + +const READ_ACL_MUTEX_NAME: &str = "Local\\CodexSandboxReadAcl"; + +pub struct ReadAclMutexGuard { + handle: HANDLE, +} + +impl Drop for ReadAclMutexGuard { + fn drop(&mut self) { + unsafe { + let _ = ReleaseMutex(self.handle); + CloseHandle(self.handle); + } + } +} + +pub fn read_acl_mutex_exists() -> Result { + let name = to_wide(OsStr::new(READ_ACL_MUTEX_NAME)); + let handle = unsafe { OpenMutexW(MUTEX_ALL_ACCESS, 0, name.as_ptr()) }; + if handle == 0 { + let err = unsafe { GetLastError() }; + if err == ERROR_FILE_NOT_FOUND { + return Ok(false); + } + return Err(anyhow::anyhow!("OpenMutexW failed: {}", err)); + } + unsafe { + CloseHandle(handle); + } + Ok(true) +} + +pub fn acquire_read_acl_mutex() -> Result> { + let name = to_wide(OsStr::new(READ_ACL_MUTEX_NAME)); + let handle = unsafe { CreateMutexW(std::ptr::null_mut(), 1, name.as_ptr()) }; + if handle == 0 { + return Err(anyhow::anyhow!("CreateMutexW failed: {}", unsafe { + GetLastError() + })); + } + let err = unsafe { GetLastError() }; + if err == ERROR_ALREADY_EXISTS { + unsafe { + CloseHandle(handle); + } + return Ok(None); + } + Ok(Some(ReadAclMutexGuard { handle })) +} 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 dccbd9769..9a2f5ec24 100644 --- a/codex-rs/windows-sandbox-rs/src/setup_main_win.rs +++ b/codex-rs/windows-sandbox-rs/src/setup_main_win.rs @@ -6,8 +6,8 @@ use base64::engine::general_purpose::STANDARD as BASE64; use base64::Engine; use codex_windows_sandbox::convert_string_sid_to_sid; use codex_windows_sandbox::dpapi_protect; +use codex_windows_sandbox::ensure_allow_mask_aces; use codex_windows_sandbox::ensure_allow_write_aces; -use codex_windows_sandbox::fetch_dacl_handle; use codex_windows_sandbox::load_or_create_cap_sids; use codex_windows_sandbox::log_note; use codex_windows_sandbox::path_mask_allows; @@ -26,10 +26,13 @@ use std::ffi::OsStr; use std::fs::File; use std::io::Write; use std::os::windows::ffi::OsStrExt; +use std::os::windows::process::CommandExt; use std::path::Path; use std::path::PathBuf; +use std::process::Command; +use std::process::Stdio; use std::sync::mpsc; -use std::time::Duration; +use std::time::Instant; use windows::core::Interface; use windows::core::BSTR; use windows::Win32::Foundation::VARIANT_TRUE; @@ -80,7 +83,11 @@ 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; -#[derive(Debug, Deserialize)] +mod read_acl_mutex; +use read_acl_mutex::acquire_read_acl_mutex; +use read_acl_mutex::read_acl_mutex_exists; + +#[derive(Debug, Clone, Deserialize, Serialize)] struct Payload { version: u32, offline_username: String, @@ -90,9 +97,24 @@ struct Payload { write_roots: Vec, real_user: String, #[serde(default)] + mode: SetupMode, + #[serde(default)] refresh_only: bool, } +#[derive(Debug, Clone, Copy, Deserialize, Serialize, PartialEq, Eq)] +#[serde(rename_all = "kebab-case")] +enum SetupMode { + Full, + ReadAclsOnly, +} + +impl Default for SetupMode { + fn default() -> Self { + Self::Full + } +} + #[derive(Serialize)] struct SandboxUserRecord { username: String, @@ -245,90 +267,191 @@ fn resolve_sid(name: &str) -> Result> { } } -fn add_inheritable_allow_no_log(path: &Path, sid: &[u8], mask: u32) -> Result<()> { - unsafe { - let mut psid: *mut c_void = std::ptr::null_mut(); - let sid_str = string_from_sid_bytes(sid).map_err(anyhow::Error::msg)?; - let sid_w = to_wide(OsStr::new(&sid_str)); - if ConvertStringSidToSidW(sid_w.as_ptr(), &mut psid) == 0 { - return Err(anyhow::anyhow!( - "ConvertStringSidToSidW failed: {}", - GetLastError() - )); +fn spawn_read_acl_helper(payload: &Payload, log: &mut File) -> Result<()> { + let mut read_payload = payload.clone(); + read_payload.mode = SetupMode::ReadAclsOnly; + read_payload.refresh_only = true; + let payload_json = serde_json::to_vec(&read_payload)?; + let payload_b64 = BASE64.encode(payload_json); + let exe = std::env::current_exe().context("locate setup helper")?; + let child = Command::new(&exe) + .arg(payload_b64) + .stdin(Stdio::null()) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .creation_flags(0x08000000) // CREATE_NO_WINDOW + .spawn() + .context("spawn read ACL helper")?; + let pid = child.id(); + log_line(log, &format!("spawned read ACL helper pid={pid}"))?; + Ok(()) +} + +struct ReadAclSubjects<'a> { + offline_psid: *mut c_void, + online_psid: *mut c_void, + rx_psids: &'a [*mut c_void], +} + +fn apply_read_acls( + read_roots: &[PathBuf], + subjects: ReadAclSubjects<'_>, + log: &mut File, + refresh_errors: &mut Vec, +) -> Result<()> { + log_line( + log, + &format!("read ACL: processing {} read roots", read_roots.len()), + )?; + let read_mask = FILE_GENERIC_READ | FILE_GENERIC_EXECUTE; + for root in read_roots { + if !root.exists() { + log_line( + log, + &format!("read root {} missing; skipping", root.display()), + )?; + continue; } - let (existing_dacl, sd) = fetch_dacl_handle(path)?; - let trustee = TRUSTEE_W { - pMultipleTrustee: std::ptr::null_mut(), - MultipleTrusteeOperation: 0, - TrusteeForm: TRUSTEE_IS_SID, - TrusteeType: TRUSTEE_IS_SID, - ptstrName: psid as *mut u16, - }; - let ea = EXPLICIT_ACCESS_W { - grfAccessPermissions: mask, - grfAccessMode: GRANT_ACCESS, - grfInheritance: OBJECT_INHERIT_ACE | CONTAINER_INHERIT_ACE, - Trustee: trustee, - }; - let mut new_dacl: *mut ACL = std::ptr::null_mut(); - let set = SetEntriesInAclW(1, &ea, existing_dacl, &mut new_dacl); - if set != 0 { - return Err(anyhow::anyhow!("SetEntriesInAclW failed: {}", set)); + let builtin_has = read_mask_allows_or_log( + root, + subjects.rx_psids, + None, + read_mask, + refresh_errors, + log, + )?; + if builtin_has { + log_line( + log, + &format!( + "Users/AU/Everyone already has RX on {}; skipping", + root.display() + ), + )?; + continue; } - let res = SetNamedSecurityInfoW( - to_wide(path.as_os_str()).as_ptr() as *mut u16, - SE_FILE_OBJECT, - DACL_SECURITY_INFORMATION, - std::ptr::null_mut(), - std::ptr::null_mut(), - new_dacl, - std::ptr::null_mut(), - ); - if res != 0 { - return Err(anyhow::anyhow!( - "SetNamedSecurityInfoW failed for {}: {}", - path.display(), - res - )); + let offline_has = read_mask_allows_or_log( + root, + &[subjects.offline_psid], + Some("offline"), + read_mask, + refresh_errors, + log, + )?; + let online_has = read_mask_allows_or_log( + root, + &[subjects.online_psid], + Some("online"), + read_mask, + refresh_errors, + log, + )?; + if offline_has && online_has { + log_line( + log, + &format!( + "sandbox users already have RX on {}; skipping", + root.display() + ), + )?; + continue; } - if !new_dacl.is_null() { - LocalFree(new_dacl as HLOCAL); + log_line( + log, + &format!("granting read ACE to {} for sandbox users", root.display()), + )?; + let mut successes = usize::from(offline_has) + usize::from(online_has); + let mut missing_psids: Vec<*mut c_void> = Vec::new(); + let mut missing_labels: Vec<&str> = Vec::new(); + if !offline_has { + missing_psids.push(subjects.offline_psid); + missing_labels.push("offline"); } - if !sd.is_null() { - LocalFree(sd as HLOCAL); + if !online_has { + missing_psids.push(subjects.online_psid); + missing_labels.push("online"); } - if !psid.is_null() { - LocalFree(psid as HLOCAL); + if !missing_psids.is_empty() { + let started = Instant::now(); + match unsafe { ensure_allow_mask_aces(root, &missing_psids, read_mask) } { + Ok(_) => { + let elapsed_ms = started.elapsed().as_millis(); + successes = 2; + log_line( + log, + &format!( + "grant read ACE succeeded on {} for {} in {elapsed_ms}ms", + root.display(), + missing_labels.join(", ") + ), + )?; + } + Err(err) => { + let elapsed_ms = started.elapsed().as_millis(); + let label_list = missing_labels.join(", "); + for label in &missing_labels { + refresh_errors.push(format!( + "grant read ACE failed on {} for {label}: {err}", + root.display() + )); + } + log_line( + log, + &format!( + "grant read ACE failed on {} for {} in {elapsed_ms}ms: {err}", + root.display(), + label_list + ), + )?; + } + } + } + if successes == 2 { + log_line(log, &format!("granted read ACE to {}", root.display()))?; + } else { + log_line( + log, + &format!( + "read ACE incomplete on {} (success {successes}/2)", + root.display() + ), + )?; } } Ok(()) } -fn try_add_inheritable_allow_with_timeout( - path: &Path, - sid: &[u8], - mask: u32, - _log: &mut File, - timeout: Duration, -) -> Result<()> { - let (tx, rx) = mpsc::channel::>(); - let path_buf = path.to_path_buf(); - let sid_vec = sid.to_vec(); - std::thread::spawn(move || { - let res = add_inheritable_allow_no_log(&path_buf, &sid_vec, mask); - let _ = tx.send(res); - }); - match rx.recv_timeout(timeout) { - Ok(res) => res, - Err(mpsc::RecvTimeoutError::Timeout) => Err(anyhow::anyhow!( - "ACL grant timed out on {} after {:?}", - path.display(), - timeout - )), - Err(e) => Err(anyhow::anyhow!( - "ACL grant channel error on {}: {e}", - path.display() - )), +fn read_mask_allows_or_log( + root: &Path, + psids: &[*mut c_void], + label: Option<&str>, + read_mask: u32, + refresh_errors: &mut Vec, + log: &mut File, +) -> Result { + match path_mask_allows(root, psids, read_mask, true) { + Ok(has) => Ok(has), + Err(e) => { + let label_suffix = label + .map(|value| format!(" for {value}")) + .unwrap_or_default(); + refresh_errors.push(format!( + "read mask check failed on {}{}: {}", + root.display(), + label_suffix, + e + )); + log_line( + log, + &format!( + "read mask check failed on {}{}: {}; continuing", + root.display(), + label_suffix, + e + ), + )?; + Ok(false) + } } } @@ -607,6 +730,70 @@ fn real_main() -> Result<()> { } fn run_setup(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<()> { + match payload.mode { + SetupMode::ReadAclsOnly => run_read_acl_only(payload, log), + SetupMode::Full => run_setup_full(payload, log, sbx_dir), + } +} + +fn run_read_acl_only(payload: &Payload, log: &mut File) -> Result<()> { + let _read_acl_guard = match acquire_read_acl_mutex()? { + Some(guard) => guard, + None => { + log_line(log, "read ACL helper already running; skipping")?; + return Ok(()); + } + }; + log_line(log, "read-acl-only mode: applying read ACLs")?; + let offline_sid = resolve_sid(&payload.offline_username)?; + let online_sid = resolve_sid(&payload.online_username)?; + let offline_psid = sid_bytes_to_psid(&offline_sid)?; + let online_psid = sid_bytes_to_psid(&online_sid)?; + let mut refresh_errors: Vec = Vec::new(); + let users_sid = resolve_sid("Users")?; + let users_psid = sid_bytes_to_psid(&users_sid)?; + let auth_sid = resolve_sid("Authenticated Users")?; + let auth_psid = sid_bytes_to_psid(&auth_sid)?; + let everyone_sid = resolve_sid("Everyone")?; + let everyone_psid = sid_bytes_to_psid(&everyone_sid)?; + let rx_psids = vec![users_psid, auth_psid, everyone_psid]; + let subjects = ReadAclSubjects { + offline_psid, + online_psid, + rx_psids: &rx_psids, + }; + apply_read_acls(&payload.read_roots, subjects, log, &mut refresh_errors)?; + unsafe { + if !offline_psid.is_null() { + LocalFree(offline_psid as HLOCAL); + } + if !online_psid.is_null() { + LocalFree(online_psid as HLOCAL); + } + if !users_psid.is_null() { + LocalFree(users_psid as HLOCAL); + } + if !auth_psid.is_null() { + LocalFree(auth_psid as HLOCAL); + } + if !everyone_psid.is_null() { + LocalFree(everyone_psid as HLOCAL); + } + } + if !refresh_errors.is_empty() { + log_line( + log, + &format!("read ACL run completed with errors: {:?}", refresh_errors), + )?; + if payload.refresh_only { + anyhow::bail!("read ACL run had errors"); + } + } + log_line(log, "read ACL run completed")?; + Ok(()) +} + +fn run_setup_full(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<()> { let refresh_only = payload.refresh_only; let offline_pwd = if refresh_only { None @@ -654,13 +841,6 @@ fn run_setup(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<()> { .ok_or_else(|| anyhow::anyhow!("convert capability SID failed"))? }; let mut refresh_errors: Vec = Vec::new(); - let users_sid = resolve_sid("Users")?; - let users_psid = sid_bytes_to_psid(&users_sid)?; - let auth_sid = resolve_sid("Authenticated Users")?; - let auth_psid = sid_bytes_to_psid(&auth_sid)?; - let everyone_sid = resolve_sid("Everyone")?; - let everyone_psid = sid_bytes_to_psid(&everyone_sid)?; - let rx_psids = vec![users_psid, auth_psid, everyone_psid]; log_line(log, &format!("resolved capability SID {}", caps.workspace))?; if !refresh_only { run_netsh_firewall(&offline_sid_str, log)?; @@ -669,94 +849,29 @@ fn run_setup(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<()> { log_line( log, &format!( - "refresh: processing {} read roots, {} write roots", + "refresh: delegating {} read roots; processing {} write roots", payload.read_roots.len(), payload.write_roots.len() ), )?; - for root in &payload.read_roots { - if !root.exists() { - log_line( - log, - &format!("read root {} missing; skipping", root.display()), - )?; - continue; - } - match path_mask_allows( - root, - &rx_psids, - FILE_GENERIC_READ | FILE_GENERIC_EXECUTE, - true, - ) { - Ok(has) => { - if has { - log_line( - log, - &format!( - "Users/AU/Everyone already has RX on {}; skipping", - root.display() - ), - )?; - continue; - } + if payload.read_roots.is_empty() { + log_line(log, "no read roots to grant; skipping read ACL helper")?; + } else { + match read_acl_mutex_exists() { + Ok(true) => { + log_line(log, "read ACL helper already running; skipping spawn")?; } - Err(e) => { - refresh_errors.push(format!( - "read mask check failed on {}: {}", - root.display(), - e - )); + Ok(false) => { + spawn_read_acl_helper(payload, log)?; + } + Err(err) => { log_line( log, - &format!( - "read mask check failed on {}: {}; continuing", - root.display(), - e - ), + &format!("read ACL mutex check failed: {err}; spawning anyway"), )?; + spawn_read_acl_helper(payload, log)?; } } - log_line( - log, - &format!("granting read ACE to {} for sandbox users", root.display()), - )?; - let mut successes = 0usize; - let read_mask = FILE_GENERIC_READ | FILE_GENERIC_EXECUTE; - for (label, sid_bytes) in [("offline", &offline_sid), ("online", &online_sid)] { - match try_add_inheritable_allow_with_timeout( - root, - sid_bytes, - read_mask, - log, - Duration::from_millis(100), - ) { - Ok(_) => { - successes += 1; - } - Err(e) => { - log_line( - log, - &format!( - "grant read ACE timed out/failed on {} for {label}: {e}", - root.display() - ), - )?; - // Best-effort: continue to next SID/root. - } - } - } - if successes == 2 { - log_line(log, &format!("granted read ACE to {}", root.display()))?; - } else { - log_line( - log, - &format!( - "read ACE incomplete on {} (success {}/2)", - root.display(), - successes - ), - )?; - } } let cap_sid_str = caps.workspace.clone(); @@ -899,8 +1014,7 @@ fn run_setup(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<()> { log_line( log, &format!( - "setup refresh: processed {} read roots, {} write roots; errors={:?}", - payload.read_roots.len(), + "setup refresh: processed {} write roots (read roots delegated); errors={:?}", payload.write_roots.len(), refresh_errors ), @@ -938,15 +1052,6 @@ fn run_setup(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<()> { if !cap_psid.is_null() { LocalFree(cap_psid as HLOCAL); } - if !users_psid.is_null() { - LocalFree(users_psid as HLOCAL); - } - if !auth_psid.is_null() { - LocalFree(auth_psid as HLOCAL); - } - if !everyone_psid.is_null() { - LocalFree(everyone_psid as HLOCAL); - } } if refresh_only && !refresh_errors.is_empty() { log_line(