904886 - (CVE-2015-4516) All property definition must enforce ES5's invariants regarding configurability, writability, etc.

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Every API that enforces that a non-configurable property can't be reconfigured (except in its writability, and except in value if it's writable), does so with one-off logic specific to that particular API.  Anything that doesn't go through there -- including the JSObject::define{Property,Element,Generic} funnels, can violate the property invariants.  This Is Bad.

Bug 901351 comment 14 provides one example of how this results in badness -- in this case, of a "frozen" object being "unfrozen" such that immutable properties become mutable, &c.:

    function thaw(obj) {
        var done = true;
        JSON.parse("[0, 1]", function (k, v) {
            if (this !== obj && k !== "" && done) {
                done = false;
                this[1 - k] = obj;
            }
            return v;
        });
    }

    var obj = Object.freeze({a: 1, b: 2, c: 3});
    obj.a = false;  // does nothing, obj.a is non-writable
    assertEq(obj.a, 1);
    thaw(obj);  // bug: the properties become writable, which shouldn't be possible
    obj.a = false;
    assertEq(obj.a, 1);  // FAILS, obj.a is now false

The internal Walk algorithm (after bug 901351, at least) uses JSObject::defineGeneric, which blithely modifies/overwrites non-configurable properties.  This instance allows improper redefinition of pretty much any user-controllable, enumerable, non-configurable property.  (It won't always work, but it's hard to describe the exceptions.  But usually, if the object isn't too weird, you could use JSON.parse to violate an arbitrarily-chosen property's invariants.)  I don't know if other places perform JSObject.define{Property,Generic,Element} so user-controllably, but there's probably at least one other place somewhere that does so.

Comment 1

[Jason Orendorff [:jorendorff]]

> This Is Bad.

To be specific:

 1. We rely on non-configurable properties to make window.window and
    window.location [Unforgeable] and this is a security property.

    I don't think the window object is not vulnerable to thaw().
    thaw(window) triggers a "too much recursion" exception without clobbering
    any properties.

    But if as Waldo suspects there are other places besides JSON.parse that
    perform an unchecked JSObject::defineProperty user-controllably, then
    we are vulnerable.

 2. Content code may rely on ES5's invariants to enforce security properties,
    and that may be insecure.

    For example, suppose Google Caja relies on Object.freeze (I don't know if it
    does), and foobar.com relies on Caja to support third-party JS apps or ads
    (I don't know who does this if anyone).

    Then foobar.com's users might be vulnerable. A malicious app might be able to
    use thaw() to escape its sandbox.

I think either danger alone would make this sec-high.

Comment 2

[Jason Orendorff [:jorendorff]]

Safari has something like this bug.

In Safari, thaw() works on array objects. It even makes the properties configurable, unlike SpiderMonkey. Here's the code I used:

    var arr = [1, 2, 3];
    Object.freeze(arr);
    thaw(arr);
    arr[0] = "X";
    console.log(arr[0] === 1 ? "no bug" : "bug!");

...with the definition of thaw() from comment 0. I just pasted them into Jesse's shell: http://www.squarefree.com/shell/shell.html

So Arrays are vulnerable. But plain old Object objects seem to be immune to thaw(). Still not good.

dveditz, we have to tell the Nitro team about this, but Waldo and I are a little concerned about their possibly releasing a patch that results in us getting 0-day'd. This might take a week or two for us to fix properly. What should I do?

Comment 3

[Jason Orendorff [:jorendorff]]

I just found out Object.defineProperty on a proxy also bypasses configurability checks.

    js> var x = Object.freeze({p:1});
    js> Object.defineProperty(new Proxy(x, {}), "p", {value: 0});
    ({})
    js> x.p
    0

Starting to feel a little urgent. Waldo, is this on your list?

I'm going to try to report this to Apple today.

Comment 4

[Jan de Mooij [:jandem]]

I'm looking at old security bugs. jorendorff, Waldo: any news here? Did Apple fix this yet?

Comment 5

[Jeff Walden [:Waldo]]

I haven't looked at this at all.  Intl, test262, and now bug 903332 and bug 630464, not to mention getting through reviews and not having abysmal response time there, have all been higher priority than this somewhat-theoretical-ish problem.

Comment 6

[Jason Orendorff [:jorendorff]]

Thanks for the ping.

I'm still a little more worried than Waldo that this is really dangerous, based on comment 2 and comment 3. Unfortunately, fixing this is a fairly big job. I'll try and find an owner for this today.

Comment 7

[Oliver Hunt]

Tracking in webkit as https://bugs.webkit.org/show_bug.cgi?id=122759

Thanks for the heads up.  If anyone wants to be cc'd create a b.w.o account and email me :D

Comment 8

[Al Billings [:abillings - ex-MoCo]]

Did you find an owner for this?

Comment 9

[Jason Orendorff [:jorendorff]]

No. Taking.

Comment 10

[Al Billings [:abillings - ex-MoCo]]

It has been a few months. Any updates on this issue?

Comment 11

[David Bolter [:davidb] (NeedInfo me for attention)]

*poke*

Comment 12

[Andrew McCreight [:mccr8]]

One month since any activity on this bug that was filed almost a year ago.  Is there somebody who can work on this?

Comment 13

[Andrew McCreight [:mccr8]]

Waldo, could you maybe take a look at this?

Comment 14

[Jason Orendorff [:jorendorff]]

It's my Q1 goal to clean up a ton of stuff in jsobj.cpp and vm/NativeObject.cpp including this bug.

Comment 15

[Andrew McCreight [:mccr8]]

I'm upgrading this to critical, because it fixes bug 1105914, which is marked critical.

Comment 16

[Jason Orendorff [:jorendorff]]

Bug 1125624 will fix this.

Comment 17

[Andrew McCreight [:mccr8]]

Hooray!

Comment 18

[Jason Orendorff [:jorendorff]]

To manually confirm this is fixed:

- Go to bug 1105914 and click the first attachment. You'll get a blank web page.

- Open the Web Console.

Correct behavior: Ignore the "character encoding" warning. You should see 6 other messages. The last three should be identical to the first three.

Buggy behavior: The last three messages just said "undefined".

Comment 19

[Jason Orendorff [:jorendorff]]

Another way to verify:

- Open the Web Console.

- Paste in the following code (from comment 0) and hit Enter:

    function thaw(obj) {
        var done = true;
        JSON.parse("[0, 1]", function (k, v) {
            if (this !== obj && k !== "" && done) {
                done = false;
                this[1 - k] = obj;
            }
            return v;
        });
    }
    var obj = Object.freeze({a: 1, b: 2, c: 3});
    thaw(obj);
    obj.a = false;
    obj.a

Correct behavior: The console replies with the number 1.

Buggy behavior: The console would reply `false`.

Comment 20

[Jason Orendorff [:jorendorff]]

One last way to verify: see bug 1073808 comment 0, test case 1.

Comment 21

[Andrei Vaida [:avaida]]

This is verified fixed on Firefox 41.0b9 (20150910171927), using Windows 10 x64, Ubuntu 14.04 x86 and Mac OS X 10.9.5.

Confirmed by following the instructions from Comment 19 and Comment 20 -- thanks Jason!

Comment 22

[Daniel Veditz [:dveditz]]

This would be risky to back-port to the ESR.