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

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect)

Description

[Jay Gilbert]

Attachment: 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.

Comment 1

[Jay Gilbert]

Attachment: feedProtocol-privilegeEscalation.html

Comment 2

[Jay Gilbert]

Attachment: 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.

Comment 3

[Jay Gilbert]

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

Comment 4

[:Gijs (out for now; he/him)]

(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?

Comment 5

[Jay Gilbert]

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.

Comment 6

[Jay Gilbert]

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.

Comment 7

[:Gijs (out for now; he/him)]

(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.

Comment 8

[Jay Gilbert]

Attachment: 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.

Comment 9

[:Gijs (out for now; he/him)]

(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.

Comment 10

[:Gijs (out for now; he/him)]

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.

Comment 11

[Jay Gilbert]

Clearing the need info from me here.  Wow, I break things good just like always!

Comment 12

[:Gijs (out for now; he/him)]

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.

Comment 13

[Jay Gilbert]

Quick pick up there.  Nice work.  I'll start now and break some more things before release ;-)

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 15

[:Gijs (out for now; he/him)]

Attachment: Patch

Comment 16

[:Gijs (out for now; he/him)]

Attachment: Tests for later

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

Comment 17

[:Gijs (out for now; he/him)]

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.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8759276 [details] [diff] [review]
Patch

r=me

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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!

Comment 20

[:Gijs (out for now; he/him)]

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

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 22

[:Gijs (out for now; he/him)]

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. :-\

Comment 23

[:Gijs (out for now; he/him)]

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

Comment 24

[Daniel Veditz [:dveditz]]

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)

Comment 25

[:Gijs (out for now; he/him)]

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?

Comment 26

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(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.

Comment 27

[:Gijs (out for now; he/him)]

Attachment: 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)

Comment 28

[:Gijs (out for now; he/him)]

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/6e2399cf03e2

Comment 29

[:Gijs (out for now; he/him)]

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.

Comment 30

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

My review queue is pretty long right now. Forwarding to Christoph.

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c714d452b06e

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/62258e4119aa

Comment 33

[Christoph Kerschbaumer [:ckerschb]]

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.

Comment 34

[Jay Gilbert]

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.

Comment 35

[:Gijs (out for now; he/him)]

(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!

Comment 36

[Jay Gilbert]

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

Comment 37

[:Gijs (out for now; he/him)]

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.

Comment 38

[:Gijs (out for now; he/him)]

Attachment: 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. :-\ )

Comment 39

[Jay Gilbert]

Can someone cc me on bug 1281787?

Comment 40

[Daniel Veditz [:dveditz]]

Done.

Comment 41

[Christoph Kerschbaumer [:ckerschb]]

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().

Comment 42

[:Gijs (out for now; he/him)]

(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.

Comment 43

[:Gijs (out for now; he/him)]

I'd still like to know if I'm OK to land the tests and/or need sec-approval for them...

Comment 44

[Daniel Veditz [:dveditz]]

Can we wait until a fix has landed for bug 1281787? they seem pretty close.

Comment 45

[:Gijs (out for now; he/him)]

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.

Comment 46

[Al Billings [:abillings - ex-MoCo]]

Ok. I didn't realize you'd put the patch over there. My bad.

Comment 47

[:Gijs (out for now; he/him)]

Attachment: 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).

Comment 48

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8770504 [details] [diff] [review]
Tests

sec-approval+ then.

Comment 49

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/10ef4e6ed0a3