core/core-agent-ide
8
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

speed and reliability improvements for setting reads ACLs (#8216)

Browse source 
- Batch read ACL creation for online/offline sandbox user
- creates a new ACL helper process that is long-lived and runs in the
background
- uses a mutex so that only one helper process is running at a time.
This commit is contained in:
iceweasel-oai 2025-12-17 15:27:52 -08:00 • committed by GitHub
parent 927a6acbea
commit 25ecd0c2e4
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
4 changed files with 360 additions and 177 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

22
codex-rs/windows-sandbox-rs/src/acl.rs
View file

@ -342,21 +342,25 @@ const WRITE_ALLOW_MASK: u32 = FILE_GENERIC_READ
| DELETE
| FILE_DELETE_CHILD;
/// Ensure all provided SIDs have a write-capable allow ACE on the path.
/// Ensure all provided SIDs have an allow ACE with the requested mask on the path.
/// Returns true if any ACE was added.
///
/// # Safety
/// Caller must pass valid SID pointers and an existing path; free the returned security descriptor with `LocalFree`.
#[allow(dead_code)]
pub unsafe fn ensure_allow_write_aces(path: &Path, sids: &[*mut c_void]) -> Result<bool> {
pub unsafe fn ensure_allow_mask_aces(
path: &Path,
sids: &[*mut c_void],
allow_mask: u32,
) -> Result<bool> {
let (p_dacl, p_sd) = fetch_dacl_handle(path)?;
let mut entries: Vec<EXPLICIT_ACCESS_W> = Vec::new();
for sid in sids {
if dacl_mask_allows(p_dacl, &[*sid], WRITE_ALLOW_MASK, true) {
if dacl_mask_allows(p_dacl, &[*sid], allow_mask, true) {
continue;
}
entries.push(EXPLICIT_ACCESS_W {
grfAccessPermissions: WRITE_ALLOW_MASK,
grfAccessPermissions: allow_mask,
grfAccessMode: 2, // SET_ACCESS
grfInheritance: CONTAINER_INHERIT_ACE | OBJECT_INHERIT_ACE,
Trustee: TRUSTEE_W {
@ -414,6 +418,16 @@ pub unsafe fn ensure_allow_write_aces(path: &Path, sids: &[*mut c_void]) -> Resu
Ok(added)
}
/// Ensure all provided SIDs have a write-capable allow ACE on the path.
/// Returns true if any ACE was added.
///
/// # Safety
/// Caller must pass valid SID pointers and an existing path; free the returned security descriptor with `LocalFree`.
#[allow(dead_code)]
pub unsafe fn ensure_allow_write_aces(path: &Path, sids: &[*mut c_void]) -> Result<bool> {
ensure_allow_mask_aces(path, sids, WRITE_ALLOW_MASK)
}
/// Adds an allow ACE granting read/write/execute to the given SID on the target path.
///
/// # Safety

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

@ -18,6 +18,8 @@ mod elevated_impl;
#[cfg(target_os = "windows")]
pub use acl::allow_null_device;
#[cfg(target_os = "windows")]
pub use acl::ensure_allow_mask_aces;
#[cfg(target_os = "windows")]
pub use acl::ensure_allow_write_aces;
#[cfg(target_os = "windows")]
pub use acl::fetch_dacl_handle;

62
codex-rs/windows-sandbox-rs/src/read_acl_mutex.rs Normal file
View file

@ -0,0 +1,62 @@
use anyhow::Result;
use std::ffi::OsStr;
use windows_sys::Win32::Foundation::CloseHandle;
use windows_sys::Win32::Foundation::GetLastError;
use windows_sys::Win32::Foundation::ERROR_ALREADY_EXISTS;
use windows_sys::Win32::Foundation::ERROR_FILE_NOT_FOUND;
use windows_sys::Win32::Foundation::HANDLE;
use windows_sys::Win32::System::Threading::CreateMutexW;
use windows_sys::Win32::System::Threading::OpenMutexW;
use windows_sys::Win32::System::Threading::ReleaseMutex;
use windows_sys::Win32::System::Threading::MUTEX_ALL_ACCESS;
use super::to_wide;
const READ_ACL_MUTEX_NAME: &str = "Local\\CodexSandboxReadAcl";
pub struct ReadAclMutexGuard {
handle: HANDLE,
}
impl Drop for ReadAclMutexGuard {
fn drop(&mut self) {
unsafe {
let _ = ReleaseMutex(self.handle);
CloseHandle(self.handle);
}
}
}
pub fn read_acl_mutex_exists() -> Result<bool> {
let name = to_wide(OsStr::new(READ_ACL_MUTEX_NAME));
let handle = unsafe { OpenMutexW(MUTEX_ALL_ACCESS, 0, name.as_ptr()) };
if handle == 0 {
let err = unsafe { GetLastError() };
if err == ERROR_FILE_NOT_FOUND {
return Ok(false);
}
return Err(anyhow::anyhow!("OpenMutexW failed: {}", err));
}
unsafe {
CloseHandle(handle);
}
Ok(true)
}
pub fn acquire_read_acl_mutex() -> Result<Option<ReadAclMutexGuard>> {
let name = to_wide(OsStr::new(READ_ACL_MUTEX_NAME));
let handle = unsafe { CreateMutexW(std::ptr::null_mut(), 1, name.as_ptr()) };
if handle == 0 {
return Err(anyhow::anyhow!("CreateMutexW failed: {}", unsafe {
GetLastError()
}));
}
let err = unsafe { GetLastError() };
if err == ERROR_ALREADY_EXISTS {
unsafe {
CloseHandle(handle);
}
return Ok(None);
}
Ok(Some(ReadAclMutexGuard { handle }))
}

451
codex-rs/windows-sandbox-rs/src/setup_main_win.rs
View file

@ -6,8 +6,8 @@ use base64::engine::general_purpose::STANDARD as BASE64;
use base64::Engine;
use codex_windows_sandbox::convert_string_sid_to_sid;
use codex_windows_sandbox::dpapi_protect;
use codex_windows_sandbox::ensure_allow_mask_aces;
use codex_windows_sandbox::ensure_allow_write_aces;
use codex_windows_sandbox::fetch_dacl_handle;
use codex_windows_sandbox::load_or_create_cap_sids;
use codex_windows_sandbox::log_note;
use codex_windows_sandbox::path_mask_allows;
@ -26,10 +26,13 @@ use std::ffi::OsStr;
use std::fs::File;
use std::io::Write;
use std::os::windows::ffi::OsStrExt;
use std::os::windows::process::CommandExt;
use std::path::Path;
use std::path::PathBuf;
use std::process::Command;
use std::process::Stdio;
use std::sync::mpsc;
use std::time::Duration;
use std::time::Instant;
use windows::core::Interface;
use windows::core::BSTR;
use windows::Win32::Foundation::VARIANT_TRUE;
@ -80,7 +83,11 @@ 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;
#[derive(Debug, Deserialize)]
mod read_acl_mutex;
use read_acl_mutex::acquire_read_acl_mutex;
use read_acl_mutex::read_acl_mutex_exists;
#[derive(Debug, Clone, Deserialize, Serialize)]
struct Payload {
version: u32,
offline_username: String,
@ -90,9 +97,24 @@ struct Payload {
write_roots: Vec<PathBuf>,
real_user: String,
#[serde(default)]
mode: SetupMode,
#[serde(default)]
refresh_only: bool,
}
#[derive(Debug, Clone, Copy, Deserialize, Serialize, PartialEq, Eq)]
#[serde(rename_all = "kebab-case")]
enum SetupMode {
Full,
ReadAclsOnly,
}
impl Default for SetupMode {
fn default() -> Self {
Self::Full
}
}
#[derive(Serialize)]
struct SandboxUserRecord {
username: String,
@ -245,90 +267,191 @@ fn resolve_sid(name: &str) -> Result<Vec<u8>> {
}
}
fn add_inheritable_allow_no_log(path: &Path, sid: &[u8], mask: u32) -> Result<()> {
unsafe {
let mut psid: *mut c_void = std::ptr::null_mut();
let sid_str = string_from_sid_bytes(sid).map_err(anyhow::Error::msg)?;
let sid_w = to_wide(OsStr::new(&sid_str));
if ConvertStringSidToSidW(sid_w.as_ptr(), &mut psid) == 0 {
return Err(anyhow::anyhow!(
"ConvertStringSidToSidW failed: {}",
GetLastError()
));
fn spawn_read_acl_helper(payload: &Payload, log: &mut File) -> Result<()> {
let mut read_payload = payload.clone();
read_payload.mode = SetupMode::ReadAclsOnly;
read_payload.refresh_only = true;
let payload_json = serde_json::to_vec(&read_payload)?;
let payload_b64 = BASE64.encode(payload_json);
let exe = std::env::current_exe().context("locate setup helper")?;
let child = Command::new(&exe)
.arg(payload_b64)
.stdin(Stdio::null())
.stdout(Stdio::null())
.stderr(Stdio::null())
.creation_flags(0x08000000) // CREATE_NO_WINDOW
.spawn()
.context("spawn read ACL helper")?;
let pid = child.id();
log_line(log, &format!("spawned read ACL helper pid={pid}"))?;
Ok(())
}
struct ReadAclSubjects<'a> {
offline_psid: *mut c_void,
online_psid: *mut c_void,
rx_psids: &'a [*mut c_void],
}
fn apply_read_acls(
read_roots: &[PathBuf],
subjects: ReadAclSubjects<'_>,
log: &mut File,
refresh_errors: &mut Vec<String>,
) -> Result<()> {
log_line(
log,
&format!("read ACL: processing {} read roots", read_roots.len()),
)?;
let read_mask = FILE_GENERIC_READ | FILE_GENERIC_EXECUTE;
for root in read_roots {
if !root.exists() {
log_line(
log,
&format!("read root {} missing; skipping", root.display()),
)?;
continue;
}
let (existing_dacl, sd) = fetch_dacl_handle(path)?;
let trustee = TRUSTEE_W {
pMultipleTrustee: std::ptr::null_mut(),
MultipleTrusteeOperation: 0,
TrusteeForm: TRUSTEE_IS_SID,
TrusteeType: TRUSTEE_IS_SID,
ptstrName: psid as *mut u16,
};
let ea = EXPLICIT_ACCESS_W {
grfAccessPermissions: mask,
grfAccessMode: GRANT_ACCESS,
grfInheritance: OBJECT_INHERIT_ACE | CONTAINER_INHERIT_ACE,
Trustee: trustee,
};
let mut new_dacl: *mut ACL = std::ptr::null_mut();
let set = SetEntriesInAclW(1, &ea, existing_dacl, &mut new_dacl);
if set != 0 {
return Err(anyhow::anyhow!("SetEntriesInAclW failed: {}", set));
let builtin_has = read_mask_allows_or_log(
root,
subjects.rx_psids,
None,
read_mask,
refresh_errors,
log,
)?;
if builtin_has {
log_line(
log,
&format!(
"Users/AU/Everyone already has RX on {}; skipping",
root.display()
),
)?;
continue;
}
let res = SetNamedSecurityInfoW(
to_wide(path.as_os_str()).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(),
);
if res != 0 {
return Err(anyhow::anyhow!(
"SetNamedSecurityInfoW failed for {}: {}",
path.display(),
res
));
let offline_has = read_mask_allows_or_log(
root,
&[subjects.offline_psid],
Some("offline"),
read_mask,
refresh_errors,
log,
)?;
let online_has = read_mask_allows_or_log(
root,
&[subjects.online_psid],
Some("online"),
read_mask,
refresh_errors,
log,
)?;
if offline_has && online_has {
log_line(
log,
&format!(
"sandbox users already have RX on {}; skipping",
root.display()
),
)?;
continue;
}
if !new_dacl.is_null() {
LocalFree(new_dacl as HLOCAL);
log_line(
log,
&format!("granting read ACE to {} for sandbox users", root.display()),
)?;
let mut successes = usize::from(offline_has) + usize::from(online_has);
let mut missing_psids: Vec<*mut c_void> = Vec::new();
let mut missing_labels: Vec<&str> = Vec::new();
if !offline_has {
missing_psids.push(subjects.offline_psid);
missing_labels.push("offline");
}
if !sd.is_null() {
LocalFree(sd as HLOCAL);
if !online_has {
missing_psids.push(subjects.online_psid);
missing_labels.push("online");
}
if !psid.is_null() {
LocalFree(psid as HLOCAL);
if !missing_psids.is_empty() {
let started = Instant::now();
match unsafe { ensure_allow_mask_aces(root, &missing_psids, read_mask) } {
Ok(_) => {
let elapsed_ms = started.elapsed().as_millis();
successes = 2;
log_line(
log,
&format!(
"grant read ACE succeeded on {} for {} in {elapsed_ms}ms",
root.display(),
missing_labels.join(", ")
),
)?;
}
Err(err) => {
let elapsed_ms = started.elapsed().as_millis();
let label_list = missing_labels.join(", ");
for label in &missing_labels {
refresh_errors.push(format!(
"grant read ACE failed on {} for {label}: {err}",
root.display()
));
}
log_line(
log,
&format!(
"grant read ACE failed on {} for {} in {elapsed_ms}ms: {err}",
root.display(),
label_list
),
)?;
}
}
}
if successes == 2 {
log_line(log, &format!("granted read ACE to {}", root.display()))?;
} else {
log_line(
log,
&format!(
"read ACE incomplete on {} (success {successes}/2)",
root.display()
),
)?;
}
}
Ok(())
}
fn try_add_inheritable_allow_with_timeout(
path: &Path,
sid: &[u8],
mask: u32,
_log: &mut File,
timeout: Duration,
) -> Result<()> {
let (tx, rx) = mpsc::channel::<Result<()>>();
let path_buf = path.to_path_buf();
let sid_vec = sid.to_vec();
std::thread::spawn(move || {
let res = add_inheritable_allow_no_log(&path_buf, &sid_vec, mask);
let _ = tx.send(res);
});
match rx.recv_timeout(timeout) {
Ok(res) => res,
Err(mpsc::RecvTimeoutError::Timeout) => Err(anyhow::anyhow!(
"ACL grant timed out on {} after {:?}",
path.display(),
timeout
)),
Err(e) => Err(anyhow::anyhow!(
"ACL grant channel error on {}: {e}",
path.display()
)),
fn read_mask_allows_or_log(
root: &Path,
psids: &[*mut c_void],
label: Option<&str>,
read_mask: u32,
refresh_errors: &mut Vec<String>,
log: &mut File,
) -> Result<bool> {
match path_mask_allows(root, psids, read_mask, true) {
Ok(has) => Ok(has),
Err(e) => {
let label_suffix = label
.map(|value| format!(" for {value}"))
.unwrap_or_default();
refresh_errors.push(format!(
"read mask check failed on {}{}: {}",
root.display(),
label_suffix,
e
));
log_line(
log,
&format!(
"read mask check failed on {}{}: {}; continuing",
root.display(),
label_suffix,
e
),
)?;
Ok(false)
}
}
}
@ -607,6 +730,70 @@ fn real_main() -> Result<()> {
}
fn run_setup(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<()> {
match payload.mode {
SetupMode::ReadAclsOnly => run_read_acl_only(payload, log),
SetupMode::Full => run_setup_full(payload, log, sbx_dir),
}
}
fn run_read_acl_only(payload: &Payload, log: &mut File) -> Result<()> {
let _read_acl_guard = match acquire_read_acl_mutex()? {
Some(guard) => guard,
None => {
log_line(log, "read ACL helper already running; skipping")?;
return Ok(());
}
};
log_line(log, "read-acl-only mode: applying read ACLs")?;
let offline_sid = resolve_sid(&payload.offline_username)?;
let online_sid = resolve_sid(&payload.online_username)?;
let offline_psid = sid_bytes_to_psid(&offline_sid)?;
let online_psid = sid_bytes_to_psid(&online_sid)?;
let mut refresh_errors: Vec<String> = Vec::new();
let users_sid = resolve_sid("Users")?;
let users_psid = sid_bytes_to_psid(&users_sid)?;
let auth_sid = resolve_sid("Authenticated Users")?;
let auth_psid = sid_bytes_to_psid(&auth_sid)?;
let everyone_sid = resolve_sid("Everyone")?;
let everyone_psid = sid_bytes_to_psid(&everyone_sid)?;
let rx_psids = vec![users_psid, auth_psid, everyone_psid];
let subjects = ReadAclSubjects {
offline_psid,
online_psid,
rx_psids: &rx_psids,
};
apply_read_acls(&payload.read_roots, subjects, log, &mut refresh_errors)?;
unsafe {
if !offline_psid.is_null() {
LocalFree(offline_psid as HLOCAL);
}
if !online_psid.is_null() {
LocalFree(online_psid as HLOCAL);
}
if !users_psid.is_null() {
LocalFree(users_psid as HLOCAL);
}
if !auth_psid.is_null() {
LocalFree(auth_psid as HLOCAL);
}
if !everyone_psid.is_null() {
LocalFree(everyone_psid as HLOCAL);
}
}
if !refresh_errors.is_empty() {
log_line(
log,
&format!("read ACL run completed with errors: {:?}", refresh_errors),
)?;
if payload.refresh_only {
anyhow::bail!("read ACL run had errors");
}
}
log_line(log, "read ACL run completed")?;
Ok(())
}
fn run_setup_full(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<()> {
let refresh_only = payload.refresh_only;
let offline_pwd = if refresh_only {
None
@ -654,13 +841,6 @@ fn run_setup(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<()> {
.ok_or_else(|| anyhow::anyhow!("convert capability SID failed"))?
};
let mut refresh_errors: Vec<String> = Vec::new();
let users_sid = resolve_sid("Users")?;
let users_psid = sid_bytes_to_psid(&users_sid)?;
let auth_sid = resolve_sid("Authenticated Users")?;
let auth_psid = sid_bytes_to_psid(&auth_sid)?;
let everyone_sid = resolve_sid("Everyone")?;
let everyone_psid = sid_bytes_to_psid(&everyone_sid)?;
let rx_psids = vec![users_psid, auth_psid, everyone_psid];
log_line(log, &format!("resolved capability SID {}", caps.workspace))?;
if !refresh_only {
run_netsh_firewall(&offline_sid_str, log)?;
@ -669,94 +849,29 @@ fn run_setup(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<()> {
log_line(
log,
&format!(
"refresh: processing {} read roots, {} write roots",
"refresh: delegating {} read roots; processing {} write roots",
payload.read_roots.len(),
payload.write_roots.len()
),
)?;
for root in &payload.read_roots {
if !root.exists() {
log_line(
log,
&format!("read root {} missing; skipping", root.display()),
)?;
continue;
}
match path_mask_allows(
root,
&rx_psids,
FILE_GENERIC_READ | FILE_GENERIC_EXECUTE,
true,
) {
Ok(has) => {
if has {
log_line(
log,
&format!(
"Users/AU/Everyone already has RX on {}; skipping",
root.display()
),
)?;
continue;
}
if payload.read_roots.is_empty() {
log_line(log, "no read roots to grant; skipping read ACL helper")?;
} else {
match read_acl_mutex_exists() {
Ok(true) => {
log_line(log, "read ACL helper already running; skipping spawn")?;
}
Err(e) => {
refresh_errors.push(format!(
"read mask check failed on {}: {}",
root.display(),
e
));
Ok(false) => {
spawn_read_acl_helper(payload, log)?;
}
Err(err) => {
log_line(
log,
&format!(
"read mask check failed on {}: {}; continuing",
root.display(),
e
),
&format!("read ACL mutex check failed: {err}; spawning anyway"),
)?;
spawn_read_acl_helper(payload, log)?;
}
}
log_line(
log,
&format!("granting read ACE to {} for sandbox users", root.display()),
)?;
let mut successes = 0usize;
let read_mask = FILE_GENERIC_READ | FILE_GENERIC_EXECUTE;
for (label, sid_bytes) in [("offline", &offline_sid), ("online", &online_sid)] {
match try_add_inheritable_allow_with_timeout(
root,
sid_bytes,
read_mask,
log,
Duration::from_millis(100),
) {
Ok(_) => {
successes += 1;
}
Err(e) => {
log_line(
log,
&format!(
"grant read ACE timed out/failed on {} for {label}: {e}",
root.display()
),
)?;
// Best-effort: continue to next SID/root.
}
}
}
if successes == 2 {
log_line(log, &format!("granted read ACE to {}", root.display()))?;
} else {
log_line(
log,
&format!(
"read ACE incomplete on {} (success {}/2)",
root.display(),
successes
),
)?;
}
}
let cap_sid_str = caps.workspace.clone();
@ -899,8 +1014,7 @@ fn run_setup(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<()> {
log_line(
log,
&format!(
"setup refresh: processed {} read roots, {} write roots; errors={:?}",
payload.read_roots.len(),
"setup refresh: processed {} write roots (read roots delegated); errors={:?}",
payload.write_roots.len(),
refresh_errors
),
@ -938,15 +1052,6 @@ fn run_setup(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<()> {
if !cap_psid.is_null() {
LocalFree(cap_psid as HLOCAL);
}
if !users_psid.is_null() {
LocalFree(users_psid as HLOCAL);
}
if !auth_psid.is_null() {
LocalFree(auth_psid as HLOCAL);
}
if !everyone_psid.is_null() {
LocalFree(everyone_psid as HLOCAL);
}
}
if refresh_only && !refresh_errors.is_empty() {
log_line(