Access checks in XBL compilation scope can be circumvented

RESOLVED FIXED in mozilla1.8rc1

Status

()

Core
Security
P1
normal
RESOLVED FIXED
12 years ago
6 years ago

People

(Reporter: moz_bug_r_a4, Assigned: mrbkap)

Tracking

({fixed1.7.13, fixed1.8})

Trunk
mozilla1.8rc1
x86
Windows XP
fixed1.7.13, fixed1.8
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.7.13 +
blocking-aviary1.0.8 +
blocking1.8rc1 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical])

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

12 years ago
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;
(Reporter)

Comment 1

12 years ago
Created attachment 200321 [details]
testcase - alert(Components.stack)
(Assignee)

Comment 2

12 years ago
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
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

12 years ago
Flags: blocking1.8rc1?
Priority: -- → P1
Target Milestone: --- → mozilla1.8rc1
(Assignee)

Comment 3

12 years ago
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).
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
(Assignee)

Comment 7

12 years ago
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+
(Assignee)

Updated

12 years ago
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...
(Assignee)

Comment 10

12 years ago
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.
(Assignee)

Updated

12 years ago
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+
(Assignee)

Comment 12

12 years ago
Created attachment 200400 [details] [diff] [review]
Shinier belt
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+
(Assignee)

Comment 14

12 years ago
Fix checked into trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Updated

12 years ago
Attachment #200330 - Flags: approval1.8rc1?

Comment 15

12 years ago
are we still interested in the earlier "Fix." patch? If not, can one of you
remove the approval request?
(Assignee)

Comment 16

12 years ago
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+
(Assignee)

Comment 19

12 years ago
Checked into MOZILLA_1_8_BRANCH.
Keywords: fixed1.8
Blocks: 313366

Updated

11 years ago
Flags: testcase+
(Assignee)

Updated

11 years ago
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+
(Assignee)

Comment 21

11 years ago
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.
(Assignee)

Comment 22

11 years ago
Checked into the 1.7 branches. (I still need to check the shinier belt in though).
Keywords: fixed-aviary1.0.8, fixed1.7.13
(Assignee)

Comment 23

11 years ago
Created attachment 211336 [details] [diff] [review]
Belt backported 

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+
(Assignee)

Comment 25

11 years ago
I checked the backported belt into the 1.7 branches.

Comment 26

11 years ago
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.
Keywords: fixed-aviary1.0.8 → verified-aviary1.0.8
Group: security

Updated

10 years ago
Flags: in-testsuite+ → in-testsuite?
You need to log in before you can comment on or make changes to this bug.