Last Comment Bug 313236 - Access checks in XBL compilation scope can be circumvented
: Access checks in XBL compilation scope can be circumvented
Status: RESOLVED FIXED
[sg:critical]
: fixed1.7.13, fixed1.8
Product: Core
Classification: Components
Component: Security (show other bugs)
: Trunk
: x86 Windows XP
: P1 normal (vote)
: mozilla1.8rc1
Assigned To: Blake Kaplan (:mrbkap)
:
Mentors:
Depends on: 325479
Blocks: 313366
  Show dependency treegraph
 
Reported: 2005-10-20 23:56 PDT by moz_bug_r_a4
Modified: 2011-08-05 21:29 PDT (History)
8 users (show)
dveditz: blocking1.7.13+
dveditz: blocking‑aviary1.0.8+
dveditz: blocking1.8rc1+
bob: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase - alert(Components.stack) (297 bytes, text/html)
2005-10-20 23:59 PDT, moz_bug_r_a4
no flags Details
Fix. (2.34 KB, patch)
2005-10-21 02:00 PDT, Blake Kaplan (:mrbkap)
brendan: review+
bzbarsky: superreview+
dveditz: approval‑aviary1.0.8+
dveditz: approval1.7.13+
brendan: approval1.8rc1+
Details | Diff | Splinter Review
A belt to go with those braces (8.09 KB, patch)
2005-10-21 15:36 PDT, Blake Kaplan (:mrbkap)
brendan: review+
Details | Diff | Splinter Review
Shinier belt (12.21 KB, patch)
2005-10-21 16:27 PDT, Blake Kaplan (:mrbkap)
brendan: review+
dveditz: approval‑aviary1.0.8+
dveditz: approval1.7.13+
brendan: approval1.8rc1+
Details | Diff | Splinter Review
Fix, backported (5.94 KB, patch)
2006-02-09 14:41 PST, Blake Kaplan (:mrbkap)
no flags Details | Diff | Splinter Review
Belt backported (8.41 KB, patch)
2006-02-09 18:02 PST, Blake Kaplan (:mrbkap)
brendan: review+
Details | Diff | Splinter Review

Description moz_bug_r_a4 2005-10-20 23:56:43 PDT
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 2005-10-20 23:59:17 PDT
Created attachment 200321 [details]
testcase - alert(Components.stack)
Comment 2 Blake Kaplan (:mrbkap) 2005-10-21 01:43:45 PDT
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) 2005-10-21 02:00:14 PDT
Created attachment 200330 [details] [diff] [review]
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] 2005-10-21 08:48:24 PDT
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] 2005-10-21 08:49:07 PDT
code execution exploit, taking the liberty of marking blocking.
Comment 6 Brendan Eich [:brendan] 2005-10-21 10:15:16 PDT
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) 2005-10-21 11:39:48 PDT
I'm pretty sure it was y (class "Object").
Comment 8 Brendan Eich [:brendan] 2005-10-21 12:16:53 PDT
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, jst@mozilla.com) 2005-10-21 15:21:18 PDT
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) 2005-10-21 15:36:08 PDT
Created attachment 200391 [details] [diff] [review]
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] 2005-10-21 15:52:45 PDT
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) 2005-10-21 16:27:40 PDT
Created attachment 200400 [details] [diff] [review]
Shinier belt
Comment 13 Brendan Eich [:brendan] 2005-10-21 16:35:46 PDT
Comment on attachment 200400 [details] [diff] [review]
Shinier belt

Great, thanks!

/be
Comment 14 Blake Kaplan (:mrbkap) 2005-10-21 18:46:16 PDT
Fix checked into trunk.
Comment 15 Asa Dotzler [:asa] 2005-10-22 11:14:47 PDT
are we still interested in the earlier "Fix." patch? If not, can one of you
remove the approval request?
Comment 16 Blake Kaplan (:mrbkap) 2005-10-22 12:25:05 PDT
Asa, we want both fixes here.
Comment 17 Brendan Eich [:brendan] 2005-10-22 13:16:53 PDT
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] 2005-10-23 01:44:30 PDT
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) 2005-10-23 02:36:43 PDT
Checked into MOZILLA_1_8_BRANCH.
Comment 20 Daniel Veditz [:dveditz] 2006-02-06 12:45:20 PST
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) 2006-02-09 14:41:27 PST
Created attachment 211313 [details] [diff] [review]
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) 2006-02-09 14:44:20 PST
Checked into the 1.7 branches. (I still need to check the shinier belt in though).
Comment 23 Blake Kaplan (:mrbkap) 2006-02-09 18:02:03 PST
Created attachment 211336 [details] [diff] [review]
Belt backported 

This backports the belt and braces patch.
Comment 24 Brendan Eich [:brendan] 2006-02-10 17:18:29 PST
Comment on attachment 211336 [details] [diff] [review]
Belt backported 

Looks good to me.

/be
Comment 25 Blake Kaplan (:mrbkap) 2006-02-10 17:25:26 PST
I checked the backported belt into the 1.7 branches.
Comment 26 Jay Patel [:jay] 2006-02-13 11:47:21 PST
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.

Note You need to log in before you can comment on or make changes to this bug.