Revert "Revert 203400 "Add a killswitch for CSD malware IP match and rep...""

This reverts commit 93c657cc3f043ec0699336217051ecc99284b1e7.
This CL fix the race condition on host_ variable in client_side_detection_host.cc

BUG=176647

Review URL: https://chromiumcodereview.appspot.com/19313002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@213755 0039d316-1c4b-4281-b951-d872f2087c98

8 files changed, 91 insertions, 9 deletions

diff --git a/chrome/browser/safe_browsing/client_side_detection_host.cc b/chrome/browser/safe_browsing/client_side_detection_host.cc
index 5c5a781..1f67d5c 100644
--- a/chrome/browser/safe_browsing/client_side_detection_host.cc
+++ b/chrome/browser/safe_browsing/client_side_detection_host.cc
@@ -177,18 +177,22 @@ class ClientSideDetectionHost::ShouldClassifyUrlRequest
       return;
     }
 
+    bool malware_killswitch_on = database_manager_->IsMalwareKillSwitchOn();
+
     BrowserThread::PostTask(
         BrowserThread::UI,
         FROM_HERE,
-        base::Bind(&ShouldClassifyUrlRequest::CheckCache, this));
+        base::Bind(&ShouldClassifyUrlRequest::CheckCache, this,
+                   malware_killswitch_on));
   }
 
-  void CheckCache() {
+  void CheckCache(bool malware_killswitch_on) {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
     if (canceled_) {
       return;
     }
 
+    host_->SetMalwareKillSwitch(malware_killswitch_on);
     // If result is cached, we don't want to run classification again
     bool is_phishing;
     if (csd_service_->GetValidCachedResult(params_.url, &is_phishing)) {
@@ -251,6 +255,7 @@ ClientSideDetectionHost::ClientSideDetectionHost(WebContents* tab)
       csd_service_(NULL),
       weak_factory_(this),
       unsafe_unique_page_id_(-1),
+      malware_killswitch_on_(false),
       malware_report_enabled_(false) {
   DCHECK(tab);
   // Note: csd_service_ and sb_service will be NULL here in testing.
@@ -386,15 +391,16 @@ void ClientSideDetectionHost::OnPhishingDetectionDone(
       browse_info_.get() &&
       verdict->ParseFromString(verdict_str) &&
       verdict->IsInitialized()) {
-    if (malware_report_enabled_) {
+    // We do the malware IP matching and request sending if the feature
+    // is enabled.
+    if (malware_report_enabled_ && !MalwareKillSwitchIsOn()) {
       scoped_ptr<ClientMalwareRequest> malware_verdict(
           new ClientMalwareRequest);
       // Start browser-side malware feature extraction.  Once we're done it will
       // send the malware client verdict request.
       malware_verdict->set_url(verdict->url());
       feature_extractor_->ExtractMalwareFeatures(
-          browse_info_.get(),
-          malware_verdict.get());
+          browse_info_.get(), malware_verdict.get());
       MalwareFeatureExtractionDone(malware_verdict.Pass());
     }
 
@@ -512,7 +518,8 @@ void ClientSideDetectionHost::Observe(
   DCHECK_EQ(type, content::NOTIFICATION_RESOURCE_RESPONSE_STARTED);
   const ResourceRequestDetails* req = content::Details<ResourceRequestDetails>(
       details).ptr();
-  if (req && browse_info_.get()) {
+  if (req && browse_info_.get() && malware_report_enabled_ &&
+      !MalwareKillSwitchIsOn()) {
     UpdateIPHostMap(req->socket_address.host() /* ip */,
                     req->url.host()  /* url host */);
   }
@@ -545,4 +552,14 @@ void ClientSideDetectionHost::set_safe_browsing_managers(
   database_manager_ = database_manager;
 }
 
+bool ClientSideDetectionHost::MalwareKillSwitchIsOn() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  return malware_killswitch_on_;
+}
+
+void ClientSideDetectionHost::SetMalwareKillSwitch(bool killswitch_on) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  malware_killswitch_on_ = killswitch_on;
+}
+
 }  // namespace safe_browsing
diff --git a/chrome/browser/safe_browsing/client_side_detection_host.h b/chrome/browser/safe_browsing/client_side_detection_host.h
index 7329f52..7f3e93e 100644
--- a/chrome/browser/safe_browsing/client_side_detection_host.h
+++ b/chrome/browser/safe_browsing/client_side_detection_host.h
@@ -102,6 +102,10 @@ class ClientSideDetectionHost : public content::WebContentsObserver,
       SafeBrowsingUIManager* ui_manager,
       SafeBrowsingDatabaseManager* database_manager);
 
+  // Get/Set malware_killswitch_on_ value. These methods called on UI thread.
+  bool MalwareKillSwitchIsOn();
+  void SetMalwareKillSwitch(bool killswitch_on);
+
   // This pointer may be NULL if client-side phishing detection is disabled.
   ClientSideDetectionService* csd_service_;
   // These pointers may be NULL if SafeBrowsing is disabled.
@@ -137,8 +141,14 @@ class ClientSideDetectionHost : public content::WebContentsObserver,
   int unsafe_unique_page_id_;
   scoped_ptr<SafeBrowsingUIManager::UnsafeResource> unsafe_resource_;
 
+  // Whether the malware IP matching feature killswitch is on.
+  // This should be accessed from UI thread.
+  bool malware_killswitch_on_;
+
   // Whether the malware bad ip matching and report feature is enabled.
+  // This should be accessed from UI thread.
   bool malware_report_enabled_;
+
   DISALLOW_COPY_AND_ASSIGN(ClientSideDetectionHost);
 };
 
diff --git a/chrome/browser/safe_browsing/database_manager.cc b/chrome/browser/safe_browsing/database_manager.cc
index 47b238d..db0963a 100644
--- a/chrome/browser/safe_browsing/database_manager.cc
+++ b/chrome/browser/safe_browsing/database_manager.cc
@@ -306,6 +306,14 @@ bool SafeBrowsingDatabaseManager::MatchDownloadWhitelistString(
   return database_->ContainsDownloadWhitelistedString(str);
 }
 
+bool SafeBrowsingDatabaseManager::IsMalwareKillSwitchOn() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  if (!enabled_ || !MakeDatabaseAvailable()) {
+    return true;
+  }
+  return database_->IsMalwareIPMatchKillSwitchOn();
+}
+
 bool SafeBrowsingDatabaseManager::CheckBrowseUrl(const GURL& url,
                                                  Client* client) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
diff --git a/chrome/browser/safe_browsing/database_manager.h b/chrome/browser/safe_browsing/database_manager.h
index 09f64bd..acc38ef 100644
--- a/chrome/browser/safe_browsing/database_manager.h
+++ b/chrome/browser/safe_browsing/database_manager.h
@@ -172,6 +172,9 @@ class SafeBrowsingDatabaseManager
   // This method is expected to be called on the IO thread.
   virtual bool MatchDownloadWhitelistString(const std::string& str);
 
+  // Check if the CSD malware IP matching kill switch is turned on.
+  virtual bool IsMalwareKillSwitchOn();
+
   // Called on the IO thread to cancel a pending check if the result is no
   // longer needed.
   void CancelCheck(Client* client);
diff --git a/chrome/browser/safe_browsing/safe_browsing_database.cc b/chrome/browser/safe_browsing/safe_browsing_database.cc
index 841e657..b193f50 100644
--- a/chrome/browser/safe_browsing/safe_browsing_database.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_database.cc
@@ -71,6 +71,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.
@@ -1606,3 +1612,12 @@ void SafeBrowsingDatabaseNew::LoadWhitelist(
     whitelist->first.swap(new_whitelist);
   }
 }
+
+bool SafeBrowsingDatabaseNew::IsMalwareIPMatchKillSwitchOn() {
+  SBFullHash malware_kill_switch;
+  crypto::SHA256HashString(kMalwareIPKillSwitchUrl, &malware_kill_switch,
+                           sizeof(malware_kill_switch));
+  std::vector<SBFullHash> full_hashes;
+  full_hashes.push_back(malware_kill_switch);
+  return ContainsWhitelistedHashes(csd_whitelist_, full_hashes);
+}
diff --git a/chrome/browser/safe_browsing/safe_browsing_database.h b/chrome/browser/safe_browsing/safe_browsing_database.h
index 219f1ea..6446e35 100644
--- a/chrome/browser/safe_browsing/safe_browsing_database.h
+++ b/chrome/browser/safe_browsing/safe_browsing_database.h
@@ -171,6 +171,10 @@ class SafeBrowsingDatabase {
       const std::vector<SBPrefix>& prefixes,
       const std::vector<SBFullHashResult>& full_hits) = 0;
 
+  // Returns true if the malware IP blacklisting killswitch URL is present
+  // in the csd whitelist.
+  virtual bool IsMalwareIPMatchKillSwitchOn() = 0;
+
   // The name of the bloom-filter file for the given database file.
   // NOTE(shess): OBSOLETE.  Present for deleting stale files.
   static base::FilePath BloomFilterForFilename(
@@ -297,6 +301,9 @@ class SafeBrowsingDatabaseNew : public SafeBrowsingDatabase {
       const std::vector<SBPrefix>& prefixes,
       const std::vector<SBFullHashResult>& full_hits) OVERRIDE;
 
+  // Returns the value of malware_kill_switch_;
+  virtual bool IsMalwareIPMatchKillSwitchOn() OVERRIDE;
+
  private:
   friend class SafeBrowsingDatabaseTest;
   FRIEND_TEST_ALL_PREFIXES(SafeBrowsingDatabaseTest, HashCaching);
@@ -368,8 +375,7 @@ class SafeBrowsingDatabaseNew : public SafeBrowsingDatabase {
 
   // Lock for protecting access to variables that may be used on the
   // IO thread.  This includes |prefix_set_|, |full_browse_hashes_|,
-  // |pending_browse_hashes_|, |prefix_miss_cache_|, |csd_whitelist_|,
-  // and |csd_whitelist_all_urls_|.
+  // |pending_browse_hashes_|, |prefix_miss_cache_|, |csd_whitelist_|.
   base::Lock lookup_lock_;
 
   // Underlying persistent store for chunk data.
diff --git a/chrome/browser/safe_browsing/safe_browsing_database_unittest.cc b/chrome/browser/safe_browsing/safe_browsing_database_unittest.cc
index ec50a66..7e934a9 100644
--- a/chrome/browser/safe_browsing/safe_browsing_database_unittest.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_database_unittest.cc
@@ -1365,6 +1365,19 @@ TEST_F(SafeBrowsingDatabaseTest, Whitelists) {
   EXPECT_FALSE(database_->ContainsDownloadWhitelistedUrl(
       GURL(std::string("http://www.google.com/"))));
 
+  // Test only add the malware IP killswitch
+  csd_chunks.clear();
+  chunk.hosts.clear();
+  InsertAddChunkHostFullHashes(
+      &chunk, 15, "sb-ssl.google.com/",
+      "sb-ssl.google.com/safebrowsing/csd/killswitch_malware");
+  csd_chunks.push_back(chunk);
+  EXPECT_TRUE(database_->UpdateStarted(&lists));
+  database_->InsertChunks(safe_browsing_util::kCsdWhiteList, csd_chunks);
+  database_->UpdateFinished(true);
+
+  EXPECT_TRUE(database_->IsMalwareIPMatchKillSwitchOn());
+
   // Test that the kill-switch works as intended.
   csd_chunks.clear();
   download_chunks.clear();
@@ -1373,7 +1386,6 @@ TEST_F(SafeBrowsingDatabaseTest, Whitelists) {
   InsertAddChunkHostFullHashes(&chunk, 5, "sb-ssl.google.com/",
                                "sb-ssl.google.com/safebrowsing/csd/killswitch");
   csd_chunks.push_back(chunk);
-
   chunk.hosts.clear();
   InsertAddChunkHostFullHashes(&chunk, 5, "sb-ssl.google.com/",
                                "sb-ssl.google.com/safebrowsing/csd/killswitch");
@@ -1385,6 +1397,7 @@ TEST_F(SafeBrowsingDatabaseTest, Whitelists) {
                           download_chunks);
   database_->UpdateFinished(true);
 
+  EXPECT_TRUE(database_->IsMalwareIPMatchKillSwitchOn());
   EXPECT_TRUE(database_->ContainsCsdWhitelistedUrl(
       GURL(std::string("https://") + kGood1Url2 + "/c.html")));
   EXPECT_TRUE(database_->ContainsCsdWhitelistedUrl(
@@ -1413,6 +1426,12 @@ TEST_F(SafeBrowsingDatabaseTest, Whitelists) {
   csd_chunks.push_back(sub_chunk);
 
   sub_chunk.hosts.clear();
+  InsertSubChunkHostFullHash(
+      &sub_chunk, 10, 15, "sb-ssl.google.com/",
+      "sb-ssl.google.com/safebrowsing/csd/killswitch_malware");
+  csd_chunks.push_back(sub_chunk);
+
+  sub_chunk.hosts.clear();
   InsertSubChunkHostFullHash(&sub_chunk, 1, 5,
                              "sb-ssl.google.com/",
                              "sb-ssl.google.com/safebrowsing/csd/killswitch");
@@ -1424,6 +1443,7 @@ TEST_F(SafeBrowsingDatabaseTest, Whitelists) {
                           download_chunks);
   database_->UpdateFinished(true);
 
+  EXPECT_FALSE(database_->IsMalwareIPMatchKillSwitchOn());
   EXPECT_TRUE(database_->ContainsCsdWhitelistedUrl(
       GURL(std::string("https://") + kGood1Url2 + "/c.html")));
   EXPECT_TRUE(database_->ContainsCsdWhitelistedUrl(
diff --git a/chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc b/chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc
index 20543d5..c36e23f 100644
--- a/chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc
@@ -143,6 +143,9 @@ class TestSafeBrowsingDatabase :  public SafeBrowsingDatabase {
       const std::vector<SBFullHashResult>& full_hits) OVERRIDE {
     // Do nothing for the cache.
   }
+  virtual bool IsMalwareIPMatchKillSwitchOn() OVERRIDE {
+    return false;
+  }
 
   // Fill up the database with test URL.
   void AddUrl(const GURL& url,