Fix VHD ownership during distribution moves (#41333)
Run distribution moves under a duplicated caller token whose default owner is the caller's user SID. This keeps cross-volume copies accessible without reopening the destination to rewrite its owner. Co-authored-by: Ben Hillis <benhill@ntdev.microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Ben Hillis committed
Aug 13, 2026 at 17:29 UTC
c9f0d36b2840344725e6e1229bcc0a561dc6bbfd
2 files changed
+31
-64
src/windows/service/exe/LxssUserSession.cpp
+28
-58
@@ -941,77 +941,47 @@ HRESULT LxssUserSessionImpl::MoveDistribution(_In_ LPCGUID DistroGuid, _In_ LPCW
941
942
RETURN_HR_IF(E_NOTIMPL, WI_IsFlagClear(distro.Flags, LXSS_DISTRO_FLAGS_VM_MODE));
943
944
- // Build the final vhd path.
945
- std::filesystem::path newVhdPath = Location;
946
- RETURN_HR_IF(E_INVALIDARG, newVhdPath.empty());
944
+ std::filesystem::path destDir(Location);
945
+ RETURN_HR_IF(E_INVALIDARG, destDir.empty());
946
948
- newVhdPath /= distro.VhdFilePath.filename();
947
+ const std::filesystem::path destPath = destDir / distro.VhdFilePath.filename();
948
950
- auto impersonate = wil::CoImpersonateClient();
949
+ // Cross-volume MoveFileEx creates a new file using the impersonation token's
950
+ // default owner. Normalize that owner to the caller's user SID so elevated moves
951
+ // do not produce a VHD owned by BUILTIN\Administrators.
952
+ auto tokenUser = wil::get_token_information<TOKEN_USER>(userToken.get());
953
+ TOKEN_OWNER tokenOwner{tokenUser->User.Sid};
954
+ THROW_IF_WIN32_BOOL_FALSE(SetTokenInformation(userToken.get(), TokenOwner, &tokenOwner, sizeof(tokenOwner)));
955
952
- // Create the distribution base folder
953
- std::error_code error;
954
- std::filesystem::create_directories(Location, error);
955
- if (error.value())
956
{
957
- THROW_WIN32(error.value());
958
- }
957
+ auto impersonate = wil::impersonate_token(userToken.get());
958
960
- // Read the original VHD owner before the move so we can restore it after.
961
- // Cross-volume MoveFileEx may set the owner to BUILTIN\Administrators for
962
- // elevated callers, which breaks HcsGrantVmAccess (needs WRITE_DAC via
963
- // ownership) from non-elevated contexts.
964
- PSID originalOwner = nullptr;
965
- wil::unique_hlocal originalDescriptor;
966
- THROW_IF_WIN32_ERROR(GetNamedSecurityInfoW(
967
- distro.VhdFilePath.c_str(), SE_FILE_OBJECT, OWNER_SECURITY_INFORMATION, &originalOwner, nullptr, nullptr, nullptr, &originalDescriptor));
968
-
969
- // Move the VHD to the new location.
970
- THROW_IF_WIN32_BOOL_FALSE(MoveFileEx(distro.VhdFilePath.c_str(), newVhdPath.c_str(), MOVEFILE_COPY_ALLOWED | MOVEFILE_WRITE_THROUGH));
971
-
972
- // Restore the original VHD owner on the moved file. Open the file while impersonating
973
- // the caller, then use ReOpenFile to add WRITE_OWNER as SYSTEM with SE_RESTORE_NAME
974
- // (needed since a cross-volume MoveFileEx may leave the file owned by
975
- // BUILTIN\Administrators). ReOpenFile reuses the already-open file object instead of
976
- // resolving the path again.
977
- auto setVhdOwner = [&originalOwner](const std::filesystem::path& vhdPath) {
978
- wil::unique_hfile vhdHandle(CreateFileW(
979
- vhdPath.c_str(), READ_CONTROL, FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, nullptr, OPEN_EXISTING, FILE_FLAG_OPEN_REPARSE_POINT, nullptr));
980
- THROW_LAST_ERROR_IF(!vhdHandle);
981
-
982
- auto runAsSelf = wil::run_as_self();
983
- auto privileges = wsl::windows::common::security::AcquirePrivilege(SE_RESTORE_NAME);
984
-
985
- wil::unique_hfile privilegedHandle(ReOpenFile(
986
- vhdHandle.get(), WRITE_OWNER, FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, FILE_FLAG_OPEN_REPARSE_POINT));
987
- THROW_LAST_ERROR_IF(!privilegedHandle);
988
-
989
- THROW_IF_WIN32_ERROR(::SetSecurityInfo(
990
- privilegedHandle.get(), SE_FILE_OBJECT, OWNER_SECURITY_INFORMATION, originalOwner, nullptr, nullptr, nullptr));
991
- };
959
+ std::error_code error;
960
+ std::filesystem::create_directories(destDir, error);
961
+ if (error.value())
962
+ {
963
+ THROW_WIN32(error.value());
964
+ }
965
993
- // Install the rollback before fixing up ownership so a failure there (e.g. the caller
994
- // lacking access on the moved file) still moves the VHD back instead of leaving the
995
- // registration pointing at a file that no longer exists at the old location.
996
- auto revert = wil::scope_exit_log(WI_DIAGNOSTICS_INFO, [&]() {
997
- THROW_IF_WIN32_BOOL_FALSE(MoveFileEx(
998
- newVhdPath.c_str(), distro.VhdFilePath.c_str(), MOVEFILE_COPY_ALLOWED | MOVEFILE_REPLACE_EXISTING | MOVEFILE_WRITE_THROUGH));
966
+ THROW_IF_WIN32_BOOL_FALSE(MoveFileExW(distro.VhdFilePath.c_str(), destPath.c_str(), MOVEFILE_COPY_ALLOWED | MOVEFILE_WRITE_THROUGH));
967
+ }
968
1000
- // Fix ownership on the reverted VHD in case MoveFileEx copied across volumes.
1001
- LOG_IF_FAILED(wil::ResultFromException([&] { setVhdOwner(distro.VhdFilePath); }));
969
+ auto revert = wil::scope_exit_log(WI_DIAGNOSTICS_INFO, [&]() {
970
+ auto impersonate = wil::impersonate_token(userToken.get());
971
+ if (!MoveFileExW(destPath.c_str(), distro.VhdFilePath.c_str(), MOVEFILE_COPY_ALLOWED | MOVEFILE_REPLACE_EXISTING | MOVEFILE_WRITE_THROUGH))
972
+ {
973
+ LOG_LAST_ERROR();
974
+ return;
975
+ }
976
1003
- // Write the location back to the original path in case the second registry write failed. Otherwise, this is a no-op.
1004
- registration.Write(Property::BasePath, distro.BasePath.c_str());
977
+ LOG_IF_FAILED(wil::ResultFromException(
978
+ WI_DIAGNOSTICS_INFO, [&]() { registration.Write(Property::BasePath, distro.BasePath.c_str()); }));
979
});
980
1007
- setVhdOwner(newVhdPath);
1008
-
1009
- // Update the registry location
981
registration.Write(Property::BasePath, Location);
1011
- registration.Write(Property::VhdFileName, newVhdPath.filename().c_str());
982
+ registration.Write(Property::VhdFileName, destPath.filename().c_str());
983
984
revert.release();
1014
-
985
return S_OK;
986
}
987
test/windows/UnitTests.cpp
+3
-6
@@ -3227,9 +3227,8 @@ Error code: Wsl/InstallDistro/WSL_E_DISTRO_NOT_FOUND
3227
3228
WSL2_TEST_METHOD(MoveVhdWithAdminOwner)
3229
{
3230
- // Regression test for #40716: if the VHD's owner is BUILTIN\Administrators
3231
- // (as happens after a cross-volume MoveFileEx from an elevated context),
3232
- // the move must still succeed because setVhdOwner runs as SYSTEM.
3230
+ // Regression test for #40716: a same-volume move must succeed when the VHD
3231
+ // is already owned by BUILTIN\Administrators.
3232
constexpr auto name = L"move-admin-owner-test-distro";
3233
constexpr auto firstFolder = L"move-admin-owner-first";
3234
constexpr auto secondFolder = L"move-admin-owner-second";
@@ -3277,9 +3276,7 @@ Error code: Wsl/InstallDistro/WSL_E_DISTRO_NOT_FOUND
3276
auto newVhdPath = std::format(L"{}\\ext4.vhdx", secondFolder);
3277
VERIFY_IS_TRUE(std::filesystem::exists(newVhdPath));
3278
3280
- // Verify the VHD owner was preserved. The code reads the owner before
3281
- // MoveFileEx and restores it afterward. Since we set the owner to
3282
- // Administrators before this move, it should still be Administrators.
3279
+ // A same-volume move preserves the VHD owner.
3280
{
3281
PSID ownerSid = nullptr;
3282
wil::unique_hlocal descriptor;