Fix MoveDistribution E_ACCESSDENIED when setVhdOwner fails under impersonation#40717
Open
benhillis wants to merge 2 commits into
Open
Fix MoveDistribution E_ACCESSDENIED when setVhdOwner fails under impersonation#40717benhillis wants to merge 2 commits into
benhillis wants to merge 2 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a WSL service regression where wsl --manage <distro> --move <path> could fail with E_ACCESSDENIED after a cross-volume move changes the VHD owner to BUILTIN\Administrators, leaving the VHD moved on disk but the registry BasePath unchanged (breaking the distro). The fix ensures the VHD is opened and its owner restored under SYSTEM (not the impersonated user) by moving wil::run_as_self() and privilege acquisition before CreateFileW, and adds a regression test.
Changes:
- In
MoveDistribution, run as self (SYSTEM) and acquireSE_RESTORE_NAMEbefore opening the moved VHD withWRITE_OWNER, ensuring owner restoration succeeds even when the impersonated user lacks ownership-based access. - Add a new WSL2 unit test that forces the VHD owner to
BUILTIN\Administrators, performs a non-elevated move, and validates ownership preservation and distro usability.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/windows/service/exe/LxssUserSession.cpp |
Fixes setVhdOwner to open the VHD as SYSTEM with needed privilege before CreateFileW, preventing E_ACCESSDENIED under impersonation. |
test/windows/UnitTests.cpp |
Adds regression test MoveVhdWithAdminOwner validating non-elevated move succeeds when VHD owner is Administrators and that the distro still launches. |
…rsonation (#40716) After a cross-volume MoveFileEx, the new VHD file's owner may be set to BUILTIN\Administrators. The setVhdOwner lambda was opening the file with WRITE_OWNER under user impersonation, which fails with E_ACCESSDENIED if the user doesn't own the file. The subsequent run_as_self() came too late. Move run_as_self() and AcquirePrivilege(SE_RESTORE_NAME) before the CreateFileW call so the file is opened as SYSTEM, which always has WRITE_OWNER regardless of file ownership. Previously, this failure left the VHD at the new location with the registry BasePath still pointing at the old path, corrupting the distro. Fixes #40716 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Regression test that simulates a cross-volume move scenario: after moving a distro, the VHD owner is explicitly set to BUILTIN\Administrators (mimicking MoveFileEx cross-volume behavior), then a second move is attempted as a non-elevated user. Before the fix, setVhdOwner would fail with E_ACCESSDENIED because CreateFileW(WRITE_OWNER) ran under user impersonation. The test verifies: - The move succeeds even when the VHD is owned by Administrators - The VHD owner is restored to the user's SID after the move - The distro launches successfully after the move Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
benhillis
force-pushed
the
benhill/fix-move-distro-access-denied
branch
from
326d51f to
7171848
Compare
OneBlue
requested changes
Jun 6, 2026
|
|
||
| // Simulate cross-volume MoveFileEx side-effect: change VHD owner to BUILTIN\Administrators. | ||
| { | ||
| BYTE adminsSidBuffer[SECURITY_MAX_SID_SIZE]; |
Collaborator
There was a problem hiding this comment.
nit: This can be simplified by using: wsl::windows::common::security::CreateSid (and factored since we do the same thing below)
OneBlue
approved these changes
Jun 6, 2026
Collaborator
OneBlue
left a comment
OneBlue
left a comment
There was a problem hiding this comment.
Scratch my previous review, looks like the FILE_FLAG_OPEN_REPARSE_POINT flag already protects us from that
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #40716 —
wsl --manage <distro> --move <path>fails withE_ACCESSDENIEDwhen the VHD owner becomesBUILTIN\Administratorsafter a cross-volume move.PR Checklist
Detailed Description
After a cross-volume
MoveFileEx, the new VHD file's owner may be set toBUILTIN\Administrators(Windows behavior for elevated callers). ThesetVhdOwnerlambda was opening the moved file withWRITE_OWNERunder user impersonation, which fails withE_ACCESSDENIEDbecause the impersonated token doesn't have ownership-based access to the file.The
wil::run_as_self()call that grants SYSTEM privileges was placed afterCreateFileW— too late.Fix: Move
wil::run_as_self()andAcquirePrivilege(SE_RESTORE_NAME)beforeCreateFileWso the file handle is opened as SYSTEM.FILE_FLAG_OPEN_REPARSE_POINTis retained to prevent symlink following.Previously, this failure left the VHD physically moved but the registry
BasePathstill pointing at the old location, leaving the distro unusable.Validation Steps
cmake . && cmake --build . -- -mbin\x64\debug\test.bat /name:*MoveVhdWithAdminOwner*— PASSED