Closed Bug 1172397 (CVE-2015-2732) Opened 9 years ago Closed 9 years ago

Replaying a HTMLMediaElement streamed over a PeerConnection can crash WebRTC

Categories

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

34 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox38 --- wontfix
firefox38.0.5 --- wontfix
firefox39 --- fixed
firefox40 --- fixed
firefox41 --- fixed
firefox-esr31 --- unaffected
firefox-esr38 39+ fixed
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- fixed
b2g-v2.1S --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: pehrsons, Assigned: pehrsons)

References

Details

(Keywords: sec-moderate, Whiteboard: [adv-main39+][adv-esr38.1+])

Attachments

(3 files, 2 obsolete files)

From bug 1171907:

When I try to play() a captured media element streaming over a PeerConnection that has ended (stream has not ended), we crash here:
>   * frame #0: 0x0000000101f58a1d XUL`mozilla::WebrtcAudioConduit::ValidateCodecConfig(mozilla::AudioCodecConfig const*, bool) const [inlined] std::string::size() const + 4 at basic_string.h:606
>     frame #1: 0x0000000101f58a19 XUL`mozilla::WebrtcAudioConduit::ValidateCodecConfig(mozilla::AudioCodecConfig const*, bool) const [inlined] std::string::empty() const at basic_string.h:686
>     frame #2: 0x0000000101f58a19 XUL`mozilla::WebrtcAudioConduit::ValidateCodecConfig(this=0x000000012b9fa330, codecInfo=0x000000012c684000, send=true) const + 25 at AudioConduit.cpp:1031
>     frame #3: 0x0000000101f58595 XUL`mozilla::WebrtcAudioConduit::ConfigureSendMediaCodec(this=0x000000012b9fa330, codecConfig=0x000000012c684000) + 101 at AudioConduit.cpp:364
>     frame #4: 0x0000000101f6d6f3 XUL`mozilla::MediaPipelineTransmit::PipelineListener::ProcessVideoChunk(this=<unavailable>, conduit=<unavailable>, chunk=<unavailable>) + 3107 at MediaPipeline.cpp:1229
>     frame #5: 0x0000000101f6bc4d XUL`mozilla::MediaPipelineTransmit::PipelineListener::NewData(this=0x0000000126733f20, graph=<unavailable>, tid=<unavailable>, offset=<unavailable>, events=<unavailable>, media=0x000000011eecb460) + 989 at MediaPipeline.cpp:958
>     frame #6: 0x0000000101f6c028 XUL`mozilla::MediaPipelineTransmit::PipelineListener::NotifyQueuedTrackChanges(this=<unavailable>, graph=<unavailable>, tid=<unavailable>, offset=<unavailable>, events=<unavailable>, queued_media=<unavailable>) + 808 at MediaPipeline.cpp:894
>     frame #7: 0x000000010353004a XUL`mozilla::TrackUnionStream::CopyTrackData(this=0x000000011f6f6300, aInputTrack=0x00000001152f3ec0, aMapIndex=<unavailable>, aFrom=<unavailable>, aTo=649728, aOutputTrackFinished=0x0000000137b6e87f) + 714 at TrackUnionStream.cpp:286
Please pull inbound and try again or import this patch.  Changeset 6e74f27a5ab0 should resolve it (which really fixes Bug 1163852)
I haven't been able to reproduce this in the wild yet. With bug 1171907 issue 1 fixed I can reliably reproduce this while playing an mp4 (With vp9cake.webm from our mochitests it does not occur). Seems like the video pipeline is trying to send audio and we're doing a bad cast.

Probably related to MediaDecoder's recreation of the source stream.

We could probably avoid this by never letting StreamBuffers reuse TrackIDs. I.e., if this is what's going on:

> #1  Source  |  TrackUnion   |   MediaPipelineVideo [t1] |   MediaPipelineAudio [t2]
> -- t1 video ----> t1 video ------------> Send
> -- t2 audio ----> t2 audio ---------------------------------------> Send
> 
> * Recreating SourceMediaStream *
> 
> #2  Source  |  TrackUnion               |   MediaPipelineVideo [t1] |   MediaPipelineAudio [t2]
>               --> t1 video [ends soon] -----------> Send
>               --> t2 audio [ends soon] ---------------------------------------> Send
> -- t1 video ----> t3 video ---------------|
> -- t2 audio ----> t4 audio ---------------|
> 
> * Recreating SourceMediaStream *
> 
> #3  Source  |  TrackUnion               |   MediaPipelineVideo [t1] |   MediaPipelineAudio [t2]
>               --> t3 video [ends soon] ---|
>               --> t4 audio [ends soon] ---|
> -- t1 audio ----> t1 audio ---------------------> BOOM?
> -- t2 video ----> t2 video ----------------------------------------------------> BOOM?

I need to do some deeper analysis to verify that this is what's happening, but so far it looks most likely.
(In reply to Randell Jesup [:jesup] from comment #1)
> Please pull inbound and try again or import this patch.  Changeset
> 6e74f27a5ab0 should resolve it (which really fixes Bug 1163852)

Didn't help.
I captured a log of m-i with attachment 8615806 [details] (to allow capture to a stream) and attachment 8615075 [details] (rebased - for extra logging) applied on top.

This is the interesting part of the log:

> 2015-06-08 06:01:45.869404 UTC - 1004544000[1312e9540]: TrackUnionStream 126abef00 adding track 2 for input stream 113e7f330 track 1, start ticks 444528
> 2015-06-08 06:01:45.869418 UTC - 1004544000[1312e9540]: TrackUnionStream 126abef00 adding track 3 for input stream 113e7f330 track 2, start ticks 444528

Looks like there's a stray track lying around in the TrackUnionStream when the SourceMediaStream used as input is recreated. That skews the TrackIDs so track id 2 has now changed type (before: t1 video, t2 audio; after: t2 video, t3 audio).

I'll try to get a test case up, manual or mochi.
There's a check for this in the code, but it's circumvented since bug 1032839.

From https://hg.mozilla.org/mozilla-central/rev/e09f215b65f8:
> if (track_id_ != TRACK_INVALID) {
>   if (tid != track_id_) {
>     return;
>   }
> } else if (conduit_->type() !=
>            (media.GetType() == MediaSegment::AUDIO ? MediaSessionConduit::AUDIO :
>                                                      MediaSessionConduit::VIDEO)) {
>   // Ignore data in case we have a muxed stream
>   return;

From the top,
> if (track_id_ != TRACK_INVALID)
matches so we never check the `else if` and catch the Conduit/Segment type mismatch.
Blocks: 1032839
Assignee: nobody → pehrsons
Status: NEW → ASSIGNED
Attachment #8616552 - Flags: review?(docfaraday)
Version: 41 Branch → 34 Branch
Comment on attachment 8616552 [details] [diff] [review]
Check for Conduit/Type mismatch on every frame

I misunderstood the "muxed stream" case so this won't work. I'll put a new patch up soon-ish.
Attachment #8616552 - Flags: review?(docfaraday)
This handles the muxed case.

I'd also like to fix the deeper issue that TrackUnionStream reuses TrackIDs it has previously used.
Attachment #8616552 - Attachment is obsolete: true
Attachment #8616632 - Flags: review?(docfaraday)
Comment on attachment 8616632 [details] [diff] [review]
Check for Conduit/Type mismatch on every frame

Review of attachment 8616632 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM
Attachment #8616632 - Flags: review+
Attachment #8616632 - Flags: review?(docfaraday) → review+
backlog: --- → webRTC+
Rank: 10
Priority: -- → P1
Comment on attachment 8616632 [details] [diff] [review]
Check for Conduit/Type mismatch on every frame

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
- With the video file and platform I have used to reproduce this I also required another patch to reach the faulty code. I don't know for sure how exposed this bug is until the core issue is fully analysed but I believe a specially crafted video file could make us crash on the right platform on all versions from 34 and up.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
- No. The core issue is quite far from the patched code.

Which older supported branches are affected by this flaw?
- 34 and up.

If not all supported branches, which bug introduced the flaw?
- Bug 1032839.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
- Haven't tried to port yet, but this code seems to have been stable since 34 so risk is very low and creating ports should be easy.

How likely is this patch to cause regressions; how much testing does it need?
- Not likely. This is a central piece to peerconnections that is well covered by automated tests.
Attachment #8616632 - Flags: sec-approval?
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #10)
> [Security approval request comment]
> How easily could an exploit be constructed based on the patch?
> - With the video file and platform I have used to reproduce this I also
> required another patch to reach the faulty code. I don't know for sure how
> exposed this bug is until the core issue is fully analysed but I believe a
> specially crafted video file could make us crash on the right platform on
> all versions from 34 and up.

There seems to be plenty of MediaDecoder producers of PlanarYCbCrImage according to [0]. When streamed, these could all pass through the audio MediaPipelineTransmit and end up in an audio conduit cast to a video conduit (and cause the crash).

There are still some requirements on media/track length that need to be fulfilled. The Big Buck Bunny video at w3 [1] fulfills that - at least together with the decoder on my Mac.

I believe it's the decoder producing audio and video chunks of different length, and when the tracks end and the last chunk has been produced, one of the audio or video tracks is large enough to get pruned from the StreamBuffer, [2], while the other is not, leaving one track dangling. That pruning logic landed in 33 so we're still looking at a 34+ vulnerability.

Chris, do you have an idea of how MediaDecoders on various platforms produce audio/video chunks? Are the chunk sizes typically the same for audio and video? What determines the chunk size, the decoder, the media, or both?

[0] https://dxr.mozilla.org/mozilla-central/search?q=PlanarYCbCrImage+path%3Adom%2Fmedia&case=true
[1] http://www.w3schools.com/html/html5_video.asp
[2] https://dxr.mozilla.org/mozilla-central/source/dom/media/StreamBuffer.cpp#94
Flags: needinfo?(cpearce)
Attachment #8616542 - Attachment mime type: text/plain → application/x-gzip
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #11)
> I believe it's the decoder producing audio and video chunks of different
> length, and when the tracks end and the last chunk has been produced, one of
> the audio or video tracks is large enough to get pruned from the
> StreamBuffer, [2], while the other is not, leaving one track dangling. That
> pruning logic landed in 33 so we're still looking at a 34+ vulnerability.

I figured it out now, and this preliminary analysis was not quite right.

The problem occurs when a media file has a video track and an audio track of different lengths. When the first one has ended and been processed by MediaStreamGraph, the SourceMediaStream fed by MediaDecoderStateMachine will remove it from its StreamBuffer. This will also propagate to the TrackUnionStream on the next MSG iteration which will remove its TrackMapEntry for the track that ended and make that TrackID available for re-use.

When the last track of the SourceMediaStream is ended the stream is marked as finished and automatically blocked. The track is removed from the TrackUnionStream when the input port is removed (this is all good, it happens before we connect the new port). It takes until the next MSG iteration before we remove the track from the StreamBuffer and that is too late - the new tracks will already have been created (with one reused with a different media type from before).


Conclusion: This is worse than I thought. We're exposed to this bug whenever we stream a media file over a PeerConnection, where,
1) the media file's primary audio and video tracks have a difference in length larger than one MSG iteration (~10ms).
2) the media decoder for the media file in question produces PlanarYCbCrImages. See [0] for some examples of this.

I can write a crashtest for this but I'd need a media file where 1) and 2) are true for some platform (and likely to stay true). Chris, is this something you can help with?

As a more longterm solution I'd like to make TrackIDs non-reusable in TrackUnionStream, but I'll do that as a followup in a non-sec-critical bug.


[0] https://dxr.mozilla.org/mozilla-central/search?q=PlanarYCbCrImage+path%3Adom%2Fmedia&case=true
Randell: is this something you'd want fixed on ESR-38?
Flags: needinfo?(rjesup)
Keywords: sec-moderate
Comment on attachment 8616632 [details] [diff] [review]
Check for Conduit/Type mismatch on every frame

This doesn't need sec-approval+ to go in now as a sec-moderate rated issue (sec-approval+ only being necessary for sec-high sec-critical issues).
Attachment #8616632 - Flags: sec-approval?
I got some webm videos fiddled out with ffmpeg that result in the crash on Windows at least. I tried an mp4 as well but it didn't yield anything. It also didn't play on Mac where I originally saw the crash. The only difference from the w3 video seems to be that mine is mp4v1, as ffmpeg can't do mp4v2.
Flags: needinfo?(cpearce)
Rebased on m-c. Carries forward r=jesup,bwc.
Attachment #8616632 - Attachment is obsolete: true
Attachment #8622396 - Flags: review+
Attached patch Add crashtestsSplinter Review
Here's two crashtests with niftily crafted media files (webm, different track length between audio and video) to show that we're exposed to this bug on some platforms. Windows, in this case: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0ccf135f2ff
Attachment #8622398 - Flags: review?(rjesup)
Attachment #8622398 - Flags: review?(rjesup) → review+
https://hg.mozilla.org/mozilla-central/rev/60b6d9795c7a
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Comment on attachment 8622396 [details] [diff] [review]
Check for Conduit/Type mismatch on every frame

Combo approval request,

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: A malicious website may crash the browser at will (verified on Windows but cannot be ruled out for other platforms).
Fix Landed on Version: 41
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none
Bug caused by (feature/regressing bug #): bug 1032839
Testing completed: Explicit crashtests in automation; all peerConnection tests touch this code so regression risk is low.

This request applies to all patches on this bug.
Flags: needinfo?(rjesup)
Attachment #8622396 - Flags: approval-mozilla-esr38?
Attachment #8622396 - Flags: approval-mozilla-beta?
Attachment #8622396 - Flags: approval-mozilla-b2g37?
Attachment #8622396 - Flags: approval-mozilla-b2g34?
Attachment #8622396 - Flags: approval-mozilla-aurora?
Comment on attachment 8622396 [details] [diff] [review]
Check for Conduit/Type mismatch on every frame

Stability fix, taking it for 40 & 38 esr.
Attachment #8622396 - Flags: approval-mozilla-esr38?
Attachment #8622396 - Flags: approval-mozilla-esr38+
Attachment #8622396 - Flags: approval-mozilla-aurora?
Attachment #8622396 - Flags: approval-mozilla-aurora+
Comment on attachment 8622396 [details] [diff] [review]
Check for Conduit/Type mismatch on every frame

We need this in beta too.
Attachment #8622396 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8622396 [details] [diff] [review]
Check for Conduit/Type mismatch on every frame

Approving b2g34/b2g37 for improving security.
Attachment #8622396 - Flags: approval-mozilla-b2g37?
Attachment #8622396 - Flags: approval-mozilla-b2g37+
Attachment #8622396 - Flags: approval-mozilla-b2g34?
Attachment #8622396 - Flags: approval-mozilla-b2g34+
Whiteboard: [adv-main39+][adv-esr38.1+]
Alias: CVE-2015-2732
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.