From 7e8878f60560f8ff964fe38712e53def06402db7 Mon Sep 17 00:00:00 2001 From: johnl-oai Date: Wed, 30 Sep 2026 17:31:13 +0000 Subject: [PATCH] Preserve PowerShell relative paths in the elevated Windows sandbox (#49690) ## Why An inaccessible user profile directory can prevent sandboxed PowerShell from preserving its working directory and resolving relative paths in a project beneath that profile. ## What changed Pass `USERPROFILE` to elevated setup and refresh. When the command's working directory is beneath an absolute profile path, grant the sandbox group non-inheriting `FILE_READ_ATTRIBUTES` access to the profile directory. Preserve existing permissions and deny ACEs during non-inheriting ACL updates without propagating changes to descendants. Expand generic deny masks when checking for conflicting permissions. ## Testing Add a Windows regression test that checks PowerShell's working directory and reading `.\public.txt` beneath a protected profile. Add an ACL test that verifies a non-inheriting grant leaves an unprotected child's security descriptor unchanged. GitOrigin-RevId: 7f142a7ffdee795c3255d4a6e79568fa9391c055 --- codex-rs/core/tests/suite/windows_sandbox.rs | 128 ++++++++++++++++++ codex-rs/windows-sandbox-rs/src/acl.rs | 84 ++++++++++-- codex-rs/windows-sandbox-rs/src/acl_tests.rs | 85 ++++++++++++ codex-rs/windows-sandbox-rs/src/setup.rs | 9 ++ .../src/setup_provisioning.rs | 47 +++++++ .../src/setup_provisioning/acl_tests.rs | 1 + .../src/setup_provisioning/service.rs | 1 + 7 files changed, 344 insertions(+), 11 deletions(-) diff --git a/codex-rs/core/tests/suite/windows_sandbox.rs b/codex-rs/core/tests/suite/windows_sandbox.rs index ccbabef97f..8369249ed7 100644 --- a/codex-rs/core/tests/suite/windows_sandbox.rs +++ b/codex-rs/core/tests/suite/windows_sandbox.rs @@ -669,6 +669,134 @@ async fn windows_elevated_enforces_deny_read_and_protects_setup_marker() -> anyh Ok(()) } +#[tokio::test] +#[serial(codex_home)] +async fn windows_elevated_powershell_preserves_relative_paths() -> anyhow::Result<()> { + let _account_guard = WindowsSandboxAccountTestGuard::acquire()?; + let codex_home = codex_home_for_windows_sandbox_test( + "windows-elevated-powershell-relative-paths-codex-home", + )?; + let _codex_home_guard = EnvVarGuard::set("CODEX_HOME", codex_home.path().as_os_str()); + stage_windows_sandbox_helpers()?; + // The refresh caller must own the helper directory after provisioning protects its DACL. + std::fs::create_dir_all(codex_windows_sandbox::sandbox_bin_dir(codex_home.path()))?; + + let profile_dir = TempDir::new()?; + let fake_profile = dunce::canonicalize(profile_dir.path())?; + // Exclude inherited sandbox permissions so this reproduces an inaccessible profile root. + let profile_acl_setup = std::process::Command::new("powershell.exe") + .args(["-NoLogo", "-NoProfile", "-NonInteractive", "-Command"]) + .arg( + r#"$ErrorActionPreference = 'Stop' +$owner = [System.Security.Principal.WindowsIdentity]::GetCurrent().User.Value +$security = [System.Security.AccessControl.DirectorySecurity]::new() +$security.SetSecurityDescriptorSddlForm("D:P(A;OICI;FA;;;${owner})(A;OICI;FA;;;SY)(A;OICI;FA;;;BA)", [System.Security.AccessControl.AccessControlSections]::Access) +[System.IO.DirectoryInfo]::new($env:CODEX_TEST_PROFILE).SetAccessControl($security)"#, + ) + .env("CODEX_TEST_PROFILE", &fake_profile) + .output() + .context("configure protected synthetic user-profile ACL")?; + assert!( + profile_acl_setup.status.success(), + "synthetic profile ACL setup failed: {profile_acl_setup:?}" + ); + let workspace = fake_profile.join("project"); + std::fs::create_dir(&workspace)?; + let cwd = dunce::canonicalize(&workspace)?.abs(); + std::fs::write(cwd.join("public.txt"), "public ok\n")?; + // Elevated setup requires root read access, including PowerShell's runtime. + let file_system_sandbox_policy = FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry::new( + FileSystemPath::Special { + value: FileSystemSpecialPath::Root, + }, + FileSystemAccessMode::Read, + ), + FileSystemSandboxEntry::new(cwd.clone().into(), FileSystemAccessMode::Write), + ]); + let permission_profile = PermissionProfile::from_runtime_permissions( + &file_system_sandbox_policy, + NetworkSandboxPolicy::Restricted, + ); + let env = HashMap::from([ + ( + "USERPROFILE".to_string(), + fake_profile.to_string_lossy().into_owned(), + ), + ( + "SystemRoot".to_string(), + std::env::var("SystemRoot").context("Windows PowerShell requires SystemRoot")?, + ), + ]); + // Provision and refresh synchronously so USERPROFILE is restored before any await. + { + let _user_profile_guard = EnvVarGuard::set("USERPROFILE", fake_profile.as_os_str()); + codex_core::windows_sandbox::prepare_elevated_sandbox( + &permission_profile, + std::slice::from_ref(&cwd), + cwd.as_path(), + &env, + codex_home.path(), + )?; + } + + let expected = format!("CWD={}\nPUBLIC=public ok", cwd.as_path().display()); + let mut attempts = 0; + loop { + let output = process_exec_tool_call( + ExecParams { + command: [ + "powershell.exe", + "-NoLogo", + "-NoProfile", + "-NonInteractive", + "-Command", + r#"$ErrorActionPreference = 'Stop' +Write-Output ('CWD=' + (Get-Location).ProviderPath) +Write-Output ('PUBLIC=' + (Get-Content -LiteralPath .\public.txt -Raw).Trim())"#, + ] + .map(str::to_owned) + .into(), + cwd: cwd.clone(), + expiration: 30_000.into(), + capture_policy: ExecCapturePolicy::ShellTool, + env: env.clone(), + network: None, + network_environment_id: None, + sandbox_permissions: SandboxPermissions::UseDefault, + windows_sandbox_level: WindowsSandboxLevel::Elevated, + justification: None, + arg0: None, + }, + &permission_profile, + &cwd, + std::slice::from_ref(&cwd), + &None, + /*codex_self_exe*/ &None, + /*use_legacy_landlock*/ false, + /*stdout_stream*/ None, + ) + .await?; + if output.exit_code == 0 + && output + .stdout + .text + .trim() + .replace("\r\n", "\n") + .eq_ignore_ascii_case(&expected) + { + return Ok(()); + } + attempts += 1; + assert!( + attempts < 5, + "PowerShell should preserve cwd and read .\\public.txt: {output:?}" + ); + // The async read-ACL helper briefly uses a temporary startup junction. + tokio::time::sleep(std::time::Duration::from_millis(200)).await; + } +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] #[serial(codex_home)] async fn windows_elevated_unified_exec_enforces_large_recursive_deny_reads() -> anyhow::Result<()> { diff --git a/codex-rs/windows-sandbox-rs/src/acl.rs b/codex-rs/windows-sandbox-rs/src/acl.rs index c91fb91a79..b0f466f2ac 100644 --- a/codex-rs/windows-sandbox-rs/src/acl.rs +++ b/codex-rs/windows-sandbox-rs/src/acl.rs @@ -12,6 +12,7 @@ use std::os::windows::io::AsRawHandle; use std::path::Path; use windows_sys::Win32::Foundation::CloseHandle; use windows_sys::Win32::Foundation::ERROR_SUCCESS; +use windows_sys::Win32::Foundation::HANDLE; use windows_sys::Win32::Foundation::HLOCAL; use windows_sys::Win32::Foundation::INVALID_HANDLE_VALUE; use windows_sys::Win32::Foundation::LocalFree; @@ -63,6 +64,7 @@ use windows_sys::Win32::Storage::FileSystem::READ_CONTROL; use windows_sys::Win32::Storage::FileSystem::VOLUME_NAME_NONE; use windows_sys::Win32::Storage::FileSystem::WRITE_DAC; use windows_sys::Win32::Storage::FileSystem::WRITE_OWNER; +use windows_sys::Win32::System::SystemServices::MAXIMUM_ALLOWED; const SE_KERNEL_OBJECT: u32 = 6; const OBJECT_INHERIT_ACE_FLAG: u8 = 0x01; const INHERIT_ONLY_ACE: u8 = 0x08; @@ -469,6 +471,12 @@ unsafe fn dacl_has_deny_mask(p_dacl: *mut ACL, scope: DenyAceScope, deny_mask: u if ok == 0 { return false; } + let mapping = GENERIC_MAPPING { + GenericRead: FILE_GENERIC_READ, + GenericWrite: FILE_GENERIC_WRITE, + GenericExecute: FILE_GENERIC_EXECUTE, + GenericAll: FILE_ALL_ACCESS, + }; 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 { @@ -485,7 +493,9 @@ unsafe fn dacl_has_deny_mask(p_dacl: *mut ACL, scope: DenyAceScope, deny_mask: u { continue; } - if (ace.Mask & deny_mask) != 0 { + let mut denied_mask = ace.Mask; + MapGenericMask(&mut denied_mask, &mapping); + if ((ace.Mask | denied_mask) & deny_mask) != 0 { return true; } } @@ -536,7 +546,52 @@ unsafe fn ensure_allow_mask_aces_with_inheritance_impl( access_mode: ACCESS_MODE, inheritance: u32, ) -> Result { - let (p_dacl, p_sd) = fetch_dacl_handle(path)?; + let directory = if inheritance == 0 { + Some( + OpenOptions::new() + .access_mode(MAXIMUM_ALLOWED) + .share_mode(FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE) + .custom_flags(FILE_FLAG_BACKUP_SEMANTICS) + .open(path) + .context("open ACL target for root-only update")?, + ) + } else { + None + }; + // A root-only grant must preserve existing permissions and deny ACEs. + let access_mode = if directory.is_some() { + GRANT_ACCESS + } else { + access_mode + }; + + let (p_dacl, p_sd) = match directory.as_ref() { + Some(directory) => { + let mut security_descriptor: *mut c_void = std::ptr::null_mut(); + let mut acl: *mut ACL = std::ptr::null_mut(); + let code = GetSecurityInfo( + directory.as_raw_handle() as HANDLE, + /*objecttype*/ 1, + DACL_SECURITY_INFORMATION, + std::ptr::null_mut(), + std::ptr::null_mut(), + &mut acl, + std::ptr::null_mut(), + &mut security_descriptor, + ); + if code != ERROR_SUCCESS { + if !security_descriptor.is_null() { + LocalFree(security_descriptor as HLOCAL); + } + return Err(anyhow!( + "read ACL for root-only update on {}: {code}", + path.display() + )); + } + (acl, security_descriptor) + } + None => 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 @@ -554,7 +609,9 @@ unsafe fn ensure_allow_mask_aces_with_inheritance_impl( { continue; } - if !dacl_allow_mask_needs_refresh(p_dacl, *sid, allow_mask, disallow_mask) { + if !dacl_allow_mask_needs_refresh(p_dacl, *sid, allow_mask, disallow_mask) + || directory.is_some() && p_dacl.is_null() + { continue; } entries.push(EXPLICIT_ACCESS_W { @@ -586,14 +643,18 @@ unsafe fn ensure_allow_mask_aces_with_inheritance_impl( if code2 != ERROR_SUCCESS { return Err(anyhow!("SetEntriesInAclW failed: {code2}")); } - // Keep no-op checks read-only; request ACL-write access only when - // a grant actually needs updating. Use the same long-path support. - let handle = OpenOptions::new() - .access_mode(READ_CONTROL | WRITE_DAC) - .share_mode(FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE) - .custom_flags(FILE_FLAG_BACKUP_SEMANTICS) - .open(path) - .context("open ACL target for update")?; + // For inheriting grants, request ACL-write access only when an update + // is needed. Use the same long-path support as the read handle. + let handle = match directory { + // MAXIMUM_ALLOWED suppresses propagation of existing inheritable ACEs too. + Some(directory) => directory, + None => OpenOptions::new() + .access_mode(READ_CONTROL | WRITE_DAC) + .share_mode(FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE) + .custom_flags(FILE_FLAG_BACKUP_SEMANTICS) + .open(path) + .context("open ACL target for update")?, + }; let code3 = SetSecurityInfo( handle.as_raw_handle() as _, 1, @@ -619,6 +680,7 @@ unsafe fn ensure_allow_mask_aces_with_inheritance_impl( /// Ensure all provided SIDs have an allow ACE with the requested mask on the path. /// Returns true if any ACE was added. +/// Zero inheritance preserves existing ACEs without propagating changes to descendants. /// /// # Safety /// Caller must pass valid SID pointers and an existing path; free the returned security descriptor with `LocalFree`. diff --git a/codex-rs/windows-sandbox-rs/src/acl_tests.rs b/codex-rs/windows-sandbox-rs/src/acl_tests.rs index 4a1cd62444..44a1cd9067 100644 --- a/codex-rs/windows-sandbox-rs/src/acl_tests.rs +++ b/codex-rs/windows-sandbox-rs/src/acl_tests.rs @@ -1,21 +1,30 @@ use super::acl_api_result; use super::deny_ace_already_present; +use super::ensure_allow_mask_aces_with_inheritance; use super::ensure_handle_is_not_filesystem_root; +use super::fetch_dacl_handle; use crate::token::LocalSid; +use crate::winutil::to_wide; use pretty_assertions::assert_eq; +use std::ffi::c_void; use std::fs::OpenOptions; use std::os::windows::fs::OpenOptionsExt; +use std::path::Path; use windows_sys::Win32::Foundation::ERROR_ACCESS_DENIED; use windows_sys::Win32::Foundation::HLOCAL; use windows_sys::Win32::Foundation::LocalFree; +use windows_sys::Win32::Security::ACL_SIZE_INFORMATION; +use windows_sys::Win32::Security::AclSizeInformation; use windows_sys::Win32::Security::Authorization::ConvertStringSecurityDescriptorToSecurityDescriptorW; use windows_sys::Win32::Security::Authorization::SDDL_REVISION_1; use windows_sys::Win32::Security::DACL_SECURITY_INFORMATION; +use windows_sys::Win32::Security::GetAclInformation; use windows_sys::Win32::Security::GetSecurityDescriptorControl; use windows_sys::Win32::Security::SE_DACL_PROTECTED; use windows_sys::Win32::Security::SetFileSecurityW; use windows_sys::Win32::Security::UNPROTECTED_DACL_SECURITY_INFORMATION; use windows_sys::Win32::Storage::FileSystem::FILE_FLAG_BACKUP_SEMANTICS; +use windows_sys::Win32::Storage::FileSystem::FILE_READ_ATTRIBUTES; use windows_sys::Win32::Storage::FileSystem::READ_CONTROL; #[test] @@ -110,3 +119,79 @@ fn revoking_absent_sid_preserves_child_null_dacl() { } } } + +fn acl_snapshot(path: &Path) -> (u16, Vec) { + unsafe { + let (acl, descriptor) = fetch_dacl_handle(path).expect("read directory ACL"); + let mut control = 0; + let mut revision = 0; + assert_ne!( + GetSecurityDescriptorControl(descriptor, &mut control, &mut revision), + 0 + ); + let mut info: ACL_SIZE_INFORMATION = std::mem::zeroed(); + assert_ne!( + GetAclInformation( + acl, + &mut info as *mut _ as *mut c_void, + std::mem::size_of::() as u32, + AclSizeInformation, + ), + 0 + ); + let dacl = + std::slice::from_raw_parts(acl.cast::(), info.AclBytesInUse as usize).to_vec(); + LocalFree(descriptor as HLOCAL); + (control, dacl) + } +} + +#[test] +fn noninheriting_read_attributes_preserve_unprotected_child_dacl() { + let parent = tempfile::tempdir().expect("create parent"); + let child = parent.path().join("child"); + let marker = LocalSid::from_string("S-1-5-21-915763429-1123581321-271828182-4243").unwrap(); + let target = LocalSid::from_string("S-1-5-21-915763429-1123581321-271828182-4244").unwrap(); + std::fs::create_dir(&child).unwrap(); + let original = acl_snapshot(&child).1; + + // Restore the original child DACL after adding an inheritable parent ACE. + // An ordinary parent update would re-propagate this missing permission. + unsafe { + let (_, descriptor) = fetch_dacl_handle(&child).unwrap(); + ensure_allow_mask_aces_with_inheritance( + parent.path(), + &[marker.as_ptr()], + FILE_READ_ATTRIBUTES, + super::CONTAINER_INHERIT_ACE | super::OBJECT_INHERIT_ACE, + ) + .unwrap(); + assert_ne!( + acl_snapshot(&child).1, + original, + "child should inherit the ACE" + ); + assert_ne!( + SetFileSecurityW( + to_wide(&child).as_ptr(), + DACL_SECURITY_INFORMATION, + descriptor + ), + 0 + ); + LocalFree(descriptor as HLOCAL); + } + let before = acl_snapshot(&child); + assert_eq!(before.0 & SE_DACL_PROTECTED, 0); + + assert!(unsafe { + ensure_allow_mask_aces_with_inheritance( + parent.path(), + &[target.as_ptr()], + FILE_READ_ATTRIBUTES, + /*inheritance*/ 0, + ) + .unwrap() + }); + assert_eq!(acl_snapshot(&child), before); +} diff --git a/codex-rs/windows-sandbox-rs/src/setup.rs b/codex-rs/windows-sandbox-rs/src/setup.rs index 484f286713..618ea85011 100644 --- a/codex-rs/windows-sandbox-rs/src/setup.rs +++ b/codex-rs/windows-sandbox-rs/src/setup.rs @@ -360,6 +360,9 @@ fn run_setup_refresh_inner( allow_local_binding: offline_proxy_settings.allow_local_binding, otel: None, real_user: crate::runtime_ownership::current_setup_user()?, + user_profile: std::env::var_os("USERPROFILE") + .filter(|value| !value.is_empty()) + .map(PathBuf::from), mode: SetupMode::Full, runtime, refresh_only: true, @@ -715,6 +718,8 @@ struct ElevationPayload { allow_local_binding: bool, otel: Option, real_user: String, + #[serde(skip_serializing_if = "Option::is_none")] + user_profile: Option, mode: SetupMode, #[serde(default, skip_serializing_if = "SetupRuntime::is_legacy")] runtime: SetupRuntime, @@ -1163,6 +1168,7 @@ fn elevated_provisioning_payload( allow_local_binding: offline_proxy_settings.allow_local_binding, real_user, otel: codex_otel::global_statsig_metrics_settings(), + user_profile: None, mode: SetupMode::InteractiveProvision, runtime: SetupRuntime::Legacy, refresh_only: false, @@ -1243,6 +1249,9 @@ pub fn run_elevated_provisioning_setup_with_retained_handles( allow_local_binding: settings.allow_local_binding, otel: codex_otel::global_statsig_metrics_settings(), real_user: real_user.to_string(), + user_profile: std::env::var_os("USERPROFILE") + .filter(|value| !value.is_empty()) + .map(PathBuf::from), mode: SetupMode::ProvisionOnly, runtime, refresh_only: false, diff --git a/codex-rs/windows-sandbox-rs/src/setup_provisioning.rs b/codex-rs/windows-sandbox-rs/src/setup_provisioning.rs index f60b1d9d5c..516b327357 100644 --- a/codex-rs/windows-sandbox-rs/src/setup_provisioning.rs +++ b/codex-rs/windows-sandbox-rs/src/setup_provisioning.rs @@ -81,6 +81,7 @@ 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; use windows_sys::Win32::Storage::FileSystem::FILE_GENERIC_WRITE; +use windows_sys::Win32::Storage::FileSystem::FILE_READ_ATTRIBUTES; use windows_sys::Win32::Storage::FileSystem::FILE_SHARE_DELETE; use windows_sys::Win32::Storage::FileSystem::FILE_SHARE_READ; use windows_sys::Win32::Storage::FileSystem::FILE_SHARE_WRITE; @@ -124,6 +125,8 @@ struct Payload { otel: Option, real_user: String, #[serde(default)] + user_profile: Option, + #[serde(default)] mode: SetupMode, #[serde(default, skip_serializing_if = "SetupRuntime::is_legacy")] runtime: SetupRuntime, @@ -946,6 +949,50 @@ fn run_setup_full(payload: &Payload, log: &mut dyn Write, sbx_dir: &Path) -> Res )?; } + if let Some(user_profile) = payload.user_profile.as_deref() + && user_profile.is_absolute() + { + let mut cwd_components = payload.command_cwd.components(); + let cwd_is_under_profile = user_profile.components().all(|profile_component| { + cwd_components.next().is_some_and(|cwd_component| { + cwd_component + .as_os_str() + .to_string_lossy() + .eq_ignore_ascii_case(&profile_component.as_os_str().to_string_lossy()) + }) + }); + if cwd_is_under_profile { + match unsafe { + ensure_allow_mask_aces_with_inheritance( + user_profile, + &[sandbox_group_psid], + FILE_READ_ATTRIBUTES, + /*inheritance*/ 0, + ) + } { + Ok(true) => { + log_line( + log, + &format!( + "granted non-inheriting read-attributes ACE on user profile {}", + user_profile.display() + ), + )?; + } + Ok(false) => {} + Err(err) => { + log_line( + log, + &format!( + "failed to grant non-inheriting read-attributes ACE on user profile {}: {err:#}; continuing setup", + user_profile.display() + ), + )?; + } + } + } + } + if payload.read_roots.is_empty() { log_line(log, "no read roots to grant; skipping read ACL helper")?; } else { diff --git a/codex-rs/windows-sandbox-rs/src/setup_provisioning/acl_tests.rs b/codex-rs/windows-sandbox-rs/src/setup_provisioning/acl_tests.rs index 3e2120eeae..1b08a52f91 100644 --- a/codex-rs/windows-sandbox-rs/src/setup_provisioning/acl_tests.rs +++ b/codex-rs/windows-sandbox-rs/src/setup_provisioning/acl_tests.rs @@ -90,6 +90,7 @@ fn lock_sandbox_dir_blocks_inherited_write_for_runner_files() { allow_local_binding: false, otel: None, real_user, + user_profile: None, mode: setup_mode, runtime: SetupRuntime::Legacy, refresh_only: false, diff --git a/codex-rs/windows-sandbox-rs/src/setup_provisioning/service.rs b/codex-rs/windows-sandbox-rs/src/setup_provisioning/service.rs index 013c1a4f20..f0c5ddcdf1 100644 --- a/codex-rs/windows-sandbox-rs/src/setup_provisioning/service.rs +++ b/codex-rs/windows-sandbox-rs/src/setup_provisioning/service.rs @@ -34,6 +34,7 @@ pub fn provision_sandbox_in_process( allow_local_binding: settings.allow_local_binding, otel: codex_otel::global_statsig_metrics_settings(), real_user: real_user.to_string(), + user_profile: None, mode: SetupMode::ProvisionOnly, runtime, refresh_only: false,