1397308 - Implement "is element nonceable?" CSP checks

Status: RESOLVED FIXED

(Core :: DOM: Security, enhancement, P1)

Description

[Jack Will]

PoC:

<meta http-equiv="Content-Security-Policy" content="script-src 'nonce-abcd'">
<script src=data:,alert(1) <script nonce="abcd" > </script>

Comment 1

[:Gijs (he/him)]

Christoph, can you take a look at what is (not) going on here?

Comment 2

[Daniel Veditz [:dveditz]]

Henri: is the parser doing the right thing here? The symptoms look like both the src and nonce attributes are being applied to the same <script> element.


Not sure how useful this would be in an attack. You'd need an injection point immediately before a legit <script nonce=> tag because you wouldn't otherwise know what nonce to use.

Comment 3

[Henri Sivonen (:hsivonen)]

Firefox behaves as expected per spec (which is based on the IE6 behavior, in this case, IIRC).

The second "<script" becomes an attribute of the first, because < falls under "anything else" at https://html.spec.whatwg.org/multipage/parsing.html#before-attribute-name-state

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Henri Sivonen (:hsivonen) from comment #3)
> Firefox behaves as expected per spec (which is based on the IE6 behavior, in
> this case, IIRC).
> 
> The second "<script" becomes an attribute of the first, because < falls
> under "anything else" at
> https://html.spec.whatwg.org/multipage/parsing.html#before-attribute-name-
> state

Given that, I think this becomes a WORKSFORME. If the second script becomes an attribute then the nonce applies to the outer script and should execute because the nonce matches.

Comment 5

[Daniel Veditz [:dveditz]]

Mike: are you happy with Henri's conclusion that this is correct behavior? It may be correct parsing, but is it how CSP ought to work? This means that if your page were unfortunate enough to have an injection point immediately prior to a <script> tag with a nonce, someone could inject arbitrary script.

Comment 6

[Mike West]

1.  This is correct parsing per spec.

2.  CSP attempts to mitigate this via the checks in https://w3c.github.io/webappsec-csp/#is-element-nonceable, which fail the nonce check if an attribute's name or value contains "<script" or "<style". I've had those behind a flag in Chrome forever, and should have shipped them a long time ago.

Comment 7

[Daniel Veditz [:dveditz]]

We have not implemented that part of the CSP3 spec.

Comment 8

[Daniel Veditz [:dveditz]]

We can unhide this since this interaction between correct parsing and the negative impacts on CSP are documented in the CSP spec itself.

Comment 10

[Daniel Veditz [:dveditz]]

Not sure how useful this would be in an attack. You'd need an injection point immediately before a legit <script nonce=> tag because you wouldn't otherwise know what nonce to use.

As seen in bug 1761215 and the linked Chrome bug, injecting an attribute with an unclosed quote is one way to consume a bunch of page content between the injection and the next script tag if there's no intervening quote. Makes it a little more practical.

Comment 12

[Pedro Bernardo [:pedrob]]

Hello, co-reporter of bug#1817272 here.

We investigated the root cause of the issue and tested it against Chrome, Firefox, and Safari. This is a summary of our findings.

The problem, as mentioned in previous comments, stems from not implementing the Is element nonceable? algorithm from the CSP spec.

To be "Nonceable", an element:

1. must contain an attribute named nonce
2. if the element is a <script>, then for each of the attributes in its attribute list, the attribute's name and value must not contain the strings <script or <style.
3. must not raise a duplicate-attribute parse error during tokenization.

Otherwise, the element is "Not Nonceable" and the nonce attribute is ignored by the Content-Security-Policy.

Notice that point 3 is required to prevent a bypass due to a dangling markup attack.

According to our tests, only Firefox does not implement this part of the CSP. Honestly, we were quite surprised to (re)discover this issue, since it breaks our expectations on the security guarantees provided by the CSP, and it would be great to get it fixed in Firefox.

Comment 13

[Tom Schuster (MoCo)]

Hi Henri! Looking at the specification for 6.7.3.1. Is element nonceable? most of the implementation would presumably happen in the HTML parser. How hard do you think it would be to bubble up this information?

Comment 14

[Henri Sivonen (:hsivonen)]

It looks like in 2018 I've suggested making any script with any duplicate attribute non-executable: https://github.com/whatwg/html/issues/3257#issuecomment-437786069

Anyway:

It would be easy to add a "had a duplicate attribute" flag to the nsHtml5HtmlAttributes object, since we don't need to record which attribute was duplicated.

What code would look at that flag and where would be mainly a matter of performance and micro optimization depending on which elements need to do something with the flag, whether they need to know about the flag already upon construction, etc.

Comment 15

[Tom Schuster (MoCo)]

Attachment: Bug 1397308 - Implement CSP 'Is element nonceable?' check. r?emilio!,hsivonen!,freddyb!

Comment 16

[Tom Schuster (MoCo)]

So, I am wondering if this approach makes sense. I added a new ELEMENT_FLAG_BIT for ELEMENT_PARSER_HAD_DUPLICATE_ATTR_ERROR based on a flag that is set by the parser on nsHtml5HtmlAttributes in nsHtml5Tokenizer::errDuplicateAttribute.

1. Is there a better place to put the flag? (i.e. do we worry about running out?)
2. Should we clone the flag? I think the following test case shows that Chrome doesn't, but it seems almost overkill when trying to prevent simple dangling markup injections.

<template><script nonce="abc" nonce="abc">console.warn("executed");</script></template>
<script nonce="abc">
let template = document.querySelector("template");
// This doesn't execute
// document.body.append(template.content.firstChild)
document.body.append(template.content.firstChild.cloneNode(true))
</script>

Comment 17

[Tom Schuster (MoCo)]

Hi Andrew, can you have a look at or redirect the patch?

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/16329a884928
Implement CSP 'Is element nonceable?' check. r=emilio,hsivonen,freddyb

Comment 19

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/44228 for changes under testing/web-platform/tests

Comment 20

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/16329a884928

Comment 21

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot