From c682493aea96e57a0bd759867f5813ec2486f0cc Mon Sep 17 00:00:00 2001 From: mattm Date: Tue, 16 Sep 2014 21:27:44 -0700 Subject: [PATCH] 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} --- .../safe_browsing/download_protection_service.cc | 29 ++++++-- .../download_protection_service_unittest.cc | 78 ++++++++++++++++++++-- chrome/common/safe_browsing/csd.proto | 3 +- .../safe_browsing/download_protection_util.cc | 11 +++ 4 files changed, 109 insertions(+), 12 deletions(-) diff --git a/chrome/browser/safe_browsing/download_protection_service.cc b/chrome/browser/safe_browsing/download_protection_service.cc index a17daed873d4..bedb410f78e3 100644 --- a/chrome/browser/safe_browsing/download_protection_service.cc +++ b/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 diff --git a/chrome/browser/safe_browsing/download_protection_service_unittest.cc b/chrome/browser/safe_browsing/download_protection_service_unittest.cc index 15d8dd297da1..9242ef4097a1 100644 --- a/chrome/browser/safe_browsing/download_protection_service_unittest.cc +++ b/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(); @@ -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 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 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); diff --git a/chrome/common/safe_browsing/csd.proto b/chrome/common/safe_browsing/csd.proto index 67757922ddaa..ca77f3b91ef5 100644 --- a/chrome/common/safe_browsing/csd.proto +++ b/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]; diff --git a/chrome/common/safe_browsing/download_protection_util.cc b/chrome/common/safe_browsing/download_protection_util.cc index ee09cd1e1fe2..a5a4befa3696 100644 --- a/chrome/common/safe_browsing/download_protection_util.cc +++ b/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; } -- 2.11.4.GIT