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

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, &params);
 

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

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_

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);
+}

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;
   }

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

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);

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();
 }