diff options
author | krstnmnlsn@chromium.org <krstnmnlsn@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2014-06-27 19:27:07 +0000 |
---|---|---|
committer | krstnmnlsn@chromium.org <krstnmnlsn@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2014-06-27 19:27:07 +0000 |
commit | 1c2bbf47bd5c796939967934f23862b2ef5a4814 (patch) | |
tree | 3c62b5b7bc8e3c7c8f79b1d40c9307aa1507fad0 | |
parent | b3313b71eb51e5fc5e83343e437c86b98c49be54 (diff) | |
download | chromium_src-1c2bbf47bd5c796939967934f23862b2ef5a4814.zip chromium_src-1c2bbf47bd5c796939967934f23862b2ef5a4814.tar.gz chromium_src-1c2bbf47bd5c796939967934f23862b2ef5a4814.tar.bz2 |
Adding blacklisted dlls to safe browsing incident reports.
Also added a unit test to environment_data_collection_win_unittest.cc
Edited chrome_elf_init_win.cc and blacklist.cc so that the registry is
only cleared immediately before being repopulated (and safe browsing is not left
with an empty registry to read from).
BUG=
Review URL: https://codereview.chromium.org/346763003
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@280380 0039d316-1c4b-4281-b951-d872f2087c98
-rw-r--r-- | chrome/browser/chrome_elf_init_win.cc | 9 | ||||
-rw-r--r-- | chrome/browser/safe_browsing/environment_data_collection_win.cc | 15 | ||||
-rw-r--r-- | chrome/browser/safe_browsing/environment_data_collection_win.h | 5 | ||||
-rw-r--r-- | chrome/browser/safe_browsing/environment_data_collection_win_unittest.cc | 56 | ||||
-rw-r--r-- | chrome/common/safe_browsing/csd.proto | 1 | ||||
-rw-r--r-- | chrome_elf/blacklist/blacklist.cc | 21 | ||||
-rw-r--r-- | chrome_elf/blacklist/blacklist.h | 2 | ||||
-rw-r--r-- | chrome_elf/blacklist/test/blacklist_test.cc | 4 |
8 files changed, 99 insertions, 14 deletions
diff --git a/chrome/browser/chrome_elf_init_win.cc b/chrome/browser/chrome_elf_init_win.cc index 9be2d0a..ce9d3e7 100644 --- a/chrome/browser/chrome_elf_init_win.cc +++ b/chrome/browser/chrome_elf_init_win.cc @@ -109,6 +109,10 @@ 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); @@ -117,6 +121,11 @@ void AddFinchBlacklistToRegistry() { 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<std::string, std::string> params; chrome_variations::GetVariationParams(kBrowserBlacklistTrialName, ¶ms); diff --git a/chrome/browser/safe_browsing/environment_data_collection_win.cc b/chrome/browser/safe_browsing/environment_data_collection_win.cc index a6498d3..4872b54 100644 --- a/chrome/browser/safe_browsing/environment_data_collection_win.cc +++ b/chrome/browser/safe_browsing/environment_data_collection_win.cc @@ -10,11 +10,13 @@ #include "base/i18n/case_conversion.h" #include "base/strings/string_util.h" #include "base/strings/utf_string_conversions.h" +#include "base/win/registry.h" #include "chrome/browser/install_verification/win/module_info.h" #include "chrome/browser/install_verification/win/module_verification_common.h" #include "chrome/browser/net/service_providers_win.h" #include "chrome/browser/safe_browsing/path_sanitizer.h" #include "chrome/common/safe_browsing/csd.pb.h" +#include "chrome_elf/chrome_elf_constants.h" namespace safe_browsing { @@ -88,10 +90,23 @@ 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 CollectPlatformProcessData( ClientIncidentReport_EnvironmentData_Process* process) { CollectDlls(process); RecordLspFeature(process); + CollectDllBlacklistData(process); } } // namespace safe_browsing diff --git a/chrome/browser/safe_browsing/environment_data_collection_win.h b/chrome/browser/safe_browsing/environment_data_collection_win.h index f32bc83..f46a3a0 100644 --- a/chrome/browser/safe_browsing/environment_data_collection_win.h +++ b/chrome/browser/safe_browsing/environment_data_collection_win.h @@ -18,6 +18,11 @@ bool CollectDlls(ClientIncidentReport_EnvironmentData_Process* process); // check one of them is a registered LSP. void RecordLspFeature(ClientIncidentReport_EnvironmentData_Process* process); +// Populates |process| with the dll names that have been added to the chrome elf +// blacklist through the Windows registry. +void CollectDllBlacklistData( + ClientIncidentReport_EnvironmentData_Process* process); + } // namespace safe_browsing #endif // CHROME_BROWSER_SAFE_BROWSING_ENVIRONMENT_DATA_COLLECTION_WIN_H_ diff --git a/chrome/browser/safe_browsing/environment_data_collection_win_unittest.cc b/chrome/browser/safe_browsing/environment_data_collection_win_unittest.cc index 7b8729e..3d6fda2 100644 --- a/chrome/browser/safe_browsing/environment_data_collection_win_unittest.cc +++ b/chrome/browser/safe_browsing/environment_data_collection_win_unittest.cc @@ -6,14 +6,23 @@ #include <string> +#include "base/base_paths.h" #include "base/files/file_path.h" +#include "base/path_service.h" #include "base/scoped_native_library.h" +#include "base/strings/utf_string_conversions.h" +#include "base/test/test_reg_util_win.h" +#include "base/win/registry.h" +#include "chrome/browser/safe_browsing/path_sanitizer.h" #include "chrome/common/safe_browsing/csd.pb.h" +#include "chrome_elf/chrome_elf_constants.h" #include "net/base/winsock_init.h" #include "testing/gtest/include/gtest/gtest.h" namespace { +const wchar_t test_dll[] = L"test_name.dll"; + // Helper function that returns true if a dll with filename |dll_name| is // found in |process_report|. bool ProcessReportContainsDll( @@ -115,3 +124,50 @@ TEST(SafeBrowsingEnvironmentDataCollectionWinTest, RecordLspFeature) { ASSERT_TRUE(lsp_feature_found); } + +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, L"safe_browsing_test"); + + 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. + safe_browsing::ClientIncidentReport_EnvironmentData_Process process_report; + safe_browsing::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); + safe_browsing::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()); + safe_browsing::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); +} diff --git a/chrome/common/safe_browsing/csd.proto b/chrome/common/safe_browsing/csd.proto index 284ec45..77c015a 100644 --- a/chrome/common/safe_browsing/csd.proto +++ b/chrome/common/safe_browsing/csd.proto @@ -410,6 +410,7 @@ message ClientIncidentReport { repeated Feature feature = 4; } repeated Dll dll = 9; + repeated string blacklisted_dll = 10; } optional Process process = 3; } diff --git a/chrome_elf/blacklist/blacklist.cc b/chrome_elf/blacklist/blacklist.cc index 46f06975..55afb87 100644 --- a/chrome_elf/blacklist/blacklist.cc +++ b/chrome_elf/blacklist/blacklist.cc @@ -386,7 +386,7 @@ bool Initialize(bool force) { return NT_SUCCESS(ret) && page_executable; } -bool AddDllsFromRegistryToBlacklist() { +void AddDllsFromRegistryToBlacklist() { HKEY key = NULL; LONG result = ::RegOpenKeyEx(HKEY_CURRENT_USER, kRegistryFinchListPath, @@ -395,9 +395,9 @@ bool AddDllsFromRegistryToBlacklist() { &key); if (result != ERROR_SUCCESS) - return false; + return; - // We add dlls from the registry to the blacklist, and then clear registry. + // We add dlls from the registry to the blacklist. DWORD value_len; DWORD name_len = MAX_PATH; std::vector<wchar_t> name_buffer(name_len); @@ -406,24 +406,23 @@ bool AddDllsFromRegistryToBlacklist() { value_len = 0; result = ::RegEnumValue( key, i, &name_buffer[0], &name_len, NULL, NULL, NULL, &value_len); + if (result != ERROR_SUCCESS) + break; + name_len = name_len + 1; value_len = value_len + 1; std::vector<wchar_t> value_buffer(value_len); result = ::RegEnumValue(key, i, &name_buffer[0], &name_len, NULL, NULL, reinterpret_cast<BYTE*>(&value_buffer[0]), &value_len); + if (result != ERROR_SUCCESS) + break; value_buffer[value_len - 1] = L'\0'; - - if (result == ERROR_SUCCESS) { - AddDllToBlacklist(&value_buffer[0]); - } + AddDllToBlacklist(&value_buffer[0]); } - // Delete the finch registry key to clear the values. - result = ::RegDeleteKey(key, L""); - ::RegCloseKey(key); - return result == ERROR_SUCCESS; + return; } } // namespace blacklist diff --git a/chrome_elf/blacklist/blacklist.h b/chrome_elf/blacklist/blacklist.h index 58cc16b..9ea680c 100644 --- a/chrome_elf/blacklist/blacklist.h +++ b/chrome_elf/blacklist/blacklist.h @@ -62,7 +62,7 @@ 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" bool AddDllsFromRegistryToBlacklist(); +extern "C" void AddDllsFromRegistryToBlacklist(); // Record that the dll at the given index was blocked. 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 2e3f9a4..4f9859d 100644 --- a/chrome_elf/blacklist/test/blacklist_test.cc +++ b/chrome_elf/blacklist/test/blacklist_test.cc @@ -32,7 +32,7 @@ extern "C" { // When modifying the blacklist in the test process, use the exported test dll // functions on the test blacklist dll, not the ones linked into the test // executable itself. -__declspec(dllimport) bool TestDll_AddDllsFromRegistryToBlacklist(); +__declspec(dllimport) void TestDll_AddDllsFromRegistryToBlacklist(); __declspec(dllimport) bool TestDll_AddDllToBlacklist(const wchar_t* dll_name); __declspec(dllimport) bool TestDll_IsBlacklistInitialized(); __declspec(dllimport) bool TestDll_RemoveDllFromBlacklist( @@ -240,7 +240,7 @@ TEST_F(BlacklistTest, AddDllsFromRegistryToBlacklist) { test_data[i].dll_name); } - EXPECT_TRUE(TestDll_AddDllsFromRegistryToBlacklist()); + TestDll_AddDllsFromRegistryToBlacklist(); CheckBlacklistedDllsNotLoaded(); } |