Fix MoveDistribution VHD ownership restore to reuse the resolved file handle (#41131)
setVhdOwner now opens the destination impersonated with READ_CONTROL, then uses ReOpenFile to upgrade the same handle to WRITE_OWNER while running as SYSTEM, instead of re-opening the path by name as SYSTEM. This avoids a second path resolution under elevated privileges. Also moves the rollback scope_exit registration earlier so a failure in setVhdOwner while moving to the new path correctly triggers rollback. Co-authored-by: Ben Hillis <benhill@ntdev.microsoft.com>
Ben Hillis committed
Jul 22, 2026 at 14:27 UTC
d92c3984820bfab1e86f3b9b75fb7a3bd412b522
1 file changed
+19
-11
src/windows/service/exe/LxssUserSession.cpp
+19
-11
@@ -951,24 +951,30 @@ HRESULT LxssUserSessionImpl::MoveDistribution(_In_ LPCGUID DistroGuid, _In_ LPCW
951
// Move the VHD to the new location.
952
THROW_IF_WIN32_BOOL_FALSE(MoveFileEx(distro.VhdFilePath.c_str(), newVhdPath.c_str(), MOVEFILE_COPY_ALLOWED | MOVEFILE_WRITE_THROUGH));
953
954
- // Restore the original VHD owner on the moved file.
955
- // Run as self (SYSTEM) for both the file open and the SetSecurityInfo call,
956
- // because after a cross-volume MoveFileEx the new file's owner may be
957
- // BUILTIN\Administrators and the impersonated user token may lack WRITE_OWNER.
954
+ // Restore the original VHD owner on the moved file. Open the file while impersonating
955
+ // the caller, then use ReOpenFile to add WRITE_OWNER as SYSTEM with SE_RESTORE_NAME
956
+ // (needed since a cross-volume MoveFileEx may leave the file owned by
957
+ // BUILTIN\Administrators). ReOpenFile reuses the already-open file object instead of
958
+ // resolving the path again.
959
auto setVhdOwner = [&originalOwner](const std::filesystem::path& vhdPath) {
960
+ wil::unique_hfile vhdHandle(CreateFileW(
961
+ vhdPath.c_str(), READ_CONTROL, FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, nullptr, OPEN_EXISTING, FILE_FLAG_OPEN_REPARSE_POINT, nullptr));
962
+ THROW_LAST_ERROR_IF(!vhdHandle);
963
+
964
auto runAsSelf = wil::run_as_self();
965
auto privileges = wsl::windows::common::security::AcquirePrivilege(SE_RESTORE_NAME);
966
962
- wil::unique_hfile vhdHandle(CreateFileW(
963
- vhdPath.c_str(), WRITE_OWNER, FILE_SHARE_READ | FILE_SHARE_WRITE, nullptr, OPEN_EXISTING, FILE_FLAG_OPEN_REPARSE_POINT, nullptr));
964
- THROW_LAST_ERROR_IF(!vhdHandle);
967
+ wil::unique_hfile privilegedHandle(ReOpenFile(
968
+ vhdHandle.get(), WRITE_OWNER, FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, FILE_FLAG_OPEN_REPARSE_POINT));
969
+ THROW_LAST_ERROR_IF(!privilegedHandle);
970
966
- THROW_IF_WIN32_ERROR(
967
- ::SetSecurityInfo(vhdHandle.get(), SE_FILE_OBJECT, OWNER_SECURITY_INFORMATION, originalOwner, nullptr, nullptr, nullptr));
971
+ THROW_IF_WIN32_ERROR(::SetSecurityInfo(
972
+ privilegedHandle.get(), SE_FILE_OBJECT, OWNER_SECURITY_INFORMATION, originalOwner, nullptr, nullptr, nullptr));
973
};
974
970
- setVhdOwner(newVhdPath);
971
-
975
+ // Install the rollback before fixing up ownership so a failure there (e.g. the caller
976
+ // lacking access on the moved file) still moves the VHD back instead of leaving the
977
+ // registration pointing at a file that no longer exists at the old location.
978
auto revert = wil::scope_exit_log(WI_DIAGNOSTICS_INFO, [&]() {
979
THROW_IF_WIN32_BOOL_FALSE(MoveFileEx(
980
newVhdPath.c_str(), distro.VhdFilePath.c_str(), MOVEFILE_COPY_ALLOWED | MOVEFILE_REPLACE_EXISTING | MOVEFILE_WRITE_THROUGH));
@@ -980,6 +986,8 @@ HRESULT LxssUserSessionImpl::MoveDistribution(_In_ LPCGUID DistroGuid, _In_ LPCW
986
registration.Write(Property::BasePath, distro.BasePath.c_str());
987
});
988
989
+ setVhdOwner(newVhdPath);
990
+
991
// Update the registry location
992
registration.Write(Property::BasePath, Location);
993
registration.Write(Property::VhdFileName, newVhdPath.filename().c_str());