1071482 - [EME] Decrypted content needs to be inaccessible from JS

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

Content from media elements that are decrypted using EME is required to be inaccessible from other JS APIs, like canvas, WebAudio, etc.

I assume the easiest way to do this is to trick the MediaDecoder to think it's always cross origin.

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 1: Factor EME setup test code

This makes it easier to re-use the EME setup code in other tests.

Comment 2

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Comment on attachment 8505212 [details] [diff] [review]
Patch 1: Factor EME setup test code

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

::: content/media/test/eme.js
@@ +129,5 @@
> +    })
> +  });
> +}
> +
> +// Returns a promise that is resovled when the media element is ready to have

nit: resovled

::: content/media/test/test_encryptedMediaExtensions.html
@@ +45,5 @@
> +        session.addEventListener("keyschange", KeysChangeFunc(session, test.keys), false);
> +      }
> +    }
> +  );
> +  

nit: whitespace

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 2: Assign NullPrincipal to EME HTMLMediaElements

(Based on patches from bug 1082239 and bug 1081755)

* Make HTMLMediaElement::GetCurrentPrincipal() return a NullPrincipal if it's loading EME content. Canvas/WebAudio check this when reading back decoded data.
* Make HTMLMediaElement::GetCORSMode() direct callers to ignore the crossorigin/CORS attribute/mode when loading EME content. Otherwise setting an crossOrigin attribute on the video allows ignoring the principal check on canvas readback.
* Add tests to ensure that readback from canvas of video and webaudio don't work for EME content.
* Rename test_encryptedMediaExtensions to test_eme_playback, so we can now go `./mach mochitest-plain content/media/test/test_eme* to

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Make HTMLMediaElement::GetCORSMode() direct callers to ignore the crossorigin/CORS
> attribute/mode when loading EME content.

Is there a spec for this somewhere?  Is this supposed to affect just the canvas drawing bits, or the load itself?

If the former, then we should make sure that code inside this class itself uses mCORSMode, not GetCORSMode() (e.g. in HTMLMediaElement::LoadResource).

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

The spec has a section with a bunch of "may"s in it:
https://dvcs.w3.org/hg/html-media/raw-file/default/encrypted-media/encrypted-media.html#media-element-restictions but does not actually define what these conditions are.

What's been communicated to me is that we need to prevent readback, we don't need to affect the load algorithm.

I think you're right that we should use mCORSMode in HTMLMediaElement::LoadResource().

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8505225 [details] [diff] [review]
Patch 2: Assign NullPrincipal to EME HTMLMediaElements

>+    nsRefPtr<nsIPrincipal> principal = do_CreateInstance(NS_NULLPRINCIPAL_CONTRACTID, &rv);

Please cache this principal, so we don't have to keep creating new ones on every call.

With that and the GetCORSMode() replaced by mCORSMode in LoadResource(), I guess r=me.  But note that the spec doesn't actually allow this behavior as far as I can tell: it allows not exposing data to drawImage, but doesn't allow drawing it and then not allowing readbacks, which is what this patch implements.  That seems like a spec issue.

Comment 7

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Comment on attachment 8505225 [details] [diff] [review]
Patch 2: Assign NullPrincipal to EME HTMLMediaElements

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

::: content/html/content/src/HTMLMediaElement.cpp
@@ +3429,5 @@
>  
>    bool subsumes;
>    mDecoder->UpdateSameOriginStatus(
> +    (!principal ||
> +    (NS_SUCCEEDED(NodePrincipal()->Subsumes(principal, &subsumes)) && subsumes)));

This change doesn't do anything.

::: content/media/test/test_eme_canvas_readback_blocked.html
@@ +19,5 @@
> +  var sessions = [];
> +
> +  var v = SetupEME(test, token);
> +  v.preload = "auto"; // Required due to "canplay" not firing for MSE unless we do this.
> +  

nit: whitespace

@@ +33,5 @@
> +      ok(true, token + " - Shouldn't throw exception while drawing to canvas from EME video");
> +    } catch (ex) {
> +      ok(false, token + " - Shouldn't throw exception while drawing to canvas from video");
> +    }
> +    

nit: whitespace

::: content/media/test/test_eme_webaudio_readback_blocked.html
@@ +15,5 @@
> +function startTest(test, token)
> +{
> +  manager.started(token);
> +
> +  var sessions = [];

unused.

@@ +19,5 @@
> +  var sessions = [];
> +
> +  var v = SetupEME(test, token);
> +  v.preload = "auto"; // Required due to "canplay" not firing for MSE unless we do this.
> +  

nit: whitespace here and through the rest of the file

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 1v2: Factor EME setup test code

Stripped whitespace. Carry forward r=edwin.

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 2: Rename test_encryptedMediaExtension

Rename test, and add more logging. This makes it easier to go `./mach mochitest-plain content/media/test/test_eme*` and catch all the EME tests.

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 3: Block readbackable APIs from EME media

Taking a different approach from the previous patch, block APIs that enable readback directly. We need to prevent EME content being readback by JS APIs, including JS APIs running with chrome privileges.

My previous patch would give EME media a null principal, but (I think?) a chrome content addon would still be able to readback content with a null principal. So instead just block canvas drawImage, WebAudio MediaElementSourceNode, and MediaStreams APIs from accepting EME content in the first place.

Did I miss any APIs?

Switching reviewers from bz to roc, as I assume roc understands more about the MediaStreams stuff...)

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 4: BreakCycle in SourceBufferDecoder's MediaTaskQueue

For some reason, the test I added in test_eme_canvas_blocked.html when feeding WebAudio from a MediaElementSourceNode created from a media element playing MSE content that requires EME to decrypt and play where the EME setup never completes due to being blocked causes a shutdown hang.

This is because of a cycle in SourceBufferDecoder, which has a reference to a MediaTaskQueue which stores a reference to a SharedThreadPool which is never cleared, and we never shutdown because the SharedThreadPool never is deleted.

I don't really understand the object graph/lifecycle in MSE, but breaking the cycle in TrackBuffer::BreakCycle() means the SharedThreadPool can shutdown, and fixes the shutdown hang.

Comment 12

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b6226d911ac5
https://tbpl.mozilla.org/?tree=Try&rev=b6226d911ac5

Comment 13

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/913e16c2877b
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6761281d389
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9a9008a1e93
https://hg.mozilla.org/integration/mozilla-inbound/rev/62a8be146b4b

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/913e16c2877b
https://hg.mozilla.org/mozilla-central/rev/e6761281d389
https://hg.mozilla.org/mozilla-central/rev/d9a9008a1e93
https://hg.mozilla.org/mozilla-central/rev/62a8be146b4b

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Backed out for causing various new intermittent failures. The timeouts are covered in bug 1090523 and the log below shows another failure mode.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a542bd0c39e7

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=554243&repo=mozilla-central

Comment 16

[Chris Pearce [:cpearce (Not reading bugmail)]]

There are a couple of things happening here...

It looks like the new tests are only failing on MSE video. It looks like test_eme_canvas_blocked was failing because "canplay" isn't firing there reliably for MSE video.

test_eme_playback was renamed in these patches, it was previously test_encryptedMediaExtensions.html, and there was already a timeout filed against that test: bug 1083924

So the timeout in test_eme_playback is not new, and it only occurs on MSE video.

Since I cannot reproduce failure locally (of course), so I will add more logging and re-land. There is no way I can debug this without re-landing, as the failure rate is so low (5 starred failures in ~41 hours in the tree).

I will also switch test_eme_canvas_blocked to use the loadeddata event rather than the canplay, as that may be more reliable...

If failure gets bad, we can remove the MSE the gizmo-frag-cencinit.mp4 test case from gEMETests in dom/media/test/manifest.js, and that should lower the failure rate. Of course we won't be able to debug the failures then.

Comment 17

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 5: Add more logging to help debug rare intermittent failures

Comment 18

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Comment on attachment 8513960 [details] [diff] [review]
Patch 5: Add more logging to help debug rare intermittent failures

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

::: dom/media/test/eme.js
@@ +119,5 @@
>        });
>  
> +      req.addEventListener("error", bail("Error fetching " + fragmentFile));
> +      req.addEventListener("abort", bail("Aborted fetching " + fragmentFile));
> +      

nit: trailing whitespace.

@@ +160,5 @@
> +    v.addEventListener(e, function(event) {
> +      info(token + " " + e);
> +    }, false);
> +  });  
> +  

nit: trailing whitespace.

Comment 19

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/34d86b491ac1
https://hg.mozilla.org/integration/mozilla-inbound/rev/396575d789e4
https://hg.mozilla.org/integration/mozilla-inbound/rev/971beced9390
https://hg.mozilla.org/integration/mozilla-inbound/rev/8839a3db6421
https://hg.mozilla.org/integration/mozilla-inbound/rev/e72c87749c0f

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/34d86b491ac1
https://hg.mozilla.org/mozilla-central/rev/396575d789e4
https://hg.mozilla.org/mozilla-central/rev/971beced9390
https://hg.mozilla.org/mozilla-central/rev/8839a3db6421
https://hg.mozilla.org/mozilla-central/rev/e72c87749c0f

Comment 21

[Chris Pearce [:cpearce (Not reading bugmail)]]

Mass update firefox-status to track EME uplift.