1314401 - Crash [@ intrinsic_UnsafeGetInt32FromReservedSlot(JSContext*, unsigned int, JS::Value*)] or Assertion failure: this->is<T>(), at js/src/jsobj.h:567

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, critical)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 2c773b971672 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-debug --enable-optimize, run with --fuzzing-safe --ion-offthread-compile=off --ion-eager):

for (var ret of[null, {}, [], /a/]) {
    assertEq(RegExp.prototype[Symbol.match].call({
        exec(S) {
            return ret;
        }
    }, "foo"), ret);
}


Backtrace:

 received signal SIGSEGV, Segmentation fault.
0x000000000095c6c6 in intrinsic_UnsafeGetInt32FromReservedSlot(JSContext*, unsigned int, JS::Value*) () at js/src/vm/SelfHosting.cpp:139
#0  0x000000000095c6c6 in intrinsic_UnsafeGetInt32FromReservedSlot(JSContext*, unsigned int, JS::Value*) () at js/src/vm/SelfHosting.cpp:139
#1  0x0000000000922570 in js::CallJSNative (args=..., native=<optimized out>, cx=0x7ffff695f000) at js/src/jscntxtinlines.h:239
#2  js::InternalCallOrConstruct (cx=cx@entry=0x7ffff695f000, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:458
#3  0x00000000009226b5 in InternalCall (cx=cx@entry=0x7ffff695f000, args=...) at js/src/vm/Interpreter.cpp:503
#4  0x00000000009226e5 in js::CallFromStack (cx=cx@entry=0x7ffff695f000, args=...) at js/src/vm/Interpreter.cpp:509
#5  0x000000000056237b in js::jit::DoCallFallback (cx=0x7ffff695f000, frame=0x7fffffffcc08, stub_=0x7ffff69d5540, argc=2, vp=0x7fffffffcb70, res=...) at js/src/jit/BaselineIC.cpp:6012
#6  0x00007ffff7e44daf in ?? ()
[...]
#38 0x0000000000000000 in ?? ()
rax	0xfff8800000000002	-2111062325329918
rbx	0x7ffff695f000	140737330409472
rcx	0x7ffff0700560	140737227261280
rdx	0x7fffffffcb70	140737488341872
rsi	0x0	0
rdi	0x7ffff695f000	140737330409472
rbp	0x7fffffffc810	140737488341008
rsp	0x7fffffffc7e8	140737488340968
r8	0x7fffffffffff	140737488355327
r9	0xfffdffffffffffff	-562949953421313
r10	0x7ffff03d2400	140737223926784
r11	0x7ffffffff000	140737488351232
r12	0x7fffffffc8f0	140737488341232
r13	0x95c6b0	9815728
r14	0x7ffff69d5540	140737330894144
r15	0x7ffff695f000	140737330409472
rip	0x95c6c6 <intrinsic_UnsafeGetInt32FromReservedSlot(JSContext*, unsigned int, JS::Value*)+22>
=> 0x95c6c6 <_ZL40intrinsic_UnsafeGetInt32FromReservedSlotP9JSContextjPN2JS5ValueE+22>:	mov    0x10(%rsi),%esi
   0x95c6c9 <_ZL40intrinsic_UnsafeGetInt32FromReservedSlotP9JSContextjPN2JS5ValueE+25>:	shr    $0x1b,%esi


Not sure if this is s-s or not, but having a function with the name "unsafe" on the stack in combination with a crash is probably a reason to be cautious.

Comment 1

[Fuzzing Team]

JSBugMon: Bisection requested, result:
=== Treeherder Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20161025030604" and the hash "00b6c424d4409ec920f4d9c206680dcad897d99e".
The "bad" changeset has the timestamp "20161025031004" and the hash "1077b4771709b0df601d86e52f486d5e4345fb3f".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=00b6c424d4409ec920f4d9c206680dcad897d99e&tochange=1077b4771709b0df601d86e52f486d5e4345fb3f

Comment 2

[Gary Kwong [:gkw] [:nth10sd]]

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/1077b4771709
user:        Tooru Fujisawa
date:        Tue Oct 25 19:09:19 2016 +0900
summary:     Bug 1263340 - Part 9: Add optimizable path for RegExpMatch with global flag. r=till

Arai-san, is bug 1263340 a likely regressor?

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Eagerly check IsRegExpObject in IsRegExpMethodOptimizable.

I forgot to check IsRegExpObject in IsRegExpMethodOptimizable...

RegExpInstanceOptimizable should be called only with RegExpObject,
otherwise code in RegExpObject::isInitialShape doesn't make sense.
the underlying issue is from bug 887016 (firefox 49, backed out from firefox 48 by bug 1265307), and affects all branches except esr45.

here's the changes:
  * Added IsRegExpObject to IsRegExpMethodOptimizable
    now all RegExpInstanceOptimizable caller checks IsRegExpObject before calling it
  * Removed isNative() check from js::RegExpInstanceOptimizableRaw
    since it should be checked in IsRegExpObject
  * Changed type used in js::RegExpInstanceOptimizableRaw from NativeObject* to RegExpObject*

test will be in another patch, that should be landed separately, after the fix is landed to all affected branches.

Comment 4

[Till Schneidereit [:till]]

Comment on attachment 8806545 [details] [diff] [review]
Part 1: Eagerly check IsRegExpObject in IsRegExpMethodOptimizable.

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

Makes sense.

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: (do not land this now) Part 2: Add testcase for RegExp.prototype[@@match] with non-RegExp object.

thanks :)

here's the testcase, that is the same as comment #0.

Comment 6

[Tooru Fujisawa [:arai]]

Comment on attachment 8806545 [details] [diff] [review]
Part 1: Eagerly check IsRegExpObject in IsRegExpMethodOptimizable.

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
It could easily hit the crash.
Not sure how it's easy to do some more, but potentially it could.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Yes, it says type check is insufficient.

This patch doesn't contain test, since the test contains more info.
will land the test after the fix is landed to all affected branches.

> Which older supported branches are affected by this flaw?
firefox49

> If not all supported branches, which bug introduced the flaw?
underlying issue was from bug 887016,
(that was landed to firefox48, but backed out from firefox48 aurora)
and exposed to more by bug 1263340 etc.

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
not yet, but should be easy.
will prepare shortly and attach them, and ask approval.

> How likely is this patch to cause regressions; how much testing does it need?
Less likely.  Adds some more checks before applying optimization.
Affected cases falls back to slow path.

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: (mozilla-aurora/mozilla-beta/mozilla-release) Part 1: Eagerly check IsRegExpObject in IsRegExpMethodOptimizable. r=till

this patch is applicable to mozilla-aurora/mozilla-beta/mozilla-release
no actual changes from original one, just surrounding comment change.

Approval Request Comment
> [Feature/regressing bug #]
underlying issue was from bug 887016,
(that was landed to firefox48, but backed out from firefox48 aurora)
and exposed to more by bug 1263340 etc.

> [User impact if declined]
crash, and possible more critical exploit, by dereferencing wrong memory block as pointer

> [Describe test coverage new/current, TreeHerder]
not tested in automated tests.
passed part 2 testcase locally.

> [Risks and why]
Less likely.  Adds some more checks before applying optimization.
Affected cases falls back to slow path.

> [String/UUID change made/needed]
None

Comment 8

[Marcia Knous [:marcia - needinfo? me]]

Tracking 52+ for this sec bug - the user impact is described in Comment 7.

Comment 9

[Gerry Chang [:gchang]]

Track 51+ as crash and possible exploit.

Comment 10

[Daniel Veditz [:dveditz] OOO until Sept 23]

Comment on attachment 8806545 [details] [diff] [review]
Part 1: Eagerly check IsRegExpObject in IsRegExpMethodOptimizable.

sec-approval=dveditz

I'm not sure we can get this into 50 at this point, but if not it should go into 50.1

Comment 11

[Daniel Veditz [:dveditz] OOO until Sept 23]

Comment on attachment 8806690 [details] [diff] [review]
(mozilla-aurora/mozilla-beta/mozilla-release) Part 1: Eagerly check IsRegExpObject in IsRegExpMethodOptimizable. r=till

a=dveditz for aurora. Leaving beta/release approvals for release managers. The 50.1 release is OK if it doesn't make 50.

Comment 12

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/05773111312ff06c776ab82602204e84a13dbb2c
Bug 1314401 - Part 1: Eagerly check IsRegExpObject in IsRegExpMethodOptimizable. r=till sec-approval=dveditz

Comment 13

[Ritu Kothari (:ritu)]

I agree with Dan's suggestion. We are only uplifting release blocking issues this late in RC 50 week. Let's uplift this in time for 50.1.0 release.

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/05773111312f

Comment 15

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bb22a0218997

Comment 17

[Gerry Chang [:gchang]]

Comment on attachment 8806690 [details] [diff] [review]
(mozilla-aurora/mozilla-beta/mozilla-release) Part 1: Eagerly check IsRegExpObject in IsRegExpMethodOptimizable. r=till

Since merge day is passed, beta is 51 and release is 50. The patch has already been in 51 so I remove the approval‑mozilla‑beta? flag and keep approval‑mozilla‑release? flag.

Comment 18

[Ritu Kothari (:ritu)]

Comment on attachment 8806690 [details] [diff] [review]
(mozilla-aurora/mozilla-beta/mozilla-release) Part 1: Eagerly check IsRegExpObject in IsRegExpMethodOptimizable. r=till

Sec-high, let's uplift to 50.1.0

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/780969e63d4d

Comment 20

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed on Fx51