Closed
Bug 880591
Opened 11 years ago
Closed 11 years ago
Assertion failure: lengthShape->writable() == lengthIsWritable, at jsarray.cpp
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
VERIFIED
FIXED
mozilla24
Tracking | Status | |
---|---|---|
firefox22 | --- | unaffected |
firefox23 | + | fixed |
firefox24 | + | verified |
firefox-esr17 | --- | unaffected |
b2g18 | --- | unaffected |
People
(Reporter: gkw, Assigned: Waldo)
References
Details
(4 keywords, Whiteboard: [jsbugmon:update][adv-main23-])
Attachments
(2 files)
5.31 KB,
text/plain
|
Details | |
2.80 KB,
patch
|
bhackett1024
:
review+
akeybl
:
approval-mozilla-aurora+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
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
Updated•11 years ago
|
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
Comment 1•11 years ago
|
||
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.
Assignee | ||
Comment 3•11 years ago
|
||
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
Status: NEW → ASSIGNED
Flags: needinfo?(jwalden+bmo)
OS: Mac OS X → All
Hardware: x86_64 → All
Assignee | ||
Comment 4•11 years ago
|
||
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)
Updated•11 years ago
|
Attachment #760008 -
Flags: review?(bhackett1024) → review+
Updated•11 years ago
|
Blocks: 858381
status-b2g18:
--- → unaffected
status-firefox22:
--- → unaffected
status-firefox23:
--- → affected
status-firefox24:
--- → affected
status-firefox-esr17:
--- → unaffected
Assignee | ||
Comment 5•11 years ago
|
||
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.
Attachment #760008 -
Flags: sec-approval?
Comment 6•11 years ago
|
||
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+
Assignee | ||
Comment 7•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9404c1d1df0c
Target Milestone: --- → mozilla24
Reporter | ||
Updated•11 years ago
|
tracking-firefox23:
--- → ?
tracking-firefox24:
--- → ?
Comment 8•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9404c1d1df0c
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Flags: in-testsuite+
Updated•11 years ago
|
Status: RESOLVED → VERIFIED
Comment 9•11 years ago
|
||
JSBugMon: This bug has been automatically verified fixed.
Assignee | ||
Comment 11•11 years ago
|
||
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?
Updated•11 years ago
|
Attachment #760008 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 12•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/8cfa59681649
Comment 13•11 years ago
|
||
Marking status-firefox24:verified based on comment 9.
Updated•11 years ago
|
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main23-]
You need to log in
before you can comment on or make changes to this bug.
Description
•