Add PK11_ExportDERPrivateKeyInfo and PK11_ExportPrivKeyInfo.

Add PK11_ExportDERPrivateKeyInfo and PK11_ExportPrivKeyInfo.

NSS bug: https://bugzilla.mozilla.org/show_bug.cgi?id=519255

R=eroman@chromium.org,rsleevi@chromium.org
BUG=none
TEST=none

[eroman, 2014-03-20 00:12:04]

https://codereview.chromium.org/205343004/diff/1/nss/lib/pk11wrap/pk11akey.c
File nss/lib/pk11wrap/pk11akey.c (right):

https://codereview.chromium.org/205343004/diff/1/nss/lib/pk11wrap/pk11akey.c#...
nss/lib/pk11wrap/pk11akey.c:1709: pki = PK11_ExportPrivKeyInfo(pk, wincx);
FYI this looks like it is mixing tabs and spaces. Not sure what the NSS style is

https://codereview.chromium.org/205343004/diff/1/nss/lib/pk11wrap/pk11pk12.c
File nss/lib/pk11wrap/pk11pk12.c (right):

https://codereview.chromium.org/205343004/diff/1/nss/lib/pk11wrap/pk11pk12.c#...
nss/lib/pk11wrap/pk11pk12.c:21: #include "prerror.h"
is there an expected ordering to headers?

https://codereview.chromium.org/205343004/diff/1/nss/lib/pk11wrap/pk11pk12.c#...
nss/lib/pk11wrap/pk11pk12.c:559: rawKey =  PORT_ArenaZNew(arena,
SECKEYRawPrivateKey);
why allocate rawKey using arena rather than stack? Is for zero-initialization?

https://codereview.chromium.org/205343004/diff/1/nss/lib/pk11wrap/pk11pk12.c#...
nss/lib/pk11wrap/pk11pk12.c:566: prepare_rsa_priv_key_export_for_asn1(rawKey);
What guarantees does PK11_ReadAttribute give? I would have expected that
PK11_ReadAttribute can mutate the SECItem's type, in which case this should be
called after all of the ReadAttributes().

https://codereview.chromium.org/205343004/diff/1/nss/lib/pk11wrap/pk11pk12.c#...
nss/lib/pk11wrap/pk11pk12.c:582: }
Is it worth calling out that the PrivateKeyInfo's "attributes" field is not
written?

https://codereview.chromium.org/205343004/diff/1/nss/lib/pk11wrap/pk11pk12.c#...
nss/lib/pk11wrap/pk11pk12.c:585: encoded = SEC_ASN1EncodeItem(arena,
&pki->privateKey, rawKey,
Should you check the return value for success?

https://codereview.chromium.org/205343004/diff/1/nss/lib/pk11wrap/pk11pk12.c#...
nss/lib/pk11wrap/pk11pk12.c:592: pki->arena = arena;
Apologies for lack of NSS foo: does this mean the caller is reponsible for
freeing the arena pool?

[eroman, 2014-03-20 00:23:35]

https://codereview.chromium.org/205343004/diff/1/nss/lib/pk11wrap/pk11pk12.c
File nss/lib/pk11wrap/pk11pk12.c (right):

https://codereview.chromium.org/205343004/diff/1/nss/lib/pk11wrap/pk11pk12.c#...
nss/lib/pk11wrap/pk11pk12.c:590: pki->version.data = &pkiVersion;
I found this a bit subtle, but maybe it is in line with the NSS style.

I would suggest using SEC_ASN1EncodeInteger() so it more robustly handles future
changes.

I also had to do a double-take and verify that this wasn't returning a pointer
to a local variable. (But then realized it it function-static so *should* be
ok).

[eroman, 2014-03-20 00:32:55]

Last question: are there NSS unit tests to update?

[Ryan Sleevi, 2014-03-20 01:24:27]

https://codereview.chromium.org/205343004/diff/1/nss/lib/pk11wrap/pk11pk12.c
File nss/lib/pk11wrap/pk11pk12.c (right):

https://codereview.chromium.org/205343004/diff/1/nss/lib/pk11wrap/pk11pk12.c#...
nss/lib/pk11wrap/pk11pk12.c:559: rawKey =  PORT_ArenaZNew(arena,
SECKEYRawPrivateKey);
On 2014/03/20 00:12:04, eroman wrote:
> why allocate rawKey using arena rather than stack? Is for zero-initialization?

More importantly, this strikes me as a bit inconsistent, since
SECKEY_DestroyPrivateKeyInfo attempts to zeroize the private key data associated
with the SECKEYPrivateKeyInfo, but we're not doing the same when dealing with
the SECKEYRawPrivateKey here.

Not that NSS's zeroization does anything though...

https://codereview.chromium.org/205343004/diff/1/nss/lib/pk11wrap/pk11pk12.c#...
nss/lib/pk11wrap/pk11pk12.c:582: }
On 2014/03/20 00:12:04, eroman wrote:
> Is it worth calling out that the PrivateKeyInfo's "attributes" field is not
> written?

nah

https://codereview.chromium.org/205343004/diff/1/nss/lib/pk11wrap/pk11pk12.c#...
nss/lib/pk11wrap/pk11pk12.c:585: encoded = SEC_ASN1EncodeItem(arena,
&pki->privateKey, rawKey,
On 2014/03/20 00:12:04, eroman wrote:
> Should you check the return value for success?

+1

https://codereview.chromium.org/205343004/diff/1/nss/lib/pk11wrap/pk11pk12.c#...
nss/lib/pk11wrap/pk11pk12.c:590: pki->version.data = &pkiVersion;
On 2014/03/20 00:23:35, eroman wrote:
> I found this a bit subtle, but maybe it is in line with the NSS style.
> 
> I would suggest using SEC_ASN1EncodeInteger() so it more robustly handles
future
> changes.
> 
> I also had to do a double-take and verify that this wasn't returning a pointer
> to a local variable. (But then realized it it function-static so *should* be
> ok).

I have to agree with Eric, that this strikes me as a little dangerous because
it's relying on an internal implementation detail of
SECKEY_DestroyPrivateKeyInfo (namely, if arena != NULL, it will only free the
arena - but if arena isn't set, it will call SECITEM_ZfreeItem and potentially
nuke the stack)

https://codereview.chromium.org/205343004/diff/1/nss/lib/pk11wrap/pk11pk12.c#...
nss/lib/pk11wrap/pk11pk12.c:592: pki->arena = arena;
On 2014/03/20 00:12:04, eroman wrote:
> Apologies for lack of NSS foo: does this mean the caller is reponsible for
> freeing the arena pool?

Not directly, but through using SECKEY_DestroyPrivateKeyInfo, yes.

[wtc, 2014-03-22 01:08:00]

Eric, Ryan: please review patch set 2.

Thank you very much for your review. I made all the changes
you suggested.

https://codereview.chromium.org/205343004/diff/1/nss/lib/pk11wrap/pk11akey.c
File nss/lib/pk11wrap/pk11akey.c (right):

https://codereview.chromium.org/205343004/diff/1/nss/lib/pk11wrap/pk11akey.c#...
nss/lib/pk11wrap/pk11akey.c:1709: pki = PK11_ExportPrivKeyInfo(pk, wincx);

On 2014/03/20 00:12:04, eroman wrote:
> FYI this looks like it is mixing tabs and spaces. Not sure what the NSS style
is

You're right. Unfortunately in several directories of the NSS source
tree, such as lib/softoken, lib/pk11wrap, and lib/ssl, tabs are used.
This is a common source of confusion for new NSS contributors. At
some point I should run a tool to convert all those tabs to spaces.

https://codereview.chromium.org/205343004/diff/1/nss/lib/pk11wrap/pk11pk12.c
File nss/lib/pk11wrap/pk11pk12.c (right):

https://codereview.chromium.org/205343004/diff/1/nss/lib/pk11wrap/pk11pk12.c#...
nss/lib/pk11wrap/pk11pk12.c:21: #include "prerror.h"

On 2014/03/20 00:12:04, eroman wrote:
> is there an expected ordering to headers?

No. Some NSS headers, like the Unix system headers, actually have
dependencies and must be included in a particular order. As a result
NSS headers cannot be included in alphabetical order, and therefore
it is hard to recommend an ordering to headers.

Here I either just added the new header at the end, or I added it
next to the related secerr.h.

https://codereview.chromium.org/205343004/diff/1/nss/lib/pk11wrap/pk11pk12.c#...
nss/lib/pk11wrap/pk11pk12.c:559: rawKey =  PORT_ArenaZNew(arena,
SECKEYRawPrivateKey);

On 2014/03/20 00:12:04, eroman wrote:
> why allocate rawKey using arena rather than stack? Is for zero-initialization?

I may have copied this code from some other function. I agree rawKey
can be a local variable on the stack, so I made this change.

On 2014/03/20 01:24:27, Ryan Sleevi wrote:
> 
> More importantly, this strikes me as a bit inconsistent, since
> SECKEY_DestroyPrivateKeyInfo attempts to zeroize the private key data
associated
> with the SECKEYPrivateKeyInfo, but we're not doing the same when dealing with
> the SECKEYRawPrivateKey here.

The private key data associated with SECKEYRawPrivateKey is all
allocated from the arena of the SECKEYPrivateKeyInfo, so the data
wll all be zeroized by SECKEY_DestroyPrivateKeyInfo.

This approach uses more memory because the data associated with
SECKEYRawPrivateKey is not freed as soon as possible. My expectation
is that people will mainly use the PK11_ExportDERPrivateKeyInfo
function, so this approach allows me to save one arena creation
call.

https://codereview.chromium.org/205343004/diff/1/nss/lib/pk11wrap/pk11pk12.c#...
nss/lib/pk11wrap/pk11pk12.c:566: prepare_rsa_priv_key_export_for_asn1(rawKey);

PK11_ReadAttribute doesn't modify the SECItem's type. I added a comment
to document this.

https://codereview.chromium.org/205343004/diff/1/nss/lib/pk11wrap/pk11pk12.c#...
nss/lib/pk11wrap/pk11pk12.c:585: encoded = SEC_ASN1EncodeItem(arena,
&pki->privateKey, rawKey,
On 2014/03/20 00:12:04, eroman wrote:
> Should you check the return value for success?

Done.

https://codereview.chromium.org/205343004/diff/1/nss/lib/pk11wrap/pk11pk12.c#...
nss/lib/pk11wrap/pk11pk12.c:590: pki->version.data = &pkiVersion;
On 2014/03/20 01:24:27, Ryan Sleevi wrote:
> On 2014/03/20 00:23:35, eroman wrote:
> > I found this a bit subtle, but maybe it is in line with the NSS style.
> > 
> > I would suggest using SEC_ASN1EncodeInteger() so it more robustly handles
> future
> > changes.
> > 
> > I also had to do a double-take and verify that this wasn't returning a
pointer
> > to a local variable. (But then realized it it function-static so *should* be
> > ok).
> 
> I have to agree with Eric, that this strikes me as a little dangerous because
> it's relying on an internal implementation detail of
> SECKEY_DestroyPrivateKeyInfo (namely, if arena != NULL, it will only free the
> arena - but if arena isn't set, it will call SECITEM_ZfreeItem and potentially
> nuke the stack)
> 

Done.

[eroman, 2014-03-24 22:46:38]

lgtm. however don't consider my review as actually worth anything :)

https://codereview.chromium.org/205343004/diff/20001/nss/lib/pk11wrap/pk11pk12.c
File nss/lib/pk11wrap/pk11pk12.c (right):

https://codereview.chromium.org/205343004/diff/20001/nss/lib/pk11wrap/pk11pk1...
nss/lib/pk11wrap/pk11pk12.c:573: rawKey.u.rsa.version.data = &rsaVersion;
Should this allocate in the arenapool too? I guess it doesn't matter since
rawKey isn't returned back to caller.

[Ryan Sleevi, 2014-03-24 23:57:40]

LGTM mod nit

https://codereview.chromium.org/205343004/diff/20001/nss/lib/pk11wrap/pk11pk12.c
File nss/lib/pk11wrap/pk11pk12.c (right):

https://codereview.chromium.org/205343004/diff/20001/nss/lib/pk11wrap/pk11pk1...
nss/lib/pk11wrap/pk11pk12.c:577: prepare_rsa_priv_key_export_for_asn1(&rawKey);
Shouldn't you prepare after reading all attributes (eg: line 593), rather than
before?

[wtc, 2014-03-26 01:47:14]

Thank you for the review. Patch set 4 has the last change suggested by Eric and
is ready for checkin.

https://codereview.chromium.org/205343004/diff/20001/nss/lib/pk11wrap/pk11pk12.c
File nss/lib/pk11wrap/pk11pk12.c (right):

https://codereview.chromium.org/205343004/diff/20001/nss/lib/pk11wrap/pk11pk1...
nss/lib/pk11wrap/pk11pk12.c:573: rawKey.u.rsa.version.data = &rsaVersion;

On 2014/03/24 22:46:38, eroman wrote:
> Should this allocate in the arenapool too? I guess it doesn't matter since
> rawKey isn't returned back to caller.

Correct. If I allocate this in the arena pool, it is only for consistency.

https://codereview.chromium.org/205343004/diff/20001/nss/lib/pk11wrap/pk11pk1...
nss/lib/pk11wrap/pk11pk12.c:577: prepare_rsa_priv_key_export_for_asn1(&rawKey);

On 2014/03/24 23:57:40, Ryan Sleevi wrote:
> Shouldn't you prepare after reading all attributes (eg: line 593), rather than
> before?

The order doesn't matter.

prepare_rsa_priv_key_export_for_asn1 sets the |type| field of the SECItems.

The ReadAttribute calls set the |data| and |len| fields of the SECItems.

In the SECItem structure definition, |type| is listed before |data| and |len|,
so setting them in this order looks nicer.

[wtc, 2014-03-26 01:51:51]

The CQ bit was checked by wtc@chromium.org

[commit-bot: I haz the power, 2014-03-26 01:52:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wtc@chromium.org/205343004/170001

[commit-bot: I haz the power, 2014-03-26 01:52:53]

Change committed as 259440