Bug 1216748 (CVE-2015-7222)

stagefright: potential underflow in 'covr', unchecked allocation&copy in Metadata::setData

RESOLVED FIXED in Firefox 43, Firefox OS v2.5

Status

()

P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: gerald, Assigned: gerald)

Tracking

({csectype-bounds, sec-high})

44 Branch
mozilla45
csectype-bounds, sec-high
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 wontfix, firefox43+ fixed, firefox44+ fixed, firefox45+ fixed, firefox-esr3843+ fixed, b2g-v2.5 fixed)

Details

(Whiteboard: [adv-main43+][adv-esr38.5+] AndroidID-20923261, published in August 2015; uplift 1206211 first on beta&esr-38)

Attachments

(5 attachments, 2 obsolete attachments)

(Assignee)

Description

3 years ago
AndroidID-20923261, patched in May 2015, published in August 2015:
https://android.googlesource.com/platform/frameworks/av/+/c87faed60483afb2466e03892bda80b72e5822c7%5E!/#F0
"Fix integer underflow in covr MPEG4 processing
When the 'chunk_data_size' variable is less than 'kSkipBytesOfDataBox', an
integer underflow can occur. This causes an extraordinarily large value to
be passed to MetaData::setData, leading to a buffer overflow."

As well as fixing the underflow there, Metadata::setData and dependent methods could do with more checks before/after allocating memory and copying data.

As the malloc in Metadata could be given an enormous value, and the return pointer is not checked, and data is blindly copied there, it should be considered a potential security risk (though probably just a crash risk, but let's keep it hidden for now).
(Assignee)

Comment 1

3 years ago
Created attachment 8677841 [details] [diff] [review]
1216748-p1-gtest.patch

Part 1: Added test case with different 'covr' sizes.
Attachment #8677841 - Flags: review?(giles)
(Assignee)

Comment 2

3 years ago
Created attachment 8677842 [details] [diff] [review]
1216748-p2-check-metadata-allocation.patch

Part 2: Handle failed malloc in Metadata storage.
Attachment #8677842 - Flags: review?(giles)
(Assignee)

Comment 3

3 years ago
Created attachment 8677843 [details] [diff] [review]
1216748-p3-reject-small-covr-data.patch

Part 3: Ensure 'covr' data size cannot create underflow.
Attachment #8677843 - Flags: review?(giles)
(Assignee)

Comment 4

3 years ago
Created attachment 8677844 [details] [diff] [review]
1216748-p4-check-other-setData-uses.patch

Part 4: Check other Metadata::setData uses.

Found only one other use that needed better checks: the size of the pssh data was only checked after all items were added up; so it would be possible to create a set of big items such that they create an overflow, but the final sum looks reasonable.
Instead each item size should be checked, and the sum should also be checked at each step.
Attachment #8677844 - Flags: review?(giles)
Group: core-security → media-core-security
Attachment #8677841 - Flags: review?(giles) → review+
Comment on attachment 8677842 [details] [diff] [review]
1216748-p2-check-metadata-allocation.patch

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

::: media/libstagefright/frameworks/av/media/libstagefright/MetaData.cpp
@@ +276,1 @@
>      mSize = size;

Would feel safer to only update mSize of success, but I suppose the ordering doesn't matter much.
Attachment #8677842 - Flags: review?(giles) → review+
Comment on attachment 8677843 [details] [diff] [review]
1216748-p3-reject-small-covr-data.patch

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

Is this verified by the new test file as well?
Attachment #8677843 - Flags: review?(giles) → review+
Attachment #8677844 - Flags: review?(giles) → review+
(Assignee)

Comment 7

3 years ago
(In reply to Ralph Giles (:rillian) from comment #5)
> Comment on attachment 8677842 [details] [diff] [review]
> 1216748-p2-check-metadata-allocation.patch
> 
> Review of attachment 8677842 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/libstagefright/frameworks/av/media/libstagefright/MetaData.cpp
> @@ +276,1 @@
> >      mSize = size;
> 
> Would feel safer to only update mSize of success, but I suppose the ordering
> doesn't matter much.

This ordering is actually necessary because the next statement |if (usesReservoir()) { return true; }| depends on the new value of mSize.
And you can see I reset mSize to 0 below if things go wrong.
I'll add a comment to explain this.


(In reply to Ralph Giles (:rillian) from comment #6)
> Comment on attachment 8677843 [details] [diff] [review]
> 1216748-p3-reject-small-covr-data.patch
> 
> Review of attachment 8677843 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is this verified by the new test file as well?

p3 is covered by the test file, yes -- It's actually the first fix I coded to make the test pass; p2 came after, as a deeper-level fix, and also makes the test pass without p3.

However p4 is not tested. It would be tricky to create a test file, I'll give it a try.
Flags: needinfo?(gsquelart)
Keywords: csectype-bounds, sec-high
(Assignee)

Comment 8

3 years ago
Created attachment 8679801 [details] [diff] [review]
1216748-p2-check-metadata-allocation.patch

Added documentation to address concerns from comment 5.
No actual code change; carrying r+ from comment 5.
Attachment #8677842 - Attachment is obsolete: true
Attachment #8679801 - Flags: review+
(Assignee)

Comment 9

3 years ago
Comment on attachment 8677843 [details] [diff] [review]
1216748-p3-reject-small-covr-data.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Fairly easy, it shows that the old code didn't check if the 'covr' data size was smaller than 16, in which case an underflow would result in trying to allocate almost all the memory, fail at that, and start copying data to 0x0, most likely resulting in a crash.
I don't believe it could be exploited more than that, but a security expert should verify that.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Yes: The first patch provides a POC file that triggers the issue.
The 2nd patch shows that the malloc() was not checked for failures.
The 3rd and 4th patches shows where this bad malloc could be triggered.

Which older supported branches are affected by this flaw?
As old as libstagefright, so it's in ESR38 at least.

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?
If patches don't just apply, it would be trivial to backport them as required.

How likely is this patch to cause regressions; how much testing does it need?
Very unlikely: All patches just add simple checks before carrying fallible operations, and report tested failure codes.
There is a new gtest for this failure, and lots of existing media tests.

(Do I need to request sec-approval for all patches?)
Attachment #8677843 - Flags: sec-approval?
(Assignee)

Comment 10

3 years ago
Also, please note that this issue has been visibly patched on Android's repository for almost 3 months (although there is no associated bug visible at this time).
Flags: needinfo?(gsquelart)
sec-approval+ for checkin on November 10, after we are into the new cycle a bit.
Whiteboard: [checkin on 11/10]
Attachment #8677843 - Flags: sec-approval? → sec-approval+
(Assignee)

Updated

3 years ago
Blocks: 1216845
(Assignee)

Comment 12

3 years ago
Created attachment 8685834 [details] [diff] [review]
1216748-p1-gtest.patch v2

Just a simple rebase, carrying r+.
Attachment #8677841 - Attachment is obsolete: true
Attachment #8685834 - Flags: review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a0eedfc2faec
https://hg.mozilla.org/mozilla-central/rev/0962522f86ab
https://hg.mozilla.org/mozilla-central/rev/71113b0fe61f
https://hg.mozilla.org/mozilla-central/rev/330c1763806e
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Group: media-core-security → core-security-release
This one should have landed on all affected branches.
status-firefox42: --- → wontfix
status-firefox43: --- → affected
status-firefox-esr38: --- → affected
tracking-firefox43: --- → +
tracking-firefox44: --- → +
tracking-firefox45: --- → +
tracking-firefox-esr38: --- → 43+
Whiteboard: AndroidID-20923261, published in August 2015
Flags: needinfo?(gsquelart)
(Assignee)

Updated

3 years ago
Depends on: 1206211
(Assignee)

Comment 16

3 years ago
Created attachment 8694546 [details] [diff] [review]
1216748-p4-check-other-setData-uses-ESR38.patch

Just a rebase for ESR48
Attachment #8694546 - Flags: review+
(Assignee)

Comment 17

3 years ago
ni?rkothari just to make sure releng is tracking this bug.

Please note:
1. Uplift bug 1206211 to beta and esr first (aurora already has it).
2. Do NOT uplift "1216748-p1-gtest.patch v2".
3. Uplift p2, p3, p4 to aurora and beta.
4. Uplift p2, p3, p4-ESR38 to esr.
Flags: needinfo?(gsquelart) → needinfo?(rkothari)
Whiteboard: AndroidID-20923261, published in August 2015 → AndroidID-20923261, published in August 2015; see comment 17 before uplifting
Gerald, it is usually easier for RelMan team (me, Sylvestre, Liz) and Sheriffs if you make specific uplift requests by going to the patch->details and settings mozilla-approval-aurora/beta/esr38/release (as needed) to "?" and filling out the template describing risk assessment, test coverage and other details. Hope that helps!
Flags: needinfo?(rkothari)
(Assignee)

Comment 19

3 years ago
Comment on attachment 8679801 [details] [diff] [review]
1216748-p2-check-metadata-allocation.patch

Aurora/Beta Approval Request Comment
[Feature/regressing bug #]: Stagefright MP4 metadata parser
[User impact if declined]: OOMs/crashes with bad MP4 files
[Describe test coverage new/current, TreeHerder]: specific gtests, broad mochitests
[Risks and why]: Very little, it's only added checks with parsing bailouts.
[String/UUID change made/needed]: None

ESR [Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: It is sec-high.
User impact if declined: OOMs/crashes with bad MP4 files
Fix Landed on Version: 45
Risk to taking this patch (and alternatives if risky): Very little, it's only added checks with parsing bailouts.
String or UUID changes made by this patch: None
Attachment #8679801 - Flags: approval-mozilla-esr38?
Attachment #8679801 - Flags: approval-mozilla-beta?
Attachment #8679801 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 20

3 years ago
Comment on attachment 8677843 [details] [diff] [review]
1216748-p3-reject-small-covr-data.patch

Aurora/Beta Approval Request Comment
[Feature/regressing bug #]: Stagefright MP4 metadata parser
[User impact if declined]: OOMs/crashes with bad MP4 files
[Describe test coverage new/current, TreeHerder]: specific gtests, broad mochitests
[Risks and why]: Very little, it's only added checks with parsing bailouts.
[String/UUID change made/needed]: None

ESR [Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: It is sec-high.
User impact if declined: OOMs/crashes with bad MP4 files
Fix Landed on Version: 45
Risk to taking this patch (and alternatives if risky): Very little, it's only added checks with parsing bailouts.
String or UUID changes made by this patch: None
Attachment #8677843 - Flags: approval-mozilla-esr38?
Attachment #8677843 - Flags: approval-mozilla-beta?
Attachment #8677843 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 21

3 years ago
Comment on attachment 8677844 [details] [diff] [review]
1216748-p4-check-other-setData-uses.patch

Aurora/Beta Approval Request Comment
[Feature/regressing bug #]: Stagefright MP4 metadata parser
[User impact if declined]: OOMs/crashes with bad MP4 files
[Describe test coverage new/current, TreeHerder]: specific gtests, broad mochitests
[Risks and why]: Very little, it's only added checks with parsing bailouts.
[String/UUID change made/needed]: None
Attachment #8677844 - Flags: approval-mozilla-beta?
Attachment #8677844 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 22

3 years ago
Comment on attachment 8694546 [details] [diff] [review]
1216748-p4-check-other-setData-uses-ESR38.patch

ESR [Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: It is sec-high.
User impact if declined: OOMs/crashes with bad MP4 files
Fix Landed on Version: 45
Risk to taking this patch (and alternatives if risky): Very little, it's only added checks with parsing bailouts.
String or UUID changes made by this patch: None
Attachment #8694546 - Flags: approval-mozilla-esr38?
(Assignee)

Comment 23

3 years ago
Thank you for the advice, Ritu.
Comment on attachment 8679801 [details] [diff] [review]
1216748-p2-check-metadata-allocation.patch

Approved for uplift to aurora and beta, should help avoid OOM/crashes. 
Note, instructions for uplift order are at https://bugzilla.mozilla.org/show_bug.cgi?id=1216748#c17
Attachment #8679801 - Flags: approval-mozilla-beta?
Attachment #8679801 - Flags: approval-mozilla-beta+
Attachment #8679801 - Flags: approval-mozilla-aurora?
Attachment #8679801 - Flags: approval-mozilla-aurora+
Comment on attachment 8677844 [details] [diff] [review]
1216748-p4-check-other-setData-uses.patch

Please uplift to aurora and beta, part of fix for OOM crashes.
Attachment #8677844 - Flags: approval-mozilla-beta?
Attachment #8677844 - Flags: approval-mozilla-beta+
Attachment #8677844 - Flags: approval-mozilla-aurora?
Attachment #8677844 - Flags: approval-mozilla-aurora+
Comment on attachment 8677843 [details] [diff] [review]
1216748-p3-reject-small-covr-data.patch

Another patch for aurora and beta, OOM/crash fix for mp4.
Attachment #8677843 - Flags: approval-mozilla-beta?
Attachment #8677843 - Flags: approval-mozilla-beta+
Attachment #8677843 - Flags: approval-mozilla-aurora?
Attachment #8677843 - Flags: approval-mozilla-aurora+
(Assignee)

Updated

3 years ago
Whiteboard: AndroidID-20923261, published in August 2015; see comment 17 before uplifting → AndroidID-20923261, published in August 2015; uplift 1206211 first on beta&esr-38
(In reply to Gerald Squelart [:gerald] from comment #23)
> Thank you for the advice, Ritu.

You are most welcome! :) I know this probably took a few extra minutes (so thank you!) but it really helps everyone stay on the same page and reduces the chances of making any mistakes.
https://hg.mozilla.org/releases/mozilla-aurora/rev/f15dc0e76382
https://hg.mozilla.org/releases/mozilla-aurora/rev/da91739d1443
https://hg.mozilla.org/releases/mozilla-aurora/rev/ab26e636d6ad

for beta part 4 has problems to apply to beta, gerald could you take a look, thanks!
status-firefox44: affected → fixed
Flags: needinfo?(gsquelart)
(Assignee)

Comment 29

3 years ago
(In reply to Carsten Book [:Tomcat] from comment #28)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/f15dc0e76382
> https://hg.mozilla.org/releases/mozilla-aurora/rev/da91739d1443
> https://hg.mozilla.org/releases/mozilla-aurora/rev/ab26e636d6ad
> 
> for beta part 4 has problems to apply to beta, gerald could you take a look,
> thanks!

Bug 1206211 didn't land (approval was set on the wrong patches), that's why this one didn't work.
Hopefully next time 1206211 will go through, so you can just apply this one.

Please ping me if there are still issues, or :rillian if I'm not available.
Flags: needinfo?(gsquelart)
Are we taking this on ESR38? I see it nominated but not approved.
Flags: needinfo?(lhenry)
Sylvestre... this is a question for you!
Flags: needinfo?(lhenry) → needinfo?(sledru)
Whiteboard: AndroidID-20923261, published in August 2015; uplift 1206211 first on beta&esr-38 → [adv-main43+] AndroidID-20923261, published in August 2015; uplift 1206211 first on beta&esr-38
Flags: needinfo?(sledru)
Attachment #8677843 - Flags: approval-mozilla-esr38? → approval-mozilla-esr38+
Attachment #8679801 - Flags: approval-mozilla-esr38? → approval-mozilla-esr38+
Attachment #8694546 - Flags: approval-mozilla-esr38? → approval-mozilla-esr38+
 1216748-p4-check-other-setData-uses-ESR38.patch  fails to apply to esr38 :

patching file media/libstagefright/frameworks/av/media/libstagefright/MPEG4Extractor.cpp
Hunk #1 FAILED at 505
1 out of 1 hunks FAILED -- saving rejects to file media/libstagefright/frameworks/av/media/libstagefright/MPEG4Extractor.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh 1216748p4esr.patch
Flags: needinfo?(gsquelart)
(Assignee)

Comment 35

3 years ago
Bug 1206211 failed to apply, this prevents patches here to apply.
Once bug 1206211 patches land in ESR38, please try again. (I've tried locally and they worked.)
Flags: needinfo?(gsquelart)
Alias: CVE-2015-7222
(In reply to Gerald Squelart [:gerald] from comment #35)
> Bug 1206211 failed to apply, this prevents patches here to apply.
> Once bug 1206211 patches land in ESR38, please try again. (I've tried
> locally and they worked.)

yeah worked great now :)

https://hg.mozilla.org/releases/mozilla-esr38/rev/b9286af98c12
https://hg.mozilla.org/releases/mozilla-esr38/rev/212eb2d26bc8
https://hg.mozilla.org/releases/mozilla-esr38/rev/a8d9bd6ebef3
status-firefox-esr38: affected → fixed
Whiteboard: [adv-main43+] AndroidID-20923261, published in August 2015; uplift 1206211 first on beta&esr-38 → [adv-main43+][adv-esr38.5+] AndroidID-20923261, published in August 2015; uplift 1206211 first on beta&esr-38
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.