1432870 - (CVE-2018-5137) Path traversal on chrome:// URLs

Status: VERIFIED FIXED

(Core :: Security, defect)

Description

[Masato Kinugawa]

Attachment: csp_bypass_require_chromeurl.html

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:60.0) Gecko/20100101 Firefox/60.0
Build ID: 20180122220231

Steps to reproduce:

This bug exists in Firefox's legacy extension system. The legacy extension is still used in the browser internally.

Usually, only if the extension's resources are defined on `chrome.manifest` and it has a `contentaccessible=yes` flag, the defined resources can be loaded from web pages via <script> tag or other tags. However, by using crafted path string, even non-contentaccessible resources can be loaded from web pages. Due to this bug, as I described in bug 1432358, it causes an universal bypass of CSP strict-dynamic via chrome:// URL. I attached PoC again.

Steps to Reproduce:

1. See `chrome.manifest` file of browser internal resources from the following URL (in case of  Windows):
jar:file:///C:/Program%20Files/Nightly/browser/omni.ja!/chrome/chrome.manifest


2. Find this line:

content browser browser/content/browser/ contentaccessible=yes

This means that any web pages can load the resources placed on jar:file:///C:/Program%20Files/Nightly/browser/omni.ja!/chrome/browser/content/browser/* via chrome://browser/content/* URLs.

3. The resources placed on jar:file:///C:/Program%20Files/Nightly/browser/omni.ja!/chrome/devtools/* do not have a contentaccessible flag. So, usually it should not be loaded from web pages.

However, using the following crafted URL, it can be loaded from web pages:
(Please take notice to the two slashes of "content//chrome". )

chrome://browser/content//chrome/devtools/modules/devtools/client/jsonview/lib/require.js

It seems that the following path format causes this issue:

chrome:// [ACCESSIBLE_PATH] // [NON_ACCESSIBLE_PATH]

Comment 1

[:Gijs (he/him)]

Basically this means all of omni.ja (and all of any jar file with any contentaccessible content, afaict) is contentaccessible=yes. That's not great. :-(

Comment 2

[:Gijs (he/him)]

This also works with a '%2F' suffix, and also with ' /' (ie whitespace before the extra slash).

Comment 4

[:Gijs (he/him)]

Attachment: Tentative patch

Bobby, is this a crazy idea? It seems to work in my testing. Not sure how terrible this is for performance and/or if we should pursue something else or there's a simpler way to accomplish this.

Comment 5

[Bobby Holley (:bholley)]

Comment on attachment 8945500 [details] [diff] [review]
Tentative patch

Review of attachment 8945500 [details] [diff] [review]:
-----------------------------------------------------------------

I'll defer to Boris on this stuff.

Comment 6

[Daniel Veditz [:dveditz]]

Calling this sec-moderate for the path traversal. We have another on the CSP strict-dynamic bypass using devtools.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8945500 [details] [diff] [review]
Tentative patch

So the fundamental problem is that we're doing this:

  NS_NewURI(aResult, path, nullptr, baseURI)

where "path" is something that starts with '/', right?

I wonder whether we could even have "path" start with something like "file://whatever" and get somewhere that way...  That is, is there an actual guarantee that baseURI and resultingURI have the same scheme here?

I also wonder how ".." bits in the relative path would work.

I guess this approach of checking the full spec or jar path mostly handles all those cases in the situations when the baseURI is jar: or file:, which is good.  But we should probably also fail out for any case in which baseURI and resultingURI have different schemes, even if baseURI's scheme is something other than "jar" or "file".

We should check what happens with ".." in the file:// case.  Does that get canonicalized or preserved?  If the latter, then we can have the substring thing testing true but still be walking all over the disk...

I did check, and nsIURI.resolve would _not_ help us out here.  So this seems like the sanest approach barring a better necko API around this stuff or changes to how we represent these URLs.

Comment 8

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #7)
> Comment on attachment 8945500 [details] [diff] [review]
> Tentative patch
> 
> So the fundamental problem is that we're doing this:
> 
>   NS_NewURI(aResult, path, nullptr, baseURI)
> 
> where "path" is something that starts with '/', right?

Yes.

> I wonder whether we could even have "path" start with something like
> "file://whatever" and get somewhere that way...

I'm not sure I understand.

>  That is, is there an actual
> guarantee that baseURI and resultingURI have the same scheme here?

I don't know of any way to change the scheme of a URI when the path is relative, unless you include a protocol in `path`, which we guard for at the link below.

> I also wonder how ".." bits in the relative path would work.

Not obvious from this diff, but those should be being caught here, right: https://dxr.mozilla.org/mozilla-central/rev/474d58c9137360c0fa1c85cdd11e3313b33b7cad/chrome/nsChromeRegistry.cpp#234-258 ?

I considered adding stuff there, but I decided I didn't want to risk missing some bizarre escape sequence that would still end up with a relative path starting with '/', given that leading whitespace is just getting ignored (and, presumably, ditto for things like leading close-to-0 bytes, given the URL parsing spec...).


Would you prefer if I tried to add stuff to Canonify? Because relative paths must be allowed (ie chrome://browser/content/foo/bar/baz.xul should still work) just outlawing '/' the way we outlaw ':' won't work. I did consider restricting the path component to something like [a-zA-Z0-9#_-] but figured that might still break things. Particularly query strings. And once you get URI escaping for query strings / hashes, restricting the unescaped version to that set is harder, and might break things again. Perhaps I'm too pessimistic about that... Anyway, I'd given up on that avenue, but perhaps that was premature or there was an easy fix I was missing...

Comment 9

[:Gijs (he/him)]

Mostly asking because I'm somewhat nervous about the perf implication of running all these resolve() calls and full string/spec comparisons for any attempt to use chrome URIs. :-(

I'd push to try but this is a sec bug, and anyway I'm not sure how to easily check the perf implications here.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> I'm not sure I understand.

  chrome://browser/content/file:///etc/passwd

or so is what I meant here.  As you note, nsChromeRegistry::Canonify already handles this.

I have to admit that it seems to me like checking for leading whitespace or leading '/' in canonify and bailing out if found should fix this, possibly at lower perf cost.  But as you note with a bit more room for error...

Comment 11

[Boris Zbarsky [:bzbarsky]]

One option is to land the string comparison for now and then at more leisure investigate beefing up Canonify and removing the string compare.

Comment 12

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #11)
> One option is to land the string comparison for now and then at more leisure
> investigate beefing up Canonify and removing the string compare.

Hm. I guess we could just require the first character of the path to be [a-zA-Z0-9]. That'd break this attack, including any whitespace variants, and be a lot cheaper. Unless I'm missing something? :-)

(As a bonus, it doesn't quite directly point at the issue here as much as blocking leading '/' explicitly would)

Comment 13

[Boris Zbarsky [:bzbarsky]]

>I guess we could just require the first character of the path to be [a-zA-Z0-9].

That seems like an _excellent_ mitigation.  In practice, I assume we can require it to be [a-zA-Z], even, no?  Or even [a-z].

And I like the bonus.  ;)

Comment 14

[:Gijs (he/him)]

Attachment: ,

Comment 15

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #13)
> In practice, I assume we can require it to be [a-zA-Z], even, no?  Or even [a-z].

It seems we have non-test files that start with numbers (several dozen in browser/ alone). I don't know if any of them get packaged in the root of any chrome packages but I decided I didn't want to take the risk (and the potential confusion in the future if people tried to use such names).

Comment 16

[:Gijs (he/him)]

Attachment: Patch v1

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8946610 [details] [diff] [review]
Patch v1

I think this would be a lot clearer like so:

 if (!('a' <= *pos && *pos <= 'z') &&
     !('A' <= *pos && *pos <= 'Z') &&
     !('0' <= *pos && *pos <= '9')) {
  return NS_ERROR_DOM_BAD_URI;
 }

or so.  Thoughts?

Comment 18

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #17)
> Comment on attachment 8946610 [details] [diff] [review]
> Patch v1
> 
> I think this would be a lot clearer like so:
> 
>  if (!('a' <= *pos && *pos <= 'z') &&
>      !('A' <= *pos && *pos <= 'Z') &&
>      !('0' <= *pos && *pos <= '9')) {
>   return NS_ERROR_DOM_BAD_URI;
>  }
> 
> or so.  Thoughts?

Oh, of course. That works. I just never think of things like this because my brain is too JS-wired. No char-to-int conversion, you only get "0".charCodeAt(0) and that * 6 is just yucky.

Comment 19

[:Gijs (he/him)]

Attachment: Patch v2

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8946787 [details]
Patch v2

r=me

Comment 21

[:Gijs (he/him)]

sec-moderate so no sec-approval:

https://hg.mozilla.org/integration/mozilla-inbound/rev/54491805113acdce065ed6164b524acc411986ef

Comment 22

[:Gijs (he/him)]

Comment on attachment 8946787 [details]
Patch v2

Approval Request Comment
[Feature/Bug causing the regression]: n/a
[User impact if declined]: security issue

[Is this code covered by automated tests?]: There are existing automated tests. I didn't add any because it's a security fix and that'd reveal what the specific issue is. Also because it's reasonably trivial to test manually. I'll file a follow-up to add automated tests specifically for this issue.

[Has the fix been verified in Nightly?]: not yet, will land on central later today.

[Needs manual test from QE? If yes, steps to reproduce]:  yes:

1. open browser
2. try loading these URLs:

chrome://browser/content//

chrome://browser/content/     /
(lots of spaces)


They should load the default search engine or an error page, as opposed to a file listing (pre-patch)


[List of other uplifts needed for the feature/fix]: none.

[Is the change risky?]: no

[Why is the change risky/not risky?]: simple checks that limit chrome URI syntax to avoid this issue, which is a safe change to make because we no longer have to deal with external add-ons, and because in-product consumers would never use these URIs.

[String changes made/needed]: no.

Comment 23

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

https://hg.mozilla.org/mozilla-central/rev/54491805113a

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Calling ESR52 wontfix since it's rated sec-moderate, but feel free to set the status to affected and nominate it for approval if you feel strongly that it should be considered anyway.

Comment 25

[Liz Henry (:lizzard) (relman/hg->git project)]

Andrei, can your team verify this issue? I think we may go ahead with uplift but I'd like to have the confirmation. Thanks.

Comment 26

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8946787 [details]
Patch v2

Let's uplift this for 59 beta 7, sounds reasonable and doesn't seem like it will break anything (famous last words)

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/43c1a3c2f4c5

Comment 28

[Andrei Vaida [:avaida]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #25)
> Andrei, can your team verify this issue? I think we may go ahead with uplift
> but I'd like to have the confirmation. Thanks.

We are working on this now, Oana will follow up with test results as soon as possible.

Comment 29

[Oana Botisan, Desktop QA]

I managed to reproduce the issue with the steps from comment 22 on Windows 10 x64 using an older version of Nightly (2018-01-24).

I retested everything on latest Nightly 60.0a1 and beta 59.0b7 on Windows 10 x 64, Ubuntu 16.04 x64 and macOS 10.13 and the issue is not reproducing anymore. Every time the default search engine page is loaded.

I did find an issue though. If I try to open the page with the command "Paste & Go", the page never loads and in the Network Monitor there are no request displayed.

Comment 30

[:Gijs (he/him)]

(In reply to Oana Botisan from comment #29)
> I did find an issue though. If I try to open the page with the command
> "Paste & Go", the page never loads and in the Network Monitor there are no
> request displayed.

Please file a separate bug for this that is not security sensitive. Do not link to this bug in any way. As a test-case for that bug, use "chrome://browser/content/../../", which shows the same behaviour even before this patch.

I assume otherwise this bug can be marked verified?

Comment 31

[Oana Botisan, Desktop QA]

I will file a different bug for that issue.
Yes, I will mark this bug as verified.

Comment 32

[:Gijs (he/him)]

(In reply to Oana Botisan from comment #31)
> I will file a different bug for that issue.

This is bug 1436294. Thank you!