From d39cfa8a2de2ba6eb57828cd272ec0b7c794d194 Mon Sep 17 00:00:00 2001 From: zm-oai Date: Mon, 14 Sep 2026 22:20:52 +0000 Subject: [PATCH] Harden and share Windows sandbox identity helpers (#45533) ## What changed - Share process package-family queries, token user SID extraction, and account-name lookup through `codex_windows_sandbox`, and use them in the provisioning service. - Bound token query sizes and validate SID pointers, revisions, and lengths before copying SIDs into owned storage. - Use a drop guard to balance firewall COM initialization, and track the package lifecycle directory guard separately so cleanup releases it while keeping ancestor and home handles pinned. ## Testing Add tests that verify copied SIDs outlive their query buffers and reject truncated token data, malformed SIDs, and invalid SID pointers. GitOrigin-RevId: 2b893e4524d3e00cb114df05215beef7b9eebbe9 --- codex-rs/windows-sandbox-rs/Cargo.toml | 1 + codex-rs/windows-sandbox-rs/src/lib.rs | 8 ++ .../src/package_identity.rs | 48 ++++++++ .../src/setup_provisioning/firewall.rs | 52 ++++----- codex-rs/windows-sandbox-rs/src/token_user.rs | 70 +++++++----- .../src/token_user_tests.rs | 66 ++++++++++- codex-rs/windows-sandbox-rs/src/winutil.rs | 34 ++++++ .../src/ipc/authentication.rs | 89 +++------------ .../src/package_identity.rs | 108 ++---------------- .../src/package_lifecycle.rs | 28 ++--- .../src/package_lifecycle/cleanup.rs | 2 +- 11 files changed, 259 insertions(+), 247 deletions(-) create mode 100644 codex-rs/windows-sandbox-rs/src/package_identity.rs diff --git a/codex-rs/windows-sandbox-rs/Cargo.toml b/codex-rs/windows-sandbox-rs/Cargo.toml index 62f969e8ab..b9e5ce694c 100644 --- a/codex-rs/windows-sandbox-rs/Cargo.toml +++ b/codex-rs/windows-sandbox-rs/Cargo.toml @@ -80,6 +80,7 @@ features = [ "Win32_System_Console", "Win32_System_RemoteDesktop", "Win32_Storage_FileSystem", + "Win32_Storage_Packaging_Appx", "Win32_System_Diagnostics_ToolHelp", "Win32_NetworkManagement_NetManagement", "Win32_NetworkManagement_WindowsFilteringPlatform", diff --git a/codex-rs/windows-sandbox-rs/src/lib.rs b/codex-rs/windows-sandbox-rs/src/lib.rs index 26323dec30..ceb39f19f2 100644 --- a/codex-rs/windows-sandbox-rs/src/lib.rs +++ b/codex-rs/windows-sandbox-rs/src/lib.rs @@ -85,6 +85,8 @@ mod logging; #[cfg(target_os = "windows")] mod no_reparse_dir; #[cfg(target_os = "windows")] +mod package_identity; +#[cfg(target_os = "windows")] mod path_normalization; #[cfg(target_os = "windows")] mod process; @@ -303,6 +305,8 @@ pub use no_reparse_dir::open_directory_no_reparse; #[cfg(target_os = "windows")] pub use no_reparse_dir::validate_local_directory_path; #[cfg(target_os = "windows")] +pub use package_identity::process_package_family; +#[cfg(target_os = "windows")] pub use path_normalization::canonicalize_path; #[cfg(target_os = "windows")] pub use process::ConsoleMode; @@ -412,6 +416,8 @@ pub use token::create_workspace_write_token_with_caps_from; #[cfg(target_os = "windows")] pub use token::get_current_token_for_restriction; #[cfg(target_os = "windows")] +pub use token_user::get_user_sid_bytes; +#[cfg(target_os = "windows")] pub use unified_exec::WindowsSandboxSessionRequest; #[cfg(target_os = "windows")] pub use unified_exec::spawn_windows_sandbox_session_elevated_for_permission_profile; @@ -440,6 +446,8 @@ pub use windows_impl::run_windows_sandbox_legacy_preflight; #[cfg(target_os = "windows")] pub use winutil::SANDBOX_USERS_GROUP; #[cfg(target_os = "windows")] +pub use winutil::account_name_from_sid; +#[cfg(target_os = "windows")] pub use winutil::ensure_sandbox_users_group; #[cfg(target_os = "windows")] pub use winutil::local_user_flags; diff --git a/codex-rs/windows-sandbox-rs/src/package_identity.rs b/codex-rs/windows-sandbox-rs/src/package_identity.rs new file mode 100644 index 0000000000..9ee9679756 --- /dev/null +++ b/codex-rs/windows-sandbox-rs/src/package_identity.rs @@ -0,0 +1,48 @@ +//! Queries held process package identities without trusting routing hints or environment state. + +use anyhow::Context; +use anyhow::Result; +use anyhow::bail; +use std::io; +use windows_sys::Win32::Foundation as foundation; +use windows_sys::Win32::Foundation::HANDLE; +use windows_sys::Win32::Storage::Packaging::Appx::GetPackageFamilyName; + +/// Reads a process's OS package family. Absence is not authorization. +/// +/// # Safety +/// The caller must keep the process handle valid throughout the query. +pub unsafe fn process_package_family(process: HANDLE) -> Result> { + let mut length = 0; + let status = unsafe { GetPackageFamilyName(process, &mut length, std::ptr::null_mut()) }; + if status == foundation::APPMODEL_ERROR_NO_PACKAGE { + return Ok(None); + } + if status != foundation::ERROR_INSUFFICIENT_BUFFER { + return Err(io::Error::from_raw_os_error(status as i32)) + .context("query the process package family"); + } + if length == 0 || length > 256 { + bail!("the process package family has an invalid length"); + } + + let mut buffer = vec![0_u16; length as usize]; + let status = unsafe { GetPackageFamilyName(process, &mut length, buffer.as_mut_ptr()) }; + if status != foundation::ERROR_SUCCESS { + return Err(io::Error::from_raw_os_error(status as i32)) + .context("read the process package family"); + } + + let value = buffer + .get(..length as usize) + .context("the package-family API returned an invalid length")?; + let Some((&0, value)) = value.split_last() else { + bail!("the process package family is not null-terminated"); + }; + if value.is_empty() || value.contains(&0) { + bail!("the process package family is malformed"); + } + Ok(Some( + String::from_utf16(value).context("the package family contains invalid UTF-16")?, + )) +} diff --git a/codex-rs/windows-sandbox-rs/src/setup_provisioning/firewall.rs b/codex-rs/windows-sandbox-rs/src/setup_provisioning/firewall.rs index bb7c0fced0..8359f25d2f 100644 --- a/codex-rs/windows-sandbox-rs/src/setup_provisioning/firewall.rs +++ b/codex-rs/windows-sandbox-rs/src/setup_provisioning/firewall.rs @@ -58,6 +58,28 @@ struct BlockRuleSpec<'a> { remote_ports: Option<&'a str>, } +// Balance successful COM initialization on every return path. +struct FirewallComApartment; + +impl FirewallComApartment { + fn initialize() -> Result { + let hr = unsafe { CoInitializeEx(None, COINIT_APARTMENTTHREADED) }; + if hr.is_err() { + return Err(anyhow::Error::new(SetupFailure::new( + SetupErrorCode::HelperFirewallComInitFailed, + format!("CoInitializeEx failed: {hr:?}"), + ))); + } + Ok(Self) + } +} + +impl Drop for FirewallComApartment { + fn drop(&mut self) { + unsafe { CoUninitialize() }; + } +} + pub fn ensure_offline_proxy_allowlist( offline_sid: &str, proxy_ports: &[u16], @@ -66,15 +88,9 @@ pub fn ensure_offline_proxy_allowlist( ) -> Result<()> { let local_user_spec = format!("O:LSD:(A;;CC;;;{offline_sid})"); - let hr = unsafe { CoInitializeEx(None, COINIT_APARTMENTTHREADED) }; - if hr.is_err() { - return Err(anyhow::Error::new(SetupFailure::new( - SetupErrorCode::HelperFirewallComInitFailed, - format!("CoInitializeEx failed: {hr:?}"), - ))); - } + let _apartment = FirewallComApartment::initialize()?; - let result = unsafe { + unsafe { (|| -> Result<()> { let policy: INetFwPolicy2 = CoCreateInstance(&NetFwPolicy2, None, CLSCTX_INPROC_SERVER) .map_err(|err| { @@ -154,26 +170,15 @@ pub fn ensure_offline_proxy_allowlist( } Ok(()) })() - }; - - unsafe { - CoUninitialize(); } - result } pub fn ensure_offline_network_blocks(offline_sid: &str, log: &mut dyn Write) -> Result<()> { let local_user_spec = format!("O:LSD:(A;;CC;;;{offline_sid})"); - let hr = unsafe { CoInitializeEx(None, COINIT_APARTMENTTHREADED) }; - if hr.is_err() { - return Err(anyhow::Error::new(SetupFailure::new( - SetupErrorCode::HelperFirewallComInitFailed, - format!("CoInitializeEx failed: {hr:?}"), - ))); - } + let _apartment = FirewallComApartment::initialize()?; - let result = unsafe { + unsafe { (|| -> Result<()> { let policy: INetFwPolicy2 = CoCreateInstance(&NetFwPolicy2, None, CLSCTX_INPROC_SERVER) .map_err(|err| { @@ -221,12 +226,7 @@ pub fn ensure_offline_network_blocks(offline_sid: &str, log: &mut dyn Write) -> )?; Ok(()) })() - }; - - unsafe { - CoUninitialize(); } - result } fn remove_rule_if_present( diff --git a/codex-rs/windows-sandbox-rs/src/token_user.rs b/codex-rs/windows-sandbox-rs/src/token_user.rs index 2b511b5d40..3c8689ddf8 100644 --- a/codex-rs/windows-sandbox-rs/src/token_user.rs +++ b/codex-rs/windows-sandbox-rs/src/token_user.rs @@ -1,22 +1,27 @@ -//! Owns the token-user SID query used by sandbox token construction. +//! Copies token user identities with bounded Windows queries and owned SID storage. use anyhow::Result; use anyhow::anyhow; +use anyhow::ensure; use std::ffi::c_void; use windows_sys::Win32::Foundation::GetLastError; use windows_sys::Win32::Foundation::HANDLE; -use windows_sys::Win32::Security::CopySid; -use windows_sys::Win32::Security::GetLengthSid; use windows_sys::Win32::Security::GetTokenInformation; +use windows_sys::Win32::Security::IsValidSid; use windows_sys::Win32::Security::TOKEN_USER; use windows_sys::Win32::Security::TokenUser; -pub(crate) unsafe fn get_user_sid_bytes(h_token: HANDLE) -> Result> { +/// Copies a token's user SID into owned storage after a bounded TokenUser query. +/// +/// # Safety +/// The caller must keep a token handle with TOKEN_QUERY access valid during this call. +pub unsafe fn get_user_sid_bytes(h_token: HANDLE) -> Result> { let mut needed: u32 = 0; GetTokenInformation(h_token, TokenUser, std::ptr::null_mut(), 0, &mut needed); - if needed == 0 { - return Err(anyhow!("TokenUser size query returned 0")); - } + ensure!( + needed as usize >= std::mem::size_of::() && needed <= 4096, + "invalid TokenUser query size" + ); let mut user_buf: Vec = vec![0u8; needed as usize]; let ok = GetTokenInformation( h_token, @@ -25,30 +30,43 @@ pub(crate) unsafe fn get_user_sid_bytes(h_token: HANDLE) -> Result> { needed, &mut needed, ); - if ok == 0 || (needed as usize) < std::mem::size_of::() { + if ok == 0 { return Err(anyhow!( "GetTokenInformation(TokenUser) failed: {}", GetLastError() )); } - let token_user: TOKEN_USER = std::ptr::read_unaligned(user_buf.as_ptr() as *const TOKEN_USER); - let sid_len = GetLengthSid(token_user.User.Sid); - if sid_len == 0 { - return Err(anyhow!( - "GetLengthSid(TokenUser) failed: {}", - GetLastError() - )); - } - let mut user_sid_bytes = vec![0u8; sid_len as usize]; - if CopySid( - sid_len, - user_sid_bytes.as_mut_ptr() as *mut c_void, - token_user.User.Sid, - ) == 0 - { - return Err(anyhow!("CopySid(TokenUser) failed: {}", GetLastError())); - } - Ok(user_sid_bytes) + ensure!( + needed as usize <= user_buf.len(), + "invalid TokenUser result size" + ); + decode_token_user(&user_buf[..needed as usize]) +} + +fn decode_token_user(buffer: &[u8]) -> Result> { + ensure!( + buffer.len() >= std::mem::size_of::(), + "truncated TokenUser" + ); + let user = unsafe { std::ptr::read_unaligned(buffer.as_ptr().cast::()) }; + copy_token_sid(buffer, user.User.Sid) +} + +fn copy_token_sid(buffer: &[u8], pointer: *mut c_void) -> Result> { + // Bound the header and every subauthority before calling a SID API. + let offset = (pointer as usize).wrapping_sub(buffer.as_ptr() as usize); + ensure!( + buffer.len() >= 8 && offset <= buffer.len() - 8, + "invalid token SID pointer" + ); + ensure!(buffer[offset] == 1, "invalid token SID revision"); + let length = 8 + usize::from(buffer[offset + 1]) * 4; + ensure!( + length <= 68 && length <= buffer.len() - offset, + "invalid token SID size" + ); + ensure!(unsafe { IsValidSid(pointer) } != 0, "invalid token SID"); + Ok(buffer[offset..offset + length].to_vec()) } #[cfg(test)] diff --git a/codex-rs/windows-sandbox-rs/src/token_user_tests.rs b/codex-rs/windows-sandbox-rs/src/token_user_tests.rs index b062d52e6e..15a7c1e58a 100644 --- a/codex-rs/windows-sandbox-rs/src/token_user_tests.rs +++ b/codex-rs/windows-sandbox-rs/src/token_user_tests.rs @@ -1,15 +1,71 @@ -//! Checks the existing token-user query against real process and invalid handles. +//! Verifies owned token-user SID results and rejects malformed query buffers. -use super::get_user_sid_bytes; -use anyhow::Result; -use anyhow::ensure; +use super::*; +use crate::token::world_sid; +use pretty_assertions::assert_eq; use std::os::windows::io::FromRawHandle; use std::os::windows::io::OwnedHandle; -use windows_sys::Win32::Security::IsValidSid; +use windows_sys::Win32::Security::SID_AND_ATTRIBUTES; use windows_sys::Win32::Security::TOKEN_QUERY; use windows_sys::Win32::System::Threading::GetCurrentProcess; use windows_sys::Win32::System::Threading::OpenProcessToken; +fn user_buffer(sid: &[u8]) -> Vec { + let offset = std::mem::size_of::(); + let mut buffer = vec![0; offset + sid.len()]; + buffer[offset..].copy_from_slice(sid); + unsafe { + let user = TOKEN_USER { + User: SID_AND_ATTRIBUTES { + Sid: buffer.as_mut_ptr().add(offset).cast(), + Attributes: 0, + }, + }; + std::ptr::write_unaligned(buffer.as_mut_ptr().cast::(), user); + } + buffer +} + +#[test] +fn user_sid_is_owned_after_the_query_buffer_is_dropped() -> Result<()> { + let expected = unsafe { world_sid() }?; + let actual = { + let buffer = user_buffer(&expected); + decode_token_user(&buffer)? + }; + assert_eq!(actual, expected); + Ok(()) +} + +#[test] +fn rejects_truncated_user_and_malformed_sid() -> Result<()> { + assert!(decode_token_user(&[]).is_err()); + let mut buffer = user_buffer(&unsafe { world_sid() }?); + let offset = std::mem::size_of::(); + buffer[offset] = 2; + assert!(decode_token_user(&buffer).is_err()); + buffer[offset] = 1; + buffer[offset + 1] = 15; + assert!(decode_token_user(&buffer).is_err()); + for pointer in [std::ptr::null_mut(), unsafe { + buffer.as_mut_ptr().add(buffer.len() - 1).cast() + }] { + unsafe { + std::ptr::write_unaligned( + buffer.as_mut_ptr().cast::(), + TOKEN_USER { + User: SID_AND_ATTRIBUTES { + Sid: pointer, + Attributes: 0, + }, + }, + ); + } + assert!(decode_token_user(&buffer).is_err()); + } + Ok(()) +} + #[test] fn queries_current_user_and_rejects_invalid_token() -> Result<()> { let mut raw = 0; diff --git a/codex-rs/windows-sandbox-rs/src/winutil.rs b/codex-rs/windows-sandbox-rs/src/winutil.rs index 502d9024bf..0899362684 100644 --- a/codex-rs/windows-sandbox-rs/src/winutil.rs +++ b/codex-rs/windows-sandbox-rs/src/winutil.rs @@ -35,6 +35,40 @@ pub fn to_wide>(s: S) -> Vec { v } +/// Resolve a named Windows user without consulting environment variables. +/// +/// # Safety +/// `sid` must point to a valid SID for the duration of this call. +pub unsafe fn account_name_from_sid(sid: *mut std::ffi::c_void) -> Result { + use windows_sys::Win32::Security as security; + let mut name = [0_u16; 256]; + let mut domain = [0_u16; 256]; + let mut name_length = name.len() as u32; + let mut domain_length = domain.len() as u32; + let mut account_type: security::SID_NAME_USE = 0; + if unsafe { + security::LookupAccountSidW( + std::ptr::null(), + sid, + name.as_mut_ptr(), + &mut name_length, + domain.as_mut_ptr(), + &mut domain_length, + &mut account_type, + ) + } == 0 + { + return Err(std::io::Error::last_os_error()).context("resolve Windows account name"); + } + anyhow::ensure!( + account_type == security::SidTypeUser && domain_length > 0, + "SID is not a named Windows user" + ); + let name = String::from_utf16(&name[..name_length as usize])?; + let domain = String::from_utf16(&domain[..domain_length as usize])?; + Ok(format!("{domain}\\{name}")) +} + /// Quote a single Windows command-line argument following the rules used by /// CommandLineToArgvW/CRT so that spaces, quotes, and backslashes are preserved. /// Reference behavior matches Rust std::process::Command on Windows. diff --git a/codex-rs/windows-sandbox-service/src/ipc/authentication.rs b/codex-rs/windows-sandbox-service/src/ipc/authentication.rs index d9c34980ed..5d678fb965 100644 --- a/codex-rs/windows-sandbox-service/src/ipc/authentication.rs +++ b/codex-rs/windows-sandbox-service/src/ipc/authentication.rs @@ -13,7 +13,6 @@ use std::os::windows::ffi::OsStringExt; use std::os::windows::io::BorrowedHandle; use std::os::windows::io::IntoRawHandle; use std::path::PathBuf; -use std::ptr; use windows_sys::Win32::Foundation::HANDLE; use windows_sys::Win32::Security as security; use windows_sys::Win32::Storage::FileSystem as filesystem; @@ -44,13 +43,12 @@ pub(super) fn authenticate_client( std::thread::scope(|scope| { scope .spawn(|| { - if unsafe { pipes::ImpersonateNamedPipeClient(pipe) } == 0 { - return Err(std::io::Error::last_os_error()) - .context("impersonate provisioning client"); - } - - let identity = - authenticate_impersonated_client(authorized_process, sandbox_sid, request)?; + let identity = authenticate_impersonated_client( + pipe, + authorized_process, + sandbox_sid, + request, + )?; let policy_result = match request { // Registration does not provision resources or change sandbox policy. ServiceRequest::RegisterInstallation { .. } => Ok(()), @@ -71,10 +69,14 @@ pub(super) fn authenticate_client( } fn authenticate_impersonated_client( + pipe: HANDLE, authorized_process: &crate::package_identity::AuthorizedClientProcess, sandbox_sid: &[u8], request: &ServiceRequest, ) -> Result { + if unsafe { pipes::ImpersonateNamedPipeClient(pipe) } == 0 { + return Err(std::io::Error::last_os_error()).context("impersonate provisioning client"); + } let mut raw_token = 0; if unsafe { threading::OpenThreadToken( @@ -123,44 +125,11 @@ fn authenticate_impersonated_client( bail!("sandbox accounts cannot request provisioning"); } - let mut length = 0; - unsafe { - security::GetTokenInformation( - token.0, - security::TokenUser, - ptr::null_mut(), - 0, - &mut length, - ) - }; - if length < size_of::() as u32 { - return Err(std::io::Error::last_os_error()).context("size provisioning client identity"); - } - let mut user = vec![0_u8; length as usize]; - if unsafe { - security::GetTokenInformation( - token.0, - security::TokenUser, - user.as_mut_ptr().cast(), - length, - &mut length, - ) - } == 0 - { - return Err(std::io::Error::last_os_error()).context("read provisioning client identity"); - } - let sid = unsafe { ptr::read_unaligned(user.as_ptr().cast::()) } - .User - .Sid; - let sid_length = unsafe { security::GetLengthSid(sid) }; - if sid_length == 0 { - return Err(std::io::Error::last_os_error()).context("size provisioning client SID"); - } - let user_sid = string_from_sid_bytes(unsafe { - std::slice::from_raw_parts(sid.cast::(), sid_length as usize) - }) - .map_err(anyhow::Error::msg)?; - let account = account_name(sid)?; + let user = unsafe { codex_windows_sandbox::get_user_sid_bytes(token.0) } + .context("read provisioning client identity")?; + let user_sid = string_from_sid_bytes(&user).map_err(anyhow::Error::msg)?; + let account = unsafe { codex_windows_sandbox::account_name_from_sid(user.as_ptr() as _) } + .context("resolve provisioning client account")?; let requested_home = match request { ServiceRequest::RegisterInstallation { codex_home } => codex_home, ServiceRequest::ProvisionSandbox(request) => &request.codex_home, @@ -231,31 +200,3 @@ fn authenticate_impersonated_client( directory_handles: handles, }) } - -fn account_name(sid: *mut c_void) -> Result { - let mut name = [0_u16; 256]; - let mut domain = [0_u16; 256]; - let mut name_length = name.len() as u32; - let mut domain_length = domain.len() as u32; - let mut account_type: security::SID_NAME_USE = 0; - if unsafe { - security::LookupAccountSidW( - std::ptr::null(), - sid, - name.as_mut_ptr(), - &mut name_length, - domain.as_mut_ptr(), - &mut domain_length, - &mut account_type, - ) - } == 0 - { - return Err(std::io::Error::last_os_error()).context("resolve provisioning client name"); - } - if account_type != security::SidTypeUser || domain_length == 0 { - bail!("provisioning client is not a named Windows user"); - } - let name = String::from_utf16(&name[..name_length as usize])?; - let domain = String::from_utf16(&domain[..domain_length as usize])?; - Ok(format!("{domain}\\{name}")) -} diff --git a/codex-rs/windows-sandbox-service/src/package_identity.rs b/codex-rs/windows-sandbox-service/src/package_identity.rs index 53a12410b7..b37a8cd114 100644 --- a/codex-rs/windows-sandbox-service/src/package_identity.rs +++ b/codex-rs/windows-sandbox-service/src/package_identity.rs @@ -1,8 +1,6 @@ //! Binds provisioning requests to the packaged Codex client and its Windows user. use std::io; -use std::mem::size_of; -use std::ptr; #[cfg(debug_assertions)] use std::sync::atomic::AtomicBool; #[cfg(debug_assertions)] @@ -14,13 +12,9 @@ use anyhow::bail; use windows_sys::Win32::Foundation as foundation; use windows_sys::Win32::Foundation::HANDLE; use windows_sys::Win32::Security as security; -use windows_sys::Win32::Storage::Packaging::Appx; use windows_sys::Win32::System::Pipes; use windows_sys::Win32::System::Threading; -const MAX_PACKAGE_FAMILY_LENGTH: usize = 256; -const MAX_TOKEN_USER_BYTES: usize = 4096; - #[cfg(debug_assertions)] static FOREGROUND_MODE: AtomicBool = AtomicBool::new(false); @@ -58,15 +52,11 @@ pub(crate) fn authorize_client_process(pipe: HANDLE) -> Result match service_family.as_deref() { Some(service_family) if client_family != service_family => { @@ -104,95 +94,17 @@ pub(crate) fn authorize_client( .context("open the provisioning client process token"); } let process_token = OwnedHandle(process_token); - let process_user = token_user(process_token.0).context("read the client process user")?; - let impersonated_user = - token_user(client_token).context("read the impersonated client user")?; - let process_sid = unsafe { - ptr::read_unaligned(process_user.as_ptr().cast::()) - .User - .Sid - }; - let impersonated_sid = unsafe { - ptr::read_unaligned(impersonated_user.as_ptr().cast::()) - .User - .Sid - }; - if process_sid.is_null() - || impersonated_sid.is_null() - || unsafe { security::EqualSid(process_sid, impersonated_sid) } == 0 - { + let process_user = unsafe { codex_windows_sandbox::get_user_sid_bytes(process_token.0) } + .context("read the client process user")?; + let impersonated_user = unsafe { codex_windows_sandbox::get_user_sid_bytes(client_token) } + .context("read the impersonated client user")?; + if process_user != impersonated_user { bail!("provisioning client process does not belong to the impersonated user"); } Ok(()) } -fn package_family_name( - mut query: impl FnMut(*mut u32, *mut u16) -> u32, - subject: &str, -) -> Result> { - let mut length = 0; - let status = query(&mut length, ptr::null_mut()); - if status == foundation::APPMODEL_ERROR_NO_PACKAGE { - return Ok(None); - } - if status != foundation::ERROR_INSUFFICIENT_BUFFER { - return Err(io::Error::from_raw_os_error(status as i32)) - .with_context(|| format!("query the {subject} package family")); - } - if length == 0 || length as usize > MAX_PACKAGE_FAMILY_LENGTH { - bail!("the {subject} package family has an invalid length"); - } - - let mut buffer = vec![0_u16; length as usize]; - let status = query(&mut length, buffer.as_mut_ptr()); - if status != foundation::ERROR_SUCCESS { - return Err(io::Error::from_raw_os_error(status as i32)) - .with_context(|| format!("read the {subject} package family")); - } - - let value = buffer - .get(..length as usize) - .context("the package-family API returned an invalid length")?; - let Some((&0, value)) = value.split_last() else { - bail!("the {subject} package family is not null-terminated"); - }; - if value.is_empty() || value.contains(&0) { - bail!("the {subject} package family is malformed"); - } - Ok(Some( - String::from_utf16(value).context("the package family contains invalid UTF-16")?, - )) -} - -pub(crate) fn token_user(token: HANDLE) -> Result> { - let mut length = 0; - unsafe { - security::GetTokenInformation(token, security::TokenUser, ptr::null_mut(), 0, &mut length) - }; - if length < size_of::() as u32 || length as usize > MAX_TOKEN_USER_BYTES { - bail!("the Windows token returned an invalid user identity length"); - } - - let mut buffer = vec![0_u8; length as usize]; - if unsafe { - security::GetTokenInformation( - token, - security::TokenUser, - buffer.as_mut_ptr().cast(), - length, - &mut length, - ) - } == 0 - { - return Err(io::Error::last_os_error()).context("read the Windows token user"); - } - if length < size_of::() as u32 || length as usize > buffer.len() { - bail!("the Windows token returned a malformed user identity"); - } - Ok(buffer) -} - #[cfg(debug_assertions)] fn is_known_codex_package_family(package_family: &str) -> bool { matches!( diff --git a/codex-rs/windows-sandbox-service/src/package_lifecycle.rs b/codex-rs/windows-sandbox-service/src/package_lifecycle.rs index 1117837e7b..38ea581660 100644 --- a/codex-rs/windows-sandbox-service/src/package_lifecycle.rs +++ b/codex-rs/windows-sandbox-service/src/package_lifecycle.rs @@ -39,8 +39,9 @@ mod cleanup; struct UserInstallation { codex_home: Option, - // Ancestors, home, then a guard that prevents in-place junction conversion. + // Ancestors and home remain pinned until owner-scoped cleanup finishes. directory_handles: Vec, + directory_guard: Option, record: InstallationRecord, user_token: OwnedHandle, catalog: PackageCatalog, @@ -98,6 +99,7 @@ impl PackageLifecycle { let saved_record = record.clone(); with_owner_impersonation(user_token.0, || { let mut directory_handles = Vec::new(); + let mut directory_guard = None; let codex_home = match crate::ipc::pin_existing_ancestors( &record.codex_home, &mut directory_handles, @@ -114,7 +116,7 @@ impl PackageLifecycle { filesystem::FILE_READ_ATTRIBUTES, DirectoryOpenDisposition::OpenExisting, )?); - directory_handles.push(OwnedHandle(guard.into_raw_handle() as HANDLE)); + directory_guard = Some(OwnedHandle(guard.into_raw_handle() as HANDLE)); Ok(()) }) { Ok(()) => Some(record.codex_home.clone()), @@ -132,6 +134,7 @@ impl PackageLifecycle { if let Some(installation) = active.as_mut() { installation.codex_home = codex_home; installation.directory_handles = directory_handles; + installation.directory_guard = directory_guard; installation.record = record; return Ok(()); } @@ -158,6 +161,7 @@ impl PackageLifecycle { active.replace(UserInstallation { codex_home, directory_handles, + directory_guard, record, user_token, catalog, @@ -209,18 +213,8 @@ impl PackageLifecycle { return Err(io::Error::last_os_error()).context("open the logged-in user's token"); } let token = crate::ipc::OwnedHandle(raw_token); - let user = crate::package_identity::token_user(token.0)?; - let sid = unsafe { std::ptr::read_unaligned(user.as_ptr().cast::()) } - .User - .Sid; - let sid_length = unsafe { security::GetLengthSid(sid) }; - if sid_length == 0 { - return Err(io::Error::last_os_error()).context("read the logged-in user's SID"); - } - let user_sid = string_from_sid_bytes(unsafe { - std::slice::from_raw_parts(sid.cast::(), sid_length as usize) - }) - .map_err(anyhow::Error::msg)?; + let user = unsafe { codex_windows_sandbox::get_user_sid_bytes(token.0) }?; + let user_sid = string_from_sid_bytes(&user).map_err(anyhow::Error::msg)?; ensure!( user_sid == record.user_sid, "logged-in user does not match the recorded sandbox owner" @@ -245,10 +239,10 @@ impl PackageLifecycle { } } -fn with_owner_impersonation( +pub(crate) fn with_owner_impersonation( user_token: HANDLE, - operation: impl FnOnce() -> Result<()>, -) -> Result<()> { + operation: impl FnOnce() -> Result, +) -> Result { if unsafe { security::ImpersonateLoggedOnUser(user_token) } == 0 { return Err(io::Error::last_os_error()).context("impersonate the sandbox owner"); } diff --git a/codex-rs/windows-sandbox-service/src/package_lifecycle/cleanup.rs b/codex-rs/windows-sandbox-service/src/package_lifecycle/cleanup.rs index de552b9b8f..b879369ed7 100644 --- a/codex-rs/windows-sandbox-service/src/package_lifecycle/cleanup.rs +++ b/codex-rs/windows-sandbox-service/src/package_lifecycle/cleanup.rs @@ -23,7 +23,7 @@ pub(super) fn clean_up(installation: &mut UserInstallation) -> Result<()> { let codex_home = installation.codex_home.clone(); clean_up_packaged_windows_sandbox(codex_home.as_deref(), || { // Privileged file cleanup is finished; release the guard before owner-scoped removal. - installation.directory_handles.pop(); + installation.directory_guard.take(); let Some(desktop) = &installation.record.desktop_installation else { return Ok(()); };