diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 1743ce5901..b24fc38fd5 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -5220,6 +5220,7 @@ dependencies = [ "codex-config", "codex-core", "codex-windows-sandbox", + "pretty_assertions", "tokio", "toml 0.9.11+spec-1.1.0", "windows 0.58.0", diff --git a/codex-rs/app-server/src/request_processors/initialize_processor.rs b/codex-rs/app-server/src/request_processors/initialize_processor.rs index 0702c4418d..493867c5a5 100644 --- a/codex-rs/app-server/src/request_processors/initialize_processor.rs +++ b/codex-rs/app-server/src/request_processors/initialize_processor.rs @@ -174,6 +174,22 @@ impl InitializeRequestProcessor { *suffix = Some(user_agent_suffix); } + #[cfg(windows)] + if matches!(session.origin, ConnectionOrigin::Stdio) && name == "Codex Desktop" { + // Uninstall ownership must not depend on account sign-in or sandbox setup. + // Keep this bounded attempt ahead of the response; background registration can race uninstall. + let home = codex_home.clone(); + if !matches!( + tokio::task::spawn_blocking(move || { + codex_windows_sandbox::register_desktop_installation(&home) + }) + .await, + Ok(Ok(())) + ) { + tracing::warn!("could not register desktop uninstall ownership"); + } + } + let user_agent = get_codex_user_agent(); let response = InitializeResponse { user_agent, diff --git a/codex-rs/windows-sandbox-rs/src/acl.rs b/codex-rs/windows-sandbox-rs/src/acl.rs index 064b7fe6fb..ace264360d 100644 --- a/codex-rs/windows-sandbox-rs/src/acl.rs +++ b/codex-rs/windows-sandbox-rs/src/acl.rs @@ -899,7 +899,11 @@ pub unsafe fn add_deny_read_ace(path: &Path, psid: *mut c_void) -> Result #[path = "acl_tests.rs"] mod tests; -pub unsafe fn revoke_ace(path: &Path, psid: *mut c_void) { +/// Removes explicit ACEs for one SID and propagates the updated inherited ACL. +/// +/// # Safety +/// Caller must pass a valid SID pointer and have authority to edit the target DACL. +pub unsafe fn revoke_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( @@ -916,7 +920,14 @@ pub unsafe fn revoke_ace(path: &Path, psid: *mut c_void) { if !p_sd.is_null() { LocalFree(p_sd as HLOCAL); } - return; + return acl_api_result(path, "GetNamedSecurityInfoW", code); + } + if p_dacl.is_null() { + // A null DACL has no ACE to revoke; replacing it with an empty ACL would deny access. + if !p_sd.is_null() { + LocalFree(p_sd as HLOCAL); + } + return Ok(()); } let trustee = TRUSTEE_W { pMultipleTrustee: std::ptr::null_mut(), @@ -931,9 +942,17 @@ pub unsafe fn revoke_ace(path: &Path, psid: *mut c_void) { 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 _ = SetNamedSecurityInfoW( + let result = acl_api_result( + path, + "SetEntriesInAclW", + SetEntriesInAclW(1, &explicit, p_dacl, &mut p_new_dacl), + ) + .and_then(|()| { + // REVOKE_ACCESS only removes ACEs. An unchanged ACL must not propagate inheritance. + if (*p_new_dacl).AceCount == (*p_dacl).AceCount { + return Ok(()); + } + let code = SetNamedSecurityInfoW( to_wide(path).as_ptr() as *mut u16, 1, DACL_SECURITY_INFORMATION, @@ -942,13 +961,15 @@ pub unsafe fn revoke_ace(path: &Path, psid: *mut c_void) { p_new_dacl, std::ptr::null_mut(), ); - if !p_new_dacl.is_null() { - LocalFree(p_new_dacl as HLOCAL); - } + acl_api_result(path, "SetNamedSecurityInfoW", code) + }); + if !p_new_dacl.is_null() { + LocalFree(p_new_dacl as HLOCAL); } if !p_sd.is_null() { LocalFree(p_sd as HLOCAL); } + result } /// Grants RX to the null device for the given SID to support stdout/stderr redirection. diff --git a/codex-rs/windows-sandbox-rs/src/acl_tests.rs b/codex-rs/windows-sandbox-rs/src/acl_tests.rs index 597df03ced..4a1cd62444 100644 --- a/codex-rs/windows-sandbox-rs/src/acl_tests.rs +++ b/codex-rs/windows-sandbox-rs/src/acl_tests.rs @@ -6,6 +6,15 @@ use pretty_assertions::assert_eq; use std::fs::OpenOptions; use std::os::windows::fs::OpenOptionsExt; use windows_sys::Win32::Foundation::ERROR_ACCESS_DENIED; +use windows_sys::Win32::Foundation::HLOCAL; +use windows_sys::Win32::Foundation::LocalFree; +use windows_sys::Win32::Security::Authorization::ConvertStringSecurityDescriptorToSecurityDescriptorW; +use windows_sys::Win32::Security::Authorization::SDDL_REVISION_1; +use windows_sys::Win32::Security::DACL_SECURITY_INFORMATION; +use windows_sys::Win32::Security::GetSecurityDescriptorControl; +use windows_sys::Win32::Security::SE_DACL_PROTECTED; +use windows_sys::Win32::Security::SetFileSecurityW; +use windows_sys::Win32::Security::UNPROTECTED_DACL_SECURITY_INFORMATION; use windows_sys::Win32::Storage::FileSystem::FILE_FLAG_BACKUP_SEMANTICS; use windows_sys::Win32::Storage::FileSystem::READ_CONTROL; @@ -53,3 +62,51 @@ fn existing_deny_ace_is_visible_without_write_dac() { .expect("read existing deny ACE"); assert!(already_present); } + +#[test] +fn revoking_absent_sid_preserves_child_null_dacl() { + let parent = tempfile::tempdir().expect("parent directory"); + let child = parent.path().join("child"); + std::fs::create_dir(&child).expect("child directory"); + let sid = LocalSid::from_string("S-1-5-21-10-20-30-40").expect("absent SID"); + let other_sid = LocalSid::from_string("S-1-5-21-10-20-30-41").expect("inherited SID"); + + unsafe { + super::add_allow_ace(parent.path(), other_sid.as_ptr()).expect("inheritable parent ACE"); + let mut descriptor = std::ptr::null_mut(); + assert_ne!( + ConvertStringSecurityDescriptorToSecurityDescriptorW( + crate::winutil::to_wide("D:NO_ACCESS_CONTROL").as_ptr(), + SDDL_REVISION_1, + &mut descriptor, + std::ptr::null_mut(), + ), + 0, + ); + // The legacy setter preserves a null DACL without applying automatic inheritance. + let set = SetFileSecurityW( + crate::winutil::to_wide(&child).as_ptr(), + DACL_SECURITY_INFORMATION | UNPROTECTED_DACL_SECURITY_INFORMATION, + descriptor, + ); + LocalFree(descriptor as HLOCAL); + assert_ne!(set, 0, "set an unprotected null DACL"); + for revoke in [false, true] { + if revoke { + super::revoke_ace(parent.path(), sid.as_ptr()).expect("revoke absent SID"); + } + let (dacl, descriptor) = super::fetch_dacl_handle(&child).expect("child permissions"); + let mut control = 0; + let mut revision = 0; + let valid = GetSecurityDescriptorControl(descriptor, &mut control, &mut revision); + LocalFree(descriptor as HLOCAL); + + assert_ne!(valid, 0, "read child inheritance flags"); + assert_eq!(control & SE_DACL_PROTECTED, 0, "child permits inheritance"); + assert!( + dacl.is_null(), + "revocation must preserve the child's null DACL" + ); + } + } +} diff --git a/codex-rs/windows-sandbox-rs/src/deny_read_acl.rs b/codex-rs/windows-sandbox-rs/src/deny_read_acl.rs index fd1dd1b7f3..06eda3b6ef 100644 --- a/codex-rs/windows-sandbox-rs/src/deny_read_acl.rs +++ b/codex-rs/windows-sandbox-rs/src/deny_read_acl.rs @@ -81,7 +81,7 @@ pub unsafe fn apply_deny_read_acls(paths: &[PathBuf], psid: *mut c_void) -> Resu Ok(added) => added, Err(err) => { for added_path in &added_in_this_call { - revoke_ace(added_path, psid); + let _ = revoke_ace(added_path, psid); } return Err(err); } diff --git a/codex-rs/windows-sandbox-rs/src/deny_read_state.rs b/codex-rs/windows-sandbox-rs/src/deny_read_state.rs index fd073a4d45..274ff004e9 100644 --- a/codex-rs/windows-sandbox-rs/src/deny_read_state.rs +++ b/codex-rs/windows-sandbox-rs/src/deny_read_state.rs @@ -51,7 +51,7 @@ pub unsafe fn sync_persistent_deny_read_acls( for path in previous_paths { if !desired_keys.contains(&lexical_path_key(&path)) { - revoke_ace(&path, psid); + let _ = revoke_ace(&path, psid); } } diff --git a/codex-rs/windows-sandbox-rs/src/lib.rs b/codex-rs/windows-sandbox-rs/src/lib.rs index 683cbbc6d7..1478caef32 100644 --- a/codex-rs/windows-sandbox-rs/src/lib.rs +++ b/codex-rs/windows-sandbox-rs/src/lib.rs @@ -200,6 +200,8 @@ pub use acl::path_or_child_file_has_standard_user_mutation_allow; #[cfg(target_os = "windows")] pub use acl::path_write_aces_need_refresh; #[cfg(target_os = "windows")] +pub use acl::revoke_ace; +#[cfg(target_os = "windows")] pub use audit::apply_world_writable_scan_and_denies_for_permissions; #[cfg(target_os = "windows")] pub use cap::load_or_create_cap_sids; @@ -319,6 +321,8 @@ pub use provisioning_client::WindowsSandboxProvisioningOutcome; #[cfg(target_os = "windows")] pub use provisioning_client::provision_windows_sandbox_via_service; #[cfg(target_os = "windows")] +pub use provisioning_client::register_desktop_installation; +#[cfg(target_os = "windows")] pub use provisioning_protocol::FramedProvisioningMessage; #[cfg(target_os = "windows")] pub use provisioning_protocol::PROVISIONING_PROTOCOL_VERSION; diff --git a/codex-rs/windows-sandbox-rs/src/provisioning_client.rs b/codex-rs/windows-sandbox-rs/src/provisioning_client.rs index d02072b054..b80e094c1c 100644 --- a/codex-rs/windows-sandbox-rs/src/provisioning_client.rs +++ b/codex-rs/windows-sandbox-rs/src/provisioning_client.rs @@ -119,9 +119,44 @@ pub fn provision_windows_sandbox_via_service( }, }; - let deadline = Instant::now() + PROVISIONING_TIMEOUT; + match send_service_request(request, PROVISIONING_TIMEOUT)? { + crate::SandboxProvisioningResponse::Ok => { + Ok(WindowsSandboxProvisioningOutcome::Provisioned) + } + crate::SandboxProvisioningResponse::Unavailable => { + Ok(WindowsSandboxProvisioningOutcome::Unavailable) + } + crate::SandboxProvisioningResponse::Error { message } => Err(anyhow!(message)), + } +} + +/// Records desktop uninstall ownership without creating or enabling a sandbox. +pub fn register_desktop_installation(codex_home: &Path) -> anyhow::Result<()> { + let request = crate::FramedProvisioningMessage { + version: crate::PROVISIONING_PROTOCOL_VERSION, + message: crate::ProvisioningMessage::RegisterInstallationRequest { + codex_home: codex_home + .to_str() + .context("desktop home is not valid UTF-8")? + .to_owned(), + }, + }; + match send_service_request(request, Duration::from_secs(5))? { + crate::SandboxProvisioningResponse::Ok => Ok(()), + crate::SandboxProvisioningResponse::Unavailable => { + bail!("desktop uninstall registration service is unavailable") + } + crate::SandboxProvisioningResponse::Error { message } => Err(anyhow!(message)), + } +} + +fn send_service_request( + request: crate::FramedProvisioningMessage, + timeout: Duration, +) -> anyhow::Result { + let deadline = Instant::now() + timeout; let Some(mut pipe) = connect(deadline)? else { - return Ok(WindowsSandboxProvisioningOutcome::Unavailable); + return Ok(crate::SandboxProvisioningResponse::Unavailable); }; let response = (|| -> anyhow::Result { verify_server(pipe.as_raw_handle() as HANDLE) @@ -156,26 +191,18 @@ pub fn provision_windows_sandbox_via_service( ) }) => { - return Ok(WindowsSandboxProvisioningOutcome::Unavailable); + return Ok(crate::SandboxProvisioningResponse::Unavailable); } Err(error) => return Err(error), }; if response.version != crate::PROVISIONING_PROTOCOL_VERSION { - return Ok(WindowsSandboxProvisioningOutcome::Unavailable); + return Ok(crate::SandboxProvisioningResponse::Unavailable); } let crate::ProvisioningMessage::ProvisionSandboxResponse { payload } = response.message else { bail!("unexpected sandbox provisioning response message"); }; - match payload { - crate::SandboxProvisioningResponse::Ok => { - Ok(WindowsSandboxProvisioningOutcome::Provisioned) - } - crate::SandboxProvisioningResponse::Unavailable => { - Ok(WindowsSandboxProvisioningOutcome::Unavailable) - } - crate::SandboxProvisioningResponse::Error { message } => Err(anyhow!(message)), - } + Ok(payload) } fn connect(deadline: Instant) -> anyhow::Result> { diff --git a/codex-rs/windows-sandbox-rs/src/provisioning_protocol.rs b/codex-rs/windows-sandbox-rs/src/provisioning_protocol.rs index f2524faf79..3bb4fc4821 100644 --- a/codex-rs/windows-sandbox-rs/src/provisioning_protocol.rs +++ b/codex-rs/windows-sandbox-rs/src/provisioning_protocol.rs @@ -25,6 +25,10 @@ pub struct FramedProvisioningMessage { #[derive(Debug, Serialize, Deserialize, Clone)] #[serde(tag = "type", rename_all = "snake_case")] pub enum ProvisioningMessage { + /// Records uninstall ownership without provisioning sandbox resources. + RegisterInstallationRequest { + codex_home: String, + }, ProvisionSandboxRequest { payload: SandboxProvisioningRequest, }, diff --git a/codex-rs/windows-sandbox-rs/src/setup_provisioning.rs b/codex-rs/windows-sandbox-rs/src/setup_provisioning.rs index 2f7c0c9542..13faad8ef5 100644 --- a/codex-rs/windows-sandbox-rs/src/setup_provisioning.rs +++ b/codex-rs/windows-sandbox-rs/src/setup_provisioning.rs @@ -813,13 +813,14 @@ fn lock_persistent_sandbox_dirs(payload: &Payload, sandbox_group_sid: &[u8]) -> } fn lock_sandbox_bin_dir(payload: &Payload, sandbox_group_sid: &[u8]) -> Result<()> { + // The owner's unelevated refresh must be able to reapply this protected DACL. lock_sandbox_dir( &sandbox_bin_dir(&payload.codex_home), &payload.real_user, sandbox_group_sid, GRANT_ACCESS, FILE_GENERIC_READ | FILE_GENERIC_EXECUTE, - FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE | DELETE, + FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE | DELETE | WRITE_DAC, DaclInheritance::Protected, payload.mode, ) diff --git a/codex-rs/windows-sandbox-service/Cargo.toml b/codex-rs/windows-sandbox-service/Cargo.toml index 12595b9a8f..0ba371bed7 100644 --- a/codex-rs/windows-sandbox-service/Cargo.toml +++ b/codex-rs/windows-sandbox-service/Cargo.toml @@ -25,6 +25,9 @@ codex-windows-sandbox = { workspace = true } tokio = { workspace = true, features = ["macros", "rt", "sync", "time"] } toml = { workspace = true } +[dev-dependencies] +pretty_assertions = { workspace = true } + [target.'cfg(windows)'.dependencies.windows] version = "0.58" features = [ diff --git a/codex-rs/windows-sandbox-service/src/ipc.rs b/codex-rs/windows-sandbox-service/src/ipc.rs index 59f4437acc..a1ff38eb84 100644 --- a/codex-rs/windows-sandbox-service/src/ipc.rs +++ b/codex-rs/windows-sandbox-service/src/ipc.rs @@ -1,6 +1,6 @@ //! Authenticated local IPC for the Windows sandbox provisioning service. -//! Configuration parse failures and unsupported home drives defer provisioning to -//! the client's elevated helper. +//! Expected service limitations defer provisioning to the client's elevated helper; +//! authentication and policy rejections remain errors. //! Shutdown wakeups are retried until the listener connects or stops. mod authentication; @@ -21,9 +21,11 @@ use codex_windows_sandbox::string_from_sid_bytes; use codex_windows_sandbox::to_wide; use codex_windows_sandbox::write_provisioning_frame; pub(crate) use home::OwnedHandle; +pub(crate) use home::pin_directory; pub(crate) use home::pin_existing_ancestors; #[cfg(test)] use request::ProvisioningRequest; +pub(crate) use request::ServiceRequest; use request::validate_request; use std::mem::size_of; use std::ptr; @@ -48,6 +50,18 @@ const MAX_RESPONSE_MESSAGE_BYTES: usize = 512; const REQUEST_IDLE_TIMEOUT: Duration = Duration::from_secs(5); const PIPE_USER_ACCESS: &str = "0x0012019b"; +/// The service cannot complete this request; the interactive setup helper may still work. +#[derive(Debug)] +pub(crate) struct ServiceUnavailable(pub(crate) &'static str); + +impl std::fmt::Display for ServiceUnavailable { + fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + formatter.write_str(self.0) + } +} + +impl std::error::Error for ServiceUnavailable {} + struct SecurityDescriptor(security::PSECURITY_DESCRIPTOR); impl Drop for SecurityDescriptor { @@ -65,7 +79,7 @@ enum PipeConnection { pub(crate) fn run( shutdown: Arc, on_ready: impl FnOnce() -> Result<()>, - on_authenticated_user: impl Fn(&InstallationRecord, OwnedHandle) -> Result<()>, + register_installation: impl Fn(InstallationRecord, OwnedHandle) -> Result<()>, on_session_change: impl Fn() -> Result<()>, ) -> Result<()> { let sandbox_sid = ensure_sandbox_users_group()?; @@ -134,11 +148,12 @@ pub(crate) fn run( &authorized_process, &sandbox_sid, &shutdown, - &on_authenticated_user, + ®ister_installation, ); let response = match result { Ok(response) => response, - Err(error) if error.is::() => { + Err(error) if error.is::() => { + eprintln!("sandbox provisioning service unavailable: {error:#}"); SandboxProvisioningResponse::Unavailable } Err(error) => { @@ -253,7 +268,7 @@ fn handle_request( authorized_process: &crate::package_identity::AuthorizedClientProcess, sandbox_sid: &[u8], shutdown: &AtomicBool, - on_authenticated_user: &dyn Fn(&InstallationRecord, OwnedHandle) -> Result<()>, + register_installation: &dyn Fn(InstallationRecord, OwnedHandle) -> Result<()>, ) -> Result { let deadline = Instant::now() + REQUEST_IDLE_TIMEOUT; let mut request = [0_u8; MAX_REQUEST_BYTES]; @@ -335,7 +350,7 @@ fn handle_request( return Err(error) .context("requested sandbox settings violate administrator-controlled machine policy"); } - crate::provisioning::run(identity, request.settings, on_authenticated_user) + crate::provisioning::run(identity, request, register_installation) } fn is_config_parse_error(error: &anyhow::Error) -> bool { diff --git a/codex-rs/windows-sandbox-service/src/ipc/authentication.rs b/codex-rs/windows-sandbox-service/src/ipc/authentication.rs index 6ffd837130..d9c34980ed 100644 --- a/codex-rs/windows-sandbox-service/src/ipc/authentication.rs +++ b/codex-rs/windows-sandbox-service/src/ipc/authentication.rs @@ -3,20 +3,26 @@ use anyhow::Context; use anyhow::Result; use anyhow::bail; +use codex_windows_sandbox::DirectoryOpenDisposition; +use codex_windows_sandbox::create_directory_guard; use codex_windows_sandbox::string_from_sid_bytes; +use std::ffi::OsString; use std::ffi::c_void; use std::mem::size_of; -use std::path::Path; +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; use windows_sys::Win32::System::Pipes as pipes; use windows_sys::Win32::System::Threading as threading; use super::home::OwnedHandle; use super::home::prepare_codex_home; -use super::request::ProvisioningRequest; +use super::request::ServiceRequest; pub(crate) struct ClientIdentity { pub(crate) account: String, @@ -25,7 +31,7 @@ pub(crate) struct ClientIdentity { pub(crate) session_id: u32, pub(crate) token: OwnedHandle, pub(crate) desktop_installation: Option, - // Retained by both the service and helper throughout provisioning. + // Pins and guards remain live through registration and the provisioning helper. pub(crate) directory_handles: Vec, } @@ -33,7 +39,7 @@ pub(super) fn authenticate_client( pipe: HANDLE, authorized_process: &crate::package_identity::AuthorizedClientProcess, sandbox_sid: &[u8], - request: &ProvisioningRequest, + request: &ServiceRequest, ) -> Result<(ClientIdentity, Result<()>)> { std::thread::scope(|scope| { scope @@ -43,17 +49,20 @@ pub(super) fn authenticate_client( .context("impersonate provisioning client"); } - let identity = authenticate_impersonated_client( - authorized_process, - sandbox_sid, - &request.codex_home, - )?; - let policy_result = crate::machine_policy::validate_provisioning_settings( - &identity.codex_home, - &request.settings, - &request.listeners, - identity.token.0, - ); + let identity = + authenticate_impersonated_client(authorized_process, sandbox_sid, request)?; + let policy_result = match request { + // Registration does not provision resources or change sandbox policy. + ServiceRequest::RegisterInstallation { .. } => Ok(()), + ServiceRequest::ProvisionSandbox(request) => { + crate::machine_policy::validate_provisioning_settings( + &identity.codex_home, + &request.settings, + &request.listeners, + identity.token.0, + ) + } + }; Ok((identity, policy_result)) }) .join() @@ -64,7 +73,7 @@ pub(super) fn authenticate_client( fn authenticate_impersonated_client( authorized_process: &crate::package_identity::AuthorizedClientProcess, sandbox_sid: &[u8], - requested_home: &Path, + request: &ServiceRequest, ) -> Result { let mut raw_token = 0; if unsafe { @@ -152,7 +161,57 @@ fn authenticate_impersonated_client( }) .map_err(anyhow::Error::msg)?; let account = account_name(sid)?; - let (codex_home, handles) = prepare_codex_home(requested_home)?; + let requested_home = match request { + ServiceRequest::RegisterInstallation { codex_home } => codex_home, + ServiceRequest::ProvisionSandbox(request) => &request.codex_home, + }; + let (codex_home, handles) = match request { + ServiceRequest::ProvisionSandbox(_) => prepare_codex_home(requested_home)?, + ServiceRequest::RegisterInstallation { .. } => { + // Registration must not create the sandbox directories or change their ACLs. + codex_windows_sandbox::validate_local_directory_path(requested_home)?; + let mut handles = Vec::new(); + super::home::pin_existing_ancestors(requested_home, &mut handles)?; + // Uninstall removes sandbox files as SYSTEM; read access cannot grant that authority. + let home = super::home::pin_directory( + requested_home, + filesystem::FILE_ADD_FILE + | filesystem::FILE_ADD_SUBDIRECTORY + | filesystem::WRITE_DAC, + DirectoryOpenDisposition::OpenExisting, + )?; + // Bind the guard to the authorized directory before resolving its pathname. + let guard = create_directory_guard(unsafe { BorrowedHandle::borrow_raw(home.0 as _) })?; + drop(super::home::pin_directory( + requested_home, + filesystem::FILE_READ_ATTRIBUTES, + DirectoryOpenDisposition::OpenExisting, + )?); + // Preserve the authorized directory's literal name (including trailing dots). + let mut buffer = vec![0_u16; 260]; + let path = loop { + let length = unsafe { + filesystem::GetFinalPathNameByHandleW( + home.0, + buffer.as_mut_ptr(), + buffer.len() as u32, + filesystem::FILE_NAME_NORMALIZED | filesystem::VOLUME_NAME_DOS, + ) + }; + if length == 0 { + return Err(std::io::Error::last_os_error()) + .context("resolve authorized desktop home"); + } + if (length as usize) < buffer.len() { + break PathBuf::from(OsString::from_wide(&buffer[..length as usize])); + } + buffer.resize(length as usize, /*value*/ 0); + }; + handles.push(home); + handles.push(OwnedHandle(guard.into_raw_handle() as HANDLE)); + (path, handles) + } + }; let desktop_installation = crate::installation_record::read_desktop_installation(&codex_home, token.0) .inspect_err(|_| { diff --git a/codex-rs/windows-sandbox-service/src/ipc/home.rs b/codex-rs/windows-sandbox-service/src/ipc/home.rs index 97f48c78a7..62c1007d6b 100644 --- a/codex-rs/windows-sandbox-service/src/ipc/home.rs +++ b/codex-rs/windows-sandbox-service/src/ipc/home.rs @@ -19,20 +19,10 @@ use windows_sys::Win32::Foundation as foundation; use windows_sys::Win32::Foundation::HANDLE; use windows_sys::Win32::Storage::FileSystem as filesystem; +use super::ServiceUnavailable; + const DRIVE_FIXED: u32 = 3; -/// The service does not support this drive; the interactive helper may still work. -#[derive(Debug)] -pub(super) struct UnsupportedHomeDrive; - -impl std::fmt::Display for UnsupportedHomeDrive { - fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - formatter.write_str("Codex home must be located on a fixed local drive") - } -} - -impl std::error::Error for UnsupportedHomeDrive {} - pub(crate) struct OwnedHandle(pub(crate) HANDLE); impl Drop for OwnedHandle { @@ -53,7 +43,7 @@ pub(super) fn prepare_codex_home(requested: &Path) -> Result<(PathBuf, Vec Result<(PathBuf, Vec Result<(PathBuf, Vec().is_some_and(|error| { + error.raw_os_error() == Some(foundation::ERROR_ACCESS_DENIED as i32) + }) + { + error.context(ServiceUnavailable("sandbox binary directory needs elevated repair")) + } else { + error + } + }) .with_context(|| { if index == 0 { format!( @@ -153,7 +156,7 @@ pub(crate) fn pin_existing_ancestors(path: &Path, handles: &mut Vec Ok(()) } -fn pin_directory( +pub(crate) fn pin_directory( path: &Path, access: u32, disposition: DirectoryOpenDisposition, diff --git a/codex-rs/windows-sandbox-service/src/ipc/request.rs b/codex-rs/windows-sandbox-service/src/ipc/request.rs index d5fd7eac92..7a7f47d291 100644 --- a/codex-rs/windows-sandbox-service/src/ipc/request.rs +++ b/codex-rs/windows-sandbox-service/src/ipc/request.rs @@ -13,13 +13,19 @@ use std::path::PathBuf; use super::MAX_REQUEST_BYTES; #[derive(Debug, Eq, PartialEq)] -pub(super) struct ProvisioningRequest { - pub(super) codex_home: PathBuf, - pub(super) listeners: WindowsSandboxProxyListeners, - pub(super) settings: WindowsSandboxProvisioningSettings, +pub(crate) enum ServiceRequest { + RegisterInstallation { codex_home: PathBuf }, + ProvisionSandbox(ProvisioningRequest), } -pub(super) fn validate_request(request: &[u8]) -> Result { +#[derive(Debug, Eq, PartialEq)] +pub(crate) struct ProvisioningRequest { + pub(super) codex_home: PathBuf, + pub(super) listeners: WindowsSandboxProxyListeners, + pub(crate) settings: WindowsSandboxProvisioningSettings, +} + +pub(super) fn validate_request(request: &[u8]) -> Result { if request.len() > MAX_REQUEST_BYTES { bail!("provisioning request exceeds size limit"); } @@ -36,12 +42,17 @@ pub(super) fn validate_request(request: &[u8]) -> Result { frame.version ); } - let ProvisioningMessage::ProvisionSandboxRequest { payload: request } = frame.message else { - bail!("expected a sandbox provisioning request"); + let request = match frame.message { + ProvisioningMessage::RegisterInstallationRequest { codex_home } => { + validate_home(&codex_home)?; + return Ok(ServiceRequest::RegisterInstallation { + codex_home: PathBuf::from(codex_home), + }); + } + ProvisioningMessage::ProvisionSandboxRequest { payload } => payload, + ProvisioningMessage::ProvisionSandboxResponse { .. } => bail!("expected a service request"), }; - if request.codex_home.is_empty() || request.codex_home.contains(['\0', '\r', '\n']) { - bail!("Codex home is empty or contains an invalid control character"); - } + validate_home(&request.codex_home)?; let mut settings = request.settings; let mut listeners = request.listeners; for ports in [ @@ -63,9 +74,16 @@ pub(super) fn validate_request(request: &[u8]) -> Result { { bail!("provisioning listener is absent from the proxy settings"); } - Ok(ProvisioningRequest { + Ok(ServiceRequest::ProvisionSandbox(ProvisioningRequest { codex_home: PathBuf::from(request.codex_home), listeners, settings, - }) + })) +} + +fn validate_home(home: &str) -> Result<()> { + if home.is_empty() || home.contains(['\0', '\r', '\n']) { + bail!("Codex home is empty or contains an invalid control character"); + } + Ok(()) } diff --git a/codex-rs/windows-sandbox-service/src/ipc_tests.rs b/codex-rs/windows-sandbox-service/src/ipc_tests.rs index 43e77e827f..00221e025a 100644 --- a/codex-rs/windows-sandbox-service/src/ipc_tests.rs +++ b/codex-rs/windows-sandbox-service/src/ipc_tests.rs @@ -1,6 +1,7 @@ use super::OwnedHandle; use super::PipeConnection; use super::ProvisioningRequest; +use super::ServiceRequest; use super::accept_pipe_connection; use super::is_config_parse_error; use super::pin_existing_ancestors; @@ -17,6 +18,7 @@ use codex_windows_sandbox::WindowsSandboxProxyListeners; use codex_windows_sandbox::read_provisioning_frame; use codex_windows_sandbox::to_wide; use codex_windows_sandbox::write_provisioning_frame; +use pretty_assertions::assert_eq; use std::path::Path; use std::path::PathBuf; use std::ptr; @@ -113,11 +115,11 @@ fn provisioning_request_preserves_home_spaces_and_unicode() { }); assert_eq!( validate_request(&request).unwrap(), - ProvisioningRequest { + ServiceRequest::ProvisionSandbox(ProvisioningRequest { codex_home: PathBuf::from("D:\\Codex Homes\\Jos\u{00e9}\\.codex"), listeners: WindowsSandboxProxyListeners::default(), settings: WindowsSandboxProvisioningSettings::default(), - } + }) ); } @@ -139,7 +141,7 @@ fn structured_provisioning_request_carries_normalized_proxy_settings() { }); assert_eq!( validate_request(&request).unwrap(), - ProvisioningRequest { + ServiceRequest::ProvisionSandbox(ProvisioningRequest { codex_home: PathBuf::from("D:\\Codex Homes\\Jos\u{00e9}\\.codex"), listeners: WindowsSandboxProxyListeners { http_ports: vec![http_port], @@ -149,7 +151,7 @@ fn structured_provisioning_request_carries_normalized_proxy_settings() { proxy_ports, allow_local_binding: true, }, - }, + }), ); } } @@ -176,11 +178,11 @@ fn structured_provisioning_request_accepts_independent_and_additional_proxy_port }); assert_eq!( validate_request(&request).unwrap(), - ProvisioningRequest { + ServiceRequest::ProvisionSandbox(ProvisioningRequest { codex_home: PathBuf::from(r"C:\Users\alice\.codex"), settings, listeners, - } + }) ); } } @@ -194,11 +196,11 @@ fn structured_provisioning_request_accepts_disabled_listeners() { }); assert_eq!( validate_request(&request).unwrap(), - ProvisioningRequest { + ServiceRequest::ProvisionSandbox(ProvisioningRequest { codex_home: PathBuf::from(r"C:\Users\alice\.codex"), listeners: WindowsSandboxProxyListeners::default(), settings: WindowsSandboxProvisioningSettings::default(), - } + }) ); } @@ -496,3 +498,24 @@ fn pipe_descriptor_denies_sandbox_group_before_interactive_users() { let interactive = descriptor.find("(A;;0x0012019b;;;IU)").unwrap(); assert!(deny < interactive); } + +#[test] +fn installation_registration_requires_no_sandbox_settings() { + let mut frame = Vec::new(); + write_provisioning_frame( + &mut frame, + &FramedProvisioningMessage { + version: PROVISIONING_PROTOCOL_VERSION, + message: ProvisioningMessage::RegisterInstallationRequest { + codex_home: r"C:\Users\alice\.codex".to_string(), + }, + }, + ) + .unwrap(); + assert_eq!( + validate_request(&frame).unwrap(), + ServiceRequest::RegisterInstallation { + codex_home: PathBuf::from(r"C:\Users\alice\.codex") + }, + ); +} diff --git a/codex-rs/windows-sandbox-service/src/package_lifecycle.rs b/codex-rs/windows-sandbox-service/src/package_lifecycle.rs index 544567e3fa..7beae074f6 100644 --- a/codex-rs/windows-sandbox-service/src/package_lifecycle.rs +++ b/codex-rs/windows-sandbox-service/src/package_lifecycle.rs @@ -2,6 +2,8 @@ use std::cell::RefCell; use std::io; +use std::os::windows::io::BorrowedHandle; +use std::os::windows::io::IntoRawHandle; use std::path::PathBuf; use std::sync::Arc; use std::sync::atomic::AtomicBool; @@ -10,6 +12,8 @@ use std::sync::atomic::Ordering; use anyhow::Context; use anyhow::Result; use anyhow::ensure; +use codex_windows_sandbox::DirectoryOpenDisposition; +use codex_windows_sandbox::create_directory_guard; use codex_windows_sandbox::string_from_sid_bytes; use windows::ApplicationModel::Package; use windows::ApplicationModel::PackageCatalog; @@ -22,12 +26,12 @@ use windows::Win32::System::WinRT::RoUninitialize; use windows::core::HSTRING; use windows_sys::Win32::Foundation::HANDLE; use windows_sys::Win32::Security as security; +use windows_sys::Win32::Storage::FileSystem as filesystem; use windows_sys::Win32::System::RemoteDesktop::WTS_CURRENT_SERVER_HANDLE; use windows_sys::Win32::System::RemoteDesktop::WTSEnumerateSessionsW; use windows_sys::Win32::System::RemoteDesktop::WTSFreeMemory; use windows_sys::Win32::System::RemoteDesktop::WTSQueryUserToken; -use crate::installation_record::DesktopInstallation; use crate::installation_record::InstallationRecord; use crate::ipc::OwnedHandle; @@ -35,8 +39,9 @@ mod cleanup; struct UserInstallation { codex_home: Option, + // Ancestors, home, then a guard that prevents in-place junction conversion. directory_handles: Vec, - desktop_installation: Option, + record: InstallationRecord, user_token: OwnedHandle, catalog: PackageCatalog, token: EventRegistrationToken, @@ -59,22 +64,59 @@ impl PackageLifecycle { }) } - pub(crate) fn watch_authenticated_user( + pub(crate) fn register_authenticated_user( &self, - installation: &InstallationRecord, + mut record: InstallationRecord, user_token: OwnedHandle, ) -> Result<()> { - if self.installation.borrow().is_some() { + let mut active = self.installation.borrow_mut(); + let previous = match active.as_ref() { + Some(installation) => Some(installation.record.clone()), + None => crate::installation_record::load()?, + }; + if let Some(previous) = previous { + // A missing watcher does not retire its owner; other clients can use elevated setup. + ensure!( + previous.user_sid == record.user_sid && previous.codex_home == record.codex_home, + crate::ipc::ServiceUnavailable( + "installation is already registered to a different owner or home" + ) + ); + record.desktop_installation = previous + .desktop_installation + .or(record.desktop_installation); + } + crate::installation_record::save(&record)?; + if let Some(installation) = active.as_mut() + && installation.codex_home.is_some() + { + // A restored watcher must immediately use newly registered desktop ownership. + installation.record = record; return Ok(()); } with_owner_impersonation(user_token.0, || { let mut directory_handles = Vec::new(); let codex_home = match crate::ipc::pin_existing_ancestors( - &installation.codex_home, + &record.codex_home, &mut directory_handles, - ) { - Ok(()) => Some(installation.codex_home.clone()), + ) + .and_then(|()| { + let home = directory_handles + .last() + .context("pin the registered home")?; + let guard = + create_directory_guard(unsafe { BorrowedHandle::borrow_raw(home.0 as _) })?; + // Reject a conversion that happened before the handle-relative guard was created. + drop(crate::ipc::pin_directory( + &record.codex_home, + filesystem::FILE_READ_ATTRIBUTES, + DirectoryOpenDisposition::OpenExisting, + )?); + directory_handles.push(OwnedHandle(guard.into_raw_handle() as HANDLE)); + Ok(()) + }) { + Ok(()) => Some(record.codex_home.clone()), Err(error) => { directory_handles.clear(); crate::service::log_error( @@ -86,8 +128,15 @@ impl PackageLifecycle { None } }; - let catalog = PackageCatalog::OpenForCurrentUser() - .context("open the authenticated user's package catalog")?; + if let Some(installation) = active.as_mut() { + installation.codex_home = codex_home; + installation.directory_handles = directory_handles; + installation.record = record; + return Ok(()); + } + let catalog = PackageCatalog::OpenForCurrentUser().context( + crate::ipc::ServiceUnavailable("open the authenticated user's package catalog"), + )?; let package_name = self.package_name.clone(); let uninstalling = Arc::clone(&self.uninstalling); let token = catalog @@ -102,15 +151,17 @@ impl PackageLifecycle { } Ok(()) })) - .context("subscribe to authenticated package uninstall notifications")?; - self.installation.replace(Some(UserInstallation { + .context(crate::ipc::ServiceUnavailable( + "subscribe to authenticated package uninstall notifications", + ))?; + active.replace(UserInstallation { codex_home, directory_handles, - desktop_installation: installation.desktop_installation.clone(), + record, user_token, catalog, token, - })); + }); Ok(()) }) } @@ -173,14 +224,13 @@ impl PackageLifecycle { "logged-in user does not match the recorded sandbox owner" ); - self.watch_authenticated_user(&record, token)?; - if session_id != record.session_id { - crate::installation_record::save(&crate::installation_record::InstallationRecord { + self.register_authenticated_user( + InstallationRecord { session_id, ..record - })?; - } - Ok(()) + }, + token, + ) } pub(crate) fn clean_up(&self) -> Result<()> { 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 d52e386de2..de552b9b8f 100644 --- a/codex-rs/windows-sandbox-service/src/package_lifecycle/cleanup.rs +++ b/codex-rs/windows-sandbox-service/src/package_lifecycle/cleanup.rs @@ -7,6 +7,8 @@ use anyhow::Context; use anyhow::Result; use anyhow::ensure; use codex_windows_sandbox::clean_up_packaged_windows_sandbox; +use codex_windows_sandbox::resolve_sid; +use codex_windows_sandbox::revoke_ace; use super::UserInstallation; use super::with_owner_impersonation; @@ -20,7 +22,9 @@ pub(super) fn clean_up(installation: &mut UserInstallation) -> Result<()> { crate::installation_record::remove()?; let codex_home = installation.codex_home.clone(); clean_up_packaged_windows_sandbox(codex_home.as_deref(), || { - let Some(desktop) = &installation.desktop_installation else { + // Privileged file cleanup is finished; release the guard before owner-scoped removal. + installation.directory_handles.pop(); + let Some(desktop) = &installation.record.desktop_installation else { return Ok(()); }; // The marker is user-writable. It must never authorize deletion as LocalSystem. @@ -33,12 +37,21 @@ pub(super) fn clean_up(installation: &mut UserInstallation) -> Result<()> { errors.push(error.to_string()); } }; - if let Some(home) = &codex_home - && desktop.created_codex_home - { - // Release the home itself so it can be deleted; keep its ancestors pinned. - installation.directory_handles.pop(); - record_error(std::fs::remove_dir_all(home)); + if let Some(home) = &codex_home { + if desktop.created_codex_home { + // Release the home itself so it can be deleted; keep its ancestors pinned. + installation.directory_handles.pop(); + record_error(std::fs::remove_dir_all(home)); + } else { + // Preserve CLI data without leaving inherited permissions for the deleted group. + record_error( + resolve_sid("CodexSandboxUsers") + .and_then(|mut sid| unsafe { + revoke_ace(home, sid.as_mut_ptr().cast()) + }) + .map_err(io::Error::other), + ); + } } // The cache may have been created after provisioning. Pin it only for cleanup. let mut cache_directory_handles = Vec::new(); diff --git a/codex-rs/windows-sandbox-service/src/provisioning.rs b/codex-rs/windows-sandbox-service/src/provisioning.rs index 4ed8e7190e..0ca2f9cea6 100644 --- a/codex-rs/windows-sandbox-service/src/provisioning.rs +++ b/codex-rs/windows-sandbox-service/src/provisioning.rs @@ -8,7 +8,6 @@ use anyhow::Context; use anyhow::Result; use anyhow::bail; use codex_windows_sandbox::SandboxProvisioningResponse; -use codex_windows_sandbox::WindowsSandboxProvisioningSettings; use codex_windows_sandbox::run_elevated_provisioning_setup_with_retained_handles; use codex_windows_sandbox::sandbox_setup_is_complete_with_settings; use windows_sys::Win32::Storage::FileSystem as filesystem; @@ -16,28 +15,31 @@ use windows_sys::Win32::Storage::FileSystem as filesystem; use crate::installation_record::InstallationRecord; use crate::ipc::ClientIdentity; use crate::ipc::OwnedHandle; +use crate::ipc::ServiceRequest; pub(crate) fn run( identity: ClientIdentity, - settings: WindowsSandboxProvisioningSettings, - on_authenticated_user: &dyn Fn(&InstallationRecord, OwnedHandle) -> Result<()>, + request: ServiceRequest, + register_installation: &dyn Fn(InstallationRecord, OwnedHandle) -> Result<()>, ) -> Result { // A policy-rejected request must not choose the uninstall owner. Use the // token already authenticated above instead of impersonating the pipe again. - let previous = crate::installation_record::load()?.filter(|record| { - record.user_sid == identity.user_sid && record.codex_home == identity.codex_home - }); - let installation = InstallationRecord { - codex_home: identity.codex_home.clone(), - user_sid: identity.user_sid, - session_id: identity.session_id, - desktop_installation: previous - .and_then(|record| record.desktop_installation) - .or(identity.desktop_installation), + register_installation( + InstallationRecord { + codex_home: identity.codex_home.clone(), + user_sid: identity.user_sid, + session_id: identity.session_id, + desktop_installation: identity.desktop_installation, + }, + identity.token, + )?; + let request = match request { + ServiceRequest::RegisterInstallation { .. } => { + return Ok(SandboxProvisioningResponse::Ok); + } + ServiceRequest::ProvisionSandbox(request) => request, }; - on_authenticated_user(&installation, identity.token)?; - if sandbox_setup_is_complete_with_settings(&identity.codex_home, &settings) { - crate::service::record_provisioned_user(&installation)?; + if sandbox_setup_is_complete_with_settings(&identity.codex_home, &request.settings) { return Ok(SandboxProvisioningResponse::Ok); } let helper = std::env::current_exe() @@ -63,11 +65,10 @@ pub(crate) fn run( match run_elevated_provisioning_setup_with_retained_handles( &identity.codex_home, &identity.account, - settings, + request.settings, &retained_handles, ) { Ok(()) => { - crate::service::record_provisioned_user(&installation)?; crate::service::log_information( crate::service::EVENT_PROVISIONING_SUCCEEDED, "Codex sandbox provisioning completed successfully.", diff --git a/codex-rs/windows-sandbox-service/src/service.rs b/codex-rs/windows-sandbox-service/src/service.rs index 40bcb86276..870c3b1e09 100644 --- a/codex-rs/windows-sandbox-service/src/service.rs +++ b/codex-rs/windows-sandbox-service/src/service.rs @@ -175,14 +175,7 @@ fn service_main_inner(state: &ServiceState) -> Result<()> { Ok(()) }, |installation, user_token| { - if let Err(error) = package_lifecycle.watch_authenticated_user(installation, user_token) - { - log_error( - EVENT_SERVICE_FAILED, - &format!("unable to observe package uninstall: {error:#}"), - ); - } - Ok(()) + package_lifecycle.register_authenticated_user(installation, user_token) }, || { let session = state.changed_session.swap(u32::MAX, Ordering::AcqRel); @@ -276,15 +269,6 @@ pub(crate) fn log_information(event_id: u32, message: &str) { log_event(EVENTLOG_INFORMATION_TYPE, event_id, message); } -pub(crate) fn record_provisioned_user( - installation: &crate::installation_record::InstallationRecord, -) -> Result<()> { - if SERVICE_STATE.get().is_some() { - crate::installation_record::save(installation)?; - } - Ok(()) -} - pub(crate) fn log_error(event_id: u32, message: &str) { log_event(EVENTLOG_ERROR_TYPE, event_id, message); }