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)
Core
DOM: Security
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)
2.92 KB,
patch
|
timhuang
:
review+
|
Details | Diff | Splinter Review |
7.53 KB,
patch
|
timhuang
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•8 years ago
|
||
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 | ||
Updated•8 years ago
|
Assignee: nobody → tihuang
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•8 years ago
|
||
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)
Assignee | ||
Comment 4•8 years ago
|
||
Reporter | ||
Comment 5•8 years ago
|
||
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)
Comment 6•8 years ago
|
||
(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.
Reporter | ||
Comment 7•8 years ago
|
||
(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)
Assignee | ||
Comment 8•8 years ago
|
||
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.
Updated•8 years ago
|
Whiteboard: [userContextId][domsecurity-backlog][OA] → [userContextId][domsecurity-active][OA]
Assignee | ||
Comment 9•8 years ago
|
||
Attachment #8771884 -
Flags: review?(cpearce)
Assignee | ||
Updated•8 years ago
|
Attachment #8768054 -
Attachment is obsolete: true
Assignee | ||
Comment 10•8 years ago
|
||
Attachment #8771888 -
Flags: review?(cpearce)
Comment 11•8 years ago
|
||
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 12•8 years ago
|
||
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+
Assignee | ||
Comment 13•8 years ago
|
||
Rebase to m-c and update the commit message.
Attachment #8772315 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Attachment #8771884 -
Attachment is obsolete: true
Assignee | ||
Comment 14•8 years ago
|
||
Addressing the review comment, and update the commit message. Thanks for your review, cpearce.
Attachment #8772317 -
Flags: review+
Assignee | ||
Comment 15•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9874e3a7082
Assignee | ||
Updated•8 years ago
|
Attachment #8771888 -
Attachment is obsolete: true
Comment 17•8 years ago
|
||
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
Assignee | ||
Updated•8 years ago
|
Attachment #8772315 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(tihuang)
Assignee | ||
Comment 19•8 years ago
|
||
Modify as the comment 12 said becasue the Bug 1278198 was landed.
Attachment #8774389 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Attachment #8772317 -
Attachment is obsolete: true
Assignee | ||
Comment 20•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=47957101294c
Comment 22•8 years ago
|
||
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
Comment 23•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e47092922af2 https://hg.mozilla.org/mozilla-central/rev/6e16fdf4bb1a
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in
before you can comment on or make changes to this bug.
Description
•