1216748 - (CVE-2015-7222) stagefright: potential underflow in 'covr', unchecked allocation&copy in Metadata::setData

Status: RESOLVED FIXED

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

Description

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

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).

Comment 1

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

Attachment: 1216748-p1-gtest.patch

Part 1: Added test case with different 'covr' sizes.

Comment 2

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

Attachment: 1216748-p2-check-metadata-allocation.patch

Part 2: Handle failed malloc in Metadata storage.

Comment 3

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

Attachment: 1216748-p3-reject-small-covr-data.patch

Part 3: Ensure 'covr' data size cannot create underflow.

Comment 4

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

Attachment: 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.

Comment 5

[Ralph Giles (:rillian)]

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.

Comment 6

[Ralph Giles (:rillian)]

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?

Comment 7

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

(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.

Comment 8

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

Attachment: 1216748-p2-check-metadata-allocation.patch

Added documentation to address concerns from comment 5.
No actual code change; carrying r+ from comment 5.

Comment 9

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

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?)

Comment 10

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

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).

Comment 11

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

sec-approval+ for checkin on November 10, after we are into the new cycle a bit.

Comment 12

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

Attachment: 1216748-p1-gtest.patch v2

Just a simple rebase, carrying r+.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a0eedfc2faec	
https://hg.mozilla.org/integration/mozilla-inbound/rev/0962522f86ab	
https://hg.mozilla.org/integration/mozilla-inbound/rev/71113b0fe61f	
https://hg.mozilla.org/integration/mozilla-inbound/rev/330c1763806e

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if 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

Comment 15

[Daniel Veditz [:dveditz]]

This one should have landed on all affected branches.

Comment 16

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

Attachment: 1216748-p4-check-other-setData-uses-ESR38.patch

Just a rebase for ESR48

Comment 17

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

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.

Comment 18

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

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!

Comment 19

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

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

Comment 20

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

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

Comment 21

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

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

Comment 22

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

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

Comment 23

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

Thank you for the advice, Ritu.

Comment 24

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

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

Comment 25

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

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.

Comment 26

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

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.

Comment 27

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

(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.

Comment 28

[Carsten Book [:Tomcat]]

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!

Comment 29

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

(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.

Comment 30

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

https://hg.mozilla.org/releases/mozilla-beta/rev/107691986306
https://hg.mozilla.org/releases/mozilla-beta/rev/3afbbd31560d
https://hg.mozilla.org/releases/mozilla-beta/rev/dd66dc8e2eb1

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/ab26e636d6ad
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/da91739d1443
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/f15dc0e76382

Comment 32

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

Are we taking this on ESR38? I see it nominated but not approved.

Comment 33

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

Sylvestre... this is a question for you!

Comment 34

[Carsten Book [:Tomcat]]

 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

Comment 35

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

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.)

Comment 36

[Carsten Book [:Tomcat]]

(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