1296649 - Crash [@ JSScript::code] or Crash [@ js::jit::ICEntry::pc] with use-after-free

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision fe895421dfbe (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-debug --without-intl-api --enable-optimize --target=i686-pc-linux-gnu, run with --fuzzing-safe --thread-count=2 --disable-oom-functions --baseline-eager --ion-offthread-compile=off --ion-pgo=on --ion-eager):

See attachment.


Backtrace:

 received signal SIGSEGV, Segmentation fault.
JSScript::code (this=0xf2d68890) at js/src/jsscript.h:1345
#0  JSScript::code (this=0xf2d68890) at js/src/jsscript.h:1345
#1  JSScript::offsetToPC (offset=<optimized out>, this=0xf2d68890) at js/src/jsscript.h:1371
#2  js::jit::ICEntry::pc (script=0xf2d68890, this=<optimized out>) at js/src/jit/SharedIC.h:302
#3  js::jit::DoTypeMonitorFallback (cx=0xf7949000, payload=0x0, stub=0xf4232070, value=..., res=...) at js/src/jit/SharedIC.cpp:3987
#4  0xf7be51fb in ?? ()
Backtrace stopped: previous frame inner to this frame (corrupt stack?)
eax	0x49494949	1229539657
ebx	0xf4232070	-199024528
ecx	0xfffd023c	-196036
edx	0xf2d68890	-220821360
esi	0xf2d68890	-220821360
edi	0xf7949000	-141258752
ebp	0xfffd02f4	4294771444
esp	0xfffd0220	4294771232
eip	0x82d7433 <js::jit::DoTypeMonitorFallback(JSContext*, void*, js::jit::ICTypeMonitor_Fallback*, JS::HandleValue, JS::MutableHandleValue)+115>
=> 0x82d7433 <js::jit::DoTypeMonitorFallback(JSContext*, void*, js::jit::ICTypeMonitor_Fallback*, JS::HandleValue, JS::MutableHandleValue)+115>:	mov    0x8(%eax),%edx
   0x82d7436 <js::jit::DoTypeMonitorFallback(JSContext*, void*, js::jit::ICTypeMonitor_Fallback*, JS::HandleValue, JS::MutableHandleValue)+118>:	lea    0x10(%eax,%edx,4),%eax


Marking s-s and sec-critical due to use-after-free pattern.

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Fuzzing Team]

JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.

Comment 3

[Fuzzing Team]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/dbcad237aa05
user:        Sambuddha Basu
date:        Mon Aug 08 17:30:37 2016 +0800
summary:     Bug 1252976 - Offer |mach artifact| builds in |mach bootstrap| for non-Fennec builds r=nalexander

This iteration took 191.575 seconds to run.

Comment 4

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

Testcase probably turned intermittent during the process of bisection, so please disregard comment 3.

Comment 5

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

Naveed, can you get someone to look at this?

Comment 6

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

A few weeks later, I will ni? Naveed... :-)

Comment 7

[Naveed Ihsanullah [:naveed]]

Jan please take a look or assign.

Comment 8

[Jan de Mooij [:jandem]]

This has to do with shared stubs. We call DoGetPropGeneric from Ion, this ends up triggering a CGC that moves the script. Then we call the type monitor fallback stub, but we crash when we use its IonICEntry::script_ as it wasn't updated.

Hannes do you know if we have something to handle this?

Comment 9

[Hannes Verschore [:h4writer]]

(In reply to Jan de Mooij [:jandem] from comment #8)
> Hannes do you know if we have something to handle this?

This is from before JSScripts were movable. I'll look into a solution Thursday or Friday.

Comment 10

[Hannes Verschore [:h4writer]]

Attachment: Patch (nightly + aurora)

The actual change is in SharedIC.cpp/h. I added a trace method to IonICEntry which is called to fix the JSScripts. This solves the issue.
At the same time I created BaselineICEntry. That way one cannot interchangeable use IonICEntry/ICEntry (since ICEntry does less in the trace method). I could have used a virtual functions, but that increases the size of ICEntry. Therefore I created BaselineICEntry/IonICEntry distinction. Both have a trace function, but the ICEntry hasn't. That way we can be sure the correct trace function is called.

Comment 11

[Jan de Mooij [:jandem]]

Comment on attachment 8792007 [details] [diff] [review]
Patch (nightly + aurora)

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

Great!

Comment 12

[Hannes Verschore [:h4writer]]

Comment on attachment 8792007 [details] [diff] [review]
Patch (nightly + aurora)

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
No idea. It would definitely be intermittent and take a lot of time to get all the timings right. And even harder to be able to do something with it.

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?
FF48+

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
I didn't create them yet. The actual change is minimal. And the rest of the patch are mechanical changes.

How likely is this patch to cause regressions; how much testing does it need?
I think it is a quite easy/well understood/safe patch.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

[Tracking Requested - why for this release]:

Comment 14

[Marcia Knous [:marcia]]

Tracking 52+ for this sec-critical crash.

Comment 15

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

Comment on attachment 8792007 [details] [diff] [review]
Patch (nightly + aurora)

sec-approval+ for trunk. We should nominate patches for Beta and Aurora as well.

Comment 16

[Hannes Verschore [:h4writer]]

Attachment: Beta patch

Comment 17

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f7baf71da7ef

Comment 18

[Hannes Verschore [:h4writer]]

Comment on attachment 8792007 [details] [diff] [review]
Patch (nightly + aurora)

Approval Request Comment
[Feature/regressing bug #]:
Bug 1259180

[User impact if declined]:
Potentially exploitable

[Describe test coverage new/current, TreeHerder]:
Landed a few hours ago.

[Risks and why]: 
Quite low. It patch seems massive, but is mostly s/ICEntry/BaselineICEntry/. The logic change is quite small.

[String/UUID change made/needed]:
/

Comment 19

[Hannes Verschore [:h4writer]]

Comment on attachment 8798500 [details] [diff] [review]
Beta patch

Approval Request Comment
[Feature/regressing bug #]:
Bug 1259180

[User impact if declined]:
Potentially exploitable

[Describe test coverage new/current, TreeHerder]:
Landed a few hours ago.

[Risks and why]: 
Quite low. It patch seems massive, but is mostly s/ICEntry/BaselineICEntry/. The logic change is quite small.

[String/UUID change made/needed]:
/

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f7baf71da7ef

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8792007 [details] [diff] [review]
Patch (nightly + aurora)

Sec-crit, Aurora51+

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8798500 [details] [diff] [review]
Beta patch

Sec-crit, Beta50+

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1eb8829009a2
https://hg.mozilla.org/releases/mozilla-beta/rev/f4385d3a94d4

Comment 24

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.
JSBugMon: This bug has been automatically verified fixed on Fx50

Comment 25

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed on Fx51