Closed Bug 904886 (CVE-2015-4516) Opened 11 years ago Closed 9 years ago

All property definition must enforce ES5's invariants regarding configurability, writability, etc.

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Tracking Status
firefox36 --- wontfix
firefox37 --- wontfix
firefox38 --- wontfix
firefox38.0.5 --- wontfix
firefox39 --- wontfix
firefox40 --- wontfix
firefox41 --- verified
firefox42 --- fixed
firefox-esr31 --- wontfix
firefox-esr38 --- wontfix

People

(Reporter: Waldo, Assigned: jorendorff)

References

Details

(Keywords: sec-high, Whiteboard: [fixed by bug 1125624][adv-main41+])

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.
> 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.
Keywords: sec-high
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?
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.
I'm looking at old security bugs. jorendorff, Waldo: any news here? Did Apple fix this yet?
Flags: needinfo?(jorendorff)
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.
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.
Flags: needinfo?(jorendorff)
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
Did you find an owner for this?
Flags: needinfo?(jorendorff)
No. Taking.
Assignee: general → jorendorff
Flags: needinfo?(jorendorff)
It has been a few months. Any updates on this issue?
Flags: needinfo?(jorendorff)
Group: javascript-core-security
One month since any activity on this bug that was filed almost a year ago.  Is there somebody who can work on this?
Waldo, could you maybe take a look at this?
Flags: needinfo?(jwalden+bmo)
It's my Q1 goal to clean up a ton of stuff in jsobj.cpp and vm/NativeObject.cpp including this bug.
Flags: needinfo?(jwalden+bmo)
Flags: needinfo?(jorendorff)
I'm upgrading this to critical, because it fixes bug 1105914, which is marked critical.
Bug 1125624 will fix this.
Depends on: 1125624
Keywords: sec-audit
Hooray!
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Group: javascript-core-security
Group: core-security → core-security-release
Flags: qe-verify+
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".
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`.
One last way to verify: see bug 1073808 comment 0, test case 1.
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!
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Whiteboard: [fixed by bug 1125624] → [fixed by bug 1125624][adv-main41+]
Alias: CVE-2015-4516
This would be risky to back-port to the ESR.
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.