A regression has made it possible to perform privilege escalation/local file disclosure in 47+ via feed: URIs

RESOLVED FIXED in Firefox 47

Status

()

Core
Security: CAPS
RESOLVED FIXED
2 years ago
11 months ago

People

(Reporter: Jay Gilbert, Assigned: Gijs)

Tracking

({csectype-sop, regression, sec-high})

49 Branch
mozilla49
csectype-sop, regression, sec-high
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty +
in-testsuite ?

Firefox Tracking Flags

(firefox46 unaffected, firefox47+ fixed, firefox48 fixed, firefox49 fixed, firefox-esr45 unaffected, firefox50 fixed)

Details

Attachments

(6 attachments, 3 obsolete attachments)

(Reporter)

Description

2 years ago
Created attachment 8759188 [details]
index.html

I hope I'm interpreting this right and filing this in the right place.  I believe the inline preview of feeds has made it possible to perform privilege escalation or view local files on the end user's machine.

I'm currently piecing together the testcase and will have it uploaded shortly.  I'll probably have to upload two separate files as the feed: protocol is ignored if it's not in a top level window.

AFAIK this only works in the newer nightly builds, I just tested the current release builds and they are not vulnerable to this.  A build made from the source downloaded on 06/01/16 is vulnerable.

I'll have the two piece testcase ready for bugzilla here very shortly hopefully.

I'm marking this windows 10 only for now because that's all I've verified.
(Reporter)

Comment 1

2 years ago
Created attachment 8759190 [details]
feedProtocol-privilegeEscalation.html
(Reporter)

Comment 2

2 years ago
Created attachment 8759197 [details]
feedProtocol-privilegeEscalation-revision1.html

Hmm, either https protocol or the filename(attachments.cgi) is breaking this.  It needs to keep the feed: protocol as the beginning of it's URI for this to work hence the index.html upload.
Attachment #8759190 - Attachment is obsolete: true
(Reporter)

Comment 3

2 years ago
Ok, I'll need to talk to someone to be able to show off this issue.  With my local webserver and no https I simply made a a folder called feed, and pointed the new opened window to feed:http://127.0.0.1/feed/

When done like this, even though index.html is served up with the text/html mime type it keeps the feed: protocol at the beginning and allows loading even chrome://content/browser/content.js.

Meh
(Reporter)

Updated

2 years ago
Attachment #8759188 - Attachment mime type: text/html → application/rss+xml
(Reporter)

Updated

2 years ago
Attachment #8759188 - Attachment mime type: application/rss+xml → text/html
(Assignee)

Comment 4

2 years ago
(In reply to Jay Gilbert from comment #3)
> chrome://content/browser/content.js.

You can load this file off any webpage. For instance:

http://jsbin.com/korirideqo/edit?html,output

That in and of itself doesn't mean you've achieved a privilege escalation - the source file is shipped with Firefox and not confidential. It's evaluated in an unprivileged page and so it won't be privileged. If you could load files off file:/// URIs that would be a different matter...

if I try to use:

feed:http://localhost/path/to/some/apache/directory/listing/

then I get redirected to just

http://localhost/path/to/some/apache/directory/listing/

is that not what happens when you're doing this?
Flags: needinfo?(jay.gilbert.nhs)
(Reporter)

Comment 5

2 years ago
You can load files, and chrome://browser/content/browser.js is privileged and has the openDialog function available to open other privileged windows.  Maybe you should talk to to Al for a minute and he might have an interesting back story for you about me.  I *do* know what I'm talking about here.
Flags: needinfo?(jay.gilbert.nhs)
(Reporter)

Comment 6

2 years ago
I missed that one part, the redirection never happens.  The beginning remains feed: and this in turn makes the security checks treat what is after it as a nested URI and it simply loads up.  I can take screenshots or even record a video of it working locally.

Trust me this is legit and needs to be handled.  I'm sure with a few more hours I could find a chrome page that parses user input to allow script injection.  It's game over at that point.
(Assignee)

Comment 7

2 years ago
(In reply to Jay Gilbert from comment #6)
> I missed that one part, the redirection never happens.  The beginning
> remains feed: and this in turn makes the security checks treat what is after
> it as a nested URI and it simply loads up.  I can take screenshots or even
> record a video of it working locally.

OK, can you clarify what you're nesting inside "feed" that causes it to remain visible, and what specifically you're arguing is broken?

For context: from attachment 8759197 [details] I assumed you were nesting in an HTTP URI that loaded a chrome JS script (which is possible even without the feed: URI, for chrome: marked contentaccessible, which anything under chrome://browser/content/ is).

I now noticed that the index.html page you've uploaded loads feed:chrome:// URIs directly in an image/iframe. I don't see those being treated any differently from loading the chrome:// URIs directly, on either 47 beta or nightly. (So the image loads, again because of the contentaccessible flag, and the iframe is denied, because it's an iframe - if you load a <script> tag pointing to any chrome:// script file, that'll work.

> Trust me this is legit and needs to be handled.

It's not so much about trust as it is about having something that reproduces an actual problem. Without clarity about the problem, it's hard to fix anything.

Orthogonally, comment #0 says this regressed on Nightly. Can you determine when it regressed using mozregression? That would also help.
Flags: needinfo?(jay.gilbert.nhs)
(Reporter)

Comment 8

2 years ago
Created attachment 8759208 [details]
YouKnowMe.png

What is your nick on IRC?  Note that in this screen shot not only have I kept the feed: at the beginning, I have loaded a fully privileged chrome page, and about:tabcrashed from content which isn't supposed to be possible anymore.  I could probably narrow down the regression range, but I was planning on spending my time finding more very big security holes.
(Assignee)

Comment 9

2 years ago
(In reply to Jay Gilbert from comment #8)
> Created attachment 8759208 [details]
> YouKnowMe.png
> 
> What is your nick on IRC?

Just "Gijs" generally. Happens to be "Gijs_away" right now.
(Assignee)

Comment 10

2 years ago
Almost done with regression-checking this, and 99% sure I broke this (don't know how yet, the regression window doesn't tell me that...) in bug 1172165.

Which means we're about to ship this in 47. This is probably sec-high/sec-crit.
Assignee: nobody → gijskruitbosch+bugs
Status: UNCONFIRMED → ASSIGNED
status-firefox46: --- → unaffected
status-firefox47: --- → affected
status-firefox48: --- → affected
status-firefox49: --- → affected
tracking-firefox47: --- → ?
Ever confirmed: true
(Reporter)

Comment 11

2 years ago
Clearing the need info from me here.  Wow, I break things good just like always!
Flags: needinfo?(jay.gilbert.nhs)
(Assignee)

Comment 12

2 years ago
https://dxr.mozilla.org/mozilla-central/source/caps/nsScriptSecurityManager.cpp#688-722

Gahhh.

static bool
AllSchemesMatch(nsIURI* aURI, nsIURI* aOtherURI)
{
    nsCOMPtr<nsINestedURI> nestedURI = do_QueryInterface(aURI);
    nsCOMPtr<nsINestedURI> nestedOtherURI = do_QueryInterface(aOtherURI);
    auto stringComparator = nsCaseInsensitiveCStringComparator();
    if (!nestedURI && !nestedOtherURI) {
        // Neither of the URIs is nested, compare their schemes directly:
        nsAutoCString scheme, otherScheme;
        aURI->GetScheme(scheme);
        aOtherURI->GetScheme(otherScheme);
        return scheme.Equals(otherScheme, stringComparator);
    }
    while (nestedURI && nestedOtherURI) {
        nsCOMPtr<nsIURI> currentURI = do_QueryInterface(nestedURI);
        nsCOMPtr<nsIURI> currentOtherURI = do_QueryInterface(nestedOtherURI);
        nsAutoCString scheme, otherScheme;
        currentURI->GetScheme(scheme);
        currentOtherURI->GetScheme(otherScheme);
        if (!scheme.Equals(otherScheme, stringComparator)) {
            return false;
        }

        nestedURI->GetInnerURI(getter_AddRefs(currentURI));
        nestedOtherURI->GetInnerURI(getter_AddRefs(currentOtherURI));
        nestedURI = do_QueryInterface(currentURI);
        nestedOtherURI = do_QueryInterface(currentOtherURI);
    }
    if (!!nestedURI != !!nestedOtherURI) {
        // If only one of the scheme chains runs out at one point, clearly the chains
        // aren't of the same length, so we bail:
        return false;
    }
    return true;
}

So... let's say you pass in URL a and URL b. URL a is "feed:http://..." and URL b is "feed:chrome://".

you hit the loop. Both outer URIs are "feed" schemes, so you pass the initial scheme comparison in the loop.

Now both URIs are no longer nested, so we bail out of the loop. And then we return true.

Head-desk-bang. :-(

I have a fix, just want to find our tests for this stuff and murder them, err, I mean, add some there.
(Assignee)

Updated

2 years ago
Group: firefox-core-security → core-security
Component: RSS Discovery and Preview → Security: CAPS
Product: Firefox → Core
(Reporter)

Comment 13

2 years ago
Quick pick up there.  Nice work.  I'll start now and break some more things before release ;-)
Tracked for Fx47. Gijs is working on a fix and my hope is it will be r+d and land in time for us to gtb RC2.
tracking-firefox47: ? → +
(Assignee)

Comment 15

2 years ago
Created attachment 8759276 [details] [diff] [review]
Patch
(Assignee)

Comment 16

2 years ago
Created attachment 8759281 [details] [diff] [review]
Tests for later

I'm not thrilled about how feed: is reachable from everywhere. Can probably fix that orthogonally.
(Assignee)

Comment 17

2 years ago
Comment on attachment 8759276 [details] [diff] [review]
Patch

Dan, are you happy to review this? bz's busy, bholley's out, there's tests in the next patch. Happy to take alternative suggestions.
Attachment #8759276 - Flags: review?(dveditz)
Comment on attachment 8759276 [details] [diff] [review]
Patch

r=me
Attachment #8759276 - Flags: review?(dveditz) → review+
Hi Gijs, since time is of essence here, I'd prefer to uplift this to moz-release without waiting for this to land on Nightly. Please nominate for uplift to m-r (aurora/beta). Thanks!
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 20

2 years ago
Comment on attachment 8759276 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: bug 1172165. :-\
[User impact if declined]: security issue
[Describe test coverage new/current, TreeHerder]: I have a test, will be post-review and post-landing it.
[Risks and why]: low enough, I guess...
[String/UUID change made/needed]: no
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8759276 - Flags: approval-mozilla-release?
Attachment #8759276 - Flags: approval-mozilla-beta?
Attachment #8759276 - Flags: approval-mozilla-aurora?
Hi Dan, Al, can we get a sec-approval on the patch that was r+d by bz? Do you have any concerns in uplifting this promptly to moz-release? I am hoping to get this in RC2 build which will be kicked off in ~2 hrs or so.
Flags: needinfo?(dveditz)
Flags: needinfo?(abillings)
(Assignee)

Comment 22

2 years ago
Comment on attachment 8759276 [details] [diff] [review]
Patch

(nb: I already landed this, technically... mostly because ritu pinged me about getting this into 47, and earlier things including 46 and 38esr/45esr are not affected)

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
it's not trivial, but possible

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
the general issue, probably yes.

Which older supported branches are affected by this flaw?
47-release/beta/aurora/trunk

If not all supported branches, which bug introduced the flaw?
bug 1172165

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
simple, the code in question hasn't changed

How likely is this patch to cause regressions; how much testing does it need?
I wrote an automated test which is on the bug, which produces reasonable results. I think the risk is reasonably low, but the time pressure means we don't have an awful lot of time to evaluate otherwise. :-\
Attachment #8759276 - Flags: sec-approval?
(Assignee)

Comment 23

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c714d452b06e0cdb916909de3682abea7cc987e5
Comment on attachment 8759276 [details] [diff] [review]
Patch

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

sec-approval+ dveditz
a=dveditz for branch landing (including mozilla-release)
Attachment #8759276 - Flags: approval-mozilla-release?
Attachment #8759276 - Flags: approval-mozilla-release+
Attachment #8759276 - Flags: approval-mozilla-beta?
Attachment #8759276 - Flags: approval-mozilla-beta+
Attachment #8759276 - Flags: approval-mozilla-aurora?
Attachment #8759276 - Flags: approval-mozilla-aurora+
Flags: needinfo?(dveditz)
Attachment #8759276 - Flags: sec-approval? → sec-approval+
Flags: needinfo?(abillings)
(Assignee)

Comment 25

2 years ago
https://hg.mozilla.org/releases/mozilla-release/rev/8815c6471061f5d3f218e53cb74dfba79666edfb

I'll land on aurora when there's more green on my inbound/release pushes and I know I've not completely broken stuff. I'm assuming that if I do that before tomorrow, it'll get merged to beta next week and I don't need to touch beta (as 47 has already moved to mozilla-release). Ritu, can you confirm that's right?
status-firefox47: affected → fixed
Flags: needinfo?(rkothari)
OS: Windows 10 → All
(In reply to :Gijs Kruitbosch (no reviews until June 6) from comment #25)
> https://hg.mozilla.org/releases/mozilla-release/rev/
> 8815c6471061f5d3f218e53cb74dfba79666edfb
> 
> I'll land on aurora when there's more green on my inbound/release pushes and
> I know I've not completely broken stuff. I'm assuming that if I do that
> before tomorrow, it'll get merged to beta next week and I don't need to
> touch beta (as 47 has already moved to mozilla-release). Ritu, can you
> confirm that's right?

Hi Gijs, theoretically that sounds right to me. Though KWierso landed a gfx crash fix (that will also be included in RC2) on both m-r and m-b yesterday. Perhaps it doesn't hurt to land it at both places, just to be safe.
Flags: needinfo?(rkothari)
(Assignee)

Comment 27

2 years ago
Created attachment 8759333 [details]
Test for QA verification

STR:

1. extract to somewhere within your local webserver of choice
2. open index.html
3. click the submit button


AR: depending on the build and/or fairies (I think it might be e10s vs. non-e10s, but I'm not sure), you might see browser UI / menus in the iframe, or the tab spinner becomes permanently activated and you'll see a load of error messages along these lines in the browser console:

TypeError: window.messageManager is undefined printUtils.js:69:5
TypeError: invalid path component ospath_unix.jsm:90:13
NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIAppShellService.hiddenDOMWindow]browser-devedition.js:127
TypeError: window.messageManager is undefined browser-media.js:259:1
ReferenceError: Task is not defined browser-places.js:157:3
TypeError: mm is undefined browser-plugins.js:23:7
ReferenceError: Task is not defined browser-syncui.js:377:3
ReferenceError: Task is not defined browser-trackingprotection.js:199:3

either these error messages or seeing browser UI is BAD.

ER:
the iframe is blank, the tab spinner rests, and the error console displays something along the lines of:

Security Error: Content at feed:http://localhost/~gkruitbosch/testcases/feed-privesc/index2.html may not load or link to feed:chrome://browser/content/browser.xul.

this error message is GOOD. We want a security error.

(clearing ni because ritu answered on IRC)
(Assignee)

Comment 28

2 years ago
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/6e2399cf03e2
(Assignee)

Updated

2 years ago
Summary: A regression has made it possible to perform privilege escalation/local file disclosure in newer nightly builds. → A regression has made it possible to perform privilege escalation/local file disclosure in 47+ via feed: URIs
(Assignee)

Updated

2 years ago
Blocks: 1277685
(Assignee)

Comment 29

2 years ago
Comment on attachment 8759281 [details] [diff] [review]
Tests for later

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

I would have made an xpcshell test but I need the feed component (which is in browser/) to be loaded otherwise the URIs I get for feed: URIs aren't nested and we're not testing what we ship.
Attachment #8759281 - Flags: review?(jonas)
Comment on attachment 8759281 [details] [diff] [review]
Tests for later

My review queue is pretty long right now. Forwarding to Christoph.
Attachment #8759281 - Flags: review?(jonas) → review?(ckerschb)
(Assignee)

Updated

2 years ago
Blocks: 1277697
(Assignee)

Updated

2 years ago
Blocks: 1277698
https://hg.mozilla.org/mozilla-central/rev/c714d452b06e
status-firefox49: affected → fixed
Target Milestone: --- → mozilla49
See Also: → bug 1277685
https://hg.mozilla.org/releases/mozilla-aurora/rev/62258e4119aa
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: affected → fixed
Flags: in-testsuite?
Resolution: --- → FIXED
Group: core-security → core-security-release
Comment on attachment 8759281 [details] [diff] [review]
Tests for later

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

Gijs, since you added a test for data: URLs, would it also make sense to add one for blob:?

::: caps/tests/mochitest/browser_checkloaduri.js
@@ +57,5 @@
> +    threw = true;
> +  }
> +  ok(threw == !canLoadWithoutInherit,
> +     "Should " + (canLoadWithoutInherit ? "not " : "") + "throw an error when loading " +
> +     target + " from " + source.URI.spec + " when inheriting principals is forbidden.");

Nit: The second part of the test is basically duplicate code with the exception of the 'DISALLOW_INHERIT_PRINCIPAL flag, right? You could change the signature to
testURL(source, target, canLoad, baseflags)
and call the function twice in a row from within test() passing different base flags.

If you wanna keep the current setup, that's fine with me too.
Attachment #8759281 - Flags: review?(ckerschb) → review+
(Reporter)

Comment 34

2 years ago
I don't want to sound any alarms but this may not be completely done yet.  Sorry guys I took the weekend to relax.

There's either a completely new issue I've stumbled on, or this definitely still needs some work.  A more definitive way of handling feed: and the legitimate use cases would really help things.

I really wont be able to be sure until all the follow up work cloned off of this bug is handled.
(Assignee)

Comment 35

2 years ago
(In reply to Jay Gilbert from comment #34)
> I don't want to sound any alarms but this may not be completely done yet. 
> Sorry guys I took the weekend to relax.
> 
> There's either a completely new issue I've stumbled on, or this definitely
> still needs some work.  A more definitive way of handling feed: and the
> legitimate use cases would really help things.
> 
> I really wont be able to be sure until all the follow up work cloned off of
> this bug is handled.

I would really like to know what you think is still broken. None of the code fixed in this bug was specific to feed: as such, and so I would be worried about focusing too much on that as a protocol (which is what all the followup bugs do) in terms of fixing whatever new issue you're still seeing. Can you ping me on IRC to discuss what you're looking at? Thanks!
Flags: needinfo?(jay.gilbert.nhs)
(Reporter)

Comment 36

2 years ago
I will as soon as I get somewhere else.  Checkout time where I am now is in 1 hour so I have to put everything on hold for a little while at least.

Leaving the NI flag until I catch you on IRC.  I don't even have the luxury of planning my moves anymore.  Ah the best laid plans lol
(Reporter)

Updated

2 years ago
Flags: needinfo?(jay.gilbert.nhs)
Flags: sec-bounty?
(Assignee)

Comment 37

2 years ago
I need to remember to update the tests and re-request review (feed: is now no longer linkable (bug 1277698) and blob: may or may not be possible, but either way I'll need to poke at it). Ni'ing myself so I don't forget.
Flags: needinfo?(gijskruitbosch+bugs)
status-firefox-esr45: --- → unaffected
Keywords: csectype-sop, regression, sec-high
(Assignee)

Updated

2 years ago
See Also: → bug 1281787
(Assignee)

Comment 38

2 years ago
Created attachment 8764592 [details] [diff] [review]
Tests

Updated to include blob: and some more testing URIs, and to fix review comments.

Dan/Al, am I good to land this or do I need to hold off? (see also bug 1281787. :-\ )
Attachment #8759281 - Attachment is obsolete: true
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(dveditz)
Flags: needinfo?(abillings)
Attachment #8764592 - Flags: sec-approval?
Attachment #8764592 - Flags: review?(ckerschb)
(Reporter)

Comment 39

2 years ago
Can someone cc me on bug 1281787?
Done.
Flags: needinfo?(dveditz)
Comment on attachment 8764592 [details] [diff] [review]
Tests

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

looks great, thanks!

::: caps/tests/mochitest/browser_checkloaduri.js
@@ +90,5 @@
> +      browser,
> +      testURL.toString(),
> +      function* (testURLFn) {
> +        let testURL = eval("(" + testURLFn + ")");
> +        let ssm = Services.scriptSecurityManager;

nit: you already define ssm at the top of the file.

@@ +91,5 @@
> +      testURL.toString(),
> +      function* (testURLFn) {
> +        let testURL = eval("(" + testURLFn + ")");
> +        let ssm = Services.scriptSecurityManager;
> +        let baseFlags = ssm.STANDARD | ssm.DONT_REPORT_ERRORS;

nit: you could also define baseFlags at the top and reuse within add_task().
Attachment #8764592 - Flags: review?(ckerschb) → review+
(Assignee)

Comment 42

2 years ago
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #41)
> Comment on attachment 8764592 [details] [diff] [review]
> Tests
> 
> Review of attachment 8764592 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> looks great, thanks!
> 
> ::: caps/tests/mochitest/browser_checkloaduri.js
> @@ +90,5 @@
> > +      browser,
> > +      testURL.toString(),
> > +      function* (testURLFn) {
> > +        let testURL = eval("(" + testURLFn + ")");
> > +        let ssm = Services.scriptSecurityManager;
> 
> nit: you already define ssm at the top of the file.
> 
> @@ +91,5 @@
> > +      testURL.toString(),
> > +      function* (testURLFn) {
> > +        let testURL = eval("(" + testURLFn + ")");
> > +        let ssm = Services.scriptSecurityManager;
> > +        let baseFlags = ssm.STANDARD | ssm.DONT_REPORT_ERRORS;
> 
> nit: you could also define baseFlags at the top and reuse within add_task().

I can't do these things because the testURL function gets serialized and executed in the content process in e10s, so the toplevel declarations don't exist there. Hence the redeclaration. I'll add comments to this effect.
Flags: needinfo?(abillings)
(Assignee)

Comment 43

2 years ago
I'd still like to know if I'm OK to land the tests and/or need sec-approval for them...
Flags: needinfo?(dveditz)
Can we wait until a fix has landed for bug 1281787? they seem pretty close.
Flags: sec-bounty?
Flags: sec-bounty+
Flags: needinfo?(dveditz)
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 45

a year ago
Sorry Al, what's the needinfo for? I put up a patch in 1281787. The tests can wait, but equally, the tests don't mention blob: and the blob bug got rated sec-low... I guess I'm fine either way, but I should probably poke Kyle about the patch in 1281787 tomorrow.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(abillings)
Ok. I didn't realize you'd put the patch over there. My bad.
Flags: needinfo?(abillings)
(Assignee)

Comment 47

a year ago
Created attachment 8770504 [details] [diff] [review]
Tests

Carrying over r+ for tests. sec-approval? to land after bug 1281787 lands (it's sec-low, and all the other stuff here has been addressed on everything we ship for a while  now).
Attachment #8764592 - Attachment is obsolete: true
Attachment #8764592 - Flags: sec-approval?
Attachment #8770504 - Flags: sec-approval?
Attachment #8770504 - Flags: review+
Comment on attachment 8770504 [details] [diff] [review]
Tests

sec-approval+ then.
Attachment #8770504 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/10ef4e6ed0a3
status-firefox50: --- → fixed

Updated

11 months ago
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.