Stop supporting invalid CSP directives in meta tags

Stop supporting invalid CSP directives in meta tags

https://www.w3.org/TR/CSP2/#delivery-html-meta-element says that
frame-ancestors, sandbox, and report-uri should be discarded when
parsing Content Security Policies delivered in meta elements. This CL
changes CSPDirectiveList to discard such directives, log to the console
upon encountering them, and increment a UseCounter so we can see if this
breaks the web badly.

A bunch of layout tests used meta elements to set report-uris, so this
CL also updates these tests to deliver their CSPs in headers instead.

BUG=594645
Committed: https://crrev.com/f5c8a2b4ed6b3a3cb3bd148f5ede561419b1e40a
Cr-Commit-Position: refs/heads/master@{#385199}

[estark, 2016-03-24 22:51:49]

Description was changed from

==========
Stop supporting invalid CSP directives in meta tags

https://www.w3.org/TR/CSP2/#delivery-html-meta-element says that
frame-ancestors, sandbox, and report-uri should be discarded when
parsing Content Security Policies delivered in meta elements. This CL
changes CSPDirectiveList to discard such directives, log to the console
upon encountering them, and increment a UseCounter so we can see if this
breaks the web badly.

BUG=594645
==========

to

==========
Stop supporting invalid CSP directives in meta tags

https://www.w3.org/TR/CSP2/#delivery-html-meta-element says that
frame-ancestors, sandbox, and report-uri should be discarded when
parsing Content Security Policies delivered in meta elements. This CL
changes CSPDirectiveList to discard such directives, log to the console
upon encountering them, and increment a UseCounter so we can see if this
breaks the web badly.

A bunch of layout tests used meta elements to set report-uris, so this
CL also updates these tests to deliver their CSPs in headers instead.

BUG=594645
==========

[estark, 2016-03-31 00:38:19]

estark@chromium.org changed reviewers:
+ mkwst@chromium.org

[estark, 2016-03-31 00:38:20]

Hi Mike, welcome back! Could you take a look please? (Don't be frightened by the
list of files -- the vast majority of this is tweaking tests that relied on the
feature that I want to rip out.)

Also, I'm open to being convinced that we should move more slowly on this and
just track these invalid CSP directives in UseCounters before killing them
outright.

And finally, I'm getting a linker error on some of the bots that I don't
understand at all. Let me know if it makes any sense to you...

Thanks!

[Mike West, 2016-04-01 13:37:38]

On 2016/03/31 at 00:38:20, estark wrote:
> Hi Mike, welcome back! Could you take a look please? (Don't be frightened by
the list of files -- the vast majority of this is tweaking tests that relied on
the feature that I want to rip out.)

Hi! Thanks for this patch!

Sooooooo. We should absolutely get rid of `report-uri`. I have no idea why we
still support it... my fault, thanks for fixing it! `frame-ancestors` also seems
uncontroversial (though we do "support" `X-Frame-Options` in `<meta>` today...
it's probably worth measuring whether that functionality is at all used, and
removing it).

I'm not sure we should remove `sandbox`, however. There's no other way to put a
top-level page on one of the hojillion static shared-hosting sites into a
sandbox, and it seems intuitively valuable to be able to lock down an
application/page at runtime rather than at delivery time. Is there value in
doing so beyond spec compliance? Because that's easy to fix (see
https://w3c.github.io/webappsec-csp/#issue-8cff6e27). :)

> Also, I'm open to being convinced that we should move more slowly on this and
just track these invalid CSP directives in UseCounters before killing them
outright.

I don't think there's value in doing so. `report-uri` isn't something we should
support in the page itself, it is actively harmful to support. `frame-ancestors`
is worth removing now.  *shrug* Firefox doesn't support either of them in
`<meta>`, no other browser does either, and I suspect the usage is tiny. Let's
just drop them.
 
> And finally, I'm getting a linker error on some of the bots that I don't
understand at all. Let me know if it makes any sense to you...

Which bots? I see layout test failures related to sandbox being used all over
the place, but I didn't see the linker errors?

-mike

[Mike West, 2016-04-01 13:38:55]

So, in summary, `report-uri` and `frame-ancestors` LGTM. I'm not convinced about
`sandbox`, but let's chat. :)

[estark, 2016-04-01 17:51:53]

On 2016/04/01 13:37:38, Mike West (OOO through 31st) wrote:
> On 2016/03/31 at 00:38:20, estark wrote:
> > Hi Mike, welcome back! Could you take a look please? (Don't be frightened by
> the list of files -- the vast majority of this is tweaking tests that relied
on
> the feature that I want to rip out.)
> 
> Hi! Thanks for this patch!
> 
> Sooooooo. We should absolutely get rid of `report-uri`. I have no idea why we
> still support it... my fault, thanks for fixing it! `frame-ancestors` also
seems
> uncontroversial (though we do "support" `X-Frame-Options` in `<meta>` today...
> it's probably worth measuring whether that functionality is at all used, and
> removing it).
> 
> I'm not sure we should remove `sandbox`, however. There's no other way to put
a
> top-level page on one of the hojillion static shared-hosting sites into a
> sandbox, and it seems intuitively valuable to be able to lock down an
> application/page at runtime rather than at delivery time. Is there value in
> doing so beyond spec compliance? Because that's easy to fix (see
> https://w3c.github.io/webappsec-csp/#issue-8cff6e27). :)

I see 2 (okay, maybe more like 1.5) reasons besides spec compliance to get rid
of sandbox:

1.) It creates a bit of a mess for OOPIFs. Changing the origin at runtime
introduces a need for a whole new set of plumbing methods that we wouldn't need
otherwise. (I *think* meta sandboxing is the only way that a frame's origin can
change outside of a navigation committing.) And if you change sandbox flags at
runtime, it currently doesn't ever notify the browser process about it, which is
a bug that we'll need to fix if we keep this behavior around. I call this one
half of a reason because these are probably surmountable challenges if we decide
that it's worth keeping -- I'm mostly just noting that it introduces a fair
amount of complexity.

2.) It scares the heck out of me for a page to be able to change its origin at
runtime! What does it even mean? Like if a page whose origin is https://foo.com
loads a script, and then later the page changes to be a unique origin, what
origin is that first script now executing in?

Can you expand on the shared hosting use case a bit? Is the idea that I might
want to isolate my http://university.edu/~emily page from
http://university.edu/~mike? Could I accomplish the same thing (albeit in a
slightly clunkier way) by just putting my content in a big sandboxed iframe?

> 
> > Also, I'm open to being convinced that we should move more slowly on this
and
> just track these invalid CSP directives in UseCounters before killing them
> outright.
> 
> I don't think there's value in doing so. `report-uri` isn't something we
should
> support in the page itself, it is actively harmful to support.
`frame-ancestors`
> is worth removing now.  *shrug* Firefox doesn't support either of them in
> `<meta>`, no other browser does either, and I suspect the usage is tiny. Let's
> just drop them.
>  
> > And finally, I'm getting a linker error on some of the bots that I don't
> understand at all. Let me know if it makes any sense to you...
> 
> Which bots? I see layout test failures related to sandbox being used all over
> the place, but I didn't see the linker errors?

win_clang_x64_dbg is an example. Here's a log (though it might get cleaned up at
some point so I'll re-run the try bots to get a more recent one also):
https://build.chromium.org/p/tryserver.chromium.win/builders/win_clang_x64_db...

> 
> -mike

[Mike West, 2016-04-02 05:18:54]

On 2016/04/01 at 17:51:53, estark wrote:
> On 2016/04/01 13:37:38, Mike West (OOO through 31st) wrote:
> > On 2016/03/31 at 00:38:20, estark wrote:
> > > Hi Mike, welcome back! Could you take a look please? (Don't be frightened
by
> > the list of files -- the vast majority of this is tweaking tests that relied
on
> > the feature that I want to rip out.)
> > 
> > Hi! Thanks for this patch!
> > 
> > Sooooooo. We should absolutely get rid of `report-uri`. I have no idea why
we
> > still support it... my fault, thanks for fixing it! `frame-ancestors` also
seems
> > uncontroversial (though we do "support" `X-Frame-Options` in `<meta>`
today...
> > it's probably worth measuring whether that functionality is at all used, and
> > removing it).
> > 
> > I'm not sure we should remove `sandbox`, however. There's no other way to
put a
> > top-level page on one of the hojillion static shared-hosting sites into a
> > sandbox, and it seems intuitively valuable to be able to lock down an
> > application/page at runtime rather than at delivery time. Is there value in
> > doing so beyond spec compliance? Because that's easy to fix (see
> > https://w3c.github.io/webappsec-csp/#issue-8cff6e27). :)
> 
> I see 2 (okay, maybe more like 1.5) reasons besides spec compliance to get rid
of sandbox:
> 
> 1.) It creates a bit of a mess for OOPIFs. Changing the origin at runtime
introduces a need for a whole new set of plumbing methods that we wouldn't need
otherwise. (I *think* meta sandboxing is the only way that a frame's origin can
change outside of a navigation committing.) And if you change sandbox flags at
runtime, it currently doesn't ever notify the browser process about it, which is
a bug that we'll need to fix if we keep this behavior around. I call this one
half of a reason because these are probably surmountable challenges if we decide
that it's worth keeping -- I'm mostly just noting that it introduces a fair
amount of complexity.

Hrm. Let's talk about it on Monday? I'll send a calendar invite. I was planning
on reusing that functionality for
https://w3c.github.io/webappsec-clear-site-data/#neuter-contexts, so I'd like to
understand the complexity. I don't want to keep it around if it's broken and too
much work to fix, but I don't want to remove it _just_ because it's complex, you
know? :)

> 2.) It scares the heck out of me for a page to be able to change its origin at
runtime! What does it even mean? Like if a page whose origin is https://foo.com
loads a script, and then later the page changes to be a unique origin, what
origin is that first script now executing in?

Now the unique origin. It would still have things in memory, but would no longer
be able to persist them, poke at locally stored data, etc.

> Can you expand on the shared hosting use case a bit? Is the idea that I might
want to isolate my http://university.edu/~emily page from
http://university.edu/~mike? Could I accomplish the same thing (albeit in a
slightly clunkier way) by just putting my content in a big sandboxed iframe?

> > Which bots? I see layout test failures related to sandbox being used all
over
> > the place, but I didn't see the linker errors?
> 
> win_clang_x64_dbg is an example. Here's a log (though it might get cleaned up
at some point so I'll re-run the try bots to get a more recent one also):
https://build.chromium.org/p/tryserver.chromium.win/builders/win_clang_x64_db...

Ah, got it. You'll probably need to add `CORE_EXPORT` to `CSPDirectiveList`'s
class definition (because the test files are in a separate binary).

[estark, 2016-04-05 06:10:28]

Mike: if and when you feel convinced about dropping sandbox support, this is
ready for you to take another look. (Or, alternatively, if you still feel
unconvinced about dropping sandbox support, I can just drop the sandbox part of
this patch.)

I fixed some more tests and fixed the linker error. (Thanks for the CORE_EXPORT
tip, that did the trick.) Note: most of the test changes are a mechanical
transformation from meta elements to headers, but I did remove one test
outright, in
http/tests/serviceworker/ServiceWorkerGlobalScope/extendable-message-event.html.
Based on the discussion in https://codereview.chromium.org/1701843002, it sounds
like that test was added specifically to handle the case of a dynamically added
sandboxing policy, so it shouldn't be relevant anymore.

[Mike West, 2016-04-05 08:10:51]

On 2016/04/05 at 06:10:28, estark wrote:
> Mike: if and when you feel convinced about dropping sandbox support, this is
ready for you to take another look. (Or, alternatively, if you still feel
unconvinced about dropping sandbox support, I can just drop the sandbox part of
this patch.)
> 
> I fixed some more tests and fixed the linker error. (Thanks for the
CORE_EXPORT tip, that did the trick.) Note: most of the test changes are a
mechanical transformation from meta elements to headers, but I did remove one
test outright, in
http/tests/serviceworker/ServiceWorkerGlobalScope/extendable-message-event.html.
Based on the discussion in https://codereview.chromium.org/1701843002, it sounds
like that test was added specifically to handle the case of a dynamically added
sandboxing policy, so it shouldn't be relevant anymore.

I'm still not convinced! But LGTM anyway, because neither Firefox nor Edge has
implemented this and it's against the spec, so my fuzzy feelings aren't terribly
relevant.

[estark, 2016-04-05 15:32:02]

estark@chromium.org changed reviewers:
+ mpearson@chromium.org

[estark, 2016-04-05 15:32:04]

mpearson, can you please review histograms.xml?

[Mark P, 2016-04-05 16:56:32]

histograms.xml lgtm

[estark, 2016-04-05 16:57:45]

On 2016/04/05 08:10:51, Mike West (OOO through 31st) wrote:
> On 2016/04/05 at 06:10:28, estark wrote:
> > Mike: if and when you feel convinced about dropping sandbox support, this is
> ready for you to take another look. (Or, alternatively, if you still feel
> unconvinced about dropping sandbox support, I can just drop the sandbox part
of
> this patch.)
> > 
> > I fixed some more tests and fixed the linker error. (Thanks for the
> CORE_EXPORT tip, that did the trick.) Note: most of the test changes are a
> mechanical transformation from meta elements to headers, but I did remove one
> test outright, in
>
http/tests/serviceworker/ServiceWorkerGlobalScope/extendable-message-event.html.
> Based on the discussion in https://codereview.chromium.org/1701843002, it
sounds
> like that test was added specifically to handle the case of a dynamically
added
> sandboxing policy, so it shouldn't be relevant anymore.
> 
> I'm still not convinced! But LGTM anyway, because neither Firefox nor Edge has
> implemented this and it's against the spec, so my fuzzy feelings aren't
terribly
> relevant.

Don't lose hope, if it turns out that it breaks the internet I'll roll it back
promptly. :)

[estark, 2016-04-05 16:58:06]

The CQ bit was checked by estark@chromium.org

[commit-bot: I haz the power, 2016-04-05 16:58:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1835463002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1835463002/160001

[commit-bot: I haz the power, 2016-04-05 17:06:08]

Description was changed from

==========
Stop supporting invalid CSP directives in meta tags

https://www.w3.org/TR/CSP2/#delivery-html-meta-element says that
frame-ancestors, sandbox, and report-uri should be discarded when
parsing Content Security Policies delivered in meta elements. This CL
changes CSPDirectiveList to discard such directives, log to the console
upon encountering them, and increment a UseCounter so we can see if this
breaks the web badly.

A bunch of layout tests used meta elements to set report-uris, so this
CL also updates these tests to deliver their CSPs in headers instead.

BUG=594645
==========

to

==========
Stop supporting invalid CSP directives in meta tags

https://www.w3.org/TR/CSP2/#delivery-html-meta-element says that
frame-ancestors, sandbox, and report-uri should be discarded when
parsing Content Security Policies delivered in meta elements. This CL
changes CSPDirectiveList to discard such directives, log to the console
upon encountering them, and increment a UseCounter so we can see if this
breaks the web badly.

A bunch of layout tests used meta elements to set report-uris, so this
CL also updates these tests to deliver their CSPs in headers instead.

BUG=594645
==========

[commit-bot: I haz the power, 2016-04-05 17:06:09]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-04-05 17:07:57]

Description was changed from

==========
Stop supporting invalid CSP directives in meta tags

https://www.w3.org/TR/CSP2/#delivery-html-meta-element says that
frame-ancestors, sandbox, and report-uri should be discarded when
parsing Content Security Policies delivered in meta elements. This CL
changes CSPDirectiveList to discard such directives, log to the console
upon encountering them, and increment a UseCounter so we can see if this
breaks the web badly.

A bunch of layout tests used meta elements to set report-uris, so this
CL also updates these tests to deliver their CSPs in headers instead.

BUG=594645
==========

to

==========
Stop supporting invalid CSP directives in meta tags

https://www.w3.org/TR/CSP2/#delivery-html-meta-element says that
frame-ancestors, sandbox, and report-uri should be discarded when
parsing Content Security Policies delivered in meta elements. This CL
changes CSPDirectiveList to discard such directives, log to the console
upon encountering them, and increment a UseCounter so we can see if this
breaks the web badly.

A bunch of layout tests used meta elements to set report-uris, so this
CL also updates these tests to deliver their CSPs in headers instead.

BUG=594645

Committed: https://crrev.com/f5c8a2b4ed6b3a3cb3bd148f5ede561419b1e40a
Cr-Commit-Position: refs/heads/master@{#385199}
==========

[commit-bot: I haz the power, 2016-04-05 17:07:59]

Patchset 9 (id:??) landed as
https://crrev.com/f5c8a2b4ed6b3a3cb3bd148f5ede561419b1e40a
Cr-Commit-Position: refs/heads/master@{#385199}