Closed Bug 1144107 (CVE-2015-4480) Opened 6 years ago Closed 5 years ago

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

Categories

(Core :: Audio/Video, defect)

34 Branch
x86_64
Windows 7
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox38 --- wontfix
firefox38.0.5 --- wontfix
firefox39 --- wontfix
firefox40 + verified
firefox41 + verified
firefox42 + verified
firefox-esr31 --- unaffected
firefox-esr38 40+ 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

People

(Reporter: kiuhnm03, Assigned: jya)

References

(Blocks 1 open bug)

Details

(6 keywords, Whiteboard: [adv-main40+][adv-esr38.2+])

Crash Data

Attachments

(6 files, 1 obsolete file)

Attached 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.
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
Severity: normal → critical
Duplicate of this bug: 1144198
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)
alice, any idea about the bug which has regressed?
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
Blocks: 1057879
Flags: needinfo?(cpearce)
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
Blocks: 908503
Demuxer crash -> ajones
Flags: needinfo?(cpearce) → needinfo?(ajones)
QA Contact: jyavenard
Assignee: nobody → jyavenard
QA Contact: jyavenard
Flags: needinfo?(ajones)
[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
Flags: needinfo?(ajones)
Blocks: 1179497
Group: core-security
Attachment #8629141 - Flags: review?(ajones)
Attachment #8629142 - Flags: review?(ajones)
this would occur if we attempt to read exactly the end of the array.
Attachment #8629157 - Flags: review?(ajones)
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)
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+
(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.
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.
Attachment #8629142 - Attachment is obsolete: true
Attachment #8629204 - Flags: review?(ajones)
Attachment #8629204 - Flags: review?(ajones) → review+
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?
The above is a request for all patches. not just the first one
this would occur if we attempt to read exactly the end of the array.
esr38 rebase.
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]
It does.
Flags: needinfo?(jyavenard)
To speed up the process, can you please nominate this for Aurora/Beta/esr38 when you get a chance? :)
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?
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+
Sorry for that, replace mData->Elements() with mData.Elements()

Or I'll upload a new patch first thing tomorrow morning
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
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.