Add a killswitch for CSD malware IP match and report feature. Use a new killswitch whitelist URL wh…

chrome/browser/safe_browsing/safe_browsing_database.cc

Index: chrome/browser/safe_browsing/safe_browsing_database.cc
diff --git a/chrome/browser/safe_browsing/safe_browsing_database.cc b/chrome/browser/safe_browsing/safe_browsing_database.cc
index c969b66ce3a63dea81a5122916b6f311517895a4..fa33858f985845963e4c3d01041f6438d72651bb 100644
--- a/chrome/browser/safe_browsing/safe_browsing_database.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_database.cc
@@ -67,6 +67,12 @@ const size_t kMaxWhitelistSize = 5000;
 const char kWhitelistKillSwitchUrl[] =
     "sb-ssl.google.com/safebrowsing/csd/killswitch";  // Don't change this!
 
+// If the hash of this exact expression is on a whitelist then the
+// malware IP blacklisting feature will be disabled in csd.
+// Don't change this!
+const char kMalwareIPKillSwitchUrl[] =
+    "sb-ssl.google.com/safebrowsing/csd/killswitch_malware";
+
 // To save space, the incoming |chunk_id| and |list_id| are combined
 // into an |encoded_chunk_id| for storage by shifting the |list_id|
 // into the low-order bits.  These functions decode that information.
@@ -427,6 +433,7 @@ SafeBrowsingDatabaseNew::SafeBrowsingDatabaseNew()
       download_store_(NULL),
       csd_whitelist_store_(NULL),
       download_whitelist_store_(NULL),
+      csd_malware_ipmatch_killswitch_on_(false),
       reset_factory_(this),
       corruption_detected_(false),
       change_detected_(false) {
@@ -505,7 +512,7 @@ void SafeBrowsingDatabaseNew::Init(const base::FilePath& filename_base) {
     DVLOG(1) << "Init csd whitelist store: " << csd_whitelist_filename_.value();
     std::vector<SBAddFullHash> full_hashes;
     if (csd_whitelist_store_->GetAddFullHashes(&full_hashes)) {
-      LoadWhitelist(full_hashes, &csd_whitelist_);
+      LoadWhitelist(full_hashes, &csd_whitelist_, true);
     } else {
       WhitelistEverything(&csd_whitelist_);
     }
@@ -523,7 +530,7 @@ void SafeBrowsingDatabaseNew::Init(const base::FilePath& filename_base) {
              << download_whitelist_filename_.value();
     std::vector<SBAddFullHash> full_hashes;
     if (download_whitelist_store_->GetAddFullHashes(&full_hashes)) {
-      LoadWhitelist(full_hashes, &download_whitelist_);
+      LoadWhitelist(full_hashes, &download_whitelist_, false);
     } else {
       WhitelistEverything(&download_whitelist_);
     }
@@ -1101,10 +1108,10 @@ void SafeBrowsingDatabaseNew::UpdateFinished(bool update_succeeded) {
   UpdateBrowseStore();
   UpdateWhitelistStore(csd_whitelist_filename_,
                        csd_whitelist_store_.get(),
-                       &csd_whitelist_);
+                       &csd_whitelist_, true);
   UpdateWhitelistStore(download_whitelist_filename_,
                        download_whitelist_store_.get(),
-                       &download_whitelist_);
+                       &download_whitelist_, false);
 
   if (extension_blacklist_store_) {
     int64 size_bytes = UpdateHashPrefixStore(
@@ -1119,7 +1126,8 @@ void SafeBrowsingDatabaseNew::UpdateFinished(bool update_succeeded) {
 void SafeBrowsingDatabaseNew::UpdateWhitelistStore(
     const base::FilePath& store_filename,
     SafeBrowsingStore* store,
-    SBWhitelist* whitelist) {
+    SBWhitelist* whitelist,
+    bool check_malware_killswitch) {
   if (!store)
     return;
 
@@ -1145,7 +1153,7 @@ void SafeBrowsingDatabaseNew::UpdateWhitelistStore(
   base::mac::SetFileBackupExclusion(store_filename);
 #endif
 
-  LoadWhitelist(full_hashes, whitelist);
+  LoadWhitelist(full_hashes, whitelist, check_malware_killswitch);
 }
 
 int64 SafeBrowsingDatabaseNew::UpdateHashPrefixStore(
@@ -1397,7 +1405,8 @@ void SafeBrowsingDatabaseNew::WhitelistEverything(SBWhitelist* whitelist) {
 
 void SafeBrowsingDatabaseNew::LoadWhitelist(
     const std::vector<SBAddFullHash>& full_hashes,
-    SBWhitelist* whitelist) {
+    SBWhitelist* whitelist,
+    bool check_malware_killswitch) {
   DCHECK_EQ(creation_loop_, MessageLoop::current());
   if (full_hashes.size() > kMaxWhitelistSize) {
     WhitelistEverything(whitelist);
@@ -1424,4 +1433,29 @@ void SafeBrowsingDatabaseNew::LoadWhitelist(
     whitelist->second = false;
     whitelist->first.swap(new_whitelist);
   }
+
+  // The killswitch is only in csd_whitelist, not download_whitelist
+  if (!check_malware_killswitch) {

[mattm, 2013/05/21 02:14:09]

I tihnk it would be clearer to move the check into the calling function rather
than having this arg for this function to special case it.

Actually I wonder if it would be acceptable to just make
MalwareIPMatchKillSwitchOn itself do a search (just call
ContainsWhitelistedHashes?) rather than having the result stored as a bool
member of the class?

[kewang, 2013/05/22 10:14:00]

I think this is a good idea. changed to call ContainsWhitelistedHashes.

+    return;
+  }
+
+  SBFullHash malware_kill_switch;
+  crypto::SHA256HashString(kMalwareIPKillSwitchUrl, &malware_kill_switch,
+                           sizeof(malware_kill_switch));
+  if (std::binary_search(new_whitelist.begin(), new_whitelist.end(),

[mattm, 2013/05/21 02:14:09]

I think this is a bug, new_whitelist here will actually be the old whitelist
unless the new whitelist also containts the CSD kill switch url ( due to
whitelist->first.swap(new_whitelist) above.)

I guses the unittest should also test the malware kill switch independently of
the csd whitelist killswitch.

[kewang, 2013/05/22 10:14:00]

removed this part totally.

+                         malware_kill_switch)) {
+    // Turn on the malware IP matching kill switch
+    base::AutoLock locked(lookup_lock_);
+    csd_malware_ipmatch_killswitch_on_ = true;
+  } else {
+    // Turn off the malware IP matching kill switch
+    base::AutoLock locked(lookup_lock_);
+    csd_malware_ipmatch_killswitch_on_ = false;
+  }
 }
+
+
+bool SafeBrowsingDatabaseNew::MalwareIPMatchKillSwitchOn() {
+  base::AutoLock locked(lookup_lock_);
+  return csd_malware_ipmatch_killswitch_on_;
+};