Fix disk-attach restore TOCTOU by impersonating the mounting user on VHD restore by benhillis · Pull Request #40782 · microsoft/WSL

benhillis · 2026-06-11T21:13:14Z

Summary

Closes a time-of-check/time-of-use (TOCTOU) gap in disk attach where the restore path re-resolved a user-controllable VHD path as SYSTEM after the VM was recreated.

Background

For a live wsl --mount --vhd, a junction/symlink swap is already harmless: the VM access grant runs while impersonating the user (GrantVmAccess), and the SYSTEM-side AddVhd only succeeds on a file the VM was granted access to. A swap therefore yields ACCESS_DENIED, not disclosure.

The real gap was disk restore: when the utility VM is torn down on idle and later recreated, LxssUserSession::_LoadDiskMount re-attached persisted VHDs with no user token (as SYSTEM), re-resolving the user-supplied path and reopening the swap window.

Fix

Disk-mount state is stored under the user's SID in a volatile (per-boot) registry key, so the disk being restored was mounted earlier in the same boot by the same user, and that user's token is already available at VM-create time. So restore simply passes the user token for VHDs and lets the existing impersonated grant close the window:

VHD restore now runs under the mounting user's identity — a privileged operation can only ever touch a file that user can already reach.
Pass-through (raw block device) restore stays SYSTEM: it is elevation-gated and \\.\PhysicalDriveN paths have no reparse-point surface, so there is nothing to swap.

This replaces the earlier handle-pinning / reparse-point-rejection approach (per review feedback), which also regressed legitimate symlinked VHDs.

Tests

MountVhdThroughSymlinkSucceeds — a symlinked VHD mounts (regression guard).
MountVhdThroughSymlinkSurvivesVmTimeout — a symlinked VHD re-attaches after a VM idle-timeout restore.

Copilot

Pull request overview

This PR hardens WSL’s disk attach path against a TOCTOU junction/symlink swap by resolving user-supplied disk paths under impersonation, rejecting final-component reparse points, canonicalizing via handle-based resolution, and (for VHDs) pinning the target file with an open handle during the attach window.

Changes:

Added WslCoreVm::ResolveDiskPath() to open + validate + canonicalize disk paths and (for VHDs) keep a non-FILE_SHARE_DELETE handle open to pin the attach target during privileged operations.
Updated attach/detach/eject/destructor paths to perform privileged operations (grant/revoke/online/restore) against the stored canonical path (DiskState::ResolvedPath).
Added TAEF tests covering symlink rejection and directory-junction canonicalization for VHD mounts.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
`src/windows/service/exe/WslCoreVm.h`	Introduces `DiskState::ResolvedPath` and the `ResolveDiskPath`/`ResolvedDisk` API to support canonicalized, pinned disk operations.
`src/windows/service/exe/WslCoreVm.cpp`	Wires canonical path usage into attach/detach/eject/teardown and implements handle-based resolution/pinning logic.
`test/windows/MountTests.cpp`	Adds mount tests for rejecting symlink final components and allowing directory junctions via canonicalization.

benhillis · 2026-06-11T21:30:25Z

Thanks for the review. Addressed in 627b2b9:

Persisted path (SaveDiskState/_LoadDiskMount): Good catch on the intent. _LoadDiskMount does re-run ResolveDiskPath (reparse-reject + canonicalize) at restore, but to match the stated goal and add defense in depth, SaveDiskState now persists DiskState::ResolvedPath (canonical, reparse-free) instead of the raw user string. Registry entries are keyed by LUN, not by path, so no string-equality matching needed updating.
Test comment: Reworded — the unmount passes the junction path and the service canonicalizes internally to match the attached-disk entry.
pinHandle C4101 / /WX: This is a false positive. wil::unique_hfile has a non-trivial destructor, so MSVC does not emit C4101 (the variable is "used" for its RAII lifetime, exactly like the existing auto cleanup = wil::scope_exit(...) and auto runAsUser = wil::impersonate_token(...) guards in this file). A full /WX build of the solution compiles cleanly; left as-is for consistency with those existing guard patterns.

benhillis · 2026-06-12T01:10:22Z

Fixed the wsl2 CI failures (commit 7a60b63):

Persisted path / comment 2 follow-up: Persisting the canonical resolved path broke the persisted-path contract that the mount-state tests assert (the registry Disk value must equal the mount path that clients look up). The restore path in _LoadDiskMount already re-runs ResolveDiskPath (reparse rejection + canonicalize + pin), so the SYSTEM-side restore is re-validated regardless of what string is persisted. Reverted SaveDiskState to persist the original map key; security is unaffected.

Error-code expectations: Resolving the disk by handle now surfaces a missing file/path from the CreateFileW in ResolveDiskPath rather than from the HCS layer, so two ErrorMessages expectations changed (/MountDisk/HCS/ERROR_* -> /MountDisk/ERROR_*) for --mount DoesNotExist and the broken-distro VHD attach. Same human-readable message; the error is just caught one layer earlier.

Verified locally (Version=2): ErrorMessages, TestBareMountVhd, MountVhdThroughSymlinkIsRejected, MountVhdThroughDirectoryJunctionSucceeds all pass.

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

benhillis · 2026-06-12T03:03:25Z

Addressed the latest review (commit a91041f):

Other /HCS expectations (good catch): PolicyTest::MountPolicyAllowed asserts the same --mount DoesNotExist scenario, so dropped its /HCS segment too. I also checked UnitTests::CorruptedVhd (--import-in-place with an open handle): it correctly keeps /HCS/ERROR_SHARING_VIOLATION because the pin-handle open succeeds there and the sharing violation still surfaces from the HCS AddVhd. Both verified locally (Version=2): pass.
Header comment scope: Clarified that ResolveDiskPath rejects a reparse point only on the final path component; intermediate junctions are followed and canonicalized via the opened handle (which is what MountVhdThroughDirectoryJunctionSucceeds exercises).
pinHandle C4101//WX: This is a false positive — wil::unique_hfile has a non-trivial destructor, so MSVC does not emit C4101 (the full /WX build passes: 0 warnings). I've annotated it [[maybe_unused]] anyway to document the lifetime-only intent and stop the recurring flag.

benhillis · 2026-06-12T04:35:45Z

/azp run wsl-github-pr

azure-pipelines · 2026-06-12T04:35:53Z

Azure Pipelines successfully started running 1 pipeline(s).

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

…VHD restore A standard user's live ' wsl --mount --vhd' is already safe from a junction/symlink swap: the VM access grant runs while impersonating the user, and the SYSTEM-side AddVhd only succeeds on a file the VM was granted access to, so a swap yields ACCESS_DENIED rather than disclosure. The actual gap was disk restore: when the VM is recreated, _LoadDiskMount re-attached persisted VHDs as SYSTEM (no token), re-resolving a user-controllable path and reopening the TOCTOU. Because the disk-mount state is stored under the user's SID in a volatile (per-boot) key, the disk being restored was mounted by this same user in this same boot, so we can simply pass the user token and let the existing impersonated grant close the window. Pass-through devices stay SYSTEM (elevation-gated; \\.\PhysicalDriveN has no reparse surface). This replaces the earlier handle-pinning/reparse-rejection approach, which also regressed legitimate symlinked VHDs. Add tests covering a symlinked VHD mounting and surviving a VM idle-timeout restore. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

OneBlue

LGTM. Minor test comment

…re symlink creation in mount tests Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…VHD restore (#40782) (#40907) * Fix disk-attach restore TOCTOU by impersonating the mounting user on VHD restore A standard user's live ' wsl --mount --vhd' is already safe from a junction/symlink swap: the VM access grant runs while impersonating the user, and the SYSTEM-side AddVhd only succeeds on a file the VM was granted access to, so a swap yields ACCESS_DENIED rather than disclosure. The actual gap was disk restore: when the VM is recreated, _LoadDiskMount re-attached persisted VHDs as SYSTEM (no token), re-resolving a user-controllable path and reopening the TOCTOU. Because the disk-mount state is stored under the user's SID in a volatile (per-boot) key, the disk being restored was mounted by this same user in this same boot, so we can simply pass the user token and let the existing impersonated grant close the window. Pass-through devices stay SYSTEM (elevation-gated; \\.\PhysicalDriveN has no reparse surface). This replaces the earlier handle-pinning/reparse-rejection approach, which also regressed legitimate symlinked VHDs. Add tests covering a symlinked VHD mounting and surviving a VM idle-timeout restore. * Address PR feedback: re-query block device after VM timeout and require symlink creation in mount tests --------- (cherry picked from commit f0f4b10) Co-authored-by: Ben Hillis <benhill@ntdev.microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings June 11, 2026 21:13

Copilot started reviewing on behalf of benhillis June 11, 2026 21:14 View session

Copilot AI reviewed Jun 11, 2026

View reviewed changes

Comment thread src/windows/service/exe/WslCoreVm.cpp Outdated

Comment thread src/windows/service/exe/WslCoreVm.cpp

Comment thread test/windows/MountTests.cpp Outdated

Copilot AI review requested due to automatic review settings June 12, 2026 01:10

Copilot started reviewing on behalf of benhillis June 12, 2026 01:10 View session

Copilot AI reviewed Jun 12, 2026

View reviewed changes

Comment thread src/windows/service/exe/WslCoreVm.cpp Outdated

Comment thread src/windows/service/exe/WslCoreVm.h Outdated

Comment thread test/windows/UnitTests.cpp Outdated

benhillis marked this pull request as ready for review June 12, 2026 15:30

benhillis requested a review from a team as a code owner June 12, 2026 15:30

Copilot AI review requested due to automatic review settings June 12, 2026 15:30

Copilot started reviewing on behalf of benhillis June 12, 2026 15:31 View session

Copilot AI reviewed Jun 12, 2026

View reviewed changes

Comment thread src/windows/service/exe/WslCoreVm.cpp Outdated

Comment thread test/windows/MountTests.cpp Outdated

OneBlue reviewed Jun 13, 2026

View reviewed changes

Comment thread src/windows/service/exe/WslCoreVm.cpp Outdated

benhillis force-pushed the fix/disk-attach-toctou-handle-pinning branch from a91041f to 9ad3a64 Compare June 17, 2026 17:01

benhillis changed the title ~~Pin disk-attach target by handle to close junction-swap race~~ Jun 17, 2026

benhillis assigned OneBlue Jun 23, 2026

benhillis requested a review from Copilot June 23, 2026 16:31

Copilot started reviewing on behalf of benhillis June 23, 2026 16:32 View session

Copilot AI reviewed Jun 23, 2026

View reviewed changes

Comment thread test/windows/MountTests.cpp Outdated

OneBlue previously approved these changes Jun 23, 2026

View reviewed changes

Comment thread test/windows/MountTests.cpp Outdated

Address PR feedback: re-query block device after VM timeout and requi…

aabb210

…re symlink creation in mount tests Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

benhillis dismissed OneBlue’s stale review via aabb210 June 23, 2026 19:32

OneBlue approved these changes Jun 23, 2026

View reviewed changes

benhillis enabled auto-merge (squash) June 23, 2026 20:21

benhillis merged commit f0f4b10 into master Jun 23, 2026
11 checks passed

benhillis deleted the fix/disk-attach-toctou-handle-pinning branch June 23, 2026 21:22

benhillis mentioned this pull request Jun 25, 2026

Pass the mounting user's token on VHD restore (2.7 backport) #40907

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix disk-attach restore TOCTOU by impersonating the mounting user on VHD restore#40782

Fix disk-attach restore TOCTOU by impersonating the mounting user on VHD restore#40782
benhillis merged 2 commits into
masterfrom
fix/disk-attach-toctou-handle-pinning

benhillis commented Jun 11, 2026 •

edited

Loading

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

benhillis commented Jun 11, 2026

benhillis commented Jun 12, 2026

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

benhillis commented Jun 12, 2026

benhillis commented Jun 12, 2026

azure-pipelines Bot commented Jun 12, 2026

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

OneBlue left a comment

Uh oh!

Uh oh!

Labels

3 participants

Uh oh!

Conversation

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

Uh oh!

Summary

Background

Fix

Tests

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

benhillis commented Jun 11, 2026

benhillis commented Jun 12, 2026

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

benhillis commented Jun 12, 2026

benhillis commented Jun 12, 2026

azure-pipelines Bot commented Jun 12, 2026

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

OneBlue left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Labels

3 participants

benhillis commented Jun 11, 2026 •

edited

Loading