Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
100323004:
Update Android IPC messages to EME WD (Closed)
|
Created:
7 years ago by jrummell Modified:
7 years ago CC:
chromium-reviews, fischman+watch_chromium.org, jam, mcasas+watch_chromium.org, joi+watch-content_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, wjia+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionUpdate Android IPC messages to EME WD
EME implementation now supports EME WD, so update the Android IPC messages
to match the method names used in EME WD. Also adds in the additional
callback SessionClosed.
TEST=browser_tests for encrypted media pass
BUG=224786
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=240016
Patch Set 1 #
Total comments: 18
Patch Set 2 : Remove unnecessary parm from UpdateSession #
Total comments: 2
Patch Set 3 : Address nit #
Total comments: 16
Patch Set 4 : Add additional validations #Patch Set 5 : rebase w/session_id renames #Patch Set 6 : Add buffer size checks #
Total comments: 19
Patch Set 7 : Add constants for maximum sizes #
Messages
Total messages: 20 (0 generated)
PTAL.
lgtm % comments Please add the bug number 224786. In CL subject/description, s/CDM_3/EME WD/ or something similar. Android doesn't use CDM_3 interface at all. This CL updates Android code to use names in EME WD. https://codereview.chromium.org/100323004/diff/1/content/browser/media/androi... File content/browser/media/android/browser_media_player_manager.cc (right): https://codereview.chromium.org/100323004/diff/1/content/browser/media/androi... content/browser/media/android/browser_media_player_manager.cc:356: // retry the process once the CreateSession() is allowed to proceed drop the second "the" https://codereview.chromium.org/100323004/diff/1/content/browser/media/androi... content/browser/media/android/browser_media_player_manager.cc:393: // TODO(jrummell): Update Android calls and IPC names. do we still need this todo?
https://codereview.chromium.org/100323004/diff/1/content/browser/media/androi... File content/browser/media/android/browser_media_player_manager.cc (right): https://codereview.chromium.org/100323004/diff/1/content/browser/media/androi... content/browser/media/android/browser_media_player_manager.cc:593: const std::vector<uint8>& key, ditto x2 https://codereview.chromium.org/100323004/diff/1/content/browser/media/androi... File content/browser/media/android/browser_media_player_manager.h (right): https://codereview.chromium.org/100323004/diff/1/content/browser/media/androi... content/browser/media/android/browser_media_player_manager.h:144: const std::vector<uint8>& key, |response| https://codereview.chromium.org/100323004/diff/1/content/browser/media/androi... content/browser/media/android/browser_media_player_manager.h:145: const std::vector<uint8>& init_data); drop https://codereview.chromium.org/100323004/diff/1/content/common/media/media_p... File content/common/media/media_player_messages_android.h (right): https://codereview.chromium.org/100323004/diff/1/content/common/media/media_p... content/common/media/media_player_messages_android.h:299: std::vector<uint8> /* key */, ditto x2 https://codereview.chromium.org/100323004/diff/1/content/common/media/media_p... content/common/media/media_player_messages_android.h:309: std::string /* session_id */) web_session_id? https://codereview.chromium.org/100323004/diff/1/content/renderer/media/andro... File content/renderer/media/android/renderer_media_player_manager.cc (right): https://codereview.chromium.org/100323004/diff/1/content/renderer/media/andro... content/renderer/media/android/renderer_media_player_manager.cc:248: const std::vector<uint8>& key, ditto x2
https://codereview.chromium.org/100323004/diff/1/content/browser/media/androi... File content/browser/media/android/browser_media_player_manager.cc (right): https://codereview.chromium.org/100323004/diff/1/content/browser/media/androi... content/browser/media/android/browser_media_player_manager.cc:603: DCHECK(init_data.empty()); We should be able to drop |init_data| in this function and in the IPC message.
PTA(nother)L. +jschuh@ for IPC messages (added SessionClosed, rest are renames, removed 1 parm from UpdateSession). https://codereview.chromium.org/100323004/diff/1/content/browser/media/androi... File content/browser/media/android/browser_media_player_manager.cc (right): https://codereview.chromium.org/100323004/diff/1/content/browser/media/androi... content/browser/media/android/browser_media_player_manager.cc:356: // retry the process once the CreateSession() is allowed to proceed On 2013/12/03 17:47:48, xhwang wrote: > drop the second "the" Done. https://codereview.chromium.org/100323004/diff/1/content/browser/media/androi... content/browser/media/android/browser_media_player_manager.cc:393: // TODO(jrummell): Update Android calls and IPC names. On 2013/12/03 17:47:48, xhwang wrote: > do we still need this todo? Nope. Removed. https://codereview.chromium.org/100323004/diff/1/content/browser/media/androi... content/browser/media/android/browser_media_player_manager.cc:593: const std::vector<uint8>& key, On 2013/12/03 18:11:30, ddorwin wrote: > ditto x2 Done. https://codereview.chromium.org/100323004/diff/1/content/browser/media/androi... content/browser/media/android/browser_media_player_manager.cc:603: DCHECK(init_data.empty()); On 2013/12/03 18:21:12, xhwang wrote: > We should be able to drop |init_data| in this function and in the IPC message. Done. https://codereview.chromium.org/100323004/diff/1/content/browser/media/androi... File content/browser/media/android/browser_media_player_manager.h (right): https://codereview.chromium.org/100323004/diff/1/content/browser/media/androi... content/browser/media/android/browser_media_player_manager.h:144: const std::vector<uint8>& key, On 2013/12/03 18:11:30, ddorwin wrote: > |response| Done. https://codereview.chromium.org/100323004/diff/1/content/browser/media/androi... content/browser/media/android/browser_media_player_manager.h:145: const std::vector<uint8>& init_data); On 2013/12/03 18:11:30, ddorwin wrote: > drop Done. https://codereview.chromium.org/100323004/diff/1/content/common/media/media_p... File content/common/media/media_player_messages_android.h (right): https://codereview.chromium.org/100323004/diff/1/content/common/media/media_p... content/common/media/media_player_messages_android.h:299: std::vector<uint8> /* key */, On 2013/12/03 18:11:30, ddorwin wrote: > ditto x2 Done. https://codereview.chromium.org/100323004/diff/1/content/common/media/media_p... content/common/media/media_player_messages_android.h:309: std::string /* session_id */) On 2013/12/03 18:11:30, ddorwin wrote: > web_session_id? Done. https://codereview.chromium.org/100323004/diff/1/content/renderer/media/andro... File content/renderer/media/android/renderer_media_player_manager.cc (right): https://codereview.chromium.org/100323004/diff/1/content/renderer/media/andro... content/renderer/media/android/renderer_media_player_manager.cc:248: const std::vector<uint8>& key, On 2013/12/03 18:11:30, ddorwin wrote: > ditto x2 Done.
lgtm
lgtm % nit https://codereview.chromium.org/100323004/diff/20001/content/browser/media/an... File content/browser/media/android/browser_media_player_manager.cc (right): https://codereview.chromium.org/100323004/diff/20001/content/browser/media/an... content/browser/media/android/browser_media_player_manager.cc:356: // retry the process once CreateSession() is allowed to proceed period at the end.
+palmer@ for security review of media_player_messages_android.h Updated for nit. https://codereview.chromium.org/100323004/diff/20001/content/browser/media/an... File content/browser/media/android/browser_media_player_manager.cc (right): https://codereview.chromium.org/100323004/diff/20001/content/browser/media/an... content/browser/media/android/browser_media_player_manager.cc:356: // retry the process once CreateSession() is allowed to proceed On 2013/12/03 20:06:23, xhwang wrote: > period at the end. Done.
These IPC messages are very weakly typed. Strings and vector<uint8> must be strongly validated on the rare occasions when we have to use them. https://codereview.chromium.org/100323004/diff/40001/content/browser/media/an... File content/browser/media/android/browser_media_player_manager.cc (right): https://codereview.chromium.org/100323004/diff/40001/content/browser/media/an... content/browser/media/android/browser_media_player_manager.cc:601: drm_bridge->UpdateSession(reference_id, &response[0], response.size()); Validate the value of response.size() ? What if it's 0 or 0x7fffffff or something crazy? https://codereview.chromium.org/100323004/diff/40001/content/common/media/med... File content/common/media/media_player_messages_android.h (right): https://codereview.chromium.org/100323004/diff/40001/content/common/media/med... content/common/media/media_player_messages_android.h:287: std::vector<uint8> /* uuid */, OnInitializeCDM should verify that this is a real UUID (correct size, at least), and reject messages that don't have real UUIDs. https://codereview.chromium.org/100323004/diff/40001/content/common/media/med... content/common/media/media_player_messages_android.h:293: std::string /* type */, What is the range of values for this string? https://codereview.chromium.org/100323004/diff/40001/content/common/media/med... content/common/media/media_player_messages_android.h:294: std::vector<uint8> /* init_data */) What is in this vector? https://codereview.chromium.org/100323004/diff/40001/content/common/media/med... content/common/media/media_player_messages_android.h:299: std::vector<uint8> /* response */) What is in this vector? https://codereview.chromium.org/100323004/diff/40001/content/common/media/med... content/common/media/media_player_messages_android.h:308: std::string /* web_session_id */) Does this really have to be a string? We disprefer blob types in IPC messages. https://codereview.chromium.org/100323004/diff/40001/content/common/media/med... content/common/media/media_player_messages_android.h:313: std::vector<uint8> /* message */, What is in this vector? https://codereview.chromium.org/100323004/diff/40001/content/common/media/med... content/common/media/media_player_messages_android.h:314: std::string /* destination_url */) Use a GURL instead of a string.
Updated with additional checks. PTAL. I've opened http://crbug.com/326663 to track the parameter change from std::string to GURL (since it will affect more files). palmer@, I'm not sure what other types would be better. Feel free to add to the bug. https://codereview.chromium.org/100323004/diff/40001/content/browser/media/an... File content/browser/media/android/browser_media_player_manager.cc (right): https://codereview.chromium.org/100323004/diff/40001/content/browser/media/an... content/browser/media/android/browser_media_player_manager.cc:601: drm_bridge->UpdateSession(reference_id, &response[0], response.size()); On 2013/12/06 19:07:32, Chromium Palmer wrote: > Validate the value of response.size() ? What if it's 0 or 0x7fffffff or > something crazy? 0 is valid (although we don't currently have a use case for this). Limiting to 10K. https://codereview.chromium.org/100323004/diff/40001/content/common/media/med... File content/common/media/media_player_messages_android.h (right): https://codereview.chromium.org/100323004/diff/40001/content/common/media/med... content/common/media/media_player_messages_android.h:287: std::vector<uint8> /* uuid */, On 2013/12/06 19:07:32, Chromium Palmer wrote: > OnInitializeCDM should verify that this is a real UUID (correct size, at least), > and reject messages that don't have real UUIDs. Done. https://codereview.chromium.org/100323004/diff/40001/content/common/media/med... content/common/media/media_player_messages_android.h:293: std::string /* type */, On 2013/12/06 19:07:32, Chromium Palmer wrote: > What is the range of values for this string? It's a MIME type, possibly with codecs specified. https://codereview.chromium.org/100323004/diff/40001/content/common/media/med... content/common/media/media_player_messages_android.h:294: std::vector<uint8> /* init_data */) On 2013/12/06 19:07:32, Chromium Palmer wrote: > What is in this vector? https://dvcs.w3.org/hg/html-media/raw-file/default/encrypted-media/encrypted-... https://codereview.chromium.org/100323004/diff/40001/content/common/media/med... content/common/media/media_player_messages_android.h:299: std::vector<uint8> /* response */) On 2013/12/06 19:07:32, Chromium Palmer wrote: > What is in this vector? This is a message from the server to the decryption engine (e.g. license). https://codereview.chromium.org/100323004/diff/40001/content/common/media/med... content/common/media/media_player_messages_android.h:308: std::string /* web_session_id */) On 2013/12/06 19:07:32, Chromium Palmer wrote: > Does this really have to be a string? We disprefer blob types in IPC messages. This is going from the browser to the renderer. Is this still a concern? Is a vector better? Definition: https://dvcs.w3.org/hg/html-media/raw-file/default/encrypted-media/encrypted-... https://codereview.chromium.org/100323004/diff/40001/content/common/media/med... content/common/media/media_player_messages_android.h:313: std::vector<uint8> /* message */, On 2013/12/06 19:07:32, Chromium Palmer wrote: > What is in this vector? This is a message from the CDM to the server(renderer). https://codereview.chromium.org/100323004/diff/40001/content/common/media/med... content/common/media/media_player_messages_android.h:314: std::string /* destination_url */) On 2013/12/06 19:07:32, Chromium Palmer wrote: > Use a GURL instead of a string. Good idea. Suggest another CL to do this, as it has bigger impact than just these few files where I'm mostly renaming existing calls. Opened http://crbug.com/326663 to track this.
rebase w/session_id renames (https://codereview.chromium.org/105383002/). palmer@ -- please check media_player_messages_android.h
> > Validate the value of response.size() ? What if it's 0 or 0x7fffffff or > > something crazy? > > 0 is valid (although we don't currently have a use case for this). Limiting to > 10K. That is good. (Mostly it's about having some stated limit.) > > What is the range of values for this string? > > It's a MIME type, possibly with codecs specified. Does the recipient validate that it's a real MIME type? (I almost wonder if we should have a legit MimeType type. But that's beyond the scope of this CL, of course.) > > What is in this vector? > > https://dvcs.w3.org/hg/html-media/raw-file/default/encrypted-media/encrypted-... So we transparently pass it to the CDM, and it's the CDM's problem to interpret it? That would make sense. But, is there any sanity checking we can do, such as for size? (We don't want to let attackers control the contents of memory too much; spraying a huge amount of data can help attackers defeat ASLR, for example.) > > What is in this vector? > > This is a message from the server to the decryption engine (e.g. license). Same concerns as above. > > Does this really have to be a string? We disprefer blob types in IPC messages. > > This is going from the browser to the renderer. Is this still a concern? Yeah, weak typing is a concern always. But yes, the render does already fully trust the browser. > Is a > vector better? Definition: > https://dvcs.w3.org/hg/html-media/raw-file/default/encrypted-media/encrypted-... string is vector from a security perspective (blob of bytes). But again, is there a known range of sizes for session IDs, do they take some standard form, et c.? E.g. I normally expect something called a "Session ID" to be exactly 16 random bytes. > On 2013/12/06 19:07:32, Chromium Palmer wrote: > > What is in this vector? > > This is a message from the CDM to the server(renderer). The renderer should limit its trust in the CDM, and should verify something about the message. Size and shape of contents, magic header (is it a decrypted video frame? Does it look like a real video frame?), et c.
Updated with buffer size checks (receiving side). Also opened http://crbug.com/327449 since it appears that CreateSession/type could be an enum rather than a string (limited values supported). PTAL again.
Minor stuff. LG, but let's seem what palmer@ has to say. https://codereview.chromium.org/100323004/diff/100001/content/browser/media/a... File content/browser/media/android/browser_media_player_manager.cc (right): https://codereview.chromium.org/100323004/diff/100001/content/browser/media/a... content/browser/media/android/browser_media_player_manager.cc:550: DLOG(WARNING) << "Invalid UUID for ID: " << media_keys_id; NOTREACHED()? This is a programming bug, so I think we can just DCHECK. https://codereview.chromium.org/100323004/diff/100001/content/browser/media/a... content/browser/media/android/browser_media_player_manager.cc:565: if (type.length() > 512) { 50 is more than enough for type. https://codereview.chromium.org/100323004/diff/100001/content/browser/media/a... content/browser/media/android/browser_media_player_manager.cc:566: DLOG(WARNING) << "'type' for ID: " << media_keys_id We probably don't need to log since this should never happen. Same below. https://codereview.chromium.org/100323004/diff/100001/content/browser/media/a... content/browser/media/android/browser_media_player_manager.cc:572: if (init_data.size() > 10240) { Should we note that these are checks for security (and the sizes are arbitrary)? https://codereview.chromium.org/100323004/diff/100001/content/renderer/media/... File content/renderer/media/android/renderer_media_player_manager.cc (right): https://codereview.chromium.org/100323004/diff/100001/content/renderer/media/... content/renderer/media/android/renderer_media_player_manager.cc:284: if (destination_url.length() > 10240) { 2K is plenty for a URL.
LGTM after some straightforward tweaks. I'll rubberstamp your future CLs to move to GURLs and enums, if you like. https://codereview.chromium.org/100323004/diff/100001/content/browser/media/a... File content/browser/media/android/browser_media_player_manager.cc (right): https://codereview.chromium.org/100323004/diff/100001/content/browser/media/a... content/browser/media/android/browser_media_player_manager.cc:548: // This failure will be discovered and reported by OnCreateSession() That's not immediately obvious to me. I think it is most clear to put the protective code here, right at the IPC boundary, and then "internal" code like GetDrmBridge can proceed knowing that things are OK. (That is, assuming OnCreateSession, or this IPC boundary code generally, is the only caller of GetDrmBridge. If GetDrmBridge has other callers in other places, it will need to defend itself from them.) https://codereview.chromium.org/100323004/diff/100001/content/browser/media/a... content/browser/media/android/browser_media_player_manager.cc:550: DLOG(WARNING) << "Invalid UUID for ID: " << media_keys_id; > NOTREACHED()? > This is a programming bug, so I think we can just DCHECK. No, because you never know what kind of weirdness a compromised or insane renderer will send. Keep it as a run-time check. https://codereview.chromium.org/100323004/diff/100001/content/browser/media/a... content/browser/media/android/browser_media_player_manager.cc:572: if (init_data.size() > 10240) { > Should we note that these are checks for security (and the sizes are arbitrary)? Yes to both. I'd use some kind of named constant in place of the magic numbers. If we can't appeal to the W3C specification for limits (is that a gap in the spec?), then yes, note that they are arbitrary. https://codereview.chromium.org/100323004/diff/100001/content/browser/media/a... content/browser/media/android/browser_media_player_manager.cc:621: if (response.size() > 10240) { Same thing here, and it's another argument in favor of a named constant — we wouldn't want our arbitrary numbers to get out of sync. :) https://codereview.chromium.org/100323004/diff/100001/content/renderer/media/... File content/renderer/media/android/renderer_media_player_manager.cc (right): https://codereview.chromium.org/100323004/diff/100001/content/renderer/media/... content/renderer/media/android/renderer_media_player_manager.cc:284: if (destination_url.length() > 10240) { And use a named constant. :) There might be a standard one already defined?
https://codereview.chromium.org/100323004/diff/100001/content/browser/media/a... File content/browser/media/android/browser_media_player_manager.cc (right): https://codereview.chromium.org/100323004/diff/100001/content/browser/media/a... content/browser/media/android/browser_media_player_manager.cc:550: DLOG(WARNING) << "Invalid UUID for ID: " << media_keys_id; On 2013/12/11 00:18:18, Chromium Palmer wrote: > > NOTREACHED()? > > This is a programming bug, so I think we can just DCHECK. > > No, because you never know what kind of weirdness a compromised or insane > renderer will send. Keep it as a run-time check. Right, keep the runtime check at 547, but NOTREACHED (a DCHECK) instead of DLOG at 550. Basically, in debug builds, this programming error is fatal. https://codereview.chromium.org/100323004/diff/100001/content/browser/media/a... content/browser/media/android/browser_media_player_manager.cc:572: if (init_data.size() > 10240) { On 2013/12/11 00:18:18, Chromium Palmer wrote: > > Should we note that these are checks for security (and the sizes are > arbitrary)? > > Yes to both. I'd use some kind of named constant in place of the magic numbers. > If we can't appeal to the W3C specification for limits (is that a gap in the > spec?), then yes, note that they are arbitrary. initData is container-specific and the messages and response values are key system-specific, so there's not pre-determined limits. We could say something like these parameters must be smaller than 50 KB, but that's arbitrary and someone is likely to complain. Did you have something else in mind?
> Right, keep the runtime check at 547, but NOTREACHED (a DCHECK) instead of DLOG > at 550. Basically, in debug builds, this programming error is fatal. Ohh. Right. > initData is container-specific and the messages and response values are key > system-specific, so there's not pre-determined limits. We could say something > like these parameters must be smaller than 50 KB, but that's arbitrary and > someone is likely to complain. Did you have something else in mind? Nope, nothing else in mind. As long as it's not like 100 MiB it is fine by me at this point.
Thanks for the reviews, starting CQ. If there are any remaining tweaks I will get them in the next set of changes. https://codereview.chromium.org/100323004/diff/100001/content/browser/media/a... File content/browser/media/android/browser_media_player_manager.cc (right): https://codereview.chromium.org/100323004/diff/100001/content/browser/media/a... content/browser/media/android/browser_media_player_manager.cc:548: // This failure will be discovered and reported by OnCreateSession() On 2013/12/11 00:18:18, Chromium Palmer wrote: > That's not immediately obvious to me. I think it is most clear to put the > protective code here, right at the IPC boundary, and then "internal" code like > GetDrmBridge can proceed knowing that things are OK. (That is, assuming > OnCreateSession, or this IPC boundary code generally, is the only caller of > GetDrmBridge. If GetDrmBridge has other callers in other places, it will need to > defend itself from them.) Errors can only be thrown at sessions, so this initialization code has nowhere to report to. I just copied what AddDrmBridge() does for failures. We already have the code to check this in every place it is used. https://codereview.chromium.org/100323004/diff/100001/content/browser/media/a... content/browser/media/android/browser_media_player_manager.cc:550: DLOG(WARNING) << "Invalid UUID for ID: " << media_keys_id; On 2013/12/11 00:30:04, ddorwin wrote: > On 2013/12/11 00:18:18, Chromium Palmer wrote: > > > NOTREACHED()? > > > This is a programming bug, so I think we can just DCHECK. > > > > No, because you never know what kind of weirdness a compromised or insane > > renderer will send. Keep it as a run-time check. > > Right, keep the runtime check at 547, but NOTREACHED (a DCHECK) instead of DLOG > at 550. Basically, in debug builds, this programming error is fatal. Done. https://codereview.chromium.org/100323004/diff/100001/content/browser/media/a... content/browser/media/android/browser_media_player_manager.cc:565: if (type.length() > 512) { On 2013/12/10 23:48:08, ddorwin wrote: > 50 is more than enough for type. Done. https://codereview.chromium.org/100323004/diff/100001/content/browser/media/a... content/browser/media/android/browser_media_player_manager.cc:566: DLOG(WARNING) << "'type' for ID: " << media_keys_id On 2013/12/10 23:48:08, ddorwin wrote: > We probably don't need to log since this should never happen. Same below. Done. https://codereview.chromium.org/100323004/diff/100001/content/browser/media/a... content/browser/media/android/browser_media_player_manager.cc:572: if (init_data.size() > 10240) { On 2013/12/11 00:18:18, Chromium Palmer wrote: > > Should we note that these are checks for security (and the sizes are > arbitrary)? > > Yes to both. I'd use some kind of named constant in place of the magic numbers. > If we can't appeal to the W3C specification for limits (is that a gap in the > spec?), then yes, note that they are arbitrary. Done. https://codereview.chromium.org/100323004/diff/100001/content/browser/media/a... content/browser/media/android/browser_media_player_manager.cc:621: if (response.size() > 10240) { On 2013/12/11 00:18:18, Chromium Palmer wrote: > Same thing here, and it's another argument in favor of a named constant — we > wouldn't want our arbitrary numbers to get out of sync. :) Done. https://codereview.chromium.org/100323004/diff/100001/content/renderer/media/... File content/renderer/media/android/renderer_media_player_manager.cc (right): https://codereview.chromium.org/100323004/diff/100001/content/renderer/media/... content/renderer/media/android/renderer_media_player_manager.cc:284: if (destination_url.length() > 10240) { On 2013/12/11 00:18:18, Chromium Palmer wrote: > And use a named constant. :) There might be a standard one already defined? Done. Can't find a constant in GURL with a max size.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jrummell@chromium.org/100323004/120001
Message was sent while issue was closed.
Change committed as 240016 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
