Reland Safebrowsing: allow sending enhanced download protection pings on OSX.

The re-land fixes the unittests to test with/without the field trial set.

Sending is controlled by a finch experiment.

The verdict from the server is ignored for now.

BUG=413967

Review URL: https://codereview.chromium.org/580593003

Cr-Commit-Position: refs/heads/master@{#295227}

chrome/browser/safe_browsing/download_protection_service.cc

@@ -9,6 +9,7 @@
 #include "base/format_macros.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
+#include "base/metrics/field_trial.h"
 #include "base/metrics/histogram.h"
 #include "base/metrics/sparse_histogram.h"
 #include "base/sequenced_task_runner_helpers.h"
@@ -615,7 +616,15 @@ class DownloadProtectionService::CheckClientDownloadRequest
     // Currently, the UI only works on Windows so we don't even bother with
     // pinging the server if we're not on Windows.
     // TODO(noelutz): change this code once the UI is done for Linux and Mac.
-#if defined(OS_WIN)
+#if defined(OS_MACOSX)
+  // TODO(mattm): remove this (see crbug.com/414834).
+  if (base::FieldTrialList::FindFullName("SafeBrowsingOSXClientDownloadPings")
+      != "Enabled") {
+    PostFinishTask(UNKNOWN, REASON_OS_NOT_SUPPORTED);
+    return;
+  }
+#endif
+#if defined(OS_WIN) || defined(OS_MACOSX)
     // The URLFetcher is owned by the UI thread, so post a message to
     // start the pingback.
     BrowserThread::PostTask(
@@ -780,6 +789,13 @@ class DownloadProtectionService::CheckClientDownloadRequest
       UMA_HISTOGRAM_ENUMERATION("SBClientDownload.CheckDownloadStats",
                                 reason,
                                 REASON_MAX);
+#if defined(OS_MACOSX)
+      // OSX is currently sending pings only for evaluation purposes, ignore
+      // the result for now.
+      // TODO(mattm): remove this and update the ifdef in
+      // DownloadItemImpl::IsDangerous (see crbug.com/413968).
+      result = UNKNOWN;
+#endif
       callback_.Run(result);
       item_->RemoveObserver(this);
       item_ = NULL;
@@ -929,18 +945,17 @@ void DownloadProtectionService::CheckDownloadUrl(
 bool DownloadProtectionService::IsSupportedDownload(
     const content::DownloadItem& item,
     const base::FilePath& target_path) const {
-  // Currently, the UI only works on Windows.  On Linux and Mac we still
+  // Currently, the UI is only enabled on Windows.  On Mac we send the ping but
+  // ignore the result (see ifdef in FinishRequest).  On Linux we still
   // want to show the dangerous file type warning if the file is possibly
   // dangerous which means we have to always return false here.
 #if defined(OS_WIN)
   DownloadCheckResultReason reason = REASON_MAX;
   ClientDownloadRequest::DownloadType type =
       ClientDownloadRequest::WIN_EXECUTABLE;
-  return (CheckClientDownloadRequest::IsSupportedDownload(item, target_path,
-                                                          &reason, &type) &&
-          (ClientDownloadRequest::ANDROID_APK == type ||
-           ClientDownloadRequest::WIN_EXECUTABLE == type ||
-           ClientDownloadRequest::ZIPPED_EXECUTABLE == type));
+  return (CheckClientDownloadRequest::IsSupportedDownload(
+              item, target_path, &reason, &type) &&
+          (ClientDownloadRequest::CHROME_EXTENSION != type));
 #else
   return false;
 #endif

chrome/browser/safe_browsing/download_protection_service_unittest.cc

@@ -41,6 +41,11 @@
 #include "third_party/zlib/google/zip.h"
 #include "url/gurl.h"
 
+#if defined(OS_MACOSX)
+#include "base/metrics/field_trial.h"
+#include "components/variations/entropy_provider.h"
+#endif
+
 using ::testing::Assign;
 using ::testing::ContainerEq;
 using ::testing::DoAll;
@@ -197,6 +202,13 @@ class DownloadProtectionServiceTest : public testing::Test {
             content::TestBrowserThreadBundle::IO_MAINLOOP) {
   }
   virtual void SetUp() {
+#if defined(OS_MACOSX)
+    field_trial_list_.reset(new base::FieldTrialList(
+          new metrics::SHA1EntropyProvider("42")));
+    ASSERT_TRUE(base::FieldTrialList::CreateFieldTrial(
+          "SafeBrowsingOSXClientDownloadPings",
+          "Enabled"));
+#endif
     // Start real threads for the IO and File threads so that the DCHECKs
     // to test that we're on the correct thread work.
     sb_service_ = new StrictMock<FakeSafeBrowsingService>();
@@ -358,6 +370,9 @@ class DownloadProtectionServiceTest : public testing::Test {
   content::TestBrowserThreadBundle test_browser_thread_bundle_;
   content::InProcessUtilityThreadHelper in_process_utility_thread_helper_;
   base::FilePath testdata_path_;
+#if defined(OS_MACOSX)
+  scoped_ptr<base::FieldTrialList> field_trial_list_;
+#endif
 };
 
 TEST_F(DownloadProtectionServiceTest, CheckClientDownloadInvalidUrl) {
@@ -510,7 +525,11 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadWhitelistedUrl) {
       base::Bind(&DownloadProtectionServiceTest::CheckDoneCallback,
                  base::Unretained(this)));
   MessageLoop::current()->Run();
+#if defined(OS_MACOSX)
+  EXPECT_TRUE(IsResult(DownloadProtectionService::UNKNOWN));
+#else
   EXPECT_TRUE(IsResult(DownloadProtectionService::SAFE));
+#endif
 }
 
 TEST_F(DownloadProtectionServiceTest, CheckClientDownloadFetchFailed) {
@@ -943,6 +962,57 @@ TEST_F(DownloadProtectionServiceTest, CheckClientCrxDownloadSuccess) {
   EXPECT_TRUE(IsResult(DownloadProtectionService::UNKNOWN));
 }
 
+#if defined(OS_MACOSX)
+// TODO(mattm): remove this (see crbug.com/414834).
+TEST_F(DownloadProtectionServiceTest,
+       CheckClientDownloadPingOnOSXRequiresFieldTrial) {
+  // Clear the field trial that was set in SetUp().
+  field_trial_list_.reset();
+
+  net::TestURLFetcherFactory factory;
+
+  base::FilePath tmp_path(FILE_PATH_LITERAL("bla.tmp"));
+  base::FilePath final_path(FILE_PATH_LITERAL("bla.exe"));
+  std::vector<GURL> url_chain;
+  url_chain.push_back(GURL("http://www.google.com/"));
+  url_chain.push_back(GURL("http://www.google.com/bla.exe"));
+  GURL referrer("http://www.google.com/");
+  std::string hash = "hash";
+  std::string remote_address = "10.11.12.13";
+
+  content::MockDownloadItem item;
+  EXPECT_CALL(item, GetFullPath()).WillRepeatedly(ReturnRef(tmp_path));
+  EXPECT_CALL(item, GetTargetFilePath()).WillRepeatedly(ReturnRef(final_path));
+  EXPECT_CALL(item, GetUrlChain()).WillRepeatedly(ReturnRef(url_chain));
+  EXPECT_CALL(item, GetReferrerUrl()).WillRepeatedly(ReturnRef(referrer));
+  EXPECT_CALL(item, GetTabUrl()).WillRepeatedly(ReturnRef(GURL::EmptyGURL()));
+  EXPECT_CALL(item, GetTabReferrerUrl())
+      .WillRepeatedly(ReturnRef(GURL::EmptyGURL()));
+  EXPECT_CALL(item, GetHash()).WillRepeatedly(ReturnRef(hash));
+  EXPECT_CALL(item, GetReceivedBytes()).WillRepeatedly(Return(100));
+  EXPECT_CALL(item, HasUserGesture()).WillRepeatedly(Return(true));
+  EXPECT_CALL(item, GetRemoteAddress()).WillRepeatedly(Return(remote_address));
+
+  EXPECT_CALL(*sb_service_->mock_database_manager(),
+              MatchDownloadWhitelistUrl(_))
+      .WillRepeatedly(Return(false));
+  EXPECT_CALL(*binary_feature_extractor_.get(), CheckSignature(tmp_path, _))
+      .WillOnce(SetCertificateContents("dummy cert data"));
+  EXPECT_CALL(*binary_feature_extractor_.get(),
+              ExtractImageHeaders(tmp_path, _))
+      .WillOnce(SetDosHeaderContents("dummy dos header"));
+  download_service_->CheckClientDownload(
+      &item,
+      base::Bind(&DownloadProtectionServiceTest::CheckDoneCallback,
+                 base::Unretained(this)));
+
+  // SendRequest is not called.  Wait for FinishRequest to call our callback.
+  MessageLoop::current()->Run();
+  net::TestURLFetcher* fetcher = factory.GetFetcherByID(0);
+  EXPECT_EQ(NULL, fetcher);
+}
+#endif
+
 TEST_F(DownloadProtectionServiceTest, CheckClientDownloadValidateRequest) {
   net::TestURLFetcherFactory factory;
 
@@ -981,7 +1051,7 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadValidateRequest) {
       base::Bind(&DownloadProtectionServiceTest::CheckDoneCallback,
                  base::Unretained(this)));
 
-#if !defined(OS_WIN)
+#if !defined(OS_WIN) && !defined(OS_MACOSX)
   // SendRequest is not called.  Wait for FinishRequest to call our callback.
   MessageLoop::current()->Run();
   net::TestURLFetcher* fetcher = factory.GetFetcherByID(0);
@@ -1066,7 +1136,7 @@ TEST_F(DownloadProtectionServiceTest,
       base::Bind(&DownloadProtectionServiceTest::CheckDoneCallback,
                  base::Unretained(this)));
 
-#if !defined(OS_WIN)
+#if !defined(OS_WIN) && !defined(OS_MACOSX)
   // SendRequest is not called.  Wait for FinishRequest to call our callback.
   MessageLoop::current()->Run();
   net::TestURLFetcher* fetcher = factory.GetFetcherByID(0);
@@ -1154,7 +1224,7 @@ TEST_F(DownloadProtectionServiceTest,
         base::Bind(&DownloadProtectionServiceTest::CheckDoneCallback,
                    base::Unretained(this)));
 
-#if !defined(OS_WIN)
+#if !defined(OS_WIN) && !defined(OS_MACOSX)
     // SendRequest is not called.  Wait for FinishRequest to call our callback.
     MessageLoop::current()->Run();
     net::TestURLFetcher* fetcher = factory.GetFetcherByID(0);
@@ -1229,7 +1299,7 @@ TEST_F(DownloadProtectionServiceTest,
         &item,
         base::Bind(&DownloadProtectionServiceTest::CheckDoneCallback,
                    base::Unretained(this)));
-#if !defined(OS_WIN)
+#if !defined(OS_WIN) && !defined(OS_MACOSX)
     // SendRequest is not called.  Wait for FinishRequest to call our callback.
     MessageLoop::current()->Run();
     net::TestURLFetcher* fetcher = factory.GetFetcherByID(0);

chrome/common/safe_browsing/csd.proto

@@ -215,8 +215,9 @@ message ClientDownloadRequest {
     WIN_EXECUTABLE = 0;    // Currently all .exe, .cab and .msi files.
     CHROME_EXTENSION = 1;  // .crx files.
     ANDROID_APK = 2;       // .apk files.
-    // .zip files containing one of the above executable types.
+    // .zip files containing one of the other executable types.
     ZIPPED_EXECUTABLE = 3;
+    MAC_EXECUTABLE = 4;    // .dmg, .pkg, etc.
   }
   optional DownloadType download_type = 10 [default = WIN_EXECUTABLE];
 

chrome/common/safe_browsing/download_protection_util.cc

@@ -11,6 +11,7 @@ namespace safe_browsing {
 namespace download_protection_util {
 
 bool IsArchiveFile(const base::FilePath& file) {
+  // TODO(mattm): should .dmg be checked here instead of IsBinaryFile?
   return file.MatchesExtension(FILE_PATH_LITERAL(".zip"));
 }
 
@@ -33,6 +34,11 @@ bool IsBinaryFile(const base::FilePath& file) {
       // Chrome extensions and android APKs are also reported.
       file.MatchesExtension(FILE_PATH_LITERAL(".crx")) ||
       file.MatchesExtension(FILE_PATH_LITERAL(".apk")) ||
+      // Mac extensions.
+      file.MatchesExtension(FILE_PATH_LITERAL(".dmg")) ||
+      file.MatchesExtension(FILE_PATH_LITERAL(".pkg")) ||
+      file.MatchesExtension(FILE_PATH_LITERAL(".osx")) ||
+      file.MatchesExtension(FILE_PATH_LITERAL(".app")) ||
       // Archives _may_ contain binaries, we'll check in ExtractFileFeatures.
       IsArchiveFile(file));
 }
@@ -48,6 +54,11 @@ ClientDownloadRequest::DownloadType GetDownloadType(
   // the pingback if we find an executable inside the zip archive.
   else if (file.MatchesExtension(FILE_PATH_LITERAL(".zip")))
     return ClientDownloadRequest::ZIPPED_EXECUTABLE;
+  else if (file.MatchesExtension(FILE_PATH_LITERAL(".dmg")) ||
+           file.MatchesExtension(FILE_PATH_LITERAL(".pkg")) ||
+           file.MatchesExtension(FILE_PATH_LITERAL(".osx")) ||
+           file.MatchesExtension(FILE_PATH_LITERAL(".app")))
+    return ClientDownloadRequest::MAC_EXECUTABLE;
   return ClientDownloadRequest::WIN_EXECUTABLE;
 }