diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 6320bff2e39..322d92b7a9b 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -2023,12 +2023,6 @@ impl Config { } }) .map_err(std::io::Error::from)?; - - if cfg!(target_os = "windows") { - startup_warnings.push(format!( - "managed filesystem deny_read from {filesystem_requirements_source} is only enforced for direct file tools on Windows; shell subprocess reads are not sandboxed" - )); - } } apply_requirement_constrained_value( "approvals_reviewer", diff --git a/codex-rs/core/src/exec.rs b/codex-rs/core/src/exec.rs index 396b7701bec..c6f16ed91cd 100644 --- a/codex-rs/core/src/exec.rs +++ b/codex-rs/core/src/exec.rs @@ -96,15 +96,17 @@ pub struct ExecParams { /// Resolved filesystem overrides for the Windows sandbox backends. /// -/// The unelevated restricted-token backend only consumes extra deny-write -/// carveouts on top of the legacy `WorkspaceWrite` allow set. The elevated -/// backend can also consume explicit read and write roots during setup/refresh. -/// Read-root overrides are layered on top of the baseline helper/platform roots -/// that the elevated setup path needs to launch the sandboxed command. +/// Both Windows backends consume extra deny-read paths, and the unelevated +/// restricted-token backend also consumes extra deny-write carveouts on top of +/// the legacy `WorkspaceWrite` allow set. The elevated backend can also consume +/// explicit read and write roots during setup/refresh. Read-root overrides are +/// layered on top of the baseline helper/platform roots that the elevated setup +/// path needs to launch the sandboxed command. #[derive(Debug, Clone, PartialEq, Eq)] pub(crate) struct WindowsSandboxFilesystemOverrides { pub(crate) read_roots_override: Option>, pub(crate) write_roots_override: Option>, + pub(crate) additional_deny_read_paths: Vec, pub(crate) additional_deny_write_paths: Vec, } @@ -490,7 +492,7 @@ async fn exec_windows_sandbox( ) -> Result { use crate::config::find_codex_home; use codex_windows_sandbox::run_windows_sandbox_capture_elevated; - use codex_windows_sandbox::run_windows_sandbox_capture_with_extra_deny_write_paths; + use codex_windows_sandbox::run_windows_sandbox_capture_with_filesystem_overrides; let ExecParams { command, @@ -531,27 +533,21 @@ async fn exec_windows_sandbox( let proxy_enforced = network.is_some(); let use_elevated = windows_sandbox_uses_elevated_backend(sandbox_level, proxy_enforced); let additional_deny_write_paths = windows_sandbox_filesystem_overrides - .map(|overrides| { - overrides - .additional_deny_write_paths - .iter() - .map(AbsolutePathBuf::to_path_buf) - .collect::>() - }) - .unwrap_or_default(); + .map(|overrides| overrides.additional_deny_write_paths.clone()) + .unwrap_or_default() + .into_iter() + .map(AbsolutePathBuf::into_path_buf) + .collect::>(); + let additional_deny_read_paths = windows_sandbox_filesystem_overrides + .map(|overrides| overrides.additional_deny_read_paths.clone()) + .unwrap_or_default() + .into_iter() + .map(AbsolutePathBuf::into_path_buf) + .collect::>(); let elevated_read_roots_override = windows_sandbox_filesystem_overrides .and_then(|overrides| overrides.read_roots_override.clone()); let elevated_write_roots_override = windows_sandbox_filesystem_overrides .and_then(|overrides| overrides.write_roots_override.clone()); - let elevated_deny_write_paths = windows_sandbox_filesystem_overrides - .map(|overrides| { - overrides - .additional_deny_write_paths - .iter() - .map(AbsolutePathBuf::to_path_buf) - .collect::>() - }) - .unwrap_or_default(); let spawn_res = tokio::task::spawn_blocking(move || { if use_elevated { run_windows_sandbox_capture_elevated( @@ -567,11 +563,12 @@ async fn exec_windows_sandbox( proxy_enforced, read_roots_override: elevated_read_roots_override.as_deref(), write_roots_override: elevated_write_roots_override.as_deref(), - deny_write_paths_override: &elevated_deny_write_paths, + deny_read_paths_override: &additional_deny_read_paths, + deny_write_paths_override: &additional_deny_write_paths, }, ) } else { - run_windows_sandbox_capture_with_extra_deny_write_paths( + run_windows_sandbox_capture_with_filesystem_overrides( policy_str.as_str(), &sandbox_cwd, codex_home.as_ref(), @@ -579,6 +576,7 @@ async fn exec_windows_sandbox( &cwd, env, timeout_ms, + &additional_deny_read_paths, &additional_deny_write_paths, windows_sandbox_private_desktop, ) @@ -971,25 +969,21 @@ pub(crate) fn resolve_windows_restricted_token_filesystem_overrides( )); } - if !file_system_sandbox_policy.has_full_disk_read_access() { + // The restricted-token backend can add deny ACL overlays, but it cannot + // synthesize a smaller read allowlist. A policy that leaves the filesystem + // root readable and only adds deny-read carveouts is therefore enforceable; + // a policy that removes root read access is not. + if !windows_policy_has_root_read_access(file_system_sandbox_policy, sandbox_policy_cwd) { return Err( "windows unelevated restricted-token sandbox cannot enforce split filesystem read restrictions directly; refusing to run unsandboxed" .to_string(), ); } - if !file_system_sandbox_policy - .get_unreadable_roots_with_cwd(sandbox_policy_cwd) - .is_empty() - || !file_system_sandbox_policy - .get_unreadable_globs_with_cwd(sandbox_policy_cwd) - .is_empty() - { - return Err( - "windows unelevated restricted-token sandbox cannot enforce unreadable split filesystem carveouts directly; refusing to run unsandboxed" - .to_string(), - ); - } + let additional_deny_read_paths = codex_windows_sandbox::resolve_windows_deny_read_paths( + file_system_sandbox_policy, + sandbox_policy_cwd, + )?; let legacy_writable_roots = sandbox_policy.get_writable_roots_with_cwd(sandbox_policy_cwd); let split_writable_roots = @@ -1053,13 +1047,14 @@ pub(crate) fn resolve_windows_restricted_token_filesystem_overrides( } } - if additional_deny_write_paths.is_empty() { + if additional_deny_read_paths.is_empty() && additional_deny_write_paths.is_empty() { return Ok(None); } Ok(Some(WindowsSandboxFilesystemOverrides { read_roots_override: None, write_roots_override: None, + additional_deny_read_paths, additional_deny_write_paths: additional_deny_write_paths .into_iter() .map(|path| AbsolutePathBuf::from_absolute_path(path).map_err(|err| err.to_string())) @@ -1073,6 +1068,16 @@ fn normalize_windows_override_path(path: &Path) -> std::result::Result bool { + let Some(root) = cwd.as_path().ancestors().last() else { + return false; + }; + file_system_sandbox_policy.can_read_path_with_cwd(root, cwd.as_path()) +} + pub(crate) fn resolve_windows_elevated_filesystem_overrides( sandbox: SandboxType, sandbox_policy: &SandboxPolicy, @@ -1096,18 +1101,10 @@ pub(crate) fn resolve_windows_elevated_filesystem_overrides( )); } - if !file_system_sandbox_policy - .get_unreadable_roots_with_cwd(sandbox_policy_cwd) - .is_empty() - || !file_system_sandbox_policy - .get_unreadable_globs_with_cwd(sandbox_policy_cwd) - .is_empty() - { - return Err( - "windows elevated sandbox cannot enforce unreadable split filesystem carveouts directly; refusing to run unsandboxed" - .to_string(), - ); - } + let additional_deny_read_paths = codex_windows_sandbox::resolve_windows_deny_read_paths( + file_system_sandbox_policy, + sandbox_policy_cwd, + )?; let split_writable_roots = file_system_sandbox_policy.get_writable_roots_with_cwd(sandbox_policy_cwd); @@ -1145,11 +1142,16 @@ pub(crate) fn resolve_windows_elevated_filesystem_overrides( .collect(); let split_root_path_set: BTreeSet = split_root_paths.iter().cloned().collect(); - let matches_legacy_read_access = file_system_sandbox_policy.has_full_disk_read_access() - == sandbox_policy.has_full_disk_read_access(); + // `has_full_disk_read_access()` is intentionally false when deny-read + // entries exist. For Windows setup overrides, the important question is + // whether the baseline still reads from the filesystem root and only needs + // additional deny ACLs layered on top. + let split_has_root_read_access = + windows_policy_has_root_read_access(file_system_sandbox_policy, sandbox_policy_cwd); + let matches_legacy_read_access = + split_has_root_read_access == sandbox_policy.has_full_disk_read_access(); let read_roots_override = if matches_legacy_read_access - && (file_system_sandbox_policy.has_full_disk_read_access() - || split_readable_root_set == legacy_readable_root_set) + && (split_has_root_read_access || split_readable_root_set == legacy_readable_root_set) { None } else { @@ -1198,6 +1200,7 @@ pub(crate) fn resolve_windows_elevated_filesystem_overrides( if read_roots_override.is_none() && write_roots_override.is_none() + && additional_deny_read_paths.is_empty() && additional_deny_write_paths.is_empty() { return Ok(None); @@ -1206,6 +1209,7 @@ pub(crate) fn resolve_windows_elevated_filesystem_overrides( Ok(Some(WindowsSandboxFilesystemOverrides { read_roots_override, write_roots_override, + additional_deny_read_paths, additional_deny_write_paths, })) } diff --git a/codex-rs/core/src/exec_tests.rs b/codex-rs/core/src/exec_tests.rs index 1cfa87ff3f6..b5c0df621dd 100644 --- a/codex-rs/core/src/exec_tests.rs +++ b/codex-rs/core/src/exec_tests.rs @@ -659,11 +659,66 @@ fn windows_restricted_token_supports_full_read_split_write_read_carveouts() { Ok(Some(WindowsSandboxFilesystemOverrides { read_roots_override: None, write_roots_override: None, + additional_deny_read_paths: vec![], additional_deny_write_paths: expected_deny_write_paths, })) ); } +#[test] +fn windows_restricted_token_supports_unreadable_split_carveouts() { + let temp_dir = tempfile::TempDir::new().expect("tempdir"); + let cwd = dunce::canonicalize(temp_dir.path()) + .expect("canonicalize temp dir") + .abs(); + let blocked = cwd.join("blocked"); + std::fs::create_dir_all(blocked.as_path()).expect("create blocked"); + let policy = SandboxPolicy::WorkspaceWrite { + writable_roots: vec![], + read_only_access: codex_protocol::protocol::ReadOnlyAccess::FullAccess, + network_access: false, + exclude_tmpdir_env_var: true, + exclude_slash_tmp: true, + }; + let file_system_policy = FileSystemSandboxPolicy::restricted(vec![ + codex_protocol::permissions::FileSystemSandboxEntry { + path: codex_protocol::permissions::FileSystemPath::Special { + value: codex_protocol::permissions::FileSystemSpecialPath::Root, + }, + access: codex_protocol::permissions::FileSystemAccessMode::Read, + }, + codex_protocol::permissions::FileSystemSandboxEntry { + path: codex_protocol::permissions::FileSystemPath::Special { + value: codex_protocol::permissions::FileSystemSpecialPath::CurrentWorkingDirectory, + }, + access: codex_protocol::permissions::FileSystemAccessMode::Write, + }, + codex_protocol::permissions::FileSystemSandboxEntry { + path: codex_protocol::permissions::FileSystemPath::Path { + path: blocked.clone(), + }, + access: codex_protocol::permissions::FileSystemAccessMode::None, + }, + ]); + + assert_eq!( + resolve_windows_restricted_token_filesystem_overrides( + SandboxType::WindowsRestrictedToken, + &policy, + &file_system_policy, + NetworkSandboxPolicy::Restricted, + &cwd, + WindowsSandboxLevel::RestrictedToken, + ), + Ok(Some(WindowsSandboxFilesystemOverrides { + read_roots_override: None, + write_roots_override: None, + additional_deny_read_paths: vec![blocked.clone()], + additional_deny_write_paths: vec![blocked], + })) + ); +} + #[test] fn windows_elevated_supports_split_restricted_read_roots() { let temp_dir = tempfile::TempDir::new().expect("tempdir"); @@ -696,6 +751,7 @@ fn windows_elevated_supports_split_restricted_read_roots() { Ok(Some(WindowsSandboxFilesystemOverrides { read_roots_override: Some(vec![expected_docs]), write_roots_override: None, + additional_deny_read_paths: vec![], additional_deny_write_paths: vec![], })) ); @@ -748,6 +804,7 @@ fn windows_elevated_supports_split_write_read_carveouts() { Ok(Some(WindowsSandboxFilesystemOverrides { read_roots_override: None, write_roots_override: None, + additional_deny_read_paths: vec![], additional_deny_write_paths: vec![ codex_utils_absolute_path::AbsolutePathBuf::from_absolute_path(expected_docs) .expect("absolute docs"), @@ -757,10 +814,11 @@ fn windows_elevated_supports_split_write_read_carveouts() { } #[test] -fn windows_elevated_rejects_unreadable_split_carveouts() { +fn windows_elevated_supports_unreadable_split_carveouts() { let temp_dir = tempfile::TempDir::new().expect("tempdir"); let blocked = temp_dir.path().join("blocked"); std::fs::create_dir_all(&blocked).expect("create blocked"); + let expected_blocked = dunce::canonicalize(&blocked).expect("canonical blocked"); let policy = SandboxPolicy::WorkspaceWrite { writable_roots: vec![], read_only_access: codex_protocol::protocol::ReadOnlyAccess::FullAccess, @@ -791,24 +849,37 @@ fn windows_elevated_rejects_unreadable_split_carveouts() { ]); assert_eq!( - unsupported_windows_restricted_token_sandbox_reason( + resolve_windows_elevated_filesystem_overrides( SandboxType::WindowsRestrictedToken, &policy, &file_system_policy, NetworkSandboxPolicy::Restricted, &temp_dir.path().abs(), - WindowsSandboxLevel::Elevated, + /*use_windows_elevated_backend*/ true, ), - Some( - "windows elevated sandbox cannot enforce unreadable split filesystem carveouts directly; refusing to run unsandboxed" - .to_string() - ) + Ok(Some(WindowsSandboxFilesystemOverrides { + read_roots_override: None, + write_roots_override: None, + additional_deny_read_paths: vec![ + codex_utils_absolute_path::AbsolutePathBuf::from_absolute_path( + expected_blocked.clone(), + ) + .expect("absolute blocked"), + ], + additional_deny_write_paths: vec![ + codex_utils_absolute_path::AbsolutePathBuf::from_absolute_path(expected_blocked) + .expect("absolute blocked"), + ], + })) ); } #[test] -fn windows_elevated_rejects_unreadable_globs() { +fn windows_elevated_supports_unreadable_globs() { let temp_dir = tempfile::TempDir::new().expect("tempdir"); + let secret = temp_dir.path().join("app").join(".env"); + std::fs::create_dir_all(secret.parent().expect("parent")).expect("create parent"); + std::fs::write(&secret, "secret").expect("write secret"); let policy = SandboxPolicy::WorkspaceWrite { writable_roots: vec![], read_only_access: codex_protocol::protocol::ReadOnlyAccess::FullAccess, @@ -838,18 +909,23 @@ fn windows_elevated_rejects_unreadable_globs() { ]); assert_eq!( - unsupported_windows_restricted_token_sandbox_reason( + resolve_windows_elevated_filesystem_overrides( SandboxType::WindowsRestrictedToken, &policy, &file_system_policy, NetworkSandboxPolicy::Restricted, &temp_dir.path().abs(), - WindowsSandboxLevel::Elevated, + /*use_windows_elevated_backend*/ true, ), - Some( - "windows elevated sandbox cannot enforce unreadable split filesystem carveouts directly; refusing to run unsandboxed" - .to_string() - ) + Ok(Some(WindowsSandboxFilesystemOverrides { + read_roots_override: None, + write_roots_override: None, + additional_deny_read_paths: vec![ + codex_utils_absolute_path::AbsolutePathBuf::from_absolute_path(secret) + .expect("absolute secret"), + ], + additional_deny_write_paths: vec![], + })) ); } diff --git a/codex-rs/core/tests/suite/mod.rs b/codex-rs/core/tests/suite/mod.rs index 9f8389c45e2..b6dc78577d5 100644 --- a/codex-rs/core/tests/suite/mod.rs +++ b/codex-rs/core/tests/suite/mod.rs @@ -110,3 +110,5 @@ mod view_image; mod web_search; mod websocket_fallback; mod window_headers; +#[cfg(target_os = "windows")] +mod windows_sandbox; diff --git a/codex-rs/core/tests/suite/windows_sandbox.rs b/codex-rs/core/tests/suite/windows_sandbox.rs new file mode 100644 index 00000000000..716769757d9 --- /dev/null +++ b/codex-rs/core/tests/suite/windows_sandbox.rs @@ -0,0 +1,131 @@ +#![cfg(target_os = "windows")] + +use codex_core::exec::ExecCapturePolicy; +use codex_core::exec::ExecParams; +use codex_core::exec::process_exec_tool_call; +use codex_core::sandboxing::SandboxPermissions; +use codex_protocol::config_types::WindowsSandboxLevel; +use codex_protocol::permissions::FileSystemAccessMode; +use codex_protocol::permissions::FileSystemPath; +use codex_protocol::permissions::FileSystemSandboxEntry; +use codex_protocol::permissions::FileSystemSandboxPolicy; +use codex_protocol::permissions::FileSystemSpecialPath; +use codex_protocol::permissions::NetworkSandboxPolicy; +use codex_protocol::protocol::ReadOnlyAccess; +use codex_protocol::protocol::SandboxPolicy; +use core_test_support::PathExt; +use pretty_assertions::assert_eq; +use serial_test::serial; +use std::collections::HashMap; +use std::ffi::OsString; +use tempfile::TempDir; + +struct EnvVarGuard { + key: &'static str, + original: Option, +} + +impl EnvVarGuard { + fn set(key: &'static str, value: &std::ffi::OsStr) -> Self { + let original = std::env::var_os(key); + unsafe { + std::env::set_var(key, value); + } + Self { key, original } + } +} + +impl Drop for EnvVarGuard { + fn drop(&mut self) { + unsafe { + match &self.original { + Some(value) => std::env::set_var(self.key, value), + None => std::env::remove_var(self.key), + } + } + } +} + +#[tokio::test] +#[serial] +async fn windows_restricted_token_enforces_exact_and_glob_deny_read_policy() -> anyhow::Result<()> { + let temp_home = TempDir::new()?; + let _codex_home_guard = EnvVarGuard::set("CODEX_HOME", temp_home.path().as_os_str()); + let workspace = TempDir::new()?; + let cwd = workspace.path().abs(); + let secret = workspace.path().join("secret.env"); + let future_secret = cwd.join("future.env"); + let public = workspace.path().join("public.txt"); + std::fs::write(&secret, "glob secret\n")?; + std::fs::write(&public, "public ok\n")?; + + let sandbox_policy = SandboxPolicy::WorkspaceWrite { + writable_roots: vec![], + read_only_access: ReadOnlyAccess::FullAccess, + network_access: false, + exclude_tmpdir_env_var: true, + exclude_slash_tmp: true, + }; + let file_system_sandbox_policy = FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: FileSystemSpecialPath::Root, + }, + access: FileSystemAccessMode::Read, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: FileSystemSpecialPath::CurrentWorkingDirectory, + }, + access: FileSystemAccessMode::Write, + }, + FileSystemSandboxEntry { + path: FileSystemPath::GlobPattern { + pattern: "**/*.env".to_string(), + }, + access: FileSystemAccessMode::None, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { + path: future_secret, + }, + access: FileSystemAccessMode::None, + }, + ]); + + let output = process_exec_tool_call( + ExecParams { + command: vec![ + "cmd.exe".to_string(), + "/D".to_string(), + "/C".to_string(), + "type secret.env >NUL 2>NUL & echo exact secret 1>future.env 2>NUL & type future.env 2>NUL & type public.txt & exit /B 0" + .to_string(), + ], + cwd: cwd.clone(), + expiration: 10_000.into(), + capture_policy: ExecCapturePolicy::ShellTool, + env: HashMap::new(), + network: None, + sandbox_permissions: SandboxPermissions::UseDefault, + windows_sandbox_level: WindowsSandboxLevel::RestrictedToken, + windows_sandbox_private_desktop: false, + justification: None, + arg0: None, + }, + &sandbox_policy, + &file_system_sandbox_policy, + NetworkSandboxPolicy::Restricted, + &cwd, + &None, + /*use_legacy_landlock*/ false, + /*stdout_stream*/ None, + ) + .await?; + + assert_eq!(output.exit_code, 0); + assert!(output.stdout.text.contains("public ok")); + assert!(!output.stdout.text.contains("glob secret")); + assert!(!output.stdout.text.contains("exact secret")); + Ok(()) +} diff --git a/codex-rs/windows-sandbox-rs/src/acl.rs b/codex-rs/windows-sandbox-rs/src/acl.rs index f351dba190a..71f80cf60cd 100644 --- a/codex-rs/windows-sandbox-rs/src/acl.rs +++ b/codex-rs/windows-sandbox-rs/src/acl.rs @@ -22,6 +22,7 @@ use windows_sys::Win32::Security::EqualSid; use windows_sys::Win32::Security::GetAce; use windows_sys::Win32::Security::GetAclInformation; use windows_sys::Win32::Security::MapGenericMask; +use windows_sys::Win32::Security::ACCESS_DENIED_ACE; use windows_sys::Win32::Security::ACCESS_ALLOWED_ACE; use windows_sys::Win32::Security::ACE_HEADER; use windows_sys::Win32::Security::ACL; @@ -48,6 +49,9 @@ use windows_sys::Win32::Storage::FileSystem::READ_CONTROL; use windows_sys::Win32::Storage::FileSystem::DELETE; const SE_KERNEL_OBJECT: u32 = 6; const INHERIT_ONLY_ACE: u8 = 0x08; +const ACCESS_ALLOWED_ACE_TYPE: u8 = 0; +const ACCESS_DENIED_ACE_TYPE: u8 = 1; +const GENERIC_READ_MASK: u32 = 0x8000_0000; const GENERIC_WRITE_MASK: u32 = 0x4000_0000; const DENY_ACCESS: i32 = 3; @@ -125,7 +129,7 @@ pub unsafe fn dacl_mask_allows( continue; } let hdr = &*(p_ace as *const ACE_HEADER); - if hdr.AceType != 0 { + if hdr.AceType != ACCESS_ALLOWED_ACE_TYPE { continue; // not ACCESS_ALLOWED } if (hdr.AceFlags & INHERIT_ONLY_ACE) != 0 { @@ -194,7 +198,7 @@ pub unsafe fn dacl_has_write_allow_for_sid(p_dacl: *mut ACL, psid: *mut c_void) continue; } let hdr = &*(p_ace as *const ACE_HEADER); - if hdr.AceType != 0 { + if hdr.AceType != ACCESS_ALLOWED_ACE_TYPE { continue; // ACCESS_ALLOWED_ACE_TYPE } // Ignore ACEs that are inherit-only (do not apply to the current object) @@ -242,13 +246,13 @@ pub unsafe fn dacl_has_write_deny_for_sid(p_dacl: *mut ACL, psid: *mut c_void) - continue; } let hdr = &*(p_ace as *const ACE_HEADER); - if hdr.AceType != 1 { + if hdr.AceType != ACCESS_DENIED_ACE_TYPE { continue; // ACCESS_DENIED_ACE_TYPE } if (hdr.AceFlags & INHERIT_ONLY_ACE) != 0 { continue; } - let ace = &*(p_ace as *const ACCESS_ALLOWED_ACE); + let ace = &*(p_ace as *const ACCESS_DENIED_ACE); let base = p_ace as usize; let sid_ptr = (base + std::mem::size_of::() + std::mem::size_of::()) as *mut c_void; @@ -259,6 +263,44 @@ pub unsafe fn dacl_has_write_deny_for_sid(p_dacl: *mut ACL, psid: *mut c_void) - false } +pub unsafe fn dacl_has_read_deny_for_sid(p_dacl: *mut ACL, psid: *mut c_void) -> bool { + if p_dacl.is_null() { + return false; + } + let mut info: ACL_SIZE_INFORMATION = std::mem::zeroed(); + let ok = GetAclInformation( + p_dacl as *const ACL, + &mut info as *mut _ as *mut c_void, + std::mem::size_of::() as u32, + AclSizeInformation, + ); + if ok == 0 { + return false; + } + let deny_read_mask = FILE_GENERIC_READ | GENERIC_READ_MASK; + for i in 0..info.AceCount { + let mut p_ace: *mut c_void = std::ptr::null_mut(); + if GetAce(p_dacl as *const ACL, i, &mut p_ace) == 0 { + continue; + } + let hdr = &*(p_ace as *const ACE_HEADER); + if hdr.AceType != ACCESS_DENIED_ACE_TYPE { + continue; // ACCESS_DENIED_ACE_TYPE + } + if (hdr.AceFlags & INHERIT_ONLY_ACE) != 0 { + continue; + } + let ace = &*(p_ace as *const ACCESS_DENIED_ACE); + let base = p_ace as usize; + let sid_ptr = + (base + std::mem::size_of::() + std::mem::size_of::()) as *mut c_void; + if EqualSid(sid_ptr, psid) != 0 && (ace.Mask & deny_read_mask) != 0 { + return true; + } + } + false +} + const WRITE_ALLOW_MASK: u32 = FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE @@ -516,6 +558,85 @@ pub unsafe fn add_deny_write_ace(path: &Path, psid: *mut c_void) -> Result Ok(added) } +/// Adds a deny ACE to prevent reads for the given SID on the target path. +/// +/// `SetEntriesInAclW` places newly-created deny ACEs before allow ACEs, which +/// keeps the resulting DACL in the order Windows expects for denies to win. +/// The ACE is inheritable so a deny applied to a materialized directory also +/// covers files and directories later created underneath it. +/// +/// # Safety +/// Caller must ensure `psid` points to a valid SID and `path` refers to an existing file or directory. +pub unsafe fn add_deny_read_ace(path: &Path, psid: *mut c_void) -> Result { + let mut p_sd: *mut c_void = std::ptr::null_mut(); + let mut p_dacl: *mut ACL = std::ptr::null_mut(); + let code = GetNamedSecurityInfoW( + to_wide(path).as_ptr(), + 1, + DACL_SECURITY_INFORMATION, + std::ptr::null_mut(), + std::ptr::null_mut(), + &mut p_dacl, + std::ptr::null_mut(), + &mut p_sd, + ); + if code != ERROR_SUCCESS { + return Err(anyhow!("GetNamedSecurityInfoW failed: {code}")); + } + let mut added = false; + if !dacl_has_read_deny_for_sid(p_dacl, psid) { + let trustee = TRUSTEE_W { + pMultipleTrustee: std::ptr::null_mut(), + MultipleTrusteeOperation: 0, + TrusteeForm: TRUSTEE_IS_SID, + TrusteeType: TRUSTEE_IS_UNKNOWN, + ptstrName: psid as *mut u16, + }; + let mut explicit: EXPLICIT_ACCESS_W = std::mem::zeroed(); + explicit.grfAccessPermissions = FILE_GENERIC_READ | GENERIC_READ_MASK; + explicit.grfAccessMode = DENY_ACCESS; + explicit.grfInheritance = CONTAINER_INHERIT_ACE | OBJECT_INHERIT_ACE; + explicit.Trustee = trustee; + let mut p_new_dacl: *mut ACL = std::ptr::null_mut(); + let code2 = SetEntriesInAclW(1, &explicit, p_dacl, &mut p_new_dacl); + if code2 != ERROR_SUCCESS { + if !p_sd.is_null() { + LocalFree(p_sd as HLOCAL); + } + if !p_new_dacl.is_null() { + LocalFree(p_new_dacl as HLOCAL); + } + return Err(anyhow!("SetEntriesInAclW failed: {code2}")); + } + let code3 = SetNamedSecurityInfoW( + to_wide(path).as_ptr() as *mut u16, + 1, + DACL_SECURITY_INFORMATION, + std::ptr::null_mut(), + std::ptr::null_mut(), + p_new_dacl, + std::ptr::null_mut(), + ); + if code3 != ERROR_SUCCESS { + if !p_sd.is_null() { + LocalFree(p_sd as HLOCAL); + } + if !p_new_dacl.is_null() { + LocalFree(p_new_dacl as HLOCAL); + } + return Err(anyhow!("SetNamedSecurityInfoW failed: {code3}")); + } + added = true; + if !p_new_dacl.is_null() { + LocalFree(p_new_dacl as HLOCAL); + } + } + if !p_sd.is_null() { + LocalFree(p_sd as HLOCAL); + } + Ok(added) +} + pub unsafe fn revoke_ace(path: &Path, psid: *mut c_void) { let mut p_sd: *mut c_void = std::ptr::null_mut(); let mut p_dacl: *mut ACL = std::ptr::null_mut(); diff --git a/codex-rs/windows-sandbox-rs/src/deny_read_acl.rs b/codex-rs/windows-sandbox-rs/src/deny_read_acl.rs new file mode 100644 index 00000000000..6dbbe27d935 --- /dev/null +++ b/codex-rs/windows-sandbox-rs/src/deny_read_acl.rs @@ -0,0 +1,224 @@ +use crate::acl::add_deny_read_ace; +use crate::acl::revoke_ace; +use crate::path_normalization::canonicalize_path; +use crate::setup::sandbox_dir; +use anyhow::Context; +use anyhow::Result; +use serde::Deserialize; +use serde::Serialize; +use std::collections::HashSet; +use std::ffi::c_void; +use std::path::Path; +use std::path::PathBuf; + +/// Identifies which Windows sandbox principal owns a persistent deny-read ACL. +/// +/// The elevated backend applies deny ACEs to the shared sandbox users group, +/// while the restricted-token backend applies them to capability SIDs. Keeping +/// separate records prevents stale cleanup for one backend from revoking entries +/// that are still owned by the other backend. +#[derive(Debug, Clone, Copy)] +pub enum DenyReadAclRecordKind { + SandboxGroup, + RestrictedToken, +} + +impl DenyReadAclRecordKind { + fn file_name(self) -> &'static str { + match self { + Self::SandboxGroup => "deny_read_acls_sandbox_group.json", + Self::RestrictedToken => "deny_read_acls_restricted_token.json", + } + } +} + +#[derive(Debug, Default, Deserialize, Serialize)] +struct DenyReadAclRecord { + paths: Vec, +} + +pub fn deny_read_acl_record_path(codex_home: &Path, kind: DenyReadAclRecordKind) -> PathBuf { + sandbox_dir(codex_home).join(kind.file_name()) +} + +/// Build the exact ACL paths that should receive a deny-read ACE. +/// +/// We keep both the lexical policy path and, when it already exists, the +/// canonical target. The lexical path covers the path users configured and lets +/// missing exact denies be materialized later; the canonical path also covers +/// an existing reparse-point target so a sandbox cannot read the same object +/// through the resolved location. +pub fn plan_deny_read_acl_paths(paths: &[PathBuf]) -> Vec { + let mut planned = Vec::new(); + let mut seen = HashSet::new(); + for path in paths { + push_planned_path(&mut planned, &mut seen, path.to_path_buf()); + if path.exists() { + push_planned_path(&mut planned, &mut seen, canonicalize_path(path)); + } + } + planned +} + +fn push_planned_path(planned: &mut Vec, seen: &mut HashSet, path: PathBuf) { + if seen.insert(lexical_path_key(&path)) { + planned.push(path); + } +} + +fn lexical_path_key(path: &Path) -> String { + path.to_string_lossy() + .replace('\\', "/") + .trim_end_matches('/') + .to_ascii_lowercase() +} + +fn read_record(path: &Path) -> Result { + match std::fs::read_to_string(path) { + Ok(contents) => serde_json::from_str(&contents) + .with_context(|| format!("parse deny-read ACL record {}", path.display())), + Err(err) if err.kind() == std::io::ErrorKind::NotFound => Ok(DenyReadAclRecord::default()), + Err(err) => Err(err) + .with_context(|| format!("read deny-read ACL record {}", path.display())), + } +} + +pub fn write_persistent_deny_read_acl_record( + codex_home: &Path, + kind: DenyReadAclRecordKind, + paths: &[PathBuf], +) -> Result<()> { + let record_path = deny_read_acl_record_path(codex_home, kind); + let planned_paths = plan_deny_read_acl_paths(paths); + if planned_paths.is_empty() { + match std::fs::remove_file(&record_path) { + Ok(()) => return Ok(()), + Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Ok(()), + Err(err) => { + return Err(err).with_context(|| { + format!("remove deny-read ACL record {}", record_path.display()) + }); + } + } + } + if let Some(parent) = record_path.parent() { + std::fs::create_dir_all(parent) + .with_context(|| format!("create deny-read ACL record dir {}", parent.display()))?; + } + let record = DenyReadAclRecord { + paths: planned_paths, + }; + let contents = serde_json::to_vec_pretty(&record) + .with_context(|| format!("serialize deny-read ACL record {}", record_path.display()))?; + std::fs::write(&record_path, contents) + .with_context(|| format!("write deny-read ACL record {}", record_path.display())) +} + +/// Removes deny-read ACEs that were applied for a previous policy but are not +/// present in the current policy. This uses the same broad revoke primitive as +/// the rest of the Windows sandbox ACL guard path, so callers should run stale +/// cleanup before re-granting any read ACLs for the same SID in this refresh. +/// That ordering matters because `revoke_ace` removes all ACEs for the SID, not +/// only the deny-read ACEs recorded here. +/// +/// # Safety +/// Caller must pass a valid SID pointer for the ACEs recorded under `kind`. +pub unsafe fn cleanup_stale_persistent_deny_read_acls( + codex_home: &Path, + kind: DenyReadAclRecordKind, + desired_paths: &[PathBuf], + psid: *mut c_void, +) -> Result> { + let record_path = deny_read_acl_record_path(codex_home, kind); + let record = read_record(&record_path)?; + let desired_keys = plan_deny_read_acl_paths(desired_paths) + .into_iter() + .map(|path| lexical_path_key(&path)) + .collect::>(); + let mut cleaned = Vec::new(); + for path in record.paths { + if desired_keys.contains(&lexical_path_key(&path)) || !path.exists() { + continue; + } + revoke_ace(&path, psid); + cleaned.push(path); + } + Ok(cleaned) +} + +/// Applies deny-read ACEs to explicit paths. Missing paths are materialized as +/// directories before the ACE is applied so a sandboxed command cannot create a +/// previously absent denied path and then read from it in the same run. +/// If any path fails, deny ACEs applied by this call are revoked before the +/// error is returned so a one-shot sandbox run does not leave partial state. +/// +/// # Safety +/// Caller must pass a valid SID pointer for the sandbox principal being denied. +pub unsafe fn apply_deny_read_acls(paths: &[PathBuf], psid: *mut c_void) -> Result> { + let planned = plan_deny_read_acl_paths(paths); + let mut applied = Vec::new(); + let mut seen = HashSet::new(); + let mut added_in_this_call = Vec::new(); + for path in planned { + let result = (|| -> Result { + if !path.exists() { + std::fs::create_dir_all(&path) + .with_context(|| format!("create deny-read path {}", path.display()))?; + } + add_deny_read_ace(&path, psid) + .with_context(|| format!("apply deny-read ACE to {}", path.display())) + })(); + let added = match result { + Ok(added) => added, + Err(err) => { + for added_path in &added_in_this_call { + revoke_ace(added_path, psid); + } + return Err(err); + } + }; + if added { + added_in_this_call.push(path.clone()); + } + push_planned_path(&mut applied, &mut seen, path); + } + Ok(applied) +} + +#[cfg(test)] +mod tests { + use super::plan_deny_read_acl_paths; + use pretty_assertions::assert_eq; + use std::collections::HashSet; + use std::path::PathBuf; + use tempfile::TempDir; + + #[test] + fn plan_preserves_missing_paths() { + let tmp = TempDir::new().expect("tempdir"); + let missing = tmp.path().join("future-secret.env"); + + assert_eq!(plan_deny_read_acl_paths(std::slice::from_ref(&missing)), vec![ + missing + ]); + } + + #[test] + fn plan_includes_existing_canonical_targets() { + let tmp = TempDir::new().expect("tempdir"); + let existing = tmp.path().join("secret.env"); + std::fs::write(&existing, "secret").expect("write secret"); + + let planned: HashSet = plan_deny_read_acl_paths(std::slice::from_ref(&existing)) + .into_iter() + .collect(); + let expected: HashSet = [ + existing.clone(), + dunce::canonicalize(&existing).expect("canonical path"), + ] + .into_iter() + .collect(); + + assert_eq!(planned, expected); + } +} diff --git a/codex-rs/windows-sandbox-rs/src/deny_read_resolver.rs b/codex-rs/windows-sandbox-rs/src/deny_read_resolver.rs new file mode 100644 index 00000000000..ad24199f82f --- /dev/null +++ b/codex-rs/windows-sandbox-rs/src/deny_read_resolver.rs @@ -0,0 +1,298 @@ +use codex_protocol::permissions::FileSystemAccessMode; +use codex_protocol::permissions::FileSystemPath; +use codex_protocol::permissions::FileSystemSandboxEntry; +use codex_protocol::permissions::FileSystemSandboxPolicy; +use codex_protocol::permissions::ReadDenyMatcher; +use codex_utils_absolute_path::AbsolutePathBuf; +use std::collections::HashSet; +use std::path::Path; +use std::path::PathBuf; + +struct GlobScanPlan { + root: PathBuf, + max_depth: Option, +} + +/// Resolve split filesystem `None` read entries into concrete Windows ACL targets. +/// +/// Windows ACLs do not understand Codex filesystem glob patterns directly. Exact +/// unreadable roots can be passed through as-is, including paths that do not +/// exist yet. Glob entries are snapshot-expanded to the files/directories that +/// already exist under their literal scan root; future exact paths are handled +/// later by materializing them before the deny ACE is applied. +pub fn resolve_windows_deny_read_paths( + file_system_sandbox_policy: &FileSystemSandboxPolicy, + cwd: &AbsolutePathBuf, +) -> Result, String> { + let mut paths = Vec::new(); + let mut seen = HashSet::new(); + + for path in file_system_sandbox_policy.get_unreadable_roots_with_cwd(cwd.as_path()) { + push_absolute_path(&mut paths, &mut seen, path.into_path_buf())?; + } + + let unreadable_globs = file_system_sandbox_policy.get_unreadable_globs_with_cwd(cwd.as_path()); + if unreadable_globs.is_empty() { + return Ok(paths); + } + + let glob_policy = FileSystemSandboxPolicy::restricted( + unreadable_globs + .iter() + .map(|pattern| FileSystemSandboxEntry { + path: FileSystemPath::GlobPattern { + pattern: pattern.clone(), + }, + access: FileSystemAccessMode::None, + }) + .collect(), + ); + let Some(matcher) = ReadDenyMatcher::new(&glob_policy, cwd.as_path()) else { + return Ok(paths); + }; + + let mut seen_scan_dirs = HashSet::new(); + for pattern in unreadable_globs { + let scan_plan = glob_scan_plan(&pattern); + collect_existing_glob_matches( + &scan_plan.root, + &matcher, + &mut paths, + &mut seen, + &mut seen_scan_dirs, + scan_plan.max_depth, + 0, + )?; + } + + Ok(paths) +} + +fn collect_existing_glob_matches( + path: &Path, + matcher: &ReadDenyMatcher, + paths: &mut Vec, + seen_paths: &mut HashSet, + seen_scan_dirs: &mut HashSet, + max_depth: Option, + depth: usize, +) -> Result<(), String> { + if !path.exists() { + return Ok(()); + } + + if matcher.is_read_denied(path) { + push_absolute_path(paths, seen_paths, path.to_path_buf())?; + } + + let Ok(metadata) = path.metadata() else { + return Ok(()); + }; + if !metadata.is_dir() { + return Ok(()); + } + + // Canonical directory keys keep recursive scans from following a symlink or + // junction cycle forever while preserving the original matched path for the + // ACL layer. + let scan_key = dunce::canonicalize(path).unwrap_or_else(|_| path.to_path_buf()); + if !seen_scan_dirs.insert(scan_key) { + return Ok(()); + } + + if max_depth.is_some_and(|max_depth| depth >= max_depth) { + return Ok(()); + } + + let Ok(entries) = std::fs::read_dir(path) else { + return Ok(()); + }; + for entry in entries.flatten() { + collect_existing_glob_matches( + &entry.path(), + matcher, + paths, + seen_paths, + seen_scan_dirs, + max_depth, + depth + 1, + )?; + } + + Ok(()) +} + +fn push_absolute_path( + paths: &mut Vec, + seen: &mut HashSet, + path: PathBuf, +) -> Result<(), String> { + let absolute_path = AbsolutePathBuf::from_absolute_path(dunce::simplified(&path)) + .map_err(|err| err.to_string())?; + if seen.insert(absolute_path.to_path_buf()) { + paths.push(absolute_path); + } + Ok(()) +} + +fn glob_scan_plan(pattern: &str) -> GlobScanPlan { + // Start scanning at the deepest literal directory prefix before the first + // glob metacharacter. For example, `C:\repo\**\*.env` only scans `C:\repo` + // instead of the current directory or drive root. + let first_glob = pattern + .char_indices() + .find(|(_, ch)| matches!(ch, '*' | '?' | '[')) + .map(|(index, _)| index) + .unwrap_or(pattern.len()); + let literal_prefix = &pattern[..first_glob]; + let Some(separator_index) = literal_prefix.rfind(['/', '\\']) else { + return GlobScanPlan { + root: PathBuf::from("."), + max_depth: glob_scan_max_depth(pattern), + }; + }; + let pattern_suffix = &pattern[separator_index + 1..]; + let is_drive_root_separator = separator_index > 0 + && literal_prefix + .as_bytes() + .get(separator_index - 1) + .is_some_and(|ch| *ch == b':'); + if separator_index == 0 || is_drive_root_separator { + return GlobScanPlan { + root: PathBuf::from(&literal_prefix[..=separator_index]), + max_depth: glob_scan_max_depth(pattern_suffix), + }; + } + GlobScanPlan { + root: PathBuf::from(literal_prefix[..separator_index].to_string()), + max_depth: glob_scan_max_depth(pattern_suffix), + } +} + +fn glob_scan_max_depth(pattern_suffix: &str) -> Option { + let components = pattern_suffix + .split(['/', '\\']) + .filter(|component| !component.is_empty()) + .collect::>(); + if components.contains(&"**") { + return None; + } + Some(components.len()) +} + +#[cfg(test)] +mod tests { + use super::glob_scan_plan; + use super::resolve_windows_deny_read_paths; + use codex_protocol::permissions::FileSystemAccessMode; + use codex_protocol::permissions::FileSystemPath; + use codex_protocol::permissions::FileSystemSandboxEntry; + use codex_protocol::permissions::FileSystemSandboxPolicy; + use codex_utils_absolute_path::AbsolutePathBuf; + use pretty_assertions::assert_eq; + use std::collections::HashSet; + use std::path::PathBuf; + use tempfile::TempDir; + + fn unreadable_glob_entry(pattern: String) -> FileSystemSandboxEntry { + FileSystemSandboxEntry { + path: FileSystemPath::GlobPattern { pattern }, + access: FileSystemAccessMode::None, + } + } + + fn unreadable_path_entry(path: PathBuf) -> FileSystemSandboxEntry { + FileSystemSandboxEntry { + path: FileSystemPath::Path { + path: AbsolutePathBuf::from_absolute_path(path).expect("absolute path"), + }, + access: FileSystemAccessMode::None, + } + } + + #[test] + fn scan_root_uses_literal_prefix_before_glob() { + assert_eq!( + glob_scan_plan("/tmp/work/**/*.env").root, + PathBuf::from("/tmp/work") + ); + assert_eq!( + glob_scan_plan(r"C:\Users\dev\repo\**\*.env").root, + PathBuf::from(r"C:\Users\dev\repo") + ); + assert_eq!(glob_scan_plan(r"C:\*.env").root, PathBuf::from(r"C:\")); + } + + #[test] + fn scan_depth_is_bounded_for_non_recursive_globs() { + assert_eq!(glob_scan_plan("/tmp/work/*.env").max_depth, Some(1)); + assert_eq!(glob_scan_plan("/tmp/work/*/*.env").max_depth, Some(2)); + assert_eq!(glob_scan_plan("/tmp/work/**/*.env").max_depth, None); + } + + #[test] + fn exact_missing_paths_are_preserved() { + let tmp = TempDir::new().expect("tempdir"); + let cwd = AbsolutePathBuf::from_absolute_path(tmp.path()).expect("absolute cwd"); + let missing = tmp.path().join("missing.env"); + let policy = FileSystemSandboxPolicy::restricted(vec![unreadable_path_entry(missing)]); + + assert_eq!( + resolve_windows_deny_read_paths(&policy, &cwd).expect("resolve"), + vec![ + AbsolutePathBuf::from_absolute_path( + dunce::canonicalize(tmp.path()) + .expect("canonical tempdir") + .join("missing.env") + ) + .expect("absolute missing") + ] + ); + } + + #[test] + fn glob_patterns_expand_to_existing_matches() { + let tmp = TempDir::new().expect("tempdir"); + let cwd = AbsolutePathBuf::from_absolute_path(tmp.path()).expect("absolute cwd"); + let root_env = tmp.path().join(".env"); + let nested_env = tmp.path().join("app").join(".env"); + let notes = tmp.path().join("app").join("notes.txt"); + std::fs::create_dir_all(notes.parent().expect("parent")).expect("create parent"); + std::fs::write(&root_env, "secret").expect("write root env"); + std::fs::write(&nested_env, "secret").expect("write nested env"); + std::fs::write(¬es, "notes").expect("write notes"); + let policy = FileSystemSandboxPolicy::restricted(vec![unreadable_glob_entry(format!( + "{}/**/*.env", + tmp.path().display() + ))]); + + let actual: HashSet = resolve_windows_deny_read_paths(&policy, &cwd) + .expect("resolve") + .into_iter() + .map(AbsolutePathBuf::into_path_buf) + .collect(); + let expected = [root_env, nested_env].into_iter().collect(); + + assert_eq!(actual, expected); + } + + #[test] + fn non_recursive_globs_do_not_expand_nested_matches() { + let tmp = TempDir::new().expect("tempdir"); + let cwd = AbsolutePathBuf::from_absolute_path(tmp.path()).expect("absolute cwd"); + let root_env = tmp.path().join(".env"); + let nested_env = tmp.path().join("app").join(".env"); + std::fs::create_dir_all(nested_env.parent().expect("parent")).expect("create parent"); + std::fs::write(&root_env, "secret").expect("write root env"); + std::fs::write(&nested_env, "secret").expect("write nested env"); + let policy = FileSystemSandboxPolicy::restricted(vec![unreadable_glob_entry(format!( + "{}/*.env", + tmp.path().display() + ))]); + + assert_eq!( + resolve_windows_deny_read_paths(&policy, &cwd).expect("resolve"), + vec![AbsolutePathBuf::from_absolute_path(root_env).expect("absolute root env")] + ); + } +} diff --git a/codex-rs/windows-sandbox-rs/src/elevated_impl.rs b/codex-rs/windows-sandbox-rs/src/elevated_impl.rs index 327425bd079..72bd108becf 100644 --- a/codex-rs/windows-sandbox-rs/src/elevated_impl.rs +++ b/codex-rs/windows-sandbox-rs/src/elevated_impl.rs @@ -14,6 +14,7 @@ pub struct ElevatedSandboxCaptureRequest<'a> { pub proxy_enforced: bool, pub read_roots_override: Option<&'a [PathBuf]>, pub write_roots_override: Option<&'a [PathBuf]>, + pub deny_read_paths_override: &'a [PathBuf], pub deny_write_paths_override: &'a [PathBuf], } @@ -239,6 +240,7 @@ mod windows_impl { proxy_enforced, read_roots_override, write_roots_override, + deny_read_paths_override, deny_write_paths_override, } = request; let policy = parse_policy(policy_json_or_preset)?; @@ -260,6 +262,7 @@ mod windows_impl { codex_home, read_roots_override, write_roots_override, + deny_read_paths_override, deny_write_paths_override, proxy_enforced, )?; diff --git a/codex-rs/windows-sandbox-rs/src/identity.rs b/codex-rs/windows-sandbox-rs/src/identity.rs index 12b02105456..01fcbc19669 100644 --- a/codex-rs/windows-sandbox-rs/src/identity.rs +++ b/codex-rs/windows-sandbox-rs/src/identity.rs @@ -138,6 +138,7 @@ pub fn require_logon_sandbox_creds( codex_home: &Path, read_roots_override: Option<&[PathBuf]>, write_roots_override: Option<&[PathBuf]>, + deny_read_paths_override: &[PathBuf], deny_write_paths_override: &[PathBuf], proxy_enforced: bool, ) -> Result { @@ -199,6 +200,7 @@ pub fn require_logon_sandbox_creds( crate::setup::SetupRootOverrides { read_roots: Some(needed_read.clone()), write_roots: Some(needed_write.clone()), + deny_read_paths: Some(deny_read_paths_override.to_vec()), deny_write_paths: Some(deny_write_paths_override.to_vec()), }, )?; @@ -217,6 +219,7 @@ pub fn require_logon_sandbox_creds( crate::setup::SetupRootOverrides { read_roots: Some(needed_read), write_roots: Some(needed_write), + deny_read_paths: Some(deny_read_paths_override.to_vec()), deny_write_paths: Some(deny_write_paths_override.to_vec()), }, )?; diff --git a/codex-rs/windows-sandbox-rs/src/lib.rs b/codex-rs/windows-sandbox-rs/src/lib.rs index 90176b08ead..18ac562caa8 100644 --- a/codex-rs/windows-sandbox-rs/src/lib.rs +++ b/codex-rs/windows-sandbox-rs/src/lib.rs @@ -14,6 +14,7 @@ windows_modules!( audit, cap, desktop, + deny_read_acl, dpapi, env, helper_materialization, @@ -28,6 +29,8 @@ windows_modules!( workspace_acl ); +mod deny_read_resolver; + #[cfg(target_os = "windows")] #[path = "conpty/mod.rs"] mod conpty; @@ -46,6 +49,8 @@ mod elevated_impl; #[cfg(target_os = "windows")] mod setup_error; +#[cfg(target_os = "windows")] +pub use acl::add_deny_read_ace; #[cfg(target_os = "windows")] pub use acl::add_deny_write_ace; @@ -70,6 +75,17 @@ pub use cap::workspace_cap_sid_for_cwd; #[cfg(target_os = "windows")] pub use conpty::spawn_conpty_process_as_user; #[cfg(target_os = "windows")] +pub use deny_read_acl::DenyReadAclRecordKind; +#[cfg(target_os = "windows")] +pub use deny_read_acl::apply_deny_read_acls; +#[cfg(target_os = "windows")] +pub use deny_read_acl::cleanup_stale_persistent_deny_read_acls; +#[cfg(target_os = "windows")] +pub use deny_read_acl::plan_deny_read_acl_paths; +#[cfg(target_os = "windows")] +pub use deny_read_acl::write_persistent_deny_read_acl_record; +pub use deny_read_resolver::resolve_windows_deny_read_paths; +#[cfg(target_os = "windows")] pub use dpapi::protect as dpapi_protect; #[cfg(target_os = "windows")] pub use dpapi::unprotect as dpapi_unprotect; @@ -180,7 +196,7 @@ pub use windows_impl::CaptureResult; #[cfg(target_os = "windows")] pub use windows_impl::run_windows_sandbox_capture; #[cfg(target_os = "windows")] -pub use windows_impl::run_windows_sandbox_capture_with_extra_deny_write_paths; +pub use windows_impl::run_windows_sandbox_capture_with_filesystem_overrides; #[cfg(target_os = "windows")] pub use windows_impl::run_windows_sandbox_legacy_preflight; #[cfg(target_os = "windows")] @@ -211,6 +227,10 @@ mod windows_impl { 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::DenyReadAclRecordKind; + use super::deny_read_acl::apply_deny_read_acls; + use super::deny_read_acl::cleanup_stale_persistent_deny_read_acls; + use super::deny_read_acl::write_persistent_deny_read_acl_record; use super::env::apply_no_network_to_env; use super::env::ensure_non_interactive_pager; use super::env::normalize_null_device_env; @@ -298,7 +318,7 @@ mod windows_impl { timeout_ms: Option, use_private_desktop: bool, ) -> Result { - run_windows_sandbox_capture_with_extra_deny_write_paths( + run_windows_sandbox_capture_with_filesystem_overrides( policy_json_or_preset, sandbox_policy_cwd, codex_home, @@ -307,12 +327,13 @@ mod windows_impl { env_map, timeout_ms, &[], + &[], use_private_desktop, ) } #[allow(clippy::too_many_arguments)] - pub fn run_windows_sandbox_capture_with_extra_deny_write_paths( + pub fn run_windows_sandbox_capture_with_filesystem_overrides( policy_json_or_preset: &str, sandbox_policy_cwd: &Path, codex_home: &Path, @@ -320,6 +341,7 @@ mod windows_impl { cwd: &Path, mut env_map: HashMap, timeout_ms: Option, + additional_deny_read_paths: &[PathBuf], additional_deny_write_paths: &[PathBuf], use_private_desktop: bool, ) -> Result { @@ -405,6 +427,20 @@ mod windows_impl { } let canonical_cwd = canonicalize_path(¤t_dir); let mut guards: Vec<(PathBuf, *mut c_void)> = Vec::new(); + if persist_aces { + // Persistent workspace-write ACEs survive between commands, so drop + // deny-read ACLs from the previous policy before applying the new + // overlay. Non-persistent runs use guards and clean up at process + // exit instead. + unsafe { + cleanup_stale_persistent_deny_read_acls( + codex_home, + DenyReadAclRecordKind::RestrictedToken, + additional_deny_read_paths, + psid_generic, + )?; + } + } unsafe { for p in &allow { let psid = if is_workspace_write && is_command_cwd_root(p, &canonical_cwd) { @@ -432,6 +468,30 @@ mod windows_impl { 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 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 { + write_persistent_deny_read_acl_record( + codex_home, + DenyReadAclRecordKind::RestrictedToken, + &applied_deny_read_paths, + )?; + } else { + 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); @@ -454,6 +514,7 @@ 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); @@ -562,11 +623,7 @@ mod windows_impl { } if !persist_aces { - unsafe { - for (p, sid) in guards { - revoke_ace(&p, sid); - } - } + cleanup_acl_guards(&mut guards); } Ok(CaptureResult { @@ -577,6 +634,14 @@ 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, 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 78c0a8be8e5..c5c8168b6dd 100644 --- a/codex-rs/windows-sandbox-rs/src/setup_main_win.rs +++ b/codex-rs/windows-sandbox-rs/src/setup_main_win.rs @@ -6,13 +6,16 @@ use anyhow::Context; use anyhow::Result; use base64::Engine; use base64::engine::general_purpose::STANDARD as BASE64; +use codex_windows_sandbox::DenyReadAclRecordKind; use codex_windows_sandbox::LOG_FILE_NAME; use codex_windows_sandbox::SETUP_VERSION; use codex_windows_sandbox::SetupErrorCode; use codex_windows_sandbox::SetupErrorReport; use codex_windows_sandbox::SetupFailure; use codex_windows_sandbox::add_deny_write_ace; +use codex_windows_sandbox::apply_deny_read_acls; use codex_windows_sandbox::canonicalize_path; +use codex_windows_sandbox::cleanup_stale_persistent_deny_read_acls; use codex_windows_sandbox::convert_string_sid_to_sid; use codex_windows_sandbox::ensure_allow_mask_aces_with_inheritance; use codex_windows_sandbox::ensure_allow_write_aces; @@ -28,6 +31,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_persistent_deny_read_acl_record; use codex_windows_sandbox::write_setup_error_report; use serde::Deserialize; use serde::Serialize; @@ -84,6 +88,8 @@ struct Payload { read_roots: Vec, write_roots: Vec, #[serde(default)] + deny_read_paths: Vec, + #[serde(default)] deny_write_paths: Vec, proxy_ports: Vec, #[serde(default)] @@ -457,38 +463,83 @@ fn run_read_acl_only(payload: &Payload, log: &mut File) -> Result<()> { let sandbox_group_sid = resolve_sandbox_users_group_sid()?; let sandbox_group_psid = sid_bytes_to_psid(&sandbox_group_sid)?; let mut refresh_errors: Vec = Vec::new(); - let users_sid = resolve_sid("Users")?; - let users_psid = sid_bytes_to_psid(&users_sid)?; - let auth_sid = resolve_sid("Authenticated Users")?; - let auth_psid = sid_bytes_to_psid(&auth_sid)?; - let everyone_sid = resolve_sid("Everyone")?; - let everyone_psid = sid_bytes_to_psid(&everyone_sid)?; - let rx_psids = vec![users_psid, auth_psid, everyone_psid]; - let subjects = ReadAclSubjects { - sandbox_group_psid, - rx_psids: &rx_psids, - }; - apply_read_acls( - &payload.read_roots, - &subjects, - log, - &mut refresh_errors, - FILE_GENERIC_READ | FILE_GENERIC_EXECUTE, - "read", - OBJECT_INHERIT_ACE | CONTAINER_INHERIT_ACE, - )?; - unsafe { - if !sandbox_group_psid.is_null() { - LocalFree(sandbox_group_psid as HLOCAL); + // Stale cleanup must happen before the helper re-grants read ACLs because + // the cleanup primitive revokes all ACEs for the sandbox group SID. + match unsafe { + cleanup_stale_persistent_deny_read_acls( + &payload.codex_home, + DenyReadAclRecordKind::SandboxGroup, + &payload.deny_read_paths, + sandbox_group_psid, + ) + } { + Ok(cleaned) => { + if !cleaned.is_empty() { + log_line( + log, + &format!("cleaned {} stale deny-read ACLs", cleaned.len()), + )?; + } + } + Err(err) => { + refresh_errors.push(format!("cleanup stale deny-read ACLs failed: {err}")); + log_line(log, &format!("cleanup stale deny-read ACLs failed: {err}"))?; + } + } + if !payload.read_roots.is_empty() { + let users_sid = resolve_sid("Users")?; + let users_psid = sid_bytes_to_psid(&users_sid)?; + let auth_sid = resolve_sid("Authenticated Users")?; + let auth_psid = sid_bytes_to_psid(&auth_sid)?; + let everyone_sid = resolve_sid("Everyone")?; + let everyone_psid = sid_bytes_to_psid(&everyone_sid)?; + let rx_psids = vec![users_psid, auth_psid, everyone_psid]; + let subjects = ReadAclSubjects { + sandbox_group_psid, + rx_psids: &rx_psids, + }; + apply_read_acls( + &payload.read_roots, + &subjects, + log, + &mut refresh_errors, + FILE_GENERIC_READ | FILE_GENERIC_EXECUTE, + "read", + OBJECT_INHERIT_ACE | CONTAINER_INHERIT_ACE, + )?; + unsafe { + if !users_psid.is_null() { + LocalFree(users_psid as HLOCAL); + } + if !auth_psid.is_null() { + LocalFree(auth_psid as HLOCAL); + } + if !everyone_psid.is_null() { + LocalFree(everyone_psid as HLOCAL); + } } - if !users_psid.is_null() { - LocalFree(users_psid as HLOCAL); + } + // Deny-read ACEs are applied after read grants so the DACL ends with + // explicit deny entries that take precedence over the broad read allowlist. + match unsafe { apply_deny_read_acls(&payload.deny_read_paths, sandbox_group_psid) } { + Ok(applied) => { + write_persistent_deny_read_acl_record( + &payload.codex_home, + DenyReadAclRecordKind::SandboxGroup, + &applied, + )?; + if !applied.is_empty() { + log_line(log, &format!("applied {} deny-read ACLs", applied.len()))?; + } } - if !auth_psid.is_null() { - LocalFree(auth_psid as HLOCAL); + Err(err) => { + refresh_errors.push(format!("apply deny-read ACLs failed: {err}")); + log_line(log, &format!("apply deny-read ACLs failed: {err}"))?; } - if !everyone_psid.is_null() { - LocalFree(everyone_psid as HLOCAL); + } + unsafe { + if !sandbox_group_psid.is_null() { + LocalFree(sandbox_group_psid as HLOCAL); } } if !refresh_errors.is_empty() { @@ -603,8 +654,14 @@ fn run_setup_full(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<( } } - if payload.read_roots.is_empty() { - log_line(log, "no read roots to grant; skipping read ACL helper")?; + // The read ACL helper is also responsible for persistent deny-read cleanup, + // so it must run whenever deny-read paths are present even if no new read + // roots need to be granted. + if payload.read_roots.is_empty() && payload.deny_read_paths.is_empty() { + log_line( + log, + "no read roots or deny-read paths; skipping read ACL helper", + )?; } else { match read_acl_mutex_exists() { Ok(true) => { diff --git a/codex-rs/windows-sandbox-rs/src/setup_orchestrator.rs b/codex-rs/windows-sandbox-rs/src/setup_orchestrator.rs index 4effc0b70df..673a636d9cc 100644 --- a/codex-rs/windows-sandbox-rs/src/setup_orchestrator.rs +++ b/codex-rs/windows-sandbox-rs/src/setup_orchestrator.rs @@ -15,6 +15,7 @@ use crate::allow::compute_allow_paths; use crate::helper_materialization::helper_bin_dir; use crate::logging::log_note; use crate::path_normalization::canonical_path_key; +use crate::path_normalization::canonicalize_path; use crate::policy::SandboxPolicy; use crate::setup_error::SetupErrorCode; use crate::setup_error::SetupFailure; @@ -92,6 +93,7 @@ pub struct SandboxSetupRequest<'a> { pub struct SetupRootOverrides { pub read_roots: Option>, pub write_roots: Option>, + pub deny_read_paths: Option>, pub deny_write_paths: Option>, } @@ -146,6 +148,7 @@ pub fn run_setup_refresh_with_extra_read_roots( SetupRootOverrides { read_roots: Some(read_roots), write_roots: Some(Vec::new()), + deny_read_paths: None, deny_write_paths: None, }, ) @@ -163,6 +166,7 @@ fn run_setup_refresh_inner( return Ok(()); } 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 network_identity = SandboxNetworkIdentity::from_policy(request.policy, request.proxy_enforced); @@ -175,6 +179,7 @@ fn run_setup_refresh_inner( command_cwd: request.command_cwd.to_path_buf(), read_roots, write_roots, + deny_read_paths, deny_write_paths, proxy_ports: offline_proxy_settings.proxy_ports, allow_local_binding: offline_proxy_settings.allow_local_binding, @@ -445,6 +450,8 @@ struct ElevationPayload { read_roots: Vec, write_roots: Vec, #[serde(default)] + deny_read_paths: Vec, + #[serde(default)] deny_write_paths: Vec, proxy_ports: Vec, #[serde(default)] @@ -746,6 +753,7 @@ 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 network_identity = SandboxNetworkIdentity::from_policy(request.policy, request.proxy_enforced); @@ -758,6 +766,7 @@ pub fn run_elevated_setup( command_cwd: request.command_cwd.to_path_buf(), read_roots, write_roots, + deny_read_paths, deny_write_paths, proxy_ports: offline_proxy_settings.proxy_ports, allow_local_binding: offline_proxy_settings.allow_local_binding, @@ -822,18 +831,23 @@ fn build_payload_deny_write_paths( let mut deny_write_paths: Vec = explicit_deny_write_paths .unwrap_or_default() .into_iter() - .map(|path| { - if path.exists() { - dunce::canonicalize(&path).unwrap_or(path) - } else { - path - } - }) + .map(|path| canonicalize_path(&path)) .collect(); deny_write_paths.extend(allow_deny_paths.deny); deny_write_paths } +fn build_payload_deny_read_paths(explicit_deny_read_paths: Option>) -> Vec { + // Preserve missing exact deny paths so the Windows helper can materialize + // and deny them before the sandboxed process runs. Existing paths are + // canonicalized here to make the elevated helper operate on stable targets. + explicit_deny_read_paths + .unwrap_or_default() + .into_iter() + .map(|path| canonicalize_path(&path)) + .collect() +} + fn filter_sensitive_write_roots(mut roots: Vec, codex_home: &Path) -> Vec { // Never grant capability write access to CODEX_HOME or anything under CODEX_HOME/.sandbox, // CODEX_HOME/.sandbox-bin, or CODEX_HOME/.sandbox-secrets. These locations contain sandbox @@ -1237,6 +1251,7 @@ mod tests { &super::SetupRootOverrides { read_roots: Some(vec![readable_root.clone()]), write_roots: None, + deny_read_paths: None, deny_write_paths: None, }, ); @@ -1284,6 +1299,7 @@ mod tests { &super::SetupRootOverrides { read_roots: Some(vec![readable_root.clone()]), write_roots: None, + deny_read_paths: None, deny_write_paths: None, }, ); @@ -1368,4 +1384,20 @@ mod tests { .all(|path| roots.contains(&path)) ); } + + #[test] + fn build_payload_deny_read_paths_keeps_missing_paths_and_canonicalizes_existing_paths() { + let tmp = TempDir::new().expect("tempdir"); + let existing = tmp.path().join("secret.env"); + let missing = tmp.path().join("future.env"); + fs::write(&existing, "secret").expect("write existing"); + + assert_eq!( + super::build_payload_deny_read_paths(Some(vec![existing.clone(), missing.clone()])), + vec![ + dunce::canonicalize(existing).expect("canonical existing"), + missing + ] + ); + } }