Add shared plumbing for the Windows Sandbox rewrite (Phase 0) by MGudgin · Pull Request #576 · microsoft/mxc

MGudgin · 2026-06-26T18:20:45Z

Summary

Phase 0 of the Windows Sandbox rewrite stack: land the small, additive
wxc_common plumbing first so the larger lifecycle/one-shot PR stays focused.
Nothing on main references the new items yet, so this merges independently.

Details

filesystem_dacl: add set_owner_only_dacl (+ owner_is_self and the
current_user_sid / current_token_owner_sid / self_owner_sids helpers)
to stamp an owner-only, optionally-inheritable, PROTECTED DACL (current user
- Local SYSTEM) on a path MXC owns. owner_is_self fails closed when a
  shared-temp path was pre-created by another user (a foreign owner retains
  implicit WRITE_DAC/READ_CONTROL regardless of the DACL). This is the
  primitive the rewrite's nonce / state-aware daemon.json files use to keep
  other users on a shared host off them.
models: add two FailurePhase variants -- Rejected (request can't be
honored without changing input/host) and PostLaunchFailed (guest/sandbox
infra failed after a successful launch) -- and sharpen the doc comments on
the existing variants. Purely additive: the only match on FailurePhase
(mxc-sdk map_spawn_error) has a wildcard arm, so adding variants is
non-breaking.

Tests

cargo test -p wxc_common: 363 passed, including the new
set_owner_only_dacl_strips_inheritance_and_grants_owner, owner_is_self_*,
and failure_phase_serde_round_trips_all_variants.
cargo fmt -p wxc_common -- --check and
cargo clippy -p wxc_common --all-targets -- -D warnings: clean.
cargo check -p mxc-sdk -p appcontainer_common: clean (verified the new
enum variants don't break downstream consumers).

Related Issues

Part of the Windows Sandbox rewrite (user/gudge/windows_sandbox_rewrite),
split into a 0-3 stacked-PR series; this is Phase 0.

Microsoft Reviewers: Open in CodeFlow

This PR adds the small, additive `wxc_common` pieces that the upcoming Windows Sandbox rewrite stack builds on, landed first so the larger lifecycle/one-shot PR stays focused. Details * filesystem_dacl: add `set_owner_only_dacl` (+ `owner_is_self` and the `current_user_sid` / `current_token_owner_sid` / `self_owner_sids` helpers) to stamp an owner-only, optionally-inheritable DACL on a path. This is the primitive the rewrite's nonce/daemon-record files use to keep other users off them. * models: add two `FailurePhase` variants -- `Rejected` (request can't be honored without changing input/host) and `PostLaunchFailed` (guest/ sandbox infra failed after a successful launch) -- and sharpen the doc comments on the existing variants. Purely additive: the only `match` on `FailurePhase` (mxc-sdk `map_spawn_error`) has a wildcard arm, so adding variants is non-breaking. Tests * `cargo test -p wxc_common`: 363 passed, including the new `set_owner_only_dacl_strips_inheritance_and_grants_owner`, `owner_is_self_*`, and `failure_phase_serde_round_trips_all_variants`. * `cargo fmt -p wxc_common -- --check` and `cargo clippy -p wxc_common --all-targets -- -D warnings`: clean. * `cargo check -p mxc-sdk -p appcontainer_common`: clean (verified the new enum variants don't break downstream consumers). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Generated-with: claude-opus-4.8

Copilot

Pull request overview

Adds foundational, additive wxc_common primitives intended to support the upcoming Windows Sandbox rewrite without yet wiring them into mainline execution paths.

Changes:

Introduces FailurePhase::Rejected and FailurePhase::PostLaunchFailed, and refines phase documentation for clearer error classification.
Adds Windows filesystem security helpers to detect “foreign-owned” paths (owner_is_self) and to stamp an owner-only, PROTECTED DACL (set_owner_only_dacl), plus unit tests around the new behavior.

Show a summary per file

File	Description
`src/core/wxc_common/src/models.rs`	Adds new `FailurePhase` variants and a serde round-trip test for the enum.
`src/core/wxc_common/src/filesystem_dacl.rs`	Adds helpers to read token/user/owner SIDs, validate filesystem ownership, and apply a protected owner-only DACL with tests.

Review details

Files reviewed: 2/2 changed files
Comments generated: 2
Review effort level: Low

+    #[test]
+    fn failure_phase_serde_round_trips_all_variants() {
+        let cases = [
+            (FailurePhase::None, "\"None\""),
+            (FailurePhase::LaunchFailed, "\"LaunchFailed\""),
+            (FailurePhase::Rejected, "\"Rejected\""),
+            (FailurePhase::PostLaunchFailed, "\"PostLaunchFailed\""),
+            (FailurePhase::ProcessExited, "\"ProcessExited\""),
+            (FailurePhase::BackendUnavailable, "\"BackendUnavailable\""),
+        ];
+        for (variant, wire) in cases {
+            let s = serde_json::to_string(&variant).unwrap();
+            assert_eq!(s, wire, "serialize {variant:?}");
+            let back: FailurePhase = serde_json::from_str(wire).unwrap();
+            assert_eq!(back, variant, "round-trip {wire}");
+        }
+    }


+pub fn set_owner_only_dacl(path: &Path, inheritable: bool) -> Result<(), DaclError> {
+    let user = current_user_sid()?;
+    let system = OwnedSid::parse("S-1-5-18")?; // Local SYSTEM.
+


dhoehna · 2026-06-26T18:59:31Z

+        OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY, &mut token)
+            .map_err(|e| win32_err_str(token_path, &format!("OpenProcessToken: {e}")))?;
+
+        let mut len = 0u32;


Missed adding the comment // First call sizes the buffer (returns ERROR_INSUFFICIENT_BUFFER).

dhoehna · 2026-06-26T19:00:25Z

+            .map_err(|e| win32_err_str(token_path, &format!("OpenProcessToken: {e}")))?;
+
+        let mut len = 0u32;
+        let _ = GetTokenInformation(token, TokenOwner, None, 0, &mut len);


Why is throwing away the GetTokenInformation result okay? I am suspicious when return codes are ignored.

dhoehna · 2026-06-26T19:02:21Z

+/// be owned by: the token user SID and (when elevated) the token owner SID.
+fn self_owner_sids() -> Result<Vec<OwnedSid>, DaclError> {
+    let mut sids = vec![current_user_sid()?];
+    // Best-effort: only meaningful (and only differs) under elevation.


What is the difference under elevation and why is it meaningful?

dhoehna · 2026-06-26T19:08:55Z

+        }
+    }
+    unsafe {
+        let _ = LocalFree(Some(HLOCAL(sd.0)));


I'm new to rust and I might not understand the code flow correctly. Is LocalFree called if the code returns on line 1311?

dhoehna · 2026-06-26T19:09:17Z

+    Ok(is_self)
+}
+
+///


NIT: newline with no comment.

dhoehna · 2026-06-26T19:10:51Z

+        grfInheritance: ACE_FLAGS(inheritance),
+        Trustee: trustee_for(sid),
+    };
+    let ea = [grant(&user), grant(&system)];


What does ea represent? Each Access, Every Access, Early Access? I can not figure it out from code.

dhoehna · 2026-06-26T19:12:00Z

-    // Note: the analogous compile-time `const _: () = assert!(...)`
+    /// A directory THIS process creates must be reported as owned by us.
+    #[test]
+    fn owner_is_self_true_for_dir_we_create() {


+1 for including the expected result in the test name. :)

dhoehna

I do not know the architecture of the code base and I do not feel comfortable approving the PR.

A comment for the change in general. Consider expanding variable names to describe what they are for instead of the type. Aside from well known monikers like rc.

Otherwise the code looks sound.

Copilot AI review requested due to automatic review settings June 26, 2026 18:20

MGudgin requested a review from a team as a code owner June 26, 2026 18:20

Copilot started reviewing on behalf of MGudgin June 26, 2026 18:22 View session

Copilot AI reviewed Jun 26, 2026

View reviewed changes

MGudgin mentioned this pull request Jun 26, 2026

Rewrite the Windows Sandbox one-shot path onto an authenticated protocol (Phase 1) #577

Open

3 tasks

dhoehna reviewed Jun 26, 2026

View reviewed changes

MGudgin mentioned this pull request Jun 26, 2026

Add the Windows Sandbox state-aware lifecycle + host daemon (Phase 2) #578

Open

3 tasks

dhoehna reviewed Jun 26, 2026

View reviewed changes

Comment thread src/core/wxc_common/src/filesystem_dacl.rs

Ok(is_self)

}

///

dhoehna Jun 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

NIT: newline with no comment.

dhoehna reviewed Jun 26, 2026

View reviewed changes

MGudgin mentioned this pull request Jun 26, 2026

Add SDK state-aware support for the Windows Sandbox backend (Phase 3) #579

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add shared plumbing for the Windows Sandbox rewrite (Phase 0)#576

Add shared plumbing for the Windows Sandbox rewrite (Phase 0)#576
MGudgin wants to merge 1 commit into
mainfrom
user/gudge/wsb-phase0-shared-plumbing

MGudgin commented Jun 26, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

dhoehna Jun 26, 2026

Uh oh!

dhoehna Jun 26, 2026

Uh oh!

dhoehna Jun 26, 2026

Uh oh!

dhoehna Jun 26, 2026

Uh oh!

dhoehna Jun 26, 2026

Uh oh!

dhoehna Jun 26, 2026

Uh oh!

dhoehna Jun 26, 2026

Uh oh!

dhoehna left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

MGudgin commented Jun 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Related Issues

Microsoft Reviewers: Open in CodeFlow

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Review details

Uh oh!

dhoehna Jun 26, 2026

Choose a reason for hiding this comment

Uh oh!

dhoehna Jun 26, 2026

Choose a reason for hiding this comment

Uh oh!

dhoehna Jun 26, 2026

Choose a reason for hiding this comment

Uh oh!

dhoehna Jun 26, 2026

Choose a reason for hiding this comment

Uh oh!

dhoehna Jun 26, 2026

Choose a reason for hiding this comment

Uh oh!

dhoehna Jun 26, 2026

Choose a reason for hiding this comment

Uh oh!

dhoehna Jun 26, 2026

Choose a reason for hiding this comment

Uh oh!

dhoehna left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MGudgin commented Jun 26, 2026 •

edited

Loading