Sanitize referrers for programmatic downloads.

Sanitize referrers for programmatic downloads.

These can come from context menu saves, download extensions, and other locations.

BUG=369201, 357473

[cbentzel, 2014-05-09 12:17:07]

Any suggestions for how to do automated tests for this? I believe we have some
for browser_tests for context menu downloads (since I've touched them in the
past) but perhaps unit tests are more appropriate here.

I also considered a number of other places to do the sanitization:
  - In the BeginDownload() callers, particularly the context menu.
  - On Blink side when providing the IPC message for the context menu.
  - In SetReferrerForRequest, but that is unnecessary for most cases (such as
general resource loads from web pages).

[cbentzel, 2014-05-09 14:16:56]

On 2014/05/09 12:17:07, cbentzel wrote:
> Any suggestions for how to do automated tests for this? I believe we have some
> for browser_tests for context menu downloads (since I've touched them in the
> past) but perhaps unit tests are more appropriate here.
> 
> I also considered a number of other places to do the sanitization:
>   - In the BeginDownload() callers, particularly the context menu.
>   - On Blink side when providing the IPC message for the context menu.
>   - In SetReferrerForRequest, but that is unnecessary for most cases (such as
> general resource loads from web pages).

I'll plan to unit test SanitizeReferrerForRequest but also wonder if we want
more systems integration tests as well.

[asanka, 2014-05-09 15:18:37]

On 2014/05/09 12:17:07, cbentzel wrote:
> Any suggestions for how to do automated tests for this? I believe we have some
> for browser_tests for context menu downloads (since I've touched them in the
> past) but perhaps unit tests are more appropriate here.
> 
> I also considered a number of other places to do the sanitization:
>   - In the BeginDownload() callers, particularly the context menu.
>   - On Blink side when providing the IPC message for the context menu.
>   - In SetReferrerForRequest, but that is unnecessary for most cases (such as
> general resource loads from web pages).

Ah. I see you've considered adding the sanitization logic to
SetReferrerForRequest. If we aren't doing that, shall we DCHECK that the
assumptions made by SetReferrerForRequest are met? Looking at this bug, it seems
we may not always be able to rely on callers to have applied the referrer policy
to the first request.

[asanka, 2014-05-09 15:21:53]

On 2014/05/09 14:16:56, cbentzel wrote:
> On 2014/05/09 12:17:07, cbentzel wrote:
> > Any suggestions for how to do automated tests for this? I believe we have
some
> > for browser_tests for context menu downloads (since I've touched them in the
> > past) but perhaps unit tests are more appropriate here.
> > 
> > I also considered a number of other places to do the sanitization:
> >   - In the BeginDownload() callers, particularly the context menu.
> >   - On Blink side when providing the IPC message for the context menu.
> >   - In SetReferrerForRequest, but that is unnecessary for most cases (such
as
> > general resource loads from web pages).
> 
> I'll plan to unit test SanitizeReferrerForRequest but also wonder if we want
> more systems integration tests as well.

For content_browsertest level tests see
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/do...
for an example. However you may not need the overhead in that test if you are
testing by DownloadManager::DownloadUrl() directly with an unsanitized referrer.

[cbentzel, 2014-05-09 15:33:11]

On 2014/05/09 15:18:37, asanka wrote:
> On 2014/05/09 12:17:07, cbentzel wrote:
> > Any suggestions for how to do automated tests for this? I believe we have
some
> > for browser_tests for context menu downloads (since I've touched them in the
> > past) but perhaps unit tests are more appropriate here.
> > 
> > I also considered a number of other places to do the sanitization:
> >   - In the BeginDownload() callers, particularly the context menu.
> >   - On Blink side when providing the IPC message for the context menu.
> >   - In SetReferrerForRequest, but that is unnecessary for most cases (such
as
> > general resource loads from web pages).
> 
> Ah. I see you've considered adding the sanitization logic to
> SetReferrerForRequest. If we aren't doing that, shall we DCHECK that the
> assumptions made by SetReferrerForRequest are met? Looking at this bug, it
seems
> we may not always be able to rely on callers to have applied the referrer
policy
> to the first request.

I like the idea to put DHCECKs (or even CHECKs for non-official builds) there.
URLRequest does this for a subset of cases.

[asanka, 2014-05-09 15:51:09]

On 2014/05/09 15:33:11, cbentzel wrote:
> On 2014/05/09 15:18:37, asanka wrote:
> > On 2014/05/09 12:17:07, cbentzel wrote:
> > > Any suggestions for how to do automated tests for this? I believe we have
> some
> > > for browser_tests for context menu downloads (since I've touched them in
the
> > > past) but perhaps unit tests are more appropriate here.
> > > 
> > > I also considered a number of other places to do the sanitization:
> > >   - In the BeginDownload() callers, particularly the context menu.
> > >   - On Blink side when providing the IPC message for the context menu.
> > >   - In SetReferrerForRequest, but that is unnecessary for most cases (such
> as
> > > general resource loads from web pages).
> > 
> > Ah. I see you've considered adding the sanitization logic to
> > SetReferrerForRequest. If we aren't doing that, shall we DCHECK that the
> > assumptions made by SetReferrerForRequest are met? Looking at this bug, it
> seems
> > we may not always be able to rely on callers to have applied the referrer
> policy
> > to the first request.
> 
> I like the idea to put DHCECKs (or even CHECKs for non-official builds) there.
> URLRequest does this for a subset of cases.

It might also be that the overhead of verifying is similar to that of
sanitizing. In which case it makes sense to fold the sanitization logic into
SetReferrerForRequest.

[davidben, 2014-05-09 21:43:35]

+jochen since it looks like he added content::Referrer in
https://crbug.com/105028.

We should possibly sort out whether content::Referrer and WebCore::Referrer
contain pre-policy or post-policy[*] values and decide where to sanitize
consistent with that.

I started looking into what this would take and made some notes, but it's hairy
enough that we probably shouldn't block fixing this bug on it. Even within
Blink, WebCore::Referrer isn't very consistent and generateReferrerHeader gets
called more than once when, say, doing location.assign. I can look into cleaning
that up as a follow-up.

Though fixing the two might be orthogonal, so one possibility for this bug to
fix everything that creates an unsanitized content::Referrer. I found these
places, but they might not be comprehensive:

- RenderViewContextMenu::ExecuteCommand
- SaveAs in ppb_pdf_impl.cc
- PrepareDragForDownload in web_contents_view_aura.cc
- dragPromisedFileTo: in web_drag_source_mac.mm


[*] Arguably post-policy has two meanings. It could be pre- or post- initial
https -> http transition redaction. The former doesn't require the request URL
to compute it, the latter does.

https://codereview.chromium.org/277903002/diff/20001/content/browser/loader/r...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/277903002/diff/20001/content/browser/loader/r...
content/browser/loader/resource_dispatcher_host_impl.cc:198: const Referrer&
referrer) {
This would be the third copy of this logic that I can find. The other two are in

- SecurityOrigin::generateReferrerHeader.

- SerializedNavigationEntry::Sanitize.

I wonder if it makes sense to do this as a content::Referrer::Create(policy,
request_url, referrer_url) and share it between this and
SerializedNavigationEntry::Sanitize. (SerializedNavigationEntry::Sanitize also
includes an extra bit, which is that non-http and https URLs are stripped. And
then that one's missing the GetAsReferrer from here.)

jochen, is that consistent with what you meant for content::Referrer? (I.e. it
contains the post-policy, post-initial-https-http-stripping value.)

[jochen (gone - plz use gerrit), 2014-05-14 08:32:19]

https://codereview.chromium.org/277903002/diff/60001/content/browser/loader/r...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/277903002/diff/60001/content/browser/loader/r...
content/browser/loader/resource_dispatcher_host_impl.cc:2046: Referrer
SanitizeReferrerForRequest(const GURL& request,
maybe that should be on content::Referrer (a static method, so it stays a
struct)?

[jochen (gone - plz use gerrit), 2014-05-14 09:00:48]

alternatively, we could think about exposing the method on
SecurityPolicy::generateReferrerHeader and use that all over the place?

[davidben, 2014-05-14 15:37:40]

On 2014/05/14 09:00:48, jochen wrote:
> alternatively, we could think about exposing the method on
> SecurityPolicy::generateReferrerHeader and use that all over the place?

Looks like it is exposed already as WebSecurityPolicy::generateReferrerHeader.
Though I don't know how calling Blink code from the browser works... I guess
we'd want to add an entry to content/browser/DEPS' whitelist?

Are content::Referrer and WebCore::Referrer intended to store the pre-policy
URLs or the post-policy ones? I'm wondering if this should be a constructor
(static content::Referrer::Create(policy, request_url, referrer_url)) or if it's
intended to store pre-policy ones and we add a
content::Referrer::GenerateReferrerHeader(request_url) method for the entry
point where a Referrer gets converted to a header. Slight awkwardness of the
latter is that I believe Blink doesn't get informed if the browser process
changes the request, so document.referrer may not be accurate. We seem to
currently have a mix of the two.

[jochen (gone - plz use gerrit), 2014-05-15 10:47:13]

On 2014/05/14 15:37:40, David Benjamin wrote:
> On 2014/05/14 09:00:48, jochen wrote:
> > alternatively, we could think about exposing the method on
> > SecurityPolicy::generateReferrerHeader and use that all over the place?
> 
> Looks like it is exposed already as WebSecurityPolicy::generateReferrerHeader.
> Though I don't know how calling Blink code from the browser works... I guess
> we'd want to add an entry to content/browser/DEPS' whitelist?

I thought we had the weborigin bits extracted as a separate library, but
apparently that's not the case, so we can't easily call it :-/

> 
> Are content::Referrer and WebCore::Referrer intended to store the pre-policy
> URLs or the post-policy ones? I'm wondering if this should be a constructor
> (static content::Referrer::Create(policy, request_url, referrer_url)) or if
it's
> intended to store pre-policy ones and we add a
> content::Referrer::GenerateReferrerHeader(request_url) method for the entry
> point where a Referrer gets converted to a header. Slight awkwardness of the
> latter is that I believe Blink doesn't get informed if the browser process
> changes the request, so document.referrer may not be accurate. We seem to
> currently have a mix of the two.

It's supposed to be the post-policy value outside of blink. That doesn't hold
true if the URL + referrer comes in from the outside (e.g. context menu click or
something)

[cbentzel, 2014-05-15 13:21:08]

On 2014/05/15 10:47:13, jochen wrote:
> On 2014/05/14 15:37:40, David Benjamin wrote:
> > On 2014/05/14 09:00:48, jochen wrote:
> > > alternatively, we could think about exposing the method on
> > > SecurityPolicy::generateReferrerHeader and use that all over the place?
> > 
> > Looks like it is exposed already as
WebSecurityPolicy::generateReferrerHeader.
> > Though I don't know how calling Blink code from the browser works... I guess
> > we'd want to add an entry to content/browser/DEPS' whitelist?
> 
> I thought we had the weborigin bits extracted as a separate library, but
> apparently that's not the case, so we can't easily call it :-/
> 
> > 
> > Are content::Referrer and WebCore::Referrer intended to store the pre-policy
> > URLs or the post-policy ones? I'm wondering if this should be a constructor
> > (static content::Referrer::Create(policy, request_url, referrer_url)) or if
> it's
> > intended to store pre-policy ones and we add a
> > content::Referrer::GenerateReferrerHeader(request_url) method for the entry
> > point where a Referrer gets converted to a header. Slight awkwardness of the
> > latter is that I believe Blink doesn't get informed if the browser process
> > changes the request, so document.referrer may not be accurate. We seem to
> > currently have a mix of the two.
> 
> It's supposed to be the post-policy value outside of blink. That doesn't hold
> true if the URL + referrer comes in from the outside (e.g. context menu click
or
> something)

Context menu click could potentially contain a post-policy value, which was
something I considered.

The context menu details are ultimately built up using WebContextMenuData in
Blink (and sent from render->browser via a ContextMenuParams IPC value). Right
now the browser generates the referrer using either page_url or frame_url, but
we could also add a Referrer argument to WebContextMenuData and
ContextMenuParams and be sanitized.

[cbentzel, 2014-05-15 13:23:37]

However, there still may be other browser initiated places for referrers
which don't go through Blink. It's possible we could do a constructor for
those cases, and put additional CHECK logic in SetReferrerForRequest to
make sure everything is sanitized.


On Thu, May 15, 2014 at 9:21 AM, <cbentzel@chromium.org> wrote:

> On 2014/05/15 10:47:13, jochen wrote:
>
>> On 2014/05/14 15:37:40, David Benjamin wrote:
>> > On 2014/05/14 09:00:48, jochen wrote:
>> > > alternatively, we could think about exposing the method on
>> > > SecurityPolicy::generateReferrerHeader and use that all over the
>> place?
>> >
>> > Looks like it is exposed already as
>>
> WebSecurityPolicy::generateReferrerHeader.
>
>> > Though I don't know how calling Blink code from the browser works... I
>> guess
>> > we'd want to add an entry to content/browser/DEPS' whitelist?
>>
>
>  I thought we had the weborigin bits extracted as a separate library, but
>> apparently that's not the case, so we can't easily call it :-/
>>
>
>  >
>> > Are content::Referrer and WebCore::Referrer intended to store the
>> pre-policy
>> > URLs or the post-policy ones? I'm wondering if this should be a
>> constructor
>> > (static content::Referrer::Create(policy, request_url, referrer_url))
>> or if
>> it's
>> > intended to store pre-policy ones and we add a
>> > content::Referrer::GenerateReferrerHeader(request_url) method for the
>> entry
>> > point where a Referrer gets converted to a header. Slight awkwardness
>> of the
>> > latter is that I believe Blink doesn't get informed if the browser
>> process
>> > changes the request, so document.referrer may not be accurate. We seem
>> to
>> > currently have a mix of the two.
>>
>
>  It's supposed to be the post-policy value outside of blink. That doesn't
>> hold
>> true if the URL + referrer comes in from the outside (e.g. context menu
>> click
>>
> or
>
>> something)
>>
>
> Context menu click could potentially contain a post-policy value, which was
> something I considered.
>
> The context menu details are ultimately built up using WebContextMenuData
> in
> Blink (and sent from render->browser via a ContextMenuParams IPC value).
> Right
> now the browser generates the referrer using either page_url or frame_url,
> but
> we could also add a Referrer argument to WebContextMenuData and
> ContextMenuParams and be sanitized.
>
>
>
> https://codereview.chromium.org/277903002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jochen (gone - plz use gerrit), 2014-05-19 13:33:59]

After thinking a bit more about this:

we have two use cases for the referrer struct - one is recording the information
that has been sent and one for creating a request.

In the "has been sent" case, the referrer should match its policy. Note that
it's kinda difficult to "match the policy" without knowing the URL that was
requested, but hey, details...

I would claim that the remaining bugs in chrome are in the latter case - we
create a new request and either take an existing referrer or create a new one
without applying the policy.

So having a static method e.g. on content::Referrer that says create referrer
for request and takes a URL and a referrer might be the way to go, wdyt?

[davidben, 2014-05-19 17:16:20]

On 2014/05/19 13:33:59, jochen wrote:
> After thinking a bit more about this:
> 
> we have two use cases for the referrer struct - one is recording the
information
> that has been sent and one for creating a request.
> 
> In the "has been sent" case, the referrer should match its policy. Note that
> it's kinda difficult to "match the policy" without knowing the URL that was
> requested, but hey, details...
> 
> I would claim that the remaining bugs in chrome are in the latter case - we
> create a new request and either take an existing referrer or create a new one
> without applying the policy.
> 
> So having a static method e.g. on content::Referrer that says create referrer
> for request and takes a URL and a referrer might be the way to go, wdyt?

That sounds reasonable to me. Then we can go through and find all the places
that create {content,WebCore}::Referrer directly and rewire them to use the
constructor. When I looked at it last week, I think every place that creates
them also has access to the target URL, so we should be good. And after that, we
can clean things up and fix the places within the renderer where we double-apply
the policy right now. (It's idempotent, but good to be consistent about only
doing it at the entry-point to the referrer struct.)

[cbentzel, 2014-05-19 23:07:09]

On 2014/05/19 17:16:20, David Benjamin wrote:
> On 2014/05/19 13:33:59, jochen wrote:
> > After thinking a bit more about this:
> > 
> > we have two use cases for the referrer struct - one is recording the
> information
> > that has been sent and one for creating a request.
> > 
> > In the "has been sent" case, the referrer should match its policy. Note that
> > it's kinda difficult to "match the policy" without knowing the URL that was
> > requested, but hey, details...
> > 
> > I would claim that the remaining bugs in chrome are in the latter case - we
> > create a new request and either take an existing referrer or create a new
one
> > without applying the policy.
> > 
> > So having a static method e.g. on content::Referrer that says create
referrer
> > for request and takes a URL and a referrer might be the way to go, wdyt?
> 
> That sounds reasonable to me. Then we can go through and find all the places
> that create {content,WebCore}::Referrer directly and rewire them to use the
> constructor. When I looked at it last week, I think every place that creates
> them also has access to the target URL, so we should be good. And after that,
we
> can clean things up and fix the places within the renderer where we
double-apply
> the policy right now. (It's idempotent, but good to be consistent about only
> doing it at the entry-point to the referrer struct.)

Sounds reasonable to me too.

On my flight today I plumbed the context menu IPC to take a Referrer (instead of
just a policy) as well as Blink/Chromium side changes to handle this correctly
so these cases would be OK.

But I like the idea of the static method and we may need this in other cases.

[cbentzel, 2016-01-09 17:48:14]

Description was changed from

==========
Sanitize referrers for programmatic downloads.

These can come from context menu saves, download extensions, and other
locations.

BUG=369201,357473
==========

to

==========
Sanitize referrers for programmatic downloads.

These can come from context menu saves, download extensions, and other
locations.

BUG=369201,357473
==========