- Flip the flag for improved SafeBrowsing downoad protection.

- Send the binary sha256 hash.
- Cancel requests that timed out.

BUG=102540
TEST=Run DownloadProtectionServiceTest. Also, if you have SafeBrowsing enabled you should see a server request for every binary download that doesn't match our whitelist.


Review URL: http://codereview.chromium.org/8586011

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

chrome/browser/safe_browsing/download_protection_service.cc

@@ -5,8 +5,10 @@
 #include "chrome/browser/safe_browsing/download_protection_service.h"
 
 #include "base/bind.h"
+#include "base/compiler_specific.h"
 #include "base/format_macros.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
 #include "base/metrics/histogram.h"
 #include "base/stl_util.h"
 #include "base/string_number_conversions.h"
@@ -27,6 +29,10 @@
 
 using content::BrowserThread;
 
+namespace {
+static const int64 kDownloadRequestTimeoutMs = 3000;
+}  // namespace
+
 namespace safe_browsing {
 
 const char DownloadProtectionService::kDownloadRequestUrl[] =
@@ -144,7 +150,7 @@ DownloadProtectionService::DownloadInfo::FromDownloadItem(
   download_info.target_file = item.GetTargetFilePath();
   download_info.download_url_chain = item.url_chain();
   download_info.referrer_url = item.referrer_url();
-  // TODO(bryner): Fill in the hash (we shouldn't compute it again)
+  download_info.sha256_hash = item.hash();
   download_info.total_bytes = item.total_bytes();
   // TODO(bryner): Populate user_initiated
   return download_info;
@@ -332,7 +338,9 @@ class DownloadProtectionService::CheckClientDownloadRequest
         service_(service),
         signature_util_(signature_util),
         sb_service_(sb_service),
-        pingback_enabled_(service_->enabled()) {
+        pingback_enabled_(service_->enabled()),
+        finished_(false),
+        ALLOW_THIS_IN_INITIALIZER_LIST(timeout_weakptr_factory_(this)) {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   }
 
@@ -373,24 +381,35 @@ class DownloadProtectionService::CheckClientDownloadRequest
         BrowserThread::FILE,
         FROM_HERE,
         base::Bind(&CheckClientDownloadRequest::ExtractFileFeatures, this));
+
+    // If the request takes too long we cancel it.
+    BrowserThread::PostDelayedTask(
+        BrowserThread::UI,
+        FROM_HERE,
+        base::Bind(&CheckClientDownloadRequest::Cancel,
+                   timeout_weakptr_factory_.GetWeakPtr()),
+        service_->download_request_timeout_ms());
   }
 
-  // Canceling a request will cause us to always report the result as SAFE.
-  // In addition, the DownloadProtectionService will not be notified when the
-  // request finishes, so it must drop its reference after calling Cancel.
+  // Canceling a request will cause us to always report the result as SAFE
+  // unless a pending request is about to call FinishRequest.
   void Cancel() {
+    // Calling FinishRequest might delete this object if we don't keep a
+    // reference around until Cancel() is finished running.
+    scoped_refptr<CheckClientDownloadRequest> request(this);
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-    service_ = NULL;
+    FinishRequest(SAFE);
     if (fetcher_.get()) {
       // The DownloadProtectionService is going to release its reference, so we
       // might be destroyed before the URLFetcher completes.  Cancel the
       // fetcher so it does not try to invoke OnURLFetchComplete.
-      FinishRequest(SAFE);
       fetcher_.reset();
     }
     // Note: If there is no fetcher, then some callback is still holding a
     // reference to this object.  We'll eventually wind up in some method on
-    // the UI thread that will call FinishRequest() and run the callback.
+    // the UI thread that will call FinishRequest() again.  If FinishRequest()
+    // is called a second time, it will be a no-op.
+    service_ = NULL;
   }
 
   // From the content::URLFetcherDelegate interface.
@@ -552,6 +571,7 @@ class DownloadProtectionService::CheckClientDownloadRequest
                                                content::URLFetcher::POST,
                                                this));
     fetcher_->SetLoadFlags(net::LOAD_DISABLE_CACHE);
+    fetcher_->SetAutomaticallyRetryOn5xx(false);  // Don't retry on error.
     fetcher_->SetRequestContext(service_->request_context_getter_.get());
     fetcher_->SetUploadData("application/octet-stream", request_data);
     fetcher_->Start();
@@ -566,6 +586,10 @@ class DownloadProtectionService::CheckClientDownloadRequest
 
   void FinishRequest(DownloadCheckResult result) {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+    if (finished_) {
+      return;
+    }
+    finished_ = true;
     if (service_) {
       callback_.Run(result);
       service_->RequestFinished(this);
@@ -591,6 +615,8 @@ class DownloadProtectionService::CheckClientDownloadRequest
   scoped_refptr<SafeBrowsingService> sb_service_;
   const bool pingback_enabled_;
   scoped_ptr<content::URLFetcher> fetcher_;
+  bool finished_;
+  base::WeakPtrFactory<CheckClientDownloadRequest> timeout_weakptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(CheckClientDownloadRequest);
 };
@@ -601,7 +627,8 @@ DownloadProtectionService::DownloadProtectionService(
     : sb_service_(sb_service),
       request_context_getter_(request_context_getter),
       enabled_(false),
-      signature_util_(new SignatureUtil()) {}
+      signature_util_(new SignatureUtil()),
+      download_request_timeout_ms_(kDownloadRequestTimeoutMs) {}
 
 DownloadProtectionService::~DownloadProtectionService() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ -651,10 +678,13 @@ void DownloadProtectionService::CancelPendingRequests() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   for (std::set<scoped_refptr<CheckClientDownloadRequest> >::iterator it =
            download_requests_.begin();
-       it != download_requests_.end(); ++it) {
-    (*it)->Cancel();
+       it != download_requests_.end();) {
+    // We need to advance the iterator before we cancel because canceling
+    // the request will invalidate it when RequestFinished is called below.
+    scoped_refptr<CheckClientDownloadRequest> tmp = *it++;
+    tmp->Cancel();
   }
-  download_requests_.clear();
+  DCHECK(download_requests_.empty());
 }
 
 void DownloadProtectionService::RequestFinished(

chrome/browser/safe_browsing/download_protection_service.h

@@ -101,6 +101,11 @@ class DownloadProtectionService {
     return enabled_;
   }
 
+  // Returns the timeout that is used by CheckClientDownload().
+  int64 download_request_timeout_ms() const {
+    return download_request_timeout_ms_;
+  }
+
  protected:
   // Enum to keep track why a particular download verdict was chosen.
   // This is used to keep some stats around.
@@ -132,6 +137,8 @@ class DownloadProtectionService {
                            CheckClientDownloadSuccess);
   FRIEND_TEST_ALL_PREFIXES(DownloadProtectionServiceTest,
                            CheckClientDownloadFetchFailed);
+  FRIEND_TEST_ALL_PREFIXES(DownloadProtectionServiceTest,
+                           TestDownloadRequestTimeout);
 
   static const char kDownloadRequestUrl[];
 
@@ -164,6 +171,8 @@ class DownloadProtectionService {
   // SignatureUtil object, may be overridden for testing.
   scoped_refptr<SignatureUtil> signature_util_;
 
+  int64 download_request_timeout_ms_;
+
   DISALLOW_COPY_AND_ASSIGN(DownloadProtectionService);
 };
 }  // namespace safe_browsing

chrome/browser/safe_browsing/download_protection_service_unittest.cc

@@ -190,7 +190,7 @@ class DownloadProtectionServiceTest : public testing::Test {
  public:
   void CheckDoneCallback(
       DownloadProtectionService::DownloadCheckResult result) {
-    result_ = result;
+    result_.reset(new DownloadProtectionService::DownloadCheckResult(result));
     msg_loop_.Quit();
   }
 
@@ -198,12 +198,17 @@ class DownloadProtectionServiceTest : public testing::Test {
     fetcher->delegate()->OnURLFetchComplete(fetcher);
   }
 
+  void ExpectResult(DownloadProtectionService::DownloadCheckResult expected) {
+    ASSERT_TRUE(result_.get());
+    EXPECT_EQ(expected, *result_);
+  }
+
  protected:
   scoped_refptr<MockSafeBrowsingService> sb_service_;
   scoped_refptr<MockSignatureUtil> signature_util_;
   DownloadProtectionService* download_service_;
   MessageLoop msg_loop_;
-  DownloadProtectionService::DownloadCheckResult result_;
+  scoped_ptr<DownloadProtectionService::DownloadCheckResult> result_;
   scoped_ptr<content::TestBrowserThread> io_thread_;
   scoped_ptr<content::TestBrowserThread> file_thread_;
   scoped_ptr<content::TestBrowserThread> ui_thread_;
@@ -216,7 +221,7 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadInvalidUrl) {
       base::Bind(&DownloadProtectionServiceTest::CheckDoneCallback,
                  base::Unretained(this)));
   msg_loop_.Run();
-  EXPECT_EQ(DownloadProtectionService::SAFE, result_);
+  ExpectResult(DownloadProtectionService::SAFE);
 
   // Only https is not supported for now for privacy reasons.
   info.local_file = FilePath(FILE_PATH_LITERAL("a.tmp"));
@@ -227,7 +232,7 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadInvalidUrl) {
       base::Bind(&DownloadProtectionServiceTest::CheckDoneCallback,
                  base::Unretained(this)));
   msg_loop_.Run();
-  EXPECT_EQ(DownloadProtectionService::SAFE, result_);
+  ExpectResult(DownloadProtectionService::SAFE);
 }
 
 TEST_F(DownloadProtectionServiceTest, CheckClientDownloadWhitelistedUrl) {
@@ -250,7 +255,7 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadWhitelistedUrl) {
       base::Bind(&DownloadProtectionServiceTest::CheckDoneCallback,
                  base::Unretained(this)));
   msg_loop_.Run();
-  EXPECT_EQ(DownloadProtectionService::SAFE, result_);
+  ExpectResult(DownloadProtectionService::SAFE);
 
   // Check that the referrer is matched against the whitelist.
   info.download_url_chain.pop_back();
@@ -260,7 +265,7 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadWhitelistedUrl) {
       base::Bind(&DownloadProtectionServiceTest::CheckDoneCallback,
                  base::Unretained(this)));
   msg_loop_.Run();
-  EXPECT_EQ(DownloadProtectionService::SAFE, result_);
+  ExpectResult(DownloadProtectionService::SAFE);
 }
 
 TEST_F(DownloadProtectionServiceTest, CheckClientDownloadFetchFailed) {
@@ -284,7 +289,7 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadFetchFailed) {
       base::Bind(&DownloadProtectionServiceTest::CheckDoneCallback,
                  base::Unretained(this)));
   msg_loop_.Run();
-  EXPECT_EQ(DownloadProtectionService::SAFE, result_);
+  ExpectResult(DownloadProtectionService::SAFE);
 }
 
 TEST_F(DownloadProtectionServiceTest, CheckClientDownloadSuccess) {
@@ -312,7 +317,7 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadSuccess) {
       base::Bind(&DownloadProtectionServiceTest::CheckDoneCallback,
                  base::Unretained(this)));
   msg_loop_.Run();
-  EXPECT_EQ(DownloadProtectionService::SAFE, result_);
+  ExpectResult(DownloadProtectionService::SAFE);
 
   // Invalid response should be safe too.
   response.Clear();
@@ -326,7 +331,7 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadSuccess) {
       base::Bind(&DownloadProtectionServiceTest::CheckDoneCallback,
                  base::Unretained(this)));
   msg_loop_.Run();
-  EXPECT_EQ(DownloadProtectionService::SAFE, result_);
+  ExpectResult(DownloadProtectionService::SAFE);
 
   // If the response is dangerous the result should also be marked as dangerous.
   response.set_verdict(ClientDownloadResponse::DANGEROUS);
@@ -340,7 +345,7 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadSuccess) {
       base::Bind(&DownloadProtectionServiceTest::CheckDoneCallback,
                  base::Unretained(this)));
   msg_loop_.Run();
-  EXPECT_EQ(DownloadProtectionService::DANGEROUS, result_);
+  ExpectResult(DownloadProtectionService::DANGEROUS);
 }
 
 TEST_F(DownloadProtectionServiceTest, CheckClientDownloadValidateRequest) {
@@ -473,7 +478,7 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadDigestList) {
       base::Bind(&DownloadProtectionServiceTest::CheckDoneCallback,
                  base::Unretained(this)));
   msg_loop_.Run();
-  EXPECT_EQ(DownloadProtectionService::SAFE, result_);
+  ExpectResult(DownloadProtectionService::SAFE);
   Mock::VerifyAndClearExpectations(sb_service_);
 
   // The hash does not match the bad binary digest list.
@@ -486,7 +491,7 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadDigestList) {
       base::Bind(&DownloadProtectionServiceTest::CheckDoneCallback,
                  base::Unretained(this)));
   msg_loop_.Run();
-  EXPECT_EQ(DownloadProtectionService::SAFE, result_);
+  ExpectResult(DownloadProtectionService::SAFE);
   Mock::VerifyAndClearExpectations(sb_service_);
 
   // The hash matches the bad binary URL list but not the bad binary digest
@@ -502,7 +507,7 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadDigestList) {
       base::Bind(&DownloadProtectionServiceTest::CheckDoneCallback,
                  base::Unretained(this)));
   msg_loop_.Run();
-  EXPECT_EQ(DownloadProtectionService::SAFE, result_);
+  ExpectResult(DownloadProtectionService::SAFE);
   Mock::VerifyAndClearExpectations(sb_service_);
 
   // A match is found with the bad binary digest list.  We currently do not
@@ -517,7 +522,7 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadDigestList) {
       base::Bind(&DownloadProtectionServiceTest::CheckDoneCallback,
                  base::Unretained(this)));
   msg_loop_.Run();
-  EXPECT_EQ(DownloadProtectionService::SAFE, result_);
+  ExpectResult(DownloadProtectionService::SAFE);
   Mock::VerifyAndClearExpectations(sb_service_);
 
   // If the download is not an executable we do not send a server ping but we
@@ -534,7 +539,7 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadDigestList) {
       base::Bind(&DownloadProtectionServiceTest::CheckDoneCallback,
                  base::Unretained(this)));
   msg_loop_.Run();
-  EXPECT_EQ(DownloadProtectionService::SAFE, result_);
+  ExpectResult(DownloadProtectionService::SAFE);
   Mock::VerifyAndClearExpectations(sb_service_);
 
   // If the URL or the referrer matches the download whitelist we cannot send
@@ -555,7 +560,7 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadDigestList) {
       base::Bind(&DownloadProtectionServiceTest::CheckDoneCallback,
                  base::Unretained(this)));
   msg_loop_.Run();
-  EXPECT_EQ(DownloadProtectionService::SAFE, result_);
+  ExpectResult(DownloadProtectionService::SAFE);
   Mock::VerifyAndClearExpectations(sb_service_);
 
   // If the binary is a trusted executable we will not ping the server but
@@ -573,7 +578,7 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadDigestList) {
       base::Bind(&DownloadProtectionServiceTest::CheckDoneCallback,
                  base::Unretained(this)));
   msg_loop_.Run();
-  EXPECT_EQ(DownloadProtectionService::SAFE, result_);
+  ExpectResult(DownloadProtectionService::SAFE);
 }
 
 TEST_F(DownloadProtectionServiceTest, TestCheckDownloadUrl) {
@@ -594,7 +599,7 @@ TEST_F(DownloadProtectionServiceTest, TestCheckDownloadUrl) {
       base::Bind(&DownloadProtectionServiceTest::CheckDoneCallback,
                  base::Unretained(this)));
   msg_loop_.Run();
-  EXPECT_EQ(DownloadProtectionService::SAFE, result_);
+  ExpectResult(DownloadProtectionService::SAFE);
   Mock::VerifyAndClearExpectations(sb_service_);
 
   EXPECT_CALL(*sb_service_,
@@ -607,7 +612,7 @@ TEST_F(DownloadProtectionServiceTest, TestCheckDownloadUrl) {
       base::Bind(&DownloadProtectionServiceTest::CheckDoneCallback,
                  base::Unretained(this)));
   msg_loop_.Run();
-  EXPECT_EQ(DownloadProtectionService::SAFE, result_);
+  ExpectResult(DownloadProtectionService::SAFE);
   Mock::VerifyAndClearExpectations(sb_service_);
 
   EXPECT_CALL(*sb_service_,
@@ -621,7 +626,7 @@ TEST_F(DownloadProtectionServiceTest, TestCheckDownloadUrl) {
       base::Bind(&DownloadProtectionServiceTest::CheckDoneCallback,
                  base::Unretained(this)));
   msg_loop_.Run();
-  EXPECT_EQ(DownloadProtectionService::SAFE, result_);
+  ExpectResult(DownloadProtectionService::SAFE);
   Mock::VerifyAndClearExpectations(sb_service_);
 
   EXPECT_CALL(*sb_service_,
@@ -635,6 +640,33 @@ TEST_F(DownloadProtectionServiceTest, TestCheckDownloadUrl) {
       base::Bind(&DownloadProtectionServiceTest::CheckDoneCallback,
                  base::Unretained(this)));
   msg_loop_.Run();
-  EXPECT_EQ(DownloadProtectionService::DANGEROUS, result_);
+  ExpectResult(DownloadProtectionService::DANGEROUS);
+}
+
+TEST_F(DownloadProtectionServiceTest, TestDownloadRequestTimeout) {
+  TestURLFetcherFactory factory;
+
+  DownloadProtectionService::DownloadInfo info;
+  info.download_url_chain.push_back(GURL("http://www.evil.com/bla.exe"));
+  info.referrer_url = GURL("http://www.google.com/");
+  info.local_file = FilePath(FILE_PATH_LITERAL("a.tmp"));
+  info.target_file = FilePath(FILE_PATH_LITERAL("a.exe"));
+
+  EXPECT_CALL(*sb_service_, MatchDownloadWhitelistUrl(_))
+      .WillRepeatedly(Return(false));
+  EXPECT_CALL(*signature_util_, CheckSignature(info.local_file, _));
+
+  download_service_->download_request_timeout_ms_ = 10;
+  download_service_->CheckClientDownload(
+      info,
+      base::Bind(&DownloadProtectionServiceTest::CheckDoneCallback,
+                 base::Unretained(this)));
+  // Run the message loop(s) until SendRequest is called.
+  FlushThreadMessageLoops();
+
+  // The request should time out because the HTTP request hasn't returned
+  // anything yet.
+  msg_loop_.Run();
+  ExpectResult(DownloadProtectionService::SAFE);
 }
 }  // namespace safe_browsing

chrome/browser/safe_browsing/safe_browsing_service.cc

@@ -1349,7 +1349,7 @@ void SafeBrowsingService::RefreshState() {
     csd_service_->SetEnabledAndRefreshState(enable);
   if (download_service_.get()) {
     download_service_->SetEnabled(
-        enable && CommandLine::ForCurrentProcess()->HasSwitch(
-            switches::kEnableImprovedDownloadProtection));
+        enable && !CommandLine::ForCurrentProcess()->HasSwitch(
+            switches::kDisableImprovedDownloadProtection));
   }
 }

chrome/common/chrome_switches.cc

@@ -262,6 +262,10 @@ const char kDisableHistoryQuickProvider[]   = "disable-history-quick-provider";
 // Disable the use of the HistoryURLProvider for autocomplete results.
 const char kDisableHistoryURLProvider[]     = "disable-history-url-provider";
 
+// Disables improved SafeBrowsing download protection.
+const char kDisableImprovedDownloadProtection[] =
+    "disable-improved-download-protection";
+
 // Disables HTML5 Forms interactive validation.
 const char kDisableInteractiveFormValidation[] =
     "disable-interactive-form-validation";
@@ -470,10 +474,6 @@ const char kEnableFileCookies[]             = "enable-file-cookies";
 // Without this flag, pipelining will never be used.
 const char kEnableHttpPipelining[]          = "enable-http-pipelining";
 
-// Enables improved SafeBrowsing download protection.
-const char kEnableImprovedDownloadProtection[] =
-    "enable-improved-download-protection";
-
 // Enables the in-browser thumbnailing, which is more efficient than the
 // in-renderer thumbnailing, as we can use more information to determine if we
 // need to update thumbnails.

chrome/common/chrome_switches.h

@@ -81,6 +81,7 @@ extern const char kDisableExtensions[];
 extern const char kDisableFlashSandbox[];
 extern const char kDisableHistoryQuickProvider[];
 extern const char kDisableHistoryURLProvider[];
+extern const char kDisableImprovedDownloadProtection[];
 extern const char kDisableInteractiveFormValidation[];
 extern const char kDisableInternalFlash[];
 extern const char kDisableIPv6[];
@@ -137,7 +138,6 @@ extern const char kEnableExtensionTimelineApi[];
 extern const char kEnableFastback[];
 extern const char kEnableFileCookies[];
 extern const char kEnableHttpPipelining[];
-extern const char kEnableImprovedDownloadProtection[];
 extern const char kEnableInBrowserThumbnailing[];
 extern const char kEnableIPv6[];
 extern const char kEnableIPCFuzzing[];