Implement symmetric key for openssl

implement openssl symmetric key
add AES derived key test

Also includes some build fixes.

BUG=None
TEST=./out/Debug/base_unittests --gtest_filter=SymmetricKey*

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

[Ryan Sleevi, 2010-11-10 23:18:45]

The build changes look fine, but perhaps it's time to re-evaluate the comment in
build/linux/system.gyp and make USE_NSS and USE_OPENSSL mutually exclusive, so
that these checks can all become simply USE_NSS. It may already be done, looking
at build/build_config.h, although perhaps I'm misreading things. Maybe splitting
the build changes from this into a separate CL?

A few small nits below

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

http://codereview.chromium.org/4691003/diff/4001/base/crypto/symmetric_key.h#...
base/crypto/symmetric_key.h:70: SymmetricKey(std::string* key);
Ctor should be marked explicit.

http://codereview.chromium.org/4691003/diff/4001/base/crypto/symmetric_key_op...
File base/crypto/symmetric_key_openssl.cc (right):

http://codereview.chromium.org/4691003/diff/4001/base/crypto/symmetric_key_op...
base/crypto/symmetric_key_openssl.cc:18: key->swap(key_);
Why the swap, as opposed to assignment or just taking a const reference and
copying? You copy anyways with Import(), so I can't imagine for optimization.

Just wondering.

http://codereview.chromium.org/4691003/diff/4001/base/crypto/symmetric_key_op...
base/crypto/symmetric_key_openssl.cc:29: DCHECK_EQ(AES, algorithm);
Why restrict |algorithm| to AES? See symmetric_key_mac.cc's CheckKeyParams() for
a comparable implementation that will generate random data of sufficient length.

http://codereview.chromium.org/4691003/diff/4001/base/crypto/symmetric_key_op...
base/crypto/symmetric_key_openssl.cc:30: int key_size_in_bytes =
key_size_in_bits / 8;
My bit-byte sense tingles on this, though it may be a false alarm. For paranoia
sake, perhaps:

int key_size_in_bytes = (key_size_in_bits + 7) / 8;

See also symmetric_key_mac.cc GenerateRandomKey(), although it appears the rest
of that file remains inconsistent and alternates with just / 8.

Also on line 57

http://codereview.chromium.org/4691003/diff/4001/base/crypto/symmetric_key_op...
base/crypto/symmetric_key_openssl.cc:42: unsigned long err = ERR_get_error();
As a defensive measure, whenever programming OpenSSL, I try to drain the entire
OpenSSL error queue on an error. Not doing so has lead to real bugs in the past
(eg: CVE-2008-1531 http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2008-1531 )

[joth, 2010-11-11 15:07:12]

Thanks for the quick comment, now addressed except the non-AES random key. As
mentioned I'm following NSS in this, would appreciate your thoughts on where
it's desirable to diverge?

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

http://codereview.chromium.org/4691003/diff/4001/base/crypto/symmetric_key.h#...
base/crypto/symmetric_key.h:70: SymmetricKey(std::string* key);
On 2010/11/10 23:18:46, rsleevi wrote:
> Ctor should be marked explicit.

Done.

http://codereview.chromium.org/4691003/diff/4001/base/crypto/symmetric_key_op...
File base/crypto/symmetric_key_openssl.cc (right):

http://codereview.chromium.org/4691003/diff/4001/base/crypto/symmetric_key_op...
base/crypto/symmetric_key_openssl.cc:18: key->swap(key_);
On 2010/11/10 23:18:46, rsleevi wrote:
> Why the swap, as opposed to assignment or just taking a const reference and
> copying? You copy anyways with Import(), so I can't imagine for optimization.
> 
> Just wondering.

Just trying to reduce the number of copies of the confidential |key| left lying
around in memory. Only doing it this way as the c'tor is private, so this is all
an internal detail.

http://codereview.chromium.org/4691003/diff/4001/base/crypto/symmetric_key_op...
base/crypto/symmetric_key_openssl.cc:29: DCHECK_EQ(AES, algorithm);
On 2010/11/10 23:18:46, rsleevi wrote:
> Why restrict |algorithm| to AES? See symmetric_key_mac.cc's CheckKeyParams()
for
> a comparable implementation that will generate random data of sufficient
length.

Just copied this from the NSS version, I didn't want to deviate from its API
more than necessary.

http://codereview.chromium.org/4691003/diff/4001/base/crypto/symmetric_key_op...
base/crypto/symmetric_key_openssl.cc:30: int key_size_in_bytes =
key_size_in_bits / 8;
On 2010/11/10 23:18:46, rsleevi wrote:
> My bit-byte sense tingles on this, though it may be a false alarm. For
paranoia
> sake, perhaps:
> 
> int key_size_in_bytes = (key_size_in_bits + 7) / 8;
> 
> See also symmetric_key_mac.cc GenerateRandomKey(), although it appears the
rest
> of that file remains inconsistent and alternates with just / 8.
> 
> Also on line 57

As above, although I originally had a CHECK that the remainder was 0, which I
prefer to trying to second guess the best handling of it.
I'll put that back in.

http://codereview.chromium.org/4691003/diff/4001/base/crypto/symmetric_key_op...
base/crypto/symmetric_key_openssl.cc:42: unsigned long err = ERR_get_error();
On 2010/11/10 23:18:46, rsleevi wrote:
> As a defensive measure, whenever programming OpenSSL, I try to drain the
entire
> OpenSSL error queue on an error. Not doing so has lead to real bugs in the
past
> (eg: CVE-2008-1531 http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2008-1531
)

Done.

[Ryan Sleevi, 2010-11-11 18:07:15]

LGTM, with a few nits below.

http://codereview.chromium.org/4691003/diff/17001/base/openssl_util.cc
File base/openssl_util.cc (right):

http://codereview.chromium.org/4691003/diff/17001/base/openssl_util.cc#newcode18
base/openssl_util.cc:18: DVLOG(1) << "SSL error stack:";
nit: s/SSL/OpenSSL/ 

There is a separate SSL error stack (SSL_get_error), which runs on top
of/parallel to the OpenSSL error stack (some is maintained in the error queue,
some in the SSL*).

http://codereview.chromium.org/4691003/diff/17001/base/openssl_util.h
File base/openssl_util.h (right):

http://codereview.chromium.org/4691003/diff/17001/base/openssl_util.h#newcode56
base/openssl_util.h:56: // Pass FROM_HERE as the parameter.
What parameter?

http://codereview.chromium.org/4691003/diff/20002/base/crypto/symmetric_key_o...
File base/crypto/symmetric_key_openssl.cc (right):

http://codereview.chromium.org/4691003/diff/20002/base/crypto/symmetric_key_o...
base/crypto/symmetric_key_openssl.cc:66: NOTREACHED() << "HMAC SHA1 failed. res
= " << res;
nit: The other impl's don't NOTREACHED(). DLOG(ERROR) would be appropriate (the
others DCHECK)

[joth, 2010-11-11 19:54:36]

Thanks! Will land this in the morning.

http://codereview.chromium.org/4691003/diff/17001/base/openssl_util.cc
File base/openssl_util.cc (right):

http://codereview.chromium.org/4691003/diff/17001/base/openssl_util.cc#newcode18
base/openssl_util.cc:18: DVLOG(1) << "SSL error stack:";
On 2010/11/11 18:07:15, rsleevi wrote:
> nit: s/SSL/OpenSSL/ 
> 
> There is a separate SSL error stack (SSL_get_error), which runs on top
> of/parallel to the OpenSSL error stack (some is maintained in the error queue,
> some in the SSL*). 

Done.

http://codereview.chromium.org/4691003/diff/17001/base/openssl_util.h
File base/openssl_util.h (right):

http://codereview.chromium.org/4691003/diff/17001/base/openssl_util.h#newcode56
base/openssl_util.h:56: // Pass FROM_HERE as the parameter.
On 2010/11/11 18:07:15, rsleevi wrote:
> What parameter?

Opps, removed.

http://codereview.chromium.org/4691003/diff/20002/base/crypto/symmetric_key_o...
File base/crypto/symmetric_key_openssl.cc (right):

http://codereview.chromium.org/4691003/diff/20002/base/crypto/symmetric_key_o...
base/crypto/symmetric_key_openssl.cc:66: NOTREACHED() << "HMAC SHA1 failed. res
= " << res;
On 2010/11/11 18:07:16, rsleevi wrote:
> nit: The other impl's don't NOTREACHED(). DLOG(ERROR) would be appropriate
(the
> others DCHECK)

Done.

[wtc, 2010-11-11 23:39:15]

Please consider these as drive-by review comments.  I didn't
review some changes carefully.

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

http://codereview.chromium.org/4691003/diff/33001/base/crypto/symmetric_key.h...
base/crypto/symmetric_key.h:69: // Swaps in the content of the |key|.
Just copy the key (it is <= 32 bytes), and use this
constructor for Import only.

GenerateRandomKey and DeriveKeyFromPassword are class static
methods, so they should be able to write into key_ directly.

http://codereview.chromium.org/4691003/diff/33001/base/crypto/symmetric_key_u...
File base/crypto/symmetric_key_unittest.cc (right):

http://codereview.chromium.org/4691003/diff/33001/base/crypto/symmetric_key_u...
base/crypto/symmetric_key_unittest.cc:9: #include
"base/string_number_conversions.h"
Nit: list this header after "base/scoped_ptr.h".

http://codereview.chromium.org/4691003/diff/33001/base/openssl_util.h
File base/openssl_util.h (right):

http://codereview.chromium.org/4691003/diff/33001/base/openssl_util.h#newcode56
base/openssl_util.h:56: void ClearERRStack();
IMPORTANT: this function name should include "OpenSSL", since
this function is not in a namespace whose name indicates
OpenSSL.  This comment also applies to ScopedERRStackClearer.

[joth, 2010-11-12 12:15:53]

Cheers! Landed.

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

http://codereview.chromium.org/4691003/diff/33001/base/crypto/symmetric_key.h...
base/crypto/symmetric_key.h:69: // Swaps in the content of the |key|.
On 2010/11/11 23:39:15, wtc wrote:
> Just copy the key (it is <= 32 bytes), and use this
> constructor for Import only.
> 
> GenerateRandomKey and DeriveKeyFromPassword are class static
> methods, so they should be able to write into key_ directly.

Done.

As mentioned to rsleevi, the swap() was intended to avoid making unnecessary
copies of the confidential key, not for performance gain, but as it's not not
clear I have switched the factory methods to access the key_ member directly
(including for Import, as it seems as well to keep them all  consistent).

http://codereview.chromium.org/4691003/diff/33001/base/crypto/symmetric_key_u...
File base/crypto/symmetric_key_unittest.cc (right):

http://codereview.chromium.org/4691003/diff/33001/base/crypto/symmetric_key_u...
base/crypto/symmetric_key_unittest.cc:9: #include
"base/string_number_conversions.h"
On 2010/11/11 23:39:15, wtc wrote:
> Nit: list this header after "base/scoped_ptr.h".

Done.

http://codereview.chromium.org/4691003/diff/33001/base/openssl_util.h
File base/openssl_util.h (right):

http://codereview.chromium.org/4691003/diff/33001/base/openssl_util.h#newcode56
base/openssl_util.h:56: void ClearERRStack();
On 2010/11/11 23:39:15, wtc wrote:
> IMPORTANT: this function name should include "OpenSSL", since
> this function is not in a namespace whose name indicates
> OpenSSL.  This comment also applies to ScopedERRStackClearer.

Done.

[wtc, 2010-11-12 15:01:50]

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

http://codereview.chromium.org/4691003/diff/33001/base/crypto/symmetric_key.h...
base/crypto/symmetric_key.h:69: // Swaps in the content of the |key|.
On 2010/11/12 12:15:53, joth wrote:
>
> As mentioned to rsleevi, the swap() was intended to avoid making unnecessary
> copies of the confidential key, not for performance gain,

Ah, I see.  It was not obvious that's the purpose of the
swap.

> but as it's not not
> clear I have switched the factory methods to access the key_ member directly
> (including for Import, as it seems as well to keep them all  consistent).

This sounds good.  Thanks!