Prevent invalid memory read when AES-CBC decrypting.

The issue happens when the ciphertext is not a multiple of the block size.

BUG=300681

Review URL: https://codereview.chromium.org/25164002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@226199 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 38 insertions, 3 deletions

diff --git a/crypto/encryptor_nss.cc b/crypto/encryptor_nss.cc
index 280e38b..4aee21f 100644
--- a/crypto/encryptor_nss.cc
+++ b/crypto/encryptor_nss.cc
@@ -95,9 +95,18 @@ bool Encryptor::Decrypt(const base::StringPiece& ciphertext,
   if (!context.get())
     return false;
 
-  return (mode_ == CTR) ?
-      CryptCTR(context.get(), ciphertext, plaintext) :
-      Crypt(context.get(), ciphertext, plaintext);
+  if (mode_ == CTR)
+    return CryptCTR(context.get(), ciphertext, plaintext);
+
+  if (ciphertext.size() % AES_BLOCK_SIZE != 0) {
+    // Decryption will fail if the input is not a multiple of the block size.
+    // PK11_CipherOp has a bug where it will do an invalid memory access before
+    // the start of the input, so avoid calling it. (Possibly NSS bug 921687).
+    plaintext->clear();
+    return false;
+  }
+
+  return Crypt(context.get(), ciphertext, plaintext);
 }
 
 bool Encryptor::Crypt(PK11Context* context,
diff --git a/crypto/encryptor_unittest.cc b/crypto/encryptor_unittest.cc
index 2f28518..2a21a8e 100644
--- a/crypto/encryptor_unittest.cc
+++ b/crypto/encryptor_unittest.cc
@@ -530,3 +530,29 @@ TEST(EncryptorTest, EmptyEncrypt) {
   EXPECT_EQ(expected_ciphertext_hex, base::HexEncode(ciphertext.data(),
                                                      ciphertext.size()));
 }
+
+TEST(EncryptorTest, CipherTextNotMultipleOfBlockSize) {
+  std::string key = "128=SixteenBytes";
+  std::string iv = "Sweet Sixteen IV";
+
+  scoped_ptr<crypto::SymmetricKey> sym_key(crypto::SymmetricKey::Import(
+      crypto::SymmetricKey::AES, key));
+  ASSERT_TRUE(sym_key.get());
+
+  crypto::Encryptor encryptor;
+  // The IV must be exactly as long a the cipher block size.
+  EXPECT_EQ(16U, iv.size());
+  EXPECT_TRUE(encryptor.Init(sym_key.get(), crypto::Encryptor::CBC, iv));
+
+  // Use a separately allocated array to improve the odds of the memory tools
+  // catching invalid accesses.
+  //
+  // Otherwise when using std::string as the other tests do, accesses several
+  // bytes off the end of the buffer may fall inside the reservation of
+  // the string and not be detected.
+  scoped_ptr<char[]> ciphertext(new char[1]);
+
+  std::string plaintext;
+  EXPECT_FALSE(
+      encryptor.Decrypt(base::StringPiece(ciphertext.get(), 1), &plaintext));
+}