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

media/crypto/aes_decryptor.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/crypto/aes_decryptor.h"
 
 #include "base/logging.h"
 #include "base/stl_util.h"
 #include "base/string_number_conversions.h"
 #include "crypto/encryptor.h"
@@ skipping 48 matching lines @@
                       const base::StringPiece& hmac_key) {
   CHECK(input.GetDataSize());
   CHECK(input.GetDecryptConfig());
   CHECK_GT(input.GetDecryptConfig()->checksum_size(), 0);
   CHECK(!hmac_key.empty());
 
   crypto::HMAC hmac(crypto::HMAC::SHA1);
   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
 // into |input| that the encrypted data starts.
 // Returns a DecoderBuffer with the decrypted data if decryption succeeded or
 // NULL if decryption failed.
 static scoped_refptr<DecoderBuffer> DecryptData(const DecoderBuffer& input,
                                                 crypto::SymmetricKey* key,
                                                 int encrypted_data_offset) {
   CHECK(input.GetDataSize());
@@ skipping 205 matching lines @@
 
   decryption_key_.reset(
       crypto::SymmetricKey::Import(crypto::SymmetricKey::AES, secret_));
   if (!decryption_key_.get()) {
     return false;
   }
   return true;
 }
 
 }  // namespace media