Implement PBKDF2-based key derivation, random key generation,...

Implement PBKDF2-based key derivation, random key generation,
and AES-CBC encryption/decryption using CryptoAPI.

Contributed by Ryan Sleevi <ryan.sleevi@gmail.com>;.

Original review URL: http://codereview.chromium.org/1558018

R=wtc,albertb
BUG=none
TEST=SymmetricKeyTest.* and EncryptorTest.*

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

[wtc, 2010-04-09 00:11:22]

The CL is now ready for review.  Patch Set 1 is Ryan Sleevi's
original CL.  I made some changes to his CL and included
four more files:
  encryptor_{mac,nss}.cc
  symmetric_key_{mac,nss}.cc

albertb: this is a lot of code to review.  I already reviewed
it carefully.  Please focus on the .h and unit test files.
You can just skim through the _win.cc files.

rsleevi: please compare the latest Patch Set with Patch Set
1 to see the changes I made to your CL.  I have some
questions for you below.

http://codereview.chromium.org/1528021/diff/10002/14004
File base/crypto/symmetric_key_win.cc (right):

http://codereview.chromium.org/1528021/diff/10002/14004#newcode75
base/crypto/symmetric_key_win.cc:75: HCRYPTKEY unsafe_key = NULL;
Ryan, why did you name this variable unsafe_key?  What's unsafe
about it?

http://codereview.chromium.org/1528021/diff/10002/14004#newcode117
base/crypto/symmetric_key_win.cc:117: PROV_RSA_AES, CRYPT_VERIFYCONTEXT);
It seems that to use Triple DES, I have to use the provider
type PROV_RSA_SCHANNEL?

http://codereview.chromium.org/1528021/diff/10002/14004#newcode153
base/crypto/symmetric_key_win.cc:153: PROV_RSA_FULL, CRYPT_VERIFYCONTEXT);
Should we use PROV_RSA_AES when generating HMAC keys?
PROV_RSA_FULL doesn't support SHA-2.

http://codereview.chromium.org/1528021/diff/10002/14004#newcode177
base/crypto/symmetric_key_win.cc:177: ok = CryptGenRandom(safe_provider,
key_size_in_bytes,
Why don't we use CryptGenKey to generate an HMAC key?

[rsleevi-old, 2010-04-09 03:04:49]

Just addressing comments, haven't reviewed in depth. 

In addition to the provided comments, I did some double-checking and I am not
sure if it will (ever) be possible to include/use PBKDF2 as the derivation
method, as it's a non NIST/FIPS approved KDF. The list is at
http://csrc.nist.gov/groups/ST/toolkit/kdf.html , but even implementing one of
those may trigger "concerns" if it needs to be CMVP validated. I think it 
requires some further research, but an alternative solution would be to disable
/ not use the function if configured for FIPS mode (either as a Chromium config
option/command line flag or by reading the MSFT registry key). Both methods are
discussed at http://technet.microsoft.com/en-us/library/cc750357.aspx#c1 , under
Implementors and Developers sections.

The code as is still has a number of FIPS Approved use cases, so I think it's
still a good idea - just maybe not derivation.

http://codereview.chromium.org/1528021/diff/10002/14004
File base/crypto/symmetric_key_win.cc (right):

http://codereview.chromium.org/1528021/diff/10002/14004#newcode75
base/crypto/symmetric_key_win.cc:75: HCRYPTKEY unsafe_key = NULL;
Just that it doesn't automatically free (not a scoped handle), and keeping with
the naming scheme of scoped vars = safe_, but here it is not that critical as it
doesn't need to be freed unless everything suceeds, and if it does, it's placed
into a scoped handle right away.

http://codereview.chromium.org/1528021/diff/10002/14004#newcode117
base/crypto/symmetric_key_win.cc:117: PROV_RSA_AES, CRYPT_VERIFYCONTEXT);
Less to do with the provider type, moreso the provider name/default for that
type. None of the types require 3DES as a MUST implement, IIRC. From a naive
MSFT standpoint, only the Strong and Enhanced providers expose 3DES. XP+'s
default should be Enhanced for PROV_RSA_FULL, so I'm not sure why it wouldn't be
exposed.

http://codereview.chromium.org/1528021/diff/10002/14004#newcode153
base/crypto/symmetric_key_win.cc:153: PROV_RSA_FULL, CRYPT_VERIFYCONTEXT);
Should be fine, PROV_RSA_AES is required to implement all of _FULL.

http://codereview.chromium.org/1528021/diff/10002/14004#newcode177
base/crypto/symmetric_key_win.cc:177: ok = CryptGenRandom(safe_provider,
key_size_in_bytes,
1) MSFT's security policy says HMAC keys of should be CryptImport'ed. 
http://csrc.nist.gov/groups/STM/cmvp/documents/140-1/140sp/140sp989.pdf See
CryptCreateHash

2) Try it. Should get NTE_BAD_ALG if it is not an encryption (or pub/priv) alg

However, I admit it's a bit troubling since one can argue it's key generation
happening outside the boundary, even if the RNG is inside it. It may need
clarification from MSFT as to their Security Policy.

[rsleevi-old, 2010-04-09 03:16:35]

http://codereview.chromium.org/1528021/diff/10002/14004
File base/crypto/symmetric_key_win.cc (right):

http://codereview.chromium.org/1528021/diff/10002/14004#newcode177
base/crypto/symmetric_key_win.cc:177: ok = CryptGenRandom(safe_provider,
key_size_in_bytes,
One more link I forgot, but why I don't think it would work - the docs for
CPGenKey, which is what CryptGenKey calls (it's the CSP exported function) say
only encryption/keypair keys.

http://msdn.microsoft.com/en-us/library/aa378204(VS.85).aspx

Generating a random RC2 key /may/ work, I'm just not sure if the IPSEC_HMAC flag
when importing alters the key or disables checks that CryptGenKey would
encounter.

[albertb, 2010-04-09 16:41:07]

LGTM for headers and tests.

http://codereview.chromium.org/1528021/diff/44002/2012
File base/crypto/encryptor_unittest.cc (right):

http://codereview.chromium.org/1528021/diff/44002/2012#newcode42
base/crypto/encryptor_unittest.cc:42: #if defined(OS_WIN)
Can this test run on other platforms?

http://codereview.chromium.org/1528021/diff/44002/2005
File base/crypto/scoped_capi_types.h (right):

http://codereview.chromium.org/1528021/diff/44002/2005#newcode33
base/crypto/scoped_capi_types.h:33: DWORD flags>
nit: Align DWORD with typename

http://codereview.chromium.org/1528021/diff/44002/2005#newcode66
base/crypto/scoped_capi_types.h:66: CAPIHandle* receive() {
nit: Isn't this called get() in scoped_ptr?

http://codereview.chromium.org/1528021/diff/44002/2006
File base/crypto/symmetric_key_win.cc (right):

http://codereview.chromium.org/1528021/diff/44002/2006#newcode55
base/crypto/symmetric_key_win.cc:55: scoped_array<BYTE> tmp_data(new
BYTE[actual_size]);
nit: Shouldn't you use a vector here?

http://codereview.chromium.org/1528021/diff/44002/2006#newcode59
base/crypto/symmetric_key_win.cc:59: =
reinterpret_cast<PlaintextBlobHeader*>(actual_key);
nit: The = should be on the previous line.

http://codereview.chromium.org/1528021/diff/44002/2006#newcode416
base/crypto/symmetric_key_win.cc:416: DCHECK_GT(l, 0);
nit: Maybe call this len instead? The DCHECK looks like 1 > 0 :)

[wtc, 2010-04-09 17:55:22]

albertb: thanks for the review!

I made all the changes you suggested in Patch Set 6.  I respond
to two of your comments below.

http://codereview.chromium.org/1528021/diff/44002/2005
File base/crypto/scoped_capi_types.h (right):

http://codereview.chromium.org/1528021/diff/44002/2005#newcode66
base/crypto/scoped_capi_types.h:66: CAPIHandle* receive() {
On 2010/04/09 16:41:07, albertb wrote:
> nit: Isn't this called get() in scoped_ptr?

No.  scoped_ptr doesn't have an equivalent method.  The
receive() method returns a pointer to |handle_| to the
caller to allow the caller to set |handle_| to a value
via the pointer.

scoped_comptr_win.h has this method.

http://codereview.chromium.org/1528021/diff/44002/2006
File base/crypto/symmetric_key_win.cc (right):

http://codereview.chromium.org/1528021/diff/44002/2006#newcode416
base/crypto/symmetric_key_win.cc:416: DCHECK_GT(l, 0);
On 2010/04/09 16:41:07, albertb wrote:
> nit: Maybe call this len instead? The DCHECK looks like 1 > 0 :)

I agree.  I changed it to capital L.

The variable names L, dkLen, and hLen violate the Style Guide.
I (or rather, Ryan) named them after the names used in the
spec.  This makes it easier to verify our code conforms to
the spec.

If we want to follow the Style Guide, these variables can
be renamed as follows:
  L: num_blocks
  dkLen: derived_key_len
  hLen: hash_len or hmac_len