Add Mac implementations of new SymmetricKey and Encryptor classes.

Add Mac implementations of new SymmetricKey and Encryptor classes.
BUG=none
TEST=EncryptorTest, SymmetricKeyTest

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

[Jens Alfke, 2010-03-25 22:49:09]

Additionally this patch:
* Changes the interpretation of the key size in SymmetricKey to bits instead of
bytes. (I renamed all the parameters to key_size_in_bits as a reminder.)
* Changes the existing Mac CSSM based code to use a single shared
CSSM_CSP_HANDLE managed by cssm_init.

[albertb, 2010-03-26 01:21:11]

http://codereview.chromium.org/1347002/diff/2001/3001
File base/crypto/cssm_init.cc (right):

http://codereview.chromium.org/1347002/diff/2001/3001#newcode50
base/crypto/cssm_init.cc:50: DCHECK(crtn == CSSM_OK);
Should this also be a NOTREACHED?

http://codereview.chromium.org/1347002/diff/2001/3004
File base/crypto/encryptor_mac.cc (right):

http://codereview.chromium.org/1347002/diff/2001/3004#newcode13
base/crypto/encryptor_mac.cc:13: //
http://developer.apple.com/mac/library/documentation/Darwin/Reference/ManPage...
Do we still need a link to the documentation?

http://codereview.chromium.org/1347002/diff/2001/3004#newcode48
base/crypto/encryptor_mac.cc:48: scoped_ptr_malloc<char> output_buffer(
In my patch, wtc suggested I use a vector in a similar situation:

std::vector<char> output_buffer(output_buffer_size);
And then use &output_buffer[0] and output_buffer.size() as arguments into the
crypto library.

http://codereview.chromium.org/1347002/diff/2001/3004#newcode65
base/crypto/encryptor_mac.cc:65: *output = std::string(output_buffer.get(),
output_size);
output is already a string, you can just do:
output->assign(output_buffer.begin(), output_buffer.end());

http://codereview.chromium.org/1347002/diff/2001/3012
File base/crypto/symmetric_key.h (right):

http://codereview.chromium.org/1347002/diff/2001/3012#newcode15
base/crypto/symmetric_key.h:15: #if defined(OS_MACOSX)
Nit: Use #elif and remove my comment.

http://codereview.chromium.org/1347002/diff/2001/3012#newcode49
base/crypto/symmetric_key.h:49: #if defined(OS_MACOSX)
Nit: #elif

http://codereview.chromium.org/1347002/diff/2001/3012#newcode65
base/crypto/symmetric_key.h:65: #endif  // USE_NSS
Remove this comment.

http://codereview.chromium.org/1347002/diff/2001/3013
File base/crypto/symmetric_key_mac.cc (right):

http://codereview.chromium.org/1347002/diff/2001/3013#newcode16
base/crypto/symmetric_key_mac.cc:16: CSSM_KEY_TYPE
CheckKeyParams(base::SymmetricKey::Algorithm algorithm,
I don't think you need to indent these two functions.

http://codereview.chromium.org/1347002/diff/2001/3015
File base/crypto/symmetric_key_unittest.cc (right):

http://codereview.chromium.org/1347002/diff/2001/3015#newcode43
base/crypto/symmetric_key_unittest.cc:43: const char expected[32];
The expected keys all have 160 bits, shouldn't this array have size 20?

http://codereview.chromium.org/1347002/diff/2001/3015#newcode165
base/crypto/symmetric_key_unittest.cc:165: 20,
Shouldn't this be 160?

[wtc, 2010-03-26 01:59:53]

LGTM.  Please don't respond with "Done" to my comments.

http://codereview.chromium.org/1347002/diff/2001/3001
File base/crypto/cssm_init.cc (right):

http://codereview.chromium.org/1347002/diff/2001/3001#newcode52
base/crypto/cssm_init.cc:52: loaded_ = true;
Nit: move this line up to line 45, because loaded_ refers to
the CSSM_ModuleLoad call.

http://codereview.chromium.org/1347002/diff/2001/3001#newcode122
base/crypto/cssm_init.cc:122: LOG(ERROR) << function_name << " returned " <<
err_name;
I suggest we also log the error code in this case.

http://codereview.chromium.org/1347002/diff/2001/3002
File base/crypto/cssm_init.h (right):

http://codereview.chromium.org/1347002/diff/2001/3002#newcode21
base/crypto/cssm_init.h:21: CSSM_CSP_HANDLE GetSharedCSPHandle();
Nit: Should this function have "CSSM" in its name?

http://codereview.chromium.org/1347002/diff/2001/3004
File base/crypto/encryptor_mac.cc (right):

http://codereview.chromium.org/1347002/diff/2001/3004#newcode13
base/crypto/encryptor_mac.cc:13: //
http://developer.apple.com/mac/library/documentation/Darwin/Reference/ManPage...
Nit: remove this comment, or just say "See the CCCryptor(3cc)
man page."

http://codereview.chromium.org/1347002/diff/2001/3004#newcode48
base/crypto/encryptor_mac.cc:48: scoped_ptr_malloc<char> output_buffer(
Nit: use std::vector:
  std::vector<char> output_buffer(output_buffer_size);

and pass &output_buffer[0] instead of output_buffer.get()
below.

Actually, there is a WriteInto template that you can use
to let CCrypt directly write into the |output| string.

http://codereview.chromium.org/1347002/diff/2001/3007
File base/crypto/rsa_private_key_mac.cc (right):

http://codereview.chromium.org/1347002/diff/2001/3007#newcode21
base/crypto/rsa_private_key_mac.cc:21: crtn =
CSSM_CSP_CreateKeyGenContext(GetSharedCSPHandle(), CSSM_ALGID_RSA,
Nit: store the return value of GetSharedCSPHandle() in a
local variable to avoid calling it twice.

Make the same change to the next function below.

http://codereview.chromium.org/1347002/diff/2001/3011
File base/crypto/signature_verifier_mac.cc (right):

http://codereview.chromium.org/1347002/diff/2001/3011#newcode46
base/crypto/signature_verifier_mac.cc:46: crtn =
CSSM_QueryKeySizeInBits(GetSharedCSPHandle(), NULL,
Nit: save the return value of GetSharedCSPHandle() in a local
variable.

http://codereview.chromium.org/1347002/diff/2001/3012
File base/crypto/symmetric_key.h (right):

http://codereview.chromium.org/1347002/diff/2001/3012#newcode62
base/crypto/symmetric_key.h:62: explicit SymmetricKey(const void* key_data,
Nit: remove the "explicit" keyword.

http://codereview.chromium.org/1347002/diff/2001/3013
File base/crypto/symmetric_key_mac.cc (right):

http://codereview.chromium.org/1347002/diff/2001/3013#newcode24
base/crypto/symmetric_key_mac.cc:24: } else {
Nit: in general, don't use "else" after a return statement. 
It's okay in this case because this is essentially a switch
statement.

http://codereview.chromium.org/1347002/diff/2001/3013#newcode26
base/crypto/symmetric_key_mac.cc:26: CHECK(key_size_in_bits == 160)
160 is the output size of HMAC-SHA-1.  The key size for
HMAC-SHA-1 is actually flexible.  We probably should only
require that the key size be a multiple of 8.

You can see if FIPS 198 has any guidance on the key sizes
for HMAC-SHA-1.

http://codereview.chromium.org/1347002/diff/2001/3013#newcode50
base/crypto/symmetric_key_mac.cc:50: random_bits[i] = RandUint64();
We should call the RNG function in CSSM, if possible.

RandUint64 reads from /dev/urandom.  I'm more comfortable
using the RNG of a crypto library to generate keys.

http://codereview.chromium.org/1347002/diff/2001/3013#newcode63
base/crypto/symmetric_key_mac.cc:63: CSSM_KEY cssm_key = {};
Should there be a 0 or NULL inside the curly braces?

Does "{}" initialize all members to 0?

http://codereview.chromium.org/1347002/diff/2001/3013#newcode69
base/crypto/symmetric_key_mac.cc:69: err =
CSSM_CSP_CreateDeriveKeyContext(GetSharedCSPHandle(),
Nit: save the return value of GetSharedCSPHandle() in a local
variable.

http://codereview.chromium.org/1347002/diff/2001/3013#newcode85
base/crypto/symmetric_key_mac.cc:85: params.PseudoRandomFunction =
CSSM_PKCS5_PBKDF2_PRF_HMAC_SHA1;
Just curious: does CSSM have other PBKDF2 PRFs defined?

http://codereview.chromium.org/1347002/diff/2001/3013#newcode110
base/crypto/symmetric_key_mac.cc:110: key_size_in_bits / 8) { }
Nit: the style guide recommends no space in {}.

http://codereview.chromium.org/1347002/diff/2001/3015
File base/crypto/symmetric_key_unittest.cc (right):

http://codereview.chromium.org/1347002/diff/2001/3015#newcode35
base/crypto/symmetric_key_unittest.cc:35: EXPECT_TRUE(raw_key != raw_key2);
Can we use EXPECT_NE here?

http://codereview.chromium.org/1347002/diff/2001/3015#newcode185
base/crypto/symmetric_key_unittest.cc:185: SCOPED_TRACE(StringPrintf("Test[%u]",
i));
I suspect this SCOPED_TRACE line should be the first line
in the lexical scope.

http://codereview.chromium.org/1347002/diff/2001/3016
File base/crypto/symmetric_key_win.cc (right):

http://codereview.chromium.org/1347002/diff/2001/3016#newcode13
base/crypto/symmetric_key_win.cc:13: unsigned int key_size_in_bits) {
This should have the size_t type, right?

[Jens Alfke, 2010-03-26 18:56:24]

http://codereview.chromium.org/1347002/diff/2001/3001
File base/crypto/cssm_init.cc (right):

http://codereview.chromium.org/1347002/diff/2001/3001#newcode50
base/crypto/cssm_init.cc:50: DCHECK(crtn == CSSM_OK);
On 2010/03/26 01:21:11, albertb wrote:
> Should this also be a NOTREACHED?

More likely the others should be changed to DCHECK, since NOTREACHED is just
defined as DCHECK(false), which has the same meaning but provides less
information in the log.

http://codereview.chromium.org/1347002/diff/2001/3007
File base/crypto/rsa_private_key_mac.cc (right):

http://codereview.chromium.org/1347002/diff/2001/3007#newcode21
base/crypto/rsa_private_key_mac.cc:21: crtn =
CSSM_CSP_CreateKeyGenContext(GetSharedCSPHandle(), CSSM_ALGID_RSA,
On 2010/03/26 01:59:54, wtc wrote:
> Nit: store the return value of GetSharedCSPHandle() in a
> local variable to avoid calling it twice.

This method (RSAPrivateKey::Create) takes several seconds to run, so I don't
think the overhead of calling GetSharedCSPHandle twice is going to matter!
CreateFromPrivateKeyInfo is less slow but I think the same still holds.

http://codereview.chromium.org/1347002/diff/2001/3013
File base/crypto/symmetric_key_mac.cc (right):

http://codereview.chromium.org/1347002/diff/2001/3013#newcode26
base/crypto/symmetric_key_mac.cc:26: CHECK(key_size_in_bits == 160)
On 2010/03/26 01:59:54, wtc wrote:
> 160 is the output size of HMAC-SHA-1.  The key size for
> HMAC-SHA-1 is actually flexible.  We probably should only
> require that the key size be a multiple of 8.
> 
> You can see if FIPS 198 has any guidance on the key sizes
> for HMAC-SHA-1.

It says "The size of the key, K, shall be equal to or greater than L/2, where L
is the size of the hash function output." So I've changed the test to:
    key_size_in_bits >= 80 && (key_size_in_bits % 8) == 0

http://codereview.chromium.org/1347002/diff/2001/3013#newcode50
base/crypto/symmetric_key_mac.cc:50: random_bits[i] = RandUint64();
On 2010/03/26 01:59:54, wtc wrote:
> We should call the RNG function in CSSM, if possible.

OK, I've changed it to call CSSM_GenerateRandom.

http://codereview.chromium.org/1347002/diff/2001/3013#newcode63
base/crypto/symmetric_key_mac.cc:63: CSSM_KEY cssm_key = {};
On 2010/03/26 01:59:54, wtc wrote:
> Does "{}" initialize all members to 0?

Yes. All unspecified fields are zero-filled. Using "{0}" is a syntax error if
the first struct field is itself a struct or array, so I avoid it.

http://codereview.chromium.org/1347002/diff/2001/3013#newcode85
base/crypto/symmetric_key_mac.cc:85: params.PseudoRandomFunction =
CSSM_PKCS5_PBKDF2_PRF_HMAC_SHA1;
On 2010/03/26 01:59:54, wtc wrote:
> Just curious: does CSSM have other PBKDF2 PRFs defined?

No, that's the only value defined for that enum. And I've seen a comment in
Apple's CryptoSample that it's the only one available.

http://codereview.chromium.org/1347002/diff/2001/3015
File base/crypto/symmetric_key_unittest.cc (right):

http://codereview.chromium.org/1347002/diff/2001/3015#newcode43
base/crypto/symmetric_key_unittest.cc:43: const char expected[32];
On 2010/03/26 01:21:11, albertb wrote:
> The expected keys all have 160 bits, shouldn't this array have size 20?

The test structs I got from the OpenBSD sources have 32 bytes, but that's
because they were generating 256-bit keys. I've trimmed them down to 20 bytes.
(Still need to declare the array as uint8[21] because the pre-existing string
literals have a NUL byte at the end.)

http://codereview.chromium.org/1347002/diff/2001/3015#newcode165
base/crypto/symmetric_key_unittest.cc:165: 20,
On 2010/03/26 01:21:11, albertb wrote:
> Shouldn't this be 160?

Yes, thanks. This test gets skipped on Mac because iterations < 1000. I haven't
tested on the Linux try bot since making that change, apparently.

[wtc, 2010-03-26 21:14:30]

LGTM.  Thanks for making the changes.

http://codereview.chromium.org/1347002/diff/2001/3013
File base/crypto/symmetric_key_mac.cc (right):

http://codereview.chromium.org/1347002/diff/2001/3013#newcode50
base/crypto/symmetric_key_mac.cc:50: random_bits[i] = RandUint64();
On 2010/03/26 18:56:24, Jens Alfke wrote:
>
> OK, I've changed it to call CSSM_GenerateRandom.

Thanks!  Actually it would be better to change RandUint64()
to call CSSM_GenerateRandom...

http://codereview.chromium.org/1347002/diff/15001/16001
File base/crypto/cssm_init.cc (right):

http://codereview.chromium.org/1347002/diff/15001/16001#newcode114
base/crypto/cssm_init.cc:114: void LogCSSMError(const char *fn_name, CSSM_RETURN
err) {
Nit: I like the original |function_name| better.  I know it
requires wrapping the LOG(ERROR) statement at line 121...

http://codereview.chromium.org/1347002/diff/15001/16013
File base/crypto/symmetric_key_mac.cc (right):

http://codereview.chromium.org/1347002/diff/15001/16013#newcode29
base/crypto/symmetric_key_mac.cc:29: return CSSM_ALGID_SHA1HMAC_LEGACY;
Just curious: why does this ALGID say "LEGACY"?

http://codereview.chromium.org/1347002/diff/15001/16013#newcode33
base/crypto/symmetric_key_mac.cc:33: void* CreateRandomBytes(size_t size) {
It seems better to take a result buffer as an input argument.
Is that possible with the CSSM API?

http://codereview.chromium.org/1347002/diff/15001/16013#newcode51
base/crypto/symmetric_key_mac.cc:51: return random_data.Data;  // Caller
responsible for freeing this
Nit: could you move this comment to the beginning of this
function?

[Jens Alfke, 2010-03-29 18:34:17]

http://codereview.chromium.org/1347002/diff/15001/16013
File base/crypto/symmetric_key_mac.cc (right):

http://codereview.chromium.org/1347002/diff/15001/16013#newcode29
base/crypto/symmetric_key_mac.cc:29: return CSSM_ALGID_SHA1HMAC_LEGACY;
On 2010/03/26 21:14:30, wtc wrote:
> Just curious: why does this ALGID say "LEGACY"?

I don't know -- the header comment says "HMAC/SHA1, legacy compatible".

http://codereview.chromium.org/1347002/diff/15001/16013#newcode33
base/crypto/symmetric_key_mac.cc:33: void* CreateRandomBytes(size_t size) {
On 2010/03/26 21:14:30, wtc wrote:
> It seems better to take a result buffer as an input argument.
> Is that possible with the CSSM API?

It would be possible, but not any more efficient, as the CSSM_GenerateRandom
function returns a malloc'ed buffer that I'd have to copy.