Bug 1446062 (CVE-2018-5146)

ZDI-CAN-5822 - Mozilla Firefox Audio Driver Out of Bounds

VERIFIED FIXED in Firefox -esr52

Status

()

defect
VERIFIED FIXED
a year ago
6 months ago

People

(Reporter: abillings, Assigned: TD-Linux)

Tracking

({csectype-bounds, sec-critical})

unspecified
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr5259+ verified, firefox59blocking verified, firefox60+ verified, firefox61+ verified)

Details

(Whiteboard: [Pwn2Own 2018])

Attachments

(6 attachments, 8 obsolete attachments)

6.39 KB, video/ogg
Details
24.27 KB, text/plain
Details
461 bytes, text/html
Details
3.01 KB, patch
jesup
: review+
dveditz
: review+
abillings
: sec-approval+
Details | Diff | Splinter Review
2.20 KB, patch
jmvalin
: review+
Details | Diff | Splinter Review
2.66 KB, patch
jmvalin
: review+
dveditz
: sec-approval+
Details | Diff | Splinter Review
(Reporter)

Description

a year ago
Posted file POC (obsolete) —
From exploit files:
Mozilla Firefox to SYSTEM exploit by Richard Zhu (fluorescence)

The exploit uses an out of bounds bug in audio parsing to achieve code execution inside Firefox.

The vulnerability occurs when parsing vorbis audio formats:

long vorbis_book_decodev_add(codebook *book,float *a,oggpack_buffer *b,int n){
  if(book->used_entries>0){
    int i,j,entry;
    float *t;

    if(book->dim>8){
      for(i=0;i<n;){
        entry = decode_packed_entry_number(book,b);
        if(entry==-1)return(-1);
        t     = book->valuelist+entry*book->dim;
        for (j=0;j<book->dim;)
          a[i++]+=t[j++]; // out of bounds access, i can be greater than n
      }

To exploit this, I do a mixed spray of Arrays and ArrayBuffers.
I load the audio file to trigger the out of bounds bug, which will overwrite the length of an Array.

I then find an ArrayBuffer allocated after the corrupted Array.
Because the array's length has been changed, I can freely read and write into the ArrayBuffer from the Array.
I write JS objects into the ArrayBuffer and read their addresses.
Then, I use this ArrayBuffer to create fake objects.
These objects can be retrieved by the corrupted Array, leading to arbitrary read/write.

Then, I hijack a vtable and ROP to get code execution.
(Reporter)

Comment 1

a year ago
Posted file POC
Attachment #8959226 - Attachment is obsolete: true
Group: core-security → media-core-security
(Reporter)

Comment 2

a year ago
Posted file test.html
Test POC file, loads contents of directory 'i_wonder_whats_in_here'
(Reporter)

Comment 3

a year ago
Posted video pwnage.ogg
Contents of 'i_wonder_whats_in_here' directory
(Reporter)

Updated

a year ago
Summary: Out of bounds while parsing audio to achieve code execution → ZDI-CAN-5822 - Mozilla Firefox Audio Driver Out of Bounds
Flags: needinfo?(tdaede)
Assignee: nobody → tdaede
(Assignee)

Comment 4

a year ago
This code exists in vorbis upstream as well. How would you like me to coordinate landing the patch in upstream / releasing?
(Reporter)

Comment 5

a year ago
FYI, this was the pwn2own 2018 bug that popped Firefox.
Group: media-core-security → core-security-release
Whiteboard: [Pwn2Own 2018]
(Assignee)

Comment 6

a year ago
Based on jesup's suggestion. Overly conservative wrt checking the final tail as well. An alternative is to bail with a return(-1) here - do you think that's safer?
Flags: needinfo?(tdaede)
Attachment #8959284 - Flags: review?(jmvalin)
(Assignee)

Comment 7

a year ago
Monty confirmed that the return(-1) method is correct.
Attachment #8959284 - Attachment is obsolete: true
Attachment #8959284 - Flags: review?(jmvalin)
Attachment #8959299 - Flags: review?(jmvalin)
(Assignee)

Comment 9

a year ago
Comment on attachment 8959299 [details] [diff] [review]
0001-Correct-Duff-s-device-in-vorbis_book_decodev_add.patch

Dropping this patch, xiphmont has a more comprehensive one that hits all the decodev functions at once (with a fix that was previously only implemented for one)
Attachment #8959299 - Flags: review?(jmvalin)
This patch corrects the reported bug, as well as the same pattern elsewhere in the decode functions.  It was apparently previously patched in one place, I've followed that fix pattern.
Correct Duff's device in vorbis_book_decodev_add and other cases
updated patch fixes a silly error
tested here against the legacy [good] sample set
TD spotted one bug in the 0003 patch; corrected
Tested good against legacy regression sample set
Attachment #8959326 - Attachment is obsolete: true
Attachment #8959299 - Attachment is obsolete: true
Attachment #8959318 - Attachment is obsolete: true
Attachment #8959339 - Flags: review?(tdaede)
(Assignee)

Comment 14

a year ago
Attachment #8959339 - Attachment is obsolete: true
Attachment #8959339 - Flags: review?(tdaede)
(In reply to Thomas Daede [:TD-Linux] from comment #14)
> Created attachment 8959349 [details] [diff] [review]
> 0001-Vorbis-Remove-unnecessary-unrolling-and-correct-code.patch

Updated patch passes legacy regression tests here.
Comment on attachment 8959349 [details] [diff] [review]
0001-Vorbis-Remove-unnecessary-unrolling-and-correct-code.patch

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

Looks good. It's a lot simpler too.
Attachment #8959349 - Flags: review+
Attachment #8959349 - Attachment is obsolete: true
Assignee: tdaede → rjesup
Status: NEW → ASSIGNED
Comment on attachment 8959357 [details] [diff] [review]
Vorbis fix

Carry forward r=jmspeex.  This applies against the mozilla tree (inbound, but should work against them all)
Attachment #8959357 - Flags: review+
Comment on attachment 8959357 [details] [diff] [review]
Vorbis fix

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

LGTM
Comment on attachment 8959357 [details] [diff] [review]
Vorbis fix

r= credit on this patch is lots of people: TD-linux, jmspeex, rjesup, monty
Attachment #8959357 - Flags: review+
Comment on attachment 8959357 [details] [diff] [review]
Vorbis fix

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Not horribly hard once you realize what happened with pwn2own

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

Which older supported branches are affected by this flaw? all

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Trivial

How likely is this patch to cause regressions; how much testing does it need?  Unlikely, but some basic ogg testing should be done
Attachment #8959357 - Flags: sec-approval?
(Reporter)

Updated

a year ago
Attachment #8959357 - Flags: sec-approval? → sec-approval+
Comment on attachment 8959357 [details] [diff] [review]
Vorbis fix

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined:
  hacked by Pwn2Own-winable bug
Fix Landed on Version:
 nightly 61
Risk to taking this patch (and alternatives if risky): 
 no good alternatives. small risk from patch to uncommon feature
String or UUID changes made by this patch: 
 none


Approval Request Comment
[Feature/Bug causing the regression]:  ancient
[User impact if declined]:             Pwn2Own exploit
[Is this code covered by automated tests?]: some, not much
[Has the fix been verified in Nightly?]: 
    by multiple people in self-builds, but not yet nightly
[Needs manual test from QE? If yes, steps to reproduce]: 
  testcase in the bug, especially the .ogg file standalone
[List of other uplifts needed for the feature/fix]:
  none
[Is the change risky?]:
  no
[Why is the change risky/not risky?]:
  small change for bounds problem, reviewed by multiple people
[String changes made/needed]:
  none
Attachment #8959357 - Flags: approval-mozilla-release?
Attachment #8959357 - Flags: approval-mozilla-esr52?
Attachment #8959357 - Flags: approval-mozilla-beta?
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment on attachment 8959357 [details] [diff] [review]
Vorbis fix

Tests are looking pretty good on treeherder - let's go ahead with uplifts.
Attachment #8959357 - Flags: approval-mozilla-release?
Attachment #8959357 - Flags: approval-mozilla-release+
Attachment #8959357 - Flags: approval-mozilla-esr52?
Attachment #8959357 - Flags: approval-mozilla-esr52+
Attachment #8959357 - Flags: approval-mozilla-beta?
Attachment #8959357 - Flags: approval-mozilla-beta+
Huzaifa Sidhpurwala points out on the security-group mailing list that the same problematic code is in libtremor.  It seems that we make the choice of whether to use libvorbis or libtremor based on this:
https://searchfox.org/mozilla-central/rev/6e96a3f1e44e286ddae5fdafab737709741d237a/old-configure.in#2397
so it looks like we use libtremor instead of libvorbis on all Android (any architecture) and all arm-architecture machines.

https://searchfox.org/mozilla-central/source/media/libtremor/lib/tremor_codebook.c#243
(Assignee)

Comment 27

a year ago
I have actually already prepared the patch for tremor but didn't realize we ship it. Patch attached.

Note that I cannot reproduce the crash with libtremor.
Attachment #8959434 - Flags: review?(jmvalin)
How hard/risky would it be to simply dump Tremor entirely? Are we actually supporting any platform that doesn't have a reasonably fast FPU?
(In reply to Jean-Marc Valin (:jmspeex) from comment #28)
> How hard/risky would it be to simply dump Tremor entirely? Are we actually
> supporting any platform that doesn't have a reasonably fast FPU?

Mike Hommey replied on security group saying it is used on Android and ARM linux.
The comment in configure.in from comment 26 says:

dnl Use integers over floats for audio on B2G and Android
dnl (regarless of the CPU architecture, because audio
dnl backends for those platforms don't support floats. We also
dnl use integers on ARM with other OS, because it's more efficient.
Comment on attachment 8959434 [details] [diff] [review]
0001-Prevent-out-of-bounds-write-in-codebook-decoding.patch

Shouldn't the changes to vorbis_book_decodev_add and vorbis_book_decodevv_add be testing i rather than testing j.  (That is, adding "i<n && " or "i<m && " rather than "j<n && " or "j<m && "?
(In reply to Paul Theriault [:pauljt] from comment #29)
> Mike Hommey replied on security group saying it is used on Android and ARM
> linux.

What I meant is that all vaguely recent ARM chips have reasonable FPUs, so maybe we can dump Tremor and use libvorbis (which is better maintained) everywhere including ARM. Any thoughts?
I was separately trying to port the patch to libtremor before the above patch was uploaded, and I got this slightly-different result.
(In reply to Jean-Marc Valin (:jmspeex) from comment #32)
> What I meant is that all vaguely recent ARM chips have reasonable FPUs, so
> maybe we can dump Tremor and use libvorbis (which is better maintained)
> everywhere including ARM. Any thoughts?

I think trying to do that for a security respin introduces substantial risk to getting that security release out promptly; it's a much safer course of action to try to get the patch ported correctly.
OK, currently trying to understand why/how the two codebases differ in vorbis_book_decodevv_add()
(Assignee)

Comment 36

a year ago
Good catch! I'm still unable to reproduce on Tremor, so the typo had no effect.
Attachment #8959434 - Attachment is obsolete: true
Attachment #8959434 - Flags: review?(jmvalin)
Alias: CVE-2018-5146
Yes, we're not going to try to reconfigure our builds in a chemspill, nor would it be appropriate to do on ESR. We need to fix it, and file a separate bug (hidden bug for now) to remove libtremor in a regular release.
(Assignee)

Comment 38

a year ago
I've now been able to verify the vulnerability on Tremor with asan, and verified that the latest version of my patch fixes it. I've also verified that dbaron's patch is identical other than applying to m-c rather than tremor git, so I believe it is ready to land.
Comment on attachment 8959439 [details] [diff] [review]
0001-Prevent-out-of-bounds-write-in-codebook-decoding.patch

r=me
I agree with the Tremor port of this patch.
Attachment #8959439 - Flags: review?(jmvalin) → review+
Comment on attachment 8959439 [details] [diff] [review]
0001-Prevent-out-of-bounds-write-in-codebook-decoding.patch

a=dveditz to land all the places
Attachment #8959439 - Flags: sec-approval+
Attachment #8959439 - Flags: approval-mozilla-release+
Attachment #8959439 - Flags: approval-mozilla-esr52+
Attachment #8959439 - Flags: approval-mozilla-beta+
Comment on attachment 8959436 [details] [diff] [review]
different attempt to port the patch to libtremor

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

r=me
That patch is identical to TD's latest one except for a single newline.
Attachment #8959436 - Flags: review+
The libtremor version needs a separate CVE (CVE-2018-5147) so I created bug 1446365 as a hook for that.
We manage to reproduce the crash on Fennec 58.0.2 and 60.0b3 (API 16 and x86) with https://bugzilla.mozilla.org/show_bug.cgi?id=1446062 - POC and based on the same POC, we can confirm that builds 59.0.1 and 60.0b4 build 2 had no crashes.
Devices: Samsung Galaxy Tab S3 (Android 7.0) and Xiaomi Mi Pad 2 (Android 5.1 x86).
We also managed to reproduce this crash on Win 10, 8.1, 7, macOS 10.13 and Ubuntu 16.04/14.04 using https://bug1446062.bmoattachments.org/attachment.cgi?id=8959231&t=GXtqUmdfIWt3HBfxYlUNaC.

Confirming that the crash is not reproducible anymore on 59.0.1 (20180315233128), 52.7.2esr (20180315163333) and 60.0b4 (20180315232954) under the same platforms mentioned above.
Assignee: rjesup → tdaede
FYI: a testcase that triggers this issue has been released publicly.
https://github.com/google/fuzzer-test-suite/tree/master/vorbis-2017-12-11
Is that actually a testcase which triggers it? It looks to me like it's a seed to start fuzzing from.
On mobile side, we tested on 61.0b3 with the following devices: Oneplus Two - Android 6.0.1, Prestigio Grace X5 - Android 4.4.2, Samsung Galaxy S8 - Android 8.0, Google Pixel(Android 8.1.0) and we can confirm that the issue is fixed.
Tested on the desktop side as well, with Beta 61.0b3 (20180507191226) using the PoC from comment 0. We can confirm that this crash is fixed on the following OSes: Windows 10 x64, macOS 10.13 and Ubuntu 16.04 x64.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.