Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions src/windows/service/exe/LxssUserSession.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -939,13 +939,39 @@ HRESULT LxssUserSessionImpl::MoveDistribution(_In_ LPCGUID DistroGuid, _In_ LPCW
THROW_WIN32(error.value());
}

// Read the original VHD owner before the move so we can restore it after.
// Cross-volume MoveFileEx may set the owner to BUILTIN\Administrators for
// elevated callers, which breaks HcsGrantVmAccess (needs WRITE_DAC via
// ownership) from non-elevated contexts.
PSID originalOwner = nullptr;
wil::unique_hlocal originalDescriptor;
THROW_IF_WIN32_ERROR(GetNamedSecurityInfoW(
distro.VhdFilePath.c_str(), SE_FILE_OBJECT, OWNER_SECURITY_INFORMATION, &originalOwner, nullptr, nullptr, nullptr, &originalDescriptor));

// Move the VHD to the new location.
THROW_IF_WIN32_BOOL_FALSE(MoveFileEx(distro.VhdFilePath.c_str(), newVhdPath.c_str(), MOVEFILE_COPY_ALLOWED | MOVEFILE_WRITE_THROUGH));

// Restore the original VHD owner on the moved file.
auto setVhdOwner = [&originalOwner](const std::filesystem::path& vhdPath) {
wil::unique_hfile vhdHandle(CreateFileW(
vhdPath.c_str(), WRITE_OWNER, FILE_SHARE_READ | FILE_SHARE_WRITE, nullptr, OPEN_EXISTING, FILE_FLAG_OPEN_REPARSE_POINT, nullptr));
THROW_LAST_ERROR_IF(!vhdHandle);

auto runAsSelf = wil::run_as_self();
auto privileges = wsl::windows::common::security::AcquirePrivilege(SE_RESTORE_NAME);
Comment thread
benhillis marked this conversation as resolved.
THROW_IF_WIN32_ERROR(
::SetSecurityInfo(vhdHandle.get(), SE_FILE_OBJECT, OWNER_SECURITY_INFORMATION, originalOwner, nullptr, nullptr, nullptr));
};

setVhdOwner(newVhdPath);

auto revert = wil::scope_exit_log(WI_DIAGNOSTICS_INFO, [&]() {
THROW_IF_WIN32_BOOL_FALSE(MoveFileEx(
newVhdPath.c_str(), distro.VhdFilePath.c_str(), MOVEFILE_COPY_ALLOWED | MOVEFILE_REPLACE_EXISTING | MOVEFILE_WRITE_THROUGH));

// Fix ownership on the reverted VHD in case MoveFileEx copied across volumes.
LOG_IF_FAILED(wil::ResultFromException([&] { setVhdOwner(distro.VhdFilePath); }));

// Write the location back to the original path in case the second registry write failed. Otherwise, this is a no-op.
registration.Write(Property::BasePath, distro.BasePath.c_str());
});
Expand Down
74 changes: 74 additions & 0 deletions test/windows/UnitTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2801,6 +2801,80 @@ Error code: Wsl/InstallDistro/WSL_E_DISTRO_NOT_FOUND
}
}

TEST_METHOD(MoveVhdOwnership)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: This should be WSL2_TEST_METHOD since test only applies to wsl2

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will fix in follow up.

{
constexpr auto name = L"move-owner-test-distro";
constexpr auto moveElevatedFolder = L"move-owner-elevated";
constexpr auto moveNonElevatedFolder = L"move-owner-non-elevated";
Comment thread
benhillis marked this conversation as resolved.

// Import a WSL2 distro.
VERIFY_ARE_EQUAL(LxsstuLaunchWsl(std::format(L"--import {} . \"{}\" --version 2", name, g_testDistroPath)), 0L);

auto cleanup = wil::scope_exit_log(WI_DIAGNOSTICS_INFO, [name]() {
LxsstuLaunchWsl(std::format(L"--unregister {}", name));
std::filesystem::remove_all(moveElevatedFolder);
std::filesystem::remove_all(moveNonElevatedFolder);
});
Comment thread
benhillis marked this conversation as resolved.

auto verifyVhdOwner = [](const std::wstring& path) {
PSID ownerSid = nullptr;
wil::unique_hlocal descriptor;
THROW_IF_WIN32_ERROR(GetNamedSecurityInfoW(
path.c_str(), SE_FILE_OBJECT, OWNER_SECURITY_INFORMATION, &ownerSid, nullptr, nullptr, nullptr, &descriptor));

auto userToken = wil::open_current_access_token(TOKEN_QUERY);
auto tokenUser = wil::get_token_information<TOKEN_USER>(userToken.get());

VERIFY_IS_TRUE(EqualSid(ownerSid, tokenUser->User.Sid));
};

const auto nonElevatedToken = GetNonElevatedToken();

// Move as elevated, launch as non-elevated.
// This is the primary bug scenario: MoveFileEx sets owner to BUILTIN\Administrators,
// then HcsGrantVmAccess fails with E_ACCESSDENIED when impersonating the non-elevated user.
{
WslShutdown();
VERIFY_ARE_EQUAL(LxsstuLaunchWsl(std::format(L"--manage {} --move {}", name, moveElevatedFolder)), 0L);

auto vhdPath = std::format(L"{}\\ext4.vhdx", moveElevatedFolder);
VERIFY_IS_TRUE(std::filesystem::exists(vhdPath));
verifyVhdOwner(vhdPath);

WslShutdown();
auto [out, err] = LxsstuLaunchWslAndCaptureOutput(std::format(L"-d {} echo ok", name), 0, nullptr, nonElevatedToken.get());
VERIFY_ARE_EQUAL(out, L"ok\n");
}

// Move as non-elevated, launch as elevated.
{
WslShutdown();
VERIFY_ARE_EQUAL(
LxsstuLaunchWsl(
std::format(L"--manage {} --move {}", name, moveNonElevatedFolder),
nullptr,
nullptr,
nullptr,
nonElevatedToken.get()),
0L);

auto vhdPath = std::format(L"{}\\ext4.vhdx", moveNonElevatedFolder);
VERIFY_IS_TRUE(std::filesystem::exists(vhdPath));
verifyVhdOwner(vhdPath);

WslShutdown();
auto [out, err] = LxsstuLaunchWslAndCaptureOutput(std::format(L"-d {} echo ok", name));
VERIFY_ARE_EQUAL(out, L"ok\n");
}

// Also launch as non-elevated after the non-elevated move.
{
WslShutdown();
auto [out, err] = LxsstuLaunchWslAndCaptureOutput(std::format(L"-d {} echo ok", name), 0, nullptr, nonElevatedToken.get());
VERIFY_ARE_EQUAL(out, L"ok\n");
}
}

WSL2_TEST_METHOD(Resize)
{
constexpr auto name = L"resize-test-distro";
Expand Down
Loading