880591 - Assertion failure: lengthShape->writable() == lengthIsWritable, at jsarray.cpp

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack

x = [];
Object.defineProperty(x, 2, {
    configurable: true,
    get: (function() {})
});
Array.prototype.pop.call(x);
Object.freeze(x);
Object.defineProperty(x, "length", {});


asserts js debug shell on m-c changeset 57d30169ddd4 without any CLI arguments at Assertion failure: lengthShape->writable() == lengthIsWritable, at jsarray.cpp

Comment 1

[Christian Holler (:decoder)]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/8eac2a78a791
user:        Jeff Walden
date:        Tue Mar 19 17:12:06 2013 -0700
summary:     Bug 858381 - Implement non-writable array lengths, and add a boatload of tests.  r=jorendorff and r=bhackett for the major parts of this, r=jandem for the methodjit changes, r=jimb on a debugger test change, r=nmatsakis for the parallel test.  (More details available in the bug, where individual components of the fix were separately reviewed.)

This iteration took 174.563 seconds to run.

Comment 2

[Christian Holler (:decoder)]

Needinfo from Waldo based on comment 1 :)

Comment 3

[Jeff Walden [:Waldo]]

The consequences of having elements-header array-length-nonwritability not synced up with shape-writability may extend beyond simple wrong results and assertions.  I don't know that they do, but use of the flag is tricky enough that don't want to bet there are no security concerns when the flag is wrong.  Closing out of an abundance of caution, then, til we get this patched.

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Mark the length as non-writable in the header when freezing a dictionary-mode array

I handled this in the array-not-dictionary case, wrongly thinking that setGenericAttributes handled it in the dictionary case.  Oops.

I can't wait til we get rid of mutating property attributes being a separate operation from redefining properties entirely, so that there won't always be two places to patch to change property attributes.  Soon!

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 760008 [details] [diff] [review]
Mark the length as non-writable in the header when freezing a dictionary-mode array

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
If there's faulty logic to be exploited -- I don't know there is -- I suspect someone knowledgeable could audit bug 858381's patches to find it in a few hours of work.  Parlaying that faulty logic into a working exploit would take longer; how much, I don't know.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No.

Which older supported branches are affected by this flaw?
Just 23, this was introduced by a fairly recent fix.

If not all supported branches, which bug introduced the flaw?
Bug 858381.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Pretty sure this'll apply straightforwardly.

How likely is this patch to cause regressions; how much testing does it need?
I don't think there's much risk of regression now that both paths through JSObject::sealOrFreeze are covered, without possibility for logical lapses.  I'd expect fuzzing of randomly-frobbed arrays, with Object.freeze then called, with Object.defineProperty on the "length" property, would expose any issues fairly quickly -- wouldn't hurt to poke fuzzer principals and ask if they could whip something up along those lines.

Comment 6

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

Comment on attachment 760008 [details] [diff] [review]
Mark the length as non-writable in the header when freezing a dictionary-mode array

sec-approval+ for trunk.

Let's get this into Aurora after it is in trunk.

Comment 7

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9404c1d1df0c

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9404c1d1df0c

Comment 9

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

please nominate for aurora uplift.

Comment 11

[Jeff Walden [:Waldo]]

Comment on attachment 760008 [details] [diff] [review]
Mark the length as non-writable in the header when freezing a dictionary-mode array

[Approval Request Comment]
Bug caused by (feature/regressing bug #): non-writable array length feature, bug 858381
User impact if declined: potential security issue (unclear if one exists, would take too much work to prove there isn't one)
Testing completed (on m-c, etc.): in on m-c
Risk to taking this patch (and alternatives if risky): small, expands edge-case handling to cover more cases, pretty straightforward change
String or IDL/UUID changes made by this patch: N/A

Comment 12

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8cfa59681649

Comment 13

[u279076]

Marking status-firefox24:verified based on comment 9.