From 4150bb3696f07bd04b8010c1c4ed60b193ff16f7 Mon Sep 17 00:00:00 2001 From: Gudge Date: Mon, 18 May 2026 09:57:21 -0700 Subject: [PATCH] fallback_detector: tier-selection module with hardened bfscfg.exe resolution Adds a pure detection module that produces a TierDecision from policy + runtime probes (BaseContainer API, bfscfg.exe presence, WRITE_DAC). Module: - Three-tier model: BaseContainer / AppContainerBfs / AppContainerDacl - allow_dacl_fallback=false blocks any tier requiring host DACL modification (including T1/T2 deny-augmentation) - MXC_FORCE_TIER injection seam (debug builds only) for testing - Pure module: no Logger dependency, no telemetry emission; Phase 4 wires this into the dispatcher Security hardening for the Tier 2 (AppContainer + BFS) path: 1. Environment-driven downgrade. The previous probe read the SystemRoot env var when locating bfscfg.exe, letting an attacker who could influence the process environment force a silent demotion to Tier 3 (DACL) even with the real binary in System32. Release builds now resolve via GetWindowsDirectoryW exclusively, which the OS populates from boot configuration rather than the process environment. 2. Executable-search-order hijacking. FileSystemBfsManager previously invoked CreateProcessW with lpApplicationName = NULL and a bare `bfscfg.exe` command line, exposing the standard image-dir / CWD / System32 / PATH search order to attackers who could drop a malicious bfscfg.exe alongside wxc-exec.exe or earlier on PATH. find_bfscfg_exe now returns the absolute path resolved at probe time and FileSystemBfsManager passes it as lpApplicationName, so Windows loads exactly that binary and skips the search order. Probe and execution share one resolution via TierDecision.bfscfg_path, eliminating any TOCTOU gap between tier decision and broker invocation. Debug-only test seam MXC_BFSCFG_PATH overrides resolution under `#[cfg(debug_assertions)]`; release builds never read it. Follow-up TODO in fallback_detector.rs flags BaseContainerRunner's LoadLibrary on processmodel.dll for the same DLL-search-order audit on the Tier 1 surface. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/Cargo.toml | 1 + src/wxc/src/main.rs | 19 +- src/wxc_common/src/appcontainer_runner.rs | 15 +- src/wxc_common/src/fallback_detector.rs | 729 ++++++++++++++++++++++ src/wxc_common/src/filesystem_bfs.rs | 177 ++++-- src/wxc_common/src/lib.rs | 2 + src/wxc_common/src/process_util.rs | 36 +- 7 files changed, 927 insertions(+), 52 deletions(-) create mode 100644 src/wxc_common/src/fallback_detector.rs diff --git a/src/Cargo.toml b/src/Cargo.toml index f103c1aa2..d1a170abd 100644 --- a/src/Cargo.toml +++ b/src/Cargo.toml @@ -61,6 +61,7 @@ windows = { version = "0.62", features = [ "Win32_System_SystemInformation", "Win32_System_Time", "Win32_System_SystemServices", + "Win32_System_SystemInformation", "Win32_System_JobObjects", ] } windows-core = "0.62" diff --git a/src/wxc/src/main.rs b/src/wxc/src/main.rs index 5a9edc2ba..7b2fae711 100644 --- a/src/wxc/src/main.rs +++ b/src/wxc/src/main.rs @@ -151,8 +151,23 @@ fn print_error_envelope(error: &MxcError) { } fn delete_app_container_profile(name: &str, logger: &mut Logger) -> bool { - // Clear BFS policy first - let mut bfs = FileSystemBfsManager::new(name.to_string()); + // Clear BFS policy first. We need an absolute path to `bfscfg.exe` + // here for the same security reason as the runner — pass an + // authoritative path to `CreateProcessW` rather than a bare name. + // If resolution fails (rare; only on hosts where + // `GetWindowsDirectoryW` itself returns 0), we log and skip the BFS + // clearing step; deleting the AppContainer profile below is still + // worth attempting. + let bfscfg_path = match wxc_common::fallback_detector::find_bfscfg_exe() { + Ok(p) => p, + Err(e) => { + logger.log_line(&format!( + "Skipping BFS policy clear: could not resolve bfscfg.exe ({e})" + )); + None + } + }; + let mut bfs = FileSystemBfsManager::new(name.to_string(), bfscfg_path); bfs.remove_configuration(logger); // Delete the AppContainer profile diff --git a/src/wxc_common/src/appcontainer_runner.rs b/src/wxc_common/src/appcontainer_runner.rs index d568b7df8..9a39e0e87 100644 --- a/src/wxc_common/src/appcontainer_runner.rs +++ b/src/wxc_common/src/appcontainer_runner.rs @@ -599,7 +599,20 @@ impl ScriptRunner for AppContainerScriptRunner { let principal_id = self.get_principal_id(); logger.log_line(&format!("AppContainerSID: {principal_id}")); - let mut bfs_manager = FileSystemBfsManager::new(self.app_container_name.clone()); + // Resolve `bfscfg.exe` by absolute path so probe and execution + // agree on the binary — defeats executable-search-order + // hijacking (see `fallback_detector::find_bfscfg_exe`). On + // hosts where SystemRoot itself cannot be resolved (a + // pathological state on any healthy Windows install) we surface + // the resolution error rather than silently demoting to a + // weaker isolation tier. + let bfscfg_path = match crate::fallback_detector::find_bfscfg_exe() { + Ok(p) => p, + Err(e) => return ScriptResponse::error(&e.to_string()), + }; + + let mut bfs_manager = + FileSystemBfsManager::new(self.app_container_name.clone(), bfscfg_path); if let Err(e) = bfs_manager.configure(&request.policy, logger) { return ScriptResponse::error(&e.to_string()); } diff --git a/src/wxc_common/src/fallback_detector.rs b/src/wxc_common/src/fallback_detector.rs new file mode 100644 index 000000000..c6301bd85 --- /dev/null +++ b/src/wxc_common/src/fallback_detector.rs @@ -0,0 +1,729 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +//! Fallback tier detector. +//! +//! Pure detection module that, given a parsed [`ContainerPolicy`] and a few +//! runtime probes, produces a [`TierDecision`]. Tiers are described in +//! `docs/proposals/downlevel_support/basecontainer-fallback-plan-v2.md`: +//! +//! 1. **Tier 1 — BaseContainer** (`Experimental_CreateProcessInSandbox`) +//! 2. **Tier 2 — AppContainer + BFS** (`bfscfg.exe`-driven filesystem policy) +//! 3. **Tier 3 — AppContainer + DACL** (host-side DACL ACE augmentation) +//! +//! This module does not log, emit telemetry, or have any side effects. It is +//! intentionally Logger-free so it can be unit-tested in isolation. Phase 4 +//! will wire it into the dispatcher in `main.rs`. + +use std::path::{Path, PathBuf}; + +use crate::models::ContainerPolicy; + +/// Selected isolation tier. The variant order corresponds to descending +/// security strength. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum IsolationTier { + /// Tier 1 — `Experimental_CreateProcessInSandbox` from `processmodel.dll`. + BaseContainer, + /// Tier 2 — AppContainer + `bfscfg.exe` BFS filesystem policy. + AppContainerBfs, + /// Tier 3 — AppContainer + DACL-based filesystem policy on host paths. + AppContainerDacl, +} + +impl IsolationTier { + /// Stable kebab-case identifier for serialization. + pub fn as_str(self) -> &'static str { + match self { + IsolationTier::BaseContainer => "base-container", + IsolationTier::AppContainerBfs => "appcontainer-bfs", + IsolationTier::AppContainerDacl => "appcontainer-dacl", + } + } +} + +/// Outcome of [`detect`]: the chosen tier plus any operator-visible warnings +/// gathered while walking the decision algorithm. +#[derive(Debug, Clone)] +pub struct TierDecision { + /// The selected isolation tier. + pub tier: IsolationTier, + /// `true` if this tier needs DACL augmentation on host paths to enforce + /// the policy. T3 always sets this; T1/T2 set it when `denied_paths` is + /// non-empty (since neither BaseContainer nor BFS currently models a + /// "deny" semantic and we have to fall back to host DACLs for those). + pub needs_dacl_augmentation: bool, + /// Absolute path to `bfscfg.exe` as resolved at probe time. + /// + /// Populated only when [`IsolationTier::AppContainerBfs`] is selected. + /// Callers MUST pass this exact path to [`crate::filesystem_bfs`] so + /// that probe and execution agree on the binary — preventing + /// executable-search-order hijacking by an attacker who can plant a + /// rogue `bfscfg.exe` next to `wxc-exec.exe`, in the CWD, or in a + /// `PATH` entry that precedes `System32`. + pub bfscfg_path: Option, + /// Human-readable degradation messages explaining why a higher tier was + /// rejected. Empty when the preferred tier was selected. + pub warnings: Vec, +} + +/// Errors that abort tier selection. +#[derive(Debug, thiserror::Error)] +pub enum FallbackError { + /// The chosen tier needs to modify host DACLs but the caller set + /// `fallback.allow_dacl_mutation = false`. + #[error("DACL fallback required but fallback.allowDaclMutation is false")] + DaclFallbackDisabled, + + /// The current process lacks `WRITE_DAC` on a path that needs ACE + /// augmentation (or the path could not be opened at all). + #[error("WRITE_DAC unavailable on path {path}: {reason}")] + WriteDacUnavailable { + /// The path that failed the probe. + path: PathBuf, + /// The OS-level reason (typically a Win32 error description). + reason: String, + }, + + /// Neither the `GetWindowsDirectoryW` Win32 API call nor (in debug + /// builds) the `MXC_BFSCFG_PATH` override could identify a usable + /// Windows installation directory. We refuse to fall back to a + /// hardcoded `C:\Windows` guess because doing so would allow an + /// attacker who can scrub the process environment to silently + /// downgrade Tier 2 → Tier 3 on hosts where Windows lives elsewhere. + #[error("could not resolve %SystemRoot%: {reason}")] + SystemRootUnresolved { + /// Human-readable description of why resolution failed. + reason: String, + }, +} + +/// Decide which isolation tier to use for a run. +/// +/// The algorithm matches the design doc: +/// +/// 1. If `MXC_FORCE_TIER` is set in a debug build, honor it (test seam). +/// 2. Try Tier 1 (BaseContainer) when `prefer_base_container` is true and the +/// API surface is detected. +/// 3. Otherwise try Tier 2 (AppContainer + BFS) when there's no filesystem +/// policy at all, or `bfscfg.exe` is on disk. +/// 4. Otherwise fall back to Tier 3 (AppContainer + DACL). +/// +/// Any tier that needs to modify host DACLs (T3 always; T1/T2 when +/// `denied_paths` is non-empty) requires `fallback.allow_dacl_mutation = true` +/// and `WRITE_DAC` on every target path. If either check fails the function +/// returns the corresponding [`FallbackError`]. +/// +/// Probing for Tier 2 resolves `%SystemRoot%` exclusively via the +/// `GetWindowsDirectoryW` Win32 API — the `SystemRoot` environment +/// variable is deliberately ignored to deny attackers an +/// environment-driven Tier 2 → Tier 3 downgrade primitive. Callers can +/// receive [`FallbackError::SystemRootUnresolved`] when the OS API itself +/// fails, which on a healthy Windows host should never happen. +pub fn detect( + policy: &ContainerPolicy, + prefer_base_container: bool, +) -> Result { + let denied = !policy.denied_paths.is_empty(); + let has_fs_policy = + !policy.readwrite_paths.is_empty() || !policy.readonly_paths.is_empty() || denied; + + // Test-only injection seam. An invalid value is silently ignored and we + // proceed with the real probe chain — that lets tests assert + // pass-through behavior without any error plumbing. + #[cfg(debug_assertions)] + if let Ok(forced) = std::env::var("MXC_FORCE_TIER") { + if let Some(tier) = parse_force_tier(&forced) { + return forced_decision(tier, policy, denied); + } + } + + let mut warnings: Vec = Vec::new(); + + // Tier 1 — BaseContainer + if prefer_base_container && is_base_container_api_present() { + if denied { + ensure_dacl_augmentation_allowed(policy)?; + verify_write_dac_all(&policy.denied_paths)?; + } + return Ok(TierDecision { + tier: IsolationTier::BaseContainer, + needs_dacl_augmentation: denied, + bfscfg_path: None, + warnings, + }); + } + warnings.push( + "BaseContainer API not present or not preferred; falling back to AppContainer + BFS" + .to_string(), + ); + + // Tier 2 — AppContainer + BFS + // + // When the policy has no filesystem rules at all there is nothing for + // BFS to enforce, so we can stay on T2 without resolving bfscfg.exe. + // Otherwise we need a real path: probe-time resolution doubles as the + // execution path (see `TierDecision::bfscfg_path`). + let bfscfg_path = if has_fs_policy { + find_bfscfg_exe()? + } else { + None + }; + if !has_fs_policy || bfscfg_path.is_some() { + if denied { + ensure_dacl_augmentation_allowed(policy)?; + verify_write_dac_all(&policy.denied_paths)?; + } + return Ok(TierDecision { + tier: IsolationTier::AppContainerBfs, + needs_dacl_augmentation: denied, + bfscfg_path, + warnings, + }); + } + warnings.push("bfscfg.exe not present; falling back to AppContainer + DACL".to_string()); + + // Tier 3 — AppContainer + DACL + ensure_dacl_augmentation_allowed(policy)?; + verify_write_dac_all( + policy + .readwrite_paths + .iter() + .chain(policy.readonly_paths.iter()) + .chain(policy.denied_paths.iter()), + )?; + Ok(TierDecision { + tier: IsolationTier::AppContainerDacl, + needs_dacl_augmentation: true, + bfscfg_path: None, + warnings, + }) +} + +fn ensure_dacl_augmentation_allowed(policy: &ContainerPolicy) -> Result<(), FallbackError> { + if policy.fallback.allow_dacl_mutation { + Ok(()) + } else { + Err(FallbackError::DaclFallbackDisabled) + } +} + +fn verify_write_dac_all>( + paths: impl IntoIterator, +) -> Result<(), FallbackError> { + for p in paths { + check_write_dac_path(p.as_ref())?; + } + Ok(()) +} + +fn check_write_dac_path(path: &Path) -> Result<(), FallbackError> { + match has_write_dac(path) { + Ok(true) => Ok(()), + Ok(false) => Err(FallbackError::WriteDacUnavailable { + path: path.to_path_buf(), + reason: "ERROR_ACCESS_DENIED (WRITE_DAC not granted)".to_string(), + }), + Err(e) => Err(FallbackError::WriteDacUnavailable { + path: path.to_path_buf(), + reason: e.to_string(), + }), + } +} + +#[cfg(debug_assertions)] +fn parse_force_tier(s: &str) -> Option { + match s { + "base-container" => Some(IsolationTier::BaseContainer), + "appcontainer-bfs" => Some(IsolationTier::AppContainerBfs), + "appcontainer-dacl" => Some(IsolationTier::AppContainerDacl), + _ => None, + } +} + +#[cfg(debug_assertions)] +fn forced_decision( + tier: IsolationTier, + policy: &ContainerPolicy, + denied: bool, +) -> Result { + // Forced tiers honor the same DACL-fallback guard the real algorithm + // does: if the operator forbade host DACL changes we must still refuse + // any tier that would touch them. + let needs_dacl = match tier { + IsolationTier::AppContainerDacl => true, + IsolationTier::BaseContainer | IsolationTier::AppContainerBfs => denied, + }; + if needs_dacl && !policy.fallback.allow_dacl_mutation { + return Err(FallbackError::DaclFallbackDisabled); + } + Ok(TierDecision { + tier, + needs_dacl_augmentation: needs_dacl, + bfscfg_path: None, + warnings: Vec::new(), + }) +} + +// --------------------------------------------------------------------------- +// Probes +// --------------------------------------------------------------------------- + +/// Returns `true` when `processmodel.dll!Experimental_CreateProcessInSandbox` +/// can be resolved — i.e. the BaseContainer (Tier 1) API is present on this +/// machine. +pub(crate) fn is_base_container_api_present() -> bool { + crate::base_container_runner::BaseContainerRunner::is_base_container_api_present().is_ok() +} + +/// Returns `Ok(Some(path))` when `bfscfg.exe` is present, where `path` +/// is the **absolute** path the caller MUST pass to +/// `CreateProcessW`'s `lpApplicationName` (or as a quoted absolute +/// argv[0]) so probe and execution agree on which binary they're +/// talking about. +/// +/// Resolution policy: +/// +/// - **Release builds** consult `GetWindowsDirectoryW` exclusively. The +/// `SystemRoot` environment variable is deliberately ignored to deny +/// an attacker who can scrub or rewrite the process environment a +/// Tier 2 → Tier 3 downgrade primitive. +/// - **Debug builds** additionally honor `MXC_BFSCFG_PATH` as a narrow +/// test seam. Its value is used verbatim as the resolved path; an +/// empty value simulates "not present" by returning `Ok(None)`. The +/// release build cannot read this variable at all. +/// - We deliberately do not look in `SysWOW64`: `bfscfg.exe` is shipped +/// only in the native System32 directory. +/// +/// Returns `Err(FallbackError::SystemRootUnresolved)` only when the +/// Win32 API itself fails — on a healthy Windows host this should never +/// happen. +pub fn find_bfscfg_exe() -> Result, FallbackError> { + #[cfg(debug_assertions)] + if let Ok(override_path) = std::env::var("MXC_BFSCFG_PATH") { + if override_path.is_empty() { + return Ok(None); + } + let p = PathBuf::from(override_path); + return Ok(if p.exists() { Some(p) } else { None }); + } + + let mut p = resolve_windows_directory()?; + p.push("System32"); + p.push(crate::filesystem_bfs::BFSCFG_EXE); + Ok(if p.exists() { Some(p) } else { None }) +} + +/// Resolve the Windows install directory via `GetWindowsDirectoryW`. +/// +/// The OS populates the answer from boot configuration; it does not +/// consult the process environment. Returns +/// [`FallbackError::SystemRootUnresolved`] when the API itself fails. +fn resolve_windows_directory() -> Result { + use windows::Win32::System::SystemInformation::GetWindowsDirectoryW; + + // The Windows directory path is always short in practice (e.g. + // `C:\Windows`), but we size for MAX_PATH and grow once if the OS + // asks for more. + let mut buf = vec![0u16; 260]; + // SAFETY: `buf` is a contiguous, writable slice of `u16`. The slice + // length is passed to the API via the `Option<&mut [u16]>` adapter, + // so out-of-bounds writes are impossible. + let len = unsafe { GetWindowsDirectoryW(Some(&mut buf)) } as usize; + if len == 0 { + return Err(FallbackError::SystemRootUnresolved { + reason: "GetWindowsDirectoryW returned 0".to_string(), + }); + } + if len > buf.len() { + buf.resize(len, 0); + // SAFETY: same justification as above; `buf` has been resized to + // the length the API requested. + let len2 = unsafe { GetWindowsDirectoryW(Some(&mut buf)) } as usize; + if len2 == 0 || len2 >= buf.len() { + return Err(FallbackError::SystemRootUnresolved { + reason: format!( + "GetWindowsDirectoryW retry failed (returned {len2}, buffer {})", + buf.len() + ), + }); + } + return parse_utf16(&buf[..len2]); + } + parse_utf16(&buf[..len]) +} + +fn parse_utf16(slice: &[u16]) -> Result { + String::from_utf16(slice) + .map(PathBuf::from) + .map_err(|e| FallbackError::SystemRootUnresolved { + reason: format!("invalid UTF-16 from GetWindowsDirectoryW: {e}"), + }) +} + +// TODO(security follow-up): audit other native-binary lookups for +// executable/DLL search-order hijacking. In particular, +// `BaseContainerRunner::is_base_container_api_present` performs a +// `LoadLibrary` on `processmodel.dll`; verify it uses +// `LOAD_LIBRARY_SEARCH_SYSTEM32` (or an absolute path) so an attacker +// who can plant `processmodel.dll` next to `wxc-exec.exe`, in the CWD, +// or in `PATH` cannot impersonate the Tier 1 API surface. Tracked +// separately from this commit. + +/// Returns `Ok(true)` if the current process holds (or can be granted) +/// `WRITE_DAC` on `path`, `Ok(false)` if the OS reported access denied, and +/// an `Err` for any other failure (e.g. the path does not exist). +pub(crate) fn has_write_dac(path: &Path) -> Result { + use windows::Win32::Foundation::{CloseHandle, ERROR_ACCESS_DENIED}; + use windows::Win32::Storage::FileSystem::{ + CreateFileW, FILE_FLAG_BACKUP_SEMANTICS, FILE_SHARE_DELETE, FILE_SHARE_READ, + FILE_SHARE_WRITE, OPEN_EXISTING, + }; + use windows_core::PCWSTR; + + const WRITE_DAC: u32 = 0x0004_0000; + + let path_str = path + .to_str() + .ok_or_else(|| std::io::Error::new(std::io::ErrorKind::InvalidInput, "non-UTF-8 path"))?; + let wide = crate::string_util::to_wide(path_str); + + // SAFETY: `wide` lives for the duration of the call and is null- + // terminated by `to_wide`. CreateFileW is documented to accept directory + // handles when `FILE_FLAG_BACKUP_SEMANTICS` is set. + let handle = unsafe { + CreateFileW( + PCWSTR(wide.as_ptr()), + WRITE_DAC, + FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, + None, + OPEN_EXISTING, + FILE_FLAG_BACKUP_SEMANTICS, + None, + ) + }; + + match handle { + Ok(h) => { + // SAFETY: `h` is a valid handle returned by CreateFileW. + unsafe { + let _ = CloseHandle(h); + } + Ok(true) + } + Err(e) => { + if e.code() == ERROR_ACCESS_DENIED.to_hresult() { + Ok(false) + } else { + // Only HRESULTs in FACILITY_WIN32 (0x8007xxxx) have a Win32 + // error code embedded in the low 16 bits. For any other + // facility, the masked value is not a valid Win32 error and + // would surface as a misleading `io::Error`. + let hr = e.code().0 as u32; + if (hr & 0xFFFF_0000) == 0x8007_0000 { + Err(std::io::Error::from_raw_os_error((hr & 0xFFFF) as i32)) + } else { + Err(std::io::Error::other(e.to_string())) + } + } + } + } +} + +// --------------------------------------------------------------------------- +// Tests +// --------------------------------------------------------------------------- + +#[cfg(test)] +mod tests { + use super::*; + #[cfg(debug_assertions)] + use crate::models::ContainerPolicy; + #[cfg(debug_assertions)] + use std::sync::Mutex; + + /// Serializes tests that mutate `MXC_FORCE_TIER` because env vars are + /// process-global. We don't have `serial_test` as a dev-dep so we roll + /// our own. Gated with the force-tier mechanism itself. + #[cfg(debug_assertions)] + static ENV_LOCK: Mutex<()> = Mutex::new(()); + + #[cfg(debug_assertions)] + struct ForceTierGuard { + // Holding the lock inside the guard ensures the env-var clear in + // `Drop` happens *before* the lock is released. The previous + // `(Self, MutexGuard)` tuple shape released the lock first (rev. + // declaration order), allowing concurrent tests to observe stale + // `MXC_FORCE_TIER`. + _lock: std::sync::MutexGuard<'static, ()>, + } + #[cfg(debug_assertions)] + impl ForceTierGuard { + fn set(value: &str) -> Self { + let lock = ENV_LOCK.lock().unwrap_or_else(|p| p.into_inner()); + // SAFETY: env-var mutation in tests; serialized by ENV_LOCK. + unsafe { + std::env::set_var("MXC_FORCE_TIER", value); + } + ForceTierGuard { _lock: lock } + } + } + #[cfg(debug_assertions)] + impl Drop for ForceTierGuard { + fn drop(&mut self) { + // SAFETY: serialized by ENV_LOCK still held in `_lock`; the lock + // is released only after this `Drop` returns. + unsafe { + std::env::remove_var("MXC_FORCE_TIER"); + } + } + } + + #[cfg(debug_assertions)] + fn empty_policy() -> ContainerPolicy { + ContainerPolicy::default() + } + + #[cfg(debug_assertions)] + fn policy_with_denied() -> ContainerPolicy { + let mut p = ContainerPolicy::default(); + p.denied_paths.push("C:\\Windows".to_string()); + p + } + + #[cfg(debug_assertions)] + #[test] + fn empty_policy_t1_when_bc_present_and_preferred() { + let _g = ForceTierGuard::set("base-container"); + let policy = empty_policy(); + let d = detect(&policy, true).expect("forced base-container should succeed"); + assert!(matches!(d.tier, IsolationTier::BaseContainer)); + assert!(!d.needs_dacl_augmentation); + assert!(d.warnings.is_empty()); + } + + #[cfg(debug_assertions)] + #[test] + fn empty_policy_no_filesystem_t2_path() { + let _g = ForceTierGuard::set("appcontainer-bfs"); + let policy = empty_policy(); + let d = detect(&policy, true).expect("forced bfs should succeed"); + assert!(matches!(d.tier, IsolationTier::AppContainerBfs)); + assert!(!d.needs_dacl_augmentation); + } + + #[cfg(debug_assertions)] + #[test] + fn denied_paths_disabled_blocks_t1() { + let _g = ForceTierGuard::set("base-container"); + let mut policy = policy_with_denied(); + policy.fallback.allow_dacl_mutation = false; + assert!(matches!( + detect(&policy, true), + Err(FallbackError::DaclFallbackDisabled) + )); + } + + #[cfg(debug_assertions)] + #[test] + fn denied_paths_disabled_blocks_t2() { + let _g = ForceTierGuard::set("appcontainer-bfs"); + let mut policy = policy_with_denied(); + policy.fallback.allow_dacl_mutation = false; + assert!(matches!( + detect(&policy, true), + Err(FallbackError::DaclFallbackDisabled) + )); + } + + #[cfg(debug_assertions)] + #[test] + fn denied_paths_disabled_blocks_t3() { + let _g = ForceTierGuard::set("appcontainer-dacl"); + let mut policy = policy_with_denied(); + policy.fallback.allow_dacl_mutation = false; + assert!(matches!( + detect(&policy, true), + Err(FallbackError::DaclFallbackDisabled) + )); + } + + #[cfg(debug_assertions)] + #[test] + fn force_tier_env_var_parses_all_three_values() { + assert!(matches!( + parse_force_tier("base-container"), + Some(IsolationTier::BaseContainer) + )); + assert!(matches!( + parse_force_tier("appcontainer-bfs"), + Some(IsolationTier::AppContainerBfs) + )); + assert!(matches!( + parse_force_tier("appcontainer-dacl"), + Some(IsolationTier::AppContainerDacl) + )); + } + + #[cfg(debug_assertions)] + #[test] + fn force_tier_env_var_invalid_value_falls_through_to_real_probes() { + // An unrecognized value must NOT raise an error. The detector should + // ignore it and run the real probe chain. Empty filesystem policy + // means the probe chain succeeds regardless of which tier the host + // can satisfy. We assert only the contract — `Ok(_)` — because the + // resulting tier depends on host state (BC API presence, bfscfg + // presence) and any tier-specific check here would be coincidental. + let _g = ForceTierGuard::set("not-a-real-tier"); + let policy = empty_policy(); + detect(&policy, false).expect("invalid value should not error"); + } + + #[test] + fn find_bfscfg_exe_smoke() { + // Tests run in parallel by default and other tests below mutate + // `MXC_BFSCFG_PATH`. We must therefore observe the unset state + // under `ENV_LOCK` so we don't race them. In release builds the + // override is ignored entirely, so the lock is debug-only. + #[cfg(debug_assertions)] + let _lock = { + let lock = ENV_LOCK.lock().unwrap_or_else(|p| p.into_inner()); + // SAFETY: env-var mutation in tests; serialized by ENV_LOCK. + unsafe { + std::env::remove_var("MXC_BFSCFG_PATH"); + } + lock + }; + let result = find_bfscfg_exe().expect("GetWindowsDirectoryW must succeed on Windows"); + if let Some(path) = result { + assert!( + path.is_absolute(), + "find_bfscfg_exe must return an absolute path, got {path:?}" + ); + assert!( + path.ends_with("bfscfg.exe"), + "resolved path {path:?} should end with bfscfg.exe" + ); + } + } + + #[test] + fn resolve_windows_directory_returns_existing_dir() { + // `GetWindowsDirectoryW` always succeeds on any real Windows + // host. We assert the returned path exists; absent that we have + // bigger problems than this test can diagnose. + let resolved = resolve_windows_directory() + .expect("GetWindowsDirectoryW must succeed on a Windows test host"); + assert!( + resolved.is_dir(), + "resolved Windows directory {resolved:?} should be an existing directory" + ); + } + + /// Guard for `MXC_BFSCFG_PATH` mirroring `ForceTierGuard`. Holds + /// `ENV_LOCK` for the duration of the test so concurrent threads + /// don't observe stale values. + #[cfg(debug_assertions)] + struct BfscfgPathGuard { + _lock: std::sync::MutexGuard<'static, ()>, + } + #[cfg(debug_assertions)] + impl BfscfgPathGuard { + fn set(value: &str) -> Self { + let lock = ENV_LOCK.lock().unwrap_or_else(|p| p.into_inner()); + // SAFETY: env-var mutation in tests; serialized by ENV_LOCK. + unsafe { + std::env::set_var("MXC_BFSCFG_PATH", value); + } + BfscfgPathGuard { _lock: lock } + } + } + #[cfg(debug_assertions)] + impl Drop for BfscfgPathGuard { + fn drop(&mut self) { + // SAFETY: serialized by ENV_LOCK still held in `_lock`. + unsafe { + std::env::remove_var("MXC_BFSCFG_PATH"); + } + } + } + + #[cfg(debug_assertions)] + #[test] + fn mxc_bfscfg_path_empty_value_simulates_missing() { + let _g = BfscfgPathGuard::set(""); + let result = find_bfscfg_exe().expect("empty override must succeed"); + assert!( + result.is_none(), + "empty MXC_BFSCFG_PATH must yield Ok(None), got {result:?}" + ); + } + + #[cfg(debug_assertions)] + #[test] + fn mxc_bfscfg_path_nonexistent_path_is_none() { + let _g = BfscfgPathGuard::set("C:\\__mxc_does_not_exist__\\bfscfg.exe"); + let result = find_bfscfg_exe().expect("non-existent override must succeed"); + assert!( + result.is_none(), + "non-existent MXC_BFSCFG_PATH must yield Ok(None), got {result:?}" + ); + } + + #[cfg(debug_assertions)] + #[test] + fn mxc_bfscfg_path_existing_path_is_returned_verbatim() { + // Use a file we know exists (this source file itself, via the + // standard CARGO_MANIFEST_DIR mechanism is not portable here, so + // pin to `cmd.exe` which is always present on Windows). + let cmd_exe = PathBuf::from("C:\\Windows\\System32\\cmd.exe"); + if !cmd_exe.exists() { + // Highly unusual; skip silently rather than fail. + return; + } + let _g = BfscfgPathGuard::set(cmd_exe.to_str().unwrap()); + let result = find_bfscfg_exe().expect("existing override must succeed"); + assert_eq!( + result.as_deref(), + Some(cmd_exe.as_path()), + "MXC_BFSCFG_PATH must be returned verbatim when it exists" + ); + } + + #[test] + fn base_container_api_probe_smoke() { + let _ = is_base_container_api_present(); + } + + #[test] + fn has_write_dac_on_temp_dir() { + let dir = tempfile::tempdir().expect("create temp dir"); + let ok = has_write_dac(dir.path()).expect("temp dir should be openable"); + assert!(ok, "expected WRITE_DAC on freshly-created temp dir"); + } + + #[test] + fn has_write_dac_on_nonexistent_path() { + let bogus = Path::new("C:\\__mxc_definitely_does_not_exist__\\nope.txt"); + let res = has_write_dac(bogus); + assert!( + res.is_err(), + "expected error for non-existent path, got {res:?}" + ); + } + + #[cfg(debug_assertions)] + #[test] + fn compute_decision_with_force_tier_carries_warnings_empty() { + let _g = ForceTierGuard::set("appcontainer-dacl"); + let mut policy = empty_policy(); + policy.fallback.allow_dacl_mutation = true; + let d = detect(&policy, true).expect("forced dacl with allow_dacl_mutation=true"); + assert!(matches!(d.tier, IsolationTier::AppContainerDacl)); + assert!(d.needs_dacl_augmentation); + assert!( + d.warnings.is_empty(), + "forced decisions should not accumulate fallback-chain warnings" + ); + } +} diff --git a/src/wxc_common/src/filesystem_bfs.rs b/src/wxc_common/src/filesystem_bfs.rs index 135cb90f5..66993dc04 100644 --- a/src/wxc_common/src/filesystem_bfs.rs +++ b/src/wxc_common/src/filesystem_bfs.rs @@ -1,24 +1,42 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. +use std::path::PathBuf; + use crate::error::WxcError; use crate::logger::Logger; use crate::models::ContainerPolicy; use crate::process_util; -const BFSCFG_EXE: &str = "bfscfg.exe"; +pub(crate) const BFSCFG_EXE: &str = "bfscfg.exe"; const UNABLE_TO_PERFORM: &str = "Unable to perform policy operation"; const BFSCFG_TIMEOUT_MS: u32 = 10_000; +/// Manages the BFS (Brokered File System) policy for an AppContainer. +/// +/// `bfscfg_path` is the **absolute path** of `bfscfg.exe` resolved at +/// detector probe time (see +/// [`crate::fallback_detector::find_bfscfg_exe`]). It is passed verbatim +/// as `lpApplicationName` to `CreateProcessW` so probe and execution +/// agree on the binary, defeating executable-search-order hijacking. +/// +/// `bfscfg_path` may be `None` when the caller has not yet resolved a +/// path — e.g. cleanup paths that need to be able to delete an +/// AppContainer profile even when the BFS broker is not available on +/// this host. In that case [`Self::configure`] returns an error when the +/// policy actually requires BFS; [`Self::remove_configuration`] is a +/// no-op when nothing was configured. pub struct FileSystemBfsManager { app_container_name: String, + bfscfg_path: Option, configured: bool, } impl FileSystemBfsManager { - pub fn new(app_container_name: String) -> Self { + pub fn new(app_container_name: String, bfscfg_path: Option) -> Self { Self { app_container_name, + bfscfg_path, configured: false, } } @@ -34,7 +52,8 @@ impl FileSystemBfsManager { /// for cleanup of externally-created sandboxes (e.g., BaseContainer profiles /// created by the OS via `CreateProcessInSandbox`). pub fn clear_policy(app_container_name: &str, logger: &mut Logger) { - let mut mgr = Self::new(app_container_name.to_string()); + let bfscfg_path = crate::fallback_detector::find_bfscfg_exe().unwrap_or(None); + let mut mgr = Self::new(app_container_name.to_string(), bfscfg_path); mgr.configured = true; mgr.remove_configuration(logger); } @@ -49,6 +68,13 @@ impl FileSystemBfsManager { return Ok(()); } + if self.bfscfg_path.is_none() { + return Err(WxcError::FilesystemPolicy( + "Cannot configure BFS policy: bfscfg.exe was not resolved at probe time" + .to_string(), + )); + } + for path in &policy.readwrite_paths { let inherit = test_for_root_path(path); if let Err(e) = self.add_bfs_path(path, inherit, logger) { @@ -151,9 +177,33 @@ impl FileSystemBfsManager { } fn run_bfscfg(&self, args: &[&str]) -> Result { - let cmd_line = build_bfscfg_cmd_line(args); - - let output = process_util::run_process_with_captured_output(&cmd_line, BFSCFG_TIMEOUT_MS)?; + // Resolved at probe time; `configure` errors before we get here + // when this is `None`. The `remove_configuration` path also + // requires it via `configured = true`, which only flips after a + // successful `configure` (i.e. after a successful resolve). + let bfscfg_path = self.bfscfg_path.as_ref().ok_or_else(|| { + WxcError::FilesystemPolicy( + "bfscfg.exe path not resolved; refusing to invoke by bare name".to_string(), + ) + })?; + let bfscfg_path_str = bfscfg_path.to_str().ok_or_else(|| { + WxcError::FilesystemPolicy(format!( + "bfscfg.exe path is not valid UTF-8: {}", + bfscfg_path.display() + )) + })?; + let cmd_line = build_bfscfg_cmd_line(bfscfg_path_str, args); + + // Pass `lpApplicationName = Some(bfscfg_path_str)` so Windows + // loads exactly this binary, bypassing the executable search + // order. This is the security-critical half of the absolute- + // path execution policy; the command-line argv[0] is purely + // cosmetic for the child. + let output = process_util::run_process_with_captured_output( + Some(bfscfg_path_str), + &cmd_line, + BFSCFG_TIMEOUT_MS, + )?; let stdout = output.stdout; let stderr = output.stderr; @@ -183,8 +233,23 @@ fn test_for_root_path(path: &str) -> bool { // is sufficient for our needs. Arguments are only quoted when they contain spaces. When quoting // is needed, trailing backslashes are doubled to prevent them from escaping the closing quote // (e.g. `C:\My Folder\` becomes `"C:\My Folder\\"` so bfscfg sees the path correctly). -fn build_bfscfg_cmd_line(args: &[&str]) -> String { - let mut cmd_line = BFSCFG_EXE.to_string(); +// +// `exe_path` is the absolute path of `bfscfg.exe` and becomes argv[0]. It's quoted whenever it +// contains a space, which covers the unusual case of Windows installed under a path with spaces. +// Note that `lpApplicationName` (passed separately to `CreateProcessW`) is the authoritative +// source for *which* binary executes; this command line is only what the child sees as its +// argv. We still include the full absolute path here for tools that scrape it from logs. +fn build_bfscfg_cmd_line(exe_path: &str, args: &[&str]) -> String { + let mut cmd_line = if exe_path.contains(' ') { + let escaped = if exe_path.ends_with('\\') { + format!("{}\\", exe_path) + } else { + exe_path.to_string() + }; + format!("\"{escaped}\"") + } else { + exe_path.to_string() + }; for arg in args { if arg.contains(' ') { // Double any trailing backslashes so they don't escape the closing quote @@ -220,71 +285,97 @@ mod tests { #[test] fn test_new_manager() { - let mgr = FileSystemBfsManager::new("test_container".to_string()); + let mgr = FileSystemBfsManager::new("test_container".to_string(), None); assert!(!mgr.configured()); } #[test] fn test_build_cmd_line_quotes_args_with_spaces() { - let cmd = build_bfscfg_cmd_line(&[ - "--addpolicy", - "--filename", - r"C:\Program Files\PowerShell\7", - "--appid", - "test_container", - ]); + let cmd = build_bfscfg_cmd_line( + r"C:\Windows\System32\bfscfg.exe", + &[ + "--addpolicy", + "--filename", + r"C:\Program Files\PowerShell\7", + "--appid", + "test_container", + ], + ); assert_eq!( cmd, - r#"bfscfg.exe --addpolicy --filename "C:\Program Files\PowerShell\7" --appid test_container"# + r#"C:\Windows\System32\bfscfg.exe --addpolicy --filename "C:\Program Files\PowerShell\7" --appid test_container"# ); } #[test] fn test_build_cmd_line_no_quotes_without_spaces() { - let cmd = build_bfscfg_cmd_line(&[ - "--addpolicy", - "--policybrokerreadonly", - "--filename", - r"C:\Users", - "--appid", - "test", - ]); + let cmd = build_bfscfg_cmd_line( + r"C:\Windows\System32\bfscfg.exe", + &[ + "--addpolicy", + "--policybrokerreadonly", + "--filename", + r"C:\Users", + "--appid", + "test", + ], + ); assert_eq!( cmd, - r"bfscfg.exe --addpolicy --policybrokerreadonly --filename C:\Users --appid test" + r"C:\Windows\System32\bfscfg.exe --addpolicy --policybrokerreadonly --filename C:\Users --appid test" ); } #[test] fn test_build_cmd_line_trailing_backslash() { - let cmd = build_bfscfg_cmd_line(&[ - "--addpolicy", - "--policybrokerreadonly", - "--filename", - r"C:\", - "--appid", - "test", - ]); + let cmd = build_bfscfg_cmd_line( + r"C:\Windows\System32\bfscfg.exe", + &[ + "--addpolicy", + "--policybrokerreadonly", + "--filename", + r"C:\", + "--appid", + "test", + ], + ); // C:\ has no spaces, so no quoting needed — trailing backslash is safe assert_eq!( cmd, - r"bfscfg.exe --addpolicy --policybrokerreadonly --filename C:\ --appid test" + r"C:\Windows\System32\bfscfg.exe --addpolicy --policybrokerreadonly --filename C:\ --appid test" ); } #[test] fn test_build_cmd_line_path_with_spaces_and_trailing_backslash() { - let cmd = build_bfscfg_cmd_line(&[ - "--addpolicy", - "--filename", - r"C:\My Folder\", - "--appid", - "test", - ]); + let cmd = build_bfscfg_cmd_line( + r"C:\Windows\System32\bfscfg.exe", + &[ + "--addpolicy", + "--filename", + r"C:\My Folder\", + "--appid", + "test", + ], + ); // Trailing backslash is doubled inside quotes to prevent escaping the quote assert_eq!( cmd, - r#"bfscfg.exe --addpolicy --filename "C:\My Folder\\" --appid test"# + r#"C:\Windows\System32\bfscfg.exe --addpolicy --filename "C:\My Folder\\" --appid test"# + ); + } + + #[test] + fn test_build_cmd_line_quotes_exe_path_with_spaces() { + // Unusual but legal: Windows installed under a path with spaces. + // The exe path itself must be quoted. + let cmd = build_bfscfg_cmd_line( + r"C:\My Windows\System32\bfscfg.exe", + &["--clearpolicy", "--appid", "test"], + ); + assert_eq!( + cmd, + r#""C:\My Windows\System32\bfscfg.exe" --clearpolicy --appid test"# ); } } diff --git a/src/wxc_common/src/lib.rs b/src/wxc_common/src/lib.rs index 8a4b32bb7..0c4ee0115 100644 --- a/src/wxc_common/src/lib.rs +++ b/src/wxc_common/src/lib.rs @@ -27,6 +27,8 @@ pub mod validator; #[cfg(target_os = "windows")] pub mod appcontainer_runner; #[cfg(target_os = "windows")] +pub mod fallback_detector; +#[cfg(target_os = "windows")] pub mod filesystem_bfs; #[cfg(target_os = "windows")] pub mod job_object; diff --git a/src/wxc_common/src/process_util.rs b/src/wxc_common/src/process_util.rs index 2fcd4640b..4b91565f7 100644 --- a/src/wxc_common/src/process_util.rs +++ b/src/wxc_common/src/process_util.rs @@ -367,9 +367,24 @@ pub struct CapturedOutput { /// Run an external process and capture its stdout/stderr into strings. /// Uses reader threads to avoid pipe deadlocks, with a configurable timeout. /// +/// `application_name`, when `Some`, is passed verbatim as +/// `lpApplicationName` to `CreateProcessW`. This **disables Windows' +/// executable search order** — the OS will load exactly the binary +/// named, with no fallback to the loader's directory, the CWD, the +/// system directories, or `PATH`. Callers that have an authoritative +/// absolute path (resolved from a probe, e.g. via +/// [`crate::fallback_detector::find_bfscfg_exe`]) should pass it here +/// to defeat executable-search-order hijacking. +/// +/// When `application_name` is `None`, the executable is resolved from +/// the first token of `command_line` according to the standard +/// `CreateProcessW` rules — vulnerable to search-order attacks if the +/// first token is a bare name. +/// /// This is used by `FileSystemBfsManager` (for `bfscfg.exe`) and the test /// driver — anywhere we need to inspect process output rather than relay it. pub fn run_process_with_captured_output( + application_name: Option<&str>, command_line: &str, timeout_ms: u32, ) -> Result { @@ -377,7 +392,7 @@ pub fn run_process_with_captured_output( CreateProcessW, GetExitCodeProcess, TerminateProcess, CREATE_NO_WINDOW, PROCESS_INFORMATION, STARTF_USESTDHANDLES, STARTUPINFOW, }; - use windows_core::PWSTR; + use windows_core::{PCWSTR, PWSTR}; // Create pipes (stdin not connected to anything — child gets EOF) let (_stdin_read, _stdin_write) = @@ -397,11 +412,19 @@ pub fn run_process_with_captured_output( }; let mut cmd_wide = string_util::to_wide(command_line); + // When `application_name` is `Some`, keep the wide buffer alive for + // the duration of the `CreateProcessW` call so the `PCWSTR` we pass + // remains valid. + let app_wide: Option> = application_name.map(string_util::to_wide); + let app_pcwstr: PCWSTR = match app_wide.as_ref() { + Some(w) => PCWSTR(w.as_ptr()), + None => PCWSTR::null(), + }; let mut pi = PROCESS_INFORMATION::default(); unsafe { CreateProcessW( - PCWSTR::null(), + app_pcwstr, Some(PWSTR(cmd_wide.as_mut_ptr())), None, None, @@ -820,7 +843,7 @@ mod tests { #[test] fn test_captured_output_stdout() { let output = - run_process_with_captured_output("cmd.exe /c echo hello world", 10000).unwrap(); + run_process_with_captured_output(None, "cmd.exe /c echo hello world", 10000).unwrap(); assert!(output.stdout.trim().contains("hello world")); assert_eq!(output.exit_code, 0); } @@ -828,20 +851,21 @@ mod tests { #[test] fn test_captured_output_stderr() { let output = - run_process_with_captured_output("cmd.exe /c echo error_msg 1>&2", 10000).unwrap(); + run_process_with_captured_output(None, "cmd.exe /c echo error_msg 1>&2", 10000) + .unwrap(); assert!(output.stderr.trim().contains("error_msg")); } #[test] fn test_captured_output_exit_code() { - let output = run_process_with_captured_output("cmd.exe /c exit 42", 10000).unwrap(); + let output = run_process_with_captured_output(None, "cmd.exe /c exit 42", 10000).unwrap(); assert_eq!(output.exit_code, 42); } #[test] fn test_captured_output_timeout() { // Use a very short timeout with a command that sleeps - let result = run_process_with_captured_output("cmd.exe /c ping -n 10 127.0.0.1", 500); + let result = run_process_with_captured_output(None, "cmd.exe /c ping -n 10 127.0.0.1", 500); assert!(result.is_err()); let err_msg = result.unwrap_err().to_string(); assert!(err_msg.contains("timed out"));