Closed Bug 1254721 (CVE-2016-2814) Opened 4 years ago Closed 4 years ago

Crash [@ stagefright::SampleTable::parseSampleCencInfo] with heap buffer overflow in libstagefright.

Categories

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

x86_64
Linux
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla48
Tracking Status
firefox45 --- wontfix
firefox46 + verified
firefox47 + verified
firefox48 + verified
firefox-esr38 46+ verified
firefox-esr45 46+ verified

People

(Reporter: methos-bugzilla, Assigned: jya)

References

Details

(Keywords: csectype-bounds, regression, sec-high, Whiteboard: [adv-main46+][adv-esr45.1+][adv-esr38.8+])

Crash Data

Attachments

(4 files)

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)
Attached file Testcase
Congratulations on filing your first fuzz bug Sascha \o/

Ccing some people that might know this code a little better according to hg log.
Flags: needinfo?(jyavenard)
Flags: needinfo?(gsquelart)
Assignee: nobody → jyavenard
Flags: needinfo?(jyavenard)
Group: core-security → media-core-security
Flags: sec-bounty?
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
Flags: needinfo?(gsquelart)
(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)
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.
Attachment #8730549 - Flags: review?(gsquelart)
Attachment #8730549 - Flags: review?(gsquelart) → review+
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.
Attachment #8730549 - Flags: sec-approval?
Making this a sec-high and marking for tracking.

sec-approval+ for trunk.

We'll want this on branches as well. Please nominate patches.
Attachment #8730549 - Flags: sec-approval? → sec-approval+
Attachment #8731056 - Attachment description: Ensure consistency between Cenc offsets and sizes table. → ESR38: Ensure consistency between Cenc offsets and sizes table.
https://hg.mozilla.org/mozilla-central/rev/f437498da477
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
jya can you rebase and request uplift? Thanks!
Flags: needinfo?(jyavenard)
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.
Flags: needinfo?(jyavenard)
Attachment #8731056 - Flags: approval-mozilla-esr38?
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
Attachment #8730549 - Flags: approval-mozilla-beta?
Attachment #8730549 - Flags: approval-mozilla-aurora?
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.
Attachment #8730549 - Flags: approval-mozilla-beta?
Attachment #8730549 - Flags: approval-mozilla-beta+
Attachment #8730549 - Flags: approval-mozilla-aurora?
Attachment #8730549 - Flags: approval-mozilla-aurora+
Do we also want to request approval for mozilla-release and/or esr45?
Flags: needinfo?(jyavenard)
Flags: sec-bounty? → sec-bounty+
Keywords: regression
Group: media-core-security → core-security-release
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.
Attachment #8731056 - Flags: approval-mozilla-esr45+
Attachment #8731056 - Flags: approval-mozilla-esr38?
Attachment #8731056 - Flags: approval-mozilla-esr38+
sorry, had missed the ni?. Question has been answered already.
Flags: needinfo?(jyavenard)
Whiteboard: [adv-main46+][adv-esr45.1+][adv-esr38.8+]
Alias: CVE-2016-2814
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.
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.