Bug 1144107 (CVE-2015-4480)

crash in [@ stagefright::SampleTable::isValid() ] with h264 mp4

RESOLVED FIXED in Firefox 40

Status

()

defect
--
critical
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: kiuhnm03, Assigned: jya)

Tracking

(Blocks 1 bug, 6 keywords)

34 Branch
mozilla42
x86_64
Windows 7
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty +
in-testsuite ?

Firefox Tracking Flags

(firefox38 wontfix, firefox38.0.5 wontfix, firefox39 wontfix, firefox40+ verified, firefox41+ verified, firefox42+ verified, firefox-esr31 unaffected, firefox-esr3840+ verified, b2g-v2.0 unaffected, b2g-v2.0M unaffected, b2g-v2.1 unaffected, b2g-v2.1S unaffected, b2g-v2.2 fixed, b2g-v2.2r fixed, b2g-master fixed)

Details

(Whiteboard: [adv-main40+][adv-esr38.2+], crash signature)

Attachments

(6 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
Posted file videofuzzer5.html
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:36.0) Gecko/20100101 Firefox/36.0
Build ID: 20150305021524

Steps to reproduce:

I wrote a little fuzzer. It should crash in a matter of seconds.

Comment 1

4 years ago
Regression range:
good=2014-08-26
bad=2014-08-2 7
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=dc352a7bf234&tochange=0753f7b93ab7

Suspected bug:
Benjamin Chen — Bug 1050664 - 1. Fix IsDormantNeeded() function. 2. Invoke mSource->stop at the suspend code path that will rewind the sampletable implicitly. r=cpearce

Error message:
I/SampleTable( 6532): There are reordered frames present.


CR with FF39 on Win 7:
https://crash-stats.mozilla.com/report/index/d4aed583-ce74-4fc3-a923-d565f2150317

Frame 	Module 	Signature 	Source
0 	xul.dll 	stagefright::SampleTable::isValid() 	media/libstagefright/frameworks/av/media/libstagefright/SampleTable.cpp
1 	xul.dll 	stagefright::MPEG4Extractor::verifyTrack(stagefright::MPEG4Extractor::Track*) 	media/libstagefright/frameworks/av/media/libstagefright/MPEG4Extractor.cpp
2 	xul.dll 	stagefright::MPEG4Extractor::parseChunk(__int64*, int) 	media/libstagefright/frameworks/av/media/libstagefright/MPEG4Extractor.cpp
3 	xul.dll 	stagefright::MPEG4Extractor::parseChunk(__int64*, int) 	media/libstagefright/frameworks/av/media/libstagefright/MPEG4Extractor.cpp
4 	xul.dll 	stagefright::MPEG4Extractor::readMetaData() 	media/libstagefright/frameworks/av/media/libstagefright/MPEG4Extractor.cpp
5 	mozglue.dll 	je_malloc 	memory/mozjemalloc/jemalloc.c
6 	xul.dll 	mp4_demuxer::MP4Demuxer::MP4Demuxer(mp4_demuxer::Stream*, __int64, mozilla::Monitor*) 	media/libstagefright/binding/mp4_demuxer.cpp
7 	xul.dll 	mozilla::MP4Reader::InitDemuxer() 	dom/media/fmp4/MP4Reader.cpp
8 	nss3.dll 	PR_Lock 	nsprpub/pr/src/threads/combined/prulock.c
9 	xul.dll 	mozilla::MediaDecoderStateMachine::DecodeMetadata() 	dom/media/MediaDecoderStateMachine.cpp
10 	xul.dll 	mozilla::ChannelMediaResource::CacheClientSeek(__int64, bool) 	dom/media/MediaResource.cpp
Blocks: 1050664
Status: UNCONFIRMED → NEW
Crash Signature: [@ stagefright::SampleTable::isValid() ]
Component: Untriaged → Video/Audio
Ever confirmed: true
Flags: needinfo?(bechen)
Product: Firefox → Core
Summary: crash with h264 mp4 → crash in [@ stagefright::SampleTable::isValid() ] with h264 mp4
Version: 36 Branch → 34 Branch

Updated

4 years ago
Severity: normal → critical

Updated

4 years ago
Duplicate of this bug: 1144198

Comment 3

4 years ago
Bug 1144198 has a testcase.
I don't think it is relevant to bug 1050664 because the preference is off.
No longer blocks: 1050664
Flags: needinfo?(bechen)

Comment 7

4 years ago
alice, any idea about the bug which has regressed?

Comment 8

4 years ago
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b91deae84856&tochange=efe5844c9a5b

Triggered by:
efe5844c9a5b	Chris Pearce — Bug 1057879 - Enable MP4Reader on Windows. r=kentuckyfriedtakahe

Updated

4 years ago
Blocks: 1057879

Updated

4 years ago
Flags: needinfo?(cpearce)

Comment 9

4 years ago
Regression window with forced enabled MSE mp4
  user_pref("media.mediasource.enabled", true);
  user_pref("media.mediasource.mp4.enabled", true);
  user_pref("media.fragmented-mp4.exposed", true);

Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=fc8ba992b5b0&tochange=a58872df7e34

Triggered by : Bug 908503

Updated

4 years ago
Blocks: 908503
Demuxer crash -> ajones
Flags: needinfo?(cpearce) → needinfo?(ajones)
(Assignee)

Updated

4 years ago
QA Contact: jyavenard
(Assignee)

Updated

4 years ago
Assignee: nobody → jyavenard
QA Contact: jyavenard
(Assignee)

Updated

4 years ago
Flags: needinfo?(ajones)

Comment 11

4 years ago
[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:  crashes on Beta40.0, Aurora41.0a2(2015-07-01) and Nightly42.0a1(2015-07-01)

I do not understand that he marked "unaffected" for 40+, because it crashes the build with STR in comment #0.


STR
1. Open attachment 8578619 [details]


Beta40.0(buildID=20150630083644) crashes (no reports)
Aurora41.0a2(2015-07-01) bp-94ad2e14-41bc-4c38-b342-439e32150701
Nightly42.0a1(2015-07-01) bp-a49ad44a-791b-4242-85de-9a7582150701

Updated

4 years ago
Flags: needinfo?(ajones)

Updated

4 years ago
Blocks: 1179497
(Assignee)

Updated

4 years ago
Group: core-security
(Assignee)

Comment 13

4 years ago
Attachment #8629141 - Flags: review?(ajones)
(Assignee)

Comment 14

4 years ago
Attachment #8629142 - Flags: review?(ajones)
(Assignee)

Comment 15

4 years ago
this would occur if we attempt to read exactly the end of the array.
Attachment #8629157 - Flags: review?(ajones)
(Assignee)

Comment 16

4 years ago
Thank you for this fuzzer...

This need to be uplifted to all version up to ESR38. Patches should apply on all of them but will verify that.
Flags: needinfo?(ajones)
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1179497
Attachment #8629140 - Flags: review?(ajones) → review+
Attachment #8629141 - Flags: review?(ajones) → review+
Comment on attachment 8629142 [details] [diff] [review]
P3. Prevent int overflow in sort.

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

::: media/libstagefright/frameworks/av/media/libstagefright/MPEG4Extractor.cpp
@@ +4098,5 @@
>  
>  static int compositionOrder(MediaSource::Indice* const* indice0,
>          MediaSource::Indice* const* indice1)
>  {
> +  return (*indice0)->start_composition > (*indice1)->start_composition;

This function should return <0 =0 or >0 but this code doesn't do that.
Attachment #8629142 - Flags: review?(ajones) → review-
Attachment #8629157 - Flags: review?(ajones) → review+
(Assignee)

Comment 19

4 years ago
(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #18)
> Comment on attachment 8629142 [details] [diff] [review]
> P3. Prevent int overflow in sort.
> 
> Review of attachment 8629142 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> media/libstagefright/frameworks/av/media/libstagefright/MPEG4Extractor.cpp
> @@ +4098,5 @@
> >  
> >  static int compositionOrder(MediaSource::Indice* const* indice0,
> >          MediaSource::Indice* const* indice1)
> >  {
> > +  return (*indice0)->start_composition > (*indice1)->start_composition;
> 
> This function should return <0 =0 or >0 but this code doesn't do that.

yes, but the sort algorithm only cares if the result is > 0 or not.
(Assignee)

Comment 20

4 years ago
The sort algorithm only checks if the returned value is > 0.
The original code was substracting two unsigned 64bits integer and converting
it to a signed integer. Behaviour that is undefined. It's a wonder it ever worked.
(Assignee)

Updated

4 years ago
Attachment #8629142 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8629204 - Flags: review?(ajones)
Attachment #8629204 - Flags: review?(ajones) → review+
(Assignee)

Comment 21

4 years ago
Comment on attachment 8629140 [details] [diff] [review]
P1. Mark tracks with no samples table as invalid.

[Security approval request comment]
>How easily could an exploit be constructed based on the patch?
Craft special MP4 with a samples table using invalid size

>Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Not really, the commit log only states that an overflow was fixed

> Which older supported branches are affected by this flaw?
35 and all using libstagefright, affects ESR38.

> If not all supported branches, which bug introduced the flaw?
We have several bugs at play, introducing different flaws.
libstagefright was introduced in bug 908503. However it was only made active by default on windows for FF 35 (bug 1057879)

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
I believe patches will all apply on all branches, except maybe patch 3 that would require small rebase.

> How likely is this patch to cause regressions; how much testing does it need?
Extremely low, we only add extra tests around known failure points.
Attachment #8629140 - Flags: sec-approval?
(Assignee)

Comment 22

4 years ago
The above is a request for all patches. not just the first one
(Assignee)

Comment 23

4 years ago
this would occur if we attempt to read exactly the end of the array.
esr38 rebase.
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1180986
All the crashes people linked to in this and the dupe bugs are safe null derefs (+ small offset for the struct member) so I don't think we can call it "critical", though :jya seems to be worried enough based on patching the problem that "high" is justified.
Flags: sec-bounty?
Keywords: sec-high
Comment on attachment 8629140 [details] [diff] [review]
P1. Mark tracks with no samples table as invalid.

sec-approval+ for checkin on July 14 or later. We'll want patches for affected branches as well.
Attachment #8629140 - Flags: sec-approval? → sec-approval+
Whiteboard: [checkin on 7/14]
(Assignee)

Comment 30

4 years ago
It does.
Flags: needinfo?(jyavenard)
To speed up the process, can you please nominate this for Aurora/Beta/esr38 when you get a chance? :)
(Assignee)

Comment 32

4 years ago
Comment on attachment 8629140 [details] [diff] [review]
P1. Mark tracks with no samples table as invalid.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Crash
Fix Landed on Version: in inbound
Risk to taking this patch (and alternatives if risky): Low
String or UUID changes made by this patch: None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/regressing bug #]: We have several bugs at play, introducing different flaws. libstagefright was introduced in bug 908503. However it was only made active by default on windows for FF 35 (bug 1057879)
[User impact if declined]: Crash, potentially security sensitive.
[Describe test coverage new/current, TreeHerder]: Been in a few try runs, tested locally for over a week
[Risks and why]: Low, only adding boundaries check to code
[String/UUID change made/needed]: Low
Flags: needinfo?(jyavenard)
Attachment #8629140 - Flags: approval-mozilla-esr38?
Attachment #8629140 - Flags: approval-mozilla-beta?
Attachment #8629140 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 33

4 years ago
Uplift approval is for all 4 patches.
Attachment #8629140 - Flags: approval-mozilla-esr38?
Attachment #8629140 - Flags: approval-mozilla-esr38+
Attachment #8629140 - Flags: approval-mozilla-beta?
Attachment #8629140 - Flags: approval-mozilla-beta+
Attachment #8629140 - Flags: approval-mozilla-aurora?
Attachment #8629140 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 38

4 years ago
Sorry for that, replace mData->Elements() with mData.Elements()

Or I'll upload a new patch first thing tomorrow morning
(Assignee)

Comment 42

4 years ago
I can do a rebase, however stagefright isn't used in 34.
Flags: needinfo?(jyavenard)
Flags: qe-verify+
Flags: sec-bounty? → sec-bounty+
Reproduced the initial crash on Firefox 40 beta 1 under Windows 7 64-bit:
bp-94ad2e14-41bc-4c38-b342-439e32150701

Socorro shows 84 crashes in the last week but all of them are on builds before this fix, no crashes with builds after fix.

Verified that using the attachment from comment 0, latest builds (Firefox 40 beta 6, latest Aurora, latest Nightly and latest ESR 38.1.0 tinderbox builds) don`t receive any crashes across platforms (Windows 7 64-bit, Mac OS X 10.10.4 and Ubuntu 14.04 32-bit).
Whiteboard: [adv-main40+][adv-esr38.2+]
Alias: CVE-2015-4480

Updated

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