Implements encryptor_openssl.cc

Implements encryptor_openssl.cc
Depends on pending CL http://codereview.chromium.org/4691003/

BUG=None
TEST=base_unittests Encryptor*

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=65952

[joth, 2010-11-10 21:52:43]

Thanks in advance for the reviews....!

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

Please consider this a drive-by review because I'm not
familiar with OpenSSL, even though I reviewed the entire changelist.

http://codereview.chromium.org/4777001/diff/6001/base/crypto/encryptor_openss...
File base/crypto/encryptor_openssl.cc (right):

http://codereview.chromium.org/4777001/diff/6001/base/crypto/encryptor_openss...
base/crypto/encryptor_openssl.cc:50: const EVP_CIPHER* cipher =
EVP_aes_256_cbc();
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().

http://codereview.chromium.org/4777001/diff/6001/base/crypto/encryptor_openss...
base/crypto/encryptor_openssl.cc:62: const size_t output_size = input.size() +
iv_.size();
Nit: I believe that you only need to add iv_.size() for Encrypt.

http://codereview.chromium.org/4777001/diff/6001/base/crypto/encryptor_openss...
base/crypto/encryptor_openssl.cc:73: // Buffer passed to EVP_EncryptFinal() must
be after data just
Nit: you're calling EVP_CipherFinal_ex, not EVP_EncryptFinal.

http://codereview.chromium.org/4777001/diff/6001/base/crypto/symmetric_key.h
File base/crypto/symmetric_key.h (right):

http://codereview.chromium.org/4777001/diff/6001/base/crypto/symmetric_key.h#...
base/crypto/symmetric_key.h:72: SymmetricKey(std::string* key);
Nit: add 'explicit'.

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

A few comments below with regards to the error handling

http://codereview.chromium.org/4777001/diff/6001/base/crypto/encryptor.h
File base/crypto/encryptor.h (right):

http://codereview.chromium.org/4777001/diff/6001/base/crypto/encryptor.h#newc...
base/crypto/encryptor.h:50: #elif defined(OS_MACOSX) || defined(USE_OPENSSL)
I'm a little nervous with this here. CCOperation is an enumeration from
CommonCryptor.h, but it appears you've defined it for OpenSSL to be a
integer-as-a-bool with do_encrypt. Maybe a comment would be appropriate for
explaining here?

http://codereview.chromium.org/4777001/diff/6001/base/crypto/encryptor_openss...
File base/crypto/encryptor_openssl.cc (right):

http://codereview.chromium.org/4777001/diff/6001/base/crypto/encryptor_openss...
base/crypto/encryptor_openssl.cc:31: key_ = key;
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)

http://codereview.chromium.org/4777001/diff/6001/base/crypto/encryptor_openss...
base/crypto/encryptor_openssl.cc:58: EVP_CipherInit_ex(&ctx, cipher, NULL,
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.

http://codereview.chromium.org/4777001/diff/6001/base/crypto/encryptor_openss...
base/crypto/encryptor_openssl.cc:62: const size_t output_size = input.size() +
iv_.size();
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.

http://codereview.chromium.org/4777001/diff/6001/base/crypto/encryptor_openss...
base/crypto/encryptor_openssl.cc:69: EVP_CIPHER_CTX_cleanup(&ctx);
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.

http://codereview.chromium.org/4777001/diff/6001/base/crypto/encryptor_openss...
base/crypto/encryptor_openssl.cc:74: // encrypted to avoid overwriting it.
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]

Thanks both. I'm not entirely clear on what key sizes the portable API is
required to support, but I've made a stab at doing aes 128.

Cheers!

http://codereview.chromium.org/4777001/diff/6001/base/crypto/encryptor.h
File base/crypto/encryptor.h (right):

http://codereview.chromium.org/4777001/diff/6001/base/crypto/encryptor.h#newc...
base/crypto/encryptor.h:50: #elif defined(OS_MACOSX) || defined(USE_OPENSSL)
On 2010/11/10 23:46:58, rsleevi wrote:
> I'm a little nervous with this here. CCOperation is an enumeration from
> CommonCryptor.h, but it appears you've defined it for OpenSSL to be a
> integer-as-a-bool with do_encrypt. Maybe a comment would be appropriate for
> explaining here?

The comment looked more complex than just duplicating the statements, which is
probably less brittle anyway.

http://codereview.chromium.org/4777001/diff/6001/base/crypto/encryptor_openss...
File base/crypto/encryptor_openssl.cc (right):

http://codereview.chromium.org/4777001/diff/6001/base/crypto/encryptor_openss...
base/crypto/encryptor_openssl.cc:31: key_ = key;
On 2010/11/10 23:46:58, rsleevi wrote:
> 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)

Done.

http://codereview.chromium.org/4777001/diff/6001/base/crypto/encryptor_openss...
base/crypto/encryptor_openssl.cc:50: const EVP_CIPHER* cipher =
EVP_aes_256_cbc();
On 2010/11/10 23:40:32, wtc wrote:
> Why do you hardcode the AES key size to 256 bits?
> 
The tests only cover the 256 bit case. OK if I add a 128 bit test?

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

Done.

http://codereview.chromium.org/4777001/diff/6001/base/crypto/encryptor_openss...
base/crypto/encryptor_openssl.cc:58: EVP_CipherInit_ex(&ctx, cipher, NULL,
On 2010/11/10 23:46:58, rsleevi wrote:
> 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.

Done.

http://codereview.chromium.org/4777001/diff/6001/base/crypto/encryptor_openss...
base/crypto/encryptor_openssl.cc:62: const size_t output_size = input.size() +
iv_.size();
On 2010/11/10 23:40:32, wtc wrote:
> Nit: I believe that you only need to add iv_.size() for Encrypt.

Done.

http://codereview.chromium.org/4777001/diff/6001/base/crypto/encryptor_openss...
base/crypto/encryptor_openssl.cc:62: const size_t output_size = input.size() +
iv_.size();
On 2010/11/10 23:40:32, wtc wrote:
> Nit: I believe that you only need to add iv_.size() for Encrypt.

Done.

http://codereview.chromium.org/4777001/diff/6001/base/crypto/encryptor_openss...
base/crypto/encryptor_openssl.cc:69: EVP_CIPHER_CTX_cleanup(&ctx);
On 2010/11/10 23:46:58, rsleevi wrote:
> 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.

Done.

http://codereview.chromium.org/4777001/diff/6001/base/crypto/encryptor_openss...
base/crypto/encryptor_openssl.cc:73: // Buffer passed to EVP_EncryptFinal() must
be after data just
On 2010/11/10 23:40:32, wtc wrote:
> Nit: you're calling EVP_CipherFinal_ex, not EVP_EncryptFinal.

Done.

http://codereview.chromium.org/4777001/diff/6001/base/crypto/encryptor_openss...
base/crypto/encryptor_openssl.cc:74: // encrypted to avoid overwriting it.
On 2010/11/10 23:46:58, rsleevi wrote:
> 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. 

Done.

http://codereview.chromium.org/4777001/diff/6001/base/crypto/symmetric_key.h
File base/crypto/symmetric_key.h (right):

http://codereview.chromium.org/4777001/diff/6001/base/crypto/symmetric_key.h#...
base/crypto/symmetric_key.h:72: SymmetricKey(std::string* key);
On 2010/11/10 23:40:32, wtc wrote:
> Nit: add 'explicit'.

Done.

[Ryan Sleevi, 2010-11-11 17:59:30]

LGTM. Two nits below, but nothing requiring re-review. Feel free to ignore the
AES-192 if you disagree.

http://codereview.chromium.org/4777001/diff/13001/base/crypto/encryptor_opens...
File base/crypto/encryptor_openssl.cc (right):

http://codereview.chromium.org/4777001/diff/13001/base/crypto/encryptor_opens...
base/crypto/encryptor_openssl.cc:25: return NULL;
Nit: The other impls also support EVP_aes_192_cbc(). For NSS & Win, it's derived
from the key length. For Mac, it looks to be bugged (I'll clean it up
separately), but I guess that's a sign it's not being used at present.

http://codereview.chromium.org/4777001/diff/13001/base/crypto/encryptor_opens...
base/crypto/encryptor_openssl.cc:99: return false;
Nit: Add an extra space for 96-98; alignment should be to inside the paren

[agl, 2010-11-11 18:05:36]

LGTM

http://codereview.chromium.org/4777001/diff/13001/base/crypto/encryptor_opens...
File base/crypto/encryptor_openssl.cc (right):

http://codereview.chromium.org/4777001/diff/13001/base/crypto/encryptor_opens...
base/crypto/encryptor_openssl.cc:80: if (input.size() == 0)
Why is this a failure? Everything is well defined for a zero length input.

http://codereview.chromium.org/4777001/diff/13001/base/crypto/encryptor_opens...
base/crypto/encryptor_openssl.cc:101: // When encrypting, add another block size
of space to allow for any padding.
I guess that we don't actually know if each of the different implementations
does padding the same. I wonder if we depend on that anywhere...

[joth, 2010-11-11 20:32:01]

Thanks both

http://codereview.chromium.org/4777001/diff/13001/base/crypto/encryptor_opens...
File base/crypto/encryptor_openssl.cc (right):

http://codereview.chromium.org/4777001/diff/13001/base/crypto/encryptor_opens...
base/crypto/encryptor_openssl.cc:25: return NULL;
On 2010/11/11 17:59:30, rsleevi wrote:
> Nit: The other impls also support EVP_aes_192_cbc(). For NSS & Win, it's
derived
> from the key length. For Mac, it looks to be bugged (I'll clean it up
> separately), but I guess that's a sign it's not being used at present.

Done.

http://codereview.chromium.org/4777001/diff/13001/base/crypto/encryptor_opens...
base/crypto/encryptor_openssl.cc:80: if (input.size() == 0)
On 2010/11/11 18:05:36, agl wrote:
> Why is this a failure? Everything is well defined for a zero length input.

Good question. I just copied it from NSS.... :)

More tests added. (I'll likely disable them for != openssl builds, and do a
follow-up to make platforms consistent)

Works fine for encrypt, but fails for decrypt: the openssl functions fail
gracefully for us, but the return code is 'false'. Seem OK?

http://codereview.chromium.org/4777001/diff/13001/base/crypto/encryptor_opens...
base/crypto/encryptor_openssl.cc:99: return false;
On 2010/11/11 17:59:30, rsleevi wrote:
> Nit: Add an extra space for 96-98; alignment should be to inside the paren

Done.

http://codereview.chromium.org/4777001/diff/13001/base/crypto/encryptor_opens...
base/crypto/encryptor_openssl.cc:101: // When encrypting, add another block size
of space to allow for any padding.
On 2010/11/11 18:05:36, agl wrote:
> I guess that we don't actually know if each of the different implementations
> does padding the same. I wonder if we depend on that anywhere...

AFAICS they all claim to do padding, don't know if it's being done the same way
or not though. Maybe my new tests will help answer this.

[agl, 2010-11-11 20:39:44]

http://codereview.chromium.org/4777001/diff/13001/base/crypto/encryptor_opens...
File base/crypto/encryptor_openssl.cc (right):

http://codereview.chromium.org/4777001/diff/13001/base/crypto/encryptor_opens...
base/crypto/encryptor_openssl.cc:80: if (input.size() == 0)
On 2010/11/11 20:32:01, joth wrote:
> Works fine for encrypt, but fails for decrypt: the openssl functions fail
> gracefully for us, but the return code is 'false'. Seem OK?

Well a zero length input to decrypt *is* invalid. It sounds like OpenSSL is
doing the right thing and we can just report its errors back.