openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-08-23 13:09:46 +00:00
Code Issues Packages Projects Releases Wiki Activity

Ignore inherited ACEs when refreshing Windows write roots (#34392)

Browse Source 
## Why

An inherited `FILE_DELETE_CHILD` grant can make a write root look stale, but
`SET_ACCESS` cannot replace an ACE inherited from an ancestor. Treating that
grant as explicit causes unnecessary ACL refresh attempts that cannot converge.

## What changed

- Add explicit-ACE filtering to the Windows DACL permission checks.
- Refresh a write root only when `FILE_DELETE_CHILD` is present in an explicit
  allow ACE, while retaining effective-permission checks for required rights.
- Ignore inherited stale rights when deciding whether `SET_ACCESS` must repair
  an allow ACE.

## Testing

Add a Windows regression test covering a write root that inherits
`FILE_DELETE_CHILD`, verifying that repeated refresh checks leave its explicit
write ACE unchanged.

GitOrigin-RevId: d0df9429efcf299da3ff3c1bce92942684803293
(cherry picked from commit bd92b056dd)
This commit is contained in:
Felipe Coury
2026-07-20 18:23:01 +00:00
committed by Roy Han
parent a7caf1bd7a
commit 3f29c6799a
3 changed files with 116 additions and 8 deletions
Download Patch File Download Diff File Expand all files Collapse all files

64
codex-rs/windows-sandbox-rs/src/acl.rs
View File

@@ -49,6 +49,7 @@ use windows_sys::Win32::Storage::FileSystem::OPEN_EXISTING;
use windows_sys::Win32::Storage::FileSystem::READ_CONTROL;
const SE_KERNEL_OBJECT: u32 = 6;
const INHERIT_ONLY_ACE: u8 = 0x08;
const INHERITED_ACE: u8 = 0x10;
const ACCESS_ALLOWED_ACE_TYPE: u8 = 0;
const ACCESS_DENIED_ACE_TYPE: u8 = 1;
const GENERIC_READ_MASK: u32 = 0x8000_0000;
@@ -103,6 +104,28 @@ pub unsafe fn dacl_mask_allows(
psids: &[*mut c_void],
desired_mask: u32,
require_all_bits: bool,
) -> bool {
dacl_mask_allows_with_scope(
p_dacl,
psids,
desired_mask,
require_all_bits,
AceScope::Effective,
)
}
#[derive(Clone, Copy)]
enum AceScope {
Effective,
Explicit,
}
unsafe fn dacl_mask_allows_with_scope(
p_dacl: *mut ACL,
psids: &[*mut c_void],
desired_mask: u32,
require_all_bits: bool,
scope: AceScope,
) -> bool {
if p_dacl.is_null() {
return false;
@@ -135,6 +158,11 @@ pub unsafe fn dacl_mask_allows(
if (hdr.AceFlags & INHERIT_ONLY_ACE) != 0 {
continue;
}
// SET_ACCESS cannot replace an ACE inherited from an ancestor, so it cannot make
// an explicit-only repair converge when that inherited ACE contains stale rights.
if matches!(scope, AceScope::Explicit) && (hdr.AceFlags & INHERITED_ACE) != 0 {
continue;
}
let base = p_ace as usize;
let sid_ptr =
(base + std::mem::size_of::<ACE_HEADER>() + std::mem::size_of::<u32>()) as *mut c_void;
@@ -166,10 +194,41 @@ pub fn path_mask_allows(
psids: &[*mut c_void],
desired_mask: u32,
require_all_bits: bool,
) -> Result<bool> {
path_mask_allows_with_scope(
path,
psids,
desired_mask,
require_all_bits,
AceScope::Effective,
)
}
/// Returns whether an explicit allow ACE for one of the provided SIDs grants any bit in `desired_mask`.
pub fn path_mask_has_explicit_allow_ace(
path: &Path,
psids: &[*mut c_void],
desired_mask: u32,
) -> Result<bool> {
path_mask_allows_with_scope(
path,
psids,
desired_mask,
/*require_all_bits*/ false,
AceScope::Explicit,
)
}
fn path_mask_allows_with_scope(
path: &Path,
psids: &[*mut c_void],
desired_mask: u32,
require_all_bits: bool,
scope: AceScope,
) -> Result<bool> {
unsafe {
let (p_dacl, sd) = fetch_dacl_handle(path)?;
let has = dacl_mask_allows(p_dacl, psids, desired_mask, require_all_bits);
let has = dacl_mask_allows_with_scope(p_dacl, psids, desired_mask, require_all_bits, scope);
if !sd.is_null() {
LocalFree(sd as HLOCAL);
}
@@ -318,11 +377,12 @@ unsafe fn ensure_allow_mask_aces_with_inheritance_impl(
let mut entries: Vec<EXPLICIT_ACCESS_W> = Vec::new();
for sid in sids {
if dacl_mask_allows(p_dacl, &[*sid], allow_mask, /*require_all_bits*/ true)
&& !dacl_mask_allows(
&& !dacl_mask_allows_with_scope(
p_dacl,
&[*sid],
disallow_mask,
/*require_all_bits*/ false,
AceScope::Explicit,
)
{
continue;

58
codex-rs/windows-sandbox-rs/src/bin/setup_main/win.rs
View File

@@ -22,6 +22,7 @@ use codex_windows_sandbox::is_command_cwd_root;
use codex_windows_sandbox::log_note;
use codex_windows_sandbox::log_writer;
use codex_windows_sandbox::path_mask_allows;
use codex_windows_sandbox::path_mask_has_explicit_allow_ace;
use codex_windows_sandbox::sandbox_bin_dir;
use codex_windows_sandbox::sandbox_dir;
use codex_windows_sandbox::sandbox_secrets_dir;
@@ -169,12 +170,7 @@ fn write_root_needs_refresh(root: &Path, psid: *mut c_void) -> Result<bool> {
)? {
return Ok(true);
}
path_mask_allows(
root,
&[psid],
FILE_DELETE_CHILD,
/*require_all_bits*/ false,
)
path_mask_has_explicit_allow_ace(root, &[psid], FILE_DELETE_CHILD)
}
fn spawn_read_acl_helper(payload: &Payload, _log: &mut dyn Write) -> Result<()> {
@@ -1060,6 +1056,7 @@ mod tests {
use codex_windows_sandbox::ensure_allow_mask_aces;
use codex_windows_sandbox::ensure_allow_write_aces;
use codex_windows_sandbox::load_or_create_cap_sids;
use codex_windows_sandbox::path_mask_allows;
use codex_windows_sandbox::workspace_write_cap_sid_for_root;
use pretty_assertions::assert_eq;
use serde_json::json;
@@ -1144,6 +1141,55 @@ mod tests {
);
}
#[test]
fn write_root_refresh_ignores_inherited_delete_child_grant() {
let temp = tempfile::tempdir().expect("tempdir");
let codex_home = temp.path().join("codex-home");
let parent = temp.path().join("parent");
let workspace = parent.join("workspace");
fs::create_dir_all(&codex_home).expect("create codex home");
fs::create_dir_all(&workspace).expect("create workspace");
let sid = workspace_write_cap_sid_for_root(&codex_home, &workspace, &workspace)
.expect("workspace sid");
let psid = unsafe { convert_string_sid_to_sid(&sid).expect("convert workspace sid") };
let seeded_explicit =
unsafe { ensure_allow_mask_aces(&workspace, &[psid], WRITE_ROOT_ALLOW_MASK) }
.expect("seed explicit write ACE");
let seeded_parent = unsafe {
ensure_allow_mask_aces(&parent, &[psid], WRITE_ROOT_ALLOW_MASK | FILE_DELETE_CHILD)
}
.expect("seed inherited stale write ACE");
let has_inherited_delete_child = path_mask_allows(
&workspace,
&[psid],
FILE_DELETE_CHILD,
/*require_all_bits*/ false,
)
.expect("check inherited stale write ACE");
let needs_refresh =
write_root_needs_refresh(&workspace, psid).expect("check inherited stale write ACE");
let first_refresh = unsafe { ensure_allow_write_aces(&workspace, &[psid]) }
.expect("first inherited write ACE refresh");
let second_refresh = unsafe { ensure_allow_write_aces(&workspace, &[psid]) }
.expect("second inherited write ACE refresh");
unsafe {
LocalFree(psid as HLOCAL);
}
assert_eq!(
(
seeded_explicit,
seeded_parent,
has_inherited_delete_child,
needs_refresh,
first_refresh,
second_refresh,
),
(true, true, true, false, false, false)
);
}
#[test]
fn deny_path_under_active_root_uses_only_matching_root_sid() {
let temp = tempfile::tempdir().expect("tempdir");

2
codex-rs/windows-sandbox-rs/src/lib.rs
View File

@@ -148,6 +148,8 @@ pub use acl::fetch_dacl_handle;
#[cfg(target_os = "windows")]
pub use acl::path_mask_allows;
#[cfg(target_os = "windows")]
pub use acl::path_mask_has_explicit_allow_ace;
#[cfg(target_os = "windows")]
pub use audit::apply_world_writable_scan_and_denies_for_permissions;
#[cfg(target_os = "windows")]
pub use cap::load_or_create_cap_sids;