From 5667598c8d8d57ae5dd2cbaeed1db2ceb2462c8d Mon Sep 17 00:00:00 2001 From: viyatb-oai Date: Tue, 3 Nov 2026 20:53:18 +0000 Subject: [PATCH] Use resolved filesystem permissions in Windows sandbox policies (#59233) ## What changed Use `resolve_for_enforcement` when preparing Windows restricted-token and elevated sandbox overrides, preserving read-only carveouts at volume roots. Expand deny-read ACL targets from the resolved permissions. Use the same resolution in MXC policy preparation when volume roots do not require materialization, while retaining special handling for symbolic root denies. ## Testing Add regression coverage for read-only carveouts in a volume-root workspace. Extend the Windows package smoke test to verify that a helper-created log matching a deny glob cannot be read while a neighboring allowed file remains readable. GitOrigin-RevId: cee0834444464b3a7b0c8dbeecb8abcd5828b4b0 --- codex-rs/mxc-sandbox/src/policy.rs | 32 ++++++++--- codex-rs/sandboxing/src/manager_tests.rs | 29 ++++++++++ codex-rs/sandboxing/src/windows.rs | 51 +++++++---------- .../src/deny_read_resolver.rs | 55 +++++++++++++----- .../src/deny_read_walker.rs | 2 +- codex-rs/windows-sandbox-rs/src/lib.rs | 1 + .../smoke_tests/test_codex_package.py | 57 ++++++++++++++++--- 7 files changed, 164 insertions(+), 63 deletions(-) diff --git a/codex-rs/mxc-sandbox/src/policy.rs b/codex-rs/mxc-sandbox/src/policy.rs index b78ecce864..f2e4253840 100644 --- a/codex-rs/mxc-sandbox/src/policy.rs +++ b/codex-rs/mxc-sandbox/src/policy.rs @@ -8,12 +8,14 @@ use std::collections::HashSet; use std::path::Path; use std::path::PathBuf; +use codex_protocol::permissions::FileSystemPolicyProjection; use codex_protocol::protocol::FileSystemAccessMode; use codex_protocol::protocol::FileSystemPath; use codex_protocol::protocol::FileSystemSandboxPolicy; use codex_protocol::protocol::FileSystemSpecialPath; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_path_uri::PathUri; +use codex_windows_sandbox::expand_windows_deny_read_paths; use codex_windows_sandbox::resolve_windows_deny_read_paths; use mxc_sdk::mxc_common::cmdline::CommandLineContext; use mxc_sdk::mxc_common::cmdline::CommandLineError; @@ -153,10 +155,26 @@ pub(super) fn build_request( #[cfg(not(windows))] let read_volumes = volumes.clone(); let fs = materialize_volume_roots(policy.clone(), &volumes, &read_volumes)?; - let roots = if full_disk_write { - Vec::new() + let absolute_cwd = AbsolutePathBuf::from_absolute_path(cwd) + .map_err(|error| PolicyError::PolicyResolution(error.to_string()))?; + let (roots, readable_roots, denied_paths) = if fs == policy { + let resolved = + fs.resolve_for_enforcement(cwd, FileSystemPolicyProjection::PreserveMutableRoots)?; + let denies = expand_windows_deny_read_paths(&resolved, &absolute_cwd); + (resolved.writable_roots, resolved.readable_roots, denies) } else { - fs.get_writable_roots_with_cwd_preserving_mutable_paths(cwd) + // A symbolic root deny sets the default on every volume. Expanding it into ACL + // targets would mask narrower grants and probe disconnected drives. + let roots = if full_disk_write { + Vec::new() + } else { + fs.get_writable_roots_with_cwd_preserving_mutable_paths(cwd) + }; + ( + roots, + fs.get_readable_roots_with_cwd(cwd), + resolve_windows_deny_read_paths(&policy, &absolute_cwd), + ) }; let mut write = collect_paths(roots.iter().map(|root| root.root.to_path_buf()))?; if full_disk_write { @@ -171,17 +189,13 @@ pub(super) fn build_request( } } let mut read = collect_paths( - fs.get_readable_roots_with_cwd(cwd) + readable_roots .into_iter() .filter(|path| !fs.can_write_local_path_with_cwd(path.as_path(), cwd)) .map(|path| path.to_path_buf()), )?; - let absolute_cwd = AbsolutePathBuf::from_absolute_path(cwd) - .map_err(|error| PolicyError::PolicyResolution(error.to_string()))?; - // A symbolic root deny is the default, not a recursive native mask that - // should erase narrower grants. Only materialize grants and carveouts. let deny = collect_paths( - resolve_windows_deny_read_paths(&policy, &absolute_cwd) + denied_paths .map_err(PolicyError::PolicyResolution)? .into_iter() .map(AbsolutePathBuf::into_path_buf), diff --git a/codex-rs/sandboxing/src/manager_tests.rs b/codex-rs/sandboxing/src/manager_tests.rs index 265849e53d..670117d859 100644 --- a/codex-rs/sandboxing/src/manager_tests.rs +++ b/codex-rs/sandboxing/src/manager_tests.rs @@ -842,3 +842,32 @@ fn transform_for_direct_spawn_windows_materializes_inner_helper() { ); assert!(materialized_helper.exists()); } + +#[test] +fn restricted_token_preserves_read_only_carveouts_at_a_volume_root() -> anyhow::Result<()> { + let cwd = AbsolutePathBuf::current_dir()?; + let root = AbsolutePathBuf::from_absolute_path(cwd.ancestors().last().expect("volume root"))?; + // Exercise policy preparation without installing ACLs on the test host. + let read_only = root.join("codex-test-read-only"); + let mut file_system = FileSystemSandboxPolicy::workspace_write( + &[], + /*exclude_tmpdir_env_var*/ true, + /*exclude_slash_tmp*/ true, + ); + file_system.entries.push(FileSystemSandboxEntry::new( + read_only.clone().into(), + FileSystemAccessMode::Read, + )); + let profile = + PermissionProfile::from_runtime_permissions(&file_system, NetworkSandboxPolicy::Restricted); + let overrides = crate::resolve_windows_restricted_token_filesystem_overrides( + SandboxType::WindowsRestrictedToken, + &profile, + &root, + WindowsSandboxLevel::RestrictedToken, + ) + .expect("prepare volume-root workspace") + .expect("read-only child"); + assert_eq!(overrides.additional_deny_write_paths, [read_only]); + Ok(()) +} diff --git a/codex-rs/sandboxing/src/windows.rs b/codex-rs/sandboxing/src/windows.rs index 0f9df8191a..af65c89e32 100644 --- a/codex-rs/sandboxing/src/windows.rs +++ b/codex-rs/sandboxing/src/windows.rs @@ -5,6 +5,7 @@ use std::path::PathBuf; use codex_protocol::config_types::WindowsSandboxLevel; use codex_protocol::models::PermissionProfile; +use codex_protocol::permissions::FileSystemPolicyProjection; use codex_protocol::permissions::FileSystemSandboxPolicy; use codex_protocol::protocol::WritableRoot; use codex_utils_absolute_path::AbsolutePathBuf; @@ -119,10 +120,11 @@ pub fn resolve_windows_restricted_token_filesystem_overrides( ); } - let additional_deny_read_paths = codex_windows_sandbox::resolve_windows_deny_read_paths( - &file_system_sandbox_policy, - sandbox_policy_cwd, - )?; + let resolved = file_system_sandbox_policy + .resolve_for_enforcement(sandbox_policy_cwd, FileSystemPolicyProjection::Native) + .map_err(|err| err.to_string())?; + let additional_deny_read_paths = + codex_windows_sandbox::expand_windows_deny_read_paths(&resolved, sandbox_policy_cwd)?; if !additional_deny_read_paths.is_empty() { return Err( "windows unelevated restricted-token sandbox cannot enforce deny-read restrictions directly; refusing to run unsandboxed" @@ -135,8 +137,7 @@ pub fn resolve_windows_restricted_token_filesystem_overrides( sandbox_policy_cwd.as_path(), ); let legacy_writable_roots = legacy_projection.get_writable_roots_with_cwd(sandbox_policy_cwd); - let split_writable_roots = - file_system_sandbox_policy.get_writable_roots_with_cwd(sandbox_policy_cwd); + let split_writable_roots = resolved.writable_roots; let legacy_root_paths: BTreeSet = legacy_writable_roots .iter() .map(|root| normalize_windows_override_path(root.root.as_path())) @@ -153,21 +154,11 @@ pub fn resolve_windows_restricted_token_filesystem_overrides( ); } - for writable_root in &split_writable_roots { - for read_only_subpath in &writable_root.read_only_subpaths { - if split_writable_roots.iter().any(|candidate| { - candidate.root.as_path() != writable_root.root.as_path() - && candidate - .root - .as_path() - .starts_with(read_only_subpath.as_path()) - }) { - return Err( - "windows unelevated restricted-token sandbox cannot reopen writable descendants under read-only carveouts directly; refusing to run unsandboxed" - .to_string(), - ); - } - } + if has_reopened_writable_descendant(&split_writable_roots) { + return Err( + "windows unelevated restricted-token sandbox cannot reopen writable descendants under read-only carveouts directly; refusing to run unsandboxed" + .to_string(), + ); } let mut additional_deny_write_paths = BTreeSet::new(); @@ -247,13 +238,13 @@ pub fn resolve_windows_elevated_filesystem_overrides( #[cfg(not(target_os = "windows"))] let _ = workload_env; - let additional_deny_read_paths = codex_windows_sandbox::resolve_windows_deny_read_paths( - &file_system_sandbox_policy, - sandbox_policy_cwd, - )?; + let resolved = file_system_sandbox_policy + .resolve_for_enforcement(sandbox_policy_cwd, FileSystemPolicyProjection::Native) + .map_err(|err| err.to_string())?; + let additional_deny_read_paths = + codex_windows_sandbox::expand_windows_deny_read_paths(&resolved, sandbox_policy_cwd)?; - let split_writable_roots = - file_system_sandbox_policy.get_writable_roots_with_cwd(sandbox_policy_cwd); + let split_writable_roots = resolved.writable_roots; if has_reopened_writable_descendant(&split_writable_roots) { return Err( "windows elevated sandbox cannot reopen writable descendants under read-only carveouts directly; refusing to run unsandboxed" @@ -281,8 +272,8 @@ pub fn resolve_windows_elevated_filesystem_overrides( .iter() .map(|root| normalize_path(root.root.to_path_buf())) .collect(); - let split_readable_roots: Vec = file_system_sandbox_policy - .get_readable_roots_with_cwd(sandbox_policy_cwd) + let split_readable_roots: Vec = resolved + .readable_roots .into_iter() .map(AbsolutePathBuf::into_path_buf) .map(&normalize_path) @@ -362,7 +353,7 @@ pub fn resolve_windows_elevated_filesystem_overrides( Ok(Some(WindowsSandboxFilesystemOverrides { read_roots_include_platform_defaults: read_roots_override.is_some() - && file_system_sandbox_policy.include_platform_defaults(), + && resolved.include_platform_defaults, read_roots_override, write_roots_override, additional_deny_read_paths, diff --git a/codex-rs/windows-sandbox-rs/src/deny_read_resolver.rs b/codex-rs/windows-sandbox-rs/src/deny_read_resolver.rs index e9a92a46c2..340b0614bb 100644 --- a/codex-rs/windows-sandbox-rs/src/deny_read_resolver.rs +++ b/codex-rs/windows-sandbox-rs/src/deny_read_resolver.rs @@ -3,6 +3,7 @@ use codex_protocol::permissions::FileSystemPath; use codex_protocol::permissions::FileSystemSandboxEntry; use codex_protocol::permissions::FileSystemSandboxPolicy; use codex_protocol::permissions::ReadDenyMatcher; +use codex_protocol::permissions::ResolvedFileSystemPermissions; use codex_protocol::permissions::windows_deny_read_glob_scan; use codex_utils_absolute_path::AbsolutePathBuf; use std::collections::HashMap; @@ -38,7 +39,27 @@ pub fn resolve_windows_deny_read_paths( file_system_sandbox_policy: &FileSystemSandboxPolicy, cwd: &AbsolutePathBuf, ) -> Result, String> { - resolve_deny_read_paths(file_system_sandbox_policy, cwd, |_| {}) + expand_deny_read_paths( + &file_system_sandbox_policy.get_unreadable_roots_with_cwd(cwd), + &file_system_sandbox_policy.get_unreadable_globs_with_cwd(cwd), + file_system_sandbox_policy.glob_scan_max_depth, + cwd, + |_| {}, + ) +} + +/// Expand the denies from an already resolved filesystem policy into Windows ACL targets. +pub fn expand_windows_deny_read_paths( + permissions: &ResolvedFileSystemPermissions, + cwd: &AbsolutePathBuf, +) -> Result, String> { + expand_deny_read_paths( + &permissions.unreadable_roots, + &permissions.unreadable_globs, + permissions.glob_scan_max_depth, + cwd, + |_| {}, + ) } /// Resolve deny paths with the launcher's scanner in an explicit command environment. @@ -49,24 +70,31 @@ pub fn resolve_windows_deny_read_paths_in_environment( env: &HashMap, command_cwd: &Path, ) -> Result, String> { - resolve_deny_read_paths(file_system_sandbox_policy, cwd, |command| { - command.env_clear().envs(env).current_dir(command_cwd); - }) + expand_deny_read_paths( + &file_system_sandbox_policy.get_unreadable_roots_with_cwd(cwd), + &file_system_sandbox_policy.get_unreadable_globs_with_cwd(cwd), + file_system_sandbox_policy.glob_scan_max_depth, + cwd, + |command| { + command.env_clear().envs(env).current_dir(command_cwd); + }, + ) } -fn resolve_deny_read_paths( - file_system_sandbox_policy: &FileSystemSandboxPolicy, +fn expand_deny_read_paths( + unreadable_roots: &[AbsolutePathBuf], + unreadable_globs: &[String], + glob_scan_max_depth: Option, cwd: &AbsolutePathBuf, configure: impl Fn(&mut Command), ) -> 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())?; + for path in unreadable_roots { + push_absolute_path(&mut paths, &mut seen, path.as_path())?; } - let unreadable_globs = file_system_sandbox_policy.get_unreadable_globs_with_cwd(cwd.as_path()); if unreadable_globs.is_empty() { return Ok(paths); } @@ -88,10 +116,7 @@ fn resolve_deny_read_paths( return Ok(paths); }; - let scan_plans = glob_scan_plans( - &unreadable_globs, - file_system_sandbox_policy.glob_scan_max_depth, - )?; + let scan_plans = glob_scan_plans(unreadable_globs, glob_scan_max_depth)?; for scan_plan in scan_plans { if !scan_plan.root.exists() { @@ -200,9 +225,9 @@ fn ripgrep_files( fn push_absolute_path( paths: &mut Vec, seen: &mut HashSet, - path: PathBuf, + path: impl AsRef, ) -> Result<(), String> { - let absolute_path = AbsolutePathBuf::from_absolute_path(dunce::simplified(&path)) + let absolute_path = AbsolutePathBuf::from_absolute_path(dunce::simplified(path.as_ref())) .map_err(|err| err.to_string())?; if seen.insert(absolute_path.to_path_buf()) { paths.push(absolute_path); diff --git a/codex-rs/windows-sandbox-rs/src/deny_read_walker.rs b/codex-rs/windows-sandbox-rs/src/deny_read_walker.rs index dd1b6e3cca..08656ff783 100644 --- a/codex-rs/windows-sandbox-rs/src/deny_read_walker.rs +++ b/codex-rs/windows-sandbox-rs/src/deny_read_walker.rs @@ -73,7 +73,7 @@ impl GlobWalker<'_> { None => self.matcher.is_local_path_read_denied(path), }; if denied { - push_absolute_path(self.paths, self.seen_paths, path.to_path_buf())?; + push_absolute_path(self.paths, self.seen_paths, path)?; } if !file_type.is_dir() || self.max_depth.is_some_and(|limit| depth >= limit) diff --git a/codex-rs/windows-sandbox-rs/src/lib.rs b/codex-rs/windows-sandbox-rs/src/lib.rs index f3de67d568..2dc203ebd2 100644 --- a/codex-rs/windows-sandbox-rs/src/lib.rs +++ b/codex-rs/windows-sandbox-rs/src/lib.rs @@ -266,6 +266,7 @@ 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::GLOB_SCAN_PROGRAM; +pub use deny_read_resolver::expand_windows_deny_read_paths; pub use deny_read_resolver::resolve_windows_deny_read_paths; pub use deny_read_resolver::resolve_windows_deny_read_paths_in_environment; #[cfg(target_os = "windows")] diff --git a/scripts/codex_package/smoke_tests/test_codex_package.py b/scripts/codex_package/smoke_tests/test_codex_package.py index 0abd78cf5f..c3171827f4 100644 --- a/scripts/codex_package/smoke_tests/test_codex_package.py +++ b/scripts/codex_package/smoke_tests/test_codex_package.py @@ -26,6 +26,7 @@ import subprocess import sys import tomllib from concurrent.futures import ThreadPoolExecutor +from datetime import UTC, datetime from pathlib import Path import pytest @@ -155,28 +156,68 @@ def test_packaged_executor_initializes_and_enforces_sandbox( assert reported_environment["executorVersion"] == metadata["version"] process.stdin.write('{"method":"initialized","params":{}}\n') - # Restricted writes always launch the filesystem helper, even when allowed. - for request_id, path in [(2, workspace / "allowed.txt"), (3, blocked)]: + def file_request( + request_id: int, + method: str, + path: Path, + *, + timeout: int = 45, + **params: str, + ) -> dict: process.stdin.write( json.dumps( { "id": request_id, - "method": "fs/writeFile", + "method": method, "params": { "path": path.as_uri(), - "dataBase64": "b2s=", "sandbox": sandbox, + **params, }, } ) + "\n" ) process.stdin.flush() + return json.loads( + reader.submit(process.stdout.readline).result(timeout=timeout) + ) + + if windows: + # Resolving the inner helper creates today's log. The first ACL scan must see it. + log_dir = home / ".sandbox" + log_dir.mkdir(exist_ok=True) + sibling = log_dir / "allowed.txt" + sibling.write_bytes(b"ok") + day = datetime.now(UTC).date() + log = log_dir / f"sandbox.{day}.log" + assert not log.exists() + sandbox["permissions"]["file_system"]["entries"].append( + { + "path": { + "type": "glob_pattern", + "pattern": (log_dir / "*.log").as_posix(), + }, + "access": "deny", + } + ) # The first Windows helper can provision sandbox accounts before replying. - response = json.loads( - reader.submit(process.stdout.readline).result( - timeout=180 if windows and request_id == 2 else 45 - ) + denied = file_request(5, "fs/readFile", log, timeout=180) + if not log.exists() and day != datetime.now(UTC).date(): + # The helper crossed UTC midnight before creating its log. + log = next(log_dir.glob("sandbox.*.log")) + denied = file_request(7, "fs/readFile", log) + assert "Access is denied" in denied.get("error", {}).get("message", "") + assert log.is_file() + assert file_request(6, "fs/readFile", sibling) == { + "id": 6, + "result": {"dataBase64": "b2s="}, + } + + # Restricted writes always launch the filesystem helper, even when allowed. + for request_id, path in [(2, workspace / "allowed.txt"), (3, blocked)]: + response = file_request( + request_id, "fs/writeFile", path, dataBase64="b2s=" ) if request_id == 2: assert response == {"id": 2, "result": {}}, response