853709 - (CVE-2013-1670) Its possible to call a content level constructor as if from a chrome/privileged page

Status: VERIFIED FIXED

(Core :: XPConnect, defect)

Description

[Cody Crews]

Attachment: ccpoc.html

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/20100101 Firefox/19.0
Build ID: 20130307023931

Steps to reproduce:

I used the mozContact constructor to gain access to a chrome level array and then added a content level constructor to it and defined a setter to do the calling for me.


Actual results:

The content level constructor behaves as if called from a privileged page as shown from the error, this works with most content level constructors(that are function objects) but using the File constructor was just the easiest way to show this off.  I had discussed this issue in passing in an email with Bobby Holly but hadn't had an easy way to show it off.


Expected results:

Chrome level Objects exposed to content should be careful about any properties that are 'rw' as defined in __exposedProps__, I think it may be best to avoid it at all if possible.  Arrays from chrome and the normal COWs(this works with them too) are going to have to be rethought.  This happens I believe because Function Objects inherit their principals from the nearest Object in the scope of their execution.

Comment 1

[Bobby Holley (:bholley)]

Testcase works in a Desktop Nightly. Nice job, Cody.

Comment 2

[Bobby Holley (:bholley)]

Looks this there's a couple of things going on here. I'll look at it again when I'm less tired.

Comment 3

[Cody Crews]

Yeah there's definitely a couple of things to this.  Without the ability to define setters/getters on COW's or arrays the Function objects will continue to act as if called from content.  The 'this' keyword always becomes the object of and I believe the beginning of the lexical scope when dealing with setters and getters even when inherited through the prototype chain.  Also obtaining a reference to the created object would pose another problem and yet could probably be done, but at present just with the File constructor and using error messages it would be possible to map a users local Filesystem giving information about dlls and other files which could be used to bypass ASLR etc.  I usually like to fully work out what all can be done with something before filing but that alone seemed serious enough for this bug to be filed ASAP.

Comment 4

[Bobby Holley (:bholley)]

Cody, this is extremely clever.

For those who don't want to pore over the testcase, here's what it does:

* It creates a mozContact, which is implemented as a COW-wrapped chrome object, with an array marked 'rw'.
* It puts the File constructor at index 0 in the array.
* It then defines a setter at index 1 whose setter function is index 0 of the array, i.e. the File constructor.
* It does arr[1] = 'c:\some\file\path\'. This trips the setter, which ends up invoking the File constructor with the associated string as its only argument.

The key point here is that the File constructor never gets any sort of Xray waiver around it, and so the chrome caller invokes it with Xray vision, thus never entering the compartment and never dropping from chrome principal to page principal. So the IsCallerChrome() check in nsDOMMultipartFile::InitFile succeeds. Yikes.

Now, the real question is how to fix this in the general case. Our security model assumes that it's safe to call an Xrayed function because it never enters the content compartment, meaning that there's no opportunity for the target to confuse it. But here we have the content entering the chrome compartment instead, and setting things up so that the chrome code shoots itself in the foot. :-(

There are number of mitigation techniques we can apply here:
* Stop defining (or even supporting?) rw properties on COWs.
* Don't allow accessors to be defined on COWs.

But in general, I think we fundamentally need to move away from letting content muck with privileged objects at all.

Comment 6

[Cody Crews]

This one was one that I was particularly interested in, yet there's still a few more things that I feel could lead to issues, though nothing quite like this.  As to ways to mitigate this I'm sort of at a loss. I mean Bobby Holly has presented the viable options and its just about how much code relies on COW's with 'rw' properties.  I'm pretty tired tonight but tomorrow I'll try to dig through mozilla internal code that those fixes might affect, addons will be a different story though.

Comment 7

[Boris Zbarsky [:bzbarsky]]

So the fundamental issue here is that chrome is ending up with an Xray for a function when it doesn't expect one, right?

Disallowing accessors on COWs seems like a good step.  I doubt anyone setting a property 'w' expects that to mean "content can redefine it as an accessor property".

But past that, I agree; we need to move away from letting content mess directly with privileged objects.  The question is how we get there...

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Boris Zbarsky (:bz) from comment #7)
> Disallowing accessors on COWs seems like a good step.  I doubt anyone
> setting a property 'w' expects that to mean "content can redefine it as an
> accessor property".

For some reason, treating accessor properties specially rubs me the wrong way. I did it for some XPCNativeWrapper holes back in the day, but it never felt right. I guess my question is: what is the fundamental difference between accessor properties and calling functions?

> But past that, I agree; we need to move away from letting content mess
> directly with privileged objects.  The question is how we get there...

What do you mean with "mess with?" As long as we want to implement privileged APIs in chrome JS, we're going to have interaction between content and chrome (at the very least in the form of function parameters).

Can we make functions passed to chrome be non-waiver non-Xray wrappers (so the only Xray functions called by chrome would be functions explicitly gotten off of existing Xray wrapper objects which have guaranteed behavior)?

Comment 9

[Boris Zbarsky [:bzbarsky]]

> what is the fundamental difference between accessor properties and calling functions?

The fact that the latter is syntactically obvious to the chrome code, and thus easier to avoid, imo.

> What do you mean with "mess with?" 

"Put them into a state the chrome code operating on those objects does not expect them to be in".

> As long as we want to implement privileged APIs in chrome JS, we're going to have
> interaction between content and chrome (at the very least in the form of function
> parameters).

To some extent.  For example, in the JS-implemented WebIDL most function parameters are ensured to be sane (in the sense of not having any unexpected behavior) by the binding layer, with the obvious exceptions being explicit "object" and "any" arguments and callbacks.

> Can we make functions passed to chrome be non-waiver non-Xray wrappers 

So what I proposed to Bobby earlier today is that _calling_ a DOM interface object Xray should simply enter the content compartment for the call, just like an Xray for a scripted function would.  These are Xrays because we want to be able to get at their properties, not because we want to call them in the chrome compartment, imo.

Comment 10

[Bobby Holley (:bholley)]

(In reply to Blake Kaplan (:mrbkap) from comment #8)
> Can we make functions passed to chrome be non-waiver non-Xray wrappers (so
> the only Xray functions called by chrome would be functions explicitly
> gotten off of existing Xray wrapper objects which have guaranteed behavior)?

This is effectively a third state for Xrayable objects, which I'm not really wild about implementing. I'd much prefer to just remove the ability for content to inject functions into the chrome scope.

(In reply to Boris Zbarsky (:bz) from comment #9)

> > As long as we want to implement privileged APIs in chrome JS, we're going to have
> > interaction between content and chrome (at the very least in the form of function
> > parameters).
> 
> To some extent.  For example, in the JS-implemented WebIDL most function
> parameters are ensured to be sane (in the sense of not having any unexpected
> behavior) by the binding layer, with the obvious exceptions being explicit
> "object" and "any" arguments and callbacks.

I agree with bz here that it's significantly harder to mount an attack like this with a C++ layer in between. And I think we can get there. The stuff in mozilla-central should just be moved to WebIDL, and the jetpack folks are moving towards an API that does everything via makeObjectPropsNormal-style function wrappers.

> So what I proposed to Bobby earlier today is that _calling_ a DOM interface
> object Xray should simply enter the content compartment for the call

As mentioned, I disagree here. I think it significantly muddles the Xray semantics.

> just like an Xray for a scripted function would. 

There isn't really such a thing. It's true that we currently default to transparent for non-waived non-Xrayable objects, but I consider that to be a legacy bug that I'd like to move away from, if possible. IMO any raw JS objects used by chrome should be wrapped in a waiver.

Comment 11

[Gabor Krizsanits (INACTIVE)]

This is a hard problem and has been causing headaches for a while, importing pure JS objects from chrome to content. While I always thought about providing the least limiting high level API to JS developers is the best, I kind of gave up on it.

Based on the talks I had in the past few weeks with pure JS developers, my impression is that they tend to prefer a limiting low level API a lot more, than dealing with weird, non standard JavaScript wrappers with __exposedProps__ magics, or that twin object version we came up with the last DOM work week. The trick we use in makeObjectPropsNormal seems to be quite safe way for exporting functions (only) to content. And if we only expose that trick to JS developers they seems to be happy to do everything else in JS (for states they can simply use closures, for simple object they can just use structure clone, etc.). This whole approach seems to be very safe (there is only a very thin layer between chrome and content to guard), and very flexible (the high level utility functions for exposing various more complex objects in some way is implemented in standard JS unlike __exposedProps__ or makeObjectPropsNormal), plus it is a lot easier to explain to JS people who are the main users of this API. That's just my two cents.

Comment 12

[Bobby Holley (:bholley)]

Attachment: Deny accessor definitions in SecurityWrapper. v1

Comment 13

[Bobby Holley (:bholley)]

Attachment: Tests. v1

Comment 14

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=e9b901c94ef6

Comment 15

[Bobby Holley (:bholley)]

Comment on attachment 731234 [details] [diff] [review]
Deny accessor definitions in SecurityWrapper. v1

Pre-emptively flagging for sec-approval in case we decide we want to take this before the merge.

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

Somewhat. Cody's exploit uses a number of tricks, and this fixes one of them. In general, defining a getter or setter on a COW wouldn't seem to be a huge problem, because we'd always enter the content compartment. So they'd have to think of the Xray trick here.

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

It's pretty clear that we don't want accessors to be defined on security wrappers. But I managed to make this patch general to all security wrappers and avoid talking about COWs at all.

Which older supported branches are affected by this flaw?

All.

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

This patch should work everywhere we have bug 800915, which should be all supported branches at this point.

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

It should probably be fine. The only regression would be if some COW (maybe in an extension, though it's unlikely) was depending on having content define accessor properties. We now explicitly throw a very descriptive error message in that case (that's the whole point of the patch). That's probably unlikely.

Comment 16

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ea262c8bb9c

Comment 17

[Bobby Holley (:bholley)]

Comment on attachment 731234 [details] [diff] [review]
Deny accessor definitions in SecurityWrapper. v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): longstanding issue, exploitable via the contact API
User impact if declined: security bugs 
Testing completed (on m-c, etc.): just pushed to m-i
Risk to taking this patch (and alternatives if risky): Not very risky. No real alternatives.
String or IDL/UUID changes made by this patch: There's a change to js.msg, but I don't believe that affects l10n (correct me if I'm wrong).

Comment 18

[Bobby Holley (:bholley)]

Comment on attachment 731234 [details] [diff] [review]
Deny accessor definitions in SecurityWrapper. v1

I guess I meant aurora here, which will be beta in a few days. :-)

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8ea262c8bb9c

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

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

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

::: js/xpconnect/tests/unit/test_bug853709.js
@@ +30,5 @@
> +
> +  checkDefineThrows(contentSB, 'chromeObj', 'a', {get: function() { return 2; }});
> +  checkDefineThrows(contentSB, 'chromeObj', 'a', {configurable: true, get: function() { return 2; }});
> +  checkDefineThrows(contentSB, 'chromeObj', 'b', {configurable: true, get: function() { return 2; }, set: function() {}});
> +  checkDefineThrows(contentSB, 'chromeArr', '1', {configurable: true, get: function() { return 2; }});

Also add a test for setter-only properties?

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b4db16c3fc30

This is going to need some love for b2g18/esr17 uplift.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Looks like beta is going to need some love too. Backed out for OSX and Windows build bustage.
https://hg.mozilla.org/releases/mozilla-beta/rev/8ce4ff5714bd

https://tbpl.mozilla.org/php/getParsedLog.php?id=21399661&tree=Mozilla-Beta

In file included from ../../../../js/src/jswrapper.cpp:15:
../../../../js/src/jswrapper.h:190:18: error: 'defineProperty' marked 'override' but does not override any member functions
    virtual bool defineProperty(JSContext *cx, HandleObject wrapper, HandleId id,
                 ^
../../../../js/src/jswrapper.cpp:892:20: note: in instantiation of template class 'js::SecurityWrapper<js::Wrapper>' requested here
template class js::SecurityWrapper<Wrapper>;
                   ^
In file included from ../../../../js/src/jswrapper.cpp:15:
../../../../js/src/jswrapper.h:190:18: warning: 'js::SecurityWrapper<js::Wrapper>::defineProperty' hides overloaded virtual function [-Woverloaded-virtual]
    virtual bool defineProperty(JSContext *cx, HandleObject wrapper, HandleId id,
                 ^
../../../../js/src/jswrapper.h:90:18: note: hidden overloaded virtual function 'js::Wrapper::defineProperty' declared here
    virtual bool defineProperty(JSContext *cx, JSObject *wrapper, jsid id,
                 ^
../../../../js/src/jswrapper.h:190:18: error: 'defineProperty' marked 'override' but does not override any member functions
    virtual bool defineProperty(JSContext *cx, HandleObject wrapper, HandleId id,
                 ^
../../../../js/src/jswrapper.cpp:893:20: note: in instantiation of template class 'js::SecurityWrapper<js::CrossCompartmentWrapper>' requested here
template class js::SecurityWrapper<CrossCompartmentWrapper>;
                   ^
In file included from ../../../../js/src/jswrapper.cpp:15:
../../../../js/src/jswrapper.h:190:18: warning: 'js::SecurityWrapper<js::CrossCompartmentWrapper>::defineProperty' hides overloaded virtual function [-Woverloaded-virtual]
    virtual bool defineProperty(JSContext *cx, HandleObject wrapper, HandleId id,
                 ^
../../../../js/src/jswrapper.h:138:18: note: hidden overloaded virtual function 'js::CrossCompartmentWrapper::defineProperty' declared here
    virtual bool defineProperty(JSContext *cx, JSObject *wrapper, jsid id,
                 ^
2 warnings and 2 errors generated.
make[6]: *** [jswrapper.o] Error 1

Comment 23

[Bobby Holley (:bholley)]

You just need to change the HandleObject to JSObject* and the HandleId to jsid (in both the header and the cpp file).

https://hg.mozilla.org/releases/mozilla-beta/rev/4ad0a5f5f017

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/12c625607d61
https://hg.mozilla.org/releases/mozilla-esr17/rev/b4c7ce9eb24a

Much better, thanks :)

Comment 25

[Matt Wobensmith [:mwobensmith][:matt:]]

Reproduced on FF17.0.5esr
Confirmed fixed on FF17.0.6esr candidate build 1
Confirmed fixed on FF21 candidate build 2
Confirmed fixed on FF22 2013-05-09
Confirmed fixed on FF23 2013-05-09

Comment 27

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9956ea441d91

Comment 28

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

https://hg.mozilla.org/mozilla-central/rev/9956ea441d91