1200148 - (CVE-2015-4511) Heap-buffer-overflow due to overflow in nestegg_track_codec_data

Status: VERIFIED FIXED

(Core :: Audio/Video: Playback, defect)

Description

[Atte Kettunen]

Attachment: repro-file.webm

Tested on: 

OS: Ubuntu 14.04

Firefox: ASAN debug-build from https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan-debug/latest/

ASAN-trace:

=================================================================
==28309==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6070000d1b9b at pc 0x46fac7 bp 0x7f81856f75f0 sp 0x7f81856f6db0
READ of size 110 at 0x6070000d1b9b thread T26 (MediaPl~back #1)
    #0 0x46fac6 in memcpy /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_interceptors.cc:366
    #1 0x7f81acac2b87 in void AssignRangeAlgorithm<true, true>::implementation<unsigned char, unsigned char, unsigned long, unsigned long>(unsigned char*, unsigned long, unsigned long, unsigned char const*
) /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/obj-firefox/media/libstagefright/../../dist/include/nsTArray.h:559
    #2 0x7f81aca86265 in unsigned char* nsTArray_Impl<unsigned char, nsTArrayInfallibleAllocator>::AppendElements<unsigned char, nsTArrayInfallibleAllocator>(unsigned char const*, unsigned long) /builds/sl
ave/m-cen-l64-asan-d-0000000000000/build/src/obj-firefox/media/libstagefright/../../dist/include/nsTArray.h:1510
    #3 0x7f81b07467d0 in mozilla::XiphHeadersToExtradata(mozilla::MediaByteBuffer*, nsTArray<unsigned char const*> const&, nsTArray<unsigned long> const&) /builds/slave/m-cen-l64-asan-d-0000000000000/build
/src/dom/media/XiphExtradata.cpp:26
    #4 0x7f81b098652b in mozilla::WebMDemuxer::ReadMetadata() /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/dom/media/webm/WebMDemuxer.cpp:412
    #5 0x7f81b09857c6 in mozilla::WebMDemuxer::Init() /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/dom/media/webm/WebMDemuxer.cpp:161
    #6 0x7f81b062c2e2 in mozilla::MediaFormatReader::AsyncReadMetadata() /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/dom/media/MediaFormatReader.cpp:293
    #7 0x7f81b06a8815 in nsRefPtr<mozilla::MozPromise<nsRefPtr<mozilla::MetadataHolder>, mozilla::ReadMetadataFailureReason, true> > mozilla::detail::MethodCallInvokeHelper<nsRefPtr<mozilla::MozPromise<nsR
efPtr<mozilla::MetadataHolder>, mozilla::ReadMetadataFailureReason, true> >, mozilla::MediaDecoderReader>(nsRefPtr<mozilla::MozPromise<nsRefPtr<mozilla::MetadataHolder>, mozilla::ReadMetadataFailureReason,
 true> > (mozilla::MediaDecoderReader::*)(), mozilla::MediaDecoderReader*, mozilla::Tuple<>&, mozilla::IndexSequence<>) /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/obj-firefox/dom/media/../../di
st/include/mozilla/MozPromise.h:869
    #8 0x7f81b06a86ce in mozilla::detail::MethodCall<mozilla::MozPromise<nsRefPtr<mozilla::MetadataHolder>, mozilla::ReadMetadataFailureReason, true>, mozilla::MediaDecoderReader>::Invoke() /builds/slave/m
-cen-l64-asan-d-0000000000000/build/src/obj-firefox/dom/media/../../dist/include/mozilla/MozPromise.h:894
    #9 0x7f81b06a849c in mozilla::detail::ProxyRunnable<mozilla::MozPromise<nsRefPtr<mozilla::MetadataHolder>, mozilla::ReadMetadataFailureReason, true>, mozilla::MediaDecoderReader>::Run() /builds/slave/m
-cen-l64-asan-d-0000000000000/build/src/obj-firefox/dom/media/../../dist/include/mozilla/MozPromise.h:912
    #10 0x7f81acc68727 in mozilla::AutoTaskDispatcher::TaskGroupRunnable::Run() /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/obj-firefox/xpcom/threads/../../dist/include/mozilla/TaskDispatcher.h:
180
.
.
.
0x6070000d1b9b is located 0 bytes to the right of 75-byte region [0x6070000d1b50,0x6070000d1b9b)
allocated by thread T26 (MediaPl~back #1) here:
    #0 0x48290b in realloc /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:95
    #1 0x7f81accafb02 in mozilla::CountingAllocatorBase<NesteggReporter>::CountingRealloc(void*, unsigned long) /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/obj-firefox/xpcom/build/../../dist/include/mozilla/CountingAllocatorBase.h:79
    #2 0x7f81b2b11113 in halloc /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/media/libnestegg/src/halloc.c:78
    #3 0x7f81b2b197c4 in ne_pool_alloc /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/media/libnestegg/src/nestegg.c:534
    #4 0x7f81b2b1970d in ne_read_binary /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/media/libnestegg/src/nestegg.c:753
    #5 0x7f81b2b191da in ne_read_simple /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/media/libnestegg/src/nestegg.c:1017
    #6 0x7f81b2b128cf in ne_parse /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/media/libnestegg/src/nestegg.c:1088
    #7 0x7f81b2b11c23 in nestegg_init /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/media/libnestegg/src/nestegg.c:1948
    #8 0x7f81b0985bc0 in mozilla::WebMDemuxer::ReadMetadata() /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/dom/media/webm/WebMDemuxer.cpp:272
    #9 0x7f81b09857c6 in mozilla::WebMDemuxer::Init() /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/dom/media/webm/WebMDemuxer.cpp:161
    #10 0x7f81b062c2e2 in mozilla::MediaFormatReader::AsyncReadMetadata() /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/dom/media/MediaFormatReader.cpp:293
.
.
.

Comment 1

[Daniel Veditz [:dveditz]]

Is this a stagefright bug or a webm bug?

Comment 2

[Timothy B. Terriberry (:derf)]

WebM(In reply to Daniel Veditz [:dveditz] from comment #1)
> Is this a stagefright bug or a webm bug?

WebM.

Comment 3

[Timothy B. Terriberry (:derf)]

I believe this is an overflow in nestegg_track_codec_data(). It does not check that the sizes of the headers it parses out of the extra data are actually valid.

The amount of data in the codec_private field in the test file is 43 bytes. It claims there are two headers, and the first claims to be 110 bytes. It computes the size of the second as 18446744073709551547 bytes.

Then we try to pack them to send to the decoder and we crash.

Comment 4

[Matthew Gregan [:kinetik]]

Discussed this with Tim offline.  I'll adopt the parsing code in XiphExtradataToHeaders for nestegg_track_codec_data.

Comment 5

[Matthew Gregan [:kinetik]]

Attachment: bug1200148_v0.patch

Comment 6

[Timothy B. Terriberry (:derf)]

Comment on attachment 8657681 [details] [diff] [review]
bug1200148_v0.patch

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

r=me with a few comments (address or not as you see fit).

::: src/nestegg.c
@@ +2286,5 @@
> +        size += *p;
> +        avail -= 1;
> +        if (*p++ != 255)
> +          break;
> +      }

I know this is copied from my code, but looking at it I don't know why I didn't just make this a do-while loop.

@@ +2295,5 @@
>      }
> +    sizes[i] = avail - total;
> +
> +    if (item >= count)
> +      return -1;

Any reason not to this check before the loop above?

Comment 7

[Matthew Gregan [:kinetik]]

(In reply to Timothy B. Terriberry (:derf) from comment #6)
> I know this is copied from my code, but looking at it I don't know why I
> didn't just make this a do-while loop.

Seems nicer, I've changed the nestegg version.

> @@ +2295,5 @@
> Any reason not to this check before the loop above?

No.  I moved it up and merged it with the count > 3 check.

Comment 8

[Matthew Gregan [:kinetik]]

Attachment: bug1200148_v1.patch

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

Medium.

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

Pretty much, and the patch itself is small enough that it's pretty obvious where to explore to produce an exploit.

Which older supported branches are affected by this flaw?

All, although the severity is less/different on older branches due to the simpler way this API was used.

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

N/A

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

Patch should apply as-is, or be trivial to backport at worst.

How likely is this patch to cause regressions; how much testing does it need?

Patch is pretty safe, the same approach has been used elsewhere (in XiphExtradataToHeaders) for a couple of weeks, existing regression test suite passes locally.

Comment 9

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

Sec-approval+ for trunk. We should get patches made and nominated for Aurora, Beta, and ESR38 once it is on trunk.

Comment 10

[Jean-Yves Avenard [:jya]]

XiphHeadersToExtradata is only in 43 and was introduced in bug 1196353

Comment 11

[Matthew Gregan [:kinetik]]

(In reply to Jean-Yves Avenard [:jya] from comment #10)
> XiphHeadersToExtradata is only in 43 and was introduced in bug 1196353

Read comment 3.

Comment 12

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

Matthew, please nominate beta patch for uplift? We gtb Beta9 tomorrow so it would be nice if we have the patch ready to land asap.

Comment 13

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

also nom it for ESR please :) Thanks.

Comment 14

[Matthew Gregan [:kinetik]]

Comment on attachment 8657965 [details] [diff] [review]
bug1200148_v1.patch

The patch applies to ESR38 as-is, except for the update to README_MOZILLA, but that can simply be ignored as it's only updated the upstream git commit ID which doesn't make sense when backporting a patch.

Approval Request Comment
[Feature/regressing bug #]: bug 566246 (when libnestegg as added), but may not have become exploitable until recently due to change in API usage pattern
[User impact if declined]: possible memory corruption, crash, and/or remotely exploitable bug
[Describe test coverage new/current, TreeHerder]: n/a (regression test added to upstream repo)
[Risks and why]: 
[String/UUID change made/needed]: none

Comment 15

[Matthew Gregan [:kinetik]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f682bc4d5e2c

Comment 16

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

Comment on attachment 8657965 [details] [diff] [review]
bug1200148_v1.patch

Taking it as it's a sec-high. Beta41+, Aurora42+

Comment 17

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

KWierso told me that this bug hasn't landed on inbound so this will miss Beta9.

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Landed to beta in https://hg.mozilla.org/releases/mozilla-beta/rev/360c43a9bb74

Still need to land to inbound and merge that to m-c and also uplift to aurora, but those are on less of a time crunch.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/78050272add4

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/78050272add4

Comment 21

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

Comment on attachment 8657965 [details] [diff] [review]
bug1200148_v1.patch

Approved for uplift to esr38. Stability improvement, has tests.

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b1c8608314d6

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8657965 [details] [diff] [review]
bug1200148_v1.patch

sec-high/crit don't need approval to land on affected B2G branches once the fix has landed on affected Fx branches.

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-esr38/rev/b826b0f74cbc

Comment 25

[Bogdan Maris, Desktop Test Engineering]

Reproduced the initial crash and similar asan-trace using old Asan debug build from 2015-08-31, verified that the crash does not occur using latest Nightly and Aurora asan debug build from 2015-09-21 (Ubuntu 14.04 64-bit), Firefox ESR 38.3.0 build 2 and Firefox 41.0 build 3 across platforms (Windows 10 32-bit, Ubuntu 14.04 64-bit and Mac OS X 10.10.5).