From a461f916f4fb8a70a1b8ae0a91d2ba02f9af2f73 Mon Sep 17 00:00:00 2001 From: "csharp@chromium.org" Date: Mon, 13 Jan 2014 16:57:04 +0000 Subject: Use a Finch Experiment to control the Browser Blacklist Also increase the states stored in the register to prevent the setup code from running multiple times per version if it fails to start one time. Also, this uses RegQueryValueEx and RegSetValueEx instead of RegSetKeyValue and RegGetValue since they aren't available on XP. TBR=sky@chromium.org BUG=329023 Review URL: https://codereview.chromium.org/133923002 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@244517 0039d316-1c4b-4281-b951-d872f2087c98 --- chrome_elf/blacklist.gypi | 6 +- chrome_elf/blacklist/blacklist.cc | 95 +++++++++++++++++++++++++---- chrome_elf/blacklist/blacklist.h | 32 +++++++--- chrome_elf/blacklist/test/blacklist_test.cc | 45 ++++++++++---- chrome_elf/chrome_elf.gyp | 1 + chrome_elf/chrome_elf_main.cc | 2 +- 6 files changed, 147 insertions(+), 34 deletions(-) (limited to 'chrome_elf') diff --git a/chrome_elf/blacklist.gypi b/chrome_elf/blacklist.gypi index ab5597d..2db4941 100644 --- a/chrome_elf/blacklist.gypi +++ b/chrome_elf/blacklist.gypi @@ -6,6 +6,9 @@ { 'target_name': 'blacklist', 'type': 'static_library', + 'include_dirs': [ + '<(SHARED_INTERMEDIATE_DIR)', + ], 'sources': [ 'blacklist/blacklist.cc', 'blacklist/blacklist.h', @@ -17,7 +20,8 @@ # as that would risk pulling in base's link-time dependencies which # chrome_elf cannot do. '../base/base.gyp:base_static', - '../sandbox/sandbox.gyp:sandbox', + '../chrome/chrome.gyp:chrome_version_header', + '../sandbox/sandbox.gyp:sandbox', ], }, { diff --git a/chrome_elf/blacklist/blacklist.cc b/chrome_elf/blacklist/blacklist.cc index c7d1a5d..eda611db 100644 --- a/chrome_elf/blacklist/blacklist.cc +++ b/chrome_elf/blacklist/blacklist.cc @@ -12,6 +12,7 @@ #include "sandbox/win/src/internal_types.h" #include "sandbox/win/src/sandbox_utils.h" #include "sandbox/win/src/service_resolver.h" +#include "version.h" // NOLINT // http://blogs.msdn.com/oldnewthing/archive/2004/10/25/247180.aspx extern "C" IMAGE_DOS_HEADER __ImageBase; @@ -22,6 +23,8 @@ const wchar_t* g_troublesome_dlls[kTroublesomeDllsMaxCount] = {}; int g_troublesome_dlls_cur_index = 0; const wchar_t kRegistryBeaconPath[] = L"SOFTWARE\\Google\\Chrome\\BLBeacon"; +const wchar_t kBeaconVersion[] = L"version"; +const wchar_t kBeaconState[] = L"state"; } // namespace blacklist @@ -140,27 +143,93 @@ bool IsNonBrowserProcess() { namespace blacklist { -bool CreateBeacon() { - HKEY beacon_key = NULL; +bool LeaveSetupBeacon() { + HKEY key = NULL; DWORD disposition = 0; LONG result = ::RegCreateKeyEx(HKEY_CURRENT_USER, kRegistryBeaconPath, 0, NULL, - 0, - KEY_WRITE, + REG_OPTION_NON_VOLATILE, + KEY_QUERY_VALUE | KEY_SET_VALUE, NULL, - &beacon_key, + &key, &disposition); - bool success = (result == ERROR_SUCCESS && - disposition != REG_OPENED_EXISTING_KEY); - if (result == ERROR_SUCCESS) - ::RegCloseKey(beacon_key); - return success; + if (result != ERROR_SUCCESS) + return false; + + // Retrieve the current blacklist state. + DWORD blacklist_state = BLACKLIST_DISABLED; + DWORD blacklist_state_size = sizeof(blacklist_state); + DWORD type = 0; + result = ::RegQueryValueEx(key, + kBeaconState, + 0, + &type, + reinterpret_cast(&blacklist_state), + &blacklist_state_size); + + if (blacklist_state != BLACKLIST_ENABLED || + result != ERROR_SUCCESS || type != REG_DWORD) { + ::RegCloseKey(key); + return false; + } + + // If the blacklist wasn't set as enabled for this version, don't + // use it. + wchar_t key_data[255] = {}; + DWORD key_data_size = sizeof(key_data); + result = ::RegQueryValueEx(key, + blacklist::kBeaconVersion, + 0, + &type, + reinterpret_cast(key_data), + &key_data_size); + + if (wcscmp(key_data, TEXT(CHROME_VERSION_STRING)) != 0 || + result != ERROR_SUCCESS || type != REG_SZ) { + ::RegCloseKey(key); + return false; + } + + // Mark the blacklist setup code as running so if it crashes the blacklist + // won't be enabled for the next run. + blacklist_state = BLACKLIST_SETUP_RUNNING; + result = ::RegSetValueEx(key, + kBeaconState, + 0, + REG_DWORD, + reinterpret_cast(&blacklist_state), + sizeof(blacklist_state)); + ::RegCloseKey(key); + + return (result == ERROR_SUCCESS); } -bool ClearBeacon() { - LONG result = ::RegDeleteKey(HKEY_CURRENT_USER, kRegistryBeaconPath); +bool ResetBeacon() { + HKEY key = NULL; + DWORD disposition = 0; + LONG result = ::RegCreateKeyEx(HKEY_CURRENT_USER, + kRegistryBeaconPath, + 0, + NULL, + REG_OPTION_NON_VOLATILE, + KEY_QUERY_VALUE | KEY_SET_VALUE, + NULL, + &key, + &disposition); + if (result != ERROR_SUCCESS) + return false; + + DWORD blacklist_state = BLACKLIST_ENABLED; + result = ::RegSetValueEx(key, + kBeaconState, + 0, + REG_DWORD, + reinterpret_cast(&blacklist_state), + sizeof(blacklist_state)); + ::RegCloseKey(key); + return (result == ERROR_SUCCESS); } @@ -212,7 +281,7 @@ bool Initialize(bool force) { return false; // Check to see if a beacon is present, abort if so. - if (!force && !CreateBeacon()) + if (!force && !LeaveSetupBeacon()) return false; // Don't try blacklisting on unsupported OS versions. diff --git a/chrome_elf/blacklist/blacklist.h b/chrome_elf/blacklist/blacklist.h index 5787ddd..08ca572 100644 --- a/chrome_elf/blacklist/blacklist.h +++ b/chrome_elf/blacklist/blacklist.h @@ -16,20 +16,36 @@ extern const wchar_t* g_troublesome_dlls[kTroublesomeDllsMaxCount]; // Cursor to the current last element in the blacklist. extern int g_troublesome_dlls_cur_index; -// The registry path of the blacklist beacon. Exposed here for testing. +// The registry path of the blacklist beacon. extern const wchar_t kRegistryBeaconPath[]; -// Attempts to create a beacon in the current user's registry hive. -// If the beacon already exists or any other error occurs when creating the -// beacon, returns false. Otherwise returns true. +// The properties for the blacklist beacon. +extern const wchar_t kBeaconVersion[]; +extern const wchar_t kBeaconState[]; + +// The states for the blacklist setup code. +enum BlacklistState { + BLACKLIST_DISABLED = 0, + BLACKLIST_ENABLED, + // The blacklist setup code is running. If this is still set at startup, + // it means the last setup crashed. + BLACKLIST_SETUP_RUNNING, + // Always keep this at the end. + BLACKLIST_STATE_MAX, +}; + +// Attempts to leave a beacon in the current user's registry hive. +// If the blacklist beacon doesn't say it is enabled or there are any other +// errors when creating the beacon, returns false. Otherwise returns true. // The intent of the beacon is to act as an extra failure mode protection // whereby if Chrome for some reason fails to start during blacklist setup, // it will skip blacklisting on the subsequent run. -bool CreateBeacon(); +bool LeaveSetupBeacon(); -// Looks for the beacon that CreateBeacon() creates and attempts to delete it. -// Returns true if the beacon was found and deleted. -bool ClearBeacon(); +// Looks for the beacon that LeaveSetupBeacon() creates and resets it to +// to show the setup was successful. +// Returns true if the beacon was successfully set to BLACKLIST_ENABLED. +bool ResetBeacon(); // Adds the given dll name to the blacklist. Returns true if the dll name is in // the blacklist when this returns, false on error. Note that this will copy diff --git a/chrome_elf/blacklist/test/blacklist_test.cc b/chrome_elf/blacklist/test/blacklist_test.cc index dbb662f..90ce267 100644 --- a/chrome_elf/blacklist/test/blacklist_test.cc +++ b/chrome_elf/blacklist/test/blacklist_test.cc @@ -11,9 +11,11 @@ #include "base/strings/string_number_conversions.h" #include "base/strings/utf_string_conversions.h" #include "base/test/test_reg_util_win.h" +#include "base/win/registry.h" #include "chrome_elf/blacklist/blacklist.h" #include "chrome_elf/blacklist/test/blacklist_test_main_dll.h" #include "testing/gtest/include/gtest/gtest.h" +#include "version.h" // NOLINT const wchar_t kTestDllName1[] = L"blacklist_test_dll_1.dll"; const wchar_t kTestDllName2[] = L"blacklist_test_dll_2.dll"; @@ -37,9 +39,6 @@ class BlacklistTest : public testing::Test { virtual void SetUp() { // Force an import from blacklist_test_main_dll. InitBlacklistTestDll(); - - // Ensure that the beacon state starts off cleared. - blacklist::ClearBeacon(); } virtual void TearDown() { @@ -52,17 +51,41 @@ TEST_F(BlacklistTest, Beacon) { registry_util::RegistryOverrideManager override_manager; override_manager.OverrideRegistry(HKEY_CURRENT_USER, L"beacon_test"); - // First call should succeed as the beacon is newly created. - EXPECT_TRUE(blacklist::CreateBeacon()); + base::win::RegKey blacklist_registry_key(HKEY_CURRENT_USER, + blacklist::kRegistryBeaconPath, + KEY_QUERY_VALUE | KEY_SET_VALUE); + + // Ensure that the beacon state starts off enabled for this version. + LONG result = blacklist_registry_key.WriteValue(blacklist::kBeaconState, + blacklist::BLACKLIST_ENABLED); + EXPECT_EQ(ERROR_SUCCESS, result); + + result = blacklist_registry_key.WriteValue(blacklist::kBeaconVersion, + TEXT(CHROME_VERSION_STRING)); + EXPECT_EQ(ERROR_SUCCESS, result); + + // First call should find the beacon and reset it. + EXPECT_TRUE(blacklist::ResetBeacon()); + + // First call should succeed as the beacon is enabled. + EXPECT_TRUE(blacklist::LeaveSetupBeacon()); + + // Second call should fail indicating the beacon wasn't set as enabled. + EXPECT_FALSE(blacklist::LeaveSetupBeacon()); + + // Resetting the beacon should work when setup beacon is present. + EXPECT_TRUE(blacklist::ResetBeacon()); - // Second call should fail indicating the beacon already existed. - EXPECT_FALSE(blacklist::CreateBeacon()); + // Change the version and ensure that the setup fails due to the version + // mismatch. + base::string16 different_version(L"other_version"); + ASSERT_NE(different_version, TEXT(CHROME_VERSION_STRING)); - // First call should find the beacon and delete it. - EXPECT_TRUE(blacklist::ClearBeacon()); + result = blacklist_registry_key.WriteValue(blacklist::kBeaconVersion, + different_version.c_str()); + EXPECT_EQ(ERROR_SUCCESS, result); - // Second call should fail to find the beacon and delete it. - EXPECT_FALSE(blacklist::ClearBeacon()); + EXPECT_FALSE(blacklist::LeaveSetupBeacon()); } TEST_F(BlacklistTest, AddAndRemoveModules) { diff --git a/chrome_elf/chrome_elf.gyp b/chrome_elf/chrome_elf.gyp index 7227fe6..8a4e87e 100644 --- a/chrome_elf/chrome_elf.gyp +++ b/chrome_elf/chrome_elf.gyp @@ -49,6 +49,7 @@ ], 'include_dirs': [ '..', + '<(SHARED_INTERMEDIATE_DIR)', ], 'dependencies': [ 'chrome_elf_lib', diff --git a/chrome_elf/chrome_elf_main.cc b/chrome_elf/chrome_elf_main.cc index 9ad8299..772c906 100644 --- a/chrome_elf/chrome_elf_main.cc +++ b/chrome_elf/chrome_elf_main.cc @@ -10,7 +10,7 @@ #include "chrome_elf/ntdll_cache.h" void SignalChromeElf() { - blacklist::ClearBeacon(); + blacklist::ResetBeacon(); } BOOL APIENTRY DllMain(HMODULE module, DWORD reason, LPVOID reserved) { -- cgit v1.1