diff options
author | mattm@chromium.org <mattm@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2013-06-15 10:51:29 +0000 |
---|---|---|
committer | mattm@chromium.org <mattm@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2013-06-15 10:51:29 +0000 |
commit | 5d30ae6153623cc8f67a036afdb7537ebce0d681 (patch) | |
tree | fbc7a57ba86164b8f5cfd8c1a13e4abfa6269048 | |
parent | e479082bf5cfe5895dff60271a24b7a410c07c2f (diff) | |
download | chromium_src-5d30ae6153623cc8f67a036afdb7537ebce0d681.zip chromium_src-5d30ae6153623cc8f67a036afdb7537ebce0d681.tar.gz chromium_src-5d30ae6153623cc8f67a036afdb7537ebce0d681.tar.bz2 |
Implement safebrowsing download feedback service, enabled for dev & canary only.
For eligible files, the "Discard" button in the download shelf becomes
"Report & Discard". (Plain "Discard" is still available in the popup menu.)
BUG=169557
Review URL: https://chromiumcodereview.appspot.com/15881012
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@206571 0039d316-1c4b-4281-b951-d872f2087c98
21 files changed, 1402 insertions, 8 deletions
diff --git a/chrome/app/generated_resources.grd b/chrome/app/generated_resources.grd index 2c69841..3136880 100644 --- a/chrome/app/generated_resources.grd +++ b/chrome/app/generated_resources.grd @@ -2920,6 +2920,10 @@ Psst! Incognito mode <ph name="SHORTCUT_KEY">$1<ex>(Ctrl+Shift+N)</ex></ph> may desc="Text for the button used to stop a dangerous download."> Discard </message> + <message name="IDS_REPORT_AND_DISCARD_DOWNLOAD" + desc="Text for the button used to upload a malicious file to Google for analysis and then discard it."> + Report & Discard + </message> <!-- Download Tab Items --> <message name="IDS_DOWNLOAD_LINK_PAUSE" diff --git a/chrome/browser/download/download_item_model.cc b/chrome/browser/download/download_item_model.cc index 347a554e..8561c6b 100644 --- a/chrome/browser/download/download_item_model.cc +++ b/chrome/browser/download/download_item_model.cc @@ -12,6 +12,7 @@ #include "base/supports_user_data.h" #include "base/time.h" #include "chrome/browser/download/download_crx_util.h" +#include "chrome/browser/safe_browsing/download_feedback_service.h" #include "chrome/common/time_format.h" #include "content/public/browser/download_danger_type.h" #include "content/public/browser/download_interrupt_reasons.h" @@ -393,6 +394,13 @@ bool DownloadItemModel::IsMalicious() const { return false; } +bool DownloadItemModel::ShouldAllowDownloadFeedback() const { + if (!IsDangerous()) + return false; + return safe_browsing::DownloadFeedbackService::IsEnabledForDownload( + *download_); +} + bool DownloadItemModel::ShouldRemoveFromShelfWhenComplete() const { // If the download was already opened automatically, it should be removed. if (download_->GetAutoOpened()) diff --git a/chrome/browser/download/download_item_model.h b/chrome/browser/download/download_item_model.h index 66021d3..157024f 100644 --- a/chrome/browser/download/download_item_model.h +++ b/chrome/browser/download/download_item_model.h @@ -79,6 +79,9 @@ class DownloadItemModel { // Is this considered a malicious download? Implies IsDangerous(). bool IsMalicious() const; + // Is safe browsing download feedback feature available for this download? + bool ShouldAllowDownloadFeedback() const; + // Returns |true| if this download is expected to complete successfully and // thereafter be removed from the shelf. Downloads that are opened // automatically or are temporary will be removed from the shelf on successful diff --git a/chrome/browser/safe_browsing/download_feedback.cc b/chrome/browser/safe_browsing/download_feedback.cc new file mode 100644 index 0000000..fef426a --- /dev/null +++ b/chrome/browser/safe_browsing/download_feedback.cc @@ -0,0 +1,226 @@ +// Copyright 2013 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "chrome/browser/safe_browsing/download_feedback.h" + +#include "base/bind.h" +#include "base/command_line.h" +#include "base/files/file_util_proxy.h" +#include "base/metrics/histogram.h" +#include "base/task_runner.h" +#include "chrome/common/chrome_switches.h" +#include "chrome/common/safe_browsing/csd.pb.h" +#include "net/base/net_errors.h" + +namespace safe_browsing { + +namespace { + +// The default URL where browser sends download feedback requests. +const char kSbDefaultFeedbackURL[] = + "https://safebrowsing.google.com/safebrowsing/uploads/chrome"; + +// This enum is used by histograms. Do not change the ordering or remove items. +enum UploadResultType { + UPLOAD_SUCCESS, + UPLOAD_CANCELLED, + UPLOAD_METADATA_NET_ERROR, + UPLOAD_METADATA_RESPONSE_ERROR, + UPLOAD_FILE_NET_ERROR, + UPLOAD_FILE_RESPONSE_ERROR, + UPLOAD_COMPLETE_RESPONSE_ERROR, + // Memory space for histograms is determined by the max. + // ALWAYS ADD NEW VALUES BEFORE THIS ONE. + UPLOAD_RESULT_MAX +}; + +// Handles the uploading of a single downloaded binary to the safebrowsing +// download feedback service. +class DownloadFeedbackImpl : public DownloadFeedback { + public: + DownloadFeedbackImpl(net::URLRequestContextGetter* request_context_getter, + base::TaskRunner* file_task_runner, + const base::FilePath& file_path, + const std::string& ping_request, + const std::string& ping_response); + virtual ~DownloadFeedbackImpl(); + + virtual void Start(const base::Closure& finish_callback) OVERRIDE; + + virtual const std::string& GetPingRequestForTesting() const OVERRIDE { + return ping_request_; + } + + virtual const std::string& GetPingResponseForTesting() const OVERRIDE { + return ping_response_; + } + + private: + // Callback for TwoPhaseUploader completion. Relays the result to the + // |finish_callback|. + void FinishedUpload(base::Closure finish_callback, + TwoPhaseUploader::State state, + int net_error, + int response_code, + const std::string& response); + + void RecordUploadResult(UploadResultType result); + + scoped_refptr<net::URLRequestContextGetter> request_context_getter_; + scoped_refptr<base::TaskRunner> file_task_runner_; + const base::FilePath file_path_; + int64 file_size_; + + // The safebrowsing request and response of checking that this binary is + // unsafe. + std::string ping_request_; + std::string ping_response_; + + scoped_ptr<TwoPhaseUploader> uploader_; + + DISALLOW_COPY_AND_ASSIGN(DownloadFeedbackImpl); +}; + +DownloadFeedbackImpl::DownloadFeedbackImpl( + net::URLRequestContextGetter* request_context_getter, + base::TaskRunner* file_task_runner, + const base::FilePath& file_path, + const std::string& ping_request, + const std::string& ping_response) + : request_context_getter_(request_context_getter), + file_task_runner_(file_task_runner), + file_path_(file_path), + file_size_(-1), + ping_request_(ping_request), + ping_response_(ping_response) { + DVLOG(1) << "DownloadFeedback constructed " << this << " for " + << file_path.AsUTF8Unsafe(); +} + +DownloadFeedbackImpl::~DownloadFeedbackImpl() { + DCHECK(CalledOnValidThread()); + DVLOG(1) << "DownloadFeedback destructed " << this; + + if (uploader_) { + RecordUploadResult(UPLOAD_CANCELLED); + // Destroy the uploader before attempting to delete the file. + uploader_.reset(); + } + + base::FileUtilProxy::Delete(file_task_runner_, file_path_, false, + base::FileUtilProxy::StatusCallback()); +} + +void DownloadFeedbackImpl::Start(const base::Closure& finish_callback) { + DCHECK(CalledOnValidThread()); + DCHECK(!uploader_); + + CommandLine* cmdline = CommandLine::ForCurrentProcess(); + + ClientDownloadReport report_metadata; + + bool r = report_metadata.mutable_download_request()->ParseFromString( + ping_request_); + DCHECK(r); + r = report_metadata.mutable_download_response()->ParseFromString( + ping_response_); + DCHECK(r); + file_size_ = report_metadata.download_request().length(); + + GURL url = GURL(cmdline->HasSwitch(switches::kSbDownloadFeedbackURL) ? + cmdline->GetSwitchValueASCII(switches::kSbDownloadFeedbackURL) : + kSbDefaultFeedbackURL); + + std::string metadata_string; + bool ok = report_metadata.SerializeToString(&metadata_string); + DCHECK(ok); + uploader_.reset(TwoPhaseUploader::Create( + request_context_getter_, + file_task_runner_, + url, + metadata_string, + file_path_, + TwoPhaseUploader::ProgressCallback(), + base::Bind(&DownloadFeedbackImpl::FinishedUpload, base::Unretained(this), + finish_callback))); + uploader_->Start(); +} + +void DownloadFeedbackImpl::FinishedUpload(base::Closure finish_callback, + TwoPhaseUploader::State state, + int net_error, + int response_code, + const std::string& response_data) { + DCHECK(CalledOnValidThread()); + DVLOG(1) << __FUNCTION__ << " " << state << " rlen=" << response_data.size(); + + switch (state) { + case TwoPhaseUploader::STATE_SUCCESS: { + ClientUploadResponse response; + if (!response.ParseFromString(response_data) || + response.status() != ClientUploadResponse::SUCCESS) + RecordUploadResult(UPLOAD_COMPLETE_RESPONSE_ERROR); + else + RecordUploadResult(UPLOAD_SUCCESS); + break; + } + case TwoPhaseUploader::UPLOAD_FILE: + if (net_error != net::OK) + RecordUploadResult(UPLOAD_FILE_NET_ERROR); + else + RecordUploadResult(UPLOAD_FILE_RESPONSE_ERROR); + break; + case TwoPhaseUploader::UPLOAD_METADATA: + if (net_error != net::OK) + RecordUploadResult(UPLOAD_METADATA_NET_ERROR); + else + RecordUploadResult(UPLOAD_METADATA_RESPONSE_ERROR); + break; + default: + NOTREACHED(); + } + + uploader_.reset(); + + finish_callback.Run(); + // We may be deleted here. +} + +void DownloadFeedbackImpl::RecordUploadResult(UploadResultType result) { + if (result == UPLOAD_SUCCESS) + UMA_HISTOGRAM_CUSTOM_COUNTS( + "SBDownloadFeedback.SizeSuccess", file_size_, 1, kMaxUploadSize, 50); + else + UMA_HISTOGRAM_CUSTOM_COUNTS( + "SBDownloadFeedback.SizeFailure", file_size_, 1, kMaxUploadSize, 50); + UMA_HISTOGRAM_ENUMERATION( + "SBDownloadFeedback.UploadResult", result, UPLOAD_RESULT_MAX); +} + +} // namespace + +// static +const int64 DownloadFeedback::kMaxUploadSize = 50 * 1024 * 1024; + +// static +DownloadFeedbackFactory* DownloadFeedback::factory_ = NULL; + +// static +DownloadFeedback* DownloadFeedback::Create( + net::URLRequestContextGetter* request_context_getter, + base::TaskRunner* file_task_runner, + const base::FilePath& file_path, + const std::string& ping_request, + const std::string& ping_response) { + if (!DownloadFeedback::factory_) + return new DownloadFeedbackImpl( + request_context_getter, file_task_runner, file_path, ping_request, + ping_response); + return DownloadFeedback::factory_->CreateDownloadFeedback( + request_context_getter, file_task_runner, file_path, ping_request, + ping_response); +} + +} // namespace safe_browsing + diff --git a/chrome/browser/safe_browsing/download_feedback.h b/chrome/browser/safe_browsing/download_feedback.h new file mode 100644 index 0000000..2a9a6c7 --- /dev/null +++ b/chrome/browser/safe_browsing/download_feedback.h @@ -0,0 +1,79 @@ +// Copyright 2013 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef CHROME_BROWSER_SAFE_BROWSING_DOWNLOAD_FEEDBACK_H_ +#define CHROME_BROWSER_SAFE_BROWSING_DOWNLOAD_FEEDBACK_H_ + +#include <string> + +#include "base/callback_forward.h" +#include "base/files/file_path.h" +#include "base/memory/scoped_ptr.h" +#include "base/threading/non_thread_safe.h" +#include "chrome/browser/safe_browsing/two_phase_uploader.h" + +namespace content { +class DownloadItem; +} + +namespace safe_browsing { + +class DownloadFeedbackFactory; + +// Handles the uploading of a single downloaded binary to the safebrowsing +// download feedback service. +class DownloadFeedback : public base::NonThreadSafe { + public: + // Takes ownership of the file pointed to be |file_path|, it will be deleted + // when the DownloadFeedback is destructed. + static DownloadFeedback* Create( + net::URLRequestContextGetter* request_context_getter, + base::TaskRunner* file_task_runner, + const base::FilePath& file_path, + const std::string& ping_request, + const std::string& ping_response); + + // The largest file size we support uploading. + // Note: changing this will affect the max size of + // SBDownloadFeedback.SizeSuccess and SizeFailure histograms. + static const int64 kMaxUploadSize; + + virtual ~DownloadFeedback() {} + + // Makes the passed |factory| the factory used to instantiate + // a DownloadFeedback. Useful for tests. + static void RegisterFactory(DownloadFeedbackFactory* factory) { + factory_ = factory; + } + + // Start uploading the file to the download feedback service. + // |finish_callback| will be called when the upload completes or fails, but is + // not called if the upload is cancelled by deleting the DownloadFeedback + // while the upload is in progress. + virtual void Start(const base::Closure& finish_callback) = 0; + + virtual const std::string& GetPingRequestForTesting() const = 0; + virtual const std::string& GetPingResponseForTesting() const = 0; + + private: + // The factory that controls the creation of DownloadFeedback objects. + // This is used by tests. + static DownloadFeedbackFactory* factory_; +}; + +class DownloadFeedbackFactory { + public: + virtual ~DownloadFeedbackFactory() {} + + virtual DownloadFeedback* CreateDownloadFeedback( + net::URLRequestContextGetter* request_context_getter, + base::TaskRunner* file_task_runner, + const base::FilePath& file_path, + const std::string& ping_request, + const std::string& ping_response) = 0; +}; + +} // namespace safe_browsing + +#endif // CHROME_BROWSER_SAFE_BROWSING_DOWNLOAD_FEEDBACK_H_ diff --git a/chrome/browser/safe_browsing/download_feedback_service.cc b/chrome/browser/safe_browsing/download_feedback_service.cc new file mode 100644 index 0000000..3e5b378 --- /dev/null +++ b/chrome/browser/safe_browsing/download_feedback_service.cc @@ -0,0 +1,216 @@ +// Copyright 2013 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "chrome/browser/safe_browsing/download_feedback_service.h" + +#include "base/bind.h" +#include "base/command_line.h" +#include "base/files/file_path.h" +#include "base/files/file_util_proxy.h" +#include "base/metrics/histogram.h" +#include "base/supports_user_data.h" +#include "base/task_runner.h" +#include "chrome/browser/safe_browsing/download_feedback.h" +#include "chrome/common/chrome_switches.h" +#include "chrome/common/chrome_version_info.h" +#include "content/public/browser/download_danger_type.h" +#include "content/public/browser/download_item.h" + +namespace safe_browsing { + +namespace { + +const void* kPingKey = &kPingKey; + +bool IsEnabled() { + CommandLine* cmdline = CommandLine::ForCurrentProcess(); + if (cmdline->HasSwitch(switches::kSbEnableDownloadFeedback)) + return true; + + chrome::VersionInfo::Channel channel = chrome::VersionInfo::GetChannel(); + if (channel == chrome::VersionInfo::CHANNEL_UNKNOWN || + channel == chrome::VersionInfo::CHANNEL_CANARY || + channel == chrome::VersionInfo::CHANNEL_DEV) + return true; + + return false; +} + +class DownloadFeedbackPings : public base::SupportsUserData::Data { + public: + DownloadFeedbackPings(const std::string& ping_request, + const std::string& ping_response); + + // Stores the ping data in the given |download|. + static void CreateForDownload(content::DownloadItem* download, + const std::string& ping_request, + const std::string& ping_response); + + // Returns the DownloadFeedbackPings object associated with |download|. May + // return NULL. + static DownloadFeedbackPings* FromDownload( + const content::DownloadItem& download); + + + const std::string& ping_request() const { + return ping_request_; + } + + const std::string& ping_response() const { + return ping_response_; + } + + private: + std::string ping_request_; + std::string ping_response_; +}; + +DownloadFeedbackPings::DownloadFeedbackPings(const std::string& ping_request, + const std::string& ping_response) + : ping_request_(ping_request), + ping_response_(ping_response) { +} + +// static +void DownloadFeedbackPings::CreateForDownload( + content::DownloadItem* download, + const std::string& ping_request, + const std::string& ping_response) { + DownloadFeedbackPings* pings = new DownloadFeedbackPings(ping_request, + ping_response); + download->SetUserData(kPingKey, pings); +} + +// static +DownloadFeedbackPings* DownloadFeedbackPings::FromDownload( + const content::DownloadItem& download) { + return static_cast<DownloadFeedbackPings*>(download.GetUserData(kPingKey)); +} + +} // namespace + +DownloadFeedbackService::DownloadFeedbackService( + net::URLRequestContextGetter* request_context_getter, + base::TaskRunner* file_task_runner) + : request_context_getter_(request_context_getter), + file_task_runner_(file_task_runner), + weak_ptr_factory_(this) { +} + +DownloadFeedbackService::~DownloadFeedbackService() { + DCHECK(CalledOnValidThread()); +} + +// static +void DownloadFeedbackService::MaybeStorePingsForDownload( + DownloadProtectionService::DownloadCheckResult result, + content::DownloadItem* download, + const std::string& ping, + const std::string& response) { + if (!IsEnabled() || !(result == DownloadProtectionService::UNCOMMON || + result == DownloadProtectionService::DANGEROUS_HOST)) + return; + UMA_HISTOGRAM_COUNTS("SBDownloadFeedback.SizeEligibleKB", + download->GetReceivedBytes() / 1024); + if (download->GetReceivedBytes() > DownloadFeedback::kMaxUploadSize) + return; + + DownloadFeedbackPings::CreateForDownload(download, ping, response); +} + +// static +bool DownloadFeedbackService::IsEnabledForDownload( + const content::DownloadItem& download) { + return !!DownloadFeedbackPings::FromDownload(download); +} + +// static +bool DownloadFeedbackService::GetPingsForDownloadForTesting( + const content::DownloadItem& download, + std::string* ping, + std::string* response) { + DownloadFeedbackPings* pings = DownloadFeedbackPings::FromDownload(download); + if (!pings) + return false; + + *ping = pings->ping_request(); + *response = pings->ping_response(); + return true; +} + +// static +void DownloadFeedbackService::RecordFeedbackButtonShown( + content::DownloadDangerType danger_type) { + UMA_HISTOGRAM_ENUMERATION("SBDownloadFeedback.Shown", + danger_type, + content::DOWNLOAD_DANGER_TYPE_MAX); +} + + +void DownloadFeedbackService::BeginFeedbackForDownload( + content::DownloadItem* download) { + DCHECK(CalledOnValidThread()); + + UMA_HISTOGRAM_ENUMERATION("SBDownloadFeedback.Activations", + download->GetDangerType(), + content::DOWNLOAD_DANGER_TYPE_MAX); + + DownloadFeedbackPings* pings = DownloadFeedbackPings::FromDownload(*download); + DCHECK(pings); + + download->StealDangerousDownload( + base::Bind(&DownloadFeedbackService::BeginFeedbackOrDeleteFile, + file_task_runner_, + weak_ptr_factory_.GetWeakPtr(), + pings->ping_request(), + pings->ping_response())); +} + +// static +void DownloadFeedbackService::BeginFeedbackOrDeleteFile( + const scoped_refptr<base::TaskRunner>& file_task_runner, + const base::WeakPtr<DownloadFeedbackService>& service, + const std::string& ping_request, + const std::string& ping_response, + const base::FilePath& path) { + if (service) { + service->BeginFeedback(ping_request, ping_response, path); + } else { + base::FileUtilProxy::Delete(file_task_runner, path, false, + base::FileUtilProxy::StatusCallback()); + } +} + +void DownloadFeedbackService::StartPendingFeedback() { + DCHECK(!active_feedback_.empty()); + active_feedback_.front()->Start(base::Bind( + &DownloadFeedbackService::FeedbackComplete, base::Unretained(this))); +} + +void DownloadFeedbackService::BeginFeedback( + const std::string& ping_request, + const std::string& ping_response, + const base::FilePath& path) { + DCHECK(CalledOnValidThread()); + DownloadFeedback* feedback = DownloadFeedback::Create( + request_context_getter_, file_task_runner_, path, + ping_request, ping_response); + active_feedback_.push_back(feedback); + UMA_HISTOGRAM_COUNTS_100("SBDownloadFeedback.ActiveFeedbacks", + active_feedback_.size()); + + if (active_feedback_.size() == 1) + StartPendingFeedback(); +} + +void DownloadFeedbackService::FeedbackComplete() { + DVLOG(1) << __FUNCTION__; + DCHECK(CalledOnValidThread()); + DCHECK(!active_feedback_.empty()); + active_feedback_.erase(active_feedback_.begin()); + if (!active_feedback_.empty()) + StartPendingFeedback(); +} + +} // namespace safe_browsing diff --git a/chrome/browser/safe_browsing/download_feedback_service.h b/chrome/browser/safe_browsing/download_feedback_service.h new file mode 100644 index 0000000..bbfc211 --- /dev/null +++ b/chrome/browser/safe_browsing/download_feedback_service.h @@ -0,0 +1,96 @@ +// Copyright 2013 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef CHROME_BROWSER_SAFE_BROWSING_DOWNLOAD_FEEDBACK_SERVICE_H_ +#define CHROME_BROWSER_SAFE_BROWSING_DOWNLOAD_FEEDBACK_SERVICE_H_ + +#include <string> + +#include "base/basictypes.h" +#include "base/memory/ref_counted.h" +#include "base/memory/scoped_vector.h" +#include "base/threading/non_thread_safe.h" +#include "chrome/browser/safe_browsing/download_protection_service.h" +#include "content/public/browser/download_danger_type.h" + +namespace base { +class TaskRunner; +} + +namespace content { +class DownloadItem; +} + +namespace net { +class URLRequestContextGetter; +} + +namespace safe_browsing { + +class DownloadFeedback; + +// Tracks active DownloadFeedback objects, provides interface for storing ping +// data for malicious downloads. +class DownloadFeedbackService : public base::NonThreadSafe { + public: + DownloadFeedbackService(net::URLRequestContextGetter* request_context_getter, + base::TaskRunner* file_task_runner); + ~DownloadFeedbackService(); + + // Stores the request and response ping data from the download check, if the + // check result and file size are eligible. This must be called after a + // download has been flagged as malicious in order for the download to be + // enabled for uploading. + static void MaybeStorePingsForDownload( + DownloadProtectionService::DownloadCheckResult result, + content::DownloadItem* download, + const std::string& ping, + const std::string& response); + + // Test if pings have been stored for |download|. + static bool IsEnabledForDownload(const content::DownloadItem& download); + + // Get the ping values stored in |download|. Returns false if no ping values + // are present. + static bool GetPingsForDownloadForTesting( + const content::DownloadItem& download, + std::string* ping, + std::string* response); + + // Records histogram for download feedback option shown to user. + static void RecordFeedbackButtonShown( + content::DownloadDangerType danger_type); + + // Begin download feedback for |download|. The |download| will be deleted + // when this function returns. This must only be called if + // IsEnabledForDownload is true for |download|. + void BeginFeedbackForDownload(content::DownloadItem* download); + + private: + static void BeginFeedbackOrDeleteFile( + const scoped_refptr<base::TaskRunner>& file_task_runner, + const base::WeakPtr<DownloadFeedbackService>& service, + const std::string& ping_request, + const std::string& ping_response, + const base::FilePath& path); + void StartPendingFeedback(); + void BeginFeedback(const std::string& ping_request, + const std::string& ping_response, + const base::FilePath& path); + void FeedbackComplete(); + + scoped_refptr<net::URLRequestContextGetter> request_context_getter_; + scoped_refptr<base::TaskRunner> file_task_runner_; + + // Currently active & pending uploads. The first item is active, remaining + // items are pending. + ScopedVector<DownloadFeedback> active_feedback_; + + base::WeakPtrFactory<DownloadFeedbackService> weak_ptr_factory_; + + DISALLOW_COPY_AND_ASSIGN(DownloadFeedbackService); +}; +} // namespace safe_browsing + +#endif // CHROME_BROWSER_SAFE_BROWSING_DOWNLOAD_FEEDBACK_SERVICE_H_ diff --git a/chrome/browser/safe_browsing/download_feedback_service_unittest.cc b/chrome/browser/safe_browsing/download_feedback_service_unittest.cc new file mode 100644 index 0000000..8f0a874 --- /dev/null +++ b/chrome/browser/safe_browsing/download_feedback_service_unittest.cc @@ -0,0 +1,374 @@ +// Copyright 2013 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "chrome/browser/safe_browsing/download_feedback_service.h" + +#include "base/command_line.h" +#include "base/file_util.h" +#include "base/files/scoped_temp_dir.h" +#include "base/run_loop.h" +#include "base/strings/string_number_conversions.h" +#include "chrome/browser/safe_browsing/download_feedback.h" +#include "chrome/common/chrome_switches.h" +#include "content/public/browser/browser_thread.h" +#include "content/public/test/mock_download_item.h" +#include "content/public/test/test_browser_thread_bundle.h" +#include "net/url_request/url_request_test_util.h" +#include "testing/gmock/include/gmock/gmock.h" +#include "testing/gtest/include/gtest/gtest.h" + +using ::testing::_; +using ::testing::Return; +using ::testing::SaveArg; + +namespace safe_browsing { + +namespace { + +class FakeDownloadFeedback : public DownloadFeedback { + public: + FakeDownloadFeedback(net::URLRequestContextGetter* request_context_getter, + base::TaskRunner* file_task_runner, + const base::FilePath& file_path, + const std::string& ping_request, + const std::string& ping_response, + base::Closure deletion_callback) + : ping_request_(ping_request), + ping_response_(ping_response), + deletion_callback_(deletion_callback), + start_called_(false) { + } + + virtual ~FakeDownloadFeedback() { + deletion_callback_.Run(); + } + + virtual void Start(const base::Closure& finish_callback) OVERRIDE { + start_called_ = true; + finish_callback_ = finish_callback; + } + + virtual const std::string& GetPingRequestForTesting() const OVERRIDE { + return ping_request_; + } + + virtual const std::string& GetPingResponseForTesting() const OVERRIDE { + return ping_response_; + } + + base::Closure finish_callback() const { + return finish_callback_; + } + + bool start_called() const { + return start_called_; + } + + private: + scoped_refptr<net::URLRequestContextGetter> request_context_getter_; + scoped_refptr<base::TaskRunner> file_task_runner_; + base::FilePath file_path_; + std::string ping_request_; + std::string ping_response_; + + base::Closure finish_callback_; + base::Closure deletion_callback_; + bool start_called_; +}; + +class FakeDownloadFeedbackFactory : public DownloadFeedbackFactory { + public: + virtual ~FakeDownloadFeedbackFactory() {} + + virtual DownloadFeedback* CreateDownloadFeedback( + net::URLRequestContextGetter* request_context_getter, + base::TaskRunner* file_task_runner, + const base::FilePath& file_path, + const std::string& ping_request, + const std::string& ping_response) OVERRIDE { + FakeDownloadFeedback* feedback = new FakeDownloadFeedback( + request_context_getter, + file_task_runner, + file_path, + ping_request, + ping_response, + base::Bind(&FakeDownloadFeedbackFactory::DownloadFeedbackDeleted, + base::Unretained(this), + feedbacks_.size())); + feedbacks_.push_back(feedback); + return feedback; + } + + void DownloadFeedbackDeleted(size_t n) { + feedbacks_[n] = NULL; + } + + FakeDownloadFeedback* feedback(size_t n) const { + return feedbacks_[n]; + } + + size_t num_feedbacks() const { + return feedbacks_.size(); + } + + private: + std::vector<FakeDownloadFeedback*> feedbacks_; +}; + +bool WillStorePings(DownloadProtectionService::DownloadCheckResult result, + int64 size) { + content::MockDownloadItem item; + EXPECT_CALL(item, GetReceivedBytes()).WillRepeatedly(Return(size)); + + EXPECT_FALSE(DownloadFeedbackService::IsEnabledForDownload(item)); + DownloadFeedbackService::MaybeStorePingsForDownload(result, &item, "a", "b"); + return DownloadFeedbackService::IsEnabledForDownload(item); +} + +} // namespace + +class DownloadFeedbackServiceTest : public testing::Test { + public: + DownloadFeedbackServiceTest() + : file_task_runner_(content::BrowserThread::GetMessageLoopProxyForThread( + content::BrowserThread::FILE)), + io_task_runner_(content::BrowserThread::GetMessageLoopProxyForThread( + content::BrowserThread::IO)), + request_context_getter_( + new net::TestURLRequestContextGetter(io_task_runner_)) { + } + + virtual void SetUp() OVERRIDE { + ASSERT_TRUE(temp_dir_.CreateUniqueTempDir()); + CommandLine::ForCurrentProcess()->AppendSwitch( + switches::kSbEnableDownloadFeedback); + DownloadFeedback::RegisterFactory(&download_feedback_factory_); + } + + virtual void TearDown() OVERRIDE { + DownloadFeedback::RegisterFactory(NULL); + } + + base::FilePath CreateTestFile(int n) const { + base::FilePath upload_file_path( + temp_dir_.path().AppendASCII("test file " + base::IntToString(n))); + const std::string upload_file_data = "data"; + int wrote = file_util::WriteFile( + upload_file_path, upload_file_data.data(), upload_file_data.size()); + EXPECT_EQ(static_cast<int>(upload_file_data.size()), wrote); + return upload_file_path; + } + + FakeDownloadFeedback* feedback(size_t n) const { + return download_feedback_factory_.feedback(n); + } + + size_t num_feedbacks() const { + return download_feedback_factory_.num_feedbacks(); + } + protected: + base::ScopedTempDir temp_dir_; + content::TestBrowserThreadBundle thread_bundle_; + scoped_refptr<base::SingleThreadTaskRunner> file_task_runner_; + scoped_refptr<base::SingleThreadTaskRunner> io_task_runner_; + scoped_refptr<net::TestURLRequestContextGetter> request_context_getter_; + FakeDownloadFeedbackFactory download_feedback_factory_; + +}; + +TEST_F(DownloadFeedbackServiceTest, MaybeStorePingsForDownload) { + const int64 ok_size = DownloadFeedback::kMaxUploadSize; + const int64 bad_size = DownloadFeedback::kMaxUploadSize + 1; + + EXPECT_FALSE(WillStorePings(DownloadProtectionService::SAFE, ok_size)); + EXPECT_FALSE(WillStorePings(DownloadProtectionService::DANGEROUS, ok_size)); + EXPECT_TRUE(WillStorePings(DownloadProtectionService::UNCOMMON, ok_size)); + EXPECT_TRUE( + WillStorePings(DownloadProtectionService::DANGEROUS_HOST, ok_size)); + + EXPECT_FALSE(WillStorePings(DownloadProtectionService::SAFE, bad_size)); + EXPECT_FALSE(WillStorePings(DownloadProtectionService::DANGEROUS, bad_size)); + EXPECT_FALSE(WillStorePings(DownloadProtectionService::UNCOMMON, bad_size)); + EXPECT_FALSE( + WillStorePings(DownloadProtectionService::DANGEROUS_HOST, bad_size)); +} + +TEST_F(DownloadFeedbackServiceTest, SingleFeedbackComplete) { + const base::FilePath file_path(CreateTestFile(0)); + const std::string ping_request = "ping"; + const std::string ping_response = "resp"; + + content::DownloadItem::AcquireFileCallback download_discarded_callback; + + content::MockDownloadItem item; + EXPECT_CALL(item, GetDangerType()) + .WillRepeatedly(Return(content::DOWNLOAD_DANGER_TYPE_UNCOMMON_CONTENT)); + EXPECT_CALL(item, GetReceivedBytes()).WillRepeatedly(Return(1000)); + EXPECT_CALL(item, StealDangerousDownload(_)) + .WillOnce(SaveArg<0>(&download_discarded_callback)); + + DownloadFeedbackService service(request_context_getter_, file_task_runner_); + service.MaybeStorePingsForDownload( + DownloadProtectionService::UNCOMMON, &item, ping_request, ping_response); + ASSERT_TRUE(DownloadFeedbackService::IsEnabledForDownload(item)); + service.BeginFeedbackForDownload(&item); + ASSERT_FALSE(download_discarded_callback.is_null()); + EXPECT_EQ(0U, num_feedbacks()); + + download_discarded_callback.Run(file_path); + ASSERT_EQ(1U, num_feedbacks()); + ASSERT_TRUE(feedback(0)); + EXPECT_TRUE(feedback(0)->start_called()); + EXPECT_EQ(ping_request, feedback(0)->GetPingRequestForTesting()); + EXPECT_EQ(ping_response, feedback(0)->GetPingResponseForTesting()); + + feedback(0)->finish_callback().Run(); + EXPECT_FALSE(feedback(0)); + + // File should still exist since our FakeDownloadFeedback does not delete it. + base::RunLoop().RunUntilIdle(); + EXPECT_TRUE(file_util::PathExists(file_path)); +} + +TEST_F(DownloadFeedbackServiceTest, MultiplePendingFeedbackComplete) { + const std::string ping_request = "ping"; + const std::string ping_response = "resp"; + const size_t num_downloads = 3; + + content::DownloadItem::AcquireFileCallback + download_discarded_callback[num_downloads]; + + base::FilePath file_path[num_downloads]; + content::MockDownloadItem item[num_downloads]; + for (size_t i = 0; i < num_downloads; ++i) { + file_path[i] = CreateTestFile(i); + EXPECT_CALL(item[i], GetDangerType()) + .WillRepeatedly(Return(content::DOWNLOAD_DANGER_TYPE_UNCOMMON_CONTENT)); + EXPECT_CALL(item[i], GetReceivedBytes()).WillRepeatedly(Return(1000)); + EXPECT_CALL(item[i], StealDangerousDownload(_)) + .WillOnce(SaveArg<0>(&download_discarded_callback[i])); + DownloadFeedbackService::MaybeStorePingsForDownload( + DownloadProtectionService::UNCOMMON, &item[i], ping_request, + ping_response); + ASSERT_TRUE(DownloadFeedbackService::IsEnabledForDownload(item[i])); + } + + { + DownloadFeedbackService service(request_context_getter_, file_task_runner_); + for (size_t i = 0; i < num_downloads; ++i) { + SCOPED_TRACE(i); + service.BeginFeedbackForDownload(&item[i]); + ASSERT_FALSE(download_discarded_callback[i].is_null()); + } + EXPECT_EQ(0U, num_feedbacks()); + + for (size_t i = 0; i < num_downloads; ++i) { + download_discarded_callback[i].Run(file_path[i]); + } + + ASSERT_EQ(3U, num_feedbacks()); + EXPECT_TRUE(feedback(0)->start_called()); + EXPECT_FALSE(feedback(1)->start_called()); + EXPECT_FALSE(feedback(2)->start_called()); + + feedback(0)->finish_callback().Run(); + + EXPECT_FALSE(feedback(0)); + EXPECT_TRUE(feedback(1)->start_called()); + EXPECT_FALSE(feedback(2)->start_called()); + + feedback(1)->finish_callback().Run(); + + EXPECT_FALSE(feedback(0)); + EXPECT_FALSE(feedback(1)); + EXPECT_TRUE(feedback(2)->start_called()); + + feedback(2)->finish_callback().Run(); + + EXPECT_FALSE(feedback(0)); + EXPECT_FALSE(feedback(1)); + EXPECT_FALSE(feedback(2)); + } + + base::RunLoop().RunUntilIdle(); + // These files should still exist since the FakeDownloadFeedback does not + // delete them. + EXPECT_TRUE(file_util::PathExists(file_path[0])); + EXPECT_TRUE(file_util::PathExists(file_path[1])); + EXPECT_TRUE(file_util::PathExists(file_path[2])); +} + +TEST_F(DownloadFeedbackServiceTest, MultiFeedbackWithIncomplete) { + const std::string ping_request = "ping"; + const std::string ping_response = "resp"; + const size_t num_downloads = 3; + + content::DownloadItem::AcquireFileCallback + download_discarded_callback[num_downloads]; + + base::FilePath file_path[num_downloads]; + content::MockDownloadItem item[num_downloads]; + for (size_t i = 0; i < num_downloads; ++i) { + file_path[i] = CreateTestFile(i); + EXPECT_CALL(item[i], GetDangerType()) + .WillRepeatedly(Return(content::DOWNLOAD_DANGER_TYPE_UNCOMMON_CONTENT)); + EXPECT_CALL(item[i], GetReceivedBytes()).WillRepeatedly(Return(1000)); + EXPECT_CALL(item[i], StealDangerousDownload(_)) + .WillOnce(SaveArg<0>(&download_discarded_callback[i])); + DownloadFeedbackService::MaybeStorePingsForDownload( + DownloadProtectionService::UNCOMMON, &item[i], ping_request, + ping_response); + ASSERT_TRUE(DownloadFeedbackService::IsEnabledForDownload(item[i])); + } + + { + DownloadFeedbackService service(request_context_getter_, file_task_runner_); + for (size_t i = 0; i < num_downloads; ++i) { + SCOPED_TRACE(i); + service.BeginFeedbackForDownload(&item[i]); + ASSERT_FALSE(download_discarded_callback[i].is_null()); + } + EXPECT_EQ(0U, num_feedbacks()); + + download_discarded_callback[0].Run(file_path[0]); + ASSERT_EQ(1U, num_feedbacks()); + ASSERT_TRUE(feedback(0)); + EXPECT_TRUE(feedback(0)->start_called()); + + download_discarded_callback[1].Run(file_path[1]); + ASSERT_EQ(2U, num_feedbacks()); + ASSERT_TRUE(feedback(1)); + EXPECT_FALSE(feedback(1)->start_called()); + + feedback(0)->finish_callback().Run(); + EXPECT_FALSE(feedback(0)); + EXPECT_TRUE(feedback(1)->start_called()); + } + + EXPECT_EQ(2U, num_feedbacks()); + for (size_t i = 0; i < num_feedbacks(); ++i) { + SCOPED_TRACE(i); + EXPECT_FALSE(feedback(i)); + } + + // Running a download acquired callback after the DownloadFeedbackService is + // destroyed should delete the file. + download_discarded_callback[2].Run(file_path[2]); + EXPECT_EQ(2U, num_feedbacks()); + + // File should still exist since the FileUtilProxy task hasn't run yet. + EXPECT_TRUE(file_util::PathExists(file_path[2])); + + base::RunLoop().RunUntilIdle(); + // File should be deleted since the AcquireFileCallback ran after the service + // was deleted. + EXPECT_FALSE(file_util::PathExists(file_path[2])); + + // These files should still exist since the FakeDownloadFeedback does not + // delete them. + EXPECT_TRUE(file_util::PathExists(file_path[0])); + EXPECT_TRUE(file_util::PathExists(file_path[1])); +} + +} // namespace safe_browsing diff --git a/chrome/browser/safe_browsing/download_feedback_unittest.cc b/chrome/browser/safe_browsing/download_feedback_unittest.cc new file mode 100644 index 0000000..4a83868 --- /dev/null +++ b/chrome/browser/safe_browsing/download_feedback_unittest.cc @@ -0,0 +1,242 @@ +// Copyright 2013 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "chrome/browser/safe_browsing/download_feedback.h" + +#include "base/command_line.h" +#include "base/file_util.h" +#include "base/files/scoped_temp_dir.h" +#include "base/message_loop.h" +#include "base/run_loop.h" +#include "base/single_thread_task_runner.h" +#include "chrome/browser/safe_browsing/two_phase_uploader.h" +#include "chrome/common/chrome_switches.h" +#include "chrome/common/safe_browsing/csd.pb.h" +#include "content/public/browser/browser_thread.h" +#include "content/public/test/test_browser_thread_bundle.h" +#include "net/url_request/url_request_test_util.h" +#include "testing/gtest/include/gtest/gtest.h" + +namespace safe_browsing { + +namespace { + +class FakeUploader : public TwoPhaseUploader { + public: + FakeUploader(net::URLRequestContextGetter* url_request_context_getter, + base::TaskRunner* file_task_runner, + const GURL& base_url, + const std::string& metadata, + const base::FilePath& file_path, + const ProgressCallback& progress_callback, + const FinishCallback& finish_callback); + virtual ~FakeUploader() {} + + virtual void Start() OVERRIDE { + start_called_ = true; + } + + scoped_refptr<net::URLRequestContextGetter> url_request_context_getter_; + scoped_refptr<base::TaskRunner> file_task_runner_; + GURL base_url_; + std::string metadata_; + base::FilePath file_path_; + ProgressCallback progress_callback_; + FinishCallback finish_callback_; + + bool start_called_; +}; + +FakeUploader::FakeUploader( + net::URLRequestContextGetter* url_request_context_getter, + base::TaskRunner* file_task_runner, + const GURL& base_url, + const std::string& metadata, + const base::FilePath& file_path, + const ProgressCallback& progress_callback, + const FinishCallback& finish_callback) + : url_request_context_getter_(url_request_context_getter), + file_task_runner_(file_task_runner), + base_url_(base_url), + metadata_(metadata), + file_path_(file_path), + progress_callback_(progress_callback), + finish_callback_(finish_callback), + start_called_(false) { +} + +class FakeUploaderFactory : public TwoPhaseUploaderFactory { + public: + FakeUploaderFactory() : uploader_(NULL) {} + virtual ~FakeUploaderFactory() {} + + virtual TwoPhaseUploader* CreateTwoPhaseUploader( + net::URLRequestContextGetter* url_request_context_getter, + base::TaskRunner* file_task_runner, + const GURL& base_url, + const std::string& metadata, + const base::FilePath& file_path, + const TwoPhaseUploader::ProgressCallback& progress_callback, + const TwoPhaseUploader::FinishCallback& finish_callback) OVERRIDE; + + FakeUploader* uploader_; +}; + +TwoPhaseUploader* FakeUploaderFactory::CreateTwoPhaseUploader( + net::URLRequestContextGetter* url_request_context_getter, + base::TaskRunner* file_task_runner, + const GURL& base_url, + const std::string& metadata, + const base::FilePath& file_path, + const TwoPhaseUploader::ProgressCallback& progress_callback, + const TwoPhaseUploader::FinishCallback& finish_callback) { + EXPECT_FALSE(uploader_); + + uploader_ = new FakeUploader(url_request_context_getter, file_task_runner, + base_url, metadata, file_path, progress_callback, + finish_callback); + return uploader_; +} + +} // namespace + +class DownloadFeedbackTest : public testing::Test { + public: + DownloadFeedbackTest() + : file_task_runner_(content::BrowserThread::GetMessageLoopProxyForThread( + content::BrowserThread::FILE)), + io_task_runner_(content::BrowserThread::GetMessageLoopProxyForThread( + content::BrowserThread::IO)), + url_request_context_getter_( + new net::TestURLRequestContextGetter(io_task_runner_)), + feedback_finish_called_(false) { + EXPECT_NE(io_task_runner_, file_task_runner_); + } + + virtual void SetUp() OVERRIDE { + ASSERT_TRUE(temp_dir_.CreateUniqueTempDir()); + upload_file_path_ = temp_dir_.path().AppendASCII("test file"); + upload_file_data_ = "data"; + ASSERT_EQ(static_cast<int>(upload_file_data_.size()), + file_util::WriteFile(upload_file_path_, upload_file_data_.data(), + upload_file_data_.size())); + TwoPhaseUploader::RegisterFactory(&two_phase_uploader_factory_); + } + + virtual void TearDown() OVERRIDE { + TwoPhaseUploader::RegisterFactory(NULL); + } + + FakeUploader* uploader() const { + return two_phase_uploader_factory_.uploader_; + } + + void FinishCallback(DownloadFeedback* feedback) { + EXPECT_FALSE(feedback_finish_called_); + feedback_finish_called_ = true; + delete feedback; + } + + protected: + base::ScopedTempDir temp_dir_; + base::FilePath upload_file_path_; + std::string upload_file_data_; + content::TestBrowserThreadBundle thread_bundle_; + scoped_refptr<base::SingleThreadTaskRunner> file_task_runner_; + scoped_refptr<base::SingleThreadTaskRunner> io_task_runner_; + FakeUploaderFactory two_phase_uploader_factory_; + scoped_refptr<net::TestURLRequestContextGetter> url_request_context_getter_; + + bool feedback_finish_called_; +}; + +TEST_F(DownloadFeedbackTest, CompleteUpload) { + ClientDownloadReport expected_report_metadata; + expected_report_metadata.mutable_download_request()->set_url("http://test"); + expected_report_metadata.mutable_download_request()->set_length( + upload_file_data_.size()); + expected_report_metadata.mutable_download_request()->mutable_digests( + )->set_sha1("hi"); + expected_report_metadata.mutable_download_response()->set_verdict( + ClientDownloadResponse::DANGEROUS_HOST); + std::string ping_request( + expected_report_metadata.download_request().SerializeAsString()); + std::string ping_response( + expected_report_metadata.download_response().SerializeAsString()); + + const char kTestFeedbackURL[] = "https://example.com/test/upload"; + CommandLine::ForCurrentProcess()->AppendSwitchASCII( + switches::kSbDownloadFeedbackURL, kTestFeedbackURL); + + DownloadFeedback* feedback = DownloadFeedback::Create( + url_request_context_getter_, + file_task_runner_, + upload_file_path_, + ping_request, + ping_response); + EXPECT_FALSE(uploader()); + + feedback->Start(base::Bind(&DownloadFeedbackTest::FinishCallback, + base::Unretained(this), + feedback)); + ASSERT_TRUE(uploader()); + EXPECT_FALSE(feedback_finish_called_); + EXPECT_TRUE(uploader()->start_called_); + + EXPECT_EQ(url_request_context_getter_, + uploader()->url_request_context_getter_); + EXPECT_EQ(file_task_runner_, uploader()->file_task_runner_); + EXPECT_EQ(upload_file_path_, uploader()->file_path_); + EXPECT_EQ(expected_report_metadata.SerializeAsString(), + uploader()->metadata_); + EXPECT_EQ(kTestFeedbackURL, uploader()->base_url_.spec()); + + EXPECT_TRUE(file_util::PathExists(upload_file_path_)); + + EXPECT_FALSE(feedback_finish_called_); + uploader()->finish_callback_.Run( + TwoPhaseUploader::STATE_SUCCESS, net::OK, 0, ""); + EXPECT_TRUE(feedback_finish_called_); + base::RunLoop().RunUntilIdle(); + EXPECT_FALSE(file_util::PathExists(upload_file_path_)); +} + +TEST_F(DownloadFeedbackTest, CancelUpload) { + ClientDownloadReport expected_report_metadata; + expected_report_metadata.mutable_download_request()->set_url("http://test"); + expected_report_metadata.mutable_download_request()->set_length( + upload_file_data_.size()); + expected_report_metadata.mutable_download_request()->mutable_digests( + )->set_sha1("hi"); + expected_report_metadata.mutable_download_response()->set_verdict( + ClientDownloadResponse::DANGEROUS_HOST); + std::string ping_request( + expected_report_metadata.download_request().SerializeAsString()); + std::string ping_response( + expected_report_metadata.download_response().SerializeAsString()); + + DownloadFeedback* feedback = DownloadFeedback::Create( + url_request_context_getter_, + file_task_runner_, + upload_file_path_, + ping_request, + ping_response); + EXPECT_FALSE(uploader()); + + feedback->Start(base::Bind(&DownloadFeedbackTest::FinishCallback, + base::Unretained(this), + feedback)); + ASSERT_TRUE(uploader()); + EXPECT_FALSE(feedback_finish_called_); + EXPECT_TRUE(uploader()->start_called_); + EXPECT_TRUE(file_util::PathExists(upload_file_path_)); + + delete feedback; + EXPECT_FALSE(feedback_finish_called_); + + base::RunLoop().RunUntilIdle(); + EXPECT_FALSE(file_util::PathExists(upload_file_path_)); +} + +} // namespace safe_browsing diff --git a/chrome/browser/safe_browsing/download_protection_service.cc b/chrome/browser/safe_browsing/download_protection_service.cc index 901d5e9..9fe83807 100644 --- a/chrome/browser/safe_browsing/download_protection_service.cc +++ b/chrome/browser/safe_browsing/download_protection_service.cc @@ -17,6 +17,7 @@ #include "base/strings/stringprintf.h" #include "base/threading/sequenced_worker_pool.h" #include "base/time.h" +#include "chrome/browser/safe_browsing/download_feedback_service.h" #include "chrome/browser/safe_browsing/safe_browsing_service.h" #include "chrome/browser/safe_browsing/sandboxed_zip_analyzer.h" #include "chrome/browser/safe_browsing/signature_util.h" @@ -421,6 +422,8 @@ class DownloadProtectionService::CheckClientDownloadRequest << response.verdict(); reason = REASON_INVALID_RESPONSE_VERDICT; } + DownloadFeedbackService::MaybeStorePingsForDownload( + result, item_, client_download_request_data_, data); } // We don't need the fetcher anymore. fetcher_.reset(); @@ -636,8 +639,7 @@ class DownloadProtectionService::CheckClientDownloadRequest item_->GetTargetFilePath().BaseName().AsUTF8Unsafe()); request.set_download_type(type_); request.mutable_signature()->CopyFrom(signature_info_); - std::string request_data; - if (!request.SerializeToString(&request_data)) { + if (!request.SerializeToString(&client_download_request_data_)) { FinishRequest(SAFE, REASON_INVALID_REQUEST_PROTO); return; } @@ -651,7 +653,8 @@ class DownloadProtectionService::CheckClientDownloadRequest 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_->SetUploadData("application/octet-stream", + client_download_request_data_); fetcher_->Start(); } @@ -754,6 +757,7 @@ class DownloadProtectionService::CheckClientDownloadRequest base::TimeTicks zip_analysis_start_time_; bool finished_; ClientDownloadRequest::DownloadType type_; + std::string client_download_request_data_; base::WeakPtrFactory<CheckClientDownloadRequest> weakptr_factory_; base::TimeTicks start_time_; // Used for stats. @@ -766,7 +770,9 @@ DownloadProtectionService::DownloadProtectionService( : request_context_getter_(request_context_getter), enabled_(false), signature_util_(new SignatureUtil()), - download_request_timeout_ms_(kDownloadRequestTimeoutMs) { + download_request_timeout_ms_(kDownloadRequestTimeoutMs), + feedback_service_(new DownloadFeedbackService( + request_context_getter, BrowserThread::GetBlockingPool())) { if (sb_service) { ui_manager_ = sb_service->ui_manager(); diff --git a/chrome/browser/safe_browsing/download_protection_service.h b/chrome/browser/safe_browsing/download_protection_service.h index d7407b2..045bb2f 100644 --- a/chrome/browser/safe_browsing/download_protection_service.h +++ b/chrome/browser/safe_browsing/download_protection_service.h @@ -33,6 +33,7 @@ class X509Certificate; } // namespace net namespace safe_browsing { +class DownloadFeedbackService; class SignatureUtil; // This class provides an asynchronous API to check whether a particular @@ -100,6 +101,10 @@ class DownloadProtectionService { return download_request_timeout_ms_; } + DownloadFeedbackService* feedback_service() { + return feedback_service_.get(); + } + protected: // Enum to keep track why a particular download verdict was chosen. // This is used to keep some stats around. @@ -186,6 +191,8 @@ class DownloadProtectionService { int64 download_request_timeout_ms_; + scoped_ptr<DownloadFeedbackService> feedback_service_; + DISALLOW_COPY_AND_ASSIGN(DownloadProtectionService); }; } // namespace safe_browsing diff --git a/chrome/browser/safe_browsing/download_protection_service_unittest.cc b/chrome/browser/safe_browsing/download_protection_service_unittest.cc index e693429..e5baf4c 100644 --- a/chrome/browser/safe_browsing/download_protection_service_unittest.cc +++ b/chrome/browser/safe_browsing/download_protection_service_unittest.cc @@ -10,6 +10,7 @@ #include "base/base_paths.h" #include "base/bind.h" #include "base/callback.h" +#include "base/command_line.h" #include "base/file_util.h" #include "base/files/file_path.h" #include "base/files/scoped_temp_dir.h" @@ -20,8 +21,10 @@ #include "base/strings/string_number_conversions.h" #include "base/threading/sequenced_worker_pool.h" #include "chrome/browser/safe_browsing/database_manager.h" +#include "chrome/browser/safe_browsing/download_feedback_service.h" #include "chrome/browser/safe_browsing/safe_browsing_service.h" #include "chrome/browser/safe_browsing/signature_util.h" +#include "chrome/common/chrome_switches.h" #include "chrome/common/safe_browsing/csd.pb.h" #include "content/public/test/mock_download_item.h" #include "content/public/test/test_browser_thread.h" @@ -146,6 +149,8 @@ ACTION_P(CheckDownloadUrlDone, threat_type) { class DownloadProtectionServiceTest : public testing::Test { protected: virtual void SetUp() { + CommandLine::ForCurrentProcess()->AppendSwitch( + switches::kSbEnableDownloadFeedback); ui_thread_.reset(new content::TestBrowserThread(BrowserThread::UI, &msg_loop_)); // Start real threads for the IO and File threads so that the DCHECKs @@ -485,6 +490,10 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadSuccess) { base::Unretained(this))); msg_loop_.Run(); EXPECT_TRUE(IsResult(DownloadProtectionService::SAFE)); + std::string feedback_ping; + std::string feedback_response; + EXPECT_FALSE(DownloadFeedbackService::GetPingsForDownloadForTesting( + item, &feedback_ping, &feedback_response)); // If the response is dangerous the result should also be marked as dangerous. response.set_verdict(ClientDownloadResponse::DANGEROUS); @@ -498,6 +507,8 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadSuccess) { base::Bind(&DownloadProtectionServiceTest::CheckDoneCallback, base::Unretained(this))); msg_loop_.Run(); + EXPECT_FALSE(DownloadFeedbackService::GetPingsForDownloadForTesting( + item, &feedback_ping, &feedback_response)); #if defined(OS_WIN) EXPECT_TRUE(IsResult(DownloadProtectionService::DANGEROUS)); #else @@ -518,6 +529,12 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadSuccess) { msg_loop_.Run(); #if defined(OS_WIN) EXPECT_TRUE(IsResult(DownloadProtectionService::UNCOMMON)); + EXPECT_TRUE(DownloadFeedbackService::GetPingsForDownloadForTesting( + item, &feedback_ping, &feedback_response)); + ClientDownloadRequest decoded_request; + EXPECT_TRUE(decoded_request.ParseFromString(feedback_ping)); + EXPECT_EQ(url_chain.back().spec(), decoded_request.url()); + EXPECT_EQ(response.SerializeAsString(), feedback_response); #else EXPECT_TRUE(IsResult(DownloadProtectionService::SAFE)); #endif @@ -537,6 +554,9 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadSuccess) { msg_loop_.Run(); #if defined(OS_WIN) EXPECT_TRUE(IsResult(DownloadProtectionService::DANGEROUS_HOST)); + EXPECT_TRUE(DownloadFeedbackService::GetPingsForDownloadForTesting( + item, &feedback_ping, &feedback_response)); + EXPECT_EQ(response.SerializeAsString(), feedback_response); #else EXPECT_TRUE(IsResult(DownloadProtectionService::SAFE)); #endif diff --git a/chrome/browser/safe_browsing/two_phase_uploader.cc b/chrome/browser/safe_browsing/two_phase_uploader.cc index c5558d1..6fb3dc4 100644 --- a/chrome/browser/safe_browsing/two_phase_uploader.cc +++ b/chrome/browser/safe_browsing/two_phase_uploader.cc @@ -153,7 +153,7 @@ void TwoPhaseUploaderImpl::OnURLFetchUploadProgress( DCHECK(CalledOnValidThread()); DVLOG(3) << __FUNCTION__ << " " << source->GetURL().spec() << " " << current << "/" << total; - if (state_ == UPLOAD_FILE) + if (state_ == UPLOAD_FILE && !progress_callback_.is_null()) progress_callback_.Run(current, total); } diff --git a/chrome/browser/safe_browsing/two_phase_uploader.h b/chrome/browser/safe_browsing/two_phase_uploader.h index 8aced0a..c10cf7b 100644 --- a/chrome/browser/safe_browsing/two_phase_uploader.h +++ b/chrome/browser/safe_browsing/two_phase_uploader.h @@ -54,7 +54,7 @@ class TwoPhaseUploader : public base::NonThreadSafe { // The first phase request will be sent to |base_url|, with |metadata| // included. // |progress_callback| will be called periodically as the second phase - // progresses. + // progresses, if it is non-null. // On success |finish_callback| will be called with state = STATE_SUCCESS and // the server response in response_data. On failure, state will specify // which step the failure occurred in, and net_error, response_code, and diff --git a/chrome/browser/ui/views/download/download_item_view.cc b/chrome/browser/ui/views/download/download_item_view.cc index 9b7f385..032d83c 100644 --- a/chrome/browser/ui/views/download/download_item_view.cc +++ b/chrome/browser/ui/views/download/download_item_view.cc @@ -21,6 +21,9 @@ #include "chrome/browser/download/chrome_download_manager_delegate.h" #include "chrome/browser/download/download_item_model.h" #include "chrome/browser/download/download_util.h" +#include "chrome/browser/safe_browsing/download_feedback_service.h" +#include "chrome/browser/safe_browsing/download_protection_service.h" +#include "chrome/browser/safe_browsing/safe_browsing_service.h" #include "chrome/browser/themes/theme_properties.h" #include "chrome/browser/ui/views/download/download_shelf_context_menu_view.h" #include "chrome/browser/ui/views/download/download_shelf_view.h" @@ -524,6 +527,8 @@ void DownloadItemView::ShowContextMenuForView(View* source, void DownloadItemView::ButtonPressed( views::Button* sender, const ui::Event& event) { if (sender == discard_button_) { + if (model_.ShouldAllowDownloadFeedback() && BeginDownloadFeedback()) + return; UMA_HISTOGRAM_LONG_TIMES("clickjacking.discard_download", base::Time::Now() - creation_time_); download()->Remove(); @@ -842,6 +847,22 @@ void DownloadItemView::OpenDownload() { UpdateAccessibleName(); } +bool DownloadItemView::BeginDownloadFeedback() { + SafeBrowsingService* sb_service = g_browser_process->safe_browsing_service(); + if (!sb_service) + return false; + safe_browsing::DownloadProtectionService* download_protection_service = + sb_service->download_protection_service(); + if (!download_protection_service) + return false; + UMA_HISTOGRAM_LONG_TIMES("clickjacking.report_and_discard_download", + base::Time::Now() - creation_time_); + download_protection_service->feedback_service()->BeginFeedbackForDownload( + download()); + // WARNING: we are deleted at this point. Don't access 'this'. + return true; +} + void DownloadItemView::LoadIcon() { IconManager* im = g_browser_process->icon_manager(); last_download_item_path_ = download()->GetTargetFilePath(); @@ -1049,8 +1070,15 @@ void DownloadItemView::ShowWarningDialog() { save_button_->SetStyle(views::Button::STYLE_NATIVE_TEXTBUTTON); AddChildView(save_button_); } - discard_button_ = new views::LabelButton( - this, l10n_util::GetStringUTF16(IDS_DISCARD_DOWNLOAD)); + if (model_.ShouldAllowDownloadFeedback()) { + safe_browsing::DownloadFeedbackService::RecordFeedbackButtonShown( + download()->GetDangerType()); + discard_button_ = new views::LabelButton( + this, l10n_util::GetStringUTF16(IDS_REPORT_AND_DISCARD_DOWNLOAD)); + } else { + discard_button_ = new views::LabelButton( + this, l10n_util::GetStringUTF16(IDS_DISCARD_DOWNLOAD)); + } discard_button_->SetStyle(views::Button::STYLE_NATIVE_TEXTBUTTON); AddChildView(discard_button_); diff --git a/chrome/browser/ui/views/download/download_item_view.h b/chrome/browser/ui/views/download/download_item_view.h index 47f2684..1c64923 100644 --- a/chrome/browser/ui/views/download/download_item_view.h +++ b/chrome/browser/ui/views/download/download_item_view.h @@ -146,6 +146,11 @@ class DownloadItemView : public views::ButtonListener, void OpenDownload(); + // Submit the downloaded file to the safebrowsing download feedback service. + // If true is returned, the DownloadItem and |this| have been deleted. If + // false is returned, nothing has changed. + bool BeginDownloadFeedback(); + void LoadIcon(); void LoadIconIfItemPathChanged(); diff --git a/chrome/chrome_browser.gypi b/chrome/chrome_browser.gypi index 668b682..e279a7b 100644 --- a/chrome/chrome_browser.gypi +++ b/chrome/chrome_browser.gypi @@ -1697,6 +1697,10 @@ 'browser/safe_browsing/client_side_detection_service.h', 'browser/safe_browsing/database_manager.cc', 'browser/safe_browsing/database_manager.h', + 'browser/safe_browsing/download_feedback.cc', + 'browser/safe_browsing/download_feedback.h', + 'browser/safe_browsing/download_feedback_service.cc', + 'browser/safe_browsing/download_feedback_service.h', 'browser/safe_browsing/download_protection_service.cc', 'browser/safe_browsing/download_protection_service.h', 'browser/safe_browsing/malware_details.cc', diff --git a/chrome/chrome_tests_unit.gypi b/chrome/chrome_tests_unit.gypi index 50d0632..e9383e3 100644 --- a/chrome/chrome_tests_unit.gypi +++ b/chrome/chrome_tests_unit.gypi @@ -1085,6 +1085,8 @@ 'browser/safe_browsing/chunk_range_unittest.cc', 'browser/safe_browsing/client_side_detection_host_unittest.cc', 'browser/safe_browsing/client_side_detection_service_unittest.cc', + 'browser/safe_browsing/download_feedback_service_unittest.cc', + 'browser/safe_browsing/download_feedback_unittest.cc', 'browser/safe_browsing/download_protection_service_unittest.cc', 'browser/safe_browsing/local_two_phase_testserver.cc', 'browser/safe_browsing/malware_details_unittest.cc', diff --git a/chrome/common/chrome_switches.cc b/chrome/common/chrome_switches.cc index 33694c5..706c1e7 100644 --- a/chrome/common/chrome_switches.cc +++ b/chrome/common/chrome_switches.cc @@ -1248,6 +1248,13 @@ const char kSbDisableExtensionBlacklist[] = const char kSbDisableSideEffectFreeWhitelist[] = "safebrowsing-disable-side-effect-free-whitelist"; +// URL to send safebrowsing download feedback reports to. +const char kSbDownloadFeedbackURL[] = "safebrowsing-download-feedback-url"; + +// Enable safebrowsing download feedback. +const char kSbEnableDownloadFeedback[] = + "safebrowsing-enable-download-feedback"; + // Enables or disables extension scripts badges in the location bar. const char kScriptBadges[] = "script-badges"; diff --git a/chrome/common/chrome_switches.h b/chrome/common/chrome_switches.h index 84629d9..083b3d8 100644 --- a/chrome/common/chrome_switches.h +++ b/chrome/common/chrome_switches.h @@ -329,6 +329,8 @@ extern const char kSbDisableAutoUpdate[]; extern const char kSbDisableDownloadProtection[]; extern const char kSbDisableExtensionBlacklist[]; extern const char kSbDisableSideEffectFreeWhitelist[]; +extern const char kSbDownloadFeedbackURL[]; +extern const char kSbEnableDownloadFeedback[]; extern const char kScriptBadges[]; extern const char kScriptBubble[]; extern const char kServiceProcess[]; diff --git a/tools/metrics/histograms/histograms.xml b/tools/metrics/histograms/histograms.xml index 186501c..b66fc5d 100644 --- a/tools/metrics/histograms/histograms.xml +++ b/tools/metrics/histograms/histograms.xml @@ -941,6 +941,13 @@ other types of suffix sets. <summary>TBD</summary> </histogram> +<histogram name="clickjacking.report_and_discard_download" units="ms"> + <summary> + Time between "Report and Discard" button being shown and it being + clicked. + </summary> +</histogram> + <histogram name="clickjacking.save_download"> <summary>TBD</summary> </histogram> @@ -10581,6 +10588,53 @@ other types of suffix sets. </summary> </histogram> +<histogram name="SBDownloadFeedback.Activations" enum="DownloadItem.DangerType"> + <summary> + Count of times download feedback has been started, broken down by danger + type. + </summary> +</histogram> + +<histogram name="SBDownloadFeedback.ActiveFeedbacks"> + <summary> + When a new download feedback request is added, records the number of + download requests currently active and/or pending. + </summary> +</histogram> + +<histogram name="SBDownloadFeedback.Shown" enum="DownloadItem.DangerType"> + <summary> + Count of times download feedback button has been shown, broken down by + danger type. + </summary> +</histogram> + +<histogram name="SBDownloadFeedback.SizeEligibleKB" units="KB"> + <summary> + Size of downloads that were of the correct danger type, regardless if they + meet the max file size check or if they are actually uploaded or not. + </summary> +</histogram> + +<histogram name="SBDownloadFeedback.SizeFailure" units="bytes"> + <summary> + Size of downloads that failed to be uploaded to the feedback service. + </summary> +</histogram> + +<histogram name="SBDownloadFeedback.SizeSuccess" units="bytes"> + <summary> + Size of downloads that were successfully uploaded to the feedback service. + </summary> +</histogram> + +<histogram name="SBDownloadFeedback.UploadResult" + enum="SBDownloadFeedbackUploadResult"> + <summary> + Final result of attempt to upload binary to download feedback service. + </summary> +</histogram> + <histogram name="Search.DefaultSearchProvider" enum="OmniboxSearchEngine"> <summary> The id of the default search engine that is loaded after Chrome startup. See @@ -17223,6 +17277,17 @@ other types of suffix sets. <int value="5" label="MODEL_MISSING_FIELDS"/> </enum> +<enum name="SBDownloadFeedbackUploadResult" type="int"> + <int value="0" label="SUCCESS"/> + <int value="1" label="UPLOAD_SUCCESS"/> + <int value="2" label="UPLOAD_CANCELLED"/> + <int value="3" label="UPLOAD_METADATA_NET_ERROR"/> + <int value="4" label="UPLOAD_METADATA_RESPONSE_ERROR"/> + <int value="5" label="UPLOAD_FILE_NET_ERROR"/> + <int value="6" label="UPLOAD_FILE_RESPONSE_ERROR"/> + <int value="7" label="UPLOAD_COMPLETE_RESPONSE_ERROR"/> +</enum> + <enum name="ShillTerminationActionResult" type="int"> <summary> The termination action result types come from TerminationActionResult in |