Skip to content

Commit

Permalink
[windows] Do not follow symlinks when deleting keys in the registry
Browse files Browse the repository at this point in the history 
Also delete DeleteRegistryKeyTest.DeleteAccessRightIsEnoughToDelete,
which is incompatible with the new RegKey::DeleteKey.

Bug: 1472558
Change-Id: I15df7756766923772b477285249a6027f1211a7c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4789249
Reviewed-by: S Ganesh <[email protected]>
Commit-Queue: Greg Thompson <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1186355}
  • Loading branch information
@GregTho
GregTho authored and Chromium LUCI CQ committed Aug 22, 2023
1 parent 9332438 commit 73fb7b0
Show file tree
Hide file tree
Showing 4 changed files with 189 additions and 128 deletions.
145 changes: 102 additions & 43 deletions base/win/registry.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

#include "base/win/registry.h"

#include <ntstatus.h>
#include <stddef.h>

#include <algorithm>
Expand All @@ -23,6 +24,8 @@
#include "base/win/scoped_handle.h"
#include "base/win/shlwapi.h"

extern "C" NTSTATUS WINAPI NtDeleteKey(IN HANDLE KeyHandle);

namespace base::win {

namespace {
Expand Down Expand Up @@ -192,17 +195,7 @@ LONG RegKey::CreateKey(const wchar_t* name, REGSAM access) {
}

LONG RegKey::Open(HKEY rootkey, const wchar_t* subkey, REGSAM access) {
DCHECK(rootkey && subkey && access);
HKEY subhkey = nullptr;

LONG result = RegOpenKeyEx(rootkey, subkey, 0, access, &subhkey);
if (result == ERROR_SUCCESS) {
Close();
key_ = subhkey;
wow64access_ = access & kWow64AccessMask;
}

return result;
return Open(rootkey, subkey, /*options=*/0, access);
}

LONG RegKey::OpenKey(const wchar_t* relative_key_name, REGSAM access) {
Expand Down Expand Up @@ -312,31 +305,33 @@ LONG RegKey::DeleteKey(const wchar_t* name) {
LONG RegKey::DeleteEmptyKey(const wchar_t* name) {
DCHECK(name);

// `RegOpenKeyEx()` will return an error if `key_` is invalid.
HKEY target_key = nullptr;
LONG result =
RegOpenKeyEx(key_, name, 0, KEY_READ | wow64access_, &target_key);

if (!Valid()) {
return ERROR_INVALID_HANDLE;
}
RegKey target_key;
LONG result = target_key.Open(key_, name, REG_OPTION_OPEN_LINK,
wow64access_ | KEY_QUERY_VALUE | DELETE);
if (result != ERROR_SUCCESS) {
return result;
}

DWORD count = 0;
result =
RegQueryInfoKey(target_key, nullptr, nullptr, nullptr, nullptr, nullptr,
nullptr, &count, nullptr, nullptr, nullptr, nullptr);

RegCloseKey(target_key);
// A symbolic link has one REG_LINK value identifying the target and no
// subkeys, so it satisfies the criteria of being an "empty" key.
if (auto deleted_link = target_key.DeleteIfLink(); deleted_link.has_value()) {
return deleted_link.value();
}

// The key is not a symbolic link, so see if it has any values.
DWORD value_count = 0;
result = RegQueryInfoKey(target_key.key_, nullptr, nullptr, nullptr, nullptr,
nullptr, nullptr, &value_count, nullptr, nullptr,
nullptr, nullptr);
if (result != ERROR_SUCCESS) {
return result;
}

if (count == 0) {
return RegDeleteKeyEx(key_, name, wow64access_, 0);
}

return ERROR_DIR_NOT_EMPTY;
target_key.Close();
return value_count == 0 ? RegDeleteKeyEx(key_, name, wow64access_, 0)
: ERROR_DIR_NOT_EMPTY;
}

LONG RegKey::DeleteValue(const wchar_t* value_name) {
Expand Down Expand Up @@ -490,23 +485,89 @@ bool RegKey::StartWatching(ChangeCallback callback) {
return true;
}

// static
LONG RegKey::RegDelRecurse(HKEY root_key, const wchar_t* name, REGSAM access) {
// First, see if the key can be deleted without having to recurse.
LONG result = RegDeleteKeyEx(root_key, name, access, 0);
LONG RegKey::Open(HKEY rootkey,
const wchar_t* subkey,
DWORD options,
REGSAM access) {
DCHECK(options == 0 || options == REG_OPTION_OPEN_LINK) << options;
DCHECK(rootkey && subkey && access);
HKEY subhkey = nullptr;

LONG result = RegOpenKeyEx(rootkey, subkey, options, access, &subhkey);
if (result == ERROR_SUCCESS) {
return result;
Close();
key_ = subhkey;
wow64access_ = access & kWow64AccessMask;
}

HKEY target_key = nullptr;
result = RegOpenKeyEx(root_key, name, 0, KEY_ENUMERATE_SUB_KEYS | access,
&target_key);
return result;
}

expected<bool, LONG> RegKey::IsLink() const {
DWORD value_type = 0;
LONG result = ::RegQueryValueEx(key_, L"SymbolicLinkValue",
/*lpReserved=*/nullptr, &value_type,
/*lpData=*/nullptr, /*lpcbData=*/nullptr);
if (result == ERROR_FILE_NOT_FOUND) {
return ok(false);
}
if (result == ERROR_SUCCESS) {
return ok(value_type == REG_LINK);
}
return unexpected(result);
}

absl::optional<LONG> RegKey::DeleteIfLink() {
if (auto is_link = IsLink(); !is_link.has_value()) {
return is_link.error(); // Failed to determine if a link.
} else if (is_link.value() == false) {
return absl::nullopt; // Not a link.
}

const NTSTATUS delete_result = ::NtDeleteKey(key_);
if (delete_result == STATUS_SUCCESS) {
return ERROR_SUCCESS;
}
if (result != ERROR_SUCCESS)
using RtlNtStatusToDosErrorFunction = ULONG(WINAPI*)(NTSTATUS);
static const RtlNtStatusToDosErrorFunction rtl_nt_status_to_dos_error =
reinterpret_cast<RtlNtStatusToDosErrorFunction>(::GetProcAddress(
::GetModuleHandle(L"ntdll.dll"), "RtlNtStatusToDosError"));
if (rtl_nt_status_to_dos_error) {
return static_cast<LONG>(rtl_nt_status_to_dos_error(delete_result));
}
if (delete_result == STATUS_CANNOT_DELETE) {
// The most common cause of failure is the presence of subkeys.
return ERROR_DIR_NOT_EMPTY;
}
// This shouldn't happen since the key was opened with DELETE rights, but it
// is a sensible fallback.
return ERROR_ACCESS_DENIED;
}

// static
LONG RegKey::RegDelRecurse(HKEY root_key, const wchar_t* name, REGSAM access) {
// First, open the key; taking care not to traverse symbolic links.
RegKey target_key;
LONG result = target_key.Open(
root_key, name, REG_OPTION_OPEN_LINK,
access | KEY_ENUMERATE_SUB_KEYS | KEY_QUERY_VALUE | DELETE);
if (result == ERROR_FILE_NOT_FOUND) { // The key doesn't exist.
return ERROR_SUCCESS;
}
if (result != ERROR_SUCCESS) {
return result;
}

// Next, try to delete the key if it is a symbolic link.
if (auto deleted_link = target_key.DeleteIfLink(); deleted_link.has_value()) {
return deleted_link.value();
}

// It's not a symbolic link, so try to delete it without recursing.
result = ::RegDeleteKeyEx(target_key.key_, L"", access, 0);
if (result == ERROR_SUCCESS) {
return result;
}

std::wstring subkey_name(name);

Expand All @@ -522,9 +583,9 @@ LONG RegKey::RegDelRecurse(HKEY root_key, const wchar_t* name, REGSAM access) {
std::wstring key_name;
while (result == ERROR_SUCCESS) {
DWORD key_size = kMaxKeyNameLength;
result =
RegEnumKeyEx(target_key, 0, WriteInto(&key_name, kMaxKeyNameLength),
&key_size, nullptr, nullptr, nullptr, nullptr);
result = ::RegEnumKeyEx(target_key.key_, 0,
WriteInto(&key_name, kMaxKeyNameLength), &key_size,
nullptr, nullptr, nullptr, nullptr);

if (result != ERROR_SUCCESS) {
break;
Expand All @@ -539,10 +600,8 @@ LONG RegKey::RegDelRecurse(HKEY root_key, const wchar_t* name, REGSAM access) {
}
}

RegCloseKey(target_key);

// Try again to delete the key.
result = RegDeleteKeyEx(root_key, name, access, 0);
result = ::RegDeleteKeyEx(target_key.key_, L"", access, 0);

return result;
}
Expand Down
22 changes: 22 additions & 0 deletions base/win/registry.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@

#include "base/base_export.h"
#include "base/functional/callback_forward.h"
#include "base/types/expected.h"
#include "base/win/windows_types.h"
#include "third_party/abseil-cpp/absl/types/optional.h"

namespace base {
namespace win {
Expand Down Expand Up @@ -154,6 +156,26 @@ class BASE_EXPORT RegKey {
private:
class Watcher;

// Opens the key `subkey` under `rootkey` with the given options and
// access rights. `options` may be 0 or `REG_OPTION_OPEN_LINK`. Returns
// ERROR_SUCCESS or a Windows error code.
[[nodiscard]] LONG Open(HKEY rootkey,
const wchar_t* subkey,
DWORD options,
REGSAM access);

// Returns true if the key is a symbolic link, false if it is not, or a
// Windows error code in case of a failure to determine. `this` *MUST* have
// been opened via at least `Open(..., REG_OPTION_OPEN_LINK,
// REG_QUERY_VALUE);`.
expected<bool, LONG> IsLink() const;

// Deletes the key if it is a symbolic link. Returns ERROR_SUCCESS if the key
// was a link and was deleted, a Windows error code if checking the key or
// deleting it failed, or `nullopt` if the key exists and is not a symbolic
// link.
absl::optional<LONG> DeleteIfLink();

// Recursively deletes a key and all of its subkeys.
static LONG RegDelRecurse(HKEY root_key, const wchar_t* name, REGSAM access);

Expand Down
67 changes: 65 additions & 2 deletions base/win/registry_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
#include "base/test/test_mock_time_task_runner.h"
#include "base/test/test_reg_util_win.h"
#include "base/threading/simple_thread.h"
#include "base/win/win_util.h"
#include "base/win/windows_version.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"
Expand All @@ -43,8 +44,8 @@ class RegistryTest : public testing::Test {
RegistryTest() : root_key_(std::wstring(L"Software\\") + kRootKey) {}

void SetUp() override {
ASSERT_NO_FATAL_FAILURE(
registry_override_.OverrideRegistry(HKEY_CURRENT_USER));
ASSERT_NO_FATAL_FAILURE(registry_override_.OverrideRegistry(
HKEY_CURRENT_USER, &override_path_));

// Create the test's root key.
RegKey key(HKEY_CURRENT_USER, L"", KEY_CREATE_SUB_KEY);
Expand All @@ -58,9 +59,12 @@ class RegistryTest : public testing::Test {
// use by a test.
const std::wstring& root_key() const { return root_key_; }

const std::wstring& override_path() const { return override_path_; }

private:
registry_util::RegistryOverrideManager registry_override_;
const std::wstring root_key_;
std::wstring override_path_;
};

} // namespace
Expand Down Expand Up @@ -464,6 +468,65 @@ TEST_F(RegistryTest, DeleteWithInvalidRegKey) {
EXPECT_EQ(key.DeleteValue(kFooName), ERROR_INVALID_HANDLE);
}

// Test that DeleteKey does not follow symbolic links.
TEST_F(RegistryTest, DeleteDoesNotFollowLinks) {
// Create a subkey that should not be deleted.
std::wstring target_path = root_key() + L"\\LinkTarget";
{
RegKey target;
ASSERT_EQ(target.Create(HKEY_CURRENT_USER, target_path.c_str(), KEY_WRITE),
ERROR_SUCCESS);
ASSERT_EQ(target.WriteValue(L"IsTarget", 1U), ERROR_SUCCESS);
}

// Create a link to the above key.
std::wstring source_path = root_key() + L"\\LinkSource";
{
HKEY link_handle = {};
ASSERT_EQ(RegCreateKeyEx(HKEY_CURRENT_USER, source_path.c_str(), 0, nullptr,
REG_OPTION_CREATE_LINK | REG_OPTION_NON_VOLATILE,
KEY_WRITE, nullptr, &link_handle, nullptr),
ERROR_SUCCESS);
RegKey link(std::exchange(link_handle, HKEY{}));
ASSERT_TRUE(link.Valid());

std::wstring user_sid;
ASSERT_TRUE(GetUserSidString(&user_sid));

std::wstring value =
base::StrCat({L"\\Registry\\User\\", user_sid, L"\\", override_path(),
L"\\", root_key(), L"\\LinkTarget"});
ASSERT_EQ(link.WriteValue(L"SymbolicLinkValue", value.data(),
value.size() * sizeof(wchar_t), REG_LINK),
ERROR_SUCCESS);
}

// Verify that the link works.
{
RegKey link;
ASSERT_EQ(link.Open(HKEY_CURRENT_USER, source_path.c_str(), KEY_READ),
ERROR_SUCCESS);
DWORD value = 0;
ASSERT_EQ(link.ReadValueDW(L"IsTarget", &value), ERROR_SUCCESS);
ASSERT_EQ(value, 1U);
}

// Now delete the link and ensure that it was deleted, but not the target.
ASSERT_EQ(RegKey(HKEY_CURRENT_USER, root_key().c_str(), KEY_READ)
.DeleteKey(L"LinkSource"),
ERROR_SUCCESS);
{
RegKey source;
ASSERT_NE(source.Open(HKEY_CURRENT_USER, source_path.c_str(), KEY_READ),
ERROR_SUCCESS);
}
{
RegKey target;
ASSERT_EQ(target.Open(HKEY_CURRENT_USER, target_path.c_str(), KEY_READ),
ERROR_SUCCESS);
}
}

// A test harness for tests that use HKLM to test WoW redirection and such.
// TODO(https://crbug.com/377917): The tests here that write to the registry are
// disabled because they need work to handle parallel runs of different tests.
Expand Down
Loading

0 comments on commit 73fb7b0

Please sign in to comment.