Closed Bug 1493903 (CVE-2018-12387) Opened 3 years ago Closed 3 years ago
Infoleak bug from Hack2Win
179.19 KB, application/pdf
2.05 KB, text/html
1.43 KB, patch
|Details | Diff | Splinter Review|
1.85 KB, patch
|Details | Diff | Splinter Review|
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."
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 fun.call, like |push.call(arr, x, ...)|. The safest and fastest thing is to just disable this optimization for now.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
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.
I think this one goes back to bug 966743.
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 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".
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] Patch 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 Function.prototype.call. 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] 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.
(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.
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 on attachment 9011734 [details] [diff] [review] Patch 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+
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.
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)
SecuriTeam has published full details at https://blogs.securiteam.com/index.php/archives/3766 -- we can unhide this now.
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.
Attachment #9011906 - Flags: review?(nicolas.b.pierron)
Please move any follow-up work to a new bug for proper tracking.
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.
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.
You need to log in before you can comment on or make changes to this bug.