From 0131f99fd5d166cdeb35219474f4e2e65480afd2 Mon Sep 17 00:00:00 2001 From: iceweasel-oai Date: Mon, 13 Apr 2026 10:49:42 -0700 Subject: [PATCH] Include legacy deny paths in elevated Windows sandbox setup (#17365) ## Summary This updates the Windows elevated sandbox setup/refresh path to include the legacy `compute_allow_paths(...).deny` protected children in the same deny-write payload pipe added for split filesystem carveouts. Concretely, elevated setup and elevated refresh now both build deny-write payload paths from: - explicit split-policy deny-write paths, preserving missing paths so setup can materialize them before applying ACLs - legacy `compute_allow_paths(...).deny`, which includes existing `.git`, `.codex`, and `.agents` children under writable roots This lets the elevated backend protect `.git` consistently with the unelevated/restricted-token path, and removes the old janky hard-coded `.codex` / `.agents` elevated setup helpers in favor of the shared payload path. ## Root Cause The landed split-carveout PR threaded a `deny_write_paths` pipe through elevated setup/refresh, but the legacy workspace-write deny set from `compute_allow_paths(...).deny` was not included in that payload. As a result, elevated workspace-write did not apply the intended deny-write ACLs for existing protected children like `/.git`. ## Notes The legacy protected children still only enter the deny set if they already exist, because `compute_allow_paths` filters `.git`, `.codex`, and `.agents` with `exists()`. Missing explicit split-policy deny paths are preserved separately because setup intentionally materializes those before applying ACLs. ## Validation - `cargo fmt --check -p codex-windows-sandbox` - `cargo test -p codex-windows-sandbox` - `cargo build -p codex-cli -p codex-windows-sandbox --bins` - Elevated `codex exec` smoke with `windows.sandbox='elevated'`: fresh git repo, attempted append to `.git/config`, observed `Access is denied`, marker not written, Deny ACE present on `.git` - Unelevated `codex exec` smoke with `windows.sandbox='unelevated'`: fresh git repo, attempted append to `.git/config`, observed `Access is denied`, marker not written, Deny ACE present on `.git` --- codex-rs/windows-sandbox-rs/src/lib.rs | 10 --- .../windows-sandbox-rs/src/setup_main_win.rs | 68 ++-------------- .../src/setup_orchestrator.rs | 79 ++++++++++++++++++- .../windows-sandbox-rs/src/workspace_acl.rs | 24 ------ 4 files changed, 85 insertions(+), 96 deletions(-) diff --git a/codex-rs/windows-sandbox-rs/src/lib.rs b/codex-rs/windows-sandbox-rs/src/lib.rs index dd8f23d00..90176b08e 100644 --- a/codex-rs/windows-sandbox-rs/src/lib.rs +++ b/codex-rs/windows-sandbox-rs/src/lib.rs @@ -191,10 +191,6 @@ pub use winutil::string_from_sid_bytes; pub use winutil::to_wide; #[cfg(target_os = "windows")] pub use workspace_acl::is_command_cwd_root; -#[cfg(target_os = "windows")] -pub use workspace_acl::protect_workspace_agents_dir; -#[cfg(target_os = "windows")] -pub use workspace_acl::protect_workspace_codex_dir; #[cfg(not(target_os = "windows"))] pub use stub::CaptureResult; @@ -228,8 +224,6 @@ 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 super::workspace_acl::protect_workspace_agents_dir; - use super::workspace_acl::protect_workspace_codex_dir; use anyhow::Result; use std::collections::HashMap; use std::ffi::c_void; @@ -441,8 +435,6 @@ mod windows_impl { allow_null_device(psid_generic); if let Some(psid) = psid_workspace { allow_null_device(psid); - let _ = protect_workspace_codex_dir(¤t_dir, psid); - let _ = protect_workspace_agents_dir(¤t_dir, psid); } } @@ -625,8 +617,6 @@ mod windows_impl { } allow_null_device(psid_generic); allow_null_device(psid_workspace); - let _ = protect_workspace_codex_dir(¤t_dir, psid_workspace); - let _ = protect_workspace_agents_dir(¤t_dir, psid_workspace); } Ok(()) diff --git a/codex-rs/windows-sandbox-rs/src/setup_main_win.rs b/codex-rs/windows-sandbox-rs/src/setup_main_win.rs index 6619f60c4..78c0a8be8 100644 --- a/codex-rs/windows-sandbox-rs/src/setup_main_win.rs +++ b/codex-rs/windows-sandbox-rs/src/setup_main_win.rs @@ -22,8 +22,6 @@ use codex_windows_sandbox::is_command_cwd_root; use codex_windows_sandbox::load_or_create_cap_sids; use codex_windows_sandbox::log_note; use codex_windows_sandbox::path_mask_allows; -use codex_windows_sandbox::protect_workspace_agents_dir; -use codex_windows_sandbox::protect_workspace_codex_dir; use codex_windows_sandbox::sandbox_bin_dir; use codex_windows_sandbox::sandbox_dir; use codex_windows_sandbox::sandbox_secrets_dir; @@ -767,17 +765,15 @@ fn run_setup_full(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<( continue; } - // These are explicit read-only-under-a-writable-root carveouts from the transformed - // sandbox policy; they are not deny-read paths. + // These are deny-write carveouts, not deny-read paths. They may come from explicit + // read-only-under-a-writable-root carveouts in the transformed sandbox policy, or from + // legacy protected children such as `.git`, `.codex`, and `.agents`. // - // They are also not optional workspace sentinels such as `.codex` or `.agents`: those - // are protected best-effort below and still skip missing directories so we do not leave - // empty protection artifacts behind in a workspace. - // - // Deny ACEs attach to filesystem objects; if a policy carveout does not exist during - // setup, the sandbox could otherwise create it later under a writable parent and - // bypass the carveout. Materialize missing carveouts as directories so the deny-write - // ACL is present before the command starts. + // Deny ACEs attach to filesystem objects; if an explicit policy carveout does not exist + // during setup, the sandbox could otherwise create it later under a writable parent and + // bypass the carveout. Materialize missing carveouts as directories so the deny-write ACL + // is present before the command starts. Legacy protected children are filtered before + // payload creation, so this should not create sentinel directories in a workspace. if !path.exists() { std::fs::create_dir_all(path) .with_context(|| format!("failed to create deny-write path {}", path.display()))?; @@ -880,54 +876,6 @@ fn run_setup_full(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<( } } - // Protect the current workspace's `.codex` and `.agents` directories from tampering - // (write/delete) by using a workspace-specific capability SID. If a directory doesn't exist - // yet, skip it (it will be picked up on the next refresh). - match unsafe { protect_workspace_codex_dir(&payload.command_cwd, workspace_psid) } { - Ok(true) => { - let cwd_codex = payload.command_cwd.join(".codex"); - log_line( - log, - &format!( - "applied deny ACE to protect workspace .codex {}", - cwd_codex.display() - ), - )?; - } - Ok(false) => {} - Err(err) => { - let cwd_codex = payload.command_cwd.join(".codex"); - refresh_errors.push(format!("deny ACE failed on {}: {err}", cwd_codex.display())); - log_line( - log, - &format!("deny ACE failed on {}: {err}", cwd_codex.display()), - )?; - } - } - match unsafe { protect_workspace_agents_dir(&payload.command_cwd, workspace_psid) } { - Ok(true) => { - let cwd_agents = payload.command_cwd.join(".agents"); - log_line( - log, - &format!( - "applied deny ACE to protect workspace .agents {}", - cwd_agents.display() - ), - )?; - } - Ok(false) => {} - Err(err) => { - let cwd_agents = payload.command_cwd.join(".agents"); - refresh_errors.push(format!( - "deny ACE failed on {}: {err}", - cwd_agents.display() - )); - log_line( - log, - &format!("deny ACE failed on {}: {err}", cwd_agents.display()), - )?; - } - } unsafe { if !sandbox_group_psid.is_null() { LocalFree(sandbox_group_psid as HLOCAL); diff --git a/codex-rs/windows-sandbox-rs/src/setup_orchestrator.rs b/codex-rs/windows-sandbox-rs/src/setup_orchestrator.rs index 01d9e34f4..e0a5a063f 100644 --- a/codex-rs/windows-sandbox-rs/src/setup_orchestrator.rs +++ b/codex-rs/windows-sandbox-rs/src/setup_orchestrator.rs @@ -163,6 +163,7 @@ fn run_setup_refresh_inner( return Ok(()); } let (read_roots, write_roots) = build_payload_roots(&request, &overrides); + 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); let offline_proxy_settings = offline_proxy_settings_from_env(request.env_map, network_identity); @@ -174,7 +175,7 @@ fn run_setup_refresh_inner( command_cwd: request.command_cwd.to_path_buf(), read_roots, write_roots, - deny_write_paths: overrides.deny_write_paths.unwrap_or_default(), + deny_write_paths, proxy_ports: offline_proxy_settings.proxy_ports, allow_local_binding: offline_proxy_settings.allow_local_binding, real_user: std::env::var("USERNAME").unwrap_or_else(|_| "Administrators".to_string()), @@ -735,6 +736,7 @@ pub fn run_elevated_setup( ) })?; let (read_roots, write_roots) = build_payload_roots(&request, &overrides); + 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); let offline_proxy_settings = offline_proxy_settings_from_env(request.env_map, network_identity); @@ -746,7 +748,7 @@ pub fn run_elevated_setup( command_cwd: request.command_cwd.to_path_buf(), read_roots, write_roots, - deny_write_paths: overrides.deny_write_paths.unwrap_or_default(), + deny_write_paths, proxy_ports: offline_proxy_settings.proxy_ports, allow_local_binding: offline_proxy_settings.allow_local_binding, real_user: std::env::var("USERNAME").unwrap_or_else(|_| "Administrators".to_string()), @@ -797,6 +799,31 @@ fn build_payload_roots( (read_roots, write_roots) } +fn build_payload_deny_write_paths( + request: &SandboxSetupRequest<'_>, + explicit_deny_write_paths: Option>, +) -> Vec { + let allow_deny_paths: AllowDenyPaths = compute_allow_paths( + request.policy, + request.policy_cwd, + request.command_cwd, + request.env_map, + ); + 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 + } + }) + .collect(); + deny_write_paths.extend(allow_deny_paths.deny); + deny_write_paths +} + fn filter_sensitive_write_roots(mut roots: Vec, codex_home: &Path) -> Vec { // Never grant capability write access to CODEX_HOME or anything under CODEX_HOME/.sandbox, // CODEX_HOME/.sandbox-bin, or CODEX_HOME/.sandbox-secrets. These locations contain sandbox @@ -1267,6 +1294,54 @@ mod tests { ); } + #[test] + fn payload_deny_write_paths_merge_explicit_and_protected_children() { + let tmp = TempDir::new().expect("tempdir"); + let codex_home = tmp.path().join("codex-home"); + let command_cwd = tmp.path().join("workspace"); + let extra_write_root = tmp.path().join("extra-write-root"); + let command_git = command_cwd.join(".git"); + let extra_codex = extra_write_root.join(".codex"); + let explicit_deny = tmp.path().join("explicit-deny"); + fs::create_dir_all(&command_git).expect("create command .git"); + fs::create_dir_all(&extra_codex).expect("create extra .codex"); + let policy = SandboxPolicy::WorkspaceWrite { + writable_roots: vec![ + AbsolutePathBuf::from_absolute_path(&extra_write_root) + .expect("absolute writable root"), + ], + read_only_access: ReadOnlyAccess::Restricted { + include_platform_defaults: false, + readable_roots: Vec::new(), + }, + network_access: false, + exclude_tmpdir_env_var: true, + exclude_slash_tmp: true, + }; + let request = super::SandboxSetupRequest { + policy: &policy, + policy_cwd: &command_cwd, + command_cwd: &command_cwd, + env_map: &HashMap::new(), + codex_home: &codex_home, + proxy_enforced: false, + }; + + let deny_write_paths = + super::build_payload_deny_write_paths(&request, Some(vec![explicit_deny.clone()])); + + assert_eq!( + [ + dunce::canonicalize(&command_git).expect("canonical command .git"), + dunce::canonicalize(&extra_codex).expect("canonical extra .codex"), + explicit_deny, + ] + .into_iter() + .collect::>(), + deny_write_paths.into_iter().collect() + ); + } + #[test] fn full_read_roots_preserve_legacy_platform_defaults() { let tmp = TempDir::new().expect("tempdir"); diff --git a/codex-rs/windows-sandbox-rs/src/workspace_acl.rs b/codex-rs/windows-sandbox-rs/src/workspace_acl.rs index 7143212b5..d011db30b 100644 --- a/codex-rs/windows-sandbox-rs/src/workspace_acl.rs +++ b/codex-rs/windows-sandbox-rs/src/workspace_acl.rs @@ -1,30 +1,6 @@ -use crate::acl::add_deny_write_ace; use crate::path_normalization::canonicalize_path; -use anyhow::Result; -use std::ffi::c_void; use std::path::Path; pub fn is_command_cwd_root(root: &Path, canonical_command_cwd: &Path) -> bool { canonicalize_path(root) == canonical_command_cwd } - -/// # Safety -/// Caller must ensure `psid` is a valid SID pointer. -pub unsafe fn protect_workspace_codex_dir(cwd: &Path, psid: *mut c_void) -> Result { - protect_workspace_subdir(cwd, psid, ".codex") -} - -/// # Safety -/// Caller must ensure `psid` is a valid SID pointer. -pub unsafe fn protect_workspace_agents_dir(cwd: &Path, psid: *mut c_void) -> Result { - protect_workspace_subdir(cwd, psid, ".agents") -} - -unsafe fn protect_workspace_subdir(cwd: &Path, psid: *mut c_void, subdir: &str) -> Result { - let path = cwd.join(subdir); - if path.is_dir() { - add_deny_write_ace(&path, psid) - } else { - Ok(false) - } -}