Closed Bug 1204580 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Audio/Video: Playback, defect, P1)

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox41 --- wontfix
firefox42 + fixed
firefox43 + fixed
firefox44 --- fixed
firefox-esr38 42+ fixed

People

(Reporter: tsmith, Assigned: mozbugz)

References

Details

(Keywords: crash, csectype-intoverflow, sec-high, Whiteboard: [post-critsmash-triage][adv-main42+][adv-esr38.4+])

Attachments

(5 files)

Attached 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.
Attached video test_case.mp4
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)
Part 2: Check box ranges for overflow before building MediaByteRange's.
Attachment #8667673 - Flags: review?(giles)
Assignee: nobody → gsquelart
Attachment #8667672 - Flags: review?(giles) → review+
Attachment #8667673 - Flags: review?(giles) → review+
Group: media-core-security → core-security-release
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?
(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)
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)
(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?
(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)
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.

Attachment

General

Created:
Updated:
Size: