Linux (nss) implementations of RSAPrivateKey and SignatureCreator

Linux (nss) implementations of RSAPrivateKey and SignatureCreator

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

[rafaelw, 2009-09-18 21:22:15]

Wan-Teh, here's the cleaned up patch. Note that the PK11_ReadRawAttributes still
don't do anything about freeing the data attached to the SECItem(s).

Also, I considered creating an rsa_private_key_util.cc to contain the asn1
encoding and decoding that the windows and nss clients are using, but it didn't
seem worth it yet since they would currently only share a few consts and three
functions (the nss only needs encoding, plus it uses big-endian integers).

[wtc, 2009-09-18 23:05:04]

Hi Rafael,

I looked into how to free the SECItems returned by NSS
functions.  Please my comments below.  I will really review
this CL next Monday.

It is fine to copy the manual ASN.1 encoding code to the
NSS file.  This gives us an incentive to use NSS's ASN.1
encoder in the future :-)

http://codereview.chromium.org/208032/diff/4009/4012
File base/crypto/rsa_private_key_nss.cc (right):

http://codereview.chromium.org/208032/diff/4009/4012#newcode227
Line 227: PrependInteger(modulus_item.data, modulus_item.len, &content);
After we are done with these SECItems, we need to free their
data buffers using SECITEM_FreeItem calls.  For example,
  SECITEM_FreeItem(&modulus_item, PR_FALSE);

The PR_FALSE argument means we don't want to free the
SECItem structure itself; we just want to free the data
buffer it points to.

I will add a comment to the NSS <pk11pub.h> header to document this.

http://codereview.chromium.org/208032/diff/4009/4012#newcode255
Line 255: SECITEM_FreeItem(der_pubkey, PR_TRUE);
This is the right awy to free der_pubkey.  I'll add a comment
to pk11pub.h to document this.

[rafaelw, 2009-09-19 00:46:42]

http://codereview.chromium.org/208032/diff/4009/4012
File base/crypto/rsa_private_key_nss.cc (right):

http://codereview.chromium.org/208032/diff/4009/4012#newcode227
Line 227: PrependInteger(modulus_item.data, modulus_item.len, &content);
On 2009/09/18 23:05:05, wtc wrote:
> After we are done with these SECItems, we need to free their
> data buffers using SECITEM_FreeItem calls.  For example,
>   SECITEM_FreeItem(&modulus_item, PR_FALSE);
> 
> The PR_FALSE argument means we don't want to free the
> SECItem structure itself; we just want to free the data
> buffer it points to.
> 
> I will add a comment to the NSS <pk11pub.h> header to document this.

Done.

http://codereview.chromium.org/208032/diff/4009/4012#newcode255
Line 255: SECITEM_FreeItem(der_pubkey, PR_TRUE);
On 2009/09/18 23:05:05, wtc wrote:
> This is the right awy to free der_pubkey.  I'll add a comment
> to pk11pub.h to document this.

Cool.

http://codereview.chromium.org/208032/diff/5002/5005#newcode132
Line 132: return result.release();
Note that in addition to the FreeItems below, I added this PK11_FreeSlot. Now
the unittests no longer report nss shutdown failure.

[Aaron Boodman, 2009-09-19 02:45:24]

Looking good.

http://codereview.chromium.org/208032/diff/5002/5003
File base/base.gyp (right):

http://codereview.chromium.org/208032/diff/5002/5003#newcode24
Line 24: 'crypto/rsa_private_key_nss.cc',
alphabetize

http://codereview.chromium.org/208032/diff/5002/5003#newcode27
Line 27: 'crypto/signature_creator_nss.cc',
same

http://codereview.chromium.org/208032/diff/5002/5005
File base/crypto/rsa_private_key_nss.cc (right):

http://codereview.chromium.org/208032/diff/5002/5005#newcode36
Line 36: int count = num_bytes;
No need for the local, num_bytes is passed by value.

http://codereview.chromium.org/208032/diff/5002/5005#newcode37
Line 37: while(count-- > start) {
Post-decrement is strongly discouraged in google style. Recommend just doing the
decrement inside the while loop, or converting to a for loop.

http://codereview.chromium.org/208032/diff/5002/5005#newcode69
Line 69: // Skip leading null bytes off the MSB start
Reading through this a second time, I think this comment suffers from explaining
the how, not the what. What about something like:

// ASN.1 integers are unpadded byte arrays, so skip any 
// null padding bytes from the most-significant end of the 
// integer.

http://codereview.chromium.org/208032/diff/5002/5005#newcode76
Line 76: // If the MSB is set, we need to add an extra null byte, otherwise the
integer
Again here:

// ASN.1 integers are signed. To encode a positive integer
// whose sign bit (the most significant bit) would otherwise
// be set and make the number negative, ASN.1 requires a
// leading null byte to force the integer to be positive.

http://codereview.chromium.org/208032/diff/5002/5005#newcode141
Line 141: if (key_) {
Brace not needed here.

[rafaelw, 2009-09-20 17:14:07]

http://codereview.chromium.org/208032/diff/5002/5003
File base/base.gyp (right):

http://codereview.chromium.org/208032/diff/5002/5003#newcode24
Line 24: 'crypto/rsa_private_key_nss.cc',
On 2009/09/19 02:45:24, Aaron Boodman wrote:
> alphabetize

Done.

http://codereview.chromium.org/208032/diff/5002/5003#newcode27
Line 27: 'crypto/signature_creator_nss.cc',
On 2009/09/19 02:45:24, Aaron Boodman wrote:
> same

Done.

http://codereview.chromium.org/208032/diff/5002/5005
File base/crypto/rsa_private_key_nss.cc (right):

http://codereview.chromium.org/208032/diff/5002/5005#newcode36
Line 36: int count = num_bytes;
On 2009/09/19 02:45:24, Aaron Boodman wrote:
> No need for the local, num_bytes is passed by value.

Done.

http://codereview.chromium.org/208032/diff/5002/5005#newcode37
Line 37: while(count-- > start) {
On 2009/09/19 02:45:24, Aaron Boodman wrote:
> Post-decrement is strongly discouraged in google style. Recommend just doing
the
> decrement inside the while loop, or converting to a for loop.

Done.

http://codereview.chromium.org/208032/diff/5002/5005#newcode69
Line 69: // Skip leading null bytes off the MSB start
On 2009/09/19 02:45:24, Aaron Boodman wrote:
> Reading through this a second time, I think this comment suffers from
explaining
> the how, not the what. What about something like:
> 
> // ASN.1 integers are unpadded byte arrays, so skip any 
> // null padding bytes from the most-significant end of the 
> // integer.

Done.

http://codereview.chromium.org/208032/diff/5002/5005#newcode76
Line 76: // If the MSB is set, we need to add an extra null byte, otherwise the
integer
On 2009/09/19 02:45:24, Aaron Boodman wrote:
> Again here:
> 
> // ASN.1 integers are signed. To encode a positive integer
> // whose sign bit (the most significant bit) would otherwise
> // be set and make the number negative, ASN.1 requires a
> // leading null byte to force the integer to be positive.

Done.

http://codereview.chromium.org/208032/diff/5002/5005#newcode141
Line 141: if (key_) {
On 2009/09/19 02:45:24, Aaron Boodman wrote:
> Brace not needed here.

Done.

[Aaron Boodman, 2009-09-21 17:49:33]

lgtm. Are you going to try and refactor the shared code in this CL or later?

[wtc, 2009-09-22 19:06:20]

LGTM.  Excellent work!  Some suggested changes below.

http://codereview.chromium.org/208032/diff/7001/7004
File base/crypto/rsa_private_key_nss.cc (right):

http://codereview.chromium.org/208032/diff/7001/7004#newcode7
Line 7: #include <cryptohi.h>
Nit: separate the C and C++ system headers.

#include <cryptohi.h>
#include <keyhi.h>
#include <pk11pub.h>

#include <iostream>
#include <list>

See
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde...

http://codereview.chromium.org/208032/diff/7001/7004#newcode19
Line 19: // rsa_private_key_win.cc.
Add: "or using NSS's ASN.1 encoder"

http://codereview.chromium.org/208032/diff/7001/7004#newcode98
Line 98: PK11SlotInfo *slot = PK11_GetBestSlot(CKM_RSA_PKCS_KEY_PAIR_GEN, NULL);
Could you try PK11_GetInternalSlot() instead?  This
uses NSS's built-in software crypto module.

http://codereview.chromium.org/208032/diff/7001/7004#newcode112
Line 112: SECKEY_DestroyPublicKey(public_key);
I wonder if we should keep the public key around for the
ExportPublicKey method...

http://codereview.chromium.org/208032/diff/7001/7004#newcode121
Line 121: PK11SlotInfo *slot = PK11_GetBestSlot(CKM_RSA_PKCS_KEY_PAIR_GEN,
NULL);
Same here: please use PK11_GetInternalSlot() instead.

http://codereview.chromium.org/208032/diff/7001/7004#newcode135
Line 135: PK11_FreeSlot(slot);
Move this up to line 131, before the if (rv != SECSuccess)
check.

http://codereview.chromium.org/208032/diff/7001/7004#newcode163
Line 163: if (rv != SECSuccess) {
We need to call SECITEM_FreeItem before we return on error.
In NSS (C code), we do this with goto statements.  You can
use goto here, too.  It requires declaring all the SECItem
variables upfront and initializing all of them to
{siBuffer, NULL, 0}.  Alternatively, you can add a C++
wrapper class for SECItem whose destructor calls
SECITEM_FreeItem.

http://codereview.chromium.org/208032/diff/7001/7004#newcode263
Line 263: SECItem* der_pubkey = PK11_DEREncodePublicKey(public_key);
Nit: check for a NULL return value from SECKEY_ConvertToPublicKey
or PK11_DEREncodePublicKey.  Remember to destroy public_key
if PK11_DEREncodePublicKey fails.

http://codereview.chromium.org/208032/diff/7001/7006
File base/crypto/signature_creator_nss.cc (right):

http://codereview.chromium.org/208032/diff/7001/7006#newcode50
Line 50: SECStatus rv = SGN_Update(sign_context_,
Nit: format function calls in one of the formats recommended by the style guide:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Calls

http://codereview.chromium.org/208032/diff/7001/7007
File base/crypto/signature_creator_win.cc (right):

http://codereview.chromium.org/208032/diff/7001/7007#newcode26
Line 26: SignatureCreator::SignatureCreator() : hash_object_(NULL) {}
Nit: Use 0 instead of NULL (see line 33 and line 64).

[rafaelw, 2009-09-22 21:21:58]

http://codereview.chromium.org/208032/diff/7001/7004
File base/crypto/rsa_private_key_nss.cc (right):

http://codereview.chromium.org/208032/diff/7001/7004#newcode7
Line 7: #include <cryptohi.h>
On 2009/09/22 19:06:20, wtc wrote:
> Nit: separate the C and C++ system headers.
> 
> #include <cryptohi.h>
> #include <keyhi.h>
> #include <pk11pub.h>
> 
> #include <iostream>
> #include <list>
> 
> See
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde...
> 

Done.

http://codereview.chromium.org/208032/diff/7001/7004#newcode19
Line 19: // rsa_private_key_win.cc.
On 2009/09/22 19:06:20, wtc wrote:
> Add: "or using NSS's ASN.1 encoder"

Done.

http://codereview.chromium.org/208032/diff/7001/7004#newcode98
Line 98: PK11SlotInfo *slot = PK11_GetBestSlot(CKM_RSA_PKCS_KEY_PAIR_GEN, NULL);
On 2009/09/22 19:06:20, wtc wrote:
> Could you try PK11_GetInternalSlot() instead?  This
> uses NSS's built-in software crypto module.

Done.

http://codereview.chromium.org/208032/diff/7001/7004#newcode112
Line 112: SECKEY_DestroyPublicKey(public_key);
On 2009/09/22 19:06:20, wtc wrote:
> I wonder if we should keep the public key around for the
> ExportPublicKey method...

Done.

http://codereview.chromium.org/208032/diff/7001/7004#newcode121
Line 121: PK11SlotInfo *slot = PK11_GetBestSlot(CKM_RSA_PKCS_KEY_PAIR_GEN,
NULL);
On 2009/09/22 19:06:20, wtc wrote:
> Same here: please use PK11_GetInternalSlot() instead.

Done.

http://codereview.chromium.org/208032/diff/7001/7004#newcode135
Line 135: PK11_FreeSlot(slot);
On 2009/09/22 19:06:20, wtc wrote:
> Move this up to line 131, before the if (rv != SECSuccess)
> check.

Done.

http://codereview.chromium.org/208032/diff/7001/7004#newcode163
Line 163: if (rv != SECSuccess) {
On 2009/09/22 19:06:20, wtc wrote:
> We need to call SECITEM_FreeItem before we return on error.
> In NSS (C code), we do this with goto statements.  You can
> use goto here, too.  It requires declaring all the SECItem
> variables upfront and initializing all of them to
> {siBuffer, NULL, 0}.  Alternatively, you can add a C++
> wrapper class for SECItem whose destructor calls
> SECITEM_FreeItem.

Done.

http://codereview.chromium.org/208032/diff/7001/7004#newcode263
Line 263: SECItem* der_pubkey = PK11_DEREncodePublicKey(public_key);
On 2009/09/22 19:06:20, wtc wrote:
> Nit: check for a NULL return value from SECKEY_ConvertToPublicKey
> or PK11_DEREncodePublicKey.  Remember to destroy public_key
> if PK11_DEREncodePublicKey fails.

Done.

http://codereview.chromium.org/208032/diff/7001/7006
File base/crypto/signature_creator_nss.cc (right):

http://codereview.chromium.org/208032/diff/7001/7006#newcode50
Line 50: SECStatus rv = SGN_Update(sign_context_,
On 2009/09/22 19:06:20, wtc wrote:
> Nit: format function calls in one of the formats recommended by the style
guide:
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Calls

Done.

http://codereview.chromium.org/208032/diff/7001/7007
File base/crypto/signature_creator_win.cc (right):

http://codereview.chromium.org/208032/diff/7001/7007#newcode26
Line 26: SignatureCreator::SignatureCreator() : hash_object_(NULL) {}
On 2009/09/22 19:06:20, wtc wrote:
> Nit: Use 0 instead of NULL (see line 33 and line 64).

Done.

[wtc, 2009-09-22 23:02:53]

LGTM.  Just three nits below.  You can commit without further review by me.

http://codereview.chromium.org/208032/diff/17/20
File base/crypto/rsa_private_key_nss.cc (right):

http://codereview.chromium.org/208032/diff/17/20#newcode8
Line 8: #include <iostream>
Move <iostream> below to the C++ system header section.

http://codereview.chromium.org/208032/diff/17/20#newcode90
Line 90: static bool ReadAttributeAndPrependInteger(SECKEYPrivateKey* key,
Nice solution!

http://codereview.chromium.org/208032/diff/17/20#newcode91
Line 91: CK_ATTRIBUTE_TYPE type,
Nit: indentation is off by one character.

http://codereview.chromium.org/208032/diff/17/22
File base/crypto/signature_creator_nss.cc (right):

http://codereview.chromium.org/208032/diff/17/22#newcode68
Line 68: signature->assign(signature_item.data, signature_item.data +
Nit: please also fix the formatting of this function call.

[rafaelw, 2009-09-22 23:12:31]

http://codereview.chromium.org/208032/diff/17/20
File base/crypto/rsa_private_key_nss.cc (right):

http://codereview.chromium.org/208032/diff/17/20#newcode8
Line 8: #include <iostream>
On 2009/09/22 23:02:53, wtc wrote:
> Move <iostream> below to the C++ system header section.

Woops. sorry. done.

http://codereview.chromium.org/208032/diff/17/20#newcode90
Line 90: static bool ReadAttributeAndPrependInteger(SECKEYPrivateKey* key,
On 2009/09/22 23:02:53, wtc wrote:
> Nice solution!

thanks

http://codereview.chromium.org/208032/diff/17/20#newcode91
Line 91: CK_ATTRIBUTE_TYPE type,
On 2009/09/22 23:02:53, wtc wrote:
> Nit: indentation is off by one character.

Done.

http://codereview.chromium.org/208032/diff/17/22
File base/crypto/signature_creator_nss.cc (right):

http://codereview.chromium.org/208032/diff/17/22#newcode68
Line 68: signature->assign(signature_item.data, signature_item.data +
On 2009/09/22 23:02:53, wtc wrote:
> Nit: please also fix the formatting of this function call.

Done.