Bug 1254721 (CVE-2016-2814)

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

VERIFIED FIXED in Firefox 46

Status

()

Core
Audio/Video: Playback
VERIFIED FIXED
2 years ago
9 months ago

People

(Reporter: Sascha Just, Assigned: jya)

Tracking

({csectype-bounds, regression, sec-high})

Trunk
mozilla48
x86_64
Linux
csectype-bounds, regression, sec-high
Points:
---
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox45 wontfix, firefox46+ verified, firefox47+ verified, firefox48+ verified, firefox-esr3846+ verified, firefox-esr4546+ verified)

Details

(Whiteboard: [adv-main46+][adv-esr45.1+][adv-esr38.8+], crash signature)

Attachments

(4 attachments)

(Reporter)

Description

2 years ago
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)
(Reporter)

Comment 1

2 years ago
Created attachment 8728118 [details]
Detailed Crash Information
(Reporter)

Comment 2

2 years ago
Created attachment 8728119 [details]
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)

Updated

2 years ago
Assignee: nobody → jyavenard
Flags: needinfo?(jyavenard)

Updated

2 years ago
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)
(Assignee)

Comment 6

2 years ago
(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)
(Assignee)

Comment 7

2 years ago
Created attachment 8730549 [details] [diff] [review]
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.
Attachment #8730549 - Flags: review?(gsquelart)
Attachment #8730549 - Flags: review?(gsquelart) → review+
(Assignee)

Comment 8

2 years ago
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.
status-firefox45: --- → wontfix
status-firefox46: --- → affected
status-firefox47: --- → affected
status-firefox48: --- → affected
status-firefox-esr38: --- → affected
status-firefox-esr45: --- → affected
tracking-firefox46: --- → +
tracking-firefox47: --- → +
tracking-firefox48: --- → +
tracking-firefox-esr38: --- → ?
tracking-firefox-esr45: --- → ?
Keywords: sec-high
Attachment #8730549 - Flags: sec-approval? → sec-approval+
(Assignee)

Comment 10

2 years ago
Created attachment 8731056 [details] [diff] [review]
ESR38: Ensure consistency between Cenc offsets and sizes table.

Rebase for ESR38
(Assignee)

Updated

2 years ago
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
Last Resolved: 2 years ago
status-firefox48: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
jya can you rebase and request uplift? Thanks!
Flags: needinfo?(jyavenard)
(Assignee)

Comment 13

2 years ago
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?
(Assignee)

Comment 14

2 years ago
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)
https://hg.mozilla.org/releases/mozilla-aurora/rev/c6af39f64775
status-firefox47: affected → fixed
https://hg.mozilla.org/releases/mozilla-beta/rev/4c7b6e5d1b61
status-firefox46: affected → fixed
Blocks: 1185115
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+
tracking-firefox-esr38: ? → 46+
tracking-firefox-esr45: ? → 46+
https://hg.mozilla.org/releases/mozilla-esr45/rev/43e0d0dbe7ee
status-firefox-esr45: affected → fixed
https://hg.mozilla.org/releases/mozilla-esr38/rev/a13c0bc84d6e
status-firefox-esr38: affected → fixed
(Assignee)

Comment 22

2 years ago
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.
Status: RESOLVED → VERIFIED
status-firefox46: fixed → verified
status-firefox47: fixed → verified
status-firefox48: fixed → verified
status-firefox-esr38: fixed → verified
status-firefox-esr45: fixed → verified
Group: core-security-release
Keywords: csectype-bounds
You need to log in before you can comment on or make changes to this bug.