Don't warn about modules that are added to the blacklist cache.

Bug: 846953
Change-Id: I33e710005af02f9e6a553cfc0ef18acd3eaa2d8f
Reviewed-on: https://chromium-review.googlesource.com/1128371
Reviewed-by: Chris Hamilton <chrisha@chromium.org>
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Commit-Queue: Patrick Monette <pmonette@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573838}

chrome/browser/conflicts/incompatible_applications_updater_win.cc

@@ -280,12 +280,14 @@ void IncompatibleApplicationsUpdater::OnNewModuleFound(
     const ModuleInfoData& module_data) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
-  // Only consider loaded modules that are not shell extensions or IMEs.
-  static constexpr uint32_t kModuleTypesBitmask =
-      ModuleInfoData::kTypeLoadedModule | ModuleInfoData::kTypeShellExtension |
-      ModuleInfoData::kTypeIme;
-  if ((module_data.module_types & kModuleTypesBitmask) !=
-      ModuleInfoData::kTypeLoadedModule) {
+  // Only consider loaded modules that are not shell extensions or IMEs, and
+  // that will not be blocked on the next browser launch.
+  static constexpr uint32_t kModulePropertiesBitmask =
+      ModuleInfoData::kPropertyLoadedModule |
+      ModuleInfoData::kPropertyShellExtension | ModuleInfoData::kPropertyIme |
+      ModuleInfoData::kPropertyAddedToBlacklist;
+  if ((module_data.module_properties & kModulePropertiesBitmask) !=
+      ModuleInfoData::kPropertyLoadedModule) {
     return;
   }
 

chrome/browser/conflicts/incompatible_applications_updater_win_unittest.cc

@@ -82,7 +82,7 @@ constexpr wchar_t kDllPath2[] = L"c:\\some\\shellextension.dll";
 // empty.
 ModuleInfoData CreateLoadedModuleInfoData() {
   ModuleInfoData module_data;
-  module_data.module_types |= ModuleInfoData::kTypeLoadedModule;
+  module_data.module_properties |= ModuleInfoData::kPropertyLoadedModule;
   module_data.inspection_result = std::make_unique<ModuleInspectionResult>();
   return module_data;
 }
@@ -330,9 +330,9 @@ TEST_F(IncompatibleApplicationsUpdaterTest, IgnoreRegisteredModules) {
 
   // Set the respective bit for registered modules.
   auto module_data1 = CreateLoadedModuleInfoData();
-  module_data1.module_types |= ModuleInfoData::kTypeShellExtension;
+  module_data1.module_properties |= ModuleInfoData::kPropertyShellExtension;
   auto module_data2 = CreateLoadedModuleInfoData();
-  module_data2.module_types |= ModuleInfoData::kTypeIme;
+  module_data2.module_properties |= ModuleInfoData::kPropertyIme;
 
   // Simulate the modules loading into the process.
   incompatible_applications_updater->OnNewModuleFound(
@@ -346,3 +346,25 @@ TEST_F(IncompatibleApplicationsUpdaterTest, IgnoreRegisteredModules) {
       IncompatibleApplicationsUpdater::GetCachedApplications();
   ASSERT_EQ(0u, application_names.size());
 }
+
+TEST_F(IncompatibleApplicationsUpdaterTest, IgnoreModulesAddedToTheBlacklist) {
+  AddIncompatibleApplication(dll1_, L"Blacklisted Application",
+                             Option::ADD_REGISTRY_ENTRY);
+
+  auto incompatible_applications_updater =
+      CreateIncompatibleApplicationsUpdater();
+
+  // Set the respective bit for the module.
+  auto module_data = CreateLoadedModuleInfoData();
+  module_data.module_properties |= ModuleInfoData::kPropertyAddedToBlacklist;
+
+  // Simulate the module loading into the process.
+  incompatible_applications_updater->OnNewModuleFound(
+      ModuleInfoKey(dll1_, 0, 0, 0), module_data);
+  incompatible_applications_updater->OnModuleDatabaseIdle();
+
+  EXPECT_FALSE(IncompatibleApplicationsUpdater::HasCachedApplications());
+  auto application_names =
+      IncompatibleApplicationsUpdater::GetCachedApplications();
+  ASSERT_EQ(0u, application_names.size());
+}

chrome/browser/conflicts/module_blacklist_cache_updater_win.cc

@@ -192,10 +192,10 @@ void ModuleBlacklistCacheUpdater::OnNewModuleFound(
 
   // Only consider loaded modules that are not IMEs. Shell extensions are still
   // blocked.
-  static constexpr uint32_t kModuleTypesBitmask =
-      ModuleInfoData::kTypeLoadedModule | ModuleInfoData::kTypeIme;
-  if ((module_data.module_types & kModuleTypesBitmask) !=
-      ModuleInfoData::kTypeLoadedModule) {
+  static constexpr uint32_t kModulePropertiesBitmask =
+      ModuleInfoData::kPropertyLoadedModule | ModuleInfoData::kPropertyIme;
+  if ((module_data.module_properties & kModulePropertiesBitmask) !=
+      ModuleInfoData::kPropertyLoadedModule) {
     return;
   }
 
@@ -252,6 +252,18 @@ void ModuleBlacklistCacheUpdater::OnNewModuleFound(
                       module_code_id.length(), module.code_id_hash);
 
   module.time_date_stamp = CalculateTimeDateStamp(base::Time::Now());
+
+  // Signal the module database that this module will be added to the cache.
+  // Note that observers that care about this information should register to
+  // the Module Database's observer interface after the ModuleBlacklistCache
+  // instance.
+  // The Module Database can be null during tests.
+  auto* module_database = ModuleDatabase::GetInstance();
+  if (module_database) {
+    module_database->OnModuleAddedToBlacklist(
+        module_key.module_path, module_key.module_size,
+        module_key.module_time_date_stamp);
+  }
 }
 
 void ModuleBlacklistCacheUpdater::OnModuleDatabaseIdle() {

chrome/browser/conflicts/module_blacklist_cache_updater_win_unittest.cc

@@ -48,7 +48,7 @@ constexpr base::FilePath::CharType kDllPath2[] =
 // empty.
 ModuleInfoData CreateLoadedModuleInfoData() {
   ModuleInfoData module_data;
-  module_data.module_types |= ModuleInfoData::kTypeLoadedModule;
+  module_data.module_properties |= ModuleInfoData::kPropertyLoadedModule;
   module_data.inspection_result = std::make_unique<ModuleInspectionResult>();
   return module_data;
 }
@@ -295,11 +295,11 @@ TEST_F(ModuleBlacklistCacheUpdaterTest, RegisteredModules) {
   // Set the respective bit for registered modules.
   ModuleInfoKey module_key1(dll1_, 123u, 456u, 0);
   ModuleInfoData module_data1 = CreateLoadedModuleInfoData();
-  module_data1.module_types |= ModuleInfoData::kTypeIme;
+  module_data1.module_properties |= ModuleInfoData::kPropertyIme;
 
   ModuleInfoKey module_key2(dll2_, 456u, 789u, 0);
   ModuleInfoData module_data2 = CreateLoadedModuleInfoData();
-  module_data2.module_types |= ModuleInfoData::kTypeShellExtension;
+  module_data2.module_properties |= ModuleInfoData::kPropertyShellExtension;
 
   // Simulate the modules loading into the process.
   module_blacklist_cache_updater->OnNewModuleFound(module_key1, module_data1);

chrome/browser/conflicts/module_database_win.cc

@@ -96,7 +96,8 @@ void ModuleDatabase::OnShellExtensionEnumerated(const base::FilePath& path,
 
   auto* module_info =
       FindOrCreateModuleInfo(path, size_of_image, time_date_stamp);
-  module_info->second.module_types |= ModuleInfoData::kTypeShellExtension;
+  module_info->second.module_properties |=
+      ModuleInfoData::kPropertyShellExtension;
 }
 
 void ModuleDatabase::OnShellExtensionEnumerationFinished() {
@@ -118,7 +119,7 @@ void ModuleDatabase::OnImeEnumerated(const base::FilePath& path,
 
   auto* module_info =
       FindOrCreateModuleInfo(path, size_of_image, time_date_stamp);
-  module_info->second.module_types |= ModuleInfoData::kTypeIme;
+  module_info->second.module_properties |= ModuleInfoData::kPropertyIme;
 }
 
 void ModuleDatabase::OnImeEnumerationFinished() {
@@ -152,12 +153,25 @@ void ModuleDatabase::OnModuleLoad(content::ProcessType process_type,
   auto* module_info =
       FindOrCreateModuleInfo(module_path, module_size, module_time_date_stamp);
 
-  module_info->second.module_types |= ModuleInfoData::kTypeLoadedModule;
+  module_info->second.module_properties |=
+      ModuleInfoData::kPropertyLoadedModule;
 
   // Update the list of process types that this module has been seen in.
   module_info->second.process_types |= ProcessTypeToBit(process_type);
 }
 
+void ModuleDatabase::OnModuleAddedToBlacklist(const base::FilePath& module_path,
+                                              uint32_t module_size,
+                                              uint32_t module_time_date_stamp) {
+  auto iter = modules_.find(
+      ModuleInfoKey(module_path, module_size, module_time_date_stamp, 0));
+
+  // Only known modules should be added to the blacklist.
+  DCHECK(iter != modules_.end());
+
+  iter->second.module_properties |= ModuleInfoData::kPropertyAddedToBlacklist;
+}
+
 void ModuleDatabase::AddObserver(ModuleDatabaseObserver* observer) {
   observer_list_.AddObserver(observer);
 

chrome/browser/conflicts/module_database_win.h

@@ -98,6 +98,12 @@ class ModuleDatabase : public ModuleDatabaseEventSource {
                     uint32_t module_time_date_stamp,
                     uintptr_t module_load_address);
 
+  // Marks the module as added to the module blacklist cache, which means it
+  // will be blocked on the next browser launch.
+  void OnModuleAddedToBlacklist(const base::FilePath& module_path,
+                                uint32_t module_size,
+                                uint32_t module_time_date_stamp);
+
   // TODO(chrisha): Module analysis code, and various accessors for use by
   // chrome://conflicts.
 

chrome/browser/conflicts/module_info_win.cc

@@ -82,7 +82,7 @@ ModuleInspectionResult::~ModuleInspectionResult() = default;
 
 // ModuleInfoData --------------------------------------------------------------
 
-ModuleInfoData::ModuleInfoData() : process_types(0), module_types(0) {}
+ModuleInfoData::ModuleInfoData() : process_types(0), module_properties(0) {}
 
 ModuleInfoData::~ModuleInfoData() = default;
 

chrome/browser/conflicts/module_info_win.h

@@ -78,15 +78,17 @@ struct ModuleInspectionResult {
 // Contains the inspection result of a module and additional information that is
 // useful to the ModuleDatabase.
 struct ModuleInfoData {
-  // The possible types of module we are dealing with. Used as bit set values.
-  enum ModuleType : uint32_t {
+  // Different properties that the module can have. Used as bit set values.
+  enum ModuleProperty : uint32_t {
     // These modules are or were loaded into one of chrome's process at some
     // point.
-    kTypeLoadedModule = 1 << 0,
+    kPropertyLoadedModule = 1 << 0,
     // These modules are registered as a shell extension.
-    kTypeShellExtension = 1 << 1,
+    kPropertyShellExtension = 1 << 1,
     // These modules are registered as an Input Method Editor.
-    kTypeIme = 1 << 2,
+    kPropertyIme = 1 << 2,
+    // The module was added to the module blacklist cache.
+    kPropertyAddedToBlacklist = 1 << 3,
   };
 
   ModuleInfoData();
@@ -100,8 +102,8 @@ struct ModuleInfoData {
   // "BitIndexToProcessType" for details.
   uint32_t process_types;
 
-  // Set that describes the type of the module.
-  uint32_t module_types;
+  // Set that describes the properties of the module.
+  uint32_t module_properties;
 
   // The inspection result obtained via InspectModule().
   std::unique_ptr<ModuleInspectionResult> inspection_result;

chrome/browser/conflicts/third_party_conflicts_manager_win.cc

@@ -281,13 +281,6 @@ void ThirdPartyConflictsManager::InitializeIfReady() {
     return;
   }
 
-  if (installed_applications_) {
-    incompatible_applications_updater_ =
-        std::make_unique<IncompatibleApplicationsUpdater>(
-            module_database_event_source_, *exe_certificate_info_,
-            module_list_filter_, *installed_applications_);
-  }
-
   if (base::FeatureList::IsEnabled(features::kThirdPartyModulesBlocking)) {
     // It is safe to use base::Unretained() since the callback will not be
     // invoked if the updater is freed.
@@ -300,6 +293,18 @@ void ThirdPartyConflictsManager::InitializeIfReady() {
                 base::Unretained(this)));
   }
 
+  // The |incompatible_applications_updater_| instance must be created last so
+  // that it is registered to the Module Database observer's API after the
+  // ModuleBlacklistCacheUpdater instance. This way, it knows about which
+  // modules were added to the module blacklist cache so that it's possible to
+  // not warn about them.
+  if (installed_applications_) {
+    incompatible_applications_updater_ =
+        std::make_unique<IncompatibleApplicationsUpdater>(
+            module_database_event_source_, *exe_certificate_info_,
+            module_list_filter_, *installed_applications_);
+  }
+
   SetTerminalState(State::kInitialized);
 }
 

chrome/browser/conflicts/third_party_conflicts_manager_win.h

@@ -173,15 +173,15 @@ class ThirdPartyConflictsManager
   // Retrieves the list of installed applications.
   std::unique_ptr<InstalledApplications> installed_applications_;
 
+  // Maintains the module blacklist cache. This member is only initialized when
+  // the ThirdPartyModuleBlocking feature is enabled.
+  std::unique_ptr<ModuleBlacklistCacheUpdater> module_blacklist_cache_updater_;
+
   // Maintains the cache of incompatible applications. This member is only
   // initialized when the IncompatibleApplicationsWarning feature is enabled.
   std::unique_ptr<IncompatibleApplicationsUpdater>
       incompatible_applications_updater_;
 
-  // Maintains the module blacklist cache. This member is only initialized when
-  // the ThirdPartyModuleBlocking feature is enabled.
-  std::unique_ptr<ModuleBlacklistCacheUpdater> module_blacklist_cache_updater_;
-
   // The final state of this instance.
   base::Optional<State> terminal_state_;
 

chrome/browser/conflicts/third_party_metrics_recorder_win.cc

@@ -48,7 +48,8 @@ void ThirdPartyMetricsRecorder::OnNewModuleFound(
     } else {
       // Count modules that are neither signed by Google nor Microsoft.
       // These are considered "third party" modules.
-      if (module_data.module_types & ModuleInfoData::kTypeLoadedModule) {
+      if (module_data.module_properties &
+          ModuleInfoData::kPropertyLoadedModule) {
         ++loaded_third_party_module_count_;
       } else {
         ++not_loaded_third_party_module_count_;

chrome/browser/ui/webui/conflicts_handler.cc

@@ -42,7 +42,7 @@ void ConflictsHandler::OnNewModuleFound(const ModuleInfoKey& module_key,
   data->SetInteger("status", kGoodStatus);
 
   base::string16 type_string;
-  if (module_data.module_types & ModuleInfoData::kTypeShellExtension)
+  if (module_data.module_properties & ModuleInfoData::kPropertyShellExtension)
     type_string = L"Shell extension";
   data->SetString("type_description", type_string);