Bug 1154683 (CVE-2015-2717)

Integer overflow in libstagefright (data tag in mp4) might lead to heap overflow

RESOLVED FIXED in Firefox 38, Firefox OS v2.0

Status

()

--
major
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: laf.intel, Assigned: jya)

Tracking

({csectype-bounds, sec-high})

Trunk
mozilla40
All
Android
csectype-bounds, sec-high
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox37 wontfix, firefox38+ fixed, firefox38.0.5 fixed, firefox39+ fixed, firefox40- fixed, firefox-esr31 unaffected, firefox-esr38 fixed, b2g-v1.4 unaffected, b2g-v2.0 fixed, b2g-v2.0M fixed, b2g-v2.1 fixed, b2g-v2.1S fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

(Whiteboard: [adv-main38+][see bug 1158568 and don't open until July 8])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
Created attachment 8592760 [details]
integer overflow PoC

A specially crafted mp4 file can cause a integer overflow in the MPEG4Extractor::parseMetaData() function. When 'size' is 0xffffffff a integer overflow will occur which leads to an allocation of a (almost) 0 bytes big buffer (https://dxr.mozilla.org/mozilla-central/source/media/libstagefright/frameworks/av/media/libstagefright/MPEG4Extractor.cpp#2196).
Later the 0xffffffff bytes should be copied into the small buffer 'buffer' which would lead to a heap overflow.

Attached is a PoC which causes the integer overflow. On the tested firefox (build from mozilla-central) using a Nexus 5 (contrary to the expected behavior) no heap overflow occurred. This behavior might be caused by the implementation of read(). read() is called (with the anticipated 0xffffffff as size) but no write into the buffer occurs.
Component: Audio/Video → Video/Audio
Product: Firefox for Android → Core
Keywords: csectype-bounds
Flags: needinfo?(jyavenard)
Keywords: sec-high
(Assignee)

Comment 1

3 years ago
Created attachment 8593114 [details] [diff] [review]
Fix potential size overflow

prevent size_t overflow.
Attachment #8593114 - Flags: review?(ajones)
(Assignee)

Updated

3 years ago
Assignee: nobody → jyavenard
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 2

3 years ago
Comment on attachment 8593114 [details] [diff] [review]
Fix potential size overflow

there's a few like those, going for a different approach.
Attachment #8593114 - Attachment is obsolete: true
Flags: needinfo?(jyavenard)
Attachment #8593114 - Flags: review?(ajones)
(Assignee)

Comment 3

3 years ago
Created attachment 8593128 [details] [diff] [review]
Fix potential size overflow

Ensure no size will ever overflow.
Attachment #8593128 - Flags: review?(ajones)
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1154672
Comment on attachment 8593128 [details] [diff] [review]
Fix potential size overflow

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

::: media/libstagefright/frameworks/av/media/libstagefright/MPEG4Extractor.cpp
@@ +1844,5 @@
>              }
>  
> +            if (size > (size_t)-1 - chunk_size) {
> +                return ERROR_MALFORMED;
> +            }

This statement is really hard to read. IIUC it is preventing an overflow. It would make sense to add a comment to that effect.
Attachment #8593128 - Flags: review?(ajones) → review+
[Tracking Requested - why for this release]: We probably want this on beta before 38 goes out the door.
status-firefox37: --- → affected
status-firefox38: --- → affected
status-firefox39: --- → affected
status-firefox40: --- → affected
tracking-firefox38: --- → ?
tracking-firefox39: --- → ?
tracking-firefox40: --- → ?
(Assignee)

Comment 8

3 years ago
Comment on attachment 8593128 [details] [diff] [review]
Fix potential size overflow

Approval Request Comment
[Feature/regressing bug #]:1154683
[User impact if declined]: Potential overflow and out of bound read.
[Describe test coverage new/current, TreeHerder]: Tested locally.
[Risks and why]: Low. Just extra checks that no size calculation will ever overflow
[String/UUID change made/needed]: None
Attachment #8593128 - Flags: approval-mozilla-beta?
Attachment #8593128 - Flags: approval-mozilla-aurora?
sec-high, tracking.
status-firefox37: affected → wontfix
tracking-firefox38: ? → +
tracking-firefox39: ? → +
tracking-firefox40: ? → -
https://hg.mozilla.org/mozilla-central/rev/41787c8d077d
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox40: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Comment on attachment 8593128 [details] [diff] [review]
Fix potential size overflow


(In reply to Jean-Yves Avenard [:jya] from comment #8)
> Approval Request Comment
> [Feature/regressing bug #]:1154683

Bug 1154683 is this bug. In which bug was this issue introduced? More to the point, how far back does this go? Is ESR31 affected?

This is now on m-c. Let's get it on Beta and Aurora as well.
Flags: needinfo?(jya)
Attachment #8593128 - Flags: approval-mozilla-beta?
Attachment #8593128 - Flags: approval-mozilla-beta+
Attachment #8593128 - Flags: approval-mozilla-aurora?
Attachment #8593128 - Flags: approval-mozilla-aurora+
status-firefox-esr31: --- → ?
(Assignee)

Comment 12

3 years ago
(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #11)
> Comment on attachment 8593128 [details] [diff] [review]
> Fix potential size overflow
> 
> 
> (In reply to Jean-Yves Avenard [:jya] from comment #8)
> > Approval Request Comment
> > [Feature/regressing bug #]:1154683
> 
> Bug 1154683 is this bug. In which bug was this issue introduced? More to the
> point, how far back does this go? Is ESR31 affected?
> 
> This is now on m-c. Let's get it on Beta and Aurora as well.

I'd say since stagefright was added to our tree.
However, looking at ESR31 source code, it doesn't use libstagefright.
Flags: needinfo?(jya)
(Assignee)

Comment 13

3 years ago
Bug stagefright was introduced is bug 908503
(Assignee)

Comment 14

3 years ago
Lawrence, I read that 38 was merged today, should I uplift this to m-r ?
Flags: needinfo?(lmandel)
Comment on attachment 8593128 [details] [diff] [review]
Fix potential size overflow

[Triage Comment]
Yes, that should be in m-r
Attachment #8593128 - Flags: approval-mozilla-beta+ → approval-mozilla-release+
Flags: needinfo?(lmandel)
(Assignee)

Comment 16

3 years ago
when does this need to be done by? pretty late here, was hoping to do it first thing tomorrow morning
tomorrow is fine, no worries!
https://hg.mozilla.org/releases/mozilla-aurora/rev/4131212a78ce

This shouldn't have landed without sec-approval since it's a sec-high affecting multiple releases. Please request it retroactively.
https://wiki.mozilla.org/Security/Bug_Approval_Process
status-b2g-v1.4: --- → unaffected
status-b2g-v2.0: --- → affected
status-b2g-v2.0M: --- → affected
status-b2g-v2.1: --- → affected
status-b2g-v2.1S: --- → affected
status-b2g-v2.2: --- → affected
status-b2g-master: --- → fixed
status-firefox39: affected → fixed
status-firefox-esr31: ? → unaffected
status-firefox-esr38: --- → fixed
Flags: needinfo?(jyavenard)
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #18)
> This shouldn't have landed without sec-approval since it's a sec-high
> affecting multiple releases. Please request it retroactively.
> https://wiki.mozilla.org/Security/Bug_Approval_Process

Ack! I missed that this bug didn't have sec approval when I saw that it had landed on m-c. We could really use an Hg hook to prevent sec-high and sec-crit bugs from landing without sec approval.
(Assignee)

Comment 23

3 years ago
Comment on attachment 8593128 [details] [diff] [review]
Fix potential size overflow

[Security approval request comment]
How easily could an exploit be constructed based on the patch? You'll need to craft an MP4 file with bogus atom's size offset. If you set the offset to be INT32_MAX - [1-8] ; you could end up allocating only [1-8] bytes and write in this allocated buffer INT32_MAX bytes.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? no. Only mentions that we prevent an overflow from happening (and reject the entire mp4 as being invalid)

Which older supported branches are affected by this flaw? anything including libstagefright. The flow was in this 3rd party library/

If not all supported branches, which bug introduced the flaw? bug 908503

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? The patch should apply on all branches with stagefright

How likely is this patch to cause regressions; how much testing does it need? Little, we just ensure that no integer overflow can occur.

As a note, all android devices ship with stagefright of which we have no control. They will have that flaw too. Firefox on Android makes use of the system's stagefright. Hopefully google will fix it.
Flags: needinfo?(jyavenard)
Attachment #8593128 - Flags: sec-approval?
Comment on attachment 8593128 [details] [diff] [review]
Fix potential size overflow

I might as well give sec-approval+ at this point. It's in.
Attachment #8593128 - Flags: sec-approval? → sec-approval+
(Assignee)

Updated

3 years ago
Blocks: 1158568
status-firefox38.0.5: --- → fixed
adding sec-bounty since the reporter pinged me on irc asking about it.
Flags: sec-bounty?
Whiteboard: [adv-main38+]
Alias: CVE-2015-2717
Whiteboard: [adv-main38+] → [adv-main38+][see bug 1158568 and don't open until July 8]
Flags: sec-bounty? → sec-bounty+
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1194479

Updated

3 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.