Changed OAuth token+secret encryption to use supplemental user key.

Changed OAuth token+secret encryption to use supplemental user key from NSS DB.

BUG=chromium-os:18633
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=99912

[zel, 2011-09-01 22:52:14]

This still uses DES3 instead of AEC. I am planning to spend more time on trying
to get AEC working...

[zel, 2011-09-01 23:30:06]

I've hashed out AEC story, PK11_TokenKeyGen() needed to get keysize in bytes,
not bits :P

[wtc, 2011-09-02 22:31:08]

zelidrag: I'm sorry I didn't have time to do a careful
review of the meat of this CL (the code in nss_util.cc).

Here are some comments on Patch Set 6.

http://codereview.chromium.org/7756025/diff/9015/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/parallel_authenticator.cc (right):

http://codereview.chromium.org/7756025/diff/9015/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/parallel_authenticator.cc:713:
encrypted_token_bytes.size());
Nit: this argument should be left-aligned with the
"reinterpret_cast" above.

http://codereview.chromium.org/7756025/diff/9015/crypto/nss_util.cc
File crypto/nss_util.cc (right):

http://codereview.chromium.org/7756025/diff/9015/crypto/nss_util.cc#newcode88
crypto/nss_util.cc:88: static unsigned char kSupplementalUserKeyId[] = {
Nit: remove 'static' because this is in the anonymous
namespace.

Can you document what this key ID is?  I am very curious.

http://codereview.chromium.org/7756025/diff/9015/crypto/nss_util.cc#newcode96
crypto/nss_util.cc:96: {
Nit: put '{' on the previous line.

Document what "SDR" stands for.

http://codereview.chromium.org/7756025/diff/9015/crypto/nss_util.cc#newcode101
crypto/nss_util.cc:101: typedef struct SDRResult SDRResult;
Remove this typedef.  This is not necessary in C++.

http://codereview.chromium.org/7756025/diff/9015/crypto/nss_util.cc#newcode117
crypto/nss_util.cc:117: SECStatus PadBlock(SECItem *data, unsigned int
blockSize, SECItem *result) {
The function arguments and variables in these functions
should follow the Google C++ naming convention.
So blockSize, padLength, etc. should be renamed
block_size, pad_length, etc.

http://codereview.chromium.org/7756025/diff/9015/crypto/nss_util.cc#newcode129
crypto/nss_util.cc:129: result->data = (unsigned char *)
PORT_Alloc(result->len);
Nit: try to use C++ casts where possible.

This is the only place where this function may fail.
You should add:
  if (!result->data)
    return SECFailure;

Alternatively, since Chrome does not check memory
allcation failures, you can change this function to
return void.

http://codereview.chromium.org/7756025/diff/9015/crypto/nss_util.cc#newcode224
crypto/nss_util.cc:224: }
If these functions are copied from NSS or Mozilla, we need
to put them under a third_party subdirectory.

http://codereview.chromium.org/7756025/diff/9015/crypto/nss_util.cc#newcode227
crypto/nss_util.cc:227: 
Nit: remove one blank line.

http://codereview.chromium.org/7756025/diff/9015/crypto/nss_util.cc#newcode503
crypto/nss_util.cc:503: */
Nit: please use #if 0 to comment out a block of code.

BUT, should we just remove this GetSupplementalUserKey
function (and its declaration in nss_util.h)?

http://codereview.chromium.org/7756025/diff/9015/crypto/nss_util.cc#newcode544
crypto/nss_util.cc:544: 32,   /* keysize in bytes*/
This is just a comment: 256-bit AES is extremely strong :-)

Since this AES key is protected with a key derived from
a password (I guess), the strength of the whole encryption
is as strong as the password.

http://codereview.chromium.org/7756025/diff/9015/crypto/nss_util.h
File crypto/nss_util.h (right):

http://codereview.chromium.org/7756025/diff/9015/crypto/nss_util.h#newcode144
crypto/nss_util.h:144: std::string* encryped_data);
It seems better to move these functions to some other files.
They don't seem like "NSS utils" to me.  Perhaps new files
for supplemental user key would be a good idea.

Nit: on line 140, change "Encrypt/decrypt" to "Encrypts/decrypts".

Nit: on line 144, the parameter should be named decrypted_data.

I am surprised that I don't need to pass the supplemental
user key as an argument to EncryptWithSupplementalUserKey
and DecryptWithSupplementalUserKey.

[zel, 2011-09-03 01:52:22]

PTAL

I have made this CL much simpler overall. I am only fetching the supplemental
key in nss_util.cc and using the rest of crypto:: stack to do the actual
encoding.

http://codereview.chromium.org/7756025/diff/9015/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/parallel_authenticator.cc (right):

http://codereview.chromium.org/7756025/diff/9015/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/parallel_authenticator.cc:713:
encrypted_token_bytes.size());
On 2011/09/02 22:31:08, wtc wrote:
> Nit: this argument should be left-aligned with the
> "reinterpret_cast" above.

Done.

http://codereview.chromium.org/7756025/diff/9015/crypto/nss_util.cc
File crypto/nss_util.cc (right):

http://codereview.chromium.org/7756025/diff/9015/crypto/nss_util.cc#newcode88
crypto/nss_util.cc:88: static unsigned char kSupplementalUserKeyId[] = {
On 2011/09/02 22:31:08, wtc wrote:
> Nit: remove 'static' because this is in the anonymous
> namespace.
> 
> Can you document what this key ID is?  I am very curious.

On ChromeOS, this key will be used to perform AES encryption of OAuth all access
token+secret. The encrypted OAuth is stored in user's Preferences (which are
already TPM-protected in cryptohome). Stored OAuth token+secret will let us 
authenticate user with other Google services such as sync. This stuff is also
used to verify user credentials in parallel authentication process (a sign in
perf optimization on ChromeOS).

The bits used to identify this key are pulled out of thin air. Let me know if
this needs to be something meaningful, registered, etc.

http://codereview.chromium.org/7756025/diff/9015/crypto/nss_util.cc#newcode96
crypto/nss_util.cc:96: {
On 2011/09/02 22:31:08, wtc wrote:
> Nit: put '{' on the previous line.
> 
> Document what "SDR" stands for.

I have removed this code addressed in this and comments below.

http://codereview.chromium.org/7756025/diff/9015/crypto/nss_util.cc#newcode101
crypto/nss_util.cc:101: typedef struct SDRResult SDRResult;
On 2011/09/02 22:31:08, wtc wrote:
> Remove this typedef.  This is not necessary in C++.

Removed.

http://codereview.chromium.org/7756025/diff/9015/crypto/nss_util.cc#newcode117
crypto/nss_util.cc:117: SECStatus PadBlock(SECItem *data, unsigned int
blockSize, SECItem *result) {
On 2011/09/02 22:31:08, wtc wrote:
> The function arguments and variables in these functions
> should follow the Google C++ naming convention.
> So blockSize, padLength, etc. should be renamed
> block_size, pad_length, etc.

Removed.

http://codereview.chromium.org/7756025/diff/9015/crypto/nss_util.cc#newcode129
crypto/nss_util.cc:129: result->data = (unsigned char *)
PORT_Alloc(result->len);
On 2011/09/02 22:31:08, wtc wrote:
> Nit: try to use C++ casts where possible.
> 
> This is the only place where this function may fail.
> You should add:
>   if (!result->data)
>     return SECFailure;
> 
> Alternatively, since Chrome does not check memory
> allcation failures, you can change this function to
> return void.

Removed.

http://codereview.chromium.org/7756025/diff/9015/crypto/nss_util.cc#newcode224
crypto/nss_util.cc:224: }
On 2011/09/02 22:31:08, wtc wrote:
> If these functions are copied from NSS or Mozilla, we need
> to put them under a third_party subdirectory.

Removed.

http://codereview.chromium.org/7756025/diff/9015/crypto/nss_util.cc#newcode503
crypto/nss_util.cc:503: */
On 2011/09/02 22:31:08, wtc wrote:
> Nit: please use #if 0 to comment out a block of code.
> 
> BUT, should we just remove this GetSupplementalUserKey
> function (and its declaration in nss_util.h)?

Rewritten part.

[Will Drewry, 2011-09-06 20:20:21]

I'm not super familiar with this code, but on the whole it looks good. I'm
curious about the failure case with key naming.

http://codereview.chromium.org/7756025/diff/20024/crypto/nss_util.cc
File crypto/nss_util.cc (right):

http://codereview.chromium.org/7756025/diff/20024/crypto/nss_util.cc#newcode313
crypto/nss_util.cc:313: CK_MECHANISM_TYPE type = CKM_AES_ECB;
Not that it really matters for this usage, but might be good to pick something
other than ECB (CBC, OFB maybe? :)

http://codereview.chromium.org/7756025/diff/20024/crypto/nss_util.cc#newcode338
crypto/nss_util.cc:338: goto done;
How is this any different than doing nothing since the next bit after the
conditonal is the done label.

Should it release the key if key != NULL but the nickname fails or will this
possibly leak generated keys?

[zel, 2011-09-06 21:02:22]

http://codereview.chromium.org/7756025/diff/20024/crypto/nss_util.cc
File crypto/nss_util.cc (right):

http://codereview.chromium.org/7756025/diff/20024/crypto/nss_util.cc#newcode313
crypto/nss_util.cc:313: CK_MECHANISM_TYPE type = CKM_AES_ECB;
On 2011/09/06 20:20:21, Will Drewry wrote:
> Not that it really matters for this usage, but might be good to pick something
> other than ECB (CBC, OFB maybe? :)

crypto::Encryptor class seems to support CKM_AES_CBC_PAD (Encryptor::CBC) and
CKM_AES_ECB (Encryptor::CTR) only. Since CBC needed IV and I didn't have any, I
went with CKM_AES_ECB (Encryptor::CTR). WE already used that one for other AES
encryption in ParallelAuthenticator.

http://codereview.chromium.org/7756025/diff/20024/crypto/nss_util.cc#newcode338
crypto/nss_util.cc:338: goto done;
On 2011/09/06 20:20:21, Will Drewry wrote:
> How is this any different than doing nothing since the next bit after the
> conditonal is the done label.
> 
> Should it release the key if key != NULL but the nickname fails or will this
> possibly leak generated keys?

Done.

[wtc, 2011-09-06 21:35:17]

LGTM on Patch Set 11.

High-level comments:

After reading the whole CL, I think the best new API is
probably a new static method of SymmetricKey that generates
a new persistent key in a specified crypto module.  You can
do this later.

http://codereview.chromium.org/7756025/diff/20024/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/cert_library.h (right):

http://codereview.chromium.org/7756025/diff/20024/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/cert_library.h:104: // Returns supplemental user
key.

Nit: add "the".

Does the caller own the returned crypto::SymmetricKey?

http://codereview.chromium.org/7756025/diff/20024/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/login_utils.cc (right):

http://codereview.chromium.org/7756025/diff/20024/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_utils.cc:943: } else {
Nit: omit "else" after a return statement.

http://codereview.chromium.org/7756025/diff/20024/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/parallel_authenticator.cc (right):

http://codereview.chromium.org/7756025/diff/20024/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/parallel_authenticator.cc:55: std::string
DecryptTokenWithKey(
Nit: as recommended by the Style Guide, it'd be nice to
add a comment to document what this function does.  At
least say it is the code common to DecryptToken and
DecryptLegacyToken.

http://codereview.chromium.org/7756025/diff/20024/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/parallel_authenticator.h (right):

http://codereview.chromium.org/7756025/diff/20024/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/parallel_authenticator.h:246: // Stores a hash of
|password|, salted with the ascii of |system_salt_|.
Nit: undo the whitespace.

http://codereview.chromium.org/7756025/diff/20024/crypto/nss_util.cc
File crypto/nss_util.cc (right):

http://codereview.chromium.org/7756025/diff/20024/crypto/nss_util.cc#newcode95
crypto/nss_util.cc:95: };

Nit: on line 92, remove 'static'.

You said:
> The bits used to identify this key are pulled out of thin
> air. Let me know if this needs to be something meaningful,
> registered, etc.

You mean it's 16 random bytes?  The reason I suspected it
may be a non-random key ID was that there were two 0x00
bytes.

The key ID doesn't need to be meaningful nor registered.

http://codereview.chromium.org/7756025/diff/20024/crypto/nss_util.cc#newcode97
crypto/nss_util.cc:97: const char kSupplementalKeyNickname[] =
"ChromeOS_SupplementalUserKey";
I believe the nickname can contain spaces, so you don't need
to use an underscore '_' or CamelCase.

http://codereview.chromium.org/7756025/diff/20024/crypto/nss_util.cc#newcode311
crypto/nss_util.cc:311: PLArenaPool *arena = 0;
Nit: on line 310, put '*' next to the type.

On line 311, delete 'arena' (and all the code that creates
and frees 'arena' below).  'arena' is no longer being used.

http://codereview.chromium.org/7756025/diff/20024/crypto/nss_util.cc#newcode334
crypto/nss_util.cc:334: 32,   /* keysize in bytes*/
Nit: use C++ style comment:
  // keysize in bytes

Please create a constant for 32 around line 95 above.

http://codereview.chromium.org/7756025/diff/20024/crypto/nss_util.cc#newcode337
crypto/nss_util.cc:337: kSupplementalKeyNickname) != SECSuccess) {
I think this if statement should say:
    if (!key || PK11_SetSymKeyNickname(key,
         kSupplementalKeyNickname) != SECSuccess) {

IMPORTANT: does the supplemental key have to have a nickname?
I don't think a nickname is necessary.  If you get rid of
the PK11_SetSymKeyNickname call, then you don't need to
fix the leak that Will Drewry pointed out.

http://codereview.chromium.org/7756025/diff/20024/crypto/nss_util.h
File crypto/nss_util.h (right):

http://codereview.chromium.org/7756025/diff/20024/crypto/nss_util.h#newcode148
crypto/nss_util.h:148: CRYPTO_EXPORT SymmetricKey* GetSupplementalUserKey();
Remove this line?  This is a duplicate declaration of the
GetSupplementalUserKey() function.

On line 139, can you define what a "supplemental user key"
is?  I did a Google search for "supplemental user key", and
the first search result is this CL :-)

http://codereview.chromium.org/7756025/diff/20024/crypto/symmetric_key.h
File crypto/symmetric_key.h (right):

http://codereview.chromium.org/7756025/diff/20024/crypto/symmetric_key.h#newc...
crypto/symmetric_key.h:36: explicit SymmetricKey(PK11SymKey* key);

Maybe it's better to add an ImportPlatformKey or ImportNSSKey
static method, rather than making this constructor public.

Please document that this constructor (or static method)
takes ownership of |key|.

[zel, 2011-09-06 22:33:35]

http://codereview.chromium.org/7756025/diff/20024/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/cert_library.h (right):

http://codereview.chromium.org/7756025/diff/20024/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/cert_library.h:104: // Returns supplemental user
key.
On 2011/09/06 21:35:17, wtc wrote:
> 
> Nit: add "the".
> 
> Does the caller own the returned crypto::SymmetricKey?

Done.

http://codereview.chromium.org/7756025/diff/20024/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/login_utils.cc (right):

http://codereview.chromium.org/7756025/diff/20024/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_utils.cc:943: } else {
On 2011/09/06 21:35:17, wtc wrote:
> Nit: omit "else" after a return statement.

Done.

http://codereview.chromium.org/7756025/diff/20024/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/parallel_authenticator.cc (right):

http://codereview.chromium.org/7756025/diff/20024/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/parallel_authenticator.cc:55: std::string
DecryptTokenWithKey(
On 2011/09/06 21:35:17, wtc wrote:
> Nit: as recommended by the Style Guide, it'd be nice to
> add a comment to document what this function does.  At
> least say it is the code common to DecryptToken and
> DecryptLegacyToken.

Done.

http://codereview.chromium.org/7756025/diff/20024/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/parallel_authenticator.h (right):

http://codereview.chromium.org/7756025/diff/20024/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/parallel_authenticator.h:246: // Stores a hash of
|password|, salted with the ascii of |system_salt_|.
On 2011/09/06 21:35:17, wtc wrote:
> Nit: undo the whitespace.

Done.

http://codereview.chromium.org/7756025/diff/20024/crypto/nss_util.cc
File crypto/nss_util.cc (right):

http://codereview.chromium.org/7756025/diff/20024/crypto/nss_util.cc#newcode95
crypto/nss_util.cc:95: };
On 2011/09/06 21:35:17, wtc wrote:
> 
> Nit: on line 92, remove 'static'.
> 
> You said:
> > The bits used to identify this key are pulled out of thin
> > air. Let me know if this needs to be something meaningful,
> > registered, etc.
> 
> You mean it's 16 random bytes?  The reason I suspected it
> may be a non-random key ID was that there were two 0x00
> bytes.
> 
> The key ID doesn't need to be meaningful nor registered.

Done.

http://codereview.chromium.org/7756025/diff/20024/crypto/nss_util.cc#newcode97
crypto/nss_util.cc:97: const char kSupplementalKeyNickname[] =
"ChromeOS_SupplementalUserKey";
On 2011/09/06 21:35:17, wtc wrote:
> I believe the nickname can contain spaces, so you don't need
> to use an underscore '_' or CamelCase.

I have removed the nickname, I have no good use for it anyway.

http://codereview.chromium.org/7756025/diff/20024/crypto/nss_util.cc#newcode311
crypto/nss_util.cc:311: PLArenaPool *arena = 0;
On 2011/09/06 21:35:17, wtc wrote:
> Nit: on line 310, put '*' next to the type.
> 
> On line 311, delete 'arena' (and all the code that creates
> and frees 'arena' below).  'arena' is no longer being used.

Done.

http://codereview.chromium.org/7756025/diff/20024/crypto/nss_util.cc#newcode334
crypto/nss_util.cc:334: 32,   /* keysize in bytes*/
On 2011/09/06 21:35:17, wtc wrote:
> Nit: use C++ style comment:
>   // keysize in bytes
> 
> Please create a constant for 32 around line 95 above.

Done.

http://codereview.chromium.org/7756025/diff/20024/crypto/nss_util.cc#newcode337
crypto/nss_util.cc:337: kSupplementalKeyNickname) != SECSuccess) {
On 2011/09/06 21:35:17, wtc wrote:
> I think this if statement should say:
>     if (!key || PK11_SetSymKeyNickname(key,
>          kSupplementalKeyNickname) != SECSuccess) {
> 
> IMPORTANT: does the supplemental key have to have a nickname?
> I don't think a nickname is necessary.  If you get rid of
> the PK11_SetSymKeyNickname call, then you don't need to
> fix the leak that Will Drewry pointed out.

Done.

http://codereview.chromium.org/7756025/diff/20024/crypto/nss_util.h
File crypto/nss_util.h (right):

http://codereview.chromium.org/7756025/diff/20024/crypto/nss_util.h#newcode148
crypto/nss_util.h:148: CRYPTO_EXPORT SymmetricKey* GetSupplementalUserKey();
On 2011/09/06 21:35:17, wtc wrote:
> Remove this line?  This is a duplicate declaration of the
> GetSupplementalUserKey() function.
> 
> On line 139, can you define what a "supplemental user key"
> is?  I did a Google search for "supplemental user key", and
> the first search result is this CL :-)

Done.

http://codereview.chromium.org/7756025/diff/20024/crypto/symmetric_key.h
File crypto/symmetric_key.h (right):

http://codereview.chromium.org/7756025/diff/20024/crypto/symmetric_key.h#newc...
crypto/symmetric_key.h:36: explicit SymmetricKey(PK11SymKey* key);
On 2011/09/06 21:35:17, wtc wrote:
> 
> Maybe it's better to add an ImportPlatformKey or ImportNSSKey
> static method, rather than making this constructor public.
> 
> Please document that this constructor (or static method)
> takes ownership of |key|.

Added comment.

I thought about that ImportNSSKey approach but I still don't have a good public
SymmetricKey constructor on ChromeOS. I've create a factory function
SymmetricKey::CreateFromKey() instead.