Closed Bug 1277583 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Security: CAPS, defect)

49 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox46 --- unaffected
firefox47 + fixed
firefox48 --- fixed
firefox49 --- fixed
firefox-esr45 --- unaffected
firefox50 --- fixed

People

(Reporter: jay.gilbert.nhs, Assigned: Gijs)

References

Details

(Keywords: csectype-sop, regression, sec-high)

Attachments

(6 files, 3 obsolete files)

Attached file 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.
Attached file feedProtocol-privilegeEscalation.html (obsolete) —
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
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
Attachment #8759188 - Attachment mime type: text/html → application/rss+xml
Attachment #8759188 - Attachment mime type: application/rss+xml → text/html
(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)
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)
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.
(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)
Attached image 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.
(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.
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
Ever confirmed: true
Clearing the need info from me here.  Wow, I break things good just like always!
Flags: needinfo?(jay.gilbert.nhs)
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.
Group: firefox-core-security → core-security
Component: RSS Discovery and Preview → Security: CAPS
Product: Firefox → Core
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.
Attached patch PatchSplinter Review
Attached patch Tests for later (obsolete) — Splinter Review
I'm not thrilled about how feed: is reachable from everywhere. Can probably fix that orthogonally.
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)
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)
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?
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)
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?
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)
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)
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
Blocks: 1277685
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)
Blocks: 1277697
Blocks: 1277698
See Also: → 1277685
https://hg.mozilla.org/releases/mozilla-aurora/rev/62258e4119aa
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
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+
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.
(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)
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
Flags: needinfo?(jay.gilbert.nhs)
Flags: sec-bounty?
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)
See Also: → 1281787
Attached patch Tests (obsolete) — Splinter Review
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)
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+
(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)
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)
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)
Attached patch TestsSplinter Review
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+
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.