From 46f30d02828bd4c52827e5f0482a6f2a982cce5b Mon Sep 17 00:00:00 2001 From: viyatb-oai Date: Mon, 11 May 2026 23:04:28 -0700 Subject: [PATCH] feat(sandbox): add Windows deny-read parity (#18202) ## Why The split filesystem policy stack already supports exact and glob `access = none` read restrictions on macOS and Linux. Windows still needed subprocess handling for those deny-read policies without claiming enforcement from a backend that cannot provide it. ## Key finding The unelevated restricted-token backend cannot safely enforce deny-read overlays. Its `WRITE_RESTRICTED` token model is authoritative for write checks, not read denials, so this PR intentionally fails that backend closed when deny-read overrides are present instead of claiming unsupported enforcement. ## What changed This PR adds the Windows deny-read enforcement layer and makes the backend split explicit: - Resolves Windows deny-read filesystem policy entries into concrete ACL targets. - Preserves exact missing paths so they can be materialized and denied before an enforceable sandboxed process starts. - Snapshot-expands existing glob matches into ACL targets for Windows subprocess enforcement. - Honors `glob_scan_max_depth` when expanding Windows deny-read globs. - Plans both the configured lexical path and the canonical target for existing paths so reparse-point aliases are covered. - Threads deny-read overrides through the elevated/logon-user Windows sandbox backend and unified exec. - Applies elevated deny-read ACLs synchronously before command launch rather than delegating them to the background read-grant helper. - Reconciles persistent deny-read ACEs per sandbox principal so policy changes do not leave stale deny-read ACLs behind. - Fails closed on the unelevated restricted-token backend when deny-read overrides are present, because its `WRITE_RESTRICTED` token model is not authoritative for read denials. ## Landed prerequisites These prerequisite PRs are already on `main`: 1. #15979 `feat(permissions): add glob deny-read policy support` 2. #18096 `feat(sandbox): add glob deny-read platform enforcement` 3. #17740 `feat(config): support managed deny-read requirements` This PR targets `main` directly and contains only the Windows deny-read enforcement layer. ## Implementation notes - Exact deny-read paths remain enforceable on the elevated path even when they do not exist yet: Windows materializes the missing path before applying the deny ACE, so the sandboxed command cannot create and read it during the same run. - Existing exact deny paths are preserved lexically until the ACL planner, which then adds the canonical target as a second ACL target when needed. That keeps both the configured alias and the resolved object covered. - Windows ACLs do not consume Codex glob syntax directly, so glob deny-read entries are expanded to the concrete matches that exist before process launch. - Glob traversal deduplicates directory visits within each pattern walk to avoid cycles, without collapsing distinct lexical roots that happen to resolve to the same target. - Persistent deny-read ACL state is keyed by sandbox principal SID, so cleanup only removes ACEs owned by the same backend principal. - Deny-read ACEs are fail-closed on the elevated path: setup aborts if mandatory deny-read ACL application fails. - Unelevated restricted-token sessions reject deny-read overrides early instead of running with a silently unenforceable read policy. ## Verification - `cargo test -p codex-core windows_restricted_token_rejects_unreadable_split_carveouts` - `just fmt` - `just fix -p codex-core` - `just fix -p codex-windows-sandbox` - GitHub Actions rerun is in progress on the pushed head. --------- Co-authored-by: Codex --- codex-rs/cli/src/debug_sandbox.rs | 7 + codex-rs/core/BUILD.bazel | 2 + codex-rs/core/src/config/mod.rs | 6 - codex-rs/core/src/exec.rs | 101 ++--- codex-rs/core/src/exec_tests.rs | 103 ++++- .../core/src/unified_exec/process_manager.rs | 29 ++ codex-rs/core/tests/suite/mod.rs | 2 + codex-rs/core/tests/suite/windows_sandbox.rs | 243 +++++++++++ codex-rs/protocol/src/permissions.rs | 70 +++- codex-rs/rmcp-client/src/program_resolver.rs | 21 +- codex-rs/windows-sandbox-rs/src/acl.rs | 109 ++++- .../src/bin/setup_main/win.rs | 88 ++-- .../windows-sandbox-rs/src/deny_read_acl.rs | 119 ++++++ .../src/deny_read_resolver.rs | 381 ++++++++++++++++++ .../windows-sandbox-rs/src/deny_read_state.rs | 87 ++++ .../windows-sandbox-rs/src/elevated_impl.rs | 17 +- codex-rs/windows-sandbox-rs/src/identity.rs | 3 + codex-rs/windows-sandbox-rs/src/lib.rs | 134 ++++-- codex-rs/windows-sandbox-rs/src/setup.rs | 38 +- codex-rs/windows-sandbox-rs/src/spawn_prep.rs | 57 ++- .../src/unified_exec/backends/elevated.rs | 20 + .../src/unified_exec/backends/legacy.rs | 22 +- .../src/unified_exec/mod.rs | 16 + .../src/unified_exec/tests.rs | 49 +++ 24 files changed, 1548 insertions(+), 176 deletions(-) create mode 100644 codex-rs/core/tests/suite/windows_sandbox.rs create mode 100644 codex-rs/windows-sandbox-rs/src/deny_read_acl.rs create mode 100644 codex-rs/windows-sandbox-rs/src/deny_read_resolver.rs create mode 100644 codex-rs/windows-sandbox-rs/src/deny_read_state.rs diff --git a/codex-rs/cli/src/debug_sandbox.rs b/codex-rs/cli/src/debug_sandbox.rs index e9bc6a046..2722f6984 100644 --- a/codex-rs/cli/src/debug_sandbox.rs +++ b/codex-rs/cli/src/debug_sandbox.rs @@ -370,6 +370,11 @@ async fn run_command_under_windows_session( cwd.as_path(), env, None, + /*read_roots_override*/ None, + /*read_roots_include_platform_defaults*/ false, + /*write_roots_override*/ None, + /*deny_read_paths_override*/ &[], + /*deny_write_paths_override*/ &[], /*tty*/ false, /*stdin_open*/ true, config.permissions.windows_sandbox_private_desktop, @@ -384,6 +389,8 @@ async fn run_command_under_windows_session( cwd.as_path(), env, None, + /*additional_deny_read_paths*/ &[], + /*additional_deny_write_paths*/ &[], /*tty*/ false, /*stdin_open*/ true, config.permissions.windows_sandbox_private_desktop, diff --git a/codex-rs/core/BUILD.bazel b/codex-rs/core/BUILD.bazel index c78750576..e826027cb 100644 --- a/codex-rs/core/BUILD.bazel +++ b/codex-rs/core/BUILD.bazel @@ -57,5 +57,7 @@ codex_rust_crate( "//codex-rs/rmcp-client:test_stdio_server", "//codex-rs/rmcp-client:test_streamable_http_server", "//codex-rs/cli:codex", + "//codex-rs/windows-sandbox-rs:codex-command-runner", + "//codex-rs/windows-sandbox-rs:codex-windows-sandbox-setup", ], ) diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index b22158012..c65b713a9 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -2905,12 +2905,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 fd5cd7bcd..a6d958b10 100644 --- a/codex-rs/core/src/exec.rs +++ b/codex-rs/core/src/exec.rs @@ -97,17 +97,19 @@ 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 roots that the -/// elevated setup path needs to launch the sandboxed command. Split policies -/// that opt into platform defaults carry that explicitly with the override. +/// The elevated Windows backend consumes extra deny-read paths plus explicit +/// read and write roots during setup/refresh. The unelevated restricted-token +/// backend only consumes extra deny-write carveouts on top of the legacy +/// `WorkspaceWrite` allow set. Read-root overrides are layered on top of the +/// baseline helper roots that the elevated setup path needs to launch the +/// sandboxed command; split policies that opt into platform defaults carry +/// that explicitly with the override. #[derive(Debug, Clone, PartialEq, Eq)] pub(crate) struct WindowsSandboxFilesystemOverrides { pub(crate) read_roots_override: Option>, pub(crate) read_roots_include_platform_defaults: bool, pub(crate) write_roots_override: Option>, + pub(crate) additional_deny_read_paths: Vec, pub(crate) additional_deny_write_paths: Vec, } @@ -564,7 +566,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, @@ -605,13 +607,10 @@ 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::>() - }) + .map(|overrides| overrides.additional_deny_write_paths.clone()) + .unwrap_or_default(); + let additional_deny_read_paths = windows_sandbox_filesystem_overrides + .map(|overrides| overrides.additional_deny_read_paths.clone()) .unwrap_or_default(); let elevated_read_roots_override = windows_sandbox_filesystem_overrides .and_then(|overrides| overrides.read_roots_override.clone()); @@ -619,15 +618,6 @@ async fn exec_windows_sandbox( .is_some_and(|overrides| overrides.read_roots_include_platform_defaults); 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( @@ -645,11 +635,12 @@ async fn exec_windows_sandbox( read_roots_include_platform_defaults: elevated_read_roots_include_platform_defaults, 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(), @@ -657,6 +648,7 @@ async fn exec_windows_sandbox( &cwd, env, timeout_ms, + &additional_deny_read_paths, &additional_deny_write_paths, windows_sandbox_private_desktop, ) @@ -1049,22 +1041,24 @@ pub(crate) fn resolve_windows_restricted_token_filesystem_overrides( )); } - if !file_system_sandbox_policy.has_full_disk_read_access() { + // The restricted-token backend can still enforce split write restrictions, + // but its WRITE_RESTRICTED token does not make capability SID deny-read ACEs + // participate in read access checks. Read restrictions therefore require the + // elevated backend, even when the filesystem root remains readable. + 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() - { + let additional_deny_read_paths = codex_windows_sandbox::resolve_windows_deny_read_paths( + file_system_sandbox_policy, + sandbox_policy_cwd, + )?; + if !additional_deny_read_paths.is_empty() { return Err( - "windows unelevated restricted-token sandbox cannot enforce unreadable split filesystem carveouts directly; refusing to run unsandboxed" + "windows unelevated restricted-token sandbox cannot enforce deny-read restrictions directly; refusing to run unsandboxed" .to_string(), ); } @@ -1131,7 +1125,7 @@ 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); } @@ -1139,6 +1133,7 @@ pub(crate) fn resolve_windows_restricted_token_filesystem_overrides( read_roots_override: None, read_roots_include_platform_defaults: false, 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())) @@ -1152,6 +1147,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, @@ -1175,18 +1180,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); @@ -1217,7 +1214,13 @@ pub(crate) fn resolve_windows_elevated_filesystem_overrides( .collect(); let split_root_path_set: BTreeSet = split_root_paths.iter().cloned().collect(); - let read_roots_override = if file_system_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 read_roots_override = if split_has_root_read_access { None } else { Some(split_readable_roots) @@ -1265,6 +1268,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); @@ -1275,6 +1279,7 @@ pub(crate) fn resolve_windows_elevated_filesystem_overrides( && file_system_sandbox_policy.include_platform_defaults(), 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 9d335a81c..1e802121a 100644 --- a/codex-rs/core/src/exec_tests.rs +++ b/codex-rs/core/src/exec_tests.rs @@ -662,11 +662,63 @@ fn windows_restricted_token_supports_full_read_split_write_read_carveouts() { read_roots_override: None, read_roots_include_platform_defaults: false, write_roots_override: None, + additional_deny_read_paths: vec![], additional_deny_write_paths: expected_deny_write_paths, })) ); } +#[test] +fn windows_restricted_token_rejects_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![], + 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::project_roots( + /*subpath*/ None, + ), + }, + access: codex_protocol::permissions::FileSystemAccessMode::Write, + }, + codex_protocol::permissions::FileSystemSandboxEntry { + path: codex_protocol::permissions::FileSystemPath::Path { path: blocked }, + 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, + ), + Err( + "windows unelevated restricted-token sandbox cannot enforce deny-read restrictions directly; refusing to run unsandboxed" + .to_string() + ) + ); +} + #[test] fn windows_elevated_supports_split_restricted_read_roots() { let temp_dir = tempfile::TempDir::new().expect("tempdir"); @@ -699,6 +751,7 @@ fn windows_elevated_supports_split_restricted_read_roots() { read_roots_override: Some(vec![expected_docs]), read_roots_include_platform_defaults: false, write_roots_override: None, + additional_deny_read_paths: vec![], additional_deny_write_paths: vec![], })) ); @@ -753,6 +806,7 @@ fn windows_elevated_supports_split_write_read_carveouts() { read_roots_override: None, read_roots_include_platform_defaults: false, 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"), @@ -762,10 +816,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![], network_access: false, @@ -797,24 +852,38 @@ 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, + read_roots_include_platform_defaults: false, + 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![], network_access: false, @@ -845,18 +914,24 @@ 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, + read_roots_include_platform_defaults: false, + 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/src/unified_exec/process_manager.rs b/codex-rs/core/src/unified_exec/process_manager.rs index 64cb74f60..fa1288be4 100644 --- a/codex-rs/core/src/unified_exec/process_manager.rs +++ b/codex-rs/core/src/unified_exec/process_manager.rs @@ -875,6 +875,28 @@ impl UnifiedExecProcessManager { "windows sandbox: failed to resolve codex_home: {err}" )) })?; + let additional_deny_write_paths = request + .windows_sandbox_filesystem_overrides + .as_ref() + .map(|overrides| overrides.additional_deny_write_paths.clone()) + .unwrap_or_default(); + let additional_deny_read_paths = request + .windows_sandbox_filesystem_overrides + .as_ref() + .map(|overrides| overrides.additional_deny_read_paths.clone()) + .unwrap_or_default(); + let elevated_read_roots_override = request + .windows_sandbox_filesystem_overrides + .as_ref() + .and_then(|overrides| overrides.read_roots_override.clone()); + let elevated_read_roots_include_platform_defaults = request + .windows_sandbox_filesystem_overrides + .as_ref() + .is_some_and(|overrides| overrides.read_roots_include_platform_defaults); + let elevated_write_roots_override = request + .windows_sandbox_filesystem_overrides + .as_ref() + .and_then(|overrides| overrides.write_roots_override.clone()); let spawned = match request.windows_sandbox_level { codex_protocol::config_types::WindowsSandboxLevel::Elevated => { codex_windows_sandbox::spawn_windows_sandbox_session_elevated( @@ -885,6 +907,11 @@ impl UnifiedExecProcessManager { request.cwd.as_path(), request.env.clone(), None, + elevated_read_roots_override.as_deref(), + elevated_read_roots_include_platform_defaults, + elevated_write_roots_override.as_deref(), + &additional_deny_read_paths, + &additional_deny_write_paths, tty, tty, request.windows_sandbox_private_desktop, @@ -901,6 +928,8 @@ impl UnifiedExecProcessManager { request.cwd.as_path(), request.env.clone(), None, + &additional_deny_read_paths, + &additional_deny_write_paths, tty, tty, request.windows_sandbox_private_desktop, diff --git a/codex-rs/core/tests/suite/mod.rs b/codex-rs/core/tests/suite/mod.rs index 843b558eb..2b5caf5a5 100644 --- a/codex-rs/core/tests/suite/mod.rs +++ b/codex-rs/core/tests/suite/mod.rs @@ -114,3 +114,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 000000000..d0919b3de --- /dev/null +++ b/codex-rs/core/tests/suite/windows_sandbox.rs @@ -0,0 +1,243 @@ +use anyhow::Context; +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::exec_output::ExecToolCallOutput; +use codex_protocol::models::PermissionProfile; +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 core_test_support::PathExt; +use pretty_assertions::assert_eq; +use serial_test::serial; +use std::collections::HashMap; +use std::ffi::OsString; +use std::path::Path; +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), + } + } + } +} + +fn stage_windows_sandbox_helpers() -> anyhow::Result<()> { + let test_exe = std::env::current_exe().context("resolve current Windows test executable")?; + let test_exe_dir = test_exe + .parent() + .context("Windows test executable should have a parent directory")?; + let resources_dir = test_exe_dir.join("codex-resources"); + std::fs::create_dir_all(&resources_dir)?; + for helper_name in ["codex-windows-sandbox-setup", "codex-command-runner"] { + let helper = codex_utils_cargo_bin::cargo_bin(helper_name)?; + let file_name = Path::new(helper_name).with_extension("exe"); + std::fs::copy(helper, resources_dir.join(file_name))?; + } + Ok(()) +} + +#[tokio::test] +#[serial(codex_home)] +async fn windows_restricted_token_rejects_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 = dunce::canonicalize(workspace.path())?.abs(); + let secret = cwd.join("secret.env"); + let future_secret = cwd.join("future.env"); + let public = cwd.join("public.txt"); + std::fs::write(&secret, "glob secret\n")?; + std::fs::write(&public, "public ok\n")?; + + let file_system_sandbox_policy = FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: FileSystemSpecialPath::Root, + }, + access: FileSystemAccessMode::Read, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: FileSystemSpecialPath::project_roots(/*subpath*/ None), + }, + 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 permission_profile = PermissionProfile::from_runtime_permissions( + &file_system_sandbox_policy, + NetworkSandboxPolicy::Restricted, + ); + + let err = 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, + }, + &permission_profile, + &cwd, + &None, + /*use_legacy_landlock*/ false, + /*stdout_stream*/ None, + ) + .await + .expect_err("restricted-token sandbox should reject deny-read restrictions"); + + assert_eq!( + err.to_string(), + "unsupported operation: windows unelevated restricted-token sandbox cannot enforce deny-read restrictions directly; refusing to run unsandboxed" + ); + Ok(()) +} + +#[tokio::test] +#[serial(codex_home)] +async fn windows_elevated_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()); + stage_windows_sandbox_helpers()?; + let workspace = TempDir::new()?; + let cwd = dunce::canonicalize(workspace.path())?.abs(); + let glob_secret = cwd.join("secret.env"); + let exact_secret = cwd.join("exact-secret.txt"); + let public = cwd.join("public.txt"); + std::fs::write(&glob_secret, "glob secret\n")?; + std::fs::write(&exact_secret, "exact secret\n")?; + std::fs::write(&public, "public ok\n")?; + + let file_system_sandbox_policy = FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: FileSystemSpecialPath::Root, + }, + access: FileSystemAccessMode::Read, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: FileSystemSpecialPath::project_roots(/*subpath*/ None), + }, + access: FileSystemAccessMode::Write, + }, + FileSystemSandboxEntry { + path: FileSystemPath::GlobPattern { + pattern: "**/*.env".to_string(), + }, + access: FileSystemAccessMode::None, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { path: exact_secret }, + access: FileSystemAccessMode::None, + }, + ]); + let permission_profile = PermissionProfile::from_runtime_permissions( + &file_system_sandbox_policy, + NetworkSandboxPolicy::Restricted, + ); + + let ExecToolCallOutput { + exit_code, + stdout, + stderr, + .. + } = process_exec_tool_call( + ExecParams { + command: vec![ + "cmd.exe".to_string(), + "/D".to_string(), + "/C".to_string(), + "(type secret.env 1>NUL 2>NUL && echo GLOB-READ || echo GLOB-DENIED) & (type exact-secret.txt 1>NUL 2>NUL && echo EXACT-READ || echo EXACT-DENIED) & type public.txt".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::Elevated, + windows_sandbox_private_desktop: false, + justification: None, + arg0: None, + }, + &permission_profile, + &cwd, + &None, + /*use_legacy_landlock*/ false, + /*stdout_stream*/ None, + ) + .await?; + + assert_eq!(exit_code, 0, "sandboxed command should complete"); + assert!( + stdout.text.contains("GLOB-DENIED"), + "glob deny-read should block the secret: {stdout:?}" + ); + assert!( + !stdout.text.contains("GLOB-READ"), + "glob deny-read should not allow the secret: {stdout:?}" + ); + assert!( + stdout.text.contains("EXACT-DENIED"), + "exact deny-read should block the secret: {stdout:?}" + ); + assert!( + !stdout.text.contains("EXACT-READ"), + "exact deny-read should not allow the secret: {stdout:?}" + ); + assert!( + stdout.text.contains("public ok"), + "allowed reads should still work: {stdout:?}" + ); + assert_eq!(stderr.text, ""); + Ok(()) +} diff --git a/codex-rs/protocol/src/permissions.rs b/codex-rs/protocol/src/permissions.rs index c1f3d0724..abbfc0b19 100644 --- a/codex-rs/protocol/src/permissions.rs +++ b/codex-rs/protocol/src/permissions.rs @@ -232,8 +232,39 @@ impl ReadDenyMatcher { /// can skip read-deny checks without allocating matcher state. The `cwd` /// resolves cwd-relative policy paths and special paths before matching. pub fn new(file_system_sandbox_policy: &FileSystemSandboxPolicy, cwd: &Path) -> Option { + match Self::build( + file_system_sandbox_policy, + cwd, + InvalidDenyReadGlobBehavior::FailClosed, + ) { + Ok(matcher) => matcher, + Err(_) => unreachable!("fail-closed glob handling does not return errors"), + } + } + + /// Builds a matcher for callers that must reject malformed glob patterns. + /// + /// Runtime read checks intentionally fail closed on malformed deny patterns. + /// Host-side expansion work should use this constructor instead so a typo + /// cannot broaden the set of paths it mutates before execution starts. + pub fn try_new( + file_system_sandbox_policy: &FileSystemSandboxPolicy, + cwd: &Path, + ) -> Result, String> { + Self::build( + file_system_sandbox_policy, + cwd, + InvalidDenyReadGlobBehavior::ReturnError, + ) + } + + fn build( + file_system_sandbox_policy: &FileSystemSandboxPolicy, + cwd: &Path, + invalid_glob_behavior: InvalidDenyReadGlobBehavior, + ) -> Result, String> { if !file_system_sandbox_policy.has_denied_read_restrictions() { - return None; + return Ok(None); } // Exact roots are stored as all meaningful path spellings we can derive @@ -247,22 +278,23 @@ impl ReadDenyMatcher { // Pattern entries stay as policy-level globs. They are matched at read // time here instead of being snapshotted to startup filesystem state. let mut invalid_pattern = false; - let deny_read_matchers = file_system_sandbox_policy - .get_unreadable_globs_with_cwd(cwd) - .into_iter() - .filter_map(|pattern| match build_glob_matcher(&pattern) { - Some(matcher) => Some(matcher), - None => { - invalid_pattern = true; - None - } - }) - .collect(); - Some(Self { + let mut deny_read_matchers = Vec::new(); + for pattern in file_system_sandbox_policy.get_unreadable_globs_with_cwd(cwd) { + match build_glob_matcher(&pattern) { + Ok(matcher) => deny_read_matchers.push(matcher), + Err(err) => match invalid_glob_behavior { + InvalidDenyReadGlobBehavior::FailClosed => invalid_pattern = true, + InvalidDenyReadGlobBehavior::ReturnError => { + return Err(format!("invalid deny-read glob pattern `{pattern}`: {err}")); + } + }, + } + } + Ok(Some(Self { denied_candidates, deny_read_matchers, invalid_pattern, - }) + })) } /// Returns whether `path` is denied by the policy used to build this matcher. @@ -295,6 +327,12 @@ impl ReadDenyMatcher { } } +#[derive(Clone, Copy)] +enum InvalidDenyReadGlobBehavior { + FailClosed, + ReturnError, +} + #[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize, JsonSchema, TS)] #[serde(tag = "type", rename_all = "snake_case")] #[ts(tag = "type")] @@ -1291,15 +1329,15 @@ fn push_unique(candidates: &mut Vec, candidate: PathBuf) { } } -fn build_glob_matcher(pattern: &str) -> Option { +fn build_glob_matcher(pattern: &str) -> Result { // Keep `*` and `?` within a single path component and preserve an unclosed // `[` as a literal so matcher behavior stays aligned with config parsing. GlobBuilder::new(pattern) .literal_separator(true) .allow_unclosed_class(true) .build() - .ok() .map(|glob| glob.compile_matcher()) + .map_err(|err| err.to_string()) } fn resolve_file_system_special_path( diff --git a/codex-rs/rmcp-client/src/program_resolver.rs b/codex-rs/rmcp-client/src/program_resolver.rs index 3666b1871..395b330d7 100644 --- a/codex-rs/rmcp-client/src/program_resolver.rs +++ b/codex-rs/rmcp-client/src/program_resolver.rs @@ -75,10 +75,25 @@ mod tests { #[tokio::test] async fn test_unix_executes_script_without_extension() -> Result<()> { let env = TestExecutableEnv::new()?; - let mut cmd = Command::new(&env.program_name); - cmd.envs(&env.mcp_env); + // Linux can transiently report ETXTBSY while the freshly written test + // script is becoming executable on the backing filesystem. + let mut retries = 0; + let output = loop { + let mut cmd = Command::new(&env.program_name); + cmd.envs(&env.mcp_env); + + let output = cmd.output().await; + if !output + .as_ref() + .is_err_and(|err| err.kind() == std::io::ErrorKind::ExecutableFileBusy) + || retries == 2 + { + break output; + } + retries += 1; + tokio::time::sleep(std::time::Duration::from_millis(10)).await; + }; - let output = cmd.output().await; assert!( output.is_ok(), "Unix should execute PATH-resolved scripts directly: {output:?}" diff --git a/codex-rs/windows-sandbox-rs/src/acl.rs b/codex-rs/windows-sandbox-rs/src/acl.rs index 68a17c979..a368371dc 100644 --- a/codex-rs/windows-sandbox-rs/src/acl.rs +++ b/codex-rs/windows-sandbox-rs/src/acl.rs @@ -9,6 +9,7 @@ use windows_sys::Win32::Foundation::HLOCAL; use windows_sys::Win32::Foundation::INVALID_HANDLE_VALUE; use windows_sys::Win32::Foundation::LocalFree; use windows_sys::Win32::Security::ACCESS_ALLOWED_ACE; +use windows_sys::Win32::Security::ACCESS_DENIED_ACE; use windows_sys::Win32::Security::ACE_HEADER; use windows_sys::Win32::Security::ACL; use windows_sys::Win32::Security::ACL_SIZE_INFORMATION; @@ -48,6 +49,9 @@ use windows_sys::Win32::Storage::FileSystem::OPEN_EXISTING; use windows_sys::Win32::Storage::FileSystem::READ_CONTROL; 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 | DELETE | FILE_DELETE_CHILD; @@ -445,6 +487,41 @@ pub unsafe fn add_allow_ace(path: &Path, psid: *mut c_void) -> Result { /// # Safety /// Caller must ensure `psid` points to a valid SID and `path` refers to an existing file or directory. pub unsafe fn add_deny_write_ace(path: &Path, psid: *mut c_void) -> Result { + add_deny_ace(path, psid, DenyAceKind::Write) +} + +#[derive(Clone, Copy)] +enum DenyAceKind { + Read, + Write, +} + +impl DenyAceKind { + fn mask(self) -> u32 { + match self { + Self::Read => FILE_GENERIC_READ | GENERIC_READ_MASK, + Self::Write => { + FILE_GENERIC_WRITE + | FILE_WRITE_DATA + | FILE_APPEND_DATA + | FILE_WRITE_EA + | FILE_WRITE_ATTRIBUTES + | GENERIC_WRITE_MASK + | DELETE + | FILE_DELETE_CHILD + } + } + } + + unsafe fn already_present(self, p_dacl: *mut ACL, psid: *mut c_void) -> bool { + match self { + Self::Read => dacl_has_read_deny_for_sid(p_dacl, psid), + Self::Write => dacl_has_write_deny_for_sid(p_dacl, psid), + } + } +} + +unsafe fn add_deny_ace(path: &Path, psid: *mut c_void, kind: DenyAceKind) -> 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( @@ -461,7 +538,7 @@ pub unsafe fn add_deny_write_ace(path: &Path, psid: *mut c_void) -> Result return Err(anyhow!("GetNamedSecurityInfoW failed: {code}")); } let mut added = false; - if !dacl_has_write_deny_for_sid(p_dacl, psid) { + if !kind.already_present(p_dacl, psid) { let trustee = TRUSTEE_W { pMultipleTrustee: std::ptr::null_mut(), MultipleTrusteeOperation: 0, @@ -470,14 +547,7 @@ pub unsafe fn add_deny_write_ace(path: &Path, psid: *mut c_void) -> Result ptstrName: psid as *mut u16, }; let mut explicit: EXPLICIT_ACCESS_W = std::mem::zeroed(); - explicit.grfAccessPermissions = FILE_GENERIC_WRITE - | FILE_WRITE_DATA - | FILE_APPEND_DATA - | FILE_WRITE_EA - | FILE_WRITE_ATTRIBUTES - | GENERIC_WRITE_MASK - | DELETE - | FILE_DELETE_CHILD; + explicit.grfAccessPermissions = kind.mask(); explicit.grfAccessMode = DENY_ACCESS; explicit.grfInheritance = CONTAINER_INHERIT_ACE | OBJECT_INHERIT_ACE; explicit.Trustee = trustee; @@ -507,6 +577,19 @@ 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 { + add_deny_ace(path, psid, DenyAceKind::Read) +} + 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/bin/setup_main/win.rs b/codex-rs/windows-sandbox-rs/src/bin/setup_main/win.rs index 549eb2426..d862f33ec 100644 --- a/codex-rs/windows-sandbox-rs/src/bin/setup_main/win.rs +++ b/codex-rs/windows-sandbox-rs/src/bin/setup_main/win.rs @@ -27,6 +27,7 @@ use codex_windows_sandbox::sandbox_bin_dir; use codex_windows_sandbox::sandbox_dir; use codex_windows_sandbox::sandbox_secrets_dir; use codex_windows_sandbox::string_from_sid_bytes; +use codex_windows_sandbox::sync_persistent_deny_read_acls; use codex_windows_sandbox::to_wide; use codex_windows_sandbox::workspace_cap_sid_for_cwd; use codex_windows_sandbox::write_setup_error_report; @@ -85,6 +86,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)] @@ -460,39 +463,43 @@ 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, - )?; + 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); + } + } + } unsafe { if !sandbox_group_psid.is_null() { LocalFree(sandbox_group_psid as HLOCAL); } - 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 !refresh_errors.is_empty() { log_line( @@ -556,6 +563,8 @@ fn run_setup_full(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<( format!("convert sandbox users group SID to PSID failed: {err}"), )) })?; + let sandbox_group_sid_str = + string_from_sid_bytes(&sandbox_group_sid).map_err(anyhow::Error::msg)?; let caps = load_or_create_cap_sids(&payload.codex_home).map_err(|err| { anyhow::Error::new(SetupFailure::new( @@ -613,6 +622,25 @@ fn run_setup_full(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<( ); } + // Deny-read ACEs must be present before the sandboxed command starts. Apply + // them synchronously here instead of delegating them to the background + // helper used for read grants. + let applied_deny_read_paths = unsafe { + sync_persistent_deny_read_acls( + &payload.codex_home, + &sandbox_group_sid_str, + &payload.deny_read_paths, + sandbox_group_psid, + ) + } + .context("apply deny-read ACLs")?; + if !applied_deny_read_paths.is_empty() { + log_line( + log, + &format!("applied {} deny-read ACLs", applied_deny_read_paths.len()), + )?; + } + if payload.read_roots.is_empty() { log_line(log, "no read roots to grant; skipping read ACL helper")?; } else { @@ -654,8 +682,6 @@ fn run_setup_full(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<( } let cap_sid_str = caps.workspace; - let sandbox_group_sid_str = - string_from_sid_bytes(&sandbox_group_sid).map_err(anyhow::Error::msg)?; let write_mask = FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE | DELETE | FILE_DELETE_CHILD; let mut grant_tasks: Vec = Vec::new(); 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 000000000..dd3bc350e --- /dev/null +++ b/codex-rs/windows-sandbox-rs/src/deny_read_acl.rs @@ -0,0 +1,119 @@ +use crate::acl::add_deny_read_ace; +use crate::acl::revoke_ace; +use crate::path_normalization::canonicalize_path; +use anyhow::Context; +use anyhow::Result; +use std::collections::HashSet; +use std::ffi::c_void; +use std::path::Path; +use std::path::PathBuf; + +/// 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); + } +} + +pub(crate) fn lexical_path_key(path: &Path) -> String { + path.to_string_lossy() + .replace('\\', "/") + .trim_end_matches('/') + .to_ascii_lowercase() +} + +/// 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 = 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 000000000..1e24ca096 --- /dev/null +++ b/codex-rs/windows-sandbox-rs/src/deny_read_resolver.rs @@ -0,0 +1,381 @@ +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::try_new(&glob_policy, cwd.as_path())? else { + return Ok(paths); + }; + + for pattern in unreadable_globs { + let mut seen_scan_dirs = HashSet::new(); + let scan_plan = glob_scan_plan(&pattern, file_system_sandbox_policy.glob_scan_max_depth); + collect_existing_glob_matches( + &scan_plan.root, + &matcher, + &mut paths, + &mut seen, + &mut seen_scan_dirs, + scan_plan.max_depth, + /*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, configured_max_depth: Option) -> 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: effective_glob_scan_max_depth(pattern, configured_max_depth), + }; + }; + 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: effective_glob_scan_max_depth(pattern_suffix, configured_max_depth), + }; + } + GlobScanPlan { + root: PathBuf::from(literal_prefix[..separator_index].to_string()), + max_depth: effective_glob_scan_max_depth(pattern_suffix, configured_max_depth), + } +} + +fn effective_glob_scan_max_depth( + pattern_suffix: &str, + configured_max_depth: Option, +) -> Option { + let components = pattern_suffix + .split(['/', '\\']) + .filter(|component| !component.is_empty()) + .collect::>(); + if components.contains(&"**") { + return configured_max_depth; + } + Some(configured_max_depth.map_or(components.len(), |max_depth| { + max_depth.min(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; + + #[cfg(unix)] + use std::os::unix::fs::symlink; + + 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", /*configured_max_depth*/ None).root, + PathBuf::from("/tmp/work") + ); + assert_eq!( + glob_scan_plan( + r"C:\Users\dev\repo\**\*.env", + /*configured_max_depth*/ None, + ) + .root, + PathBuf::from(r"C:\Users\dev\repo") + ); + assert_eq!( + glob_scan_plan(r"C:\*.env", /*configured_max_depth*/ None).root, + PathBuf::from(r"C:\") + ); + } + + #[test] + fn scan_depth_is_bounded_for_non_recursive_globs() { + assert_eq!( + glob_scan_plan("/tmp/work/*.env", /*configured_max_depth*/ None).max_depth, + Some(1) + ); + assert_eq!( + glob_scan_plan("/tmp/work/*/*.env", /*configured_max_depth*/ None).max_depth, + Some(2) + ); + assert_eq!( + glob_scan_plan("/tmp/work/**/*.env", /*configured_max_depth*/ None).max_depth, + None + ); + } + + #[test] + fn configured_depth_caps_recursive_glob_scans() { + assert_eq!( + glob_scan_plan("/tmp/work/**/*.env", Some(2)).max_depth, + Some(2) + ); + assert_eq!( + glob_scan_plan("/tmp/work/*/*.env", Some(1)).max_depth, + Some(1) + ); + } + + #[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 invalid_glob_patterns_fail_before_expansion() { + let tmp = TempDir::new().expect("tempdir"); + let cwd = AbsolutePathBuf::from_absolute_path(tmp.path()).expect("absolute cwd"); + let policy = FileSystemSandboxPolicy::restricted(vec![unreadable_glob_entry(format!( + "{}/**/[z-a]", + tmp.path().display() + ))]); + + let err = resolve_windows_deny_read_paths(&policy, &cwd).expect_err("invalid glob"); + assert!( + err.contains("invalid deny-read glob pattern"), + "unexpected error: {err}" + ); + assert!(err.contains("invalid range"), "unexpected error: {err}"); + } + + #[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")] + ); + } + + #[cfg(unix)] + #[test] + fn aliased_glob_roots_each_preserve_their_lexical_matches() { + let tmp = TempDir::new().expect("tempdir"); + let cwd = AbsolutePathBuf::from_absolute_path(tmp.path()).expect("absolute cwd"); + let target = tmp.path().join("target"); + let alias_a = tmp.path().join("alias-a"); + let alias_b = tmp.path().join("alias-b"); + let secret = target.join("secret.env"); + std::fs::create_dir_all(&target).expect("create target"); + std::fs::write(&secret, "secret").expect("write secret"); + symlink(&target, &alias_a).expect("create alias a"); + symlink(&target, &alias_b).expect("create alias b"); + let policy = FileSystemSandboxPolicy::restricted(vec![ + unreadable_glob_entry(format!("{}/**/*.env", alias_a.display())), + unreadable_glob_entry(format!("{}/**/*.env", alias_b.display())), + ]); + + let actual: HashSet = resolve_windows_deny_read_paths(&policy, &cwd) + .expect("resolve") + .into_iter() + .map(AbsolutePathBuf::into_path_buf) + .collect(); + let expected = [alias_a.join("secret.env"), alias_b.join("secret.env")] + .into_iter() + .collect(); + + assert_eq!(actual, expected); + } +} diff --git a/codex-rs/windows-sandbox-rs/src/deny_read_state.rs b/codex-rs/windows-sandbox-rs/src/deny_read_state.rs new file mode 100644 index 000000000..fd073a4d4 --- /dev/null +++ b/codex-rs/windows-sandbox-rs/src/deny_read_state.rs @@ -0,0 +1,87 @@ +use crate::acl::revoke_ace; +use crate::deny_read_acl::apply_deny_read_acls; +use crate::deny_read_acl::lexical_path_key; +use crate::setup::sandbox_dir; +use anyhow::Context; +use anyhow::Result; +use serde::Deserialize; +use serde::Serialize; +use std::collections::BTreeMap; +use std::collections::HashSet; +use std::ffi::c_void; +use std::path::Path; +use std::path::PathBuf; + +const DENY_READ_ACL_STATE_FILE: &str = "deny_read_acl_state.json"; + +#[derive(Default, Deserialize, Serialize)] +struct PersistentDenyReadAclState { + principals: BTreeMap>, +} + +/// Reconciles the persistent deny-read ACEs owned by one sandbox principal. +/// +/// Workspace-write and elevated sandbox sessions intentionally leave ACLs in +/// place after a command exits, because descendants may outlive the launcher. +/// That makes the ACL set stateful across runs. Persist the paths applied for +/// each SID, apply the new desired set first, and only then revoke stale paths +/// from the same SID so profile changes do not leave old deny-read ACEs behind. +/// +/// # Safety +/// Caller must pass a valid SID pointer matching `principal_sid`. +pub unsafe fn sync_persistent_deny_read_acls( + codex_home: &Path, + principal_sid: &str, + desired_paths: &[PathBuf], + psid: *mut c_void, +) -> Result> { + let state_path = sandbox_dir(codex_home).join(DENY_READ_ACL_STATE_FILE); + let mut state = load_state(&state_path)?; + let previous_paths = state + .principals + .get(principal_sid) + .cloned() + .unwrap_or_default(); + + let applied_paths = unsafe { apply_deny_read_acls(desired_paths, psid) }?; + let desired_keys = applied_paths + .iter() + .map(|path| lexical_path_key(path)) + .collect::>(); + + for path in previous_paths { + if !desired_keys.contains(&lexical_path_key(&path)) { + revoke_ace(&path, psid); + } + } + + if applied_paths.is_empty() { + state.principals.remove(principal_sid); + } else { + state + .principals + .insert(principal_sid.to_string(), applied_paths.clone()); + } + store_state(&state_path, &state)?; + + Ok(applied_paths) +} + +fn load_state(path: &Path) -> Result { + match std::fs::read(path) { + Ok(bytes) => serde_json::from_slice(&bytes) + .with_context(|| format!("parse deny-read ACL state {}", path.display())), + Err(err) if err.kind() == std::io::ErrorKind::NotFound => { + Ok(PersistentDenyReadAclState::default()) + } + Err(err) => { + Err(err).with_context(|| format!("read deny-read ACL state {}", path.display())) + } + } +} + +fn store_state(path: &Path, state: &PersistentDenyReadAclState) -> Result<()> { + let bytes = serde_json::to_vec_pretty(state).context("serialize deny-read ACL state")?; + std::fs::write(path, bytes) + .with_context(|| format!("write deny-read ACL state {}", path.display())) +} diff --git a/codex-rs/windows-sandbox-rs/src/elevated_impl.rs b/codex-rs/windows-sandbox-rs/src/elevated_impl.rs index be9f1cfb9..e1ce8c1db 100644 --- a/codex-rs/windows-sandbox-rs/src/elevated_impl.rs +++ b/codex-rs/windows-sandbox-rs/src/elevated_impl.rs @@ -1,3 +1,4 @@ +use codex_utils_absolute_path::AbsolutePathBuf; use std::collections::HashMap; use std::path::Path; use std::path::PathBuf; @@ -15,7 +16,8 @@ pub struct ElevatedSandboxCaptureRequest<'a> { pub read_roots_override: Option<&'a [PathBuf]>, pub read_roots_include_platform_defaults: bool, pub write_roots_override: Option<&'a [PathBuf]>, - pub deny_write_paths_override: &'a [PathBuf], + pub deny_read_paths_override: &'a [AbsolutePathBuf], + pub deny_write_paths_override: &'a [AbsolutePathBuf], } mod windows_impl { @@ -41,6 +43,7 @@ mod windows_impl { use crate::sandbox_utils::inject_git_safe_directory; use crate::token::convert_string_sid_to_sid; use anyhow::Result; + use codex_utils_absolute_path::AbsolutePathBuf; use std::path::Path; pub use crate::windows_impl::CaptureResult; @@ -63,8 +66,17 @@ mod windows_impl { read_roots_override, read_roots_include_platform_defaults, write_roots_override, + deny_read_paths_override, deny_write_paths_override, } = request; + let deny_read_paths_override = deny_read_paths_override + .iter() + .map(AbsolutePathBuf::to_path_buf) + .collect::>(); + let deny_write_paths_override = deny_write_paths_override + .iter() + .map(AbsolutePathBuf::to_path_buf) + .collect::>(); let policy = parse_policy(policy_json_or_preset)?; normalize_null_device_env(&mut env_map); ensure_non_interactive_pager(&mut env_map); @@ -85,7 +97,8 @@ mod windows_impl { read_roots_override, read_roots_include_platform_defaults, write_roots_override, - deny_write_paths_override, + &deny_read_paths_override, + &deny_write_paths_override, proxy_enforced, )?; // Build capability SID for ACL grants. diff --git a/codex-rs/windows-sandbox-rs/src/identity.rs b/codex-rs/windows-sandbox-rs/src/identity.rs index 6e2d392b3..e49af3017 100644 --- a/codex-rs/windows-sandbox-rs/src/identity.rs +++ b/codex-rs/windows-sandbox-rs/src/identity.rs @@ -139,6 +139,7 @@ pub fn require_logon_sandbox_creds( read_roots_override: Option<&[PathBuf]>, read_roots_include_platform_defaults: bool, write_roots_override: Option<&[PathBuf]>, + deny_read_paths_override: &[PathBuf], deny_write_paths_override: &[PathBuf], proxy_enforced: bool, ) -> Result { @@ -201,6 +202,7 @@ pub fn require_logon_sandbox_creds( read_roots: Some(needed_read.clone()), read_roots_include_platform_defaults, 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()), }, )?; @@ -220,6 +222,7 @@ pub fn require_logon_sandbox_creds( read_roots: Some(needed_read), read_roots_include_platform_defaults, 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 6d1d8b213..78081b5e1 100644 --- a/codex-rs/windows-sandbox-rs/src/lib.rs +++ b/codex-rs/windows-sandbox-rs/src/lib.rs @@ -14,14 +14,14 @@ mod audit; #[cfg(target_os = "windows")] mod cap; #[cfg(target_os = "windows")] +mod deny_read_acl; +#[cfg(target_os = "windows")] +mod deny_read_state; +#[cfg(target_os = "windows")] mod desktop; #[cfg(target_os = "windows")] mod dpapi; #[cfg(target_os = "windows")] -mod elevated; -#[cfg(target_os = "windows")] -mod elevated_impl; -#[cfg(target_os = "windows")] mod env; #[cfg(target_os = "windows")] mod helper_materialization; @@ -36,22 +36,10 @@ mod path_normalization; #[cfg(target_os = "windows")] mod policy; #[cfg(target_os = "windows")] -mod proc_thread_attr; -#[cfg(target_os = "windows")] mod process; #[cfg(target_os = "windows")] -mod sandbox_utils; -#[cfg(target_os = "windows")] -mod setup; -#[cfg(target_os = "windows")] -mod setup_error; -#[cfg(target_os = "windows")] -mod spawn_prep; -#[cfg(target_os = "windows")] mod token; #[cfg(target_os = "windows")] -mod unified_exec; -#[cfg(target_os = "windows")] mod wfp; #[cfg(target_os = "windows")] mod wfp_setup; @@ -60,16 +48,46 @@ mod winutil; #[cfg(target_os = "windows")] mod workspace_acl; +mod deny_read_resolver; + #[cfg(target_os = "windows")] mod conpty; +#[cfg(target_os = "windows")] +mod elevated; + +#[cfg(target_os = "windows")] +mod elevated_impl; + +#[cfg(target_os = "windows")] +mod proc_thread_attr; + +#[cfg(target_os = "windows")] +mod sandbox_utils; + +#[cfg(target_os = "windows")] +mod setup; + +#[cfg(target_os = "windows")] +mod setup_error; + +#[cfg(target_os = "windows")] +mod spawn_prep; + +#[cfg(target_os = "windows")] +mod unified_exec; + #[cfg(target_os = "windows")] pub(crate) use elevated::ipc_framed; + #[cfg(target_os = "windows")] pub(crate) use elevated::runner_client; + #[cfg(target_os = "windows")] pub(crate) use elevated::runner_pipe; +#[cfg(target_os = "windows")] +pub use acl::add_deny_read_ace; #[cfg(target_os = "windows")] pub use acl::add_deny_write_ace; @@ -96,6 +114,13 @@ pub use conpty::ConptyInstance; #[cfg(target_os = "windows")] pub use conpty::spawn_conpty_process_as_user; #[cfg(target_os = "windows")] +pub use deny_read_acl::apply_deny_read_acls; +#[cfg(target_os = "windows")] +pub use deny_read_acl::plan_deny_read_acl_paths; +pub use deny_read_resolver::resolve_windows_deny_read_paths; +#[cfg(target_os = "windows")] +pub use deny_read_state::sync_persistent_deny_read_acls; +#[cfg(target_os = "windows")] pub use desktop::LaunchDesktop; #[cfg(target_os = "windows")] pub use dpapi::protect as dpapi_protect; @@ -225,7 +250,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")] @@ -256,6 +281,8 @@ 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::apply_deny_read_acls; + use super::deny_read_state::sync_persistent_deny_read_acls; use super::logging::log_failure; use super::logging::log_success; use super::path_normalization::canonicalize_path; @@ -266,7 +293,9 @@ mod windows_impl { use super::token::convert_string_sid_to_sid; use super::token::create_workspace_write_token_with_caps_from; use super::workspace_acl::is_command_cwd_root; + use anyhow::Context; use anyhow::Result; + use codex_utils_absolute_path::AbsolutePathBuf; use std::collections::HashMap; use std::ffi::c_void; use std::io; @@ -331,7 +360,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, @@ -340,12 +369,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, @@ -353,9 +383,18 @@ mod windows_impl { cwd: &Path, mut env_map: HashMap, timeout_ms: Option, - additional_deny_write_paths: &[PathBuf], + additional_deny_read_paths: &[AbsolutePathBuf], + additional_deny_write_paths: &[AbsolutePathBuf], use_private_desktop: bool, ) -> Result { + let additional_deny_read_paths = additional_deny_read_paths + .iter() + .map(AbsolutePathBuf::to_path_buf) + .collect::>(); + let additional_deny_write_paths = additional_deny_write_paths + .iter() + .map(AbsolutePathBuf::to_path_buf) + .collect::>(); let common = prepare_legacy_spawn_context( policy_json_or_preset, codex_home, @@ -374,6 +413,11 @@ mod windows_impl { "Restricted read-only access requires the elevated Windows sandbox backend" ); } + // WRITE_RESTRICTED tokens consult restricting SIDs only for writes, so this + // backend cannot make capability-SID deny-read ACLs authoritative. + if !additional_deny_read_paths.is_empty() { + anyhow::bail!("deny-read overrides require the elevated Windows sandbox backend"); + } let caps = load_or_create_cap_sids(codex_home)?; let (h_token, psid_generic, psid_workspace): (HANDLE, *mut c_void, Option<*mut c_void>) = unsafe { match &policy { @@ -424,9 +468,14 @@ mod windows_impl { let AllowDenyPaths { allow, mut deny } = compute_allow_paths(&policy, sandbox_policy_cwd, ¤t_dir, &env_map); for path in additional_deny_write_paths { - if path.exists() { - deny.insert(path.clone()); + // Explicit deny-write carveouts must already exist when the process + // starts, otherwise it could create a missing path under a writable + // parent before the deny-write ACE exists. + if !path.exists() { + std::fs::create_dir_all(&path) + .with_context(|| format!("create deny-write path {}", path.display()))?; } + deny.insert(path.clone()); } let canonical_cwd = canonicalize_path(¤t_dir); let mut guards: Vec<(PathBuf, *mut c_void)> = Vec::new(); @@ -457,6 +506,32 @@ 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 if persist_aces { + sync_persistent_deny_read_acls( + codex_home, + &caps.workspace, + &additional_deny_read_paths, + psid_generic, + ) + } else { + apply_deny_read_acls(&additional_deny_read_paths, psid_generic) + } { + Ok(paths) => paths, + Err(err) => { + if !persist_aces { + cleanup_acl_guards(&mut guards); + } + return Err(err); + } + }; + if !persist_aces { + for path in applied_deny_read_paths { + guards.push((path, psid_generic)); + } + } allow_null_device(psid_generic); if let Some(psid) = psid_workspace { allow_null_device(psid); @@ -478,6 +553,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); @@ -586,11 +662,7 @@ mod windows_impl { } if !persist_aces { - unsafe { - for (p, sid) in guards { - revoke_ace(&p, sid); - } - } + cleanup_acl_guards(&mut guards); } Ok(CaptureResult { exit_code, @@ -600,6 +672,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.rs b/codex-rs/windows-sandbox-rs/src/setup.rs index ef952a4e0..3b6e47086 100644 --- a/codex-rs/windows-sandbox-rs/src/setup.rs +++ b/codex-rs/windows-sandbox-rs/src/setup.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; @@ -96,6 +97,7 @@ pub struct SetupRootOverrides { pub read_roots: Option>, pub read_roots_include_platform_defaults: bool, pub write_roots: Option>, + pub deny_read_paths: Option>, pub deny_write_paths: Option>, } @@ -151,6 +153,7 @@ pub fn run_setup_refresh_with_extra_read_roots( read_roots: Some(read_roots), read_roots_include_platform_defaults: false, write_roots: Some(Vec::new()), + deny_read_paths: None, deny_write_paths: None, }, ) @@ -168,6 +171,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); @@ -180,6 +184,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, @@ -418,6 +423,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)] @@ -720,6 +727,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); @@ -732,6 +740,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, @@ -805,18 +814,18 @@ 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 { + // Keep the configured spelling here so the ACL layer can plan both the + // lexical path and any existing canonical target for reparse-point aliases. + explicit_deny_read_paths.unwrap_or_default() +} + fn expand_user_profile_root(roots: Vec) -> Vec { let Ok(user_profile) = std::env::var("USERPROFILE") else { return roots; @@ -1327,6 +1336,7 @@ mod tests { read_roots: Some(vec![readable_root.clone()]), read_roots_include_platform_defaults: true, write_roots: None, + deny_read_paths: None, deny_write_paths: None, }, ); @@ -1374,6 +1384,7 @@ mod tests { read_roots: Some(vec![readable_root.clone()]), read_roots_include_platform_defaults: false, write_roots: None, + deny_read_paths: None, deny_write_paths: None, }, ); @@ -1454,4 +1465,17 @@ mod tests { .all(|path| roots.contains(&path)) ); } + + #[test] + fn build_payload_deny_read_paths_preserves_explicit_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![existing, missing] + ); + } } diff --git a/codex-rs/windows-sandbox-rs/src/spawn_prep.rs b/codex-rs/windows-sandbox-rs/src/spawn_prep.rs index 1b3704d6a..b0cf0f307 100644 --- a/codex-rs/windows-sandbox-rs/src/spawn_prep.rs +++ b/codex-rs/windows-sandbox-rs/src/spawn_prep.rs @@ -5,6 +5,8 @@ use crate::allow::AllowDenyPaths; use crate::allow::compute_allow_paths; use crate::cap::load_or_create_cap_sids; use crate::cap::workspace_cap_sid_for_cwd; +use crate::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; use crate::env::inherit_path_env; @@ -25,6 +27,7 @@ use crate::token::get_logon_sid_bytes; use crate::workspace_acl::is_command_cwd_root; use crate::workspace_acl::protect_workspace_agents_dir; use crate::workspace_acl::protect_workspace_codex_dir; +use anyhow::Context; use anyhow::Result; use std::collections::HashMap; use std::ffi::c_void; @@ -211,20 +214,34 @@ pub(crate) fn allow_null_device_for_workspace_write(is_workspace_write: bool) { } } +#[allow(clippy::too_many_arguments)] pub(crate) fn apply_legacy_session_acl_rules( policy: &SandboxPolicy, sandbox_policy_cwd: &Path, + codex_home: &Path, current_dir: &Path, env_map: &HashMap, psid_generic: &LocalSid, psid_workspace: Option<&LocalSid>, + cap_sid_str: &str, + additional_deny_read_paths: &[PathBuf], + additional_deny_write_paths: &[PathBuf], persist_aces: bool, -) -> Vec { - let AllowDenyPaths { allow, deny } = +) -> Result> { + let AllowDenyPaths { allow, mut deny } = compute_allow_paths(policy, sandbox_policy_cwd, current_dir, env_map); let mut guards: Vec = Vec::new(); let canonical_cwd = canonicalize_path(current_dir); unsafe { + for path in additional_deny_write_paths { + // Explicit carveouts must exist before the command starts so the + // sandbox cannot create them under a writable parent first. + if !path.exists() { + std::fs::create_dir_all(path) + .with_context(|| format!("create deny-write path {}", path.display()))?; + } + deny.insert(path.clone()); + } for p in &allow { let psid = if matches!(policy, SandboxPolicy::WorkspaceWrite { .. }) && is_command_cwd_root(p, &canonical_cwd) @@ -245,6 +262,19 @@ pub(crate) fn apply_legacy_session_acl_rules( guards.push(p.clone()); } } + let applied_deny_read_paths = if persist_aces { + sync_persistent_deny_read_acls( + codex_home, + cap_sid_str, + additional_deny_read_paths, + psid_generic.as_ptr(), + )? + } else { + apply_deny_read_acls(additional_deny_read_paths, psid_generic.as_ptr())? + }; + if !persist_aces { + guards.extend(applied_deny_read_paths); + } allow_null_device(psid_generic.as_ptr()); if let Some(psid_workspace) = psid_workspace { allow_null_device(psid_workspace.as_ptr()); @@ -254,9 +284,10 @@ pub(crate) fn apply_legacy_session_acl_rules( } } } - guards + Ok(guards) } +#[allow(clippy::too_many_arguments)] pub(crate) fn prepare_elevated_spawn_context( policy_json_or_preset: &str, sandbox_policy_cwd: &Path, @@ -264,6 +295,11 @@ pub(crate) fn prepare_elevated_spawn_context( cwd: &Path, env_map: &mut HashMap, command: &[String], + read_roots_override: Option<&[PathBuf]>, + read_roots_include_platform_defaults: bool, + write_roots_override: Option<&[PathBuf]>, + deny_read_paths_override: &[PathBuf], + deny_write_paths_override: &[PathBuf], ) -> Result { let common = prepare_spawn_context_common( policy_json_or_preset, @@ -283,7 +319,7 @@ pub(crate) fn prepare_elevated_spawn_context( ); let write_roots: Vec = allow.into_iter().collect(); let deny_write_paths: Vec = deny.into_iter().collect(); - let write_roots_override = if common.is_workspace_write { + let computed_write_roots_override = if common.is_workspace_write { Some(write_roots.as_slice()) } else { None @@ -294,10 +330,15 @@ pub(crate) fn prepare_elevated_spawn_context( cwd, env_map, codex_home, - /*read_roots_override*/ None, - /*read_roots_include_platform_defaults*/ false, - write_roots_override, - &deny_write_paths, + read_roots_override, + read_roots_include_platform_defaults, + write_roots_override.or(computed_write_roots_override), + deny_read_paths_override, + if deny_write_paths_override.is_empty() { + &deny_write_paths + } else { + deny_write_paths_override + }, /*proxy_enforced*/ false, )?; let caps = load_or_create_cap_sids(codex_home)?; diff --git a/codex-rs/windows-sandbox-rs/src/unified_exec/backends/elevated.rs b/codex-rs/windows-sandbox-rs/src/unified_exec/backends/elevated.rs index 4ee2b5fac..0d7e94a1f 100644 --- a/codex-rs/windows-sandbox-rs/src/unified_exec/backends/elevated.rs +++ b/codex-rs/windows-sandbox-rs/src/unified_exec/backends/elevated.rs @@ -10,10 +10,12 @@ use crate::ipc_framed::SpawnRequest; use crate::runner_client::spawn_runner_transport; use crate::spawn_prep::prepare_elevated_spawn_context; use anyhow::Result; +use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_pty::ProcessDriver; use codex_utils_pty::SpawnedProcess; use std::collections::HashMap; use std::path::Path; +use std::path::PathBuf; use tokio::sync::broadcast; use tokio::sync::mpsc; use tokio::sync::oneshot; @@ -27,10 +29,23 @@ pub(crate) async fn spawn_windows_sandbox_session_elevated( cwd: &Path, mut env_map: HashMap, timeout_ms: Option, + read_roots_override: Option<&[PathBuf]>, + read_roots_include_platform_defaults: bool, + write_roots_override: Option<&[PathBuf]>, + deny_read_paths_override: &[AbsolutePathBuf], + deny_write_paths_override: &[AbsolutePathBuf], tty: bool, stdin_open: bool, use_private_desktop: bool, ) -> Result { + let deny_read_paths_override = deny_read_paths_override + .iter() + .map(AbsolutePathBuf::to_path_buf) + .collect::>(); + let deny_write_paths_override = deny_write_paths_override + .iter() + .map(AbsolutePathBuf::to_path_buf) + .collect::>(); let elevated = prepare_elevated_spawn_context( policy_json_or_preset, sandbox_policy_cwd, @@ -38,6 +53,11 @@ pub(crate) async fn spawn_windows_sandbox_session_elevated( cwd, &mut env_map, &command, + read_roots_override, + read_roots_include_platform_defaults, + write_roots_override, + &deny_read_paths_override, + &deny_write_paths_override, )?; let spawn_request = SpawnRequest { 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 8458d5c8c..81096e8c6 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 @@ -16,6 +16,7 @@ use crate::spawn_prep::apply_legacy_session_acl_rules; use crate::spawn_prep::prepare_legacy_session_security; use crate::spawn_prep::prepare_legacy_spawn_context; use anyhow::Result; +use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_pty::ProcessDriver; use codex_utils_pty::SpawnedProcess; use codex_utils_pty::TerminalSize; @@ -287,6 +288,8 @@ pub(crate) async fn spawn_windows_sandbox_session_legacy( cwd: &Path, mut env_map: HashMap, timeout_ms: Option, + additional_deny_read_paths: &[AbsolutePathBuf], + additional_deny_write_paths: &[AbsolutePathBuf], tty: bool, stdin_open: bool, use_private_desktop: bool, @@ -303,6 +306,19 @@ pub(crate) async fn spawn_windows_sandbox_session_legacy( if !common.policy.has_full_disk_read_access() { anyhow::bail!("Restricted read-only access requires the elevated Windows sandbox backend"); } + // WRITE_RESTRICTED tokens consult restricting SIDs only for writes, so this + // backend cannot make capability-SID deny-read ACLs authoritative. + if !additional_deny_read_paths.is_empty() { + anyhow::bail!("deny-read overrides require the elevated Windows sandbox backend"); + } + let additional_deny_read_paths = additional_deny_read_paths + .iter() + .map(AbsolutePathBuf::to_path_buf) + .collect::>(); + let additional_deny_write_paths = additional_deny_write_paths + .iter() + .map(AbsolutePathBuf::to_path_buf) + .collect::>(); let security = prepare_legacy_session_security(&common.policy, codex_home, cwd)?; allow_null_device_for_workspace_write(common.is_workspace_write); @@ -310,12 +326,16 @@ pub(crate) async fn spawn_windows_sandbox_session_legacy( let guards = apply_legacy_session_acl_rules( &common.policy, sandbox_policy_cwd, + codex_home, &common.current_dir, &env_map, &security.psid_generic, security.psid_workspace.as_ref(), + &security.cap_sid_str, + &additional_deny_read_paths, + &additional_deny_write_paths, persist_aces, - ); + )?; let (writer_tx, writer_rx) = mpsc::channel::>(128); let (stdout_tx, stdout_rx) = broadcast::channel::>(256); diff --git a/codex-rs/windows-sandbox-rs/src/unified_exec/mod.rs b/codex-rs/windows-sandbox-rs/src/unified_exec/mod.rs index ced69433d..adb653b6f 100644 --- a/codex-rs/windows-sandbox-rs/src/unified_exec/mod.rs +++ b/codex-rs/windows-sandbox-rs/src/unified_exec/mod.rs @@ -10,9 +10,11 @@ mod backends; use anyhow::Result; +use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_pty::SpawnedProcess; use std::collections::HashMap; use std::path::Path; +use std::path::PathBuf; #[allow(clippy::too_many_arguments)] pub async fn spawn_windows_sandbox_session_legacy( @@ -23,6 +25,8 @@ pub async fn spawn_windows_sandbox_session_legacy( cwd: &Path, env_map: HashMap, timeout_ms: Option, + additional_deny_read_paths: &[AbsolutePathBuf], + additional_deny_write_paths: &[AbsolutePathBuf], tty: bool, stdin_open: bool, use_private_desktop: bool, @@ -35,6 +39,8 @@ pub async fn spawn_windows_sandbox_session_legacy( cwd, env_map, timeout_ms, + additional_deny_read_paths, + additional_deny_write_paths, tty, stdin_open, use_private_desktop, @@ -51,6 +57,11 @@ pub async fn spawn_windows_sandbox_session_elevated( cwd: &Path, env_map: HashMap, timeout_ms: Option, + read_roots_override: Option<&[PathBuf]>, + read_roots_include_platform_defaults: bool, + write_roots_override: Option<&[PathBuf]>, + deny_read_paths_override: &[AbsolutePathBuf], + deny_write_paths_override: &[AbsolutePathBuf], tty: bool, stdin_open: bool, use_private_desktop: bool, @@ -63,6 +74,11 @@ pub async fn spawn_windows_sandbox_session_elevated( cwd, env_map, timeout_ms, + read_roots_override, + read_roots_include_platform_defaults, + write_roots_override, + deny_read_paths_override, + deny_write_paths_override, tty, stdin_open, use_private_desktop, diff --git a/codex-rs/windows-sandbox-rs/src/unified_exec/tests.rs b/codex-rs/windows-sandbox-rs/src/unified_exec/tests.rs index 66f21807b..9cd3d6d96 100644 --- a/codex-rs/windows-sandbox-rs/src/unified_exec/tests.rs +++ b/codex-rs/windows-sandbox-rs/src/unified_exec/tests.rs @@ -5,6 +5,7 @@ use crate::ipc_framed::Message; use crate::ipc_framed::decode_bytes; use crate::ipc_framed::read_frame; use crate::run_windows_sandbox_capture; +use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_pty::ProcessDriver; use pretty_assertions::assert_eq; use std::collections::HashMap; @@ -160,6 +161,8 @@ fn legacy_non_tty_cmd_emits_output() { cwd.as_path(), HashMap::new(), Some(5_000), + &[], + &[], /*tty*/ false, /*stdin_open*/ false, /*use_private_desktop*/ true, @@ -176,6 +179,44 @@ fn legacy_non_tty_cmd_emits_output() { }); } +#[test] +fn legacy_non_tty_cmd_rejects_deny_read_overrides() { + let _guard = legacy_process_test_guard(); + let runtime = current_thread_runtime(); + runtime.block_on(async move { + let cwd = sandbox_cwd(); + let codex_home = sandbox_home("legacy-non-tty-deny-read"); + let secret_path = + AbsolutePathBuf::from_absolute_path(cwd.join("legacy-non-tty-deny-read-secret.env")) + .expect("absolute deny-read fixture path"); + let err = spawn_windows_sandbox_session_legacy( + "workspace-write", + cwd.as_path(), + codex_home.path(), + vec![ + "C:\\Windows\\System32\\cmd.exe".to_string(), + "/c".to_string(), + "echo deny-read".to_string(), + ], + cwd.as_path(), + HashMap::new(), + Some(5_000), + std::slice::from_ref(&secret_path), + &[], + /*tty*/ false, + /*stdin_open*/ false, + /*use_private_desktop*/ true, + ) + .await + .expect_err("legacy deny-read should require the elevated backend"); + assert!( + err.to_string() + .contains("deny-read overrides require the elevated Windows sandbox backend"), + "unexpected error: {err:#}" + ); + }); +} + #[test] fn legacy_non_tty_powershell_emits_output() { let Some(pwsh) = pwsh_path() else { @@ -200,6 +241,8 @@ fn legacy_non_tty_powershell_emits_output() { cwd.as_path(), HashMap::new(), Some(5_000), + &[], + &[], /*tty*/ false, /*stdin_open*/ false, /*use_private_desktop*/ true, @@ -424,6 +467,8 @@ fn legacy_tty_powershell_emits_output_and_accepts_input() { cwd.as_path(), HashMap::new(), Some(10_000), + &[], + &[], /*tty*/ true, /*stdin_open*/ true, /*use_private_desktop*/ true, @@ -472,6 +517,8 @@ fn legacy_tty_cmd_emits_output_and_accepts_input() { cwd.as_path(), HashMap::new(), Some(10_000), + &[], + &[], /*tty*/ true, /*stdin_open*/ true, /*use_private_desktop*/ true, @@ -523,6 +570,8 @@ fn legacy_tty_cmd_default_desktop_emits_output_and_accepts_input() { cwd.as_path(), HashMap::new(), Some(10_000), + &[], + &[], /*tty*/ true, /*stdin_open*/ true, /*use_private_desktop*/ false,