PVer4: Filter out the safe crxs before calling OnCheckExtensionsResult

The argument for OnCheckExtensionsResult should only contain the bad
extensions, but PVer4 code was including all CRXs (blacklisted or not).

BUG=700145

Review-Url: https://codereview.chromium.org/2742913002
Cr-Commit-Position: refs/heads/master@{#456212}

components/safe_browsing_db/v4_local_database_manager.cc

@@ -111,12 +111,14 @@ V4LocalDatabaseManager::PendingCheck::PendingCheck(
     const std::vector<GURL>& urls)
     : client(client),
       client_callback_type(client_callback_type),
-      result_threat_type(SB_THREAT_TYPE_SAFE),
+      most_severe_threat_type(SB_THREAT_TYPE_SAFE),
       stores_to_check(stores_to_check),
       urls(urls) {
   for (const auto& url : urls) {
     V4ProtocolManagerUtil::UrlToFullHashes(url, &full_hashes);
   }
+  DCHECK(full_hashes.size());
+  full_hash_threat_types.assign(full_hashes.size(), SB_THREAT_TYPE_SAFE);
 }
 
 V4LocalDatabaseManager::PendingCheck::PendingCheck(
@@ -126,9 +128,11 @@ V4LocalDatabaseManager::PendingCheck::PendingCheck(
     const std::set<FullHash>& full_hashes_set)
     : client(client),
       client_callback_type(client_callback_type),
-      result_threat_type(SB_THREAT_TYPE_SAFE),
+      most_severe_threat_type(SB_THREAT_TYPE_SAFE),
       stores_to_check(stores_to_check) {
   full_hashes.assign(full_hashes_set.begin(), full_hashes_set.end());
+  DCHECK(full_hashes.size());
+  full_hash_threat_types.assign(full_hashes.size(), SB_THREAT_TYPE_SAFE);
 }
 
 V4LocalDatabaseManager::PendingCheck::~PendingCheck() {}
@@ -503,20 +507,25 @@ bool V4LocalDatabaseManager::GetPrefixMatches(
 }
 
 void V4LocalDatabaseManager::GetSeverestThreatTypeAndMetadata(
-    SBThreatType* result_threat_type,
+    const std::vector<FullHashInfo>& full_hash_infos,
+    const std::vector<FullHash>& full_hashes,
+    std::vector<SBThreatType>* full_hash_threat_types,
+    SBThreatType* most_severe_threat_type,
     ThreatMetadata* metadata,
-    FullHash* matching_full_hash,
-    const std::vector<FullHashInfo>& full_hash_infos) {
-  DCHECK(result_threat_type);
-  DCHECK(metadata);
-  DCHECK(matching_full_hash);
-
+    FullHash* matching_full_hash) {
   ThreatSeverity most_severe_yet = kLeastSeverity;
   for (const FullHashInfo& fhi : full_hash_infos) {
     ThreatSeverity severity = GetThreatSeverity(fhi.list_id);
+    SBThreatType threat_type = GetSBThreatTypeForList(fhi.list_id);
+
+    const auto& it =
+        std::find(full_hashes.begin(), full_hashes.end(), fhi.full_hash);
+    DCHECK(it != full_hashes.end());
+    (*full_hash_threat_types)[it - full_hashes.begin()] = threat_type;
+
     if (severity < most_severe_yet) {
       most_severe_yet = severity;
-      *result_threat_type = GetSBThreatTypeForList(fhi.list_id);
+      *most_severe_threat_type = threat_type;
       *metadata = fhi.metadata;
       *matching_full_hash = fhi.full_hash;
     }
@@ -612,9 +621,10 @@ void V4LocalDatabaseManager::OnFullHashResponse(
   }
 
   // Find out the most severe threat, if any, to report to the client.
-  GetSeverestThreatTypeAndMetadata(&check->result_threat_type,
-                                   &check->url_metadata,
-                                   &check->matching_full_hash, full_hash_infos);
+  GetSeverestThreatTypeAndMetadata(
+      full_hash_infos, check->full_hashes, &check->full_hash_threat_types,
+      &check->most_severe_threat_type, &check->url_metadata,
+      &check->matching_full_hash);
   pending_checks_.erase(it);
   RespondToClient(std::move(check));
 }
@@ -673,24 +683,31 @@ void V4LocalDatabaseManager::RespondToClient(
     case ClientCallbackType::CHECK_URL_FOR_SUBRESOURCE_FILTER:
       DCHECK_EQ(1u, check->urls.size());
       check->client->OnCheckBrowseUrlResult(
-          check->urls[0], check->result_threat_type, check->url_metadata);
+          check->urls[0], check->most_severe_threat_type, check->url_metadata);
       break;
 
     case ClientCallbackType::CHECK_DOWNLOAD_URLS:
       check->client->OnCheckDownloadUrlResult(check->urls,
-                                              check->result_threat_type);
+                                              check->most_severe_threat_type);
       break;
 
     case ClientCallbackType::CHECK_RESOURCE_URL:
       DCHECK_EQ(1u, check->urls.size());
-      check->client->OnCheckResourceUrlResult(
-          check->urls[0], check->result_threat_type, check->matching_full_hash);
+      check->client->OnCheckResourceUrlResult(check->urls[0],
+                                              check->most_severe_threat_type,
+                                              check->matching_full_hash);
       break;
 
     case ClientCallbackType::CHECK_EXTENSION_IDS: {
-      const std::set<FullHash> extension_ids(check->full_hashes.begin(),
-                                             check->full_hashes.end());
-      check->client->OnCheckExtensionsResult(extension_ids);
+      DCHECK_EQ(check->full_hash_threat_types.size(),
+                check->full_hashes.size());
+      std::set<FullHash> unsafe_extension_ids;
+      for (size_t i = 0; i < check->full_hash_threat_types.size(); i++) {
+        if (check->full_hash_threat_types[i] == SB_THREAT_TYPE_EXTENSION) {
+          unsafe_extension_ids.insert(check->full_hashes[i]);
+        }
+      }
+      check->client->OnCheckExtensionsResult(unsafe_extension_ids);
       break;
     }
     case ClientCallbackType::CHECK_OTHER:

components/safe_browsing_db/v4_local_database_manager.h

@@ -134,8 +134,8 @@ class V4LocalDatabaseManager : public SafeBrowsingDatabaseManager {
     // know whether the URL in |url| is safe or unsafe.
     const ClientCallbackType client_callback_type;
 
-    // The threat verdict for the URL being checked.
-    SBThreatType result_threat_type;
+    // The most severe threat verdict for the URLs/hashes being checked.
+    SBThreatType most_severe_threat_type;
 
     // When the check was sent to the SafeBrowsing service. Used to record the
     // time it takes to get the uncached full hashes from the service (or a
@@ -149,10 +149,13 @@ class V4LocalDatabaseManager : public SafeBrowsingDatabaseManager {
     // one of |full_hashes| and |urls| should be greater than 0.
     const std::vector<GURL> urls;
 
-    // The full hashes that are being checked for being safe. The size of
-    // exactly one of |full_hashes| and |urls| should be greater than 0.
+    // The full hashes that are being checked for being safe.
     std::vector<FullHash> full_hashes;
 
+    // The most severe SBThreatType for each full hash in |full_hashes|. The
+    // length of |full_hash_threat_type| must always match |full_hashes|.
+    std::vector<SBThreatType> full_hash_threat_types;
+
     // The metadata associated with the full hash of the severest match found
     // for that URL.
     ThreatMetadata url_metadata;
@@ -196,13 +199,18 @@ class V4LocalDatabaseManager : public SafeBrowsingDatabaseManager {
       const std::unique_ptr<PendingCheck>& check,
       FullHashToStoreAndHashPrefixesMap* full_hash_to_store_and_hash_prefixes);
 
-  // Finds the most severe |SBThreatType| and the corresponding |metadata|, and
-  // |matching_full_hash| from |full_hash_infos|.
+  // Goes over the |full_hash_infos| and stores the most severe SBThreatType in
+  // |most_severe_threat_type|, the corresponding metadata in |metadata|, and
+  // the matching full hash in |matching_full_hash|. Also, updates in
+  // |full_hash_threat_types|, the threat type for each full hash in
+  // |full_hashes|.
   void GetSeverestThreatTypeAndMetadata(
-      SBThreatType* result_threat_type,
+      const std::vector<FullHashInfo>& full_hash_infos,
+      const std::vector<FullHash>& full_hashes,
+      std::vector<SBThreatType>* full_hash_threat_types,
+      SBThreatType* most_severe_threat_type,
       ThreatMetadata* metadata,
-      FullHash* matching_full_hash,
-      const std::vector<FullHashInfo>& full_hash_infos);
+      FullHash* matching_full_hash);
 
   // Returns the SBThreatType for a given ListIdentifier.
   SBThreatType GetSBThreatTypeForList(const ListIdentifier& list_id);

components/safe_browsing_db/v4_local_database_manager_unittest.cc

@@ -191,6 +191,21 @@ class TestClient : public SafeBrowsingDatabaseManager::Client {
   V4LocalDatabaseManager* manager_to_cancel_;
 };
 
+class TestExtensionClient : public SafeBrowsingDatabaseManager::Client {
+ public:
+  TestExtensionClient(const std::set<FullHash>& expected_bad_crxs)
+      : expected_bad_crxs(expected_bad_crxs),
+        on_check_extensions_result_called_(false) {}
+
+  void OnCheckExtensionsResult(const std::set<FullHash>& bad_crxs) override {
+    EXPECT_EQ(expected_bad_crxs, bad_crxs);
+    on_check_extensions_result_called_ = true;
+  }
+
+  const std::set<FullHash> expected_bad_crxs;
+  bool on_check_extensions_result_called_;
+};
+
 class FakeV4LocalDatabaseManager : public V4LocalDatabaseManager {
  public:
   void PerformFullHashCheck(std::unique_ptr<PendingCheck> check,
@@ -404,32 +419,48 @@ TEST_F(V4LocalDatabaseManagerTest,
 TEST_F(V4LocalDatabaseManagerTest, TestGetSeverestThreatTypeAndMetadata) {
   WaitForTasksOnTaskRunner();
 
-  FullHash full_hash("Malware");
-  FullHashInfo fhi_malware(full_hash, GetUrlMalwareId(), base::Time::Now());
+  FullHash fh_malware("Malware");
+  FullHashInfo fhi_malware(fh_malware, GetUrlMalwareId(), base::Time::Now());
   fhi_malware.metadata.population_id = "malware_popid";
 
-  FullHashInfo fhi_api(FullHash("api"), GetChromeUrlApiId(), base::Time::Now());
+  FullHash fh_api("api");
+  FullHashInfo fhi_api(fh_api, GetChromeUrlApiId(), base::Time::Now());
   fhi_api.metadata.population_id = "api_popid";
 
+  FullHash fh_example("example");
   std::vector<FullHashInfo> fhis({fhi_malware, fhi_api});
+  std::vector<FullHash> full_hashes({fh_malware, fh_example, fh_api});
 
+  std::vector<SBThreatType> full_hash_threat_types(full_hashes.size(),
+                                                   SB_THREAT_TYPE_SAFE);
   SBThreatType result_threat_type;
   ThreatMetadata metadata;
   FullHash matching_full_hash;
 
+  const std::vector<SBThreatType> expected_full_hash_threat_types(
+      {SB_THREAT_TYPE_URL_MALWARE, SB_THREAT_TYPE_SAFE,
+       SB_THREAT_TYPE_API_ABUSE});
+
   v4_local_database_manager_->GetSeverestThreatTypeAndMetadata(
-      &result_threat_type, &metadata, &matching_full_hash, fhis);
+      fhis, full_hashes, &full_hash_threat_types, &result_threat_type,
+      &metadata, &matching_full_hash);
+  EXPECT_EQ(expected_full_hash_threat_types, full_hash_threat_types);
+
   EXPECT_EQ(SB_THREAT_TYPE_URL_MALWARE, result_threat_type);
   EXPECT_EQ("malware_popid", metadata.population_id);
-  EXPECT_EQ(full_hash, matching_full_hash);
+  EXPECT_EQ(fh_malware, matching_full_hash);
 
   // Reversing the list has no effect.
   std::reverse(std::begin(fhis), std::end(fhis));
+  full_hash_threat_types.assign(full_hashes.size(), SB_THREAT_TYPE_SAFE);
+
   v4_local_database_manager_->GetSeverestThreatTypeAndMetadata(
-      &result_threat_type, &metadata, &matching_full_hash, fhis);
+      fhis, full_hashes, &full_hash_threat_types, &result_threat_type,
+      &metadata, &matching_full_hash);
+  EXPECT_EQ(expected_full_hash_threat_types, full_hash_threat_types);
   EXPECT_EQ(SB_THREAT_TYPE_URL_MALWARE, result_threat_type);
   EXPECT_EQ("malware_popid", metadata.population_id);
-  EXPECT_EQ(full_hash, matching_full_hash);
+  EXPECT_EQ(fh_malware, matching_full_hash);
 }
 
 TEST_F(V4LocalDatabaseManagerTest, TestChecksAreQueued) {
@@ -801,8 +832,64 @@ TEST_F(V4LocalDatabaseManagerTest, TestCheckResourceUrlReturnsBad) {
   EXPECT_TRUE(client.on_check_resource_url_result_called_);
 }
 
-// TODO(nparker): Add tests for
+TEST_F(V4LocalDatabaseManagerTest, TestCheckExtensionIDsNothingBlacklisted) {
+  // Setup to receive full-hash misses.
+  ScopedFakeGetHashProtocolManagerFactory pin(FullHashInfos({}));
+
+  // Reset the database manager so it picks up the replacement protocol manager.
+  ResetLocalDatabaseManager();
+  WaitForTasksOnTaskRunner();
+
+  // bad_extension_id is in the local DB but the full hash won't match.
+  const FullHash bad_extension_id("aaaabbbbccccdddd"),
+      good_extension_id("ddddccccbbbbaaaa");
+
+  // Put a match in the db that will cause a protocol-manager request.
+  StoreAndHashPrefixes store_and_hash_prefixes;
+  store_and_hash_prefixes.emplace_back(GetChromeExtMalwareId(),
+                                       bad_extension_id);
+  ReplaceV4Database(store_and_hash_prefixes, true /* stores_available */);
+
+  const std::set<FullHash> expected_bad_crxs({});
+  const std::set<FullHash> extension_ids({good_extension_id, bad_extension_id});
+  TestExtensionClient client(expected_bad_crxs);
+  EXPECT_FALSE(
+      v4_local_database_manager_->CheckExtensionIDs(extension_ids, &client));
+  EXPECT_FALSE(client.on_check_extensions_result_called_);
+  WaitForTasksOnTaskRunner();
+  EXPECT_TRUE(client.on_check_extensions_result_called_);
+}
+
+TEST_F(V4LocalDatabaseManagerTest, TestCheckExtensionIDsOneIsBlacklisted) {
+  // bad_extension_id is in the local DB and the full hash will match.
+  const FullHash bad_extension_id("aaaabbbbccccdddd"),
+      good_extension_id("ddddccccbbbbaaaa");
+  FullHashInfo fhi(bad_extension_id, GetChromeExtMalwareId(), base::Time());
+
+  // Setup to receive full-hash hit.
+  ScopedFakeGetHashProtocolManagerFactory pin(FullHashInfos({fhi}));
+
+  // Reset the database manager so it picks up the replacement protocol manager.
+  ResetLocalDatabaseManager();
+  WaitForTasksOnTaskRunner();
+
+  // Put a match in the db that will cause a protocol-manager request.
+  StoreAndHashPrefixes store_and_hash_prefixes;
+  store_and_hash_prefixes.emplace_back(GetChromeExtMalwareId(),
+                                       bad_extension_id);
+  ReplaceV4Database(store_and_hash_prefixes, true /* stores_available */);
+
+  const std::set<FullHash> expected_bad_crxs({bad_extension_id});
+  const std::set<FullHash> extension_ids({good_extension_id, bad_extension_id});
+  TestExtensionClient client(expected_bad_crxs);
+  EXPECT_FALSE(
+      v4_local_database_manager_->CheckExtensionIDs(extension_ids, &client));
+  EXPECT_FALSE(client.on_check_extensions_result_called_);
+  WaitForTasksOnTaskRunner();
+  EXPECT_TRUE(client.on_check_extensions_result_called_);
+}
+
+// TODO(vakh): By 03/15/2017: Add tests for
 //   CheckDownloadUrl()
-//   CheckExtensionIDs()
 
 }  // namespace safe_browsing