diff --git a/codex-rs/windows-sandbox-rs/src/cap.rs b/codex-rs/windows-sandbox-rs/src/cap.rs index a699d81d20..c55498e096 100644 --- a/codex-rs/windows-sandbox-rs/src/cap.rs +++ b/codex-rs/windows-sandbox-rs/src/cap.rs @@ -15,11 +15,13 @@ use crate::path_normalization::canonical_path_key; pub struct CapSids { pub workspace: String, pub readonly: String, - /// Per-workspace capability SIDs keyed by canonicalized CWD string. + /// Path-scoped capability SIDs keyed by canonicalized writable-root strings. /// - /// This is used to isolate workspaces from other workspace sandbox writes and to - /// apply per-workspace denies (e.g. protect `CWD/.codex`) - /// without permanently affecting other workspaces. + /// This is used to isolate writable roots from one another so stale ACL grants on + /// one workspace do not automatically authorize later sessions in unrelated roots. + /// + /// The serialized field name is kept for backwards compatibility with existing + /// `cap_sid` files on disk. #[serde(default)] pub workspace_by_cwd: HashMap, } @@ -77,9 +79,14 @@ pub fn load_or_create_cap_sids(codex_home: &Path) -> Result { /// Returns the workspace-specific capability SID for `cwd`, creating and persisting it if missing. pub fn workspace_cap_sid_for_cwd(codex_home: &Path, cwd: &Path) -> Result { + write_cap_sid_for_root(codex_home, cwd) +} + +/// Returns the path-scoped capability SID for `root`, creating and persisting it if missing. +pub fn write_cap_sid_for_root(codex_home: &Path, root: &Path) -> Result { let path = cap_sid_file(codex_home); let mut caps = load_or_create_cap_sids(codex_home)?; - let key = canonical_path_key(cwd); + let key = canonical_path_key(root); if let Some(sid) = caps.workspace_by_cwd.get(&key) { return Ok(sid.clone()); } @@ -92,8 +99,10 @@ pub fn workspace_cap_sid_for_cwd(codex_home: &Path, cwd: &Path) -> Result = caps.workspace_by_cwd.values().cloned().collect(); + assert_eq!(values.len(), 2); + } } diff --git a/codex-rs/windows-sandbox-rs/src/elevated_impl.rs b/codex-rs/windows-sandbox-rs/src/elevated_impl.rs index f6c4563e17..7b399d257a 100644 --- a/codex-rs/windows-sandbox-rs/src/elevated_impl.rs +++ b/codex-rs/windows-sandbox-rs/src/elevated_impl.rs @@ -3,6 +3,7 @@ mod windows_impl { use crate::allow::compute_allow_paths; use crate::allow::AllowDenyPaths; use crate::cap::load_or_create_cap_sids; + use crate::cap::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; @@ -239,25 +240,40 @@ mod windows_impl { anyhow::bail!("DangerFullAccess and ExternalSandbox are not supported for sandboxing") } let caps = load_or_create_cap_sids(codex_home)?; + let AllowDenyPaths { allow, deny: _ } = + compute_allow_paths(&policy, sandbox_policy_cwd, ¤t_dir, &env_map); + let canonical_cwd = crate::path_normalization::canonicalize_path(¤t_dir); + let mut allow_roots: Vec = allow.into_iter().collect(); + allow_roots.sort_by_key(|path| std::cmp::Reverse(path.components().count())); let (psid_to_use, cap_sids) = match &policy { SandboxPolicy::ReadOnly { .. } => ( unsafe { convert_string_sid_to_sid(&caps.readonly).unwrap() }, vec![caps.readonly.clone()], ), - SandboxPolicy::WorkspaceWrite { .. } => ( - unsafe { convert_string_sid_to_sid(&caps.workspace).unwrap() }, - vec![ - caps.workspace.clone(), - crate::cap::workspace_cap_sid_for_cwd(codex_home, cwd)?, - ], - ), + SandboxPolicy::WorkspaceWrite { .. } => { + let mut cap_sids = Vec::with_capacity(allow_roots.len()); + for root in &allow_roots { + let sid = if crate::workspace_acl::is_command_cwd_root(root, &canonical_cwd) { + crate::cap::workspace_cap_sid_for_cwd(codex_home, root)? + } else { + write_cap_sid_for_root(codex_home, root)? + }; + cap_sids.push(sid); + } + let psid_to_use = unsafe { + convert_string_sid_to_sid( + cap_sids + .first() + .expect("workspace-write should always have a writable root"), + ) + .unwrap() + }; + (psid_to_use, cap_sids) + } SandboxPolicy::DangerFullAccess | SandboxPolicy::ExternalSandbox { .. } => { unreachable!("DangerFullAccess handled above") } }; - - let AllowDenyPaths { allow: _, deny: _ } = - compute_allow_paths(&policy, sandbox_policy_cwd, ¤t_dir, &env_map); // Deny/allow ACEs are now applied during setup; avoid per-command churn. unsafe { allow_null_device(psid_to_use); diff --git a/codex-rs/windows-sandbox-rs/src/lib.rs b/codex-rs/windows-sandbox-rs/src/lib.rs index cb4be52750..989642ea86 100644 --- a/codex-rs/windows-sandbox-rs/src/lib.rs +++ b/codex-rs/windows-sandbox-rs/src/lib.rs @@ -64,6 +64,8 @@ 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::write_cap_sid_for_root; +#[cfg(target_os = "windows")] pub use conpty::spawn_conpty_process_as_user; #[cfg(target_os = "windows")] pub use dpapi::protect as dpapi_protect; @@ -179,6 +181,7 @@ mod windows_impl { use super::allow::AllowDenyPaths; use super::cap::load_or_create_cap_sids; use super::cap::workspace_cap_sid_for_cwd; + use super::cap::write_cap_sid_for_root; use super::env::apply_no_network_to_env; use super::env::ensure_non_interactive_pager; use super::env::normalize_null_device_env; @@ -213,6 +216,40 @@ mod windows_impl { type PipeHandles = ((HANDLE, HANDLE), (HANDLE, HANDLE), (HANDLE, HANDLE)); + fn sorted_allow_roots(allow: &std::collections::HashSet) -> Vec { + let mut roots: Vec = allow.iter().cloned().collect(); + roots.sort_by_key(|path| std::cmp::Reverse(path.components().count())); + roots + } + + fn capability_sid_strings_for_allow_roots( + codex_home: &Path, + allow_roots: &[PathBuf], + canonical_cwd: &Path, + ) -> Result> { + let mut sid_by_root = HashMap::new(); + for root in allow_roots { + let sid = if is_command_cwd_root(root, canonical_cwd) { + workspace_cap_sid_for_cwd(codex_home, root)? + } else { + write_cap_sid_for_root(codex_home, root)? + }; + sid_by_root.insert(root.clone(), sid); + } + Ok(sid_by_root) + } + + fn capability_sid_for_path<'a>( + sid_by_root: &'a HashMap, + sorted_allow_roots: &[PathBuf], + path: &Path, + ) -> Option<*mut c_void> { + sorted_allow_roots + .iter() + .find(|root| path.starts_with(root)) + .and_then(|root| sid_by_root.get(root).copied()) + } + fn should_apply_network_block(policy: &SandboxPolicy) -> bool { !policy.has_full_network_access() } @@ -282,6 +319,11 @@ mod windows_impl { let logs_base_dir = Some(sandbox_base.as_path()); log_start(&command, logs_base_dir); let is_workspace_write = matches!(&policy, SandboxPolicy::WorkspaceWrite { .. }); + let current_dir = cwd.to_path_buf(); + let AllowDenyPaths { allow, deny } = + compute_allow_paths(&policy, sandbox_policy_cwd, ¤t_dir, &env_map); + let canonical_cwd = canonicalize_path(¤t_dir); + let sorted_allow_roots = sorted_allow_roots(&allow); if matches!( &policy, @@ -295,25 +337,38 @@ mod windows_impl { ); } 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 { + let sid_strings_by_root = capability_sid_strings_for_allow_roots( + codex_home, + &sorted_allow_roots, + &canonical_cwd, + )?; + let (h_token, psid_readonly, psids_by_root): ( + HANDLE, + Option<*mut c_void>, + HashMap, + ) = unsafe { match &policy { SandboxPolicy::ReadOnly { .. } => { let psid = convert_string_sid_to_sid(&caps.readonly).unwrap(); let (h, _) = super::token::create_readonly_token_with_cap(psid)?; - (h, psid, None) + (h, Some(psid), HashMap::new()) } SandboxPolicy::WorkspaceWrite { .. } => { - let psid_generic = convert_string_sid_to_sid(&caps.workspace).unwrap(); - let ws_sid = workspace_cap_sid_for_cwd(codex_home, cwd)?; - let psid_workspace = convert_string_sid_to_sid(&ws_sid).unwrap(); + let mut psids_by_root = HashMap::new(); + let mut token_caps = Vec::with_capacity(sorted_allow_roots.len()); + for root in &sorted_allow_roots { + let sid = sid_strings_by_root + .get(root) + .expect("missing writable-root SID"); + let psid = convert_string_sid_to_sid(sid).unwrap(); + token_caps.push(psid); + psids_by_root.insert(root.clone(), psid); + } let base = super::token::get_current_token_for_restriction()?; - let h_res = create_workspace_write_token_with_caps_from( - base, - &[psid_generic, psid_workspace], - ); + let h_res = create_workspace_write_token_with_caps_from(base, &token_caps); windows_sys::Win32::Foundation::CloseHandle(base); let h = h_res?; - (h, psid_generic, Some(psid_workspace)) + (h, None, psids_by_root) } SandboxPolicy::DangerFullAccess | SandboxPolicy::ExternalSandbox { .. } => { unreachable!("DangerFullAccess handled above") @@ -335,16 +390,15 @@ mod windows_impl { } let persist_aces = is_workspace_write; - let AllowDenyPaths { allow, deny } = - compute_allow_paths(&policy, sandbox_policy_cwd, ¤t_dir, &env_map); - 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) + let Some(psid) = (if is_workspace_write { + capability_sid_for_path(&psids_by_root, &sorted_allow_roots, p) } else { - psid_generic + psid_readonly + }) else { + continue; }; if let Ok(added) = add_allow_ace(p, psid) { if added { @@ -359,15 +413,28 @@ mod windows_impl { } } for p in &deny { - if let Ok(added) = add_deny_write_ace(p, psid_generic) { + let Some(psid) = (if is_workspace_write { + capability_sid_for_path(&psids_by_root, &sorted_allow_roots, p) + } else { + psid_readonly + }) else { + continue; + }; + if let Ok(added) = add_deny_write_ace(p, psid) { if added && !persist_aces { - guards.push((p.clone(), psid_generic)); + guards.push((p.clone(), psid)); } } } - allow_null_device(psid_generic); - if let Some(psid) = psid_workspace { + if let Some(psid) = psid_readonly { allow_null_device(psid); + } + for psid in psids_by_root.values().copied() { + allow_null_device(psid); + } + if let Some(psid) = + capability_sid_for_path(&psids_by_root, &sorted_allow_roots, ¤t_dir) + { let _ = protect_workspace_codex_dir(¤t_dir, psid); let _ = protect_workspace_agents_dir(¤t_dir, psid); } @@ -525,33 +592,49 @@ mod windows_impl { } ensure_codex_home_exists(codex_home)?; - let caps = load_or_create_cap_sids(codex_home)?; - 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)?; - 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); + let sorted_allow_roots = sorted_allow_roots(&allow); + let sid_strings_by_root = capability_sid_strings_for_allow_roots( + codex_home, + &sorted_allow_roots, + &canonical_cwd, + )?; + let mut psids_by_root = HashMap::new(); + for root in &sorted_allow_roots { + let sid = sid_strings_by_root + .get(root) + .expect("missing writable-root SID"); + let psid = unsafe { convert_string_sid_to_sid(sid) }.expect("valid workspace SID"); + psids_by_root.insert(root.clone(), psid); + } unsafe { for p in &allow { - let psid = if is_command_cwd_root(p, &canonical_cwd) { - psid_workspace - } else { - psid_generic + let Some(psid) = capability_sid_for_path(&psids_by_root, &sorted_allow_roots, p) + else { + continue; }; let _ = add_allow_ace(p, psid); } for p in &deny { - let _ = add_deny_write_ace(p, psid_generic); + let Some(psid) = capability_sid_for_path(&psids_by_root, &sorted_allow_roots, p) + else { + continue; + }; + let _ = add_deny_write_ace(p, psid); + } + for psid in psids_by_root.values().copied() { + allow_null_device(psid); + } + if let Some(psid) = + capability_sid_for_path(&psids_by_root, &sorted_allow_roots, ¤t_dir) + { + let _ = protect_workspace_codex_dir(¤t_dir, psid); + let _ = protect_workspace_agents_dir(¤t_dir, psid); } - allow_null_device(psid_generic); - allow_null_device(psid_workspace); - let _ = protect_workspace_codex_dir(¤t_dir, psid_workspace); - let _ = protect_workspace_agents_dir(¤t_dir, psid_workspace); } 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 86c1d104e0..b4178b14e8 100644 --- a/codex-rs/windows-sandbox-rs/src/setup_main_win.rs +++ b/codex-rs/windows-sandbox-rs/src/setup_main_win.rs @@ -13,7 +13,6 @@ use codex_windows_sandbox::ensure_allow_write_aces; use codex_windows_sandbox::extract_setup_failure; use codex_windows_sandbox::hide_newly_created_users; 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::protect_workspace_agents_dir; @@ -24,6 +23,7 @@ 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::write_cap_sid_for_root; use codex_windows_sandbox::write_setup_error_report; use codex_windows_sandbox::SetupErrorCode; use codex_windows_sandbox::SetupErrorReport; @@ -551,25 +551,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 firewall_result = firewall::ensure_offline_outbound_block(&offline_sid_str, log); @@ -616,7 +597,6 @@ fn run_setup_full(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<( } } - let cap_sid_str = caps.workspace.clone(); let sandbox_group_sid_str = string_from_sid_bytes(&sandbox_group_sid).map_err(anyhow::Error::msg)?; let write_mask = @@ -639,19 +619,18 @@ fn run_setup_full(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<( } let mut need_grant = false; let is_command_cwd = is_command_cwd_root(root, &canonical_command_cwd); - let cap_label = if is_command_cwd { - "workspace_cap" + let cap_sid_str_for_root = if is_command_cwd { + workspace_cap_sid_for_cwd(&payload.codex_home, root)? } else { - "cap" + write_cap_sid_for_root(&payload.codex_home, root)? }; - let cap_psid_for_root = if is_command_cwd { - workspace_psid - } else { - cap_psid + let cap_psid_for_root = unsafe { + convert_string_sid_to_sid(&cap_sid_str_for_root) + .ok_or_else(|| anyhow::anyhow!("convert writable-root capability SID failed"))? }; for (label, psid) in [ ("sandbox_group", sandbox_group_psid), - (cap_label, cap_psid_for_root), + ("path_cap", cap_psid_for_root), ] { let has = match path_mask_allows(root, &[psid], write_mask, /*require_all_bits*/ true) { @@ -694,9 +673,15 @@ fn run_setup_full(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<( 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()] + vec![ + sandbox_group_sid_str.clone(), + workspace_cap_sid_for_cwd(&payload.codex_home, &root)?, + ] } else { - vec![sandbox_group_sid_str.clone(), cap_sid_str.clone()] + vec![ + sandbox_group_sid_str.clone(), + write_cap_sid_for_root(&payload.codex_home, &root)?, + ] }; let tx = tx.clone(); scope.spawn(move || {