Add PK11_ExportDERPrivateKeyInfo and PK11_ExportPrivKeyInfo.

nss/lib/pk11wrap/pk11pk12.c

Index: nss/lib/pk11wrap/pk11pk12.c
===================================================================
--- nss/lib/pk11wrap/pk11pk12.c	(revision 257452)
+++ nss/lib/pk11wrap/pk11pk12.c	(working copy)
@@ -18,6 +18,7 @@
 #include "secoid.h"
 #include "secasn1.h"
 #include "secerr.h"
+#include "prerror.h"

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

is there an expected ordering to headers?

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

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.

 
 
 
@@ -516,3 +517,79 @@
 
 }
 
+SECItem *
+PK11_ExportDERPrivateKeyInfo(SECKEYPrivateKey *pk, void *wincx)
+{
+    SECKEYPrivateKeyInfo *pki = PK11_ExportPrivKeyInfo(pk, wincx);
+    SECItem *derPKI;
+    derPKI = SEC_ASN1EncodeItem(NULL, NULL, pki,
+                                SECKEY_PrivateKeyInfoTemplate);
+    SECKEY_DestroyPrivateKeyInfo(pki, PR_TRUE);
+    return derPKI;
+}
+
+static PRBool
+ReadAttribute(SECKEYPrivateKey* key, CK_ATTRIBUTE_TYPE type,
+              PLArenaPool* arena, SECItem* output)
+{
+    SECStatus rv = PK11_ReadAttribute(key->pkcs11Slot, key->pkcs11ID, type,
+                                      arena, output);
+    return rv == SECSuccess;
+}
+
+SECKEYPrivateKeyInfo *
+PK11_ExportPrivKeyInfo(SECKEYPrivateKey *pk, void *wincx)
+{
+    /* PrivateKeyInfo version (always zero) */
+    static const unsigned char pkiVersion = 0;
+    /* RSAPrivateKey version (always zero) */
+    static const unsigned char rsaVersion = 0;
+    PLArenaPool *arena;
+    SECKEYRawPrivateKey *rawKey;
+    SECKEYPrivateKeyInfo *pki;
+    SECItem *encoded;
+    SECStatus rv;
+
+    if (pk->keyType != rsaKey) {
+        PORT_SetError(PR_NOT_IMPLEMENTED_ERROR);
+        return NULL;
+    }
+
+    arena = PORT_NewArena(DER_DEFAULT_CHUNKSIZE);
+    rawKey =  PORT_ArenaZNew(arena, SECKEYRawPrivateKey);

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

why allocate rawKey using arena rather than stack? Is for zero-initialization?

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

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...

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

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:
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.

+    rawKey->keyType = pk->keyType;
+    rawKey->u.rsa.version.type = siUnsignedInteger;
+    rawKey->u.rsa.version.data = &rsaVersion;
+    rawKey->u.rsa.version.len = 1;
+
+    /* Read the component attributes of the private key */
+    prepare_rsa_priv_key_export_for_asn1(rawKey);

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

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().

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

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

+    if (!ReadAttribute(pk, CKA_MODULUS, arena, &rawKey->u.rsa.modulus) ||
+        !ReadAttribute(pk, CKA_PUBLIC_EXPONENT, arena,
+                       &rawKey->u.rsa.publicExponent) ||
+        !ReadAttribute(pk, CKA_PRIVATE_EXPONENT, arena,
+                       &rawKey->u.rsa.privateExponent) ||
+        !ReadAttribute(pk, CKA_PRIME_1, arena, &rawKey->u.rsa.prime1) ||
+        !ReadAttribute(pk, CKA_PRIME_2, arena, &rawKey->u.rsa.prime2) ||
+        !ReadAttribute(pk, CKA_EXPONENT_1, arena,
+                       &rawKey->u.rsa.exponent1) ||
+        !ReadAttribute(pk, CKA_EXPONENT_2, arena,
+                       &rawKey->u.rsa.exponent2) ||
+        !ReadAttribute(pk, CKA_COEFFICIENT, arena,
+                       &rawKey->u.rsa.coefficient)) {
+        PORT_FreeArena(arena, PR_TRUE);
+        return NULL;
+    }

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

Is it worth calling out that the PrivateKeyInfo's "attributes" field is not
written?

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

nah

+ 
+    pki = PORT_ArenaZNew(arena, SECKEYPrivateKeyInfo);
+    encoded = SEC_ASN1EncodeItem(arena, &pki->privateKey, rawKey,

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

Should you check the return value for success?

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

+1

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

Done.

+                                 SECKEY_RSAPrivateKeyExportTemplate);
+    rv = SECOID_SetAlgorithmID(arena, &pki->algorithm,
+                               SEC_OID_PKCS1_RSA_ENCRYPTION, NULL);
+    pki->version.type = siUnsignedInteger;
+    pki->version.data = &pkiVersion;

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

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).

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

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)

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

Done.

+    pki->version.len = 1;
+    pki->arena = arena;

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

Apologies for lack of NSS foo: does this mean the caller is reponsible for
freeing the arena pool?

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

Not directly, but through using SECKEY_DestroyPrivateKeyInfo, yes.

+
+    return pki;
+}