Closed Bug 1071482 Opened 5 years ago Closed 5 years ago

[EME] Decrypted content needs to be inaccessible from JS


(Core :: Audio/Video, defect)

Windows 8.1
Not set



Tracking Status
firefox37 --- fixed
firefox38 --- fixed
firefox39 --- fixed


(Reporter: cpearce, Assigned: cpearce)


(Blocks 1 open bug)



(5 files, 2 obsolete files)

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.
Assignee: nobody → cpearce
This makes it easier to re-use the EME setup code in other tests.
Attachment #8505212 - Flags: review?(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
Attachment #8505212 - Flags: review?(edwin) → review+
(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
Attachment #8505225 - Flags: review?(edwin)
Attachment #8505225 - Flags: review?(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).
Flags: needinfo?(cpearce)
The spec has a section with a bunch of "may"s in it: 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().
Flags: needinfo?(cpearce)
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.
Attachment #8505225 - Flags: review?(bzbarsky) → review+
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 = [];


@@ +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
Attachment #8505225 - Flags: review?(edwin) → review+
Stripped whitespace. Carry forward r=edwin.
Attachment #8505225 - Attachment is obsolete: true
Attachment #8510771 - Flags: review+
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.
Attachment #8505212 - Attachment is obsolete: true
Attachment #8510773 - Flags: review?(edwin)
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...)
Attachment #8510776 - Flags: review?(roc)
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.
Attachment #8510781 - Flags: review?(cajbir.bugzilla)
Attachment #8510781 - Flags: review?(cajbir.bugzilla) → review+
Depends on: 1090523
Backed out for causing various new intermittent failures. The timeouts are covered in bug 1090523 and the log below shows another failure mode.
Resolution: FIXED → ---
Target Milestone: mozilla36 → ---
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 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.
Attachment #8513960 - Flags: review?(edwin) → review+
Mass update firefox-status to track EME uplift.
You need to log in before you can comment on or make changes to this bug.