Remove the checksum/HMAC code from the decryptor.

We are removing checksum/HMAC in WebM spec for encrypted content.

xhwang's CL http://codereview.chromium.org/10918276 has been merged into this one. 10918276 CL updates the CDM interface, ClearKeyCdm, CdmWrapper and PPAPI code to reflect this change.

- Remove checksum from DecryptConfig.
- Remove HMAC from AesDecryptor.
- Update AesDecryptorTest as all decrypts will not fail but the
  data checks will.
- Remove FakeCheckSum from ffmpeg_video_decoder_unittest.
- Remove HMAC from WebMClusterParser.
- Remove checksum from ppapi_plugin_instance.

BUG=150014
TEST=All media_unittests pass.
TBR=dmichael

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=157322

[xhwang, 2012-09-18 00:37:11]

Comments for patch set 1.

http://codereview.chromium.org/10917308/diff/1/media/crypto/aes_decryptor.h
File media/crypto/aes_decryptor.h (right):

http://codereview.chromium.org/10917308/diff/1/media/crypto/aes_decryptor.h#n...
media/crypto/aes_decryptor.h:57: class DecryptionKey {
Do we still need an extra class DecryptionKey if it only contains the
decryption_key (which is a crypto::SymmetricKey)? No need to remove today, but
if we plan to remove this later, put a TODO here.

http://codereview.chromium.org/10917308/diff/1/media/crypto/aes_decryptor_uni...
File media/crypto/aes_decryptor_unittest.cc (right):

http://codereview.chromium.org/10917308/diff/1/media/crypto/aes_decryptor_uni...
media/crypto/aes_decryptor_unittest.cc:52: 0x10, 0x11, 0x12, 0x13,
Nit: why do we want these trailing commas?

http://codereview.chromium.org/10917308/diff/1/media/crypto/aes_decryptor_uni...
media/crypto/aes_decryptor_unittest.cc:284: void DecryptAndExpectDataMismatch(
For these two new functions. From the perspective of a black box test, is it
clear about which kind of failure we should check (among DataMisMatch,
SizeAndDataMisMatch and decrypting failure), without running these tests to see
what error actually happened? If not, we should not relay on the current
behavior of the crypto implementation.

http://codereview.chromium.org/10917308/diff/1/media/crypto/aes_decryptor_uni...
media/crypto/aes_decryptor_unittest.cc:447: TEST_F(AesDecryptorTest,
IvCheckFailure) {
Here and in the next test (DataCheckFailure), we are not checking the IV or data
anymore. Should we rename the tests to something like CorruptedIv and
CorruptedFrame?

http://codereview.chromium.org/10917308/diff/1/media/crypto/aes_decryptor_uni...
media/crypto/aes_decryptor_unittest.cc:505:
ASSERT_NO_FATAL_FAILURE(DecryptAndExpectSizeDataMismatch(
Here and below. Indentation seems weird. Break after "ASSERT_NO_FATAL_FAILURE("
?

http://codereview.chromium.org/10917308/diff/1/media/webm/webm_cluster_parser.cc
File media/webm/webm_cluster_parser.cc (right):

http://codereview.chromium.org/10917308/diff/1/media/webm/webm_cluster_parser...
media/webm/webm_cluster_parser.cc:234: 
Add a comment or TODO noting that even for unencrypted frame we are still
setting DecryptConfig?

[fgalligan1, 2012-09-18 01:11:36]

https://codereview.chromium.org/10917308/diff/1/media/crypto/aes_decryptor.h
File media/crypto/aes_decryptor.h (right):

https://codereview.chromium.org/10917308/diff/1/media/crypto/aes_decryptor.h#...
media/crypto/aes_decryptor.h:57: class DecryptionKey {
On 2012/09/18 00:37:11, xhwang wrote:
> Do we still need an extra class DecryptionKey if it only contains the
> decryption_key (which is a crypto::SymmetricKey)? No need to remove today, but
> if we plan to remove this later, put a TODO here.

Yeah we don't need it. But I didn't want to change too much right now. Added a
TODO.

https://codereview.chromium.org/10917308/diff/1/media/crypto/aes_decryptor_un...
File media/crypto/aes_decryptor_unittest.cc (right):

https://codereview.chromium.org/10917308/diff/1/media/crypto/aes_decryptor_un...
media/crypto/aes_decryptor_unittest.cc:52: 0x10, 0x11, 0x12, 0x13,
On 2012/09/18 00:37:11, xhwang wrote:
> Nit: why do we want these trailing commas?

Done.

https://codereview.chromium.org/10917308/diff/1/media/crypto/aes_decryptor_un...
media/crypto/aes_decryptor_unittest.cc:284: void DecryptAndExpectDataMismatch(
On 2012/09/18 00:37:11, xhwang wrote:
> For these two new functions. From the perspective of a black box test, is it
> clear about which kind of failure we should check (among DataMisMatch,
> SizeAndDataMisMatch and decrypting failure), without running these tests to
see
> what error actually happened? If not, we should not relay on the current
> behavior of the crypto implementation.
I think from a blackbox point of view we are good. If we change the decryption
key or the ciphertext, we know we should get back data that does not match
plaintext, but the size should be the same. 

If we change the size of ciphertext we know that we should get back data that
does not match plaintext and the size should be different.

We can't fail the decrypt anymore because as far as the decryptor is concerned
it produced valid decryptedtext given the ciphertext and key. Or were you
thinking of something else?

https://codereview.chromium.org/10917308/diff/1/media/crypto/aes_decryptor_un...
media/crypto/aes_decryptor_unittest.cc:447: TEST_F(AesDecryptorTest,
IvCheckFailure) {
On 2012/09/18 00:37:11, xhwang wrote:
> Here and in the next test (DataCheckFailure), we are not checking the IV or
data
> anymore. Should we rename the tests to something like CorruptedIv and
> CorruptedFrame?

Done.

https://codereview.chromium.org/10917308/diff/1/media/crypto/aes_decryptor_un...
media/crypto/aes_decryptor_unittest.cc:505:
ASSERT_NO_FATAL_FAILURE(DecryptAndExpectSizeDataMismatch(
On 2012/09/18 00:37:11, xhwang wrote:
> Here and below. Indentation seems weird. Break after
"ASSERT_NO_FATAL_FAILURE("
> ?

Done.

https://codereview.chromium.org/10917308/diff/1/media/webm/webm_cluster_parse...
File media/webm/webm_cluster_parser.cc (right):

https://codereview.chromium.org/10917308/diff/1/media/webm/webm_cluster_parse...
media/webm/webm_cluster_parser.cc:234: 
On 2012/09/18 00:37:11, xhwang wrote:
> Add a comment or TODO noting that even for unencrypted frame we are still
> setting DecryptConfig?
Added both.

[xhwang, 2012-09-18 01:22:42]

You also need to change the mp4 parser, eg mp4_stream_parser.cc:416 and
track_run_iterator.cc:442.

http://codereview.chromium.org/10917308/diff/1/media/crypto/aes_decryptor_uni...
File media/crypto/aes_decryptor_unittest.cc (right):

http://codereview.chromium.org/10917308/diff/1/media/crypto/aes_decryptor_uni...
media/crypto/aes_decryptor_unittest.cc:284: void DecryptAndExpectDataMismatch(
On 2012/09/18 01:11:36, fgalligan1 wrote:
> On 2012/09/18 00:37:11, xhwang wrote:
> > For these two new functions. From the perspective of a black box test, is it
> > clear about which kind of failure we should check (among DataMisMatch,
> > SizeAndDataMisMatch and decrypting failure), without running these tests to
> see
> > what error actually happened? If not, we should not relay on the current
> > behavior of the crypto implementation.
> I think from a blackbox point of view we are good. If we change the decryption
> key or the ciphertext, we know we should get back data that does not match
> plaintext, but the size should be the same. 
> 
> If we change the size of ciphertext we know that we should get back data that
> does not match plaintext and the size should be different.
> 
> We can't fail the decrypt anymore because as far as the decryptor is concerned
> it produced valid decryptedtext given the ciphertext and key. Or were you
> thinking of something else?

If it's determined behavior then it's fine. Thanks for clarifying this.

[fgalligan1, 2012-09-18 01:35:35]

On 2012/09/18 01:22:42, xhwang wrote:
> You also need to change the mp4 parser, eg mp4_stream_parser.cc:416 and
> track_run_iterator.cc:442.
> 
>
http://codereview.chromium.org/10917308/diff/1/media/crypto/aes_decryptor_uni...
> File media/crypto/aes_decryptor_unittest.cc (right):
> 
>
http://codereview.chromium.org/10917308/diff/1/media/crypto/aes_decryptor_uni...
> media/crypto/aes_decryptor_unittest.cc:284: void DecryptAndExpectDataMismatch(
> On 2012/09/18 01:11:36, fgalligan1 wrote:
> > On 2012/09/18 00:37:11, xhwang wrote:
> > > For these two new functions. From the perspective of a black box test, is
it
> > > clear about which kind of failure we should check (among DataMisMatch,
> > > SizeAndDataMisMatch and decrypting failure), without running these tests
to
> > see
> > > what error actually happened? If not, we should not relay on the current
> > > behavior of the crypto implementation.
> > I think from a blackbox point of view we are good. If we change the
decryption
> > key or the ciphertext, we know we should get back data that does not match
> > plaintext, but the size should be the same. 
> > 
> > If we change the size of ciphertext we know that we should get back data
that
> > does not match plaintext and the size should be different.
> > 
> > We can't fail the decrypt anymore because as far as the decryptor is
concerned
> > it produced valid decryptedtext given the ciphertext and key. Or were you
> > thinking of something else?
> 
> If it's determined behavior then it's fine. Thanks for clarifying this.
What project will compile the mp4 code?

[xhwang, 2012-09-18 02:00:06]

lgtm

[commit-bot: I haz the power, 2012-09-18 04:40:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fgalligan@chromium.org/10917308/22001

[commit-bot: I haz the power, 2012-09-18 04:40:18]

Presubmit check for 10917308-22001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    webkit/plugins/ppapi
    ppapi/api
    ppapi

Presubmit checks took 2.4s to calculate.

[commit-bot: I haz the power, 2012-09-18 04:43:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fgalligan@chromium.org/10917308/22001

[commit-bot: I haz the power, 2012-09-18 09:00:37]

Change committed as 157322