Closed Bug 1493903 (CVE-2018-12387) Opened 3 years ago Closed 3 years ago

Infoleak bug from Hack2Win


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox-esr60 62+ verified
firefox62 + verified
firefox63 + verified
firefox64 + verified


(Reporter: dveditz, Assigned: jandem)




(Keywords: regression, sec-critical, testcase)


(5 files, 1 obsolete file)

Attached file writeups_fifo_leak.pdf
We received the 2018 Hack2Win Firefox Infoleak exploit from Noam Rathaus of Beyond Security.

Please use the following for the acknowledgement.
"Two independent security researcher, Bruno Keith and Niklas Baumstark,
have reported this vulnerability to Beyond Security’s SecuriTeam Secure
Disclosure program."
Attached file pwn.html (script)
Attached file Reduced JS Shell testcase (obsolete) —
This testcase shows the infoleak and then crashes when run with --ion-eager. Without that flag, it will work as expected and print out the correct values.

The loop being jitted is the inner loop after the `gen(...)` call.
Note that despite this being reported as an information leak, it can be leveraged to RCE. Doing so is just very build dependent and more work compared to the first issue (bug 1493900).
This one is likely caused by inlining of push with multiple arguments. That has some complicated code for restoring the call stack and I think it's buggy when used with, like |, x, ...)|. The safest and fastest thing is to just disable this optimization for now.
Assignee: nobody → jdemooij
Attached patch PatchSplinter Review
Attachment #9011734 - Flags: review?(tcampbell)
Attachment #9011734 - Flags: feedback?(nth10sd)
Attachment #9011734 - Flags: feedback?(choller)
Oh the feedback requests here are mostly to double check the patch fixes all known test cases. This patch is very safe and doesn't need fuzzing.
autobisectjs shows this is probably related to the following changeset:

The first bad revision is:
user:        Nicolas B. Pierron
date:        Thu Sep 07 13:01:13 2017 +0000
summary:     Bug 966743 - Inline Array.prototype.push with more than one argument. r=jandem

You're right.
Comment on attachment 9011734 [details] [diff] [review]

I verify that this patch fixes the MOZ_CRASH as shown by the shell testcase. I tested with "--ion-offthread-compile=off --ion-eager".
Attachment #9011734 - Flags: feedback?(nth10sd) → feedback+
Run this test with --ion-eager --ion-offthread-compile=off on a debug build to reproduce the assertion.
Attachment #9011727 - Attachment is obsolete: true
Attachment #9011710 - Attachment mime type: text/plain → text/html
Comment on attachment 9011734 [details] [diff] [review]

r=me to disable the problematic path entirely. I'm looking into the root cause so we understand it more too.
Attachment #9011734 - Flags: review?(tcampbell) → review+
Still looking, but it seems like the issue is that when IonBuilder::inlineArrayPush ends up using IonBuilder::pushPriorCallStack, we don't also restore the |this| of We actually identified the problem in Bug 1412653 but only fixed the Function.prototype.apply case.

(Only 80% sure.. still digging).
This patch is the targeted fix to the flaw. I still strongly favor Jan's patch
to just disable this mode. We should work on giving more checks for fuzzing to
get better coverage here.
Comment on attachment 9011734 [details] [diff] [review]

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

Not trivial but targeted fuzzing could help.

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

> Which older supported branches are affected by this flaw?

> If not all supported branches, which bug introduced the flaw?
Bug 966743.

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

> How likely is this patch to cause regressions; how much testing does it need?
Worst case it will regress perf (benchmarks) but I don't expect anything serious.
Attachment #9011734 - Flags: sec-approval?
Attachment #9011734 - Flags: approval-mozilla-release?
Attachment #9011734 - Flags: approval-mozilla-esr60?
Attachment #9011734 - Flags: approval-mozilla-beta?
(In reply to Jan de Mooij [:jandem] from comment #14)
> > How easily could an exploit be constructed based on the patch?
> Not trivial but targeted fuzzing could help.

To be more clear: this *is* the kind of bug that people could figure out when doing some serious testing in this area. Landing later or a chemspill might make sense because of that.
Alias: CVE-2018-12387
If all goes as planned, ETA to land this is 10/1 am Paris time in order to gtb 10/1 afternoon Paris time.
Attachment #9011734 - Flags: feedback?(choller)
Comment on attachment 9011734 [details] [diff] [review]

sec-approval+ and a=dveditz
Attachment #9011734 - Flags: sec-approval?
Attachment #9011734 - Flags: sec-approval+
Attachment #9011734 - Flags: approval-mozilla-release?
Attachment #9011734 - Flags: approval-mozilla-release+
Attachment #9011734 - Flags: approval-mozilla-esr60?
Attachment #9011734 - Flags: approval-mozilla-esr60+
Attachment #9011734 - Flags: approval-mozilla-beta?
Attachment #9011734 - Flags: approval-mozilla-beta+
Group: javascript-core-security → core-security-release
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Flags: qe-verify?
Unfortunately, we don't have the bandwidth required to build Firefox with the required parameters today. If someone could do that and give us the required build, we'll do our best to accomodate this verification. 

An alternative would also be if someone else to confirm the fix for this issue.
NI myself to file a follow-up bug.
Flags: needinfo?(jdemooij)
So, I was able to reproduce this error on revision aa7b44bb25aa (,debug&revision=aa7b44bb25aa)
> Assertion failure: isObject(), at z:\build\build\src\obj-firefox\dist\include\js/Value.h:792

Confirming that the assestion failure is no longer reproducing on:
- Fx 63.0b11 (rev f76a129ff2f2)
- Fx 64.0a1 (rev 7cda6e1eb528)
- ESR 60.2.2 (rev 20d20599f4e8)
- Fx 62.0.3 (rev c9ed11ae5c79)
SecuriTeam has published full details at -- we can unhide this now.
Group: core-security-release
Comment on attachment 9011906 [details] [diff] [review]
Fix bailout tracking with DO_NOT_LAND

This patch should address root cause and is probably worth landing for our future sanity. I'd suggest we leave Jan's patch in place for now. I'll add a targeted testcase too.
Attachment #9011906 - Flags: review?(nicolas.b.pierron)
See Also: → 1500514
Please move any follow-up work to a new bug for proper tracking.
Comment on attachment 9011906 [details] [diff] [review]
Fix bailout tracking with DO_NOT_LAND

Review of attachment 9011906 [details] [diff] [review]:

This patch looks good to me, and should fix the issue. r=me on a follow-up bug.
Attachment #9011906 - Flags: review?(nicolas.b.pierron) → feedback+
(In reply to Jan de Mooij [:jandem] from comment #24)
> NI myself to file a follow-up bug.

Clearing. Ted is adding a test and proper fix.
Flags: needinfo?(jdemooij)
See Also: → 1502090
You need to log in before you can comment on or make changes to this bug.