Closed Bug 1283325 Opened 8 years ago Closed 8 years ago

Make EME Plugins / Gecko Media Plugins storage OriginAttribute aware

Categories

(Core :: DOM: Security, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: tanvi, Assigned: timhuang)

References

(Blocks 1 open bug)

Details

(Whiteboard: [userContextId][domsecurity-active][OA])

Attachments

(2 files, 5 obsolete files)

https://dxr.mozilla.org/mozilla-central/source/dom/media/gmp/GMPServiceParent.cpp#1353

GetNodeId() should take the originSuffix as a parameter when creating a hash key for storage.
Taken from email conversation with Chris Pearce.  Included for context:

Chris:
I work on Encrypted Media Extensions/DRM video playback in Gecko. EME plugins (called Gecko Media Plugins, or GMPs for short) have per-origin persistent storage. I'm wondering if we need to make GMP storage per-container as well as per-origin?

Typically sites that use EME will have either localStorage or cookies or both as well as GMP storage, and sometimes stuff in the browser storage corresponds to stuff in the GMP storage. In other words, if the browser storage (cookies/localStorage etc) is per-container, then I expect we'll need GMP storage to be per-origin+container as well, otherwise things may get inconsistent.


---
Chris:
We segregate GMP instances and storage by a "node id", which is a randomly generated string, generated and stored per combination of (GMP name, isInPrivateBrowsing, topLevelOrigin, origin). We generate that here:

https://dxr.mozilla.org/mozilla-central/source/dom/media/gmp/GMPServiceParent.cpp#1353

$topLevelOrigin is the origin of the top-most document in the doctree, and $origin is the origin using EME. So that for example youtube.com embedded in an iframe on a.com has a different nodeId to youtube.com embedded in an iframe on b.com.

It would be pretty easy to add the container to that hash we generate for the nodeId, since IIRC the container id is an int32_t?

----
Tanvi:
Yes, it looks like you could just add userContextId as a parameter to GetNodeId() and use it when generating the hash.  But to be more general, you should probably use all OriginAttributes in the has.  Private Browsing Mode, for example, is moving from isInPrivateBrowsing to an OriginAttribute.  Other OriginAttributes may come up in the future, so your best bet is to use the originSuffix (http://searchfox.org/mozilla-central/source/caps/nsIPrincipal.idl#229), which is a string that should include all non-default OriginAttributes.
Assignee: nobody → tihuang
Status: NEW → ASSIGNED
I am wondering that could we just pass the full origin string, the origin and the suffix, for the origin of the GetNodeId() to make GMP storage per-container. And I think we should make the topLevelOrigin originAttributes aware as well. 

Tanvi, how do you think about this?
Flags: needinfo?(tanvi)
Using the full origin string sounds good.

What do topLevelOrigin and Origin represent here?

Do we need to worry about existing storage no longer being accessible, since the keys are changing?  Henri, do you know?
Flags: needinfo?(tanvi) → needinfo?(hsivonen)
(In reply to Tanvi Vyas - behind on reviews [:tanvi] from comment #5)
> Using the full origin string sounds good.
> 
> What do topLevelOrigin and Origin represent here?

'topLevelOrigin' is the origin of the URL that you see in the browser's URL bar. 'origin' is the origin using EME, which can be different if EME is running inside an iframe.

This ensures that for example youtube.com embedded in an iframe on a.com has a different nodeId to youtube.com embedded in an iframe on b.com.


> Do we need to worry about existing storage no longer being accessible, since
> the keys are changing?  Henri, do you know?

I don't think we need to worry about clearing storage here; we've done it before. We should reach out to let Adobe and Netflix know when this lands however, so they're not surprised. I can do that.

If you increment the value set in the pref media.gmp.storage.version.expected, we'll clear all storage on startup. That way we won't leave any obsolete storage left lying around.

Having said that, could we somehow make running without the user activating a specific container result in using the old storage? For example is the container "suffix" (if that's the right term) equal to 0 when we're not running in a specific container, and can we just omit adding that to the nodeId hash so that the old nodeId (and thus old storage) would be used?

That way existing GMP records written in the pre-containers world would continue to work when we use EME without a specific container being activated by the user.
(In reply to Chris Pearce (:cpearce) from comment #6)
> > Do we need to worry about existing storage no longer being accessible, since
> > the keys are changing?  Henri, do you know?
> 
> I don't think we need to worry about clearing storage here; we've done it
> before. We should reach out to let Adobe and Netflix know when this lands
> however, so they're not surprised. I can do that.
> 
> If you increment the value set in the pref
> media.gmp.storage.version.expected, we'll clear all storage on startup. That
> way we won't leave any obsolete storage left lying around.
> 
> Having said that, could we somehow make running without the user activating
> a specific container result in using the old storage? For example is the
> container "suffix" (if that's the right term) equal to 0 when we're not
> running in a specific container, and can we just omit adding that to the
> nodeId hash so that the old nodeId (and thus old storage) would be used?
> 
> That way existing GMP records written in the pre-containers world would
> continue to work when we use EME without a specific container being
> activated by the user.

Thanks for your reply Chris!  I see what you are saying in regards to storage during non-container use.  This could work.  You will face some complications down the line.  Origin Attributes are a generic infrastructure that may be used for things other than containers.  Ex: private browsing mode is moving to OriginAttributes.  So even if we special case OriginAttributes->userContextId, we will end up with unusable hash keys when OriginAttributes->isPrivateBrowsingMode is added to the suffix.  And potentially other things down the line.  This could cause multiple storage resets, which is not ideal.  So maybe we should just add the userContextId and not the whole suffix to the hashkey?  Then add additional OriginAttributes as they come up.  That isn't great for future proofing though, and making sure we don't miss an OriginAttribute we shouldn't.  I'm not sure what to do in this case.  I'll think about it more and talk to Tim about it this evening.
Flags: needinfo?(hsivonen)
Per offline meeting with Tanvi, we agreed with that using the full origin string because the origin suffix will not be appended if all values in originAttributes are default. For the default container, the originAttributes are all default, so the origin string will remain the same as before. That is to say, the original storages will not be affected by this change. 

Only the non-default value will be added as the suffix. For example, it will append "^userContextId=1" after the origin for the personal container. By doing this to generate a different key for the personal container. The same mechanism will apply to other fields of the originAttributes, including the privateBrowsingId.
Whiteboard: [userContextId][domsecurity-backlog][OA] → [userContextId][domsecurity-active][OA]
Attachment #8768054 - Attachment is obsolete: true
Comment on attachment 8771884 [details] [diff] [review]
Part 1 : Make EME Plugins / Gecko Media Plugins storage OriginAttribute aware.

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

That's a lot simpler than I thought it would be. Nice!
Attachment #8771884 - Flags: review?(cpearce) → review+
Comment on attachment 8771888 [details] [diff] [review]
Part 2 : Add a test case to test the EME is originAttributes aware or not.

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

Very nice.

::: browser/components/contextualidentity/test/browser/browser_eme.js
@@ +86,5 @@
> +add_task(function* setup() {
> +  // Make sure userContext is enabled.
> +  yield new Promise(resolve => {
> +    SpecialPowers.pushPrefEnv({"set": [
> +      ["privacy.userContext.enabled", true],

Nit:

[ "privacy.userContext.enabled", true]

missing space between '[' and '"'.

@@ +103,5 @@
> +  let keyInfo = generateKeyInfo(TESTKEY);
> +
> +  // Update the media key for the default container.
> +  let result = yield ContentTask.spawn(defaultContainer.browser, keyInfo, function* (aKeyInfo) {
> +    let access = yield content.navigator.requestMediaKeySystemAccess('org.w3.clearkey', [{initDataTypes: [aKeyInfo.initDataType]}]);

The MediaKeySystemConfiguration dictionary will also need a videoCapabilities or audioCapabilities member at some stage, otherwise when we update our navigator.requestMediaKeySystemAccess() implementation to match the spec, we'll be required to reject this request and the test will break.

You'll also need a sessionTypes member that contains 'persistent' and "persistentState: 'required'" once bug 1278198 lands.

So make the configs dict:

[{
  initDataTypes: [aKeyInfo.initDataType],
  videoCapabilities: [{contentType: 'video/webm'}],
  sessionTypes: ['persistent'],
  persistentState: 'required',
}]

If bug 1278198 lands before you do, the sessionType will need to be 'persistent-license', not 'persistent'.
Attachment #8771888 - Flags: review?(cpearce) → review+
Rebase to m-c and update the commit message.
Attachment #8772315 - Flags: review+
Attachment #8771884 - Attachment is obsolete: true
Addressing the review comment, and update the commit message. Thanks for your review, cpearce.
Attachment #8772317 - Flags: review+
Attachment #8771888 - Attachment is obsolete: true
Try looks good.
Keywords: checkin-needed
has problems to apply:

adding 1283325 to series file
renamed 1283325 -> Bug-1283325---Part-1--Make-EME-Plugins--Gecko-Medi.patch
applying Bug-1283325---Part-1--Make-EME-Plugins--Gecko-Medi.patch
patching file dom/media/eme/MediaKeys.cpp
Hunk #1 FAILED at 342
1 out of 1 hunks FAILED -- saving rejects to file dom/media/eme/MediaKeys.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh Bug-1283325---Part-1--Make-EME-Plugins--Gecko-Medi.patch
Tomcats-MacBook-Pro-2:mozilla-inbound Tomcat$
Flags: needinfo?(tihuang)
Keywords: checkin-needed
Attachment #8772315 - Attachment is obsolete: true
Flags: needinfo?(tihuang)
Modify as the comment 12 said becasue the Bug 1278198 was landed.
Attachment #8774389 - Flags: review+
Attachment #8772317 - Attachment is obsolete: true
Try looks good.
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e47092922af2
Part 1 : Make EME Plugins / Gecko Media Plugins storage OriginAttribute aware. r=cpearce
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e16fdf4bb1a
Part 2 : Add a test case to test the EME is originAttributes aware or not. r=cpearce
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e47092922af2
https://hg.mozilla.org/mozilla-central/rev/6e16fdf4bb1a
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: