diff --git a/codex-rs/windows-sandbox-rs/src/acl.rs b/codex-rs/windows-sandbox-rs/src/acl.rs index ace264360d..a10e8cdd57 100644 --- a/codex-rs/windows-sandbox-rs/src/acl.rs +++ b/codex-rs/windows-sandbox-rs/src/acl.rs @@ -21,9 +21,12 @@ use windows_sys::Win32::Security::ACE_HEADER; use windows_sys::Win32::Security::ACL; use windows_sys::Win32::Security::ACL_SIZE_INFORMATION; use windows_sys::Win32::Security::AclSizeInformation; +use windows_sys::Win32::Security::Authorization::ACCESS_MODE; use windows_sys::Win32::Security::Authorization::EXPLICIT_ACCESS_W; +use windows_sys::Win32::Security::Authorization::GRANT_ACCESS; use windows_sys::Win32::Security::Authorization::GetNamedSecurityInfoW; use windows_sys::Win32::Security::Authorization::GetSecurityInfo; +use windows_sys::Win32::Security::Authorization::SET_ACCESS; use windows_sys::Win32::Security::Authorization::SetEntriesInAclW; use windows_sys::Win32::Security::Authorization::SetNamedSecurityInfoW; use windows_sys::Win32::Security::Authorization::SetSecurityInfo; @@ -68,6 +71,8 @@ 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 GENERIC_EXECUTE_MASK: u32 = 0x2000_0000; +const GENERIC_ALL_MASK: u32 = 0x1000_0000; const DENY_ACCESS: i32 = 3; // TrustedInstaller is a deterministic service SID, not a machine-local account SID. const TRUSTED_INSTALLER_SID: &str = @@ -444,6 +449,19 @@ pub unsafe fn dacl_has_write_deny_for_sid(p_dacl: *mut ACL, psid: *mut c_void) - } pub unsafe fn dacl_has_read_deny_for_sid(p_dacl: *mut ACL, psid: *mut c_void) -> bool { + dacl_has_deny_mask( + p_dacl, + DenyAceScope::EffectiveForSid(psid), + FILE_GENERIC_READ | GENERIC_READ_MASK, + ) +} + +enum DenyAceScope { + EffectiveForSid(*mut c_void), + Any, +} + +unsafe fn dacl_has_deny_mask(p_dacl: *mut ACL, scope: DenyAceScope, deny_mask: u32) -> bool { if p_dacl.is_null() { return false; } @@ -457,7 +475,6 @@ pub unsafe fn dacl_has_read_deny_for_sid(p_dacl: *mut ACL, psid: *mut c_void) -> 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 { @@ -467,14 +484,14 @@ pub unsafe fn dacl_has_read_deny_for_sid(p_dacl: *mut ACL, psid: *mut c_void) -> if hdr.AceType != ACCESS_DENIED_ACE_TYPE { continue; // ACCESS_DENIED_ACE_TYPE } - if (hdr.AceFlags & INHERIT_ONLY_ACE) != 0 { + let ace = &*(p_ace as *const ACCESS_DENIED_ACE); + if let DenyAceScope::EffectiveForSid(psid) = scope + && ((hdr.AceFlags & INHERIT_ONLY_ACE) != 0 + || EqualSid(std::ptr::addr_of!(ace.SidStart) as *mut c_void, psid) == 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 { + if (ace.Mask & deny_mask) != 0 { return true; } } @@ -522,17 +539,33 @@ unsafe fn ensure_allow_mask_aces_with_inheritance_impl( sids: &[*mut c_void], allow_mask: u32, disallow_mask: u32, + access_mode: ACCESS_MODE, inheritance: u32, ) -> Result { let (p_dacl, p_sd) = fetch_dacl_handle(path)?; let mut entries: Vec = Vec::new(); for sid in sids { + // A new allow can outrank another trustee's inherited deny, including a + // file-only deny on children. Without the complete token, preserve every deny. + if access_mode == GRANT_ACCESS + && dacl_has_deny_mask( + p_dacl, + DenyAceScope::Any, + allow_mask + | GENERIC_READ_MASK + | GENERIC_WRITE_MASK + | GENERIC_EXECUTE_MASK + | GENERIC_ALL_MASK, + ) + { + continue; + } if !dacl_allow_mask_needs_refresh(p_dacl, *sid, allow_mask, disallow_mask) { continue; } entries.push(EXPLICIT_ACCESS_W { grfAccessPermissions: allow_mask, - grfAccessMode: 2, // SET_ACCESS + grfAccessMode: access_mode, grfInheritance: inheritance, Trustee: TRUSTEE_W { pMultipleTrustee: std::ptr::null_mut(), @@ -605,6 +638,27 @@ pub unsafe fn ensure_allow_mask_aces_with_inheritance( sids, allow_mask, /*disallow_mask*/ 0, + SET_ACCESS, + inheritance, + ) +} + +/// Ensure read/execute grants without replacing existing entries or overriding denies. +/// Returns true if any ACE was added. +/// +/// # Safety +/// Caller must pass valid SID pointers and an existing path. +pub(crate) unsafe fn grant_read_execute_aces( + path: &Path, + sids: &[*mut c_void], + inheritance: u32, +) -> Result { + ensure_allow_mask_aces_with_inheritance_impl( + path, + sids, + FILE_GENERIC_READ | FILE_GENERIC_EXECUTE, + /*disallow_mask*/ 0, + GRANT_ACCESS, inheritance, ) } @@ -638,6 +692,7 @@ pub unsafe fn ensure_allow_write_aces(path: &Path, sids: &[*mut c_void]) -> Resu sids, WRITE_ALLOW_MASK, FILE_DELETE_CHILD, + SET_ACCESS, CONTAINER_INHERIT_ACE | OBJECT_INHERIT_ACE, ) } diff --git a/codex-rs/windows-sandbox-rs/src/setup_provisioning/setup_runtime_bin.rs b/codex-rs/windows-sandbox-rs/src/setup_provisioning/setup_runtime_bin.rs index f2c16a8705..24965a7b56 100644 --- a/codex-rs/windows-sandbox-rs/src/setup_provisioning/setup_runtime_bin.rs +++ b/codex-rs/windows-sandbox-rs/src/setup_provisioning/setup_runtime_bin.rs @@ -1,11 +1,14 @@ use std::ffi::c_void; use std::io::Write; use std::os::windows::fs::MetadataExt as _; +use std::path::Path; use std::path::PathBuf; -use crate::ensure_allow_mask_aces_with_inheritance; +use crate::acl::grant_read_execute_aces; use crate::path_mask_allows; +use anyhow::Context; use anyhow::Result; +use anyhow::ensure; use windows_sys::Win32::Security::CONTAINER_INHERIT_ACE; use windows_sys::Win32::Security::OBJECT_INHERIT_ACE; use windows_sys::Win32::Storage::FileSystem::FILE_ATTRIBUTE_REPARSE_POINT; @@ -22,8 +25,9 @@ pub(super) fn ensure_codex_app_runtime_paths_readable( log: &mut dyn Write, ) -> Result<()> { let read_execute_mask = FILE_GENERIC_READ | FILE_GENERIC_EXECUTE; + let local_app_data = local_app_data_root(); let runtime_paths = runtime_paths( - local_app_data_root(), + local_app_data.clone(), std::env::var_os("USERPROFILE").map(PathBuf::from), ); @@ -68,10 +72,9 @@ pub(super) fn ensure_codex_app_runtime_paths_readable( ), )?; let result = unsafe { - ensure_allow_mask_aces_with_inheritance( + grant_read_execute_aces( &runtime_path, &[sandbox_group_psid], - read_execute_mask, OBJECT_INHERIT_ACE | CONTAINER_INHERIT_ACE, ) }; @@ -89,6 +92,93 @@ pub(super) fn ensure_codex_app_runtime_paths_readable( )?; } } + if let Some(local_app_data) = local_app_data { + let runtime_root = local_app_data.join("OpenAI").join("Codex").join("runtimes"); + if let Err(err) = ensure_runtime_tree_readable(&runtime_root, sandbox_group_psid) { + let message = format!("runtime read/execute validation failed: {err:#}"); + super::log_line(log, &message)?; + refresh_errors.push(message); + } + } + Ok(()) +} + +fn ensure_runtime_tree_readable( + runtime_root: &Path, + sandbox_group_psid: *mut c_void, +) -> Result<()> { + for ancestor in runtime_root.ancestors() { + let metadata = match std::fs::symlink_metadata(ancestor) { + Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Ok(()), + result => result + .with_context(|| format!("inspect runtime ancestor {}", ancestor.display()))?, + }; + if !metadata.is_dir() || metadata.file_attributes() & FILE_ATTRIBUTE_REPARSE_POINT != 0 { + return Ok(()); + } + } + + let read_execute_mask = FILE_GENERIC_READ | FILE_GENERIC_EXECUTE; + let mut paths = vec![runtime_root.to_path_buf()]; + while let Some(path) = paths.pop() { + let metadata = match std::fs::symlink_metadata(&path) { + Err(err) if err.kind() == std::io::ErrorKind::NotFound => continue, + result => result.with_context(|| format!("inspect runtime path {}", path.display()))?, + }; + if metadata.file_attributes() & FILE_ATTRIBUTE_REPARSE_POINT != 0 { + continue; + } + + // A readable parent does not prove that existing runtime children inherited its ACL. + let access_result = unsafe { + grant_read_execute_aces( + &path, + &[sandbox_group_psid], + if metadata.is_dir() { + OBJECT_INHERIT_ACE | CONTAINER_INHERIT_ACE + } else { + 0 + }, + ) + } + .and_then(|changed| { + if changed { + ensure!( + path_mask_allows( + &path, + &[sandbox_group_psid], + read_execute_mask, + /*require_all_bits*/ true, + )?, + "runtime read/execute access still missing" + ); + } + Ok(()) + }); + if let Err(err) = access_result { + if std::fs::symlink_metadata(&path) + .is_err_and(|err| err.kind() == std::io::ErrorKind::NotFound) + { + continue; + } + return Err(err).with_context(|| { + format!("validate runtime read/execute access on {}", path.display()) + }); + } + if metadata.is_dir() { + let entries = match std::fs::read_dir(&path) { + Err(err) if err.kind() == std::io::ErrorKind::NotFound => continue, + result => result + .with_context(|| format!("enumerate runtime directory {}", path.display()))?, + }; + for entry in entries { + match entry { + Err(err) if err.kind() == std::io::ErrorKind::NotFound => continue, + result => paths.push(result?.path()), + } + } + } + } Ok(()) } diff --git a/codex-rs/windows-sandbox-rs/src/setup_provisioning/setup_runtime_bin_tests.rs b/codex-rs/windows-sandbox-rs/src/setup_provisioning/setup_runtime_bin_tests.rs index ad8aa7c3e4..5a6c1fcc27 100644 --- a/codex-rs/windows-sandbox-rs/src/setup_provisioning/setup_runtime_bin_tests.rs +++ b/codex-rs/windows-sandbox-rs/src/setup_provisioning/setup_runtime_bin_tests.rs @@ -1,6 +1,219 @@ +use super::ensure_runtime_tree_readable; use super::runtime_paths; +use crate::LocalSid; +use crate::add_deny_read_ace; +use crate::add_deny_write_ace; +use crate::ensure_allow_mask_aces_with_inheritance; +use crate::path_mask_allows; use pretty_assertions::assert_eq; +use std::fs; use std::path::PathBuf; +use windows_sys::Win32::Foundation::HLOCAL; +use windows_sys::Win32::Foundation::LocalFree; +use windows_sys::Win32::Security::Authorization::ConvertStringSecurityDescriptorToSecurityDescriptorW; +use windows_sys::Win32::Security::Authorization::SDDL_REVISION_1; +use windows_sys::Win32::Security::CheckTokenMembership; +use windows_sys::Win32::Security::DACL_SECURITY_INFORMATION; +use windows_sys::Win32::Security::PROTECTED_DACL_SECURITY_INFORMATION; +use windows_sys::Win32::Security::SetFileSecurityW; +use windows_sys::Win32::Storage::FileSystem::DELETE; +use windows_sys::Win32::Storage::FileSystem::FILE_APPEND_DATA; +use windows_sys::Win32::Storage::FileSystem::FILE_GENERIC_EXECUTE; +use windows_sys::Win32::Storage::FileSystem::FILE_GENERIC_READ; +use windows_sys::Win32::Storage::FileSystem::FILE_WRITE_DATA; +use windows_sys::Win32::Storage::FileSystem::WRITE_DAC; + +#[test] +fn runtime_repair_preserves_other_trustees_inherited_file_denial() { + for (denied_trustee, granted_sid) in [("WD", "S-1-5-11"), ("S-1-5-32-545", "S-1-1-0")] { + let ancestor = tempfile::tempdir().expect("runtime ancestor"); + let granted_sid = LocalSid::from_string(granted_sid).expect("grant SID"); + let mut member = 0; + assert_ne!( + unsafe { + CheckTokenMembership(/*tokenhandle*/ 0, granted_sid.as_ptr(), &mut member) + }, + 0, + ); + assert_ne!(member, 0, "the real read must exercise the added grant"); + + unsafe { + let mut descriptor = std::ptr::null_mut(); + assert_ne!( + ConvertStringSecurityDescriptorToSecurityDescriptorW( + crate::winutil::to_wide(format!( + "D:P(D;OIIO;0x1;;;{denied_trustee})(A;OICI;FA;;;OW)" + )) + .as_ptr(), + SDDL_REVISION_1, + &mut descriptor, + std::ptr::null_mut(), + ), + 0, + ); + let set = SetFileSecurityW( + crate::winutil::to_wide(ancestor.path()).as_ptr(), + DACL_SECURITY_INFORMATION | PROTECTED_DACL_SECURITY_INFORMATION, + descriptor, + ); + LocalFree(descriptor as HLOCAL); + assert_ne!(set, 0, "install an inheritable file-data denial"); + } + let runtime = ancestor.path().join("runtimes"); + fs::create_dir(&runtime).expect("runtime directory"); + let module = runtime.join("private.js"); + fs::write(&module, b"private runtime content").expect("runtime file"); + for repair in [false, true] { + if repair { + ensure_runtime_tree_readable(&runtime, granted_sid.as_ptr()) + .expect("repair must preserve inherited denies"); + } + assert_eq!( + fs::read(&module) + .expect_err("file-data denial must remain effective") + .kind(), + std::io::ErrorKind::PermissionDenied, + ); + } + } +} + +#[test] +fn runtime_repair_does_not_follow_directory_junctions() { + let runtime = tempfile::tempdir().expect("runtime directory"); + let external = tempfile::tempdir().expect("external directory"); + let external_file = external.path().join("private.js"); + fs::write(&external_file, b"external content").expect("external file"); + let alias = runtime.path().join("linked-runtime"); + let output = std::process::Command::new("cmd") + .args(["/C", "mklink", "/J"]) + .arg(&alias) + .arg(external.path()) + .output() + .expect("create directory junction"); + assert!(output.status.success(), "mklink failed: {output:?}"); + let sandbox_sid = LocalSid::from_string("S-1-5-21-10-20-30-40").expect("test SID"); + for root in [runtime.path(), alias.as_path()] { + ensure_runtime_tree_readable(root, sandbox_sid.as_ptr()).expect("repair runtimes"); + for path in [external.path(), external_file.as_path()] { + assert!( + !path_mask_allows( + path, + &[sandbox_sid.as_ptr()], + FILE_GENERIC_READ | FILE_GENERIC_EXECUTE, + /*require_all_bits*/ false, + ) + .expect("external ACL must remain untouched") + ); + } + } + fs::remove_dir(&alias).expect("remove junction"); +} + +#[test] +fn repairs_children_when_runtime_root_already_has_read_execute_access() { + let runtime = tempfile::tempdir().expect("runtime directory"); + let node = runtime.path().join("bin/node.exe"); + let module = runtime.path().join("sky/dist/project/index.js"); + for path in [&node, &module] { + fs::create_dir_all(path.parent().expect("parent")).expect("runtime subdirectory"); + fs::write(path, b"runtime content").expect("runtime file"); + } + let sandbox_sid = LocalSid::from_string("S-1-5-21-10-20-30-40").expect("test SID"); + let read_execute_mask = FILE_GENERIC_READ | FILE_GENERIC_EXECUTE; + unsafe { + ensure_allow_mask_aces_with_inheritance( + runtime.path(), + &[sandbox_sid.as_ptr()], + read_execute_mask, + /*inheritance*/ 0, + ) + } + .expect("grant root-only read/execute access"); + assert!( + path_mask_allows( + runtime.path(), + &[sandbox_sid.as_ptr()], + read_execute_mask, + /*require_all_bits*/ true, + ) + .expect("root access") + ); + for path in [&node, &module] { + assert!( + !path_mask_allows( + path, + &[sandbox_sid.as_ptr()], + read_execute_mask, + /*require_all_bits*/ true, + ) + .expect("child access before repair") + ); + } + + let denied_directory = runtime.path().join("denied"); + fs::create_dir(&denied_directory).expect("denied directory"); + assert!( + unsafe { add_deny_read_ace(&denied_directory, sandbox_sid.as_ptr()) } + .expect("deny directory reads") + ); + let denied_child = denied_directory.join("private.js"); + fs::write(&denied_child, b"private runtime content").expect("denied child"); + let write_denied_file = runtime.path().join("read-only.js"); + fs::write(&write_denied_file, b"read-only runtime content").expect("write-denied file"); + assert!( + unsafe { add_deny_write_ace(&write_denied_file, sandbox_sid.as_ptr()) } + .expect("deny runtime writes") + ); + + for _ in 0..2 { + ensure_runtime_tree_readable(runtime.path(), sandbox_sid.as_ptr()) + .expect("repair runtimes"); + for path in [&denied_directory, &denied_child] { + assert!( + !unsafe { add_deny_read_ace(path, sandbox_sid.as_ptr()) } + .expect("read denial must survive repair") + ); + assert!( + !path_mask_allows( + path, + &[sandbox_sid.as_ptr()], + read_execute_mask, + /*require_all_bits*/ false, + ) + .expect("repair must not add an allow ahead of inherited denial") + ); + } + assert!( + !unsafe { add_deny_write_ace(&write_denied_file, sandbox_sid.as_ptr()) } + .expect("write denial must survive repair") + ); + for path in [ + node.as_path(), + module.as_path(), + module.parent().expect("module directory"), + ] { + assert!( + path_mask_allows( + path, + &[sandbox_sid.as_ptr()], + read_execute_mask, + /*require_all_bits*/ true, + ) + .expect("child access after repair") + ); + assert!( + !path_mask_allows( + path, + &[sandbox_sid.as_ptr()], + FILE_WRITE_DATA | FILE_APPEND_DATA | DELETE | WRITE_DAC, + /*require_all_bits*/ false, + ) + .expect("repair must not grant write or ACL management") + ); + } + } +} #[test] fn runtime_paths_include_desktop_parent_and_primary_runtime_roots() {