Replace memcmp() with HMAC.VerifyTruncated() in aes_decryptor.cc

media/crypto/aes_decryptor.cc

Index: media/crypto/aes_decryptor.cc
diff --git a/media/crypto/aes_decryptor.cc b/media/crypto/aes_decryptor.cc
index e69f4062a8afb3f00ee9d609a19785da40269efe..19514cad2bb47268fd741c1715a3ead8de01736b 100644
--- a/media/crypto/aes_decryptor.cc
+++ b/media/crypto/aes_decryptor.cc
@@ -66,21 +66,17 @@ static bool CheckData(const DecoderBuffer& input,
   if (!hmac.Init(hmac_key))
     return false;
 
+  DCHECK(input.GetDecryptConfig()->checksum_size() <=

[Ryan Sleevi, 2012/07/24 05:49:56]

nit: DCHECK_LE

[fgalligan1, 2012/07/24 15:27:25]

Done.

+         static_cast<int>(hmac.DigestLength()));
+
   // The HMAC covers the IV and the frame data.
   base::StringPiece data_to_check(
       reinterpret_cast<const char*>(input.GetData()), input.GetDataSize());
+  base::StringPiece digest(
+      reinterpret_cast<const char*>(input.GetDecryptConfig()->checksum()),

[Ryan Sleevi, 2012/07/24 05:49:56]

Are there checks elsewhere that make sure that
input.GetDecryptConfig()->checksum_size() is some sane value (eg: the 96-bits
proposed in the current draft)?

Just wondering if an attacker could, say, substitute just 1 byte instead.

If there are, it might be worth adding as a comment.

[fgalligan1, 2012/07/24 15:27:25]

In webm_cluster_parser we explicitly set the size to 96 bits. But there is not a
check after that until the check to make sure that the size <= DigestLength.

Are you worried that an attacker could append data to the digest? Wouldn't an
attacker changing any data with the digest cause an integrity check failure?

[Ryan Sleevi, 2012/07/24 15:45:25]

No, quite the opposite. I was wondering if the attacker could craft a malicious
packet, such that the checksum was lifted from a legitimate packet, but then
truncated to a single byte (or single bit, if you're dealing in bits).

Because VerifyTruncated() will accept any truncated inputs, it's up to the
caller to ensure that there is not "over" truncation.

Legitimate packet:
[IV][Data][HMAC-12bytes]

Malicious Packet:
[IV][Malicious Data][HMAC-1byte]

+      input.GetDecryptConfig()->checksum_size());
 
-  scoped_array<uint8> calculated_hmac(new uint8[hmac.DigestLength()]);
-  if (!hmac.Sign(data_to_check, calculated_hmac.get(), hmac.DigestLength()))
-    return false;
-
-  DCHECK(input.GetDecryptConfig()->checksum_size() <=
-         static_cast<int>(hmac.DigestLength()));
-  if (memcmp(input.GetDecryptConfig()->checksum(),
-             calculated_hmac.get(),
-             input.GetDecryptConfig()->checksum_size()) != 0)
-    return false;
-  return true;
+  return hmac.VerifyTruncated(data_to_check, digest);
 }
 
 // Decrypts |input| using |key|. |encrypted_data_offset| is the number of bytes