Closed Bug 880591 Opened 8 years ago Closed 8 years ago

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


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox22 --- unaffected
firefox23 + fixed
firefox24 + verified
firefox-esr17 --- unaffected
b2g18 --- unaffected


(Reporter: gkw, Assigned: Waldo)



(4 keywords, Whiteboard: [jsbugmon:update][adv-main23-])


(2 files)

Attached file stack
x = [];
Object.defineProperty(x, 2, {
    configurable: true,
    get: (function() {})
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
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
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.
Needinfo from Waldo based on comment 1 :)
Flags: needinfo?(jwalden+bmo)
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.
Assignee: general → jwalden+bmo
Group: core-security
Flags: needinfo?(jwalden+bmo)
OS: Mac OS X → All
Hardware: x86_64 → All
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!
Attachment #760008 - Flags: review?(bhackett1024)
Attachment #760008 - Flags: review?(bhackett1024) → review+
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?

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.
Attachment #760008 - Flags: sec-approval?
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.
Attachment #760008 - Flags: sec-approval? → sec-approval+
Closed: 8 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
JSBugMon: This bug has been automatically verified fixed.
please nominate for aurora uplift.
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
Attachment #760008 - Flags: approval-mozilla-aurora?
Attachment #760008 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Marking status-firefox24:verified based on comment 9.
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main23-]
Group: core-security
Keywords: sec-high
You need to log in before you can comment on or make changes to this bug.