diff --git a/codex-rs/windows-sandbox-rs/src/audit.rs b/codex-rs/windows-sandbox-rs/src/audit.rs index c45e3341b..c7e7fc0b1 100644 --- a/codex-rs/windows-sandbox-rs/src/audit.rs +++ b/codex-rs/windows-sandbox-rs/src/audit.rs @@ -2,14 +2,16 @@ 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; use crate::logging::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::Result; -use anyhow::anyhow; use std::collections::HashSet; use std::ffi::OsStr; use std::ffi::c_void; @@ -231,6 +233,7 @@ pub fn apply_world_writable_scan_and_denies( &flagged, sandbox_policy, cwd, + env_map, logs_base_dir, ) { log_note( @@ -246,6 +249,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() { @@ -255,46 +259,56 @@ 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 { .. } => { + (vec![LocalSid::from_string(&caps.readonly)?], Vec::new()) } - SandboxPolicy::ReadOnly { .. } => ( - unsafe { convert_string_sid_to_sid(&caps.readonly) } - .ok_or_else(|| anyhow!("ConvertStringSidToSidW failed for readonly capability"))?, - Vec::new(), - ), SandboxPolicy::DangerFullAccess | SandboxPolicy::ExternalSandbox { .. } => { return Ok(()); } }; 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/bin/setup_main/win.rs b/codex-rs/windows-sandbox-rs/src/bin/setup_main/win.rs index d862f33ec..1f93d0a5e 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 @@ -20,7 +20,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_secrets_dir; use codex_windows_sandbox::string_from_sid_bytes; use codex_windows_sandbox::sync_persistent_deny_read_acls; 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_overlaps_path; use codex_windows_sandbox::write_setup_error_report; use serde::Deserialize; use serde::Serialize; @@ -120,6 +120,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_overlaps_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; @@ -566,25 +602,6 @@ fn run_setup_full(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<( let sandbox_group_sid_str = string_from_sid_bytes(&sandbox_group_sid).map_err(anyhow::Error::msg)?; - 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( @@ -681,10 +698,9 @@ fn run_setup_full(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<( )?; } - let cap_sid_str = caps.workspace; 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(); @@ -706,16 +722,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) { @@ -741,6 +758,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, @@ -749,19 +769,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. @@ -823,27 +838,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); } } } @@ -924,12 +948,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( @@ -946,9 +964,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!({ @@ -986,4 +1008,104 @@ 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)); + } + + #[test] + fn deny_path_includes_nested_active_root_sid() { + let temp = tempfile::tempdir().expect("tempdir"); + let codex_home = temp.path().join("codex-home"); + let workspace = temp.path().join("workspace"); + let protected_dir = workspace.join(".codex"); + let nested_root = protected_dir.join("nested-root"); + fs::create_dir_all(&codex_home).expect("create codex home"); + fs::create_dir_all(&workspace).expect("create workspace"); + fs::create_dir_all(&nested_root).expect("create nested root"); + + let workspace_sid = workspace_write_cap_sid_for_root(&codex_home, &workspace, &workspace) + .expect("workspace sid"); + let nested_sid = workspace_write_cap_sid_for_root(&codex_home, &workspace, &nested_root) + .expect("nested sid"); + + let deny_sids = workspace_write_cap_sids_for_path( + &codex_home, + &workspace, + &[workspace.clone(), nested_root], + &protected_dir, + ) + .expect("deny sids"); + + assert_eq!(deny_sids, vec![workspace_sid, nested_sid]); + } } diff --git a/codex-rs/windows-sandbox-rs/src/cap.rs b/codex-rs/windows-sandbox-rs/src/cap.rs index 85f7e2c9d..41b33b63f 100644 --- a/codex-rs/windows-sandbox-rs/src/cap.rs +++ b/codex-rs/windows-sandbox-rs/src/cap.rs @@ -1,4 +1,5 @@ use crate::path_normalization::canonical_path_key; +use crate::path_normalization::canonicalize_path; use anyhow::Context; use anyhow::Result; use rand::RngCore; @@ -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,50 @@ 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_overlaps_path(root: &Path, path: &Path) -> bool { + workspace_write_root_contains_path(root, path) || workspace_write_root_contains_path(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::workspace_cap_sid_for_cwd; + use super::workspace_write_cap_sid_for_root; + use super::writable_root_cap_sid_for_path; use pretty_assertions::assert_eq; use std::path::PathBuf; @@ -123,4 +173,31 @@ 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 e1ce8c1db..f43715c12 100644 --- a/codex-rs/windows-sandbox-rs/src/elevated_impl.rs +++ b/codex-rs/windows-sandbox-rs/src/elevated_impl.rs @@ -24,6 +24,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; @@ -41,7 +42,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 codex_utils_absolute_path::AbsolutePathBuf; use std::path::Path; @@ -109,22 +111,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") @@ -132,7 +140,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 78081b5e1..db958c997 100644 --- a/codex-rs/windows-sandbox-rs/src/lib.rs +++ b/codex-rs/windows-sandbox-rs/src/lib.rs @@ -110,6 +110,12 @@ 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 cap::workspace_write_root_overlaps_path; +#[cfg(target_os = "windows")] pub use conpty::ConptyInstance; #[cfg(target_os = "windows")] pub use conpty::spawn_conpty_process_as_user; @@ -222,7 +228,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")] @@ -273,34 +279,25 @@ 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::deny_read_acl::apply_deny_read_acls; - use super::deny_read_state::sync_persistent_deny_read_acls; 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::LegacyAclSids; + use super::spawn_prep::allow_null_device_for_workspace_write; + use super::spawn_prep::apply_legacy_session_acl_rules; + use super::spawn_prep::legacy_session_capability_roots; + 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 anyhow::Context; + use super::spawn_prep::root_capability_sids; + use super::token::LocalSid; use anyhow::Result; use codex_utils_absolute_path::AbsolutePathBuf; use std::collections::HashMap; - use std::ffi::c_void; use std::io; use std::path::Path; - use std::path::PathBuf; use std::ptr; use windows_sys::Win32::Foundation::CloseHandle; use windows_sys::Win32::Foundation::GetLastError; @@ -418,130 +415,36 @@ mod windows_impl { if !additional_deny_read_paths.is_empty() { anyhow::bail!("deny-read overrides require 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 capability_roots = legacy_session_capability_roots( + &policy, + sandbox_policy_cwd, + ¤t_dir, + &env_map, + codex_home, + ); + let security = prepare_legacy_session_security(&policy, codex_home, cwd, capability_roots)?; + 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 { - // Explicit deny-write carveouts must already exist when the process - // starts, otherwise it could create a missing path under a writable - // parent before the deny-write ACE exists. - if !path.exists() { - std::fs::create_dir_all(&path) - .with_context(|| format!("create deny-write path {}", path.display()))?; - } - 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)); - } - } - // Read denies are layered after allow/deny-write setup so they can - // override broad read grants for the sandbox principal without - // changing the existing write policy computation. - let applied_deny_read_paths = match if persist_aces { - sync_persistent_deny_read_acls( - codex_home, - &caps.workspace, - &additional_deny_read_paths, - psid_generic, - ) - } else { - apply_deny_read_acls(&additional_deny_read_paths, psid_generic) - } { - Ok(paths) => paths, - Err(err) => { - if !persist_aces { - cleanup_acl_guards(&mut guards); - } - return Err(err); - } - }; - if !persist_aces { - for path in applied_deny_read_paths { - guards.push((path, 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, + codex_home, + ¤t_dir, + &env_map, + &additional_deny_read_paths, + &additional_deny_write_paths, + LegacyAclSids { + readonly_sid: security.readonly_sid.as_ref(), + readonly_sid_str: security.readonly_sid_str.as_deref(), + write_root_sids: &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, @@ -553,7 +456,6 @@ mod windows_impl { let created = match spawn_res { Ok(v) => v, Err(err) => { - cleanup_acl_guards(&mut guards); unsafe { CloseHandle(in_r); CloseHandle(in_w); @@ -561,7 +463,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); } @@ -643,7 +552,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(); @@ -662,7 +571,13 @@ mod windows_impl { } if !persist_aces { - cleanup_acl_guards(&mut guards); + unsafe { + for (p, sid_str) in guards { + if let Ok(sid) = LocalSid::from_string(&sid_str) { + revoke_ace(&p, sid.as_ptr()); + } + } + } } Ok(CaptureResult { exit_code, @@ -672,14 +587,6 @@ mod windows_impl { }) } - fn cleanup_acl_guards(guards: &mut Vec<(PathBuf, *mut c_void)>) { - unsafe { - for (p, sid) in guards.drain(..) { - revoke_ace(&p, sid); - } - } - } - pub fn run_windows_sandbox_legacy_preflight( sandbox_policy: &SandboxPolicy, sandbox_policy_cwd: &Path, @@ -693,33 +600,30 @@ 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 } = - 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 capability_roots = legacy_session_capability_roots( + sandbox_policy, + sandbox_policy_cwd, + ¤t_dir, + env_map, + codex_home, + ); + let write_root_sids = root_capability_sids(codex_home, cwd, capability_roots)?; + let _guards = apply_legacy_session_acl_rules( + sandbox_policy, + sandbox_policy_cwd, + codex_home, + ¤t_dir, + env_map, + &[], + &[], + LegacyAclSids { + readonly_sid: None, + readonly_sid_str: None, + write_root_sids: &write_root_sids, + }, + /*persist_aces*/ true, + )?; Ok(()) } diff --git a/codex-rs/windows-sandbox-rs/src/setup.rs b/codex-rs/windows-sandbox-rs/src/setup.rs index 3b6e47086..0de2c6e02 100644 --- a/codex-rs/windows-sandbox-rs/src/setup.rs +++ b/codex-rs/windows-sandbox-rs/src/setup.rs @@ -413,6 +413,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, @@ -761,21 +781,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. @@ -1405,6 +1418,66 @@ 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_read_paths: None, + 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 b0cf0f307..331df4f43 100644 --- a/codex-rs/windows-sandbox-rs/src/spawn_prep.rs +++ b/codex-rs/windows-sandbox-rs/src/spawn_prep.rs @@ -4,7 +4,10 @@ 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_overlaps_path; +use crate::cap::workspace_write_root_specificity; use crate::deny_read_acl::apply_deny_read_acls; use crate::deny_read_state::sync_persistent_deny_read_acls; use crate::env::apply_no_network_to_env; @@ -19,7 +22,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; @@ -35,8 +39,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, @@ -54,36 +56,21 @@ 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) readonly_sid_str: 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, +pub(crate) struct RootCapabilitySid { + pub(crate) root: PathBuf, + pub(crate) sid: LocalSid, + pub(crate) sid_str: String, } -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 LegacyAclSids<'a> { + pub(crate) readonly_sid: Option<&'a LocalSid>, + pub(crate) readonly_sid_str: Option<&'a str>, + pub(crate) write_root_sids: &'a [RootCapabilitySid], } pub(crate) fn should_apply_network_block(policy: &SandboxPolicy) -> bool { @@ -161,27 +148,31 @@ pub(crate) fn prepare_legacy_session_security( policy: &SandboxPolicy, codex_home: &Path, cwd: &Path, + capability_roots: 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, readonly_sid_str, 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), Some(caps.readonly), 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, capability_roots)?; + 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, None, write_root_sids) } SandboxPolicy::DangerFullAccess | SandboxPolicy::ExternalSandbox { .. } => { unreachable!("dangerous policies rejected before legacy session prep") @@ -191,12 +182,80 @@ pub(crate) fn prepare_legacy_session_security( Ok(LegacySessionSecurity { h_token, - psid_generic, - psid_workspace, - cap_sid_str, + readonly_sid, + readonly_sid_str, + write_root_sids, }) } +pub(crate) fn legacy_session_capability_roots( + policy: &SandboxPolicy, + policy_cwd: &Path, + current_dir: &Path, + env_map: &HashMap, + codex_home: &Path, +) -> Vec { + let allow_paths = compute_allow_paths(policy, policy_cwd, current_dir, env_map) + .allow + .into_iter() + .collect::>(); + if matches!(policy, SandboxPolicy::WorkspaceWrite { .. }) { + effective_write_roots_for_setup( + policy, + policy_cwd, + current_dir, + env_map, + codex_home, + Some(allow_paths.as_slice()), + ) + } else { + allow_paths + } +} + +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)) +} + +fn deny_root_capabilities_for_path<'a>( + path: &Path, + root_sids: &'a [RootCapabilitySid], +) -> Vec<&'a RootCapabilitySid> { + let matching_root_sids = root_sids + .iter() + .filter(|root_sid| workspace_write_root_overlaps_path(&root_sid.root, path)) + .collect::>(); + if matching_root_sids.is_empty() { + root_sids.iter().collect() + } else { + matching_root_sids + } +} + pub(crate) fn allow_null_device_for_workspace_write(is_workspace_write: bool) { if !is_workspace_write { return; @@ -221,17 +280,14 @@ pub(crate) fn apply_legacy_session_acl_rules( codex_home: &Path, current_dir: &Path, env_map: &HashMap, - psid_generic: &LocalSid, - psid_workspace: Option<&LocalSid>, - cap_sid_str: &str, additional_deny_read_paths: &[PathBuf], additional_deny_write_paths: &[PathBuf], + acl_sids: LegacyAclSids<'_>, persist_aces: bool, -) -> Result> { +) -> Result> { 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); + let mut guards: Vec<(PathBuf, String)> = Vec::new(); unsafe { for path in additional_deny_write_paths { // Explicit carveouts must exist before the command starts so the @@ -242,45 +298,94 @@ pub(crate) fn apply_legacy_session_acl_rules( } deny.insert(path.clone()); } - 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() - }; - if matches!(add_allow_ace(p, psid), Ok(true)) && !persist_aces { - guards.push(p.clone()); + if let Some(readonly_sid) = acl_sids.readonly_sid { + for p in &allow { + if matches!(add_allow_ace(p, readonly_sid.as_ptr()), Ok(true)) + && !persist_aces + && let Some(readonly_sid_str) = acl_sids.readonly_sid_str + { + guards.push((p.clone(), readonly_sid_str.to_string())); + } + } + } else { + for p in &allow { + let Some(root_sid) = matching_root_capability(p, acl_sids.write_root_sids) else { + continue; + }; + 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()); + for root_sid in deny_root_capabilities_for_path(p, acl_sids.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())); + } } } - let applied_deny_read_paths = if persist_aces { - sync_persistent_deny_read_acls( - codex_home, - cap_sid_str, - additional_deny_read_paths, - psid_generic.as_ptr(), - )? - } else { - apply_deny_read_acls(additional_deny_read_paths, psid_generic.as_ptr())? - }; - if !persist_aces { - guards.extend(applied_deny_read_paths); + if !additional_deny_read_paths.is_empty() { + if let Some(readonly_sid) = acl_sids.readonly_sid { + let Some(readonly_sid_str) = acl_sids.readonly_sid_str else { + anyhow::bail!("readonly capability SID string missing"); + }; + let applied_deny_read_paths = if persist_aces { + sync_persistent_deny_read_acls( + codex_home, + readonly_sid_str, + additional_deny_read_paths, + readonly_sid.as_ptr(), + )? + } else { + apply_deny_read_acls(additional_deny_read_paths, readonly_sid.as_ptr())? + }; + if !persist_aces { + guards.extend( + applied_deny_read_paths + .into_iter() + .map(|path| (path, readonly_sid_str.to_string())), + ); + } + } else { + for root_sid in acl_sids.write_root_sids { + let applied_deny_read_paths = if persist_aces { + sync_persistent_deny_read_acls( + codex_home, + &root_sid.sid_str, + additional_deny_read_paths, + root_sid.sid.as_ptr(), + )? + } else { + apply_deny_read_acls(additional_deny_read_paths, root_sid.sid.as_ptr())? + }; + if !persist_aces { + guards.extend( + applied_deny_read_paths + .into_iter() + .map(|path| (path, root_sid.sid_str.clone())), + ); + } + } + } } - 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()); + for root_sid in acl_sids.write_root_sids { + allow_null_device(root_sid.sid.as_ptr()); + } + if let Some(readonly_sid) = acl_sids.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, acl_sids.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()); } } } @@ -324,6 +429,24 @@ pub(crate) fn prepare_elevated_spawn_context( } else { None }; + let write_roots_for_setup = write_roots_override.or(computed_write_roots_override); + 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, + write_roots_for_setup, + ) + } else { + Vec::new() + }; + let setup_write_roots_override = if common.is_workspace_write { + Some(effective_write_roots.as_slice()) + } else { + write_roots_override + }; let sandbox_creds = require_logon_sandbox_creds( &common.policy, sandbox_policy_cwd, @@ -332,7 +455,7 @@ pub(crate) fn prepare_elevated_spawn_context( codex_home, read_roots_override, read_roots_include_platform_defaults, - write_roots_override.or(computed_write_roots_override), + setup_write_roots_override, deny_read_paths_override, if deny_write_paths_override.is_empty() { &deny_write_paths @@ -348,11 +471,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") @@ -373,9 +499,15 @@ pub(crate) fn prepare_elevated_spawn_context( #[cfg(test)] mod tests { use super::SandboxPolicy; + use super::deny_root_capabilities_for_path; + use super::legacy_session_capability_roots; 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 codex_utils_absolute_path::AbsolutePathBuf; use pretty_assertions::assert_eq; use std::collections::HashMap; use tempfile::TempDir; @@ -450,4 +582,114 @@ 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], + ) + .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)); + } + + #[test] + fn legacy_deny_path_includes_nested_active_root_sid() { + let temp = TempDir::new().expect("tempdir"); + let codex_home = temp.path().join("codex-home"); + let workspace = temp.path().join("workspace"); + let protected_dir = workspace.join(".codex"); + let nested_root = protected_dir.join("nested-root"); + let unrelated_root = temp.path().join("unrelated-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(&nested_root).expect("create nested root"); + std::fs::create_dir_all(&unrelated_root).expect("create unrelated root"); + + let workspace_sid = workspace_write_cap_sid_for_root(&codex_home, &workspace, &workspace) + .expect("workspace sid"); + let nested_sid = workspace_write_cap_sid_for_root(&codex_home, &workspace, &nested_root) + .expect("nested sid"); + let unrelated_sid = + workspace_write_cap_sid_for_root(&codex_home, &workspace, &unrelated_root) + .expect("unrelated sid"); + let root_sids = root_capability_sids( + &codex_home, + &workspace, + vec![workspace.clone(), nested_root, unrelated_root], + ) + .expect("root capabilities"); + + let deny_sid_strs = deny_root_capabilities_for_path(&protected_dir, &root_sids) + .into_iter() + .map(|root_sid| root_sid.sid_str.clone()) + .collect::>(); + + assert_eq!(deny_sid_strs, vec![workspace_sid, nested_sid]); + assert!(!deny_sid_strs.contains(&unrelated_sid)); + } + + #[test] + fn legacy_capability_roots_use_effective_write_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 sandbox_root = codex_home.join(".sandbox"); + 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(&sandbox_root).expect("create sandbox root"); + + let policy = SandboxPolicy::WorkspaceWrite { + writable_roots: vec![ + AbsolutePathBuf::try_from(active_root.as_path()).expect("active root"), + AbsolutePathBuf::try_from(codex_home.as_path()).expect("codex home"), + AbsolutePathBuf::try_from(sandbox_root.as_path()).expect("sandbox root"), + ], + network_access: false, + exclude_tmpdir_env_var: true, + exclude_slash_tmp: true, + }; + + let roots = legacy_session_capability_roots( + &policy, + &workspace, + &workspace, + &HashMap::new(), + &codex_home, + ); + + assert!(roots.contains(&dunce::canonicalize(&workspace).expect("workspace"))); + assert!(roots.contains(&dunce::canonicalize(&active_root).expect("active root"))); + assert!(!roots.contains(&dunce::canonicalize(&codex_home).expect("codex home"))); + assert!(!roots.contains(&dunce::canonicalize(&sandbox_root).expect("sandbox root"))); + } } diff --git a/codex-rs/windows-sandbox-rs/src/token.rs b/codex-rs/windows-sandbox-rs/src/token.rs index aabf77469..2c7a74647 100644 --- a/codex-rs/windows-sandbox-rs/src/token.rs +++ b/codex-rs/windows-sandbox-rs/src/token.rs @@ -139,6 +139,33 @@ pub unsafe fn convert_string_sid_to_sid(s: &str) -> Option<*mut c_void> { if ok != 0 { Some(psid) } else { None } } +/// 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 81096e8c6..2f76147ce 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 @@ -10,11 +10,13 @@ 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::LegacyAclSids; use crate::spawn_prep::allow_null_device_for_workspace_write; use crate::spawn_prep::apply_legacy_session_acl_rules; +use crate::spawn_prep::legacy_session_capability_roots; 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_absolute_path::AbsolutePathBuf; use codex_utils_pty::ProcessDriver; @@ -204,8 +206,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, ) { @@ -242,11 +243,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()); } } @@ -311,15 +310,19 @@ pub(crate) async fn spawn_windows_sandbox_session_legacy( if !additional_deny_read_paths.is_empty() { anyhow::bail!("deny-read overrides require the elevated Windows sandbox backend"); } - let additional_deny_read_paths = additional_deny_read_paths - .iter() - .map(AbsolutePathBuf::to_path_buf) - .collect::>(); let additional_deny_write_paths = additional_deny_write_paths .iter() .map(AbsolutePathBuf::to_path_buf) .collect::>(); - let security = prepare_legacy_session_security(&common.policy, codex_home, cwd)?; + let capability_roots = legacy_session_capability_roots( + &common.policy, + sandbox_policy_cwd, + &common.current_dir, + &env_map, + codex_home, + ); + let security = + prepare_legacy_session_security(&common.policy, codex_home, cwd, capability_roots)?; allow_null_device_for_workspace_write(common.is_workspace_write); let persist_aces = common.is_workspace_write; @@ -329,11 +332,13 @@ pub(crate) async fn spawn_windows_sandbox_session_legacy( codex_home, &common.current_dir, &env_map, - &security.psid_generic, - security.psid_workspace.as_ref(), - &security.cap_sid_str, - &additional_deny_read_paths, + &[], &additional_deny_write_paths, + LegacyAclSids { + readonly_sid: security.readonly_sid.as_ref(), + readonly_sid_str: security.readonly_sid_str.as_deref(), + write_root_sids: &security.write_root_sids, + }, persist_aces, )?; @@ -370,12 +375,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); @@ -389,11 +393,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; @@ -425,7 +424,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, );