From f59905647a5ca8681b87c5900036081955fb8750 Mon Sep 17 00:00:00 2001 From: iceweasel-oai Date: Wed, 2 Sep 2026 18:38:08 +0000 Subject: [PATCH] Protect Windows sandbox binaries from inherited write access (#42330) ## Why An inherited write ACE from a writable parent directory could leave sandbox runner files in `.sandbox-bin` writable or deletable by the workspace capability SID. ## What changed - Mark the `.sandbox-bin` DACL as protected when applying its explicit ACL, while preserving inherited DACL behavior for the other persistent sandbox directories. - Cover both full and provision-only setup, including runner files created before and after the directory is locked. GitOrigin-RevId: fcda4df97e3f8d729438ccb3d42daa606076e0c4 --- .../src/bin/setup_main/win.rs | 22 ++++- .../src/bin/setup_main/win_acl_tests.rs | 95 +++++++++++++++++++ 2 files changed, 115 insertions(+), 2 deletions(-) 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 763978d997..2f1ddc19b3 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 @@ -60,6 +60,7 @@ use windows_sys::Win32::Security::Authorization::TRUSTEE_W; use windows_sys::Win32::Security::CONTAINER_INHERIT_ACE; use windows_sys::Win32::Security::DACL_SECURITY_INFORMATION; use windows_sys::Win32::Security::OBJECT_INHERIT_ACE; +use windows_sys::Win32::Security::PROTECTED_DACL_SECURITY_INFORMATION; use windows_sys::Win32::Storage::FileSystem::DELETE; use windows_sys::Win32::Storage::FileSystem::FILE_GENERIC_EXECUTE; use windows_sys::Win32::Storage::FileSystem::FILE_GENERIC_READ; @@ -117,6 +118,12 @@ enum SetupMode { ReadAclsOnly, } +#[derive(Clone, Copy)] +enum DaclInheritance { + Inherited, + Protected, +} + fn log_line(log: &mut dyn Write, msg: &str) -> Result<()> { let ts = chrono::Utc::now().to_rfc3339(); writeln!(log, "[{ts}] {msg}").map_err(|err| { @@ -295,6 +302,7 @@ fn read_mask_allows_or_log( } } +#[allow(clippy::too_many_arguments)] fn lock_sandbox_dir( dir: &Path, real_user: &str, @@ -302,6 +310,7 @@ fn lock_sandbox_dir( sandbox_group_access_mode: i32, sandbox_group_mask: u32, real_user_mask: u32, + dacl_inheritance: DaclInheritance, setup_mode: SetupMode, ) -> Result<()> { // ProvisionOnly accepts another user's CODEX_HOME; keep its ACL mutation @@ -373,12 +382,18 @@ fn lock_sandbox_dir( "SetEntriesInAclW sandbox dir failed: {set}", )); } + let security_information = match dacl_inheritance { + DaclInheritance::Inherited => DACL_SECURITY_INFORMATION, + DaclInheritance::Protected => { + DACL_SECURITY_INFORMATION | PROTECTED_DACL_SECURITY_INFORMATION + } + }; let (res, api) = match directory.as_ref() { Some(directory) => ( SetSecurityInfo( directory.as_raw_handle() as _, SE_FILE_OBJECT, - DACL_SECURITY_INFORMATION, + security_information, std::ptr::null_mut(), std::ptr::null_mut(), new_dacl, @@ -392,7 +407,7 @@ fn lock_sandbox_dir( SetNamedSecurityInfoW( path_w.as_ptr() as *mut u16, SE_FILE_OBJECT, - DACL_SECURITY_INFORMATION, + security_information, std::ptr::null_mut(), std::ptr::null_mut(), new_dacl, @@ -671,6 +686,7 @@ fn lock_persistent_sandbox_dirs(payload: &Payload, sandbox_group_sid: &[u8]) -> GRANT_ACCESS, FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE | DELETE, FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE, + DaclInheritance::Inherited, payload.mode, ) .map_err(|err| { @@ -689,6 +705,7 @@ fn lock_persistent_sandbox_dirs(payload: &Payload, sandbox_group_sid: &[u8]) -> DENY_ACCESS, FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE | DELETE, FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE, + DaclInheritance::Inherited, payload.mode, ) .map_err(|err| { @@ -715,6 +732,7 @@ fn lock_sandbox_bin_dir(payload: &Payload, sandbox_group_sid: &[u8]) -> Result<( GRANT_ACCESS, FILE_GENERIC_READ | FILE_GENERIC_EXECUTE, FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE | DELETE, + DaclInheritance::Protected, payload.mode, ) .map_err(|err| { diff --git a/codex-rs/windows-sandbox-rs/src/bin/setup_main/win_acl_tests.rs b/codex-rs/windows-sandbox-rs/src/bin/setup_main/win_acl_tests.rs index e488e2a914..df39cfdd18 100644 --- a/codex-rs/windows-sandbox-rs/src/bin/setup_main/win_acl_tests.rs +++ b/codex-rs/windows-sandbox-rs/src/bin/setup_main/win_acl_tests.rs @@ -1,11 +1,23 @@ use super::DELETE; +use super::DaclInheritance; use super::FILE_GENERIC_EXECUTE; use super::FILE_GENERIC_READ; use super::FILE_GENERIC_WRITE; use super::GRANT_ACCESS; +use super::Payload; +use super::SETUP_VERSION; use super::SetupMode; +use super::convert_string_sid_to_sid; +use super::lock_sandbox_bin_dir; use super::lock_sandbox_dir; use super::resolve_sid; +use super::sid_bytes_to_psid; +use codex_windows_sandbox::ensure_allow_write_aces; +use codex_windows_sandbox::path_mask_allows; +use codex_windows_sandbox::workspace_write_cap_sid_for_root; +use std::fs; +use windows_sys::Win32::Foundation::HLOCAL; +use windows_sys::Win32::Foundation::LocalFree; #[test] fn provision_only_locks_plain_directory_via_handle() { @@ -21,9 +33,92 @@ fn provision_only_locks_plain_directory_via_handle() { GRANT_ACCESS, FILE_GENERIC_READ | FILE_GENERIC_EXECUTE, FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE | DELETE, + DaclInheritance::Protected, SetupMode::ProvisionOnly, ) .expect("lock sandbox bin via no-reparse handle"); assert!(sandbox_bin.is_dir()); } + +#[test] +fn lock_sandbox_dir_blocks_inherited_write_for_runner_files() { + for setup_mode in [SetupMode::Full, SetupMode::ProvisionOnly] { + let temp = tempfile::tempdir().expect("tempdir"); + let workspace = temp.path().join("workspace"); + let sandbox_bin = workspace.join(".sandbox-bin"); + fs::create_dir(&workspace).expect("create workspace"); + + let workspace_sid = workspace_write_cap_sid_for_root(temp.path(), temp.path(), temp.path()) + .expect("workspace SID"); + let workspace_psid = + unsafe { convert_string_sid_to_sid(&workspace_sid).expect("convert workspace SID") }; + let sandbox_group_sid = + resolve_sid("Authenticated Users").expect("resolve sandbox group SID"); + let sandbox_group_psid = + sid_bytes_to_psid(&sandbox_group_sid).expect("convert sandbox group SID"); + unsafe { ensure_allow_write_aces(&workspace, &[workspace_psid]) } + .expect("grant inherited workspace write access"); + + fs::create_dir(&sandbox_bin).expect("create sandbox bin"); + let existing_runner = sandbox_bin.join("existing-runner.exe"); + fs::write(&existing_runner, b"existing").expect("create existing runner"); + assert!( + path_mask_allows( + &existing_runner, + &[workspace_psid], + FILE_GENERIC_WRITE | DELETE, + /*require_all_bits*/ true, + ) + .expect("check inherited runner write and delete access") + ); + + let real_user = std::env::var("USERNAME").unwrap_or_else(|_| "Administrators".to_string()); + let payload = Payload { + version: SETUP_VERSION, + offline_username: String::new(), + online_username: String::new(), + codex_home: workspace.clone(), + command_cwd: workspace.clone(), + read_roots: Vec::new(), + write_roots: Vec::new(), + deny_read_paths: Vec::new(), + deny_write_paths: Vec::new(), + proxy_ports: Vec::new(), + allow_local_binding: false, + otel: None, + real_user, + mode: setup_mode, + refresh_only: false, + }; + lock_sandbox_bin_dir(&payload, &sandbox_group_sid).expect("lock sandbox bin"); + let new_runner = sandbox_bin.join("new-runner.exe"); + fs::write(&new_runner, b"new").expect("create new runner"); + + for path in [&sandbox_bin, &existing_runner, &new_runner] { + assert!( + !path_mask_allows( + path, + &[workspace_psid], + FILE_GENERIC_WRITE | DELETE, + /*require_all_bits*/ false, + ) + .expect("check protected path write and delete access") + ); + assert!( + path_mask_allows( + path, + &[sandbox_group_psid], + FILE_GENERIC_READ | FILE_GENERIC_EXECUTE, + /*require_all_bits*/ true, + ) + .expect("check protected path read and execute access") + ); + } + + unsafe { + LocalFree(workspace_psid as HLOCAL); + LocalFree(sandbox_group_psid as HLOCAL); + } + } +}