diff options
10 files changed, 28 insertions, 502 deletions
diff --git a/chrome/browser/safe_browsing/browser_feature_extractor.cc b/chrome/browser/safe_browsing/browser_feature_extractor.cc index bc89109..c00d714 100644 --- a/chrome/browser/safe_browsing/browser_feature_extractor.cc +++ b/chrome/browser/safe_browsing/browser_feature_extractor.cc @@ -50,22 +50,6 @@ static void AddFeature(const std::string& feature_name, VLOG(2) << "Browser feature: " << feature->name() << " " << feature->value(); } -static void AddMalwareFeature(const std::string& feature_name, - const std::set<std::string>& meta_infos, - double feature_value, - ClientMalwareRequest* request) { - DCHECK(request); - ClientMalwareRequest::Feature* feature = - request->add_feature_map(); - feature->set_name(feature_name); - feature->set_value(feature_value); - for (std::set<std::string>::const_iterator it = meta_infos.begin(); - it != meta_infos.end(); ++it) { - feature->add_metainfo(*it); - } - VLOG(2) << "Browser feature: " << feature->name() << " " << feature->value(); -} - static void AddNavigationFeatures( const std::string& feature_prefix, const NavigationController& controller, @@ -227,33 +211,14 @@ void BrowserFeatureExtractor::ExtractFeatures(const BrowseInfo* info, weak_factory_.GetWeakPtr(), request, callback)); } -void BrowserFeatureExtractor::ExtractMalwareFeatures( - const BrowseInfo* info, - ClientMalwareRequest* request) { - DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); - DCHECK(request); - DCHECK(info); - DCHECK_EQ(0U, request->url().find("http:")); - // get the IPs and hosts that match the malware blacklisted IP list. - if (service_) { - for (IPHostMap::const_iterator it = info->ips.begin(); - it != info->ips.end(); ++it) { - if (service_->IsBadIpAddress(it->first)) { - AddMalwareFeature(features::kBadIpFetch + it->first, - it->second, 1.0, request); - } - } - } -} - void BrowserFeatureExtractor::ExtractBrowseInfoFeatures( const BrowseInfo& info, ClientPhishingRequest* request) { if (service_) { - for (IPHostMap::const_iterator it = info.ips.begin(); + for (std::set<std::string>::const_iterator it = info.ips.begin(); it != info.ips.end(); ++it) { - if (service_->IsBadIpAddress(it->first)) { - AddFeature(features::kBadIpFetch + it->first, 1.0, request); + if (service_->IsBadIpAddress(*it)) { + AddFeature(features::kBadIpFetch + *it, 1.0, request); } } } diff --git a/chrome/browser/safe_browsing/browser_feature_extractor.h b/chrome/browser/safe_browsing/browser_feature_extractor.h index 9a4ba3b..717e150 100644 --- a/chrome/browser/safe_browsing/browser_feature_extractor.h +++ b/chrome/browser/safe_browsing/browser_feature_extractor.h @@ -18,7 +18,6 @@ #include "base/basictypes.h" #include "base/callback.h" -#include "base/hash_tables.h" #include "base/memory/scoped_ptr.h" #include "base/sequenced_task_runner_helpers.h" #include "base/time.h" @@ -35,16 +34,13 @@ class WebContents; } namespace safe_browsing { -class ClientMalwareRequest; class ClientPhishingRequest; class ClientSideDetectionService; -typedef std::map<std::string, std::set<std::string> > IPHostMap; - struct BrowseInfo { // List of IPv4 and IPv6 addresses from which content was requested - // together with the hosts on it, while browsing to the |url|. - IPHostMap ips; + // while browsing to the |url|. + std::set<std::string> ips; // If a SafeBrowsing interstitial was shown for the current URL // this will contain the UnsafeResource struct for that URL. @@ -72,7 +68,6 @@ class BrowserFeatureExtractor { // phishing request which was modified by the feature extractor. The // DoneCallback takes ownership of the request object. typedef base::Callback<void(bool, ClientPhishingRequest*)> DoneCallback; - typedef base::Callback<void(bool, ClientMalwareRequest*)> MalwareDoneCallback; // The caller keeps ownership of the tab and service objects and is // responsible for ensuring that they stay valid for the entire @@ -93,11 +88,6 @@ class BrowserFeatureExtractor { ClientPhishingRequest* request, const DoneCallback& callback); - // Extract the malware related features. The request object is owned by the - // caller. - virtual void ExtractMalwareFeatures(const BrowseInfo* info, - ClientMalwareRequest* request); - private: friend class base::DeleteHelper<BrowserFeatureExtractor>; typedef std::pair<ClientPhishingRequest*, DoneCallback> ExtractionData; diff --git a/chrome/browser/safe_browsing/browser_feature_extractor_unittest.cc b/chrome/browser/safe_browsing/browser_feature_extractor_unittest.cc index fa538d5..c31887e 100644 --- a/chrome/browser/safe_browsing/browser_feature_extractor_unittest.cc +++ b/chrome/browser/safe_browsing/browser_feature_extractor_unittest.cc @@ -141,26 +141,6 @@ class BrowserFeatureExtractorTest : public ChromeRenderViewHostTestHarness { } } - void ExtractMalwareFeatures(ClientMalwareRequest* request) { - extractor_->ExtractMalwareFeatures( - browse_info_.get(), request); - } - - void GetMalwareFeatureMap( - const ClientMalwareRequest& request, - std::map<std::string, std::set<std::string> >* features) { - for (int i = 0; i < request.feature_map_size(); ++i) { - const ClientMalwareRequest::Feature& feature = - request.feature_map(i); - EXPECT_EQ(0U, features->count(feature.name())); - std::set<std::string> meta_infos; - for (int j = 0; j < feature.metainfo_size(); ++j) { - meta_infos.insert(feature.metainfo(j)); - } - (*features)[feature.name()] = meta_infos; - } - } - content::TestBrowserThread ui_thread_; int num_pending_; scoped_ptr<BrowserFeatureExtractor> extractor_; @@ -485,10 +465,8 @@ TEST_F(BrowserFeatureExtractorTest, BrowseFeatures) { GURL("https://bankofamerica.com"), content::PAGE_TRANSITION_GENERATED); - std::set<std::string> hosts; - hosts.insert("test.com"); - browse_info_->ips.insert(std::make_pair("193.5.163.8", hosts)); - browse_info_->ips.insert(std::make_pair("23.94.78.1", hosts)); + browse_info_->ips.insert("193.5.163.8"); + browse_info_->ips.insert("23.94.78.1"); EXPECT_CALL(*service_, IsBadIpAddress("193.5.163.8")).WillOnce(Return(true)); EXPECT_CALL(*service_, IsBadIpAddress("23.94.78.1")).WillOnce(Return(false)); @@ -542,40 +520,4 @@ TEST_F(BrowserFeatureExtractorTest, SafeBrowsingFeatures) { EXPECT_DOUBLE_EQ(1.0, features[features::kSafeBrowsingIsSubresource]); EXPECT_DOUBLE_EQ(2.0, features[features::kSafeBrowsingThreatType]); } - -TEST_F(BrowserFeatureExtractorTest, MalwareFeatures) { - ClientMalwareRequest request; - request.set_url("http://www.foo.com/"); - - std::set<std::string> bad_hosts; - bad_hosts.insert("bad.com"); - bad_hosts.insert("evil.com"); - browse_info_->ips.insert(std::make_pair("193.5.163.8", bad_hosts)); - browse_info_->ips.insert(std::make_pair("92.92.92.92", bad_hosts)); - std::set<std::string> good_hosts; - good_hosts.insert("ok.com"); - browse_info_->ips.insert(std::make_pair("23.94.78.1", good_hosts)); - EXPECT_CALL(*service_, IsBadIpAddress("193.5.163.8")).WillOnce(Return(true)); - EXPECT_CALL(*service_, IsBadIpAddress("92.92.92.92")).WillOnce(Return(true)); - EXPECT_CALL(*service_, IsBadIpAddress("23.94.78.1")).WillOnce(Return(false)); - - ExtractMalwareFeatures(&request); - std::map<std::string, std::set<std::string> > features; - GetMalwareFeatureMap(request, &features); - - EXPECT_EQ(2U, features.size()); - std::string feature_name = StringPrintf("%s%s", features::kBadIpFetch, - "193.5.163.8"); - EXPECT_TRUE(features.count(feature_name)); - std::set<std::string> hosts = features[feature_name]; - EXPECT_EQ(2U, hosts.size()); - EXPECT_TRUE(hosts.find("bad.com") != hosts.end()); - EXPECT_TRUE(hosts.find("evil.com") != hosts.end()); - feature_name = StringPrintf("%s%s", features::kBadIpFetch, "92.92.92.92"); - EXPECT_TRUE(features.count(feature_name)); - hosts = features[feature_name]; - EXPECT_EQ(2U, hosts.size()); - EXPECT_TRUE(hosts.find("bad.com") != hosts.end()); - EXPECT_TRUE(hosts.find("evil.com") != hosts.end()); -} } // namespace safe_browsing diff --git a/chrome/browser/safe_browsing/client_side_detection_host.cc b/chrome/browser/safe_browsing/client_side_detection_host.cc index 69d0e05..2529937 100644 --- a/chrome/browser/safe_browsing/client_side_detection_host.cc +++ b/chrome/browser/safe_browsing/client_side_detection_host.cc @@ -43,9 +43,6 @@ using content::WebContents; namespace safe_browsing { -const int ClientSideDetectionHost::kMaxHostsPerIP = 20; -const int ClientSideDetectionHost::kMaxIPsPerBrowse = 200; - namespace { void EmptyUrlCheckCallback(bool processed) { @@ -379,32 +376,22 @@ void ClientSideDetectionHost::OnPhishingDetectionDone( !weak_factory_.HasWeakPtrs() && browse_info_.get() && verdict->ParseFromString(verdict_str) && - verdict->IsInitialized()) { - scoped_ptr<ClientMalwareRequest> malware_verdict(new ClientMalwareRequest); - // Start browser-side malware feature extraction. Once we're done it will - // send the malware client verdict request. - malware_verdict->set_url(verdict->url()); - feature_extractor_->ExtractMalwareFeatures( - browse_info_.get(), - malware_verdict.get()); - MalwareFeatureExtractionDone(malware_verdict.Pass()); - - // We only send phishing verdict to the server if the verdict is phishing or - // if a SafeBrowsing interstitial was already shown for this site. E.g., a - // malware or phishing interstitial was shown but the user clicked - // through. - if (verdict->is_phishing() || DidShowSBInterstitial()) { - if (DidShowSBInterstitial()) { - browse_info_->unsafe_resource.reset(unsafe_resource_.release()); - } - // Start browser-side feature extraction. Once we're done it will send - // the client verdict request. - feature_extractor_->ExtractFeatures( - browse_info_.get(), - verdict.release(), - base::Bind(&ClientSideDetectionHost::FeatureExtractionDone, - weak_factory_.GetWeakPtr())); + verdict->IsInitialized() && + // We only send the verdict to the server if the verdict is phishing or if + // a SafeBrowsing interstitial was already shown for this site. E.g., a + // malware or phishing interstitial was shown but the user clicked + // through. + (verdict->is_phishing() || DidShowSBInterstitial())) { + if (DidShowSBInterstitial()) { + browse_info_->unsafe_resource.reset(unsafe_resource_.release()); } + // Start browser-side feature extraction. Once we're done it will send + // the client verdict request. + feature_extractor_->ExtractFeatures( + browse_info_.get(), + verdict.release(), + base::Bind(&ClientSideDetectionHost::FeatureExtractionDone, + weak_factory_.GetWeakPtr())); } browse_info_.reset(); } @@ -459,25 +446,6 @@ void ClientSideDetectionHost::FeatureExtractionDone( callback); } -void ClientSideDetectionHost::MalwareFeatureExtractionDone( - scoped_ptr<ClientMalwareRequest> request) { - if (!request) { - DLOG(FATAL) << "Invalid request object in MalwareFeatureExtractionDone"; - return; - } - VLOG(2) << "Malware Feature extraction done for URL: " << request->url() - << ", with features count:" << request->feature_map_size(); - - // Send ping if there is matching features. - if (request->feature_map_size() > 0) { - VLOG(1) << "Start sending client malware request."; - ClientSideDetectionService::ClientReportMalwareRequestCallback callback; - csd_service_->SendClientReportMalwareRequest( - request.release(), // The service takes ownership of the request object - callback); // no action after request sent for now - } -} - void ClientSideDetectionHost::Observe( int type, const content::NotificationSource& source, @@ -487,16 +455,7 @@ void ClientSideDetectionHost::Observe( const ResourceRequestDetails* req = content::Details<ResourceRequestDetails>( details).ptr(); if (req && browse_info_.get()) { - std::string ip = req->socket_address.host(); - IPHostMap::iterator it = browse_info_->ips.find(ip); - if (it == browse_info_->ips.end() - && int(browse_info_->ips.size()) < kMaxIPsPerBrowse) { - std::set<std::string> hosts; - hosts.insert(req->url.host()); - browse_info_->ips.insert(make_pair(req->socket_address.host(), hosts)); - } else if (int(it->second.size()) < kMaxHostsPerIP) { - it->second.insert(req->url.host()); - } + browse_info_->ips.insert(req->socket_address.host()); } } diff --git a/chrome/browser/safe_browsing/client_side_detection_host.h b/chrome/browser/safe_browsing/client_side_detection_host.h index fa02863..a567439 100644 --- a/chrome/browser/safe_browsing/client_side_detection_host.h +++ b/chrome/browser/safe_browsing/client_side_detection_host.h @@ -74,8 +74,6 @@ class ClientSideDetectionHost : public content::WebContentsObserver, // This method is responsible for deleting the request object. Called on // the UI thread. void FeatureExtractionDone(bool success, ClientPhishingRequest* request); - // Function to be called when the browser malware feature extractor is done. - void MalwareFeatureExtractionDone(scoped_ptr<ClientMalwareRequest> request); // From NotificationObserver. Called when a notification comes in. This // method is called in the UI thread. @@ -121,11 +119,6 @@ class ClientSideDetectionHost : public content::WebContentsObserver, // Handles registering notifications with the NotificationService. content::NotificationRegistrar registrar_; - // Max number of ips we save for each browse - static const int kMaxIPsPerBrowse; - // Max number of hosts we report for each malware IP. - static const int kMaxHostsPerIP; - base::WeakPtrFactory<ClientSideDetectionHost> weak_factory_; // Unique page ID of the most recent unsafe site that was loaded in this tab diff --git a/chrome/browser/safe_browsing/client_side_detection_host_unittest.cc b/chrome/browser/safe_browsing/client_side_detection_host_unittest.cc index 2aebbc3..00ab995 100644 --- a/chrome/browser/safe_browsing/client_side_detection_host_unittest.cc +++ b/chrome/browser/safe_browsing/client_side_detection_host_unittest.cc @@ -61,11 +61,6 @@ MATCHER_P(PartiallyEqualVerdict, other, "") { other.is_phishing() == arg.is_phishing()); } -MATCHER_P(PartiallyEqualMalwareVerdict, other, "") { - return (other.url() == arg.url() && - other.feature_map_size() == arg.feature_map_size()); -} - // Test that the callback is NULL when the verdict is not phishing. MATCHER(CallbackIsNull, "") { return arg.is_null(); @@ -95,9 +90,6 @@ class MockClientSideDetectionService : public ClientSideDetectionService { MOCK_METHOD2(SendClientReportPhishingRequest, void(ClientPhishingRequest*, const ClientReportPhishingRequestCallback&)); - MOCK_METHOD2(SendClientReportMalwareRequest, - void(ClientMalwareRequest*, - const ClientReportMalwareRequestCallback&)); MOCK_CONST_METHOD1(IsPrivateIPAddress, bool(const std::string&)); MOCK_METHOD2(GetValidCachedResult, bool(const GURL&, bool*)); MOCK_METHOD1(IsInCache, bool(const GURL&)); @@ -165,10 +157,6 @@ class MockBrowserFeatureExtractor : public BrowserFeatureExtractor { void(const BrowseInfo* info, ClientPhishingRequest*, const BrowserFeatureExtractor::DoneCallback&)); - - MOCK_METHOD2(ExtractMalwareFeatures, - void(const BrowseInfo* info, - ClientMalwareRequest*)); }; // Helper function which quits the UI message loop from the IO message loop. @@ -622,109 +610,6 @@ TEST_F(ClientSideDetectionHostTest, EXPECT_TRUE(Mock::VerifyAndClear(csd_service_.get())); } -TEST_F(ClientSideDetectionHostTest, - OnPhishingDetectionDoneVerdictNotPhishingNotMalwareIP) { - // Case 7: renderer sends a verdict string that isn't phishing and not matches - // malware bad IP list - MockBrowserFeatureExtractor* mock_extractor = new MockBrowserFeatureExtractor( - web_contents(), - csd_service_.get()); - SetFeatureExtractor(mock_extractor); // The host class takes ownership. - - ClientPhishingRequest verdict; - verdict.set_url("http://not-phishing.com/"); - verdict.set_client_score(0.1f); - verdict.set_is_phishing(false); - - ClientMalwareRequest malware_verdict; - malware_verdict.set_url("http://not-phishing.com/"); - - EXPECT_CALL(*mock_extractor, ExtractMalwareFeatures(_, _)) - .WillOnce(SetArgumentPointee<1>(malware_verdict)); - EXPECT_CALL(*csd_service_, - SendClientReportMalwareRequest(_, _)).Times(0); - EXPECT_CALL(*mock_extractor, ExtractFeatures(_, _, _)).Times(0); - - OnPhishingDetectionDone(verdict.SerializeAsString()); - EXPECT_TRUE(Mock::VerifyAndClear(mock_extractor)); -} - -TEST_F(ClientSideDetectionHostTest, - OnPhishingDetectionDoneVerdictNotPhishingButMalwareIP) { - // Case 8: renderer sends a verdict string that isn't phishing but matches - // malware bad IP list - MockBrowserFeatureExtractor* mock_extractor = new MockBrowserFeatureExtractor( - web_contents(), - csd_service_.get()); - SetFeatureExtractor(mock_extractor); // The host class takes ownership. - - ClientPhishingRequest verdict; - verdict.set_url("http://not-phishing.com/"); - verdict.set_client_score(0.1f); - verdict.set_is_phishing(false); - - ClientMalwareRequest malware_verdict; - malware_verdict.set_url("http://not-phishing.com/"); - ClientMalwareRequest::Feature* feature = malware_verdict.add_feature_map(); - feature->set_name("malwareip1.2.3.4"); - feature->set_value(1.0); - feature->add_metainfo("badip.com"); - - EXPECT_CALL(*mock_extractor, ExtractMalwareFeatures(_, _)) - .WillOnce(SetArgumentPointee<1>(malware_verdict)); - EXPECT_CALL(*csd_service_, - SendClientReportMalwareRequest( - Pointee(PartiallyEqualMalwareVerdict(malware_verdict)), _)) - .WillOnce(DeleteArg<0>()); - EXPECT_CALL(*mock_extractor, ExtractFeatures(_, _, _)).Times(0); - - OnPhishingDetectionDone(verdict.SerializeAsString()); - EXPECT_TRUE(Mock::VerifyAndClear(mock_extractor)); -} - -TEST_F(ClientSideDetectionHostTest, - OnPhishingDetectionDoneVerdictPhishingAndMalwareIP) { - // Case 9: renderer sends a verdict string that is phishing and matches - // malware bad IP list - MockBrowserFeatureExtractor* mock_extractor = new MockBrowserFeatureExtractor( - web_contents(), - csd_service_.get()); - SetFeatureExtractor(mock_extractor); // The host class takes ownership. - - ClientSideDetectionService::ClientReportPhishingRequestCallback cb; - ClientPhishingRequest verdict; - verdict.set_url("http://not-phishing.com/"); - verdict.set_client_score(0.1f); - verdict.set_is_phishing(true); - - ClientMalwareRequest malware_verdict; - malware_verdict.set_url("http://not-phishing.com/"); - ClientMalwareRequest::Feature* feature = malware_verdict.add_feature_map(); - feature->set_name("malwareip1.2.3.4"); - feature->set_value(1.0); - feature->add_metainfo("badip.com"); - - EXPECT_CALL(*mock_extractor, ExtractMalwareFeatures(_, _)) - .WillOnce(SetArgumentPointee<1>(malware_verdict)); - EXPECT_CALL(*csd_service_, - SendClientReportMalwareRequest( - Pointee(PartiallyEqualMalwareVerdict(malware_verdict)), _)) - .WillOnce(DeleteArg<0>()); - - EXPECT_CALL(*mock_extractor, ExtractFeatures(_, _, _)) - .WillOnce(DoAll(DeleteArg<1>(), - InvokeCallbackArgument<2>(true, &verdict))); - - EXPECT_CALL(*csd_service_, - SendClientReportPhishingRequest( - Pointee(PartiallyEqualVerdict(verdict)), _)) - .WillOnce(SaveArg<1>(&cb)); - OnPhishingDetectionDone(verdict.SerializeAsString()); - EXPECT_TRUE(Mock::VerifyAndClear(mock_extractor)); - EXPECT_TRUE(Mock::VerifyAndClear(csd_service_.get())); - ASSERT_FALSE(cb.is_null()); -} - TEST_F(ClientSideDetectionHostTest, NavigationCancelsShouldClassifyUrl) { // Test that canceling pending should classify requests works as expected. diff --git a/chrome/browser/safe_browsing/client_side_detection_service.cc b/chrome/browser/safe_browsing/client_side_detection_service.cc index 7c805c2..f8d6067 100644 --- a/chrome/browser/safe_browsing/client_side_detection_service.cc +++ b/chrome/browser/safe_browsing/client_side_detection_service.cc @@ -52,8 +52,6 @@ const int ClientSideDetectionService::kPositiveCacheIntervalMinutes = 30; const char ClientSideDetectionService::kClientReportPhishingUrl[] = "https://sb-ssl.google.com/safebrowsing/clientreport/phishing"; -const char ClientSideDetectionService::kClientReportMalwareUrl[] = - "https://sb-ssl.google.com/safebrowsing/clientreport/malware-check"; const char ClientSideDetectionService::kClientModelUrl[] = "https://ssl.gstatic.com/safebrowsing/csd/client_model_v4.pb"; @@ -80,9 +78,6 @@ ClientSideDetectionService::~ClientSideDetectionService() { STLDeleteContainerPairPointers(client_phishing_reports_.begin(), client_phishing_reports_.end()); client_phishing_reports_.clear(); - STLDeleteContainerPairPointers(client_malware_reports_.begin(), - client_malware_reports_.end()); - client_malware_reports_.clear(); } // static @@ -125,16 +120,6 @@ void ClientSideDetectionService::SetEnabledAndRefreshState(bool enabled) { STLDeleteContainerPairPointers(client_phishing_reports_.begin(), client_phishing_reports_.end()); client_phishing_reports_.clear(); - for (std::map<const net::URLFetcher*, ClientReportInfo*>::iterator it = - client_malware_reports_.begin(); - it != client_malware_reports_.end(); ++it) { - ClientReportInfo* info = it->second; - if (!info->callback.is_null()) - info->callback.Run(info->phishing_url, false); - } - STLDeleteContainerPairPointers(client_malware_reports_.begin(), - client_malware_reports_.end()); - client_malware_reports_.clear(); cache_.clear(); } } @@ -149,16 +134,6 @@ void ClientSideDetectionService::SendClientReportPhishingRequest( weak_factory_.GetWeakPtr(), verdict, callback)); } -void ClientSideDetectionService::SendClientReportMalwareRequest( - ClientMalwareRequest* verdict, - const ClientReportMalwareRequestCallback& callback) { - DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); - MessageLoop::current()->PostTask( - FROM_HERE, - base::Bind(&ClientSideDetectionService::StartClientReportMalwareRequest, - weak_factory_.GetWeakPtr(), verdict, callback)); -} - bool ClientSideDetectionService::IsPrivateIPAddress( const std::string& ip_address) const { net::IPAddressNumber ip_number; @@ -220,11 +195,6 @@ void ClientSideDetectionService::OnURLFetchComplete( HandlePhishingVerdict( source, source->GetURL(), source->GetStatus(), source->GetResponseCode(), source->GetCookies(), data); - } else if (client_malware_reports_.find(source) != - client_malware_reports_.end()) { - HandleMalwareVerdict( - source, source->GetURL(), source->GetStatus(), - source->GetResponseCode(), source->GetCookies(), data); } else { NOTREACHED(); } @@ -335,8 +305,7 @@ void ClientSideDetectionService::StartClientReportPhishingRequest( } net::URLFetcher* fetcher = net::URLFetcher::Create( - 0 /* ID used for testing */, - GURL(GetClientReportUrl(kClientReportPhishingUrl)), + 0 /* ID used for testing */, GURL(GetClientReportPhishingUrl()), net::URLFetcher::POST, this); // Remember which callback and URL correspond to the current fetcher object. @@ -354,45 +323,6 @@ void ClientSideDetectionService::StartClientReportPhishingRequest( phishing_report_times_.push(base::Time::Now()); } -void ClientSideDetectionService::StartClientReportMalwareRequest( - ClientMalwareRequest* verdict, - const ClientReportMalwareRequestCallback& callback) { - DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); - scoped_ptr<ClientMalwareRequest> request(verdict); - - if (!enabled_) { - if (!callback.is_null()) - callback.Run(GURL(request->url()), false); - return; - } - - std::string request_data; - if (!request->SerializeToString(&request_data)) { - UMA_HISTOGRAM_COUNTS("SBClientMalware.RequestNotSerialized", 1); - VLOG(1) << "Unable to serialize the CSD request. Proto file changed?"; - if (!callback.is_null()) - callback.Run(GURL(request->url()), false); - return; - } - - net::URLFetcher* fetcher = net::URLFetcher::Create( - 0 /* ID used for testing */, - GURL(GetClientReportUrl(kClientReportMalwareUrl)), - net::URLFetcher::POST, this); - - // Remember which callback and URL correspond to the current fetcher object. - // TODO: need to modify to malware specific code - ClientReportInfo* info = new ClientReportInfo; - info->callback = callback; - info->phishing_url = GURL(request->url()); - client_malware_reports_[fetcher] = info; - - fetcher->SetLoadFlags(net::LOAD_DISABLE_CACHE); - fetcher->SetRequestContext(request_context_getter_.get()); - fetcher->SetUploadData("application/octet-stream", request_data); - fetcher->Start(); -} - void ClientSideDetectionService::HandleModelResponse( const net::URLFetcher* source, const GURL& url, @@ -461,30 +391,6 @@ void ClientSideDetectionService::HandlePhishingVerdict( delete source; } -void ClientSideDetectionService::HandleMalwareVerdict( - const net::URLFetcher* source, - const GURL& url, - const net::URLRequestStatus& status, - int response_code, - const net::ResponseCookies& cookies, - const std::string& data) { - ClientMalwareResponse response; - scoped_ptr<ClientReportInfo> info(client_malware_reports_[source]); - bool should_blacklist = false; - if (status.is_success() && net::HTTP_OK == response_code && - response.ParseFromString(data)) { - should_blacklist = response.blacklist(); - } else { - DLOG(ERROR) << "Unable to get the server verdict for URL: " - << info->phishing_url << " status: " << status.status() << " " - << "response_code:" << response_code; - } - if (!info->callback.is_null()) - info->callback.Run(info->phishing_url, should_blacklist); - client_malware_reports_.erase(source); - delete source; -} - bool ClientSideDetectionService::IsInCache(const GURL& url) { UpdateCache(); @@ -630,9 +536,8 @@ bool ClientSideDetectionService::ModelHasValidHashIds( } // static -std::string ClientSideDetectionService::GetClientReportUrl( - const std::string& report_url) { - std::string url = report_url; +std::string ClientSideDetectionService::GetClientReportPhishingUrl() { + std::string url = kClientReportPhishingUrl; std::string api_key = google_apis::GetAPIKey(); if (!api_key.empty()) { base::StringAppendF(&url, "?key=%s", diff --git a/chrome/browser/safe_browsing/client_side_detection_service.h b/chrome/browser/safe_browsing/client_side_detection_service.h index f6fe23e..77efd13 100644 --- a/chrome/browser/safe_browsing/client_side_detection_service.h +++ b/chrome/browser/safe_browsing/client_side_detection_service.h @@ -52,7 +52,6 @@ typedef std::vector<std::string> ResponseCookies; } // namespace net namespace safe_browsing { -class ClientMalwareRequest; class ClientPhishingRequest; class ClientPhishingResponse; class ClientSideModel; @@ -62,7 +61,6 @@ class ClientSideDetectionService : public net::URLFetcherDelegate, public: // void(GURL phishing_url, bool is_phishing). typedef base::Callback<void(GURL, bool)> ClientReportPhishingRequestCallback; - typedef base::Callback<void(GURL, bool)> ClientReportMalwareRequestCallback; virtual ~ClientSideDetectionService(); @@ -106,11 +104,6 @@ class ClientSideDetectionService : public net::URLFetcherDelegate, ClientPhishingRequest* verdict, const ClientReportPhishingRequestCallback& callback); - // Similar to above one, instead send ClientMalwareRequest - virtual void SendClientReportMalwareRequest( - ClientMalwareRequest* verdict, - const ClientReportMalwareRequestCallback& callback); - // Returns true if the given IP address string falls within a private // (unroutable) network block. Pages which are hosted on these IP addresses // are exempt from client-side phishing detection. This is called by the @@ -197,7 +190,6 @@ class ClientSideDetectionService : public net::URLFetcherDelegate, typedef std::map<std::string /* subnet mask */, std::set<std::string /* hashed subnet */> > BadSubnetMap; - static const char kClientReportMalwareUrl[]; static const char kClientReportPhishingUrl[]; static const char kClientModelUrl[]; static const size_t kMaxModelSizeBytes; @@ -214,10 +206,6 @@ class ClientSideDetectionService : public net::URLFetcherDelegate, ClientPhishingRequest* verdict, const ClientReportPhishingRequestCallback& callback); - void StartClientReportMalwareRequest( - ClientMalwareRequest* verdict, - const ClientReportMalwareRequestCallback& callback); - // Called by OnURLFetchComplete to handle the response from fetching the // model. void HandleModelResponse(const net::URLFetcher* source, @@ -236,15 +224,6 @@ class ClientSideDetectionService : public net::URLFetcherDelegate, const net::ResponseCookies& cookies, const std::string& data); - // Called by OnURLFetchComplete to handle the server response from - // sending the client-side malware request. - void HandleMalwareVerdict(const net::URLFetcher* source, - const GURL& url, - const net::URLRequestStatus& status, - int response_code, - const net::ResponseCookies& cookies, - const std::string& data); - // Invalidate cache results which are no longer useful. void UpdateCache(); @@ -273,7 +252,7 @@ class ClientSideDetectionService : public net::URLFetcherDelegate, static bool ModelHasValidHashIds(const ClientSideModel& model); // Returns the URL that will be used for phishing requests. - static std::string GetClientReportUrl(const std::string& report_url); + static std::string GetClientReportPhishingUrl(); // Whether the service is running or not. When the service is not running, // it won't download the model nor report detected phishing URLs. @@ -289,8 +268,6 @@ class ClientSideDetectionService : public net::URLFetcherDelegate, struct ClientReportInfo; std::map<const net::URLFetcher*, ClientReportInfo*> client_phishing_reports_; - std::map<const net::URLFetcher*, ClientReportInfo*> - client_malware_reports_; // Cache of completed requests. Used to satisfy requests for the same urls // as long as the next request falls within our caching window (which is diff --git a/chrome/browser/safe_browsing/client_side_detection_service_unittest.cc b/chrome/browser/safe_browsing/client_side_detection_service_unittest.cc index f5fd98b..232c885 100644 --- a/chrome/browser/safe_browsing/client_side_detection_service_unittest.cc +++ b/chrome/browser/safe_browsing/client_side_detection_service_unittest.cc @@ -93,18 +93,6 @@ class ClientSideDetectionServiceTest : public testing::Test { return is_phishing_; } - bool SendClientReportMalwareRequest(const GURL& url) { - scoped_ptr<ClientMalwareRequest> request(new ClientMalwareRequest()); - request->set_url(url.spec()); - csd_service_->SendClientReportMalwareRequest( - request.release(), - base::Bind(&ClientSideDetectionServiceTest::SendMalwareRequestDone, - base::Unretained(this))); - phishing_url_ = url; - msg_loop_.Run(); // Waits until callback is called. - return is_malware_; - } - void SetModelFetchResponse(std::string response_data, bool success) { factory_->SetFakeResponse(ClientSideDetectionService::kClientModelUrl, response_data, success); @@ -113,16 +101,7 @@ class ClientSideDetectionServiceTest : public testing::Test { void SetClientReportPhishingResponse(std::string response_data, bool success) { factory_->SetFakeResponse( - ClientSideDetectionService::GetClientReportUrl( - ClientSideDetectionService::kClientReportPhishingUrl), - response_data, success); - } - - void SetClientReportMalwareResponse(std::string response_data, - bool success) { - factory_->SetFakeResponse( - ClientSideDetectionService::GetClientReportUrl( - ClientSideDetectionService::kClientReportMalwareUrl), + ClientSideDetectionService::GetClientReportPhishingUrl(), response_data, success); } @@ -221,17 +200,11 @@ class ClientSideDetectionServiceTest : public testing::Test { msg_loop_.Quit(); } - void SendMalwareRequestDone(GURL url, bool is_malware) { - ASSERT_EQ(phishing_url_, url); - is_malware_ = is_malware; - msg_loop_.Quit(); - } scoped_ptr<content::TestBrowserThread> browser_thread_; scoped_ptr<content::TestBrowserThread> file_thread_; GURL phishing_url_; bool is_phishing_; - bool is_malware_; }; TEST_F(ClientSideDetectionServiceTest, FetchModelTest) { @@ -408,34 +381,6 @@ TEST_F(ClientSideDetectionServiceTest, SendClientReportPhishingRequest) { EXPECT_FALSE(csd_service_->IsInCache(second_url)); } -TEST_F(ClientSideDetectionServiceTest, SendClientReportMalwareRequest) { - SetModelFetchResponse("bogus model", true /* success */); - csd_service_.reset(ClientSideDetectionService::Create(NULL)); - csd_service_->SetEnabledAndRefreshState(true); - GURL url("http://a.com/"); - - // Invalid response body from the server. - SetClientReportMalwareResponse("invalid proto response", true /* success */); - EXPECT_FALSE(SendClientReportMalwareRequest(url)); - - // Normal behavior. - ClientMalwareResponse response; - response.set_blacklist(true); - SetClientReportMalwareResponse(response.SerializeAsString(), true); - EXPECT_TRUE(SendClientReportMalwareRequest(url)); - - // This request will fail - response.set_blacklist(false); - SetClientReportMalwareResponse(response.SerializeAsString(), - false /* success */); - EXPECT_FALSE(SendClientReportMalwareRequest(url)); - - // server blacklist decision is false, and response is succesful - response.set_blacklist(false); - SetClientReportMalwareResponse(response.SerializeAsString(), true); - EXPECT_FALSE(SendClientReportMalwareRequest(url)); -} - TEST_F(ClientSideDetectionServiceTest, GetNumReportTest) { SetModelFetchResponse("bogus model", true /* success */); csd_service_.reset(ClientSideDetectionService::Create(NULL)); diff --git a/chrome/common/safe_browsing/csd.proto b/chrome/common/safe_browsing/csd.proto index 044b9a4..530902c 100644 --- a/chrome/common/safe_browsing/csd.proto +++ b/chrome/common/safe_browsing/csd.proto @@ -87,41 +87,6 @@ message ClientPhishingResponse { repeated string OBSOLETE_whitelist_expression = 2; } -message ClientMalwareRequest { - // URL that the client visited. The CGI parameters are stripped by the - // client. - required string url = 1; - - message Feature { - // Feature name. E.g., 'BadIpFetch='. - required string name = 1; - - // Feature value is always in the range [0.0, 1.0]. Boolean features - // have value 1.0. - required double value = 2; - - // Optional meta information about this feature - repeated string metainfo = 3; - } - - // List of features that were extracted. - repeated Feature feature_map = 2; - - // Field 3 is only used on the server. - - // The referrer URL. This field might not be set, for example, in the case - // where the referrer uses HTTPS. - optional string referrer_url = 4; -} - -message ClientMalwareResponse { - required bool blacklist = 1; - // The confirmed blacklisted bad IP, which will be shown in malware warning. - // This IP string could be either in IPv4 or IPv6 format, which is the same - // as the ones client sent to server. - optional string bad_ip = 2; -} - message ClientDownloadRequest { // The final URL of the download (after all redirects). required string url = 1; |