diff --git a/codex-rs/windows-sandbox-rs/src/runtime_ownership.rs b/codex-rs/windows-sandbox-rs/src/runtime_ownership.rs index 7b208833ef..ace6baaa05 100644 --- a/codex-rs/windows-sandbox-rs/src/runtime_ownership.rs +++ b/codex-rs/windows-sandbox-rs/src/runtime_ownership.rs @@ -93,7 +93,7 @@ impl RuntimeRegistration { && self .accounts .iter() - .all(|account| account.alias_path.is_some()) + .all(|account| account.alias_path.is_some() && !account.cleanup_logon_pending) } } @@ -119,7 +119,12 @@ impl InstallationRecord { "registered sandbox resources belong to a different owner or package" ); ensure!( - self.runtime()?.retiring.is_none(), + self.runtime()?.retiring.is_none() + && self + .runtime()? + .accounts + .iter() + .all(|account| !account.cleanup_logon_pending), "registered sandbox cleanup must finish before provisioning" ); Ok(()) @@ -128,6 +133,9 @@ impl InstallationRecord { #[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize)] pub struct RuntimeAccountRegistration { + /// Persisted before temporarily enabling an account; cleared only after re-disabling it. + #[serde(default)] + pub cleanup_logon_pending: bool, pub account: SandboxRuntimeAccount, pub user_sid: String, /// OS-resolved alias written by the service, never inferred from a user name. diff --git a/codex-rs/windows-sandbox-rs/src/runtime_ownership_tests.rs b/codex-rs/windows-sandbox-rs/src/runtime_ownership_tests.rs index 9ef77269af..6fade70a0d 100644 --- a/codex-rs/windows-sandbox-rs/src/runtime_ownership_tests.rs +++ b/codex-rs/windows-sandbox-rs/src/runtime_ownership_tests.rs @@ -19,6 +19,7 @@ fn record() -> InstallationRecord { metadata_roots: Vec::new(), ready_package: None, accounts: vec![RuntimeAccountRegistration { + cleanup_logon_pending: false, account: SandboxRuntimeAccount::Offline, user_sid: "S-1-5-21-1-2-3-1001".into(), alias_path: None, @@ -41,6 +42,7 @@ fn ready_runtime() -> RuntimeRegistration { runtime.ready_package = Some(READY_PACKAGE.into()); runtime.accounts[0].alias_path = Some(PathBuf::from(r"C:\aliases\offline.exe")); runtime.accounts.push(RuntimeAccountRegistration { + cleanup_logon_pending: false, account: SandboxRuntimeAccount::Online, user_sid: "S-1-5-21-1-2-3-1002".into(), alias_path: Some(PathBuf::from(r"C:\aliases\online.exe")), @@ -200,9 +202,13 @@ fn metadata_root_history_survives_record_round_trip() { } #[test] -fn older_runtime_receipts_default_to_no_recorded_metadata_roots() { +fn older_runtime_receipts_default_to_no_metadata_roots_or_cleanup_logon() { let expected = record(); - let json = serde_json::to_value(&expected).unwrap(); + let mut json = serde_json::to_value(&expected).unwrap(); + json["runtime"]["accounts"][0] + .as_object_mut() + .unwrap() + .remove("cleanup_logon_pending"); assert!(json["runtime"].get("metadata_roots").is_none()); assert_eq!( serde_json::from_value::(json).unwrap(), @@ -243,3 +249,27 @@ fn legacy_non_null_cleanup_journal_cannot_silently_drop_its_fence() { assert!(serde_json::from_value::(json).is_err()); } } + +#[test] +fn interrupted_cleanup_logon_blocks_admission_after_record_reload() { + let mut original = record(); + original.runtime = Some(ready_runtime()); + original.runtime_mut().unwrap().accounts[0].cleanup_logon_pending = true; + let mut restored: InstallationRecord = + serde_json::from_str(&serde_json::to_string(&original).unwrap()).unwrap(); + assert_eq!(restored, original); + assert!(!restored.runtime().unwrap().ready_for_package(READY_PACKAGE)); + assert!( + restored + .admit_owner(&original, "OpenAI.Codex_publisher") + .is_err() + ); + + restored.runtime_mut().unwrap().accounts[0].cleanup_logon_pending = false; + assert!(restored.runtime().unwrap().ready_for_package(READY_PACKAGE)); + assert!( + restored + .admit_owner(&original, "OpenAI.Codex_publisher") + .is_ok() + ); +} diff --git a/codex-rs/windows-sandbox-rs/src/uninstall_windows/processes.rs b/codex-rs/windows-sandbox-rs/src/uninstall_windows/processes.rs index 84752b2030..5cb57079e0 100644 --- a/codex-rs/windows-sandbox-rs/src/uninstall_windows/processes.rs +++ b/codex-rs/windows-sandbox-rs/src/uninstall_windows/processes.rs @@ -83,11 +83,11 @@ pub(super) fn stop(users: &DisabledSandboxUsers, retained: &RetainedLogons) -> R // A token can outlive its process or exist before a runner starts. Account disable does // not revoke it, so check for remaining logins before removing protections. Only // explicitly pinned, private SYSTEM-finalizer tokens may remain; no process is exempt. - if !has_sandbox_logon_session(users, retained)? { + let Some(blocker) = blocking_logon_session(users, retained)? else { return Ok(()); - } + }; if Instant::now() >= deadline { - bail!("sandbox processes or logon tokens remain after the uninstall deadline"); + bail!("sandbox cleanup blocked after the uninstall deadline: {blocker}"); } // Repeat to catch descendants created during the previous snapshot. std::thread::sleep( @@ -159,10 +159,10 @@ fn sandbox_processes(users: &DisabledSandboxUsers) -> Result> { Ok(handles) } -fn has_sandbox_logon_session( +fn blocking_logon_session( users: &DisabledSandboxUsers, retained: &RetainedLogons, -) -> Result { +) -> Result> { let mut count = 0; let mut logons = null_mut(); let status = unsafe { LsaEnumerateLogonSessions(&mut count, &mut logons) }; @@ -171,7 +171,7 @@ fn has_sandbox_logon_session( } let logons = LsaBuffer(logons.cast()); if count == 0 { - return Ok(false); + return Ok(None); } for logon in unsafe { std::slice::from_raw_parts(logons.0.cast::(), count as usize) } { // LocalSystem uses the reserved logon ID 0:0x3e7 and has no normal logon data. @@ -188,17 +188,23 @@ fn has_sandbox_logon_session( } // Keep protections when LSA returns no session data. if data.is_null() { - return Ok(true); + return Ok(Some(format!( + "LSA returned no data for logon {:08x}:{:08x}", + logon.HighPart, logon.LowPart + ))); } let data = LsaBuffer(data.cast()); let data = unsafe { &*data.0.cast::() }; // LSA also lists sessions without a user SID, even before sandbox accounts exist. // Only sessions identified as sandbox users are evidence of remaining sandbox tokens. if is_sandbox_user(users, data.Sid) && !retained.contains(*logon) { - return Ok(true); + return Ok(Some(format!( + "sandbox logon {:08x}:{:08x} remains (logon type {}, session {})", + logon.HighPart, logon.LowPart, data.LogonType, data.Session + ))); } } - Ok(false) + Ok(None) } fn is_sandbox_user(users: &DisabledSandboxUsers, sid: *mut c_void) -> bool { diff --git a/codex-rs/windows-sandbox-service/src/package_lifecycle/registered.rs b/codex-rs/windows-sandbox-service/src/package_lifecycle/registered.rs index 4b9d10492b..3b0cdc294f 100644 --- a/codex-rs/windows-sandbox-service/src/package_lifecycle/registered.rs +++ b/codex-rs/windows-sandbox-service/src/package_lifecycle/registered.rs @@ -75,13 +75,13 @@ pub(super) fn clean_up(lifecycle: &PackageLifecycle, record: InstallationRecord) }), "waiting for the authenticated runtime owner and home before sandbox file cleanup" ); - let record = crate::registered_runtime::prepare_cleanup(record)?; + let mut record = crate::registered_runtime::prepare_cleanup(record)?; let removal = { let installation = lifecycle.installation.borrow(); let installation = installation .as_ref() .context("authenticated installation is missing")?; - crate::registered_runtime::prepare_removal(installation.user_token.0, &record)? + crate::registered_runtime::prepare_removal(installation.user_token.0, &mut record)? }; let prepared = prepare_packaged_windows_sandbox_cleanup_with_retained_tokens(&removal.tokens())?; diff --git a/codex-rs/windows-sandbox-service/src/registered_runtime.rs b/codex-rs/windows-sandbox-service/src/registered_runtime.rs index f9fc438d40..e9b82eb3fb 100644 --- a/codex-rs/windows-sandbox-service/src/registered_runtime.rs +++ b/codex-rs/windows-sandbox-service/src/registered_runtime.rs @@ -43,6 +43,7 @@ mod removal; pub(crate) use metadata::remove as remove_metadata; pub(crate) use removal::prepare as prepare_removal; +pub(crate) use removal::restore_disabled_accounts; /// Must escape request handling: pending resources are retained until this service exits. #[derive(Debug)] @@ -132,6 +133,7 @@ pub(crate) fn provision( .join(&family) .join(APP_CORE_RUNNER_ALIAS); accounts.push(RuntimeAccountRegistration { + cleanup_logon_pending: false, account, user_sid: profile.user_sid.clone(), alias_path: Some(alias_path), @@ -202,28 +204,38 @@ pub(crate) fn provision( Ok(()) } -/// Prepare one retirement fence under the caller's setup lock. Removal preparation -/// then retains exact account logons before the native guard disables those users. -pub(crate) fn prepare_cleanup(mut record: InstallationRecord) -> Result { +/// Validate cleanup ownership under the caller's setup lock before obtaining logons. +pub(crate) fn prepare_cleanup(record: InstallationRecord) -> Result { validate_record(&record)?; for account in &record.runtime()?.accounts { if codex_windows_sandbox::local_user_flags(account.account.username())?.is_some() { - let sid = lookup_sid(account.account.username())?; - ensure!( - string_from_sid_bytes(&sid).map_err(anyhow::Error::msg)? == account.user_sid, - "managed runtime account was replaced before cleanup" - ); + validate_account_sid(account)?; } } ensure!( record.runtime()?.retiring.is_none(), "interrupted runtime cleanup requires repair" ); - record.runtime_mut()?.ready_package = None; - record.runtime_mut()?.retiring = Some(format!("{:?}", windows::core::GUID::new()?)); + ensure!( + record + .runtime()? + .accounts + .iter() + .all(|account| !account.cleanup_logon_pending), + "cleanup logon recovery is pending" + ); Ok(record) } +fn validate_account_sid(account: &RuntimeAccountRegistration) -> Result<()> { + let sid = lookup_sid(account.account.username())?; + ensure!( + string_from_sid_bytes(&sid).map_err(anyhow::Error::msg)? == account.user_sid, + "managed runtime account was replaced before cleanup" + ); + Ok(()) +} + fn validate_record(record: &InstallationRecord) -> Result<()> { let runtime = record.runtime()?; ensure!( diff --git a/codex-rs/windows-sandbox-service/src/registered_runtime/removal.rs b/codex-rs/windows-sandbox-service/src/registered_runtime/removal.rs index 54de22bb25..507cfad4b6 100644 --- a/codex-rs/windows-sandbox-service/src/registered_runtime/removal.rs +++ b/codex-rs/windows-sandbox-service/src/registered_runtime/removal.rs @@ -22,6 +22,7 @@ use anyhow::ensure; use windows_sys::Win32::Foundation::DUPLICATE_SAME_ACCESS; use windows_sys::Win32::Foundation::DuplicateHandle; use windows_sys::Win32::Foundation::HANDLE; +use windows_sys::Win32::NetworkManagement::NetManagement::UF_ACCOUNTDISABLE; use windows_sys::Win32::System::Pipes::PeekNamedPipe; use windows_sys::Win32::System::SystemInformation::GetSystemDirectoryW; use windows_sys::Win32::System::Threading as threading; @@ -54,23 +55,67 @@ impl PreparedRemoval { } } +/// Restore durable temporary-enable intent before owner restoration or IPC admission. +pub(crate) fn restore_disabled_accounts() -> Result<()> { + let _lock = codex_windows_sandbox::acquire_sandbox_setup_lock(/*timeout_ms*/ 5_000)?; + let Some(mut record) = crate::installation_record::load_runtime()? else { + return Ok(()); + }; + if !crate::installation_record::is_current_package_family(&record)? { + return Ok(()); + } + super::validate_record(&record)?; + for index in 0..record.runtime()?.accounts.len() { + let account = &record.runtime()?.accounts[index]; + if !account.cleanup_logon_pending { + continue; + } + super::validate_account_sid(account)?; + let flags = codex_windows_sandbox::local_user_flags(account.account.username())? + .context("cleanup account disappeared before flag restoration")?; + codex_windows_sandbox::set_local_user_flags( + account.account.username(), + flags | UF_ACCOUNTDISABLE, + )?; + record.runtime_mut()?.accounts[index].cleanup_logon_pending = false; + crate::installation_record::save_runtime(&record)?; + } + Ok(()) +} + // The cleanup entry checked the service family; prepare_cleanup validated this record // under the same setup lock. Fresh logons below still require exact account-SID checks. -pub(crate) fn prepare(owner_token: HANDLE, record: &InstallationRecord) -> Result { +pub(crate) fn prepare( + owner_token: HANDLE, + record: &mut InstallationRecord, +) -> Result { ensure!( - record.runtime()?.retiring.is_some(), - "runtime is not retiring" + record.runtime()?.retiring.is_none(), + "runtime is already retiring" ); let mut tokens = Vec::new(); let mut targets = Vec::new(); - for account in &record.runtime()?.accounts { - if codex_windows_sandbox::local_user_flags(account.account.username())?.is_none() { + for index in 0..record.runtime()?.accounts.len() { + let account = record.runtime()?.accounts[index].clone(); + let Some(flags) = codex_windows_sandbox::local_user_flags(account.account.username())? + else { ensure!( super::registered_packages(&account.user_sid, &record.runtime()?.package_family)? .is_empty(), "runtime account is missing while its package remains registered" ); continue; + }; + // Persist the obligation before changing SAM, so process death cannot lose it. + if flags & UF_ACCOUNTDISABLE != 0 { + record.runtime_mut()?.accounts[index].cleanup_logon_pending = true; + // Older bundled clients also fail closed while account recovery is pending. + record.runtime_mut()?.ready_package = None; + crate::installation_record::save_runtime(record)?; + codex_windows_sandbox::set_local_user_flags( + account.account.username(), + flags & !UF_ACCOUNTDISABLE, + )?; } let token = with_owner_impersonation(owner_token, || { let mut pins = Vec::new(); @@ -82,7 +127,21 @@ pub(crate) fn prepare(owner_token: HANDLE, record: &InstallationRecord) -> Resul &record.codex_home, account.account, ) - })?; + }); + if flags & UF_ACCOUNTDISABLE != 0 { + super::validate_account_sid(&account)?; + let current_flags = + codex_windows_sandbox::local_user_flags(account.account.username())? + .context("cleanup account disappeared before flag restoration")?; + codex_windows_sandbox::set_local_user_flags( + account.account.username(), + current_flags | UF_ACCOUNTDISABLE, + ) + .context("restore disabled sandbox account after cleanup logon")?; + record.runtime_mut()?.accounts[index].cleanup_logon_pending = false; + crate::installation_record::save_runtime(record)?; + } + let token = token?; super::validate_target( token.as_raw_handle() as _, account.account, @@ -94,6 +153,9 @@ pub(crate) fn prepare(owner_token: HANDLE, record: &InstallationRecord) -> Resul })); tokens.push(token); } + // Flag recovery is durable before the in-memory retirement generation exists. + record.runtime_mut()?.ready_package = None; + record.runtime_mut()?.retiring = Some(format!("{:?}", windows::core::GUID::new()?)); let group_sid = if tokens.is_empty() { None } else { diff --git a/codex-rs/windows-sandbox-service/src/service/runtime_lifecycle.rs b/codex-rs/windows-sandbox-service/src/service/runtime_lifecycle.rs index f6f3341c19..76727cc21d 100644 --- a/codex-rs/windows-sandbox-service/src/service/runtime_lifecycle.rs +++ b/codex-rs/windows-sandbox-service/src/service/runtime_lifecycle.rs @@ -53,7 +53,9 @@ pub(super) fn foreground_owner( pub(super) fn run(state: &ServiceState, package_lifecycle: &PackageLifecycle) -> Result<()> { let cleaned = Cell::new(false); - let restore_owner = || { + let last_cleanup_error = Cell::new(None); + let restore_owner = || -> Result<()> { + crate::registered_runtime::restore_disabled_accounts()?; let restored = crate::installation_record::load().and_then(|record| { record.map_or(Ok(()), |record| { package_lifecycle.restore_logged_in_owner(record.session_id) @@ -65,11 +67,12 @@ pub(super) fn run(state: &ServiceState, package_lifecycle: &PackageLifecycle) -> &format!("unable to restore package uninstall listener: {error:#}"), ); } + Ok(()) }; crate::ipc::run( Arc::clone(&state.shutdown), || { - restore_owner(); + restore_owner()?; state.report_status(SERVICE_RUNNING, NO_ERROR)?; log_information( EVENT_SERVICE_STARTED, @@ -81,8 +84,9 @@ pub(super) fn run(state: &ServiceState, package_lifecycle: &PackageLifecycle) -> package_lifecycle.register_authenticated_user(installation, user_token) }, || loop { - restore_owner(); + restore_owner()?; if !crate::package_lifecycle::runtime_owner_removed()? { + last_cleanup_error.set(None); return Ok(()); } let Err(error) = package_lifecycle.clean_up() else { @@ -97,10 +101,7 @@ pub(super) fn run(state: &ServiceState, package_lifecycle: &PackageLifecycle) -> }) { return Err(error); } - log_error( - EVENT_SERVICE_FAILED, - &format!("sandbox cleanup deferred: {error:#}"), - ); + log_cleanup_error(&error, &last_cleanup_error); // Keep the pipe available during pre-fence backoff. A reinstall // resumes admission on the next owner check; SCM stops are never reset. if !wait_for_cleanup_retry(state) { @@ -122,6 +123,7 @@ pub(super) fn run(state: &ServiceState, package_lifecycle: &PackageLifecycle) -> /// Retries the current teardown step, never a phase recovered from stored intent. pub(crate) fn retry_cleanup(mut operation: impl FnMut() -> Result<()>) -> Result<()> { let mut stop_failures = 0; + let last_cleanup_error = Cell::new(None); loop { let Err(error) = operation() else { return Ok(()); @@ -129,10 +131,7 @@ pub(crate) fn retry_cleanup(mut operation: impl FnMut() -> Result<()>) -> Result let Some(state) = SERVICE_STATE.get() else { return Err(error); }; - log_error( - EVENT_SERVICE_FAILED, - &format!("sandbox cleanup deferred: {error:#}"), - ); + log_cleanup_error(&error, &last_cleanup_error); if state.stop_requested.load(Ordering::Acquire) { // SCM stop initiates uninstall cleanup rather than cancelling it. // Four one-second retry waits fit within the 10-second SCM wait hint. @@ -147,6 +146,15 @@ pub(crate) fn retry_cleanup(mut operation: impl FnMut() -> Result<()>) -> Result } } +fn log_cleanup_error(error: &anyhow::Error, previous: &Cell>) { + let message = format!("sandbox cleanup deferred: {error:#}"); + // Retrying an unchanged failure must not flood the Windows event log. + if previous.take().as_deref() != Some(message.as_str()) { + log_error(EVENT_SERVICE_FAILED, &message); + } + previous.set(Some(message)); +} + fn wait_for_cleanup_retry(state: &ServiceState) -> bool { for _ in 0..30 { if state.shutdown.load(Ordering::Acquire) {