567069 - (CVE-2010-1215) Security problem with SJOW and fast native function

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[moz_bug_r_a4]

Since SafeCallGuard sets cx->fp to null, when a fast native function is called
via SJOW, cx->fp is null.  And, when NewObject(cx, clasp, NULL, NULL) is
called, js_GetClassPrototype uses cx->globalObject as scope if cx->fp is null. 
Thus, if a code running on a chrome's context accesses a content object via
SJOW, a content code can get an object from the chrome's scope and run
arbitrary code with chrome privileges by using that object.

Trunk and 1.9.2 are affected.  (On older branches, SJOW creates scripted
callers instead of using SafeCallGuard.)

Firebug and DOM Inspector are exploitable.

Comment 1

[moz_bug_r_a4]

Attachment: testcase 1 - Arbitrary code execution with Firebug

If you don't have Firebug installed, you can run this testcase using Error
Console.

This works on trunk with Firebug 1.6X.0a9.
This works on 1.9.2 with Firebug 1.5.4.

Comment 2

[moz_bug_r_a4]

Attachment: testcase 2 - Arbitrary code execution with DOM Inspector

If you don't have DOM Inspector installed, you can run this testcase using
Error Console.

This works on trunk with DOMI 2.0.5 or 2.0.4.
This works on 1.9.2 with DOMI 2.0.4.

This does not work on 1.9.2 with DOMI 2.0.5 because 1.9.2 does not have
XPCNativeWrapper.unwrap method and thus JS Object viewer does not unwrap
XPCNativeWrapper.  See bug 533599 and bug 533596.

Comment 3

[moz_bug_r_a4]

Bug 558540 is related to this bug.  (Bug 558540 is not yet fixed.  I can
reproduce that bug on fx-3.7a5pre-2010-05-19-04.)

http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/content/aboutSessionRestore.js

Basically, restoreSession() does:

var s = new Components.utils.Sandbox("about:blank");
var sjow = Components.utils.evalInSandbox("({ a : [ {}, {} ] })", s);
sjow.a = sjow.a.filter(function() true);
sjow.toSource();

The return value of sjow.a.filter() is an array that came from chrome, and when
setting sjow.a, that array is wrapped in a COW.  And then, when getting
toSource method from the COW, since cx->fp is null, RewrapForContent uses
cx->globalObject as scope and thus the return value (array_toSource method) is
not wrapped in XPC_COW_FunctionWrapper.  Thus, array_toSource is called on the
COW and throws:
Error: Array.prototype.toSource called on incompatible ChromeObjectWrapper

Note: Since this comment could reveal the security problem, I commented here
instead of commenting in bug 558540.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Blake, can you take a look?

This sounds like it should probably be sg:critical...

Comment 5

[Damon Sicore (:damons)]

Assigning to Blake.  Blocking 1.9.3 final.

Comment 6

[Damon Sicore (:damons)]

Blake, when you get a chance, please add an ETA in the whiteboard.  Thanks!

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

After talking this over with some folks, a fake stack frame is the way to go. I'm hoping that my ETA is overly conservative.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Eww

I'm asking lots of people for review on this; it's pretty ugly, and it stems from a discussion I had with jorendorff that ended with "well, cx->globalObject is causing the pain, so fix the pain there." I'm mostly worried about code that depends the cx private data having some sort of relationship to globalObject. This does fix the bug and seems somewhat "semantically" correct in some ways: namely that we are, in a way, taking control of the context we're running on.

Opinions welcome.

Comment 9

[Brendan Eich [:brendan]]

Fake frame seems better...

/be

Comment 10

[Jason Orendorff [:jorendorff]]

Comment on attachment 448126 [details] [diff] [review]
Eww

The more we talked this over, the worse it sounded. Who knows what it would break.

Long-term, I think bug 568886 is the way to go.

In the meantime, fake a stack frame. The safest thing would be to make it look exactly like a JSNative stack frame. You could even use a special magic JSNative which JS_FrameIterator (and other places) could skip, if you want to make sure behavior doesn't change.

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 448126 [details] [diff] [review]
Eww

What jorendorff said.

/be

Comment 12

[Damon Sicore (:damons)]

Blake, can we get an updated ETA here, please?

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v1

This seems to work.

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 450387 [details] [diff] [review]
patch v1

Luke, mind looking over my fake frame stuff?

Comment 15

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

FWIW, about:sessionrestore now uses JSON.parse instead of evalInSandbox() & toSource() (bug 387859), so this is no longer causing problems there.

Comment 16

[Luke Wagner [:luke]]

Comment on attachment 450387 [details] [diff] [review]
patch v1

From your explanation of the problem to me, this patch makes sense.

Being a security fix, I suppose the goal is to keep the patch as small as possible, so perhaps this comment is more a request for a follow-up bug, but it seems that SetupFakeFrame could be generalized a small amount (to handle fun->isInterpreted() and fun->u.n.extra > 0) and then the fake frame gunk in js_watch_set could be replaced with a call to SetupFakeFrame.

Comment 17

[Jason Orendorff [:jorendorff]]

Comment on attachment 450387 [details] [diff] [review]
patch v1

Nit: Please name those "Safe" functions something that doesn't contain the word "safe". Perhaps js_GetPropertyByIdWithDummyFrame etc.

> // Reserved slot indexes on safe wrappers.
> 
> // Slot for holding on to the principal to use if a principal other
> // than that of the unsafe object is desired for this wrapper
> // (nsIPrincipal, strong reference).
> static const PRUint32 sPrincipalSlot = sNumSlots;
>+static const PRUint32 sScopeFunSlot = sNumSlots + 1;
>+static const PRUint32 sSJOWSlots = sNumSlots + 2;

Nit: Want a blank line here to separate these constants from the commented one? Or else add more info to the comment?

In GetScopeFunction:
>+  if (!JS_SetReservedSlot(cx, outerObj, sScopeFunSlot, OBJECT_TO_JSVAL(funobj))) {

This makes a reference from a wrapper (in the chrome compartment) to the dummy function (in the unsafe compartment). Is there any way to avoid that? I don't see anything obvious. Even with bug 568886, I think we will still want the dummy stack frame, so this has to be solved somehow for compartmentalization.

r=me with the nits picked; if you can't think of a clever solution for the compartment problem offhand, we'll come up with something.

Comment 18

[Jason Orendorff [:jorendorff]]

I suspect we need the dummy stack frame in a lot more places if Proxy is going to be exposed to scripts.

This is observation is analogous to how in bug 571168 it wouldn't be enough just to put JS_CHECK_RECURSION in JSProxy::{get,set,call,construct}, and similarly in bug 563099 the sixth patch it is insufficient to do "AutoCompartment compartment(cx, other);" only in JSCrossCompartmentWrapper::{call,construct}.

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #16)
> Being a security fix, I suppose the goal is to keep the patch as small as
> possible, so perhaps this comment is more a request for a follow-up bug, but it
> seems that SetupFakeFrame could be generalized a small amount (to handle
> fun->isInterpreted() and fun->u.n.extra > 0) and then the fake frame gunk in
> js_watch_set could be replaced with a call to SetupFakeFrame.

I'll file a followup once this patch is checked in.

(In reply to comment #17)
> This makes a reference from a wrapper (in the chrome compartment) to the dummy
> function (in the unsafe compartment). Is there any way to avoid that?

This is a problem for all wrappers/proxies isn't it? We need some way for the wrapper to refer to its wrapped object, so hopefully we can use whatever solution we come up with for that for this, too.

Comment 20

[Jason Orendorff [:jorendorff]]

(In reply to comment #19)
> > This makes a reference from a wrapper (in the chrome compartment) to the dummy
> > function (in the unsafe compartment). Is there any way to avoid that?
> 
> This is a problem for all wrappers/proxies isn't it? We need some way for the
> wrapper to refer to its wrapped object, so hopefully we can use whatever
> solution we come up with for that for this, too.

My hope was to have the only pointers across compartments be pointers from a wrapper object to its wrappee.

(Well, also pointers from other compartments to interned strings in the default compartment. But that's all, really.)

Maybe we can just give each JSCompartment a dummy function.

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/7939c5504933

I like the last idea there, it seems silly to me to have more than one fake function for this purpose.

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: For 1.9.2

Comment 23

[LegNeato]

Thanks for the quick review Luke!

Comment 24

[LegNeato]

Comment on attachment 455040 [details] [diff] [review]
For 1.9.2

a=LegNeato for 1.9.2.7. Please land on mozilla-1.9.2 default.

Thanks for getting to this!

Comment 25

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/366280a7943e