312871 - Content can access XBL compilation scope by using xbl.method.valueOf.call() or xbl.method.valueOf.apply()

Status: VERIFIED FIXED

(Core :: Security, defect, P2)

Description

[moz_bug_r_a4]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.12) Gecko/20050915
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051017 Firefox/1.6a1

Content can access XBL compilation scope by using xbl.method.valueOf.call() or 
xbl.method.valueOf.apply(). And can create a privileged function by using 
Function constructor in the XBL compilation scope.

trunk 20051017 and fx 1.0.7 and suite 1.7.12 are vulnerable.


<marquee id="m">marquee</marquee>
m = document.getElementById("m");
x = m.init.valueOf.call(null); // or .apply(null)
y = m.init.valueOf.call(); // or .apply()

On trunk:
x is: [object nsXBLPrototypeScript compilation scope]
y is: [object nsXBLPrototypeScript compilation scope]

On 1.0.x:
x is: [object nsXBLPrototypeScript compilation scope]
y is: [object Object]

x.Function can be used to create a privileged function, on trunk and 1.0.x.


When m.init.valueOf.call is called, m.init.valueOf's parent chain is used to 
find an object that should be used as |this| object, if |this| parameter is not
passed, or |this| parameter is null or undefined.

m.init.valueOf.__parent__ is:
  [object Object]
m.init.valueOf.__parent__.__parent__ is:
  [object nsXBLPrototypeScript compilation scope]


On trunk and 1.0.x:
m.init.valueOf.call(null) returns m.init.valueOf.__parent__.__parent__

http://lxr.mozilla.org/mozilla/source/js/src/jsinterp.c#461
461 js_ComputeThis(JSContext *cx, JSObject *thisp, JSStackFrame *fp)
  ...
492         if (JSVAL_IS_PRIMITIVE(fp->argv[-2]) ||
493             !(parent = OBJ_GET_PARENT(cx, JSVAL_TO_OBJECT(fp->argv[-2])))) {
494             thisp = cx->globalObject;
495         } else {
496             /* walk up to find the top-level object */
497             thisp = parent;
498             while ((parent = OBJ_GET_PARENT(cx, thisp)) != NULL)
499                 thisp = parent;
500         }


On trunk:
m.init.valueOf.call() returns m.init.valueOf.__parent__.__parent__

http://lxr.mozilla.org/mozilla/source/js/src/jsfun.c#1456
1456 fun_call(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval)
  ...
1477     if (argc == 0) {
1478         /* Call fun with its global object as the 'this' param if no args. */
1479         while ((tmp = OBJ_GET_PARENT(cx, obj)) != NULL)
1480             obj = tmp;
1481     } else {


On 1.0.x:
m.init.valueOf.call() returns m.init.valueOf.__parent__

http://lxr.mozilla.org/aviary101branch/source/js/src/jsfun.c#1437
1437 fun_call(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval)
  ...
1457     if (argc == 0) {
1458         /* Call fun with its parent as the 'this' parameter if no args. */
1459         obj = OBJ_GET_PARENT(cx, obj);
1460     } else {


Reproducible: Always

Steps to Reproduce:

Comment 1

[moz_bug_r_a4]

Attachment: testcase 1 - alert(Components.stack)

Comment 2

[moz_bug_r_a4]

Attachment: testcase 2 - show .call(null), .call(), .apply(null), .apply()

Comment 3

[Brendan Eich [:brendan]]

js_ComputeThis is low-level, it should not do any access checks.  On the other
hand, something has to, and fun_call and fun_apply, at least, need to.

The capability model in Gecko does not support mixing JS objects from different
trust domains, so we need to check for other paths up the parent chain that
might be troublesome.

/be

Comment 4

[Brendan Eich [:brendan]]

(In reply to comment #3)
> The capability model in Gecko does not support mixing JS objects from different
> trust domains

within a single window-like container, I mean.

/be

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

I get to take this one. The plan is to make the XBL compilation scope do
security checks on gets and sets.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> The capability model in Gecko does not support mixing JS objects from different
> trust domains

I'm not sure I follow this statement.  If I do foo.prop = bar where foo and bar
are objects from different trust domains, nothing stops me (assuming I have
enough privileges to access both trust domains).

Comment 7

[Brendan Eich [:brendan]]

(In reply to comment #6)
> > The capability model in Gecko does not support mixing JS objects from different
> > trust domains
> 
> I'm not sure I follow this statement.  If I do foo.prop = bar where foo and bar
> are objects from different trust domains, nothing stops me (assuming I have
> enough privileges to access both trust domains).

My statement was incomplete, sorry.  If you have UniversalBrowserRead or similar
capability, you can have your way.  That's not what concerns us here, or in
other bugs to-do with same-origin or chrome/content violations.

The problem I'm talking about needs to be formally stated, and I'm working on
the language to do that.  The essence is that we check access mainly in
window-like object getProperty and setProperty hooks, and not deeper in the DOM
or JS object trees connected to a given window.

Thus, windows are articulation points in the object graph a given script may
construct, and while evil.org may load in window A, open window B, load
victim.com in it, and try to access B's DOM or JS objects, the access checks at
the window layer stop evil.org.

There are other checks needed besides these articulation point checks, but with
XBL, we were missing these.

I wrote that the existing capability model doesn't cope with mixed trust to mean
that we go up the JS-lexical and DOM scope chains (same linkage for
SpiderMonkey) to find trust labels associated with windows.  We don't label
individual data items.

So we don't check access in the middle of a sub-graph, nor do we have the
discrete labels to do that with disconnected sub-graphs.  These are helpful
optimizations, or were.  We might evolve toward a system that labels everything
(and taints flow of control, etc., to handle timing channels).  But given where
we are, and how XBL works, we need at least this bug's fix, which was suggested
by the above thinking.

Ask me anything!

/be

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: potential patch

I still need to make sure this doesn't break XBL bindings accessing their own
code (especially user XBL bindings), but this does fix the testcase in this
bug.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: fixing some style nits

I'm still not sure this isn't totally breaking the world (marquee still works
though!).

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 200006 [details] [diff] [review]
fixing some style nits

>Index: content/xbl/src/nsXBLDocumentInfo.cpp
>+  nsIScriptSecurityManager *GetSecurityManager()
>+  {

Is there a reason this is not using nsContentUtils::SecurityManager() ?  I bet
it should.

>+doCheckAccess(JSContext *cx, JSObject *obj, jsval id, PRUint32 
>+  if (!ssm) {
>+    NS_WARNING("Unable to get the script security manager");
>+    return JS_TRUE;

I'd prefer JS_FALSE, I think...  ssm might be null during shutdown; will that
break anyone?

>+  nsresult rv = ssm->CheckPropertyAccess(cx, obj, ::JS_GetClass(cx, obj)->name,

Hmm... Will that give the same classname as classinfo does for objects with
classinfo?  If not, should we be passing null instead, perhaps (so that the
security manager looks up the classname in classinfo)?

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #10)
> Is there a reason this is not using nsContentUtils::SecurityManager() ?  I bet
> it should.

I could have sworn I looked for SecurityManager, bug didn't find it. That makes
things simpler.

> I'd prefer JS_FALSE, I think...  ssm might be null during shutdown; will that
> break anyone?

I don't think we should be running any web page (untrusted) content during
shutdown. I think the only XBL we should be running here would be either browser
or extension XBL, but I'll make this change if you feel strongly about it.

> Hmm... Will that give the same classname as classinfo does for objects with
> classinfo?  If not, should we be passing null instead, perhaps (so that the
> security manager looks up the classname in classinfo)?

Except that we can only ever get into this function when we're doing a lookup on
the actual global object (with this particular class), I think. If we're looking
up a property on another object that actually has classinfo, then we'll do the
resolution in XPConnect (maybe through native wrappers).

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: updated to comments

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 200022 [details] [diff] [review]
updated to comments

> JSClass nsXBLDocGlobalObject::gSharedGlobalClass = {
>     "nsXBLPrototypeScript compilation scope",
>     JSCLASS_HAS_PRIVATE | JSCLASS_PRIVATE_IS_NSISUPPORTS,
>-    JS_PropertyStub,  JS_PropertyStub, JS_PropertyStub, JS_PropertyStub,
>-    JS_EnumerateStub, nsXBLDocGlobalObject_resolve,  JS_ConvertStub,
>-    nsXBLDocGlobalObject_finalize
>+    JS_PropertyStub,  JS_PropertyStub, nsXBLDocGlobalObject_getProperty,
>+    nsXBLDocGlobalObject_setProperty, JS_EnumerateStub,
>+    nsXBLDocGlobalObject_resolve,  JS_ConvertStub,
>+    nsXBLDocGlobalObject_finalize, NULL, nsXBLDocGlobalObject_checkAccess

Nit, maybe warning abatement on some compilers: organize the initializers into
two columns, eight rows, nulls for the trailing stuff (that supposedly
eliminates warnings, perhaps only for gcc -- what's the point of C initializers
if you can't leave stuff out and assume 0 static init!	I guess the idea is
that you forgot to include something; bleah).

sr=me, thanks for tackling this.

/be

Comment 14

[Asa Dotzler [:asa]]

can we get this landed and verified on the trunk today so that we can properly
evaluate for the branch?

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 16

[Asa Dotzler [:asa]]

who can verify this on today's trunk build? 

Comment 17

[Brendan Eich [:brendan]]

Asa: you can, with the attached testcase.

/be

Comment 18

[Daniel Veditz [:dveditz]]

Verified on trunk. Note: second testcase still shows nsXBLPrototypeScript
compilation scope, the fix as demonstrated in the first testcase is that we
don't let people use the object.

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into MOZILLA_1_8_BRANCH.

Comment 20

[Daniel Veditz [:dveditz]]

Comment on attachment 200022 [details] [diff] [review]
updated to comments

aviary101/moz17 landing approval: a=dveditz for drivers. Please add the fixed1.7.13 and fixed-aviary1.0.8 keywords when landed.

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

This is now fixed on the 1.7 branches (see my comment in bug 313236).

Comment 22

[Jay Patel [:jay]]

v.fixed on 1.0.1 Aviary branch with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.13) Gecko/20060213 Firefox/1.0.8, saw the same results as dveditz did in comment #18 using the testcases.