54976 - remove __defineGetter__ fropm public API

Status: VERIFIED INVALID

(Core :: Security, defect, P3)

Description

[Mitchell Stoltz (not reading bugmail)]

Looking at bug 36946 and a few other of Guninski's bugs, it looks like
__defineGetter__ and __defineSetter__ lead to a lot of malicious behavior.
Patrick suggested that because these are not part of the published Javascript
API and few people outside of Netscape are depending on these functions, they
should be removed from the public API anf permitted only to system code. That
way no one will come to depend on these functions. Comments?

Comment 1

[Mitchell Stoltz (not reading bugmail)]

Nominating for rtm as there could be some more exploits lurking here.

Comment 2

[Brendan Eich [:brendan]]

Why not just make those DOM properties permanent, which is the right fix, and 
which means we can WONTFIX this bug and avoid breaking everyone who uses 
getter=/setter= or __defineGetter__/__defineSetter__ (and there are many who do 
-- you should attempt a survey starting in m.jseng and m.xpcom)?

Please enumerate the other Gunninski bugs, or at least the properties such as 
location that should be permanent.

Note that without making location permanent, one can always delete it and then 
spy on the recreated property, which will lack a tinyid.  I therefore do not 
believe that removing __defineGetter__ fixes anything, and argue that this bug 
should be resolved INVALID.

/be

Comment 3

[Brendan Eich [:brendan]]

Removing rtm, adding shaver and jband.

/be

Comment 4

[Mitchell Stoltz (not reading bugmail)]

Brendan,
   Patrick suggested this change in addition to marking certain properties 
permanent (I have that listed as 44014). His rationale, if I understand it, is 
that under some circumstances, attacking a getter or setter to an existing object 
is even more powerful than replacing that object, and that in general, attaching 
getters and setters to an existing object with __defineGetter__ or 
__defineSetter__ is risky from a security point of view. We're not talking about 
blocking getter= or setter=, these are the correct ways of using this 
functionality. __defineGetter__ is an internal function which should never have 
been exposed to scripts. It was Prtick;s impression that very few pages outside 
Netscape use this syntax and that it can be removed without much breakage. This 
will cease to be true as people start using this syntax. Why do you need 
__defineGetter__ in addition to getter= ?

Comment 5

[Mitchell Stoltz (not reading bugmail)]

Restoring rtm. 

Comment 6

[Brendan Eich [:brendan]]

Can someone please cite the other bugs, or at least vulnerable properties?  Not 
that we know the complete list, just to get what's known out there (to me, at 
any rate).

__defineGetter__ is the JS2-compatible form of getter= -- they're equivalent.  
Same for __defineSetter__ and setter=.

We absolutely should not remove these from JS1.5, because there *are* XPCOM 
components that use them (at least, there were). If you need to safeguard DOM 
stuff from web page JS, use the JSACC_WATCH access check.  Oh wait, that check 
doesn't help in the spoofing attack case, where the getter= and setter= are done 
by script loaded in the same page as the location object.

I think the *only* property vulnerable to this "self-attack" (really, a spoofing 
attack).  Furthermore, I am now questioning why this attack works.  If you make 
a getter for location that returns "http://www.yahoo.com", what code in our UI 
makes use of that result?  Shouldn't that code be asking something other than 
the level 0 DOM location object for the page's true URI?

I really don't think you can mark this bug rtm, because there's no patch yet, 
and we need to get to the bottom of the issues above.

/be

Comment 7

[Mitchell Stoltz (not reading bugmail)]

The other bugs (presumably fixed now, but they're illustrations of a pattern)
are 36948, 40760, and 44013, in addition to 44014 which was already mentioned.
Would these exploits have been possible with getter= syntax in addition to
__defineGetter__ ?

Comment 8

[Brendan Eich [:brendan]]

bug 36948, which was dup'd by bug 44013 (I've reopened and so marked) was a bug 
where getter= and __defineGetter__ weren't equivalent, because the latter lacked 
a necessary access check.  That's fixed, so I don't think the bug counts against 
__defineGetter__ any more than it does against getter=.  And I don't think it 
counts against both any more than against watchpoints, which we are not removing 
(and which are correctly access-checked).

bug 40760, before it morphed into other bugs, was about the failure of the 
permanent (ECMA DontDelete) attribute to prevent redefinition via getter= or 
__defineGetter__ (same for setter=/__defineSetter__).  This bug was independent 
of which syntax you used.

I still don't see a problem except for location, and there I think the bug is 
that the UI derives the window title or whatever from window.location.  Mitch, 
can you verify that?

We should make location permanent anyway, but if the UI can get a more reliable 
source for the page's URL, we should do that too.

Removing __defineGetter__ and getter=, etc., still are not the right fix, I 
think, because spoofers can just do

    delete location;
    location = "http://www.yahoo.com";

This might have the same effect as defining the getter, no?  Please test and let 
us know.

/be 

Comment 9

[Mitchell Stoltz (not reading bugmail)]

Yes, it has the same effect. I'm convinced that making location permanent will
solve the remaining problems, if making it permanent prevents the assignment of
getters by any means. Invalid.

Comment 10

[John Unruh]

Verified invalid per mstoltz's comments.