1147655 - Crash [@ js::Invoke] or [@ js::InvokeGetterOrSetter] or Assertion failure: shape->hasDefaultSetter(), at vm/NativeObject.cpp

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

(function() {
    Object.defineProperty(arguments, 0, {
        get: function() {}
    })
    Array.prototype.shift.call(arguments)
})(0, 0)

asserts js debug shell on m-c changeset 5330c6f461a4 with --fuzzing-safe --no-threads --ion-eager at Assertion failure: shape->hasDefaultSetter(), at vm/NativeObject.cpp and crashes js opt shell at js::Invoke with js::InvokeGetterOrSetter on the stack.

Debug configure options:

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 --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

python -u ~/fuzzing/js/compileShell.py -b "--enable-debug --enable-more-deterministic --enable-nspr-build" -r 5330c6f461a4

Opt configure options:

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 --disable-debug --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

python -u ~/fuzzing/js/compileShell.py -b "--disable-debug --enable-more-deterministic --enable-nspr-build" -r 5330c6f461a4

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/a25cc961aaf0
user:        Jason Orendorff
date:        Mon Feb 16 10:48:07 2015 -0600
summary:     Bug 1139683 - Rewrite SetExistingProperty with comments and references to the standard. r=efaust.

Setting s-s, not sure if 0x4155415641574155 is indeed being accessed here.

Jason, is bug 1139683 a likely regressor?

Comment 1

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

Attachment: debug stack

(lldb) bt 5
* thread #1: tid = 0xcf0e0, 0x000000010029a1e8 js-dbg-64-dm-nsprBuild-darwin-5330c6f461a4`js::NativeSetProperty(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<JSObject*>, JS::Handle<jsid>, js::QualifiedBool, JS::MutableHandle<JS::Value>, JS::ObjectOpResult&) [inlined] SetExistingProperty(cx=<unavailable>, root=0x00000001028a51a0, root=0x00000001028a5198, dummy=<unavailable>, dummy=<unavailable>) + 150 at NativeObject.cpp:2222, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x000000010029a1e8 js-dbg-64-dm-nsprBuild-darwin-5330c6f461a4`js::NativeSetProperty(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<JSObject*>, JS::Handle<jsid>, js::QualifiedBool, JS::MutableHandle<JS::Value>, JS::ObjectOpResult&) [inlined] SetExistingProperty(cx=<unavailable>, root=0x00000001028a51a0, root=0x00000001028a5198, dummy=<unavailable>, dummy=<unavailable>) + 150 at NativeObject.cpp:2222
    frame #1: 0x000000010029a152 js-dbg-64-dm-nsprBuild-darwin-5330c6f461a4`js::NativeSetProperty(cx=<unavailable>, qualified=<unavailable>, result=<unavailable>, obj=<unavailable>, receiver=<unavailable>, id=<unavailable>, vp=<unavailable>) + 2050 at NativeObject.cpp:2259
    frame #2: 0x000000010006e9b9 js-dbg-64-dm-nsprBuild-darwin-5330c6f461a4`SetArrayElement(JSContext*, JS::Handle<JSObject*>, double, JS::Handle<JS::Value>) [inlined] js::SetProperty(cx=0x00000001028a5180, result=0xffffffffffffffff) + 569 at NativeObject.h:1451
    frame #3: 0x000000010006e98d js-dbg-64-dm-nsprBuild-darwin-5330c6f461a4`SetArrayElement(JSContext*, JS::Handle<JSObject*>, double, JS::Handle<JS::Value>) [inlined] js::SetProperty(cx=0x00000001028a5180, root=0x00000001028a51e0, root=0x00000001028a51e8, dummy=<unavailable>) + 60 at jsobj.h:888
    frame #4: 0x000000010006e951 js-dbg-64-dm-nsprBuild-darwin-5330c6f461a4`SetArrayElement(cx=0x00000001028a5180, index=<unavailable>, obj=<unavailable>, v=<unavailable>) + 465 at jsarray.cpp:368
(lldb)

Comment 2

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

Attachment: stack of opt crash

(lldb) bt 5
* thread #1: tid = 0xcf209, 0x00000001001182b0 js-64-dm-nsprBuild-darwin-5330c6f461a4`js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) [inlined] js::ObjectGroup::clasp(this=0x4155415641574155) const at ObjectGroup.h:199, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=EXC_I386_GPFLT)
  * frame #0: 0x00000001001182b0 js-64-dm-nsprBuild-darwin-5330c6f461a4`js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) [inlined] js::ObjectGroup::clasp(this=0x4155415641574155) const at ObjectGroup.h:199
    frame #1: 0x00000001001182b0 js-64-dm-nsprBuild-darwin-5330c6f461a4`js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) [inlined] JSObject::getClass(this=0x0000000100517e30) const + 3 at jsobj.h:128
    frame #2: 0x00000001001182ad js-64-dm-nsprBuild-darwin-5330c6f461a4`js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) [inlined] JS::Value::toObject(this=0x0000000100517e30) const + 3 at jsobj.h:520
    frame #3: 0x00000001001182aa js-64-dm-nsprBuild-darwin-5330c6f461a4`js::Invoke(cx=0x00000001021ab0f0, thisv=<unavailable>, fval=<unavailable>, argc=<unavailable>, argv=<unavailable>, rval=<unavailable>) + 346 at Interpreter.cpp:545
    frame #4: 0x000000010014043f js-64-dm-nsprBuild-darwin-5330c6f461a4`js::InvokeGetterOrSetter(cx=<unavailable>, obj=<unavailable>, fval=Value at 0x00007fff5fbfcb98, argc=<unavailable>, argv=<unavailable>, rval=<unavailable>) + 111 at Interpreter.cpp:628
(lldb)

Comment 3

[Jason Orendorff [:jorendorff]]

Very likely, yes. Taking.

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: When redefining a mapped arguments element, as an accessor property, it should stop being mapped

Comment 5

[Jason Orendorff [:jorendorff]]

Tapping efaust for this review since he reviewed the original patch... although, if you're like me, it started fading from your memory as soon as you stamped it...

The assertion that's failing is new in that changeset. It asserts that we don't have accessor properties with JSSetterOps. I guess it was a pretty bold assertion.

But I really want to establish that invariant, so I'm patching this so that Object.defineProperty can't accidentally create such a property and we'll see if it holds after that.

This entire region of code (StandardDefineProperty and its helpers) is going to be deleted in bug 1125624, so the significant thing here is landing some tests for this weird case.

Comment 6

[Eric Faust [:efaust]]

Comment on attachment 8584144 [details] [diff] [review]
When redefining a mapped arguments element, as an accessor property, it should stop being mapped

Review of attachment 8584144 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsobj.cpp
@@ +548,1 @@
>          }

is it worth MOZ_ASSERT(getter || setter)? It's trivially true, but maybe a useful sanity check?

::: js/src/tests/ecma_6/Object/mapped-arguments-redefinition-2.js
@@ +1,4 @@
> +function f(x, y) {
> +    Object.defineProperty(arguments, 0, {get: () => 7});
> +    arguments[0] = 133;  // silent failure in non-strict mode: arguments[0] has a getter and no setter.
> +    assertEq(y, "thirteen");

what, if anything, happens to |x|? Should we also check that?

Comment 7

[Andrew McCreight [:mccr8]]

Should this be, say, sec-high?  I can't really tell how bad this is.

Comment 8

[Jason Orendorff [:jorendorff]]

The crash here happens because we are treating a pointer to a C++ function as a pointer to a JS object and trying to read from the object.

If trying to read from a page that has program code on it is always going to crash hard on the platforms we care about, this is sec-moderate at worst. (I think?)

Comment 9

[Jason Orendorff [:jorendorff]]

Comment on attachment 8584144 [details] [diff] [review]
When redefining a mapped arguments element, as an accessor property, it should stop being mapped

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

    No known exploit; might be hard or impossible to exploit.

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

    The test case makes it pretty clear how to trigger this crash,
    but that's all.

Which older supported branches are affected by this flaw?

    The patch landed March 21, so it's in FF39. Aurora.

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

    rev a25cc961aaf0
    bug 1139683

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

    Super easy to create.

How likely is this patch to cause regressions; how much testing does it need?

    This patch is about as likely as an average patch its size to cause
    regressions. Which is to say, maybe 5%.

    No special testing required.

Comment 10

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

We need to figure out a security rating here before sec-approval+ can be given. If it isn't a sec-critical or sec-high, it doesn't even need sec-approval. Comment 8 makes this sound like a sec-moderate, which I assume means the crash isn't exploitable or cannot be easily triggered.

Comment 11

[Andrew McCreight [:mccr8]]

(In reply to Jason Orendorff [:jorendorff] from comment #8)
> The crash here happens because we are treating a pointer to a C++ function
> as a pointer to a JS object and trying to read from the object.

I agree that this doesn't sound too exploitable in practice, but some clever person could find some C++ code that looks like something useful, so I'm just going to mark it sec-high to be safe.

Comment 12

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

Comment on attachment 8584144 [details] [diff] [review]
When redefining a mapped arguments element, as an accessor property, it should stop being mapped

sec-approval+ for trunk. We should get this into all affected branches too.

Comment 13

[Christian Holler (:decoder)]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision fec90cbfbaad).

Comment 14

[Jason Orendorff [:jorendorff]]

Christian, can we find out what fixed this?

I wonder if it was bug 1150906. If so, that's a better fix.

I need to check whether FF39 was affected.

Comment 15

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

autoBisect shows this is probably related to the following changeset:

The first good revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/95357a580708
user:        Jason Orendorff
date:        Tue Apr 07 15:44:09 2015 -0500
summary:     Bug 1150906 - Fix "Assertion failure: !has(SHADOWABLE)" and subsequent GC crashes introduced in rev 034027f41aaf. r=Waldo.

Yes, bug 1150906 likely fixed this.

Leaving needinfo? for Jason to check the status on 39.

Comment 16

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 17

[Jason Orendorff [:jorendorff]]

Tested it using mozilla-aurora rev 428734cf3a67. No crash. This makes sense, as rev 034027f41aaf (the regressing changeset fingered in bug 1150906) is not in mozilla-aurora.