Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10535029:
Add support for encrypted WebM files as defined in the RFC. (Closed)
|
Created:
8 years, 6 months ago by fgalligan1 Modified:
8 years, 5 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, jshin+watch_chromium.org, strobe_ Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionAdd support for encrypted WebM files as defined in the RFC.
The WebM parser now reads the HMAC and IV from every encrypted
Block and stores that information in the DecryptorConfig object.
Added two new elements ContentEncAESSettings and
AESSettingsCipherMode used by encrypted WebM files.
BUG=119845
TEST=media_unittests
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=147169
Committed as https://chromiumcodereview.appspot.com/10810026
Patch Set 1 #
Total comments: 14
Patch Set 2 : Added support for RFC encrypted WebM to decryptor. #Patch Set 3 : Rebase PS2 onto master. #Patch Set 4 : Add OVERRIDE and fix a few issues. #Patch Set 5 : Add more OVERRIDEs. #Patch Set 6 : Add tests for encrypted WebM RFC. #Patch Set 7 : Moved decryptor from media/base to media/crypto. #Patch Set 8 : Updated encrypted WebM test data. #
Total comments: 22
Patch Set 9 : Fix DecodeEncryptedFrame_WrongKey test. #
Total comments: 10
Patch Set 10 : Addressing comments in the CL. #
Total comments: 6
Patch Set 11 : Rebase to master #
Total comments: 122
Patch Set 12 : Addressing comments in the CL. #
Total comments: 47
Patch Set 13 : Addressing comments from Patch Set 12. #
Total comments: 23
Patch Set 14 : Addressing comments from Patch Set 13. #
Total comments: 6
Patch Set 15 : Addressing comments from Patch Set 14. #
Total comments: 2
Patch Set 16 : Rebase to master #Patch Set 17 : Fix uint64 literal. #
Messages
Total messages: 28 (0 generated)
Some preliminary comments. https://chromiumcodereview.appspot.com/10535029/diff/1/media/webm/webm_cluste... File media/webm/webm_cluster_parser.cc (right): https://chromiumcodereview.appspot.com/10535029/diff/1/media/webm/webm_cluste... media/webm/webm_cluster_parser.cc:189: (cluster_timecode_ + timecode) * timecode_multiplier_); I remember scherkus@ told me that in media stack code, we don't use const so proactively. It's mostly used in const-ref parameter passing. https://chromiumcodereview.appspot.com/10535029/diff/1/media/webm/webm_cluste... File media/webm/webm_cluster_parser.h (right): https://chromiumcodereview.appspot.com/10535029/diff/1/media/webm/webm_cluste... media/webm/webm_cluster_parser.h:22: // The sizes are from the WebM encrypted specification. Do we have a public link to the spec to post here? https://chromiumcodereview.appspot.com/10535029/diff/1/media/webm/webm_cluste... media/webm/webm_cluster_parser.h:24: static const int kIVSize = 8; s/kIVSize/kIvSize https://chromiumcodereview.appspot.com/10535029/diff/1/media/webm/webm_consta... File media/webm/webm_constants.h (right): https://chromiumcodereview.appspot.com/10535029/diff/1/media/webm/webm_consta... media/webm/webm_constants.h:15: const int kWebMIdAESSettingsCipherMode = 0x47E8; kWebMIdAesSettingsCipherMode https://chromiumcodereview.appspot.com/10535029/diff/1/media/webm/webm_consta... media/webm/webm_constants.h:66: const int kWebMIdContentEncAESSettings = 0x47E7; ditto s/AES/Aes/ https://chromiumcodereview.appspot.com/10535029/diff/1/media/webm/webm_conten... File media/webm/webm_content_encodings.h (right): https://chromiumcodereview.appspot.com/10535029/diff/1/media/webm/webm_conten... media/webm/webm_content_encodings.h:47: kCipherModeCTR = 1, s/CTR/Ctr https://chromiumcodereview.appspot.com/10535029/diff/1/media/webm/webm_parser.cc File media/webm/webm_parser.cc (right): https://chromiumcodereview.appspot.com/10535029/diff/1/media/webm/webm_parser... media/webm/webm_parser.cc:10: Can we also post the RFC here?
https://chromiumcodereview.appspot.com/10535029/diff/1/media/webm/webm_cluste... File media/webm/webm_cluster_parser.cc (right): https://chromiumcodereview.appspot.com/10535029/diff/1/media/webm/webm_cluste... media/webm/webm_cluster_parser.cc:189: (cluster_timecode_ + timecode) * timecode_multiplier_); On 2012/06/06 20:54:06, xhwang wrote: > I remember scherkus@ told me that in media stack code, we don't use const so > proactively. It's mostly used in const-ref parameter passing. Thanks for the info. Done. https://chromiumcodereview.appspot.com/10535029/diff/1/media/webm/webm_cluste... File media/webm/webm_cluster_parser.h (right): https://chromiumcodereview.appspot.com/10535029/diff/1/media/webm/webm_cluste... media/webm/webm_cluster_parser.h:22: // The sizes are from the WebM encrypted specification. On 2012/06/06 20:54:06, xhwang wrote: > Do we have a public link to the spec to post here? Done. https://chromiumcodereview.appspot.com/10535029/diff/1/media/webm/webm_cluste... media/webm/webm_cluster_parser.h:24: static const int kIVSize = 8; On 2012/06/06 20:54:06, xhwang wrote: > s/kIVSize/kIvSize Done. https://chromiumcodereview.appspot.com/10535029/diff/1/media/webm/webm_consta... File media/webm/webm_constants.h (right): https://chromiumcodereview.appspot.com/10535029/diff/1/media/webm/webm_consta... media/webm/webm_constants.h:15: const int kWebMIdAESSettingsCipherMode = 0x47E8; On 2012/06/06 20:54:06, xhwang wrote: > kWebMIdAesSettingsCipherMode AES follows the convention in this file of matching the webm element capitalization. https://chromiumcodereview.appspot.com/10535029/diff/1/media/webm/webm_consta... media/webm/webm_constants.h:66: const int kWebMIdContentEncAESSettings = 0x47E7; On 2012/06/06 20:54:06, xhwang wrote: > ditto s/AES/Aes/ ditto. https://chromiumcodereview.appspot.com/10535029/diff/1/media/webm/webm_conten... File media/webm/webm_content_encodings.h (right): https://chromiumcodereview.appspot.com/10535029/diff/1/media/webm/webm_conten... media/webm/webm_content_encodings.h:47: kCipherModeCTR = 1, On 2012/06/06 20:54:06, xhwang wrote: > s/CTR/Ctr Done. https://chromiumcodereview.appspot.com/10535029/diff/1/media/webm/webm_parser.cc File media/webm/webm_parser.cc (right): https://chromiumcodereview.appspot.com/10535029/diff/1/media/webm/webm_parser... media/webm/webm_parser.cc:10: On 2012/06/06 20:54:06, xhwang wrote: > Can we also post the RFC here? Done.
http://codereview.chromium.org/10535029/diff/2002/media/base/decrypt_config.h File media/base/decrypt_config.h (right): http://codereview.chromium.org/10535029/diff/2002/media/base/decrypt_config.h... media/base/decrypt_config.h:19: // http://wiki.webmproject.org/encryption/webm-encryption-rfc. Remove the trailing "." as people may also copy that as part of the URL. http://codereview.chromium.org/10535029/diff/2002/media/base/decrypt_config.h... media/base/decrypt_config.h:22: static const int kWebMIntegrityCheckSize = 12; If we pass the whole encrypted frame (with prepending hmac and iv) to the decryptor, we don't need to expose these here: we can hide them in the HmacAesDecryptor. The DecryptConfig should contain data that is needed for decryption but not readily available in the encrypted frame. Also, we try to limit WebM specific stuff in the media/webm/ folder. http://codereview.chromium.org/10535029/diff/2002/media/crypto/hmac_aes_decry... File media/crypto/hmac_aes_decryptor.h (right): http://codereview.chromium.org/10535029/diff/2002/media/crypto/hmac_aes_decry... media/crypto/hmac_aes_decryptor.h:26: class MEDIA_EXPORT HmacAesDecryptor : public Decryptor { With the new HmacAesDecryptor, we don't need to keep the old AesDecryptor, because nobody is going to use it anymore. In that case, we don't need this Decryptor base class and can just keep Decryptor methods within HmacAesDecryptor. Also, it'll make it easier for review to keep the name AesDecryptor for now, and rename it later, either before commit, or in a separate CL. http://codereview.chromium.org/10535029/diff/2002/media/webm/webm_cluster_par... File media/webm/webm_cluster_parser.cc (right): http://codereview.chromium.org/10535029/diff/2002/media/webm/webm_cluster_par... media/webm/webm_cluster_parser.cc:208: const uint64 iv = base::NetToHost64(network_iv); Can we hide all these in the HmacAesDecryptor? 1, We are passing the whole encrypted frame with prepending HMAC and IV to the decryptor anyway. (Actually we have to do this because we need the "iv_frame" for hmac checking.) 2, The demuxer and media stack doesn't need to know about these details. 3, The HmacAesDecryptor should be able to work well on the encrypted frame. http://codereview.chromium.org/10535029/diff/2002/webkit/media/filter_helpers.h File webkit/media/filter_helpers.h (right): http://codereview.chromium.org/10535029/diff/2002/webkit/media/filter_helpers... webkit/media/filter_helpers.h:47: media::Decryptor* decryptor, We can keep the use of AesDecryptor (HmacAesDecryptor after renaming) as is if we don't add a new Decryptor (see other comment about this). FYI, I am also working on a CL that defines the Decryptor interface and replaces AesDecryptor with Decryptor in many decryptor access points: http://codereview.chromium.org/10539150/ I didn't do this earlier because in media stack code, if we only have one implementation, then we don't play the interface-implementation game. I am doing this now because we'll have a CdmDecryptor very soon. http://codereview.chromium.org/10535029/diff/14001/media/crypto/hmac_aes_decr... File media/crypto/hmac_aes_decryptor_unittest.cc (right): http://codereview.chromium.org/10535029/diff/14001/media/crypto/hmac_aes_decr... media/crypto/hmac_aes_decryptor_unittest.cc:37: "\xfb\xe7\x1d\xbb\x4c\x23\xce\xba\xcc\xf8\xda\xc0\xff\xff\xff\xff" FYI, I am switching to {0x00, 0x11, ...} style for binary array definition: http://codereview.chromium.org/10534096/diff/2003/media/crypto/aes_decryptor_... I switched mainly to avoid the "arraysize(array) - 1" hassle and not need to specify the size all the time (see comment below). Also, I found this is used more commonly: http://code.google.com/searchframe#OAMlx_jo-ck/src/crypto/encryptor_unittest.... http://code.google.com/searchframe#OAMlx_jo-ck/src/media/webm/webm_cluster_pa... http://codereview.chromium.org/10535029/diff/14001/media/crypto/hmac_aes_decr... media/crypto/hmac_aes_decryptor_unittest.cc:117: const uint8* key_id, int key_id_size) { Since data, plain_text and key_id are all arrays and this function is only used in this test, we can use template to derive array size automatically: template <int KeyIdSize, int KeySize> void AddKeyAndExpectToSucceed(const uint8 (&key_id)[KeyIdSize], const uint8 (&key)[KeySize]) { decryptor_.AddKey(kClearKeySystem, key, KeySize, key_id, KeyIdSize, session_id_string_); } Note that if the arrays are defined using string literal ("\x**"), we have to use "KeySize - 1". That's another reason I switched to { 0x** } for defining arrays. http://codereview.chromium.org/10535029/diff/14001/media/filters/ffmpeg_video... File media/filters/ffmpeg_video_decoder_unittest.cc (right): http://codereview.chromium.org/10535029/diff/14001/media/filters/ffmpeg_video... media/filters/ffmpeg_video_decoder_unittest.cc:48: "\xeb\x1c\xa5\x2e\x51\x27\xd0\x83\xa2\xad\x38\x80"; I suppose this kHmac should match the hmac in the encrypted frame? This seems another case of redundancy. If we just hide all iv/hmac processing code in HmacAesDecryptor, we don't need to specify them here. http://codereview.chromium.org/10535029/diff/14001/media/filters/pipeline_int... File media/filters/pipeline_integration_test.cc (right): http://codereview.chromium.org/10535029/diff/14001/media/filters/pipeline_int... media/filters/pipeline_integration_test.cc:16: "\x5b\x4e\xe8\xb6\xd0\x7e\x4e\x58\xea\x24\x4c\x40\x13\xfd\xb5\x2d"; ditto about using { 0x5b, 0x4e, ... } style. But I can also do that later when I land my patch :) http://codereview.chromium.org/10535029/diff/14001/media/webm/webm_parser.cc File media/webm/webm_parser.cc (right): http://codereview.chromium.org/10535029/diff/14001/media/webm/webm_parser.cc#... media/webm/webm_parser.cc:12: // http://wiki.webmproject.org/encryption/webm-encryption-rfc. ditto, remove trailing "."
Mainly high-level comments after discussions with xhwang. I didn't review much of the code yet. http://codereview.chromium.org/10535029/diff/2002/media/base/decrypt_config.h File media/base/decrypt_config.h (right): http://codereview.chromium.org/10535029/diff/2002/media/base/decrypt_config.h... media/base/decrypt_config.h:22: static const int kWebMIntegrityCheckSize = 12; On 2012/06/14 19:42:27, xhwang wrote: > If we pass the whole encrypted frame (with prepending hmac and iv) to the > decryptor, we don't need to expose these here: we can hide them in the > HmacAesDecryptor. The DecryptConfig should contain data that is needed for > decryption but not readily available in the encrypted frame. > > Also, we try to limit WebM specific stuff in the media/webm/ folder. This class should be container-independent. We want all the container stuff in the demuxer because we want the decryptors to be container-independent (even if they have decoders). All of these values are WebM-specific (though CENC uses 128-bit keys too). http://codereview.chromium.org/10535029/diff/2002/media/base/decrypt_config.h... media/base/decrypt_config.h:24: DecryptConfig(const uint8* hmac, int hmac_size, CENC (and other containers) do not support HMAC, so we probably need something closer to the current AesDecryptor. Preferably, we can do this with just if statements (and method extraction, if necessary). We can specify that hmac_size==0 means the container does not support it (or one was not present in the stream?) http://codereview.chromium.org/10535029/diff/2002/media/base/decrypt_config.h... media/base/decrypt_config.h:25: uint64 iv, In CENC, the IV_size can vary and be as large as 16 bytes. I suggest we just support longer IVs (and specifying size) now. Unless there is a 128 type, switch to pointer and size now; that also has the advantage of all parameters being consistent. http://codereview.chromium.org/10535029/diff/2002/media/crypto/hmac_aes_decry... File media/crypto/hmac_aes_decryptor.cc (right): http://codereview.chromium.org/10535029/diff/2002/media/crypto/hmac_aes_decry... media/crypto/hmac_aes_decryptor.cc:67: const char* iv_frame = reinterpret_cast<const char*>(input.GetData() + Is "iv_frame" really "iv_and_frame"? As mentioned in the webm_cluster_parser, data_to_verify might be better. http://codereview.chromium.org/10535029/diff/2002/media/crypto/hmac_aes_decry... media/crypto/hmac_aes_decryptor.cc:68: DecryptConfig::kWebMIntegrityCheckSize); Would like to avoid any WebM-specific values in the decryptor. http://codereview.chromium.org/10535029/diff/2002/media/crypto/hmac_aes_decry... media/crypto/hmac_aes_decryptor.cc:71: std::string data_to_check(iv_frame, iv_frame_size); Can we avoid this memory copy? (If we separate IV and the frame in the parameters, that may not be possible.) http://codereview.chromium.org/10535029/diff/2002/media/crypto/hmac_aes_decry... media/crypto/hmac_aes_decryptor.cc:148: if (!CheckData(*encrypted, keys->hmac_key())) { Support containers without HMAC support: if (HasHmac() && !CheckData... http://codereview.chromium.org/10535029/diff/2002/media/crypto/hmac_aes_decry... File media/crypto/hmac_aes_decryptor.h (right): http://codereview.chromium.org/10535029/diff/2002/media/crypto/hmac_aes_decry... media/crypto/hmac_aes_decryptor.h:26: class MEDIA_EXPORT HmacAesDecryptor : public Decryptor { On 2012/06/14 19:42:27, xhwang wrote: > With the new HmacAesDecryptor, we don't need to keep the old AesDecryptor, > because nobody is going to use it anymore. In that case, we don't need this > Decryptor base class and can just keep Decryptor methods within > HmacAesDecryptor. > > Also, it'll make it easier for review to keep the name AesDecryptor for now, and > rename it later, either before commit, or in a separate CL. I think we should just keep the name AesDecryptor. Certainly for the code review, but also because HMAC support should be an optional feature. We don't want to have too much logic related to whether an HMAC is supported or not, and the media stack shouldn't need to decide which to create. As an example, we will create the same CDM regardless of the container. http://codereview.chromium.org/10535029/diff/2002/media/webm/webm_cluster_par... File media/webm/webm_cluster_parser.cc (right): http://codereview.chromium.org/10535029/diff/2002/media/webm/webm_cluster_par... media/webm/webm_cluster_parser.cc:208: const uint64 iv = base::NetToHost64(network_iv); On 2012/06/14 19:42:27, xhwang wrote: > Can we hide all these in the HmacAesDecryptor? > > 1, We are passing the whole encrypted frame with prepending HMAC and IV to the > decryptor anyway. (Actually we have to do this because we need the "iv_frame" > for hmac checking.) > 2, The demuxer and media stack doesn't need to know about these details. > 3, The HmacAesDecryptor should be able to work well on the encrypted frame. As mentioned earlier, we want DecryptConfig to be container-independent. Unfortunately, I see the IV is verified as part of the HMAC, which makes separation difficult. Maybe we can hide this by having char* data_to_verify and an int offset_to_frame. That way, it's not known to the decryptor what exactly is before the frame. We would still pass the iv separately.
Is there an RFC version or date you can reference in the title or description? I assume there will be changes, so it would be nice to document what version this is referring to.
Just some nits from reading through comments as I'm working on ppapi CDM stuff. https://chromiumcodereview.appspot.com/10535029/diff/4005/media/crypto/aes_de... File media/crypto/aes_decryptor.cc (right): https://chromiumcodereview.appspot.com/10535029/diff/4005/media/crypto/aes_de... media/crypto/aes_decryptor.cc:17: // Derives a key from an HAMC using SHA1. |secret| is the base secret to derive s/HAMC/HMAC https://chromiumcodereview.appspot.com/10535029/diff/4005/media/crypto/aes_de... media/crypto/aes_decryptor.cc:18: // the key from. |seed| is the knwon input to the HMAC. |key_size| is how many s/knwon/known https://chromiumcodereview.appspot.com/10535029/diff/4005/media/crypto/aes_de... media/crypto/aes_decryptor.cc:46: // |kWebMIntegrityCheckSize| in bytes of |ipunt| matches the rest of the data s/ipunt/input
https://chromiumcodereview.appspot.com/10535029/diff/2002/media/base/decrypt_... File media/base/decrypt_config.h (right): https://chromiumcodereview.appspot.com/10535029/diff/2002/media/base/decrypt_... media/base/decrypt_config.h:19: // http://wiki.webmproject.org/encryption/webm-encryption-rfc. On 2012/06/14 19:42:27, xhwang wrote: > Remove the trailing "." as people may also copy that as part of the URL. Done. https://chromiumcodereview.appspot.com/10535029/diff/2002/media/base/decrypt_... media/base/decrypt_config.h:25: uint64 iv, On 2012/06/14 21:41:24, ddorwin wrote: > In CENC, the IV_size can vary and be as large as 16 bytes. I suggest we just > support longer IVs (and specifying size) now. Unless there is a 128 type, switch > to pointer and size now; that also has the advantage of all parameters being > consistent. Done. https://chromiumcodereview.appspot.com/10535029/diff/2002/media/crypto/hmac_a... File media/crypto/hmac_aes_decryptor.cc (right): https://chromiumcodereview.appspot.com/10535029/diff/2002/media/crypto/hmac_a... media/crypto/hmac_aes_decryptor.cc:67: const char* iv_frame = reinterpret_cast<const char*>(input.GetData() + On 2012/06/14 21:41:24, ddorwin wrote: > Is "iv_frame" really "iv_and_frame"? As mentioned in the webm_cluster_parser, > data_to_verify might be better. I only send the IV and frame data in the input buffer now. https://chromiumcodereview.appspot.com/10535029/diff/2002/media/crypto/hmac_a... media/crypto/hmac_aes_decryptor.cc:68: DecryptConfig::kWebMIntegrityCheckSize); On 2012/06/14 21:41:24, ddorwin wrote: > Would like to avoid any WebM-specific values in the decryptor. Done. https://chromiumcodereview.appspot.com/10535029/diff/2002/media/crypto/hmac_a... media/crypto/hmac_aes_decryptor.cc:71: std::string data_to_check(iv_frame, iv_frame_size); On 2012/06/14 21:41:24, ddorwin wrote: > Can we avoid this memory copy? > (If we separate IV and the frame in the parameters, that may not be possible.) Done. https://chromiumcodereview.appspot.com/10535029/diff/2002/media/crypto/hmac_a... media/crypto/hmac_aes_decryptor.cc:148: if (!CheckData(*encrypted, keys->hmac_key())) { On 2012/06/14 21:41:24, ddorwin wrote: > Support containers without HMAC support: > if (HasHmac() && !CheckData... Done. https://chromiumcodereview.appspot.com/10535029/diff/2002/media/crypto/hmac_a... File media/crypto/hmac_aes_decryptor.h (right): https://chromiumcodereview.appspot.com/10535029/diff/2002/media/crypto/hmac_a... media/crypto/hmac_aes_decryptor.h:26: class MEDIA_EXPORT HmacAesDecryptor : public Decryptor { On 2012/06/14 21:41:24, ddorwin wrote: > On 2012/06/14 19:42:27, xhwang wrote: > > With the new HmacAesDecryptor, we don't need to keep the old AesDecryptor, > > because nobody is going to use it anymore. In that case, we don't need this > > Decryptor base class and can just keep Decryptor methods within > > HmacAesDecryptor. > > > > Also, it'll make it easier for review to keep the name AesDecryptor for now, > and > > rename it later, either before commit, or in a separate CL. > > I think we should just keep the name AesDecryptor. Certainly for the code > review, but also because HMAC support should be an optional feature. We don't > want to have too much logic related to whether an HMAC is supported or not, and > the media stack shouldn't need to decide which to create. As an example, we will > create the same CDM regardless of the container. The media stack cannot create the same CDM for CENC and WebM with the only difference being CENC does not have an HMAC. The problem is WebM derives the encryption key and the HAMC key from the base secret (I.e. the key) with a very specific algorithm. This was the main reason I created this decryptor https://chromiumcodereview.appspot.com/10535029/diff/2002/media/webm/webm_clu... File media/webm/webm_cluster_parser.cc (right): https://chromiumcodereview.appspot.com/10535029/diff/2002/media/webm/webm_clu... media/webm/webm_cluster_parser.cc:208: const uint64 iv = base::NetToHost64(network_iv); On 2012/06/14 21:41:24, ddorwin wrote: > On 2012/06/14 19:42:27, xhwang wrote: > > Can we hide all these in the HmacAesDecryptor? > > > > 1, We are passing the whole encrypted frame with prepending HMAC and IV to the > > decryptor anyway. (Actually we have to do this because we need the "iv_frame" > > for hmac checking.) > > 2, The demuxer and media stack doesn't need to know about these details. > > 3, The HmacAesDecryptor should be able to work well on the encrypted frame. > > As mentioned earlier, we want DecryptConfig to be container-independent. > Unfortunately, I see the IV is verified as part of the HMAC, which makes > separation difficult. Maybe we can hide this by having char* data_to_verify and > an int offset_to_frame. That way, it's not known to the decryptor what exactly > is before the frame. We would still pass the iv separately. So now I'm passing the IV and the frame data. I put the HMAC into a data_to_verify member. I also set an offset into the data in the config. I still can't generalize the decryptor because of the what I said in the other comment. https://chromiumcodereview.appspot.com/10535029/diff/14001/media/crypto/hmac_... File media/crypto/hmac_aes_decryptor_unittest.cc (right): https://chromiumcodereview.appspot.com/10535029/diff/14001/media/crypto/hmac_... media/crypto/hmac_aes_decryptor_unittest.cc:37: "\xfb\xe7\x1d\xbb\x4c\x23\xce\xba\xcc\xf8\xda\xc0\xff\xff\xff\xff" On 2012/06/14 19:42:27, xhwang wrote: > FYI, I am switching to {0x00, 0x11, ...} style for binary array definition: > http://codereview.chromium.org/10534096/diff/2003/media/crypto/aes_decryptor_... > > I switched mainly to avoid the "arraysize(array) - 1" hassle and not need to > specify the size all the time (see comment below). > > Also, I found this is used more commonly: > http://code.google.com/searchframe#OAMlx_jo-ck/src/crypto/encryptor_unittest.... > http://code.google.com/searchframe#OAMlx_jo-ck/src/media/webm/webm_cluster_pa... Done. https://chromiumcodereview.appspot.com/10535029/diff/14001/media/crypto/hmac_... media/crypto/hmac_aes_decryptor_unittest.cc:117: const uint8* key_id, int key_id_size) { On 2012/06/14 19:42:27, xhwang wrote: > Since data, plain_text and key_id are all arrays and this function is only used > in this test, we can use template to derive array size automatically: > > template <int KeyIdSize, int KeySize> > void AddKeyAndExpectToSucceed(const uint8 (&key_id)[KeyIdSize], > const uint8 (&key)[KeySize]) { > decryptor_.AddKey(kClearKeySystem, key, KeySize, key_id, KeyIdSize, > session_id_string_); > } > > Note that if the arrays are defined using string literal ("\x**"), we have to > use "KeySize - 1". That's another reason I switched to { 0x** } for defining > arrays. I can't use the template function here because it will be the size of the reserved memory instead of what I initialize the arrays too. https://chromiumcodereview.appspot.com/10535029/diff/14001/media/filters/ffmp... File media/filters/ffmpeg_video_decoder_unittest.cc (right): https://chromiumcodereview.appspot.com/10535029/diff/14001/media/filters/ffmp... media/filters/ffmpeg_video_decoder_unittest.cc:48: "\xeb\x1c\xa5\x2e\x51\x27\xd0\x83\xa2\xad\x38\x80"; On 2012/06/14 19:42:27, xhwang wrote: > I suppose this kHmac should match the hmac in the encrypted frame? This seems > another case of redundancy. If we just hide all iv/hmac processing code in > HmacAesDecryptor, we don't need to specify them here. Correct. https://chromiumcodereview.appspot.com/10535029/diff/14001/media/filters/pipe... File media/filters/pipeline_integration_test.cc (right): https://chromiumcodereview.appspot.com/10535029/diff/14001/media/filters/pipe... media/filters/pipeline_integration_test.cc:16: "\x5b\x4e\xe8\xb6\xd0\x7e\x4e\x58\xea\x24\x4c\x40\x13\xfd\xb5\x2d"; On 2012/06/14 19:42:27, xhwang wrote: > ditto about using { 0x5b, 0x4e, ... } style. But I can also do that later when I > land my patch :) Done. https://chromiumcodereview.appspot.com/10535029/diff/14001/media/webm/webm_pa... File media/webm/webm_parser.cc (right): https://chromiumcodereview.appspot.com/10535029/diff/14001/media/webm/webm_pa... media/webm/webm_parser.cc:12: // http://wiki.webmproject.org/encryption/webm-encryption-rfc. On 2012/06/14 19:42:27, xhwang wrote: > ditto, remove trailing "." Done. https://chromiumcodereview.appspot.com/10535029/diff/4005/media/crypto/aes_de... File media/crypto/aes_decryptor.cc (right): https://chromiumcodereview.appspot.com/10535029/diff/4005/media/crypto/aes_de... media/crypto/aes_decryptor.cc:17: // Derives a key from an HAMC using SHA1. |secret| is the base secret to derive On 2012/06/15 15:09:08, tomf wrote: > s/HAMC/HMAC Done. https://chromiumcodereview.appspot.com/10535029/diff/4005/media/crypto/aes_de... media/crypto/aes_decryptor.cc:18: // the key from. |seed| is the knwon input to the HMAC. |key_size| is how many On 2012/06/15 15:09:08, tomf wrote: > s/knwon/known Done. https://chromiumcodereview.appspot.com/10535029/diff/4005/media/crypto/aes_de... media/crypto/aes_decryptor.cc:46: // |kWebMIntegrityCheckSize| in bytes of |ipunt| matches the rest of the data On 2012/06/15 15:09:08, tomf wrote: > s/ipunt/input Done.
Reviewed the first 4 files. More tomorrow. Some of the comments relate to how we might generalize this to also support ISO. We can address those later, but we do need to make sure we're clear about what is WebM-specific in the current implementation. http://codereview.chromium.org/10535029/diff/17001/media/base/decrypt_config.h File media/base/decrypt_config.h (right): http://codereview.chromium.org/10535029/diff/17001/media/base/decrypt_config.... media/base/decrypt_config.h:17: DecryptConfig(const uint8* data_to_verify, int data_to_verify_size, It's not obvious what's being passed here. I think a comment block explaining the values would be helpful. Also, maybe move these after key_id_size so all the "data"/offset values are together. It's probably worth noting that this class does not take ownership of the pointers / copies the data. (I initially thought it did take ownership of them based on the member types below.) http://codereview.chromium.org/10535029/diff/17001/media/base/decrypt_config.... media/base/decrypt_config.h:20: int offset_to_data); offset_to_frame? encrypted_data_offset? It's unclear what this means and how it relates to the first parameter http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor.cc File media/crypto/aes_decryptor.cc (right): http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:22: // Derives a key from an HMAC using SHA1. |secret| is the base secret to derive s/from an HMAC using SHA1/using a SHA1 HMAC/ ? http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:23: // the key from. |seed| is the kwnon input to the HMAC. |key_size| is how many known http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:38: return key; It might be more readable to just return an empty string in the errors. There's no partially correct key to return. You can also remove line 33 to line 46. http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:46: key.assign(reinterpret_cast<const char*>(calculated_hmac), key_size); empty line for consistency http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:47: return key; You can combine 33, 46-47 into return std::string(...); http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:51: // |kWebMIntegrityCheckSize| in bytes of |input| matches the rest of the data Comment is out of date. http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:63: DVLOG(1) << "Could not initialize HMAC."; Are all these errors likely to occur? It seems they would not assuming we don't hit the CHECKs. I think the team prefers to eliminate such logging unless it is really useful. http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:74: AesDecryptor::kSha1DigestSize)) { arraysize(calculated_hmac)? http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:79: if (memcmp(input.GetDecryptConfig()->data_to_verify(), "data_to_verify" makes me think it is the data I am verifying when it is really the expected value. Should it be verification_value or something like that? http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:81: input.GetDecryptConfig()->data_to_verify_size()) != 0) { Need to ensure that both buffers are the same size before calling this function. That really means ensuring this value is == Sha1DigestSize, which seems like a precondition. However, could this function be written such that Sha1DigestSize isn't needed at all? or would crypto::HMAC fail? http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:95: return counter_block; same - return empty string and return a constructed string at 108. http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:102: // Set block counter to all 0's. Is this comment accurate? Or are we just "zero-extending" the unused portion of the counter block? http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:105: AesDecryptor::kKeySize - iv_size); Is it okay for the size to be 0? http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:111: // Decrypt |input| using |key|. |offset| is the number of bytes into |input| s/offset/encrypted_data_offset/ (or just data_offset). http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:112: // the encrypted data is. that the encrypted data starts. http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:214: HmacEncryptionKeys* keys = new HmacEncryptionKeys(key_string); Could we generalize the class right here? WebM does this, and ISO creates EncryptionKey? If we do that, "key_map" might make more sense since it's not always plural and is easier to say. http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:265: if (verify_size > 0 && !CheckData(*encrypted, keys->hmac_key())) { Might have to check that hmac_key() is not NULL here too for ISO. http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:267: // TODO(fgalligan): Should we signal a decryptor error here? I think so. This was clearly a problem related to decryption (verification failure). http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:303: DVLOG(1) << "Could not create encryption key."; "... import decryption key." but probably just delete. http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor.h File media/crypto/aes_decryptor.h (right): http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.h:26: // Checks the integrity of the encrypted data and decrypts the AES encrypted Eventually, this should be "Optionally..." http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.h:33: static const int kSha1DigestSize = 20; kWebMSha1DigestSize Same for 35 and 36. Then 34 should be separate since it will be common for ISO too. Can we move these to WebMHmacEncryptionKeys or the .cc file? http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.h:57: // Return a DecoderBuffer with the decrypted data if the check and *integrity* check http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.h:59: // TODO(fgalligan): Do we need to differentiate between a check failure Not to the application. There is no error to report verification failure. http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.h:65: // Helper class that manages the HMAC and encryption keys. It's probably worth referring to the RFC here. http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.h:66: class HmacEncryptionKeys { Since this uses WebM-specific constants, it should be named WebMHmacEncryptionKeys. Eventually (when adding ISO), I think we can create an interface (or base class without HMAC) and only define that here and move this inside the .cc file. http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.h:75: crypto::SymmetricKey* encryption_key() { return encryption_key_.get(); } decryption_key seems better. http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.h:86: }; DISALLOW_COPY_AND_ASSIGN
http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor.cc File media/crypto/aes_decryptor.cc (right): http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:28: int key_size) { Can we pass |secret| and |seed| as StringPiece? This way we don't need to do the cast in hmac.Init() on line 35. Also, on line 42, hmac.Sign() converts |seed| to StringPiece implicitly anyways. http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:55: const std::string& hmac_key) { ditto, pass |hmac_key| as string piece. http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:89: // | iv | block counter |. |iv| is a CTR IV. |iv_size| is the size Can we have "| iv | block counter |" on a separate line to emphasize? It's a little confusing to have both | iv | and |iv| on the same line. http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:214: HmacEncryptionKeys* keys = new HmacEncryptionKeys(key_string); Use scoped_ptr here to make ownership clear. Then on line 221, we don't need to "delete keys". Also, on line 234, we do "keys_map_[key_id_string] = keys.Pass()". http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:311: } Add empty line here to be consistent. http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor.h File media/crypto/aes_decryptor.h (right): http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.h:56: // Check and Decrypt |input| buffer. The |input| should not be NULL. "Checks and Decrypts" http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.h:74: std::string hmac_key() { return hmac_key_; } We can return StringPiece here. Also see the comment on CheckData in aes_decryptor.cc file. http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.h:81: // The key used to perform the intergrity check. s/intergrity/integrity http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor... File media/crypto/aes_decryptor_unittest.cc (right): http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor... media/crypto/aes_decryptor_unittest.cc:197: frame.key_id_size)); Can we pass in |frame| (const WebmEncryptedData&) directly to DecryptAndExpectToSucceed and DecryptAndEx[ectToFail? In the last three tests (failure tests), we can memcpy frame.encrypted_data before calling DecryptAndExpectToFail. http://codereview.chromium.org/10535029/diff/17001/media/filters/pipeline_int... File media/filters/pipeline_integration_test.cc (right): http://codereview.chromium.org/10535029/diff/17001/media/filters/pipeline_int... media/filters/pipeline_integration_test.cc:25: }; Here and in other unittests: Can we have 8 or 12 numbers per line instead of 13? It's hard to know how many bytes are there in the array. I found that in most crypto:: unit tests, they use 8 numbers per line (e.g. http://code.google.com/searchframe#OAMlx_jo-ck/src/crypto/p224_unittest.cc&l=16). I also see some cases with 12 numbers per line but it's rare. http://codereview.chromium.org/10535029/diff/17001/media/webm/webm_cluster_pa... File media/webm/webm_cluster_parser.cc (right): http://codereview.chromium.org/10535029/diff/17001/media/webm/webm_cluster_pa... media/webm/webm_cluster_parser.cc:201: video_encryption_key_id_.get(); indentation: should align with "track_num".
Finished reviewing, plus one more comment in one one of the files I already reviewed. http://codereview.chromium.org/10535029/diff/17001/media/base/decrypt_config.h File media/base/decrypt_config.h (right): http://codereview.chromium.org/10535029/diff/17001/media/base/decrypt_config.... media/base/decrypt_config.h:17: DecryptConfig(const uint8* data_to_verify, int data_to_verify_size, On 2012/07/10 01:12:20, ddorwin wrote: > It's not obvious what's being passed here. I think a comment block explaining > the values would be helpful. > Also, maybe move these after key_id_size so all the "data"/offset values are > together. > It's probably worth noting that this class does not take ownership of the > pointers / copies the data. (I initially thought it did take ownership of them > based on the member types below.) I now realize that offset_to_data is completely unrelated to data_to_verify, so keeping them apart is good. However, it's clear we need to fix the naming (offset_to_data_in_decoder_buffer is long but what it really means). SringPiece might also help (me) avoid thinking that the former indexes into the latter. http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor... File media/crypto/aes_decryptor_unittest.cc (right): http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor... media/crypto/aes_decryptor_unittest.cc:36: static const int kIntegrityCheckSize = 12; WebM "Check" seems like an action. Maybe "Value" or something like that. Also, should we just use the value from WebM constants or is that a DEPS violation? http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor... media/crypto/aes_decryptor_unittest.cc:90: static const unsigned char kFrame0InvalidHmac[] = { How are these invalid? Size? "Wrong" (does not match actual)? http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor... media/crypto/aes_decryptor_unittest.cc:90: static const unsigned char kFrame0InvalidHmac[] = { Are these really encrypted blocks with wrong Hmac, IV, etc.? I didn't get that from the name and only figured it out when I saw how they are used. Is there another name that conveys this? http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor... media/crypto/aes_decryptor_unittest.cc:111: scoped_refptr<DecoderBuffer> CreateEncryptedBuffer(const uint8* data, WebM http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor... media/crypto/aes_decryptor_unittest.cc:119: // Every encrypted Block has an HMAC and IV prepended to it. Current WebM Seems like a function level comment. Should at least come before 115. http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor... media/crypto/aes_decryptor_unittest.cc:129: data, kIntegrityCheckSize, Might be more understandable for others if you made an alias for data named integrity_value or something like that. http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor... media/crypto/aes_decryptor_unittest.cc:197: frame.key_id_size)); On 2012/07/10 06:31:25, xhwang wrote: > Can we pass in |frame| (const WebmEncryptedData&) directly to > DecryptAndExpectToSucceed and DecryptAndEx[ectToFail? In the last three tests > (failure tests), we can memcpy frame.encrypted_data before calling > DecryptAndExpectToFail. Since it's already this way, we might just leave it and see what happens when we have to generalize this for ISO. http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor... media/crypto/aes_decryptor_unittest.cc:213: TEST_F(AesDecryptorTest, MultipleKeys) { Suggest moving above the other MultipleKeys test and maybe just combining them. http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor... media/crypto/aes_decryptor_unittest.cc:269: const WebmEncryptedData& frame2 = kEncryptedFrames[2]; Probably more interesting if the keys are added before either decryption occurs. http://codereview.chromium.org/10535029/diff/17001/media/filters/ffmpeg_video... File media/filters/ffmpeg_video_decoder_unittest.cc (right): http://codereview.chromium.org/10535029/diff/17001/media/filters/ffmpeg_video... media/filters/ffmpeg_video_decoder_unittest.cc:54: static const uint8 kRawKey[] = { kSecret or kSecretKey seems more general. Would need to change 49 to kWrongSecretKey. http://codereview.chromium.org/10535029/diff/17001/media/filters/ffmpeg_video... media/filters/ffmpeg_video_decoder_unittest.cc:58: static const uint64 kIv = 0; Suggest choosing a more interesting value. 0 seems like it could be ignored. http://codereview.chromium.org/10535029/diff/17001/media/filters/ffmpeg_video... media/filters/ffmpeg_video_decoder_unittest.cc:59: static const int kHmacSize = 12; suggest swapping with next. Better yet, maybe use arraysize like the others. http://codereview.chromium.org/10535029/diff/17001/media/filters/pipeline_int... File media/filters/pipeline_integration_test.cc (right): http://codereview.chromium.org/10535029/diff/17001/media/filters/pipeline_int... media/filters/pipeline_integration_test.cc:22: static const uint8 kKey[] = { secret key or encryption key? http://codereview.chromium.org/10535029/diff/17001/media/filters/pipeline_int... media/filters/pipeline_integration_test.cc:105: DCHECK_GT(init_data_size, 0); For WebM, why isn't it a fixed size? http://codereview.chromium.org/10535029/diff/17001/media/webm/webm_cluster_pa... File media/webm/webm_cluster_parser.cc (right): http://codereview.chromium.org/10535029/diff/17001/media/webm/webm_cluster_pa... media/webm/webm_cluster_parser.cc:197: // Every encrypted Block has an HMAC and IV prepended to it. Current WebM "encrypted WebM" or "WebM encryption"? Same in next file and other references to the RFC. http://codereview.chromium.org/10535029/diff/17001/media/webm/webm_cluster_pa... media/webm/webm_cluster_parser.cc:198: // encrypted request for comments specification is here I suggest reiterating here that for encrypted blocks: * the decoder buffer includes the IV and encrypted frame but not the HMAC * the HMAC and IV are in the DecryptConfig http://codereview.chromium.org/10535029/diff/17001/media/webm/webm_cluster_pa... media/webm/webm_cluster_parser.cc:216: data + kWebMIntegrityCheckSize, local var iv_offset might help readability (and fit on one line) http://codereview.chromium.org/10535029/diff/17001/media/webm/webm_constants.h File media/webm/webm_constants.h (right): http://codereview.chromium.org/10535029/diff/17001/media/webm/webm_constants.... media/webm/webm_constants.h:205: const int kWebMIntegrityCheckSize = 12; Same as in an earlier file: "Check" seems like an action. Maybe "Value" or something like that.
https://chromiumcodereview.appspot.com/10535029/diff/17001/media/base/decrypt... File media/base/decrypt_config.h (right): https://chromiumcodereview.appspot.com/10535029/diff/17001/media/base/decrypt... media/base/decrypt_config.h:17: DecryptConfig(const uint8* data_to_verify, int data_to_verify_size, On 2012/07/10 01:12:20, ddorwin wrote: > It's not obvious what's being passed here. I think a comment block explaining > the values would be helpful. > Also, maybe move these after key_id_size so all the "data"/offset values are > together. > It's probably worth noting that this class does not take ownership of the > pointers / copies the data. (I initially thought it did take ownership of them > based on the member types below.) Done. https://chromiumcodereview.appspot.com/10535029/diff/17001/media/base/decrypt... media/base/decrypt_config.h:17: DecryptConfig(const uint8* data_to_verify, int data_to_verify_size, On 2012/07/11 01:00:27, ddorwin wrote: > On 2012/07/10 01:12:20, ddorwin wrote: > > It's not obvious what's being passed here. I think a comment block explaining > > the values would be helpful. > > Also, maybe move these after key_id_size so all the "data"/offset values are > > together. > > It's probably worth noting that this class does not take ownership of the > > pointers / copies the data. (I initially thought it did take ownership of them > > based on the member types below.) > > I now realize that offset_to_data is completely unrelated to data_to_verify, so > keeping them apart is good. However, it's clear we need to fix the naming > (offset_to_data_in_decoder_buffer is long but what it really means). SringPiece > might also help (me) avoid thinking that the former indexes into the latter. I want to keep them separate wrt to the decryptor as it shouldn't think the 2 values are correlated. What about the new names? https://chromiumcodereview.appspot.com/10535029/diff/17001/media/base/decrypt... media/base/decrypt_config.h:20: int offset_to_data); On 2012/07/10 01:12:20, ddorwin wrote: > offset_to_frame? encrypted_data_offset? > It's unclear what this means and how it relates to the first parameter It actually shouldn't relate to the first parameter wrt the decryptor. The first parameter is a checksum of the encrypted buffer. The last parameter is offset into the encrypted buffer the encrypted frame starts. wrt the decryptor it doesn't care about any data that is skipped over (with the latest patch). I think the latest patch makes things clearer. If not let me know. https://chromiumcodereview.appspot.com/10535029/diff/17001/media/crypto/aes_d... File media/crypto/aes_decryptor.cc (right): https://chromiumcodereview.appspot.com/10535029/diff/17001/media/crypto/aes_d... media/crypto/aes_decryptor.cc:22: // Derives a key from an HMAC using SHA1. |secret| is the base secret to derive On 2012/07/10 01:12:20, ddorwin wrote: > s/from an HMAC using SHA1/using a SHA1 HMAC/ ? Done. https://chromiumcodereview.appspot.com/10535029/diff/17001/media/crypto/aes_d... media/crypto/aes_decryptor.cc:23: // the key from. |seed| is the kwnon input to the HMAC. |key_size| is how many On 2012/07/10 01:12:20, ddorwin wrote: > known Done. https://chromiumcodereview.appspot.com/10535029/diff/17001/media/crypto/aes_d... media/crypto/aes_decryptor.cc:28: int key_size) { On 2012/07/10 06:31:25, xhwang wrote: > Can we pass |secret| and |seed| as StringPiece? This way we don't need to do the > cast in hmac.Init() on line 35. Also, on line 42, hmac.Sign() converts |seed| to > StringPiece implicitly anyways. Done. https://chromiumcodereview.appspot.com/10535029/diff/17001/media/crypto/aes_d... media/crypto/aes_decryptor.cc:38: return key; On 2012/07/10 01:12:20, ddorwin wrote: > It might be more readable to just return an empty string in the errors. There's > no partially correct key to return. You can also remove line 33 to line 46. Done. https://chromiumcodereview.appspot.com/10535029/diff/17001/media/crypto/aes_d... media/crypto/aes_decryptor.cc:46: key.assign(reinterpret_cast<const char*>(calculated_hmac), key_size); On 2012/07/10 01:12:20, ddorwin wrote: > empty line for consistency Done. https://chromiumcodereview.appspot.com/10535029/diff/17001/media/crypto/aes_d... media/crypto/aes_decryptor.cc:47: return key; On 2012/07/10 01:12:20, ddorwin wrote: > You can combine 33, 46-47 into > return std::string(...); Done. https://chromiumcodereview.appspot.com/10535029/diff/17001/media/crypto/aes_d... media/crypto/aes_decryptor.cc:51: // |kWebMIntegrityCheckSize| in bytes of |input| matches the rest of the data On 2012/07/10 01:12:20, ddorwin wrote: > Comment is out of date. Done. https://chromiumcodereview.appspot.com/10535029/diff/17001/media/crypto/aes_d... media/crypto/aes_decryptor.cc:55: const std::string& hmac_key) { On 2012/07/10 06:31:25, xhwang wrote: > ditto, pass |hmac_key| as string piece. Done. https://chromiumcodereview.appspot.com/10535029/diff/17001/media/crypto/aes_d... media/crypto/aes_decryptor.cc:63: DVLOG(1) << "Could not initialize HMAC."; On 2012/07/10 01:12:20, ddorwin wrote: > Are all these errors likely to occur? It seems they would not assuming we don't > hit the CHECKs. I think the team prefers to eliminate such logging unless it is > really useful. I'm not really sure, but I'm guessing these errors won't happen often. I can remove the logging and log an IC failure from the call to CheckData(). https://chromiumcodereview.appspot.com/10535029/diff/17001/media/crypto/aes_d... media/crypto/aes_decryptor.cc:74: AesDecryptor::kSha1DigestSize)) { On 2012/07/10 01:12:20, ddorwin wrote: > arraysize(calculated_hmac)? Done. https://chromiumcodereview.appspot.com/10535029/diff/17001/media/crypto/aes_d... media/crypto/aes_decryptor.cc:79: if (memcmp(input.GetDecryptConfig()->data_to_verify(), On 2012/07/10 01:12:20, ddorwin wrote: > "data_to_verify" makes me think it is the data I am verifying when it is really > the expected value. Should it be verification_value or something like that? I changed the name to "checksum". Sound better to you? https://chromiumcodereview.appspot.com/10535029/diff/17001/media/crypto/aes_d... media/crypto/aes_decryptor.cc:81: input.GetDecryptConfig()->data_to_verify_size()) != 0) { On 2012/07/10 01:12:20, ddorwin wrote: > Need to ensure that both buffers are the same size before calling this function. > That really means ensuring this value is == Sha1DigestSize, which seems like a > precondition. However, could this function be written such that Sha1DigestSize > isn't needed at all? or would crypto::HMAC fail? Currently the two sizes are different. checksum_size == 12 and Sha1DigestSize == 20. I added a check to make sure checksum_size <= Sha1DigestSize. I checked and hmac.cc had a DigestSize function or I'm not sure what you meant by "Sha1DigestSize isn't needed at all" What were you thinking? https://chromiumcodereview.appspot.com/10535029/diff/17001/media/crypto/aes_d... media/crypto/aes_decryptor.cc:89: // | iv | block counter |. |iv| is a CTR IV. |iv_size| is the size On 2012/07/10 06:31:25, xhwang wrote: > Can we have "| iv | block counter |" on a separate line to emphasize? It's a > little confusing to have both | iv | and |iv| on the same line. Changed too "Generates a 16 byte CTR counter block. The CTR counter block format is a CTR IV appended with a CTR block counter." https://chromiumcodereview.appspot.com/10535029/diff/17001/media/crypto/aes_d... media/crypto/aes_decryptor.cc:95: return counter_block; On 2012/07/10 01:12:20, ddorwin wrote: > same - return empty string and return a constructed string at 108. Done. https://chromiumcodereview.appspot.com/10535029/diff/17001/media/crypto/aes_d... media/crypto/aes_decryptor.cc:102: // Set block counter to all 0's. On 2012/07/10 01:12:20, ddorwin wrote: > Is this comment accurate? Or are we just "zero-extending" the unused portion of > the counter block? Yes and Yes. It probably is confusing because "block counter" is a part of "counter block". https://chromiumcodereview.appspot.com/10535029/diff/17001/media/crypto/aes_d... media/crypto/aes_decryptor.cc:105: AesDecryptor::kKeySize - iv_size); On 2012/07/10 01:12:20, ddorwin wrote: > Is it okay for the size to be 0? Should be. https://chromiumcodereview.appspot.com/10535029/diff/17001/media/crypto/aes_d... media/crypto/aes_decryptor.cc:111: // Decrypt |input| using |key|. |offset| is the number of bytes into |input| On 2012/07/10 01:12:20, ddorwin wrote: > s/offset/encrypted_data_offset/ (or just data_offset). Done. https://chromiumcodereview.appspot.com/10535029/diff/17001/media/crypto/aes_d... media/crypto/aes_decryptor.cc:112: // the encrypted data is. On 2012/07/10 01:12:20, ddorwin wrote: > that the encrypted data starts. Done. https://chromiumcodereview.appspot.com/10535029/diff/17001/media/crypto/aes_d... media/crypto/aes_decryptor.cc:214: HmacEncryptionKeys* keys = new HmacEncryptionKeys(key_string); On 2012/07/10 06:31:25, xhwang wrote: > Use scoped_ptr here to make ownership clear. Then on line 221, we don't need to > "delete keys". Also, on line 234, we do "keys_map_[key_id_string] = > keys.Pass()". I can't Pass() to the map. Can I move scoped_ptr<> to * or should I make HmacEncryptionKeys derive from scoped_refptr? https://chromiumcodereview.appspot.com/10535029/diff/17001/media/crypto/aes_d... media/crypto/aes_decryptor.cc:214: HmacEncryptionKeys* keys = new HmacEncryptionKeys(key_string); On 2012/07/10 01:12:20, ddorwin wrote: > Could we generalize the class right here? WebM does this, and ISO creates > EncryptionKey? > > If we do that, "key_map" might make more sense since it's not always plural and > is easier to say. Changed the variable name to key_map. I think we can generalize it here. The CL I had before had AesDecryptor and HmacAesDecryptor derived from the same interface. I think we can have an EncryptionKey class that contains a SymmetricKey and a HMAC (The HAMC can be NULL). We still would be generating the HMAC and SymmetricKey in a WebM specific way if the HMAC is valid. The other issue is that at this point we currently don't have data to differentiate what we should do with |key| so I delayed initialization until Decrypt. I'm deciding to perform the WebM initialization if there is a checksum in DecryptConfig, which is fine for now, but would need to be revisited if we added a checksum in ClearKey which used another algorithm. https://chromiumcodereview.appspot.com/10535029/diff/17001/media/crypto/aes_d... media/crypto/aes_decryptor.cc:265: if (verify_size > 0 && !CheckData(*encrypted, keys->hmac_key())) { On 2012/07/10 01:12:20, ddorwin wrote: > Might have to check that hmac_key() is not NULL here too for ISO. Currently ISO does not define an IC. If they do we can add it. https://chromiumcodereview.appspot.com/10535029/diff/17001/media/crypto/aes_d... media/crypto/aes_decryptor.cc:267: // TODO(fgalligan): Should we signal a decryptor error here? On 2012/07/10 01:12:20, ddorwin wrote: > I think so. This was clearly a problem related to decryption (verification > failure). I updated the comment with an explanation of what should happen with an IC failure. https://chromiumcodereview.appspot.com/10535029/diff/17001/media/crypto/aes_d... media/crypto/aes_decryptor.cc:303: DVLOG(1) << "Could not create encryption key."; On 2012/07/10 01:12:20, ddorwin wrote: > "... import decryption key." > but probably just delete. Done. https://chromiumcodereview.appspot.com/10535029/diff/17001/media/crypto/aes_d... media/crypto/aes_decryptor.cc:311: } On 2012/07/10 06:31:25, xhwang wrote: > Add empty line here to be consistent. Done. https://chromiumcodereview.appspot.com/10535029/diff/17001/media/crypto/aes_d... File media/crypto/aes_decryptor.h (right): https://chromiumcodereview.appspot.com/10535029/diff/17001/media/crypto/aes_d... media/crypto/aes_decryptor.h:26: // Checks the integrity of the encrypted data and decrypts the AES encrypted On 2012/07/10 01:12:20, ddorwin wrote: > Eventually, this should be "Optionally..." Done. https://chromiumcodereview.appspot.com/10535029/diff/17001/media/crypto/aes_d... media/crypto/aes_decryptor.h:33: static const int kSha1DigestSize = 20; On 2012/07/10 01:12:20, ddorwin wrote: > kWebMSha1DigestSize > Same for 35 and 36. Then 34 should be separate since it will be common for ISO > too. > Can we move these to WebMHmacEncryptionKeys or the .cc file? Added Webm to the consts. Moved the Webm consts to the .cc file. Moved kKeySize to webm_cluster_parser.cc to behave more like the iso parser. https://chromiumcodereview.appspot.com/10535029/diff/17001/media/crypto/aes_d... media/crypto/aes_decryptor.h:56: // Check and Decrypt |input| buffer. The |input| should not be NULL. On 2012/07/10 06:31:25, xhwang wrote: > "Checks and Decrypts" Done. https://chromiumcodereview.appspot.com/10535029/diff/17001/media/crypto/aes_d... media/crypto/aes_decryptor.h:57: // Return a DecoderBuffer with the decrypted data if the check and On 2012/07/10 01:12:20, ddorwin wrote: > *integrity* check Done. https://chromiumcodereview.appspot.com/10535029/diff/17001/media/crypto/aes_d... media/crypto/aes_decryptor.h:59: // TODO(fgalligan): Do we need to differentiate between a check failure On 2012/07/10 01:12:20, ddorwin wrote: > Not to the application. There is no error to report verification failure. Done. https://chromiumcodereview.appspot.com/10535029/diff/17001/media/crypto/aes_d... media/crypto/aes_decryptor.h:65: // Helper class that manages the HMAC and encryption keys. On 2012/07/10 01:12:20, ddorwin wrote: > It's probably worth referring to the RFC here. Done. https://chromiumcodereview.appspot.com/10535029/diff/17001/media/crypto/aes_d... media/crypto/aes_decryptor.h:66: class HmacEncryptionKeys { On 2012/07/10 01:12:20, ddorwin wrote: > Since this uses WebM-specific constants, it should be named > WebMHmacEncryptionKeys. > Eventually (when adding ISO), I think we can create an interface (or base class > without HMAC) and only define that here and move this inside the .cc file. Done. https://chromiumcodereview.appspot.com/10535029/diff/17001/media/crypto/aes_d... media/crypto/aes_decryptor.h:74: std::string hmac_key() { return hmac_key_; } On 2012/07/10 06:31:25, xhwang wrote: > We can return StringPiece here. Also see the comment on CheckData in > aes_decryptor.cc file. Done. https://chromiumcodereview.appspot.com/10535029/diff/17001/media/crypto/aes_d... media/crypto/aes_decryptor.h:75: crypto::SymmetricKey* encryption_key() { return encryption_key_.get(); } On 2012/07/10 01:12:20, ddorwin wrote: > decryption_key seems better. Done. https://chromiumcodereview.appspot.com/10535029/diff/17001/media/crypto/aes_d... media/crypto/aes_decryptor.h:81: // The key used to perform the intergrity check. On 2012/07/10 06:31:25, xhwang wrote: > s/intergrity/integrity Done. https://chromiumcodereview.appspot.com/10535029/diff/17001/media/crypto/aes_d... media/crypto/aes_decryptor.h:86: }; On 2012/07/10 01:12:20, ddorwin wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://chromiumcodereview.appspot.com/10535029/diff/17001/media/crypto/aes_d... File media/crypto/aes_decryptor_unittest.cc (right): https://chromiumcodereview.appspot.com/10535029/diff/17001/media/crypto/aes_d... media/crypto/aes_decryptor_unittest.cc:36: static const int kIntegrityCheckSize = 12; On 2012/07/11 01:00:27, ddorwin wrote: > WebM > "Check" seems like an action. Maybe "Value" or something like that. > Also, should we just use the value from WebM constants or is that a DEPS > violation? Done. https://chromiumcodereview.appspot.com/10535029/diff/17001/media/crypto/aes_d... media/crypto/aes_decryptor_unittest.cc:90: static const unsigned char kFrame0InvalidHmac[] = { On 2012/07/11 01:00:27, ddorwin wrote: > Are these really encrypted blocks with wrong Hmac, IV, etc.? I didn't get that > from the name and only figured it out when I saw how they are used. Is there > another name that conveys this? I changed the names and added comments on what was explicitly changed anf from where. https://chromiumcodereview.appspot.com/10535029/diff/17001/media/crypto/aes_d... media/crypto/aes_decryptor_unittest.cc:111: scoped_refptr<DecoderBuffer> CreateEncryptedBuffer(const uint8* data, On 2012/07/11 01:00:27, ddorwin wrote: > WebM Done. https://chromiumcodereview.appspot.com/10535029/diff/17001/media/crypto/aes_d... media/crypto/aes_decryptor_unittest.cc:119: // Every encrypted Block has an HMAC and IV prepended to it. Current WebM On 2012/07/11 01:00:27, ddorwin wrote: > Seems like a function level comment. Should at least come before 115. Done. https://chromiumcodereview.appspot.com/10535029/diff/17001/media/crypto/aes_d... media/crypto/aes_decryptor_unittest.cc:129: data, kIntegrityCheckSize, On 2012/07/11 01:00:27, ddorwin wrote: > Might be more understandable for others if you made an alias for data named > integrity_value or something like that. Done. https://chromiumcodereview.appspot.com/10535029/diff/17001/media/crypto/aes_d... media/crypto/aes_decryptor_unittest.cc:197: frame.key_id_size)); On 2012/07/11 01:00:27, ddorwin wrote: > On 2012/07/10 06:31:25, xhwang wrote: > > Can we pass in |frame| (const WebmEncryptedData&) directly to > > DecryptAndExpectToSucceed and DecryptAndEx[ectToFail? In the last three tests > > (failure tests), we can memcpy frame.encrypted_data before calling > > DecryptAndExpectToFail. > > Since it's already this way, we might just leave it and see what happens when we > have to generalize this for ISO. OK, did nothing :). https://chromiumcodereview.appspot.com/10535029/diff/17001/media/crypto/aes_d... media/crypto/aes_decryptor_unittest.cc:213: TEST_F(AesDecryptorTest, MultipleKeys) { On 2012/07/11 01:00:27, ddorwin wrote: > Suggest moving above the other MultipleKeys test and maybe just combining them. Removed. https://chromiumcodereview.appspot.com/10535029/diff/17001/media/crypto/aes_d... media/crypto/aes_decryptor_unittest.cc:269: const WebmEncryptedData& frame2 = kEncryptedFrames[2]; On 2012/07/11 01:00:27, ddorwin wrote: > Probably more interesting if the keys are added before either decryption occurs. I changed this to add_key1, decrypt_key1, add_key2, decrypt_key1, decrypt_key2. I think this should follow more real world use. https://chromiumcodereview.appspot.com/10535029/diff/17001/media/filters/ffmp... File media/filters/ffmpeg_video_decoder_unittest.cc (right): https://chromiumcodereview.appspot.com/10535029/diff/17001/media/filters/ffmp... media/filters/ffmpeg_video_decoder_unittest.cc:54: static const uint8 kRawKey[] = { On 2012/07/11 01:00:27, ddorwin wrote: > kSecret or kSecretKey seems more general. Would need to change 49 to > kWrongSecretKey. Done. https://chromiumcodereview.appspot.com/10535029/diff/17001/media/filters/ffmp... media/filters/ffmpeg_video_decoder_unittest.cc:58: static const uint64 kIv = 0; On 2012/07/11 01:00:27, ddorwin wrote: > Suggest choosing a more interesting value. 0 seems like it could be ignored. Done. https://chromiumcodereview.appspot.com/10535029/diff/17001/media/filters/ffmp... media/filters/ffmpeg_video_decoder_unittest.cc:59: static const int kHmacSize = 12; On 2012/07/11 01:00:27, ddorwin wrote: > suggest swapping with next. > Better yet, maybe use arraysize like the others. Done. https://chromiumcodereview.appspot.com/10535029/diff/17001/media/filters/pipe... File media/filters/pipeline_integration_test.cc (right): https://chromiumcodereview.appspot.com/10535029/diff/17001/media/filters/pipe... media/filters/pipeline_integration_test.cc:22: static const uint8 kKey[] = { On 2012/07/11 01:00:27, ddorwin wrote: > secret key or encryption key? Done. https://chromiumcodereview.appspot.com/10535029/diff/17001/media/filters/pipe... media/filters/pipeline_integration_test.cc:25: }; On 2012/07/10 06:31:25, xhwang wrote: > Here and in other unittests: > Can we have 8 or 12 numbers per line instead of 13? It's hard to know how many > bytes are there in the array. I found that in most crypto:: unit tests, they use > 8 numbers per line (e.g. > http://code.google.com/searchframe#OAMlx_jo-ck/src/crypto/p224_unittest.cc&l=16). > I also see some cases with 12 numbers per line but it's rare. Done. https://chromiumcodereview.appspot.com/10535029/diff/17001/media/filters/pipe... media/filters/pipeline_integration_test.cc:105: DCHECK_GT(init_data_size, 0); On 2012/07/11 01:00:27, ddorwin wrote: > For WebM, why isn't it a fixed size? ContentEncKeyID is a binary element. I just didn't put a size in the WebM spec because I didn't think it needed one. Are you thinking we should put a max size? We can also put an explicit size, maybe that will prevent issues later. https://chromiumcodereview.appspot.com/10535029/diff/17001/media/webm/webm_cl... File media/webm/webm_cluster_parser.cc (right): https://chromiumcodereview.appspot.com/10535029/diff/17001/media/webm/webm_cl... media/webm/webm_cluster_parser.cc:197: // Every encrypted Block has an HMAC and IV prepended to it. Current WebM On 2012/07/11 01:00:27, ddorwin wrote: > "encrypted WebM" or "WebM encryption"? Same in next file and other references to > the RFC. Done. https://chromiumcodereview.appspot.com/10535029/diff/17001/media/webm/webm_cl... media/webm/webm_cluster_parser.cc:198: // encrypted request for comments specification is here On 2012/07/11 01:00:27, ddorwin wrote: > I suggest reiterating here that for encrypted blocks: > * the decoder buffer includes the IV and encrypted frame but not the HMAC > * the HMAC and IV are in the DecryptConfig Done. https://chromiumcodereview.appspot.com/10535029/diff/17001/media/webm/webm_cl... media/webm/webm_cluster_parser.cc:201: video_encryption_key_id_.get(); On 2012/07/10 06:31:25, xhwang wrote: > indentation: should align with "track_num". Done. https://chromiumcodereview.appspot.com/10535029/diff/17001/media/webm/webm_cl... media/webm/webm_cluster_parser.cc:216: data + kWebMIntegrityCheckSize, On 2012/07/11 01:00:27, ddorwin wrote: > local var iv_offset might help readability (and fit on one line) It fits on one line (must have had a refactor). If you still want me to add a local var for readability let me know. https://chromiumcodereview.appspot.com/10535029/diff/17001/media/webm/webm_co... File media/webm/webm_constants.h (right): https://chromiumcodereview.appspot.com/10535029/diff/17001/media/webm/webm_co... media/webm/webm_constants.h:205: const int kWebMIntegrityCheckSize = 12; On 2012/07/11 01:00:27, ddorwin wrote: > Same as in an earlier file: "Check" seems like an action. Maybe "Value" or > something like that. Done.
http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor.cc File media/crypto/aes_decryptor.cc (right): http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:214: HmacEncryptionKeys* keys = new HmacEncryptionKeys(key_string); On 2012/07/11 22:06:33, fgalligan1 wrote: > On 2012/07/10 06:31:25, xhwang wrote: > > Use scoped_ptr here to make ownership clear. Then on line 221, we don't need > to > > "delete keys". Also, on line 234, we do "keys_map_[key_id_string] = > > keys.Pass()". > I can't Pass() to the map. Can I move scoped_ptr<> to * or should I make > HmacEncryptionKeys derive from scoped_refptr? Pass is for refptr, which doesn't make sense here. Use scoped_ptr.release(). The map will then own the pointer, though it won't automatically delete the data. http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:214: HmacEncryptionKeys* keys = new HmacEncryptionKeys(key_string); On 2012/07/11 22:06:33, fgalligan1 wrote: > On 2012/07/10 01:12:20, ddorwin wrote: > > Could we generalize the class right here? WebM does this, and ISO creates > > EncryptionKey? > > > > If we do that, "key_map" might make more sense since it's not always plural > and > > is easier to say. > Changed the variable name to key_map. > > I think we can generalize it here. The CL I had before had AesDecryptor and > HmacAesDecryptor derived from the same interface. I think we can have an > EncryptionKey class that contains a SymmetricKey and a HMAC (The HAMC can be > NULL). We still would be generating the HMAC and SymmetricKey in a WebM > specific way if the HMAC is valid. The other issue is that at this point we > currently don't have data to differentiate what we should do with |key| so I > delayed initialization until Decrypt. I'm deciding to perform the WebM > initialization if there is a checksum in DecryptConfig, which is fine for now, > but would need to be revisited if we added a checksum in ClearKey which used > another algorithm. I think you should do the init() here. It's more natural here, and we can throw an error instead of keyadded if it fails. Just put a TODO somewhere to do a nop for ISO. Actually implementing that TODO will be interesting. We'll probably need a has_hmac Boolean in the constructor of this object and to try to find the type of the source in WMPI. (It looks like container might be specified in the EME object's constructor, so that will make it even easier in the future.) http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:265: if (verify_size > 0 && !CheckData(*encrypted, keys->hmac_key())) { On 2012/07/11 22:06:33, fgalligan1 wrote: > On 2012/07/10 01:12:20, ddorwin wrote: > > Might have to check that hmac_key() is not NULL here too for ISO. > > Currently ISO does not define an IC. If they do we can add it. I think I meant we shouldn't call CheckData() if keys->hmac_key() is NULL. verify_size should cover this, though. It might be nice to DCHECK(!verify_size == !hmac_key) or something like that. This would already be caught in CheckData(), though. http://codereview.chromium.org/10535029/diff/17001/media/filters/pipeline_int... File media/filters/pipeline_integration_test.cc (right): http://codereview.chromium.org/10535029/diff/17001/media/filters/pipeline_int... media/filters/pipeline_integration_test.cc:105: DCHECK_GT(init_data_size, 0); On 2012/07/11 22:06:33, fgalligan1 wrote: > On 2012/07/11 01:00:27, ddorwin wrote: > > For WebM, why isn't it a fixed size? > ContentEncKeyID is a binary element. I just didn't put a size in the WebM spec > because I didn't think it needed one. Are you thinking we should put a max size? > We can also put an explicit size, maybe that will prevent issues later. Oh, this is fine then. I don't recall whether the other code handles variable size keys. Does it? (I know the tests use 16.) If we do support other sizes, we should have a test. http://codereview.chromium.org/10535029/diff/19009/media/base/decrypt_config.h File media/base/decrypt_config.h (right): http://codereview.chromium.org/10535029/diff/19009/media/base/decrypt_config.... media/base/decrypt_config.h:17: // |key_id| is the initialization data defined in the Encrypted Media key_id is a key ID here. This is the ID that the decryptor should use for this frame. (This is true for the ISO CL too.) http://codereview.chromium.org/10535029/diff/19009/media/base/decrypt_config.... media/base/decrypt_config.h:31: const uint8* checksum() const { return checksum_.get(); } Move these below for consistency with new parameter order. Same with the member vars. http://codereview.chromium.org/10535029/diff/19009/media/crypto/aes_decryptor.cc File media/crypto/aes_decryptor.cc (right): http://codereview.chromium.org/10535029/diff/19009/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:77: CHECK(input.GetDecryptConfig()->checksum_size() <= DCHECK? Do we need this in opt builds? http://codereview.chromium.org/10535029/diff/19009/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:86: // Decrypt |input| using |key|. |encrypted_data_offset| is the number of bytes Decrypts... Returns... http://codereview.chromium.org/10535029/diff/19009/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:112: if (!encryptor.SetCounter(counter_block)) { This version handles smaller IVs so we don't need to zero-out as before? Or are you assuming they are correct? If the latter, we should probably specify that somewhere. Also, we could at least DCHECK(iv_size==16), right? And does DecryptConfig need to assume/enforce this too? http://codereview.chromium.org/10535029/diff/19009/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:187: DecryptionKey* decrypt_key = new DecryptionKey(key_string); decryption_key for consistency? http://codereview.chromium.org/10535029/diff/19009/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:225: // TODO(fgalligan): Should this throw a decryptor error here? That or needkey. There is some debate about which and how. Maybe xhwang has some thoughts, but at least pick one. http://codereview.chromium.org/10535029/diff/19009/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:234: if (!key->Init(checksum_size > 0)) { As mentioned in a reply, I don't think this should be here. Moving it back also allows you to throw an error (line 242). http://codereview.chromium.org/10535029/diff/19009/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:249: // continue with the next frame. Hmm. What is the reason for that? Dealing with network issues? Shouldn't the user be made aware that something is wrong with the stream? What does the HTML5 spec say about single-frame decode failures? Could similar network corruption issues cause unencrypted content to stop but encrypted content to continue? http://codereview.chromium.org/10535029/diff/19009/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:276: bool AesDecryptor::DecryptionKey::Init(bool derive_hmac) { derive_webm_keys It's not really deriving an hmac (only), right? http://codereview.chromium.org/10535029/diff/19009/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:280: if (!derive_hmac) { nit: Prefer positive logic when there is an if-else. http://codereview.chromium.org/10535029/diff/19009/media/crypto/aes_decryptor.h File media/crypto/aes_decryptor.h (right): http://codereview.chromium.org/10535029/diff/19009/media/crypto/aes_decryptor... media/crypto/aes_decryptor.h:68: bool initialized() { return initialized_; } is_initialized for both. http://codereview.chromium.org/10535029/diff/19009/media/crypto/aes_decryptor... media/crypto/aes_decryptor.h:69: base::StringPiece hmac_key() { return base::StringPiece(hmac_key_); } nit: swap with below since it is common. swap the members too. http://codereview.chromium.org/10535029/diff/19009/media/crypto/aes_decryptor... media/crypto/aes_decryptor.h:73: // Flag telling if the object was initialized. unnecessary http://codereview.chromium.org/10535029/diff/19009/media/crypto/aes_decryptor... File media/crypto/aes_decryptor_unittest.cc (right): http://codereview.chromium.org/10535029/diff/19009/media/crypto/aes_decryptor... media/crypto/aes_decryptor_unittest.cc:41: const WebmEncryptedData kEncryptedFrames[] = { We should probably change all these frame constants to contain "WebM" in the name. http://codereview.chromium.org/10535029/diff/19009/media/crypto/aes_decryptor... media/crypto/aes_decryptor_unittest.cc:151: static std::string GenerateCounterBlock(const uint8* iv, int iv_size) { Why is this here now? Does AesDecryptor assume 16 bytes now? Do we check that? Has the meaning of the DecryptConfig parameter changed? I don't recall seeing any comments about that. http://codereview.chromium.org/10535029/diff/19009/media/filters/ffmpeg_video... File media/filters/ffmpeg_video_decoder_unittest.cc (right): http://codereview.chromium.org/10535029/diff/19009/media/filters/ffmpeg_video... media/filters/ffmpeg_video_decoder_unittest.cc:419: uint8 counter_block[kDecryptionKeySize]; Why was this a separate function in the other tests but not here? It seems the code is duplicated several times here. Are CBs always the size of the key? http://codereview.chromium.org/10535029/diff/19009/media/filters/ffmpeg_video... media/filters/ffmpeg_video_decoder_unittest.cc:430: counter_block, kDecryptionKeySize, suggest arraysize(c_b). it's not obvious that the key size is the size of this pointer. http://codereview.chromium.org/10535029/diff/19009/media/webm/webm_cluster_pa... File media/webm/webm_cluster_parser.cc (right): http://codereview.chromium.org/10535029/diff/19009/media/webm/webm_cluster_pa... media/webm/webm_cluster_parser.cc:20: if (iv_size <= 0 || iv_size > WebMClusterParser::kDecryptionKeySize) so we do have a specific key size. are we not spec compliant? http://codereview.chromium.org/10535029/diff/19009/media/webm/webm_cluster_pa... media/webm/webm_cluster_parser.cc:239: std::string webm_iv = not really an IV anymore is it? http://codereview.chromium.org/10535029/diff/19009/media/webm/webm_cluster_pa... media/webm/webm_cluster_parser.cc:240: GenerateCounterBlock(reinterpret_cast<const uint8*>(&iv), sizeof(iv)); Why did you move this logic here? Is this what ISO does? http://codereview.chromium.org/10535029/diff/19009/media/webm/webm_cluster_pa... File media/webm/webm_cluster_parser.h (right): http://codereview.chromium.org/10535029/diff/19009/media/webm/webm_cluster_pa... media/webm/webm_cluster_parser.h:21: static const int kDecryptionKeySize = 16; does not match the more permissive RFC, right?
http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor.cc File media/crypto/aes_decryptor.cc (right): http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:214: HmacEncryptionKeys* keys = new HmacEncryptionKeys(key_string); On 2012/07/13 00:48:00, ddorwin wrote: > On 2012/07/11 22:06:33, fgalligan1 wrote: > > On 2012/07/10 06:31:25, xhwang wrote: > > > Use scoped_ptr here to make ownership clear. Then on line 221, we don't need > > to > > > "delete keys". Also, on line 234, we do "keys_map_[key_id_string] = > > > keys.Pass()". > > I can't Pass() to the map. Can I move scoped_ptr<> to * or should I make > > HmacEncryptionKeys derive from scoped_refptr? > > Pass is for refptr, which doesn't make sense here. Use scoped_ptr.release(). The > map will then own the pointer, though it won't automatically delete the data. Pass() is for scoped_ptr, not scoped_refptr. The problem here is that objects in STL containers needs to be assignable and copy constructible, which scoped_ptr is not. So we can't use scoped_ptr here. I met this before but forgot when I reviewed your patch. Sorry! http://codereview.chromium.org/10535029/diff/19009/media/crypto/aes_decryptor.cc File media/crypto/aes_decryptor.cc (right): http://codereview.chromium.org/10535029/diff/19009/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:225: // TODO(fgalligan): Should this throw a decryptor error here? On 2012/07/13 00:48:00, ddorwin wrote: > That or needkey. There is some debate about which and how. Maybe xhwang has some > thoughts, but at least pick one. Yeah, either way. Based on the spec, we should fire a needkey event here. But that part is not implemented yet.
http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor.cc File media/crypto/aes_decryptor.cc (right): http://codereview.chromium.org/10535029/diff/17001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:214: HmacEncryptionKeys* keys = new HmacEncryptionKeys(key_string); On 2012/07/13 01:23:21, xhwang wrote: > On 2012/07/13 00:48:00, ddorwin wrote: > > On 2012/07/11 22:06:33, fgalligan1 wrote: > > > On 2012/07/10 06:31:25, xhwang wrote: > > > > Use scoped_ptr here to make ownership clear. Then on line 221, we don't > need > > > to > > > > "delete keys". Also, on line 234, we do "keys_map_[key_id_string] = > > > > keys.Pass()". > > > I can't Pass() to the map. Can I move scoped_ptr<> to * or should I make > > > HmacEncryptionKeys derive from scoped_refptr? > > > > Pass is for refptr, which doesn't make sense here. Use scoped_ptr.release(). > The > > map will then own the pointer, though it won't automatically delete the data. > > Pass() is for scoped_ptr, not scoped_refptr. The problem here is that objects in > STL containers needs to be assignable and copy constructible, which scoped_ptr > is not. So we can't use scoped_ptr here. I met this before but forgot when I > reviewed your patch. Sorry! But as ddrowin suggested, you should still use scopted_ptr here, then use keys_map_[key_id_string] = keys.release() below so that you don't need "delete keys" on line 221.
https://chromiumcodereview.appspot.com/10535029/diff/19009/media/base/decrypt... File media/base/decrypt_config.h (right): https://chromiumcodereview.appspot.com/10535029/diff/19009/media/base/decrypt... media/base/decrypt_config.h:17: // |key_id| is the initialization data defined in the Encrypted Media On 2012/07/13 00:48:00, ddorwin wrote: > key_id is a key ID here. This is the ID that the decryptor should use for this > frame. (This is true for the ISO CL too.) Done. https://chromiumcodereview.appspot.com/10535029/diff/19009/media/base/decrypt... media/base/decrypt_config.h:31: const uint8* checksum() const { return checksum_.get(); } On 2012/07/13 00:48:00, ddorwin wrote: > Move these below for consistency with new parameter order. Same with the member > vars. Done. https://chromiumcodereview.appspot.com/10535029/diff/19009/media/crypto/aes_d... File media/crypto/aes_decryptor.cc (right): https://chromiumcodereview.appspot.com/10535029/diff/19009/media/crypto/aes_d... media/crypto/aes_decryptor.cc:77: CHECK(input.GetDecryptConfig()->checksum_size() <= On 2012/07/13 00:48:00, ddorwin wrote: > DCHECK? Do we need this in opt builds? Done. https://chromiumcodereview.appspot.com/10535029/diff/19009/media/crypto/aes_d... media/crypto/aes_decryptor.cc:86: // Decrypt |input| using |key|. |encrypted_data_offset| is the number of bytes On 2012/07/13 00:48:00, ddorwin wrote: > Decrypts... > Returns... Done. https://chromiumcodereview.appspot.com/10535029/diff/19009/media/crypto/aes_d... media/crypto/aes_decryptor.cc:112: if (!encryptor.SetCounter(counter_block)) { On 2012/07/13 00:48:00, ddorwin wrote: > This version handles smaller IVs so we don't need to zero-out as before? Or are > you assuming they are correct? If the latter, we should probably specify that > somewhere. Also, we could at least DCHECK(iv_size==16), right? > And does DecryptConfig need to assume/enforce this too? The latter. This code could handle larger or smaller depending on the key size. But webm and ISO currently define that encryption must be 128bits, so this will always be 16. SetCounter() checks for 16 bytes as well. So I think it is fine to enforce this here and DecryptConfig now that I moved the generation of the counter_block to the demuxer. https://chromiumcodereview.appspot.com/10535029/diff/19009/media/crypto/aes_d... media/crypto/aes_decryptor.cc:187: DecryptionKey* decrypt_key = new DecryptionKey(key_string); On 2012/07/13 00:48:00, ddorwin wrote: > decryption_key for consistency? Done. https://chromiumcodereview.appspot.com/10535029/diff/19009/media/crypto/aes_d... media/crypto/aes_decryptor.cc:225: // TODO(fgalligan): Should this throw a decryptor error here? On 2012/07/13 01:23:21, xhwang wrote: > On 2012/07/13 00:48:00, ddorwin wrote: > > That or needkey. There is some debate about which and how. Maybe xhwang has > some > > thoughts, but at least pick one. > > Yeah, either way. Based on the spec, we should fire a needkey event here. But > that part is not implemented yet. Added a TODO to fire a need_key event here. https://chromiumcodereview.appspot.com/10535029/diff/19009/media/crypto/aes_d... media/crypto/aes_decryptor.cc:234: if (!key->Init(checksum_size > 0)) { On 2012/07/13 00:48:00, ddorwin wrote: > As mentioned in a reply, I don't think this should be here. Moving it back also > allows you to throw an error (line 242). Done. https://chromiumcodereview.appspot.com/10535029/diff/19009/media/crypto/aes_d... media/crypto/aes_decryptor.cc:249: // continue with the next frame. On 2012/07/13 00:48:00, ddorwin wrote: > Hmm. What is the reason for that? Dealing with network issues? Shouldn't the > user be made aware that something is wrong with the stream? > > What does the HTML5 spec say about single-frame decode failures? Could similar > network corruption issues cause unencrypted content to stop but encrypted > content to continue? This was requested by one of the security guys, but I don't have why written down and it was a while ago. I don't know under what situation playback would be good after an IC failure. Either you have corrupted data or someone is explicitly messing with the data. Either situation seems like you should stop playing the stream and throw an error. I can remove the text from the RFC and add it as an open issue of what the client should do after an IC failure. https://chromiumcodereview.appspot.com/10535029/diff/19009/media/crypto/aes_d... media/crypto/aes_decryptor.cc:276: bool AesDecryptor::DecryptionKey::Init(bool derive_hmac) { On 2012/07/13 00:48:00, ddorwin wrote: > derive_webm_keys > It's not really deriving an hmac (only), right? Right, I was just trying to keep webm name out of the class. But derive_webm_keys is more accurate. https://chromiumcodereview.appspot.com/10535029/diff/19009/media/crypto/aes_d... media/crypto/aes_decryptor.cc:280: if (!derive_hmac) { On 2012/07/13 00:48:00, ddorwin wrote: > nit: Prefer positive logic when there is an if-else. Done. https://chromiumcodereview.appspot.com/10535029/diff/19009/media/crypto/aes_d... File media/crypto/aes_decryptor.h (right): https://chromiumcodereview.appspot.com/10535029/diff/19009/media/crypto/aes_d... media/crypto/aes_decryptor.h:68: bool initialized() { return initialized_; } On 2012/07/13 00:48:00, ddorwin wrote: > is_initialized for both. Done. https://chromiumcodereview.appspot.com/10535029/diff/19009/media/crypto/aes_d... media/crypto/aes_decryptor.h:69: base::StringPiece hmac_key() { return base::StringPiece(hmac_key_); } On 2012/07/13 00:48:00, ddorwin wrote: > nit: swap with below since it is common. swap the members too. Done. https://chromiumcodereview.appspot.com/10535029/diff/19009/media/crypto/aes_d... media/crypto/aes_decryptor.h:73: // Flag telling if the object was initialized. On 2012/07/13 00:48:00, ddorwin wrote: > unnecessary Done. https://chromiumcodereview.appspot.com/10535029/diff/19009/media/crypto/aes_d... File media/crypto/aes_decryptor_unittest.cc (right): https://chromiumcodereview.appspot.com/10535029/diff/19009/media/crypto/aes_d... media/crypto/aes_decryptor_unittest.cc:41: const WebmEncryptedData kEncryptedFrames[] = { On 2012/07/13 00:48:00, ddorwin wrote: > We should probably change all these frame constants to contain "WebM" in the > name. Done. https://chromiumcodereview.appspot.com/10535029/diff/19009/media/crypto/aes_d... media/crypto/aes_decryptor_unittest.cc:151: static std::string GenerateCounterBlock(const uint8* iv, int iv_size) { On 2012/07/13 00:48:00, ddorwin wrote: > Why is this here now? Does AesDecryptor assume 16 bytes now? Do we check that? > Has the meaning of the DecryptConfig parameter changed? I don't recall seeing > any comments about that. I was debating whether I should add this function here or just define the WebM IV in the last patch. I added the function here because this test is also performing some tasks handled in the demuxer with creating the encrypted buffer. I could define the data such that it will look more like when it comes form the demuxer. I.e. the encrypted data will only the IV and the data, not the HMAC. As for the comments, I added a comment to the patch message, but I also added a comment to DecryptConfig now. https://chromiumcodereview.appspot.com/10535029/diff/19009/media/filters/ffmp... File media/filters/ffmpeg_video_decoder_unittest.cc (right): https://chromiumcodereview.appspot.com/10535029/diff/19009/media/filters/ffmp... media/filters/ffmpeg_video_decoder_unittest.cc:419: uint8 counter_block[kDecryptionKeySize]; On 2012/07/13 00:48:00, ddorwin wrote: > Why was this a separate function in the other tests but not here? It seems the > code is duplicated several times here. > > Are CBs always the size of the key? Moved to one function. Yes CBs are always size of the key. https://chromiumcodereview.appspot.com/10535029/diff/19009/media/filters/ffmp... media/filters/ffmpeg_video_decoder_unittest.cc:430: counter_block, kDecryptionKeySize, On 2012/07/13 00:48:00, ddorwin wrote: > suggest arraysize(c_b). it's not obvious that the key size is the size of this > pointer. Not valid anymore. https://chromiumcodereview.appspot.com/10535029/diff/19009/media/webm/webm_cl... File media/webm/webm_cluster_parser.cc (right): https://chromiumcodereview.appspot.com/10535029/diff/19009/media/webm/webm_cl... media/webm/webm_cluster_parser.cc:20: if (iv_size <= 0 || iv_size > WebMClusterParser::kDecryptionKeySize) On 2012/07/13 00:48:00, ddorwin wrote: > so we do have a specific key size. are we not spec compliant? We have a specific key size. I.e. 128 bits. We have a specific Counter Block size. I.e. 16 bytes because we specify CTR and 128 bit key encryption. We also specify an IV size of 8 bytes. At first I had this in aes_decryptor and left in general because CENC supports 8 and 16 byte counter blocks (though they call them IVs which screws up the terms.) But now that this is in the muxer and we specify 8 bytes for the IV I will check that. https://chromiumcodereview.appspot.com/10535029/diff/19009/media/webm/webm_cl... media/webm/webm_cluster_parser.cc:239: std::string webm_iv = On 2012/07/13 00:48:00, ddorwin wrote: > not really an IV anymore is it? Done. https://chromiumcodereview.appspot.com/10535029/diff/19009/media/webm/webm_cl... media/webm/webm_cluster_parser.cc:240: GenerateCounterBlock(reinterpret_cast<const uint8*>(&iv), sizeof(iv)); On 2012/07/13 00:48:00, ddorwin wrote: > Why did you move this logic here? Is this what ISO does? Yeah. And ISO can generate 2 different types of counter blocks, but they will still be used the same and will be 16 bytes. Also the generation of the counter block is defined by WebM spec. https://chromiumcodereview.appspot.com/10535029/diff/19009/media/webm/webm_cl... File media/webm/webm_cluster_parser.h (right): https://chromiumcodereview.appspot.com/10535029/diff/19009/media/webm/webm_cl... media/webm/webm_cluster_parser.h:21: static const int kDecryptionKeySize = 16; On 2012/07/13 00:48:00, ddorwin wrote: > does not match the more permissive RFC, right? This matches the RFC. I.e. This must be 16 because we specify the encryption MUST be CTR and 128 bit. I think what you are referring to is the key_id which currently does not put a size restriction.
http://codereview.chromium.org/10535029/diff/38001/media/crypto/aes_decryptor.cc File media/crypto/aes_decryptor.cc (right): http://codereview.chromium.org/10535029/diff/38001/media/crypto/aes_decryptor... media/crypto/aes_decryptor.cc:290: } nit: return true here and eliminate the following "else". http://codereview.chromium.org/10535029/diff/38001/media/filters/ffmpeg_video... File media/filters/ffmpeg_video_decoder_unittest.cc (right): http://codereview.chromium.org/10535029/diff/38001/media/filters/ffmpeg_video... media/filters/ffmpeg_video_decoder_unittest.cc:226: std::string GenerateCounterBlock(uint64 iv) { It seems odd that we return std::string here and then later convert to const uint8*. We are doing unnecessary data copying and casting. Can we pass in a counter_block_size and return by scoped_array<unit8>, so that the caller knows what the size of the data in the returned scoped_array is? Or anything else that eliminates this extra copy and cast.
Thanks. Just minor comments. https://chromiumcodereview.appspot.com/10535029/diff/19009/media/crypto/aes_d... File media/crypto/aes_decryptor_unittest.cc (right): https://chromiumcodereview.appspot.com/10535029/diff/19009/media/crypto/aes_d... media/crypto/aes_decryptor_unittest.cc:151: static std::string GenerateCounterBlock(const uint8* iv, int iv_size) { On 2012/07/13 21:40:41, fgalligan1 wrote: > As for the comments, I added a comment to the patch message, but I also added a > comment to DecryptConfig now. What comment is that? If they are really all counter blocks, should we rename the parameters to DecryptConfig? I guess that might make it CTR-specific, but all of this is right now. https://chromiumcodereview.appspot.com/10535029/diff/38001/media/base/decrypt... File media/base/decrypt_config.h (right): https://chromiumcodereview.appspot.com/10535029/diff/38001/media/base/decrypt... media/base/decrypt_config.h:19: // |iv_size| must be 16 bytes as defined be WebM and ISO. |checksum| is the s/be/by https://chromiumcodereview.appspot.com/10535029/diff/38001/media/base/decrypt... media/base/decrypt_config.h:39: scoped_array<uint8> key_id_; const for all, at least for the sizes. Can the scoped_arrays be initialized in the initialization list so they can be const? https://chromiumcodereview.appspot.com/10535029/diff/38001/media/base/decrypt... media/base/decrypt_config.h:47: // be set to NULL for some formats. nit: remove "set to" https://chromiumcodereview.appspot.com/10535029/diff/38001/media/crypto/aes_d... File media/crypto/aes_decryptor.cc (right): https://chromiumcodereview.appspot.com/10535029/diff/38001/media/crypto/aes_d... media/crypto/aes_decryptor.cc:104: DCHECK_EQ(input.GetDecryptConfig()->iv_size(), 16); magic # https://chromiumcodereview.appspot.com/10535029/diff/38001/media/crypto/aes_d... media/crypto/aes_decryptor.cc:234: // TODO(fgalligan): Fire a need_key event here. Hmm, I just realized that for this event, we _only_ have a Key ID and not initData! For WebM, this is the same. But for ISO, they are different. I'll file a spec bug to evaluate this. Please add "and add a test" to your TODO. :) https://chromiumcodereview.appspot.com/10535029/diff/38001/media/crypto/aes_d... media/crypto/aes_decryptor.cc:249: return NULL; Until the open question above is resolved, let's fire an error. This actually needs to be a MediaError, which is different from KeyError(). Does returning NULL here cause this? If so, I'm not sure how we would have implemented dropping the frame and continuing. https://chromiumcodereview.appspot.com/10535029/diff/38001/media/crypto/aes_d... File media/crypto/aes_decryptor.h (right): https://chromiumcodereview.appspot.com/10535029/diff/38001/media/crypto/aes_d... media/crypto/aes_decryptor.h:65: // Creates the HMAC and encryption key. If |derive_webm_keys| is true then encryption key and HMAC https://chromiumcodereview.appspot.com/10535029/diff/38001/media/webm/webm_cl... File media/webm/webm_cluster_parser.cc (right): https://chromiumcodereview.appspot.com/10535029/diff/38001/media/webm/webm_cl... media/webm/webm_cluster_parser.cc:222: video_encryption_key_id_.get(); any idea why this is "video_"?
https://chromiumcodereview.appspot.com/10535029/diff/38001/media/crypto/aes_d... File media/crypto/aes_decryptor.cc (right): https://chromiumcodereview.appspot.com/10535029/diff/38001/media/crypto/aes_d... media/crypto/aes_decryptor.cc:249: return NULL; On 2012/07/14 00:50:31, ddorwin wrote: > Until the open question above is resolved, let's fire an error. This actually > needs to be a MediaError, which is different from KeyError(). Does returning > NULL here cause this? If so, I'm not sure how we would have implemented dropping > the frame and continuing. Currently when the decoder sees NULL it will fire a kDecryptError, which will be translated to a PIPELINE_ERROR_DECRYPT, which will trigger NetworkStateDecodeError (will be NetworkStateDecryptError in the future).
https://chromiumcodereview.appspot.com/10535029/diff/19009/media/crypto/aes_d... File media/crypto/aes_decryptor_unittest.cc (right): https://chromiumcodereview.appspot.com/10535029/diff/19009/media/crypto/aes_d... media/crypto/aes_decryptor_unittest.cc:151: static std::string GenerateCounterBlock(const uint8* iv, int iv_size) { On 2012/07/14 00:50:31, ddorwin wrote: > On 2012/07/13 21:40:41, fgalligan1 wrote: > > As for the comments, I added a comment to the patch message, but I also added > a > > comment to DecryptConfig now. > > What comment is that? > If they are really all counter blocks, should we rename the parameters to > DecryptConfig? I guess that might make it CTR-specific, but all of this is right > now. The issue is ISO named this IV in CENC where the CTR RFC calls this Counter Block. I'm fine with calling it counter block. I'm also fine with leaving it as it is. https://chromiumcodereview.appspot.com/10535029/diff/38001/media/base/decrypt... File media/base/decrypt_config.h (right): https://chromiumcodereview.appspot.com/10535029/diff/38001/media/base/decrypt... media/base/decrypt_config.h:19: // |iv_size| must be 16 bytes as defined be WebM and ISO. |checksum| is the On 2012/07/14 00:50:31, ddorwin wrote: > s/be/by Done. https://chromiumcodereview.appspot.com/10535029/diff/38001/media/base/decrypt... media/base/decrypt_config.h:39: scoped_array<uint8> key_id_; On 2012/07/14 00:50:31, ddorwin wrote: > const for all, at least for the sizes. Can the scoped_arrays be initialized in > the initialization list so they can be const? Const on the sizes. I can change the scoped_array, do you think having them const is better then performing the CHECK() on the sizes? https://chromiumcodereview.appspot.com/10535029/diff/38001/media/base/decrypt... media/base/decrypt_config.h:47: // be set to NULL for some formats. On 2012/07/14 00:50:31, ddorwin wrote: > nit: remove "set to" Done. https://chromiumcodereview.appspot.com/10535029/diff/38001/media/crypto/aes_d... File media/crypto/aes_decryptor.cc (right): https://chromiumcodereview.appspot.com/10535029/diff/38001/media/crypto/aes_d... media/crypto/aes_decryptor.cc:104: DCHECK_EQ(input.GetDecryptConfig()->iv_size(), 16); On 2012/07/14 00:50:31, ddorwin wrote: > magic # Done. https://chromiumcodereview.appspot.com/10535029/diff/38001/media/crypto/aes_d... media/crypto/aes_decryptor.cc:234: // TODO(fgalligan): Fire a need_key event here. On 2012/07/14 00:50:31, ddorwin wrote: > Hmm, I just realized that for this event, we _only_ have a Key ID and not > initData! For WebM, this is the same. But for ISO, they are different. I'll file > a spec bug to evaluate this. > > Please add "and add a test" to your TODO. :) Done. https://chromiumcodereview.appspot.com/10535029/diff/38001/media/crypto/aes_d... media/crypto/aes_decryptor.cc:249: return NULL; On 2012/07/14 01:02:31, xhwang wrote: > On 2012/07/14 00:50:31, ddorwin wrote: > > Until the open question above is resolved, let's fire an error. This actually > > needs to be a MediaError, which is different from KeyError(). Does returning > > NULL here cause this? If so, I'm not sure how we would have implemented > dropping > > the frame and continuing. > > Currently when the decoder sees NULL it will fire a kDecryptError, which will be > translated to a PIPELINE_ERROR_DECRYPT, which will trigger > NetworkStateDecodeError (will be NetworkStateDecryptError in the future). Is this fine for now? https://chromiumcodereview.appspot.com/10535029/diff/38001/media/crypto/aes_d... media/crypto/aes_decryptor.cc:290: } On 2012/07/14 00:05:22, xhwang wrote: > nit: return true here and eliminate the following "else". Done. https://chromiumcodereview.appspot.com/10535029/diff/38001/media/crypto/aes_d... File media/crypto/aes_decryptor.h (right): https://chromiumcodereview.appspot.com/10535029/diff/38001/media/crypto/aes_d... media/crypto/aes_decryptor.h:65: // Creates the HMAC and encryption key. If |derive_webm_keys| is true then On 2012/07/14 00:50:31, ddorwin wrote: > encryption key and HMAC Done. https://chromiumcodereview.appspot.com/10535029/diff/38001/media/filters/ffmp... File media/filters/ffmpeg_video_decoder_unittest.cc (right): https://chromiumcodereview.appspot.com/10535029/diff/38001/media/filters/ffmp... media/filters/ffmpeg_video_decoder_unittest.cc:226: std::string GenerateCounterBlock(uint64 iv) { On 2012/07/14 00:05:22, xhwang wrote: > It seems odd that we return std::string here and then later convert to const > uint8*. We are doing unnecessary data copying and casting. Can we pass in a > counter_block_size and return by scoped_array<unit8>, so that the caller knows > what the size of the data in the returned scoped_array is? Or anything else that > eliminates this extra copy and cast. Done. https://chromiumcodereview.appspot.com/10535029/diff/38001/media/webm/webm_cl... File media/webm/webm_cluster_parser.cc (right): https://chromiumcodereview.appspot.com/10535029/diff/38001/media/webm/webm_cl... media/webm/webm_cluster_parser.cc:222: video_encryption_key_id_.get(); On 2012/07/14 00:50:31, ddorwin wrote: > any idea why this is "video_"? No, besides the usual private member to the class.
LGTM with comments. Thanks. https://chromiumcodereview.appspot.com/10535029/diff/38001/media/base/decrypt... File media/base/decrypt_config.h (right): https://chromiumcodereview.appspot.com/10535029/diff/38001/media/base/decrypt... media/base/decrypt_config.h:39: scoped_array<uint8> key_id_; On 2012/07/16 23:51:42, fgalligan1 wrote: > On 2012/07/14 00:50:31, ddorwin wrote: > > const for all, at least for the sizes. Can the scoped_arrays be initialized in > > the initialization list so they can be const? > > Const on the sizes. I can change the scoped_array, do you think having them > const is better then performing the CHECK() on the sizes? Const just says the pointer won't change. Since this is really just a bucket of values with only getters, it seems appropriate that they are const. https://chromiumcodereview.appspot.com/10535029/diff/50002/media/base/decrypt... File media/base/decrypt_config.h (right): https://chromiumcodereview.appspot.com/10535029/diff/50002/media/base/decrypt... media/base/decrypt_config.h:17: // Size in bytes of a 128bit key. Replace with: // Keys are always 128 bits. https://chromiumcodereview.appspot.com/10535029/diff/50002/media/filters/ffmp... File media/filters/ffmpeg_video_decoder_unittest.cc (right): https://chromiumcodereview.appspot.com/10535029/diff/50002/media/filters/ffmp... media/filters/ffmpeg_video_decoder_unittest.cc:59: //static const int kDecryptionKeySize = 16; delete https://chromiumcodereview.appspot.com/10535029/diff/50002/media/webm/webm_cl... File media/webm/webm_cluster_parser.cc (right): https://chromiumcodereview.appspot.com/10535029/diff/50002/media/webm/webm_cl... media/webm/webm_cluster_parser.cc:16: // CTR IV appended with a CTR block counter. |iv| is an 8 byte CTR IV. // Always returns a valid pointer to a buffer of kDecryptionKeySize bytes.
LGTM On 2012/07/17 00:19:52, ddorwin wrote: > LGTM with comments. Thanks. > > https://chromiumcodereview.appspot.com/10535029/diff/38001/media/base/decrypt... > File media/base/decrypt_config.h (right): > > https://chromiumcodereview.appspot.com/10535029/diff/38001/media/base/decrypt... > media/base/decrypt_config.h:39: scoped_array<uint8> key_id_; > On 2012/07/16 23:51:42, fgalligan1 wrote: > > On 2012/07/14 00:50:31, ddorwin wrote: > > > const for all, at least for the sizes. Can the scoped_arrays be initialized > in > > > the initialization list so they can be const? > > > > Const on the sizes. I can change the scoped_array, do you think having them > > const is better then performing the CHECK() on the sizes? > > Const just says the pointer won't change. Since this is really just a bucket of > values with only getters, it seems appropriate that they are const. > > https://chromiumcodereview.appspot.com/10535029/diff/50002/media/base/decrypt... > File media/base/decrypt_config.h (right): > > https://chromiumcodereview.appspot.com/10535029/diff/50002/media/base/decrypt... > media/base/decrypt_config.h:17: // Size in bytes of a 128bit key. > Replace with: > // Keys are always 128 bits. > > https://chromiumcodereview.appspot.com/10535029/diff/50002/media/filters/ffmp... > File media/filters/ffmpeg_video_decoder_unittest.cc (right): > > https://chromiumcodereview.appspot.com/10535029/diff/50002/media/filters/ffmp... > media/filters/ffmpeg_video_decoder_unittest.cc:59: //static const int > kDecryptionKeySize = 16; > delete > > https://chromiumcodereview.appspot.com/10535029/diff/50002/media/webm/webm_cl... > File media/webm/webm_cluster_parser.cc (right): > > https://chromiumcodereview.appspot.com/10535029/diff/50002/media/webm/webm_cl... > media/webm/webm_cluster_parser.cc:16: // CTR IV appended with a CTR block > counter. |iv| is an 8 byte CTR IV. > // Always returns a valid pointer to a buffer of kDecryptionKeySize bytes.
https://chromiumcodereview.appspot.com/10535029/diff/38001/media/base/decrypt... File media/base/decrypt_config.h (right): https://chromiumcodereview.appspot.com/10535029/diff/38001/media/base/decrypt... media/base/decrypt_config.h:39: scoped_array<uint8> key_id_; On 2012/07/17 00:19:52, ddorwin wrote: > On 2012/07/16 23:51:42, fgalligan1 wrote: > > On 2012/07/14 00:50:31, ddorwin wrote: > > > const for all, at least for the sizes. Can the scoped_arrays be initialized > in > > > the initialization list so they can be const? > > > > Const on the sizes. I can change the scoped_array, do you think having them > > const is better then performing the CHECK() on the sizes? > > Const just says the pointer won't change. Since this is really just a bucket of > values with only getters, it seems appropriate that they are const. Done. https://chromiumcodereview.appspot.com/10535029/diff/50002/media/base/decrypt... File media/base/decrypt_config.h (right): https://chromiumcodereview.appspot.com/10535029/diff/50002/media/base/decrypt... media/base/decrypt_config.h:17: // Size in bytes of a 128bit key. On 2012/07/17 00:19:52, ddorwin wrote: > Replace with: > // Keys are always 128 bits. Done. https://chromiumcodereview.appspot.com/10535029/diff/50002/media/filters/ffmp... File media/filters/ffmpeg_video_decoder_unittest.cc (right): https://chromiumcodereview.appspot.com/10535029/diff/50002/media/filters/ffmp... media/filters/ffmpeg_video_decoder_unittest.cc:59: //static const int kDecryptionKeySize = 16; On 2012/07/17 00:19:52, ddorwin wrote: > delete Done. https://chromiumcodereview.appspot.com/10535029/diff/50002/media/webm/webm_cl... File media/webm/webm_cluster_parser.cc (right): https://chromiumcodereview.appspot.com/10535029/diff/50002/media/webm/webm_cl... media/webm/webm_cluster_parser.cc:16: // CTR IV appended with a CTR block counter. |iv| is an 8 byte CTR IV. On 2012/07/17 00:19:52, ddorwin wrote: > // Always returns a valid pointer to a buffer of kDecryptionKeySize bytes. Done.
https://chromiumcodereview.appspot.com/10535029/diff/51004/media/base/decrypt... File media/base/decrypt_config.cc (right): https://chromiumcodereview.appspot.com/10535029/diff/51004/media/base/decrypt... media/base/decrypt_config.cc:19: checksum_(new uint8[checksum_size]), This could be a 0-sized array. Something like the following would be better: checksum_(checksum_size > 0 ? new uint8[checksum_size] : NULL)
https://chromiumcodereview.appspot.com/10535029/diff/51004/media/base/decrypt... File media/base/decrypt_config.cc (right): https://chromiumcodereview.appspot.com/10535029/diff/51004/media/base/decrypt... media/base/decrypt_config.cc:19: checksum_(new uint8[checksum_size]), On 2012/07/17 16:57:45, ddorwin wrote: > This could be a 0-sized array. Something like the following would be better: > checksum_(checksum_size > 0 ? new uint8[checksum_size] : NULL) Done.
lgtm Assuming all but one change are due to the rebase.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fgalligan@chromium.org/10535029/47008
Change committed as 147169
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fgalligan@chromium.org/10535029/59005 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
