Stagefright: crash [@stagefright::SampleTable::setCompositionTimeToSampleParams]

RESOLVED FIXED in Firefox 42

Status

()

defect
P1
critical
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: tsmith, Assigned: gerald)

Tracking

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

Trunk
mozilla44
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 wontfix, firefox42+ fixed, firefox43+ fixed, firefox44 fixed, firefox-esr3842+ fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main42+][adv-esr38.4+])

Attachments

(5 attachments)

Reporter

Description

4 years ago
Posted file call_stack.txt
Since I can not consistently reproduce this issue I am going to assume there is a race component and that this may not always be a null crash. Feel free to open it up if this is not the case.
Reporter

Comment 1

4 years ago
Posted video test_case.mp4
Assignee

Comment 2

4 years ago
Part 1: Added test case file.

When running gtests, this triggers an assertion:
###!!! ASSERTION: Range should end after start!: 'mStart <= mEnd', file dist/include/MediaResource.h, line 143
#01: mozilla::MediaByteRange::MediaByteRange(long long, long long)
#02: mp4_demuxer::Box::Box(mp4_demuxer::BoxContext*, unsigned long long, mp4_demuxer::Box const*)

Not exactly the same call stack as recorded in this bug.
But if this assertion didn't fire in non-debug, the overflowed range could later cause just about anything, including the recorded crash.
Attachment #8667672 - Flags: review?(giles)
Assignee

Comment 3

4 years ago
Part 2: Check box ranges for overflow before building MediaByteRange's.
Attachment #8667673 - Flags: review?(giles)
Assignee

Updated

4 years ago
Assignee: nobody → gsquelart
Attachment #8667672 - Flags: review?(giles) → review+
Attachment #8667673 - Flags: review?(giles) → review+
Assignee

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0f18e328113b
https://hg.mozilla.org/mozilla-central/rev/35958f485b29
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Group: media-core-security → core-security-release
Assignee

Comment 6

4 years ago
Comment on attachment 8667673 [details] [diff] [review]
1204580-p2-check-box-ranges.patch

Approval Request Comment
[Feature/regressing bug #]: mp4 playback
[User impact if declined]: crashes or worse with some invalid videos
[Describe test coverage new/current, TreeHerder]: gtest, 2 weeks in central
[Risks and why]: very low, only adding overflow checks
[String/UUID change made/needed]: n/a
Attachment #8667673 - Flags: approval-mozilla-beta?
Attachment #8667673 - Flags: approval-mozilla-aurora?
Gerald, do you know how many crashes have been caused by this? 
This is very late in the cycle. Next beta is beta 9...
Flags: needinfo?(gsquelart)
This needed sec-approval? set and a security rating before this was checked into trunk since it affects more than one version of Firefox. 

I'm getting a little annoyed at people checking in security bugs and ignoring process.

https://wiki.mozilla.org/Security/Bug_Approval_Process
Gerald, please set sec-approval? and answer the sec-approval template questions. We need a rating here. Is this crash exploitable?
Assignee

Comment 10

4 years ago
(In reply to Sylvestre Ledru [:sylvestre] from comment #7)
> Gerald, do you know how many crashes have been caused by this? 
> This is very late in the cycle. Next beta is beta 9...

On Socorro, in the last 6 months I only see 3 reports related to this fix, all on 38 and 39 in June-July. So it's probably not urgent.
And I don't believe it's exploitable (more on that below).
Flags: needinfo?(gsquelart)
Assignee

Comment 11

4 years ago
Comment on attachment 8667673 [details] [diff] [review]
1204580-p2-check-box-ranges.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
The patch reveals a few potential overflows. But the consequences would either be failing checked reads (stopping the process short) or triggering an assertion in the MediaByteRange constructor.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
They point at the overflows.

Which older supported branches are affected by this flaw?
If not all supported branches, which bug introduced the flaw?
Since we've imported stagefright, before ESR38.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
I think it should backport without risks, it's only adding some checks and returning early.

How likely is this patch to cause regressions; how much testing does it need?
Unlikely: There is a specific gtest for this, plus lots of media tests.
Attachment #8667673 - Flags: sec-approval?
(In reply to Gerald Squelart [:gerald] from comment #11)
> The patch reveals a few potential overflows. But the consequences would
> either be failing checked reads (stopping the process short)

not necessarily: a lot of exploitation research is in how to manipulate the process memory so the read goes where you want it to. With a predictable allocator like jemalloc it's not that hard.

> or triggering an assertion in the MediaByteRange constructor.

A release build assert, or a debug-only assert? Where? The one in comment 2 is debug-only so that won't save us. Is there another after that?
We should take this on Aurora, Beta, and ESR38 since we already have it on trunk. Please prepare and nominate patches there.
Flags: needinfo?(gsquelart)
Assignee

Comment 14

4 years ago
(In reply to Daniel Veditz [:dveditz] from comment #12)
> (In reply to Gerald Squelart [:gerald] from comment #11)
> > The patch reveals a few potential overflows. But the consequences would
> > either be failing checked reads (stopping the process short)
> 
> not necessarily: a lot of exploitation research is in how to manipulate the
> process memory so the read goes where you want it to. With a predictable
> allocator like jemalloc it's not that hard.

By "checked reads" I was referring to mContext->mSource->CachedReadAt(), effectively BufferStream::CachedReadAt(), which performs range checks (which I believe would handle overflowed offset&length) before actually reading. The copy target is a small fixed-size stack-based buffer.

> > or triggering an assertion in the MediaByteRange constructor.
> 
> A release build assert, or a debug-only assert? Where? The one in comment 2
> is debug-only so that won't save us. Is there another after that?

True, it's a debug-only NS_ASSERTION.

So we could end up with Box::mRange when mStart>mEnd. There are some checks after building that object, but a carefully-tailored range could still pass through.
Now, from what I can see, this object is used is "our" code (i.e., not Google's libstagefright), where media buffer accesses are checked before reading, and we don't blindly alloc&memcpy, so as far as I can see there is no security risk. Caveat: This is based on some limited review of the calling code, and input from :jya.

Should I spend (a lot) more time to check this code further?
Assignee

Comment 15

4 years ago
(In reply to Al Billings [:abillings] from comment #13)
> We should take this on Aurora, Beta, and ESR38 since we already have it on
> trunk. Please prepare and nominate patches there.

We only need attachement 8667673 (1204580-p2-check-box-ranges.patch). It applies cleanly to Aurora and Beta.

However it fails to apply to ESR38 because of code introduced in bug 1171067, which was not uplifted in 38. I'll prepare a new patch.
Flags: needinfo?(gsquelart)
Assignee

Comment 16

4 years ago
Rebased patch for ESR38. Carrying r+.
Attachment #8677177 - Flags: review+
Attachment #8677177 - Flags: approval-mozilla-esr38?
Attachment #8677177 - Flags: approval-mozilla-esr38? → approval-mozilla-esr38+
Attachment #8667673 - Flags: sec-approval?
Attachment #8667673 - Flags: sec-approval+
Attachment #8667673 - Flags: approval-mozilla-beta?
Attachment #8667673 - Flags: approval-mozilla-beta+
Attachment #8667673 - Flags: approval-mozilla-aurora?
Attachment #8667673 - Flags: approval-mozilla-aurora+
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main42+][adv-esr38.4+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.