openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-11-12 21:20:34 +00:00
Code Issues Packages Projects Releases Wiki Activity

Harden Windows sandbox provisioning against reparse points (#39083)

Browse Source 
## Why

Elevated provisioning can apply ACLs beneath a user-supplied `CODEX_HOME`.
Following a directory junction or another reparse point could apply those ACLs
to a different directory than the one provisioning intended to secure.

## What changed

- Require provisioning paths to be absolute paths on a local Windows disk.
- In provision-only mode, open or create sandbox directories with
  `OBJ_DONT_REPARSE` and keep the resulting handle open while applying ACLs
  with `SetSecurityInfo`.
- Reject reparse points in both the final directory and its ancestors while
  preserving the existing path-based behavior for full setup.

## Testing

Add Windows tests covering plain directory creation and rejection of junctions
at both the leaf and ancestor levels.

GitOrigin-RevId: d165b8b3685c89bc171e4d58c63c739fa9fb2cf9
This commit is contained in:
iceweasel-oai
2026-08-17 21:10:58 +00:00
committed by copyberry
parent 34e4823a1d
commit a4f37a5b7f
4 changed files with 287 additions and 32 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/windows-sandbox-rs/src/bin/setup_main/win.rs
+53 -32
View File
@@ -1,4 +1,5 @@
mod firewall;
mod no_reparse_dir;
mod read_acl_mutex;
use anyhow::Context;
@@ -36,6 +37,7 @@ use std::collections::HashSet;
use std::ffi::OsStr;
use std::ffi::c_void;
use std::io::Write;
use std::os::windows::io::AsRawHandle;
use std::os::windows::process::CommandExt;
use std::path::Path;
use std::path::PathBuf;
@@ -52,6 +54,7 @@ use windows_sys::Win32::Security::Authorization::GRANT_ACCESS;
use windows_sys::Win32::Security::Authorization::SE_FILE_OBJECT;
use windows_sys::Win32::Security::Authorization::SetEntriesInAclW;
use windows_sys::Win32::Security::Authorization::SetNamedSecurityInfoW;
use windows_sys::Win32::Security::Authorization::SetSecurityInfo;
use windows_sys::Win32::Security::Authorization::TRUSTEE_IS_SID;
use windows_sys::Win32::Security::Authorization::TRUSTEE_W;
use windows_sys::Win32::Security::CONTAINER_INHERIT_ACE;
@@ -69,6 +72,7 @@ const WRITE_ROOT_ALLOW_MASK: u32 =
mod sandbox_users;
mod setup_runtime_bin;
use no_reparse_dir::open_or_create_no_reparse;
use read_acl_mutex::acquire_read_acl_mutex;
use read_acl_mutex::read_acl_mutex_exists;
use sandbox_users::commit_setup_marker;
@@ -297,9 +301,17 @@ fn lock_sandbox_dir(
sandbox_group_access_mode: i32,
sandbox_group_mask: u32,
real_user_mask: u32,
_log: &mut dyn Write,
setup_mode: SetupMode,
) -> Result<()> {
std::fs::create_dir_all(dir)?;
// ProvisionOnly accepts another user's CODEX_HOME; keep its ACL mutation
// bound to a no-reparse handle without changing full setup behavior.
let directory = match setup_mode {
SetupMode::Full | SetupMode::ReadAclsOnly => {
std::fs::create_dir_all(dir)?;
None
}
SetupMode::ProvisionOnly => Some(open_or_create_no_reparse(dir)?),
};
let system_sid = resolve_sid("SYSTEM")?;
let admins_sid = resolve_sid("Administrators")?;
let real_sid = resolve_sid(real_user)?;
@@ -360,20 +372,37 @@ fn lock_sandbox_dir(
"SetEntriesInAclW sandbox dir failed: {set}",
));
}
let path_w = to_wide(dir.as_os_str());
let res = SetNamedSecurityInfoW(
path_w.as_ptr() as *mut u16,
SE_FILE_OBJECT,
DACL_SECURITY_INFORMATION,
std::ptr::null_mut(),
std::ptr::null_mut(),
new_dacl,
std::ptr::null_mut(),
);
let (res, api) = match directory.as_ref() {
Some(directory) => (
SetSecurityInfo(
directory.as_raw_handle() as _,
SE_FILE_OBJECT,
DACL_SECURITY_INFORMATION,
std::ptr::null_mut(),
std::ptr::null_mut(),
new_dacl,
std::ptr::null_mut(),
),
"SetSecurityInfo",
),
None => {
let path_w = to_wide(dir.as_os_str());
(
SetNamedSecurityInfoW(
path_w.as_ptr() as *mut u16,
SE_FILE_OBJECT,
DACL_SECURITY_INFORMATION,
std::ptr::null_mut(),
std::ptr::null_mut(),
new_dacl,
std::ptr::null_mut(),
),
"SetNamedSecurityInfoW",
)
}
};
if res != 0 {
return Err(anyhow::anyhow!(
"SetNamedSecurityInfoW sandbox dir failed: {res}",
));
return Err(anyhow::anyhow!("{api} sandbox dir failed: {res}"));
}
if !new_dacl.is_null() {
LocalFree(new_dacl as HLOCAL);
@@ -631,11 +660,7 @@ fn configure_offline_sandbox_network(
Ok(())
}
fn lock_persistent_sandbox_dirs(
payload: &Payload,
sandbox_group_sid: &[u8],
log: &mut dyn Write,
) -> Result<()> {
fn lock_persistent_sandbox_dirs(payload: &Payload, sandbox_group_sid: &[u8]) -> Result<()> {
lock_sandbox_dir(
&sandbox_dir(&payload.codex_home),
&payload.real_user,
@@ -643,7 +668,7 @@ fn lock_persistent_sandbox_dirs(
GRANT_ACCESS,
FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE | DELETE,
FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE,
log,
payload.mode,
)
.map_err(|err| {
anyhow::Error::new(SetupFailure::new(
@@ -661,7 +686,7 @@ fn lock_persistent_sandbox_dirs(
DENY_ACCESS,
FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE | DELETE,
FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE,
log,
payload.mode,
)
.map_err(|err| {
anyhow::Error::new(SetupFailure::new(
@@ -679,11 +704,7 @@ fn lock_persistent_sandbox_dirs(
Ok(())
}
fn lock_sandbox_bin_dir(
payload: &Payload,
sandbox_group_sid: &[u8],
log: &mut dyn Write,
) -> Result<()> {
fn lock_sandbox_bin_dir(payload: &Payload, sandbox_group_sid: &[u8]) -> Result<()> {
lock_sandbox_dir(
&sandbox_bin_dir(&payload.codex_home),
&payload.real_user,
@@ -691,7 +712,7 @@ fn lock_sandbox_bin_dir(
GRANT_ACCESS,
FILE_GENERIC_READ | FILE_GENERIC_EXECUTE,
FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE | DELETE,
log,
payload.mode,
)
.map_err(|err| {
anyhow::Error::new(SetupFailure::new(
@@ -726,8 +747,8 @@ fn run_provision_only(payload: &Payload, log: &mut dyn Write, sbx_dir: &Path) ->
configure_offline_sandbox_network(payload, &offline_sid_str, log)?;
lock_sandbox_bin_dir(payload, &sandbox_group_sid, log)?;
lock_persistent_sandbox_dirs(payload, &sandbox_group_sid, log)?;
lock_sandbox_bin_dir(payload, &sandbox_group_sid)?;
lock_persistent_sandbox_dirs(payload, &sandbox_group_sid)?;
log_note("setup provisioning binary completed", Some(sbx_dir));
Ok(())
}
@@ -982,7 +1003,7 @@ fn run_setup_full(payload: &Payload, log: &mut dyn Write, sbx_dir: &Path) -> Res
}
}
lock_sandbox_bin_dir(payload, &sandbox_group_sid, log)?;
lock_sandbox_bin_dir(payload, &sandbox_group_sid)?;
if refresh_only {
log_line(
@@ -995,7 +1016,7 @@ fn run_setup_full(payload: &Payload, log: &mut dyn Write, sbx_dir: &Path) -> Res
)?;
}
if !refresh_only {
lock_persistent_sandbox_dirs(payload, &sandbox_group_sid, log)?;
lock_persistent_sandbox_dirs(payload, &sandbox_group_sid)?;
}
unsafe {
codex-rs/windows-sandbox-rs/src/bin/setup_main/win/no_reparse_dir.rs
+152
View File
@@ -0,0 +1,152 @@
use anyhow::Context;
use anyhow::Result;
use anyhow::bail;
use anyhow::ensure;
use std::ffi::OsStr;
use std::ffi::c_void;
use std::mem::size_of;
use std::os::windows::ffi::OsStrExt;
use std::os::windows::io::FromRawHandle;
use std::os::windows::io::OwnedHandle;
use std::path::Component;
use std::path::Path;
use std::path::Prefix;
use std::ptr;
use windows_sys::Win32::Foundation::HANDLE;
use windows_sys::Win32::Foundation::INVALID_HANDLE_VALUE;
use windows_sys::Win32::Foundation::NTSTATUS;
use windows_sys::Win32::Foundation::RtlNtStatusToDosError;
use windows_sys::Win32::Foundation::UNICODE_STRING;
use windows_sys::Win32::Storage::FileSystem::FILE_ATTRIBUTE_NORMAL;
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;
use windows_sys::Win32::Storage::FileSystem::WRITE_DAC;
use windows_sys::Win32::System::IO::IO_STATUS_BLOCK;
use windows_sys::Win32::System::IO::IO_STATUS_BLOCK_0;
use windows_sys::Win32::System::Kernel::OBJ_CASE_INSENSITIVE;
use windows_sys::Win32::System::Kernel::OBJ_DONT_REPARSE;
const FILE_OPEN_IF: u32 = 3;
const FILE_DIRECTORY_FILE: u32 = 1;
const STATUS_REPARSE_POINT_ENCOUNTERED: NTSTATUS = 0xC000_050B_u32 as i32;
#[repr(C)]
struct ObjectAttributes {
length: u32,
root_directory: HANDLE,
object_name: *const UNICODE_STRING,
attributes: u32,
security_descriptor: *const c_void,
security_quality_of_service: *const c_void,
}
#[link(name = "ntdll")]
unsafe extern "system" {
fn NtCreateFile(
file_handle: *mut HANDLE,
desired_access: u32,
object_attributes: *const ObjectAttributes,
io_status_block: *mut IO_STATUS_BLOCK,
allocation_size: *const i64,
file_attributes: u32,
share_access: u32,
create_disposition: u32,
create_options: u32,
ea_buffer: *const c_void,
ea_length: u32,
) -> NTSTATUS;
}
/// Opens or creates the final directory without following a reparse point in
/// any path component.
///
/// Parent directories must already exist; only the final directory is created.
///
/// The returned handle must remain open through any security mutation so the
/// mutation stays bound to the directory that passed this validation.
pub(super) fn open_or_create_no_reparse(path: &Path) -> Result<OwnedHandle> {
ensure!(
path.is_absolute(),
"sandbox ACL path must be absolute: {}",
path.display()
);
let source_offset = match path.components().next() {
Some(Component::Prefix(prefix)) if matches!(prefix.kind(), Prefix::Disk(_)) => 0,
Some(Component::Prefix(prefix)) if matches!(prefix.kind(), Prefix::VerbatimDisk(_)) => 4,
_ => bail!(
"sandbox ACL path must have a local disk prefix: {}",
path.display()
),
};
// NtCreateFile expects an NT namespace path. Convert only the local-disk
// spellings accepted by provisioning, without canonicalizing and following
// the reparse points that OBJ_DONT_REPARSE must reject.
let source: Vec<u16> = path.as_os_str().encode_wide().collect();
let mut nt_path: Vec<u16> = OsStr::new("\\??\\").encode_wide().collect();
for unit in source.into_iter().skip(source_offset) {
nt_path.push(if unit == b'/' as u16 {
b'\\' as u16
} else {
unit
});
}
nt_path.push(0);
let name_length = u16::try_from((nt_path.len() - 1) * size_of::<u16>())
.context("sandbox ACL path is too long")?;
let maximum_length =
u16::try_from(nt_path.len() * size_of::<u16>()).context("sandbox ACL path is too long")?;
let object_name = UNICODE_STRING {
Length: name_length,
MaximumLength: maximum_length,
Buffer: nt_path.as_mut_ptr(),
};
let object_attributes = ObjectAttributes {
length: size_of::<ObjectAttributes>() as u32,
root_directory: 0,
object_name: &object_name,
attributes: OBJ_CASE_INSENSITIVE as u32 | OBJ_DONT_REPARSE as u32,
security_descriptor: ptr::null(),
security_quality_of_service: ptr::null(),
};
let mut io_status_block = IO_STATUS_BLOCK {
Anonymous: IO_STATUS_BLOCK_0 { Status: 0 },
Information: 0,
};
let mut handle = 0;
let status = unsafe {
NtCreateFile(
&mut handle,
WRITE_DAC,
&object_attributes,
&mut io_status_block,
ptr::null(),
FILE_ATTRIBUTE_NORMAL,
FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
FILE_OPEN_IF,
FILE_DIRECTORY_FILE,
ptr::null(),
/*ea_length*/ 0,
)
};
if status < 0 {
if status == STATUS_REPARSE_POINT_ENCOUNTERED {
bail!(
"sandbox ACL path contains a reparse point: {}",
path.display()
);
}
let error = unsafe { RtlNtStatusToDosError(status) };
return Err(std::io::Error::from_raw_os_error(error as i32))
.with_context(|| format!("open sandbox ACL directory {}", path.display()));
}
ensure!(
handle != 0 && handle != INVALID_HANDLE_VALUE,
"NtCreateFile returned an invalid sandbox ACL directory handle"
);
Ok(unsafe { OwnedHandle::from_raw_handle(handle as *mut c_void) })
}
#[cfg(test)]
#[path = "no_reparse_dir_tests.rs"]
mod tests;
codex-rs/windows-sandbox-rs/src/bin/setup_main/win/no_reparse_dir_tests.rs
+64
View File
@@ -0,0 +1,64 @@
use std::fs;
use std::path::Path;
use std::process::Command;
use anyhow::Result;
use super::open_or_create_no_reparse;
fn create_directory_junction(target: &Path, alias: &Path) -> Result<()> {
let output = Command::new("cmd")
.args(["/C", "mklink", "/J"])
.arg(alias)
.arg(target)
.output()?;
if !output.status.success() {
anyhow::bail!(
"mklink /J failed: stdout={} stderr={}",
String::from_utf8_lossy(&output.stdout).trim(),
String::from_utf8_lossy(&output.stderr).trim()
);
}
Ok(())
}
#[test]
fn creates_and_opens_plain_directory_leaf() -> Result<()> {
let temporary = tempfile::tempdir()?;
let directory = temporary.path().join(".sandbox-bin");
drop(open_or_create_no_reparse(&directory)?);
let _handle = open_or_create_no_reparse(&directory)?;
assert!(directory.is_dir());
Ok(())
}
#[test]
fn rejects_final_directory_junction() -> Result<()> {
let temporary = tempfile::tempdir()?;
let target = temporary.path().join("target");
let alias = temporary.path().join(".sandbox-bin");
fs::create_dir(&target)?;
create_directory_junction(&target, &alias)?;
let _ =
open_or_create_no_reparse(&alias).expect_err("final directory junction must be rejected");
fs::remove_dir(&alias)?;
Ok(())
}
#[test]
fn rejects_ancestor_directory_junction() -> Result<()> {
let temporary = tempfile::tempdir()?;
let target_home = temporary.path().join("target-home");
let alias_home = temporary.path().join("linked-home");
fs::create_dir(&target_home)?;
fs::create_dir(target_home.join(".sandbox-bin"))?;
create_directory_junction(&target_home, &alias_home)?;
let _ = open_or_create_no_reparse(&alias_home.join(".sandbox-bin"))
.expect_err("ancestor directory junction must be rejected");
fs::remove_dir(&alias_home)?;
Ok(())
}
codex-rs/windows-sandbox-rs/src/setup.rs
+18
View File
@@ -1078,6 +1078,24 @@ pub fn run_elevated_provisioning_setup(
real_user: &str,
settings: crate::WindowsSandboxProvisioningSettings,
) -> Result<()> {
if !codex_home.is_absolute()
|| !matches!(
codex_home.components().next(),
Some(std::path::Component::Prefix(prefix))
if matches!(
prefix.kind(),
std::path::Prefix::Disk(_) | std::path::Prefix::VerbatimDisk(_)
)
)
{
return Err(failure(
SetupErrorCode::OrchestratorSandboxDirCreateFailed,
format!(
"sandbox provisioning CODEX_HOME must be an absolute local disk path: {}",
codex_home.display()
),
));
}
let sbx_dir = sandbox_dir(codex_home);
std::fs::create_dir_all(&sbx_dir).map_err(|err| {
failure(