Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11139008:
Change WebM parser to treat IVs from encrypted WebM as raw data. (Closed)
|
Created:
8 years, 2 months ago by fgalligan1 Modified:
8 years, 2 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionChange WebM parser to treat IVs from encrypted WebM as raw data.
The encrypted WebM spec RFC changed to treat IVs as raw data.
BUG=155641
TEST=All media_unittests pass.
TBR=sky
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=162012
Patch Set 1 #
Total comments: 6
Patch Set 2 : Enable encrypted content_browsertests. #Patch Set 3 : Addressing comments. #
Total comments: 7
Patch Set 4 : Addressing comments. #
Total comments: 8
Patch Set 5 : Addressing comments #
Total comments: 4
Patch Set 6 : Addressing comments #
Total comments: 4
Patch Set 7 : Addressing comments #
Messages
Total messages: 20 (0 generated)
lgtm https://codereview.chromium.org/11139008/diff/1/media/crypto/aes_decryptor_un... File media/crypto/aes_decryptor_unittest.cc (right): https://codereview.chromium.org/11139008/diff/1/media/crypto/aes_decryptor_un... media/crypto/aes_decryptor_unittest.cc:172: counter_block.append(DecryptConfig::kDecryptionKeySize - iv.size(), 0); Why wasn't the scoping necessary before? https://codereview.chromium.org/11139008/diff/1/media/crypto/aes_decryptor_un... media/crypto/aes_decryptor_unittest.cc:434: scoped_refptr<DecoderBuffer> encrypted_data2 = Is there a reason the empty line was removed? https://codereview.chromium.org/11139008/diff/1/media/webm/webm_cluster_parse... File media/webm/webm_cluster_parser.cc (right): https://codereview.chromium.org/11139008/diff/1/media/webm/webm_cluster_parse... media/webm/webm_cluster_parser.cc:240: const std::string iv(reinterpret_cast<const char*>(data + data_offset), nit: We create a string just to then copy it to another string. If we just passed the pointer as before, we'd avoid a copy.
https://codereview.chromium.org/11139008/diff/1/media/crypto/aes_decryptor_un... File media/crypto/aes_decryptor_unittest.cc (right): https://codereview.chromium.org/11139008/diff/1/media/crypto/aes_decryptor_un... media/crypto/aes_decryptor_unittest.cc:172: counter_block.append(DecryptConfig::kDecryptionKeySize - iv.size(), 0); On 2012/10/13 00:26:17, ddorwin wrote: > Why wasn't the scoping necessary before? The old version defined the value at the top of the function for some reason. We are including decrypt_config.h so that seems like a better value to use. Also changed the function to perform one less copy. https://codereview.chromium.org/11139008/diff/1/media/crypto/aes_decryptor_un... media/crypto/aes_decryptor_unittest.cc:434: scoped_refptr<DecoderBuffer> encrypted_data2 = On 2012/10/13 00:26:17, ddorwin wrote: > Is there a reason the empty line was removed? No. I'll put it back to make less noise. https://codereview.chromium.org/11139008/diff/1/media/webm/webm_cluster_parse... File media/webm/webm_cluster_parser.cc (right): https://codereview.chromium.org/11139008/diff/1/media/webm/webm_cluster_parse... media/webm/webm_cluster_parser.cc:240: const std::string iv(reinterpret_cast<const char*>(data + data_offset), On 2012/10/13 00:26:17, ddorwin wrote: > nit: We create a string just to then copy it to another string. If we just > passed the pointer as before, we'd avoid a copy. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fgalligan@chromium.org/11139008/9002
Presubmit check for 11139008-9002 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
content/browser
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fgalligan@chromium.org/11139008/9002
Presubmit check for 11139008-9002 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
content/browser
lgtm % comments https://chromiumcodereview.appspot.com/11139008/diff/9002/media/webm/webm_clu... File media/webm/webm_cluster_parser.cc (right): https://chromiumcodereview.appspot.com/11139008/diff/9002/media/webm/webm_clu... media/webm/webm_cluster_parser.cc:20: std::string counter_block(reinterpret_cast<const char*>(iv), iv_size); Should we counter_block.resize(DecryptConfig::kDecryptionKeySize);? https://chromiumcodereview.appspot.com/11139008/diff/9002/media/webm/webm_clu... media/webm/webm_cluster_parser.cc:186: const uint8* data, int size) { There is no check that size is not negative. DCHECK is fine. https://chromiumcodereview.appspot.com/11139008/diff/9002/media/webm/webm_clu... media/webm/webm_cluster_parser.cc:230: uint8 signal_byte = data[0]; We don't check that size is >0 here or >8 at 239. Do we have guarantees somewhere? One check for all of these would be nice, but I'm not sure how.
PTAL https://chromiumcodereview.appspot.com/11139008/diff/9002/media/webm/webm_clu... File media/webm/webm_cluster_parser.cc (right): https://chromiumcodereview.appspot.com/11139008/diff/9002/media/webm/webm_clu... media/webm/webm_cluster_parser.cc:20: std::string counter_block(reinterpret_cast<const char*>(iv), iv_size); On 2012/10/13 17:21:27, ddorwin wrote: > Should we counter_block.resize(DecryptConfig::kDecryptionKeySize);? What about this? std::string counter_block(DecryptConfig::kDecryptionKeySize, 0); memcpy(&counter_block[0], reinterpret_cast<const char*>(iv), iv_size); https://chromiumcodereview.appspot.com/11139008/diff/9002/media/webm/webm_clu... media/webm/webm_cluster_parser.cc:186: const uint8* data, int size) { On 2012/10/13 17:21:27, ddorwin wrote: > There is no check that size is not negative. DCHECK is fine. Done. https://chromiumcodereview.appspot.com/11139008/diff/9002/media/webm/webm_clu... media/webm/webm_cluster_parser.cc:230: uint8 signal_byte = data[0]; On 2012/10/13 17:21:27, ddorwin wrote: > We don't check that size is >0 here or >8 at 239. Do we have guarantees > somewhere? One check for all of these would be nice, but I'm not sure how. I don't think we can do one check as we need to check the first byte to see if the IV is appended. I added 2 checks.
Thanks. LGTM % comments and memcpy solution. https://chromiumcodereview.appspot.com/11139008/diff/9002/media/webm/webm_clu... File media/webm/webm_cluster_parser.cc (right): https://chromiumcodereview.appspot.com/11139008/diff/9002/media/webm/webm_clu... media/webm/webm_cluster_parser.cc:20: std::string counter_block(reinterpret_cast<const char*>(iv), iv_size); On 2012/10/14 16:45:24, fgalligan1 wrote: > On 2012/10/13 17:21:27, ddorwin wrote: > > Should we counter_block.resize(DecryptConfig::kDecryptionKeySize);? > > What about this? > std::string counter_block(DecryptConfig::kDecryptionKeySize, 0); > memcpy(&counter_block[0], reinterpret_cast<const char*>(iv), iv_size); SGTM https://chromiumcodereview.appspot.com/11139008/diff/3004/media/webm/webm_clu... File media/webm/webm_cluster_parser.cc (right): https://chromiumcodereview.appspot.com/11139008/diff/3004/media/webm/webm_clu... media/webm/webm_cluster_parser.cc:187: DCHECK_GT(size, -1); DCHECK_GE(size, 0); seems better https://chromiumcodereview.appspot.com/11139008/diff/3004/media/webm/webm_clu... media/webm/webm_cluster_parser.cc:232: DVLOG(1) << "Got an encrypted block with no data."; It's not yet "encrypted", right?
https://chromiumcodereview.appspot.com/11139008/diff/3004/media/webm/webm_clu... File media/webm/webm_cluster_parser.cc (right): https://chromiumcodereview.appspot.com/11139008/diff/3004/media/webm/webm_clu... media/webm/webm_cluster_parser.cc:187: DCHECK_GT(size, -1); On 2012/10/15 02:32:49, ddorwin wrote: > DCHECK_GE(size, 0); > seems better Done. https://chromiumcodereview.appspot.com/11139008/diff/3004/media/webm/webm_clu... media/webm/webm_cluster_parser.cc:232: DVLOG(1) << "Got an encrypted block with no data."; On 2012/10/15 02:32:49, ddorwin wrote: > It's not yet "encrypted", right? No. I changed it too "Got a block from an encrypted stream with no data."
http://codereview.chromium.org/11139008/diff/3004/media/webm/webm_cluster_par... File media/webm/webm_cluster_parser.cc (right): http://codereview.chromium.org/11139008/diff/3004/media/webm/webm_cluster_par... media/webm/webm_cluster_parser.cc:16: // Generates a 16 byte CTR counter block. The CTR counter block format is a With |iv_size|, update this comment? http://codereview.chromium.org/11139008/diff/3004/media/webm/webm_constants.h File media/webm/webm_constants.h (right): http://codereview.chromium.org/11139008/diff/3004/media/webm/webm_constants.h... media/webm/webm_constants.h:205: const size_t kWebMIvSize = 8; Can we just use "int" for kWebMIvSize? "int" is used in src/media for sizes and we can eliminate a bunch of casts if we use int here. http://codereview.chromium.org/11139008/diff/2004/media/webm/webm_cluster_par... File media/webm/webm_cluster_parser.cc (right): http://codereview.chromium.org/11139008/diff/2004/media/webm/webm_cluster_par... media/webm/webm_cluster_parser.cc:21: memcpy(&counter_block[0], reinterpret_cast<const char*>(iv), iv_size); The &foo[0] trick only works for vector, whose internal memory is guaranteed to be contiguous. For std::string, we don't have this property and this memcpy is not correct. Can we keep using .append() for this? http://codereview.chromium.org/11139008/diff/2004/media/webm/webm_cluster_par... media/webm/webm_cluster_parser.cc:244: if (static_cast<size_t>(size) < sizeof(signal_byte) + kWebMIvSize) { Both sizeof(signal_byte) and sizeof(uint8) on line 231 are essentially "the size of the signal byte". Can we have a constant int kWebMSignalByteSize for it? Thus we can eliminate two sizeof and two static_cast here.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fgalligan@chromium.org/11139008/2004
Sorry, I unchecked the commit-bit as I feel we need to resolve the comments first before committing this.
I'm fine with going back to append. The only reason I suggested that was because another reviewer forced me to use it on another cl. I can't check right now but does the encryptor code do something like this to write to a string with a const cast? Sent from Android. On Oct 15, 2012 9:39 AM, <xhwang@chromium.org> wrote: > > http://codereview.chromium.**org/11139008/diff/3004/media/** > webm/webm_cluster_parser.cc<http://codereview.chromium.org/11139008/diff/3004/media/webm/webm_cluster_parser.cc> > File media/webm/webm_cluster_**parser.cc (right): > > http://codereview.chromium.**org/11139008/diff/3004/media/** > webm/webm_cluster_parser.cc#**newcode16<http://codereview.chromium.org/11139008/diff/3004/media/webm/webm_cluster_parser.cc#newcode16> > media/webm/webm_cluster_**parser.cc:16: // Generates a 16 byte CTR counter > block. The CTR counter block format is a > With |iv_size|, update this comment? > > http://codereview.chromium.**org/11139008/diff/3004/media/** > webm/webm_constants.h<http://codereview.chromium.org/11139008/diff/3004/media/webm/webm_constants.h> > File media/webm/webm_constants.h (right): > > http://codereview.chromium.**org/11139008/diff/3004/media/** > webm/webm_constants.h#**newcode205<http://codereview.chromium.org/11139008/diff/3004/media/webm/webm_constants.h#newcode205> > media/webm/webm_constants.h:**205: const size_t kWebMIvSize = 8; > Can we just use "int" for kWebMIvSize? "int" is used in src/media for > sizes and we can eliminate a bunch of casts if we use int here. > > http://codereview.chromium.**org/11139008/diff/2004/media/** > webm/webm_cluster_parser.cc<http://codereview.chromium.org/11139008/diff/2004/media/webm/webm_cluster_parser.cc> > File media/webm/webm_cluster_**parser.cc (right): > > http://codereview.chromium.**org/11139008/diff/2004/media/** > webm/webm_cluster_parser.cc#**newcode21<http://codereview.chromium.org/11139008/diff/2004/media/webm/webm_cluster_parser.cc#newcode21> > media/webm/webm_cluster_**parser.cc:21: memcpy(&counter_block[0], > reinterpret_cast<const char*>(iv), iv_size); > The &foo[0] trick only works for vector, whose internal memory is > guaranteed to be contiguous. For std::string, we don't have this > property and this memcpy is not correct. Can we keep using .append() for > this? > > http://codereview.chromium.**org/11139008/diff/2004/media/** > webm/webm_cluster_parser.cc#**newcode244<http://codereview.chromium.org/11139008/diff/2004/media/webm/webm_cluster_parser.cc#newcode244> > media/webm/webm_cluster_**parser.cc:244: if (static_cast<size_t>(size) < > sizeof(signal_byte) + kWebMIvSize) { > Both sizeof(signal_byte) and sizeof(uint8) on line 231 are essentially > "the size of the signal byte". Can we have a constant int > kWebMSignalByteSize for it? Thus we can eliminate two sizeof and two > static_cast here. > > http://codereview.chromium.**org/11139008/<http://codereview.chromium.org/111... >
Np. I will update when I get back in. Sent from Android. On Oct 15, 2012 9:51 AM, <xhwang@chromium.org> wrote: > Sorry, I unchecked the commit-bit as I feel we need to resolve the comments > first before committing this. > > http://codereview.chromium.**org/11139008/<http://codereview.chromium.org/111... >
https://chromiumcodereview.appspot.com/11139008/diff/3004/media/webm/webm_clu... File media/webm/webm_cluster_parser.cc (right): https://chromiumcodereview.appspot.com/11139008/diff/3004/media/webm/webm_clu... media/webm/webm_cluster_parser.cc:16: // Generates a 16 byte CTR counter block. The CTR counter block format is a On 2012/10/15 16:39:00, xhwang wrote: > With |iv_size|, update this comment? Done. https://chromiumcodereview.appspot.com/11139008/diff/3004/media/webm/webm_con... File media/webm/webm_constants.h (right): https://chromiumcodereview.appspot.com/11139008/diff/3004/media/webm/webm_con... media/webm/webm_constants.h:205: const size_t kWebMIvSize = 8; On 2012/10/15 16:39:00, xhwang wrote: > Can we just use "int" for kWebMIvSize? "int" is used in src/media for sizes and > we can eliminate a bunch of casts if we use int here. Done. https://chromiumcodereview.appspot.com/11139008/diff/2004/media/webm/webm_clu... File media/webm/webm_cluster_parser.cc (right): https://chromiumcodereview.appspot.com/11139008/diff/2004/media/webm/webm_clu... media/webm/webm_cluster_parser.cc:21: memcpy(&counter_block[0], reinterpret_cast<const char*>(iv), iv_size); On 2012/10/15 16:39:00, xhwang wrote: > The &foo[0] trick only works for vector, whose internal memory is guaranteed to > be contiguous. For std::string, we don't have this property and this memcpy is > not correct. Can we keep using .append() for this? Done. https://chromiumcodereview.appspot.com/11139008/diff/2004/media/webm/webm_clu... media/webm/webm_cluster_parser.cc:244: if (static_cast<size_t>(size) < sizeof(signal_byte) + kWebMIvSize) { On 2012/10/15 16:39:00, xhwang wrote: > Both sizeof(signal_byte) and sizeof(uint8) on line 231 are essentially "the size > of the signal byte". Can we have a constant int kWebMSignalByteSize for it? Thus > we can eliminate two sizeof and two static_cast here. Done.
lgtm % nits https://chromiumcodereview.appspot.com/11139008/diff/1007/media/crypto/aes_de... File media/crypto/aes_decryptor_unittest.cc (right): https://chromiumcodereview.appspot.com/11139008/diff/1007/media/crypto/aes_de... media/crypto/aes_decryptor_unittest.cc:191: int data_offset = kWebMSignalByteSize; nit: dcheck that kWebMSignalByteSize == 1 before we do "uint8 signal_byte = data[0];"? https://chromiumcodereview.appspot.com/11139008/diff/1007/media/webm/webm_clu... File media/webm/webm_cluster_parser.cc (right): https://chromiumcodereview.appspot.com/11139008/diff/1007/media/webm/webm_clu... media/webm/webm_cluster_parser.cc:236: uint8 signal_byte = data[0]; nit: same here.
https://chromiumcodereview.appspot.com/11139008/diff/1007/media/crypto/aes_de... File media/crypto/aes_decryptor_unittest.cc (right): https://chromiumcodereview.appspot.com/11139008/diff/1007/media/crypto/aes_de... media/crypto/aes_decryptor_unittest.cc:191: int data_offset = kWebMSignalByteSize; On 2012/10/15 20:09:04, xhwang wrote: > nit: dcheck that kWebMSignalByteSize == 1 before we do "uint8 signal_byte = > data[0];"? Done. https://chromiumcodereview.appspot.com/11139008/diff/1007/media/webm/webm_clu... File media/webm/webm_cluster_parser.cc (right): https://chromiumcodereview.appspot.com/11139008/diff/1007/media/webm/webm_clu... media/webm/webm_cluster_parser.cc:236: uint8 signal_byte = data[0]; On 2012/10/15 20:09:04, xhwang wrote: > nit: same here. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fgalligan@chromium.org/11139008/6006
Change committed as 162012 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
