github/codex
4
0 Fork 0
mirror of https://github.com/openai/codex.git synced 2026-11-13 06:45:07 +00:00
Code Issues Packages Projects Releases Wiki Activity

Protect Windows sandbox binaries from inherited write access (#42330)

Browse Source 
## Why

An inherited write ACE from a writable parent directory could leave sandbox
runner files in `.sandbox-bin` writable or deletable by the workspace capability
SID.

## What changed

- Mark the `.sandbox-bin` DACL as protected when applying its explicit ACL, while
  preserving inherited DACL behavior for the other persistent sandbox
  directories.
- Cover both full and provision-only setup, including runner files created before
  and after the directory is locked.

GitOrigin-RevId: fcda4df97e3f8d729438ccb3d42daa606076e0c4
This commit is contained in:
iceweasel-oai authored and copyberry committed 2026-09-02 19:03:32 +00:00
1 parent 0d502a4230
commit f59905647a
2 files changed
+115 -2
Download Patch File Download Diff File Expand all files Collapse all files

No files matched your search

codex-rs/windows-sandbox-rs/src/bin/setup_main/win.rs
+20 -2
View File
@@ -60,6 +60,7 @@ use windows_sys::Win32::Security::Authorization::TRUSTEE_W;
use windows_sys::Win32::Security::CONTAINER_INHERIT_ACE;
use windows_sys::Win32::Security::DACL_SECURITY_INFORMATION;
use windows_sys::Win32::Security::OBJECT_INHERIT_ACE;
use windows_sys::Win32::Security::PROTECTED_DACL_SECURITY_INFORMATION;
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;
@@ -117,6 +118,12 @@ enum SetupMode {
ReadAclsOnly,
}
#[derive(Clone, Copy)]
enum DaclInheritance {
Inherited,
Protected,
}
fn log_line(log: &mut dyn Write, msg: &str) -> Result<()> {
let ts = chrono::Utc::now().to_rfc3339();
writeln!(log, "[{ts}] {msg}").map_err(|err| {
@@ -295,6 +302,7 @@ fn read_mask_allows_or_log(
}
}
#[allow(clippy::too_many_arguments)]
fn lock_sandbox_dir(
dir: &Path,
real_user: &str,
@@ -302,6 +310,7 @@ fn lock_sandbox_dir(
sandbox_group_access_mode: i32,
sandbox_group_mask: u32,
real_user_mask: u32,
dacl_inheritance: DaclInheritance,
setup_mode: SetupMode,
) -> Result<()> {
// ProvisionOnly accepts another user's CODEX_HOME; keep its ACL mutation
@@ -373,12 +382,18 @@ fn lock_sandbox_dir(
"SetEntriesInAclW sandbox dir failed: {set}",
));
}
let security_information = match dacl_inheritance {
DaclInheritance::Inherited => DACL_SECURITY_INFORMATION,
DaclInheritance::Protected => {
DACL_SECURITY_INFORMATION | PROTECTED_DACL_SECURITY_INFORMATION
}
};
let (res, api) = match directory.as_ref() {
Some(directory) => (
SetSecurityInfo(
directory.as_raw_handle() as _,
SE_FILE_OBJECT,
DACL_SECURITY_INFORMATION,
security_information,
std::ptr::null_mut(),
std::ptr::null_mut(),
new_dacl,
@@ -392,7 +407,7 @@ fn lock_sandbox_dir(
SetNamedSecurityInfoW(
path_w.as_ptr() as *mut u16,
SE_FILE_OBJECT,
DACL_SECURITY_INFORMATION,
security_information,
std::ptr::null_mut(),
std::ptr::null_mut(),
new_dacl,
@@ -671,6 +686,7 @@ fn lock_persistent_sandbox_dirs(payload: &Payload, sandbox_group_sid: &[u8]) ->
GRANT_ACCESS,
FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE | DELETE,
FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE,
DaclInheritance::Inherited,
payload.mode,
)
.map_err(|err| {
@@ -689,6 +705,7 @@ fn lock_persistent_sandbox_dirs(payload: &Payload, sandbox_group_sid: &[u8]) ->
DENY_ACCESS,
FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE | DELETE,
FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE,
DaclInheritance::Inherited,
payload.mode,
)
.map_err(|err| {
@@ -715,6 +732,7 @@ fn lock_sandbox_bin_dir(payload: &Payload, sandbox_group_sid: &[u8]) -> Result<(
GRANT_ACCESS,
FILE_GENERIC_READ | FILE_GENERIC_EXECUTE,
FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE | DELETE,
DaclInheritance::Protected,
payload.mode,
)
.map_err(|err| {
codex-rs/windows-sandbox-rs/src/bin/setup_main/win_acl_tests.rs
+95
View File
@@ -1,11 +1,23 @@
use super::DELETE;
use super::DaclInheritance;
use super::FILE_GENERIC_EXECUTE;
use super::FILE_GENERIC_READ;
use super::FILE_GENERIC_WRITE;
use super::GRANT_ACCESS;
use super::Payload;
use super::SETUP_VERSION;
use super::SetupMode;
use super::convert_string_sid_to_sid;
use super::lock_sandbox_bin_dir;
use super::lock_sandbox_dir;
use super::resolve_sid;
use super::sid_bytes_to_psid;
use codex_windows_sandbox::ensure_allow_write_aces;
use codex_windows_sandbox::path_mask_allows;
use codex_windows_sandbox::workspace_write_cap_sid_for_root;
use std::fs;
use windows_sys::Win32::Foundation::HLOCAL;
use windows_sys::Win32::Foundation::LocalFree;
#[test]
fn provision_only_locks_plain_directory_via_handle() {
@@ -21,9 +33,92 @@ fn provision_only_locks_plain_directory_via_handle() {
GRANT_ACCESS,
FILE_GENERIC_READ | FILE_GENERIC_EXECUTE,
FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE | DELETE,
DaclInheritance::Protected,
SetupMode::ProvisionOnly,
)
.expect("lock sandbox bin via no-reparse handle");
assert!(sandbox_bin.is_dir());
}
#[test]
fn lock_sandbox_dir_blocks_inherited_write_for_runner_files() {
for setup_mode in [SetupMode::Full, SetupMode::ProvisionOnly] {
let temp = tempfile::tempdir().expect("tempdir");
let workspace = temp.path().join("workspace");
let sandbox_bin = workspace.join(".sandbox-bin");
fs::create_dir(&workspace).expect("create workspace");
let workspace_sid = workspace_write_cap_sid_for_root(temp.path(), temp.path(), temp.path())
.expect("workspace SID");
let workspace_psid =
unsafe { convert_string_sid_to_sid(&workspace_sid).expect("convert workspace SID") };
let sandbox_group_sid =
resolve_sid("Authenticated Users").expect("resolve sandbox group SID");
let sandbox_group_psid =
sid_bytes_to_psid(&sandbox_group_sid).expect("convert sandbox group SID");
unsafe { ensure_allow_write_aces(&workspace, &[workspace_psid]) }
.expect("grant inherited workspace write access");
fs::create_dir(&sandbox_bin).expect("create sandbox bin");
let existing_runner = sandbox_bin.join("existing-runner.exe");
fs::write(&existing_runner, b"existing").expect("create existing runner");
assert!(
path_mask_allows(
&existing_runner,
&[workspace_psid],
FILE_GENERIC_WRITE | DELETE,
/*require_all_bits*/ true,
)
.expect("check inherited runner write and delete access")
);
let real_user = std::env::var("USERNAME").unwrap_or_else(|_| "Administrators".to_string());
let payload = Payload {
version: SETUP_VERSION,
offline_username: String::new(),
online_username: String::new(),
codex_home: workspace.clone(),
command_cwd: workspace.clone(),
read_roots: Vec::new(),
write_roots: Vec::new(),
deny_read_paths: Vec::new(),
deny_write_paths: Vec::new(),
proxy_ports: Vec::new(),
allow_local_binding: false,
otel: None,
real_user,
mode: setup_mode,
refresh_only: false,
};
lock_sandbox_bin_dir(&payload, &sandbox_group_sid).expect("lock sandbox bin");
let new_runner = sandbox_bin.join("new-runner.exe");
fs::write(&new_runner, b"new").expect("create new runner");
for path in [&sandbox_bin, &existing_runner, &new_runner] {
assert!(
!path_mask_allows(
path,
&[workspace_psid],
FILE_GENERIC_WRITE | DELETE,
/*require_all_bits*/ false,
)
.expect("check protected path write and delete access")
);
assert!(
path_mask_allows(
path,
&[sandbox_group_psid],
FILE_GENERIC_READ | FILE_GENERIC_EXECUTE,
/*require_all_bits*/ true,
)
.expect("check protected path read and execute access")
);
}
unsafe {
LocalFree(workspace_psid as HLOCAL);
LocalFree(sandbox_group_psid as HLOCAL);
}
}
}