Introducing the OffDomainInclusionDetector to analyze resource requests for suspicious activity.

Introducing the OffDomainInclusionDetector to analyze resource requests for suspicious activity.

Based on top of https://codereview.chromium.org/624073002/

BUG=412468
Committed: https://crrev.com/4f7098a467ed332f3dda0811a34ceaef5c63b2e8
Cr-Commit-Position: refs/heads/master@{#304906}

[gab, 2014-09-09 19:39:55]

gab@chromium.org changed reviewers:
+ mattm@chromium.org

[gab, 2014-09-09 19:39:56]

Matt PTAL, design doc @
https://docs.google.com/a/google.com/document/d/19IWX0yWNCFtP_b2LcXcv6KqOLKrx...

Thanks,
Gab

[mattm, 2014-09-09 21:20:17]

https://codereview.chromium.org/516663002/diff/20001/chrome/browser/renderer_...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/516663002/diff/20001/chrome/browser/renderer_...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:29:
#include "chrome/browser/safe_browsing/client_side_detection_service.h"
unused

https://codereview.chromium.org/516663002/diff/20001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/off_domain_inclusion_detector.cc (right):

https://codereview.chromium.org/516663002/diff/20001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/off_domain_inclusion_detector.cc:14: namespace
safe_browsing {
Can you add tests? I found one example of testing histograms values:
https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/multi_log...

https://codereview.chromium.org/516663002/diff/20001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/off_domain_inclusion_detector.cc:75: //
ResourceRequestInfo if that's an issue.
I also have a todo about looking into keeping these referrers for other
safebrowsing reasons, so check with me first if you're thinking of working on
that.

https://codereview.chromium.org/516663002/diff/20001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/off_domain_inclusion_detector.cc:87: // Cut
|main_frame_url| down to its host's second-level domain.
Should use registry_controlled_domains::GetDomainAndRegistry

https://codereview.chromium.org/516663002/diff/20001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

https://codereview.chromium.org/516663002/diff/20001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_service.cc:353:
off_domain_inclusion_detector_.OnResourceRequest(request, resource_type);
needs FULL_SAFE_BROWSING ifdef

https://codereview.chromium.org/516663002/diff/20001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/516663002/diff/20001/chrome/chrome_browser.gy...
chrome/chrome_browser.gypi:2642:
'browser/safe_browsing/off_domain_inclusion_detector.h',
Should this go in incident_reporting dir?

[gab, 2014-11-12 20:48:26]

Patchset #7 (id:120001) has been deleted

[gab, 2014-11-12 20:49:35]

Hey Matt, finally looping back to this after my leave followed by the UwS
interstitial code you're also reviewing :-)!

Please take a look when you have a chance.

Thanks!
Gab

https://codereview.chromium.org/516663002/diff/20001/chrome/browser/renderer_...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/516663002/diff/20001/chrome/browser/renderer_...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:29:
#include "chrome/browser/safe_browsing/client_side_detection_service.h"
On 2014/09/09 21:20:16, mattm wrote:
> unused

Done.

https://codereview.chromium.org/516663002/diff/20001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/off_domain_inclusion_detector.cc (right):

https://codereview.chromium.org/516663002/diff/20001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/off_domain_inclusion_detector.cc:75: //
ResourceRequestInfo if that's an issue.
On 2014/09/09 21:20:16, mattm wrote:
> I also have a todo about looking into keeping these referrers for other
> safebrowsing reasons, so check with me first if you're thinking of working on
> that.

Acknowledged.

https://codereview.chromium.org/516663002/diff/20001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/off_domain_inclusion_detector.cc:87: // Cut
|main_frame_url| down to its host's second-level domain.
On 2014/09/09 21:20:16, mattm wrote:
> Should use registry_controlled_domains::GetDomainAndRegistry

Oh awesome!!! I was looking for exactly that, I had on my TODO list that
non-single-level TLDs like co.uk or appspot.com were problematic but had yet to
find a way to parse those properly.

Thanks!

https://codereview.chromium.org/516663002/diff/20001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

https://codereview.chromium.org/516663002/diff/20001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_service.cc:353:
off_domain_inclusion_detector_.OnResourceRequest(request, resource_type);
On 2014/09/09 21:20:16, mattm wrote:
> needs FULL_SAFE_BROWSING ifdef

Done (and also put |off_domain_inclusion_detector_| in a scoped_ptr only
initialized when in FULL_SAFE_BROWSING mode).

Eventually I think we'll want to find a way for this to run on mobile, but for
now this is easier.

https://codereview.chromium.org/516663002/diff/20001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/516663002/diff/20001/chrome/chrome_browser.gy...
chrome/chrome_browser.gypi:2642:
'browser/safe_browsing/off_domain_inclusion_detector.h',
On 2014/09/09 21:20:16, mattm wrote:
> Should this go in incident_reporting dir?

Ah good point, makes sense to me, I overlooked this originally thinking that
incident_reporting/ was the reporting service itself but looks like its also for
the report handlers and incident detectors.

(I'll move the files in a separate patch set to make the diff prettier for you)

[mattm, 2014-11-14 04:16:00]

https://codereview.chromium.org/516663002/diff/140001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.h
(right):

https://codereview.chromium.org/516663002/diff/140001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.h:21:
enum AnalysisEvent {
Use enum class?

https://codereview.chromium.org/516663002/diff/140001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.h:37:
OffDomainInclusionDetector(
explicit

https://codereview.chromium.org/516663002/diff/140001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc
(right):

https://codereview.chromium.org/516663002/diff/140001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc:118:
false));
include variable name comments for the bool params (or even define an enum for
them)

https://codereview.chromium.org/516663002/diff/140001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_service.h (right):

https://codereview.chromium.org/516663002/diff/140001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_service.h:25: #include
"content/public/common/resource_type.h"
unused?

https://codereview.chromium.org/516663002/diff/140001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_service.h:137: // Oserves resource
requests made by the renderer and reports suspsicious
Observes
suspicious

[gab, 2014-11-14 19:48:35]

gab@chromium.org changed reviewers:
+ avi@chromium.org, jwd@chromium.org, thakis@chromium.org

[gab, 2014-11-14 19:48:36]

Thanks, PTAL.

Also adding:
jwd@ for histograms,
avi@ for the content/public/common/resource_type.h explicit API clarification,
and thakis@ for the small hook in
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc

Cheers,
Gab

https://codereview.chromium.org/516663002/diff/140001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.h
(right):

https://codereview.chromium.org/516663002/diff/140001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.h:21:
enum AnalysisEvent {
On 2014/11/14 04:16:00, mattm wrote:
> Use enum class?

Done.

https://codereview.chromium.org/516663002/diff/140001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.h:37:
OffDomainInclusionDetector(
On 2014/11/14 04:16:00, mattm wrote:
> explicit

Done.

https://codereview.chromium.org/516663002/diff/140001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc
(right):

https://codereview.chromium.org/516663002/diff/140001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc:118:
false));
On 2014/11/14 04:16:00, mattm wrote:
> include variable name comments for the bool params (or even define an enum for
> them)

Done.

https://codereview.chromium.org/516663002/diff/140001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_service.h (right):

https://codereview.chromium.org/516663002/diff/140001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_service.h:25: #include
"content/public/common/resource_type.h"
On 2014/11/14 04:16:00, mattm wrote:
> unused?

Done.

https://codereview.chromium.org/516663002/diff/140001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_service.h:137: // Oserves resource
requests made by the renderer and reports suspsicious
On 2014/11/14 04:16:00, mattm wrote:
> Observes
> suspicious

Done.

[Nico, 2014-11-14 19:52:58]

chrome_resource_dispatcher_host_delegate lgtm

https://codereview.chromium.org/516663002/diff/160001/chrome/browser/renderer...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/516663002/diff/160001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:311:
safe_browsing_->OnResourceRequest(request);
nit: if someone helpfully deletes this in the future, I don't think any tests
would fail…

[Avi (use Gerrit), 2014-11-14 19:53:05]

content/public/common/resource_type.h LGTM with nit

https://codereview.chromium.org/516663002/diff/160001/content/public/common/r...
File content/public/common/resource_type.h (right):

https://codereview.chromium.org/516663002/diff/160001/content/public/common/r...
content/public/common/resource_type.h:12: // Used in histograms, explicitly
assign each type and do not re-use old values.
That should really be a semicolon.

[mattm, 2014-11-14 23:40:10]

https://codereview.chromium.org/516663002/diff/180001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

https://codereview.chromium.org/516663002/diff/180001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_service.cc:358:
incident_service_->AddDownloadManager(download_manager);
bad merge (missing endif)

[gab, 2014-11-18 17:23:20]

Thanks, nits addressed.

@Nico: see reply below.

@Matt: PTAL.

@Jesse: PTAL @ histograms.xml

https://codereview.chromium.org/516663002/diff/160001/chrome/browser/renderer...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/516663002/diff/160001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:311:
safe_browsing_->OnResourceRequest(request);
On 2014/11/14 19:52:58, Nico wrote:
> nit: if someone helpfully deletes this in the future, I don't think any tests
> would fail…

Right, but this class itself is highly undertested already (it only has 3 smalls
tests in chrome_resource_dispatcher_host_delegate_browsertest.cc which appear to
be way too little to provide significant coverage of the logic in this class).

The simple addition in this CL could be unit tested easily if the dependencies
were injected (rather than obtained from g_browser_process) and could be mocked.
But it seems out-of-scope for this CL to modify dependency injection and
introduce unit tests for this class just for this.

I could also add a browser test which is a bit simpler (doesn't need a full
class overhaul as the real dependencies can be used), and simply hook a
TestSafeBrowsingService (just like the current TestDispatcherHostDelegate in
ChromeResourceDispatcherHostDelegateBrowserTest).
But a browser test seems overkill to test this...

In short, I don't think there is a good testing solution for this code as-is and
I think many many things in this class could use tests before this code is the
most important to test here.

Once the OffDomainInclusionDetector is fully hooked to provide incident reports,
I will add integration tests which will exercise this code (and before then I
will definitely notice if this line goes away as I'll be using this code daily).

https://codereview.chromium.org/516663002/diff/160001/content/public/common/r...
File content/public/common/resource_type.h (right):

https://codereview.chromium.org/516663002/diff/160001/content/public/common/r...
content/public/common/resource_type.h:12: // Used in histograms, explicitly
assign each type and do not re-use old values.
On 2014/11/14 19:53:05, Avi wrote:
> That should really be a semicolon.

Done.

https://codereview.chromium.org/516663002/diff/180001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

https://codereview.chromium.org/516663002/diff/180001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_service.cc:358:
incident_service_->AddDownloadManager(download_manager);
On 2014/11/14 23:40:10, mattm wrote:
> bad merge (missing endif)

Done.

[mattm, 2014-11-18 23:36:16]

lgtm

[jwd, 2014-11-19 01:04:02]

lgtm

[gab, 2014-11-19 01:26:57]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2014-11-19 01:28:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/516663002/220001

[commit-bot: I haz the power, 2014-11-19 01:58:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-19 01:58:48]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[grt (UTC plus 2), 2014-11-19 18:29:00]

drive-by

https://codereview.chromium.org/516663002/diff/220001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.h
(right):

https://codereview.chromium.org/516663002/diff/220001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.h:44:
void OnResourceRequest(const net::URLRequest* request);
why not const ref?

https://codereview.chromium.org/516663002/diff/220001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

https://codereview.chromium.org/516663002/diff/220001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_service.cc:288:
download_service_.reset();
?
  off_domain_inclusion_detector_.reset();

https://codereview.chromium.org/516663002/diff/220001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_service.h (right):

https://codereview.chromium.org/516663002/diff/220001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_service.h:263:
scoped_ptr<safe_browsing::OffDomainInclusionDetector>
this and incident_service_ should probably be wrapped in #if
defined(FULL_SAFE_BROWSING) blocks. possibly database_manager_,
download_service_ and csd_service_ as well. i think it's because they have
virtual dtors that was allowing this to build non-FULL_SAFE_BROWSING platforms.

[gab, 2014-11-19 21:24:54]

Patchset #12 (id:240001) has been deleted

[gab, 2014-11-19 21:29:09]

@grt: see comments below, I'll try to land now and will follow-up with anything
remaining.

Thanks,
Gab

https://codereview.chromium.org/516663002/diff/220001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.h
(right):

https://codereview.chromium.org/516663002/diff/220001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.h:44:
void OnResourceRequest(const net::URLRequest* request);
On 2014/11/19 18:28:59, grt wrote:
> why not const ref?

Because everything works off of pointers with net::URLRequest, e.g., the call to
content::ResourceRequestInfo::ForRequest in the .cc file would not work with a
ref as it would get a stack pointer wouldn't it (or maybe not since it's a
reference, but since everything works with pointers I just kept the train
going).

https://codereview.chromium.org/516663002/diff/220001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

https://codereview.chromium.org/516663002/diff/220001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_service.cc:288:
download_service_.reset();
On 2014/11/19 18:29:00, grt wrote:
> ?
>   off_domain_inclusion_detector_.reset();

I had considered this but initially decided against it as I don't understand why
it couldn't just wait for the scoped_ptr to go out of scope when the class is
actually deleted?

The comment for csd_service_ makes sense (although it's sad that such
dependencies have to be carefully hardcoded), but nothing justifies the explicit
reset for incident_service_ and download_service_ AFAICT? And even less for the
off_domain_inclusion_detector_.

As-is, currently, nothing prevents the off_domain_inclusion_detector_ from
receiving requests while the SafeBrowsingService is shutting down (though I'm
not even convinced that would happen).
I was later thinking of hooking the incident_reporting_service_ as a weak
pointer anyways so that it wouldn't depend on that service's lifetime.

Anyways, I guess I'll add the reset here for now since it doesn't really hurt
(and may be required for some obscure reason) but I'd like to understand why
these are required.

https://codereview.chromium.org/516663002/diff/220001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_service.h (right):

https://codereview.chromium.org/516663002/diff/220001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_service.h:263:
scoped_ptr<safe_browsing::OffDomainInclusionDetector>
On 2014/11/19 18:29:00, grt wrote:
> this and incident_service_ should probably be wrapped in #if
> defined(FULL_SAFE_BROWSING) blocks. possibly database_manager_,
> download_service_ and csd_service_ as well. i think it's because they have
> virtual dtors that was allowing this to build non-FULL_SAFE_BROWSING
platforms.

It builds fine on all platforms with GYP, only GN platforms had problems. Adding
ifdefs here implies ifdef changes all over the .cc file as well and I'd rather
avoid that in this CL.

I'll see if this builds with your recent GN safe_browsing fix
(http://crrev.com/304857).

[gab, 2014-11-19 21:29:15]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2014-11-19 21:30:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/516663002/280001

[commit-bot: I haz the power, 2014-11-19 22:31:47]

Committed patchset #13 (id:280001)

[commit-bot: I haz the power, 2014-11-19 22:33:25]

Patchset 13 (id:??) landed as
https://crrev.com/4f7098a467ed332f3dda0811a34ceaef5c63b2e8
Cr-Commit-Position: refs/heads/master@{#304906}