1172397 - (CVE-2015-2732) Replaying a HTMLMediaElement streamed over a PeerConnection can crash WebRTC

Status: RESOLVED FIXED

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

Description

[Andreas Pehrson [:pehrsons]]

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

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Please pull inbound and try again or import this patch.  Changeset 6e74f27a5ab0 should resolve it (which really fixes Bug 1163852)

Comment 2

[Andreas Pehrson [:pehrsons]]

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.

Comment 3

[Andreas Pehrson [:pehrsons]]

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

Comment 4

[Andreas Pehrson [:pehrsons]]

Attachment: Proof of audiotrack-to-video-pipeline

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.

Comment 5

[Andreas Pehrson [:pehrsons]]

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.

Comment 6

[Andreas Pehrson [:pehrsons]]

Attachment: Check for Conduit/Type mismatch on every frame

Comment 7

[Andreas Pehrson [:pehrsons]]

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.

Comment 8

[Andreas Pehrson [:pehrsons]]

Attachment: Check for Conduit/Type mismatch on every frame

This handles the muxed case.

I'd also like to fix the deeper issue that TrackUnionStream reuses TrackIDs it has previously used.

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8616632 [details] [diff] [review]
Check for Conduit/Type mismatch on every frame

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

LGTM

Comment 10

[Andreas Pehrson [:pehrsons]]

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.

Comment 11

[Andreas Pehrson [:pehrsons]]

(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

Comment 12

[Andreas Pehrson [:pehrsons]]

(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

Comment 13

[Daniel Veditz [:dveditz]]

Randell: is this something you'd want fixed on ESR-38?

Comment 14

[Al Billings [:abillings - ex-MoCo]]

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

Comment 15

[Andreas Pehrson [:pehrsons]]

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.

Comment 16

[Andreas Pehrson [:pehrsons]]

Attachment: Check for Conduit/Type mismatch on every frame

Rebased on m-c. Carries forward r=jesup,bwc.

Comment 17

[Andreas Pehrson [:pehrsons]]

Attachment: Add crashtests

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

Comment 18

[Andreas Pehrson [:pehrsons]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=501792f32fdc

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/60b6d9795c7a

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/60b6d9795c7a

Comment 21

[Andreas Pehrson [:pehrsons]]

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.

Comment 22

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8622396 [details] [diff] [review]
Check for Conduit/Type mismatch on every frame

Stability fix, taking it for 40 & 38 esr.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2e57bb363920

Comment 24

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8622396 [details] [diff] [review]
Check for Conduit/Type mismatch on every frame

We need this in beta too.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/36b1d1148896

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr38/rev/66409cad985c

Comment 27

[Josh Cheng [:josh]]

Comment on attachment 8622396 [details] [diff] [review]
Check for Conduit/Type mismatch on every frame

Approving b2g34/b2g37 for improving security.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/08627d5b256b
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/ed778b596fad
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/ed778b596fad