diff --git a/codex-rs/windows-sandbox-rs/src/acl.rs b/codex-rs/windows-sandbox-rs/src/acl.rs index 064b7fe6fb..8818d89a4e 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,10 @@ 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_for_sid(p_dacl, psid, FILE_GENERIC_READ | GENERIC_READ_MASK) +} + +unsafe fn dacl_has_deny_mask_for_sid(p_dacl: *mut ACL, psid: *mut c_void, deny_mask: u32) -> bool { if p_dacl.is_null() { return false; } @@ -457,7 +466,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 { @@ -474,7 +482,7 @@ pub unsafe fn dacl_has_read_deny_for_sid(p_dacl: *mut ACL, psid: *mut c_void) -> 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 EqualSid(sid_ptr, psid) != 0 && (ace.Mask & deny_mask) != 0 { return true; } } @@ -522,17 +530,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 { + // An explicit allow can outrank an inherited deny. All file generic rights + // overlap read/execute through READ_CONTROL or SYNCHRONIZE. + if access_mode == GRANT_ACCESS + && dacl_has_deny_mask_for_sid( + p_dacl, + *sid, + 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 +629,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 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 +683,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/bin/setup_main/win/setup_runtime_bin.rs b/codex-rs/windows-sandbox-rs/src/bin/setup_main/win/setup_runtime_bin.rs index 176ef07504..cdd3a77bc5 100644 --- a/codex-rs/windows-sandbox-rs/src/bin/setup_main/win/setup_runtime_bin.rs +++ b/codex-rs/windows-sandbox-rs/src/bin/setup_main/win/setup_runtime_bin.rs @@ -1,10 +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 anyhow::Context; use anyhow::Result; +use anyhow::ensure; use codex_windows_sandbox::ensure_allow_mask_aces_with_inheritance; +use codex_windows_sandbox::grant_read_execute_aces; use codex_windows_sandbox::path_mask_allows; use windows_sys::Win32::Security::CONTAINER_INHERIT_ACE; use windows_sys::Win32::Security::OBJECT_INHERIT_ACE; @@ -22,8 +26,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), ); @@ -89,6 +94,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/bin/setup_main/win/setup_runtime_bin_tests.rs b/codex-rs/windows-sandbox-rs/src/bin/setup_main/win/setup_runtime_bin_tests.rs index ad8aa7c3e4..14ab1bd05a 100644 --- a/codex-rs/windows-sandbox-rs/src/bin/setup_main/win/setup_runtime_bin_tests.rs +++ b/codex-rs/windows-sandbox-rs/src/bin/setup_main/win/setup_runtime_bin_tests.rs @@ -1,6 +1,124 @@ +use super::ensure_runtime_tree_readable; use super::runtime_paths; +use codex_windows_sandbox::LocalSid; +use codex_windows_sandbox::add_deny_read_ace; +use codex_windows_sandbox::add_deny_write_ace; +use codex_windows_sandbox::ensure_allow_mask_aces_with_inheritance; +use codex_windows_sandbox::path_mask_allows; use pretty_assertions::assert_eq; +use std::fs; use std::path::PathBuf; +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 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() { diff --git a/codex-rs/windows-sandbox-rs/src/desktop_tests.rs b/codex-rs/windows-sandbox-rs/src/desktop_tests.rs index 2c8f35c961..318707e625 100644 --- a/codex-rs/windows-sandbox-rs/src/desktop_tests.rs +++ b/codex-rs/windows-sandbox-rs/src/desktop_tests.rs @@ -7,6 +7,7 @@ use crate::setup::SandboxSetupRequest; use crate::setup::SetupRootOverrides; use crate::spawn_prep::legacy_session_capability_roots; use crate::spawn_prep::prepare_legacy_session_security; +use crate::winutil::current_account_name; use anyhow::Result; use codex_protocol::models::PermissionProfile; use codex_protocol::permissions::NetworkSandboxPolicy; @@ -20,10 +21,6 @@ use std::os::windows::io::FromRawHandle; use std::os::windows::io::OwnedHandle; use std::path::Path; use tempfile::TempDir; -use windows_sys::Win32::NetworkManagement::NetManagement::DNLEN; -use windows_sys::Win32::NetworkManagement::NetManagement::UNLEN; -use windows_sys::Win32::Security::Authentication::Identity::GetUserNameExW; -use windows_sys::Win32::Security::Authentication::Identity::NameSamCompatible; #[test] fn private_desktop_rejects_interactive_and_injected_names() { @@ -219,16 +216,6 @@ fn legacy_desktop_reuses_only_equivalent_permissions() -> Result<()> { Ok(()) } -fn current_account_name() -> Result { - // Bazel does not provide USERDOMAIN or USERNAME in the test environment. - let mut account = [0; (DNLEN + UNLEN + 2) as usize]; - let mut length = account.len() as u32; - if unsafe { GetUserNameExW(NameSamCompatible, account.as_mut_ptr(), &mut length) } == 0 { - return Err(std::io::Error::last_os_error().into()); - } - Ok(String::from_utf16(&account[..length as usize])?) -} - fn workspace_permissions(workspace: &Path) -> Result { ResolvedWindowsSandboxPermissions::try_from_permission_profile_for_workspace_roots( &PermissionProfile::workspace_write_with( diff --git a/codex-rs/windows-sandbox-rs/src/lib.rs b/codex-rs/windows-sandbox-rs/src/lib.rs index e83a42a59f..5b16de0609 100644 --- a/codex-rs/windows-sandbox-rs/src/lib.rs +++ b/codex-rs/windows-sandbox-rs/src/lib.rs @@ -170,6 +170,8 @@ pub use acl::ensure_allow_write_aces; #[cfg(target_os = "windows")] pub use acl::fetch_dacl_handle; #[cfg(target_os = "windows")] +pub use acl::grant_read_execute_aces; +#[cfg(target_os = "windows")] pub use acl::path_has_standard_user_mutation_allow; #[cfg(target_os = "windows")] pub use acl::path_has_trusted_system_owner; diff --git a/codex-rs/windows-sandbox-rs/src/setup.rs b/codex-rs/windows-sandbox-rs/src/setup.rs index 1e84b6fef4..031a944798 100644 --- a/codex-rs/windows-sandbox-rs/src/setup.rs +++ b/codex-rs/windows-sandbox-rs/src/setup.rs @@ -33,6 +33,7 @@ use crate::setup_error::extract_failure; use crate::setup_error::failure; use crate::setup_error::read_setup_error_report; use crate::ssh_config_dependencies::ssh_config_dependency_paths; +use crate::winutil::current_account_name; use anyhow::Result; use anyhow::anyhow; use base64::Engine; @@ -348,7 +349,7 @@ fn run_setup_refresh_inner( proxy_ports: offline_proxy_settings.proxy_ports, allow_local_binding: offline_proxy_settings.allow_local_binding, otel: None, - real_user: std::env::var("USERNAME").unwrap_or_else(|_| "Administrators".to_string()), + real_user: current_account_name()?, mode: SetupMode::Full, refresh_only: true, }; @@ -1065,7 +1066,7 @@ fn run_elevated_setup_inner( format!("failed to create sandbox dir {}: {err}", sbx_dir.display()), ) })?; - let payload = elevated_provisioning_payload(&request, offline_proxy_settings_override); + let payload = elevated_provisioning_payload(&request, offline_proxy_settings_override)?; let needs_elevation = !is_elevated().map_err(|err| { failure( SetupErrorCode::OrchestratorElevationCheckFailed, @@ -1078,10 +1079,10 @@ fn run_elevated_setup_inner( fn elevated_provisioning_payload( request: &SandboxSetupRequest<'_>, offline_proxy_settings_override: Option<&OfflineProxySettings>, -) -> ElevationPayload { +) -> Result { let offline_proxy_settings = offline_proxy_settings_for_request(request, offline_proxy_settings_override); - ElevationPayload { + Ok(ElevationPayload { version: SETUP_VERSION, offline_username: OFFLINE_USERNAME.to_string(), online_username: ONLINE_USERNAME.to_string(), @@ -1093,11 +1094,11 @@ fn elevated_provisioning_payload( deny_write_paths: Vec::new(), 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()), + real_user: current_account_name()?, otel: codex_otel::global_statsig_metrics_settings(), mode: SetupMode::InteractiveProvision, refresh_only: false, - } + }) } pub fn run_elevated_provisioning_setup( @@ -1600,7 +1601,8 @@ mod tests { let payload = super::elevated_provisioning_payload( &request, /*offline_proxy_settings_override*/ None, - ); + ) + .expect("elevated provisioning payload"); assert_eq!(payload.command_cwd, codex_home); assert_eq!(payload.read_roots, Vec::::new()); diff --git a/codex-rs/windows-sandbox-rs/src/winutil.rs b/codex-rs/windows-sandbox-rs/src/winutil.rs index 502d9024bf..711e2eab79 100644 --- a/codex-rs/windows-sandbox-rs/src/winutil.rs +++ b/codex-rs/windows-sandbox-rs/src/winutil.rs @@ -6,6 +6,7 @@ use windows_sys::Win32::Foundation::ERROR_INSUFFICIENT_BUFFER; use windows_sys::Win32::Foundation::GetLastError; use windows_sys::Win32::Foundation::HLOCAL; use windows_sys::Win32::Foundation::LocalFree; +use windows_sys::Win32::NetworkManagement::NetManagement::DNLEN; use windows_sys::Win32::NetworkManagement::NetManagement::LOCALGROUP_INFO_1; use windows_sys::Win32::NetworkManagement::NetManagement::NERR_Success; use windows_sys::Win32::NetworkManagement::NetManagement::NERR_UserNotFound; @@ -13,8 +14,11 @@ use windows_sys::Win32::NetworkManagement::NetManagement::NetApiBufferFree; use windows_sys::Win32::NetworkManagement::NetManagement::NetLocalGroupAdd; use windows_sys::Win32::NetworkManagement::NetManagement::NetUserGetInfo; use windows_sys::Win32::NetworkManagement::NetManagement::NetUserSetInfo; +use windows_sys::Win32::NetworkManagement::NetManagement::UNLEN; use windows_sys::Win32::NetworkManagement::NetManagement::USER_INFO_1; use windows_sys::Win32::NetworkManagement::NetManagement::USER_INFO_1008; +use windows_sys::Win32::Security::Authentication::Identity::GetUserNameExW; +use windows_sys::Win32::Security::Authentication::Identity::NameSamCompatible; use windows_sys::Win32::Security::Authorization::ConvertSidToStringSidW; use windows_sys::Win32::Security::Authorization::ConvertStringSidToSidW; use windows_sys::Win32::Security::CopySid; @@ -29,6 +33,16 @@ use windows_sys::Win32::System::Diagnostics::Debug::FormatMessageW; pub const SANDBOX_USERS_GROUP: &str = "CodexSandboxUsers"; const SANDBOX_USERS_GROUP_COMMENT: &str = "Codex sandbox internal group (managed)"; +pub(crate) fn current_account_name() -> Result { + // Sandbox launchers can filter USERNAME, so resolve the caller from its Windows token. + let mut account = [0; (DNLEN + UNLEN + 2) as usize]; + let mut length = account.len() as u32; + if unsafe { GetUserNameExW(NameSamCompatible, account.as_mut_ptr(), &mut length) } == 0 { + return Err(std::io::Error::last_os_error()).context("resolve current Windows account"); + } + Ok(String::from_utf16(&account[..length as usize])?) +} + pub fn to_wide>(s: S) -> Vec { let mut v: Vec = s.as_ref().encode_wide().collect(); v.push(0);