90591 - should require same-origin for getting __proto__ property

Status: VERIFIED FIXED

(Core :: Security, defect)

Description

[Jesse Ruderman]

I can get the prototype of an object in another window, and then set things on
the prototype.  Setting values for properties of the prototype isn't very
interesting, because those properties will be overridden properties with the
same name set on the victim page itself.  However, I get put getters and setters
on the prototype, which are called even if the victim page has already set a
value for the same property.

Since this bug effectively lets me put getters and setters on objects in another
domain, it has the same security impact as bug 86147:
"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."

I haven't tested for exploits involving *setting* an object's __proto__ property
in another domain, but that should be blocked as well if it isn't already.

Comment 1

[Jesse Ruderman]

Attachment: demo

Comment 2

[Mitchell Stoltz (not reading bugmail)]

Nominating for nsBranch; this is pretty seroius.

Comment 3

[Brendan Eich [:brendan]]

How about cc'ing some likely helpers?

jst, anyone: why wasn't the patch for 86147 sufficient?  Debugger session seems
like the quickest way to find out.

/be

Comment 4

[Johnny Stenback (:jst)]

Attachment: First stab at fixing this problem

Comment 5

[selmer (gone)]

PDT+, please check in if you get fully comfortable and fully reviewed.

Comment 6

[Johnny Stenback (:jst)]

Attachment: Proposed fix, do security checks for __proto__ and __parent__, don't piggy-back on JS_GetProperty() for doing the security check in the CheckAccess hook, call the security manager directly

Comment 7

[vidur (gone)]

r/sr=vidur

Comment 8

[Brendan Eich [:brendan]]

That JS_GetStringBytes scares me -- it decimates 16-bit to 8-bit chars without
checking that high bytes are all zero.  Why does the security manager take a
const char* for that method?

/be

Comment 9

[Mitchell Stoltz (not reading bugmail)]

The property name ultimately gets stored as an 8-bit string in prefs, that's why
it's a const char*. Are there cases where str could have non-sero second bytes?

Other than that, r=mstoltz.

Comment 10

[Brendan Eich [:brendan]]

mstoltz: yes, of course -- JS allows any string to identify a property, and JS
strings are UTF16 vectors without combining sequences and surrogate pairs (I'm
channeling erik).  So a string that named a property in Chinese *could*, when
chopped down to 8-bit chars by JS_GetStringBytes, happen to look like one of the
pref-controlled names.

Doesn't the Prefs API support Unicode?  If so, then so should your security
manager API.  If not, bugs need to be filed.

/be

Comment 11

[Johnny Stenback (:jst)]

Attachment: Convert unicode to UTF8 before calling into the security manager.

Comment 12

[Brendan Eich [:brendan]]

Ok, sold -- r/sr=[e-mail] on the UTF8 patch.

/be

Comment 13

[Johnny Stenback (:jst)]

Mitch, you checked in the fix for this, right? Can we mark this FIXED now?

Comment 14

[Mitchell Stoltz (not reading bugmail)]

Fixed. Trunk and branch.

Comment 15

[ckritzer (gone)]

Marking VERIFIED FIXED on TRUNK & BRANCH:
-MacOS91 2001-07-23-03-0.9.2
-LinRH62 2001-07-23-04-0.9.2
-Win98SE 2001-07-23-06-0.9.2

-MacOS91 2001-07-23-04-trunk
-LinRH62 2001-07-23-06-trunk
-Win98SE 2001-07-23-06-trunk

Comment 16

[Mitchell Stoltz (not reading bugmail)]

Removing NS_Confidential flag.