1144107 - (CVE-2015-4480) crash in [@ stagefright::SampleTable::isValid() ] with h264 mp4

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Massimiliano Tomassoli]

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

[Loic]

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

Comment 3

[Benjamin Smedberg]

Bug 1144198 has a testcase.

Comment 4

[Benjamin Chen [:bechen]]

I don't think it is relevant to bug 1050664 because the preference is off.

Comment 6

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Based on bug #1165495 Comment 4

Comment 7

[Loic]

alice, any idea about the bug which has regressed?

Comment 8

[Alice0775 White]

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

Comment 9

[Alice0775 White]

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

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

Demuxer crash -> ajones

Comment 11

[Alice0775 White]

[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

Comment 12

[Jean-Yves Avenard [:jya]]

Attachment: P1. Mark tracks with no samples table as invalid.

Comment 13

[Jean-Yves Avenard [:jya]]

Attachment: P2. Prevent uin32_t overflow.

Comment 14

[Jean-Yves Avenard [:jya]]

Attachment: P3. Prevent int overflow in sort.

Comment 15

[Jean-Yves Avenard [:jya]]

Attachment: P4. Prevent assert when hitting EOS.

this would occur if we attempt to read exactly the end of the array.

Comment 16

[Jean-Yves Avenard [:jya]]

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.

Comment 18

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

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.

Comment 19

[Jean-Yves Avenard [:jya]]

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

Comment 20

[Jean-Yves Avenard [:jya]]

Attachment: P3. Prevent int overflow in sort.

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.

Comment 21

[Jean-Yves Avenard [:jya]]

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.

Comment 22

[Jean-Yves Avenard [:jya]]

The above is a request for all patches. not just the first one

Comment 23

[Jean-Yves Avenard [:jya]]

Attachment: P4. Prevent assert when hitting EOS.

this would occur if we attempt to read exactly the end of the array.
esr38 rebase.

Comment 25

[Daniel Veditz [:dveditz]]

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.

Comment 27

[Christoph Diehl [:posidron]]

Yea that was/is a top crasher: https://bugzilla.mozilla.org/show_bug.cgi?id=1156505

Comment 28

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

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.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7518d33f23de
https://hg.mozilla.org/integration/mozilla-inbound/rev/299a024d4532
https://hg.mozilla.org/integration/mozilla-inbound/rev/e25c72e7e844
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3ab79e463eb

Just to confirm, this doesn't affect B2G?

Comment 30

[Jean-Yves Avenard [:jya]]

It does.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

To speed up the process, can you please nominate this for Aurora/Beta/esr38 when you get a chance? :)

Comment 32

[Jean-Yves Avenard [:jya]]

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

Comment 33

[Jean-Yves Avenard [:jya]]

Uplift approval is for all 4 patches.

Comment 34

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

https://hg.mozilla.org/mozilla-central/rev/7518d33f23de
https://hg.mozilla.org/mozilla-central/rev/299a024d4532
https://hg.mozilla.org/mozilla-central/rev/e25c72e7e844
https://hg.mozilla.org/mozilla-central/rev/d3ab79e463eb

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/25f872d7fb6e
https://hg.mozilla.org/releases/mozilla-aurora/rev/3b84e1fc8a98
https://hg.mozilla.org/releases/mozilla-aurora/rev/4c273575101d
https://hg.mozilla.org/releases/mozilla-aurora/rev/4b10acdea6f0

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3fdeb3497962
https://hg.mozilla.org/releases/mozilla-beta/rev/3fc2312861ad
https://hg.mozilla.org/releases/mozilla-beta/rev/20c4fa40bc79
https://hg.mozilla.org/releases/mozilla-beta/rev/716d122771c6

Comment 37

[Ryan VanderMeulen [:RyanVM]]

Part 4 backed out from beta for bustage.
https://treeherder.mozilla.org/logviewer.html#?job_id=429527&repo=mozilla-beta

https://hg.mozilla.org/releases/mozilla-beta/rev/e699a62fdcbc

Comment 38

[Jean-Yves Avenard [:jya]]

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

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

Comment 39

[Ryan VanderMeulen [:RyanVM]]

Thanks!

https://hg.mozilla.org/releases/mozilla-beta/rev/5331e255aab2

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr38/rev/7bdab1b1ff1b
https://hg.mozilla.org/releases/mozilla-esr38/rev/d56f11eb8379
https://hg.mozilla.org/releases/mozilla-esr38/rev/741cbef06435
https://hg.mozilla.org/releases/mozilla-esr38/rev/193e39f419d4

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/ec5080a739ec
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/5fee19b568eb
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/a952c442a480
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/55073bfb855f

Part 4 needs rebasing for b2g34.

Comment 42

[Jean-Yves Avenard [:jya]]

I can do a rebase, however stagefright isn't used in 34.

Comment 43

[Ryan VanderMeulen [:RyanVM]]

Don't bother then, thanks.

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/ec5080a739ec
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/5fee19b568eb
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/a952c442a480
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/55073bfb855f

Comment 45

[Bogdan Maris, Desktop Test Engineering]

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