Closed Bug 313236 Opened 19 years ago Closed 19 years ago

Access checks in XBL compilation scope can be circumvented

Categories

(Core :: Security, defect, P1)

x86
Windows XP
defect

Tracking

()

RESOLVED FIXED
mozilla1.8rc1

People

(Reporter: moz_bug_r_a4, Assigned: mrbkap)

References

Details

(Keywords: fixed1.7.13, fixed1.8, Whiteboard: [sg:critical])

Attachments

(5 files, 1 obsolete file)

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;
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.
Assignee: dveditz → mrbkap
Status: NEW → ASSIGNED
Flags: blocking1.8rc1?
Priority: -- → P1
Target Milestone: --- → mozilla1.8rc1
Attached patch Fix.Splinter Review
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).
Attachment #200330 - Flags: review?(brendan)
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.)
code execution exploit, taking the liberty of marking blocking.
Flags: blocking1.8rc1?
Flags: blocking1.8rc1+
Flags: blocking1.7.13+
Flags: blocking-aviary1.0.8+
Whiteboard: [sg:critical]
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
I'm pretty sure it was y (class "Object").
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
Attachment #200330 - Flags: review?(brendan) → review+
Attachment #200330 - Flags: superreview?(bzbarsky)
Attachment #200330 - Flags: superreview?(bzbarsky) → superreview+
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...
Attached patch A belt to go with those braces (obsolete) — Splinter Review
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.
Attachment #200391 - Flags: review?(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
Attachment #200391 - Flags: review?(brendan) → review+
Attached patch Shinier beltSplinter Review
Attachment #200391 - Attachment is obsolete: true
Attachment #200400 - Flags: review?(brendan)
Comment on attachment 200400 [details] [diff] [review]
Shinier belt

Great, thanks!

/be
Attachment #200400 - Flags: review?(brendan)
Attachment #200400 - Flags: review+
Attachment #200400 - Flags: approval1.8rc1+
Fix checked into trunk.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Attachment #200330 - Flags: approval1.8rc1?
are we still interested in the earlier "Fix." patch? If not, can one of you
remove the approval request?
Asa, we want both fixes here.
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 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
Attachment #200330 - Flags: approval1.8rc1? → approval1.8rc1+
Checked into MOZILLA_1_8_BRANCH.
Keywords: fixed1.8
Blocks: 313366
Flags: testcase+
Depends on: 325479
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.
Attachment #200330 - Flags: approval1.7.13+
Attachment #200330 - Flags: approval-aviary1.0.8+
Attachment #200400 - Flags: approval1.7.13+
Attachment #200400 - Flags: approval-aviary1.0.8+
Attached patch Fix, backportedSplinter Review
I'm about to check this into the 1.7 branches. It includes this fix and the fix for bug 312871.
Checked into the 1.7 branches. (I still need to check the shinier belt in though).
This backports the belt and braces patch.
Attachment #211336 - Flags: review?(brendan)
Comment on attachment 211336 [details] [diff] [review]
Belt backported 

Looks good to me.

/be
Attachment #211336 - Flags: review?(brendan) → review+
I checked the backported belt into the 1.7 branches.
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.
Group: security
Flags: in-testsuite+ → in-testsuite?
You need to log in before you can comment on or make changes to this bug.