Compare non-whitelisted off-domain inclusions against the browsing history.

chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.cc

Index: chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.cc
diff --git a/chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.cc b/chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.cc
index b083c2ea4d27769dd31d1915615c3ca780a9a5cc..7f8db67a729d79fd6a45d561f6e21592777904d1 100644
--- a/chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.cc
+++ b/chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.cc
@@ -7,14 +7,20 @@
 #include <string>
 
 #include "base/bind.h"
+#include "base/callback.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/metrics/histogram.h"
 #include "base/single_thread_task_runner.h"
 #include "base/thread_task_runner_handle.h"
+#include "chrome/browser/history/history_service.h"
+#include "chrome/browser/history/history_service_factory.h"
+#include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/safe_browsing/database_manager.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/resource_request_info.h"
+#include "content/public/browser/web_contents.h"
 #include "net/base/registry_controlled_domains/registry_controlled_domain.h"
 #include "net/url_request/url_request.h"
 #include "url/gurl.h"
@@ -23,6 +29,30 @@ using content::BrowserThread;
 
 namespace safe_browsing {
 
+namespace {
+
+// Returns a pointer to the Profile hosting the frame identified by
+// |render_process_id| and |render_frame_id| or NULL if the resolution can't be

[grt (UTC plus 2), 2015/01/21 01:58:24]

NULL -> null or nullptr as per chromium-dev discussion. PK prefers nullptr,
while I (and others) prefer null. Take your pick.

[gab, 2015/01/23 21:35:33]

Done.

+// completed (e.g., the tab/renderer is gone by now). Must only be called on the
+// UI thread.
+Profile* GetProfileFromRenderFrameId(int render_process_id,

[grt (UTC plus 2), 2015/01/21 01:58:24]

teeny tiny nit: drop "Get" from the name, making this
Profile* ProfileFromRenderFrameId(int render_process_id, int render_frame_id) {

[gab, 2015/01/23 21:35:33]

Done.

+                                     int render_frame_id) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  content::RenderFrameHost* render_frame_host =
+      content::RenderFrameHost::FromID(render_process_id, render_frame_id);
+  if (!render_frame_host)
+    return nullptr;
+
+  content::WebContents* web_contents =
+      content::WebContents::FromRenderFrameHost(render_frame_host);
+  if (!web_contents)
+    return nullptr;
+
+  return Profile::FromBrowserContext(web_contents->GetBrowserContext());
+}
+
+}  // namespace
+
 // A static data structure to carry information specific to a given off-domain
 // inclusions across threads, this data will be readable even after the
 // underlying request has been deleted.
@@ -31,6 +61,11 @@ struct OffDomainInclusionDetector::OffDomainInclusionInfo {
 
   content::ResourceType resource_type;
 
+  // Render process and frame IDs, used to figure out which profile this request
+  // belongs to (from the UI thread) if later required during this analysis.
+  int render_process_id;
+  int render_frame_id;
+
   // The URL of the off-domain inclusion.
   GURL request_url;
 
@@ -47,14 +82,19 @@ struct OffDomainInclusionDetector::OffDomainInclusionInfo {
 OffDomainInclusionDetector::OffDomainInclusionDetector(
     const scoped_refptr<SafeBrowsingDatabaseManager>& database_manager)
     : OffDomainInclusionDetector(database_manager,
-                                 ReportAnalysisEventCallback()) {
+                                 ReportAnalysisEventCallback(),
+                                 base::Bind(&GetProfileFromRenderFrameId)) {
 }
 
 OffDomainInclusionDetector::OffDomainInclusionDetector(
     const scoped_refptr<SafeBrowsingDatabaseManager>& database_manager,
-    const ReportAnalysisEventCallback& report_analysis_event_callback)
+    const ReportAnalysisEventCallback& report_analysis_event_callback,
+    const GetProfileFromRenderFrameIdCallback&
+        get_profile_from_render_frame_id_callback)
     : database_manager_(database_manager),
       report_analysis_event_callback_(report_analysis_event_callback),
+      get_profile_from_render_frame_id_callback_(
+          get_profile_from_render_frame_id_callback),
       main_thread_task_runner_(base::ThreadTaskRunnerHandle::Get()),
       weak_ptr_factory_(this) {
   DCHECK(database_manager);
@@ -72,8 +112,17 @@ void OffDomainInclusionDetector::OnResourceRequest(
   scoped_ptr<OffDomainInclusionInfo> off_domain_inclusion_info(
       new OffDomainInclusionInfo);
 
+  const content::ResourceRequestInfo* resource_request_info =
+      content::ResourceRequestInfo::ForRequest(request);
+
   off_domain_inclusion_info->resource_type =
-      content::ResourceRequestInfo::ForRequest(request)->GetResourceType();
+      resource_request_info->GetResourceType();
+
+  bool has_associated_render_frame =
+      resource_request_info->GetAssociatedRenderFrame(
+          &off_domain_inclusion_info->render_process_id,
+          &off_domain_inclusion_info->render_frame_id);
+  DCHECK(has_associated_render_frame);
 
   off_domain_inclusion_info->request_url = request->url();
 
@@ -146,14 +195,14 @@ void OffDomainInclusionDetector::BeginAnalysis(
       // HTTPS => HTTP requests). Consider adding the original referrer to
       // ResourceRequestInfo if that's an issue.
       ReportAnalysisResult(off_domain_inclusion_info.Pass(),
-                           AnalysisEvent::EMPTY_MAIN_FRAME_URL);
+                           AnalysisEvent::ABORT_EMPTY_MAIN_FRAME_URL);
     } else {
       // There is no reason for the main frame to start loading resources if its
       // own URL is invalid but measure this in the wild to make sure.
       // TODO(gab): UMA has proven that this never happens, remove this in a
       // follow-up CL.
       ReportAnalysisResult(off_domain_inclusion_info.Pass(),
-                           AnalysisEvent::INVALID_MAIN_FRAME_URL);
+                           AnalysisEvent::ABORT_INVALID_MAIN_FRAME_URL);
     }
     return;
   }
@@ -191,6 +240,69 @@ void OffDomainInclusionDetector::ContinueAnalysisOnWhitelistResult(
     ReportAnalysisResult(off_domain_inclusion_info.Pass(),
                          AnalysisEvent::OFF_DOMAIN_INCLUSION_WHITELISTED);
   } else {
+    ContinueAnalysisWithHistoryCheck(off_domain_inclusion_info.Pass());
+  }
+}
+
+void OffDomainInclusionDetector::ContinueAnalysisWithHistoryCheck(
+    scoped_ptr<const OffDomainInclusionInfo> off_domain_inclusion_info) {
+  DCHECK(main_thread_task_runner_->BelongsToCurrentThread());
+
+  // The |main_thread_task_runner_| is the UI thread in practice currently so
+  // this call can be made synchronously, but this would need to be made
+  // asynchronous should this ever change.
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  Profile* profile = get_profile_from_render_frame_id_callback_.Run(
+      off_domain_inclusion_info->render_process_id,
+      off_domain_inclusion_info->render_frame_id);
+  if (!profile) {
+    ReportAnalysisResult(off_domain_inclusion_info.Pass(),
+                         AnalysisEvent::ABORT_NO_PROFILE);
+    return;
+  } else if (profile->IsOffTheRecord()) {
+    ReportAnalysisResult(off_domain_inclusion_info.Pass(),
+                         AnalysisEvent::ABORT_INCOGNITO);
+    return;
+  }
+
+  HistoryService* history_service =
+      HistoryServiceFactory::GetForProfileWithoutCreating(profile);

[grt (UTC plus 2), 2015/01/21 01:58:24]

My gut tells me that this should be GetForProfileIfExists(profile,
ServiceAccessType::IMPLICIT_ACCESS). Could you look into it a bit and see what
you think?

[gab, 2015/01/23 21:35:33]

It felt to me like, since I was explicitly checking for incognito, I didn't need
to bother with the overhead of the slightly more complex call (which AFAIK may
even wrap things to prevent writes/etc. which would be overkill here).

The API doesn't make it clear at all what the difference is for these more or
less equivalent method names...

[grt (UTC plus 2), 2015/01/27 15:28:11]

My concern is about correctness rather than overhead. Reasons I think (but am
not 100% certain) IMPLICIT_ACCESS is correct:
- The doc for EXPLICIT_ACCESS says "The caller plans to perform a read or write
that takes place as a result of the user input." The detector does not perform
writes, and is not run in response to user input in the same sense as other
profile services.

- History persistence may be disabled on non-OTR profiles
(http://www.chromium.org/administrators/policy-list-3#SavingBrowserHistoryDisa...).
GetForProfileIfExists(IMPLICIT_ACCESS) will return null in this case.

Would you check with a profile or history owner to figure out which is the
correct approach? Thanks.

[gab, 2015/01/28 19:33:06]

Right, which is why I avoided using either IMPLICT_ACCESS or EXPLICIT_ACCESS.
The fact that the API provides a side-channel that essentially is equivalent to
IMPLICIT_ACCESS in practice is true, but as-is I don't depend on either
ServiceAccessType pattern.
Sounds good, @sky: any tip here on how to use HistoryServiceFactory? The API
doesn't provide any useful tips on when to use GetForProfileIfExists() vs
GetForProfileWithoutCreating().

Thanks!

[grt (UTC plus 2), 2015/01/28 21:10:50]

If by "side-channel" you mean GetForProfileWithoutCreating, then it is not
essentially equivalent to IMPLICIT_ACCESS. It is essentially equivalent to
EXPLICIT_ACCESSS, which I believe is exactly the opposite of what you should
have here. Unless, that is, I'm completely failing to read and understand the
HistoryServiceFactory code and comments (always possible). I'm looking forward
to some light from the sky on this. :-)

[sky, 2015/01/28 21:35:02]

I don't have enough context about this code to tell which it should be. How do
we end up here? What action do we want to take?

[gab, 2015/01/29 14:53:41]

Apologies for the lack of context.

tl;dr; we want to cross-check a given resource inclusion against the user's
browsing history iff the HistoryService already exists. We already abort early
if this request comes from an incognito profile (see line 264 above).
Essentially I think we want GetForProfileIfExists(IMPLICIT_ACCESS), but given we
already explicitly check for incognito above, it felt like
GetForProfileWithoutCreating() was simpler -- otherwise what's the goal of
having GetForProfileWithoutCreating() in this API if not to avoid a dependency
on ServiceAccessType given GetForProfileWithoutCreating() is really the same as
GetForProfileIfExists(EXPLICIT_ACCESS)?


Longer story on how we end up here:

We end up here if a resource on a page was "off-domain" and was not in the safe
browsing list of whitelisted inclusions.

At this point we want to cross-check the off-domain inclusion against the user's
local browsing history before declaring it suspicious.

But we don't want to force the HistoryService to be created (i.e. intentionally
dropping early analyses on profile creation).

We also don't want to analyze requests made from an incognito profile and
explicitly handle this on line 264 above.

+
+  if (!history_service) {
+    // Should only happen very early during startup, receiving many such reports
+    // relative to other report types would indicate that something is wrong.
+    ReportAnalysisResult(off_domain_inclusion_info.Pass(),
+                         AnalysisEvent::ABORT_NO_HISTORY_SERVICE);
+    return;
+  }
+
+  // Need to explicitly copy |request_url| here or this code would depend on
+  // parameter evaluation order as |off_domain_inclusion_info| is being handed
+  // off.
+  const GURL copied_request_url(off_domain_inclusion_info->request_url);
+  history_service->GetVisibleVisitCountToHost(
+      copied_request_url,
+      base::Bind(&OffDomainInclusionDetector::ContinueAnalysisOnHistoryResult,
+                 weak_ptr_factory_.GetWeakPtr(),
+                 base::Passed(&off_domain_inclusion_info)),
+      &history_task_tracker_);
+}
+
+void OffDomainInclusionDetector::ContinueAnalysisOnHistoryResult(
+    scoped_ptr<const OffDomainInclusionInfo> off_domain_inclusion_info,
+    bool success,
+    int num_visits,
+    base::Time /* first_visit_time */) {
+  if (!success) {
+    ReportAnalysisResult(off_domain_inclusion_info.Pass(),
+                         AnalysisEvent::ABORT_HISTORY_LOOKUP_FAILED);
+    return;
+  }
+
+  if (num_visits > 0) {
+    ReportAnalysisResult(off_domain_inclusion_info.Pass(),
+                         AnalysisEvent::OFF_DOMAIN_INCLUSION_IN_HISTORY);
+  } else {
     ReportAnalysisResult(off_domain_inclusion_info.Pass(),
                          AnalysisEvent::OFF_DOMAIN_INCLUSION_SUSPICIOUS);
   }
@@ -207,30 +319,50 @@ void OffDomainInclusionDetector::ReportAnalysisResult(
                             off_domain_inclusion_info->resource_type,
                             content::RESOURCE_TYPE_LAST_TYPE);
 
+  // Log a histogram for the analysis result along with the associated
+  // ResourceType.
+  std::string histogram_name;
   switch (analysis_event) {
     case AnalysisEvent::NO_EVENT:
       break;
-    case AnalysisEvent::EMPTY_MAIN_FRAME_URL:
-      UMA_HISTOGRAM_ENUMERATION("SBOffDomainInclusion.EmptyMainFrameURL",
-                                off_domain_inclusion_info->resource_type,
-                                content::RESOURCE_TYPE_LAST_TYPE);
+    case AnalysisEvent::ABORT_EMPTY_MAIN_FRAME_URL:
+      histogram_name = "SBOffDomainInclusion.Abort.EmptyMainFrameURL";
+      break;
+    case AnalysisEvent::ABORT_INVALID_MAIN_FRAME_URL:
+      // TODO(gab): This never occurs in practice, remove the code handling it.
+      break;
+    case AnalysisEvent::ABORT_NO_PROFILE:
+      histogram_name = "SBOffDomainInclusion.Abort.NoProfile";
       break;
-    case AnalysisEvent::INVALID_MAIN_FRAME_URL:
-      UMA_HISTOGRAM_ENUMERATION("SBOffDomainInclusion.InvalidMainFrameURL",
-                                off_domain_inclusion_info->resource_type,
-                                content::RESOURCE_TYPE_LAST_TYPE);
+    case AnalysisEvent::ABORT_INCOGNITO:
+      histogram_name = "SBOffDomainInclusion.Abort.Incognito";
+      break;
+    case AnalysisEvent::ABORT_NO_HISTORY_SERVICE:
+      histogram_name = "SBOffDomainInclusion.Abort.NoHistoryService";
+      break;
+    case AnalysisEvent::ABORT_HISTORY_LOOKUP_FAILED:
+      histogram_name = "SBOffDomainInclusion.Abort.HistoryLookupFailed";
       break;
     case AnalysisEvent::OFF_DOMAIN_INCLUSION_WHITELISTED:
-      UMA_HISTOGRAM_ENUMERATION("SBOffDomainInclusion.Whitelisted",
-                                off_domain_inclusion_info->resource_type,
-                                content::RESOURCE_TYPE_LAST_TYPE);
+      histogram_name = "SBOffDomainInclusion.Whitelisted";
+      break;
+    case AnalysisEvent::OFF_DOMAIN_INCLUSION_IN_HISTORY:
+      histogram_name = "SBOffDomainInclusion.InHistory";
       break;
     case AnalysisEvent::OFF_DOMAIN_INCLUSION_SUSPICIOUS:
-      UMA_HISTOGRAM_ENUMERATION("SBOffDomainInclusion.Suspicious",
-                                off_domain_inclusion_info->resource_type,
-                                content::RESOURCE_TYPE_LAST_TYPE);
+      histogram_name = "SBOffDomainInclusion.Suspicious";
       break;
   }
+  if (!histogram_name.empty()) {
+    // Expanded from the UMA_HISTOGRAM_ENUMERATION macro.
+    base::LinearHistogram::FactoryGet(
+        histogram_name,
+        1,                                     // minimum,
+        content::RESOURCE_TYPE_LAST_TYPE,      // maximum
+        content::RESOURCE_TYPE_LAST_TYPE + 1,  // bucket_count
+        base::HistogramBase::kUmaTargetedHistogramFlag)
+        ->Add(off_domain_inclusion_info->resource_type);
+  }
 
   if (!report_analysis_event_callback_.is_null() &&
       analysis_event != AnalysisEvent::NO_EVENT) {