From c4da848392dabb7b49e554a15ad47d1c9f0dac78 Mon Sep 17 00:00:00 2001 From: pennymac Date: Wed, 27 Jul 2016 16:24:35 -0700 Subject: [PATCH] [chrome_elf] Removing blacklist finch for dynamic dll changes. Part of chrome_elf cleanup. This was never used in the wild, and we won't need it in the foreseeable future. Leaving beacon and emergency disable finch switch for safety. Tests: 1) chrome_elf_unittests, chrome_elf_util_unittest.cc: BlacklistTest* 2) unit_tests, chrome_elf_init_unittest_win.cc: ChromeBlacklistTrialTest* R=robertshield@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng BUG=631771 Review-Url: https://codereview.chromium.org/2163803003 Cr-Commit-Position: refs/heads/master@{#408269} --- .../environment_data_collection_win.cc | 13 ----- ...nvironment_data_collection_win_unittest.cc | 49 ------------------ chrome/browser/win/chrome_elf_init.cc | 37 -------------- chrome/browser/win/chrome_elf_init.h | 3 -- .../browser/win/chrome_elf_init_unittest.cc | 43 ---------------- chrome/installer/setup/uninstall.cc | 11 +++- chrome_elf/blacklist/blacklist.cc | 18 ------- chrome_elf/blacklist/blacklist.h | 4 -- chrome_elf/blacklist/test/blacklist_test.cc | 51 ++----------------- .../test/blacklist_test_main_dll.def | 1 - chrome_elf/chrome_elf_constants.cc | 5 -- chrome_elf/chrome_elf_constants.h | 10 ---- 12 files changed, 14 insertions(+), 231 deletions(-) diff --git a/chrome/browser/safe_browsing/incident_reporting/environment_data_collection_win.cc b/chrome/browser/safe_browsing/incident_reporting/environment_data_collection_win.cc index bb067db15f284..f29de2b8d3b9e 100644 --- a/chrome/browser/safe_browsing/incident_reporting/environment_data_collection_win.cc +++ b/chrome/browser/safe_browsing/incident_reporting/environment_data_collection_win.cc @@ -234,18 +234,6 @@ void RecordLspFeature(ClientIncidentReport_EnvironmentData_Process* process) { } } -void CollectDllBlacklistData( - ClientIncidentReport_EnvironmentData_Process* process) { - PathSanitizer path_sanitizer; - base::win::RegistryValueIterator iter(HKEY_CURRENT_USER, - blacklist::kRegistryFinchListPath); - for (; iter.Valid(); ++iter) { - base::FilePath dll_name(iter.Value()); - path_sanitizer.StripHomeDirectory(&dll_name); - process->add_blacklisted_dll(dll_name.AsUTF8Unsafe()); - } -} - void CollectModuleVerificationData( const wchar_t* const modules_to_verify[], size_t num_modules_to_verify, @@ -311,7 +299,6 @@ void CollectPlatformProcessData( ClientIncidentReport_EnvironmentData_Process* process) { CollectDlls(process); RecordLspFeature(process); - CollectDllBlacklistData(process); CollectModuleVerificationData( kModulesToVerify, arraysize(kModulesToVerify), process); } diff --git a/chrome/browser/safe_browsing/incident_reporting/environment_data_collection_win_unittest.cc b/chrome/browser/safe_browsing/incident_reporting/environment_data_collection_win_unittest.cc index d429dacdffb3c..70d09fee2eb48 100644 --- a/chrome/browser/safe_browsing/incident_reporting/environment_data_collection_win_unittest.cc +++ b/chrome/browser/safe_browsing/incident_reporting/environment_data_collection_win_unittest.cc @@ -29,8 +29,6 @@ namespace safe_browsing { namespace { -const wchar_t test_dll[] = L"test_name.dll"; - // Returns true if a dll with filename |dll_name| is found in |process_report|, // providing a copy of it in |result|. bool GetProcessReportDll( @@ -125,53 +123,6 @@ TEST(SafeBrowsingEnvironmentDataCollectionWinTest, RecordLspFeature) { FAIL() << "No LSP feature found for " << lsp; } -TEST(SafeBrowsingEnvironmentDataCollectionWinTest, CollectDllBlacklistData) { - // Ensure that CollectDllBlacklistData correctly adds the set of sanitized dll - // names currently stored in the registry to the report. - registry_util::RegistryOverrideManager override_manager; - override_manager.OverrideRegistry(HKEY_CURRENT_USER); - - base::win::RegKey blacklist_registry_key(HKEY_CURRENT_USER, - blacklist::kRegistryFinchListPath, - KEY_QUERY_VALUE | KEY_SET_VALUE); - - // Check that with an empty registry the blacklisted dlls field is left empty. - ClientIncidentReport_EnvironmentData_Process process_report; - CollectDllBlacklistData(&process_report); - EXPECT_EQ(0, process_report.blacklisted_dll_size()); - - // Check that after adding exactly one dll to the registry it appears in the - // process report. - blacklist_registry_key.WriteValue(test_dll, test_dll); - CollectDllBlacklistData(&process_report); - ASSERT_EQ(1, process_report.blacklisted_dll_size()); - - base::string16 process_report_dll = - base::UTF8ToWide(process_report.blacklisted_dll(0)); - EXPECT_EQ(base::string16(test_dll), process_report_dll); - - // Check that if the registry contains the full path to a dll it is properly - // sanitized before being reported. - blacklist_registry_key.DeleteValue(test_dll); - process_report.clear_blacklisted_dll(); - - base::FilePath path; - ASSERT_TRUE(PathService::Get(base::DIR_HOME, &path)); - base::string16 input_path = - path.Append(FILE_PATH_LITERAL("test_path.dll")).value(); - - std::string path_expected = base::FilePath(FILE_PATH_LITERAL("~")) - .Append(FILE_PATH_LITERAL("test_path.dll")) - .AsUTF8Unsafe(); - - blacklist_registry_key.WriteValue(input_path.c_str(), input_path.c_str()); - CollectDllBlacklistData(&process_report); - - ASSERT_EQ(1, process_report.blacklisted_dll_size()); - std::string process_report_path = process_report.blacklisted_dll(0); - EXPECT_EQ(path_expected, process_report_path); -} - #if !defined(_WIN64) TEST(SafeBrowsingEnvironmentDataCollectionWinTest, VerifyLoadedModules) { // Load the test modules. diff --git a/chrome/browser/win/chrome_elf_init.cc b/chrome/browser/win/chrome_elf_init.cc index a0a4f482548e1..5c091296c46aa 100644 --- a/chrome/browser/win/chrome_elf_init.cc +++ b/chrome/browser/win/chrome_elf_init.cc @@ -95,7 +95,6 @@ void InitializeChromeElf() { base::win::RegKey blacklist_registry_key(HKEY_CURRENT_USER); blacklist_registry_key.DeleteKey(blacklist::kRegistryBeaconPath); } else { - AddFinchBlacklistToRegistry(); BrowserBlacklistBeaconSetup(); } @@ -112,42 +111,6 @@ void InitializeChromeElf() { base::TimeDelta::FromSeconds(kBlacklistReportingDelaySec)); } -// Note that running multiple chrome instances with distinct user data -// directories could lead to deletion (and/or replacement) of the finch -// blacklist registry data in one instance before the second has a chance to -// read those values. -void AddFinchBlacklistToRegistry() { - base::win::RegKey finch_blacklist_registry_key( - HKEY_CURRENT_USER, blacklist::kRegistryFinchListPath, KEY_SET_VALUE); - - // No point in trying to continue if the registry key isn't valid. - if (!finch_blacklist_registry_key.Valid()) - return; - - // Delete and recreate the key to clear the registry. - finch_blacklist_registry_key.DeleteKey(L""); - finch_blacklist_registry_key.Create( - HKEY_CURRENT_USER, blacklist::kRegistryFinchListPath, KEY_SET_VALUE); - - std::map params; - std::string value = variations::GetVariationParamValue( - kBrowserBlacklistTrialName, blacklist::kRegistryFinchListValueNameStr); - if (value.empty()) - return; - base::string16 value_wcs = base::UTF8ToWide(value); - - // The dll names are comma-separated in this param value. We need to turn - // this into REG_MULTI_SZ format (double-null terminates). - // Note that the strings are wide character in registry. - value_wcs.push_back(L'\0'); - value_wcs.push_back(L'\0'); - std::replace(value_wcs.begin(), value_wcs.end(), L',', L'\0'); - - finch_blacklist_registry_key.WriteValue( - blacklist::kRegistryFinchListValueName, value_wcs.data(), - (value_wcs.size() * sizeof(wchar_t)), REG_MULTI_SZ); -} - void BrowserBlacklistBeaconSetup() { base::win::RegKey blacklist_registry_key(HKEY_CURRENT_USER, blacklist::kRegistryBeaconPath, diff --git a/chrome/browser/win/chrome_elf_init.h b/chrome/browser/win/chrome_elf_init.h index c2661145cfb3b..f598828795f18 100644 --- a/chrome/browser/win/chrome_elf_init.h +++ b/chrome/browser/win/chrome_elf_init.h @@ -13,9 +13,6 @@ extern const char kBrowserBlacklistTrialDisabledGroupName[]; // only affect future runs since Chrome Elf is already setup by this point). void InitializeChromeElf(); -// Add the blacklist from the finch configs in the registry. -void AddFinchBlacklistToRegistry(); - // Set the required state for an enabled browser blacklist. void BrowserBlacklistBeaconSetup(); diff --git a/chrome/browser/win/chrome_elf_init_unittest.cc b/chrome/browser/win/chrome_elf_init_unittest.cc index 3c23e4929edde..295fbd763be4b 100644 --- a/chrome/browser/win/chrome_elf_init_unittest.cc +++ b/chrome/browser/win/chrome_elf_init_unittest.cc @@ -21,8 +21,6 @@ namespace { -const char kBrowserBlacklistTrialEnabledGroupName[] = "Enabled"; - class ChromeBlacklistTrialTest : public testing::Test { protected: ChromeBlacklistTrialTest() {} @@ -156,45 +154,4 @@ TEST_F(ChromeBlacklistTrialTest, VersionChanged) { ASSERT_EQ(static_cast(0), attempt_count); } -TEST_F(ChromeBlacklistTrialTest, AddFinchBlacklistToRegistry) { - // Create the field trial with the blacklist enabled group. - base::FieldTrialList field_trial_list( - new metrics::SHA1EntropyProvider("test")); - - scoped_refptr trial(base::FieldTrialList::CreateFieldTrial( - kBrowserBlacklistTrialName, kBrowserBlacklistTrialEnabledGroupName)); - - // Set up the trial with the desired parameters. - std::map desired_params; - - desired_params[blacklist::kRegistryFinchListValueNameStr] = - "TestDll1.dll,TestDll2.dll"; - - variations::AssociateVariationParams( - kBrowserBlacklistTrialName, - kBrowserBlacklistTrialEnabledGroupName, - desired_params); - - // This should add the dlls in those parameters to the registry. - AddFinchBlacklistToRegistry(); - - // Check that all the dll names in desired_params were added to the registry. - std::vector dlls; - - base::win::RegKey finch_blacklist_registry_key( - HKEY_CURRENT_USER, - blacklist::kRegistryFinchListPath, - KEY_QUERY_VALUE | KEY_SET_VALUE); - - ASSERT_TRUE(finch_blacklist_registry_key.HasValue( - blacklist::kRegistryFinchListValueName)); - ASSERT_EQ(ERROR_SUCCESS, finch_blacklist_registry_key.ReadValues( - blacklist::kRegistryFinchListValueName, &dlls)); - - ASSERT_EQ((size_t)2, - /* Number of dll names passed in this test. */ dlls.size()); - EXPECT_STREQ(L"TestDll1.dll", dlls[0].c_str()); - EXPECT_STREQ(L"TestDll2.dll", dlls[1].c_str()); -} - } // namespace diff --git a/chrome/installer/setup/uninstall.cc b/chrome/installer/setup/uninstall.cc index 24f2ba07d2e15..8f4a4dedd9d55 100644 --- a/chrome/installer/setup/uninstall.cc +++ b/chrome/installer/setup/uninstall.cc @@ -786,8 +786,17 @@ void RemoveBlacklistState() { InstallUtil::DeleteRegistryKey(HKEY_CURRENT_USER, blacklist::kRegistryBeaconPath, 0); // wow64_access +// The following key is no longer used (https://crbug.com/631771). +// This cleanup is being left in for a time though. +#if defined(GOOGLE_CHROME_BUILD) + const wchar_t kRegistryFinchListPath[] = + L"SOFTWARE\\Google\\Chrome\\BLFinchList"; +#else + const wchar_t kRegistryFinchListPath[] = + L"SOFTWARE\\Chromium\\BLFinchList"; +#endif InstallUtil::DeleteRegistryKey(HKEY_CURRENT_USER, - blacklist::kRegistryFinchListPath, + kRegistryFinchListPath, 0); // wow64_access } diff --git a/chrome_elf/blacklist/blacklist.cc b/chrome_elf/blacklist/blacklist.cc index 0fdf91d274276..2b441a6a16efd 100644 --- a/chrome_elf/blacklist/blacklist.cc +++ b/chrome_elf/blacklist/blacklist.cc @@ -347,25 +347,7 @@ bool Initialize(bool force) { VirtualProtect(&g_thunk_storage, sizeof(g_thunk_storage), PAGE_EXECUTE_READ, &old_protect); - AddDllsFromRegistryToBlacklist(); - return NT_SUCCESS(ret) && page_executable; } -void AddDllsFromRegistryToBlacklist() { - std::vector dlls; - - if (!nt::QueryRegValueMULTISZ(nt::HKCU, kRegistryFinchListPath, - kRegistryFinchListValueName, &dlls) || - dlls.empty()) - return; - - // Add each DLL to the BL in memory - for (auto name : dlls) { - AddDllToBlacklist(name.c_str()); - } - - return; -} - } // namespace blacklist diff --git a/chrome_elf/blacklist/blacklist.h b/chrome_elf/blacklist/blacklist.h index caf9dbbb7b131..0b6a0a72e1e81 100644 --- a/chrome_elf/blacklist/blacklist.h +++ b/chrome_elf/blacklist/blacklist.h @@ -66,10 +66,6 @@ extern "C" bool RemoveDllFromBlacklist(const wchar_t* dll_name); // is only exposed in tests (and should stay that way). extern "C" void SuccessfullyBlocked(const wchar_t** blocked_dlls, int* size); -// Add the dlls, originally passed in through finch, from the registry to the -// blacklist so that they will be blocked identically to those hard coded in. -extern "C" void AddDllsFromRegistryToBlacklist(); - // Record that the dll at the given index was blocked. extern "C" void BlockedDll(size_t blocked_index); diff --git a/chrome_elf/blacklist/test/blacklist_test.cc b/chrome_elf/blacklist/test/blacklist_test.cc index a0c55f2f5bba3..b970057e0d767 100644 --- a/chrome_elf/blacklist/test/blacklist_test.cc +++ b/chrome_elf/blacklist/test/blacklist_test.cc @@ -37,7 +37,6 @@ extern const wchar_t* kEnvVars[]; namespace { // Functions we need from blacklist_test_main_dll.dll -typedef void (*TestDll_AddDllsFromRegistryToBlacklistFunction)(); typedef bool (*TestDll_AddDllToBlacklistFunction)(const wchar_t* dll_name); typedef int (*TestDll_BlacklistSizeFunction)(); typedef void (*TestDll_BlockedDllFunction)(size_t blocked_index); @@ -49,8 +48,6 @@ typedef bool (*TestDll_SuccessfullyBlockedFunction)( int* size); typedef void (*InitTestDllFunction)(); -TestDll_AddDllsFromRegistryToBlacklistFunction - TestDll_AddDllsFromRegistryToBlacklist = nullptr; TestDll_AddDllToBlacklistFunction TestDll_AddDllToBlacklist = nullptr; TestDll_BlacklistSizeFunction TestDll_BlacklistSize = nullptr; TestDll_BlockedDllFunction TestDll_BlockedDll = nullptr; @@ -160,9 +157,6 @@ class BlacklistTest : public testing::Test { dll = ::LoadLibraryW(L"blacklist_test_main_dll.dll"); if (!dll) return; - TestDll_AddDllsFromRegistryToBlacklist = - reinterpret_cast( - ::GetProcAddress(dll, "TestDll_AddDllsFromRegistryToBlacklist")); TestDll_AddDllToBlacklist = reinterpret_cast( ::GetProcAddress(dll, "TestDll_AddDllToBlacklist")); @@ -184,11 +178,10 @@ class BlacklistTest : public testing::Test { ::GetProcAddress(dll, "TestDll_SuccessfullyBlocked")); InitTestDll = reinterpret_cast( ::GetProcAddress(dll, "InitTestDll")); - if (!TestDll_AddDllsFromRegistryToBlacklist || !TestDll_AddDllToBlacklist || - !TestDll_BlacklistSize || !TestDll_BlockedDll || - !TestDll_GetBlacklistIndex || !TestDll_IsBlacklistInitialized || - !TestDll_RemoveDllFromBlacklist || !TestDll_SuccessfullyBlocked || - !InitTestDll) + if (!TestDll_AddDllToBlacklist || !TestDll_BlacklistSize || + !TestDll_BlockedDll || !TestDll_GetBlacklistIndex || + !TestDll_IsBlacklistInitialized || !TestDll_RemoveDllFromBlacklist || + !TestDll_SuccessfullyBlocked || !InitTestDll) return; // We have to call this exported function every time this test setup runs. @@ -316,42 +309,6 @@ TEST_F(BlacklistTest, LoadBlacklistedLibrary) { CheckBlacklistedDllsNotLoaded(); } -TEST_F(BlacklistTest, AddDllsFromRegistryToBlacklist) { - // Ensure that the blacklist is loaded. - ASSERT_TRUE(TestDll_IsBlacklistInitialized()); - - // Delete the finch registry key to clear its values. - base::win::RegKey key(HKEY_CURRENT_USER, - blacklist::kRegistryFinchListPath, - KEY_QUERY_VALUE | KEY_SET_VALUE); - key.DeleteKey(L""); - - // Add the test dlls to the registry. - // (REG_MULTI_SZ: eos separated, double null terminated.) - base::win::RegKey finch_blacklist_registry_key( - HKEY_CURRENT_USER, - blacklist::kRegistryFinchListPath, - KEY_QUERY_VALUE | KEY_SET_VALUE); - - std::vector(reg_buffer); - for (size_t i = 0; i < arraysize(test_data); ++i) { - if (reg_buffer.size() > 0) - reg_buffer.push_back(L'\0'); - const wchar_t* dll = test_data[i].dll_name; - // Append the name, not including terminator. - reg_buffer.insert(reg_buffer.end(), dll, dll + ::wcslen(dll)); - } - reg_buffer.push_back(L'\0'); - reg_buffer.push_back(L'\0'); - - finch_blacklist_registry_key.WriteValue( - blacklist::kRegistryFinchListValueName, reg_buffer.data(), - (DWORD)(reg_buffer.size() * sizeof(wchar_t)), REG_MULTI_SZ); - - TestDll_AddDllsFromRegistryToBlacklist(); - CheckBlacklistedDllsNotLoaded(); -} - void TestResetBeacon(std::unique_ptr& key, DWORD input_state, DWORD expected_output_state) { diff --git a/chrome_elf/blacklist/test/blacklist_test_main_dll.def b/chrome_elf/blacklist/test/blacklist_test_main_dll.def index 0a6efab431969..e22f6e317be08 100644 --- a/chrome_elf/blacklist/test/blacklist_test_main_dll.def +++ b/chrome_elf/blacklist/test/blacklist_test_main_dll.def @@ -5,7 +5,6 @@ LIBRARY "blacklist_test_main_dll.dll" EXPORTS - TestDll_AddDllsFromRegistryToBlacklist=AddDllsFromRegistryToBlacklist TestDll_AddDllToBlacklist=AddDllToBlacklist TestDll_BlacklistSize=BlacklistSize TestDll_BlockedDll=BlockedDll diff --git a/chrome_elf/chrome_elf_constants.cc b/chrome_elf/chrome_elf_constants.cc index 9b9e944a1fa54..7115aaf4d14b2 100644 --- a/chrome_elf/chrome_elf_constants.cc +++ b/chrome_elf/chrome_elf_constants.cc @@ -19,10 +19,6 @@ namespace blacklist { const wchar_t kRegistryBeaconPath[] = L"SOFTWARE\\" PRODUCT_STRING_PATH L"\\BLBeacon"; -const wchar_t kRegistryFinchListPath[] = - L"SOFTWARE\\" PRODUCT_STRING_PATH L"\\BLFinchList"; -const char kRegistryFinchListValueNameStr[] = "BLDlls"; -const wchar_t kRegistryFinchListValueName[] = L"BLDlls"; const wchar_t kBeaconVersion[] = L"version"; const wchar_t kBeaconState[] = L"state"; const wchar_t kBeaconAttemptCount[] = L"failed_count"; @@ -35,7 +31,6 @@ namespace elf_sec { const wchar_t kRegSecurityFinchPath[] = L"SOFTWARE\\" PRODUCT_STRING_PATH L"\\BrowserSboxFinch"; - const wchar_t kRegSecurityPath[] = L"SOFTWARE\\" PRODUCT_STRING_PATH L"\\BrowserSec"; diff --git a/chrome_elf/chrome_elf_constants.h b/chrome_elf/chrome_elf_constants.h index 58c635bb29e02..0039b8960d526 100644 --- a/chrome_elf/chrome_elf_constants.h +++ b/chrome_elf/chrome_elf_constants.h @@ -14,16 +14,6 @@ namespace blacklist { // The registry path of the blacklist beacon. extern const wchar_t kRegistryBeaconPath[]; -// The registry path of the finch blacklist dlls. -extern const wchar_t kRegistryFinchListPath[]; - -// The registry value name for the REG_MULTI_SZ list of blacklist dlls. -// Note the char version is handy for use as the param name when -// appending dll names to the base::FieldTrial. Can be removed -// if no longer used. -extern const char kRegistryFinchListValueNameStr[]; -extern const wchar_t kRegistryFinchListValueName[]; - // The properties for the blacklist beacon. extern const wchar_t kBeaconVersion[]; extern const wchar_t kBeaconState[];