Closed Bug 1181204 Opened 5 years ago Closed 5 years ago

heap-use-after-free in mozilla::MediaFormatReader::VideoIsHardwareAccelerated()

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox39 --- wontfix
firefox40 + fixed
firefox41 + fixed
firefox42 + fixed
firefox-esr31 --- unaffected
firefox-esr38 --- unaffected
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- unaffected
b2g-v2.1S --- unaffected
b2g-v2.2 --- unaffected
b2g-v2.2r --- unaffected
b2g-master --- fixed

People

(Reporter: tsmith, Assigned: jya)

References

Details

(Keywords: crash, csectype-uaf, sec-high, Whiteboard: [post-critsmash-triage][adv-main40+])

Crash Data

Attachments

(3 files)

Attached file asan_log.txt
I have seen this issue a few times now and I can reproduce it with my fuzzer in about an hour. I'll keep trying to get a reliable test case.

==33111==ERROR: AddressSanitizer: heap-use-after-free on address 0x6040000c31d0 at pc 0x7f1a7c91223c bp 0x7f1a3b694330 sp 0x7f1a3b694328
READ of size 8 at 0x6040000c31d0 thread T66 (MediaPl~back #4)
Attached file asan_log2.txt
We're also seeing this in the wild as poison-crashes. It's one of the top crashes in aurora 41 and beta 40 is affected to a lesser degree.

1fccf9e0 11049e3e xul!mozilla::H264Converter::IsHardwareAccelerated+0xa
1fccf9e4 110316db xul!mozilla::MediaFormatReader::VideoIsHardwareAccelerated+0xf
1fccf9ec 110342b3 xul!mozilla::MediaDecoderStateMachine::GetAmpleVideoFrames+0x2b
1fccfa08 11036cd1 xul!mozilla::MediaDecoderStateMachine::HaveEnoughDecodedVideo+0x3c
1fccfa10 1102d383 xul!mozilla::MediaDecoderStateMachine::NeedToDecodeVideo+0x59
1fccfa1c 1103b751 xul!mozilla::MediaDecoderStateMachine::DispatchVideoDecodeTaskIfNeeded+0x2e
1fccfa28 1024e2ad xul!mozilla::MediaDecoderStateMachine::OnVideoPopped+0x25

mDecoder is 0x5a5a5a5a --> the H264Converter is dead.
Crash Signature: [@ mozilla::H264Converter::IsHardwareAccelerated() ]
Flags: needinfo?(jyavenard)
The core problem here is that we have the MDSM calling code that assumes we're only on the reader's task queue.
As such, we have no locking of any kind (and this is by design).

Bobby had started to remove those type of calls, looks like this one got through.
Flags: needinfo?(jyavenard)
Flags: needinfo?(matt.woodrow)
Flags: needinfo?(jwwang)
This fix a potential race when the decoder could be shutting down on the reader's taskqueue while the MediaDecoderStateMachine thread attempts to read it.
Attachment #8631922 - Flags: review?(cpearce)
This is a fix, but IMHO we should never call anything in the Reader on the MDSM thread.

This also assumes that the MediaDataDecoder::IsHardwareAccelerated code is thread-safe. Currently it's probably fine as all implementations use a boolean that is only modified during MediaDataDecoder::Init().
Assignee: nobody → jyavenard
Attachment #8631922 - Flags: review?(cpearce) → review+
(In reply to Jean-Yves Avenard [:jya] from comment #5)
> This also assumes that the MediaDataDecoder::IsHardwareAccelerated code is
> thread-safe. Currently it's probably fine as all implementations use a
> boolean that is only modified during MediaDataDecoder::Init().

There are occasions where we need to get the result immediately like AudioSink::GetPosition where promise doesn't help. I think making the function thread-safe is the right way to go.
Flags: needinfo?(jwwang)
Comment on attachment 8631922 [details] [diff] [review]
Prevent use of the decoder outside the reader's taskqueue.

[Security approval request comment]
How easily could an exploit be constructed based on the patch? I'd date to say impossible

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? 37, but seems that the crashes are only seen in 41.
The MP4Reader has the same problem.

If not all supported branches, which bug introduced the flaw? 1128380

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? low. Need similar patch for MP4Reader if we are to support version < 39

How likely is this patch to cause regressions; how much testing does it need? very little. Having said that, the same level of confidence is what caused the problem in the first place :)
Flags: needinfo?(matt.woodrow)
Attachment #8631922 - Flags: sec-approval?
Comment on attachment 8631922 [details] [diff] [review]
Prevent use of the decoder outside the reader's taskqueue.

Sec-approval+ for trunk. Let's get Aurora and Beta patches made and nominated as well.
Attachment #8631922 - Flags: sec-approval? → sec-approval+
So can I commit this?
Flags: needinfo?(abillings)
Comment on attachment 8631922 [details] [diff] [review]
Prevent use of the decoder outside the reader's taskqueue.

Approval Request Comment
[Feature/regressing bug #]: 1128380
[User impact if declined]: heap-use-after-free
[Describe test coverage new/current, TreeHerder]: in m-c. Locally tested.
[Risks and why]: Very low
[String/UUID change made/needed]: None
Attachment #8631922 - Flags: approval-mozilla-beta?
Attachment #8631922 - Flags: approval-mozilla-aurora?
Component: Video/Audio → Video/Audio: Playback
Comment on attachment 8631922 [details] [diff] [review]
Prevent use of the decoder outside the reader's taskqueue.

Let's take this in beta6 and verify that the crash signature is not in 40. Beta+ Aurora+
Attachment #8631922 - Flags: approval-mozilla-beta?
Attachment #8631922 - Flags: approval-mozilla-beta+
Attachment #8631922 - Flags: approval-mozilla-aurora?
Attachment #8631922 - Flags: approval-mozilla-aurora+
(In reply to Jean-Yves Avenard [:jya] from comment #9)
> So can I commit this?

sec-approval+ means you can commit to trunk. Aurora and Beta approvals mean you can commit to those branches.
Flags: needinfo?(abillings)
This is going to need a rebased patch for Beta.
Flags: needinfo?(jyavenard)
https://hg.mozilla.org/mozilla-central/rev/df3aedb6cd49
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Is this worth backporting to esr38/b2g37 now that you've done the MP4Reader rebasing anyway?
Flags: needinfo?(jyavenard)
Apparently not. The regression was introduced in 39.
Flags: needinfo?(jyavenard)
Duplicate of this bug: 1181727
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main40+]
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.