From bb71d758cdfe0fda6626a6cfb08e048d920a35cd Mon Sep 17 00:00:00 2001 From: iceweasel-oai Date: Wed, 9 Sep 2026 19:14:45 +0000 Subject: [PATCH] Add telemetry for the Windows system config namespace (#44284) ## What changed Probe the default Windows system config namespace at session start and record `codex.windows_system_config.namespace_squatting_probe` once per process when metrics are available. When `config.toml` or `requirements.toml` exists, check the containing directories for expected ownership and broad standard-user mutation allow ACEs, including permissions inherited by child files. Report coarse results for missing files, expected directories, unexpected types or owners, mutation permissions, and check errors. The probe is observational: it does not change configuration loading or enforce a trust decision. GitOrigin-RevId: e1c79dca5548e16e83c7b84b44ab0de4e2643d0d --- codex-rs/Cargo.lock | 1 + codex-rs/config/Cargo.toml | 1 + codex-rs/config/src/loader/mod.rs | 6 + codex-rs/config/src/loader/windows.rs | 68 ++++++++++++ codex-rs/core/src/config/metrics.rs | 10 ++ codex-rs/core/src/config/mod.rs | 2 + codex-rs/core/src/lib.rs | 2 + codex-rs/core/src/session/session.rs | 2 +- codex-rs/core/src/windows_system_config.rs | 35 ++++++ codex-rs/windows-sandbox-rs/src/acl.rs | 123 ++++++++++++++++++++- codex-rs/windows-sandbox-rs/src/lib.rs | 6 + 11 files changed, 254 insertions(+), 2 deletions(-) create mode 100644 codex-rs/config/src/loader/windows.rs create mode 100644 codex-rs/core/src/config/metrics.rs create mode 100644 codex-rs/core/src/windows_system_config.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index c0fa80e7f0..ac41557e59 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -2867,6 +2867,7 @@ dependencies = [ "codex-utils-path", "codex-utils-path-uri", "codex-utils-redacted-string", + "codex-windows-sandbox", "core-foundation 0.9.4", "dns-lookup", "dunce", diff --git a/codex-rs/config/Cargo.toml b/codex-rs/config/Cargo.toml index 37a1c32439..a6b06169a5 100644 --- a/codex-rs/config/Cargo.toml +++ b/codex-rs/config/Cargo.toml @@ -55,6 +55,7 @@ libc = { workspace = true } core-foundation = "0.9" [target.'cfg(target_os = "windows")'.dependencies] +codex-windows-sandbox = { workspace = true } winapi-util = { workspace = true } windows-sys = { version = "0.52", features = [ "Win32_Foundation", diff --git a/codex-rs/config/src/loader/mod.rs b/codex-rs/config/src/loader/mod.rs index 35f042a48a..82dbcc3d59 100644 --- a/codex-rs/config/src/loader/mod.rs +++ b/codex-rs/config/src/loader/mod.rs @@ -5,6 +5,8 @@ mod macos; mod project_discovery; #[cfg(test)] mod tests; +#[cfg(windows)] +mod windows; use self::layer_io::LoadedConfigLayers; use crate::CONFIG_TOML_FILE; @@ -58,6 +60,10 @@ pub use local::LocalConfigLayers; pub use local::LocalTomlLayer; pub use local::LocalTomlLayerStack; pub use local::load_local_config_layers; +#[cfg(windows)] +pub use windows::WindowsSystemConfigNamespaceProbe; +#[cfg(windows)] +pub use windows::probe_windows_system_config_namespace; #[cfg(unix)] const SYSTEM_CONFIG_TOML_FILE_UNIX: &str = "/etc/codex/config.toml"; diff --git a/codex-rs/config/src/loader/windows.rs b/codex-rs/config/src/loader/windows.rs new file mode 100644 index 0000000000..27375754ab --- /dev/null +++ b/codex-rs/config/src/loader/windows.rs @@ -0,0 +1,68 @@ +use super::windows_codex_system_dir; +use std::path::Path; + +/// Coarse compatibility result for the default Windows system-config namespace. +#[derive(Clone, Copy)] +pub enum WindowsSystemConfigNamespaceProbe { + Missing, + Expected, + UnexpectedType, + UnexpectedOwner, + StandardUserMutationAcl, + CheckError, +} + +/// Observes the common user-preclaimed-directory shape without changing config loading. +/// This is not a trust decision: it only checks directory owner and broad +/// standard-user mutation ACLs. +pub fn probe_windows_system_config_namespace() -> WindowsSystemConfigNamespaceProbe { + let codex_dir = windows_codex_system_dir(); + let has_system_file = match ( + codex_dir.join("config.toml").try_exists(), + codex_dir.join("requirements.toml").try_exists(), + ) { + (Ok(true), _) | (_, Ok(true)) => true, + (Ok(false), Ok(false)) => false, + _ => return WindowsSystemConfigNamespaceProbe::CheckError, + }; + if !has_system_file { + return WindowsSystemConfigNamespaceProbe::Missing; + } + let Some(openai_dir) = codex_dir.parent() else { + return WindowsSystemConfigNamespaceProbe::CheckError; + }; + let result = probe_directory( + openai_dir, + codex_windows_sandbox::path_has_standard_user_mutation_allow, + ); + if !matches!(result, WindowsSystemConfigNamespaceProbe::Expected) { + return result; + } + let result = probe_directory( + codex_dir.as_path(), + codex_windows_sandbox::path_or_child_file_has_standard_user_mutation_allow, + ); + if !matches!(result, WindowsSystemConfigNamespaceProbe::Expected) { + return result; + } + WindowsSystemConfigNamespaceProbe::Expected +} + +fn probe_directory( + path: &Path, + mutation_allow: fn(&Path) -> anyhow::Result, +) -> WindowsSystemConfigNamespaceProbe { + if !path.is_dir() { + return WindowsSystemConfigNamespaceProbe::UnexpectedType; + } + match codex_windows_sandbox::path_has_trusted_system_owner(path) { + Ok(true) => {} + Ok(false) => return WindowsSystemConfigNamespaceProbe::UnexpectedOwner, + Err(_) => return WindowsSystemConfigNamespaceProbe::CheckError, + } + match mutation_allow(path) { + Ok(false) => WindowsSystemConfigNamespaceProbe::Expected, + Ok(true) => WindowsSystemConfigNamespaceProbe::StandardUserMutationAcl, + Err(_) => WindowsSystemConfigNamespaceProbe::CheckError, + } +} diff --git a/codex-rs/core/src/config/metrics.rs b/codex-rs/core/src/config/metrics.rs new file mode 100644 index 0000000000..48d3d71215 --- /dev/null +++ b/codex-rs/core/src/config/metrics.rs @@ -0,0 +1,10 @@ +//! Metrics derived from loaded configuration at session start. + +use super::Config; +use codex_otel::SessionTelemetry; + +pub(crate) fn emit_session_start_metrics(config: &Config, telemetry: &SessionTelemetry) { + config.features.emit_metrics(telemetry); + #[cfg(windows)] + crate::windows_system_config::emit_namespace_squatting_probe(); +} diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index c4053ef20e..1d23a3dc1d 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -164,6 +164,7 @@ use toml_edit::DocumentMut; mod auth_keyring; pub mod edit; mod managed_features; +mod metrics; mod network_proxy_spec; mod otel; mod permission_profile_catalog; @@ -186,6 +187,7 @@ pub use codex_network_proxy::NetworkProxyAuditMetadata; use codex_sandboxing::compatibility_sandbox_policy_for_permission_profile; pub use codex_sandboxing::system_bwrap_warning; pub use managed_features::ManagedFeatures; +pub(crate) use metrics::emit_session_start_metrics; pub use network_proxy_spec::NetworkProxySpec; pub use network_proxy_spec::StartedNetworkProxy; pub use permission_profile_catalog::PermissionProfileCatalogEntry; diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index 3d21c8ca04..55d1f474e5 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -111,6 +111,8 @@ mod stream_events_utils; pub mod test_support; mod unified_exec; pub mod windows_sandbox; +#[cfg(windows)] +mod windows_system_config; pub use client::X_RESPONSESAPI_INCLUDE_TIMING_METRICS_HEADER; pub use codex_protocol::config_types::ModelProviderAuthInfo; mod event_mapping; diff --git a/codex-rs/core/src/session/session.rs b/codex-rs/core/src/session/session.rs index 2c3ea5fc9e..298ffb8759 100644 --- a/codex-rs/core/src/session/session.rs +++ b/codex-rs/core/src/session/session.rs @@ -1143,7 +1143,7 @@ impl Session { model: Some(session_model.clone()), slug: Some(session_model), }; - config.features.emit_metrics(&session_telemetry); + crate::config::emit_session_start_metrics(config.as_ref(), &session_telemetry); let is_worktree = session_configuration.cwd().canonicalize().ok().and_then(|cwd| { codex_git_utils::repository_identity(&cwd).and_then(|_| { get_git_repo_root(&cwd).map(|root| root.join(".git").is_file()) diff --git a/codex-rs/core/src/windows_system_config.rs b/codex-rs/core/src/windows_system_config.rs new file mode 100644 index 0000000000..bca27d47d6 --- /dev/null +++ b/codex-rs/core/src/windows_system_config.rs @@ -0,0 +1,35 @@ +//! Telemetry for the default Windows system-config namespace. + +use codex_config::loader::WindowsSystemConfigNamespaceProbe as Probe; +use std::sync::atomic::AtomicBool; +use std::sync::atomic::Ordering; + +const NAMESPACE_SQUATTING_PROBE_METRIC: &str = + "codex.windows_system_config.namespace_squatting_probe"; +static NAMESPACE_SQUATTING_PROBE_RECORDED: AtomicBool = AtomicBool::new(false); + +/// Records the coarse system-config namespace probe once after metrics exist. +pub(crate) fn emit_namespace_squatting_probe() { + let Some(metrics) = codex_otel::global() else { + return; + }; + if NAMESPACE_SQUATTING_PROBE_RECORDED + .compare_exchange(false, true, Ordering::Relaxed, Ordering::Relaxed) + .is_err() + { + return; + } + let result = match codex_config::loader::probe_windows_system_config_namespace() { + Probe::Missing => "missing", + Probe::Expected => "expected", + Probe::UnexpectedType => "unexpected_type", + Probe::UnexpectedOwner => "unexpected_owner", + Probe::StandardUserMutationAcl => "standard_user_acl", + Probe::CheckError => "check_error", + }; + let _ = metrics.counter( + NAMESPACE_SQUATTING_PROBE_METRIC, + /*inc*/ 1, + &[("result", result)], + ); +} diff --git a/codex-rs/windows-sandbox-rs/src/acl.rs b/codex-rs/windows-sandbox-rs/src/acl.rs index e35777ab78..4dbbb155ce 100644 --- a/codex-rs/windows-sandbox-rs/src/acl.rs +++ b/codex-rs/windows-sandbox-rs/src/acl.rs @@ -1,3 +1,5 @@ +use crate::winutil::resolve_sid; +use crate::winutil::sid_bytes_from_string; use crate::winutil::to_wide; use anyhow::Result; use anyhow::anyhow; @@ -28,7 +30,9 @@ use windows_sys::Win32::Security::EqualSid; use windows_sys::Win32::Security::GENERIC_MAPPING; use windows_sys::Win32::Security::GetAce; use windows_sys::Win32::Security::GetAclInformation; +use windows_sys::Win32::Security::IsValidAcl; use windows_sys::Win32::Security::MapGenericMask; +use windows_sys::Win32::Security::OWNER_SECURITY_INFORMATION; use windows_sys::Win32::Storage::FileSystem::CreateFileW; use windows_sys::Win32::Storage::FileSystem::DELETE; use windows_sys::Win32::Storage::FileSystem::FILE_ALL_ACCESS; @@ -47,7 +51,10 @@ 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; use windows_sys::Win32::Storage::FileSystem::READ_CONTROL; +use windows_sys::Win32::Storage::FileSystem::WRITE_DAC; +use windows_sys::Win32::Storage::FileSystem::WRITE_OWNER; const SE_KERNEL_OBJECT: u32 = 6; +const OBJECT_INHERIT_ACE_FLAG: u8 = 0x01; const INHERIT_ONLY_ACE: u8 = 0x08; const INHERITED_ACE: u8 = 0x10; const ACCESS_ALLOWED_ACE_TYPE: u8 = 0; @@ -55,6 +62,17 @@ const ACCESS_DENIED_ACE_TYPE: u8 = 1; const GENERIC_READ_MASK: u32 = 0x8000_0000; const GENERIC_WRITE_MASK: u32 = 0x4000_0000; const DENY_ACCESS: i32 = 3; +// TrustedInstaller is a deterministic service SID, not a machine-local account SID. +const TRUSTED_INSTALLER_SID: &str = + "S-1-5-80-956008885-3418522649-1831038044-1853292631-2271478464"; +const STANDARD_USER_MUTATION_MASK: u32 = FILE_WRITE_DATA + | FILE_APPEND_DATA + | FILE_WRITE_EA + | FILE_WRITE_ATTRIBUTES + | DELETE + | FILE_DELETE_CHILD + | WRITE_DAC + | WRITE_OWNER; fn acl_api_result(path: &Path, operation: &str, code: u32) -> Result<()> { if code == ERROR_SUCCESS { @@ -126,6 +144,7 @@ pub unsafe fn dacl_mask_allows( enum AceScope { Effective, Explicit, + EffectiveOrChildFile, } unsafe fn dacl_mask_allows_with_scope( @@ -163,7 +182,10 @@ unsafe fn dacl_mask_allows_with_scope( if hdr.AceType != ACCESS_ALLOWED_ACE_TYPE { continue; // not ACCESS_ALLOWED } - if (hdr.AceFlags & INHERIT_ONLY_ACE) != 0 { + if (hdr.AceFlags & INHERIT_ONLY_ACE) != 0 + && (!matches!(scope, AceScope::EffectiveOrChildFile) + || (hdr.AceFlags & OBJECT_INHERIT_ACE_FLAG) == 0) + { continue; } // SET_ACCESS cannot replace an ACE inherited from an ancestor, so it cannot make @@ -212,6 +234,105 @@ pub fn path_mask_allows( ) } +/// Whether a broad standard-user principal has an allow ACE that can mutate +/// the path. This is intentionally conservative and does not evaluate deny +/// ACEs against a concrete token. +pub fn path_has_standard_user_mutation_allow(path: &Path) -> Result { + path_has_standard_user_mutation_allow_with_scope(path, AceScope::Effective) +} + +/// Whether a broad standard-user principal has an allow ACE that can mutate +/// the path or a directly inherited child file. +pub fn path_or_child_file_has_standard_user_mutation_allow(path: &Path) -> Result { + path_has_standard_user_mutation_allow_with_scope(path, AceScope::EffectiveOrChildFile) +} + +fn path_has_standard_user_mutation_allow_with_scope(path: &Path, scope: AceScope) -> Result { + // These canonical labels map to fixed SIDs before any localized account lookup. + let mut users = resolve_sid("Users")?; + let mut authenticated_users = resolve_sid("Authenticated Users")?; + let mut everyone = resolve_sid("Everyone")?; + let sids = [ + users.as_mut_ptr().cast(), + authenticated_users.as_mut_ptr().cast(), + everyone.as_mut_ptr().cast(), + ]; + unsafe { + let (dacl, descriptor) = fetch_dacl_handle(path)?; + let result = if dacl.is_null() { + Ok(true) + } else if IsValidAcl(dacl) == 0 { + Err(anyhow!("invalid DACL for {}", path.display())) + } else { + Ok(dacl_mask_allows_with_scope( + dacl, + &sids, + STANDARD_USER_MUTATION_MASK, + /*require_all_bits*/ false, + scope, + )) + }; + if !descriptor.is_null() { + LocalFree(descriptor as HLOCAL); + } + result + } +} + +/// Whether the path owner is one of the conservative administrator-controlled +/// principals used by the Windows system-config shadow check. +pub fn path_has_trusted_system_owner(path: &Path) -> Result { + // These canonical labels map to fixed SIDs before any localized account lookup. + let mut administrators = resolve_sid("Administrators")?; + let mut system = resolve_sid("SYSTEM")?; + let mut trusted_installer = sid_bytes_from_string(TRUSTED_INSTALLER_SID)?; + let trusted_sids = [ + administrators.as_mut_ptr().cast(), + system.as_mut_ptr().cast(), + trusted_installer.as_mut_ptr().cast(), + ]; + path_owner_matches(path, &trusted_sids) +} + +fn path_owner_matches(path: &Path, trusted_sids: &[*mut c_void]) -> Result { + let mut owner = std::ptr::null_mut(); + let mut descriptor = std::ptr::null_mut(); + let code = unsafe { + GetNamedSecurityInfoW( + to_wide(path).as_ptr(), + 1, // SE_FILE_OBJECT + OWNER_SECURITY_INFORMATION, + &mut owner, + std::ptr::null_mut(), + std::ptr::null_mut(), + std::ptr::null_mut(), + &mut descriptor, + ) + }; + if code != ERROR_SUCCESS { + if !descriptor.is_null() { + unsafe { LocalFree(descriptor as HLOCAL) }; + } + return Err(anyhow!( + "GetNamedSecurityInfoW failed for {}: {code}", + path.display() + )); + } + if owner.is_null() { + if !descriptor.is_null() { + unsafe { LocalFree(descriptor as HLOCAL) }; + } + return Err(anyhow!("missing owner for {}", path.display())); + } + let matches = trusted_sids + .iter() + .any(|trusted_sid| unsafe { EqualSid(owner, *trusted_sid) } != 0); + if !descriptor.is_null() { + unsafe { LocalFree(descriptor as HLOCAL) }; + } + Ok(matches) +} + fn path_mask_allows_with_scope( path: &Path, psids: &[*mut c_void], diff --git a/codex-rs/windows-sandbox-rs/src/lib.rs b/codex-rs/windows-sandbox-rs/src/lib.rs index 1f6d236da5..e83a42a59f 100644 --- a/codex-rs/windows-sandbox-rs/src/lib.rs +++ b/codex-rs/windows-sandbox-rs/src/lib.rs @@ -170,8 +170,14 @@ pub use acl::ensure_allow_write_aces; #[cfg(target_os = "windows")] pub use acl::fetch_dacl_handle; #[cfg(target_os = "windows")] +pub use acl::path_has_standard_user_mutation_allow; +#[cfg(target_os = "windows")] +pub use acl::path_has_trusted_system_owner; +#[cfg(target_os = "windows")] pub use acl::path_mask_allows; #[cfg(target_os = "windows")] +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 audit::apply_world_writable_scan_and_denies_for_permissions;