From 820f4b59a5d7fdc25ed87bb38d589bd8e610512c Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Wed, 20 May 2026 10:33:31 -0700 Subject: [PATCH] windows-sandbox: drive write roots from resolved permissions --- codex-rs/windows-sandbox-rs/src/allow.rs | 33 ++- codex-rs/windows-sandbox-rs/src/audit.rs | 29 ++- .../windows-sandbox-rs/src/elevated_impl.rs | 51 ++-- codex-rs/windows-sandbox-rs/src/identity.rs | 9 +- codex-rs/windows-sandbox-rs/src/lib.rs | 46 +++- .../src/resolved_permissions.rs | 50 +++- codex-rs/windows-sandbox-rs/src/setup.rs | 84 ++++++- codex-rs/windows-sandbox-rs/src/spawn_prep.rs | 231 ++++++++++++------ .../src/unified_exec/backends/legacy.rs | 35 ++- 9 files changed, 420 insertions(+), 148 deletions(-) diff --git a/codex-rs/windows-sandbox-rs/src/allow.rs b/codex-rs/windows-sandbox-rs/src/allow.rs index e82cf8cd62..b13081cdb5 100644 --- a/codex-rs/windows-sandbox-rs/src/allow.rs +++ b/codex-rs/windows-sandbox-rs/src/allow.rs @@ -14,12 +14,12 @@ pub struct AllowDenyPaths { pub(crate) fn compute_allow_paths( policy: &SandboxPolicy, - _policy_cwd: &Path, + policy_cwd: &Path, command_cwd: &Path, env_map: &HashMap, ) -> AllowDenyPaths { let permissions = - ResolvedWindowsSandboxPermissions::from_legacy_policy_for_cwd(policy, command_cwd); + ResolvedWindowsSandboxPermissions::from_legacy_policy_for_cwd(policy, policy_cwd); compute_allow_paths_for_permissions(&permissions, command_cwd, env_map) } @@ -91,6 +91,35 @@ mod tests { assert!(paths.deny.is_empty(), "no deny paths expected"); } + #[test] + fn uses_policy_cwd_for_legacy_workspace_root() { + let tmp = TempDir::new().expect("tempdir"); + let policy_cwd = tmp.path().join("workspace"); + let command_cwd = policy_cwd.join("subdir"); + fs::create_dir_all(&command_cwd).expect("create command cwd"); + + let policy = SandboxPolicy::WorkspaceWrite { + writable_roots: vec![], + network_access: false, + exclude_tmpdir_env_var: true, + exclude_slash_tmp: true, + }; + + let paths = compute_allow_paths(&policy, &policy_cwd, &command_cwd, &HashMap::new()); + + assert!( + paths + .allow + .contains(&dunce::canonicalize(&policy_cwd).unwrap()) + ); + assert!( + !paths + .allow + .contains(&dunce::canonicalize(&command_cwd).unwrap()) + ); + assert!(paths.deny.is_empty(), "no deny paths expected"); + } + #[test] fn excludes_tmp_env_vars_when_requested() { let tmp = TempDir::new().expect("tempdir"); diff --git a/codex-rs/windows-sandbox-rs/src/audit.rs b/codex-rs/windows-sandbox-rs/src/audit.rs index c7e7fc0b11..cab0a06adf 100644 --- a/codex-rs/windows-sandbox-rs/src/audit.rs +++ b/codex-rs/windows-sandbox-rs/src/audit.rs @@ -8,7 +8,8 @@ use crate::logging::debug_log; use crate::logging::log_note; use crate::path_normalization::canonical_path_key; use crate::policy::SandboxPolicy; -use crate::setup::effective_write_roots_for_setup; +use crate::resolved_permissions::ResolvedWindowsSandboxPermissions; +use crate::setup::effective_write_roots_for_permissions; use crate::token::LocalSid; use crate::token::world_sid; use anyhow::Result; @@ -259,11 +260,18 @@ 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_sids, workspace_roots): (Vec, Vec) = match sandbox_policy { - SandboxPolicy::WorkspaceWrite { .. } => { - let roots = effective_write_roots_for_setup( - sandbox_policy, - cwd, + if matches!( + sandbox_policy, + SandboxPolicy::DangerFullAccess | SandboxPolicy::ExternalSandbox { .. } + ) { + return Ok(()); + } + let permissions = + ResolvedWindowsSandboxPermissions::from_legacy_policy_for_cwd(sandbox_policy, cwd); + let (active_sids, workspace_roots): (Vec, Vec) = + if permissions.uses_write_capabilities_for_cwd(cwd, env_map) { + let roots = effective_write_roots_for_permissions( + &permissions, cwd, env_map, codex_home, @@ -277,14 +285,9 @@ pub fn apply_capability_denies_for_world_writable( }) .collect::>>()?; (active_sids, roots) - } - SandboxPolicy::ReadOnly { .. } => { + } else { (vec![LocalSid::from_string(&caps.readonly)?], Vec::new()) - } - SandboxPolicy::DangerFullAccess | SandboxPolicy::ExternalSandbox { .. } => { - return Ok(()); - } - }; + }; for path in flagged { if workspace_roots .iter() diff --git a/codex-rs/windows-sandbox-rs/src/elevated_impl.rs b/codex-rs/windows-sandbox-rs/src/elevated_impl.rs index 5861317376..b012fd8af5 100644 --- a/codex-rs/windows-sandbox-rs/src/elevated_impl.rs +++ b/codex-rs/windows-sandbox-rs/src/elevated_impl.rs @@ -39,10 +39,11 @@ mod windows_impl { use crate::logging::log_success; use crate::policy::SandboxPolicy; use crate::policy::parse_policy; + use crate::resolved_permissions::ResolvedWindowsSandboxPermissions; 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::setup::effective_write_roots_for_setup; + use crate::setup::effective_write_roots_for_permissions; use crate::token::LocalSid; use anyhow::Result; use codex_protocol::models::PermissionProfile; @@ -81,6 +82,10 @@ mod windows_impl { .map(AbsolutePathBuf::to_path_buf) .collect::>(); let policy = parse_policy(policy_json_or_preset)?; + let permissions = ResolvedWindowsSandboxPermissions::from_legacy_policy_for_cwd( + &policy, + sandbox_policy_cwd, + ); normalize_null_device_env(&mut env_map); ensure_non_interactive_pager(&mut env_map); inherit_path_env(&mut env_map); @@ -112,32 +117,26 @@ mod windows_impl { anyhow::bail!("DangerFullAccess and ExternalSandbox are not supported for sandboxing") } let caps = load_or_create_cap_sids(codex_home)?; - let (sid_for_null, cap_sids) = match &policy { - SandboxPolicy::ReadOnly { .. } => { - let sid = LocalSid::from_string(&caps.readonly)?; - (sid, vec![caps.readonly]) - } - SandboxPolicy::WorkspaceWrite { .. } => { - 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") + let (sid_for_null, cap_sids) = if permissions.uses_write_capabilities_for_cwd(cwd, &env_map) + { + let write_roots = effective_write_roots_for_permissions( + &permissions, + 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) + } else { + let sid = LocalSid::from_string(&caps.readonly)?; + (sid, vec![caps.readonly]) }; unsafe { diff --git a/codex-rs/windows-sandbox-rs/src/identity.rs b/codex-rs/windows-sandbox-rs/src/identity.rs index e49af3017b..30bf7ef493 100644 --- a/codex-rs/windows-sandbox-rs/src/identity.rs +++ b/codex-rs/windows-sandbox-rs/src/identity.rs @@ -1,12 +1,13 @@ use crate::dpapi; use crate::logging::debug_log; use crate::policy::SandboxPolicy; +use crate::resolved_permissions::ResolvedWindowsSandboxPermissions; use crate::setup::SandboxNetworkIdentity; use crate::setup::SandboxUserRecord; use crate::setup::SandboxUsersFile; use crate::setup::SetupMarker; use crate::setup::gather_read_roots; -use crate::setup::gather_write_roots; +use crate::setup::gather_write_roots_for_permissions; use crate::setup::offline_proxy_settings_from_env; use crate::setup::run_elevated_setup; use crate::setup::run_setup_refresh_with_overrides; @@ -143,14 +144,16 @@ pub fn require_logon_sandbox_creds( deny_write_paths_override: &[PathBuf], proxy_enforced: bool, ) -> Result { + let permissions = + ResolvedWindowsSandboxPermissions::from_legacy_policy_for_cwd(policy, policy_cwd); let sandbox_dir = crate::setup::sandbox_dir(codex_home); let needed_read = read_roots_override .map(<[PathBuf]>::to_vec) .unwrap_or_else(|| gather_read_roots(command_cwd, policy, codex_home)); let needed_write = write_roots_override .map(<[PathBuf]>::to_vec) - .unwrap_or_else(|| gather_write_roots(policy, policy_cwd, command_cwd, env_map)); - let network_identity = SandboxNetworkIdentity::from_policy(policy, proxy_enforced); + .unwrap_or_else(|| gather_write_roots_for_permissions(&permissions, command_cwd, env_map)); + let network_identity = SandboxNetworkIdentity::from_permissions(&permissions, proxy_enforced); let desired_offline_proxy_settings = offline_proxy_settings_from_env(env_map, network_identity); // NOTE: Do not add CODEX_HOME/.sandbox to `needed_write`; it must remain non-writable by the // restricted capability token. The setup helper's `lock_sandbox_dir` is responsible for diff --git a/codex-rs/windows-sandbox-rs/src/lib.rs b/codex-rs/windows-sandbox-rs/src/lib.rs index d662a62db0..45e35b7be5 100644 --- a/codex-rs/windows-sandbox-rs/src/lib.rs +++ b/codex-rs/windows-sandbox-rs/src/lib.rs @@ -287,18 +287,21 @@ pub use stub::run_windows_sandbox_legacy_preflight; #[cfg(target_os = "windows")] mod windows_impl { + use super::acl::revoke_ace; use super::logging::log_failure; use super::logging::log_success; 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::SpawnPrepOptions; 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::spawn_prep::root_capability_sids; + use super::token::LocalSid; use anyhow::Result; use codex_utils_absolute_path::AbsolutePathBuf; use std::collections::HashMap; @@ -400,17 +403,20 @@ mod windows_impl { .collect::>(); let common = prepare_legacy_spawn_context( policy_json_or_preset, + sandbox_policy_cwd, codex_home, cwd, &mut env_map, &command, - /*inherit_path*/ false, - /*add_git_safe_directory*/ false, + SpawnPrepOptions { + inherit_path: false, + add_git_safe_directory: false, + }, )?; let policy = common.policy; let current_dir = common.current_dir; let logs_base_dir = common.logs_base_dir.as_deref(); - let is_workspace_write = common.is_workspace_write; + let uses_write_capabilities = common.uses_write_capabilities; if !policy.has_full_disk_read_access() { anyhow::bail!( "Restricted read-only access requires the elevated Windows sandbox backend" @@ -429,8 +435,9 @@ mod windows_impl { codex_home, ); let security = prepare_legacy_session_security(&policy, codex_home, cwd, capability_roots)?; - allow_null_device_for_workspace_write(is_workspace_write); - apply_legacy_session_acl_rules( + allow_null_device_for_workspace_write(uses_write_capabilities); + let persist_aces = uses_write_capabilities; + let guards = apply_legacy_session_acl_rules( &policy, sandbox_policy_cwd, codex_home, @@ -443,6 +450,7 @@ mod windows_impl { 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); @@ -467,6 +475,13 @@ mod windows_impl { CloseHandle(out_w); CloseHandle(err_r); CloseHandle(err_w); + 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); @@ -567,6 +582,16 @@ mod windows_impl { log_failure(&command, &format!("exit code {exit_code}"), logs_base_dir); } + if !persist_aces { + 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, stdout, @@ -597,7 +622,7 @@ mod windows_impl { codex_home, ); let write_root_sids = root_capability_sids(codex_home, cwd, capability_roots)?; - apply_legacy_session_acl_rules( + let _guards = apply_legacy_session_acl_rules( sandbox_policy, sandbox_policy_cwd, codex_home, @@ -610,6 +635,7 @@ mod windows_impl { readonly_sid_str: None, write_root_sids: &write_root_sids, }, + /*persist_aces*/ true, )?; Ok(()) @@ -618,7 +644,8 @@ mod windows_impl { #[cfg(test)] mod tests { use crate::policy::SandboxPolicy; - use crate::spawn_prep::should_apply_network_block; + use crate::resolved_permissions::ResolvedWindowsSandboxPermissions; + use std::path::Path; fn workspace_policy(network_access: bool) -> SandboxPolicy { SandboxPolicy::WorkspaceWrite { @@ -629,6 +656,11 @@ mod windows_impl { } } + fn should_apply_network_block(policy: &SandboxPolicy) -> bool { + ResolvedWindowsSandboxPermissions::from_legacy_policy_for_cwd(policy, Path::new(".")) + .should_apply_network_block() + } + #[test] fn applies_network_block_when_access_is_disabled() { assert!(should_apply_network_block(&workspace_policy( diff --git a/codex-rs/windows-sandbox-rs/src/resolved_permissions.rs b/codex-rs/windows-sandbox-rs/src/resolved_permissions.rs index 5c6b0cdffe..6a43c99d8f 100644 --- a/codex-rs/windows-sandbox-rs/src/resolved_permissions.rs +++ b/codex-rs/windows-sandbox-rs/src/resolved_permissions.rs @@ -56,16 +56,10 @@ pub fn token_mode_for_permission_profile( } impl ResolvedWindowsSandboxPermissions { - pub(crate) fn from_legacy_policy(policy: &SandboxPolicy) -> Self { - Self { - file_system: FileSystemSandboxPolicy::from(policy), - network: NetworkSandboxPolicy::from(policy), - } - } - pub(crate) fn from_legacy_policy_for_cwd(policy: &SandboxPolicy, cwd: &Path) -> Self { Self { - file_system: FileSystemSandboxPolicy::from_legacy_sandbox_policy_for_cwd(policy, cwd), + file_system: FileSystemSandboxPolicy::from_legacy_sandbox_policy_for_cwd(policy, cwd) + .materialize_project_roots_with_cwd(cwd), network: NetworkSandboxPolicy::from(policy), } } @@ -94,6 +88,18 @@ impl ResolvedWindowsSandboxPermissions { !self.network.is_enabled() } + pub(crate) fn network_policy(&self) -> NetworkSandboxPolicy { + self.network + } + + pub(crate) fn uses_write_capabilities_for_cwd( + &self, + cwd: &Path, + env_map: &HashMap, + ) -> bool { + !self.writable_roots_for_cwd(cwd, env_map).is_empty() + } + pub(crate) fn writable_roots_for_cwd( &self, cwd: &Path, @@ -207,6 +213,34 @@ mod tests { assert_eq!(expected_roots, roots); } + #[test] + fn legacy_workspace_root_stays_bound_to_policy_cwd() { + let tmp = TempDir::new().expect("tempdir"); + let policy_cwd = tmp.path().join("workspace"); + let command_cwd = policy_cwd.join("subdir"); + std::fs::create_dir_all(&command_cwd).expect("create command cwd"); + + let policy = SandboxPolicy::WorkspaceWrite { + writable_roots: Vec::new(), + network_access: false, + exclude_tmpdir_env_var: true, + exclude_slash_tmp: true, + }; + let permissions = + ResolvedWindowsSandboxPermissions::from_legacy_policy_for_cwd(&policy, &policy_cwd); + + let roots = permissions + .writable_roots_for_cwd(&command_cwd, &HashMap::new()) + .into_iter() + .map(|root| root.root) + .collect::>(); + + assert_eq!( + roots, + vec![dunce::canonicalize(&policy_cwd).expect("canonical policy cwd")] + ); + } + #[test] fn token_mode_for_profile_without_writable_roots_uses_readonly_capability() { let tmp = TempDir::new().expect("tempdir"); diff --git a/codex-rs/windows-sandbox-rs/src/setup.rs b/codex-rs/windows-sandbox-rs/src/setup.rs index 6f4dd86019..353b9efeb5 100644 --- a/codex-rs/windows-sandbox-rs/src/setup.rs +++ b/codex-rs/windows-sandbox-rs/src/setup.rs @@ -17,6 +17,7 @@ use crate::logging::log_note; use crate::path_normalization::canonical_path_key; use crate::path_normalization::canonicalize_path; use crate::policy::SandboxPolicy; +use crate::resolved_permissions::ResolvedWindowsSandboxPermissions; use crate::setup_error::SetupErrorCode; use crate::setup_error::SetupFailure; use crate::setup_error::clear_setup_error_report; @@ -173,8 +174,12 @@ fn run_setup_refresh_inner( let (read_roots, write_roots) = build_payload_roots(&request, &overrides); let deny_read_paths = build_payload_deny_read_paths(overrides.deny_read_paths); let deny_write_paths = build_payload_deny_write_paths(&request, overrides.deny_write_paths); + let permissions = ResolvedWindowsSandboxPermissions::from_legacy_policy_for_cwd( + request.policy, + request.policy_cwd, + ); let network_identity = - SandboxNetworkIdentity::from_policy(request.policy, request.proxy_enforced); + SandboxNetworkIdentity::from_permissions(&permissions, request.proxy_enforced); let offline_proxy_settings = offline_proxy_settings_from_env(request.env_map, network_identity); let payload = ElevationPayload { version: SETUP_VERSION, @@ -389,15 +394,16 @@ pub(crate) fn gather_read_roots( gather_legacy_full_read_roots(command_cwd, policy, codex_home) } -pub(crate) fn gather_write_roots( - policy: &SandboxPolicy, - policy_cwd: &Path, +pub(crate) fn gather_write_roots_for_permissions( + permissions: &ResolvedWindowsSandboxPermissions, command_cwd: &Path, env_map: &HashMap, ) -> Vec { - let AllowDenyPaths { allow, .. } = - compute_allow_paths(policy, policy_cwd, command_cwd, env_map); - let roots: Vec = allow.into_iter().collect(); + let roots = permissions + .writable_roots_for_cwd(command_cwd, env_map) + .into_iter() + .map(|root| root.root) + .collect::>(); let mut dedup: HashSet = HashSet::new(); let mut out: Vec = Vec::new(); for r in canonical_existing(&roots) { @@ -415,11 +421,29 @@ pub(crate) fn effective_write_roots_for_setup( env_map: &HashMap, codex_home: &Path, write_roots_override: Option<&[PathBuf]>, +) -> Vec { + let permissions = + ResolvedWindowsSandboxPermissions::from_legacy_policy_for_cwd(policy, policy_cwd); + effective_write_roots_for_permissions( + &permissions, + command_cwd, + env_map, + codex_home, + write_roots_override, + ) +} + +pub(crate) fn effective_write_roots_for_permissions( + permissions: &ResolvedWindowsSandboxPermissions, + 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) + gather_write_roots_for_permissions(permissions, command_cwd, env_map) }; let write_roots = expand_user_profile_root(write_roots); let write_roots = filter_user_profile_root(write_roots); @@ -463,8 +487,11 @@ pub(crate) enum SandboxNetworkIdentity { } impl SandboxNetworkIdentity { - pub(crate) fn from_policy(policy: &SandboxPolicy, proxy_enforced: bool) -> Self { - if proxy_enforced || !policy.has_full_network_access() { + pub(crate) fn from_permissions( + permissions: &ResolvedWindowsSandboxPermissions, + proxy_enforced: bool, + ) -> Self { + if proxy_enforced || !permissions.network_policy().is_enabled() { Self::Offline } else { Self::Online @@ -744,8 +771,12 @@ pub fn run_elevated_setup( let (read_roots, write_roots) = build_payload_roots(&request, &overrides); let deny_read_paths = build_payload_deny_read_paths(overrides.deny_read_paths); let deny_write_paths = build_payload_deny_write_paths(&request, overrides.deny_write_paths); + let permissions = ResolvedWindowsSandboxPermissions::from_legacy_policy_for_cwd( + request.policy, + request.policy_cwd, + ); let network_identity = - SandboxNetworkIdentity::from_policy(request.policy, request.proxy_enforced); + SandboxNetworkIdentity::from_permissions(&permissions, request.proxy_enforced); let offline_proxy_settings = offline_proxy_settings_from_env(request.env_map, network_identity); let payload = ElevationPayload { version: SETUP_VERSION, @@ -1473,6 +1504,37 @@ mod tests { assert!(!effective_write_roots.contains(&forbidden_sandbox)); } + #[test] + fn effective_write_roots_use_policy_cwd_for_legacy_workspace_root() { + let tmp = TempDir::new().expect("tempdir"); + let codex_home = tmp.path().join("codex-home"); + let policy_cwd = tmp.path().join("workspace"); + let command_cwd = policy_cwd.join("subdir"); + fs::create_dir_all(&codex_home).expect("create codex home"); + fs::create_dir_all(&command_cwd).expect("create command cwd"); + + let policy = SandboxPolicy::WorkspaceWrite { + writable_roots: vec![], + network_access: false, + exclude_tmpdir_env_var: true, + exclude_slash_tmp: true, + }; + + let effective_write_roots = super::effective_write_roots_for_setup( + &policy, + &policy_cwd, + &command_cwd, + &HashMap::new(), + &codex_home, + /*write_roots_override*/ None, + ); + + assert_eq!( + effective_write_roots, + vec![dunce::canonicalize(&policy_cwd).expect("canonical policy cwd")] + ); + } + #[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 8df1ba804f..cf73ae3e7c 100644 --- a/codex-rs/windows-sandbox-rs/src/spawn_prep.rs +++ b/codex-rs/windows-sandbox-rs/src/spawn_prep.rs @@ -3,11 +3,13 @@ use crate::acl::add_deny_write_ace; use crate::acl::allow_null_device; use crate::allow::AllowDenyPaths; use crate::allow::compute_allow_paths; +use crate::allow::compute_allow_paths_for_permissions; 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::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; use crate::env::ensure_non_interactive_pager; @@ -22,7 +24,7 @@ use crate::policy::parse_policy; use crate::resolved_permissions::ResolvedWindowsSandboxPermissions; use crate::sandbox_utils::ensure_codex_home_exists; use crate::sandbox_utils::inject_git_safe_directory; -use crate::setup::effective_write_roots_for_setup; +use crate::setup::effective_write_roots_for_permissions; use crate::token::LocalSid; use crate::token::create_readonly_token_with_cap; use crate::token::create_workspace_write_token_with_caps_from; @@ -42,10 +44,11 @@ use windows_sys::Win32::Foundation::HANDLE; pub(crate) struct SpawnContext { pub(crate) policy: SandboxPolicy, + pub(crate) permissions: ResolvedWindowsSandboxPermissions, pub(crate) current_dir: PathBuf, pub(crate) sandbox_base: PathBuf, pub(crate) logs_base_dir: Option, - pub(crate) is_workspace_write: bool, + pub(crate) uses_write_capabilities: bool, } pub(crate) struct ElevatedSpawnContext { @@ -54,6 +57,12 @@ pub(crate) struct ElevatedSpawnContext { pub(crate) cap_sids: Vec, } +#[derive(Debug, Clone, Copy)] +pub(crate) struct SpawnPrepOptions { + pub(crate) inherit_path: bool, + pub(crate) add_git_safe_directory: bool, +} + pub(crate) struct LegacySessionSecurity { pub(crate) h_token: HANDLE, pub(crate) readonly_sid: Option, @@ -73,18 +82,14 @@ pub(crate) struct LegacyAclSids<'a> { pub(crate) write_root_sids: &'a [RootCapabilitySid], } -pub(crate) fn should_apply_network_block(policy: &SandboxPolicy) -> bool { - ResolvedWindowsSandboxPermissions::from_legacy_policy(policy).should_apply_network_block() -} - fn prepare_spawn_context_common( policy_json_or_preset: &str, + policy_cwd: &Path, codex_home: &Path, cwd: &Path, env_map: &mut HashMap, command: &[String], - inherit_path: bool, - add_git_safe_directory: bool, + options: SpawnPrepOptions, ) -> Result { let policy = parse_policy(policy_json_or_preset)?; if matches!( @@ -96,10 +101,10 @@ fn prepare_spawn_context_common( normalize_null_device_env(env_map); ensure_non_interactive_pager(env_map); - if inherit_path { + if options.inherit_path { inherit_path_env(env_map); } - if add_git_safe_directory { + if options.add_git_safe_directory { inject_git_safe_directory(env_map, cwd); } @@ -109,36 +114,39 @@ fn prepare_spawn_context_common( let logs_base_dir = Some(sandbox_base.clone()); log_start(command, logs_base_dir.as_deref()); - let is_workspace_write = matches!(&policy, SandboxPolicy::WorkspaceWrite { .. }); + let permissions = + ResolvedWindowsSandboxPermissions::from_legacy_policy_for_cwd(&policy, policy_cwd); + let uses_write_capabilities = permissions.uses_write_capabilities_for_cwd(cwd, env_map); Ok(SpawnContext { policy, + permissions, current_dir: cwd.to_path_buf(), sandbox_base, logs_base_dir, - is_workspace_write, + uses_write_capabilities, }) } pub(crate) fn prepare_legacy_spawn_context( policy_json_or_preset: &str, + policy_cwd: &Path, codex_home: &Path, cwd: &Path, env_map: &mut HashMap, command: &[String], - inherit_path: bool, - add_git_safe_directory: bool, + options: SpawnPrepOptions, ) -> Result { let common = prepare_spawn_context_common( policy_json_or_preset, + policy_cwd, codex_home, cwd, env_map, command, - inherit_path, - add_git_safe_directory, + options, )?; - if should_apply_network_block(&common.policy) { + if common.permissions.should_apply_network_block() { apply_no_network_to_env(env_map)?; } Ok(common) @@ -195,14 +203,15 @@ pub(crate) fn legacy_session_capability_roots( env_map: &HashMap, codex_home: &Path, ) -> Vec { - let allow_paths = compute_allow_paths(policy, policy_cwd, current_dir, env_map) + let permissions = + ResolvedWindowsSandboxPermissions::from_legacy_policy_for_cwd(policy, policy_cwd); + let allow_paths = compute_allow_paths_for_permissions(&permissions, current_dir, env_map) .allow .into_iter() .collect::>(); - if matches!(policy, SandboxPolicy::WorkspaceWrite { .. }) { - effective_write_roots_for_setup( - policy, - policy_cwd, + if permissions.uses_write_capabilities_for_cwd(current_dir, env_map) { + effective_write_roots_for_permissions( + &permissions, current_dir, env_map, codex_home, @@ -283,9 +292,11 @@ pub(crate) fn apply_legacy_session_acl_rules( additional_deny_read_paths: &[PathBuf], additional_deny_write_paths: &[PathBuf], acl_sids: LegacyAclSids<'_>, -) -> Result<()> { + persist_aces: bool, +) -> Result> { let AllowDenyPaths { allow, mut deny } = compute_allow_paths(policy, sandbox_policy_cwd, current_dir, env_map); + 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 @@ -298,19 +309,31 @@ pub(crate) fn apply_legacy_session_acl_rules( } if let Some(readonly_sid) = acl_sids.readonly_sid { for p in &allow { - let _ = add_allow_ace(p, readonly_sid.as_ptr()); + 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; }; - let _ = add_allow_ace(p, root_sid.sid.as_ptr()); + 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 { for root_sid in deny_root_capabilities_for_path(p, acl_sids.write_root_sids) { - let _ = add_deny_write_ace(p, root_sid.sid.as_ptr()); + 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 !additional_deny_read_paths.is_empty() { @@ -318,20 +341,42 @@ pub(crate) fn apply_legacy_session_acl_rules( let Some(readonly_sid_str) = acl_sids.readonly_sid_str else { anyhow::bail!("readonly capability SID string missing"); }; - sync_persistent_deny_read_acls( - codex_home, - readonly_sid_str, - additional_deny_read_paths, - readonly_sid.as_ptr(), - )?; - } 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, + readonly_sid_str, additional_deny_read_paths, - root_sid.sid.as_ptr(), - )?; + 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())), + ); + } } } } @@ -341,7 +386,8 @@ pub(crate) fn apply_legacy_session_acl_rules( if let Some(readonly_sid) = acl_sids.readonly_sid { allow_null_device(readonly_sid.as_ptr()); } - if matches!(policy, SandboxPolicy::WorkspaceWrite { .. }) + if persist_aces + && !acl_sids.write_root_sids.is_empty() && let Some(workspace_sid) = matching_root_capability(current_dir, acl_sids.write_root_sids) { @@ -352,7 +398,7 @@ pub(crate) fn apply_legacy_session_acl_rules( } } } - Ok(()) + Ok(guards) } #[allow(clippy::too_many_arguments)] @@ -371,32 +417,30 @@ pub(crate) fn prepare_elevated_spawn_context( ) -> Result { let common = prepare_spawn_context_common( policy_json_or_preset, + sandbox_policy_cwd, codex_home, cwd, env_map, command, - /*inherit_path*/ true, - /*add_git_safe_directory*/ true, + SpawnPrepOptions { + inherit_path: true, + add_git_safe_directory: true, + }, )?; - let AllowDenyPaths { allow, deny } = compute_allow_paths( - &common.policy, - sandbox_policy_cwd, - &common.current_dir, - env_map, - ); + let AllowDenyPaths { allow, deny } = + compute_allow_paths_for_permissions(&common.permissions, &common.current_dir, env_map); let write_roots: Vec = allow.into_iter().collect(); let deny_write_paths: Vec = deny.into_iter().collect(); - let computed_write_roots_override = if common.is_workspace_write { + let computed_write_roots_override = if common.uses_write_capabilities { Some(write_roots.as_slice()) } 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, + let effective_write_roots = if common.uses_write_capabilities { + effective_write_roots_for_permissions( + &common.permissions, &common.current_dir, env_map, codex_home, @@ -405,7 +449,7 @@ pub(crate) fn prepare_elevated_spawn_context( } else { Vec::new() }; - let setup_write_roots_override = if common.is_workspace_write { + let setup_write_roots_override = if common.uses_write_capabilities { Some(effective_write_roots.as_slice()) } else { write_roots_override @@ -428,24 +472,20 @@ pub(crate) fn prepare_elevated_spawn_context( /*proxy_enforced*/ false, )?; let caps = load_or_create_cap_sids(codex_home)?; - let (psid_to_use, cap_sids) = match &common.policy { - SandboxPolicy::ReadOnly { .. } => ( + let (psid_to_use, cap_sids) = if common.uses_write_capabilities { + 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) + } else { + ( LocalSid::from_string(&caps.readonly)?, vec![caps.readonly.clone()], - ), - SandboxPolicy::WorkspaceWrite { .. } => { - 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") - } + ) }; unsafe { @@ -462,19 +502,26 @@ pub(crate) fn prepare_elevated_spawn_context( #[cfg(test)] mod tests { use super::SandboxPolicy; + use super::SpawnPrepOptions; 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 crate::resolved_permissions::ResolvedWindowsSandboxPermissions; use codex_utils_absolute_path::AbsolutePathBuf; use pretty_assertions::assert_eq; use std::collections::HashMap; + use std::path::Path; use tempfile::TempDir; + fn should_apply_network_block(policy: &SandboxPolicy) -> bool { + ResolvedWindowsSandboxPermissions::from_legacy_policy_for_cwd(policy, Path::new(".")) + .should_apply_network_block() + } + #[test] fn no_network_env_rewrite_applies_for_workspace_write() { assert!(should_apply_network_block( @@ -502,12 +549,15 @@ mod tests { let _context = prepare_legacy_spawn_context( "workspace-write", + cwd.path(), codex_home.path(), cwd.path(), &mut env_map, &["cmd.exe".to_string()], - /*inherit_path*/ true, - /*add_git_safe_directory*/ false, + SpawnPrepOptions { + inherit_path: true, + add_git_safe_directory: false, + }, ) .expect("legacy env prep"); @@ -529,12 +579,15 @@ mod tests { let context = prepare_spawn_context_common( "workspace-write", + cwd.path(), codex_home.path(), cwd.path(), &mut env_map, &["cmd.exe".to_string()], - /*inherit_path*/ true, - /*add_git_safe_directory*/ true, + SpawnPrepOptions { + inherit_path: true, + add_git_safe_directory: true, + }, ) .expect("preserve existing env prep"); assert_eq!(context.policy, SandboxPolicy::new_workspace_write_policy()); @@ -546,6 +599,36 @@ mod tests { ); } + #[test] + fn legacy_session_capability_roots_use_policy_cwd_for_workspace_root() { + let tmp = TempDir::new().expect("tempdir"); + let codex_home = tmp.path().join("codex-home"); + let policy_cwd = tmp.path().join("workspace"); + let command_cwd = policy_cwd.join("subdir"); + std::fs::create_dir_all(&codex_home).expect("create codex home"); + std::fs::create_dir_all(&command_cwd).expect("create command cwd"); + + let policy = SandboxPolicy::WorkspaceWrite { + writable_roots: Vec::new(), + network_access: false, + exclude_tmpdir_env_var: true, + exclude_slash_tmp: true, + }; + + let roots = legacy_session_capability_roots( + &policy, + &policy_cwd, + &command_cwd, + &HashMap::new(), + &codex_home, + ); + + assert_eq!( + roots, + vec![dunce::canonicalize(&policy_cwd).expect("canonical policy cwd")] + ); + } + #[test] fn root_capability_sids_only_include_active_roots() { let temp = TempDir::new().expect("tempdir"); 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 2cb12bfd71..64ec2ced6e 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,5 +1,6 @@ use super::windows_common::finish_driver_spawn; use super::windows_common::normalize_windows_tty_input; +use crate::acl::revoke_ace; use crate::conpty::ConptyInstance; use crate::conpty::spawn_conpty_process_as_user; use crate::desktop::LaunchDesktop; @@ -10,11 +11,13 @@ use crate::process::StdinMode; use crate::process::read_handle_loop; use crate::process::spawn_process_with_pipes; use crate::spawn_prep::LegacyAclSids; +use crate::spawn_prep::SpawnPrepOptions; 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; @@ -22,6 +25,7 @@ use codex_utils_pty::SpawnedProcess; use codex_utils_pty::TerminalSize; use std::collections::HashMap; use std::path::Path; +use std::path::PathBuf; use std::ptr; use std::sync::Arc; use std::sync::Mutex as StdMutex; @@ -203,6 +207,7 @@ fn finalize_exit( process_handle: Arc>>, thread_handle: HANDLE, output_join: std::thread::JoinHandle<()>, + guards: Vec<(PathBuf, String)>, logs_base_dir: Option<&Path>, command: Vec, ) { @@ -238,6 +243,14 @@ fn finalize_exit( } else { log_failure(&command, &format!("exit code {exit_code}"), logs_base_dir); } + + unsafe { + for (path, cap_sid) in guards { + if let Ok(sid) = LocalSid::from_string(&cap_sid) { + revoke_ace(&path, sid.as_ptr()); + } + } + } } fn resize_conpty_handle(hpc: &Arc>>, size: TerminalSize) -> Result<()> { @@ -283,12 +296,15 @@ pub(crate) async fn spawn_windows_sandbox_session_legacy( ) -> Result { let common = prepare_legacy_spawn_context( policy_json_or_preset, + sandbox_policy_cwd, codex_home, cwd, &mut env_map, &command, - /*inherit_path*/ false, - /*add_git_safe_directory*/ false, + SpawnPrepOptions { + inherit_path: false, + add_git_safe_directory: false, + }, )?; if !common.policy.has_full_disk_read_access() { anyhow::bail!("Restricted read-only access requires the elevated Windows sandbox backend"); @@ -311,9 +327,10 @@ pub(crate) async fn spawn_windows_sandbox_session_legacy( ); let security = prepare_legacy_session_security(&common.policy, codex_home, cwd, capability_roots)?; - allow_null_device_for_workspace_write(common.is_workspace_write); + allow_null_device_for_workspace_write(common.uses_write_capabilities); - apply_legacy_session_acl_rules( + let persist_aces = common.uses_write_capabilities; + let guards = apply_legacy_session_acl_rules( &common.policy, sandbox_policy_cwd, codex_home, @@ -326,6 +343,7 @@ pub(crate) async fn spawn_windows_sandbox_session_legacy( readonly_sid_str: security.readonly_sid_str.as_deref(), write_root_sids: &security.write_root_sids, }, + persist_aces, )?; let (writer_tx, writer_rx) = mpsc::channel::>(128); @@ -361,6 +379,13 @@ pub(crate) async fn spawn_windows_sandbox_session_legacy( Ok(handles) => handles, Err(err) => { unsafe { + 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); } return Err(err); @@ -371,6 +396,7 @@ pub(crate) async fn spawn_windows_sandbox_session_legacy( let process_handle = Arc::new(StdMutex::new(Some(pi.hProcess))); 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 hpc_for_wait = hpc_handle.clone(); std::thread::spawn(move || { let _desktop = desktop; @@ -401,6 +427,7 @@ pub(crate) async fn spawn_windows_sandbox_session_legacy( wait_handle, pi.hThread, output_join, + guards_for_wait, common.logs_base_dir.as_deref(), command_for_wait, );