Closed Bug 296397 Opened 15 years ago Closed 14 years ago

obj.func.__proto__.__parent__ problem in chrome XBL

Categories

(Core :: JavaScript Engine, defect)

x86
Windows XP
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: moz_bug_r_a4, Assigned: brendan)

References

Details

(Keywords: fixed-aviary1.0.5, fixed1.7.9, Whiteboard: [sg:fix] Bug details embargoed until August 1, 2005)

Attachments

(14 files, 2 obsolete files)

608 bytes, text/html
Details
814 bytes, text/html
Details
2.21 KB, patch
Details | Diff | Splinter Review
278 bytes, text/html
Details
1.86 KB, patch
shaver
: review+
Details | Diff | Splinter Review
1.68 KB, patch
brendan
: review+
brendan
: superreview+
brendan
: approval1.8b3+
Details | Diff | Splinter Review
3.97 KB, patch
brendan
: review+
brendan
: superreview+
brendan
: approval1.8b3+
Details | Diff | Splinter Review
443 bytes, text/html
Details
5.68 KB, patch
shaver
: review+
Details | Diff | Splinter Review
611 bytes, patch
brendan
: review+
brendan
: superreview+
brendan
: approval1.8b3+
Details | Diff | Splinter Review
5.74 KB, patch
brendan
: review+
brendan
: superreview+
Details | Diff | Splinter Review
3.15 KB, patch
shaver
: review+
Details | Diff | Splinter Review
6.81 KB, patch
shaver
: review+
Details | Diff | Splinter Review
11.35 KB, patch
shaver
: review+
brendan
: approval1.8b3+
Details | Diff | Splinter Review
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:
See bug 294795 comment 11.   Clearly the answer to my question is "for XBL, yes".
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 ***
Status: UNCONFIRMED → RESOLVED
Closed: 15 years ago
Resolution: --- → DUPLICATE
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
Unduping. I personally find it too confusing to have "fixed" bugs taking more
fixes. Separation helps QA as well
Status: RESOLVED → UNCONFIRMED
Resolution: DUPLICATE → ---
Assignee: dveditz → jst
Status: UNCONFIRMED → NEW
Ever confirmed: true
Dan, bug 294795 is not marked FIXED.  Did you mean some other bug?

/be
*** Bug 296489 has been marked as a duplicate of this bug. ***
Flags: blocking1.7.9+
Flags: blocking-aviary1.0.5+
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.
So would just nulling out the proto like we do for XPConnect not work?
Attached file 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.
(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.
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
Attachment #185627 - Flags: superreview?(jst)
Attachment #185627 - Flags: review?(shaver)
Comment on attachment 185627 [details] [diff] [review]
possible general fix

Do we need to do the same for regexps? r=shaver
Attachment #185627 - Flags: review?(shaver) → review+
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
Attachment #185627 - Flags: superreview?(jst) → superreview+
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
Attachment #185701 - Flags: superreview?(brendan)
Attachment #185701 - Flags: superreview+
Attachment #185701 - Flags: review?(shaver)
Attachment #185701 - Flags: review+
Attachment #185701 - Flags: approval1.8b3+
(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
Here's what I will check in right now.

/be
Attachment #185733 - Flags: superreview+
Attachment #185733 - Flags: review+
Attachment #185733 - Flags: approval1.8b3+
Attachment #185733 - Attachment is obsolete: true
Attachment #185733 - Flags: superreview+
Attachment #185733 - Flags: review+
Attachment #185733 - Flags: approval1.8b3+
Attachment #185734 - Flags: superreview+
Attachment #185734 - Flags: review+
Attachment #185734 - Flags: approval1.8b3+
I checked in the JS engine changes -- jst's gonna land the assertion removal too.

/be
Caps change landed on the trunk.
Flags: blocking1.8b3+
Whiteboard: [sg:fix]
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]
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
Attachment #185837 - Flags: superreview?(jst)
Attachment #185837 - Flags: review?(shaver)
Comment on attachment 185837 [details] [diff] [review]
Share the runtime-wide access check class hook harder

r=shaver
Attachment #185837 - Flags: review?(shaver) → review+
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).
Attachment #185837 - Flags: superreview?(jst) → superreview+
(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
(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.
Attachment #185837 - Attachment is obsolete: true
Attachment #185907 - Flags: superreview?(jst)
Attachment #185907 - Flags: review?(shaver)
Comment on attachment 185907 [details] [diff] [review]
revised to handle the problem jst noted in comment 29

r=shaver
Attachment #185907 - Flags: review?(shaver) → review+
This applied on top of the last patch fixes the problem jst noted.  Thanks to
him for testing.  I'm checking in now.

/be
Attachment #185908 - Flags: superreview+
Attachment #185908 - Flags: review+
Attachment #185908 - Flags: approval1.8b3+
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
Attachment #185907 - Flags: superreview?(jst) → superreview+
Waiting for approval to land.

/be
Assignee: jst → brendan
Status: NEW → ASSIGNED
Attachment #185911 - Flags: superreview+
Attachment #185911 - Flags: review+
Attachment #185911 - Flags: approval1.7.9?
Attachment #185911 - Flags: approval-aviary1.0.5?
Fixed on trunk -- dveditz will approve in good time for the branches, I'm sure.

/be
Status: ASSIGNED → RESOLVED
Closed: 15 years ago15 years ago
Component: Security → JavaScript Engine
Resolution: --- → FIXED
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.
(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]
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
Attachment #185975 - Flags: superreview?(jst)
Attachment #185975 - Flags: review?(shaver)
Comment on attachment 185975 [details] [diff] [review]
followup patch to check the proto-delegated 'constructor' access path

sr=jst
Attachment #185975 - Flags: superreview?(jst) → superreview+
Comment on attachment 185975 [details] [diff] [review]
followup patch to check the proto-delegated 'constructor' access path

r=shaver, fwiw.
Attachment #185975 - Flags: review?(shaver) → review+
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
Attachment #185911 - Flags: approval1.7.9?
Attachment #185911 - Flags: approval1.7.9+
Attachment #185911 - Flags: approval-aviary1.0.5?
Attachment #185911 - Flags: approval-aviary1.0.5+
Whiteboard: [sg:fix] → [sg:fix] need branch landing
Blocks: 297543
This should fix both bugs.

/be
Attachment #186878 - Flags: superreview?(jst)
Attachment #186878 - Flags: review?(shaver)
Comment on attachment 186878 [details] [diff] [review]
extended version of last patch to handle bug 297543

r=shaver
Attachment #186878 - Flags: review?(shaver) → review+
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 on attachment 186878 [details] [diff] [review]
extended version of last patch to handle bug 297543

sr=jst
Attachment #186878 - Flags: superreview?(jst) → superreview+
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
Attachment #186897 - Flags: superreview?(jst)
Attachment #186897 - Flags: review?(shaver)
Comment on attachment 186897 [details] [diff] [review]
simplified approach, with an unrelated fix

Yeah, much better.  Thanks, and r=shaver.
Attachment #186897 - Flags: review?(shaver) → review+
Comment on attachment 186897 [details] [diff] [review]
simplified approach, with an unrelated fix

sr=jst
Attachment #186897 - Flags: superreview?(jst) → superreview+
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
Attachment #186897 - Flags: approval1.8b3+
Attachment #186897 - Flags: approval1.7.9?
Attachment #186897 - Flags: approval-aviary1.0.5?
Fixed on the trunk.

/be
Status: REOPENED → RESOLVED
Closed: 15 years ago14 years ago
Resolution: --- → FIXED
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
Attachment #186897 - Flags: approval1.7.9?
Attachment #186897 - Flags: approval1.7.9+
Attachment #186897 - Flags: approval-aviary1.0.5?
Attachment #186897 - Flags: approval-aviary1.0.5+
Comment on attachment 186897 [details] [diff] [review]
simplified approach, with an unrelated fix

a=dveditz for branches
Fixed on the branches too.

/be
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
Adding distributors
Whiteboard: [sg:fix] need branch landing → [sg:fix] Bug details embargoed until July 20, 2005
Whiteboard: [sg:fix] Bug details embargoed until July 20, 2005 → [sg:fix] Bug details embargoed until August 1, 2005
Blocks: 298478
Flags: testcase?
This caused regression bug # 298478 , as already identified in the other bug ,
and partially by "blocking" relationship that I added .
Group: security
these tests are in the qa test farm.
Flags: testcase? → testcase+
Flags: in-testsuite+ → in-testsuite?
You need to log in before you can comment on or make changes to this bug.