From 3f74ff28fa09530bd5bcfcbfcbb86553636dde94 Mon Sep 17 00:00:00 2001 From: iceweasel-oai Date: Tue, 27 Oct 2026 04:33:05 +0000 Subject: [PATCH] Avoid object-identity preflight for MXC path grants (#57353) ## Why MXC's `normalize_object_conflicts` can reject otherwise usable Windows policies when a network share does not support `FileIdInfo` or a protected deny path cannot be opened for metadata. ## What changed Remove object-conflict normalization before and after volume-root expansion. Send explicit path grants directly to BaseContainer, which enforces path and symlink/junction alias precedence. Document the resulting limitations: - Hard-link names retain independent permissions, so an allowed name can access the same file through a less restrictive grant. - Volume roots expand using their declared access without first reconciling aliases. A writable root can therefore generate more-specific writable grants that override a read-only root alias. GitOrigin-RevId: de0b2d7b5b98eaa358a97156c02e9f7f85860335 --- codex-rs/mxc-sandbox/README.md | 20 +++++++ codex-rs/mxc-sandbox/src/policy.rs | 46 ++++------------- codex-rs/mxc-sandbox/src/policy_tests.rs | 66 ------------------------ 3 files changed, 30 insertions(+), 102 deletions(-) diff --git a/codex-rs/mxc-sandbox/README.md b/codex-rs/mxc-sandbox/README.md index c67c0bbb62..50a66c9810 100644 --- a/codex-rs/mxc-sandbox/README.md +++ b/codex-rs/mxc-sandbox/README.md @@ -26,6 +26,26 @@ direct non-loopback egress and general inbound network access. Win32k calls and desktop handles remain available for PowerShell startup; clipboard, input-injection, and desktop/system-control restrictions remain. +Filesystem conflicts in the emitted policy are enforced by BaseContainer: +equivalent path spellings and symlink/junction aliases use the strictest access +(`deny` > read-only > read-write), while more-specific paths override ancestor +grants. See the [BaseContainer path-precedence rules](https://github.com/microsoft/mxc/blob/main/docs/schema.md#overlapping-paths-for-windows-basecontainer). + +Do not call MXC's shared `normalize_object_conflicts` before or after volume +expansion. Its identity preflight is used by mount-based backends and can reject +an otherwise usable Windows policy when a network share does not support +`FileIdInfo`, or a protected deny path cannot be opened for metadata. Codex sends +explicit path grants to BaseContainer without requiring that additional lookup. +We intentionally accept two limitations: + +- Hard-link names retain independent path permissions. An allowed name can read + or modify the same file as a denied or read-only name; we do not tighten all + names to an object-wide restriction. +- Volume-root expansion uses each root's declared access without reconciling + aliases first. For example, a writable `V:\` plus a read-only junction to + `V:\` can produce a writable `V:\projects` grant. That generated, more-specific + grant permits writes through either name despite the read-only root alias. + ## Selection and rollout `windows.sandbox = "mxc"` is strict. The default-off `features.prefer_mxc` diff --git a/codex-rs/mxc-sandbox/src/policy.rs b/codex-rs/mxc-sandbox/src/policy.rs index 3f98e961a1..11ebd06cd6 100644 --- a/codex-rs/mxc-sandbox/src/policy.rs +++ b/codex-rs/mxc-sandbox/src/policy.rs @@ -1,5 +1,6 @@ -//! Translate canonical permissions into native MXC grants without expanding -//! access. Denies and read-only carveouts remain separate kernel policy lists. +//! Translate canonical permissions into MXC path grants. +//! Denies and read-only carveouts remain separate kernel policy lists. +//! Alias limitations are documented in the crate README. use std::collections::HashMap; use std::collections::HashSet; @@ -18,9 +19,6 @@ use mxc_sdk::mxc_common::cmdline::CommandLineError; use mxc_sdk::mxc_common::cmdline::cmdline_from_argv_for_context; use mxc_sdk::mxc_common::filesystem_object::ExistingObjectComparison; use mxc_sdk::mxc_common::filesystem_object::compare_existing_filesystem_objects; -use mxc_sdk::mxc_common::filesystem_object::normalize_object_conflicts; -use mxc_sdk::mxc_common::logger::Logger; -use mxc_sdk::mxc_common::logger::Mode; use mxc_sdk::mxc_common::models::BaseProcessUiConfig; use mxc_sdk::mxc_common::models::ContainerPolicy; use mxc_sdk::mxc_common::models::ExecutionRequest; @@ -205,20 +203,10 @@ pub(super) fn build_request( .collect::>>()?; prune_unavailable_volume_roots(&mut write, &volume_roots); prune_unavailable_volume_roots(&mut read, &volume_roots); - // Normalize pre-expansion aliases so generated children inherit - // any tightened access applied to their volume root. - let normalized = normalize_policy(ContainerPolicy { - readwrite_paths: unicode_paths(write)?, - readonly_paths: unicode_paths(read)?, - denied_paths: unicode_paths(deny)?, - ..Default::default() - })?; - let write = collect_paths(normalized.readwrite_paths.into_iter().map(PathBuf::from))?; - let read = collect_paths(normalized.readonly_paths.into_iter().map(PathBuf::from))?; - let deny = collect_paths(normalized.denied_paths.into_iter().map(PathBuf::from))?; - // If expansion skips an unavailable writable volume, its generated read-only - // carveouts remain below. Pruning them requires retaining their root provenance, - // so we accept that rare normalization failure for now. + // BaseContainer owns alias precedence for the emitted paths. Do not call + // MXC's object normalizer: its metadata preflight rejects usable SMB grants + // and inaccessible deny paths before the native API can evaluate them. + // The hard-link and volume-root alias tradeoffs are intentional; see README. let mut write = expand_volume_roots(write, &volume_roots)?; write.retain(|key, _| !carveouts.contains_key(key) && !deny.contains_key(key)); let mut read = expand_volume_roots(read, &volume_roots)?; @@ -269,7 +257,7 @@ pub(super) fn build_request( ports: Vec::new(), }); } - let mut request = ExecutionRequest { + Ok(ExecutionRequest { script_code: cmdline_from_argv_for_context( &command.command, CommandLineContext::WindowsCreateProcess, @@ -311,21 +299,7 @@ pub(super) fn build_request( ..Default::default() }, ..Default::default() - }; - // Reconcile aliases introduced by expansion. MXC/PSEC separately enforces - // restrictions on junction-resolved targets at access time. - request.policy = normalize_policy(request.policy)?; - Ok(request) -} - -fn normalize_policy(mut policy: ContainerPolicy) -> Result { - let mut logger = Logger::new(Mode::Buffer); - if let Some(normalized) = - normalize_object_conflicts(&policy, &mut logger).map_err(PolicyError::PolicyResolution)? - { - policy = normalized; - } - Ok(policy) + }) } fn prune_unavailable_volume_roots(paths: &mut NativePaths, volume_roots: &HashSet) { @@ -400,7 +374,7 @@ fn expand_volume_roots( continue; } // A self-comparison is MXC's public object-identity probe. - // Generated grants can be skipped; explicit paths fail closed below. + // Generated grants can be skipped; explicit paths go to BaseContainer. if compare_existing_filesystem_objects(&child, &child) != ExistingObjectComparison::Same { diff --git a/codex-rs/mxc-sandbox/src/policy_tests.rs b/codex-rs/mxc-sandbox/src/policy_tests.rs index e91761c9f9..bb2b1729cb 100644 --- a/codex-rs/mxc-sandbox/src/policy_tests.rs +++ b/codex-rs/mxc-sandbox/src/policy_tests.rs @@ -322,72 +322,6 @@ fn volume_expansion_does_not_turn_read_only_child_writable() -> Result<()> { Ok(()) } -#[test] -fn volume_expansion_does_not_turn_read_only_alias_writable() -> Result<()> { - let temp = tempfile::tempdir()?; - let root = canonical_root(&temp)?; - let readonly = root.join("protected"); - let alias = root.join("alias"); - std::fs::write(&readonly, "protected")?; - std::fs::hard_link(&readonly, &alias)?; - let fs = FileSystemSandboxPolicy::restricted(vec![ - entry(&root, FileSystemAccessMode::Write)?, - entry(&readonly, FileSystemAccessMode::Read)?, - ]); - let profile = - PermissionProfile::from_runtime_permissions(&fs, NetworkSandboxPolicy::Restricted); - let request = build_request( - &command(&profile, &root), - &root, - Vec::new(), - std::slice::from_ref(&root), - &[], - )?; - let alias = alias.display().to_string(); - assert_eq!( - ( - request.policy.readwrite_paths.contains(&alias), - request.policy.readonly_paths.contains(&alias), - ), - (false, true) - ); - Ok(()) -} - -#[cfg(unix)] -#[test] -fn volume_expansion_uses_normalized_root_access() -> Result<()> { - let temp = tempfile::tempdir()?; - let root = canonical_root(&temp)?; - let volume = root.join("volume"); - let child = volume.join("child"); - let alias = root.join("alias"); - std::fs::create_dir_all(&child)?; - std::os::unix::fs::symlink(&volume, &alias)?; - let fs = FileSystemSandboxPolicy::restricted(vec![ - entry(&volume, FileSystemAccessMode::Write)?, - entry(&alias, FileSystemAccessMode::Read)?, - ]); - let profile = - PermissionProfile::from_runtime_permissions(&fs, NetworkSandboxPolicy::Restricted); - let request = build_request( - &command(&profile, &root), - &root, - Vec::new(), - std::slice::from_ref(&volume), - &[], - )?; - let child = child.display().to_string(); - assert_eq!( - ( - request.policy.readwrite_paths.contains(&child), - request.policy.readonly_paths.contains(&child), - ), - (false, true) - ); - Ok(()) -} - #[cfg(unix)] #[test] fn volume_expansion_skips_uninspectable_children() -> Result<()> {