36117 - Assignment of getter bypasses security checks

Status: VERIFIED FIXED

(Core :: Security, defect, P3)

Description

[Norris Boyd]

The following mail thread shows some of the syntax of getters and setters. 
According to Brendan, these accesses currently bypass per-property security 
checks. 

    Subject: 
            Re: getters and setters
       Date: 
            Mon, 17 Apr 2000 15:41:35 -0700
      From: 
            Brendan Eich <[e-mail]>
        To: 
            Lynn Davidson <[e-mail]>
        CC: 
            [e-mail]
 References: 
            1




Lynn Davidson wrote: 

  Norris   or Brendan - 

  I assume that the new getter and setter capabilities apply only to 
  properties of objects that the JavaScript developer creates herself, that 
  she cannot use it to create getters and setters for predefined core objects. 

  Or am I assuming incorrectly?

Incorrect -- getters and setters may be defined on any object that allows new 
properties to be added.  (Norris: there is nothing to
prevent a getter or setter from being defined in place of an existing readonly 
and/or permanent property--this sounds like a
security hole to me!) 

All native objects support getters and setters, so for example: 

var d = Date.prototype; 
d.year getter= function()  { return this.getFullYear(); }; 
d.year setter= function(y) { return this.setFullYear(y); }; 

is legit, and adds a handy year property to all instances of the Date class: 

var now = new Date; 
print(now.year);    // 2000 
now.year = 2001;    // timewarp 
print(now);         // prints 4/17/2001's localized Date string 

/be

Comment 1

[Norris Boyd]

Added beta2 keyword.

Comment 2

[Brendan Eich [:brendan]]

Patch coming up.

/be

Comment 3

[Brendan Eich [:brendan]]

Attachment: proposed fix

Comment 4

[Brendan Eich [:brendan]]

The fix changed more than just the JSOP_GETTER/JSOP_SETTER prefix-bytecode cases 
in jsinterp.c.  I took advantage of the common OBJ_LOOKUP_PROPERTY call that 
always must precede the old revision's calls to CheckRedeclaration, pushing that 
call down into the subroutine.  That required an out param telling whether the 
property was found predeclared, but compatibly declared, for JSOP_DEFVAR (cuz 
'var x' in JS does not alter a pre-defined x property in the variables object).

I also tidied unrelated indentation in jsscope.h, found while reviewing getter 
and setter logic and error cases.

Norris, can you try this patch out and give me an r=?  Thanks.

/be

Comment 5

[Norris Boyd]

I've built the patch on Windows.

Let's say a malicious script tries to replace a victim's window.location with a 
setter of its own choosing. Does this patch attempt to stop such an attack? How 
is that accomplished? 

Comment 6

[Brendan Eich [:brendan]]

First, the malicious setter would run with malicious.org's principals, so that 
should stop some amount of mischief (seeing form field values, e.g.).

Second, the patch protects only against properties that are JSPROP_READONLY or 
JSPROP_PERMANENT (or both) from being replaced by getter/setter pairs, or just 
by setters (it's illegal to have only a getter and no setter -- see jsscope.h in 
the patch).  So if window.location is not predefined as permanent and readonly 
(usually it makes no sense to be just permanent, unless you want to enforce the 
value too), the patch offers no further defense than the usual principals (point 
1 above).

Whaddya think?

/be

/be

Comment 7

[Norris Boyd]

So it sounds like there are no protections against changing a property in 
another protection domain to be a getter/setter that you own. It seems like a 
script should be able to monitor writes to objects that it cannot itself write 
to.

Comment 8

[Brendan Eich [:brendan]]

Yeah, script variables (non-consts) are vulnerable, just as user-defined 
functions have always been wrap-able or hijack-able.

Not sure this is more serious than the status quo ante.  Thoughts?

/be

Comment 9

[Norris Boyd]

I think this is more serious than status quo ante. (BTW, I've restricted access 
to top-level user variables and functions across domains for the past several 
milestones and have heard no complaints about broken scripts. IE 5 implements a 
similar policy, so perhaps offending scripts have been changed.)

Test case that demonstrates vulnerability is in URL field above. It is the 
following two files:

36117.html:
<script>
function f() {
        w.location getter = function() { alert("getting"); }
        w.location setter = function(a) { alert("setting to " + a); }
}
w = window.open("http://makeda/tests/mocha/norris/mozilla/36117a.html");
setTimeout(f, 5000);
</script>

36117a.html:
<script>
function f() {
        alert('hi');
        window.location='http://www.yahoo.com';
}
</script>
<a href="about:blank" onclick="f()">click here</a>       

Comment 10

[Brendan Eich [:brendan]]

Ok, sold -- and thanks for patching up the loose old same-origin policy to cover 
user-defined properties.  So the problem with getters and setters is that they 
must be defined (create new property, replacing any old one).  That says to me 
we need a checkAccess call in the JSOP_GETTER: JSOP_SETTER: case in jsinterp.c. 
I could treat defining a setter and/or getter exactly like adding a watchpoint. 
That looks like it would do the trick.  Patch coming up, let me know if it works 
for you.  Thanks,

/be

Comment 11

[Brendan Eich [:brendan]]

Attachment: revised jsinterp.c fix

Comment 12

[Norris Boyd]

I still get information leakage with the new fix. 

The problem appears to be the asymetric usage of the location object: you get a 
window's URI by reading window.location.href, but you can set a new href by 
assignment to window.location. So the addition of a call to checkAccess attempts 
a read of window.location, which is allowed, and the attacking script is allowed 
to attach a setter to the victim's location property. 

IE 5 enforces same origin for read access to window.location. Should we do the 
same? I believe that would fix this problem.

Comment 13

[Brendan Eich [:brendan]]

Yeah, we must protect window.location because that property's value is an object 
whose toString() method yields the currently loaded URL (or should -- could be a 
separate Mozilla bug here).

I checked in the revised patch.  Norris, off to you for window.location lock-up!

/be

Comment 14

[Norris Boyd]

Didn't get to this. Reassign to mstoltz.

Comment 15

[Phil Peterson]

[nsbeta2+]

Comment 16

[Mitchell Stoltz (not reading bugmail)]

Moving to M17.

Comment 17

[Paul Wyskoczka]

Changed QA contact to Cathy.

Comment 18

[John Unruh]

Assigning QA to czhang

Comment 19

[Mitchell Stoltz (not reading bugmail)]

Brendan,
  This exploit is still working. What more needs to be done to fix it? We have
security checks on all of the properties of window.location, are these still
being bypassed?

Comment 20

[Mitchell Stoltz (not reading bugmail)]

Marking nsbeta3, removing nsbeta2+. Exploit still exists, but it's not public, so 
it's not a beta stopper.

Comment 21

[Brendan Eich [:brendan]]

The JS engine does the right thing.  The DOM code does implement checkAccess. 
What's left?  Is the problem here that certain DOM properties are not permanent
(may be deleted, either via delete or by getter/setter=)?

I think we should disclose bugs like this.  It's not as if they won't be found
out anyway, soon enough.  Better yet, let's just fix the DOM to use permanence.

Comment 22

[Mitchell Stoltz (not reading bugmail)]

I think the problem described in this bug is fixed (I verified this with Roger 
today.) There is a related exploit (bug 38959), but the problem there is not with 
the CheckAccess callback from the JS engine; that works as it should. Marking 
FIXED and reopening to the public.

Comment 23

[cathy zhang]

verified