Turn off safebrowsing for PREFETCH requests

Turn off safebrowsing for PREFETCH requests

Safe browsing doesn't need to be on for prefetch requests, since they go directly to cache.

BUG=56582
TEST=SafeBrowsingServiceTest.Prefetch

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=77640

[gavinp, 2011-02-05 04:26:02]

Prefetch requests cache warm only, and safebrowsing interstitials were blocking
the requesting page.  This patch lets the prefetch of the *very dangerous* page
continue.

However, while the prefetch continues, the interstitial still hits the ultimate
navigation.  I've tested the unit test included both with and without the
included patch, and it works both ways.

[gavinp, 2011-02-05 13:59:21]

I want to double check we're not letting downloads through before I land this.

[gavinp, 2011-02-05 18:44:49]

And I have; we don't get download treatment.  Prefetch documents don't get
separated for downloads, they instead always go into the disk cache.

[cbentzel, 2011-02-06 10:32:03]

I'm a bit concerned about this bypassing safebrowsing checks. Does this get
populated in the renderer's in-memory cache? If so, that would be an easy
way for a malicious site to bypass malware check - add a prefetch link,
populate the cache, and then bring the resource in directly.

On Sat, Feb 5, 2011 at 1:44 PM, <gavinp@chromium.org> wrote:

> And I have; we don't get download treatment.  Prefetch documents don't get
> separated for downloads, they instead always go into the disk cache.
>
>
> http://codereview.chromium.org/6334131/
>

[gavinp, 2011-02-07 03:38:49]

On 2011/02/06 10:32:03, cbentzel wrote:
> I'm a bit concerned about this bypassing safebrowsing checks. Does this get
> populated in the renderer's in-memory cache? If so, that would be an easy
> way for a malicious site to bypass malware check - add a prefetch link,
> populate the cache, and then bring the resource in directly.

That's a good point.  Technically, it's not a concern right now for the reasons
that follow:  The in-memory cache in the renderer is strongly typed; so in a
strict sense it does get populated, but it's populated as prefetched data, and
so when a later request for a script/image/css etc... comes in, the in-memory
cache item is dropped.  So that attack route shouldn't work.  However, I'd be
happier if there was a mechanism to add a regression test for this, since that's
basically a bug in the WebKit loader, and I know lots of people are interested
in fixing it.

Perhaps a better fix is to use safe browsing failure as an indication to just
drop the prefetch request?  Seems a better idea all around, now that I think
about it.

> 
> On Sat, Feb 5, 2011 at 1:44 PM, <mailto:gavinp@chromium.org> wrote:
> 
> > And I have; we don't get download treatment.  Prefetch documents don't get
> > separated for downloads, they instead always go into the disk cache.
> >
> >
> > http://codereview.chromium.org/6334131/
> >

[cbentzel, 2011-02-07 22:33:11]

On 2011/02/07 03:38:49, gavinp wrote:
> On 2011/02/06 10:32:03, cbentzel wrote:
> > I'm a bit concerned about this bypassing safebrowsing checks. Does this get
> > populated in the renderer's in-memory cache? If so, that would be an easy
> > way for a malicious site to bypass malware check - add a prefetch link,
> > populate the cache, and then bring the resource in directly.
> 
> That's a good point.  Technically, it's not a concern right now for the
reasons
> that follow:  The in-memory cache in the renderer is strongly typed; so in a
> strict sense it does get populated, but it's populated as prefetched data, and
> so when a later request for a script/image/css etc... comes in, the in-memory
> cache item is dropped.  So that attack route shouldn't work.  However, I'd be
> happier if there was a mechanism to add a regression test for this, since
that's
> basically a bug in the WebKit loader, and I know lots of people are interested
> in fixing it.
> 
> Perhaps a better fix is to use safe browsing failure as an indication to just
> drop the prefetch request?  Seems a better idea all around, now that I think
> about it.

It could be. 

I'm getting a bit worried about the growing number of LOAD_PREFETCH flags being
added to ResourceDispatcherHost, and am wondering if there's something else we
could do to minimize this.

Warning: next paragraph is not well thought through.

Maybe these could be retrieved through an URLFetcher mechanism? Can renderers
send an IPC to trigger an URLFetcher using the current profile's data [like
cookie store, auth cache, etc.], and not actually care about the data received?



> 
> > 
> > On Sat, Feb 5, 2011 at 1:44 PM, <mailto:gavinp@chromium.org> wrote:
> > 
> > > And I have; we don't get download treatment.  Prefetch documents don't get
> > > separated for downloads, they instead always go into the disk cache.
> > >
> > >
> > > http://codereview.chromium.org/6334131/
> > >

[gavinp, 2011-02-18 19:02:05]

This new change moves the stomping down into the safe_browsing_resource_handler,
where it stomps on the requests there; this way we don't get malware in the
in-memory cache.

[cbentzel, 2011-02-22 15:56:03]

Added lzheng for the safebrowsing changes.

http://codereview.chromium.org/6334131/diff/14003/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/6334131/diff/14003/chrome/browser/renderer_hos...
chrome/browser/renderer_host/resource_dispatcher_host.cc:496: if
(safe_browsing_->enabled())
Nit: move back to braces, since the if case is multiline.

http://codereview.chromium.org/6334131/diff/14003/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/safe_browsing_resource_handler.cc (right):

http://codereview.chromium.org/6334131/diff/14003/chrome/browser/renderer_hos...
chrome/browser/renderer_host/safe_browsing_resource_handler.cc:176: AddRef(); 
// Balanced in OnBlockingPageComplete().
Should you just move this after the PREFETCH test so you don't need to balance
with a Release() there?

Also, you could set the state_ after that block as well, so you don't need to
reclear to STATE_NONE inside of the block.

http://codereview.chromium.org/6334131/diff/14003/chrome/browser/renderer_hos...
chrome/browser/renderer_host/safe_browsing_resource_handler.cc:187:
next_handler_->OnResponseCompleted(
Why do you need to call next_handler_->OnResponseCompleted()?

In the OnBlockingPageComplete, it just does rdh_->CancelRequest().

[lzheng, 2011-02-22 19:07:18]

Thanks for adding me.
A small suggestion.

http://codereview.chromium.org/6334131/diff/14003/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/safe_browsing_resource_handler.cc (right):

http://codereview.chromium.org/6334131/diff/14003/chrome/browser/renderer_hos...
chrome/browser/renderer_host/safe_browsing_resource_handler.cc:182: 
Instead of putting this logic here, can you move it to OnBrowseUrlCheckResult
where StartDisplayingBlockingPage is called? I feel it fits better there. And
you don't need to deal with state_ and Release there. It does not seem to need
to call ClearDefrredReqeustInfo() too (unless I missed something).

[gavinp, 2011-02-22 22:51:33]

Remediated to the latest set of reviews.  Thanks!

http://codereview.chromium.org/6334131/diff/14003/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/6334131/diff/14003/chrome/browser/renderer_hos...
chrome/browser/renderer_host/resource_dispatcher_host.cc:496: if
(safe_browsing_->enabled())
On 2011/02/22 15:56:04, cbentzel wrote:
> Nit: move back to braces, since the if case is multiline.

Done.

http://codereview.chromium.org/6334131/diff/14003/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/safe_browsing_resource_handler.cc (right):

http://codereview.chromium.org/6334131/diff/14003/chrome/browser/renderer_hos...
chrome/browser/renderer_host/safe_browsing_resource_handler.cc:176: AddRef(); 
// Balanced in OnBlockingPageComplete().
On 2011/02/22 15:56:04, cbentzel wrote:
> Should you just move this after the PREFETCH test so you don't need to balance
> with a Release() there?
> 
> Also, you could set the state_ after that block as well, so you don't need to
> reclear to STATE_NONE inside of the block.

Mooted by moving the code over to OnBrowseUrlCheckResult.

http://codereview.chromium.org/6334131/diff/14003/chrome/browser/renderer_hos...
chrome/browser/renderer_host/safe_browsing_resource_handler.cc:182: 
On 2011/02/22 19:07:18, lzheng wrote:
> Instead of putting this logic here, can you move it to OnBrowseUrlCheckResult
> where StartDisplayingBlockingPage is called? I feel it fits better there. And
> you don't need to deal with state_ and Release there. It does not seem to need
> to call ClearDefrredReqeustInfo() too (unless I missed something).

Good point Lei!  Done.  However, the original reason I put the code there was to
use the "request" automatic.  In the version I'm uploading, I just make a
redundant call to request, which does an STL map lookup: probably not awful.

I removed the call to ClearDeferredRequestInfo().  Every path except the one in
OnBrowseUrlCheckResult was calling it, and I think I was overcautious.

http://codereview.chromium.org/6334131/diff/14003/chrome/browser/renderer_hos...
chrome/browser/renderer_host/safe_browsing_resource_handler.cc:187:
next_handler_->OnResponseCompleted(
On 2011/02/22 15:56:04, cbentzel wrote:
> Why do you need to call next_handler_->OnResponseCompleted()?
> 
> In the OnBlockingPageComplete, it just does rdh_->CancelRequest(). 
> 

Without the call here, the load of the current page never completes; it spins
forever.  In OnBlockPageComplete, the case is different because we're displaying
the interstitial, and not the page itself: and furthermore we know we'll never
navigate to it.

This does seem like a bug though: a page should show as complete as whether a
never-ending prefetch finishes or not.  Created Bug 73778, and added a TODO here
to track this.

[lzheng, 2011-02-23 02:19:12]

http://codereview.chromium.org/6334131/diff/10008/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/safe_browsing_resource_handler.cc (right):

http://codereview.chromium.org/6334131/diff/10008/chrome/browser/renderer_hos...
chrome/browser/renderer_host/safe_browsing_resource_handler.cc:169:
next_handler_->OnResponseCompleted(
I read your reply to Chris's question, but I still feel this looks a little
uncommon since (I believe) the OnResponseCompleted should be called from the
resource handler level instead of here.
I chatted with Ricardo and he believe CancelRequest should work too. Should we
dig a little deeper and see why it is not? (I am adding Ricardo here to provide
more knowledge).

http://codereview.chromium.org/6334131/diff/10008/chrome/browser/renderer_hos...
chrome/browser/renderer_host/safe_browsing_resource_handler.cc:171:
net::URLRequestStatus(net::URLRequestStatus::FAILED, net::ERR_ABORTED),
80 chars.

http://codereview.chromium.org/6334131/diff/10008/chrome/browser/renderer_hos...
chrome/browser/renderer_host/safe_browsing_resource_handler.cc:173: return;
Can't return here without calling Release. How about use else to wrap
StartDisplayingBlockingPage() below so Release will always be the last
statement?

[rvargas (doing something else), 2011-02-23 03:49:51]

http://codereview.chromium.org/6334131/diff/10008/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/safe_browsing_resource_handler.cc (right):

http://codereview.chromium.org/6334131/diff/10008/chrome/browser/renderer_hos...
chrome/browser/renderer_host/safe_browsing_resource_handler.cc:169:
next_handler_->OnResponseCompleted(
On 2011/02/23 02:19:12, lzheng wrote:
> I read your reply to Chris's question, but I still feel this looks a little
> uncommon since (I believe) the OnResponseCompleted should be called from the
> resource handler level instead of here.
> I chatted with Ricardo and he believe CancelRequest should work too. Should we
> dig a little deeper and see why it is not? (I am adding Ricardo here to
provide
> more knowledge).

I see what's going on here. The problem is that URLRequest::Cancel is meant to
cancel a pending operation, and it triggers an asynchronous action (the job
completes whatever it is doing and invokes the callback). If the request has not
started yet, the job is not killed and no callback is performed, so RDH doesn't
get the OnResponseCompleted signal, and the handlers are not notified about it.

So... the RDH should verify if the request will trigger a callback or not, and
inform the handlers about the request being cancelled regardless of the state of
the request (the same check is performed on other RDH methods).

[gavinp, 2011-03-07 23:26:39]

Thanks for the reviews, especially Lei & Ricardo for pointing out how weird my
explicit destruction of the request was.

Let me know what you think of this newer approach.

http://codereview.chromium.org/6334131/diff/10008/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/safe_browsing_resource_handler.cc (right):

http://codereview.chromium.org/6334131/diff/10008/chrome/browser/renderer_hos...
chrome/browser/renderer_host/safe_browsing_resource_handler.cc:169:
next_handler_->OnResponseCompleted(
On 2011/02/23 03:49:51, rvargas wrote:
> On 2011/02/23 02:19:12, lzheng wrote:
> > I read your reply to Chris's question, but I still feel this looks a little
> > uncommon since (I believe) the OnResponseCompleted should be called from the
> > resource handler level instead of here.
> > I chatted with Ricardo and he believe CancelRequest should work too. Should
we
> > dig a little deeper and see why it is not? (I am adding Ricardo here to
> provide
> > more knowledge).
> 
> I see what's going on here. The problem is that URLRequest::Cancel is meant to
> cancel a pending operation, and it triggers an asynchronous action (the job
> completes whatever it is doing and invokes the callback). If the request has
not
> started yet, the job is not killed and no callback is performed, so RDH
doesn't
> get the OnResponseCompleted signal, and the handlers are not notified about
it.
> 
> So... the RDH should verify if the request will trigger a callback or not, and
> inform the handlers about the request being cancelled regardless of the state
of
> the request (the same check is performed on other RDH methods).

OMG, Thanks for setting my head straight.

I've now added a fix in resource_dispatcher_host which I think does it, but I'd
very much like your option.

http://codereview.chromium.org/6334131/diff/10008/chrome/browser/renderer_hos...
chrome/browser/renderer_host/safe_browsing_resource_handler.cc:171:
net::URLRequestStatus(net::URLRequestStatus::FAILED, net::ERR_ABORTED),
On 2011/02/23 02:19:12, lzheng wrote:
> 80 chars.

Done.

http://codereview.chromium.org/6334131/diff/10008/chrome/browser/renderer_hos...
chrome/browser/renderer_host/safe_browsing_resource_handler.cc:173: return;
On 2011/02/23 02:19:12, lzheng wrote:
> Can't return here without calling Release. How about use else to wrap
> StartDisplayingBlockingPage() below so Release will always be the last
> statement?

Done.

[rvargas (doing something else), 2011-03-08 01:50:39]

The cancel logic LGTM.

[lzheng, 2011-03-08 18:01:34]

I like this change. Thanks.

http://codereview.chromium.org/6334131/diff/27005/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (right):

http://codereview.chromium.org/6334131/diff/27005/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:406: // After
adding the url to safebrowsing database and getfullhash result,
nit: We may want to update this comment.

Should we first test set_prefetch_for_test(false) case to make sure the
ShowingInterestitialPage() is true, then test the "set_prefetch_for_test(false)"
case? This make sure the ShowingInterstitialPage is not always false for the
html we are testing.

[gavinp, 2011-03-08 22:33:03]

I added another browser test per Lei's suggestion, but I exposed a bug in
ui_test_utils::NavigateToURL.  This upload shows my workaround, but I'll go fix
NavigateToURL first and do another upload here.

http://codereview.chromium.org/6334131/diff/27005/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (right):

http://codereview.chromium.org/6334131/diff/27005/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:406: // After
adding the url to safebrowsing database and getfullhash result,
On 2011/03/08 18:01:34, lzheng wrote:
> nit: We may want to update this comment.
> 
> Should we first test set_prefetch_for_test(false) case to make sure the
> ShowingInterestitialPage() is true, then test the
"set_prefetch_for_test(false)"
> case? This make sure the ShowingInterstitialPage is not always false for the
> html we are testing.

I fixed the comment.

With regard to testing, we'd expect the same result here for either prefetching
on or prefetching off.  The bug was that if you prefetched unsafe content, you
got an interstitial on the page that launched the prefetch.  If you turn off
prefetching, then you would definitely not expect to see a safebrowsing
interstitial: now there's no fetch of unsafe content at all!

There is a test that is worthwhile to add here though; I have added a
navigation, after the one that didn't get the interstitial, to the malware page
itself.  That navigation should get an interstitial, and shows that the safe
browsing has not been defeated through the prefetch load, however that might
happen.

http://codereview.chromium.org/6334131/diff/39001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:424:
safe_browsing_service_(g_browser_process->
I use the WindowedNotificationObserver here, instead of calling
ui_test_utils::NavigateToURL to avoid a race in ui_test_utils::NavigateToURL, I
was reliably hitting here.  I don't want to land this issue until I fix
ui_test_utils::NavigateToURL, but I did want to show this new aspect to this
test.

[gavinp, 2011-03-09 21:55:20]

I've now uploaded http://codereview.chromium.org/6662001/ , which fixes the race
our browser test was tickling; so this new diff is what I hope to land if that
gets in.

What do you think?

[lzheng, 2011-03-09 22:31:39]

LGTM.

On 2011/03/09 21:55:20, gavinp wrote:
> I've now uploaded http://codereview.chromium.org/6662001/ , which fixes the
race
> our browser test was tickling; so this new diff is what I hope to land if that
> gets in.
> 
> What do you think?