diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 0b38db1eb49..1ca7d997b53 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -1701,6 +1701,7 @@ name = "codex-windows-sandbox" version = "0.0.0" dependencies = [ "anyhow", + "chrono", "codex-protocol", "dirs-next", "dunce", diff --git a/codex-rs/windows-sandbox-rs/Cargo.toml b/codex-rs/windows-sandbox-rs/Cargo.toml index 29306371b29..1b936f05cab 100644 --- a/codex-rs/windows-sandbox-rs/Cargo.toml +++ b/codex-rs/windows-sandbox-rs/Cargo.toml @@ -10,6 +10,7 @@ path = "src/lib.rs" [dependencies] anyhow = "1.0" +chrono = { version = "0.4.42", default-features = false, features = ["clock", "std"] } dunce = "1.0" serde = { version = "1.0", features = ["derive"] } serde_json = "1.0" diff --git a/codex-rs/windows-sandbox-rs/src/acl.rs b/codex-rs/windows-sandbox-rs/src/acl.rs index f2e1e09480a..34d523d1f53 100644 --- a/codex-rs/windows-sandbox-rs/src/acl.rs +++ b/codex-rs/windows-sandbox-rs/src/acl.rs @@ -1,4 +1,4 @@ -use crate::winutil::to_wide; +use crate::winutil::to_wide; use anyhow::anyhow; use anyhow::Result; use std::ffi::c_void; @@ -9,6 +9,7 @@ use windows_sys::Win32::Foundation::ERROR_SUCCESS; use windows_sys::Win32::Foundation::HLOCAL; use windows_sys::Win32::Foundation::INVALID_HANDLE_VALUE; use windows_sys::Win32::Security::AclSizeInformation; +use windows_sys::Win32::Security::Authorization::GetEffectiveRightsFromAclW; use windows_sys::Win32::Security::Authorization::GetNamedSecurityInfoW; use windows_sys::Win32::Security::Authorization::GetSecurityInfo; use windows_sys::Win32::Security::Authorization::SetEntriesInAclW; @@ -21,28 +22,148 @@ use windows_sys::Win32::Security::Authorization::TRUSTEE_W; use windows_sys::Win32::Security::EqualSid; use windows_sys::Win32::Security::GetAce; use windows_sys::Win32::Security::GetAclInformation; +use windows_sys::Win32::Security::MapGenericMask; use windows_sys::Win32::Security::ACCESS_ALLOWED_ACE; use windows_sys::Win32::Security::ACE_HEADER; use windows_sys::Win32::Security::ACL; use windows_sys::Win32::Security::ACL_SIZE_INFORMATION; use windows_sys::Win32::Security::DACL_SECURITY_INFORMATION; +use windows_sys::Win32::Security::GENERIC_MAPPING; use windows_sys::Win32::Storage::FileSystem::CreateFileW; +use windows_sys::Win32::Storage::FileSystem::FILE_ALL_ACCESS; +use windows_sys::Win32::Storage::FileSystem::FILE_APPEND_DATA; use windows_sys::Win32::Storage::FileSystem::FILE_ATTRIBUTE_NORMAL; +use windows_sys::Win32::Storage::FileSystem::FILE_FLAG_BACKUP_SEMANTICS; 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_SHARE_DELETE; +use windows_sys::Win32::Storage::FileSystem::FILE_SHARE_READ; +use windows_sys::Win32::Storage::FileSystem::FILE_SHARE_WRITE; 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; +use windows_sys::Win32::Storage::FileSystem::READ_CONTROL; const SE_KERNEL_OBJECT: u32 = 6; const INHERIT_ONLY_ACE: u8 = 0x08; const GENERIC_WRITE_MASK: u32 = 0x4000_0000; const DENY_ACCESS: i32 = 3; +/// Fetch DACL via handle-based query; caller must LocalFree the returned SD. +pub unsafe fn fetch_dacl_handle(path: &Path) -> Result<(*mut ACL, *mut c_void)> { + let wpath = to_wide(path); + let h = CreateFileW( + wpath.as_ptr(), + READ_CONTROL, + FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, + std::ptr::null_mut(), + OPEN_EXISTING, + FILE_FLAG_BACKUP_SEMANTICS, + 0, + ); + if h == INVALID_HANDLE_VALUE { + return Err(anyhow!("CreateFileW failed for {}", path.display())); + } + let mut p_sd: *mut c_void = std::ptr::null_mut(); + let mut p_dacl: *mut ACL = std::ptr::null_mut(); + let code = GetSecurityInfo( + h, + 1, // SE_FILE_OBJECT + DACL_SECURITY_INFORMATION, + std::ptr::null_mut(), + std::ptr::null_mut(), + &mut p_dacl, + std::ptr::null_mut(), + &mut p_sd, + ); + CloseHandle(h); + if code != ERROR_SUCCESS { + return Err(anyhow!( + "GetSecurityInfo failed for {}: {}", + path.display(), + code + )); + } + Ok((p_dacl, p_sd)) +} + +/// Fast mask-based check: does any ACE for provided SIDs grant at least one desired bit? Skips inherit-only. +pub unsafe fn dacl_quick_mask_allows( + p_dacl: *mut ACL, + psids: &[*mut c_void], + desired_mask: u32, +) -> 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::() as u32, + AclSizeInformation, + ); + if ok == 0 { + return false; + } + let mapping = GENERIC_MAPPING { + GenericRead: FILE_GENERIC_READ, + GenericWrite: FILE_GENERIC_WRITE, + GenericExecute: FILE_GENERIC_EXECUTE, + GenericAll: FILE_ALL_ACCESS, + }; + for i in 0..(info.AceCount as usize) { + let mut p_ace: *mut c_void = std::ptr::null_mut(); + if GetAce(p_dacl as *const ACL, i as u32, &mut p_ace) == 0 { + continue; + } + let hdr = &*(p_ace as *const ACE_HEADER); + if hdr.AceType != 0 { + continue; // not ACCESS_ALLOWED + } + if (hdr.AceFlags & INHERIT_ONLY_ACE) != 0 { + continue; + } + let base = p_ace as usize; + let sid_ptr = + (base + std::mem::size_of::() + std::mem::size_of::()) as *mut c_void; + let mut matched = false; + for sid in psids { + if EqualSid(sid_ptr, *sid) != 0 { + matched = true; + break; + } + } + if !matched { + continue; + } + let ace = &*(p_ace as *const ACCESS_ALLOWED_ACE); + let mut mask = ace.Mask; + MapGenericMask(&mut mask, &mapping); + if (mask & desired_mask) != 0 { + return true; + } + } + false +} + +/// Path-based wrapper around the quick mask check (single DACL fetch). +pub fn path_quick_mask_allows( + path: &Path, + psids: &[*mut c_void], + desired_mask: u32, +) -> Result { + unsafe { + let (p_dacl, sd) = fetch_dacl_handle(path)?; + let has = dacl_quick_mask_allows(p_dacl, psids, desired_mask); + if !sd.is_null() { + LocalFree(sd as HLOCAL); + } + Ok(has) + } +} + pub unsafe fn dacl_has_write_allow_for_sid(p_dacl: *mut ACL, psid: *mut c_void) -> bool { if p_dacl.is_null() { return false; @@ -131,6 +252,44 @@ pub unsafe fn dacl_has_write_deny_for_sid(p_dacl: *mut ACL, psid: *mut c_void) - // This accounts for deny ACEs and ordering; falls back to a conservative per-ACE scan if the API fails. #[allow(dead_code)] pub unsafe fn dacl_effective_allows_write(p_dacl: *mut ACL, psid: *mut c_void) -> bool { + if p_dacl.is_null() { + return false; + } + 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 access: u32 = 0; + let ok = GetEffectiveRightsFromAclW(p_dacl, &trustee, &mut access); + if ok == ERROR_SUCCESS { + // Map generic bits to avoid “missing” write when generic permissions are present. + let mut mapped_access = access; + if (access & GENERIC_WRITE_MASK) != 0 { + mapped_access |= FILE_GENERIC_WRITE | FILE_WRITE_DATA | FILE_APPEND_DATA; + } + if (access & READ_CONTROL) != 0 { + mapped_access |= FILE_GENERIC_READ; + } + let write_bits = FILE_GENERIC_WRITE + | FILE_WRITE_DATA + | FILE_APPEND_DATA + | FILE_WRITE_EA + | FILE_WRITE_ATTRIBUTES; + return (mapped_access & write_bits) != 0; + } + // Fallback: simple allow ACE scan (already ignores inherit-only) + dacl_has_write_allow_for_sid(p_dacl, psid) +} + +#[allow(dead_code)] +pub unsafe fn dacl_effective_allows_mask( + p_dacl: *mut ACL, + psid: *mut c_void, + desired_mask: u32, +) -> bool { if p_dacl.is_null() { return false; } @@ -148,51 +307,59 @@ pub unsafe fn dacl_effective_allows_write(p_dacl: *mut ACL, psid: *mut c_void) - }; let mut access: u32 = 0; let ok = GetEffectiveRightsFromAclW(p_dacl, &trustee, &mut access); - if ok != 0 { - // Check for generic or specific write bits - let write_bits = FILE_GENERIC_WRITE - | windows_sys::Win32::Storage::FileSystem::FILE_WRITE_DATA - | windows_sys::Win32::Storage::FileSystem::FILE_APPEND_DATA - | windows_sys::Win32::Storage::FileSystem::FILE_WRITE_EA - | windows_sys::Win32::Storage::FileSystem::FILE_WRITE_ATTRIBUTES; - return (access & write_bits) != 0; + if ok == ERROR_SUCCESS { + // Map generic bits to avoid “missing” when generic permissions are present. + let mut mapped_access = access; + if (access & GENERIC_WRITE_MASK) != 0 { + mapped_access |= FILE_GENERIC_WRITE | FILE_WRITE_DATA | FILE_APPEND_DATA; + } + if (access & READ_CONTROL) != 0 { + mapped_access |= FILE_GENERIC_READ; + } + return (mapped_access & desired_mask) == desired_mask; } - // Fallback: simple allow ACE scan (already ignores inherit-only) - dacl_has_write_allow_for_sid(p_dacl, psid) + // Fallbacks on error: if write bits are requested, reuse the write helper; otherwise fail closed. + if (desired_mask & FILE_GENERIC_WRITE) != 0 { + return dacl_effective_allows_write(p_dacl, psid); + } + false } -pub unsafe fn add_allow_ace(path: &Path, psid: *mut c_void) -> Result { - 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)); + +#[allow(dead_code)] +const WRITE_ALLOW_MASK: u32 = FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE; + +/// Ensure all provided SIDs have a write-capable allow ACE on the path. +/// Returns true if any ACE was added. +#[allow(dead_code)] +pub unsafe fn ensure_allow_write_aces(path: &Path, sids: &[*mut c_void]) -> Result { + let (p_dacl, p_sd) = fetch_dacl_handle(path)?; + let mut entries: Vec = Vec::new(); + for sid in sids { + if dacl_quick_mask_allows(p_dacl, &[*sid], WRITE_ALLOW_MASK) { + continue; + } + entries.push(EXPLICIT_ACCESS_W { + grfAccessPermissions: WRITE_ALLOW_MASK, + grfAccessMode: 2, // SET_ACCESS + grfInheritance: CONTAINER_INHERIT_ACE | OBJECT_INHERIT_ACE, + Trustee: TRUSTEE_W { + pMultipleTrustee: std::ptr::null_mut(), + MultipleTrusteeOperation: 0, + TrusteeForm: TRUSTEE_IS_SID, + TrusteeType: TRUSTEE_IS_UNKNOWN, + ptstrName: *sid as *mut u16, + }, + }); } let mut added = false; - if !dacl_has_write_allow_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 | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE; - explicit.grfAccessMode = 2; // SET_ACCESS - explicit.grfInheritance = CONTAINER_INHERIT_ACE | OBJECT_INHERIT_ACE; - explicit.Trustee = trustee; + if !entries.is_empty() { let mut p_new_dacl: *mut ACL = std::ptr::null_mut(); - let code2 = SetEntriesInAclW(1, &explicit, p_dacl, &mut p_new_dacl); + let code2 = SetEntriesInAclW( + entries.len() as u32, + entries.as_ptr(), + p_dacl, + &mut p_new_dacl, + ); if code2 == ERROR_SUCCESS { let code3 = SetNamedSecurityInfoW( to_wide(path).as_ptr() as *mut u16, @@ -205,10 +372,89 @@ pub unsafe fn add_allow_ace(path: &Path, psid: *mut c_void) -> Result { ); if code3 == ERROR_SUCCESS { added = true; + } else { + if !p_new_dacl.is_null() { + LocalFree(p_new_dacl as HLOCAL); + } + if !p_sd.is_null() { + LocalFree(p_sd as HLOCAL); + } + return Err(anyhow!("SetNamedSecurityInfoW failed: {}", code3)); } if !p_new_dacl.is_null() { LocalFree(p_new_dacl as HLOCAL); } + } else { + if !p_sd.is_null() { + LocalFree(p_sd as HLOCAL); + } + return Err(anyhow!("SetEntriesInAclW failed: {}", code2)); + } + } + if !p_sd.is_null() { + LocalFree(p_sd as HLOCAL); + } + Ok(added) +} + +/// Adds an allow ACE granting read/write/execute to the given SID on the target path. +/// +/// # Safety +/// Caller must ensure `psid` points to a valid SID and `path` refers to an existing file or directory. +pub unsafe fn add_allow_ace(path: &Path, psid: *mut c_void) -> Result { + 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)); + } + // Already has write? Skip costly DACL rewrite. + if dacl_has_write_allow_for_sid(p_dacl, psid) { + if !p_sd.is_null() { + LocalFree(p_sd as HLOCAL); + } + return Ok(false); + } + let mut added = false; + // Always ensure write is present: if an allow ACE exists without write, add one with write+RX. + 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 | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE; + explicit.grfAccessMode = 2; // SET_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 = !dacl_has_write_allow_for_sid(p_dacl, psid); + } + if !p_new_dacl.is_null() { + LocalFree(p_new_dacl as HLOCAL); } } if !p_sd.is_null() { @@ -217,6 +463,10 @@ pub unsafe fn add_allow_ace(path: &Path, psid: *mut c_void) -> Result { Ok(added) } +/// Adds a deny ACE to prevent write/append/delete for the given SID on the target path. +/// +/// # 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 { let mut p_sd: *mut c_void = std::ptr::null_mut(); let mut p_dacl: *mut ACL = std::ptr::null_mut(); @@ -330,6 +580,10 @@ pub unsafe fn revoke_ace(path: &Path, psid: *mut c_void) { } } +/// Grants RX to the null device for the given SID to support stdout/stderr redirection. +/// +/// # Safety +/// Caller must ensure `psid` is a valid SID pointer. pub unsafe fn allow_null_device(psid: *mut c_void) { let desired = 0x00020000 | 0x00040000; // READ_CONTROL | WRITE_DAC let h = CreateFileW( diff --git a/codex-rs/windows-sandbox-rs/src/audit.rs b/codex-rs/windows-sandbox-rs/src/audit.rs index 6234dbf26af..7e35bf7517b 100644 --- a/codex-rs/windows-sandbox-rs/src/audit.rs +++ b/codex-rs/windows-sandbox-rs/src/audit.rs @@ -1,12 +1,12 @@ use crate::acl::add_deny_write_ace; +use crate::acl::path_quick_mask_allows; use crate::cap::cap_sid_file; use crate::cap::load_or_create_cap_sids; -use crate::logging::log_note; +use crate::logging::{debug_log, 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::HashSet; use std::ffi::c_void; @@ -14,38 +14,10 @@ use std::path::Path; use std::path::PathBuf; use std::time::Duration; use std::time::Instant; -use windows_sys::Win32::Foundation::CloseHandle; -use windows_sys::Win32::Foundation::ERROR_SUCCESS; -use windows_sys::Win32::Foundation::HLOCAL; -use windows_sys::Win32::Foundation::INVALID_HANDLE_VALUE; -use windows_sys::Win32::Foundation::LocalFree; -use windows_sys::Win32::Security::ACCESS_ALLOWED_ACE; -use windows_sys::Win32::Security::ACE_HEADER; -use windows_sys::Win32::Security::ACL; -use windows_sys::Win32::Security::ACL_SIZE_INFORMATION; -use windows_sys::Win32::Security::AclSizeInformation; -use windows_sys::Win32::Security::Authorization::GetNamedSecurityInfoW; -use windows_sys::Win32::Security::Authorization::GetSecurityInfo; -use windows_sys::Win32::Security::DACL_SECURITY_INFORMATION; -use windows_sys::Win32::Security::EqualSid; -use windows_sys::Win32::Security::GetAce; -use windows_sys::Win32::Security::GetAclInformation; -use windows_sys::Win32::Security::MapGenericMask; -use windows_sys::Win32::Security::GENERIC_MAPPING; -use windows_sys::Win32::Storage::FileSystem::CreateFileW; -use windows_sys::Win32::Storage::FileSystem::FILE_ALL_ACCESS; use windows_sys::Win32::Storage::FileSystem::FILE_APPEND_DATA; -use windows_sys::Win32::Storage::FileSystem::FILE_FLAG_BACKUP_SEMANTICS; -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_SHARE_DELETE; -use windows_sys::Win32::Storage::FileSystem::FILE_SHARE_READ; -use windows_sys::Win32::Storage::FileSystem::FILE_SHARE_WRITE; 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::OPEN_EXISTING; // Preflight scan limits const MAX_ITEMS_PER_DIR: i32 = 1000; @@ -109,79 +81,10 @@ fn gather_candidates(cwd: &Path, env: &std::collections::HashMap } unsafe fn path_has_world_write_allow(path: &Path) -> Result { - // Prefer handle-based query (often faster than name-based), fallback to name-based on error - let mut p_sd: *mut c_void = std::ptr::null_mut(); - let mut p_dacl: *mut ACL = std::ptr::null_mut(); - - let mut try_named = false; - let wpath = to_wide(path); - let h = CreateFileW( - wpath.as_ptr(), - 0x00020000, // READ_CONTROL - FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, - std::ptr::null_mut(), - OPEN_EXISTING, - FILE_FLAG_BACKUP_SEMANTICS, - 0, - ); - if h == INVALID_HANDLE_VALUE { - try_named = true; - } else { - let code = GetSecurityInfo( - h, - 1, // SE_FILE_OBJECT - DACL_SECURITY_INFORMATION, - std::ptr::null_mut(), - std::ptr::null_mut(), - &mut p_dacl, - std::ptr::null_mut(), - &mut p_sd, - ); - CloseHandle(h); - if code != ERROR_SUCCESS { - try_named = true; - if !p_sd.is_null() { - LocalFree(p_sd as HLOCAL); - p_sd = std::ptr::null_mut(); - p_dacl = std::ptr::null_mut(); - } - } - } - - if try_named { - let code = GetNamedSecurityInfoW( - wpath.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 { - if !p_sd.is_null() { - LocalFree(p_sd as HLOCAL); - } - return Ok(false); - } - } - let mut world = world_sid()?; let psid_world = world.as_mut_ptr() as *mut c_void; - // Very fast mask-based check for world-writable grants (includes GENERIC_*). - if !dacl_quick_world_write_mask_allows(p_dacl, psid_world) { - if !p_sd.is_null() { - LocalFree(p_sd as HLOCAL); - } - return Ok(false); - } - // Quick detector flagged a write grant for Everyone: treat as writable. - let has = true; - if !p_sd.is_null() { - LocalFree(p_sd as HLOCAL); - } - Ok(has) + let write_mask = FILE_WRITE_DATA | FILE_APPEND_DATA | FILE_WRITE_EA | FILE_WRITE_ATTRIBUTES; + path_quick_mask_allows(path, &[psid_world], write_mask) } pub fn audit_everyone_writable( @@ -193,6 +96,21 @@ pub fn audit_everyone_writable( let mut flagged: Vec = Vec::new(); let mut seen: HashSet = HashSet::new(); let mut checked = 0usize; + let check_world_writable = |path: &Path| -> bool { + match unsafe { path_has_world_write_allow(path) } { + Ok(has) => has, + Err(err) => { + debug_log( + &format!( + "AUDIT: treating unreadable ACL as not world-writable: {} ({err})", + path.display() + ), + logs_base_dir, + ); + false + } + } + }; // Fast path: check CWD immediate children first so workspace issues are caught early. if let Ok(read) = std::fs::read_dir(cwd) { for ent in read.flatten().take(MAX_ITEMS_PER_DIR as usize) { @@ -210,7 +128,7 @@ pub fn audit_everyone_writable( } let p = ent.path(); checked += 1; - let has = unsafe { path_has_world_write_allow(&p)? }; + let has = check_world_writable(&p); if has { let key = normalize_path_key(&p); if seen.insert(key) { @@ -228,7 +146,7 @@ pub fn audit_everyone_writable( break; } checked += 1; - let has_root = unsafe { path_has_world_write_allow(&root)? }; + let has_root = check_world_writable(&root); if has_root { let key = normalize_path_key(&root); if seen.insert(key) { @@ -260,7 +178,7 @@ pub fn audit_everyone_writable( } if ft.is_dir() { checked += 1; - let has_child = unsafe { path_has_world_write_allow(&p)? }; + let has_child = check_world_writable(&p); if has_child { let key = normalize_path_key(&p); if seen.insert(key) { @@ -384,57 +302,3 @@ pub fn apply_capability_denies_for_world_writable( } Ok(()) } -// Fast mask-based check: does the DACL contain any ACCESS_ALLOWED ACE for -// Everyone that grants write after generic bits are expanded? Skips inherit-only -// ACEs (do not apply to the current object). -unsafe fn dacl_quick_world_write_mask_allows(p_dacl: *mut ACL, psid_world: *mut c_void) -> bool { - if p_dacl.is_null() { - return false; - } - const INHERIT_ONLY_ACE: u8 = 0x08; - 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::() as u32, - AclSizeInformation, - ); - if ok == 0 { - return false; - } - let mapping = GENERIC_MAPPING { - GenericRead: FILE_GENERIC_READ, - GenericWrite: FILE_GENERIC_WRITE, - GenericExecute: FILE_GENERIC_EXECUTE, - GenericAll: FILE_ALL_ACCESS, - }; - for i in 0..(info.AceCount as usize) { - let mut p_ace: *mut c_void = std::ptr::null_mut(); - if GetAce(p_dacl as *const ACL, i as u32, &mut p_ace) == 0 { - continue; - } - let hdr = &*(p_ace as *const ACE_HEADER); - if hdr.AceType != 0 { - // ACCESS_ALLOWED_ACE_TYPE - continue; - } - if (hdr.AceFlags & INHERIT_ONLY_ACE) != 0 { - continue; - } - let base = p_ace as usize; - let sid_ptr = - (base + std::mem::size_of::() + std::mem::size_of::()) as *mut c_void; // skip header + mask - if EqualSid(sid_ptr, psid_world) == 0 { - continue; - } - let ace = &*(p_ace as *const ACCESS_ALLOWED_ACE); - let mut mask = ace.Mask; - // Expand generic bits to concrete file rights before checking for write. - MapGenericMask(&mut mask, &mapping); - let write_mask = FILE_WRITE_DATA | FILE_APPEND_DATA | FILE_WRITE_EA | FILE_WRITE_ATTRIBUTES; - if (mask & write_mask) != 0 { - return true; - } - } - false -} diff --git a/codex-rs/windows-sandbox-rs/src/env.rs b/codex-rs/windows-sandbox-rs/src/env.rs index a8a3cda71da..65950a0d595 100644 --- a/codex-rs/windows-sandbox-rs/src/env.rs +++ b/codex-rs/windows-sandbox-rs/src/env.rs @@ -1,11 +1,10 @@ -use anyhow::Result; +use anyhow::{anyhow, Result}; +use dirs_next::home_dir; use std::collections::HashMap; use std::env; -use std::fs::File; -use std::fs::{self}; +use std::fs::{self, File}; use std::io::Write; -use std::path::Path; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; pub fn normalize_null_device_env(env_map: &mut HashMap) { let keys: Vec = env_map.keys().cloned().collect(); @@ -29,6 +28,21 @@ pub fn ensure_non_interactive_pager(env_map: &mut HashMap) { env_map.entry("LESS".into()).or_insert_with(|| "".into()); } +// Keep PATH and PATHEXT stable for callers that rely on inheriting the parent process env. +#[allow(dead_code)] +pub fn inherit_path_env(env_map: &mut HashMap) { + if !env_map.contains_key("PATH") { + if let Ok(path) = env::var("PATH") { + env_map.insert("PATH".into(), path); + } + } + if !env_map.contains_key("PATHEXT") { + if let Ok(pathext) = env::var("PATHEXT") { + env_map.insert("PATHEXT".into(), pathext); + } + } +} + fn prepend_path(env_map: &mut HashMap, prefix: &str) { let existing = env_map .get("PATH") @@ -64,7 +78,7 @@ fn reorder_pathext_for_stubs(env_map: &mut HashMap) { .map(|s| s.to_string()) .collect(); let exts_norm: Vec = exts.iter().map(|e| e.to_ascii_uppercase()).collect(); - let want = [".BAT", ".CMD"]; // move to front if present + let want = [".BAT", ".CMD"]; let mut front: Vec = Vec::new(); for w in want { if let Some(idx) = exts_norm.iter().position(|e| e == w) { @@ -90,7 +104,7 @@ fn ensure_denybin(tools: &[&str], denybin_dir: Option<&Path>) -> Result let base = match denybin_dir { Some(p) => p.to_path_buf(), None => { - let home = dirs_next::home_dir().ok_or_else(|| anyhow::anyhow!("no home dir"))?; + let home = home_dir().ok_or_else(|| anyhow!("no home dir"))?; home.join(".sbx-denybin") } }; @@ -146,16 +160,12 @@ pub fn apply_no_network_to_env(env_map: &mut HashMap) -> Result< .entry("GIT_ALLOW_PROTOCOLS".into()) .or_insert_with(|| "".into()); - // Block interactive network tools that bypass HTTP(S) proxy settings, but - // allow curl/wget to run so commands like `curl --version` still succeed. - // Network access is disabled via proxy envs above. let base = ensure_denybin(&["ssh", "scp"], None)?; - // Clean up any stale stubs from previous runs so real curl/wget can run. for tool in ["curl", "wget"] { for ext in [".bat", ".cmd"] { let p = base.join(format!("{}{}", tool, ext)); if p.exists() { - let _ = std::fs::remove_file(&p); + let _ = fs::remove_file(&p); } } } diff --git a/codex-rs/windows-sandbox-rs/src/logging.rs b/codex-rs/windows-sandbox-rs/src/logging.rs index 9887ce81d24..2e4de1d29a6 100644 --- a/codex-rs/windows-sandbox-rs/src/logging.rs +++ b/codex-rs/windows-sandbox-rs/src/logging.rs @@ -2,9 +2,20 @@ use std::fs::OpenOptions; use std::io::Write; use std::path::Path; use std::path::PathBuf; +use std::sync::OnceLock; const LOG_COMMAND_PREVIEW_LIMIT: usize = 200; -pub const LOG_FILE_NAME: &str = "sandbox_commands.rust.log"; +pub const LOG_FILE_NAME: &str = "sandbox.log"; + +fn exe_label() -> &'static str { + static LABEL: OnceLock = OnceLock::new(); + LABEL.get_or_init(|| { + std::env::current_exe() + .ok() + .and_then(|p| p.file_name().map(|n| n.to_string_lossy().to_string())) + .unwrap_or_else(|| "proc".to_string()) + }) +} fn preview(command: &[String]) -> String { let joined = command.join(" "); @@ -35,17 +46,17 @@ fn append_line(line: &str, base_dir: Option<&Path>) { pub fn log_start(command: &[String], base_dir: Option<&Path>) { let p = preview(command); - append_line(&format!("START: {p}"), base_dir); + log_note(&format!("START: {p}"), base_dir); } pub fn log_success(command: &[String], base_dir: Option<&Path>) { let p = preview(command); - append_line(&format!("SUCCESS: {p}"), base_dir); + log_note(&format!("SUCCESS: {p}"), base_dir); } pub fn log_failure(command: &[String], detail: &str, base_dir: Option<&Path>) { let p = preview(command); - append_line(&format!("FAILURE: {p} ({detail})"), base_dir); + log_note(&format!("FAILURE: {p} ({detail})"), base_dir); } // Debug logging helper. Emits only when SBX_DEBUG=1 to avoid noisy logs. @@ -56,7 +67,8 @@ pub fn debug_log(msg: &str, base_dir: Option<&Path>) { } } -// Unconditional note logging to sandbox_commands.rust.log +// Unconditional note logging to sandbox.log pub fn log_note(msg: &str, base_dir: Option<&Path>) { - append_line(msg, base_dir); + let ts = chrono::Local::now().format("%Y-%m-%d %H:%M:%S%.3f"); + append_line(&format!("[{ts} {}] {}", exe_label(), msg), base_dir); } diff --git a/codex-rs/windows-sandbox-rs/src/process.rs b/codex-rs/windows-sandbox-rs/src/process.rs index 095dcf8b983..9f73e5d0d43 100644 --- a/codex-rs/windows-sandbox-rs/src/process.rs +++ b/codex-rs/windows-sandbox-rs/src/process.rs @@ -79,6 +79,7 @@ fn quote_arg(a: &str) -> String { out.push('"'); out } +#[allow(dead_code)] unsafe fn ensure_inheritable_stdio(si: &mut STARTUPINFOW) -> Result<()> { for kind in [STD_INPUT_HANDLE, STD_OUTPUT_HANDLE, STD_ERROR_HANDLE] { let h = GetStdHandle(kind); @@ -96,12 +97,16 @@ unsafe fn ensure_inheritable_stdio(si: &mut STARTUPINFOW) -> Result<()> { Ok(()) } +/// # Safety +/// Caller must provide a valid primary token handle (`h_token`) with appropriate access, +/// and the `argv`, `cwd`, and `env_map` must remain valid for the duration of the call. pub unsafe fn create_process_as_user( h_token: HANDLE, argv: &[String], cwd: &Path, env_map: &HashMap, logs_base_dir: Option<&Path>, + stdio: Option<(HANDLE, HANDLE, HANDLE)>, ) -> Result<(PROCESS_INFORMATION, STARTUPINFOW)> { let cmdline_str = argv .iter() @@ -117,19 +122,41 @@ pub unsafe fn create_process_as_user( // Point explicitly at the interactive desktop. let desktop = to_wide("Winsta0\\Default"); si.lpDesktop = desktop.as_ptr() as *mut u16; - ensure_inheritable_stdio(&mut si)?; let mut pi: PROCESS_INFORMATION = std::mem::zeroed(); + // Ensure handles are inheritable when custom stdio is supplied. + let inherit_handles = match stdio { + Some((stdin_h, stdout_h, stderr_h)) => { + si.dwFlags |= STARTF_USESTDHANDLES; + si.hStdInput = stdin_h; + si.hStdOutput = stdout_h; + si.hStdError = stderr_h; + for h in [stdin_h, stdout_h, stderr_h] { + if SetHandleInformation(h, HANDLE_FLAG_INHERIT, HANDLE_FLAG_INHERIT) == 0 { + return Err(anyhow!( + "SetHandleInformation failed for stdio handle: {}", + GetLastError() + )); + } + } + true + } + None => { + ensure_inheritable_stdio(&mut si)?; + true + } + }; + let ok = CreateProcessAsUserW( h_token, std::ptr::null(), cmdline.as_mut_ptr(), std::ptr::null_mut(), std::ptr::null_mut(), - 1, + inherit_handles as i32, CREATE_UNICODE_ENVIRONMENT, env_block.as_ptr() as *mut c_void, to_wide(cwd).as_ptr(), - &si, + &mut si, &mut pi, ); if ok == 0 { @@ -149,6 +176,9 @@ pub unsafe fn create_process_as_user( Ok((pi, si)) } +/// # Safety +/// Caller must provide valid process information handles. +#[allow(dead_code)] pub unsafe fn wait_process_and_exitcode(pi: &PROCESS_INFORMATION) -> Result { let res = WaitForSingleObject(pi.hProcess, INFINITE); if res != 0 { @@ -161,6 +191,9 @@ pub unsafe fn wait_process_and_exitcode(pi: &PROCESS_INFORMATION) -> Result Ok(code as i32) } +/// # Safety +/// Caller must close the returned job handle. +#[allow(dead_code)] pub unsafe fn create_job_kill_on_close() -> Result { let h = CreateJobObjectW(std::ptr::null_mut(), std::ptr::null()); if h == 0 { @@ -183,6 +216,9 @@ pub unsafe fn create_job_kill_on_close() -> Result { Ok(h) } +/// # Safety +/// Caller must pass valid handles for a job object and a process. +#[allow(dead_code)] pub unsafe fn assign_to_job(h_job: HANDLE, h_process: HANDLE) -> Result<()> { if AssignProcessToJobObject(h_job, h_process) == 0 { return Err(anyhow!( diff --git a/codex-rs/windows-sandbox-rs/src/token.rs b/codex-rs/windows-sandbox-rs/src/token.rs index 60eae9377f5..7e565bc67a2 100644 --- a/codex-rs/windows-sandbox-rs/src/token.rs +++ b/codex-rs/windows-sandbox-rs/src/token.rs @@ -24,6 +24,7 @@ use windows_sys::Win32::Security::TOKEN_DUPLICATE; use windows_sys::Win32::Security::TOKEN_PRIVILEGES; use windows_sys::Win32::Security::TOKEN_QUERY; use windows_sys::Win32::System::Threading::GetCurrentProcess; +use windows_sys::Win32::System::Threading::OpenProcessToken; const DISABLE_MAX_PRIVILEGE: u32 = 0x01; const LUA_TOKEN: u32 = 0x04; @@ -52,6 +53,8 @@ pub unsafe fn world_sid() -> Result> { Ok(buf) } +/// # Safety +/// Caller is responsible for freeing the returned SID with `LocalFree`. pub unsafe fn convert_string_sid_to_sid(s: &str) -> Option<*mut c_void> { #[link(name = "advapi32")] extern "system" { @@ -66,6 +69,9 @@ pub unsafe fn convert_string_sid_to_sid(s: &str) -> Option<*mut c_void> { } } +/// # Safety +/// Caller must close the returned token handle. +#[allow(dead_code)] pub unsafe fn get_current_token_for_restriction() -> Result { let desired = TOKEN_DUPLICATE | TOKEN_QUERY @@ -197,13 +203,55 @@ unsafe fn enable_single_privilege(h_token: HANDLE, name: &str) -> Result<()> { Ok(()) } -// removed unused create_write_restricted_token_strict +/// # Safety +/// Opens the current process token and adjusts privileges; caller should ensure this is needed in the current context. +#[allow(dead_code)] +pub unsafe fn enable_privilege_on_current(name: &str) -> Result<()> { + let mut h: HANDLE = 0; + let ok = OpenProcessToken( + GetCurrentProcess(), + TOKEN_ADJUST_PRIVILEGES | TOKEN_QUERY, + &mut h, + ); + if ok == 0 { + return Err(anyhow!("OpenProcessToken failed: {}", GetLastError())); + } + let res = enable_single_privilege(h, name); + CloseHandle(h); + res +} +/// # Safety +/// Caller must close the returned token handle. +#[allow(dead_code)] pub unsafe fn create_workspace_write_token_with_cap( psid_capability: *mut c_void, ) -> Result<(HANDLE, *mut c_void)> { let base = get_current_token_for_restriction()?; - let mut logon_sid_bytes = get_logon_sid_bytes(base)?; + let res = create_workspace_write_token_with_cap_from(base, psid_capability); + CloseHandle(base); + res +} + +/// # Safety +/// Caller must close the returned token handle. +#[allow(dead_code)] +pub unsafe fn create_readonly_token_with_cap( + psid_capability: *mut c_void, +) -> Result<(HANDLE, *mut c_void)> { + let base = get_current_token_for_restriction()?; + let res = create_readonly_token_with_cap_from(base, psid_capability); + CloseHandle(base); + res +} + +/// # Safety +/// Caller must close the returned token handle; base_token must be a valid primary token. +pub unsafe fn create_workspace_write_token_with_cap_from( + base_token: HANDLE, + psid_capability: *mut c_void, +) -> Result<(HANDLE, *mut c_void)> { + let mut logon_sid_bytes = get_logon_sid_bytes(base_token)?; let psid_logon = logon_sid_bytes.as_mut_ptr() as *mut c_void; let mut everyone = world_sid()?; let psid_everyone = everyone.as_mut_ptr() as *mut c_void; @@ -218,7 +266,7 @@ pub unsafe fn create_workspace_write_token_with_cap( let mut new_token: HANDLE = 0; let flags = DISABLE_MAX_PRIVILEGE | LUA_TOKEN | WRITE_RESTRICTED; let ok = CreateRestrictedToken( - base, + base_token, flags, 0, std::ptr::null(), @@ -235,11 +283,13 @@ pub unsafe fn create_workspace_write_token_with_cap( Ok((new_token, psid_capability)) } -pub unsafe fn create_readonly_token_with_cap( +/// # Safety +/// Caller must close the returned token handle; base_token must be a valid primary token. +pub unsafe fn create_readonly_token_with_cap_from( + base_token: HANDLE, psid_capability: *mut c_void, ) -> Result<(HANDLE, *mut c_void)> { - let base = get_current_token_for_restriction()?; - let mut logon_sid_bytes = get_logon_sid_bytes(base)?; + let mut logon_sid_bytes = get_logon_sid_bytes(base_token)?; let psid_logon = logon_sid_bytes.as_mut_ptr() as *mut c_void; let mut everyone = world_sid()?; let psid_everyone = everyone.as_mut_ptr() as *mut c_void; @@ -254,7 +304,7 @@ pub unsafe fn create_readonly_token_with_cap( let mut new_token: HANDLE = 0; let flags = DISABLE_MAX_PRIVILEGE | LUA_TOKEN | WRITE_RESTRICTED; let ok = CreateRestrictedToken( - base, + base_token, flags, 0, std::ptr::null(), diff --git a/codex-rs/windows-sandbox-rs/src/winutil.rs b/codex-rs/windows-sandbox-rs/src/winutil.rs index 5e74ce072e0..a8195612610 100644 --- a/codex-rs/windows-sandbox-rs/src/winutil.rs +++ b/codex-rs/windows-sandbox-rs/src/winutil.rs @@ -6,6 +6,7 @@ use windows_sys::Win32::System::Diagnostics::Debug::FormatMessageW; use windows_sys::Win32::System::Diagnostics::Debug::FORMAT_MESSAGE_ALLOCATE_BUFFER; use windows_sys::Win32::System::Diagnostics::Debug::FORMAT_MESSAGE_FROM_SYSTEM; use windows_sys::Win32::System::Diagnostics::Debug::FORMAT_MESSAGE_IGNORE_INSERTS; +use windows_sys::Win32::Security::Authorization::ConvertSidToStringSidW; pub fn to_wide>(s: S) -> Vec { let mut v: Vec = s.as_ref().encode_wide().collect(); @@ -41,3 +42,22 @@ pub fn format_last_error(err: i32) -> String { s } } + +#[allow(dead_code)] +pub fn string_from_sid_bytes(sid: &[u8]) -> Result { + unsafe { + let mut str_ptr: *mut u16 = std::ptr::null_mut(); + let ok = ConvertSidToStringSidW(sid.as_ptr() as *mut std::ffi::c_void, &mut str_ptr); + if ok == 0 || str_ptr.is_null() { + return Err(format!("ConvertSidToStringSidW failed: {}", std::io::Error::last_os_error())); + } + let mut len = 0; + while *str_ptr.add(len) != 0 { + len += 1; + } + let slice = std::slice::from_raw_parts(str_ptr, len); + let out = String::from_utf16_lossy(slice); + let _ = LocalFree(str_ptr as HLOCAL); + Ok(out) + } +}