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

add deny ACEs for world writable dirs (#7022)

Browse Source 
Our Restricted Token contains 3 SIDs (Logon, Everyone, {WorkspaceWrite
Capability || ReadOnly Capability})

because it must include Everyone, that left us vulnerable to directories
that allow writes to Everyone. Even though those directories do not have
ACEs that enable our capability SIDs to write to them, they could still
be written to even in ReadOnly mode, or even in WorkspaceWrite mode if
they are outside of a writable root.

A solution to this is to explicitly add *Deny* ACEs to these
directories, always for the ReadOnly Capability SID, and for the
WorkspaceWrite SID if the directory is outside of a workspace root.

Under a restricted token, Windows always checks Deny ACEs before Allow
ACEs so even though our restricted token would allow a write to these
directories due to the Everyone SID, it fails first because of the Deny
ACE on the capability SID
This commit is contained in:
iceweasel-oai
2025-11-20 14:50:33 -08:00
committed by GitHub
parent 54ee302a06
commit 3f92ad4190
4 changed files with 224 additions and 3 deletions
Download Patch File Download Diff File Expand all files Collapse all files

15
codex-rs/tui/src/app.rs
View File

@@ -343,7 +343,8 @@ impl App {
let env_map: std::collections::HashMap<String, String> = std::env::vars().collect();
let tx = app.app_event_tx.clone();
let logs_base_dir = app.config.codex_home.clone();
Self::spawn_world_writable_scan(cwd, env_map, logs_base_dir, tx);
let sandbox_policy = app.config.sandbox_policy.clone();
Self::spawn_world_writable_scan(cwd, env_map, logs_base_dir, sandbox_policy, tx);
}
}
@@ -717,7 +718,14 @@ impl App {
std::env::vars().collect();
let tx = self.app_event_tx.clone();
let logs_base_dir = self.config.codex_home.clone();
Self::spawn_world_writable_scan(cwd, env_map, logs_base_dir, tx);
let sandbox_policy = self.config.sandbox_policy.clone();
Self::spawn_world_writable_scan(
cwd,
env_map,
logs_base_dir,
sandbox_policy,
tx,
);
}
}
}
@@ -911,6 +919,7 @@ impl App {
cwd: PathBuf,
env_map: std::collections::HashMap<String, String>,
logs_base_dir: PathBuf,
sandbox_policy: codex_core::protocol::SandboxPolicy,
tx: AppEventSender,
) {
#[inline]
@@ -920,8 +929,10 @@ impl App {
}
tokio::task::spawn_blocking(move || {
let result = codex_windows_sandbox::preflight_audit_everyone_writable(
&logs_base_dir,
&cwd,
&env_map,
&sandbox_policy,
Some(logs_base_dir.as_path()),
);
if let Ok(ref paths) = result

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

@@ -31,11 +31,17 @@ use windows_sys::Win32::Storage::FileSystem::FILE_ATTRIBUTE_NORMAL;
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_APPEND_DATA;
use windows_sys::Win32::Storage::FileSystem::FILE_WRITE_ATTRIBUTES;
use windows_sys::Win32::Storage::FileSystem::FILE_WRITE_DATA;
use windows_sys::Win32::Storage::FileSystem::FILE_WRITE_EA;
use windows_sys::Win32::Storage::FileSystem::FILE_SHARE_READ;
use windows_sys::Win32::Storage::FileSystem::FILE_SHARE_WRITE;
use windows_sys::Win32::Storage::FileSystem::OPEN_EXISTING;
const SE_KERNEL_OBJECT: u32 = 6;
const INHERIT_ONLY_ACE: u8 = 0x08;
const GENERIC_WRITE_MASK: u32 = 0x4000_0000;
const DENY_ACCESS: i32 = 3;
pub unsafe fn dacl_has_write_allow_for_sid(p_dacl: *mut ACL, psid: *mut c_void) -> bool {
if p_dacl.is_null() {
@@ -78,6 +84,49 @@ pub unsafe fn dacl_has_write_allow_for_sid(p_dacl: *mut ACL, psid: *mut c_void)
false
}
pub unsafe fn dacl_has_write_deny_for_sid(p_dacl: *mut ACL, psid: *mut c_void) -> bool {
if p_dacl.is_null() {
return false;
}
let mut info: ACL_SIZE_INFORMATION = std::mem::zeroed();
let ok = GetAclInformation(
p_dacl as *const ACL,
&mut info as *mut _ as *mut c_void,
std::mem::size_of::<ACL_SIZE_INFORMATION>() as u32,
AclSizeInformation,
);
if ok == 0 {
return false;
}
let deny_write_mask = FILE_GENERIC_WRITE
| FILE_WRITE_DATA
| FILE_APPEND_DATA
| FILE_WRITE_EA
| FILE_WRITE_ATTRIBUTES
| GENERIC_WRITE_MASK;
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 {
continue;
}
let hdr = &*(p_ace as *const ACE_HEADER);
if hdr.AceType != 1 {
continue; // ACCESS_DENIED_ACE_TYPE
}
if (hdr.AceFlags & INHERIT_ONLY_ACE) != 0 {
continue;
}
let ace = &*(p_ace as *const ACCESS_ALLOWED_ACE);
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;
if EqualSid(sid_ptr, psid) != 0 && (ace.Mask & deny_write_mask) != 0 {
return true;
}
}
false
}
// Compute effective rights for a trustee SID against a DACL and decide if write is effectively allowed.
// This accounts for deny ACEs and ordering; falls back to a conservative per-ACE scan if the API fails.
#[allow(dead_code)]
@@ -168,6 +217,67 @@ pub unsafe fn add_allow_ace(path: &Path, psid: *mut c_void) -> Result<bool> {
Ok(added)
}
pub unsafe fn add_deny_write_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_write_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_WRITE
| FILE_WRITE_DATA
| FILE_APPEND_DATA
| FILE_WRITE_EA
| FILE_WRITE_ATTRIBUTES
| GENERIC_WRITE_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 {
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 {
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)
}
pub unsafe fn revoke_ace(path: &Path, psid: *mut c_void) {
let mut p_sd: *mut c_void = std::ptr::null_mut();
let mut p_dacl: *mut ACL = std::ptr::null_mut();

78
codex-rs/windows-sandbox-rs/src/audit.rs
View File

@@ -1,4 +1,11 @@
use crate::acl::add_deny_write_ace;
use crate::cap::cap_sid_file;
use crate::cap::load_or_create_cap_sids;
use crate::logging::log_note;
use crate::policy::SandboxPolicy;
use crate::token::convert_string_sid_to_sid;
use crate::token::world_sid;
use anyhow::anyhow;
use crate::winutil::to_wide;
use anyhow::Result;
use std::collections::HashMap;
@@ -271,6 +278,13 @@ pub fn audit_everyone_writable(
for p in &flagged {
list.push_str(&format!("\n - {}", p.display()));
}
crate::logging::log_note(
&format!(
"AUDIT: world-writable scan FAILED; cwd={cwd:?}; checked={checked}; duration_ms={elapsed_ms}; flagged:{}",
list
),
logs_base_dir,
);
return Ok(flagged);
}
@@ -282,6 +296,70 @@ pub fn audit_everyone_writable(
Ok(Vec::new())
}
pub fn apply_capability_denies_for_world_writable(
codex_home: &Path,
flagged: &[PathBuf],
sandbox_policy: &SandboxPolicy,
cwd: &Path,
logs_base_dir: Option<&Path>,
) -> Result<()> {
if flagged.is_empty() {
return Ok(());
}
std::fs::create_dir_all(codex_home)?;
let cap_path = cap_sid_file(codex_home);
let caps = load_or_create_cap_sids(codex_home);
std::fs::write(&cap_path, serde_json::to_string(&caps)?)?;
let (active_sid, workspace_roots): (*mut c_void, Vec<PathBuf>) = match sandbox_policy {
SandboxPolicy::WorkspaceWrite { writable_roots, .. } => {
let sid = unsafe { convert_string_sid_to_sid(&caps.workspace) }
.ok_or_else(|| anyhow!("ConvertStringSidToSidW failed for workspace capability"))?;
let mut roots: Vec<PathBuf> =
vec![dunce::canonicalize(cwd).unwrap_or_else(|_| cwd.to_path_buf())];
for root in writable_roots {
let candidate = if root.is_absolute() {
root.clone()
} else {
cwd.join(root)
};
roots.push(dunce::canonicalize(&candidate).unwrap_or(candidate));
}
(sid, roots)
}
SandboxPolicy::ReadOnly => (
unsafe { convert_string_sid_to_sid(&caps.readonly) }.ok_or_else(|| {
anyhow!("ConvertStringSidToSidW failed for readonly capability")
})?,
Vec::new(),
),
SandboxPolicy::DangerFullAccess => {
return Ok(());
}
};
for path in flagged {
if workspace_roots.iter().any(|root| path.starts_with(root)) {
continue;
}
let res = unsafe { add_deny_write_ace(path, active_sid) };
match res {
Ok(true) => log_note(
&format!("AUDIT: applied capability deny ACE to {}", path.display()),
logs_base_dir,
),
Ok(false) => {}
Err(err) => log_note(
&format!(
"AUDIT: failed to apply capability deny ACE to {}: {}",
path.display(),
err
),
logs_base_dir,
),
}
}
Ok(())
}
fn normalize_windows_path_for_display(p: impl AsRef<Path>) -> String {
let canon = dunce::canonicalize(p.as_ref()).unwrap_or_else(|_| p.as_ref().to_path_buf());
canon.display().to_string().replace('/', "\\")

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

@@ -38,6 +38,7 @@ mod windows_impl {
use super::env::normalize_null_device_env;
use super::logging::debug_log;
use super::logging::log_failure;
use super::logging::log_note;
use super::logging::log_start;
use super::logging::log_success;
use super::policy::parse_policy;
@@ -178,11 +179,29 @@ mod windows_impl {
}
pub fn preflight_audit_everyone_writable(
codex_home: &Path,
cwd: &Path,
env_map: &HashMap<String, String>,
sandbox_policy: &SandboxPolicy,
logs_base_dir: Option<&Path>,
) -> Result<Vec<PathBuf>> {
audit::audit_everyone_writable(cwd, env_map, logs_base_dir)
let flagged = audit::audit_everyone_writable(cwd, env_map, logs_base_dir)?;
if flagged.is_empty() {
return Ok(flagged);
}
if let Err(err) = audit::apply_capability_denies_for_world_writable(
codex_home,
&flagged,
sandbox_policy,
cwd,
logs_base_dir,
) {
log_note(
&format!("AUDIT: failed to apply capability deny ACEs: {}", err),
logs_base_dir,
);
}
Ok(Vec::new())
}
pub fn run_windows_sandbox_capture(
@@ -434,6 +453,7 @@ mod windows_impl {
mod stub {
use anyhow::bail;
use anyhow::Result;
use codex_protocol::protocol::SandboxPolicy;
use std::collections::HashMap;
use std::path::Path;
@@ -446,8 +466,10 @@ mod stub {
}
pub fn preflight_audit_everyone_writable(
_codex_home: &Path,
_cwd: &Path,
_env_map: &HashMap<String, String>,
_sandbox_policy: &SandboxPolicy,
_logs_base_dir: Option<&Path>,
) -> Result<Vec<std::path::PathBuf>> {
bail!("Windows sandbox is only available on Windows")