Closed Bug 619283 Opened 14 years ago Closed 13 years ago

JS built-in methods still generally box undefined/null as |this| into the global object, instead of throwing

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: erights, Assigned: Waldo)

References

Details

(Whiteboard: [softblocker][fixed-in-tracemonkey])

Attachments

(2 files, 2 obsolete files)

0 - Detabify a test that needs updating after this fix
2.20 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
1 - Reject undefined and null as this in all builtin methods, and add tests
121.39 KB, patch
dmandelin
: review+
Details | Diff | Splinter Review 
To avoid privilege escalation bugs by global object leakage, ES5 repaired Ch15 to coerce the this-bindings of its methods by "ToObject". Thus, primitive values wrap but null and undefined throw an exception instead. For example, 15.4.4.10 Array.prototype.slice step 1 says:

    1. Let O be the result of calling ToObject passing the this value as the argument. 

However, on Minefield 4.0b8pre (2010-12-14)

  window[0] = 'a';
  window[1] = 'b';
  window[2] = 'c';
  window.length = 3;
  [].slice.call(null, 0); // prints a,b,c

showing that slice still leaks access to the global object.

I am not checking the "Many users could be harmed by this security problem: it should be kept hidden from the public until it is resolved." box below because FF4 is not yet released so there's time to fix this before people can be harmed by this problem.
A more elegant demonstration:

    (1,valueOf)(); // prints [object Window]
blocking2.0: --- → ?
The ComputeThisFromVp (?) abstraction is wrong and needs to be removed, probably to be replaced by a bool ToObject(JSContext* cx, Value* vp, JSObject** thisObj) interface that much more closely follows the spec.  Easy work, just takes time to audit everything, adjust as necessary, and write some tests.
Whiteboard: [good first bug]
blocking2.0: ? → betaN+
Remember that toString needs to be fixed differently. Currently of course it prints the global object ("[object Window]"). Rather that throw an error on null or undefined, per spec, toString() should return "[object Null]" or "[object Undefined]".
Are you testing Firefox 4 for that proposition?  Object.prototype.toString is fixed in this regard.
You're right. I assumed (wrongly and without testing) that global toString was the inherited Object.prototype.toString. On Minefield 4.0b9pre (2010-12-27)

  javascript:alert(toString.call(undefined))

alerts "[object Window]", hence the report. But 

  javascript:alert({}.toString.call(undefined))

correctly alerts "[object Undefined]". My mistake.

Since the normal behavior of window.toString() is just as it would be if it had not overridden Object.prototype.toString, why does it override it?
Interesting.  It hadn't occurred to me that Window.prototype.toString might exist.  This points out that when this bug is fixed we'll also need to fix the quickstub implementations as well, to ensure they do something sensible here.  I *think* this is the same ToObject stuff most of ES5 uses.

As for it, now that you've brought it to my attention, it's a relic of XPConnect (bridge between DOM and JS) implementation details, I think.  At one time WebIDL might even have required it, although these days I don't believe it does, if my search for "[[Call]]" in that document reveals all the right algorithms.
I will do the work mentioned in comment 3.
window.prototype.toString doesn't always return the same thing as Object.prototype.toString, fwiw.
Assignee: general → evilpies
Severity: blocker → normal
OS: Mac OS X → All
Hardware: x86 → All
Summary: Primordial privilege escalation from bad this-coercion → DOM and JS built-in methods still generally box undefined/null as |this| into the global object, instead of throwing
Version: Other Branch → Trunk
Attached patch v1 (obsolete) — — Splinter Review 
This patch is not really interesting most of the time i just replaced ComputeThisFromVp with js_ValueToObject. I nearly stripped all occurrences of ComputeThisFromVp, i only didn't change those in jsiter, jsxml, jsproxy, because i wasn't sure about the correct behavior. Those in jsdate are useless and will be stripped together with some other stuff in an other bug.
Maybe we would like to have some better error reporting, for the undefined/null case.
Attachment #500108 - Flags: review?
Comment on attachment 500108 [details] [diff] [review]
v1

Flagging self to get to this, yay for un-autocompletable r? meaning r? from the wind...
Attachment #500108 - Flags: review? → review?(jwalden+bmo)
Whiteboard: [good first bug] → [good first bug][softblocker]
Comment on attachment 500108 [details] [diff] [review]
v1

As far the idioms used here go, we should aim to read more like the spec.  And these days, the js_ prefix is out, js:: is in.  So I think we want this (former in jsobjinlines.h, latter extern'd in jsobj.h and implemented in jsobj.cpp):

namespace js {

inline JSObject *
ToObject(JSContext *cx, Value *vp)
{
    if (vp->isObject())
        return &vp->toObject();
    return ToObjectSlow(cx, vp);
}

JSObject *
ToObjectSlow(JSContext *cx, Value *vp)
{
    JS_ASSERT(!vp->isObject());
    if (vp->isBoolean() || vp->isNumber() || vp->isString()) {
        if (!js_PrimitiveToObject(cx, vp))
            return NULL;
        return &vp->toObject();
    }

    JS_ASSERT(vp->isNullOrUndefined());
    // ...report the error, return null...
}

}

Note that this still assigns the (possibly-created) object into *vp.  This minimizes the risk of losing the sole root for something that we depend on being rooted.  Conservative stack scanning greatly reduced this risk, but unfortunately it couldn't, and didn't, eliminate it.

In fun_bind, why change from the current target->isCallable() system?  Seems to me there's no reason not to just change out ComputeThisFromVp for ToObject like everywhere else, without disturbing surrounding code.

"PrimtiveToObject" is misspelled.  :-)

If you're going to make the changes to js_str_escape, I think you can copy the small bit in str_escape into js_str_escape and rename the method.  Whatever the purpose was of this split in the past, MXR seems to suggest it's no longer used.

In NormalizeThis, you've put braces around a single-line if statement -- those aren't needed.  Also, just use ToObject in there, after the primitive-string check.  Better yet, this lets you reduce the indentation of the check-for-String-object case.
Attachment #500108 - Flags: review?(jwalden+bmo)
>Note that this still assigns the (possibly-created) object into *vp.  This
>minimizes the risk of losing the sole root for something that we depend on
>being rooted.  Conservative stack scanning greatly reduced this risk, but
>unfortunately it couldn't, and didn't, eliminate it.
I don't like this :( But please let me define ToObject as returning bool.
>In fun_bind, why change from the current target->isCallable() system?  Seems to
>me there's no reason not to just change out ComputeThisFromVp for ToObject like
>everywhere else, without disturbing surrounding code.
It's how ECMA defines it, and fun_call and fun_apply are similar. And why should we bother converting to object?
>Also, just use ToObject in there, after the primitive-string
>check.  Better yet, this lets you reduce the indentation of the
>check-for-String-object case.
Can't do this, because ECMA disallows primitives to be converted to objects in this case.
Number.prototype.toString = null; 
String.prototype.toUpperCase.call(1) // should still say "1"
Blocks: es5
We convert to object because, per spec, we need callable.length (and somewhat extension-y, callable.name) if we have a function, and we get that from an object.  And by converting to object early, we avoid an isObject() check in the Value-taking IsCallable method.  That probably doesn't matter in practice, of course.  Yet doing extra work, when the relevant transformation to remove it is trivial, is a bad habit to acquire, and the easiest way to avoid that is not to do it.

Oh, I see what you mean about NormalizeThis and ToObject.  Yeah, you're right, we probably even had tests for that.

At this point we probably should just do this and get it done.  Mind posting an updated patch?  (And have you run the JS test suite with the patch here?  I have to think we have at least *some* tests that this breaks.)
I won't be able to finish this in the next 1~2 weeks. My hd crashed and i wanted to buy a new computer anyway.
Taking, want to finish this out, especially since I made mention of it in the strict mode post on hacks and then forgot we haven't implemented it yet (oops :-) )...
Assignee: evilpies → jwalden+bmo
How do you prevent global object leaking via indirect eval?
[eval][0]('this') will returns global object even in strict mode.
Ironically, strict mode will prevent overwriting eval to workaround this hole.
Also, strict mode code can invoke non-strict mode functions via Function constructor;
Function('return this;')
This bug doesn't fix anything unless all of them are fixed. But it will definitely violate the current ES5 spec.
It's true you can get at the global object many ways even in ES5 strict mode, but that's not the point: a translator such as Google Caja has to do too much work if this bug is unfixed. Caja removes eval completely from its prepared environment, so that's not a source of global capability leaks.

/be
All secure variants prohibit access to eval and Function.  This is a cornerstone of the secure variants: without this you don't have a secure variant.  So Function("return this") exposing the global object even when called from strict mode code, even per ES5, is not a concern.
Jesse, your fuzzers are going to need some changes to not fall afoul of the changes to be made here.  Of the JS tests (suite only, haven't tried jit-tests and expect more there) that fail with a WIP patch, most of them are you, for your fuzzers' habit of calling once-implicitly-global |__defineSetter__|, but with the changes here that must throw.  Just a heads-up.  :-)
Cameron, pace WebIDL, it looks to me like the Window interface's methods (maybe getters and setters too?  haven't thought through the implications) should not use ToObject for determining the passed-in this value.  It's all too common to call alert(), or clearTimeout(), or setTimeout(), and the like.  With ES5-like semantics this passes in undefined as this, and ToObject dutifully throws.  And I can easily imagine case-by-case fixes may be needed for these methods.  :-\  Still investigating with the patch I'm testing out right now...
OK.  I'm looking at http://dev.w3.org/2006/webapi/WebIDL/#es-operations and in the algorithm for the behaviour of the Function object the operation corresponds to, it doesn't call ToObject -- all it does is converts its arguments from ES values into IDL values and then invokes the prose description of the operation.  I think it probably should describe what the "this" object is when handing off to the prose description, in which case it should do something like this:

  * Let O be the this object.
  * If O is undefined, set O to the global object.
  * If O does not implement I, throw a TypeError.
  * ...
  * Let R be the result of performing the actions listed in the description
    of operation op with idlarg0..m−1 as the argument values and O as the object
    on which the operation was called.
  * ...

Is that right?  This would apply to all methods, though, not just those on Window.  (This probably isn't a practical concern, though, since methods from other objects are going to end up throwing the TypeError there.)

I also am not sure about the accessor properties for attributes on Window.
When inside a worker, do you want bare calls to methods on the global object to work there too?  Such as:

  -- a.html --
  <!DOCTYPE html>
  <script>
    new Worker('a.js');
  </script>

  -- a.js --
  postMessage('howdy doody');
Honestly, if I knew what I wanted, I would be a happy developer right now.  :-)

The problem as far as ES5 is concerned is that some methods might expose the global object or permit certain sorts of DOM manipulations.  So if there were a method on Window which returned |this|, somehow, that would be a problem.  The only ones I can think of that might do that right now are addEventListener, setTimeout, and setInterval, which call back and currently pass in the global object (window).

But "secure" variants probably should be expected to block off those methods.  They basically have to block off all DOM access now (consider |node.ownerDocument.defaultView|).  So is the ToObject thing maybe not necessary for DOM methods?  Perhaps we should expect that secure variants must only expose ECMAScript built-in methods (and perhaps any extensions the JS engine itself implements), in which case the ToObject stuff in WebIDL is not the right way.

Mark, you filed this originally, you've done secure variant work, is there some reason DOM methods must never box null/undefined this for secure variants to be happy?  I can't think of any from the times I've hacked on such variants writing exploits, but that was also a few years ago, and I assume the state of the art is far advanced since then.
Hi Jeff, I'm not sure I understand your question. Secure variants need to be able to deny untrusted code access to the global object. This is especially important in the browser of course, as all the page's authority is available from that frame's window object. However, if "(1,{}.value)()" leaks the window object, then it becomes extremely difficult for a non-translation-based securing of the frame to prevent untrusted code from obtaining access to the window object.

This hole is a form of privilege escalation: code which by analysis has no reachability path to the window object is nevertheless able to obtain such access by other means.
Basically, the DOM's behavior is described in terms of methods and properties which always assume everything handed to them is sane.  |this| is always an object of the appropriate kind, arguments are always of the types they're supposed to be, and so on.

But this isn't the case -- you can pass whatever value you want to a DOM method, or as many values as you want in arguments.  So there's a conversion process in JS bindings for these methods, adapting the mishmash of a dynamic language to the constraints of statically typed DOM properties and methods.

The question is: what should that conversion process do about null or undefined |this|?  Using ES5's ToObject won't work, because that would reject commonly-unqualified calls like |alert()| or |setTimeout()|.  But if you don't use ToObject, you're relying on the DOM method to not expose the boxed |this|.  Or would you be preventing access to all DOM methods, such that it wouldn't matter if a DOM method effectively consisted of |return this|?
Calling a built-in method with |this| coerced from null or undefined to the global object does not leak the global object unless the built-in method does something like return its (coerced) |this| parameter. Like, say, valueOf :-P.

So if there's no leak there is no need to pass undefined to built-in methods as |this| instead of the global object. This bug is still a softblocker but it does not need to boil the oceans. We should fix the leaky built-in methods.

To be concrete, |window.open(url)| must pass the global object as |this|, not undefined.

/be
The "if there's no leak" is the point of concern.  I'm not certain there is no such leak, nor that will never be such a leak in new DOM methods.

I'm finishing up a patch which ignores DOM methods to get something completed, but this really is a bug where doing it right means doing it fully, seems to me.  As far as practical utility goes it's all-or-nothing, unless they want to assume everything's broken and check for global-exposure everywhere.
This is not an all-or-nothing fix scenario. If we get the known leaks plugged we may be done. If not, the remaining leaky methods allow a larger whitelist.

/be
Window objects have properties whose getter returns the window itself: "self" and "frames".

Window objects have properties whose getter returns objects from which you can reach the window, as comment 25 notes.

Window objects have methods which return the window itself.  In particular, |open("#", "_self") === this| tests true at the global scope.

Of course anything pretending to be a safe JS variant would be sanitizing the hell out of window.open, right?
Yes, the whitelist cannot include those self-getters. But we cannot break them either, in ES5 strict mode. If you

"use strict";
alert(window.self.window);

you had better not get an exception dereferencing undefined, or a C++ crash. Mark can confirm that strict mode was not intended to break DOM window object methods and getters (the first part of window.self.window is an unqualified getter call).

/be
Brendan, nothing that might change here would break |window.self.window|, because a non-undefined |this| is explicitly passed to getters at every step.  Nothing would break just plain old |self|, either, because ES5 explicitly passes the global object in that case when looking up the "self" value (10.2.1.2.4 step 5).  It's only when ES5 passes |undefined| as this that it matters.

Specifically the concern is about something like |alert()|, which does explicitly pass |this| as |undefined|.  It's pretty clear we can't break |alert()| called without qualification like that.  The question is whether the secure variants are fine with all DOM methods boxing up whatever |this| is passed to them, rather than calling ToObject and throwing for null/undefined |this|.  If they're fine with that -- say, because they block off access to all DOM properties of any kind, including all global methods -- we just leave the DOM alone.  If they aren't, I'm not sure what to do.
(In reply to comment #33)
> Brendan, nothing that might change here would break |window.self.window|,
> because a non-undefined |this| is explicitly passed to getters at every step.

Not when bug 603201 is fixed and the getter is strict. So you could argue our DOM getters and setters are non-strict, but that goes for DOM methods too.

If we self-hosted some of the DOM, this would matter. But we have old native C++ implementations that need |this| to be something derived from the window object. So in general we can't break the DOM. Therefore *in general* DOM native methods act as non-strict self-hosted methods would.

> If they're fine with that -- say, because they block off access to all
> DOM properties of any kind, including all global methods -- we just leave the
> DOM alone.  If they aren't, I'm not sure what to do.

We're going in circles. Let's refocus this bug on JS (ES5-specified) built-in methods and make progress there. Mark and others will benefit. The DOM needs a separate bug and some more care, but we still benefit from divide and conquer. The whitelist is larger, it can allow valueOf.

/be
(In reply to comment #34)
> Not when bug 603201 is fixed and the getter is strict. So you could argue our
> DOM getters and setters are non-strict, but that goes for DOM methods too.

Are you sure?  Look at 10.2.1.2.4 step 5 -- looks to me like it doesn't, and resolving unqualified names always passes the global object to a getter or setter that's invoked.
(In reply to comment #35)
> (In reply to comment #34)
> > Not when bug 603201 is fixed and the getter is strict. So you could argue our
> > DOM getters and setters are non-strict, but that goes for DOM methods too.
> 
> Are you sure?  Look at 10.2.1.2.4 step 5 -- looks to me like it doesn't, and
> resolving unqualified names always passes the global object to a getter or
> setter that's invoked.

That's fine, but window.self does a qualifed get on the same object and if self were self-hosted in strict code, then what?

/be
The bigger picture point remains: divide and conquer, fix the core engine leaks in this bug. It's doable and (Mark can chime in any time) it helps.

/be
x.y explicitly passes in x to a getter (or setter) y, so if |self| were self-hosted[0] it would receive |window| as its |this|, likewise for just |self| as well.  More generally, |this| for a strict mode function can be the global object if it's explicitly passed in that way.

0. http://rimshot.vorb.is/
One test needs non-trivial fixing post-this-patch but is so riddled with tabs it made more sense to detabify, then update it in a subsequent patch.  The change is purely mechanical and needs no review, just posting it here in case someone tries to apply the real patch and encounters failures because of it.
Attachment #500108 - Attachment is obsolete: true
Attachment #507754 - Flags: review+
For the most part I just replaced the existing ComputeThisFromVp calls with ToObject calls.  I ended up removing and/or renaming all the ComputeThis* methods so as to be sure I'd not inadvertently left any in the code anywhere; I also took the time to be a little more rigorous in use of it regarding what it would accept and what it would assert could never happen (computing this for a strict frame, for example).

Interestingly, the generic version of methods (Array.map corresponding to Array.prototype.map) need to not convert their first argument to the global object, in the same way that the originals don't convert their |this|.  That's a wrinkle I hadn't considered prior to discovering a test that broke when it did the conversion.

JS_THIS_VALUE isn't strictly needed in this patch, but at one point in its development it came in handy, and it's something we should provide -- no harm doing that now.

Out of paranoia I had ToObject write the returned object back into the location being converted to an object, just to be super-safe from GC worries.  It's probably not necessary most places, but better safe than sorry.

I noticed several places where we'd commoned up code too much, resulting in misordered execution of argument-processing and this-handling, but mostly I left them alone when I didn't need to change something to make the tests I wrote pass.  The extra churn didn't seem to pay off here, not at this moment.

I have not tested this in-browser, will do that before actually pushing this assuming it gets r+, but it seemed better to get this up than to delay further on that.  Of course all must work for the push into the tree, but if parts get broken at halfway-reviewed points it won't matter, and it'll speed up the process.
Attachment #507760 - Flags: review?(cdleary)
Sigh, forgot to qref.
Attachment #507760 - Attachment is obsolete: true
Attachment #507761 - Flags: review?(cdleary)
Attachment #507760 - Flags: review?(cdleary)
Status: NEW → ASSIGNED
Whiteboard: [good first bug][softblocker] → [softblocker]
Waldo: good point that unqualified names evaluating to getters invoke with |this| as the global object -- forgot to ack that a few comments back. A headache for Mark but it reduces the followup bug work for us: we don't have to consider built-in getters, just methods. SIAK.

Want to take "DOM and" out of the summary? I will take a look at the patch a bit later tonight.

/be
At least as far as our own code goes, making __defineGetter__ and __defineSetter__ throw for null/undefined this may be a bridge too far.  I'm reverting for those two methods only, then looking and seeing if that breaks anything that seems likely to be more widespread.  We'll see what tryserver says about it, then go from there.
Summary: DOM and JS built-in methods still generally box undefined/null as |this| into the global object, instead of throwing → JS built-in methods still generally box undefined/null as |this| into the global object, instead of throwing
Don't worry about the __ terrors -- they are censored aggressively by Caja and other such systems.

/be
(In reply to comment #44)
> Don't worry about the __ terrors -- they are censored aggressively by Caja and
> other such systems.

WAIT STOP  PLAUSIBLE BUT WRONG ASSUMPTION ALERT

For ES3 era browsers (and thus for all browsers today) Caja does this censoring by translation. On browsers implementing full ES5 + proxies, i.e., expected FF4, we hope to implement SES without translation. Without translation, we plan to remove all properties on primordials that are not on our whitelist, when we can. For those we can't, we need to examine each individual case and decide whether we can safely allow it or if there's some other way to clean up the mess. If we encounter any we can't delete and haven't decided to admit, then we fail. We fail safe, but we fail. On that platform we'd have to fall back to expensive translation.

Fortunately, all __ properties on Object.prototype (except __proto__) seem to be deletable. So long as that remains the case, we will prevent access to their exclusive initial values, so it does not matter if those values are hazardous.

So the conclusion is right -- no problem here. But I want to be sure we all understand the constraints.
Re: comment 45, I'm hip, but thanks for breaking it down. The non-translating Caja speed wins are great and it would suck to have to go back to translation.

It seems in general we have made the nasty properties configurable. Please file bugs on counterexamples (and where you can't Object.defineProperty your way to victory, per bug 591846). Thanks,

/be
Attachment #507761 - Flags: review?(dmandelin)
Comment on attachment 507761 [details] [diff] [review]
1 - Reject undefined and null as this in all builtin methods, and add tests

dmandelin said he would take this review. My reviews take a long time. :-/
Attachment #507761 - Flags: review?(cdleary)
Comment on attachment 507761 [details] [diff] [review]
1 - Reject undefined and null as this in all builtin methods, and add tests

Two questions from review:

- It looks to me like JS_ComputeThis should pass Valueify(vp-2) to BoxThisForVp--I thought it wanted an argument that was 2 elements off what ComputeThisFromVp wanted. Is that right?

- I don't understand the changes to js_generic_native_method_dispatcher. We could go over those in person if you want.
Attachment #507761 - Flags: review?(dmandelin) → review+
We went over the two questions in person.  To recap:

- dmandelin was misreading ComputeThisFromArgv as ComputeThisFromVp.  Happily
  neither signature exists post-patch, so no further confusion is possible. \o/
- The generic-dispatcher changes are necessary so that our extensionland
  Array.sort(null), which calls the dispatcher, which then calls the method
  implementing Array.prototype.sort, will not box this -- it'll be passed
  through unaltered to Array.prototype.sort, which will properly throw a
  TypeError for that case.  This applies all the generic methods.

http://hg.mozilla.org/tracemonkey/rev/87f62dc9eff2

I landed this in TM in hopes of getting it in the next beta by getting it in tonight's merge, but changes landed just prior to mine broke TM, so the merge was from the changeset before the changeset before mine.  Assuming the tree's amenable (TM approves of the change, if you combine some results from my builds plus builds with that previous offending change backed out) I'll probably expedite this bug and land it singly in m-c tomorrow morning.
Whiteboard: [softblocker] → [softblocker][fixed-in-tracemonkey]
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
On the Nightly 6.0a1 (2011-04-23) built in Web Console:

    valueOf()
    [object Window]

I have reviewed the ES5 spec carefully and this is clearly wrong. (It also makes SES much harder.) Please let me know whether I should report this by reopening this bug or filing a distinct bug.
Mark, please file a distinct bug.  That behavior seems to be specific to the sandbox environment in Web Console (e.g. doing the same in a web page throws an exception).  Depending on how that sandbox is set up (e.g. whether the valueOf on the sandbox is a bound method), the behavior may even be correct.
See Also: → 652375
Done: (In reply to comment #52)
> Mark, please file a distinct bug.

Done: https://bugzilla.mozilla.org/show_bug.cgi?id=652375
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: