313236 - Access checks in XBL compilation scope can be circumvented

Status: RESOLVED FIXED

(Core :: Security, defect, P1)

Description

[moz_bug_r_a4]

The fix for Bug 312871 can be circumvented. I've tested on trunk 2005-10-20-07.

Content can access objects in XBL compilation scope via a object that has XBL 
compilation scope object in its proto chain.

<marquee id="m">marquee</marquee>
m = document.getElementById("m");
x = m.init.valueOf.call(null);
y = { __proto__ : x };
y.Function;

Comment 1

[moz_bug_r_a4]

Attachment: testcase - alert(Components.stack)

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

I think I have a handle on this. The basic problem is that getting object
principals walks the _parent_ chain, but looking up properties and such (like
the Function object) walks the _proto_ chain.

We need to make sure to use the right object for the access checks.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix.

I was tempted to turn the if (!obj) clause into an assertion, but settled on an
XXX with an if. Is there an actual case where that can happen? Otherwise, this
patch works (and is correct, I think: I should have remembered from looking at
the object ops for native wrappers).

Comment 4

[Daniel Veditz [:dveditz]]

Comment on attachment 200330 [details] [diff] [review]
Fix.

patch looks good, but how many more unplugged holes do we have in these dikes?
Is there a better, simpler, more reliable way to re-do the whole mechanism?
(not for the 1.8 branch, obviously.)

Comment 5

[Daniel Veditz [:dveditz]]

code execution exploit, taking the liberty of marking blocking.

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 200330 [details] [diff] [review]
Fix.

>+  // Make sure to actually operate on our object, and not some object further
>+  // down on the proto chain.
>+  while (JS_GET_CLASS(cx, obj) != &nsXBLDocGlobalObject::gSharedGlobalClass) {
>+    obj = ::JS_GetPrototype(cx, obj);
>+    if (!obj) {
>+      // XXX Eh?

Since __proto__ is mutable you shouldn't XXX here.  This can happen if someone
can set __proto__ on the object that delegates to the XBL compilation scope to
null.

Question: what object is it, of what class, that delegates to the XBL
compilation scope along __proto__, yet is reached along the __parent__ linked
scope chain?

/be

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

I'm pretty sure it was y (class "Object").

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 200330 [details] [diff] [review]
Fix.

We talked about adding belt and braces to Function, a la eval and Script, in a
separate patch.

/be

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 200330 [details] [diff] [review]
Fix.

+  friend JSBool doCheckAccess(JSContext *cx, JSObject *obj, jsval id,
+			       PRUint32 accessType);

Maybe make doCheckAccess a static member of this class to avoid the friend
declaration here?

sr=jst with that. bz, feel free to double check this if you've got a moment...

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: A belt to go with those braces

I'm worried that this is going to break some native caller somewhere who is
silly enough to not provide a scripted caller but still wants Function to do
the right thing, but it doesn't look like there are any obvious callsites that
do that.

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 200391 [details] [diff] [review]
A belt to go with those braces

> #if JS_HAS_EVAL_THIS_SCOPE
>         /* If obj.eval(str), emulate 'with (obj) eval(str)' in the caller. */
>         if (indirectCall) {
>             callerScopeChain = caller->scopeChain;
>             if (obj != callerScopeChain) {
>-                if (!CheckEvalAccess(cx, obj, caller->script->principals))
>+                if (!js_CheckPrincipalsAccess(cx, obj,
>+                                              caller->script->principals,
>+                                              js_eval_str))
>                     return JS_FALSE;

Brace if any of if or loop condition, then, else or body is multiline, for
home-style cooking.

>+    /*
>+     * Belt-and-braces: Be

lowercase "be"

> absolutely certain that the caller can access the

s/can/should be able to/

or something like that, because if we got here, the caller *can* access the
Function, our belt is off, and our pants are down unless these braces hold. ;-)

>+     * parent of this function to avoid returning a function with stronger
>+     * principals than the caller should be receiving.
>+     */
>+    if (!js_CheckPrincipalsAccess(cx, parent, principals, "Function"))
>+        return JS_FALSE;
>@@ -310,31 +309,22 @@ script_exec(JSContext *cx, JSObject *obj
> 
>     OBJ_TO_INNER_OBJECT(cx, scopeobj);
>     if (!scopeobj)
>         return JS_FALSE;
> 
>     if (!js_CheckScopeChainValidity(cx, scopeobj, js_script_exec))
>         return JS_FALSE;

Hey, perfect time to push OBJ_TO_INNER_OBJECT(cx, scopeobj) down into
js_CheckScopeChainValidity, here and in obj_eval.

I'll look again if you want to put a new patch in, r=me with these fixes right
now.

/be

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Shinier belt

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 200400 [details] [diff] [review]
Shinier belt

Great, thanks!

/be

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 15

[Asa Dotzler [:asa]]

are we still interested in the earlier "Fix." patch? If not, can one of you
remove the approval request?

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Asa, we want both fixes here.

Comment 17

[Brendan Eich [:brendan]]

Right, we want belt, and braces.  We have it for eval and Script, and this pair
of patches gives us the same for Function.

/be

Comment 18

[Brendan Eich [:brendan]]

Comment on attachment 200330 [details] [diff] [review]
Fix.

We want the checked-in version of this from the trunk, on the branch, in a larger roll-up patch, with a generic checkin message.

/be

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

Checked into MOZILLA_1_8_BRANCH.

Comment 20

[Daniel Veditz [:dveditz]]

Comment on attachment 200330 [details] [diff] [review]
Fix.

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)]

Attachment: Fix, backported

I'm about to check this into the 1.7 branches. It includes this fix and the fix for bug 312871.

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

Checked into the 1.7 branches. (I still need to check the shinier belt in though).

Comment 23

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Belt backported

This backports the belt and braces patch.

Comment 24

[Brendan Eich [:brendan]]

Comment on attachment 211336 [details] [diff] [review]
Belt backported 

Looks good to me.

/be

Comment 25

[Blake Kaplan (:mrbkap) (inactive)]

I checked the backported belt into the 1.7 branches.

Comment 26

[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, permission denied in testcase.