summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorNathan Parker <nparker@chromium.org>2015-10-12 15:41:55 -0700
committerNathan Parker <nparker@chromium.org>2015-10-12 22:43:34 +0000
commita083d5c843e7f61d4978f8fae36b921f353b5816 (patch)
tree75bf8ea2592bdf33172e2c0b0f7a1f116fcda889
parent93b5a889d88e2d3e97c48bb9723470ba80b791a3 (diff)
downloadchromium_src-a083d5c843e7f61d4978f8fae36b921f353b5816.zip
chromium_src-a083d5c843e7f61d4978f8fae36b921f353b5816.tar.gz
chromium_src-a083d5c843e7f61d4978f8fae36b921f353b5816.tar.bz2
[Merge to M46] Skip checking some resource types against safe browsing, on mobile.
On complex sites this drops about 40% of checks. Though since most of them are not blocking other resources, it will likely have only slight effect on total page load time. It will reduce CPU and power usage, and contention for GMSCore. lgarron@ and I contemplated the whitelist and decided that while CSS poses a theoretical risk, it's fairly unlikely to be exploited. If it is, we can re-enable quickly with finch. BUG=507439 Review URL: https://codereview.chromium.org/1333993003 Cr-Commit-Position: refs/heads/master@{#349241} (cherry picked from commit 59428cf610c3edf2281cf21ec6a7b44403d5536a) Review URL: https://codereview.chromium.org/1396403002 . Cr-Commit-Position: refs/branch-heads/2490@{#525} Cr-Branched-From: 7790a3535f2a81a03685eca31a32cf69ae0c114f-refs/heads/master@{#344925}
-rw-r--r--chrome/browser/renderer_host/safe_browsing_resource_throttle.cc80
-rw-r--r--chrome/browser/renderer_host/safe_browsing_resource_throttle.h25
-rw-r--r--chrome/browser/safe_browsing/safe_browsing_service.cc10
-rw-r--r--chrome/browser/safe_browsing/safe_browsing_service.h15
-rw-r--r--tools/metrics/histograms/histograms.xml28
5 files changed, 134 insertions, 24 deletions
diff --git a/chrome/browser/renderer_host/safe_browsing_resource_throttle.cc b/chrome/browser/renderer_host/safe_browsing_resource_throttle.cc
index ef89752..c7d5351 100644
--- a/chrome/browser/renderer_host/safe_browsing_resource_throttle.cc
+++ b/chrome/browser/renderer_host/safe_browsing_resource_throttle.cc
@@ -5,6 +5,7 @@
#include "chrome/browser/renderer_host/safe_browsing_resource_throttle.h"
#include "base/logging.h"
+#include "base/metrics/histogram_macros.h"
#include "chrome/browser/browser_process.h"
#include "chrome/browser/prerender/prerender_contents.h"
#include "chrome/browser/safe_browsing/safe_browsing_service.h"
@@ -17,10 +18,36 @@
#include "net/url_request/redirect_info.h"
#include "net/url_request/url_request.h"
+namespace {
+
// Maximum time in milliseconds to wait for the safe browsing service to
// verify a URL. After this amount of time the outstanding check will be
// aborted, and the URL will be treated as if it were safe.
-static const int kCheckUrlTimeoutMs = 5000;
+const int kCheckUrlTimeoutMs = 5000;
+
+// Return true for resource types that are very unlikely to produce a successful
+// malware or phishing page without the main frame also being bad. This is a
+// latency / safety trade-off, used only on mobile. We're conservative here,
+// keeping this list small and leaving off uncommon types since they won't save
+// us much on aggregate latency.
+bool IsResourceTypeMostlySafe(content::ResourceType resource_type) {
+ switch (resource_type) {
+ case content::RESOURCE_TYPE_STYLESHEET:
+ case content::RESOURCE_TYPE_IMAGE:
+ case content::RESOURCE_TYPE_FONT_RESOURCE:
+ case content::RESOURCE_TYPE_FAVICON:
+ return true;
+ default:
+ return false;
+ }
+}
+
+void RecordHistogramResourceTypeSafe(content::ResourceType resource_type) {
+ UMA_HISTOGRAM_ENUMERATION("SB2.ResourceTypes.Safe", resource_type,
+ content::RESOURCE_TYPE_LAST_TYPE);
+}
+
+} // namespace
// TODO(eroman): Downgrade these CHECK()s to DCHECKs once there is more
// unit test coverage.
@@ -32,16 +59,17 @@ SafeBrowsingResourceThrottle* SafeBrowsingResourceThrottle::MaybeCreate(
SafeBrowsingService* sb_service) {
#if defined(SAFE_BROWSING_DB_LOCAL)
// Throttle consults a local database before starting the resource request.
- return new SafeBrowsingResourceThrottle(request, resource_type, sb_service,
- true /* defer_at_start */);
+ return new SafeBrowsingResourceThrottle(
+ request, resource_type, sb_service, DEFER_AT_START,
+ SafeBrowsingService::CHECK_ALL_RESOURCE_TYPES);
#elif defined(SAFE_BROWSING_DB_REMOTE)
- if (sb_service->IsAndroidFieldTrialEnabled()) {
- // Throttle consults a remote database before processing the response.
- return new SafeBrowsingResourceThrottle(request, resource_type, sb_service,
- false /* defer_at_start */);
- } else {
+ if (!sb_service->IsAndroidFieldTrialEnabled())
return nullptr;
- }
+
+ // Throttle consults a remote database before processing the response.
+ return new SafeBrowsingResourceThrottle(
+ request, resource_type, sb_service, DONT_DEFER_AT_START,
+ sb_service->GetResourceTypesToCheck());
#else
#error "Incompatible compile flags for safe_browsing_resource_throttle"
#endif
@@ -51,17 +79,17 @@ SafeBrowsingResourceThrottle::SafeBrowsingResourceThrottle(
const net::URLRequest* request,
content::ResourceType resource_type,
SafeBrowsingService* sb_service,
- bool defer_at_start)
- : defer_at_start_(defer_at_start),
+ DeferAtStartSetting defer_setting,
+ SafeBrowsingService::ResourceTypesToCheck resource_types_to_check)
+ : defer_at_start_(defer_setting == DEFER_AT_START),
+ resource_types_to_check_(resource_types_to_check),
state_(STATE_NONE),
defer_state_(DEFERRED_NONE),
threat_type_(SB_THREAT_TYPE_SAFE),
database_manager_(sb_service->database_manager()),
ui_manager_(sb_service->ui_manager()),
request_(request),
- is_subresource_(resource_type != content::RESOURCE_TYPE_MAIN_FRAME),
- is_subframe_(resource_type == content::RESOURCE_TYPE_SUB_FRAME) {
-}
+ resource_type_(resource_type) {}
SafeBrowsingResourceThrottle::~SafeBrowsingResourceThrottle() {
if (state_ == STATE_CHECKING_URL)
@@ -141,6 +169,7 @@ void SafeBrowsingResourceThrottle::OnCheckBrowseUrlResult(
state_ = STATE_NONE;
if (threat_type == SB_THREAT_TYPE_SAFE) {
+ RecordHistogramResourceTypeSafe(resource_type_);
if (defer_state_ != DEFERRED_NONE) {
// Log how much time the safe browsing check cost us.
ui_manager_->LogPauseDelay(base::TimeTicks::Now() - defer_start_time_);
@@ -154,9 +183,14 @@ void SafeBrowsingResourceThrottle::OnCheckBrowseUrlResult(
if (request_->load_flags() & net::LOAD_PREFETCH) {
// Don't prefetch resources that fail safe browsing, disallow them.
controller()->Cancel();
+ UMA_HISTOGRAM_ENUMERATION("SB2.ResourceTypes.UnsafePrefetchCanceled",
+ resource_type_, content::RESOURCE_TYPE_LAST_TYPE);
return;
}
+ UMA_HISTOGRAM_ENUMERATION("SB2.ResourceTypes.Unsafe", resource_type_,
+ content::RESOURCE_TYPE_LAST_TYPE);
+
const content::ResourceRequestInfo* info =
content::ResourceRequestInfo::ForRequest(request_);
@@ -164,8 +198,8 @@ void SafeBrowsingResourceThrottle::OnCheckBrowseUrlResult(
resource.url = url;
resource.original_url = request_->original_url();
resource.redirect_urls = redirect_urls_;
- resource.is_subresource = is_subresource_;
- resource.is_subframe = is_subframe_;
+ resource.is_subresource = resource_type_ != content::RESOURCE_TYPE_MAIN_FRAME;
+ resource.is_subframe = resource_type_ == content::RESOURCE_TYPE_SUB_FRAME;
resource.threat_type = threat_type;
resource.threat_metadata = metadata;
resource.callback = base::Bind(
@@ -233,8 +267,22 @@ void SafeBrowsingResourceThrottle::OnBlockingPageComplete(bool proceed) {
bool SafeBrowsingResourceThrottle::CheckUrl(const GURL& url) {
CHECK_EQ(state_, STATE_NONE);
+ // To reduce aggregate latency on mobile, skip checking resources that
+ // can't do much harm.
+ if (resource_types_to_check_ ==
+ SafeBrowsingService::CHECK_ONLY_DANGEROUS_TYPES &&
+ IsResourceTypeMostlySafe(resource_type_)) {
+ UMA_HISTOGRAM_ENUMERATION("SB2.ResourceTypes.Skipped", resource_type_,
+ content::RESOURCE_TYPE_LAST_TYPE);
+ return true;
+ }
+
bool succeeded_synchronously = database_manager_->CheckBrowseUrl(url, this);
+ UMA_HISTOGRAM_ENUMERATION("SB2.ResourceTypes.Checked", resource_type_,
+ content::RESOURCE_TYPE_LAST_TYPE);
+
if (succeeded_synchronously) {
+ RecordHistogramResourceTypeSafe(resource_type_);
threat_type_ = SB_THREAT_TYPE_SAFE;
ui_manager_->LogPauseDelay(base::TimeDelta()); // No delay.
return true;
diff --git a/chrome/browser/renderer_host/safe_browsing_resource_throttle.h b/chrome/browser/renderer_host/safe_browsing_resource_throttle.h
index 047f7f2..a7b5d2f 100644
--- a/chrome/browser/renderer_host/safe_browsing_resource_throttle.h
+++ b/chrome/browser/renderer_host/safe_browsing_resource_throttle.h
@@ -12,6 +12,7 @@
#include "base/time/time.h"
#include "base/timer/timer.h"
#include "chrome/browser/safe_browsing/database_manager.h"
+#include "chrome/browser/safe_browsing/safe_browsing_service.h"
#include "chrome/browser/safe_browsing/ui_manager.h"
#include "content/public/browser/resource_throttle.h"
#include "content/public/common/resource_type.h"
@@ -70,11 +71,6 @@ class SafeBrowsingResourceThrottle
content::ResourceType resource_type,
SafeBrowsingService* sb_service);
- SafeBrowsingResourceThrottle(const net::URLRequest* request,
- content::ResourceType resource_type,
- SafeBrowsingService* sb_service,
- bool defer_at_start);
-
// content::ResourceThrottle implementation (called on IO thread):
void WillStartRequest(bool* defer) override;
void WillRedirectRequest(const net::RedirectInfo& redirect_info,
@@ -88,6 +84,19 @@ class SafeBrowsingResourceThrottle
SBThreatType result,
const std::string& metadata) override;
+ protected:
+ enum DeferAtStartSetting {
+ DEFER_AT_START,
+ DONT_DEFER_AT_START
+ };
+
+ SafeBrowsingResourceThrottle(
+ const net::URLRequest* request,
+ content::ResourceType resource_type,
+ SafeBrowsingService* sb_service,
+ DeferAtStartSetting defer_setting,
+ SafeBrowsingService::ResourceTypesToCheck types_to_check);
+
private:
// Describes what phase of the check a throttle is in.
enum State {
@@ -141,6 +150,9 @@ class SafeBrowsingResourceThrottle
// deemed safe. Otherwise we let the resource partially load.
const bool defer_at_start_;
+ // Check all types, or just the dangerous ones?
+ const SafeBrowsingService::ResourceTypesToCheck resource_types_to_check_;
+
State state_;
DeferState defer_state_;
@@ -165,8 +177,7 @@ class SafeBrowsingResourceThrottle
scoped_refptr<SafeBrowsingDatabaseManager> database_manager_;
scoped_refptr<SafeBrowsingUIManager> ui_manager_;
const net::URLRequest* request_;
- const bool is_subresource_;
- const bool is_subframe_;
+ const content::ResourceType resource_type_;
DISALLOW_COPY_AND_ASSIGN(SafeBrowsingResourceThrottle);
};
diff --git a/chrome/browser/safe_browsing/safe_browsing_service.cc b/chrome/browser/safe_browsing/safe_browsing_service.cc
index 313b715..0ae7ba6 100644
--- a/chrome/browser/safe_browsing/safe_browsing_service.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_service.cc
@@ -109,6 +109,7 @@ bool IsIncidentReportingServiceEnabled() {
// Android field trial
const char kAndroidFieldExperiment[] = "SafeBrowsingAndroid";
const char kAndroidFieldParam[] = "enabled";
+const char kAndroidCheckAllTypesParam[] = "check_all_resource_types";
const char kAndroidFieldParamEnabledValue[] = "true";
#endif // defined(SAFE_BROWSING_DB_REMOTE)
} // namespace
@@ -220,6 +221,15 @@ SafeBrowsingService::SafeBrowsingService()
kAndroidFieldExperiment, kAndroidFieldParam);
is_android_field_trial_enabled_ =
(enabled_param == kAndroidFieldParamEnabledValue);
+
+ const std::string check_all_types_param = variations::GetVariationParamValue(
+ kAndroidFieldExperiment, kAndroidCheckAllTypesParam);
+ if (check_all_types_param == kAndroidFieldParamEnabledValue) {
+ resource_types_to_check_ = CHECK_ALL_RESOURCE_TYPES;
+ } else {
+ // Default
+ resource_types_to_check_ = CHECK_ONLY_DANGEROUS_TYPES;
+ }
#endif // defined(SAFE_BROWSING_DB_REMOTE)
}
diff --git a/chrome/browser/safe_browsing/safe_browsing_service.h b/chrome/browser/safe_browsing/safe_browsing_service.h
index a2b2933..c7e5c6b 100644
--- a/chrome/browser/safe_browsing/safe_browsing_service.h
+++ b/chrome/browser/safe_browsing/safe_browsing_service.h
@@ -63,7 +63,7 @@ class IncidentReportingService;
class OffDomainInclusionDetector;
class ResourceRequestDetector;
#endif
-}
+} // namespace safe_browsing
// Construction needs to happen on the main thread.
// The SafeBrowsingService owns both the UI and Database managers which do
@@ -76,6 +76,11 @@ class SafeBrowsingService
content::BrowserThread::DeleteOnUIThread>,
public content::NotificationObserver {
public:
+ enum ResourceTypesToCheck {
+ CHECK_ALL_RESOURCE_TYPES,
+ CHECK_ONLY_DANGEROUS_TYPES,
+ };
+
// Makes the passed |factory| the factory used to instanciate
// a SafeBrowsingService. Useful for tests.
static void RegisterFactory(SafeBrowsingServiceFactory* factory) {
@@ -95,6 +100,13 @@ class SafeBrowsingService
bool IsAndroidFieldTrialEnabled() const {
return is_android_field_trial_enabled_;
}
+
+ // Should we check all types, or just the dangerous ones?
+ // We can flip this with a field trial if a non-dangerous type
+ // starts getting exploited.
+ ResourceTypesToCheck GetResourceTypesToCheck() const {
+ return resource_types_to_check_;
+ }
#endif // defined(SAFE_BROWSING_DB_REMOTE)
// Called on the UI thread to initialize the service.
@@ -275,6 +287,7 @@ class SafeBrowsingService
#if defined(SAFE_BROWSING_DB_REMOTE)
bool is_android_field_trial_enabled_;
+ ResourceTypesToCheck resource_types_to_check_;
#endif // defined(SAFE_BROWSING_DB_REMOTE)
// Tracks existing PrefServices, and the safe browsing preference on each.
diff --git a/tools/metrics/histograms/histograms.xml b/tools/metrics/histograms/histograms.xml
index be11327..516165b 100644
--- a/tools/metrics/histograms/histograms.xml
+++ b/tools/metrics/histograms/histograms.xml
@@ -37853,6 +37853,14 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
</summary>
</histogram>
+<histogram name="SB2.ResourceTypes" enum="ContentResourceType">
+ <owner>nparker@chromium.org</owner>
+ <summary>
+ Resource types of resources that were inspected by Safe Browsing in the
+ SafeBrowsingResourceThrottle.
+ </summary>
+</histogram>
+
<histogram name="SB2.SetExtendedReportingEnabled" enum="BooleanEnabled">
<obsolete>
Deprecated 03/2015. Replaced by
@@ -77892,6 +77900,26 @@ To add a new entry, add it with any value and run test to compute valid value.
<affected-histogram name="SB2.PrefixSetSizeKilobytes"/>
</histogram_suffixes>
+<histogram_suffixes name="SB2ResourceTypes" separator=".">
+ <suffix name="Checked"
+ label="Resources that were checked. Logged before the resource starts
+ loading and again for each redirect."/>
+ <suffix name="Safe"
+ label="Resources that were checked and deemed safe. Logged when the URL
+ check is completed."/>
+ <suffix name="Skipped"
+ label="Resources that were not checked because they are not active-ish
+ content types. Only used on mobile. Logged before the resource
+ request starts."/>
+ <suffix name="Unsafe"
+ label="Resources that were checked and classified as unsafe. Logged
+ when the URL check is completed."/>
+ <suffix name="UnsafePrefetchCanceled"
+ label="Pre-fetched resources that were checked and classified as
+ unsafe. Logged as the request is canceled."/>
+ <affected-histogram name="SB2.ResourceTypes"/>
+</histogram_suffixes>
+
<histogram_suffixes name="SBInterstitial">
<obsolete>
deprecated November 10 2012 crrev.com/167056