86147 - SECURITY: should require same-origin when defining a getter/setter

Status: VERIFIED FIXED

(Core :: Security, defect, P1)

Description

[Jesse Ruderman]

A page can define getters and setters for attribute of any object in another
domain as long as they can get a reference to the parent object.  Putting a
getter on an object lets me give it a different value (as well as find out when
the page accesses that value), and putting a setter on an object lets me find
out what values the page tries to assign to that object.

Using this hole, I can find out what foopy is if the page does
 passwd = foopy; 
or
 document.getElementById(foopy); 
 (by redefining document.getElementById to be a function defined by me)
or
 document.frm.password = foopy;
 (by redefining document.frm to be an object with a setter for the 'password'
 property which is defined by me).

This bug exists with at least two methods of defining getter or setter:
* otherWindow.variable getter = function () { ... };  [deprecated syntax]
* otherWindow.__defineGetter__ ("variable", function () { ... });

Comment 1

[Jesse Ruderman]

Attachment: testcase

Comment 2

[Mitchell Stoltz (not reading bugmail)]

I'm sure we've seen this before, and I thought we fixed it. Oh well. I'd
consider this an RTM stopper.

Comment 3

[Mitchell Stoltz (not reading bugmail)]

Adding nsBranch keyword as these bugs need to get into RTM.

Comment 4

[Mitchell Stoltz (not reading bugmail)]

Assigining the getter/setter cross-domain was blocked in 0.9, but not now, so
this is a regression. There used to be a callback from the JS engine when a
getter was assigned; maybe this got blown away by XPCDOM?

Comment 5

[lchiang]

I am marking [PDT+] to track this security bug as RTM stopper as per Mitch's
comment on 6-20 17:17.  Don't want to lose this bug in all of the nsbranch
keyword bugs.

Comment 6

[Johnny Stenback (:jst)]

Taking bug, I know how to fix this, we can re-introduce the old security check
that caught this in pre-XPCDOM builds.

Comment 7

[Johnny Stenback (:jst)]

Really taking bug...

Comment 8

[Johnny Stenback (:jst)]

Attachment: Proposed fix.

Comment 9

[Johnny Stenback (:jst)]

The attached patch fixes this problem, along with fixing a bug in the JS engine
which is required for the real fix for this problem. Mitchell, could you review?
Brendan, could you sr?

Comment 10

[Brendan Eich [:brendan]]

Do you need to set *_retval unconditionally at the top of
nsDOMClassInfo::CheckAccess, or is it "inout" in the sense that it comes in with
a sane default value?  Other than that, sr=[e-mail].

/be

Comment 11

[Johnny Stenback (:jst)]

Brendan, *_retval is meant to be only an out parameter but jband and I decided
to default the value of *_retval to a sane value (PR_TRUE) in XPConnect so that
we wouldn't need to set it in all scriptable helper methods. We only need to set
it to false to indicate failure.

Thanks for the sr.

Comment 12

[Mitchell Stoltz (not reading bugmail)]

Perfect...r=mstoltz

Comment 13

[Johnny Stenback (:jst)]

Fix checked in on both the trunk and the .92 branch.

Comment 14

[ckritzer (gone)]

Marking VERIFIED FIXED on:
-MacOS91 2001-07-10-03-0.9.2
-LinRH62 2001-07-10-08-0.9.2
-Win98SE 2001-07-10-05-0.9.2

Comment 15

[Jesse Ruderman]

See also bug 90591, a security hole that allows me to get around this check.

Comment 16

[Mitchell Stoltz (not reading bugmail)]

Removing NS_Confidential flag.