crash in CmpInstructions

RESOLVED FIXED in Firefox 41

Status

()

--
critical
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: dmajor, Assigned: jya)

Tracking

(Blocks: 1 bug, {crash, sec-high})

39 Branch
mozilla43
x86
Windows NT
crash, sec-high
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40+ wontfix, firefox41+ fixed, firefox42+ fixed, firefox43+ fixed, firefox-esr3841+ 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)

Details

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

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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)
(Assignee)

Comment 2

4 years ago
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 ?
(Assignee)

Updated

4 years ago
Flags: needinfo?(dmajor)
Blocks: 778617
Flags: needinfo?(ajones)
(Reporter)

Comment 3

4 years ago
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)
(Reporter)

Comment 4

4 years ago
On 40b6 the signature moved back to the "real" one.
Crash Signature: [@ CmpInstructions] → [@ CmpInstructions] [@ stagefright::compositionOrder ]
(Reporter)

Comment 5

4 years ago
Any luck on the investigation?
Flags: needinfo?(jyavenard)
(Assignee)

Comment 6

4 years ago
I wonder if this is related to bug 1185115.

The size could have an overflow while reallocating the array.
Flags: needinfo?(jyavenard)
(Assignee)

Updated

4 years ago
Assignee: nobody → jyavenard
(Assignee)

Updated

4 years ago
Group: core-security
(Assignee)

Updated

4 years ago
Depends on: 1185115
(Assignee)

Comment 7

4 years ago
Created attachment 8641607 [details] [diff] [review]
Replace use of stagefright::Vector with nsTArray

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

Comment 8

4 years ago
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)
(Reporter)

Comment 9

4 years ago
> 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
(Assignee)

Comment 10

4 years ago
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+
(Assignee)

Comment 11

4 years ago
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.
status-firefox40: --- → wontfix
status-firefox41: --- → affected
status-firefox42: --- → affected
status-firefox43: --- → affected
status-firefox-esr38: --- → affected
tracking-firefox41: --- → +
tracking-firefox42: --- → +
tracking-firefox43: --- → +
tracking-firefox-esr38: --- → ?
Keywords: sec-high
Whiteboard: [checkin on 8/25]
Attachment #8641607 - Flags: sec-approval? → sec-approval+
Blocks: 1192564
status-firefox40: wontfix → affected
tracking-firefox40: --- → +
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
Last Resolved: 4 years ago
status-firefox43: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
No longer blocks: 1192564
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.
status-firefox40: affected → wontfix
(Assignee)

Comment 19

4 years ago
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?
status-b2g-v2.0: --- → unaffected
status-b2g-v2.0M: --- → unaffected
status-b2g-v2.1: --- → unaffected
status-b2g-v2.1S: --- → unaffected
status-b2g-v2.2: --- → affected
status-b2g-v2.2r: --- → affected
status-b2g-master: --- → fixed
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)
(Assignee)

Comment 24

4 years ago
Created attachment 8653264 [details] [diff] [review]
Replace use of stagefright::Vector with nsTArray. r=kentuckyfriedtakahe, a=ritu

patch for esr38 and earlier
(Assignee)

Comment 25

4 years ago
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?

Updated

4 years ago
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+
tracking-firefox-esr38: ? → 41+
Backed out from esr38 for bustage.
https://treeherder.mozilla.org/logviewer.html#?job_id=26470&repo=mozilla-esr38

https://hg.mozilla.org/releases/mozilla-esr38/rev/fcdc0a58d7c3
status-firefox-esr38: fixed → affected
Flags: needinfo?(jyavenard)
Whiteboard: [post-critsmash-triage]
(Assignee)

Updated

4 years ago
Flags: needinfo?(jyavenard)
(Assignee)

Comment 32

4 years ago
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
(Assignee)

Comment 33

4 years ago
Created attachment 8659000 [details] [diff] [review]
Replace use of stagefright::Vector with nsTArray. r=kentuckyfriedtakahe, r=nfroyd, a=lizzard
Attachment #8659000 - Flags: review?(nfroyd)
(Assignee)

Comment 34

4 years ago
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)
(Assignee)

Comment 35

4 years ago
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)
(Assignee)

Updated

4 years ago
status-firefox-esr38: affected → fixed
(Assignee)

Comment 37

4 years ago
bugger... gcc doesn't like it :(
(Assignee)

Comment 38

4 years ago
backed out in https://hg.mozilla.org/releases/mozilla-esr38/rev/7df0233a019d
status-firefox-esr38: fixed → affected
(Assignee)

Updated

4 years ago
status-firefox-esr38: affected → fixed
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
status-b2g-v2.2: fixed → affected
Flags: needinfo?(jyavenard)
(Assignee)

Comment 42

3 years ago
(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.