1446062 - (CVE-2018-5146) ZDI-CAN-5822 - Mozilla Firefox Audio Driver Out of Bounds

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

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

Attachment: POC

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.

Comment 1

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

Attachment: POC

Comment 2

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

Attachment: test.html

Test POC file, loads contents of directory 'i_wonder_whats_in_here'

Comment 3

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

Attachment: pwnage.ogg

Contents of 'i_wonder_whats_in_here' directory

Comment 4

[Thomas Daede [:TD-Linux]]

This code exists in vorbis upstream as well. How would you like me to coordinate landing the patch in upstream / releasing?

Comment 5

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

FYI, this was the pwn2own 2018 bug that popped Firefox.

Comment 6

[Thomas Daede [:TD-Linux]]

Attachment: 0001-Correct-Duff-s-device-in-vorbis_book_decodev_add.patch

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?

Comment 7

[Thomas Daede [:TD-Linux]]

Attachment: 0001-Correct-Duff-s-device-in-vorbis_book_decodev_add.patch

Monty confirmed that the return(-1) method is correct.

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

Attachment: asan hit (inbound asan build)

Comment 9

[Thomas Daede [:TD-Linux]]

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)

Comment 10

[Monty Montgomery (:xiphmont)]

Attachment: Correct Duff's device in vorbis_book_decodev_add and other cases

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.

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

Attachment: test.html that should repro in an ASAN build

Comment 12

[Monty Montgomery (:xiphmont)]

Attachment: Corrected patch, tested good against legacy regression sample set

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

Comment 13

[Monty Montgomery (:xiphmont)]

Attachment: Add vector bounds guard in vorbis_book_decodev_add and other cases

TD spotted one bug in the 0003 patch; corrected
Tested good against legacy regression sample set

Comment 14

[Thomas Daede [:TD-Linux]]

Attachment: 0001-Vorbis-Remove-unnecessary-unrolling-and-correct-code.patch

Comment 15

[Monty Montgomery (:xiphmont)]

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

[Jean-Marc Valin]

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.

Comment 17

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Vorbis fix

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

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)

Comment 19

[Nils Ohlmeier [:drno]]

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

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

LGTM

Comment 20

[Daniel Veditz [:dveditz]]

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

r= credit on this patch is lots of people: TD-linux, jmspeex, rjesup, monty

Comment 21

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 22

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/mozilla-central/rev/6b4e98424f0dcc024fcad48608ada351e4075e59

Comment 23

[Daniel Veditz [:dveditz]]

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

Comment 24

[Liz Henry (:lizzard) (relman/hg->git project)]

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

Tests are looking pretty good on treeherder - let's go ahead with uplifts.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1dfbedb54c39abae38da9329f4a79571fee74661
https://hg.mozilla.org/releases/mozilla-release/rev/3db9e3d52b17563efca181ccbb50deb8660c59ae
https://hg.mozilla.org/releases/mozilla-esr52/rev/494e5d5278ba6f5fdda9a2bb9ac7ca772653ee4a

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 27

[Thomas Daede [:TD-Linux]]

Attachment: 0001-Prevent-out-of-bounds-write-in-codebook-decoding.patch

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.

Comment 28

[Jean-Marc Valin]

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?

Comment 29

[Paul Theriault [:pauljt] (no longer reading bugmail)]

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

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 && "?

Comment 32

[Jean-Marc Valin]

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

Comment 33

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: different attempt to port the patch to libtremor

I was separately trying to port the patch to libtremor before the above patch was uploaded, and I got this slightly-different result.

Comment 34

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 35

[Jean-Marc Valin]

OK, currently trying to understand why/how the two codebases differ in vorbis_book_decodevv_add()

Comment 36

[Thomas Daede [:TD-Linux]]

Attachment: 0001-Prevent-out-of-bounds-write-in-codebook-decoding.patch

Good catch! I'm still unable to reproduce on Tremor, so the typo had no effect.

Comment 37

[Daniel Veditz [:dveditz]]

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.

Comment 38

[Thomas Daede [:TD-Linux]]

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 39

[Jean-Marc Valin]

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.

Comment 40

[Daniel Veditz [:dveditz]]

Comment on attachment 8959439 [details] [diff] [review]
0001-Prevent-out-of-bounds-write-in-codebook-decoding.patch

a=dveditz to land all the places

Comment 41

[Jean-Marc Valin]

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.

Comment 42

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 43

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/releases/mozilla-beta/rev/0ae512558ada8254d73fc982c581ffe481c11b20

Comment 44

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 45

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/releases/mozilla-esr52/rev/5cd5586a2f48424a9031a3fa4c782954a9df9a52

Comment 46

[Daniel Veditz [:dveditz]]

The libtremor version needs a separate CVE (CVE-2018-5147) so I created bug 1446365 as a hook for that.

Comment 47

[Sorina Florean [:sflorean]]

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

Comment 48

[Ciprian Georgiu, Desktop QA]

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.

Comment 49

[Tyson Smith [:tsmith]]

FYI: a testcase that triggers this issue has been released publicly.
https://github.com/google/fuzzer-test-suite/tree/master/vorbis-2017-12-11

Comment 50

[Alex Gaynor [:Alex_Gaynor]]

Is that actually a testcase which triggers it? It looks to me like it's a seed to start fuzzing from.

Comment 51

[Sorina Florean [:sflorean]]

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.

Comment 52

[Ciprian Georgiu, Desktop QA]

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.