Skip to content

Commit 81e5cd6

Browse files
Ben HillisCopilot
andcommitted
Fix VHD ownership after cross-volume move to prevent E_ACCESSDENIED
When MoveDistribution moves a VHD across volumes, MoveFileEx copies the file and the new file's owner may not be the user's SID. This causes HcsGrantVmAccess to fail with E_ACCESSDENIED when later launching the distro, because the impersonated user lacks WRITE_DAC on the file (only implicitly granted to the owner). Fix by explicitly setting the VHD owner to the user's SID after the move, matching what CreateVhd already does at creation time. Uses handle-based SetSecurityInfo with FILE_FLAG_OPEN_REPARSE_POINT to avoid TOCTOU races and symlink following. Also fixes a pre-existing build break in MountTests.cpp from the test refactor (WSL2_TEST_ONLY -> WSL2_TEST_METHOD). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 813070c commit 81e5cd6

3 files changed

Lines changed: 94 additions & 2 deletions

File tree

src/windows/service/exe/LxssUserSession.cpp

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -942,10 +942,29 @@ HRESULT LxssUserSessionImpl::MoveDistribution(_In_ LPCGUID DistroGuid, _In_ LPCW
942942
// Move the VHD to the new location.
943943
THROW_IF_WIN32_BOOL_FALSE(MoveFileEx(distro.VhdFilePath.c_str(), newVhdPath.c_str(), MOVEFILE_COPY_ALLOWED | MOVEFILE_WRITE_THROUGH));
944944

945+
// Set the VHD owner to the user's SID. Cross-volume MoveFileEx sets the
946+
// owner to BUILTIN\Administrators for elevated callers, which breaks
947+
// HcsGrantVmAccess (needs WRITE_DAC via ownership) from non-elevated contexts.
948+
auto setVhdOwner = [this](const std::filesystem::path& vhdPath) {
949+
wil::unique_hfile vhdHandle(CreateFileW(
950+
vhdPath.c_str(), WRITE_OWNER, FILE_SHARE_READ | FILE_SHARE_WRITE, nullptr, OPEN_EXISTING, FILE_FLAG_OPEN_REPARSE_POINT, nullptr));
951+
THROW_LAST_ERROR_IF(!vhdHandle);
952+
953+
auto runAsSelf = wil::run_as_self();
954+
auto privileges = wsl::windows::common::security::AcquirePrivilege(SE_RESTORE_NAME);
955+
THROW_IF_WIN32_ERROR(
956+
::SetSecurityInfo(vhdHandle.get(), SE_FILE_OBJECT, OWNER_SECURITY_INFORMATION, GetUserSid(), nullptr, nullptr, nullptr));
957+
};
958+
959+
setVhdOwner(newVhdPath);
960+
945961
auto revert = wil::scope_exit_log(WI_DIAGNOSTICS_INFO, [&]() {
946962
THROW_IF_WIN32_BOOL_FALSE(MoveFileEx(
947963
newVhdPath.c_str(), distro.VhdFilePath.c_str(), MOVEFILE_COPY_ALLOWED | MOVEFILE_REPLACE_EXISTING | MOVEFILE_WRITE_THROUGH));
948964

965+
// Fix ownership on the reverted VHD in case MoveFileEx copied across volumes.
966+
LOG_IF_FAILED(wil::ResultFromException([&] { setVhdOwner(distro.VhdFilePath); }));
967+
949968
// Write the location back to the original path in case the second registry write failed. Otherwise, this is a no-op.
950969
registration.Write(Property::BasePath, distro.BasePath.c_str());
951970
});

test/windows/MountTests.cpp

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -438,10 +438,9 @@ class MountTests
438438
VERIFY_ARE_EQUAL(LxsstuLaunchWsl(L"--unmount " + absolutePath.wstring()), (DWORD)0);
439439
}
440440

441-
TEST_METHOD(AbsolutePathVhdUnmountAfterVMTimeout)
441+
WSL2_TEST_METHOD(AbsolutePathVhdUnmountAfterVMTimeout)
442442
{
443443
SKIP_UNSUPPORTED_ARM64_MOUNT_TEST();
444-
WSL2_TEST_ONLY();
445444

446445
WslKeepAlive keepAlive;
447446

test/windows/UnitTests.cpp

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2801,6 +2801,80 @@ Error code: Wsl/InstallDistro/WSL_E_DISTRO_NOT_FOUND
28012801
}
28022802
}
28032803

2804+
TEST_METHOD(MoveVhdOwnership)
2805+
{
2806+
constexpr auto name = L"move-owner-test-distro";
2807+
constexpr auto moveElevatedFolder = L"move-owner-elevated";
2808+
constexpr auto moveNonElevatedFolder = L"move-owner-non-elevated";
2809+
2810+
// Import a WSL2 distro.
2811+
VERIFY_ARE_EQUAL(LxsstuLaunchWsl(std::format(L"--import {} . \"{}\" --version 2", name, g_testDistroPath)), 0L);
2812+
2813+
auto cleanup = wil::scope_exit_log(WI_DIAGNOSTICS_INFO, [name]() {
2814+
LxsstuLaunchWsl(std::format(L"--unregister {}", name));
2815+
std::filesystem::remove_all(moveElevatedFolder);
2816+
std::filesystem::remove_all(moveNonElevatedFolder);
2817+
});
2818+
2819+
auto verifyVhdOwner = [](const std::wstring& path) {
2820+
PSID ownerSid = nullptr;
2821+
wil::unique_hlocal descriptor;
2822+
THROW_IF_WIN32_ERROR(GetNamedSecurityInfoW(
2823+
path.c_str(), SE_FILE_OBJECT, OWNER_SECURITY_INFORMATION, &ownerSid, nullptr, nullptr, nullptr, &descriptor));
2824+
2825+
auto userToken = wil::open_current_access_token(TOKEN_QUERY);
2826+
auto tokenUser = wil::get_token_information<TOKEN_USER>(userToken.get());
2827+
2828+
VERIFY_IS_TRUE(EqualSid(ownerSid, tokenUser->User.Sid));
2829+
};
2830+
2831+
const auto nonElevatedToken = GetNonElevatedToken();
2832+
2833+
// Move as elevated, launch as non-elevated.
2834+
// This is the primary bug scenario: MoveFileEx sets owner to BUILTIN\Administrators,
2835+
// then HcsGrantVmAccess fails with E_ACCESSDENIED when impersonating the non-elevated user.
2836+
{
2837+
WslShutdown();
2838+
VERIFY_ARE_EQUAL(LxsstuLaunchWsl(std::format(L"--manage {} --move {}", name, moveElevatedFolder)), 0L);
2839+
2840+
auto vhdPath = std::format(L"{}\\ext4.vhdx", moveElevatedFolder);
2841+
VERIFY_IS_TRUE(std::filesystem::exists(vhdPath));
2842+
verifyVhdOwner(vhdPath);
2843+
2844+
WslShutdown();
2845+
auto [out, err] = LxsstuLaunchWslAndCaptureOutput(std::format(L"-d {} echo ok", name), 0, nullptr, nonElevatedToken.get());
2846+
VERIFY_ARE_EQUAL(out, L"ok\n");
2847+
}
2848+
2849+
// Move as non-elevated, launch as elevated.
2850+
{
2851+
WslShutdown();
2852+
VERIFY_ARE_EQUAL(
2853+
LxsstuLaunchWsl(
2854+
std::format(L"--manage {} --move {}", name, moveNonElevatedFolder),
2855+
nullptr,
2856+
nullptr,
2857+
nullptr,
2858+
nonElevatedToken.get()),
2859+
0L);
2860+
2861+
auto vhdPath = std::format(L"{}\\ext4.vhdx", moveNonElevatedFolder);
2862+
VERIFY_IS_TRUE(std::filesystem::exists(vhdPath));
2863+
verifyVhdOwner(vhdPath);
2864+
2865+
WslShutdown();
2866+
auto [out, err] = LxsstuLaunchWslAndCaptureOutput(std::format(L"-d {} echo ok", name));
2867+
VERIFY_ARE_EQUAL(out, L"ok\n");
2868+
}
2869+
2870+
// Also launch as non-elevated after the non-elevated move.
2871+
{
2872+
WslShutdown();
2873+
auto [out, err] = LxsstuLaunchWslAndCaptureOutput(std::format(L"-d {} echo ok", name), 0, nullptr, nonElevatedToken.get());
2874+
VERIFY_ARE_EQUAL(out, L"ok\n");
2875+
}
2876+
}
2877+
28042878
WSL2_TEST_METHOD(Resize)
28052879
{
28062880
constexpr auto name = L"resize-test-distro";

0 commit comments

Comments
 (0)