Skip to content

Phase 2: fallback_detector module for tier selection#294

Merged
MGudgin merged 1 commit into
mainfrom
user/gudge/downlevel_phase2
May 18, 2026
Merged

Phase 2: fallback_detector module for tier selection#294
MGudgin merged 1 commit into
mainfrom
user/gudge/downlevel_phase2

Conversation

@MGudgin

@MGudgin MGudgin commented May 12, 2026

Copy link
Copy Markdown
Member

📖 Description

Adds a pure detection module that produces a TierDecision from policy + runtime probes (BaseContainer API, bfscfg.exe presence, WRITE_DAC), plus security hardening for the Tier 2 (AppContainer + BFS) execution path.

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

  1. Environment-driven Tier 2 → Tier 3 downgrade. The original probe read the SystemRoot env var when locating bfscfg.exe, letting an attacker who could influence the process environment (parent process, persisted user env, service definition) force a silent demotion to Tier 3 (DACL) even with the real binary in System32. Release builds now resolve via GetWindowsDirectoryW exclusively — the OS populates it from boot configuration, not 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 → System → Windows → PATH search order. An attacker who could drop bfscfg.exe next to wxc-exec.exe, in CWD, or earlier on PATH would have their binary executed as the BFS broker regardless of what the probe concluded. find_bfscfg_exe now returns the absolute path resolved at probe time; 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 — single source of truth, no 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, so it cannot be used as an attack vector in production.

A follow-up TODO in fallback_detector.rs flags BaseContainerRunners LoadLibrary on processmodel.dll as the next surface to audit for the same DLL-search-order family of attacks (Tier 1 surface).

🔗 References

Stacked on top of #293 (phase1_integrate → phase1d).

🔍 Validation

  • cargo fmt --all -- --check clean
  • cargo clippy --workspace --all-targets -- -D warnings clean
  • cargo test -p wxc_common — 357 tests pass (16 fallback_detector incl. 4 new for the MXC_BFSCFG_PATH seam; 8 filesystem_bfs incl. 1 new for absolute-path quoting)
  • End-to-end coverage lives in Win25H2Safe-Tests.ps1 in phase5 (held)

@MGudgin MGudgin force-pushed the user/gudge/downlevel_phase1_integrate branch from 2b7f10a to 5ff6459 Compare May 12, 2026 21:06
@MGudgin MGudgin force-pushed the user/gudge/downlevel_phase2 branch from a94ed8f to 7013144 Compare May 12, 2026 21:07
@MGudgin MGudgin force-pushed the user/gudge/downlevel_phase1_integrate branch from 5ff6459 to 374c03b Compare May 12, 2026 21:17
@MGudgin MGudgin force-pushed the user/gudge/downlevel_phase2 branch from 7013144 to a9137c0 Compare May 12, 2026 21:17
@MGudgin MGudgin force-pushed the user/gudge/downlevel_phase1_integrate branch from 374c03b to cddf163 Compare May 12, 2026 21:32
@MGudgin MGudgin force-pushed the user/gudge/downlevel_phase2 branch from a9137c0 to da91c35 Compare May 12, 2026 21:32
@MGudgin MGudgin force-pushed the user/gudge/downlevel_phase1_integrate branch from cddf163 to 33af9d6 Compare May 12, 2026 22:38
@MGudgin MGudgin force-pushed the user/gudge/downlevel_phase2 branch from da91c35 to ebecee9 Compare May 12, 2026 22:38
@MGudgin MGudgin force-pushed the user/gudge/downlevel_phase1_integrate branch from 33af9d6 to c2d1bb8 Compare May 14, 2026 13:51
@MGudgin MGudgin force-pushed the user/gudge/downlevel_phase2 branch from ebecee9 to 2827445 Compare May 14, 2026 13:51
@MGudgin MGudgin force-pushed the user/gudge/downlevel_phase1_integrate branch 3 times, most recently from 1cc2ced to 8cc6e01 Compare May 14, 2026 19:16
Base automatically changed from user/gudge/downlevel_phase1_integrate to main May 14, 2026 21:32
Copilot AI review requested due to automatic review settings May 14, 2026 22:02
@MGudgin MGudgin force-pushed the user/gudge/downlevel_phase2 branch 2 times, most recently from 849884d to 3071668 Compare May 14, 2026 22:04

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds a new fallback_detector module that selects an IsolationTier (BaseContainer / AppContainerBfs / AppContainerDacl) based on ContainerPolicy plus runtime probes (BaseContainer API, bfscfg.exe, WRITE_DAC). Pure module with a debug-only MXC_FORCE_TIER test seam; no logging or telemetry.

Changes:

  • New fallback_detector module with detect(), TierDecision, FallbackError, and runtime probes.
  • Promoted BFSCFG_EXE to pub(crate) so the detector can reuse the constant.
  • Registered the new module in wxc_common's lib.rs under cfg(target_os = "windows").
Show a summary per file
File Description
src/wxc_common/src/lib.rs Exposes the new fallback_detector module on Windows.
src/wxc_common/src/filesystem_bfs.rs Widens visibility of BFSCFG_EXE to pub(crate) for reuse.
src/wxc_common/src/fallback_detector.rs New pure tier-selection module with probes, force-tier seam, and unit tests.

Copilot's findings

  • Files reviewed: 3/3 changed files
  • Comments generated: 5

Comment thread src/wxc_common/src/fallback_detector.rs Outdated
Comment thread src/wxc_common/src/fallback_detector.rs
Comment thread src/wxc_common/src/fallback_detector.rs
Comment thread src/wxc_common/src/fallback_detector.rs Outdated
Comment thread src/wxc_common/src/fallback_detector.rs Outdated
@MGudgin MGudgin force-pushed the user/gudge/downlevel_phase2 branch 4 times, most recently from 08062b6 to ba96205 Compare May 14, 2026 23:15
Comment thread src/wxc_common/src/fallback_detector.rs Outdated
@MGudgin MGudgin force-pushed the user/gudge/downlevel_phase2 branch 5 times, most recently from a54589c to 6e46423 Compare May 18, 2026 17:52
@MGudgin MGudgin force-pushed the user/gudge/downlevel_phase2 branch from 6e46423 to 77574f9 Compare May 18, 2026 18:55
@MGudgin MGudgin requested review from bbonaby and jsidewhite May 18, 2026 19:29
Comment thread 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 {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete_app_container_profile

Annoying the cleanup function isn't in the same file as the setup function (AC_runner.rs).

///
/// - **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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a real threat? If an attacker can manipulate the environment of wxc-exec.exe, then it's it's already in.

Is this merely defense in depth?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ohhh it's moreso this,

preventing
    /// executable-search-order hijacking by an attacker who can plant a
    /// rogue `bfscfg.exe` next to `wxc-exec.exe`

format!("\"{escaped}\"")
} else {
exe_path.to_string()
};

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't you use the path crate?


use std::path::Path;

let p = Path::new(r"C:\Windows").join("System32").join("kernel32.dll");

or


use std::path::PathBuf;

let mut path = PathBuf::from(r"C:\");
path.push("Windows");
path.push("System32");
path.set_extension("dll");

println!("{}", path.display());

…olution

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>
@MGudgin MGudgin force-pushed the user/gudge/downlevel_phase2 branch from 77574f9 to 4150bb3 Compare May 18, 2026 20:46

@jsidewhite jsidewhite left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@MGudgin MGudgin merged commit 36bf2b3 into main May 18, 2026
18 checks passed
@MGudgin MGudgin deleted the user/gudge/downlevel_phase2 branch May 18, 2026 21:25
MGudgin pushed a commit that referenced this pull request May 19, 2026
Move `delete_app_container_profile` from `src/wxc/src/main.rs` into
`src/wxc_common/src/appcontainer_runner.rs`, next to `AppContainerScriptRunner`
(which owns the create/setup path). Both ends of the AppContainer profile
lifecycle now live in the same module.

While moving the function, replace the open-coded
`FileSystemBfsManager::new(...) + remove_configuration(...)` block with
`FileSystemBfsManager::clear_policy(name, logger)`. The original code created a
fresh manager and called `remove_configuration` without first setting
`configured = true`, so the early-return in `remove_configuration` made the
BFS-clear step a silent no-op for `wxc-exec --delete`. `clear_policy` is the
helper purpose-built for externally-created sandboxes and sets
`configured = true` itself, so the cleanup actually runs now.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
MGudgin added a commit that referenced this pull request May 20, 2026
Move `delete_app_container_profile` from `src/wxc/src/main.rs` into
`src/wxc_common/src/appcontainer_runner.rs`, next to `AppContainerScriptRunner`
(which owns the create/setup path). Both ends of the AppContainer profile
lifecycle now live in the same module.

While moving the function, replace the open-coded
`FileSystemBfsManager::new(...) + remove_configuration(...)` block with
`FileSystemBfsManager::clear_policy(name, logger)`. The original code created a
fresh manager and called `remove_configuration` without first setting
`configured = true`, so the early-return in `remove_configuration` made the
BFS-clear step a silent no-op for `wxc-exec --delete`. `clear_policy` is the
helper purpose-built for externally-created sandboxes and sets
`configured = true` itself, so the cleanup actually runs now.

Co-authored-by: Gudge <gudge@microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
huzaifa-d pushed a commit that referenced this pull request May 21, 2026
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: Gudge <gudge@microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants