Implements support for PBKDF2-based key derivation, random key generation, an...

[Replaced by http://codereview.chromium.org/1528021 ]

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

Contributed by ryan.sleevi@gmail.com

TEST=SymmetricKeytest.*
TEST=EncryptorTest.*

[rsleevi-old, 2010-04-01 20:36:45]

Wan-Teh,

   Attached is the implementation of PKBDF2 and the corresponding updates to
encrypt/decrypt. The unit tests are all green - but please see the comments
attached.

http://codereview.chromium.org/1558018/diff/1/5
File base/crypto/encryptor_win.cc (right):

http://codereview.chromium.org/1558018/diff/1/5#newcode72
base/crypto/encryptor_win.cc:72: BOOL ok = CryptEncrypt(working_key_.get(),
NULL, TRUE, 0, &tmp[0],
If you have a cross-platform test, I'm not 100% that the TRUE should be a TRUE
when streaming the data.

The unit tests pass locally, but I'd be concerned to make sure they pass
inter-platform. Same on line 87.

http://codereview.chromium.org/1558018/diff/1/2
File base/crypto/symmetric_key_win.cc (right):

http://codereview.chromium.org/1558018/diff/1/2#newcode17
base/crypto/symmetric_key_win.cc:17: const int kSHA1SizeInBytes = 20;
Ignore this - it will be deleted.

http://codereview.chromium.org/1558018/diff/1/2#newcode199
base/crypto/symmetric_key_win.cc:199: bool CreateHMACHash(HCRYPTPROV provider,
HCRYPTKEY key, ALG_ID hash_alg,
There are two reasons for using this, as opposed to delegating to the HMAC
class. Neither are significant, but in the manner of time and not wanting to
change too much in a single CL, I opted for this.

Reason 1: Prevent HMAC from opening/closing a provider every time the object is
created or destroyed. You cannot 'reset' a hash in CryptoAPI, only create new
ones, so this would result in N*M opening/closings of the providers, where N is
the number of iterations, and M is the number of whole block sizes necessary to
generate the target key.

Reason 2: HMAC only takes the raw bytes in for a key, so that'd mean
allocating/importing N*M times as well

http://codereview.chromium.org/1558018/diff/1/2#newcode261
base/crypto/symmetric_key_win.cc:261: uint32 big_endian_block_number =
((block_number << 24) & 0xFF000000) |
I screw up endianness nearly every time I work with it, so I'd appreciate a set
of eyes on this to make sure it's correct.

[wtc, 2010-04-02 00:59:07]

Ryan,

Thanks for writing this patch quickly!

I reviewed everything except scoped_capi_types.h and
symmetric_key_win.cc.  I couldn't finish the review in one
sitting.  I'll try again tomorrow.

scoped_capi_types.h is more complicated than I expected.
Hopefully all that templates stuff is worth it.

Here are my comments on the other files.

http://codereview.chromium.org/1558018/diff/1/6
File base/crypto/encryptor.h (right):

http://codereview.chromium.org/1558018/diff/1/6#newcode55
base/crypto/encryptor.h:55: ScopedHCRYPTKEY working_key_;
Nit: let's call this capi_key_ or key_handle_.

http://codereview.chromium.org/1558018/diff/1/5
File base/crypto/encryptor_win.cc (right):

http://codereview.chromium.org/1558018/diff/1/5#newcode11
base/crypto/encryptor_win.cc:11: namespace {
Nit: add a blank line after the "namespace" line.

http://codereview.chromium.org/1558018/diff/1/5#newcode14
base/crypto/encryptor_win.cc:14: DWORD GetKeyBlockSize(HCRYPTKEY key) {
Nit: GetCipherBlockSize sounds better.

http://codereview.chromium.org/1558018/diff/1/5#newcode15
base/crypto/encryptor_win.cc:15: DWORD block_size = 0;
Nit: document this block size is in bits.

http://codereview.chromium.org/1558018/diff/1/5#newcode18
base/crypto/encryptor_win.cc:18: reinterpret_cast<BYTE*>(&block_size),
&param_size, 0);
Nit: this argument should be aligned with the first argument
on the previous line.

If that'll cause this line to exceed 80 chars, you can use
other formatting options.  See our style guide:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Calls

http://codereview.chromium.org/1558018/diff/1/5#newcode19
base/crypto/encryptor_win.cc:19: if (!ok || block_size == 0)
You can omit the block_size == 0 test and just fall through.

http://codereview.chromium.org/1558018/diff/1/5#newcode37
base/crypto/encryptor_win.cc:37: BOOL ok = CryptDuplicateKey(key->key(), NULL,
0, working_key_.receive());
Why don't we just use key->key() directly?  That's the
intention.

http://codereview.chromium.org/1558018/diff/1/5#newcode72
base/crypto/encryptor_win.cc:72: BOOL ok = CryptEncrypt(working_key_.get(),
NULL, TRUE, 0, &tmp[0],
Right. The argument |Final| should be set to FALSE when we're
streaming the data, and only set to TRUE at the end.

Since we support the one-shot mode only now, it should be
set to TRUE.

http://codereview.chromium.org/1558018/diff/1/5#newcode76
base/crypto/encryptor_win.cc:76: 
Add
  DCHECK_EQ(cipher_len, total_len);

http://codereview.chromium.org/1558018/diff/1/5#newcode89
base/crypto/encryptor_win.cc:89: if (!ok || plaintext_len > tmp.size())
Let's use a DCHECK or NOTREACHED for the
plaintext_len > tmp.size() condition.  That condition should
not happen.

http://codereview.chromium.org/1558018/diff/1/3
File base/crypto/symmetric_key.h (right):

http://codereview.chromium.org/1558018/diff/1/3#newcode70
base/crypto/symmetric_key.h:70: if (key_data)
Add curly braces around the if's body.  Although it's a single
statement, it's two lines.

http://codereview.chromium.org/1558018/diff/1/2
File base/crypto/symmetric_key_win.cc (right):

http://codereview.chromium.org/1558018/diff/1/2#newcode199
base/crypto/symmetric_key_win.cc:199: bool CreateHMACHash(HCRYPTPROV provider,
HCRYPTKEY key, ALG_ID hash_alg,
It is fine to bypass the HMAC class and use CryptoAPI directly
for HMAC.

We should figure out a way to shared a long-lived provider
handle.  We can put it in a Singleton.  snej recently made
this change for Mac's CDSA/CSSM provider handle.

[wtc, 2010-04-02 01:00:29]

http://codereview.chromium.org/1558018/diff/1/13
File base/crypto/encryptor_unittest.cc (right):

http://codereview.chromium.org/1558018/diff/1/13#newcode14
base/crypto/encryptor_unittest.cc:14: TEST(EncryptorTest, EncryptDecrypt) {
albertb: we need some known-answer tests for AES, to verify
that we will be able to encrypt on one platform and decrypt
on another.

[rsleevi-old, 2010-04-02 01:48:18]

I'm not tied to the templates. I was just going after the scoped_nss_types. 

In "base" CryptoAPI, there is only HCRYPTPROV, HCRYPTHASH, and HCRYPTKEY defined
using ULONG_PTRs. The others, such as HCERTSTORE or PCERT_CONTEXT, are defined
as void or struct pointer types, making them acceptable for use with
scoped_ptr_malloc.

Outside of that, you've got SCARDHANDLE and SCARDCONTEXT from the SCard APIs.
The former uses a dwFlagged deleter, the latter uses a no-arg deleter. You've
also got the NCrypt/CNG types (NCRYPT_HANDLE, NCRYPT_PROV_HANDLE, _KEY_HANDLE,
_HASH_HANDLE, _SECRET_HANDLE) each with their own deleters following the
flagged/no-flagged versions. That's the extent of the specializations that may
follow.

I'm fine making them non-templated, just seemed cheaper to meta-program it.
Having the header file in place also seemed like a good way to move some of the
Cert functions into scoped specializations of scoped_ptr_malloc, since there are
a number of 'safe' variants currently omitted.

http://codereview.chromium.org/1558018/diff/1/5
File base/crypto/encryptor_win.cc (right):

http://codereview.chromium.org/1558018/diff/1/5#newcode37
base/crypto/encryptor_win.cc:37: BOOL ok = CryptDuplicateKey(key->key(), NULL,
0, working_key_.receive());
On 2010/04/02 00:59:07, wtc wrote:
> Why don't we just use key->key() directly?  That's the
> intention.

Because I was forcing the IV / padding modes on the key, which ends up modifying
the key attributes destructively. The other reason would be threading -
CryptEncrypt / CryptDecrypt aren't thread-safe when sharing the same key. 

I'm not sure if those matter, I was just playing it defensively because both
CryptSetKeyParam and CryptEncrypt can end up altering the key.

http://codereview.chromium.org/1558018/diff/1/5#newcode76
base/crypto/encryptor_win.cc:76: 
On 2010/04/02 00:59:07, wtc wrote:
> Add
>   DCHECK_EQ(cipher_len, total_len);

I'm pretty sure this would fail. It's not that it has to to be equal - just that
there is enough to pad out to a multiple of block_size.

ex: Supply 27 bytes, add in the padding, get 43 bytes, cipher_len is modified to
32 bytes while total_len remains 43.

http://codereview.chromium.org/1558018/diff/1/2
File base/crypto/symmetric_key_win.cc (right):

http://codereview.chromium.org/1558018/diff/1/2#newcode199
base/crypto/symmetric_key_win.cc:199: bool CreateHMACHash(HCRYPTPROV provider,
HCRYPTKEY key, ALG_ID hash_alg,
On 2010/04/02 00:59:07, wtc wrote:
> It is fine to bypass the HMAC class and use CryptoAPI directly
> for HMAC.
> 
> We should figure out a way to shared a long-lived provider
> handle.  We can put it in a Singleton.  snej recently made
> this change for Mac's CDSA/CSSM provider handle.

Yeah, that's what my thought was as well. Some sort of long lived HashProvider
that can be used to get ephemeral hash instances, and a state-cloning method for
the hash instances as well. 

The only 'gotcha' would be if there needed to be native support for the SHA-2
families on XP SP3+, since that uses the "AES (Prototype)" provider rather than
the "Enhanced" provider. Some sort of Singleton tag or the like would be
sufficient, I'd expect?

[wtc, 2010-04-09 00:45:16]

Ryan,

Here is a belated reply to your comment from last week.

http://codereview.chromium.org/1558018/diff/1/5
File base/crypto/encryptor_win.cc (right):

http://codereview.chromium.org/1558018/diff/1/5#newcode37
base/crypto/encryptor_win.cc:37: BOOL ok = CryptDuplicateKey(key->key(), NULL,
0, working_key_.receive());
On 2010/04/02 01:48:19, rsleevi wrote:
>
> Because I was forcing the IV / padding modes on the key, which ends up
modifying
> the key attributes destructively.

Got it.  In other crypto libraries, the IV and the padding
mode are properties of an encryption operation as opposed to
the encryption key.  This is why I expected CAPI to work
that way, too.

> I'm not sure if those matter, I was just playing it defensively because both
> CryptSetKeyParam and CryptEncrypt can end up altering the key.

Just curious: why does CryptoEncrypt alter the key?

[rsleevi-old, 2010-04-09 03:23:14]

http://codereview.chromium.org/1558018/diff/1/5
File base/crypto/encryptor_win.cc (right):

http://codereview.chromium.org/1558018/diff/1/5#newcode37
base/crypto/encryptor_win.cc:37: BOOL ok = CryptDuplicateKey(key->key(), NULL,
0, working_key_.receive());
http://msdn.microsoft.com/en-us/library/aa379924(VS.85).aspx

The Remarks section explains it better than I can, especially from my phone.
Check the second note about chaining and KP_IV. Same reason as to thread safety
issue when using the same key handle.