1493903 - (CVE-2018-12387) Infoleak bug from Hack2Win

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Daniel Veditz [:dveditz]]

Attachment: 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."

Comment 1

[Daniel Veditz [:dveditz]]

Attachment: pwn.html (script)

Comment 2

[Christian Holler (:decoder)]

Attachment: Reduced JS Shell testcase

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.

Comment 3

[Christian Holler (:decoder)]

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).

Comment 4

[Jan de Mooij [:jandem]]

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 fun.call, like |push.call(arr, x, ...)|. The safest and fastest thing is to just disable this optimization for now.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Patch

Comment 6

[Jan de Mooij [:jandem]]

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.

Comment 7

[Jan de Mooij [:jandem]]

I think this one goes back to bug 966743.

Comment 8

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

autobisectjs shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/8b1881ead0b6
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 9

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

Comment on attachment 9011734 [details] [diff] [review]
Patch

I verify that this patch fixes the MOZ_CRASH as shown by the shell testcase. I tested with "--ion-offthread-compile=off --ion-eager".

Comment 10

[Christian Holler (:decoder)]

Attachment: Reduced JS Shell testcase based on testcase in writeup

Run this test with --ion-eager --ion-offthread-compile=off on a debug build to reproduce the assertion.

Comment 11

[Ted Campbell [:tcampbell]]

Comment on attachment 9011734 [details] [diff] [review]
Patch

r=me to disable the problematic path entirely. I'm looking into the root cause so we understand it more too.

Comment 12

[Ted Campbell [:tcampbell]]

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 Function.prototype.call. We actually identified the problem in Bug 1412653 but only fixed the Function.prototype.apply case.

(Only 80% sure.. still digging).

Comment 13

[Ted Campbell [:tcampbell]]

Attachment: Fix bailout tracking with fun.call. DO_NOT_LAND

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 14

[Jan de Mooij [:jandem]]

Comment on attachment 9011734 [details] [diff] [review]
Patch

[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?
No.

> Which older supported branches are affected by this flaw?
All.

> 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.

Comment 15

[Jan de Mooij [:jandem]]

(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.

Comment 16

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

If all goes as planned, ETA to land this is 10/1 am Paris time in order to gtb 10/1 afternoon Paris time.

Comment 17

[Daniel Veditz [:dveditz]]

Comment on attachment 9011734 [details] [diff] [review]
Patch

sec-approval+ and a=dveditz

Comment 18

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/65e444ed5c8cb00d4940f955ed7b7f8403434806

Comment 19

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-release/rev/dc688e0c970278799f4d0ca9bde78545380e4110

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7c790360c4a3

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/b8f5c37486e1

Comment 22

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/65e444ed5c8c

Comment 23

[Cornel Ionce [:noni] [Hubs QA]]

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.

Comment 24

[Jan de Mooij [:jandem]]

NI myself to file a follow-up bug.

Comment 25

[Cornel Ionce [:noni] [Hubs QA]]

So, I was able to reproduce this error on revision aa7b44bb25aa (https://treeherder.mozilla.org/#/jobs?repo=mozilla-release&searchStr=build,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)

Comment 26

[Daniel Veditz [:dveditz]]

SecuriTeam has published full details at https://blogs.securiteam.com/index.php/archives/3766 -- we can unhide this now.

Comment 27

[Ted Campbell [:tcampbell]]

Comment on attachment 9011906 [details] [diff] [review]
Fix bailout tracking with fun.call. 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.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Please move any follow-up work to a new bug for proper tracking.

Comment 29

[Nicolas B. Pierron [:nbp]]

Comment on attachment 9011906 [details] [diff] [review]
Fix bailout tracking with fun.call. 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.

Comment 30

[Jan de Mooij [:jandem]]

(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.