Closed Bug 1460538 (CVE-2018-12398) Opened 3 years ago Closed 2 years ago

Stylesheet's CSP bypass via reflected URL in chrome:// directories

Categories

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

60 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox-esr52 --- wontfix
firefox-esr60 --- wontfix
firefox61 --- wontfix
firefox62 --- wontfix
firefox63 + fixed

People

(Reporter: pvtolkien, Assigned: Gijs)

References

Details

(Keywords: csectype-other, sec-moderate, Whiteboard: [domsecurity-backlog1][post-critsmash-triage][adv-main63+])

Attachments

(1 file, 1 obsolete file)

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 is very creative...

It's expected that chrome: URLs themselves bypass CSP because things like add-on- and browser-provided styling still need to make its way to the page. But we shouldn't be including the query string in these listings.
Group: firefox-core-security → core-security
Component: Untriaged → Networking
Product: Firefox → Core
Group: core-security → network-core-security
Status: UNCONFIRMED → NEW
Component: Networking → DOM: Security
Ever confirmed: true
Is there a way to make CSS parsing more strict, at least special case it for our own bundled chrome: and resource: sheets? Or can we do something to directory listings to make them not parse as CSS (maybe stick an extraneous '/*' at the beginning)?
'
Flags: needinfo?(dbaron)
There are a couple other chrome pages where parameters get injected as text into pages, such as about:neterror. I guess we'd have to look at all such pages and see if CSS could be parsed out of it if we tried to go the route of spoiling individual pages.
Flags: needinfo?(cam)
Priority: -- → P3
Whiteboard: [domsecurity-backlog1]
(In reply to Daniel Veditz [:dveditz] from comment #2)
> Or can we do something to
> directory listings to make them not parse as CSS (maybe stick an extraneous
> '/*' at the beginning)?

Presumably just not echo'ing the query string (or anything after the path, really) into the directory listing result would be sufficient in this particular case. Not sure I'm comfortable with that as a "definitive" fix though...
I guess this doesn't work for <script> too because of the specific plain text format of the directory listing.  I don't know where that gets generated and transformed into the HTML directory listing page.

Can we turn off directory listings from chrome:// URLs altogether?

Can we prevent chrome:// style sheets from loading in content documents (except from XBL bindings)?
Flags: needinfo?(cam)
Are there any other URI schemes that we ignore CSP directives for?
Another option might be to just enforce CSP directives for chrome:// URL loads that come from the content document.
(In reply to Cameron McCormack (:heycam) from comment #5)
> Can we prevent chrome:// style sheets from loading in content documents
> (except from XBL bindings)?

Well, there are also content about: pages that like to load chrome:// style sheets...
(In reply to Cameron McCormack (:heycam) from comment #5)
> I guess this doesn't work for <script> too because of the specific plain
> text format of the directory listing.  I don't know where that gets
> generated and transformed into the HTML directory listing page.

It shouldn't work for chrome per the code at:

https://dxr.mozilla.org/mozilla-central/rev/24bae072acb09114c367e6b9ffde9261b2ad8a58/dom/security/nsCSPService.cpp#44

which does make chrome: subject to CSP - except for image and style loads.

> Can we turn off directory listings from chrome:// URLs altogether?

I think we probably could but I would prefer not to for debugging reasons...

> Can we prevent chrome:// style sheets from loading in content documents
> (except from XBL bindings)?

I don't know how you'd make the exception for XBL bindings, especially now that those are moving elsewhere. Also, I'm pretty sure we do the same directory listing for resource: and so you could just reproduce with those - and we definitely need those in order to include UA stylesheets.


Could we do the equivalent of X-Content-Type-Options: nosniff for all chrome: and resource: URIs?
Flags: needinfo?(cam)
> > Can we turn off directory listings from chrome:// URLs altogether?

> I think we probably could but I would prefer not to for debugging reasons...

My suggestion was to forbid loading directory listings as stylesheets or images. If that's possible, there's no need to completely disable directory listing.
(In reply to :Gijs (he/him) from comment #9)
> I don't know how you'd make the exception for XBL bindings, especially now
> that those are moving elsewhere. Also, I'm pretty sure we do the same
> directory listing for resource: and so you could just reproduce with those -
> and we definitely need those in order to include UA stylesheets.

For UA style sheets, those are already loaded and just added to the document, so we wouldn't go through this same loading path like for author sheets.  I figure there's probably some way to distinguish between sheets loaded via <style> and <link> and <?xmls-stylesheet?> in content documents and those loaded by XBL bindings.  If the result of de-XBL-ification is that the old bindings' sheets become UA sheets, that shouldn't be a problem.

But various in-content browser pages like about:preferences will want to load sheets from chrome: (and resource:) so we would need an additional check on the requesting page's URL scheme, which doesn't feel that great.

> Could we do the equivalent of X-Content-Type-Options: nosniff for all
> chrome: and resource: URIs?

That's an interesting idea.  How does the Content-Type for things served from chrome: and resource: actually get set?  Does it get set via an HTTP header in some way?  If so, we might be able to do X-Content-Type-Options too.

(In reply to cgvwzq from comment #10)
> My suggestion was to forbid loading directory listings as stylesheets or
> images. If that's possible, there's no need to completely disable directory
> listing.

I guess it depends how the directory listing gets served and whether it's possible to tell that it was a directory listing from the style sheet loader.  If the only way to tell is to change its Content-Type somehow, then X-Content-Type-Options sounds like a good solution.
Flags: needinfo?(cam)
Flags: needinfo?(cam)
(In reply to Cameron McCormack (:heycam) from comment #11)
> > Could we do the equivalent of X-Content-Type-Options: nosniff for all
> > chrome: and resource: URIs?
> 
> That's an interesting idea.  How does the Content-Type for things served
> from chrome: and resource: actually get set?  Does it get set via an HTTP
> header in some way?  If so, we might be able to do X-Content-Type-Options
> too.

I think it gets heuristically determined based on file extensions and/or sniffing, with some file extensions hardcoded. There's no concept of headers for chrome/resource/file: loads, AFAIK. But I assume the xcto header stuff is stored in the request/loadinfo somehow, and assuming that's general enough we should be able to set it for non-http channels. I could be wrong though!

> (In reply to cgvwzq from comment #10)
> > My suggestion was to forbid loading directory listings as stylesheets or
> > images. If that's possible, there's no need to completely disable directory
> > listing.
> 
> I guess it depends how the directory listing gets served and whether it's
> possible to tell that it was a directory listing from the style sheet
> loader.  If the only way to tell is to change its Content-Type somehow, then
> X-Content-Type-Options sounds like a good solution.

I think in practice these get created from nsJARInputStream and/or nsDirectoryIndexStream. I'm not seeing anything that suggests the former really exposes something that you could use to tell that it's a directory, but I guess we could change that... the Necko folks probably have a better idea.
Hey :) Any progress here?
Group: network-core-security → dom-core-security
I have a patch for this.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(cam)
To avoid chrome:// and resource:// content coming from jar: URIs being reused
as something else, copy some XCTO logic from nsHttpChannel and enforce it
against jar: channels.
(In reply to :Gijs (he/him) from comment #4)
> (In reply to Daniel Veditz [:dveditz] from comment #2)
> > Or can we do something to
> > directory listings to make them not parse as CSS (maybe stick an extraneous
> > '/*' at the beginning)?
> 
> Presumably just not echo'ing the query string (or anything after the path,
> really) into the directory listing result would be sufficient in this
> particular case. Not sure I'm comfortable with that as a "definitive" fix
> though...

I still think we should probably do this, but then as a hardening feature in a separate bug.
This seems like a reasonable approach, but I agree we should *also* try to fix the pages to avoid echoing content from the query string.
Flags: needinfo?(dbaron)
Comment on attachment 9004630 [details]
Bug 1460538, r?valentin

Valentin Gosu [:valentin] has approved the revision.
Attachment #9004630 - Flags: review+
Valentin, there were 2 failing tests on try that use (one in tests, one in production code) directory indices through fetch(). I added an extra line to allow getting these through fetch() (which shouldn't impact unprivileged code like comment #0 because they're not allowed to access such URLs through fetch() anyway). Can you confirm that works for you? I confirmed locally that this is sufficient to fix both of the failing tests on the trypush, so I think then we're good to land.
Flags: needinfo?(valentin.gosu)
One last issue I mentioned in phabricator. Then I am OK with shipping.
Flags: needinfo?(valentin.gosu)
This landed as https://hg.mozilla.org/integration/mozilla-inbound/rev/ecf1c85b657f0828a6e7541253b8226be2351db2 but got backed out:

Backed out 85b657f for failing xpcshell test_GMPInstallManager.js and browser-chrome's browser_packaged_as_locales.js:

https://hg.mozilla.org/integration/mozilla-inbound/rev/fa3847f7be7cde3353d2b84b98a272f79a59d699

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=ecf1c85b657f0828a6e7541253b8226be2351db2&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable

Failure log xpcshell: https://treeherder.mozilla.org/logviewer.html#?job_id=196599973&repo=mozilla-inbound
09:20:02     INFO -  TEST-PASS | toolkit/modules/tests/xpcshell/test_GMPInstallManager.js | test_checkForAddons_updatesWithAddons - [test_checkForAddons_updatesWithAddons : 406] "1.1" == "1.1"
09:20:02     INFO -  TEST-PASS | toolkit/modules/tests/xpcshell/test_GMPInstallManager.js | test_checkForAddons_updatesWithAddons - [test_checkForAddons_updatesWithAddons : 407] true == true
09:20:02     INFO -  TEST-PASS | toolkit/modules/tests/xpcshell/test_GMPInstallManager.js | test_checkForAddons_updatesWithAddons - [test_checkForAddons_updatesWithAddons : 408] true == true
09:20:02     INFO -  (xpcshell/head.js) | test run_next_test 16 pending (2)
09:20:02     INFO -  (xpcshell/head.js) | test test_checkForAddons_updatesWithAddons finished (2)
09:20:02     INFO -  toolkit/modules/tests/xpcshell/test_GMPInstallManager.js | Starting bound test_checkForAddons_installAddon
09:20:02     INFO -  (xpcshell/head.js) | test bound test_checkForAddons_installAddon pending (2)
09:20:02     INFO -  "Running installAddon for id: 1, includeSize: true and wantInstallReject: false"
09:20:02     INFO -  "zipURL: http://localhost:65007/test_1_GMP.zip"
09:20:02     INFO -  "zip file created on disk at: c:\\users\\task_1535619325\\appdata\\local\\temp\\xpc-profile-8lmdk8\\test_1_GMP.zip"
09:20:02     INFO -  (xpcshell/head.js) | test run_next_test 16 finished (2)
09:20:02     INFO -  (xpcshell/head.js) | test pending (2)
09:20:02     INFO -  "Notifying load"
09:20:02     INFO -  "Notifying loadend, but there are no listeners"
09:20:02     INFO -  (xpcshell/head.js) | test finished (2)
09:20:02     INFO -  TEST-PASS | toolkit/modules/tests/xpcshell/test_GMPInstallManager.js | bound test_checkForAddons_installAddon - [bound test_checkForAddons_installAddon : 454] 1 == 1
09:20:02     INFO -  TEST-PASS | toolkit/modules/tests/xpcshell/test_GMPInstallManager.js | bound test_checkForAddons_installAddon - [bound test_checkForAddons_installAddon : 456] true == true
09:20:02     INFO -  install update should not reject undefined
09:20:02     INFO -  Z:/task_1535619325/build/tests/xpcshell/tests/toolkit/modules/tests/xpcshell/test_GMPInstallManager.js:test_checkForAddons_installAddon:495
09:20:02     INFO -  exiting test
09:20:02     INFO -  Unexpected exception NS_ERROR_ABORT:
09:20:02     INFO -  _abort_failed_test@Z:\task_1535619325\build\tests\xpcshell\head.js:746:9
09:20:02     INFO -  do_throw@Z:\task_1535619325\build\tests\xpcshell\head.js:739:3
09:20:02     INFO -  test_checkForAddons_installAddon@Z:/task_1535619325/build/tests/xpcshell/tests/toolkit/modules/tests/xpcshell/test_GMPInstallManager.js:495:7
09:20:02     INFO -  Async*run_next_test/_run_next_test/<@Z:\task_1535619325\build\tests\xpcshell\head.js:1441:22
09:20:02     INFO -  async*_run_next_test@Z:\task_1535619325\build\tests\xpcshell\head.js:1441:10
09:20:02     INFO -  run@Z:\task_1535619325\build\tests\xpcshell\head.js:692:9
09:20:02     INFO -  _do_main@Z:\task_1535619325\build\tests\xpcshell\head.js:219:3
09:20:02     INFO -  _execute_test@Z:\task_1535619325\build\tests\xpcshell\head.js:533:5
09:20:02     INFO -  @-e:1:1
09:20:02     INFO -  exiting test

Failure log browser-chrome: https://treeherder.mozilla.org/logviewer.html#?job_id=196603191&repo=mozilla-inbound
09:41:20     INFO - Entering test bound test_all_packaged_locales
09:41:20     INFO - Buffered messages finished
09:41:20     INFO - TEST-UNEXPECTED-FAIL | browser/components/newtab/test/browser/browser_packaged_as_locales.js | Uncaught exception - AbortError: The operation was aborted. 

https://hg.mozilla.org/integration/mozilla-inbound/file/01ee10ce0d71/browser/components/newtab/test/browser/browser_packaged_as_locales.js#l50
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Sebastian Hengst [:aryx] (needinfo on intermittent or backout) from comment #21)
> This landed as
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> ecf1c85b657f0828a6e7541253b8226be2351db2 but got backed out:
> 
> Backed out 85b657f for failing xpcshell test_GMPInstallManager.js and
> browser-chrome's browser_packaged_as_locales.js:

Ugh. I'm really not doing well in this bug - I attached the wrong patch earlier, and now I also landed the wrong patch.

Relanding in a bit with the correct patch, which fixes these 2 tests which I'd already seen on try and fixed...
Flags: needinfo?(gijskruitbosch+bugs)
https://hg.mozilla.org/mozilla-central/rev/64696387b4b0
Group: dom-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
(In reply to :Gijs (he/him) from comment #16)
> (In reply to :Gijs (he/him) from comment #4)
> > (In reply to Daniel Veditz [:dveditz] from comment #2)
> > > Or can we do something to
> > > directory listings to make them not parse as CSS (maybe stick an extraneous
> > > '/*' at the beginning)?
> > 
> > Presumably just not echo'ing the query string (or anything after the path,
> > really) into the directory listing result would be sufficient in this
> > particular case. Not sure I'm comfortable with that as a "definitive" fix
> > though...
> 
> I still think we should probably do this, but then as a hardening feature in
> a separate bug.

Do we have a bug for this?
Flags: needinfo?(gijskruitbosch+bugs)
Depends on: 1488061
Filed bug 1488061, thanks for the reminder.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Valentin Gosu [:valentin] from comment #27)
> Created attachment 9006052 [details]
> Bug 1460538 - Remove Query/Ref from the directory listing URI r=bagder!

Did you mean to set this up for bug 1488061?
Flags: needinfo?(valentin.gosu)
Yes, my bad. I think I copy-pasted the wrong bug number.
Flags: needinfo?(valentin.gosu)
Attachment #9006052 - Attachment is obsolete: true
this change appears to be breaking the decentraleyes web-extension:
https://git.synz.io/Synzvato/decentraleyes/issues/313
Depends on: 1489875
Depends on: 1489882
Clearing esr60.3 flags because we're doing that in bug 1488061 (which addresses the issue with the directory indices echoing random stuff, just in a slightly different way).
Flags: qe-verify-
Whiteboard: [domsecurity-backlog1] → [domsecurity-backlog1][post-critsmash-triage]
Whiteboard: [domsecurity-backlog1][post-critsmash-triage] → [domsecurity-backlog1][post-critsmash-triage][adv-main63+]
Alias: CVE-2018-12398

Hey, sorry for being a bit late... I had forgotten about this one :S Does it qualify for a bounty by any chance?

Flags: needinfo?(abillings)

(In reply to cgvwzq from comment #32)

Hey, sorry for being a bit late... I had forgotten about this one :S Does it
qualify for a bounty by any chance?

I'll mark it here for review.

Note: asking in the bug isn't usually a good way to be seen (if I hadn't been needinfo? on the bug, I'd never have noticed the request). Please see "Claiming a Bug Bounty" on https://www.mozilla.org/en-US/security/client-bug-bounty/ for the process.

Flags: sec-bounty?
Flags: needinfo?(pvtolkien)
Flags: needinfo?(abillings)

(In reply to Al Billings [:abillings] from comment #33)

(In reply to cgvwzq from comment #32)

Hey, sorry for being a bit late... I had forgotten about this one :S Does it
qualify for a bounty by any chance?

I'll mark it here for review.

Note: asking in the bug isn't usually a good way to be seen (if I hadn't been needinfo? on the bug, I'd never have noticed the request). Please see "Claiming a Bug Bounty" on https://www.mozilla.org/en-US/security/client-bug-bounty/ for the process.

Got it! Sorry and thanks :)

Flags: needinfo?(pvtolkien)
Flags: sec-bounty? → sec-bounty+
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.