Implements encryptor_openssl.cc

base/crypto/encryptor_openssl.cc

Index: base/crypto/encryptor_openssl.cc
diff --git a/base/crypto/encryptor_openssl.cc b/base/crypto/encryptor_openssl.cc
index 71a84be710dcf5dc31e08063bd5b8f2dbe21fa8d..45f601ae9098a46cedead1c98abd6e3c1e47dea5 100644
--- a/base/crypto/encryptor_openssl.cc
+++ b/base/crypto/encryptor_openssl.cc
@@ -4,7 +4,12 @@
 
 #include "base/crypto/encryptor.h"
 
+#include <openssl/aes.h>
+#include <openssl/evp.h>
+
+#include "base/crypto/symmetric_key.h"
 #include "base/logging.h"
+#include "base/string_util.h"
 
 namespace base {
 
@@ -15,18 +20,69 @@ Encryptor::~Encryptor() {
 }
 
 bool Encryptor::Init(SymmetricKey* key, Mode mode, const std::string& iv) {
-  NOTIMPLEMENTED();
-  return false;
+  DCHECK(key);
+  DCHECK_EQ(CBC, mode);
+
+  if (iv.size() != AES_BLOCK_SIZE) {
+    NOTREACHED();
+    return false;
+  }
+
+  key_ = key;

[Ryan Sleevi, 2010/11/10 23:46:58]

Perhaps you should move the CHECKs from 52-54 here, to match parity with the
other implementations (which fail in Init() if |key| isn't sized appropriately)

[joth, 2010/11/11 17:35:58]

Done.

+  mode_ = mode;
+  iv_ = iv;
+  return true;
 }
 
 bool Encryptor::Encrypt(const std::string& plaintext, std::string* ciphertext) {
-  NOTIMPLEMENTED();
-  return false;
+  return Crypt(1, plaintext, ciphertext);
 }
 
 bool Encryptor::Decrypt(const std::string& ciphertext, std::string* plaintext) {
-  NOTIMPLEMENTED();
-  return false;
+  return Crypt(0, ciphertext, plaintext);
+}
+
+bool Encryptor::Crypt(int do_encrypt,
+                      const std::string& input,
+                      std::string* output) {
+  if (input.size() == 0)
+    return false;
+  const EVP_CIPHER* cipher = EVP_aes_256_cbc();

[wtc, 2010/11/10 23:40:32]

Why do you hardcode the AES key size to 256 bits?

It seems that you should choose either EVP_aes_128_cbc()
or EVP_aes_256_cbc() based on key.length().

[joth, 2010/11/11 17:35:58]

The tests only cover the 256 bit case. OK if I add a 128 bit test?
Done.

+
+  const std::string& key = key_->key();
+  CHECK_EQ(EVP_CIPHER_iv_length(cipher), static_cast<int>(iv_.length()));
+  CHECK_EQ(EVP_CIPHER_key_length(cipher), static_cast<int>(key.length()));
+
+  EVP_CIPHER_CTX ctx;
+  EVP_CIPHER_CTX_init(&ctx);
+  EVP_CipherInit_ex(&ctx, cipher, NULL,

[Ryan Sleevi, 2010/11/10 23:46:58]

Check the return value here. It can fail for a number of reasons (FIPS mode,
ENGINE failures, memory allocation), which could lead to invalid reads/writes by
line 66.

Also, you should drain the error queue here, or explicitly clear it, if the
diagnostics aren't expected to help, as well as on lines 69 and 77.

[joth, 2010/11/11 17:35:58]

Done.

+                    reinterpret_cast<const uint8*>(key.data()),
+                    reinterpret_cast<const uint8*>(iv_.data()), do_encrypt);
+
+  const size_t output_size = input.size() + iv_.size();

[wtc, 2010/11/10 23:40:32]

Nit: I believe that you only need to add iv_.size() for Encrypt.

[Ryan Sleevi, 2010/11/10 23:46:58]

Maybe a comment here explaining why adding the iv_.size(). (See EncryptorMac for
an example comment). Since you're using a padded cipher, it's to ensure at least
one block size of data is reserved in addition to the input, for the final block
padding, but that's only for encryption.

[joth, 2010/11/11 17:35:58]

Done.

[joth, 2010/11/11 17:35:58]

Done.

+  uint8* out_ptr = reinterpret_cast<uint8*>(WriteInto(output,
+                                                      output_size + 1));
+  int out_len;
+  if (!EVP_CipherUpdate(&ctx, out_ptr, &out_len,
+                        reinterpret_cast<const uint8*>(input.data()),
+                        input.length())) {
+    EVP_CIPHER_CTX_cleanup(&ctx);

[Ryan Sleevi, 2010/11/10 23:46:58]

In the case of failure here and on line 77, you should be resizing output to 0,
otherwise some of the cipher/plain text may leak out from here. The other
implementations only leave data in if everything succeeds.

[joth, 2010/11/11 17:35:58]

Done.

+    return false;
+  }
+
+  // Buffer passed to EVP_EncryptFinal() must be after data just

[wtc, 2010/11/10 23:40:32]

Nit: you're calling EVP_CipherFinal_ex, not EVP_EncryptFinal.

[joth, 2010/11/11 17:35:58]

Done.

+  // encrypted to avoid overwriting it.

[Ryan Sleevi, 2010/11/10 23:46:58]

Maybe rephrase this, to indicate you're writing the final block, plus any
appropriate padding. I think it makes it a little more obvious about what's
happening, and still provides some context as to why you'd increment out_ptr. 

[joth, 2010/11/11 17:35:58]

Done.

+  int pad_len;
+  if(!EVP_CipherFinal_ex(&ctx, out_ptr + out_len, &pad_len)) {
+    EVP_CIPHER_CTX_cleanup(&ctx);
+    return false;
+  }
+
+  out_len += pad_len;
+  DCHECK_LE(out_len, static_cast<int>(output_size));
+  output->resize(out_len);
+  EVP_CIPHER_CTX_cleanup(&ctx);
+  return true;
 }
 
 }  // namespace base