obj.func.__proto__.__parent__ problem in chrome XBL

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
12 years ago
10 years ago

People

(Reporter: moz_bug_r_a4, Assigned: brendan)

Tracking

({fixed-aviary1.0.5, fixed1.7.9})

Trunk
x86
Windows XP
fixed-aviary1.0.5, fixed1.7.9
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.7.9 +
blocking-aviary1.0.5 +
blocking1.8b3 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:fix] Bug details embargoed until August 1, 2005)

Attachments

(14 attachments, 2 obsolete attachments)

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
(Reporter)

Description

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

Comment 1

12 years ago
Created attachment 185156 [details]
testcase for Firefox 1.0.4
(Reporter)

Comment 2

12 years ago
Created attachment 185157 [details]
testcase for Mozilla 1.7.8
See bug 294795 comment 11.   Clearly the answer to my question is "for XBL, yes".
(Assignee)

Comment 4

12 years ago
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
Last Resolved: 12 years ago
Resolution: --- → DUPLICATE
(Assignee)

Comment 5

12 years ago
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 → ---
Depends on: 294795
Assignee: dveditz → jst
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 7

12 years ago
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+
Created attachment 185307 [details] [diff] [review]
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.
So would just nulling out the proto like we do for XPConnect not work?
(Reporter)

Comment 11

12 years ago
Created attachment 185329 [details]
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

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

Comment 13

12 years ago
Created attachment 185627 [details] [diff] [review]
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
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+
Created attachment 185701 [details] [diff] [review]
Additional patch to remove caps assertion from the jsfun.c change
Attachment #185701 - Flags: superreview?(brendan)
Attachment #185701 - Flags: review?(shaver)
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+
(Assignee)

Comment 17

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

Comment 18

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

Comment 19

12 years ago
Created attachment 185733 [details] [diff] [review]
updated patch to handle regexp access checks as well as function

Here's what I will check in right now.

/be
Attachment #185733 - Flags: superreview+
Attachment #185733 - Flags: review+
Attachment #185733 - Flags: approval1.8b3+
(Assignee)

Updated

12 years ago
Attachment #185733 - Attachment is obsolete: true
Attachment #185733 - Flags: superreview+
Attachment #185733 - Flags: review+
Attachment #185733 - Flags: approval1.8b3+
(Assignee)

Comment 20

12 years ago
Created attachment 185734 [details] [diff] [review]
shaver made me share the hook
Attachment #185734 - Flags: superreview+
Attachment #185734 - Flags: review+
Attachment #185734 - Flags: approval1.8b3+
(Assignee)

Comment 21

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

Comment 23

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

Comment 24

12 years ago
Created attachment 185803 [details]
testcase m.init.prototype.__proto__.__parent__
(Assignee)

Comment 25

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

Comment 28

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

Comment 30

12 years ago
Created attachment 185907 [details] [diff] [review]
revised to handle the problem jst noted in comment 29
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+
(Assignee)

Comment 32

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

Comment 34

12 years ago
Created attachment 185911 [details] [diff] [review]
AVIARY_1_0_1 branch version of patch

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

Comment 35

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

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago12 years ago
Component: Security → JavaScript Engine
Resolution: --- → FIXED
(Reporter)

Comment 36

12 years ago
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

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

Updated

12 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 38

12 years ago
Created attachment 185975 [details] [diff] [review]
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
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 41

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

Updated

12 years ago
Whiteboard: [sg:fix] → [sg:fix] need branch landing
(Assignee)

Updated

12 years ago
Blocks: 297543
(Assignee)

Comment 42

12 years ago
Created attachment 186878 [details] [diff] [review]
extended version of last patch to handle bug 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+
(Assignee)

Comment 44

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

Comment 46

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

Comment 49

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

Comment 50

12 years ago
Fixed on the trunk.

/be
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED

Comment 51

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

Comment 53

12 years ago
Fixed on the branches too.

/be
Keywords: fixed-aviary1.0.5, fixed1.7.9

Comment 54

12 years ago
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

Updated

12 years ago
Blocks: 298478

Updated

12 years ago
Flags: testcase?

Comment 56

12 years ago
This caused regression bug # 298478 , as already identified in the other bug ,
and partially by "blocking" relationship that I added .
Group: security

Comment 57

12 years ago
these tests are in the qa test farm.
Flags: testcase? → testcase+

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.