1178058 - (CVE-2015-4495) It's possible to read local files or perform privilege escalation by using a native setter

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Cody Crews]

Attachment: accessFileWithNativeSetter-testcase-new.html

This one is a really complex issue and it's me so I'm really going to try to condense this down.

https://mxr.mozilla.org/mozilla-central/source/dom/xml/nsXMLPrettyPrinter.cpp#114

You can start reading there to begin to understand this.  Just get a general idea of what is happening as it's not terribly important except for one key detail.  That is the fact that it is from c++ code that the prettyprint bindings for xml data are loaded.  This involves importing two style sheets into the document, both of which don't show up through document.childNodes, but references to them are accessible through document.styleSheets.

These references get the normal native anonymous content protection goodness, but that can be bypassed with access to the XBL execution context.  I'm using pieces of my older work from bug 1045034 for that bypass, but I'm fairly close I believe to having another method of achieving the same thing.

Now to the interesting parts. Once you have access to these references one who looks will notice that the prototypes accessible from the ownerNode of the style sheet are prototypes which provide access to native methods.  Here I'm tracing the __proto__ chain up to the Object prototype and using the native __lookupSetter__ method to bypass restrictions on windows that I shouldn't have access to.

Using this over powered __lookupSetter__ function I then get the location setter of a window that is inaccessible to me otherwise.  With this location setter it's possible to bypass all restriction when setting the location property of any window except those that are chrome privileged.

In the testcase I'm attaching with this I use this location setter on a pdf viewer window to inject javascript into it via a javascript URI.  I then have it load a local file because resource:// pages have that ability and still using the same OP location setter I show javascript execution in that window to prove that in fact the contents of the file loaded are fully accessible from content.

There, and that's as condensed down as possible for an issue this complicated I believe(for me).  I still have to mention that it's also possible to manipulate about:home windows to achieve chrome privilege escalation but without a way to inject script into it, I thought this was more interesting.

I hope I didn't kill you guys with all that, and also let me know if this works in windows ok.  you OSX guys will just have to trust the word of others.

Comment 1

[Boris Zbarsky [:bzbarsky]]

> I then get the location setter of a window that is inaccessible to me otherwise

Can you not just do:

  Object.getOwnPropertyDescriptor(i2.contentWindow, "location").set

to get the same function that you're getting via __lookupSetter__ in the attached testcase?

Comment 2

[Cody Crews]

(In reply to Boris Zbarsky [:bz] from comment #1)
> > I then get the location setter of a window that is inaccessible to me otherwise
> 
> Can you not just do:
> 
>   Object.getOwnPropertyDescriptor(i2.contentWindow, "location").set
> 
> to get the same function that you're getting via __lookupSetter__ in the
> attached testcase?

Then the location setter shouldn't allow javascript URIs.  It would only be the location setter as can be obtained from the XBL context which AFAIK doesn't allow a javascript URI in a cross domain situation(this allows for XSS even).  The prototypes available here from the NAC do not match the prototypes from i.contentWindow or i.contentWindow.wrappedJSObject.

I guess you could try that, but I wouldn't expect it to work.  What made me even think to try this was that the prototype of the ownerNode in this instance doesn't match i.contentWindow.HTMLLinkElement.prototype or the wrappedJSObject equivalent.  The only thing that is true about the ownerNode(from script) is that it is an instanceof i.contentWindow.HTMLLinkElement.

Who knows though maybe I way over thought this one.

Comment 3

[Cody Crews]

Ok, bz is right here.  I'm fairly sure I tried XSS with access to the XBL context in the past with no success, so I'm guessing this is a regression.  I'll dig into it and see what I can come up with on that.  It's still very interesting to me that the prototypes accessible from NAC aren't what they're expected to be.  Can anyone explain that?

Comment 4

[Daniel Veditz [:dveditz]]

Confirming on Win7 (had to reload to get the popup showing my hosts file).

On the error console I get 

08:55:50.695 unsafe CPOW usage1 PdfjsChromeUtils.jsm:344:0
08:55:50.696 unsafe CPOW usage1 tabbrowser.xml:542:0  (2)
08:55:50.696 Error: operation not possible on dead CPOW1 tabbrowser.xml:542:0

Comment 5

[Bobby Holley (:bholley)]

Cody, this is top-notch work - super clever, 3-part exploit (built on an earlier bug that we haven't fixed, though are working hard to get fixed). Al, I recommend a generous bounty on this one. This is exactly the kind of exploit I expect to see at pwn2own 2016 unless we assign someone to fix up our loading permission story.

IIUC, cody starts by using bug 1045034 to get access to both the playPreview scope _and_ its XBL scope running with an nsExpandedPrincipal. Cody, you mentioned above that you might have another way to do that part without using playPreview. Is that the case? if so, please file it as a separate bug and NI me.

Next, he uses the playPreview scope to load a file:// URI in an iframe. Cody, is this also part of bug 1045034, or something different? Boris, any estimation of the compat risk of preventing resource:// URIs from loading file:// ?

Finally, he exploits a bug in DocShell in which, while trying to avoid inheriting an nsEP from the caller [1], we end up inheriting the principal of the docshell's active document for the triggering principal [2]. This means that, when the nsJSThunk runs async, the security check ends up passing [3].

This is a regression from bug 821809, I think, though the previous state (inheriting the nsEP into a DOMWindow) probably wasn't much better.

Boris, what do you think? Should we just mint an nsNullPrincipal at [1] in the nsEP case? it seems like we should basically never be inheriting the principal in the javascript:// uri case, but I'm not sure how to enforce that.
 
[1] https://hg.mozilla.org/mozilla-central/annotate/f5e3bacfb60e/docshell/base/nsDocShell.cpp#l1603
 
[2] https://hg.mozilla.org/mozilla-central/annotate/f5e3bacfb60e/docshell/base/nsDocShell.cpp#l9740
 
[3] https://hg.mozilla.org/mozilla-central/annotate/cad68cc74656/dom/jsurl/nsJSProtocolHandler.cpp#l267

Comment 6

[Bobby Holley (:bholley)]

Attachment: Tests. v1

I've reduced cody's testcase a bit using SpecialPowers to replace the
dependency on bug 1045034. This demonstrates the third part of the exploit,
which is the new part discussed above.

Comment 7

[Cody Crews]

(In reply to Bobby Holley (:bholley) from comment #5)
> Next, he uses the playPreview scope to load a file:// URI in an iframe.
> Cody, is this also part of bug 1045034, or something different?

I have to admit here I kind of fell uphill on this one.  I only thought to try this because as I said the prototypes involved from the prettyprint bindings aren't what they're expected to be.

with styleSheet being a reference to the style sheet here from the XBL scope it should be like this:

styleSheet.ownerNode.__proto__ === i.contentWindow.HTMLLinkElement.prototype

and to access the prototype as the content of i.contentWindow one should use i.contentWindow.wrappedJSObject.HTMLLinkElement.prototype.

I'm sure you can get what I'm saying from that, the only strange part is neither prototype is actually the prototype of the ownerNode.  This led me to have the idea that by using the Object prototype accessible via the __proto__ chain, that it may allow one to bypass security restrictions.

(In reply to Bobby Holley (:bholley) from comment #5)
> Cody, you mentioned above that you might have another way to do that part without using playPreview.
> Is that the case? if so, please file it as a separate bug and NI me.

I've been very close to this by using the marquee bindings.  I've also ran into a most interesting bug that I'll try to get a testcase to show off from script in which by placing certain elements that generate NAC as a child of a marquee you permanently poison a profile.  With this bad profile all the methods/fields that have exposeToUntrustedContent set to true, aren't actually accessible from content anymore.  It throws no errors and doesn't crash anything therefore is extremely hard to debug.

I have concerns the above mentioned bug could possibly work in reverse and have anonymous content influence the way NAC works, so when I get a testcase that reproduces the issue I'll mark it security sensitive.  I'll also continue the work I was doing to use the marquee bindings to compromise the XBL scope and follow up as needed.

Comment 8

[Cody Crews]

Thinking about this a little more I may be able to answer Bobby's questions a little more straight forward.

I already knew that resource:// URIs could load file:// URIs as I also knew that before pwn2own 2015 that resource:// URIs could load chrome:// URIs.  I remember discussing it with Daniel or Al at some point on IRC though they might not remember.  The only magic here is the location setter obtained allows javascript URI's in windows that it shouldn't.  It also makes me want to meet Mariusz Mlynski even more than after pwn2own 2014 because I swear the guy is living in my head but seems better able to apply the knowledge floating around in there ;)

Jokes aside I absolutely have to set down and have a coffee or something with him because we seem to think scarily close in terms of security research.  If I actually planned on attending pwn2own this year my focus for the work was going to be the fact that resource:// URIs could load chrome:// URIs and whatever I could build on that.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> any estimation of the compat risk of preventing resource:// URIs from loading file://

No, sadly.  Seems pretty risky to me, but we could at least try it on nightly...

> while trying to avoid inheriting an nsEP from the caller [1], we end up inheriting the
> principal of the docshell's active document 

We did that more or less on purpose, right?  But an alternative here would in fact be to not inherit at all in this case.  That would end up not running javascript:, I expect and give data: a nullprincipal when it loads.

Comment 10

[Cody Crews]

Attachment: nsDocShell.cpp.patch

Here's just something to look at along the lines of not inheriting the owner.  I'm reading up on the proper way to do this using mercurial and hg or even MozReview.  This probably needs to be redone anyway because this is wasting a call to IsExpandedPrincipal.  I'm thinking two different conditionals representing each call individually instead of the call to IsSystemOrExpanedPrincipal.  If this is totally way off or if I'm stepping on anyone's toes just ignore it.

Comment 11

[Cody Crews]

Attachment: bug-1178058-part1.patch

Here is a more proper patch generated with hg.  This also doesn't waste a call to IsExpandedPrincipal.  Ignore the parent listed as I used the 39 branch source to test this patch as I noticed that accessing anonymous content from bug 1045034 has been stopped in the newest nightly.  I can easily generate the same patch from the newest nightly if needed.

Which would be a bigger performance hit here, a wasted call to IsExpandedPrincipal, or breaking the whole thing down into two separate conditional statements as I have?

This only takes care of the problem of inheriting the owner in the case of nsExpandedPrincipal which pertains to data, and javascript URIs plus the special use cases of about:blank.  I'll wait to hear from those more familiar with this for the other part needed.

Also bz I hope you don't mind me NI'ing you here.

Comment 12

[Cody Crews]

Attachment: bug-1178058-part1.patch

Fixes a stupid mistake in the comments explaining this change.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Not sure what the needinfo is for, exactly.  Are you looking for feedback on the patch?  Review?  Something else?

Comment 14

[Cody Crews]

I should have directed the question below directly to you I guess.  I was looking more for feedback, maybe a review once I properly generate the patch from the newest nightly.  I thought you were probably the authority on how this should be handled.  I can attempt the second patch to stop resource:// URIs from loading file:// URIs too if need be.  I'm just trying to be helpful.  

(In reply to Cody Crews from comment #11)
> Which would be a bigger performance hit here, a wasted call to
> IsExpandedPrincipal, or breaking the whole thing down into two separate
> conditional statements as I have?

Comment 15

[Boris Zbarsky [:bzbarsky]]

Ah.  Two conditional statements, with "else" so the second one doesn't even run if the first one tests true, is better.

Comment 16

[Cody Crews]

Attachment: bug-1178058-part1.patch

Can I get some feedback on this when you see it bz?  This patch is generated from changes made to the newest nightly, so possibly a review?  I'm going to start looking into the source to see where the resource:// protocol is handled and where changes need to be made to prevent loading file:// URIs.  Unless you guys have a better idea that is.  Just let me know.

Comment 17

[Cody Crews]

Comment on attachment 8631840 [details] [diff] [review]
bug-1178058-part1.patch

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

This patch prevents inheriting the owner in the case of nsExpandedPrincipal in nsDocShell::LoadURI as the first part fix of bug 1178058.

Comment 18

[Cody Crews]

I wasn't sure which of you guys should look this over.  I believe that this part is solid in terms of accomplishing what we want here.  Flagging for review is probably the only productive thing I'll get done today =/

Comment 19

[Cody Crews]

Starting tomorrow I *should* be gone for approximately a week.  I never did get to part two, but part one here is a fairly minimal change.  Just handle things as if I was here anyway(like previously before I was getting more involved).

I'll try to be in touch some during this coming week but don't expect much.  I'm sure you guys have it handled either way.  Thanks in advance for the hard work guys.

Comment 20

[Cody Crews]

I just wanted to clarify while I had a minute today.  I'm currently in the process of moving which is why I am going to have limited access to keep in touch with you guys.  I know I move a lot but in the great state of North Carolina it's almost impossible to find a place that's actually everything it's supposed to be throughout a lease.

I really wanted to get to part two of this as I felt it would be more challenging than part 1 by far.  Should I need info bz to get the review rolling?  I'll just do that anyway as I need to start packing things and such.  I still have the net right now but wont after this move starts, so also if anything on the payout front happens just tell Chris to push it through to the most current account information on file for me.

I can't be sure that I'll even have the technicians out after this move in time to be back but I've prearranged it so that it should happen in the amount of time I've listed.  When this is done I should be as active as I have been and hopefully more so as time builds.

Comment 21

[Cody Crews]

Can I get a review on this bz?  I flagged bholley on this as well as he was a suggested person but I know you're probably the one that really needs to look at it.

If you can after you look it over if nothing is needed(it's a very minimal change) then just make sure Bobby isn't bothered by it too.  Also just commit the changes as yourself if needed.  Just as long as it gets handled.

I'll try to keep an eye on things but I may go ahead and leave ASAP because my new neighbors are driving me crazy ;-)

Comment 22

[Boris Zbarsky [:bzbarsky]]

Cody, no need to needinfo me separately.  Especially over the weekend, when I'm not going to be reviewing anyway.  ;)  I do have a list of review requests waiting for me when I get back on Monday, and it usually takes a bit to get through it all.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8631840 [details] [diff] [review]
bug-1178058-part1.patch

So I thought about this a bit more and I think what we should really do is what Bobby suggested in comment 5: treat the expanded principal case just like we treat the LOAD_FLAGS_DISALLOW_INHERIT_OWNER case.  I'll post a patch to that effect in a moment...

Comment 24

[Boris Zbarsky [:bzbarsky]]

Attachment: Don't convert an expanded principal into inheriting the current principal. Just pretend like the null principal did the load (like LOAD_FLAGS_DISALLOW_INHERIT_OWNER does)

Comment 25

[Cody Crews]

(In reply to Boris Zbarsky [:bz] from comment #22)
> Cody, no need to needinfo me separately.  Especially over the weekend, when
> I'm not going to be reviewing anyway.  ;)  I do have a list of review
> requests waiting for me when I get back on Monday, and it usually takes a
> bit to get through it all.

Yeah sorry about that, I've never had the honor/burden of being flagged for review so I didn't know it would double up your nag emails :P

I'm uploading what should be a viable start to part 2 of the fix for this and I guess I'll just flag Bobby for review, but I would definitely like your feedback too.

Comment 26

[Cody Crews]

Attachment: bug-1178058-part2.patch

This patch just checks the sourceScheme in nsScriptSecurityManager::CheckLoadURIWithPrincipal to see if it matches resource and bails if trying to load a file:// URI.  Once again ignore the parent listed.

Comment 27

[Cody Crews]

Just thought I would say the only time I can get to anything during this week is late, it's currently 11:48 PM EST here and now I must sleep!

The above patch should be a good start, I'm sure we may want to allow for special use cases.  I'm also not sure what "But watch out for the view-source stylesheet?" in the comments means.  Help me anywhere I'm missing anything here guys.  Also if there's activity on this bug and there's no activity from me after 10 PM EST after that then maybe the next day.

Comment 28

[Bobby Holley (:bholley)]

Comment on attachment 8633707 [details] [diff] [review]
Don't convert an expanded principal into inheriting the current principal.  Just pretend like the null principal did the load (like LOAD_FLAGS_DISALLOW_INHERIT_OWNER does)

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

Do you think we could also use a null principal in the chrome case? Or maybe just for javascript: URIs?

r=me either way. Sorry for the lag.

Comment 29

[Bobby Holley (:bholley)]

Comment on attachment 8633878 [details] [diff] [review]
bug-1178058-part2.patch

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

Looks good, but I think we should tackle this in a separate bug. I've filed bug 1184387, can you attach it there, and do a try push?

::: caps/nsScriptSecurityManager.cpp
@@ +815,5 @@
>                                   nsIProtocolHandler::URI_IS_UI_RESOURCE,
>                                   &sourceIsChrome);
>          NS_ENSURE_SUCCESS(rv, rv);
>          if (sourceIsChrome) {
> +            nsresult rv = sourceBaseURI->GetScheme(sourceScheme);

Don't we get the source scheme already above? why do we need to get it again?

@@ +817,5 @@
>          NS_ENSURE_SUCCESS(rv, rv);
>          if (sourceIsChrome) {
> +            nsresult rv = sourceBaseURI->GetScheme(sourceScheme);
> +            if (NS_FAILED(rv)) return rv;
> +            // If the sourceScheme is resource deny load to 

Nit: "resource://", and remove the trailing whitespace.

Comment 30

[Boris Zbarsky [:bzbarsky]]

> Do you think we could also use a null principal in the chrome case?

Wouldn't that break bookmarklets?  Or does the chrome pass in an owner for those?

If it wouldn't break bookmarklets, I'm willing to at least try it.

Comment 31

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #30)
> > Do you think we could also use a null principal in the chrome case?
> 
> Wouldn't that break bookmarklets?  Or does the chrome pass in an owner for
> those?

I'm not sure - Gijs, do you know?

Comment 32

[:Gijs (he/him)]

(In reply to Bobby Holley (:bholley) from comment #31)
> (In reply to Boris Zbarsky [:bz] from comment #30)
> > > Do you think we could also use a null principal in the chrome case?
> > 
> > Wouldn't that break bookmarklets?  Or does the chrome pass in an owner for
> > those?
> 
> I'm not sure - Gijs, do you know?

Not offhand, I can look either late tonight or (my tz) tomorrow morning.

Comment 33

[Boris Zbarsky [:bzbarsky]]

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

Comment 34

[Cody Crews]

I've been looking over writing tests for changes such as this, and I think I have it figured out except one issue here.  The attempt to execute javascript in another window fails silently.  The tests I've looked over involve catching errors to confirm that things are handled properly usually.

Any suggestions?  I'll be digging deeper into making tests until I hear something.

Comment 35

[:Gijs (he/him)]

(In reply to Bobby Holley (:bholley) from comment #31)
> (In reply to Boris Zbarsky [:bz] from comment #30)
> > > Do you think we could also use a null principal in the chrome case?
> > 
> > Wouldn't that break bookmarklets?  Or does the chrome pass in an owner for
> > those?
> 
> I'm not sure - Gijs, do you know?

So... I'm not sure how complete an answer this is. But clicking a bookmarklet link on the bookmarks toolbar just ends up calling browser (like XBL browser) .loadURI with the js URL and flags being 0 (notably, we don't pass LOAD_FLAGS_DISALLOW_INHERIT_OWNER).

I'll go down the rabbithole a bit further and see what I can dig up.

Comment 36

[:Gijs (he/him)]

(In reply to Cody Crews (if gone back by 07/21) from comment #34)
> I've been looking over writing tests for changes such as this, and I think I
> have it figured out except one issue here.  The attempt to execute
> javascript in another window fails silently.  The tests I've looked over
> involve catching errors to confirm that things are handled properly usually.
> 
> Any suggestions?  I'll be digging deeper into making tests until I hear
> something.

It's hard to say much about this without knowing how you're writing a test - mochitest? mochitest-browser? Something else?

It might make sense to attach your WIP.

Comment 37

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch (away Jul 18 - Aug 2) from comment #35)
> (In reply to Bobby Holley (:bholley) from comment #31)
> > (In reply to Boris Zbarsky [:bz] from comment #30)
> > > > Do you think we could also use a null principal in the chrome case?
> > > 
> > > Wouldn't that break bookmarklets?  Or does the chrome pass in an owner for
> > > those?
> > 
> > I'm not sure - Gijs, do you know?
> 
> So... I'm not sure how complete an answer this is. But clicking a
> bookmarklet link on the bookmarks toolbar just ends up calling browser (like
> XBL browser) .loadURI with the js URL and flags being 0 (notably, we don't
> pass LOAD_FLAGS_DISALLOW_INHERIT_OWNER).
> 
> I'll go down the rabbithole a bit further and see what I can dig up.

Meh. So we call loadURI, and we pass no owner, so we hit this block:

  if (!owner && !inheritOwner && !ownerIsExplicit) {
    // See if there's system or chrome JS code running
    inheritOwner = nsContentUtils::IsCallerChrome();
  }

and inheritOwner is set to true. I presume this, at least, answers the question? If not, please re-needinfo (but be quick, because I'm leaving tonight and won't be back until August). If you need someone after my 5pm today, maybe ask :ttaubert or someone else on the frontend team.

Comment 38

[Boris Zbarsky [:bzbarsky]]

Yes, that answers the question.  So using nullprincipal in the "is system principal" case with explicitly given owner would actually not break bookmarlets, I guess.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

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

Comment 40

[Bobby Holley (:bholley)]

(In reply to Cody Crews (if gone back by 07/21) from comment #34)
> I've been looking over writing tests for changes such as this, and I think I
> have it figured out except one issue here.  The attempt to execute
> javascript in another window fails silently.  The tests I've looked over
> involve catching errors to confirm that things are handled properly usually.

Hey Cody,

Yeah, this is indeed a problem - that's why I had to use setTimeout in my test (see the one attached to this bug).

Comment 41

[Bobby Holley (:bholley)]

Comment on attachment 8629241 [details] [diff] [review]
Tests. v1

This should get reviewed so that we can check it in later on. Note that I'm building on it in bug 1185015.

Comment 42

[Cody Crews]

(In reply to :Gijs Kruitbosch (away until Aug 3) from comment #36)
> (In reply to Cody Crews (if gone back by 07/21) from comment #34)
> > I've been looking over writing tests for changes such as this, and I think I
> > have it figured out except one issue here.  The attempt to execute
> > javascript in another window fails silently.  The tests I've looked over
> > involve catching errors to confirm that things are handled properly usually.
> > 
> > Any suggestions?  I'll be digging deeper into making tests until I hear
> > something.
> 
> It's hard to say much about this without knowing how you're writing a test -
> mochitest? mochitest-browser? Something else?
> 
> It might make sense to attach your WIP.

Bobby actually answered this in comment 40.  I should have looked over his test more in depth.

Comment 43

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8629241 [details] [diff] [review]
Tests. v1

s|javascript://|javascript:|

r=me

Comment 44

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

Tracked for all the current train releases.

Comment 45

[Bobby Holley (:bholley)]

Comment on attachment 8633707 [details] [diff] [review]
Don't convert an expanded principal into inheriting the current principal.  Just pretend like the null principal did the load (like LOAD_FLAGS_DISALLOW_INHERIT_OWNER does)

Approval Request Comment
[Feature/regressing bug #]: bug 821809, sorta
[User impact if declined]: security bugs
[Describe test coverage new/current, TreeHerder]: Automated test in this bug. Not checked in.
[Risks and why]: Medium-low risk. It could theoretically break addons, but it's very doubtful.
[String/UUID change made/needed]: None

Comment 46

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

Sounds like this may also affect 39. Is that the case? If so do we need a chemspill for 39?

Comment 47

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

Assuming also this may affect esr38.

Comment 48

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

Bobby, in the uplift request risk assessment you mentioned that this could break add-ons. Did you get a chance to test the fix locally with any add-ons installed? This will give me a bit of confidence when approving uplift to Aurora. Thanks!

Comment 49

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

Bobby, can you also request sec-approval? Thanks.

Comment 50

[Bobby Holley (:bholley)]

Comment on attachment 8633707 [details] [diff] [review]
Don't convert an expanded principal into inheriting the current principal.  Just pretend like the null principal did the load (like LOAD_FLAGS_DISALLOW_INHERIT_OWNER does)

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Not easily.

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

Not really.

Which older supported branches are affected by this flaw?

All of them.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

They should be identical.

How likely is this patch to cause regressions; how much testing does it need?

See comment 45.

Comment 51

[Bobby Holley (:bholley)]

(In reply to Ritu Kothari (:ritu) from comment #48)
> Bobby, in the uplift request risk assessment you mentioned that this could
> break add-ons. Did you get a chance to test the fix locally with any add-ons
> installed? This will give me a bit of confidence when approving uplift to
> Aurora. Thanks!

This fix has been in Nightly for almost six weeks now, so presumably we'd have heard about addon bustage from any popular addons.

I'm really not worried about addon breakage here. The addon would have to have been written very recently, and basically be explicitly depending on this security flaw.

Comment 52

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

Comment on attachment 8633707 [details] [diff] [review]
Don't convert an expanded principal into inheriting the current principal.  Just pretend like the null principal did the load (like LOAD_FLAGS_DISALLOW_INHERIT_OWNER does)

Approving for ESR38 and Aurora uplift. This patch has been in Nightly for 3 weeks, seems stable.

Comment 53

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

Bobby, FF40 RC will be built off of mozilla-release branch. Can you please also request uplift to m-r?

Comment 54

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

Comment on attachment 8633707 [details] [diff] [review]
Don't convert an expanded principal into inheriting the current principal.  Just pretend like the null principal did the load (like LOAD_FLAGS_DISALLOW_INHERIT_OWNER does)

Beta+, Release+. This patch has been in Nightly for > two weeks so it seems stable. The fix was also verified which is great: https://bugzilla.mozilla.org/show_bug.cgi?id=1191284#c16

Comment 55

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

I think we shouldn't land this until we know if we are doing a chemspill to 39/ESR branches. I should know soon.

Comment 56

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

esr38 (default): http://hg.mozilla.org/releases/mozilla-esr38/rev/65c071b3116e
esr38 (GECKO3810esr_2015062417_RELBRANCH): http://hg.mozilla.org/releases/mozilla-esr38/rev/4bd38a453c6e
release (default): http://hg.mozilla.org/releases/mozilla-release/rev/f147014ff61a
release (GECKO390_2015063018_RELBRANCH): http://hg.mozilla.org/releases/mozilla-release/rev/3769169d9bf8
beta: http://hg.mozilla.org/releases/mozilla-beta/rev/f147014ff61a
aurora: http://hg.mozilla.org/releases/mozilla-aurora/rev/5c3a30a76af9

Comment 57

[Nick Thomas [:nthomas] (UTC+12)]

We're hitting a compile error on mozilla-release, on the relbranch (39.0 based) but not on default (40.0), so something changed between in a cycle. The error is:

In file included from /builds/slave/rel-m-rel-l64_bld-000000000000/build/obj-firefox/docshell/base/Unified_cpp_docshell_base0.cpp:56:0:
/builds/slave/rel-m-rel-l64_bld-000000000000/build/docshell/base/nsDocShell.cpp: In member function 'virtual nsresult nsDocShell::LoadURI(nsIURI*, nsIDocShellLoadInfo*, uint32_t, bool)':
/builds/slave/rel-m-rel-l64_bld-000000000000/build/docshell/base/nsDocShell.cpp:1610:15: error: 'Create' is not a member of 'nsNullPrincipal'

Full log: http://ftp.mozilla.org/pub/mozilla.org/firefox/candidates/39.0.3-candidates/build1/logs/release-mozilla-release-linux64_build-bm73-build1-build1.txt.gz

Comment 58

[Bobby Holley (:bholley)]

Replace the added line:

owner = nsNullPrincipal::Create();

with

owner = do_CreateInstance("@mozilla.org/nullprincipal;1");
NS_ENSURE_TRUE(owner, NS_ERROR_FAILURE);

Bed time for me.

Comment 59

[Daniel Veditz [:dveditz]]

For what it's worth I cannot reproduce this testcase on ESR-31. In the dev-tools console I get
  Error: stream must have data" pdf.worker.js:215

  assert@resource://pdf.js/build/pdf.worker.js:232:5
  init@resource://pdf.js/build/pdf.worker.js:4615:5
  PDFDocument@resource://pdf.js/build/pdf.worker.js:4606:7
  LocalPdfManager@resource://pdf.js/build/pdf.worker.js:4201:5
  getPdfManager@resource://pdf.js/build/pdf.worker.js:36831:11
  wphSetupDoc@resource://pdf.js/build/pdf.worker.js:36997:7
  messageHandlerComObjOnMessage@resource://pdf.js/build/pdf.worker.js:1115:9

I don't know if the problem can be worked around or if that version of Firefox and/or the PDF reader is missing a crucial feature, but at least the 0-day won't work as written.

Comment 60

[Daniel Veditz [:dveditz]]

r=dveditz on the "patch" in comment 58

Comment 61

[Nick Thomas [:nthomas] (UTC+12)]

Landed the fix as

esr38 (default): http://hg.mozilla.org/releases/mozilla-esr38/rev/010cae6de47b
esr38 (GECKO3810esr_2015062417_RELBRANCH): http://hg.mozilla.org/releases/mozilla-esr38/rev/63989e580394	
release (GECKO390_2015063018_RELBRANCH): http://hg.mozilla.org/releases/mozilla-release/rev/5a41909266e5

Comment 62

[Andrei Vaida [:avaida]]

According to Comment 45 and Comment 50, the fix pushed for this issue is already covered by automated testing. 

Bobby, is manual testing also required for this patch? If that is the case, could you please elaborate a bit on how Manual QA can help out here?

Comment 63

[Andrew McCreight [:mccr8]]

(In reply to Andrei Vaida, QA [:avaida] from comment #62)
> Bobby, is manual testing also required for this patch? If that is the case,
> could you please elaborate a bit on how Manual QA can help out here?

There is a test for this ("Tests. v1"), but it was not landed. So somebody needs to run those tests.

Comment 64

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/3a02cd6e6ef7
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/3a02cd6e6ef7
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/0301111193bb

Comment 65

[Bobby Holley (:bholley)]

Yes, verify the mochitest in this patch, as well as cody's original testcase attached here. Note that the testcase is designed to work on Windows and Linux. To make it work for Mac, you'll need to add an appropriate file path for the testcase to display.

Comment 66

[Landry Breuil (:gaston)]

Dunno if this adds value, but i can confirm i've tested 38.1.0esr and 39.0 on OpenBSD/amd64, they were affected by a slightly adapted version of the testcase in attachment #8626955 [details] reading /etc/passwd, and 38.1.1esr, 39.0.3 & 40.0rc2 were all immune to the bug.

Comment 67

[Bobby Holley (:bholley)]

(In reply to Landry Breuil (:gaston) from comment #66)
> Dunno if this adds value, but i can confirm i've tested 38.1.0esr and 39.0
> on OpenBSD/amd64, they were affected by a slightly adapted version of the
> testcase in attachment #8626955 [details] reading /etc/passwd, and
> 38.1.1esr, 39.0.3 & 40.0rc2 were all immune to the bug.

Thank is very helpful, thank you. :-)

Comment 68

[Frederik Braun [:freddy]]

(In reply to Cody Crews from comment #7)
> … I'll also
> continue the work I was doing to use the marquee bindings to compromise the
> XBL scope and follow up as needed.

Does this mean we could reduce attack surface by re-implementing those HTML elements which are currently implemented using XBL with something possibly safer (e.g. HTML/CSS)? Not an expert here, just wondering :-)

Comment 69

[Bobby Holley (:bholley)]

(In reply to Frederik Braun [:freddyb] (on PTO until Sep 4th) from comment #68)
> (In reply to Cody Crews from comment #7)
> > … I'll also
> > continue the work I was doing to use the marquee bindings to compromise the
> > XBL scope and follow up as needed.
> 
> Does this mean we could reduce attack surface by re-implementing those HTML
> elements which are currently implemented using XBL with something possibly
> safer (e.g. HTML/CSS)? Not an expert here, just wondering :-)

Those technologies aren't really analogous - WebComponents would be the web-y equivalent to XBL, and I'm not sure we've mapped out the steps to ship WebComponent bindings with the browser.

In general, the point of the XBL scope is to hoist in-content XBL code to a place where the content can't see it. In general compromising the XBL scope is not itself a security issue (because it runs with a limited principal), but this bug demonstrates how it can be combined with other exploits.

Comment 70

[Gervase Markham [:gerv]]

Sorry that I don't know the process, but: now we've shipped this, should this bug be opened?

Gerv

Comment 71

[Bobby Holley (:bholley)]

Given the heavy discussion of it, I want to keep this closed until bug 1184387 lands everywhere.