1757604 - (CVE-2022-31744) Stylesheet's CSP bypass via reflected URL in chrome:// directories still broken

Status: VERIFIED FIXED

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

Description

[:Gijs (he/him)]

+++ This bug was initially created as a clone of Bug #1460538 +++

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:60.0) Gecko/20100101 Firefox/60.0
Build ID: 20180503143129

Steps to reproduce:

By using the reflected URL in some special resource URIs it is possible to inject CSS and bypass CSP: http://demo.vwzq.net/php/ff_chrome.php

Stylesheet injection is not XSS, but still harmful.

Content:
<link rel="stylesheet" href="chrome://browser/content/places/?'{}{color:red}">

Actual results:

The reflected stylesheet is interpreted bypassing the strict CSP policy.

Expected results:

I guess the mitigation here should be to avoid reflecting any user content, or at least avoid loading directories.

---

This appears to still be broken.

Comment 1

[:Gijs (he/him)]

So the fix from bug 1460538 tried to use the content type in comparison with the request type to block requests where the two didn't match.

This didn't work because in a packaged build, we hit Loader::LoadSheet which explicitly sets the content type "hint" to text/css, so then the content type is what you'd expect for stylesheets, and the load passes, even though the loaded URL really isn't a text/css document.

In a non-packaged local build, it turns out we don't hit this code path because we strip the query params in file URLs so the exploit doesn't work anyway.

We could work around this by having the nsJARChannel code explicitly check for directory entries but that feels very ugly. Emilio, do you know why the stylesheet code is forcing the mimetype here, and/or if there is some other getter we should be using to get a "real" content type? Blame on those lines is nearly 20 years old so I'm not sure where to go from there. :-(

As noted in my last comments in bug 1460538, the change in bug 1488061 then also didn't work because it changes the HTML output but not the indexed stream - to fix things that way we need to change https://searchfox.org/mozilla-central/rev/8f42809e51cb07aa4f5739932a06d14581e9dd4a/netwerk/base/nsDirectoryIndexStream.cpp#100,102-103 to strip query params for file and/or jar and/or chrome (or rather, only include it for... well, for what, exactly?)

Comment 2

[Frederik Braun [:freddy]]

We also escape HTML-entities in the chrome://browser/content/places/? reflection, but not CSS-relevant characters. That's also something we could change.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

So I'm missing some context because I'm not cc'd on those bugs mentioned in comment 1, but IIUC the issue is that directory entries have the whole URI including query string in the response, like:

300: jar:file:///home/emilio/firefox/browser/omni.ja!/chrome/browser/content/browser/places/?fwaefwafwafwafwaefw
200: filename content-length last-modified file-type
201: bookmarkProperties.js 18045 Thu,%2031%20Dec%202009%2023:00:00%20GMT FILE
201: bookmarkProperties.xhtml 7644 Thu,%2031%20Dec%202009%2023:00:00%20GMT FILE
...

And that causes a CSS injection if interpreted as a stylesheet. My question is, why is the load not failing before? For example trying to fetch("chrome://browser/content/places/") fails with a SecurityError which comes from here, is any similar check not working?

Anyways, bug 120789 is what introduced that code. From taking a look, if the nsIChannel overrides the content-type post-response, we'd reject the load here. However it seems the JAR channel is intentionally always respecting the content type set as a hint:

https://searchfox.org/mozilla-central/rev/292d17c13daa61016fd082e2337297091d53a015/modules/libjar/nsJARChannel.cpp#767-768

So in terms of potential fixes, the easiest one is to perhaps use the content-type hint only if the guess is unknown?

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1757604 - Make content-type guess override content-type hint for JAR channels. r=Gijs

Make the script loader fail to load non-JS internal loads too, like the
CSS loader does for all cross-origin files.

Comment 5

[:Gijs (he/him)]

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

So I'm missing some context because I'm not cc'd on those bugs mentioned in comment 1,

Gah, sorry, copied you in.

but IIUC the issue is that directory entries have the whole URI including query string in the response, like:

300: jar:file:///home/emilio/firefox/browser/omni.ja!/chrome/browser/content/browser/places/?fwaefwafwafwafwaefw
200: filename content-length last-modified file-type
201: bookmarkProperties.js 18045 Thu,%2031%20Dec%202009%2023:00:00%20GMT FILE
201: bookmarkProperties.xhtml 7644 Thu,%2031%20Dec%202009%2023:00:00%20GMT FILE
...

And that causes a CSS injection if interpreted as a stylesheet. My question is, why is the load not failing before? For example trying to fetch("chrome://browser/content/places/") fails with a SecurityError which comes from here, is any similar check not working?

I'm confused. My understanding is that style and script loads are allowed to be cross-origin, whereas fetch needs CORS support for that. As a result, I don't think the two are the same.

A lot of the chrome/toolkit packaged styling and script can be referenced from the web because it's marked as contentaccessible. We should fix that separately but it's not really the root issue here (ie as long as any web-reference-able chrome/resource URI would exist, any of them would facilitate this attack).

Anyways, bug 120789 is what introduced that code. From taking a look, if the nsIChannel overrides the content-type post-response, we'd reject the load here. However it seems the JAR channel is intentionally always respecting the content type set as a hint:

https://searchfox.org/mozilla-central/rev/292d17c13daa61016fd082e2337297091d53a015/modules/libjar/nsJARChannel.cpp#767-768

So in terms of potential fixes, the easiest one is to perhaps use the content-type hint only if the guess is unknown?

Makes sense to me.

Comment 6

[:Gijs (he/him)]

Passing the assignee to Emilio as he's put up a patch - thank you!

Comment 7

[Frederik Braun [:freddy]]

@emilio: Can you try and nudge this along? It seems (almost) done?

Comment 8

[Frederik Braun [:freddy]]

(In reply to Frederik Braun [:freddy] from comment #7)

@emilio: Can you try and nudge this along? It seems (almost) done?

Comment 9

[Kris Maglione [:kmag]]

I would lean towards simply making it impossible to load those stylesheets except when they were inserted by privileged callers.

Comment 10

[:Gijs (he/him)]

(In reply to Kris Maglione [:kmag] from comment #9)

I would lean towards simply making it impossible to load those stylesheets except when they were inserted by privileged callers.

This would effectively "undo" contentaccessible, right? And would need a carve-out for some about pages in addition to the system principal case...

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1757604 - Make content-type on JAR channels behave the same as HTTP channels. r=Gijs

That is, treat it as a hint if called before open, and as an override if
called after. Override the hint on open.

This is a less invasive change that is green on try and also fixes the
issue.

Comment 12

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

Make content-type on JAR channels behave the same as HTTP channels. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/6192634b7401c3d4f1ee8abfe310bd68dcd5ac1f
https://hg.mozilla.org/mozilla-central/rev/6192634b7401

Comment 13

[Kris Maglione [:kmag]]

(In reply to :Gijs (he/him) from comment #10)

(In reply to Kris Maglione [:kmag] from comment #9)

I would lean towards simply making it impossible to load those stylesheets except when they were inserted by privileged callers.

This would effectively "undo" contentaccessible, right? And would need a carve-out for some about pages in addition to the system principal case...

Yes, for a large part. But I think we probably want that, anyway. Allowing for about: pages would be easy. I mostly just don't think that content should be directly injectable by web content. Especially now that we have the infrastructure to make it injectable by a privileged caller without allowing web content.

Comment 14

[BugBot [:suhaib / :marco/ :calixte]]

Changing the priority to P1 as the bug is tracked by a release manager for the current beta.
See Triage for Bugzilla for more information.
If you disagree, please discuss with a release manager.

Comment 15

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:emilio, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Gijs, your call.

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Wanted to write Gijs / Freddy, d'oh :-)

Comment 18

[Frederik Braun [:freddy]]

Sure, if it applies cleanly. Why not?

Comment 19

[:Gijs (he/him)]

(In reply to Frederik Braun [:freddy] from comment #18)

Sure, if it applies cleanly. Why not?

I think my only reservation is risk. I don't feel like I have a great understanding of potential fall-out, e.g. for add-ons. But it's super early in beta and we're likely not going to find out about fallout on nightly anyway - and on the flip side, this is fixing a publicly-disclosed issue, so on the whole uplifting seems like a good idea to me.

Comment 20

[:Gijs (he/him)]

(In reply to Kris Maglione [:kmag] from comment #13)

(In reply to :Gijs (he/him) from comment #10)

(In reply to Kris Maglione [:kmag] from comment #9)

I would lean towards simply making it impossible to load those stylesheets except when they were inserted by privileged callers.

This would effectively "undo" contentaccessible, right? And would need a carve-out for some about pages in addition to the system principal case...

Yes, for a large part. But I think we probably want that, anyway. Allowing for about: pages would be easy. I mostly just don't think that content should be directly injectable by web content. Especially now that we have the infrastructure to make it injectable by a privileged caller without allowing web content.

OK. I think there are other reasons we want this (e.g. bug 1530499). I'll follow up there.

Comment 21

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9274452 [details]
Bug 1757604 - Make content-type on JAR channels behave the same as HTTP channels. r=Gijs

Beta/Release Uplift Approval Request

• User impact if declined: see comment 19. I don't think risk is too big fwiw. We already heavily restrict add-on script extensions and so on.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Load https://demo.vwzq.net/php/ff_chrome.php, there should be no red.
• List of other uplifts needed: none
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): See above.
• String changes made/needed: none
• Is Android affected?: Unknown

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9274452 [details]
Bug 1757604 - Make content-type on JAR channels behave the same as HTTP channels. r=Gijs

Fun fact, this actually landed on 101 prior to the merge. And I was able to confirm that 101.0b2 shows the expected rendering for the testcase referenced in the approval request.

Comment 23

[Ciprian Georgiu, Desktop QA]

I've reproduced this bug using the STR from comment 21, on an affected build, Firefox 100.0.1 running Win 10 x64.

The issue is verified as fixed on latest Beta 101.0b8, across platforms: Win 10 x64, macOS 11 and Ubuntu 18.04 x64.

Comment 24

[:Gijs (he/him)]

(In reply to :Gijs (he/him) from comment #1)

As noted in my last comments in bug 1460538, the change in bug 1488061 then also didn't work because it changes the HTML output but not the indexed stream - to fix things that way we need to change https://searchfox.org/mozilla-central/rev/8f42809e51cb07aa4f5739932a06d14581e9dd4a/netwerk/base/nsDirectoryIndexStream.cpp#100,102-103 to strip query params for file and/or jar and/or chrome (or rather, only include it for... well, for what, exactly?)

I'd like a second opinion on whether it's still useful to try and fix nsDirectoryIndexStream so it doesn't include query/hash info in the echo'd content? I'm thinking yes, as a belt + suspenders kinda thing... Freddy, does that seem reasonable?

Comment 25

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: advisory.txt

Comment 26

[Frederik Braun [:freddy]]

Yeah, I believe we should try and improve upon bug 1488061 and also strip the stuff from the nsDirectoryIndexStream. Should be simple enough and likely worth the effort (before someone finds another kind of injection that isn't script or style :)).

Comment 27

[:Gijs (he/him)]

Filed bug 1771774 for the directory stream improvement.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Is this ready for an ESR91 uplift? It grafts cleanly.

Comment 29

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9274452 [details]
Bug 1757604 - Make content-type on JAR channels behave the same as HTTP channels. r=Gijs

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: simple enough sec-moderate
• User impact if declined: see comment 0
• Fix Landed on Version: 101
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): It behaves the same as HTTP channels, so shouldn't cause much trouble.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9274452 [details]
Bug 1757604 - Make content-type on JAR channels behave the same as HTTP channels. r=Gijs

Approved for 91.11esr.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/dddc25fd9e03

Comment 32

[Ciprian Georgiu, Desktop QA]

This is also verified as fixed on 91.11.0esr across platforms: Win 10 x64, macOS 11 and Ubuntu 22.04 x64.