1254721 - (CVE-2016-2814) Crash [@ stagefright::SampleTable::parseSampleCencInfo] with heap buffer overflow in libstagefright.

Status: VERIFIED FIXED

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

Description

[Sascha Just]

There is a bug in libstagefright which causes Firefox to crash (invalid read).
I attached a test case to this report. As this seems to be exploitable, I marked this report as security related.

I downloaded Firefox 48.0a1 from: https://ftp.mozilla.org/pub/firefox/tinderbox-builds/mozilla-central-linux64-asan/1457435058/firefox-48.0a1.en-US.linux-x86_64-asan.tar.bz2
The corresponding HG revision is: 05c087337043dd8e71cc27bdb5b9d55fd00aaa26

While inspecting SampleTable.cpp and investigating the issue, I stumbled upon this code fragment (SampleTable:639-640):

    if (!mCencSizes.IsEmpty() && mCencOffsets.Length() > 1 &&
        mCencSizes.IsEmpty() != mCencOffsets.Length()) {
        return ERROR_MALFORMED;
    }


which is semantically equivalent to:

    if (!mCencSizes.IsEmpty() && mCencOffsets.Length() > 1) {
        return ERROR_MALFORMED;
    }


I'm not sure what the condition is supposed to be, but it's probably supposed to be an '||' and something around comparing lengths.
The actual place where the crash appears is here (SampleTable:655-657):

   for (uint32_t i = 0; i < mCencInfoCount; i++) {
        uint8_t size = mCencDefaultSize ? mCencDefaultSize : mCencSizes[i];      /// <--
        uint64_t offset = mCencOffsets.Length() == 1 ? nextOffset : mCencOffsets[i];

The reason is the access to mCencSizes[i] with i=0 while mCencOffsets is empty. 
At this time, mCencOffsets.Length is 2 and mCencSizes.Length is 0. This goes probably back to the initial (wrong or incomplete?) malform check.


During the execution I got the ASAN trace attached to this report, as well as a crash with SIGILL. 

When executing a debug build, I also get:

	Assertion failure: aIndex < Length() (invalid array index)

Comment 1

[Sascha Just]

Attachment: Detailed Crash Information

Comment 2

[Sascha Just]

Attachment: Testcase

Comment 3

[Christian Holler (:decoder)]

Congratulations on filing your first fuzz bug Sascha \o/

Ccing some people that might know this code a little better according to hg log.

Comment 4

[Christian Holler (:decoder)]

Fwiw, this reproduces for me on an ASan build, although it appears to me (from the trace) that the overflow ASan reports occurs in this line:

> 657   uint64_t offset = mCencOffsets.Length() == 1 ? nextOffset : mCencOffsets[i];

Maybe optimizations are kicking in here compared to where the assert in the debug build catches it.

I should also note that the strange if statement mentioned in comment 0 is neither detected by Coverity nor warned by the compiler. This seems like a serious shortcoming to me because comparing to method's results where one is "bool" and the other is integral type is almost always a bug and the developer should use explicit !! if he wanted to compare integral type with bool.

Comment 5

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Assigned to jya. Ping me if you still need my input.

As a quick exercise following Christian's comment, I removed all the warning suppressions from libstagefright's moz.build. It revealed quite a few warnings, though nothing worrying at a quick glance; and nothing related to the issues described in comment 0 and comment 4, which is indeed sad.

Comment 6

[Jean-Yves Avenard [:jya]]

(In reply to Sascha Just from comment #0)
> There is a bug in libstagefright which causes Firefox to crash (invalid
> read).
> I attached a test case to this report. As this seems to be exploitable, I
> marked this report as security related.
> 
> I downloaded Firefox 48.0a1 from:
> https://ftp.mozilla.org/pub/firefox/tinderbox-builds/mozilla-central-linux64-
> asan/1457435058/firefox-48.0a1.en-US.linux-x86_64-asan.tar.bz2
> The corresponding HG revision is: 05c087337043dd8e71cc27bdb5b9d55fd00aaa26
> 
> While inspecting SampleTable.cpp and investigating the issue, I stumbled
> upon this code fragment (SampleTable:639-640):
> 
>     if (!mCencSizes.IsEmpty() && mCencOffsets.Length() > 1 &&
>         mCencSizes.IsEmpty() != mCencOffsets.Length()) {
>         return ERROR_MALFORMED;
>     }

this is a mistake I made in resolution of bug 1185115
the code used to be:
    if (!mCencSizes.isEmpty() && mCencOffsets.size() > 1 &&
        mCencSizes.size() != mCencOffsets.size()) {
        return ERROR_MALFORMED;
    }


> 
> 
> which is semantically equivalent to:
> 
>     if (!mCencSizes.IsEmpty() && mCencOffsets.Length() > 1) {
>         return ERROR_MALFORMED;
>     }
> 
> 
> I'm not sure what the condition is supposed to be, but it's probably
> supposed to be an '||' and something around comparing lengths.
> The actual place where the crash appears is here (SampleTable:655-657):
> 
>    for (uint32_t i = 0; i < mCencInfoCount; i++) {
>         uint8_t size = mCencDefaultSize ? mCencDefaultSize : mCencSizes[i]; 
> /// <--
>         uint64_t offset = mCencOffsets.Length() == 1 ? nextOffset :
> mCencOffsets[i];
> 
> The reason is the access to mCencSizes[i] with i=0 while mCencOffsets is
> empty. 
> At this time, mCencOffsets.Length is 2 and mCencSizes.Length is 0. This goes
> probably back to the initial (wrong or incomplete?) malform check.
> 
> 
> During the execution I got the ASAN trace attached to this report, as well
> as a crash with SIGILL. 
> 
> When executing a debug build, I also get:
> 
> 	Assertion failure: aIndex < Length() (invalid array index)

Comment 7

[Jean-Yves Avenard [:jya]]

Attachment: Ensure consistency between Cenc offsets and sizes table. r?gerald

MozReview-Commit-ID: E1KbKIIBR87

Ensure that there are no out of bound access in the following code.
The tests are made to be consistent on how the arrays are accessed.

Comment 8

[Jean-Yves Avenard [:jya]]

Comment on attachment 8730549 [details] [diff] [review]
Ensure consistency between Cenc offsets and sizes table. r?gerald

[Security approval request comment]
How easily could an exploit be constructed based on the patch? As usual hard to say. This would have performed a read-only out of bound

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? since libstagefright was included in our code, so a while.

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? backport would be easy to craft, though I don't anticipate that any will be required post bug 1185115. 

How likely is this patch to cause regressions; how much testing does it need? little, we have lots of mochitest ensure that normal case still works.

Comment 9

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

Making this a sec-high and marking for tracking.

sec-approval+ for trunk.

We'll want this on branches as well. Please nominate patches.

Comment 10

[Jean-Yves Avenard [:jya]]

Attachment: ESR38: Ensure consistency between Cenc offsets and sizes table.

Rebase for ESR38

Comment 11

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

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

Comment 12

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

jya can you rebase and request uplift? Thanks!

Comment 13

[Jean-Yves Avenard [:jya]]

Comment on attachment 8731056 [details] [diff] [review]
ESR38: Ensure consistency between Cenc offsets and sizes table.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: out of bound memory access
Fix Landed on Version: 48
Risk to taking this patch (and alternatives if risky): low, ESR38 doesn't support encryption anyway.
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 14

[Jean-Yves Avenard [:jya]]

Comment on attachment 8730549 [details] [diff] [review]
Ensure consistency between Cenc offsets and sizes table. r?gerald

Approval Request Comment
[Feature/regressing bug #]: 1254721
[User impact if declined]: out of bound memory access, crash.
[Describe test coverage new/current, TreeHerder]: in central, mochitest to ensure there are no regressions
[Risks and why]: low, erroring on invalid/corrupted file
[String/UUID change made/needed]: none

Comment 15

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

Comment on attachment 8730549 [details] [diff] [review]
Ensure consistency between Cenc offsets and sizes table. r?gerald

Crash fix, should make it into beta 4.

Comment 16

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

Do we also want to request approval for mozilla-release and/or esr45?

Comment 17

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

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

Comment 18

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

https://hg.mozilla.org/releases/mozilla-beta/rev/4c7b6e5d1b61

Comment 19

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8731056 [details] [diff] [review]
ESR38: Ensure consistency between Cenc offsets and sizes table.

Sec high, taking it
Should be in 45.1.0 & 38.8.0

I guess we want it in 45 too.

Comment 20

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

https://hg.mozilla.org/releases/mozilla-esr45/rev/43e0d0dbe7ee

Comment 21

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

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

Comment 22

[Jean-Yves Avenard [:jya]]

sorry, had missed the ni?. Question has been answered already.

Comment 23

[Mihai Boldan, Desktop QA [:mboldan]]

I managed to reproduce this issue on Firefox 48.0a1 (2016-03-08) asan, using the attached test case.
The crash is no longer reproducible on Firefox 46beta11, Firefox 47.0a2 (2016-04-18), Firefox 48.0a1 (2016-04-18), Firefox ESR 38.8.0, Firefox ESR 45.1.0 and on Ubuntu 14.04 x64.
I am marking this issue Verified Fixed.