Bug 179728 – CSP: Hide nonce values from the DOM

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Mike West]

(Copy/pasting from Mozilla's https://bugzilla.mozilla.org/show_bug.cgi?id=1374612)

"""
Prevent nonce exfiltration via CSS selectors and similar tricks. By hiding the nonce from the DOM those kind of CSP policy bypasses can be prevented.

See also:
https://github.com/whatwg/html/pull/2373
https://github.com/w3c/web-platform-tests/tree/master/content-security-policy/nonce-hiding
"""

[Radar WebKit Bug Importer]

<rdar://problem/35559902>

[Mike West]

Note that Mozilla implemented this for Firefox 75: https://bugzilla.mozilla.org/show_bug.cgi?id=1374612.

[Philip Jägenstedt]

A number of tests around this are now failing in only Safari:
https://wpt.fyi/results/content-security-policy/nonce-hiding?label=master&label=experimental&product=chrome&product=firefox&product=safari&aligned

A trivial demo of it is here: https://software.hixie.ch/utilities/js/live-dom-viewer/saved/8672

(I found my way here from looking into https://github.com/mdn/browser-compat-data/pull/7303.)

[Patrick Griffis]

Can somebody confirm this is still a problem?

Using the demo Philip linked on Firefox 93, Chrome 94, WebKitGTK (2.35), and Safari iOS, all behave the same.

Also running the WPT locally[0] with WebKitGTK results in all tests passing. I see that the wpt.fyi still shows failures though.


[0] run-webkit-tests 'imported/w3c/web-platform-tests/content-security-policy/nonce-hiding' --verbose --force

[Philip Jägenstedt]

Testing http://wpt.live/content-security-policy/nonce-hiding/script-nonces-hidden.html in Safari Technology Preview 133 I still see failures.

The test doesn't pass in WebKitGTK on wpt.fyi either:
https://wpt.fyi/results/content-security-policy/nonce-hiding/script-nonces-hidden.html?label=master&product=chrome%5Bexperimental%5D&product=firefox%5Bexperimental%5D&product=safari%5Bexperimental%5D&product=webkitgtk&aligned

Maybe this is behind a flag that's only enabled for testing?

[Patrick Griffis]

Created attachment 443206 [details]
Patch

[Patrick Griffis]

Created attachment 443208 [details]
Patch

[Kate Cheney]

Comment on attachment 443208 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=443208&action=review

Nice to see so many tests passing! Had one q but looks good overall.

> Source/WebCore/dom/Element.cpp:300
> +    if (nonce == emptyAtom() && !hasRareData())

I think you can use nonce.isEmpty(). Also, it seems like if you use ensureElementRareData() below you either don't need to check for !hasRareData() here or you should use elementRareData() below instead.

> Source/WebCore/dom/Element.cpp:316
> +    if (!csp->isHeaderDelivered())

I don't think I understand why we only check for a header-delivered CSP policy here, could you explain?

> Source/WebCore/dom/ElementRareData.h:183
> +    AtomString m_nonce;

Seems like this should maybe be added to OptionSet<UseType> useTypes() like other members of the class.

> Source/WebCore/html/HTMLElement.cpp:982
> +    if (value != emptyAtom())

If !value.isEmpty().

> Source/WebCore/svg/SVGElement.cpp:1009
> +    if (value != emptyAtom())

Ditto, if (!value.isEmpty()).

[Kate Cheney]

Also looks like the imported/w3c/web-platform-tests/html/dom/reflection-misc.html failure might be related.

[Patrick Griffis]

(In reply to Kate Cheney from comment #8)
> > Source/WebCore/dom/Element.cpp:300
> > +    if (nonce == emptyAtom() && !hasRareData())
> 
> I think you can use nonce.isEmpty(). Also, it seems like if you use
> ensureElementRareData() below you either don't need to check for
> !hasRareData() here or you should use elementRareData() below instead.

In the case where its set to a not empty string we need to ensure we have rare data.
This was admittedly just an "optimization" to avoid creating it when its the default value.

> > Source/WebCore/dom/Element.cpp:316
> > +    if (!csp->isHeaderDelivered())
> 
> I don't think I understand why we only check for a header-delivered CSP
> policy here, could you explain?

This is documented here: https://html.spec.whatwg.org/multipage/urls-and-fetching.html#nonce-attributes

> If CSP list contains a header-delivered Content Security Policy ...

And the test cases with `meta` in the file paths test the header vs not-header cases.

[Patrick Griffis]

(In reply to Kate Cheney from comment #8)
> > Source/WebCore/html/HTMLElement.cpp:982
> > +    if (value != emptyAtom())
> 
> If !value.isEmpty().
> 
> > Source/WebCore/svg/SVGElement.cpp:1009
> > +    if (value != emptyAtom())
> 
> Ditto, if (!value.isEmpty()).

.isEmpty() is true for nullAtom() which we do not want and they are supposed to be treated differently.

[Patrick Griffis]

(In reply to Kate Cheney from comment #9)
> Also looks like the
> imported/w3c/web-platform-tests/html/dom/reflection-misc.html failure might
> be related.

Updated the baseline, script.nonce isn't reflected on purpose and Chromium has the same result as we do now.

[Patrick Griffis]

Created attachment 443313 [details]
Patch

[Kate Cheney]

(In reply to Patrick Griffis from comment #10)
> (In reply to Kate Cheney from comment #8)
> > > Source/WebCore/dom/Element.cpp:300
> > > +    if (nonce == emptyAtom() && !hasRareData())
> > 
> > I think you can use nonce.isEmpty(). Also, it seems like if you use
> > ensureElementRareData() below you either don't need to check for
> > !hasRareData() here or you should use elementRareData() below instead.
> 
> In the case where its set to a not empty string we need to ensure we have
> rare data.
> This was admittedly just an "optimization" to avoid creating it when its the
> default value.
> 

Ok, this makes sense.

> > > Source/WebCore/dom/Element.cpp:316
> > > +    if (!csp->isHeaderDelivered())
> > 
> > I don't think I understand why we only check for a header-delivered CSP
> > policy here, could you explain?
> 
> This is documented here:
> https://html.spec.whatwg.org/multipage/urls-and-fetching.html#nonce-
> attributes
> 
> > If CSP list contains a header-delivered Content Security Policy ...
> 
> And the test cases with `meta` in the file paths test the header vs
> not-header cases.

Got it, I missed this in the spec.

(In reply to Patrick Griffis from comment #11)

> (In reply to Kate Cheney from comment #8)
> > > Source/WebCore/html/HTMLElement.cpp:982
> > > +    if (value != emptyAtom())
> > 
> > If !value.isEmpty().
> > 
> > > Source/WebCore/svg/SVGElement.cpp:1009
> > > +    if (value != emptyAtom())
> > 
> > Ditto, if (!value.isEmpty()).
> 
> .isEmpty() is true for nullAtom() which we do not want and they are supposed
> to be treated differently.

Based on steps 3 and 4 of the spec:

The following attribute change steps are used for the nonce content attribute:

1. If element does not include HTMLOrSVGElement, then return.

2. If localName is not nonce or namespace is not null, then return.

3. If value is null, then set element's [[CryptographicNonce]] to the empty string.

4. Otherwise, set element's [[CryptographicNonce]] to value.

it seems like null and empty string are treated the same, in both cases we set the nonce to be empty string. So why do we want to treat them differently here? Sorry if I am missing something obvious, just want to make sure I understand.

[Chris Dumez]

Comment on attachment 443313 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=443313&action=review

> Source/WebCore/dom/Element.cpp:319
> +    setAttribute(nonceAttr, emptyAtom());

Spec says:
```
2. Set an attribute value for element using "nonce" and the empty string.
3. Set element's [[CryptographicNonce]] to nonce.
```

Why are we doing step 2 but not step 3.

> Source/WebCore/dom/Element.h:664
> +    virtual void nonceAttributeChanged(const AtomString&);

I am not convinced this needs to be virtual, the implementation for HTMLElement and SVGElement. I would suggest we make this non-virtual, move the implementation here and do a is<SVGElement>() || is<HTMLElement>() check as in the spec.

Alternatively, since this is called only from Element::attributeChanged() and since Element::attributeChanged() is already virtual. Could could also, drop the virtual on nonceAttributeChanged() and call it from HTMLElement::attributeChanged() and SVGElement::attributeChanged() (instead of from Element::attributeChanged()). This would avoid an extra virtual function call.

> Source/WebCore/html/HTMLElement.cpp:505
> +    hideNonce();

Note that in the spec, it doesn't merely hide the Nonce, it also sets element's [[CryptographicNonce]] to nonce.

> Source/WebCore/html/HTMLElement.cpp:983
> +        setNonce(value);

The spec says:
"""
The following attribute change steps are used for the nonce content attribute:
1. If element does not include HTMLOrSVGElement, then return.
2. If localName is not nonce or namespace is not null, then return.
3. If value is null, then set element's [[CryptographicNonce]] to the empty string.
4. Otherwise, set element's [[CryptographicNonce]] to value.
"""

I believe your implementation is missing step 3.

> Source/WebCore/html/HTMLOrForeignElement.idl:31
> +    [CEReactions] attribute DOMString nonce;

The spec (https://html.spec.whatwg.org/multipage/dom.html#htmlorsvgelement) says:
`attribute DOMString nonce; // intentionally no [CEReactions]`

Why are you adding `[CEReactions]` when the spec says it intentionally doesn't have it?

> Source/WebCore/html/HTMLScriptElement.cpp:76
> +void HTMLScriptElement::nonceAttributeChanged(const AtomString& value)

Why do we need to override nonceAttributeChanged() simply to call the base class implementation. It seems it shouldn't be needed?

> Source/WebCore/page/csp/ContentSecurityPolicy.h:188
> +    bool isHeaderDelivered() { return m_isHeaderDelivered; }

This getter should be marked const.

> Source/WebCore/svg/SVGElement.cpp:1009
> +    if (value != emptyAtom())

Same comment as for HTMLElement::nonceAttributeChanged()

[Patrick Griffis]

Created attachment 443596 [details]
Patch

[Chris Dumez]

Comment on attachment 443596 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=443596&action=review

r=me with a few minor fixes (mostly WebKit coding style).

> Source/WebCore/dom/Element.cpp:340
> +    // Retain previous IDL nonce

nit: WebKit comments need to end with a period, as per coding style.

> Source/WebCore/dom/Element.cpp:341
> +    const AtomString currentNonce = nonce();

nit: I don't think the "const" is really helpful here and we rarely use const variables in WebKit.

> Source/WebCore/dom/Element.cpp:1896
> +            if (is<HTMLElement>(this) || is<SVGElement>(this)) {

The following may be a little more efficient as it may avoid a null check:
`if (is<HTMLElement>(*this) || is<SVGElement>(*this))`

> Source/WebCore/dom/Element.cpp:1897
> +                if (newValue.isNull())

nit: I'd recommend a ternary to make the code more concise:
setNonce(newValue.isNull() ? emptyAtom() : newValue);

> Source/WebCore/dom/Element.h:362
> +    // Used by the HTMLElement, HTMLScriptElement, and SVGElement IDLs

nit: I don't think this comment should still mention HTMLScriptElement.

> Source/WebCore/html/HTMLElement.cpp:505
> +    hideNonce();

Just curious, would this work?
```
hideNonce();
return Element::insertedIntoAncestor(insertionType, containerNode);
```

It would be simpler so I am wondering if there is a good reason to use the ordering in your patch.

> Source/WebCore/page/csp/ContentSecurityPolicy.cpp:198
> +    } else if (policyFrom == PolicyFrom::HTTPHeader) {

nit: Per WebKit coding style, no curly brackets for one-liners.

> LayoutTests/imported/w3c/web-platform-tests/html/dom/reflection-misc-expected.txt:970
> +FAIL script.nonce: IDL set to "" assert_equals: getAttribute() expected "" but got "test-valueOf"

I agree the test is wrong here. Would be nice to file a bug against WPT though.

[Patrick Griffis]

(In reply to Chris Dumez from comment #17)
> Comment on attachment 443596 [details]
> 
> > Source/WebCore/html/HTMLElement.cpp:505
> > +    hideNonce();
> 
> Just curious, would this work?
> ```
> hideNonce();
> return Element::insertedIntoAncestor(insertionType, containerNode);
> ```
> 
> It would be simpler so I am wondering if there is a good reason to use the
> ordering in your patch.
> 

No it fails. I believe it updates state for `isConnected()` to be correct.

[Chris Dumez]

(In reply to Patrick Griffis from comment #18)
> (In reply to Chris Dumez from comment #17)
> > Comment on attachment 443596 [details]
> > 
> > > Source/WebCore/html/HTMLElement.cpp:505
> > > +    hideNonce();
> > 
> > Just curious, would this work?
> > ```
> > hideNonce();
> > return Element::insertedIntoAncestor(insertionType, containerNode);
> > ```
> > 
> > It would be simpler so I am wondering if there is a good reason to use the
> > ordering in your patch.
> > 
> 
> No it fails. I believe it updates state for `isConnected()` to be correct.

Ok, thanks for letting me know.

[Patrick Griffis]

Created attachment 443599 [details]
Patch

[Patrick Griffis]

Created attachment 443605 [details]
Patch for landing

[EWS]

Found 1 new test failure: imported/w3c/web-platform-tests/html/dom/reflection-misc.html

[Chris Dumez]

Comment on attachment 443605 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=443605&action=review

> LayoutTests/imported/w3c/web-platform-tests/html/dom/reflection-misc-expected.txt:970
> +FAIL script.nonce: IDL set to "" assert_equals: getAttribute() expected "" but got "test-valueOf"

Looks like Mac-wk2 has its own baseline and needs to be rebaselined too. Same will apply to iOS-wk2.

[Patrick Griffis]

Created attachment 443638 [details]
Patch for landing

[EWS]

Committed r285478 (244002@main): <https://commits.webkit.org/244002@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 443638 [details].

[Chris Dumez]

Comment on attachment 443638 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=443638&action=review

> LayoutTests/platform/mac-wk2/imported/w3c/web-platform-tests/html/dom/reflection-misc-expected.txt:970
> +FAIL script.nonce: IDL set to "" assert_equals: getAttribute() expected "" but got "test-valueOf"

Did you file a bug against this WPT test upstream since we believe the test is wrong.
It would be good to fix this test upstream (or at the very least file a bug) since this regressed our WPT pass rate.

[Patrick Griffis]

(In reply to Chris Dumez from comment #26)
> Comment on attachment 443638 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=443638&action=review
> 
> > LayoutTests/platform/mac-wk2/imported/w3c/web-platform-tests/html/dom/reflection-misc-expected.txt:970
> > +FAIL script.nonce: IDL set to "" assert_equals: getAttribute() expected "" but got "test-valueOf"
> 
> Did you file a bug against this WPT test upstream since we believe the test
> is wrong.
> It would be good to fix this test upstream (or at the very least file a bug)
> since this regressed our WPT pass rate.

https://github.com/web-platform-tests/wpt/pull/32477