1478843 - (CVE-2018-12391) Cross-origin audio leak in HLS

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Audio/Video, defect, P1)

Description

[Jun (Request needinfo for comment, I ignore other updates)]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/68.0.3440.75 Safari/537.36

Steps to reproduce:

1. Go to https://test.shhnjk.com/hls/webvideo.html
2. Wait for video to stop playing and observe the alert saying "super bad"


Actual results:

Cross-origin audio data leaks. Audio tag on the right side will play stolen sound.


Expected results:

HTTP Live Stream is supported in Firefox for Android. And the M3U8 file allows audio data to be fetched from other origin. But Firefox only checks the url and misses the following redirect in https://test.shhnjk.com/hls/same.m3u8

#EXT-X-MEDIA:TYPE=AUDIO,GROUP-ID="audio",NAME="English stereo",LANGUAGE="en",AUTOSELECT=YES,URI="https://test.shhnjk.com/location.php?url=https://vuln.shhnjk.com/audio.m3u8"

Where audio file is redirected and provided from "https://vuln.shhnjk.com/audio.m3u8". We can abuse this missed check to steal the audio samples using Web Audio API and read the audio data across-origin.

Comment 1

[:Gijs (he/him)]

:drno, :padenot, seems :jya is on PTO so pinging you about this - feel free to redirect to someone else who knows our Fennec AV stack better.

Jun: thanks for the report. I'm assuming, based on you filing this against Firefox for Android, that this doesn't affect desktop (presumably because we're using platform support for m3u8 on Android, but don't do that elsewhere )?

Comment 2

[Jun (Request needinfo for comment, I ignore other updates)]

I don't think Firefox supports HLS (.m3u8 files) in Desktop. If that's supported in any other platform, then it might be affected.

Comment 3

[Paul Adenot (:padenot)]

Nick, you've reviewed some of this before. It looks like the right people are gone.

I can't repro on Nightly, but I can repro on release. On Nightly, the page does not seem to work, and Nightly eventually crashes [0].

Firefox Desktop does not support m3u8 playback so we're good there.

We think that we're handling cross-origin issues at [1]. Looks like we're doing some principal checking there but it's not about the resource, it's the URL of the manifest, so the CORS check passes.

[2] is probably where the problem lies: you can configure to bail out when a cross-protocol redirect occurs, but nothing about principals.

The root of the problem is that we're letting something that is not Gecko perform an HTTP connection to a URL that can be controlled by content, and that's VERY BAD. This bit of Java does not know anything about CORS, and is upstream code (that we appear to have patched). One solution is to plumb down a principal CORS to DefaultHttpDataSource.java and teach it about CORS, but maybe there is something better we can do.

I'm pretty sure I can trivially turn this into an arbitrary cross-origin resource access (text/image/whatever) with a variant of [3].

cc-ing network people because this is a bit bigger in scope that I would have anticipated.

[0]: https://crash-stats.mozilla.com/report/index/364db8b3-a426-4ef5-91f0-3cd8a0180727
[1]: https://searchfox.org/mozilla-central/source/dom/media/hls/HLSDecoder.cpp#146
[2]: https://searchfox.org/mozilla-central/source/mobile/android/geckoview/src/thirdparty/java/com/google/android/exoplayer2/upstream/DefaultHttpDataSource.java#360
[3]: https://jakearchibald.com/2018/i-discovered-a-browser-bug/

Comment 4

[Nick Alexander :nalexander [he/him]]

Yowza.  I have no idea about any of the details of exoplayer2 -- I really was involved only to get the build system plumbing arranged.  I'm not really surprised that Fennec's integration of that component caused issues in this manner:a mismatch of expectations between the browser and the consumed library.  Score another one for really owning our network stack, even in our component libraries.  My understanding is that this is _not_ an issue with exoplayer2, i.e., that this is an issue only because of the same-origin policy that the browser needs to enforce, which arbitrary exoplayer2 consumers don't care about.

I think the best thing I can do is expand the discussion to include snorp, who can a) vet my understanding of whether this needs to be upstreamed to exoplayer2, b) comment on whether this is relevant to GeckoView and/or future Android browsers, and c) redirect further to actually fix the underlying issue, or make the decision to disable exoplayer2/HLS in Firefox for Android.

Comment 5

[Nils Ohlmeier [:drno]]

I got referred to ask dragana and valentin for advice on this one. Based on Paul's comment #3 can help us to assess the situation here?

Comment 6

[Daniel Veditz [:dveditz]]

If we can't change exoplayer2 do we know enough in the Gecko layers to simply taint all HLS video, even same-origin ones? If most sites that use it are just playing the content (and it's not supported on Desktop anyway) maybe that's good enough?

Do we use exoplayer2 for other things? If so then the lack of CORS support is going to bite lots of other places.

Comment 7

[Jun (Request needinfo for comment, I ignore other updates)]

(In reply to Paul Adenot (:padenot) from comment #3)
> I'm pretty sure I can trivially turn this into an arbitrary cross-origin
> resource access (text/image/whatever) with a variant of [3].
> [3]: https://jakearchibald.com/2018/i-discovered-a-browser-bug/
I'm trying to see if it's possible, but crash is stopping me in stable build. Can anyone analyze the crash?

https://test.shhnjk.com/hls/try_fetch.html

Comment 8

[Jun (Request needinfo for comment, I ignore other updates)]

Ah, Null Pointer :D
https://crash-stats.mozilla.com/report/index/02b7a1dd-56c0-40a5-838e-7eb270180727

Comment 9

[Dragana Damjanovic [:dragana]]

I will ad Christoph as well.

Comment 10

[Dragana Damjanovic [:dragana]]

Can we get info about redirects from exoplayer2?

Comment 11

[Paul Adenot (:padenot)]

(In reply to Dragana Damjanovic [:dragana] from comment #10)
> Can we get info about redirects from exoplayer2?

Unclear, I've just been reading the code for now. It does follow redirect [0], but maybe you have something else in mind?

[0]: https://searchfox.org/mozilla-central/source/mobile/android/geckoview/src/thirdparty/java/com/google/android/exoplayer2/upstream/DefaultHttpDataSource.java#355

Comment 12

[Dragana Damjanovic [:dragana]]

(In reply to Paul Adenot (:padenot) from comment #11)
> (In reply to Dragana Damjanovic [:dragana] from comment #10)
> > Can we get info about redirects from exoplayer2?
> 
> Unclear, I've just been reading the code for now. It does follow redirect
> [0], but maybe you have something else in mind?
> 
> [0]:
> https://searchfox.org/mozilla-central/source/mobile/android/geckoview/src/
> thirdparty/java/com/google/android/exoplayer2/upstream/DefaultHttpDataSource.
> java#355

exoplayer2 follows redirects I have seen that. Gecko does not know about these redirects. If gecko would know that a cross-origin redirect is made it could make proper decisions and avoid this problem. My question was is there a way the exoplayer2 tells gecko that it follow a redirect?

Comment 13

[Paul Adenot (:padenot)]

(In reply to Daniel Veditz [:dveditz] from comment #6)
> If we can't change exoplayer2 do we know enough in the Gecko layers to
> simply taint all HLS video, even same-origin ones? If most sites that use it
> are just playing the content (and it's not supported on Desktop anyway)
> maybe that's good enough?

Yes, attaching a patch that does that if that's what we want. The patch is very simple.

> Do we use exoplayer2 for other things? If so then the lack of CORS support
> is going to bite lots of other places.

It's only used to playback streams in HTMLMediaElement on Android (no other parts of Gecko make use of it). However it's plausible that some content is trying to do something more complex with those HTMLMediaElement (captureStream, Web Audio, canvas, etc.).

We could teach exoplayer2 about CORS, it does not seem too complicated from a quick look. We could also find a way to find the URL from which the media is currently being fetched from (after all redirects, etc.) and use Gecko facilities to determine the CORS status. I haven't seen an API call to do so (but haven't looked very hard), but it does not seem too crazy to add.

We could also wrap Gecko channnnels in a Java interface and inject it into exoplayer2. It's a bit weird to have something making http(s) requests randomly in Java code, in the middle of the media decoder of a browser engine.

Comment 14

[Paul Adenot (:padenot)]

(In reply to Dragana Damjanovic [:dragana] from comment #12)
> exoplayer2 follows redirects I have seen that. Gecko does not know about
> these redirects. If gecko would know that a cross-origin redirect is made it
> could make proper decisions and avoid this problem. My question was is there
> a way the exoplayer2 tells gecko that it follow a redirect?

Not in the current state of the code it looks like unfortunately.

Comment 15

[Paul Adenot (:padenot)]

Attachment: hls-cors-cross-origin.patch

This makes all HLS stream CORS-cross-origin.

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Dragana Damjanovic [:dragana] from comment #12)
> exoplayer2 follows redirects I have seen that. Gecko does not know about
> these redirects. If gecko would know that a cross-origin redirect is made it
> could make proper decisions and avoid this problem. My question was is there
> a way the exoplayer2 tells gecko that it follow a redirect?

Dragana's suggestion here sounds like the best path forward to me, if technically feasible: Hooking up exoplayer2 redirects with Gecko internal redirects (e.g. by then calling nsContentSecuriyManager::AsyncOnChannelRedirect()) would automagically perform the right security checks.

Is it possible to get callbacks into Gecko whenever exoplayer2 follows a redirect?

Comment 17

[Paul Adenot (:padenot)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #16)
> (In reply to Dragana Damjanovic [:dragana] from comment #12)
> > exoplayer2 follows redirects I have seen that. Gecko does not know about
> > these redirects. If gecko would know that a cross-origin redirect is made it
> > could make proper decisions and avoid this problem. My question was is there
> > a way the exoplayer2 tells gecko that it follow a redirect?
> 
> Dragana's suggestion here sounds like the best path forward to me, if
> technically feasible: Hooking up exoplayer2 redirects with Gecko internal
> redirects (e.g. by then calling
> nsContentSecuriyManager::AsyncOnChannelRedirect()) would automagically
> perform the right security checks.
> 
> Is it possible to get callbacks into Gecko whenever exoplayer2 follows a
> redirect?

Not using current APIs offered by exoplayer2 for now. It's doable though.

Comment 18

[Nick Alexander :nalexander [he/him]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #16)
> (In reply to Dragana Damjanovic [:dragana] from comment #12)
> > exoplayer2 follows redirects I have seen that. Gecko does not know about
> > these redirects. If gecko would know that a cross-origin redirect is made it
> > could make proper decisions and avoid this problem. My question was is there
> > a way the exoplayer2 tells gecko that it follow a redirect?
> 
> Dragana's suggestion here sounds like the best path forward to me, if
> technically feasible: Hooking up exoplayer2 redirects with Gecko internal
> redirects (e.g. by then calling
> nsContentSecuriyManager::AsyncOnChannelRedirect()) would automagically
> perform the right security checks.
> 
> Is it possible to get callbacks into Gecko whenever exoplayer2 follows a
> redirect?

I think this path is the best technically and produces the best outcome, but the cost is high and Fennec is in deep maintenance mode.  I would follow this approach only if the exoplayer2 stack will be used for GeckoView.  I do see that the thirdparty source is in geckoview/src/thirdparty/exoplayer2, so that seems positive.  Even if we do try to "do the right thing", I think we should reconsider the correct path for HLS support in GeckoView.  Is the best place to work...

- our own stack (expensive to build on Android, but the best outcome?)
- upstream exoplayer2 (a chance to update to newer exoplayer2 and solve this problem at the right level)
- our vendored exoplayer2 (little chance of breaking other things)

I'd like snorp to weigh in before we spend too much time trying to pursue the "correct" fix for exoplayer2.

Comment 19

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Nick Alexander :nalexander from comment #18)
> 
> - our own stack (expensive to build on Android, but the best outcome?)

IIRC, this wasn't feasible due to patent issues, but I also remember ajones telling me this gets better at some point. 2019?

> - upstream exoplayer2 (a chance to update to newer exoplayer2 and solve this
> problem at the right level)
> - our vendored exoplayer2 (little chance of breaking other things)
> 
> I'd like snorp to weigh in before we spend too much time trying to pursue
> the "correct" fix for exoplayer2.

I don't have a strong opinion on either of those options. Depending on the patent situation, we may not need to use exoplayer much longer. It's also not clear if HLS will remain relevant.

Comment 20

[Jun (Request needinfo for comment, I ignore other updates)]

Is this bug fixed per comment 15?

Comment 21

[Paul Adenot (:padenot)]

What do we want to do here? It seems we don't have resources to properly fix this at the minute, do we want to land my patch so that at least the hole is plugged?

Comment 22

[Dragana Damjanovic [:dragana]]

(In reply to Paul Adenot (:padenot) from comment #21)
> What do we want to do here? It seems we don't have resources to properly fix
> this at the minute, do we want to land my patch so that at least the hole is
> plugged?

I would ask Christoph, but he is on PTO. Tanvi?

Comment 23

[Jean-Yves Avenard [:jya]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #19)
> I don't have a strong opinion on either of those options. Depending on the
> patent situation, we may not need to use exoplayer much longer. It's also
> not clear if HLS will remain relevant.

AFAIK, almost any streams on android is an HLS one... so it's still very relevant on mobile...

Comment 24

[Tanvi Vyas[:tanvi]]

(In reply to Paul Adenot (:padenot) from comment #15)
> Created attachment 8995893 [details] [diff] [review]
> hls-cors-cross-origin.patch
> 
> This makes all HLS stream CORS-cross-origin.

Can you describe in more detail the consequence of this change?

Comment 25

[Jun (Request needinfo for comment, I ignore other updates)]

(In reply to Tanvi Vyas[:tanvi] from comment #24)
> (In reply to Paul Adenot (:padenot) from comment #15)
> > Created attachment 8995893 [details] [diff] [review]
> > hls-cors-cross-origin.patch
> > 
> > This makes all HLS stream CORS-cross-origin.
> 
> Can you describe in more detail the consequence of this change?

The change treats all HLS stream as cross-origin, even for same-origin resources.

Comment 27

[Jun (Request needinfo for comment, I ignore other updates)]

I'm planning to present this bug on December too (though I'm not sure if I'll be selected). I can remove Firefox content if this won't be fix till December, but other browser has similar issue, so I expect other people can discover this bug too.

Comment 28

[Daniel Veditz [:dveditz]]

(In reply to Paul Adenot (:padenot) from comment #21)
> What do we want to do here? It seems we don't have resources to properly fix this
> at the minute, do we want to land my patch so that at least the hole is plugged?

Yes. Treating all HLS streams as opaque is perfectly fine.

Comment 29

[Jun (Request needinfo for comment, I ignore other updates)]

Could anyone please land patch in comment 15 to at least close the hole?

Comment 30

[Jean-Yves Avenard [:jya]]

Comment on attachment 8995893 [details] [diff] [review]
hls-cors-cross-origin.patch

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

::: dom/media/hls/HLSDecoder.cpp
@@ +182,5 @@
>  already_AddRefed<nsIPrincipal>
>  HLSDecoder::GetCurrentPrincipal()
>  {
>    MOZ_ASSERT(NS_IsMainThread());
> +  return nullptr;

This needs comments.
Why it's done that way and the downside in doing so.

Comment 31

[Jean-Yves Avenard [:jya]]

Comment on attachment 8995893 [details] [diff] [review]
hls-cors-cross-origin.patch

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

I understand the need to be discrete here on the vulnerability, but I'm concerned that the method as it is now will have its reasoning disappear into oblivion in a short amount of time.

So some explicit reference to this bug number at least is I believe necessary

Comment 32

[Paul Adenot (:padenot)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/19d5e50db843b03d4aed8e5f5736a531035c9cf0 (added a comment).

Comment 33

[:Gijs (he/him)]

(In reply to Paul Adenot (:padenot) from comment #32)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 19d5e50db843b03d4aed8e5f5736a531035c9cf0 (added a comment).

Uh, shouldn't this have had sec-approval to land, given it's sec-high? What are we doing about branches?

Comment 34

[Paul Adenot (:padenot)]

There was private irc conversation with the security team, asking me to land this. All branches are affected, but we'll let this sit a bit on central before requesting uplift.

Comment 35

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/19d5e50db843

Comment 36

[Frederik Braun [:freddy]]

(In reply to Paul Adenot (:padenot) from comment #34)
> There was private irc conversation with the security team, asking me to land
> this. All branches are affected, but we'll let this sit a bit on central
> before requesting uplift.

For posterity's sake, I talked to padenot and asked him if this bug could move forward.
I didn't clearly explain that he needs separate(!) sec-approval on the patch, that I'm not entitled to grant.
I should have made clear during the conversation. 

On a more uplifting note, padenot also tells me that there are other, unrelated bugs that actually stop the exploit from working.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta and ESR60 approval when you get a chance.

Comment 38

[Paul Adenot (:padenot)]

Comment on attachment 8995893 [details] [diff] [review]
hls-cors-cross-origin.patch

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1345752

User impact if declined: Possible vector for CORS leak, however other bugs seem to prevent this being exploitable.

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Medium

Why is the change risky/not risky? (and alternatives if risky): Some web-compat risk, we don't really know how many HLS streams are on other origin (probably quite a few), but the current code is  completely incorrect and dangerous, so we're disabling it here.

String changes made/needed: None

Comment 39

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 8995893 [details] [diff] [review]
hls-cors-cross-origin.patch

Uplift approved for 63 beta 13, thanks.

Comment 40

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/af625b20220f

Comment 41

[Daniel Veditz [:dveditz]]

RyanVM: Do we even have a Fennec ESR? As this is android-only we shouldn't need an ESR uplift.

Comment 42

[Ryan VanderMeulen [:RyanVM]]

Touche.

Comment 43

[Jun (Request needinfo for comment, I ignore other updates)]

I think this bug needs follow up (e.g. https://bugzilla.mozilla.org/show_bug.cgi?id=1486335)

Comment 44

[Sorina Florean [:sflorean]]

I was able to reproduce this issue following the steps from the description on Release 62.0.3 and I can confirm that this is fixed on 63.0b13 and latest Nightly build 64.0a1 with Motorola Nexus 6(Android 7.1.1) and Sony Xperia Z5 Premium(Android 6.0.1).

Comment 45

[Ryan VanderMeulen [:RyanVM]]

Per discussion on IRC, we're going to take this on ESR after all to benefit the Tor folks (who ship an Android browser from that branch).

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/0fa07c704ca4