Bug 1432870 (CVE-2018-5137)

Path traversal on chrome:// URLs

VERIFIED FIXED in Firefox 59

Status

()

defect
VERIFIED FIXED
a year ago
5 months ago

People

(Reporter: masatokinugawa, Assigned: Gijs)

Tracking

({sec-moderate})

60 Branch
mozilla60
Points:
---
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox58 wontfix, firefox59 verified, firefox60 verified)

Details

(Whiteboard: [adv-main59+])

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

a year ago
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]
(Assignee)

Comment 1

a year ago
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. :-(
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Updated

a year ago
Group: firefox-core-security → core-security
Component: Untriaged → Security
Product: Firefox → Core
(Assignee)

Comment 2

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

Comment 4

a year ago
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.
Attachment #8945500 - Flags: feedback?(bobbyholley)
Comment on attachment 8945500 [details] [diff] [review]
Tentative patch

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

I'll defer to Boris on this stuff.
Attachment #8945500 - Flags: feedback?(bobbyholley) → feedback?(bzbarsky)
Calling this sec-moderate for the path traversal. We have another on the CSP strict-dynamic bypass using devtools.
Keywords: sec-moderate
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.
Attachment #8945500 - Flags: feedback?(bzbarsky) → feedback+
(Assignee)

Comment 8

a year ago
(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...
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 9

a year ago
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.
> 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...
Flags: needinfo?(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.
(Assignee)

Comment 12

a year ago
(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)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(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.  ;)
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 14

a year ago
Posted patch , (obsolete) — Splinter Review
(Assignee)

Comment 15

a year ago
(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).
(Assignee)

Comment 16

a year ago
Posted patch Patch v1 (obsolete) — Splinter Review
Attachment #8946599 - Attachment is obsolete: true
Attachment #8946610 - Flags: review?(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?
Attachment #8946610 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 18

a year ago
(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.
(Assignee)

Comment 19

a year ago
Posted file Patch v2
Attachment #8946787 - Flags: review?(bzbarsky)
(Assignee)

Updated

a year ago
Attachment #8946610 - Attachment is obsolete: true
Comment on attachment 8946787 [details]
Patch v2

r=me
Attachment #8946787 - Flags: review?(bzbarsky) → review+
Group: core-security → dom-core-security
(Assignee)

Comment 22

a year ago
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.
Attachment #8946787 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/54491805113a
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
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.
Flags: qe-verify+
Andrei, can your team verify this issue? I think we may go ahead with uplift but I'd like to have the confirmation. Thanks.
Flags: needinfo?(andrei.vaida)
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)
Attachment #8946787 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: needinfo?(andrei.vaida)
(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.
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.
(Assignee)

Comment 30

a year ago
(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?
Flags: needinfo?(oana.botisan)
I will file a different bug for that issue.
Yes, I will mark this bug as verified.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Flags: needinfo?(oana.botisan)
(Assignee)

Comment 32

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

This is bug 1436294. Thank you!
Group: dom-core-security → core-security-release
Whiteboard: [adv-main59+]
Alias: CVE-2018-5137
Flags: sec-bounty?
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.