diff --git a/codex-rs/windows-sandbox-rs/src/audit.rs b/codex-rs/windows-sandbox-rs/src/audit.rs index 9ee371ce6846..fd81ef3a3fbf 100644 --- a/codex-rs/windows-sandbox-rs/src/audit.rs +++ b/codex-rs/windows-sandbox-rs/src/audit.rs @@ -2,12 +2,14 @@ use crate::acl::add_deny_write_ace; use crate::acl::path_mask_allows; use crate::cap::cap_sid_file; use crate::cap::load_or_create_cap_sids; +use crate::cap::workspace_write_cap_sid_for_root; +use crate::cap::workspace_write_root_contains_path; use crate::logging::{debug_log, log_note}; use crate::path_normalization::canonical_path_key; use crate::policy::SandboxPolicy; -use crate::token::convert_string_sid_to_sid; +use crate::setup::effective_write_roots_for_setup; +use crate::token::LocalSid; use crate::token::world_sid; -use anyhow::anyhow; use anyhow::Result; use std::collections::HashSet; use std::ffi::c_void; @@ -225,6 +227,7 @@ pub fn apply_world_writable_scan_and_denies( &flagged, sandbox_policy, cwd, + env_map, logs_base_dir, ) { log_note( @@ -240,6 +243,7 @@ pub fn apply_capability_denies_for_world_writable( flagged: &[PathBuf], sandbox_policy: &SandboxPolicy, cwd: &Path, + env_map: &std::collections::HashMap, logs_base_dir: Option<&Path>, ) -> Result<()> { if flagged.is_empty() { @@ -249,22 +253,28 @@ pub fn apply_capability_denies_for_world_writable( let cap_path = cap_sid_file(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, .. } => { - let sid = unsafe { convert_string_sid_to_sid(&caps.workspace) } - .ok_or_else(|| anyhow!("ConvertStringSidToSidW failed for workspace capability"))?; - let mut roots: Vec = - vec![dunce::canonicalize(cwd).unwrap_or_else(|_| cwd.to_path_buf())]; - for root in writable_roots { - let candidate = root.as_path(); - roots.push(dunce::canonicalize(candidate).unwrap_or_else(|_| root.to_path_buf())); - } - (sid, roots) + let (active_sids, workspace_roots): (Vec, Vec) = match sandbox_policy { + SandboxPolicy::WorkspaceWrite { .. } => { + let roots = + effective_write_roots_for_setup( + sandbox_policy, + cwd, + cwd, + env_map, + codex_home, + /*write_roots_override*/ None, + ); + let active_sids = roots + .iter() + .map(|root| { + workspace_write_cap_sid_for_root(codex_home, cwd, root) + .and_then(|sid| LocalSid::from_string(&sid)) + }) + .collect::>>()?; + (active_sids, roots) } SandboxPolicy::ReadOnly { .. } => ( - unsafe { convert_string_sid_to_sid(&caps.readonly) }.ok_or_else(|| { - anyhow!("ConvertStringSidToSidW failed for readonly capability") - })?, + vec![LocalSid::from_string(&caps.readonly)?], Vec::new(), ), SandboxPolicy::DangerFullAccess | SandboxPolicy::ExternalSandbox { .. } => { @@ -272,24 +282,29 @@ pub fn apply_capability_denies_for_world_writable( } }; for path in flagged { - if workspace_roots.iter().any(|root| path.starts_with(root)) { + if workspace_roots + .iter() + .any(|root| workspace_write_root_contains_path(root, path)) + { continue; } - let res = unsafe { add_deny_write_ace(path, active_sid) }; - match res { - Ok(true) => log_note( - &format!("AUDIT: applied capability deny ACE to {}", path.display()), - logs_base_dir, - ), - Ok(false) => {} - Err(err) => log_note( - &format!( - "AUDIT: failed to apply capability deny ACE to {}: {}", - path.display(), - err + for active_sid in &active_sids { + let res = unsafe { add_deny_write_ace(path, active_sid.as_ptr()) }; + match res { + Ok(true) => log_note( + &format!("AUDIT: applied capability deny ACE to {}", path.display()), + logs_base_dir, ), - logs_base_dir, - ), + Ok(false) => {} + Err(err) => log_note( + &format!( + "AUDIT: failed to apply capability deny ACE to {}: {}", + path.display(), + err + ), + logs_base_dir, + ), + } } } Ok(()) diff --git a/codex-rs/windows-sandbox-rs/src/cap.rs b/codex-rs/windows-sandbox-rs/src/cap.rs index 79f3b2f266ef..263b45745a84 100644 --- a/codex-rs/windows-sandbox-rs/src/cap.rs +++ b/codex-rs/windows-sandbox-rs/src/cap.rs @@ -10,6 +10,7 @@ use std::fs; use std::path::Path; use std::path::PathBuf; use crate::path_normalization::canonical_path_key; +use crate::path_normalization::canonicalize_path; #[derive(Serialize, Deserialize, Clone, Debug)] pub struct CapSids { @@ -22,6 +23,13 @@ pub struct CapSids { /// without permanently affecting other workspaces. #[serde(default)] pub workspace_by_cwd: HashMap, + /// Per-write-root capability SIDs keyed by canonicalized write-root path. + /// + /// These are included in a workspace-write token only when the root is + /// currently allowed, so stale ACLs from earlier extra roots do not expand + /// later workspace sandboxes. + #[serde(default)] + pub writable_root_by_path: HashMap, } pub fn cap_sid_file(codex_home: &Path) -> PathBuf { @@ -61,6 +69,7 @@ pub fn load_or_create_cap_sids(codex_home: &Path) -> Result { workspace: t.to_string(), readonly: make_random_cap_sid_string(), workspace_by_cwd: HashMap::new(), + writable_root_by_path: HashMap::new(), }; persist_caps(&path, &caps)?; return Ok(caps); @@ -70,6 +79,7 @@ pub fn load_or_create_cap_sids(codex_home: &Path) -> Result { workspace: make_random_cap_sid_string(), readonly: make_random_cap_sid_string(), workspace_by_cwd: HashMap::new(), + writable_root_by_path: HashMap::new(), }; persist_caps(&path, &caps)?; Ok(caps) @@ -89,10 +99,46 @@ pub fn workspace_cap_sid_for_cwd(codex_home: &Path, cwd: &Path) -> Result Result { + let path = cap_sid_file(codex_home); + let mut caps = load_or_create_cap_sids(codex_home)?; + let key = canonical_path_key(root); + if let Some(sid) = caps.writable_root_by_path.get(&key) { + return Ok(sid.clone()); + } + let sid = make_random_cap_sid_string(); + caps.writable_root_by_path.insert(key, sid.clone()); + persist_caps(&path, &caps)?; + Ok(sid) +} + +pub fn workspace_write_cap_sid_for_root( + codex_home: &Path, + cwd: &Path, + root: &Path, +) -> Result { + if canonical_path_key(root) == canonical_path_key(cwd) { + workspace_cap_sid_for_cwd(codex_home, cwd) + } else { + writable_root_cap_sid_for_path(codex_home, root) + } +} + +pub fn workspace_write_root_contains_path(root: &Path, path: &Path) -> bool { + canonicalize_path(path).starts_with(canonicalize_path(root)) +} + +pub fn workspace_write_root_specificity(root: &Path) -> usize { + canonicalize_path(root).components().count() +} + #[cfg(test)] mod tests { use super::load_or_create_cap_sids; + use super::writable_root_cap_sid_for_path; use super::workspace_cap_sid_for_cwd; + use super::workspace_write_cap_sid_for_root; use pretty_assertions::assert_eq; use std::path::PathBuf; @@ -118,4 +164,32 @@ mod tests { let caps = load_or_create_cap_sids(&codex_home).expect("load caps"); assert_eq!(caps.workspace_by_cwd.len(), 1); } + + #[test] + fn write_roots_get_path_scoped_sids() { + let temp = tempfile::tempdir().expect("tempdir"); + let codex_home = temp.path().join("codex-home"); + std::fs::create_dir_all(&codex_home).expect("create codex home"); + + let workspace = temp.path().join("workspace"); + let extra_root = temp.path().join("extra-root"); + std::fs::create_dir_all(&workspace).expect("create workspace"); + std::fs::create_dir_all(&extra_root).expect("create extra root"); + + let workspace_sid = + workspace_write_cap_sid_for_root(&codex_home, &workspace, &workspace) + .expect("workspace sid"); + let extra_sid = workspace_write_cap_sid_for_root(&codex_home, &workspace, &extra_root) + .expect("extra root sid"); + + assert_ne!(workspace_sid, extra_sid); + assert_eq!( + extra_sid, + writable_root_cap_sid_for_path(&codex_home, &extra_root).expect("extra root sid again") + ); + + let caps = load_or_create_cap_sids(&codex_home).expect("load caps"); + assert_eq!(caps.workspace_by_cwd.len(), 1); + assert_eq!(caps.writable_root_by_path.len(), 1); + } } diff --git a/codex-rs/windows-sandbox-rs/src/elevated_impl.rs b/codex-rs/windows-sandbox-rs/src/elevated_impl.rs index be9f1cfb9894..c138890c0ca6 100644 --- a/codex-rs/windows-sandbox-rs/src/elevated_impl.rs +++ b/codex-rs/windows-sandbox-rs/src/elevated_impl.rs @@ -22,6 +22,7 @@ mod windows_impl { use super::ElevatedSandboxCaptureRequest; use crate::acl::allow_null_device; use crate::cap::load_or_create_cap_sids; + use crate::cap::workspace_write_cap_sid_for_root; use crate::env::ensure_non_interactive_pager; use crate::env::inherit_path_env; use crate::env::normalize_null_device_env; @@ -39,7 +40,8 @@ mod windows_impl { use crate::runner_client::spawn_runner_transport; use crate::sandbox_utils::ensure_codex_home_exists; use crate::sandbox_utils::inject_git_safe_directory; - use crate::token::convert_string_sid_to_sid; + use crate::setup::effective_write_roots_for_setup; + use crate::token::LocalSid; use anyhow::Result; use std::path::Path; @@ -96,22 +98,28 @@ mod windows_impl { anyhow::bail!("DangerFullAccess and ExternalSandbox are not supported for sandboxing") } let caps = load_or_create_cap_sids(codex_home)?; - let (psid_to_use, cap_sids) = match &policy { + let (sid_for_null, cap_sids) = match &policy { SandboxPolicy::ReadOnly { .. } => { - #[allow(clippy::unwrap_used)] - let psid = unsafe { convert_string_sid_to_sid(&caps.readonly).unwrap() }; - (psid, vec![caps.readonly]) + let sid = LocalSid::from_string(&caps.readonly)?; + (sid, vec![caps.readonly]) } SandboxPolicy::WorkspaceWrite { .. } => { - #[allow(clippy::unwrap_used)] - let psid = unsafe { convert_string_sid_to_sid(&caps.workspace).unwrap() }; - ( - psid, - vec![ - caps.workspace, - crate::cap::workspace_cap_sid_for_cwd(codex_home, cwd)?, - ], - ) + let write_roots = effective_write_roots_for_setup( + &policy, + sandbox_policy_cwd, + cwd, + &env_map, + codex_home, + write_roots_override, + ); + let cap_sids = write_roots + .iter() + .map(|root| workspace_write_cap_sid_for_root(codex_home, cwd, root)) + .collect::>>()?; + if cap_sids.is_empty() { + anyhow::bail!("workspace-write sandbox has no writable root capability SIDs"); + } + (LocalSid::from_string(&cap_sids[0])?, cap_sids) } SandboxPolicy::DangerFullAccess | SandboxPolicy::ExternalSandbox { .. } => { unreachable!("DangerFullAccess handled above") @@ -119,7 +127,7 @@ mod windows_impl { }; unsafe { - allow_null_device(psid_to_use); + allow_null_device(sid_for_null.as_ptr()); } (|| -> Result { diff --git a/codex-rs/windows-sandbox-rs/src/lib.rs b/codex-rs/windows-sandbox-rs/src/lib.rs index 522f8926d595..0ca6dc3291b8 100644 --- a/codex-rs/windows-sandbox-rs/src/lib.rs +++ b/codex-rs/windows-sandbox-rs/src/lib.rs @@ -97,6 +97,10 @@ pub use cap::load_or_create_cap_sids; #[cfg(target_os = "windows")] pub use cap::workspace_cap_sid_for_cwd; #[cfg(target_os = "windows")] +pub use cap::workspace_write_cap_sid_for_root; +#[cfg(target_os = "windows")] +pub use cap::workspace_write_root_contains_path; +#[cfg(target_os = "windows")] pub use conpty::ConptyInstance; #[cfg(target_os = "windows")] pub use conpty::spawn_conpty_process_as_user; @@ -206,7 +210,7 @@ pub use setup_error::setup_error_path; pub use setup_error::write_setup_error_report; #[cfg(target_os = "windows")] #[doc(hidden)] -pub use spawn_prep::LocalSid; +pub use token::LocalSid; #[cfg(target_os = "windows")] pub use token::convert_string_sid_to_sid; #[cfg(target_os = "windows")] @@ -253,27 +257,21 @@ pub use stub::run_windows_sandbox_legacy_preflight; #[cfg(target_os = "windows")] mod windows_impl { - use super::acl::add_allow_ace; - use super::acl::add_deny_write_ace; - use super::acl::allow_null_device; use super::acl::revoke_ace; - use super::allow::AllowDenyPaths; use super::allow::compute_allow_paths; - use super::cap::load_or_create_cap_sids; - use super::cap::workspace_cap_sid_for_cwd; use super::logging::log_failure; use super::logging::log_success; - use super::path_normalization::canonicalize_path; use super::policy::SandboxPolicy; use super::process::create_process_as_user; use super::sandbox_utils::ensure_codex_home_exists; + use super::spawn_prep::allow_null_device_for_workspace_write; + use super::spawn_prep::apply_legacy_session_acl_rules; + use super::spawn_prep::prepare_legacy_session_security; use super::spawn_prep::prepare_legacy_spawn_context; - use super::token::convert_string_sid_to_sid; - use super::token::create_workspace_write_token_with_caps_from; - use super::workspace_acl::is_command_cwd_root; + use super::spawn_prep::root_capability_sids; + use super::token::LocalSid; use anyhow::Result; use std::collections::HashMap; - use std::ffi::c_void; use std::io; use std::path::Path; use std::path::PathBuf; @@ -379,99 +377,26 @@ mod windows_impl { "Restricted read-only access requires the elevated Windows sandbox backend" ); } - let caps = load_or_create_cap_sids(codex_home)?; - let (h_token, psid_generic, psid_workspace): (HANDLE, *mut c_void, Option<*mut c_void>) = unsafe { - match &policy { - SandboxPolicy::ReadOnly { .. } => { - #[allow(clippy::expect_used)] - let psid = - convert_string_sid_to_sid(&caps.readonly).expect("valid readonly SID"); - let (h, _) = super::token::create_readonly_token_with_cap(psid)?; - (h, psid, None) - } - SandboxPolicy::WorkspaceWrite { .. } => { - #[allow(clippy::expect_used)] - let psid_generic = - convert_string_sid_to_sid(&caps.workspace).expect("valid workspace SID"); - let ws_sid = workspace_cap_sid_for_cwd(codex_home, cwd)?; - #[allow(clippy::expect_used)] - let psid_workspace = - convert_string_sid_to_sid(&ws_sid).expect("valid workspace SID"); - let base = super::token::get_current_token_for_restriction()?; - let h_res = create_workspace_write_token_with_caps_from( - base, - &[psid_generic, psid_workspace], - ); - windows_sys::Win32::Foundation::CloseHandle(base); - let h = h_res?; - (h, psid_generic, Some(psid_workspace)) - } - SandboxPolicy::DangerFullAccess | SandboxPolicy::ExternalSandbox { .. } => { - unreachable!("DangerFullAccess handled above") - } - } - }; - - unsafe { - if is_workspace_write - && let Ok(base) = super::token::get_current_token_for_restriction() - { - if let Ok(bytes) = super::token::get_logon_sid_bytes(base) { - let mut tmp = bytes; - let psid2 = tmp.as_mut_ptr() as *mut c_void; - allow_null_device(psid2); - } - windows_sys::Win32::Foundation::CloseHandle(base); - } - } - + let allow_paths = + compute_allow_paths(&policy, sandbox_policy_cwd, ¤t_dir, &env_map).allow; + let security = prepare_legacy_session_security(&policy, codex_home, cwd, allow_paths)?; + allow_null_device_for_workspace_write(is_workspace_write); let persist_aces = is_workspace_write; - let AllowDenyPaths { allow, mut deny } = - compute_allow_paths(&policy, sandbox_policy_cwd, ¤t_dir, &env_map); - for path in additional_deny_write_paths { - if path.exists() { - deny.insert(path.clone()); - } - } - let canonical_cwd = canonicalize_path(¤t_dir); - let mut guards: Vec<(PathBuf, *mut c_void)> = Vec::new(); - unsafe { - for p in &allow { - let psid = if is_workspace_write && is_command_cwd_root(p, &canonical_cwd) { - psid_workspace.unwrap_or(psid_generic) - } else { - psid_generic - }; - if let Ok(added) = add_allow_ace(p, psid) - && added - { - if persist_aces { - if p.is_dir() { - // best-effort seeding omitted intentionally - } - } else { - guards.push((p.clone(), psid)); - } - } - } - for p in &deny { - if let Ok(added) = add_deny_write_ace(p, psid_generic) - && added - && !persist_aces - { - guards.push((p.clone(), psid_generic)); - } - } - allow_null_device(psid_generic); - if let Some(psid) = psid_workspace { - allow_null_device(psid); - } - } + let guards = apply_legacy_session_acl_rules( + &policy, + sandbox_policy_cwd, + ¤t_dir, + &env_map, + additional_deny_write_paths, + security.readonly_sid.as_ref(), + &security.write_root_sids, + persist_aces, + ); let (stdin_pair, stdout_pair, stderr_pair) = unsafe { setup_stdio_pipes()? }; let ((in_r, in_w), (out_r, out_w), (err_r, err_w)) = (stdin_pair, stdout_pair, stderr_pair); let spawn_res = unsafe { create_process_as_user( - h_token, + security.h_token, &command, cwd, &env_map, @@ -490,7 +415,14 @@ mod windows_impl { CloseHandle(out_w); CloseHandle(err_r); CloseHandle(err_w); - CloseHandle(h_token); + if !persist_aces { + for (p, sid_str) in &guards { + if let Ok(sid) = LocalSid::from_string(sid_str) { + revoke_ace(p, sid.as_ptr()); + } + } + } + CloseHandle(security.h_token); } return Err(err); } @@ -572,7 +504,7 @@ mod windows_impl { if pi.hProcess != 0 { CloseHandle(pi.hProcess); } - CloseHandle(h_token); + CloseHandle(security.h_token); } let _ = t_out.join(); let _ = t_err.join(); @@ -592,8 +524,10 @@ mod windows_impl { if !persist_aces { unsafe { - for (p, sid) in guards { - revoke_ace(&p, sid); + for (p, sid_str) in guards { + if let Ok(sid) = LocalSid::from_string(&sid_str) { + revoke_ace(&p, sid.as_ptr()); + } } } } @@ -618,33 +552,20 @@ mod windows_impl { } ensure_codex_home_exists(codex_home)?; - let caps = load_or_create_cap_sids(codex_home)?; - #[allow(clippy::expect_used)] - let psid_generic = - unsafe { convert_string_sid_to_sid(&caps.workspace) }.expect("valid workspace SID"); - let ws_sid = workspace_cap_sid_for_cwd(codex_home, cwd)?; - #[allow(clippy::expect_used)] - let psid_workspace = - unsafe { convert_string_sid_to_sid(&ws_sid) }.expect("valid workspace SID"); let current_dir = cwd.to_path_buf(); - let AllowDenyPaths { allow, deny } = + let allow_paths = compute_allow_paths(sandbox_policy, sandbox_policy_cwd, ¤t_dir, env_map); - let canonical_cwd = canonicalize_path(¤t_dir); - unsafe { - for p in &allow { - let psid = if is_command_cwd_root(p, &canonical_cwd) { - psid_workspace - } else { - psid_generic - }; - let _ = add_allow_ace(p, psid); - } - for p in &deny { - let _ = add_deny_write_ace(p, psid_generic); - } - allow_null_device(psid_generic); - allow_null_device(psid_workspace); - } + let write_root_sids = root_capability_sids(codex_home, cwd, allow_paths.allow)?; + let _guards = apply_legacy_session_acl_rules( + sandbox_policy, + sandbox_policy_cwd, + ¤t_dir, + env_map, + &[], + /*readonly_sid*/ None, + &write_root_sids, + /*persist_aces*/ true, + ); Ok(()) } 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 ca3fc1e4444d..23c66b6569b2 100644 --- a/codex-rs/windows-sandbox-rs/src/setup_main_win.rs +++ b/codex-rs/windows-sandbox-rs/src/setup_main_win.rs @@ -21,7 +21,6 @@ use codex_windows_sandbox::extract_setup_failure; use codex_windows_sandbox::hide_newly_created_users; use codex_windows_sandbox::install_wfp_filters; use codex_windows_sandbox::is_command_cwd_root; -use codex_windows_sandbox::load_or_create_cap_sids; use codex_windows_sandbox::log_note; use codex_windows_sandbox::path_mask_allows; use codex_windows_sandbox::sandbox_bin_dir; @@ -29,7 +28,8 @@ use codex_windows_sandbox::sandbox_dir; use codex_windows_sandbox::sandbox_secrets_dir; use codex_windows_sandbox::string_from_sid_bytes; use codex_windows_sandbox::to_wide; -use codex_windows_sandbox::workspace_cap_sid_for_cwd; +use codex_windows_sandbox::workspace_write_cap_sid_for_root; +use codex_windows_sandbox::workspace_write_root_contains_path; use codex_windows_sandbox::write_setup_error_report; use serde::Deserialize; use serde::Serialize; @@ -118,6 +118,42 @@ fn log_line(log: &mut File, msg: &str) -> Result<()> { Ok(()) } +fn workspace_write_cap_sids_for_path( + codex_home: &Path, + command_cwd: &Path, + write_roots: &[PathBuf], + path: &Path, +) -> Result> { + let mut sid_strs = Vec::new(); + for root in write_roots { + if workspace_write_root_contains_path(root, path) { + sid_strs.push(workspace_write_cap_sid_for_root( + codex_home, + command_cwd, + root, + )?); + } + } + if sid_strs.is_empty() { + if write_roots.is_empty() { + sid_strs.push(workspace_write_cap_sid_for_root( + codex_home, + command_cwd, + command_cwd, + )?); + } else { + for root in write_roots { + sid_strs.push(workspace_write_cap_sid_for_root( + codex_home, + command_cwd, + root, + )?); + } + } + } + Ok(sid_strs) +} + fn spawn_read_acl_helper(payload: &Payload, _log: &mut File) -> Result<()> { let mut read_payload = payload.clone(); read_payload.mode = SetupMode::ReadAclsOnly; @@ -559,25 +595,6 @@ fn run_setup_full(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<( )) })?; - let caps = load_or_create_cap_sids(&payload.codex_home).map_err(|err| { - anyhow::Error::new(SetupFailure::new( - SetupErrorCode::HelperCapabilitySidFailed, - format!("load or create capability SIDs failed: {err}"), - )) - })?; - let cap_psid = unsafe { - convert_string_sid_to_sid(&caps.workspace).ok_or_else(|| { - anyhow::Error::new(SetupFailure::new( - SetupErrorCode::HelperCapabilitySidFailed, - format!("convert capability SID {} failed", caps.workspace), - )) - })? - }; - let workspace_sid_str = workspace_cap_sid_for_cwd(&payload.codex_home, &payload.command_cwd)?; - let workspace_psid = unsafe { - convert_string_sid_to_sid(&workspace_sid_str) - .ok_or_else(|| anyhow::anyhow!("convert workspace capability SID failed"))? - }; let mut refresh_errors: Vec = Vec::new(); if !refresh_only { let proxy_allowlist_result = firewall::ensure_offline_proxy_allowlist( @@ -647,12 +664,11 @@ fn run_setup_full(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<( } } - let cap_sid_str = caps.workspace; let sandbox_group_sid_str = string_from_sid_bytes(&sandbox_group_sid).map_err(anyhow::Error::msg)?; 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 grant_tasks: Vec<(PathBuf, String)> = Vec::new(); let mut seen_deny_paths: HashSet = HashSet::new(); let mut seen_write_roots: HashSet = HashSet::new(); @@ -674,16 +690,17 @@ fn run_setup_full(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<( let cap_label = if is_command_cwd { "workspace_cap" } else { - "cap" + "root_cap" }; - let cap_psid_for_root = if is_command_cwd { - workspace_psid - } else { - cap_psid + let root_cap_sid_str = + workspace_write_cap_sid_for_root(&payload.codex_home, &payload.command_cwd, root)?; + let root_cap_psid = unsafe { + convert_string_sid_to_sid(&root_cap_sid_str) + .ok_or_else(|| anyhow::anyhow!("convert write root capability SID failed"))? }; for (label, psid) in [ ("sandbox_group", sandbox_group_psid), - (cap_label, cap_psid_for_root), + (cap_label, root_cap_psid), ] { let has = match path_mask_allows(root, &[psid], write_mask, /*require_all_bits*/ true) { @@ -709,6 +726,9 @@ fn run_setup_full(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<( need_grant = true; } } + unsafe { + LocalFree(root_cap_psid as HLOCAL); + } if need_grant { log_line( log, @@ -717,19 +737,14 @@ fn run_setup_full(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<( root.display() ), )?; - grant_tasks.push(root.clone()); + grant_tasks.push((root.clone(), root_cap_sid_str)); } } let (tx, rx) = mpsc::channel::<(PathBuf, Result)>(); std::thread::scope(|scope| { - for root in grant_tasks { - 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()] - } else { - vec![sandbox_group_sid_str.clone(), cap_sid_str.clone()] - }; + for (root, root_cap_sid_str) in grant_tasks { + let sid_strings = vec![sandbox_group_sid_str.clone(), root_cap_sid_str]; let tx = tx.clone(); scope.spawn(move || { // Convert SID strings to psids locally in this thread. @@ -791,27 +806,36 @@ fn run_setup_full(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<( .with_context(|| format!("failed to create deny-write path {}", path.display()))?; } - let canonical_path = canonicalize_path(path); - let deny_psid = if canonical_path.starts_with(&canonical_command_cwd) { - workspace_psid - } else { - cap_psid - }; + let deny_sid_strs = workspace_write_cap_sids_for_path( + &payload.codex_home, + &payload.command_cwd, + &payload.write_roots, + path, + )?; + for deny_sid_str in deny_sid_strs { + let deny_psid = unsafe { + convert_string_sid_to_sid(&deny_sid_str) + .ok_or_else(|| anyhow::anyhow!("convert deny capability SID failed"))? + }; - match unsafe { add_deny_write_ace(path, deny_psid) } { - Ok(true) => { - log_line( - log, - &format!("applied deny ACE to protect {}", path.display()), - )?; + match unsafe { add_deny_write_ace(path, deny_psid) } { + Ok(true) => { + log_line( + log, + &format!("applied deny ACE to protect {}", path.display()), + )?; + } + Ok(false) => {} + Err(err) => { + refresh_errors.push(format!("deny ACE failed on {}: {err}", path.display())); + log_line( + log, + &format!("deny ACE failed on {}: {err}", path.display()), + )?; + } } - Ok(false) => {} - Err(err) => { - refresh_errors.push(format!("deny ACE failed on {}: {err}", path.display())); - log_line( - log, - &format!("deny ACE failed on {}: {err}", path.display()), - )?; + unsafe { + LocalFree(deny_psid as HLOCAL); } } } @@ -892,12 +916,6 @@ fn run_setup_full(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<( if !sandbox_group_psid.is_null() { LocalFree(sandbox_group_psid as HLOCAL); } - if !cap_psid.is_null() { - LocalFree(cap_psid as HLOCAL); - } - if !workspace_psid.is_null() { - LocalFree(workspace_psid as HLOCAL); - } } if refresh_only && !refresh_errors.is_empty() { log_line( @@ -914,9 +932,13 @@ fn run_setup_full(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<( mod tests { use super::Payload; use super::SETUP_VERSION; + use super::workspace_write_cap_sids_for_path; use codex_otel::StatsigMetricsSettings; + 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; fn payload_json() -> serde_json::Value { json!({ @@ -954,4 +976,77 @@ mod tests { }) ); } + + #[test] + fn deny_path_under_active_root_uses_only_matching_root_sid() { + let temp = tempfile::tempdir().expect("tempdir"); + let codex_home = temp.path().join("codex-home"); + let workspace = temp.path().join("workspace"); + let active_root = temp.path().join("active-root"); + let stale_root = temp.path().join("stale-root"); + let deny_path = active_root.join("protected"); + fs::create_dir_all(&codex_home).expect("create codex home"); + fs::create_dir_all(&workspace).expect("create workspace"); + fs::create_dir_all(&active_root).expect("create active root"); + fs::create_dir_all(&stale_root).expect("create stale root"); + fs::create_dir_all(&deny_path).expect("create deny path"); + + let stale_sid = workspace_write_cap_sid_for_root(&codex_home, &workspace, &stale_root) + .expect("stale sid"); + let active_sid = workspace_write_cap_sid_for_root(&codex_home, &workspace, &active_root) + .expect("active sid"); + let workspace_sid = workspace_write_cap_sid_for_root(&codex_home, &workspace, &workspace) + .expect("workspace sid"); + let caps = load_or_create_cap_sids(&codex_home).expect("load caps"); + + let deny_sids = workspace_write_cap_sids_for_path( + &codex_home, + &workspace, + &[workspace.clone(), active_root.clone()], + &deny_path, + ) + .expect("deny sids"); + + assert_eq!(deny_sids, vec![active_sid]); + assert!(!deny_sids.contains(&workspace_sid)); + assert!(!deny_sids.contains(&stale_sid)); + assert!(!deny_sids.contains(&caps.workspace)); + } + + #[test] + fn deny_path_outside_active_roots_falls_back_to_all_active_root_sids() { + let temp = tempfile::tempdir().expect("tempdir"); + let codex_home = temp.path().join("codex-home"); + let workspace = temp.path().join("workspace"); + let active_root = temp.path().join("active-root"); + let stale_root = temp.path().join("stale-root"); + let deny_path = temp.path().join("outside-deny"); + fs::create_dir_all(&codex_home).expect("create codex home"); + fs::create_dir_all(&workspace).expect("create workspace"); + fs::create_dir_all(&active_root).expect("create active root"); + fs::create_dir_all(&stale_root).expect("create stale root"); + fs::create_dir_all(&deny_path).expect("create deny path"); + + let stale_sid = workspace_write_cap_sid_for_root(&codex_home, &workspace, &stale_root) + .expect("stale sid"); + let active_sid = workspace_write_cap_sid_for_root(&codex_home, &workspace, &active_root) + .expect("active sid"); + let workspace_sid = workspace_write_cap_sid_for_root(&codex_home, &workspace, &workspace) + .expect("workspace sid"); + let caps = load_or_create_cap_sids(&codex_home).expect("load caps"); + + let deny_sids = workspace_write_cap_sids_for_path( + &codex_home, + &workspace, + &[workspace.clone(), active_root.clone()], + &deny_path, + ) + .expect("deny sids"); + + assert_eq!(deny_sids.len(), 2); + assert!(deny_sids.contains(&workspace_sid)); + assert!(deny_sids.contains(&active_sid)); + assert!(!deny_sids.contains(&stale_sid)); + assert!(!deny_sids.contains(&caps.workspace)); + } } diff --git a/codex-rs/windows-sandbox-rs/src/setup_orchestrator.rs b/codex-rs/windows-sandbox-rs/src/setup_orchestrator.rs index ef952a4e033a..c5498f6211d6 100644 --- a/codex-rs/windows-sandbox-rs/src/setup_orchestrator.rs +++ b/codex-rs/windows-sandbox-rs/src/setup_orchestrator.rs @@ -408,6 +408,26 @@ pub(crate) fn gather_write_roots( out } +pub(crate) fn effective_write_roots_for_setup( + policy: &SandboxPolicy, + policy_cwd: &Path, + command_cwd: &Path, + env_map: &HashMap, + codex_home: &Path, + write_roots_override: Option<&[PathBuf]>, +) -> Vec { + let write_roots = if let Some(roots) = write_roots_override { + canonical_existing(roots) + } else { + gather_write_roots(policy, policy_cwd, command_cwd, env_map) + }; + let write_roots = expand_user_profile_root(write_roots); + let write_roots = filter_user_profile_root(write_roots); + let write_roots = filter_user_profile_root_exclusions(write_roots); + let write_roots = filter_ssh_config_dependency_roots(write_roots); + filter_sensitive_write_roots(write_roots, codex_home) +} + #[derive(Serialize)] struct ElevationPayload { version: u32, @@ -752,21 +772,14 @@ fn build_payload_roots( request: &SandboxSetupRequest<'_>, overrides: &SetupRootOverrides, ) -> (Vec, Vec) { - let write_roots = if let Some(roots) = overrides.write_roots.as_deref() { - canonical_existing(roots) - } else { - gather_write_roots( - request.policy, - request.policy_cwd, - request.command_cwd, - request.env_map, - ) - }; - let write_roots = expand_user_profile_root(write_roots); - let write_roots = filter_user_profile_root(write_roots); - let write_roots = filter_user_profile_root_exclusions(write_roots); - let write_roots = filter_ssh_config_dependency_roots(write_roots); - let write_roots = filter_sensitive_write_roots(write_roots, request.codex_home); + let write_roots = effective_write_roots_for_setup( + request.policy, + request.policy_cwd, + request.command_cwd, + request.env_map, + request.codex_home, + overrides.write_roots.as_deref(), + ); let mut read_roots = if let Some(roots) = overrides.read_roots.as_deref() { // An explicit override is the split policy's complete readable set. Keep only the // helper/platform roots the elevated setup needs; do not re-add legacy cwd/full-read roots. @@ -1394,6 +1407,65 @@ mod tests { ); } + #[test] + fn effective_write_roots_match_payload_filtering_for_overrides() { + let tmp = TempDir::new().expect("tempdir"); + let codex_home = tmp.path().join("codex-home"); + let command_cwd = tmp.path().join("workspace"); + let extra_root = tmp.path().join("extra-root"); + let sandbox_root = super::sandbox_dir(&codex_home); + fs::create_dir_all(&codex_home).expect("create codex home"); + fs::create_dir_all(&command_cwd).expect("create workspace"); + fs::create_dir_all(&extra_root).expect("create extra root"); + fs::create_dir_all(&sandbox_root).expect("create sandbox root"); + let policy = SandboxPolicy::WorkspaceWrite { + writable_roots: vec![], + network_access: false, + exclude_tmpdir_env_var: true, + exclude_slash_tmp: true, + }; + let override_roots = vec![ + command_cwd.clone(), + extra_root.clone(), + codex_home.clone(), + sandbox_root.clone(), + ]; + let request = super::SandboxSetupRequest { + policy: &policy, + policy_cwd: &command_cwd, + command_cwd: &command_cwd, + env_map: &HashMap::new(), + codex_home: &codex_home, + proxy_enforced: false, + }; + let overrides = super::SetupRootOverrides { + read_roots: None, + read_roots_include_platform_defaults: false, + write_roots: Some(override_roots.clone()), + deny_write_paths: None, + }; + + let effective_write_roots = super::effective_write_roots_for_setup( + &policy, + &command_cwd, + &command_cwd, + &HashMap::new(), + &codex_home, + Some(&override_roots), + ); + let (_read_roots, payload_write_roots) = build_payload_roots(&request, &overrides); + + let expected_workspace = dunce::canonicalize(&command_cwd).expect("canonical workspace"); + let expected_extra = dunce::canonicalize(&extra_root).expect("canonical extra root"); + let forbidden_codex_home = dunce::canonicalize(&codex_home).expect("canonical codex home"); + let forbidden_sandbox = dunce::canonicalize(&sandbox_root).expect("canonical sandbox root"); + assert_eq!(effective_write_roots, payload_write_roots); + assert!(effective_write_roots.contains(&expected_workspace)); + assert!(effective_write_roots.contains(&expected_extra)); + assert!(!effective_write_roots.contains(&forbidden_codex_home)); + assert!(!effective_write_roots.contains(&forbidden_sandbox)); + } + #[test] fn payload_deny_write_paths_merge_explicit_and_protected_children() { let tmp = TempDir::new().expect("tempdir"); diff --git a/codex-rs/windows-sandbox-rs/src/spawn_prep.rs b/codex-rs/windows-sandbox-rs/src/spawn_prep.rs index 1b3704d6a262..a11f1a685c48 100644 --- a/codex-rs/windows-sandbox-rs/src/spawn_prep.rs +++ b/codex-rs/windows-sandbox-rs/src/spawn_prep.rs @@ -4,7 +4,9 @@ use crate::acl::allow_null_device; use crate::allow::AllowDenyPaths; use crate::allow::compute_allow_paths; use crate::cap::load_or_create_cap_sids; -use crate::cap::workspace_cap_sid_for_cwd; +use crate::cap::workspace_write_cap_sid_for_root; +use crate::cap::workspace_write_root_contains_path; +use crate::cap::workspace_write_root_specificity; use crate::env::apply_no_network_to_env; use crate::env::ensure_non_interactive_pager; use crate::env::inherit_path_env; @@ -17,7 +19,8 @@ use crate::policy::SandboxPolicy; use crate::policy::parse_policy; use crate::sandbox_utils::ensure_codex_home_exists; use crate::sandbox_utils::inject_git_safe_directory; -use crate::token::convert_string_sid_to_sid; +use crate::setup::effective_write_roots_for_setup; +use crate::token::LocalSid; use crate::token::create_readonly_token_with_cap; use crate::token::create_workspace_write_token_with_caps_from; use crate::token::get_current_token_for_restriction; @@ -32,8 +35,6 @@ use std::path::Path; use std::path::PathBuf; use windows_sys::Win32::Foundation::CloseHandle; use windows_sys::Win32::Foundation::HANDLE; -use windows_sys::Win32::Foundation::HLOCAL; -use windows_sys::Win32::Foundation::LocalFree; pub(crate) struct SpawnContext { pub(crate) policy: SandboxPolicy, @@ -51,36 +52,14 @@ pub(crate) struct ElevatedSpawnContext { pub(crate) struct LegacySessionSecurity { pub(crate) h_token: HANDLE, - pub(crate) psid_generic: LocalSid, - pub(crate) psid_workspace: Option, - pub(crate) cap_sid_str: String, + pub(crate) readonly_sid: Option, + pub(crate) write_root_sids: Vec, } -/// Owns a SID allocated by `ConvertStringSidToSidW` and releases it with `LocalFree`. -pub struct LocalSid { - psid: *mut c_void, -} - -impl LocalSid { - pub fn from_string(sid: &str) -> Result { - let psid = unsafe { convert_string_sid_to_sid(sid) } - .ok_or_else(|| anyhow::anyhow!("invalid SID string: {sid}"))?; - Ok(Self { psid }) - } - - pub fn as_ptr(&self) -> *mut c_void { - self.psid - } -} - -impl Drop for LocalSid { - fn drop(&mut self) { - if !self.psid.is_null() { - unsafe { - LocalFree(self.psid as HLOCAL); - } - } - } +pub(crate) struct RootCapabilitySid { + pub(crate) root: PathBuf, + pub(crate) sid: LocalSid, + pub(crate) sid_str: String, } pub(crate) fn should_apply_network_block(policy: &SandboxPolicy) -> bool { @@ -158,27 +137,31 @@ pub(crate) fn prepare_legacy_session_security( policy: &SandboxPolicy, codex_home: &Path, cwd: &Path, + allow_paths: impl IntoIterator, ) -> Result { let caps = load_or_create_cap_sids(codex_home)?; - let (h_token, psid_generic, psid_workspace, cap_sid_str) = unsafe { + let (h_token, readonly_sid, write_root_sids) = unsafe { match policy { SandboxPolicy::ReadOnly { .. } => { let psid = LocalSid::from_string(&caps.readonly)?; let (h_token, _psid) = create_readonly_token_with_cap(psid.as_ptr())?; - (h_token, psid, None, caps.readonly) + (h_token, Some(psid), Vec::new()) } SandboxPolicy::WorkspaceWrite { .. } => { - let psid_generic = LocalSid::from_string(&caps.workspace)?; - let workspace_sid = workspace_cap_sid_for_cwd(codex_home, cwd)?; - let psid_workspace = LocalSid::from_string(&workspace_sid)?; + let write_root_sids = root_capability_sids(codex_home, cwd, allow_paths)?; + if write_root_sids.is_empty() { + anyhow::bail!("workspace-write sandbox has no writable root capability SIDs"); + } let base = get_current_token_for_restriction()?; - let h_token = create_workspace_write_token_with_caps_from( - base, - &[psid_generic.as_ptr(), psid_workspace.as_ptr()], - ); + let cap_ptrs: Vec<*mut c_void> = write_root_sids + .iter() + .map(|root| root.sid.as_ptr()) + .collect(); + let h_token = + create_workspace_write_token_with_caps_from(base, cap_ptrs.as_slice()); CloseHandle(base); let h_token = h_token?; - (h_token, psid_generic, Some(psid_workspace), caps.workspace) + (h_token, None, write_root_sids) } SandboxPolicy::DangerFullAccess | SandboxPolicy::ExternalSandbox { .. } => { unreachable!("dangerous policies rejected before legacy session prep") @@ -188,12 +171,39 @@ pub(crate) fn prepare_legacy_session_security( Ok(LegacySessionSecurity { h_token, - psid_generic, - psid_workspace, - cap_sid_str, + readonly_sid, + write_root_sids, }) } +pub(crate) fn root_capability_sids( + codex_home: &Path, + cwd: &Path, + allow_paths: impl IntoIterator, +) -> Result> { + let mut roots: Vec = allow_paths.into_iter().collect(); + roots.sort_by_key(|root| canonicalize_path(root.as_path())); + roots.dedup_by(|a, b| canonicalize_path(a.as_path()) == canonicalize_path(b.as_path())); + + let mut out = Vec::with_capacity(roots.len()); + for root in roots { + let sid_str = workspace_write_cap_sid_for_root(codex_home, cwd, &root)?; + let sid = LocalSid::from_string(&sid_str)?; + out.push(RootCapabilitySid { root, sid, sid_str }); + } + Ok(out) +} + +fn matching_root_capability<'a>( + path: &Path, + root_sids: &'a [RootCapabilitySid], +) -> Option<&'a RootCapabilitySid> { + root_sids + .iter() + .filter(|root_sid| workspace_write_root_contains_path(&root_sid.root, path)) + .max_by_key(|root_sid| workspace_write_root_specificity(&root_sid.root)) +} + pub(crate) fn allow_null_device_for_workspace_write(is_workspace_write: bool) { if !is_workspace_write { return; @@ -216,41 +226,67 @@ pub(crate) fn apply_legacy_session_acl_rules( sandbox_policy_cwd: &Path, current_dir: &Path, env_map: &HashMap, - psid_generic: &LocalSid, - psid_workspace: Option<&LocalSid>, + additional_deny_write_paths: &[PathBuf], + readonly_sid: Option<&LocalSid>, + write_root_sids: &[RootCapabilitySid], persist_aces: bool, -) -> Vec { - let AllowDenyPaths { allow, deny } = +) -> Vec<(PathBuf, String)> { + let AllowDenyPaths { allow, mut deny } = compute_allow_paths(policy, sandbox_policy_cwd, current_dir, env_map); - let mut guards: Vec = Vec::new(); - let canonical_cwd = canonicalize_path(current_dir); + for path in additional_deny_write_paths { + if path.exists() { + deny.insert(path.clone()); + } + } + let mut guards: Vec<(PathBuf, String)> = Vec::new(); unsafe { for p in &allow { - let psid = if matches!(policy, SandboxPolicy::WorkspaceWrite { .. }) - && is_command_cwd_root(p, &canonical_cwd) - { - psid_workspace.unwrap_or(psid_generic).as_ptr() - } else { - psid_generic.as_ptr() + let Some(root_sid) = matching_root_capability(p, write_root_sids) else { + continue; }; - if matches!(add_allow_ace(p, psid), Ok(true)) && !persist_aces { - guards.push(p.clone()); + if matches!(add_allow_ace(p, root_sid.sid.as_ptr()), Ok(true)) && !persist_aces { + guards.push((p.clone(), root_sid.sid_str.clone())); } } for p in &deny { - if let Ok(added) = add_deny_write_ace(p, psid_generic.as_ptr()) - && added - && !persist_aces - { - guards.push(p.clone()); + let mut matched_any_root = false; + for root_sid in write_root_sids { + if !workspace_write_root_contains_path(&root_sid.root, p) { + continue; + } + matched_any_root = true; + if let Ok(added) = add_deny_write_ace(p, root_sid.sid.as_ptr()) + && added + && !persist_aces + { + guards.push((p.clone(), root_sid.sid_str.clone())); + } } + if !matched_any_root { + for root_sid in write_root_sids { + if let Ok(added) = add_deny_write_ace(p, root_sid.sid.as_ptr()) + && added + && !persist_aces + { + guards.push((p.clone(), root_sid.sid_str.clone())); + } + } + } + } + for root_sid in write_root_sids { + allow_null_device(root_sid.sid.as_ptr()); } - allow_null_device(psid_generic.as_ptr()); - if let Some(psid_workspace) = psid_workspace { - allow_null_device(psid_workspace.as_ptr()); - if persist_aces && matches!(policy, SandboxPolicy::WorkspaceWrite { .. }) { - let _ = protect_workspace_codex_dir(current_dir, psid_workspace.as_ptr()); - let _ = protect_workspace_agents_dir(current_dir, psid_workspace.as_ptr()); + if let Some(readonly_sid) = readonly_sid { + allow_null_device(readonly_sid.as_ptr()); + } + if persist_aces + && matches!(policy, SandboxPolicy::WorkspaceWrite { .. }) + && let Some(workspace_sid) = matching_root_capability(current_dir, write_root_sids) + { + let canonical_cwd = canonicalize_path(current_dir); + if is_command_cwd_root(&workspace_sid.root, &canonical_cwd) { + let _ = protect_workspace_codex_dir(current_dir, workspace_sid.sid.as_ptr()); + let _ = protect_workspace_agents_dir(current_dir, workspace_sid.sid.as_ptr()); } } } @@ -283,8 +319,20 @@ pub(crate) fn prepare_elevated_spawn_context( ); let write_roots: Vec = allow.into_iter().collect(); let deny_write_paths: Vec = deny.into_iter().collect(); + let effective_write_roots = if common.is_workspace_write { + effective_write_roots_for_setup( + &common.policy, + sandbox_policy_cwd, + &common.current_dir, + env_map, + codex_home, + Some(write_roots.as_slice()), + ) + } else { + Vec::new() + }; let write_roots_override = if common.is_workspace_write { - Some(write_roots.as_slice()) + Some(effective_write_roots.as_slice()) } else { None }; @@ -307,11 +355,14 @@ pub(crate) fn prepare_elevated_spawn_context( vec![caps.readonly.clone()], ), SandboxPolicy::WorkspaceWrite { .. } => { - let cap_sid = workspace_cap_sid_for_cwd(codex_home, cwd)?; - ( - LocalSid::from_string(&caps.workspace)?, - vec![caps.workspace.clone(), cap_sid], - ) + let cap_sids = root_capability_sids(codex_home, cwd, effective_write_roots)? + .into_iter() + .map(|root_sid| root_sid.sid_str) + .collect::>(); + if cap_sids.is_empty() { + anyhow::bail!("workspace-write sandbox has no writable root capability SIDs"); + } + (LocalSid::from_string(&cap_sids[0])?, cap_sids) } SandboxPolicy::DangerFullAccess | SandboxPolicy::ExternalSandbox { .. } => { unreachable!("dangerous policies rejected before elevated session prep") @@ -334,7 +385,10 @@ mod tests { use super::SandboxPolicy; use super::prepare_legacy_spawn_context; use super::prepare_spawn_context_common; + use super::root_capability_sids; use super::should_apply_network_block; + use crate::cap::load_or_create_cap_sids; + use crate::cap::workspace_write_cap_sid_for_root; use pretty_assertions::assert_eq; use std::collections::HashMap; use tempfile::TempDir; @@ -409,4 +463,41 @@ mod tests { Some(&"http://user.proxy:8080".to_string()) ); } + + #[test] + fn root_capability_sids_only_include_active_roots() { + let temp = TempDir::new().expect("tempdir"); + let codex_home = temp.path().join("codex-home"); + let workspace = temp.path().join("workspace"); + let active_root = temp.path().join("active-root"); + let stale_root = temp.path().join("stale-root"); + std::fs::create_dir_all(&codex_home).expect("create codex home"); + std::fs::create_dir_all(&workspace).expect("create workspace"); + std::fs::create_dir_all(&active_root).expect("create active root"); + std::fs::create_dir_all(&stale_root).expect("create stale root"); + + let stale_sid = workspace_write_cap_sid_for_root(&codex_home, &workspace, &stale_root) + .expect("stale sid"); + let active_sid = workspace_write_cap_sid_for_root(&codex_home, &workspace, &active_root) + .expect("active sid"); + let workspace_sid = workspace_write_cap_sid_for_root(&codex_home, &workspace, &workspace) + .expect("workspace sid"); + let caps = load_or_create_cap_sids(&codex_home).expect("load caps"); + + let sid_strs = root_capability_sids( + &codex_home, + &workspace, + vec![workspace.clone(), active_root.clone()], + ) + .expect("root capabilities") + .into_iter() + .map(|root_sid| root_sid.sid_str) + .collect::>(); + + assert_eq!(sid_strs.len(), 2); + assert!(sid_strs.contains(&workspace_sid)); + assert!(sid_strs.contains(&active_sid)); + assert!(!sid_strs.contains(&stale_sid)); + assert!(!sid_strs.contains(&caps.workspace)); + } } diff --git a/codex-rs/windows-sandbox-rs/src/token.rs b/codex-rs/windows-sandbox-rs/src/token.rs index 71a4b1dd7c0a..9638f8cc190e 100644 --- a/codex-rs/windows-sandbox-rs/src/token.rs +++ b/codex-rs/windows-sandbox-rs/src/token.rs @@ -143,6 +143,33 @@ pub unsafe fn convert_string_sid_to_sid(s: &str) -> Option<*mut c_void> { } } +/// Owns a SID allocated by `ConvertStringSidToSidW` and releases it with `LocalFree`. +pub struct LocalSid { + psid: *mut c_void, +} + +impl LocalSid { + pub fn from_string(sid: &str) -> Result { + let psid = unsafe { convert_string_sid_to_sid(sid) } + .ok_or_else(|| anyhow!("invalid SID string: {sid}"))?; + Ok(Self { psid }) + } + + pub fn as_ptr(&self) -> *mut c_void { + self.psid + } +} + +impl Drop for LocalSid { + fn drop(&mut self) { + if !self.psid.is_null() { + unsafe { + LocalFree(self.psid as HLOCAL); + } + } + } +} + /// # Safety /// Caller must close the returned token handle. pub unsafe fn get_current_token_for_restriction() -> Result { diff --git a/codex-rs/windows-sandbox-rs/src/unified_exec/backends/legacy.rs b/codex-rs/windows-sandbox-rs/src/unified_exec/backends/legacy.rs index 8458d5c8c3d9..0490f7f70cba 100644 --- a/codex-rs/windows-sandbox-rs/src/unified_exec/backends/legacy.rs +++ b/codex-rs/windows-sandbox-rs/src/unified_exec/backends/legacy.rs @@ -1,6 +1,7 @@ use super::windows_common::finish_driver_spawn; use super::windows_common::normalize_windows_tty_input; use crate::acl::revoke_ace; +use crate::allow::compute_allow_paths; use crate::conpty::ConptyInstance; use crate::conpty::spawn_conpty_process_as_user; use crate::desktop::LaunchDesktop; @@ -10,11 +11,11 @@ use crate::process::StderrMode; use crate::process::StdinMode; use crate::process::read_handle_loop; use crate::process::spawn_process_with_pipes; -use crate::spawn_prep::LocalSid; use crate::spawn_prep::allow_null_device_for_workspace_write; use crate::spawn_prep::apply_legacy_session_acl_rules; use crate::spawn_prep::prepare_legacy_session_security; use crate::spawn_prep::prepare_legacy_spawn_context; +use crate::token::LocalSid; use anyhow::Result; use codex_utils_pty::ProcessDriver; use codex_utils_pty::SpawnedProcess; @@ -203,8 +204,7 @@ fn finalize_exit( process_handle: Arc>>, thread_handle: HANDLE, output_join: std::thread::JoinHandle<()>, - guards: Vec, - cap_sid: Option, + guards: Vec<(PathBuf, String)>, logs_base_dir: Option<&Path>, command: Vec, ) { @@ -241,11 +241,9 @@ fn finalize_exit( log_failure(&command, &format!("exit code {exit_code}"), logs_base_dir); } - if let Some(cap_sid) = cap_sid - && let Ok(sid) = LocalSid::from_string(&cap_sid) - { - unsafe { - for path in guards { + unsafe { + for (path, cap_sid) in guards { + if let Ok(sid) = LocalSid::from_string(&cap_sid) { revoke_ace(&path, sid.as_ptr()); } } @@ -303,7 +301,14 @@ pub(crate) async fn spawn_windows_sandbox_session_legacy( if !common.policy.has_full_disk_read_access() { anyhow::bail!("Restricted read-only access requires the elevated Windows sandbox backend"); } - let security = prepare_legacy_session_security(&common.policy, codex_home, cwd)?; + let allow_paths = compute_allow_paths( + &common.policy, + sandbox_policy_cwd, + &common.current_dir, + &env_map, + ) + .allow; + let security = prepare_legacy_session_security(&common.policy, codex_home, cwd, allow_paths)?; allow_null_device_for_workspace_write(common.is_workspace_write); let persist_aces = common.is_workspace_write; @@ -312,8 +317,9 @@ pub(crate) async fn spawn_windows_sandbox_session_legacy( sandbox_policy_cwd, &common.current_dir, &env_map, - &security.psid_generic, - security.psid_workspace.as_ref(), + &[], + security.readonly_sid.as_ref(), + &security.write_root_sids, persist_aces, ); @@ -350,12 +356,11 @@ pub(crate) async fn spawn_windows_sandbox_session_legacy( Ok(handles) => handles, Err(err) => { unsafe { - if !persist_aces - && !guards.is_empty() - && let Ok(sid) = LocalSid::from_string(&security.cap_sid_str) - { - for path in &guards { - revoke_ace(path, sid.as_ptr()); + if !persist_aces { + for (path, cap_sid) in &guards { + if let Ok(sid) = LocalSid::from_string(cap_sid) { + revoke_ace(path, sid.as_ptr()); + } } } CloseHandle(security.h_token); @@ -369,11 +374,6 @@ pub(crate) async fn spawn_windows_sandbox_session_legacy( let wait_handle = Arc::clone(&process_handle); let command_for_wait = command.clone(); let guards_for_wait = if persist_aces { Vec::new() } else { guards }; - let cap_sid_for_wait = if guards_for_wait.is_empty() { - None - } else { - Some(security.cap_sid_str) - }; let hpc_for_wait = hpc_handle.clone(); std::thread::spawn(move || { let _desktop = desktop; @@ -405,7 +405,6 @@ pub(crate) async fn spawn_windows_sandbox_session_legacy( pi.hThread, output_join, guards_for_wait, - cap_sid_for_wait, common.logs_base_dir.as_deref(), command_for_wait, );