Crash [@ array_length_setter]

VERIFIED FIXED in Firefox 44, Firefox OS v2.5

Status

()

Core
JavaScript Engine: JIT
--
critical
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: gkw, Assigned: jandem)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
mozilla45
x86_64
Mac OS X
crash, regression, sec-high, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 unaffected, firefox43 unaffected, firefox44+ verified, firefox45+ verified, firefox46 verified, firefox-esr38 unaffected, b2g-v2.0 unaffected, b2g-v2.0M unaffected, b2g-v2.1 unaffected, b2g-v2.1S unaffected, b2g-v2.2 unaffected, b2g-v2.5 fixed, b2g-v2.2r unaffected, b2g-master affected)

Details

(Whiteboard: [jsbugmon:update], crash signature)

Attachments

(4 attachments)

(Reporter)

Description

3 years ago
x = [];
print(Object.defineProperty(x, 0, {
    get: function() {
        this["length"] = this;
    }
}));
print(x);
print(x);

crashes js debug and opt shell on m-c changeset 42627d5369b3 with --fuzzing-safe --no-threads --ion-eager at array_length_setter

Configure options: (debug)

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --disable-threadsafe --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

python -u ~/funfuzz/js/compileShell.py -b "--enable-debug --enable-more-deterministic" -r 42627d5369b3

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/73e1760e3c4d
user:        Jan de Mooij
date:        Thu Oct 15 16:20:29 2015 +0200
summary:     Bug 1214562 part 2 - Refactor SetPropertyCache regalloc. r=bhackett

Jan, is bug 1214562 a likely regressor? Setting s-s because crashes with arrays on the stack make me uneasy.
Flags: needinfo?(jdemooij)
(Reporter)

Comment 1

3 years ago
Created attachment 8684792 [details]
debug stack

(lldb) bt 5
* thread #1: tid = 0x1a787, 0x000000010007e045 js-dbg-64-dm-darwin-42627d5369b3`array_length_setter(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>, JS::ObjectOpResult&) [inlined] js::ObjectGroup::clasp(this=0xe4e4e4e400000000) const at ObjectGroup.h:94, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=EXC_I386_GPFLT)
  * frame #0: 0x000000010007e045 js-dbg-64-dm-darwin-42627d5369b3`array_length_setter(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>, JS::ObjectOpResult&) [inlined] js::ObjectGroup::clasp(this=0xe4e4e4e400000000) const at ObjectGroup.h:94
    frame #1: 0x000000010007e045 js-dbg-64-dm-darwin-42627d5369b3`array_length_setter(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>, JS::ObjectOpResult&) [inlined] JSObject::getClass(this=0x0000000102d78000) const + 3 at jsobj.h:122
    frame #2: 0x000000010007e042 js-dbg-64-dm-darwin-42627d5369b3`array_length_setter(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>, JS::ObjectOpResult&) [inlined] bool JSObject::is<js::ArrayObject>(this=0x0000000102d78000) const at jsobj.h:543
    frame #3: 0x000000010007e042 js-dbg-64-dm-darwin-42627d5369b3`array_length_setter(cx=0x0000000102d69400, obj=<unavailable>, id=JS::HandleId @ rdx, vp=JS::MutableHandleValue @ r10, result=0x00007fff5fbfcc48) + 18 at jsarray.cpp:494
    frame #4: 0x0000000102c4bc32
(lldb)
(Reporter)

Comment 2

3 years ago
Created attachment 8684793 [details]
Opt stack

(lldb) bt 5
* thread #1: tid = 0x1a93f, 0x000000010004e8c0 js-64-dm-darwin-42627d5369b3`array_length_setter(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>, JS::ObjectOpResult&) [inlined] js::ObjectGroup::clasp(this=0x0000000000000000) const at ObjectGroup.h:94, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x000000010004e8c0 js-64-dm-darwin-42627d5369b3`array_length_setter(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>, JS::ObjectOpResult&) [inlined] js::ObjectGroup::clasp(this=0x0000000000000000) const at ObjectGroup.h:94
    frame #1: 0x000000010004e8c0 js-64-dm-darwin-42627d5369b3`array_length_setter(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>, JS::ObjectOpResult&) [inlined] JSObject::getClass(this=0x0000000101e6f000) const + 3 at jsobj.h:122
    frame #2: 0x000000010004e8bd js-64-dm-darwin-42627d5369b3`array_length_setter(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>, JS::ObjectOpResult&) [inlined] bool JSObject::is<js::ArrayObject>(this=0x0000000101e6f000) const at jsobj.h:543
    frame #3: 0x000000010004e8bd js-64-dm-darwin-42627d5369b3`array_length_setter(cx=0x0000000101e79400, obj=<unavailable>, id=JS::HandleId @ rdx, vp=JS::MutableHandleValue @ r10, result=0x00007fff5fbfd578) + 13 at jsarray.cpp:494
    frame #4: 0x00000001017c3ce7
(lldb)
Keywords: sec-high
(Assignee)

Comment 3

3 years ago
Created attachment 8686637 [details] [diff] [review]
Part 1 - Fix

The object and value registers for a setprop can now overlap:

  o.prop = o;

This fixes the JSPropertyOp setter code to not clobber the object register in this case.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8686637 - Flags: review?(bhackett1024)
(Assignee)

Comment 4

3 years ago
Created attachment 8686639 [details] [diff] [review]
Part 2 - Test
Attachment #8686637 - Flags: review?(bhackett1024) → review+
(Assignee)

Comment 5

3 years ago
Comment on attachment 8686637 [details] [diff] [review]
Part 1 - Fix

[Security approval request comment]
* How easily could an exploit be constructed based on the patch?
It's not trivial or easy. Maybe with some work.

* 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?
44+.

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

* Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should apply.

* How likely is this patch to cause regressions; how much testing does it need?
Unlikely; it's also a pretty rare case we're fixing.
Attachment #8686637 - Flags: sec-approval?
status-b2g-v2.0: --- → unaffected
status-b2g-v2.0M: --- → unaffected
status-b2g-v2.1: --- → unaffected
status-b2g-v2.1S: --- → unaffected
status-b2g-v2.2: --- → unaffected
status-b2g-v2.2r: --- → unaffected
status-b2g-v2.5: --- → affected
status-b2g-master: --- → affected
status-firefox42: --- → unaffected
status-firefox43: --- → unaffected
status-firefox44: --- → affected
status-firefox-esr38: --- → unaffected
sec-approval+. Please nominate it for Aurora as well.
tracking-firefox44: --- → +
tracking-firefox45: --- → +
Attachment #8686637 - Flags: sec-approval? → sec-approval+
(Assignee)

Comment 8

3 years ago
Comment on attachment 8686637 [details] [diff] [review]
Part 1 - Fix

Approval Request Comment
[Feature/regressing bug #]: Bug 1214562.
[User impact if declined]: Crashes, potential security bugs.
[Describe test coverage new/current, TreeHerder]: Fixes the test.
[Risks and why]: Low risk, patch is pretty straight-forward (and it's a bit of an edge case).
[String/UUID change made/needed]: None.
Attachment #8686637 - Flags: approval-mozilla-aurora?
Attachment #8686637 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/2786c62ef4c9
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox45: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
https://hg.mozilla.org/releases/mozilla-aurora/rev/3c7fef57944e
status-firefox44: affected → fixed
Group: javascript-core-security → core-security-release
Gary, can you verify fixed? Thanks.
Flags: needinfo?(gary)
Group: core-security-release

Updated

2 years ago
Status: RESOLVED → VERIFIED
status-firefox44: fixed → verified
status-firefox45: fixed → verified
status-firefox46: --- → verified

Comment 13

2 years ago
JSBugMon: This bug has been automatically verified fixed.
JSBugMon: This bug has been automatically verified fixed on Fx44
JSBugMon: This bug has been automatically verified fixed on Fx45
(Reporter)

Comment 14

2 years ago
JSBugMon has verified fixed.
Flags: needinfo?(gary)
You need to log in before you can comment on or make changes to this bug.