github/codex
4
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-10-20 00:18:49 +00:00
Code Issues Packages Projects Releases Wiki Activity

Preserve PowerShell relative paths in the elevated Windows sandbox (#49690)

Browse Source 
## 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
This commit is contained in:
johnl-oai
2026-09-30 17:50:58 +00:00
committed by copyberry
parent 0a76a2520f
commit 7e8878f605
7 changed files with 344 additions and 11 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/core/tests/suite/windows_sandbox.rs
+128
View File
@@ -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<()> {
codex-rs/windows-sandbox-rs/src/acl.rs
+73 -11
View File
@@ -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<bool> {
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<EXPLICIT_ACCESS_W> = 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`.
codex-rs/windows-sandbox-rs/src/acl_tests.rs
+85
View File
@@ -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<u8>) {
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::<ACL_SIZE_INFORMATION>() as u32,
AclSizeInformation,
),
0
);
let dacl =
std::slice::from_raw_parts(acl.cast::<u8>(), 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);
}
codex-rs/windows-sandbox-rs/src/setup.rs
+9
View File
@@ -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<codex_otel::StatsigMetricsSettings>,
real_user: String,
#[serde(skip_serializing_if = "Option::is_none")]
user_profile: Option<PathBuf>,
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,
codex-rs/windows-sandbox-rs/src/setup_provisioning.rs
+47
View File
@@ -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<StatsigMetricsSettings>,
real_user: String,
#[serde(default)]
user_profile: Option<PathBuf>,
#[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 {
codex-rs/windows-sandbox-rs/src/setup_provisioning/acl_tests.rs
+1
View File
@@ -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,
codex-rs/windows-sandbox-rs/src/setup_provisioning/service.rs
+1
View File
@@ -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,