Pin disk-attach target by handle to close junction-swap race by benhillis · Pull Request #40782 · microsoft/WSL

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

Summary

The disk-attach path operated on the user-supplied path string, re-resolving it independently at the impersonated access check (GrantVmAccess / GrantVmWorkerProcessAccessToDisk + online-state) and again at the privileged attach performed as SYSTEM (AddVhd / AddPassThroughDisk). A standard user could point the path at an NTFS junction/reparse point they control and swap its target between check and use, reaching disks or VHDs they have no rights to. The persisted-restore path (_LoadDiskMount) re-resolved the stored string as SYSTEM, exposing the same surface.

Fix

Add WslCoreVm::ResolveDiskPath, which:

Opens the target under the caller's impersonation with FILE_FLAG_OPEN_REPARSE_POINT (proves the caller can reach this exact object).
Rejects a final component that is a reparse point (junction/symlink) — the primitive used to swap the target.
Canonicalizes via GetFinalPathNameByHandle so no reparse point remains in any component.
For a VHD, returns a held read handle (GENERIC_READ, no FILE_SHARE_DELETE) kept open across GrantVmAccess + AddVhd, pinning the file (and its parent directories) so the target cannot be swapped during the attach window.

The canonical path is stored in DiskState and used for every privileged operation and for detach/teardown/eject revocation, so check and use always refer to the same object.

Tests

MountVhdThroughSymlinkIsRejected — a raw reparse path sent straight to the AttachDisk RPC is refused.
MountVhdThroughDirectoryJunctionSucceeds — a legitimate junction is canonicalized and still mounts.

Both pass against a full local build deployed to the host (Version=2 TAEF run).

The disk-attach path operated on the user-supplied path string, re-resolving it independently at the impersonated access check (GrantVmAccess / GrantVmWorkerProcessAccessToDisk + online-state) and again at the privileged attach performed as SYSTEM (AddVhd / AddPassThroughDisk). A standard user could point the path at an NTFS junction/reparse point they control and swap its target between check and use, reaching disks or VHDs they have no rights to. The persisted-restore path (_LoadDiskMount) re-resolved the stored string as SYSTEM, exposing the same surface. Add WslCoreVm::ResolveDiskPath, which opens the target under the caller's impersonation with FILE_FLAG_OPEN_REPARSE_POINT, rejects a final component that is a reparse point, and canonicalizes via GetFinalPathNameByHandle so no reparse point remains in any component. For a VHD it returns a held read handle (no FILE_SHARE_DELETE) that is kept open across GrantVmAccess and AddVhd, pinning the file (and its parent directories) so the target cannot be swapped during the attach window. The canonical path is stored in DiskState and used for every privileged operation and for detach/teardown/eject revocation, so check and use always refer to the same object. Add regression tests: MountVhdThroughSymlinkIsRejected (a raw reparse path sent straight to the AttachDisk RPC is refused) and MountVhdThroughDirectoryJunctionSucceeds (a legitimate junction is canonicalized and still mounts). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

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.

+        auto resolved = ResolveDiskPath(Disk, Type, UserToken);
+        resolvedDisk = std::move(resolved.Path);
+        const wil::unique_hfile pinHandle = std::move(resolved.PinHandle);
+


    }

-    m_attachedDisks.emplace(AttachedDisk{Type, Disk, IsUserDisk}, DiskState{Lun.value(), {}, diskFlags});
+    m_attachedDisks.emplace(AttachedDisk{Type, Disk, IsUserDisk}, DiskState{Lun.value(), {}, diskFlags, resolvedDisk});
    cleanup.release();


+        const auto disk = GetBlockDeviceInWsl();
+        VERIFY_IS_TRUE(IsBlockDevicePresent(disk));
+
+        // Unmount using the canonical path of the VHD.


Address review feedback: SaveDiskState wrote the original user-supplied path string to the registry, so the SYSTEM-side restore in _LoadDiskMount re-resolved a user-controlled string. Persist DiskState::ResolvedPath (the canonicalized, reparse-free path) instead, so the restore operates on an already-canonical path as defense in depth. Also correct the MountVhdThroughDirectoryJunctionSucceeds comment: the unmount passes the junction path and the service canonicalizes it internally to match the attached-disk entry. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

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.

Persisting the canonical resolved path broke the persisted-path contract asserted by the mount-state tests (registry 'Disk' value must equal the mount path). Restore is already re-validated by ResolveDiskPath in _LoadDiskMount, so persist the original map key instead. Resolving the disk by handle now surfaces a missing file/path from the open in ResolveDiskPath rather than from the HCS layer, so update the AttachDisk/CreateInstance error-code expectations to drop the /HCS segment. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

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.

+        auto resolved = ResolveDiskPath(Disk, Type, UserToken);
+        resolvedDisk = std::move(resolved.Path);
+        const wil::unique_hfile pinHandle = std::move(resolved.PinHandle);
+


+    // Resolves and pins the user-supplied disk path under the caller's identity, rejecting
+    // reparse points (junctions/symlinks) and holding a handle so that the path cannot be
+    // swapped between the access check and the privileged attach (TOCTOU).


                ValidateErrorMessage(
                    L"--mount DoesNotExist",
                    L"Failed to attach disk 'DoesNotExist' to WSL2: The system cannot find the file specified. ",
-                    L"Wsl/Service/AttachDisk/MountDisk/HCS/ERROR_FILE_NOT_FOUND");
+                    L"Wsl/Service/AttachDisk/MountDisk/ERROR_FILE_NOT_FOUND");


- PolicyTest::MountPolicyAllowed asserts the same --mount DoesNotExist scenario, so drop its /HCS error-code segment too (verified locally). - CorruptedVhd keeps /HCS: the pin-handle open succeeds there and the sharing violation still surfaces from HCS (verified locally). - Annotate the VHD pin handle [[maybe_unused]] to document its lifetime-only purpose. - Clarify ResolveDiskPath header comment: only the final path component is reparse-rejected; intermediate junctions are followed and canonicalized. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pin disk-attach target by handle to close junction-swap race#40782

Pin disk-attach target by handle to close junction-swap race#40782
benhillis wants to merge 4 commits into
masterfrom
fix/disk-attach-toctou-handle-pinning

benhillis commented Jun 11, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

benhillis commented Jun 11, 2026

Uh oh!

benhillis commented Jun 12, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

benhillis commented Jun 12, 2026

Uh oh!

benhillis commented Jun 12, 2026

Uh oh!

azure-pipelines Bot commented Jun 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

benhillis commented Jun 11, 2026

Summary

Fix

Tests

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

benhillis commented Jun 11, 2026

Uh oh!

benhillis commented Jun 12, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

benhillis commented Jun 12, 2026

Uh oh!

benhillis commented Jun 12, 2026

Uh oh!

azure-pipelines Bot commented Jun 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants