Closed Bug 1181651 Opened 9 years ago Closed 9 years ago

crash in CmpInstructions

Categories

(Core :: Audio/Video, defect)

39 Branch
x86
Windows NT
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox40 + wontfix
firefox41 + fixed
firefox42 + fixed
firefox43 + fixed
firefox-esr38 41+ fixed
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- unaffected
b2g-v2.1S --- unaffected
b2g-v2.2 --- affected
b2g-v2.2r --- affected
b2g-master --- fixed

People

(Reporter: away, Assigned: jya)

References

(Blocks 1 open bug)

Details

(Keywords: crash, sec-high, Whiteboard: [post-critsmash-triage][adv-main41+][adv-esr38.3+])

Crash Data

Attachments

(2 files, 1 obsolete file)

This bug was filed from the Socorro interface and is 
report bp-f09852df-ab9d-4f18-bd12-e2f962150705.
=============================================================

Moderately high crash on 40b1. Thanks to COMDAT folding, there is a substitute "cmp" function from IonMonkey, but rest assured this is media code. It looks like an out-of-bounds read on the array, since the addresses are often just over a page boundary.

0a53f394 6b2994f0 xul!CmpInstructions+0xf
0a53f3c0 6b2a14ff xul!stagefright::VectorImpl::sort+0x4d
0a53f44c 6b2a051c xul!stagefright::MPEG4Source::exportIndex+0x1b7
0a53f478 6b29eef4 xul!mp4_demuxer::MP4Metadata::ReadTrackIndex+0x52
0a53f4b0 6bc1d5f1 xul!mp4_demuxer::MP4Demuxer::Init+0xb8
0a53f4ec 6bc1eb72 xul!mozilla::InvokeAndRetry<mozilla::MP4Reader,bool>+0x37
0a53f520 6bb980de xul!mozilla::MP4Reader::ReadMetadata+0x3e
0a53f540 6bba32cd xul!mozilla::MediaDecoderReader::AsyncReadMetadata+0x9e
0a53f554 6bbae33c xul!mozilla::detail::MethodCallWithNoArgs<mozilla::MediaPromise<nsRefPtr<mozilla::MetadataHolder>,enum mozilla::ReadMetadataFailureReason,1>,mozilla::MediaDecoderReader>::Invoke+0x11
0a53f564 6bb8c3a7 xul!mozilla::detail::ProxyRunnable<mozilla::MediaPromise<nsRefPtr<mozilla::MetadataHolder>,enum mozilla::ReadMetadataFailureReason,1> >::Run+0x12
0a53f580 6bbafad5 xul!mozilla::AutoTaskDispatcher::TaskGroupRunnable::Run+0x4f
0a53f5cc 6ad7b899 xul!mozilla::MediaTaskQueue::Runner::Run+0xb7
0a53f5f4 6aabf149 xul!nsThreadPool::Run+0x238
Flags: needinfo?(cpearce)
Who's working on the MP4 demuxer these days?
Flags: needinfo?(cpearce) → needinfo?(ajones)
All this code is disabled in 40 and later.
Though it could likely occur with MediaFormatReader and the MP4Demuxer.

David, any chance to have in the trace which media they were playing, a URL of some kind ?
Flags: needinfo?(dmajor)
Blocks: MSE
Flags: needinfo?(ajones)
Most of the URLs are... not appropriate to post here. :)

I've sent them to you in email (I bet it went to spam).
Flags: needinfo?(dmajor)
On 40b6 the signature moved back to the "real" one.
Crash Signature: [@ CmpInstructions] → [@ CmpInstructions] [@ stagefright::compositionOrder ]
Any luck on the investigation?
Flags: needinfo?(jyavenard)
I wonder if this is related to bug 1185115.

The size could have an overflow while reallocating the array.
Flags: needinfo?(jyavenard)
Assignee: nobody → jyavenard
Group: core-security
Depends on: CVE-2015-4479
stagefright::Vector is too crap. Too difficult to fix ; so it's much easier to simply not use it.
This is the primary use we make of it. We make the allocation fallible so no more crashes, and the sort will be much faster than the terrible bubble sort they used.
Comment on attachment 8641607 [details] [diff] [review]
Replace use of stagefright::Vector with nsTArray

forgot to submit this for review...
Attachment #8641607 - Flags: review?(ajones)
> stagefright::Vector is too crap. Too difficult to fix ; so it's much easier
> to simply not use it.

Would it be worthwhile to remove stagefright::Vector from the codebase completely (as a separate bug)? I see only about 30 uses of it: https://dxr.mozilla.org/mozilla-central/search?q=type-ref%3Astagefright%3A%3AVector&redirect=true&case=true&limit=76&offset=0
I've already submitted a patch doing just that. It's been r+ already. 

It's in bug 1185115
Attachment #8641607 - Flags: review?(ajones) → review+
Comment on attachment 8641607 [details] [diff] [review]
Replace use of stagefright::Vector with nsTArray

[Security approval request comment]
>How easily could an exploit be constructed based on the patch?
I don't know how this actually happens. So probably hard to reproduce.

>Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No. 

> 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?
May require some special backports as this size of stagefright has changed a bit over time thanks to all the security fixes that have been applied at different stage.

> How likely is this patch to cause regressions; how much testing does it need?
Can't think of any.
Attachment #8641607 - Flags: sec-approval?
Let's check this in two weeks into the cycle on August 25.

We'll want Aurora, Beta, and ESR38 patches for this.
Attachment #8641607 - Flags: sec-approval? → sec-approval+
I'm going to override Al and ask that this be checked in now. Replacing the class used is not so obvious, and more testing is better.
Flags: needinfo?(jyavenard)
Whiteboard: [checkin on 8/25]
https://hg.mozilla.org/mozilla-central/rev/6e758736c0c6
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Since this landed a few days ago, and from comment 12, would you like to nominate this for uplift now? Thanks!
Flags: needinfo?(jyavenard)
We still want this uplifted for 41 but the 40 ship has sailed.
Comment on attachment 8641607 [details] [diff] [review]
Replace use of stagefright::Vector with nsTArray

Approval Request Comment
[Feature/regressing bug #]:1034444
[User impact if declined]:Sec-high but most likely crashes
[Describe test coverage new/current, TreeHerder]: In central
[Risks and why]: Low, this is code we control, we just use our own safe nsTArray object in place of the unsafe stagefright::vector
[String/UUID change made/needed]: None

This will likely require tweaks to uplift to beta (or esr38 as the flaw is there too)
Flags: needinfo?(jyavenard)
Attachment #8641607 - Flags: approval-mozilla-beta?
Attachment #8641607 - Flags: approval-mozilla-aurora?
Comment on attachment 8641607 [details] [diff] [review]
Replace use of stagefright::Vector with nsTArray

This fix has been in Nightly for a week, let's uplift to Aurora and Beta. Also since this fixes a crash/sec-high issue.
Attachment #8641607 - Flags: approval-mozilla-beta?
Attachment #8641607 - Flags: approval-mozilla-beta+
Attachment #8641607 - Flags: approval-mozilla-aurora?
Attachment #8641607 - Flags: approval-mozilla-aurora+
As expected, esr38/b2g37 are going to need some rebasing.
Flags: needinfo?(jyavenard)
for some reason beta and earlier can't be build on my machine any longer, so I can't test.

surprised it applied cleanly on beta.
Flags: needinfo?(jyavenard)
Comment 2 says that this is fixed in 40 and higher but then we marked 40 was "won't fix" and checked in for 41 and higher. Was 40 affected?
Comment on attachment 8653264 [details] [diff] [review]
Replace use of stagefright::Vector with nsTArray. r=kentuckyfriedtakahe, a=ritu

See comment 19.
Attachment #8653264 - Flags: approval-mozilla-esr38?
Attachment #8653264 - Flags: approval-mozilla-b2g37?
Attachment #8653264 - Flags: approval-mozilla-b2g37?
Group: core-security → core-security-release
Comment on attachment 8653264 [details] [diff] [review]
Replace use of stagefright::Vector with nsTArray. r=kentuckyfriedtakahe, a=ritu

Approved for uplift to ESR38
Attachment #8653264 - Flags: approval-mozilla-esr38? → approval-mozilla-esr38+
Whiteboard: [post-critsmash-triage]
Flags: needinfo?(jyavenard)
hopefully it will be green this time (still no idea why I can't compile anything but >= 42)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc07eabacffc
Seems my patch didn't upload properly yesterday.
This patch changes the rvalue copy constructor to be implicit just like the equivalent nsTArray is.

I explicitly didn't want to convert the FallibleTArray into a nsTArray upon returning the variable as we had OOM issues on this code which is why we moved from infallible code to fallible in the first place.

The other approach was to use an out-param but that would make the change significantly different between what's in >= 41 and esr38 and would cause later problem if any more need to be uplifted (and with stagefright it's rather a given)
Comment on attachment 8659000 [details] [diff] [review]
Replace use of stagefright::Vector with nsTArray. r=kentuckyfriedtakahe, r=nfroyd, a=lizzard

After discussing with gerald, will simply return a nsTArray
Attachment #8659000 - Attachment is obsolete: true
Attachment #8659000 - Flags: review?(nfroyd)
bugger... gcc doesn't like it :(
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main41+][adv-esr38.3+]
(In reply to Carsten Book [:Tomcat] from comment #40)
>  https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/f9afa7166832

had to back this out since this caused a bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=171275&repo=mozilla-b2g37_v2_2
Flags: needinfo?(jyavenard)
(In reply to Carsten Book [:Tomcat] from comment #41)
> (In reply to Carsten Book [:Tomcat] from comment #40)
> >  https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/f9afa7166832
> 
> had to back this out since this caused a bustage like
> https://treeherder.mozilla.org/logviewer.html#?job_id=171275&repo=mozilla-
> b2g37_v2_2

you probably need to use the same patch as the one for esr38...
Flags: needinfo?(jyavenard)
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.