Consider the allowlist as unavailable if its size is too small.

Currently there are 10k entries in the high confidence allowlist. It's
risky if the allowlist is misconfigured and somehow only a small number
of entries are left in the allowlist. In this CL, we check the size
of the allowlist. If there is less than 100 entries in the allowlist,
we consider it as unavailable and fall back to hash-based check.

Bug: 1164555
Change-Id: Ie48bf6197c2255830712b956a886f5deec78fdda
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2618654
Commit-Queue: Xinghui Lu <xinghuilu@chromium.org>
Reviewed-by: Varun Khaneja <vakh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#842195}

components/safe_browsing/core/db/v4_database.cc

@@ -297,6 +297,15 @@ bool V4Database::IsStoreAvailable(const ListIdentifier& identifier) const {
          store_pair->second->HasValidData();
 }
 
+int64_t V4Database::GetStoreSizeInBytes(
+    const ListIdentifier& identifier) const {
+  const auto& store_pair = store_map_->find(identifier);
+  if (store_pair == store_map_->end()) {
+    return 0;
+  }
+  return store_pair->second->file_size();
+}
+
 void V4Database::RecordFileSizeHistograms() {
   int64_t db_size = 0;
   for (const auto& store_map_iter : *store_map_) {

components/safe_browsing/core/db/v4_database.h

@@ -147,6 +147,10 @@ class V4Database {
       const StoresToCheck& stores_to_check,
       StoreAndHashPrefixes* matched_store_and_full_hashes);
 
+  // Returns the file size of the store in bytes. Returns 0 if the store is not
+  // found.
+  virtual int64_t GetStoreSizeInBytes(const ListIdentifier& store) const;
+
   // Resets the stores in |stores_to_reset| to an empty state. This is done if
   // the checksum doesn't match the expected value.
   void ResetStores(const std::vector<ListIdentifier>& stores_to_reset);

components/safe_browsing/core/db/v4_local_database_manager.cc

@@ -35,6 +35,13 @@ using CommandLineSwitchAndThreatType = std::pair<std::string, ThreatType>;
 // The expiration time of the full hash stored in the artificial database.
 const int64_t kFullHashExpiryTimeInMinutes = 60;
 
+// The number of bytes in a full hash entry.
+const int64_t kBytesPerFullHashEntry = 32;
+
+// The minimum number of entries in the allowlist. If the actual size is
+// smaller than this number, the allowlist is considered as unavailable.
+const int kHighConfidenceAllowlistMinimumEntryCount = 100;
+
 const ThreatSeverity kLeastSeverity =
     std::numeric_limits<ThreatSeverity>::max();
 
@@ -418,14 +425,19 @@ AsyncMatch V4LocalDatabaseManager::CheckUrlForHighConfidenceAllowlist(
   bool all_stores_available = AreAllStoresAvailableNow(stores_to_check);
   UMA_HISTOGRAM_BOOLEAN("SafeBrowsing.RT.AllStoresAvailable",
                         all_stores_available);
-  if (!enabled_ || !CanCheckUrl(url) ||
-      (!all_stores_available &&
-       artificially_marked_store_and_hash_prefixes_.empty())) {
+  bool is_artificial_prefix_empty =
+      artificially_marked_store_and_hash_prefixes_.empty();
+  bool is_allowlist_too_small =
+      IsStoreTooSmall(GetUrlHighConfidenceAllowlistId(), kBytesPerFullHashEntry,
+                      kHighConfidenceAllowlistMinimumEntryCount);
+  if (!enabled_ || (is_allowlist_too_small && is_artificial_prefix_empty) ||
+      !CanCheckUrl(url) ||
+      (!all_stores_available && is_artificial_prefix_empty)) {
     // NOTE(vakh): If Safe Browsing isn't enabled yet, or if the URL isn't a
-    // navigation URL, or if the allowlist isn't ready yet, return MATCH.
-    // The full URL check won't be performed, but hash-based check will still
-    // be done. If any artificial matches are present, consider the allowlist
-    // as ready.
+    // navigation URL, or if the allowlist isn't ready yet, or if the allowlist
+    // is too small, return MATCH. The full URL check won't be performed, but
+    // hash-based check will still be done. If any artificial matches are
+    // present, consider the allowlist as ready.
     return AsyncMatch::MATCH;
   }
 
@@ -1030,6 +1042,20 @@ bool V4LocalDatabaseManager::AreAllStoresAvailableNow(
   return (result == StoreAvailabilityResult::AVAILABLE);
 }
 
+int64_t V4LocalDatabaseManager::GetStoreEntryCount(const ListIdentifier& store,
+                                                   int bytes_per_entry) const {
+  if (!enabled_ || !v4_database_) {
+    return 0;
+  }
+  return v4_database_->GetStoreSizeInBytes(store) / bytes_per_entry;
+}
+
+bool V4LocalDatabaseManager::IsStoreTooSmall(const ListIdentifier& store,
+                                             int bytes_per_entry,
+                                             int min_entry_count) const {
+  return GetStoreEntryCount(store, bytes_per_entry) < min_entry_count;
+}
+
 bool V4LocalDatabaseManager::AreAnyStoresAvailableNow(
     const StoresToCheck& stores_to_check) const {
   return enabled_ && v4_database_ &&

components/safe_browsing/core/db/v4_local_database_manager.h

@@ -329,6 +329,16 @@ class V4LocalDatabaseManager : public SafeBrowsingDatabaseManager {
   // these stores.
   bool AreAllStoresAvailableNow(const StoresToCheck& stores_to_check) const;
 
+  // Return the number of entries in the store. If the database isn't enabled or
+  // the database is not found, return 0.
+  int64_t GetStoreEntryCount(const ListIdentifier& store,
+                             int bytes_per_entry) const;
+
+  // Return whether the size of the store is smaller than expected.
+  bool IsStoreTooSmall(const ListIdentifier& store,
+                       int bytes_per_entry,
+                       int min_entry_count) const;
+
   // Return true if we're enabled and have loaded real data for any of
   // these stores.
   bool AreAnyStoresAvailableNow(const StoresToCheck& stores_to_check) const;

components/safe_browsing/core/db/v4_local_database_manager_unittest.cc

@@ -44,6 +44,8 @@ FullHash HashForUrl(const GURL& url) {
   return full_hashes[0];
 }
 
+const int kDefaultStoreFileSizeInBytes = 320000;
+
 // Use this if you want GetFullHashes() to always return prescribed results.
 class FakeGetHashProtocolManager : public V4GetHashProtocolManager {
  public:
@@ -108,7 +110,8 @@ class FakeV4Database : public V4Database {
       std::unique_ptr<StoreMap> store_map,
       const StoreAndHashPrefixes& store_and_hash_prefixes,
       NewDatabaseReadyCallback new_db_callback,
-      bool stores_available) {
+      bool stores_available,
+      int64_t store_file_size) {
     // Mimics V4Database::Create
     const scoped_refptr<base::SingleThreadTaskRunner>& callback_task_runner =
         base::ThreadTaskRunnerHandle::Get();
@@ -117,7 +120,7 @@ class FakeV4Database : public V4Database {
         base::BindOnce(&FakeV4Database::CreateOnTaskRunner, db_task_runner,
                        std::move(store_map), store_and_hash_prefixes,
                        callback_task_runner, std::move(new_db_callback),
-                       stores_available));
+                       stores_available, store_file_size));
   }
 
   // V4Database implementation
@@ -136,6 +139,11 @@ class FakeV4Database : public V4Database {
     }
   }
 
+  // V4Database implementation
+  int64_t GetStoreSizeInBytes(const ListIdentifier& store) const override {
+    return store_file_size_;
+  }
+
   bool AreAllStoresAvailable(
       const StoresToCheck& stores_to_check) const override {
     return stores_available_;
@@ -153,11 +161,12 @@ class FakeV4Database : public V4Database {
       const StoreAndHashPrefixes& store_and_hash_prefixes,
       const scoped_refptr<base::SingleThreadTaskRunner>& callback_task_runner,
       NewDatabaseReadyCallback new_db_callback,
-      bool stores_available) {
+      bool stores_available,
+      int64_t store_file_size) {
     // Mimics the semantics of V4Database::CreateOnTaskRunner
-    std::unique_ptr<FakeV4Database> fake_v4_database(
-        new FakeV4Database(db_task_runner, std::move(store_map),
-                           store_and_hash_prefixes, stores_available));
+    std::unique_ptr<FakeV4Database> fake_v4_database(new FakeV4Database(
+        db_task_runner, std::move(store_map), store_and_hash_prefixes,
+        stores_available, store_file_size));
     callback_task_runner->PostTask(FROM_HERE,
                                    base::BindOnce(std::move(new_db_callback),
                                                   std::move(fake_v4_database)));
@@ -166,13 +175,16 @@ class FakeV4Database : public V4Database {
   FakeV4Database(const scoped_refptr<base::SequencedTaskRunner>& db_task_runner,
                  std::unique_ptr<StoreMap> store_map,
                  const StoreAndHashPrefixes& store_and_hash_prefixes,
-                 bool stores_available)
+                 bool stores_available,
+                 int64_t store_file_size)
       : V4Database(db_task_runner, std::move(store_map)),
         store_and_hash_prefixes_(store_and_hash_prefixes),
-        stores_available_(stores_available) {}
+        stores_available_(stores_available),
+        store_file_size_(store_file_size) {}
 
   const StoreAndHashPrefixes store_and_hash_prefixes_;
   const bool stores_available_;
+  int64_t store_file_size_;
 };
 
 // TODO(nparker): This might be simpler with a mock and EXPECT calls.
@@ -371,8 +383,10 @@ class V4LocalDatabaseManagerTest : public PlatformTest {
     v4_local_database_manager_->PopulateArtificialDatabase();
   }
 
-  void ReplaceV4Database(const StoreAndHashPrefixes& store_and_hash_prefixes,
-                         bool stores_available = false) {
+  void ReplaceV4Database(
+      const StoreAndHashPrefixes& store_and_hash_prefixes,
+      bool stores_available = false,
+      int64_t store_file_size = kDefaultStoreFileSizeInBytes) {
     // Disable the V4LocalDatabaseManager first so that if the callback to
     // verify checksum has been scheduled, then it doesn't do anything when it
     // is called back.
@@ -387,9 +401,9 @@ class V4LocalDatabaseManagerTest : public PlatformTest {
     NewDatabaseReadyCallback db_ready_callback =
         base::BindOnce(&V4LocalDatabaseManager::DatabaseReadyForChecks,
                        base::Unretained(v4_local_database_manager_.get()));
-    FakeV4Database::Create(task_runner_, std::make_unique<StoreMap>(),
-                           store_and_hash_prefixes,
-                           std::move(db_ready_callback), stores_available);
+    FakeV4Database::Create(
+        task_runner_, std::make_unique<StoreMap>(), store_and_hash_prefixes,
+        std::move(db_ready_callback), stores_available, store_file_size);
     WaitForTasksOnTaskRunner();
   }
 
@@ -664,7 +678,8 @@ TEST_F(V4LocalDatabaseManagerTest,
   StoreAndHashPrefixes store_and_hash_prefixes;
   store_and_hash_prefixes.emplace_back(GetUrlHighConfidenceAllowlistId(),
                                        safe_hash_prefix);
-  ReplaceV4Database(store_and_hash_prefixes, /* stores_available= */ true);
+  ReplaceV4Database(store_and_hash_prefixes, /* stores_available= */ true,
+                    /* store_file_size= */ 10000);
 
   // Setup the allowlist client to verify the callback.
   TestAllowlistClient client(
@@ -706,7 +721,8 @@ TEST_F(V4LocalDatabaseManagerTest,
   StoreAndHashPrefixes store_and_hash_prefixes;
   store_and_hash_prefixes.emplace_back(GetUrlHighConfidenceAllowlistId(),
                                        safe_hash_prefix);
-  ReplaceV4Database(store_and_hash_prefixes, /* stores_available= */ true);
+  ReplaceV4Database(store_and_hash_prefixes, /* stores_available= */ true,
+                    /* store_file_size= */ 100000);
 
   // Setup the allowlist client to verify the callback.
   TestAllowlistClient client(
@@ -745,7 +761,8 @@ TEST_F(V4LocalDatabaseManagerTest,
   StoreAndHashPrefixes store_and_hash_prefixes;
   store_and_hash_prefixes.emplace_back(GetUrlHighConfidenceAllowlistId(),
                                        safe_full_hash);
-  ReplaceV4Database(store_and_hash_prefixes, /* stores_available= */ true);
+  ReplaceV4Database(store_and_hash_prefixes, /* stores_available= */ true,
+                    /* store_file_size= */ 100000);
 
   // Setup the allowlist client to verify the callback isn't called.
   TestAllowlistClient client(
@@ -777,7 +794,8 @@ TEST_F(V4LocalDatabaseManagerTest, TestCheckUrlForHCAllowlistWithNoMatch) {
   // Add a full hash that won't match the URL we check.
   StoreAndHashPrefixes store_and_hash_prefixes;
   store_and_hash_prefixes.emplace_back(GetUrlMalwareId(), safe_full_hash);
-  ReplaceV4Database(store_and_hash_prefixes, /* stores_available= */ true);
+  ReplaceV4Database(store_and_hash_prefixes, /* stores_available= */ true,
+                    /* store_file_size= */ 100000);
 
   // Setup the allowlist client to verify the callback isn't called.
   TestAllowlistClient client(
@@ -804,7 +822,39 @@ TEST_F(V4LocalDatabaseManagerTest, TestCheckUrlForHCAllowlistUnavailable) {
 
   // Setup local database as unavailable.
   StoreAndHashPrefixes store_and_hash_prefixes;
-  ReplaceV4Database(store_and_hash_prefixes, /* stores_available= */ false);
+  ReplaceV4Database(store_and_hash_prefixes, /* stores_available= */ false,
+                    /* store_file_size= */ 100000);
+
+  // Setup the allowlist client to verify the callback isn't called.
+  TestAllowlistClient client(
+      /* match_expected= */ false,
+      /* expected_sb_threat_type= */ SB_THREAT_TYPE_HIGH_CONFIDENCE_ALLOWLIST);
+
+  const GURL url_check("https://example.com/safe");
+  EXPECT_EQ(AsyncMatch::MATCH,
+            v4_local_database_manager_->CheckUrlForHighConfidenceAllowlist(
+                url_check, &client));
+
+  WaitForTasksOnTaskRunner();
+  EXPECT_FALSE(client.callback_called());
+}
+
+// When allowlist is available but the size is too small, all URLS should be
+// considered MATCH.
+TEST_F(V4LocalDatabaseManagerTest, TestCheckUrlForHCAllowlistSmallSize) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitWithFeatures({safe_browsing::kRealTimeUrlLookupEnabled}, {});
+
+  // Setup to receive full-hash misses. We won't make URL requests.
+  ScopedFakeGetHashProtocolManagerFactory pin(FullHashInfos({}));
+  ResetLocalDatabaseManager();
+  WaitForTasksOnTaskRunner();
+
+  // Setup the size of the allowlist to be smaller than the threshold. (10
+  // entries)
+  StoreAndHashPrefixes store_and_hash_prefixes;
+  ReplaceV4Database(store_and_hash_prefixes, /* stores_available= */ true,
+                    /* store_file_size= */ 32 * 10);
 
   // Setup the allowlist client to verify the callback isn't called.
   TestAllowlistClient client(

components/safe_browsing/core/db/v4_store.h

@@ -193,6 +193,8 @@ class V4Store {
 
   const base::FilePath& store_path() const { return store_path_; }
 
+  int64_t file_size() const { return file_size_; }
+
   void ApplyUpdate(std::unique_ptr<ListUpdateResponse> response,
                    const scoped_refptr<base::SingleThreadTaskRunner>& runner,
                    UpdatedStoreReadyCallback callback);

components/safe_browsing/core/db/v4_test_util.cc

@@ -20,6 +20,7 @@ namespace {
 const char kClient[] = "unittest";
 const char kAppVer[] = "1.0";
 const char kKeyParam[] = "test_key_param";
+const int kDefaultStoreFileSizeInBytes = 320000;
 
 }  // namespace
 
@@ -74,6 +75,10 @@ void TestV4Database::MarkPrefixAsBad(ListIdentifier list_id,
   test_store->MarkPrefixAsBad(prefix);
 }
 
+int64_t TestV4Database::GetStoreSizeInBytes(const ListIdentifier& store) const {
+  return kDefaultStoreFileSizeInBytes;
+}
+
 TestV4StoreFactory::TestV4StoreFactory() = default;
 
 TestV4StoreFactory::~TestV4StoreFactory() = default;

components/safe_browsing/core/db/v4_test_util.h

@@ -59,6 +59,9 @@ class TestV4Database : public V4Database {
                  std::unique_ptr<StoreMap> store_map);
 
   void MarkPrefixAsBad(ListIdentifier list_id, HashPrefix prefix);
+
+  // V4Database implementation
+  int64_t GetStoreSizeInBytes(const ListIdentifier& store) const override;
 };
 
 class TestV4DatabaseFactory : public V4DatabaseFactory {