296397 - obj.func.__proto__.__parent__ problem in chrome XBL

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[moz_bug_r_a4]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.8) Gecko/20050511 Firefox/1.0.4

This is related to bug 294795.

<marquee id="m">marquee</marquee>
m = document.getElementById("m");

On Firefox 1.0.4 and Mozilla 1.7.8:
  m.init.__proto__.__parent__ is
  [object chrome://xbl-marquee/content/xbl-marquee.xml#marquee].

  m.init.__proto__.__parent__.__parent__ is
  [object nsXBLPrototypeScript compilation scope].

  s = new m.init.__proto__.__parent__.__parent__.Script("Components.stack");
  When chrome calls s.exec(), its code is executed with chrome privilege.

On Firefox trunk:
  m.init.__proto__.__parent__ is |null|.


There are testcases for Firefox 1.0.4 and Mozilla 1.7.8.


Reproducible: Always

Steps to Reproduce:

Comment 1

[moz_bug_r_a4]

Attachment: testcase for Firefox 1.0.4

Comment 2

[moz_bug_r_a4]

Attachment: testcase for Mozilla 1.7.8

Comment 3

[Boris Zbarsky [:bzbarsky]]

See bug 294795 comment 11.   Clearly the answer to my question is "for XBL, yes".

Comment 4

[Brendan Eich [:brendan]]

jst has a patch already in bug 294795 -- please apply that and re-test to
confirm that this is a dup.  Thanks,

/be

*** This bug has been marked as a duplicate of 294795 ***

Comment 5

[Brendan Eich [:brendan]]

Sorry, clearly we need another patch for xbl.  Still, I'd rather track all the
work here in one bug.  If others disagree, please reopen this bug and accept my
apologies for dup'ing too quickly.

/be

Comment 6

[Daniel Veditz [:dveditz]]

Unduping. I personally find it too confusing to have "fixed" bugs taking more
fixes. Separation helps QA as well

Comment 7

[Brendan Eich [:brendan]]

Dan, bug 294795 is not marked FIXED.  Did you mean some other bug?

/be

Comment 8

[Boris Zbarsky [:bzbarsky]]

*** Bug 296489 has been marked as a duplicate of this bug. ***

Comment 9

[Johnny Stenback (:jst)]

Attachment: Make dynamic XBL classes implement the checkAccess() JSClass hook

This blocks *this* testcase, but it still leaves content code with access to
the JSClasses that XBL creates and uses (m.init.__proto__.__parent__). Probably
not a good idea, though I don't know how one would exploit that.

Things are quite different on the trunk with regards to the proto/parent chain
on XBL methods. On the trunk an XBL method's proto looks like the same proto
that you see in branch builds, except that it has no parent. But it's proto
(the proto's proto) is Function.prototype in the XBL compilation scope, so the
same exploitable object is reachable on the trunk too, just through a different
path. To fix that in a similar way to what this patch does we'd need a new
mechanism in the JS engine that would do a security check when proto/parent is
accessed on Function.prototype, or on Function objects in general, rather.

The differences in the proto/parent between trunk and branch builds seems to be
due to the changes made as part of fixing bug 258832.

Comment 10

[Boris Zbarsky [:bzbarsky]]

So would just nulling out the proto like we do for XPConnect not work?

Comment 11

[moz_bug_r_a4]

Attachment: testcase for trunk

This works on:
Firefox/1.0+ (2005-06-03-23)
Firefox/1.0.4
Mozilla/1.7.8

Sorry, since I cannot see Bug 296489, I'm not sure if this testcase is
unnecessary.

Comment 12

[shutdown]

(In reply to comment #11)
> Sorry, since I cannot see Bug 296489, I'm not sure if this testcase is
> unnecessary.

attachment 185242 [details] in bug 296489 uses "constructor" property of
the xbl-marquee's "stop" method to access privileged Function()
constructor.
I've added you to CC list of bug 296489, so you can see it.

Comment 13

[Brendan Eich [:brendan]]

Attachment: possible general fix

This takes advantage of existing catch-all checking done when there isn't a
better way.  Some amount of hackishness to what is used here, but this patch
really does not compound that minor evil, and it may even fix this bug!

/be

Comment 14

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 185627 [details] [diff] [review]
possible general fix

Do we need to do the same for regexps? r=shaver

Comment 15

[Johnny Stenback (:jst)]

Attachment: Additional patch to remove caps assertion from the jsfun.c change

Comment 16

[Johnny Stenback (:jst)]

Comment on attachment 185627 [details] [diff] [review]
possible general fix

This (alone) does fix this bug, nice! But we should take my caps change too to
shut down the assertion...

sr=jst

Comment 17

[Brendan Eich [:brendan]]

Comment on attachment 185701 [details] [diff] [review]
Additional patch to remove caps assertion from the jsfun.c change

shaver sez r=, i'm a'ing too.

/be

Comment 18

[Brendan Eich [:brendan]]

(In reply to comment #14)
> (From update of attachment 185627 [details] [diff] [review] [edit])
> Do we need to do the same for regexps? r=shaver

Yes, good call.  I'll fix that in the same way.

/be

Comment 19

[Brendan Eich [:brendan]]

Attachment: updated patch to handle regexp access checks as well as function

Here's what I will check in right now.

/be

Comment 20

[Brendan Eich [:brendan]]

Attachment: shaver made me share the hook

Comment 21

[Brendan Eich [:brendan]]

I checked in the JS engine changes -- jst's gonna land the assertion removal too.

/be

Comment 22

[Johnny Stenback (:jst)]

Caps change landed on the trunk.

Comment 23

[moz_bug_r_a4]

I've tested in Firefox 2005-06-09-07-trunk. It is still exploitable.

m.init.__proto__.__proto__ causes this error:
  Permission denied to get property Function.__proto__

But, m.init.prototype.__proto__.__parent__ is:
  [object nsXBLPrototypeScript compilation scope]

Also, the testcase in bug 296489 (attachment 185242 [details]) is still alive.


2005-06-07-06-trunk
  m.init.__proto__.__proto__.__parent__ :
    [object nsXBLPrototypeScript compilation scope]

  m.init.prototype.__proto__.__parent__ :
    [object Window]

2005-06-08-07-trunk
  m.init.__proto__.__proto__.__parent__ :
    [object nsXBLPrototypeScript compilation scope]

  m.init.prototype.__proto__.__parent__ :
    [object nsXBLPrototypeScript compilation scope]

2005-06-09-07-trunk
  m.init.__proto__.__proto__.__parent__ :
    Permission denied to get property Function.__proto__

  m.init.prototype.__proto__.__parent__ :
    [object nsXBLPrototypeScript compilation scope]

Comment 24

[moz_bug_r_a4]

Attachment: testcase m.init.prototype.__proto__.__parent__

Comment 25

[Brendan Eich [:brendan]]

Attachment: Share the runtime-wide access check class hook harder

Alternative that doesn't add checkAccess non-stub to Object class is to ape
what the arguments object's class does: name itself "Object", not be
initialized, and thereby share Object.prototype yet have distinct JSClass hooks
from js_ObjectClass (this would have to be done for lazily created
function.prototype objects).  Does not seem worth doing yet, and may be less
safe/paranoid than this patch.
 
/be

Comment 26

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 185837 [details] [diff] [review]
Share the runtime-wide access check class hook harder

r=shaver

Comment 27

[Johnny Stenback (:jst)]

Comment on attachment 185837 [details] [diff] [review]
Share the runtime-wide access check class hook harder

sr=jst for this change, but this does *not* fix all aspects of this bug.

The remaining problem is that we still let content code access f.prototype
where f is a XBL method. The reason being that we end up calling the
checkAccess class hook passing in f as the object (as expected), but we pass in
JSVAL_VOID in *vp. So all our security manager code then does is to check if
the caller can access f, which it can, and since the check succeeds, we leak
f.prototype to the caller who is now free to do whatever it wants to with the
shared prototype (even if its __proto__ and __parent__ are not accessable).

Comment 28

[Brendan Eich [:brendan]]

(In reply to comment #27)
> The remaining problem is that we still let content code access f.prototype
> where f is a XBL method.

But that's ok, right?  The prototype object lazily created by fun_resolve has
the same __parent__ as f does.

> The reason being that we end up calling the
> checkAccess class hook passing in f as the object (as expected), but we pass
> in JSVAL_VOID in *vp.

Which function makes this particular call?

> So all our security manager code then does is to check if
> the caller can access f, which it can, and since the check succeeds, we leak
> f.prototype to the caller who is now free to do whatever it wants to with the
> shared prototype (even if its __proto__ and __parent__ are not accessable).

Again f.prototype for an XBL method's cloned function object should not be
shared -- it should be peculiar to the bound document's window scope, same as f
and f.__proto__, f.__parent__, etc.

/be

Comment 29

[Johnny Stenback (:jst)]

(In reply to comment #28)
> (In reply to comment #27)
> > The remaining problem is that we still let content code access f.prototype
> > where f is a XBL method.
> 
> But that's ok, right?  The prototype object lazily created by fun_resolve has
> the same __parent__ as f does.

Right you are. I got lost in the prototypes here. The problem is that
f.prototype.__proto__ is still accessable, and its parent is the compilation
scope. So f.prototype.__proto__.__parent__ is not reachable, but
f.prototype.__proto__ *is* accessable, and can be changed from any scope.
f.prototype.__proto__ is an Object, and it's reachable because the checkAccess
hook that's called when accessing that __proto__ gets a JSVAL_VOID *vp. Here's
the stack:

nsScriptSecurityManager::CheckObjectAccess(JSContext * cx=0x0304dc70, JSObject *
obj=0x029f70a8, long id=11973972, JSAccessMode mode=JSACC_PROTO, long *
vp=0x0012991c)  Line 456
js_SharedCheckAccess(JSContext * cx=0x0304dc70, JSObject * obj=0x029f70a8, long
id=11973972, JSAccessMode mode=JSACC_PROTO, long * vp=0x0012991c)  Line 3439 + 0x20
js_CheckAccess(JSContext * cx=0x0304dc70, JSObject * obj=0x029f70a8, long
id=11988928, JSAccessMode mode=JSACC_PROTO, long * vp=0x0012991c, unsigned int *
attrsp=0x00128f60)  Line 3424 + 0x4f
obj_getSlot(JSContext * cx=0x0304dc70, JSObject * obj=0x029f70a8, long
id=11988928, long * vp=0x0012991c)  Line 165 + 0x23
js_GetProperty(JSContext * cx=0x0304dc70, JSObject * obj=0x029f70a8, long
id=11988928, long * vp=0x0012991c)  Line 2808 + 0xf8
js_Interpret(JSContext * cx=0x0304dc70, unsigned char * pc=0x03279985, long *
result=0x00129994)  Line 3295 + 0x62e

js_CheckAccess() is the function that ends up setting *vp to JSVAL_VOID before
calling the checkAccess class hook.

Comment 30

[Brendan Eich [:brendan]]

Attachment: revised to handle the problem jst noted in comment 29

Comment 31

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 185907 [details] [diff] [review]
revised to handle the problem jst noted in comment 29

r=shaver

Comment 32

[Brendan Eich [:brendan]]

Attachment: interdiff against last patch to fix bug jst found

This applied on top of the last patch fixes the problem jst noted.  Thanks to
him for testing.  I'm checking in now.

/be

Comment 33

[Johnny Stenback (:jst)]

Comment on attachment 185907 [details] [diff] [review]
revised to handle the problem jst noted in comment 29

- In js_CheckAccess():

     *vp = (SPROP_HAS_VALID_SLOT(sprop, OBJ_SCOPE(pobj)))
	   ? LOCKED_OBJ_GET_SLOT(pobj, sprop->slot)
+	   : (mode == JSACC_PROTO)
+	   ? LOCKED_OBJ_GET_SLOT(pobj, JSSLOT_PROTO)
+	   : (mode == JSACC_PARENT)
+	   ? LOCKED_OBJ_GET_SLOT(pobj, JSSLOT_PARENT)

s/pobj/obj/ in the new code here, as you pointed out when we were testing this.

sr=jst

Comment 34

[Brendan Eich [:brendan]]

Attachment: AVIARY_1_0_1 branch version of patch

Waiting for approval to land.

/be

Comment 35

[Brendan Eich [:brendan]]

Fixed on trunk -- dveditz will approve in good time for the branches, I'm sure.

/be

Comment 36

[moz_bug_r_a4]

Do the fixes for this bug stop the testcase (attachment 185242 [details]) in bug 296489? 
bug 296489 indicates that |m.stop.constructor| refers to the Function 
constructor in the XBL compilation scope, and content window can use it.

Comment 37

[shutdown]

(In reply to comment #36)
> Do the fixes for this bug stop the testcase (attachment 185242 [details] [edit]) in bug 
296489? 
> bug 296489 indicates that |m.stop.constructor| refers to the Function 
> constructor in the XBL compilation scope, and content window can use it.

attachment 185242 [details] still works on:
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b2) Gecko/20050611 Firefox/1.0+

element.method.__proto__
  Error: uncaught exception: Permission denied to get property 
Function.__proto__
element.method.constructor
  function Function() { [native code] }
element.method.constructor("")
  function anonymous() {}
element.method.constructor("C", "return C.classes;")(Components)
  [object nsXPCComponents_Classes]

Comment 38

[Brendan Eich [:brendan]]

Attachment: followup patch to check the proto-delegated 'constructor' access path

This prevents the exploit testcase from bug 296489.  Thanks for catching this
case, moz_bug_r_a4 and shutdown.  It should be the only such built-in property
that might be reachable from a function object.  With patches to this bug, we
have protected:

__proto__
__parent__
constructor

That leaves:

arguments   not shared with precompiled function
arity	    int-valued, so can't reach privileged objects
caller	    access is checked by fun_getProperty
length	    int-valued, so can't reach privileged objects
name	    string-valued, so can't reach privileged objects

IOW, all of these built-in properties are either of primitive type, or of
object type but a reference to an unshared object (arguments), or an
access-checked reference to another function object (caller, constructor).

/be

Comment 39

[Johnny Stenback (:jst)]

Comment on attachment 185975 [details] [diff] [review]
followup patch to check the proto-delegated 'constructor' access path

sr=jst

Comment 40

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 185975 [details] [diff] [review]
followup patch to check the proto-delegated 'constructor' access path

r=shaver, fwiw.

Comment 41

[Jay Patel [:jay]]

Comment on attachment 185911 [details] [diff] [review]
AVIARY_1_0_1 branch version of patch

Let's get this checked in on the branches! a=jay

Comment 42

[Brendan Eich [:brendan]]

Attachment: extended version of last patch to handle bug 297543

This should fix both bugs.

/be

Comment 43

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 186878 [details] [diff] [review]
extended version of last patch to handle bug 297543

r=shaver

Comment 44

[Brendan Eich [:brendan]]

Comment on attachment 186878 [details] [diff] [review]
extended version of last patch to handle bug 297543

Oops, lost the last hunk of the previous patch from this one.  Not to worry,
I've restored it in my tree.

/be

Comment 45

[Johnny Stenback (:jst)]

Comment on attachment 186878 [details] [diff] [review]
extended version of last patch to handle bug 297543

sr=jst

Comment 46

[Brendan Eich [:brendan]]

Attachment: simplified approach, with an unrelated fix

CheckAccessHelper and its usage in the last patch was a convoluted way of
having js_CheckAccess default to rt->checkObjectAccess, if clasp->checkAccess
is null. This patch also fixes a terrible jsid (not jsval) confusion in
obj_setSlot, and a nearby problem with JSACC_* bits containing both read/write
mode flags and a low-order access type code.

By making js_CheckAccess default to the runtime-global checkObjectAccess hook,
we avoid js_SharedCheckAccess altogether.

/be

Comment 47

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 186897 [details] [diff] [review]
simplified approach, with an unrelated fix

Yeah, much better.  Thanks, and r=shaver.

Comment 48

[Johnny Stenback (:jst)]

Comment on attachment 186897 [details] [diff] [review]
simplified approach, with an unrelated fix

sr=jst

Comment 49

[Brendan Eich [:brendan]]

Comment on attachment 186897 [details] [diff] [review]
simplified approach, with an unrelated fix

This is going into the trunk now.  Looking for branch approval.

/be

Comment 50

[Brendan Eich [:brendan]]

Fixed on the trunk.

/be

Comment 51

[Jay Patel [:jay]]

Comment on attachment 186897 [details] [diff] [review]
simplified approach, with an unrelated fix

Let's get this checked in so we can get a round of regression testing done
tomorrow. a=jay

Comment 52

[Daniel Veditz [:dveditz]]

Comment on attachment 186897 [details] [diff] [review]
simplified approach, with an unrelated fix

a=dveditz for branches

Comment 53

[Brendan Eich [:brendan]]

Fixed on the branches too.

/be

Comment 54

[Jay Patel [:jay]]

v.fixed on aviary with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.9)
Gecko/20050622 Firefox/1.0.5

Comment 55

[Daniel Veditz [:dveditz]]

Adding distributors

Comment 56

[Lloyd D Budd]

This caused regression bug # 298478 , as already identified in the other bug ,
and partially by "blocking" relationship that I added .

Comment 57

[Bob Clary [:bc] (inactive)]

these tests are in the qa test farm.