diff options
author | lzheng@chromium.org <lzheng@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2010-12-22 21:07:05 +0000 |
---|---|---|
committer | lzheng@chromium.org <lzheng@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2010-12-22 21:07:05 +0000 |
commit | 9fc1e6c183707ebde09cb0057f528646f309b32d (patch) | |
tree | 4e45db2ebbca3aea8dd214855566ff8f0fc920c5 | |
parent | 6eb56b870387d485303fd0c5f08f1849df4561a3 (diff) | |
download | chromium_src-9fc1e6c183707ebde09cb0057f528646f309b32d.zip chromium_src-9fc1e6c183707ebde09cb0057f528646f309b32d.tar.gz chromium_src-9fc1e6c183707ebde09cb0057f528646f309b32d.tar.bz2 |
Add support for safebrowsing download protection. It is currently under flag --safebrowsing-download-protection.
TEST=safe_browsing_service_browsertest.cc
BUG=60822
Review URL: http://codereview.chromium.org/5141006
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@69979 0039d316-1c4b-4281-b951-d872f2087c98
5 files changed, 227 insertions, 36 deletions
diff --git a/chrome/browser/renderer_host/download_resource_handler.cc b/chrome/browser/renderer_host/download_resource_handler.cc index 6e34a30..f806406 100644 --- a/chrome/browser/renderer_host/download_resource_handler.cc +++ b/chrome/browser/renderer_host/download_resource_handler.cc @@ -7,6 +7,8 @@ #include <string> #include "base/logging.h" +#include "base/metrics/histogram.h" +#include "base/metrics/stats_counters.h" #include "base/stringprintf.h" #include "chrome/browser/browser_thread.h" #include "chrome/browser/download/download_item.h" @@ -41,7 +43,8 @@ DownloadResourceHandler::DownloadResourceHandler( save_info_(save_info), buffer_(new DownloadBuffer), rdh_(rdh), - is_paused_(false) { + is_paused_(false), + url_check_pending_(false) { } bool DownloadResourceHandler::OnUploadProgress(int request_id, @@ -59,11 +62,55 @@ bool DownloadResourceHandler::OnRequestRedirected(int request_id, return true; } +// Callback when the result of checking a download URL is known. +// TODO(lzheng): We should create a information bar with buttons to ask +// if users want to proceed when the download is malicious. +void DownloadResourceHandler::OnDownloadUrlCheckResult( + const GURL& url, SafeBrowsingService::UrlCheckResult result) { + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); + DCHECK(url_check_pending_); + + UMA_HISTOGRAM_TIMES("SB2.DownloadUrlCheckDuration", + base::TimeTicks::Now() - download_start_time_); + + if (result == SafeBrowsingService::BINARY_MALWARE) { + // TODO(lzheng): More UI work to show warnings properly on download shelf. + DLOG(WARNING) << "This url leads to a malware downloading: " + << url.spec(); + UpdateDownloadUrlCheckStats(DOWNLOAD_URL_CHECKS_MALWARE); + } + + url_check_pending_ = false; + // Note: Release() should be the last line in this call. It is for + // the AddRef in CheckSafeBrowsing. + Release(); +} + +// Send the download creation information to the download thread. +void DownloadResourceHandler::StartDownloadUrlCheck() { + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); + AddRef(); + if (!rdh_->safe_browsing_service()->CheckDownloadUrl(url_, this)) { + url_check_pending_ = true; + UpdateDownloadUrlCheckStats(DOWNLOAD_URL_CHECKS_TOTAL); + // Note: in this case, the AddRef will be balanced in + // "OnDownloadUrlCheckResult" or "OnRequestClosed". + } else { + // Immediately release the AddRef() at the beginning of this function + // since no more callbacks will happen. + Release(); + DVLOG(1) << "url: " << url_.spec() << " is safe to download."; + } +} + // Send the download creation information to the download thread. bool DownloadResourceHandler::OnResponseStarted(int request_id, ResourceResponse* response) { VLOG(20) << __FUNCTION__ << "()" << DebugString() << " request_id = " << request_id; + DCHECK(!url_check_pending_); + download_start_time_ = base::TimeTicks::Now(); + StartDownloadUrlCheck(); std::string content_disposition; request_->GetResponseHeaderByName("content-disposition", &content_disposition); @@ -183,6 +230,17 @@ bool DownloadResourceHandler::OnResponseCompleted( } void DownloadResourceHandler::OnRequestClosed() { + UMA_HISTOGRAM_TIMES("SB2.DownloadDuration", + base::TimeTicks::Now() - download_start_time_); + if (url_check_pending_) { + DVLOG(1) << "Cancel pending download url checking request: " << this; + rdh_->safe_browsing_service()->CancelCheck(this); + UpdateDownloadUrlCheckStats(DOWNLOAD_URL_CHECKS_CANCELED); + url_check_pending_ = false; + // Balance the AddRef() from StartDownloadUrlCheck() which would usually be + // balanced by OnDownloadUrlCheckResult(). + Release(); + } } // If the content-length header is not present (or contains something other @@ -249,3 +307,10 @@ std::string DownloadResourceHandler::DebugString() const { render_view_id_, save_info_.file_path.value().c_str()); } + +void DownloadResourceHandler::UpdateDownloadUrlCheckStats( + SBStatsType stat_type) { + UMA_HISTOGRAM_ENUMERATION("SB2.DownloadUrlChecks", + stat_type, + DOWNLOAD_URL_CHECKS_MAX); +} diff --git a/chrome/browser/renderer_host/download_resource_handler.h b/chrome/browser/renderer_host/download_resource_handler.h index 3fb6961..5a66f66 100644 --- a/chrome/browser/renderer_host/download_resource_handler.h +++ b/chrome/browser/renderer_host/download_resource_handler.h @@ -12,6 +12,7 @@ #include "chrome/browser/download/download_file.h" #include "chrome/browser/renderer_host/global_request_id.h" #include "chrome/browser/renderer_host/resource_handler.h" +#include "chrome/browser/safe_browsing/safe_browsing_service.h" class DownloadFileManager; class ResourceDispatcherHost; @@ -22,7 +23,8 @@ class URLRequest; } // namespace net // Forwards data to the download thread. -class DownloadResourceHandler : public ResourceHandler { +class DownloadResourceHandler : public ResourceHandler, + public SafeBrowsingService::Client { public: DownloadResourceHandler(ResourceDispatcherHost* rdh, int render_process_host_id, @@ -70,10 +72,31 @@ class DownloadResourceHandler : public ResourceHandler { std::string DebugString() const; private: + // Enumerate for histogramming purposes. DO NOT CHANGE THE + // ORDERING OF THESE VALUES. + enum SBStatsType { + DOWNLOAD_URL_CHECKS_TOTAL, + DOWNLOAD_URL_CHECKS_CANCELED, + DOWNLOAD_URL_CHECKS_MALWARE, + + // Memory space for histograms is determined by the max. ALWAYS + // ADD NEW VALUES BEFORE THIS ONE. + DOWNLOAD_URL_CHECKS_MAX + }; + ~DownloadResourceHandler(); void StartPauseTimer(); + void StartDownloadUrlCheck(); + + // Called when the result of checking a download URL is known. + void OnDownloadUrlCheckResult(const GURL& url, + SafeBrowsingService::UrlCheckResult result); + + // A helper function that updates UMA for download url checks. + static void UpdateDownloadUrlCheckStats(SBStatsType stat_type); + int download_id_; GlobalRequestID global_id_; int render_view_id_; @@ -89,6 +112,8 @@ class DownloadResourceHandler : public ResourceHandler { ResourceDispatcherHost* rdh_; bool is_paused_; base::OneShotTimer<DownloadResourceHandler> pause_timer_; + bool url_check_pending_; + base::TimeTicks download_start_time_; // used to collect stats. static const int kReadBufSize = 32768; // bytes static const size_t kLoadsToWrite = 100; // number of data buffers queued diff --git a/chrome/browser/safe_browsing/safe_browsing_service.cc b/chrome/browser/safe_browsing/safe_browsing_service.cc index 657778a..9e24b03 100644 --- a/chrome/browser/safe_browsing/safe_browsing_service.cc +++ b/chrome/browser/safe_browsing/safe_browsing_service.cc @@ -136,42 +136,35 @@ bool SafeBrowsingService::CanCheckUrl(const GURL& url) const { } void SafeBrowsingService::CheckDownloadUrlDone( - Client* client, const GURL& url, UrlCheckResult result) { + SafeBrowsingCheck* check, UrlCheckResult result) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); DCHECK(enable_download_protection_); VLOG(1) << "CheckDownloadUrlDone: " << result; - client->OnSafeBrowsingResult(url, result); + + if (checks_.find(check) == checks_.end() || !check->client) + return; + check->client->OnSafeBrowsingResult(check->url, result); + checks_.erase(check); } -void SafeBrowsingService::CheckDownloadUrlOnSBThread(const GURL& url, - Client* client) { +void SafeBrowsingService::CheckDownloadUrlOnSBThread(SafeBrowsingCheck* check) { DCHECK_EQ(MessageLoop::current(), safe_browsing_thread_->message_loop()); DCHECK(enable_download_protection_); std::vector<SBPrefix> prefix_hits; - if (!database_->ContainsDownloadUrl(url, &prefix_hits)) { + if (!database_->ContainsDownloadUrl(check->url, &prefix_hits)) { // Good, we don't have hash for this url prefix. BrowserThread::PostTask( BrowserThread::IO, FROM_HERE, NewRunnableMethod(this, &SafeBrowsingService::CheckDownloadUrlDone, - client, url, URL_SAFE)); + check, URL_SAFE)); return; } - // Now, we need to fetch the url from the safebrowsing backends. - // Needs to be asynchronous, since we could be in the constructor of a - // ResourceDispatcherHost event handler which can't pause there. - SafeBrowsingCheck* check = new SafeBrowsingCheck(); - - check->url = url; - check->client = client; - check->result = URL_SAFE; check->need_get_hash = true; check->prefix_hits.swap(prefix_hits); - checks_.insert(check); - BrowserThread::PostTask( BrowserThread::IO, FROM_HERE, NewRunnableMethod(this, &SafeBrowsingService::OnCheckDone, check)); @@ -183,8 +176,17 @@ bool SafeBrowsingService::CheckDownloadUrl(const GURL& url, if (!enabled_ || !enable_download_protection_) return true; + // We need to check the database for url prefix, and later may fetch the url + // from the safebrowsing backends. These need to be asynchronous. + SafeBrowsingCheck* check = new SafeBrowsingCheck(); + + check->url = url; + check->client = client; + check->result = URL_SAFE; + checks_.insert(check); safe_browsing_thread_->message_loop()->PostTask(FROM_HERE, NewRunnableMethod( - this, &SafeBrowsingService::CheckDownloadUrlOnSBThread, url, client)); + this, &SafeBrowsingService::CheckDownloadUrlOnSBThread, check)); + return false; } @@ -702,8 +704,7 @@ void SafeBrowsingService::DatabaseLoadComplete() { } void SafeBrowsingService::HandleChunkForDatabase( - const std::string& list_name, - SBChunkList* chunks) { + const std::string& list_name, SBChunkList* chunks) { DCHECK_EQ(MessageLoop::current(), safe_browsing_thread_->message_loop()); if (chunks) { GetDatabase()->InsertChunks(list_name, *chunks); diff --git a/chrome/browser/safe_browsing/safe_browsing_service.h b/chrome/browser/safe_browsing/safe_browsing_service.h index abade47..e87fc9c 100644 --- a/chrome/browser/safe_browsing/safe_browsing_service.h +++ b/chrome/browser/safe_browsing/safe_browsing_service.h @@ -323,11 +323,10 @@ class SafeBrowsingService // Invoked by CheckDownloadUrl. It checks the download URL on // safe_browsing_thread_. - void CheckDownloadUrlOnSBThread(const GURL& url, Client* client); + void CheckDownloadUrlOnSBThread(SafeBrowsingCheck* check); // Call the Client's callback in IO thread after CheckDownloadUrl finishes. - void CheckDownloadUrlDone(Client* client, const GURL& url, - UrlCheckResult result); + void CheckDownloadUrlDone(SafeBrowsingCheck* check, UrlCheckResult result); // The factory used to instanciate a SafeBrowsingService object. // Useful for tests, so they can provide their own implementation of diff --git a/chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc b/chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc index c495481..0b44a250 100644 --- a/chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc +++ b/chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc @@ -7,9 +7,12 @@ // service. #include "base/command_line.h" +#include "base/metrics/histogram.h" +#include "base/ref_counted.h" #include "base/sha2.h" #include "chrome/browser/browser_process.h" #include "chrome/browser/browser_thread.h" +#include "chrome/browser/renderer_host/resource_dispatcher_host.h" #include "chrome/browser/safe_browsing/protocol_manager.h" #include "chrome/browser/safe_browsing/safe_browsing_database.h" #include "chrome/browser/safe_browsing/safe_browsing_service.h" @@ -21,6 +24,9 @@ #include "chrome/test/in_process_browser_test.h" #include "chrome/test/ui_test_utils.h" +using base::Histogram; +using base::StatisticsRecorder; + namespace { // A SafeBrowingDatabase class that allows us to inject the malicious URLs. @@ -47,19 +53,17 @@ class TestSafeBrowsingDatabase : public SafeBrowsingDatabase { std::vector<SBPrefix>* prefix_hits, std::vector<SBFullHashResult>* full_hits, base::Time last_update) { - base::hash_map<std::string, Hits>::const_iterator - badurls_it = badurls_.find(url.spec()); - if (badurls_it == badurls_.end()) - return false; - *prefix_hits = badurls_it->second.prefix_hits; - *full_hits = badurls_it->second.full_hits; - return true; + return ContainsUrl(safe_browsing_util::kMalwareList, + safe_browsing_util::kPhishingList, + url, prefix_hits, full_hits); } virtual bool ContainsDownloadUrl(const GURL& url, std::vector<SBPrefix>* prefix_hits) { - ADD_FAILURE() << "Not implemented."; - return false; + std::vector<SBFullHashResult> full_hits; + return ContainsUrl(safe_browsing_util::kBinUrlList, + safe_browsing_util::kBinHashList, + url, prefix_hits, &full_hits); } virtual bool UpdateStarted(std::vector<SBListChunkRanges>* lists) { @@ -84,16 +88,42 @@ class TestSafeBrowsingDatabase : public SafeBrowsingDatabase { // Fill up the database with test URL. void AddUrl(const GURL& url, + const std::string& list_name, const std::vector<SBPrefix>& prefix_hits, const std::vector<SBFullHashResult>& full_hits) { + badurls_[url.spec()].list_name = list_name; badurls_[url.spec()].prefix_hits = prefix_hits; badurls_[url.spec()].full_hits = full_hits; } + private: struct Hits { + std::string list_name; std::vector<SBPrefix> prefix_hits; std::vector<SBFullHashResult> full_hits; }; + + bool ContainsUrl(const std::string& list_name0, + const std::string& list_name1, + const GURL& url, + std::vector<SBPrefix>* prefix_hits, + std::vector<SBFullHashResult>* full_hits) { + base::hash_map<std::string, Hits>::const_iterator + badurls_it = badurls_.find(url.spec()); + + if (badurls_it == badurls_.end()) + return false; + + if (badurls_it->second.list_name == list_name0 || + badurls_it->second.list_name == list_name1) { + *prefix_hits = badurls_it->second.prefix_hits; + *full_hits = badurls_it->second.full_hits; + return true; + } + + return false; + } + base::hash_map<std::string, Hits> badurls_; }; @@ -193,7 +223,6 @@ class TestSBProtocolManagerFactory : public SBProtocolManagerFactory { TestProtocolManager* pm_; }; - // Tests the safe browsing blocking page in a browser. class SafeBrowsingServiceTest : public InProcessBrowserTest { public: @@ -236,6 +265,8 @@ class SafeBrowsingServiceTest : public InProcessBrowserTest { // This test will fill up the database using testing prefixes // and urls. command_line->AppendSwitch(switches::kSbDisableAutoUpdate); + + command_line->AppendSwitch(switches::kSbEnableDownloadProtection); } virtual void SetUpInProcessBrowserTestFixture() { @@ -253,7 +284,7 @@ class SafeBrowsingServiceTest : public InProcessBrowserTest { // full hash is hit in database's local cache. std::vector<SBFullHashResult> empty_full_hits; TestSafeBrowsingDatabase* db = db_factory_.GetDb(); - db->AddUrl(url, prefix_hits, empty_full_hits); + db->AddUrl(url, full_hash.list_name, prefix_hits, empty_full_hits); TestProtocolManager* pm = pm_factory_.GetProtocolManager(); pm->SetGetFullHashResponse(url, full_hash); @@ -273,11 +304,14 @@ class SafeBrowsingServiceTest : public InProcessBrowserTest { }; const char kEmptyPage[] = "files/empty.html"; -const char kMalwarePage[] = "files/safe_browsing/malware.html"; +const char kMalwareFile[] = "files/downloads/dangerous/dangerous.exe"; const char kMalwareIframe[] = "files/safe_browsing/malware_iframe.html"; +const char kMalwarePage[] = "files/safe_browsing/malware.html"; +// This test goes through DownloadResourceHandler. IN_PROC_BROWSER_TEST_F(SafeBrowsingServiceTest, Malware) { GURL url = test_server()->GetURL(kEmptyPage); + // After adding the url to safebrowsing database and getfullhash result, // we should see the interstitial page. SBFullHashResult malware_full_hash; @@ -289,4 +323,71 @@ IN_PROC_BROWSER_TEST_F(SafeBrowsingServiceTest, Malware) { EXPECT_TRUE(ShowingInterstitialPage()); } +// This test uses SafeBrowsingService::Client to directly interact with +// SafeBrowsingService. +class TestSBClient + : public base::RefCountedThreadSafe<TestSBClient>, + public SafeBrowsingService::Client { + public: + TestSBClient() : result_(SafeBrowsingService::URL_SAFE), + safe_browsing_service_(g_browser_process-> + resource_dispatcher_host()-> + safe_browsing_service()) { + } + + int GetResult() { + return result_; + } + + void CheckUrl(const GURL& url) { + BrowserThread::PostTask( + BrowserThread::IO, FROM_HERE, + NewRunnableMethod(this, &TestSBClient::CheckUrlOnIOThread, url)); + ui_test_utils::RunMessageLoop(); // Will stop in OnDownloadUrlCheckResult. + } + + private: + void CheckUrlOnIOThread(const GURL& url) { + safe_browsing_service_->CheckDownloadUrl(url, this); + } + + // Called when the result of checking a download URL is known. + void OnDownloadUrlCheckResult(const GURL& url, + SafeBrowsingService::UrlCheckResult result) { + result_ = result; + BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, + NewRunnableMethod(this, &TestSBClient::DownloadUrlCheckDone)); + } + + void DownloadUrlCheckDone() { + MessageLoopForUI::current()->Quit(); + } + + SafeBrowsingService::UrlCheckResult result_; + SafeBrowsingService* safe_browsing_service_; + + DISALLOW_COPY_AND_ASSIGN(TestSBClient); +}; + +IN_PROC_BROWSER_TEST_F(SafeBrowsingServiceTest, CheckDownloadUrl) { + GURL badbin_url = test_server()->GetURL(kMalwareFile); + + scoped_refptr<TestSBClient> client(new TestSBClient); + client->CheckUrl(badbin_url); + + // Since badbin_url is not in database, it is considered to be safe. + EXPECT_EQ(SafeBrowsingService::URL_SAFE, client->GetResult()); + + SBFullHashResult badbinurl_full_hash; + int chunk_id = 0; + GenerateFullhashResult(badbin_url, safe_browsing_util::kBinUrlList, + chunk_id, &badbinurl_full_hash); + SetupResponseForUrl(badbin_url, badbinurl_full_hash); + + client->CheckUrl(badbin_url); + + // Now, the badbin_url is not safe since it is added to download database. + EXPECT_EQ(SafeBrowsingService::BINARY_MALWARE, client->GetResult()); +} + } // namespace |