clone/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-05-24 04:54:52 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(windows-sandbox): tighten deny-read acl handling

Browse Source 
Co-authored-by: Codex noreply@openai.com
This commit is contained in:
viyatb-oai
2026-05-08 20:55:44 -07:00
parent 38fbfc2fc6
commit 4a886a008b
6 changed files with 121 additions and 266 deletions
Download Patch File Download Diff File Expand all files Collapse all files

70
codex-rs/protocol/src/permissions.rs
View File

@@ -232,8 +232,39 @@ impl ReadDenyMatcher {
/// can skip read-deny checks without allocating matcher state. The `cwd`
/// resolves cwd-relative policy paths and special paths before matching.
pub fn new(file_system_sandbox_policy: &FileSystemSandboxPolicy, cwd: &Path) -> Option<Self> {
match Self::build(
file_system_sandbox_policy,
cwd,
InvalidDenyReadGlobBehavior::FailClosed,
) {
Ok(matcher) => matcher,
Err(_) => unreachable!("fail-closed glob handling does not return errors"),
}
}
/// Builds a matcher for callers that must reject malformed glob patterns.
///
/// Runtime read checks intentionally fail closed on malformed deny patterns.
/// Host-side expansion work should use this constructor instead so a typo
/// cannot broaden the set of paths it mutates before execution starts.
pub fn try_new(
file_system_sandbox_policy: &FileSystemSandboxPolicy,
cwd: &Path,
) -> Result<Option<Self>, String> {
Self::build(
file_system_sandbox_policy,
cwd,
InvalidDenyReadGlobBehavior::ReturnError,
)
}
fn build(
file_system_sandbox_policy: &FileSystemSandboxPolicy,
cwd: &Path,
invalid_glob_behavior: InvalidDenyReadGlobBehavior,
) -> Result<Option<Self>, String> {
if !file_system_sandbox_policy.has_denied_read_restrictions() {
return None;
return Ok(None);
}
// Exact roots are stored as all meaningful path spellings we can derive
@@ -247,22 +278,23 @@ impl ReadDenyMatcher {
// Pattern entries stay as policy-level globs. They are matched at read
// time here instead of being snapshotted to startup filesystem state.
let mut invalid_pattern = false;
let deny_read_matchers = file_system_sandbox_policy
.get_unreadable_globs_with_cwd(cwd)
.into_iter()
.filter_map(|pattern| match build_glob_matcher(&pattern) {
Some(matcher) => Some(matcher),
None => {
invalid_pattern = true;
None
}
})
.collect();
Some(Self {
let mut deny_read_matchers = Vec::new();
for pattern in file_system_sandbox_policy.get_unreadable_globs_with_cwd(cwd) {
match build_glob_matcher(&pattern) {
Ok(matcher) => deny_read_matchers.push(matcher),
Err(err) => match invalid_glob_behavior {
InvalidDenyReadGlobBehavior::FailClosed => invalid_pattern = true,
InvalidDenyReadGlobBehavior::ReturnError => {
return Err(format!("invalid deny-read glob pattern `{pattern}`: {err}"));
}
},
}
}
Ok(Some(Self {
denied_candidates,
deny_read_matchers,
invalid_pattern,
})
}))
}
/// Returns whether `path` is denied by the policy used to build this matcher.
@@ -295,6 +327,12 @@ impl ReadDenyMatcher {
}
}
#[derive(Clone, Copy)]
enum InvalidDenyReadGlobBehavior {
FailClosed,
ReturnError,
}
#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize, JsonSchema, TS)]
#[serde(tag = "type", rename_all = "snake_case")]
#[ts(tag = "type")]
@@ -1291,15 +1329,15 @@ fn push_unique(candidates: &mut Vec<PathBuf>, candidate: PathBuf) {
}
}
fn build_glob_matcher(pattern: &str) -> Option<GlobMatcher> {
fn build_glob_matcher(pattern: &str) -> Result<GlobMatcher, String> {
// Keep `*` and `?` within a single path component and preserve an unclosed
// `[` as a literal so matcher behavior stays aligned with config parsing.
GlobBuilder::new(pattern)
.literal_separator(true)
.allow_unclosed_class(true)
.build()
.ok()
.map(|glob| glob.compile_matcher())
.map_err(|err| err.to_string())
}
fn resolve_file_system_special_path(

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

@@ -487,6 +487,41 @@ pub unsafe fn add_allow_ace(path: &Path, psid: *mut c_void) -> Result<bool> {
/// # Safety
/// Caller must ensure `psid` points to a valid SID and `path` refers to an existing file or directory.
pub unsafe fn add_deny_write_ace(path: &Path, psid: *mut c_void) -> Result<bool> {
add_deny_ace(path, psid, DenyAceKind::Write)
}
#[derive(Clone, Copy)]
enum DenyAceKind {
Read,
Write,
}
impl DenyAceKind {
fn mask(self) -> u32 {
match self {
Self::Read => FILE_GENERIC_READ | GENERIC_READ_MASK,
Self::Write => {
FILE_GENERIC_WRITE
| FILE_WRITE_DATA
| FILE_APPEND_DATA
| FILE_WRITE_EA
| FILE_WRITE_ATTRIBUTES
| GENERIC_WRITE_MASK
| DELETE
| FILE_DELETE_CHILD
}
}
}
unsafe fn already_present(self, p_dacl: *mut ACL, psid: *mut c_void) -> bool {
match self {
Self::Read => dacl_has_read_deny_for_sid(p_dacl, psid),
Self::Write => dacl_has_write_deny_for_sid(p_dacl, psid),
}
}
}
unsafe fn add_deny_ace(path: &Path, psid: *mut c_void, kind: DenyAceKind) -> Result<bool> {
let mut p_sd: *mut c_void = std::ptr::null_mut();
let mut p_dacl: *mut ACL = std::ptr::null_mut();
let code = GetNamedSecurityInfoW(
@@ -503,7 +538,7 @@ pub unsafe fn add_deny_write_ace(path: &Path, psid: *mut c_void) -> Result<bool>
return Err(anyhow!("GetNamedSecurityInfoW failed: {code}"));
}
let mut added = false;
if !dacl_has_write_deny_for_sid(p_dacl, psid) {
if !kind.already_present(p_dacl, psid) {
let trustee = TRUSTEE_W {
pMultipleTrustee: std::ptr::null_mut(),
MultipleTrusteeOperation: 0,
@@ -512,14 +547,7 @@ pub unsafe fn add_deny_write_ace(path: &Path, psid: *mut c_void) -> Result<bool>
ptstrName: psid as *mut u16,
};
let mut explicit: EXPLICIT_ACCESS_W = std::mem::zeroed();
explicit.grfAccessPermissions = FILE_GENERIC_WRITE
| FILE_WRITE_DATA
| FILE_APPEND_DATA
| FILE_WRITE_EA
| FILE_WRITE_ATTRIBUTES
| GENERIC_WRITE_MASK
| DELETE
| FILE_DELETE_CHILD;
explicit.grfAccessPermissions = kind.mask();
explicit.grfAccessMode = DENY_ACCESS;
explicit.grfInheritance = CONTAINER_INHERIT_ACE | OBJECT_INHERIT_ACE;
explicit.Trustee = trustee;
@@ -559,73 +587,7 @@ pub unsafe fn add_deny_write_ace(path: &Path, psid: *mut c_void) -> Result<bool>
/// # Safety
/// Caller must ensure `psid` points to a valid SID and `path` refers to an existing file or directory.
pub unsafe fn add_deny_read_ace(path: &Path, psid: *mut c_void) -> Result<bool> {
let mut p_sd: *mut c_void = std::ptr::null_mut();
let mut p_dacl: *mut ACL = std::ptr::null_mut();
let code = GetNamedSecurityInfoW(
to_wide(path).as_ptr(),
1,
DACL_SECURITY_INFORMATION,
std::ptr::null_mut(),
std::ptr::null_mut(),
&mut p_dacl,
std::ptr::null_mut(),
&mut p_sd,
);
if code != ERROR_SUCCESS {
return Err(anyhow!("GetNamedSecurityInfoW failed: {code}"));
}
let mut added = false;
if !dacl_has_read_deny_for_sid(p_dacl, psid) {
let trustee = TRUSTEE_W {
pMultipleTrustee: std::ptr::null_mut(),
MultipleTrusteeOperation: 0,
TrusteeForm: TRUSTEE_IS_SID,
TrusteeType: TRUSTEE_IS_UNKNOWN,
ptstrName: psid as *mut u16,
};
let mut explicit: EXPLICIT_ACCESS_W = std::mem::zeroed();
explicit.grfAccessPermissions = FILE_GENERIC_READ | GENERIC_READ_MASK;
explicit.grfAccessMode = DENY_ACCESS;
explicit.grfInheritance = CONTAINER_INHERIT_ACE | OBJECT_INHERIT_ACE;
explicit.Trustee = trustee;
let mut p_new_dacl: *mut ACL = std::ptr::null_mut();
let code2 = SetEntriesInAclW(1, &explicit, p_dacl, &mut p_new_dacl);
if code2 != ERROR_SUCCESS {
if !p_sd.is_null() {
LocalFree(p_sd as HLOCAL);
}
if !p_new_dacl.is_null() {
LocalFree(p_new_dacl as HLOCAL);
}
return Err(anyhow!("SetEntriesInAclW failed: {code2}"));
}
let code3 = SetNamedSecurityInfoW(
to_wide(path).as_ptr() as *mut u16,
1,
DACL_SECURITY_INFORMATION,
std::ptr::null_mut(),
std::ptr::null_mut(),
p_new_dacl,
std::ptr::null_mut(),
);
if code3 != ERROR_SUCCESS {
if !p_sd.is_null() {
LocalFree(p_sd as HLOCAL);
}
if !p_new_dacl.is_null() {
LocalFree(p_new_dacl as HLOCAL);
}
return Err(anyhow!("SetNamedSecurityInfoW failed: {code3}"));
}
added = true;
if !p_new_dacl.is_null() {
LocalFree(p_new_dacl as HLOCAL);
}
}
if !p_sd.is_null() {
LocalFree(p_sd as HLOCAL);
}
Ok(added)
add_deny_ace(path, psid, DenyAceKind::Read)
}
pub unsafe fn revoke_ace(path: &Path, psid: *mut c_void) {

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

@@ -6,7 +6,6 @@ use anyhow::Result;
use base64::Engine;
use base64::engine::general_purpose::STANDARD as BASE64;
use codex_otel::StatsigMetricsSettings;
use codex_windows_sandbox::DenyReadAclRecordKind;
use codex_windows_sandbox::LOG_FILE_NAME;
use codex_windows_sandbox::SETUP_VERSION;
use codex_windows_sandbox::SetupErrorCode;
@@ -15,7 +14,6 @@ use codex_windows_sandbox::SetupFailure;
use codex_windows_sandbox::add_deny_write_ace;
use codex_windows_sandbox::apply_deny_read_acls;
use codex_windows_sandbox::canonicalize_path;
use codex_windows_sandbox::cleanup_stale_persistent_deny_read_acls;
use codex_windows_sandbox::convert_string_sid_to_sid;
use codex_windows_sandbox::ensure_allow_mask_aces_with_inheritance;
use codex_windows_sandbox::ensure_allow_write_aces;
@@ -32,7 +30,6 @@ use codex_windows_sandbox::sandbox_secrets_dir;
use codex_windows_sandbox::string_from_sid_bytes;
use codex_windows_sandbox::to_wide;
use codex_windows_sandbox::workspace_cap_sid_for_cwd;
use codex_windows_sandbox::write_persistent_deny_read_acl_record;
use codex_windows_sandbox::write_setup_error_report;
use serde::Deserialize;
use serde::Serialize;
@@ -466,29 +463,6 @@ fn run_read_acl_only(payload: &Payload, log: &mut File) -> Result<()> {
let sandbox_group_sid = resolve_sandbox_users_group_sid()?;
let sandbox_group_psid = sid_bytes_to_psid(&sandbox_group_sid)?;
let mut refresh_errors: Vec<String> = Vec::new();
// Stale cleanup must happen before the helper re-grants read ACLs because
// the cleanup primitive revokes all ACEs for the sandbox group SID.
match unsafe {
cleanup_stale_persistent_deny_read_acls(
&payload.codex_home,
DenyReadAclRecordKind::SandboxGroup,
&payload.deny_read_paths,
sandbox_group_psid,
)
} {
Ok(cleaned) => {
if !cleaned.is_empty() {
log_line(
log,
&format!("cleaned {} stale deny-read ACLs", cleaned.len()),
)?;
}
}
Err(err) => {
refresh_errors.push(format!("cleanup stale deny-read ACLs failed: {err}"));
log_line(log, &format!("cleanup stale deny-read ACLs failed: {err}"))?;
}
}
if !payload.read_roots.is_empty() {
let users_sid = resolve_sid("Users")?;
let users_psid = sid_bytes_to_psid(&users_sid)?;
@@ -526,11 +500,6 @@ fn run_read_acl_only(payload: &Payload, log: &mut File) -> Result<()> {
// explicit deny entries that take precedence over the broad read allowlist.
match unsafe { apply_deny_read_acls(&payload.deny_read_paths, sandbox_group_psid) } {
Ok(applied) => {
write_persistent_deny_read_acl_record(
&payload.codex_home,
DenyReadAclRecordKind::SandboxGroup,
&applied,
)?;
if !applied.is_empty() {
log_line(log, &format!("applied {} deny-read ACLs", applied.len()))?;
}
@@ -664,9 +633,8 @@ fn run_setup_full(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<(
);
}
// The read ACL helper is also responsible for persistent deny-read cleanup,
// so it must run whenever deny-read paths are present even if no new read
// roots need to be granted.
// The read ACL helper also applies deny-read ACEs, so it must run whenever
// deny-read paths are present even if no new read roots need to be granted.
if payload.read_roots.is_empty() && payload.deny_read_paths.is_empty() {
log_line(
log,

107
codex-rs/windows-sandbox-rs/src/deny_read_acl.rs
View File

@@ -1,46 +1,13 @@
use crate::acl::add_deny_read_ace;
use crate::acl::revoke_ace;
use crate::path_normalization::canonicalize_path;
use crate::setup::sandbox_dir;
use anyhow::Context;
use anyhow::Result;
use serde::Deserialize;
use serde::Serialize;
use std::collections::HashSet;
use std::ffi::c_void;
use std::path::Path;
use std::path::PathBuf;
/// Identifies which Windows sandbox principal owns a persistent deny-read ACL.
///
/// The elevated backend applies deny ACEs to the shared sandbox users group,
/// while the restricted-token backend applies them to capability SIDs. Keeping
/// separate records prevents stale cleanup for one backend from revoking entries
/// that are still owned by the other backend.
#[derive(Debug, Clone, Copy)]
pub enum DenyReadAclRecordKind {
SandboxGroup,
RestrictedToken,
}
impl DenyReadAclRecordKind {
fn file_name(self) -> &'static str {
match self {
Self::SandboxGroup => "deny_read_acls_sandbox_group.json",
Self::RestrictedToken => "deny_read_acls_restricted_token.json",
}
}
}
#[derive(Debug, Default, Deserialize, Serialize)]
struct DenyReadAclRecord {
paths: Vec<PathBuf>,
}
pub fn deny_read_acl_record_path(codex_home: &Path, kind: DenyReadAclRecordKind) -> PathBuf {
sandbox_dir(codex_home).join(kind.file_name())
}
/// Build the exact ACL paths that should receive a deny-read ACE.
///
/// We keep both the lexical policy path and, when it already exists, the
@@ -73,80 +40,6 @@ fn lexical_path_key(path: &Path) -> String {
.to_ascii_lowercase()
}
fn read_record(path: &Path) -> Result<DenyReadAclRecord> {
match std::fs::read_to_string(path) {
Ok(contents) => serde_json::from_str(&contents)
.with_context(|| format!("parse deny-read ACL record {}", path.display())),
Err(err) if err.kind() == std::io::ErrorKind::NotFound => Ok(DenyReadAclRecord::default()),
Err(err) => {
Err(err).with_context(|| format!("read deny-read ACL record {}", path.display()))
}
}
}
pub fn write_persistent_deny_read_acl_record(
codex_home: &Path,
kind: DenyReadAclRecordKind,
paths: &[PathBuf],
) -> Result<()> {
let record_path = deny_read_acl_record_path(codex_home, kind);
let planned_paths = plan_deny_read_acl_paths(paths);
if planned_paths.is_empty() {
match std::fs::remove_file(&record_path) {
Ok(()) => return Ok(()),
Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Ok(()),
Err(err) => {
return Err(err).with_context(|| {
format!("remove deny-read ACL record {}", record_path.display())
});
}
}
}
if let Some(parent) = record_path.parent() {
std::fs::create_dir_all(parent)
.with_context(|| format!("create deny-read ACL record dir {}", parent.display()))?;
}
let record = DenyReadAclRecord {
paths: planned_paths,
};
let contents = serde_json::to_vec_pretty(&record)
.with_context(|| format!("serialize deny-read ACL record {}", record_path.display()))?;
std::fs::write(&record_path, contents)
.with_context(|| format!("write deny-read ACL record {}", record_path.display()))
}
/// Removes deny-read ACEs that were applied for a previous policy but are not
/// present in the current policy. This uses the same broad revoke primitive as
/// the rest of the Windows sandbox ACL guard path, so callers should run stale
/// cleanup before re-granting any read ACLs for the same SID in this refresh.
/// That ordering matters because `revoke_ace` removes all ACEs for the SID, not
/// only the deny-read ACEs recorded here.
///
/// # Safety
/// Caller must pass a valid SID pointer for the ACEs recorded under `kind`.
pub unsafe fn cleanup_stale_persistent_deny_read_acls(
codex_home: &Path,
kind: DenyReadAclRecordKind,
desired_paths: &[PathBuf],
psid: *mut c_void,
) -> Result<Vec<PathBuf>> {
let record_path = deny_read_acl_record_path(codex_home, kind);
let record = read_record(&record_path)?;
let desired_keys = plan_deny_read_acl_paths(desired_paths)
.into_iter()
.map(|path| lexical_path_key(&path))
.collect::<HashSet<_>>();
let mut cleaned = Vec::new();
for path in record.paths {
if desired_keys.contains(&lexical_path_key(&path)) || !path.exists() {
continue;
}
revoke_ace(&path, psid);
cleaned.push(path);
}
Ok(cleaned)
}
/// Applies deny-read ACEs to explicit paths. Missing paths are materialized as
/// directories before the ACE is applied so a sandboxed command cannot create a
/// previously absent denied path and then read from it in the same run.

19
codex-rs/windows-sandbox-rs/src/deny_read_resolver.rs
View File

@@ -47,7 +47,7 @@ pub fn resolve_windows_deny_read_paths(
})
.collect(),
);
let Some(matcher) = ReadDenyMatcher::new(&glob_policy, cwd.as_path()) else {
let Some(matcher) = ReadDenyMatcher::try_new(&glob_policy, cwd.as_path())? else {
return Ok(paths);
};
@@ -276,6 +276,23 @@ mod tests {
assert_eq!(actual, expected);
}
#[test]
fn invalid_glob_patterns_fail_before_expansion() {
let tmp = TempDir::new().expect("tempdir");
let cwd = AbsolutePathBuf::from_absolute_path(tmp.path()).expect("absolute cwd");
let policy = FileSystemSandboxPolicy::restricted(vec![unreadable_glob_entry(format!(
"{}/**/[z-a]",
tmp.path().display()
))]);
let err = resolve_windows_deny_read_paths(&policy, &cwd).expect_err("invalid glob");
assert!(
err.contains("invalid deny-read glob pattern"),
"unexpected error: {err}"
);
assert!(err.contains("invalid range"), "unexpected error: {err}");
}
#[test]
fn non_recursive_globs_do_not_expand_nested_matches() {
let tmp = TempDir::new().expect("tempdir");

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

@@ -112,15 +112,9 @@ pub use conpty::ConptyInstance;
#[cfg(target_os = "windows")]
pub use conpty::spawn_conpty_process_as_user;
#[cfg(target_os = "windows")]
pub use deny_read_acl::DenyReadAclRecordKind;
#[cfg(target_os = "windows")]
pub use deny_read_acl::apply_deny_read_acls;
#[cfg(target_os = "windows")]
pub use deny_read_acl::cleanup_stale_persistent_deny_read_acls;
#[cfg(target_os = "windows")]
pub use deny_read_acl::plan_deny_read_acl_paths;
#[cfg(target_os = "windows")]
pub use deny_read_acl::write_persistent_deny_read_acl_record;
pub use deny_read_resolver::resolve_windows_deny_read_paths;
#[cfg(target_os = "windows")]
pub use desktop::LaunchDesktop;
@@ -283,10 +277,7 @@ mod windows_impl {
use super::allow::compute_allow_paths;
use super::cap::load_or_create_cap_sids;
use super::cap::workspace_cap_sid_for_cwd;
use super::deny_read_acl::DenyReadAclRecordKind;
use super::deny_read_acl::apply_deny_read_acls;
use super::deny_read_acl::cleanup_stale_persistent_deny_read_acls;
use super::deny_read_acl::write_persistent_deny_read_acl_record;
use super::logging::log_failure;
use super::logging::log_success;
use super::path_normalization::canonicalize_path;
@@ -297,6 +288,7 @@ mod windows_impl {
use super::token::convert_string_sid_to_sid;
use super::token::create_workspace_write_token_with_caps_from;
use super::workspace_acl::is_command_cwd_root;
use anyhow::Context;
use anyhow::Result;
use std::collections::HashMap;
use std::ffi::c_void;
@@ -457,26 +449,17 @@ mod windows_impl {
let AllowDenyPaths { allow, mut deny } =
compute_allow_paths(&policy, sandbox_policy_cwd, &current_dir, &env_map);
for path in additional_deny_write_paths {
if path.exists() {
deny.insert(path.clone());
// Explicit deny-write carveouts must already exist when the process
// starts, otherwise it could create a missing path under a writable
// parent before the deny-write ACE exists.
if !path.exists() {
std::fs::create_dir_all(path)
.with_context(|| format!("create deny-write path {}", path.display()))?;
}
deny.insert(path.clone());
}
let canonical_cwd = canonicalize_path(&current_dir);
let mut guards: Vec<(PathBuf, *mut c_void)> = Vec::new();
if persist_aces {
// Persistent workspace-write ACEs survive between commands, so drop
// deny-read ACLs from the previous policy before applying the new
// overlay. Non-persistent runs use guards and clean up at process
// exit instead.
unsafe {
cleanup_stale_persistent_deny_read_acls(
codex_home,
DenyReadAclRecordKind::RestrictedToken,
additional_deny_read_paths,
psid_generic,
)?;
}
}
unsafe {
for p in &allow {
let psid = if is_workspace_write && is_command_cwd_root(p, &canonical_cwd) {
@@ -517,13 +500,7 @@ mod windows_impl {
return Err(err);
}
};
if persist_aces {
write_persistent_deny_read_acl_record(
codex_home,
DenyReadAclRecordKind::RestrictedToken,
&applied_deny_read_paths,
)?;
} else {
if !persist_aces {
for path in applied_deny_read_paths {
guards.push((path, psid_generic));
}