From 3d14da9728b7247a1a6036a7bf5697c9b4c33e43 Mon Sep 17 00:00:00 2001 From: iceweasel-oai Date: Tue, 16 Dec 2025 09:48:29 -0800 Subject: [PATCH] bug fixes and perf improvements for elevated sandbox setup (#8094) a few fixes based on testing feedback: * ensure cap_sid file is always written by elevated setup. * always log to same file whether using elevated sandbox or not * process potentially slow ACE write operations in parallel * dedupe write roots so we don't double process any * don't try to create read/write ACEs on the same directories, due to race condition --- codex-rs/windows-sandbox-rs/src/audit.rs | 2 +- codex-rs/windows-sandbox-rs/src/cap.rs | 44 +++++--- .../windows-sandbox-rs/src/elevated_impl.rs | 54 +++------ codex-rs/windows-sandbox-rs/src/lib.rs | 29 ++--- .../windows-sandbox-rs/src/setup_main_win.rs | 104 +++++++++++++----- .../src/setup_orchestrator.rs | 73 +++++++++--- 6 files changed, 186 insertions(+), 120 deletions(-) diff --git a/codex-rs/windows-sandbox-rs/src/audit.rs b/codex-rs/windows-sandbox-rs/src/audit.rs index 4d63cf7a7..4385a3350 100644 --- a/codex-rs/windows-sandbox-rs/src/audit.rs +++ b/codex-rs/windows-sandbox-rs/src/audit.rs @@ -251,7 +251,7 @@ pub fn apply_capability_denies_for_world_writable( } std::fs::create_dir_all(codex_home)?; let cap_path = cap_sid_file(codex_home); - let caps = load_or_create_cap_sids(codex_home); + let caps = load_or_create_cap_sids(codex_home)?; std::fs::write(&cap_path, serde_json::to_string(&caps)?)?; let (active_sid, workspace_roots): (*mut c_void, Vec) = match sandbox_policy { SandboxPolicy::WorkspaceWrite { writable_roots, .. } => { diff --git a/codex-rs/windows-sandbox-rs/src/cap.rs b/codex-rs/windows-sandbox-rs/src/cap.rs index 2f4c96237..510068496 100644 --- a/codex-rs/windows-sandbox-rs/src/cap.rs +++ b/codex-rs/windows-sandbox-rs/src/cap.rs @@ -1,3 +1,5 @@ +use anyhow::Context; +use anyhow::Result; use rand::rngs::SmallRng; use rand::RngCore; use rand::SeedableRng; @@ -26,25 +28,39 @@ fn make_random_cap_sid_string() -> String { format!("S-1-5-21-{}-{}-{}-{}", a, b, c, d) } -pub fn load_or_create_cap_sids(codex_home: &Path) -> CapSids { +fn persist_caps(path: &Path, caps: &CapSids) -> Result<()> { + if let Some(dir) = path.parent() { + fs::create_dir_all(dir) + .with_context(|| format!("create cap sid dir {}", dir.display()))?; + } + let json = serde_json::to_string(caps)?; + fs::write(path, json).with_context(|| format!("write cap sid file {}", path.display()))?; + Ok(()) +} + +pub fn load_or_create_cap_sids(codex_home: &Path) -> Result { let path = cap_sid_file(codex_home); if path.exists() { - if let Ok(txt) = fs::read_to_string(&path) { - let t = txt.trim(); - if t.starts_with('{') && t.ends_with('}') { - if let Ok(obj) = serde_json::from_str::(t) { - return obj; - } - } else if !t.is_empty() { - return CapSids { - workspace: t.to_string(), - readonly: make_random_cap_sid_string(), - }; + let txt = fs::read_to_string(&path) + .with_context(|| format!("read cap sid file {}", path.display()))?; + let t = txt.trim(); + if t.starts_with('{') && t.ends_with('}') { + if let Ok(obj) = serde_json::from_str::(t) { + return Ok(obj); } + } else if !t.is_empty() { + let caps = CapSids { + workspace: t.to_string(), + readonly: make_random_cap_sid_string(), + }; + persist_caps(&path, &caps)?; + return Ok(caps); } } - CapSids { + let caps = CapSids { workspace: make_random_cap_sid_string(), readonly: make_random_cap_sid_string(), - } + }; + persist_caps(&path, &caps)?; + Ok(caps) } diff --git a/codex-rs/windows-sandbox-rs/src/elevated_impl.rs b/codex-rs/windows-sandbox-rs/src/elevated_impl.rs index b72ee7597..bf3d50147 100644 --- a/codex-rs/windows-sandbox-rs/src/elevated_impl.rs +++ b/codex-rs/windows-sandbox-rs/src/elevated_impl.rs @@ -2,7 +2,6 @@ mod windows_impl { use crate::acl::allow_null_device; use crate::allow::compute_allow_paths; use crate::allow::AllowDenyPaths; - use crate::cap::cap_sid_file; use crate::cap::load_or_create_cap_sids; use crate::env::ensure_non_interactive_pager; use crate::env::inherit_path_env; @@ -53,13 +52,6 @@ mod windows_impl { use windows_sys::Win32::System::Threading::STARTUPINFOW; /// Ensures the parent directory of a path exists before writing to it. - fn ensure_dir(p: &Path) -> Result<()> { - if let Some(d) = p.parent() { - std::fs::create_dir_all(d)?; - } - Ok(()) - } - /// Walks upward from `start` to locate the git worktree root, following gitfile redirects. fn find_git_root(start: &Path) -> Option { let mut cur = dunce::canonicalize(start).ok()?; @@ -246,44 +238,26 @@ mod windows_impl { let sandbox_creds = require_logon_sandbox_creds(&policy, sandbox_policy_cwd, cwd, &env_map, codex_home)?; log_note("cli creds ready", logs_base_dir); - let cap_sid_path = cap_sid_file(codex_home); - // Build capability SID for ACL grants. + if matches!(&policy, SandboxPolicy::DangerFullAccess) { + anyhow::bail!("DangerFullAccess is not supported for sandboxing") + } + let caps = load_or_create_cap_sids(codex_home)?; let (psid_to_use, cap_sid_str) = match &policy { - SandboxPolicy::ReadOnly => { - let caps = load_or_create_cap_sids(codex_home); - ensure_dir(&cap_sid_path)?; - fs::write(&cap_sid_path, serde_json::to_string(&caps)?)?; - ( - unsafe { convert_string_sid_to_sid(&caps.readonly).unwrap() }, - caps.readonly.clone(), - ) - } - SandboxPolicy::WorkspaceWrite { .. } => { - let caps = load_or_create_cap_sids(codex_home); - ensure_dir(&cap_sid_path)?; - fs::write(&cap_sid_path, serde_json::to_string(&caps)?)?; - ( - unsafe { convert_string_sid_to_sid(&caps.workspace).unwrap() }, - caps.workspace.clone(), - ) - } - SandboxPolicy::DangerFullAccess => { - anyhow::bail!("DangerFullAccess is not supported for sandboxing") - } + SandboxPolicy::ReadOnly => ( + unsafe { convert_string_sid_to_sid(&caps.readonly).unwrap() }, + caps.readonly.clone(), + ), + SandboxPolicy::WorkspaceWrite { .. } => ( + unsafe { convert_string_sid_to_sid(&caps.workspace).unwrap() }, + caps.workspace.clone(), + ), + SandboxPolicy::DangerFullAccess => unreachable!("DangerFullAccess handled above"), }; - let AllowDenyPaths { allow, deny } = + let AllowDenyPaths { allow: _, deny: _ } = compute_allow_paths(&policy, sandbox_policy_cwd, ¤t_dir, &env_map); // Deny/allow ACEs are now applied during setup; avoid per-command churn. - log_note( - &format!( - "cli skipping per-command ACL grants (allow_count={} deny_count={})", - allow.len(), - deny.len() - ), - logs_base_dir, - ); unsafe { allow_null_device(psid_to_use); } diff --git a/codex-rs/windows-sandbox-rs/src/lib.rs b/codex-rs/windows-sandbox-rs/src/lib.rs index 1bcfa1207..700075d7a 100644 --- a/codex-rs/windows-sandbox-rs/src/lib.rs +++ b/codex-rs/windows-sandbox-rs/src/lib.rs @@ -85,7 +85,6 @@ mod windows_impl { use super::acl::revoke_ace; use super::allow::compute_allow_paths; use super::allow::AllowDenyPaths; - use super::cap::cap_sid_file; use super::cap::load_or_create_cap_sids; use super::env::apply_no_network_to_env; use super::env::ensure_non_interactive_pager; @@ -104,7 +103,6 @@ mod windows_impl { use anyhow::Result; use std::collections::HashMap; use std::ffi::c_void; - use std::fs; use std::io; use std::path::Path; use std::path::PathBuf; @@ -130,13 +128,6 @@ mod windows_impl { !policy.has_full_network_access() } - fn ensure_dir(p: &Path) -> Result<()> { - if let Some(d) = p.parent() { - std::fs::create_dir_all(d)?; - } - Ok(()) - } - fn ensure_codex_home_exists(p: &Path) -> Result<()> { std::fs::create_dir_all(p)?; Ok(()) @@ -194,32 +185,28 @@ mod windows_impl { apply_no_network_to_env(&mut env_map)?; } ensure_codex_home_exists(codex_home)?; - let current_dir = cwd.to_path_buf(); - let logs_base_dir = Some(codex_home); + let sandbox_base = codex_home.join(".sandbox"); + std::fs::create_dir_all(&sandbox_base)?; + let logs_base_dir = Some(sandbox_base.as_path()); log_start(&command, logs_base_dir); - let cap_sid_path = cap_sid_file(codex_home); let is_workspace_write = matches!(&policy, SandboxPolicy::WorkspaceWrite { .. }); + if matches!(&policy, SandboxPolicy::DangerFullAccess) { + anyhow::bail!("DangerFullAccess is not supported for sandboxing") + } + let caps = load_or_create_cap_sids(codex_home)?; let (h_token, psid_to_use): (HANDLE, *mut c_void) = unsafe { match &policy { SandboxPolicy::ReadOnly => { - let caps = load_or_create_cap_sids(codex_home); - ensure_dir(&cap_sid_path)?; - fs::write(&cap_sid_path, serde_json::to_string(&caps)?)?; let psid = convert_string_sid_to_sid(&caps.readonly).unwrap(); super::token::create_readonly_token_with_cap(psid)? } SandboxPolicy::WorkspaceWrite { .. } => { - let caps = load_or_create_cap_sids(codex_home); - ensure_dir(&cap_sid_path)?; - fs::write(&cap_sid_path, serde_json::to_string(&caps)?)?; let psid = convert_string_sid_to_sid(&caps.workspace).unwrap(); super::token::create_workspace_write_token_with_cap(psid)? } - SandboxPolicy::DangerFullAccess => { - anyhow::bail!("DangerFullAccess is not supported for sandboxing") - } + SandboxPolicy::DangerFullAccess => unreachable!("DangerFullAccess handled above"), } }; 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 80433ebd5..dccbd9769 100644 --- a/codex-rs/windows-sandbox-rs/src/setup_main_win.rs +++ b/codex-rs/windows-sandbox-rs/src/setup_main_win.rs @@ -20,6 +20,7 @@ use rand::RngCore; use rand::SeedableRng; use serde::Deserialize; use serde::Serialize; +use std::collections::HashSet; use std::ffi::c_void; use std::ffi::OsStr; use std::fs::File; @@ -392,8 +393,8 @@ fn run_netsh_firewall(sid: &str, log: &mut File) -> Result<()> { log_line( log, &format!( - "firewall rule configured via COM with LocalUserAuthorizedList={local_user_spec}" - ), + "firewall rule configured via COM with LocalUserAuthorizedList={local_user_spec}" + ), )?; Ok(()) })() @@ -647,7 +648,7 @@ fn run_setup(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<()> { string_from_sid_bytes(&online_sid).map_err(anyhow::Error::msg)? ), )?; - let caps = load_or_create_cap_sids(&payload.codex_home); + let caps = load_or_create_cap_sids(&payload.codex_home)?; let cap_psid = unsafe { convert_string_sid_to_sid(&caps.workspace) .ok_or_else(|| anyhow::anyhow!("convert capability SID failed"))? @@ -758,7 +759,19 @@ fn run_setup(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<()> { } } + let cap_sid_str = caps.workspace.clone(); + let online_sid_str = string_from_sid_bytes(&online_sid).map_err(anyhow::Error::msg)?; + let sid_strings = vec![offline_sid_str.clone(), online_sid_str, cap_sid_str]; + let write_mask = + FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE | DELETE | FILE_DELETE_CHILD; + let mut grant_tasks: Vec = Vec::new(); + + let mut seen_write_roots: HashSet = HashSet::new(); + for root in &payload.write_roots { + if !seen_write_roots.insert(root.clone()) { + continue; + } if !root.exists() { log_line( log, @@ -766,12 +779,6 @@ fn run_setup(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<()> { )?; continue; } - let sids = vec![offline_psid, online_psid, cap_psid]; - let write_mask = FILE_GENERIC_READ - | FILE_GENERIC_WRITE - | FILE_GENERIC_EXECUTE - | DELETE - | FILE_DELETE_CHILD; let mut need_grant = false; for (label, psid) in [ ("offline", offline_psid), @@ -817,25 +824,7 @@ fn run_setup(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<()> { root.display() ), )?; - match unsafe { ensure_allow_write_aces(root, &sids) } { - Ok(res) => { - log_line( - log, - &format!( - "write ACE {} on {}", - if res { "added" } else { "already present" }, - root.display() - ), - )?; - } - Err(e) => { - refresh_errors.push(format!("write ACE failed on {}: {}", root.display(), e)); - log_line( - log, - &format!("write ACE grant failed on {}: {}", root.display(), e), - )?; - } - } + grant_tasks.push(root.clone()); } else { log_line( log, @@ -847,6 +836,65 @@ fn run_setup(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 sid_strings = sid_strings.clone(); + let tx = tx.clone(); + scope.spawn(move || { + // Convert SID strings to psids locally in this thread. + let mut psids: Vec<*mut c_void> = Vec::new(); + for sid_str in &sid_strings { + if let Some(psid) = unsafe { convert_string_sid_to_sid(sid_str) } { + psids.push(psid); + } else { + let _ = tx.send((root.clone(), Err(anyhow::anyhow!("convert SID failed")))); + return; + } + } + + let res = unsafe { ensure_allow_write_aces(&root, &psids) }; + + for psid in psids { + unsafe { + LocalFree(psid as HLOCAL); + } + } + let _ = tx.send((root, res)); + }); + } + drop(tx); + for (root, res) in rx { + match res { + Ok(added) => { + if log_line( + log, + &format!( + "write ACE {} on {}", + if added { "added" } else { "already present" }, + root.display() + ), + ) + .is_err() + { + // ignore log errors inside scoped thread + } + } + Err(e) => { + refresh_errors.push(format!("write ACE failed on {}: {}", root.display(), e)); + if log_line( + log, + &format!("write ACE grant failed on {}: {}", root.display(), e), + ) + .is_err() + { + // ignore log errors inside scoped thread + } + } + } + } + }); + if refresh_only { log_line( log, diff --git a/codex-rs/windows-sandbox-rs/src/setup_orchestrator.rs b/codex-rs/windows-sandbox-rs/src/setup_orchestrator.rs index 1bd34ebb0..8ac0157a6 100644 --- a/codex-rs/windows-sandbox-rs/src/setup_orchestrator.rs +++ b/codex-rs/windows-sandbox-rs/src/setup_orchestrator.rs @@ -1,6 +1,7 @@ use serde::Deserialize; use serde::Serialize; use std::collections::HashMap; +use std::collections::HashSet; use std::ffi::c_void; use std::os::windows::process::CommandExt; use std::path::Path; @@ -54,13 +55,22 @@ pub fn run_setup_refresh( if matches!(policy, SandboxPolicy::DangerFullAccess) { return Ok(()); } + let (read_roots, write_roots) = build_payload_roots( + policy, + policy_cwd, + command_cwd, + env_map, + codex_home, + None, + None, + ); let payload = ElevationPayload { version: SETUP_VERSION, offline_username: OFFLINE_USERNAME.to_string(), online_username: ONLINE_USERNAME.to_string(), codex_home: codex_home.to_path_buf(), - read_roots: gather_read_roots(command_cwd, policy), - write_roots: gather_write_roots(policy, policy_cwd, command_cwd, env_map), + read_roots, + write_roots, real_user: std::env::var("USERNAME").unwrap_or_else(|_| "Administrators".to_string()), refresh_only: true, }; @@ -219,7 +229,14 @@ pub(crate) fn gather_write_roots( let AllowDenyPaths { allow, .. } = compute_allow_paths(policy, policy_cwd, command_cwd, env_map); roots.extend(allow); - canonical_existing(&roots) + let mut dedup: HashSet = HashSet::new(); + let mut out: Vec = Vec::new(); + for r in canonical_existing(&roots) { + if dedup.insert(r.clone()) { + out.push(r); + } + } + out } #[derive(Serialize)] @@ -355,19 +372,15 @@ pub fn run_elevated_setup( // Ensure the shared sandbox directory exists before we send it to the elevated helper. let sbx_dir = sandbox_dir(codex_home); std::fs::create_dir_all(&sbx_dir)?; - let mut write_roots = if let Some(roots) = write_roots_override { - roots - } else { - gather_write_roots(policy, policy_cwd, command_cwd, env_map) - }; - if !write_roots.contains(&sbx_dir) { - write_roots.push(sbx_dir.clone()); - } - let read_roots = if let Some(roots) = read_roots_override { - roots - } else { - gather_read_roots(command_cwd, policy) - }; + let (read_roots, write_roots) = build_payload_roots( + policy, + policy_cwd, + command_cwd, + env_map, + codex_home, + read_roots_override, + write_roots_override, + ); let payload = ElevationPayload { version: SETUP_VERSION, offline_username: OFFLINE_USERNAME.to_string(), @@ -381,3 +394,31 @@ pub fn run_elevated_setup( let needs_elevation = !is_elevated()?; run_setup_exe(&payload, needs_elevation) } + +fn build_payload_roots( + policy: &SandboxPolicy, + policy_cwd: &Path, + command_cwd: &Path, + env_map: &HashMap, + codex_home: &Path, + read_roots_override: Option>, + write_roots_override: Option>, +) -> (Vec, Vec) { + let sbx_dir = sandbox_dir(codex_home); + let mut write_roots = if let Some(roots) = write_roots_override { + canonical_existing(&roots) + } else { + gather_write_roots(policy, policy_cwd, command_cwd, env_map) + }; + if !write_roots.contains(&sbx_dir) { + write_roots.push(sbx_dir.clone()); + } + let mut read_roots = if let Some(roots) = read_roots_override { + canonical_existing(&roots) + } else { + gather_read_roots(command_cwd, policy) + }; + let write_root_set: HashSet = write_roots.iter().cloned().collect(); + read_roots.retain(|root| !write_root_set.contains(root)); + (read_roots, write_roots) +}