Add support for encrypted WebM files as defined in the RFC.

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 129bc330131585c2a77a66dc7ab8b96fb96c76e6..4d860f03af8c60b6eceb4062e07d728869a26f8b 100644
--- a/media/crypto/aes_decryptor.cc
+++ b/media/crypto/aes_decryptor.cc
@@ -9,6 +9,7 @@
 #include "base/string_number_conversions.h"
 #include "base/string_piece.h"
 #include "crypto/encryptor.h"
+#include "crypto/hmac.h"
 #include "crypto/symmetric_key.h"
 #include "media/base/decoder_buffer.h"
 #include "media/base/decrypt_config.h"
@@ -16,31 +17,132 @@
 
 namespace media {
 
-// TODO(xhwang): Get real IV from frames.
-static const char kInitialCounter[] = "0000000000000000";
-
 uint32 AesDecryptor::next_session_id_ = 1;
 
-// Decrypt |input| using |key|.
+// Derives a key from an HMAC using SHA1. |secret| is the base secret to derive

[ddorwin, 2012/07/10 01:12:20]

s/from an HMAC using SHA1/using a SHA1 HMAC/ ?

[fgalligan1, 2012/07/11 22:06:33]

Done.

+// the key from. |seed| is the kwnon input to the HMAC. |key_size| is how many

[ddorwin, 2012/07/10 01:12:20]

known

[fgalligan1, 2012/07/11 22:06:33]

Done.

+// bytes are returned in the key. Returns a string containing the key on
+// success. Returns an empty string on failure.
+static std::string DeriveKey(const std::string& secret,
+                             const std::string& seed,
+                             int key_size) {

[xhwang, 2012/07/10 06:31:25]

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.

[fgalligan1, 2012/07/11 22:06:33]

Done.

+  CHECK(!secret.empty());
+  CHECK(!seed.empty());
+  CHECK_GT(key_size, 0);
+
+  std::string key;
+  crypto::HMAC hmac(crypto::HMAC::SHA1);
+  if (!hmac.Init(reinterpret_cast<const uint8*>(secret.data()),
+                 secret.size())) {
+    DVLOG(1) << "Could not initialize HMAC with secret data.";
+    return key;

[ddorwin, 2012/07/10 01:12:20]

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.

[fgalligan1, 2012/07/11 22:06:33]

Done.

+  }
+
+  uint8 calculated_hmac[AesDecryptor::kSha1DigestSize];
+  if (!hmac.Sign(seed, calculated_hmac, AesDecryptor::kSha1DigestSize)) {
+    DVLOG(1) << "Could not calculate HMAC.";
+    return key;
+  }
+  key.assign(reinterpret_cast<const char*>(calculated_hmac), key_size);

[ddorwin, 2012/07/10 01:12:20]

empty line for consistency

[fgalligan1, 2012/07/11 22:06:33]

Done.

+  return key;

[ddorwin, 2012/07/10 01:12:20]

You can combine 33, 46-47 into
return std::string(...);

[fgalligan1, 2012/07/11 22:06:33]

Done.

+}
+
+// Integrity check of |input|'s data. Checks that the first
+// |kWebMIntegrityCheckSize| in bytes of |input| matches the rest of the data

[ddorwin, 2012/07/10 01:12:20]

Comment is out of date.

[fgalligan1, 2012/07/11 22:06:33]

Done.

+// in |input|. The check is using the SHA1 algorithm. |hmac_key| is the key of
+// the HMAC algorithm. Returns true if the integrity check passes.
+static bool CheckData(const DecoderBuffer& input,
+                      const std::string& hmac_key) {

[xhwang, 2012/07/10 06:31:25]

ditto, pass |hmac_key| as string piece.

[fgalligan1, 2012/07/11 22:06:33]

Done.

+  CHECK(input.GetDataSize());
+  CHECK(input.GetDecryptConfig());
+  CHECK(!hmac_key.empty());
+
+  crypto::HMAC hmac(crypto::HMAC::SHA1);
+  if (!hmac.Init(reinterpret_cast<const uint8*>(hmac_key.data()),
+                 hmac_key.size())) {
+    DVLOG(1) << "Could not initialize HMAC.";

[ddorwin, 2012/07/10 01:12:20]

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.

[fgalligan1, 2012/07/11 22:06:33]

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().

+    return false;
+  }
+
+  // The HMAC covers the IV and the frame data.
+  base::StringPiece data_to_check(
+      reinterpret_cast<const char*>(input.GetData()), input.GetDataSize());
+
+  uint8 calculated_hmac[AesDecryptor::kSha1DigestSize];
+  if (!hmac.Sign(data_to_check,
+                 calculated_hmac,
+                 AesDecryptor::kSha1DigestSize)) {

[ddorwin, 2012/07/10 01:12:20]

arraysize(calculated_hmac)?

[fgalligan1, 2012/07/11 22:06:33]

Done.

+    DVLOG(1) << "Could not calculate HMAC.";
+    return false;
+  }
+
+  if (memcmp(input.GetDecryptConfig()->data_to_verify(),

[ddorwin, 2012/07/10 01:12:20]

"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?

[fgalligan1, 2012/07/11 22:06:33]

I changed the name to "checksum". Sound better to you?

+             calculated_hmac,
+             input.GetDecryptConfig()->data_to_verify_size()) != 0) {

[ddorwin, 2012/07/10 01:12:20]

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?

[fgalligan1, 2012/07/11 22:06:33]

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?

+    DVLOG(1) << "Integrity check failure.";
+    return false;
+  }
+  return true;
+}
+
+// Generates a 16 byte CTR counter block. The format is
+// | iv | block counter |. |iv| is a CTR IV. |iv_size| is the size

[xhwang, 2012/07/10 06:31:25]

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.

[fgalligan1, 2012/07/11 22:06:33]

Changed too "Generates a 16 byte CTR counter block. The CTR counter block format
is a CTR IV appended with a CTR block counter."

+// of |iv| in bytes. Returns counter block on success. Returns empty string
+// on failure.
+static std::string GenerateCounterBlock(const uint8* iv, int iv_size) {
+  std::string counter_block;
+  if (iv_size <= 0 || iv_size > AesDecryptor::kKeySize)
+    return counter_block;

[ddorwin, 2012/07/10 01:12:20]

same - return empty string and return a constructed string at 108.

[fgalligan1, 2012/07/11 22:06:33]

Done.

+
+  char counter_block_data[AesDecryptor::kKeySize];
+
+  // Set the IV.
+  memcpy(counter_block_data, iv, iv_size);
+
+  // Set block counter to all 0's.

[ddorwin, 2012/07/10 01:12:20]

Is this comment accurate? Or are we just "zero-extending" the unused portion of
the counter block?

[fgalligan1, 2012/07/11 22:06:33]

Yes and Yes. It probably is confusing because "block counter" is a part of
"counter block".

+  memset(counter_block_data + iv_size,
+         0,
+         AesDecryptor::kKeySize - iv_size);

[ddorwin, 2012/07/10 01:12:20]

Is it okay for the size to be 0?

[fgalligan1, 2012/07/11 22:06:33]

Should be.

+
+  counter_block.assign(counter_block_data, AesDecryptor::kKeySize);
+  return counter_block;
+}
+
+// Decrypt |input| using |key|. |offset| is the number of bytes into |input|

[ddorwin, 2012/07/10 01:12:20]

s/offset/encrypted_data_offset/ (or just data_offset).

[fgalligan1, 2012/07/11 22:06:33]

Done.

+// the encrypted data is.

[ddorwin, 2012/07/10 01:12:20]

that the encrypted data starts.

[fgalligan1, 2012/07/11 22:06:33]

Done.

 // Return a DecoderBuffer with the decrypted data if decryption succeeded.
 // Return NULL if decryption failed.
 static scoped_refptr<DecoderBuffer> DecryptData(const DecoderBuffer& input,
-                                                crypto::SymmetricKey* key) {
+                                                crypto::SymmetricKey* key,
+                                                int offset) {
   CHECK(input.GetDataSize());
+  CHECK(input.GetDecryptConfig());
   CHECK(key);
 
   // Initialize encryption data.
-  // The IV must be exactly as long as the cipher block size.
   crypto::Encryptor encryptor;
-  if (!encryptor.Init(key, crypto::Encryptor::CBC, kInitialCounter)) {
+  if (!encryptor.Init(key, crypto::Encryptor::CTR, "")) {
     DVLOG(1) << "Could not initialize encryptor.";
     return NULL;
   }
 
+  // Set the counter block.
+  std::string counter_block =
+      GenerateCounterBlock(input.GetDecryptConfig()->iv(),
+                           input.GetDecryptConfig()->iv_size());
+  if (counter_block.empty()) {
+    DVLOG(1) << "Could not generate counter block.";
+    return NULL;
+  }
+  if (!encryptor.SetCounter(counter_block)) {
+    DVLOG(1) << "Could not set counter block.";
+    return NULL;
+  }
+
   std::string decrypted_text;
-  base::StringPiece encrypted_text(
-      reinterpret_cast<const char*>(input.GetData()),
-      input.GetDataSize());
+  const char* frame = reinterpret_cast<const char*>(input.GetData() + offset);
+  int frame_size = input.GetDataSize() - offset;
+  base::StringPiece encrypted_text(frame, frame_size);
   if (!encryptor.Decrypt(encrypted_text, &decrypted_text)) {
     DVLOG(1) << "Could not decrypt data.";
     return NULL;
@@ -52,12 +154,15 @@ static scoped_refptr<DecoderBuffer> DecryptData(const DecoderBuffer& input,
       decrypted_text.size());
 }
 
+const char AesDecryptor::kHmacSeed[] = "hmac-key";
+const char AesDecryptor::kEncryptionSeed[] = "encryption-key";
+
 AesDecryptor::AesDecryptor(DecryptorClient* client)
     : client_(client) {
 }
 
 AesDecryptor::~AesDecryptor() {
-  STLDeleteValues(&key_map_);
+  STLDeleteValues(&keys_map_);
 }
 
 void AesDecryptor::GenerateKeyRequest(const std::string& key_system,
@@ -106,22 +211,27 @@ void AesDecryptor::AddKey(const std::string& key_system,
   std::string key_id_string(reinterpret_cast<const char*>(init_data),
                             init_data_length);
   std::string key_string(reinterpret_cast<const char*>(key) , key_length);
-  crypto::SymmetricKey* symmetric_key = crypto::SymmetricKey::Import(
-      crypto::SymmetricKey::AES, key_string);
-  if (!symmetric_key) {
-    DVLOG(1) << "Could not import key.";
+  HmacEncryptionKeys* keys = new HmacEncryptionKeys(key_string);

[ddorwin, 2012/07/10 01:12:20]

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.

[xhwang, 2012/07/10 06:31:25]

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()".

[fgalligan1, 2012/07/11 22:06:33]

I can't Pass() to the map. Can I move scoped_ptr<> to * or should I make
HmacEncryptionKeys derive from scoped_refptr?

[fgalligan1, 2012/07/11 22:06:33]

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. 

[ddorwin, 2012/07/13 00:48:00]

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.

[ddorwin, 2012/07/13 00:48:00]

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.)

[xhwang, 2012/07/13 01:23:21]

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!

[xhwang, 2012/07/13 01:28:10]

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.

+  if (!keys) {
+    DVLOG(1) << "Could not create keys.";
+    client_->KeyError(key_system, session_id, Decryptor::kUnknownError, 0);
+    return;
+  }
+  if (!keys->Init()) {
+    delete keys;
+    DVLOG(1) << "Could not create keys.";
     client_->KeyError(key_system, session_id, Decryptor::kUnknownError, 0);
     return;
   }
 
   {
-    base::AutoLock auto_lock(key_map_lock_);
-    KeyMap::iterator found = key_map_.find(key_id_string);
-    if (found != key_map_.end()) {
+    base::AutoLock auto_lock(keys_map_lock_);
+    KeysMap::iterator found = keys_map_.find(key_id_string);
+    if (found != keys_map_.end()) {
       delete found->second;
-      key_map_.erase(found);
+      keys_map_.erase(found);
     }
-    key_map_[key_id_string] = symmetric_key;
+    keys_map_[key_id_string] = keys;
   }
 
   client_->KeyAdded(key_system, session_id);
@@ -140,19 +250,28 @@ scoped_refptr<DecoderBuffer> AesDecryptor::Decrypt(
   // TODO(xhwang): Avoid always constructing a string with StringPiece?
   std::string key_id_string(reinterpret_cast<const char*>(key_id), key_id_size);
 
-  crypto::SymmetricKey* key = NULL;
+  HmacEncryptionKeys* keys = NULL;
   {
-    base::AutoLock auto_lock(key_map_lock_);
-    KeyMap::const_iterator found = key_map_.find(key_id_string);
-    if (found == key_map_.end()) {
+    base::AutoLock auto_lock(keys_map_lock_);
+    KeysMap::const_iterator found = keys_map_.find(key_id_string);
+    if (found == keys_map_.end()) {
       DVLOG(1) << "Could not find a matching key for given key ID.";
       return NULL;
     }
-    key = found->second;
+    keys = found->second;
   }
 
-  scoped_refptr<DecoderBuffer> decrypted = DecryptData(*encrypted, key);
+  int verify_size = encrypted->GetDecryptConfig()->data_to_verify_size();
+  if (verify_size > 0 && !CheckData(*encrypted, keys->hmac_key())) {

[ddorwin, 2012/07/10 01:12:20]

Might have to check that hmac_key() is not NULL here too for ISO.

[fgalligan1, 2012/07/11 22:06:33]

Currently ISO does not define an IC. If they do we can add it.

[ddorwin, 2012/07/13 00:48:00]

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.

+    DVLOG(1) << "Integrity check failed.";
+    // TODO(fgalligan): Should we signal a decryptor error here?

[ddorwin, 2012/07/10 01:12:20]

I think so. This was clearly a problem related to decryption (verification
failure).

[fgalligan1, 2012/07/11 22:06:33]

I updated the comment with an explanation of what should happen with an IC
failure.

+    return NULL;
+  }
 
+  scoped_refptr<DecoderBuffer> decrypted =
+      DecryptData(*encrypted,
+                  keys->encryption_key(),
+                  encrypted->GetDecryptConfig()->offset_to_data());
   if (decrypted) {
     decrypted->SetTimestamp(encrypted->GetTimestamp());
     decrypted->SetDuration(encrypted->GetDuration());
@@ -161,4 +280,36 @@ scoped_refptr<DecoderBuffer> AesDecryptor::Decrypt(
   return decrypted;
 }
 
+AesDecryptor::HmacEncryptionKeys::HmacEncryptionKeys(
+    const std::string& secret)
+    : secret_(secret) {
+}
+
+AesDecryptor::HmacEncryptionKeys::~HmacEncryptionKeys() {}
+
+bool AesDecryptor::HmacEncryptionKeys::Init() {
+  CHECK(!secret_.empty());
+
+  std::string raw_key = DeriveKey(secret_,
+                                  kEncryptionSeed,
+                                  secret_.length());
+  if (raw_key.empty()) {
+    DVLOG(1) << "Could not create encryption key.";
+    return false;
+  }
+  encryption_key_.reset(crypto::SymmetricKey::Import(crypto::SymmetricKey::AES,
+                                                     raw_key));
+  if (!encryption_key_.get()) {
+    DVLOG(1) << "Could not create encryption key.";

[ddorwin, 2012/07/10 01:12:20]

"... import decryption key."
but probably just delete.

[fgalligan1, 2012/07/11 22:06:33]

Done.

+    return false;
+  }
+
+  hmac_key_ = DeriveKey(secret_, kHmacSeed, kSha1DigestSize);
+  if (hmac_key_.empty()) {
+    DVLOG(1) << "Could not create HMAC key.";
+    return false;
+  }

[xhwang, 2012/07/10 06:31:25]

Add empty line here to be consistent.

[fgalligan1, 2012/07/11 22:06:33]

Done.

+  return true;
+}
+
 }  // namespace media