[webcrypto] Add PKCS#8 export for RSA private keys for NSS.

[webcrypto] Add PKCS#8 export for RSA private keys for NSS.

BUG=245025
R=rsleevi@chromium.org, wtc@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=259063

[eroman, 2014-03-19 03:29:01]

I am still going to add more tests, but was hoping for some implementation
feedaback in the meantime.

Thanks!

[Ryan Sleevi, 2014-03-19 03:52:17]

https://codereview.chromium.org/203753009/diff/20001/content/child/webcrypto/...
File content/child/webcrypto/platform_crypto_nss.cc (right):

https://codereview.chromium.org/203753009/diff/20001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:571: // doesn't use multi-prime
can safely use version=0.
comment:
Since NSS doesn't support multi-prime, it is safe to use version=0

https://codereview.chromium.org/203753009/diff/20001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:815: const SECOidTag algorithm =
SEC_OID_PKCS1_RSA_ENCRYPTION;
You should clarify this - only RSAES/RSASSA-PKCS1-v1_5 are supported

This is not the correct algorithm for RSA-PSS / RSA-OAEP.

https://codereview.chromium.org/203753009/diff/20001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:818: SECKEYPrivateKeyInfo pki =
{};
naming: private_key_info

https://codereview.chromium.org/203753009/diff/20001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:823: return Status::Error();
LEAK: You fail to assign anything to free_private_key; as a result, when this
fails, it'll leak - per your comment on line 612.

[wtc, 2014-03-19 04:04:21]

Patch set 2 LGTM. Please wait for Ryan's approval.

https://codereview.chromium.org/203753009/diff/20001/content/child/webcrypto/...
File content/child/webcrypto/platform_crypto_nss.cc (right):

https://codereview.chromium.org/203753009/diff/20001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:599: static bool
ReadUint(SECKEYPrivateKey* key,

Nit: this function doesn't need to be marked as static because it is inside the
unnamed namespace.

https://codereview.chromium.org/203753009/diff/20001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:615: bool
InitRSAPrivateKey(SECKEYPrivateKey* key, RSAPrivateKey* out) {

This function probably should zero |*out| first, or require the caller to do
that. Otherwise, you can't call FreeRSAPrivateKey safely on failure.

https://codereview.chromium.org/203753009/diff/20001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:818: SECKEYPrivateKeyInfo pki =
{};

I usually see {0} as this kind of initializer. Are you sure {} works?

https://codereview.chromium.org/203753009/diff/20001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:843: NULL, NULL, &pki,
SEC_ASN1_GET(SECKEY_PrivateKeyInfoTemplate)));

When I read this code, I finally remembered I wrote an NSS patch for this
before: https://bugzilla.mozilla.org/show_bug.cgi?id=519255

Maybe we should get that patch reviewed and checked into NSS now.

[eroman, 2014-03-19 04:27:42]

https://codereview.chromium.org/203753009/diff/20001/content/child/webcrypto/...
File content/child/webcrypto/platform_crypto_nss.cc (right):

https://codereview.chromium.org/203753009/diff/20001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:599: static bool
ReadUint(SECKEYPrivateKey* key,
On 2014/03/19 04:04:21, wtc wrote:
> 
> Nit: this function doesn't need to be marked as static because it is inside
the
> unnamed namespace.

Done. Agreed (copy paste error).

https://codereview.chromium.org/203753009/diff/20001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:615: bool
InitRSAPrivateKey(SECKEYPrivateKey* key, RSAPrivateKey* out) {
On 2014/03/19 04:04:21, wtc wrote:
> 
> This function probably should zero |*out| first, or require the caller to do
> that. Otherwise, you can't call FreeRSAPrivateKey safely on failure.

Correct. It is the expectation that caller provides a zero-ed RSAPrivateKey. It
felt cleaner to use {} initialization in caller rather than a memset() in here.

For now I added DCHECK() to test, as well as usage comment.

Let me know what your preference is and I can change.

https://codereview.chromium.org/203753009/diff/20001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:815: const SECOidTag algorithm =
SEC_OID_PKCS1_RSA_ENCRYPTION;
On 2014/03/19 03:52:18, Ryan Sleevi wrote:
> You should clarify this - only RSAES/RSASSA-PKCS1-v1_5 are supported
> 
> This is not the correct algorithm for RSA-PSS / RSA-OAEP.

Added this code to cover this:

   // TODO(eroman): Support other RSA key types as they are added to blink.
   if (key_algorithm.id() != blink::WebCryptoAlgorithmIdRsaEsPkcs1v1_5 &&
       key_algorithm.id() != blink::WebCryptoAlgorithmIdRsaSsaPkcs1v1_5)
     return Status::ErrorUnsupported();

https://codereview.chromium.org/203753009/diff/20001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:818: SECKEYPrivateKeyInfo pki =
{};
On 2014/03/19 03:52:18, Ryan Sleevi wrote:
> naming: private_key_info

Done.

https://codereview.chromium.org/203753009/diff/20001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:818: SECKEYPrivateKeyInfo pki =
{};
On 2014/03/19 04:04:21, wtc wrote:
> 
> I usually see {0} as this kind of initializer. Are you sure {} works?

Yes. In fact I cannot use {0} in C++ since it tries to apply that as the ctor
for the SECItems and fails to compile.

{} is defined as default initializing everything, and for POD this means zeros.

https://codereview.chromium.org/203753009/diff/20001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:823: return Status::Error();
On 2014/03/19 03:52:18, Ryan Sleevi wrote:
> LEAK: You fail to assign anything to free_private_key; as a result, when this
> fails, it'll leak - per your comment on line 612.

Good catch!

https://codereview.chromium.org/203753009/diff/20001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:843: NULL, NULL, &pki,
SEC_ASN1_GET(SECKEY_PrivateKeyInfoTemplate)));
On 2014/03/19 04:04:21, wtc wrote:
> 
> When I read this code, I finally remembered I wrote an NSS patch for this
> before: https://bugzilla.mozilla.org/show_bug.cgi?id=519255
> 
> Maybe we should get that patch reviewed and checked into NSS now.

I think adding it to NSS would be great!

We will still need comparable code in Chrome though if we want things to work on
Linux.

And certainly my version is pretty incomplete.

[wtc, 2014-03-19 16:59:53]

https://codereview.chromium.org/203753009/diff/20001/content/child/webcrypto/...
File content/child/webcrypto/platform_crypto_nss.cc (right):

https://codereview.chromium.org/203753009/diff/20001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:843: NULL, NULL, &pki,
SEC_ASN1_GET(SECKEY_PrivateKeyInfoTemplate)));

On 2014/03/19 04:27:42, eroman wrote:
> 
> I think adding it to NSS would be great!
> 
> We will still need comparable code in Chrome though if we want things to work
on
> Linux.

You can consider copying my C code to this file. I found that my C code uses an
NSS internal type SECKEYRawPrivateKey and an internal template
SECKEY_RSAPrivateKeyExportTemplate. Both of them are very simple. To avoid
source code license issues, I can specify what they should be, so you can
reproduce them in a clean-room fashion:

SECKEYRawPrivateKey: a struct containing three fields:
- a pointer to a PLArenaPool
- a KeyType field
- a union of SECKEYRSAPrivateKey, SECKEYDSAPrivateKey, and SECKEYDHPrivateKey.
You only need SECKEYRSAPrivateKey.

SECKEYRSAPrivateKey: essentially the same as your RSAPrivateKey struct, except
that it also has a pointer to a PLArenaPool at the beginning.

SECKEY_RSAPrivateKeyExportTemplate: essentially the same as your
RSAPrivateKeyTemplate.

The advantage of copying my C code is that once you verify it works, I can check
it into NSS.

[padolph, 2014-03-19 17:16:40]

On 2014/03/19 04:27:42, eroman wrote:
>
https://codereview.chromium.org/203753009/diff/20001/content/child/webcrypto/...
> File content/child/webcrypto/platform_crypto_nss.cc (right):
> 
>
https://codereview.chromium.org/203753009/diff/20001/content/child/webcrypto/...
> content/child/webcrypto/platform_crypto_nss.cc:599: static bool
> ReadUint(SECKEYPrivateKey* key,
> On 2014/03/19 04:04:21, wtc wrote:
> > 
> > Nit: this function doesn't need to be marked as static because it is inside
> the
> > unnamed namespace.
> 
> Done. Agreed (copy paste error).
> 
>
https://codereview.chromium.org/203753009/diff/20001/content/child/webcrypto/...
> content/child/webcrypto/platform_crypto_nss.cc:615: bool
> InitRSAPrivateKey(SECKEYPrivateKey* key, RSAPrivateKey* out) {
> On 2014/03/19 04:04:21, wtc wrote:
> > 
> > This function probably should zero |*out| first, or require the caller to do
> > that. Otherwise, you can't call FreeRSAPrivateKey safely on failure.
> 
> Correct. It is the expectation that caller provides a zero-ed RSAPrivateKey.
It
> felt cleaner to use {} initialization in caller rather than a memset() in
here.
> 
> For now I added DCHECK() to test, as well as usage comment.
> 
> Let me know what your preference is and I can change.
> 
>
https://codereview.chromium.org/203753009/diff/20001/content/child/webcrypto/...
> content/child/webcrypto/platform_crypto_nss.cc:815: const SECOidTag algorithm
=
> SEC_OID_PKCS1_RSA_ENCRYPTION;
> On 2014/03/19 03:52:18, Ryan Sleevi wrote:
> > You should clarify this - only RSAES/RSASSA-PKCS1-v1_5 are supported
> > 
> > This is not the correct algorithm for RSA-PSS / RSA-OAEP.
> 
> Added this code to cover this:
> 
>    // TODO(eroman): Support other RSA key types as they are added to blink.
>    if (key_algorithm.id() != blink::WebCryptoAlgorithmIdRsaEsPkcs1v1_5 &&
>        key_algorithm.id() != blink::WebCryptoAlgorithmIdRsaSsaPkcs1v1_5)
>      return Status::ErrorUnsupported();
> 
>
https://codereview.chromium.org/203753009/diff/20001/content/child/webcrypto/...
> content/child/webcrypto/platform_crypto_nss.cc:818: SECKEYPrivateKeyInfo pki =
> {};
> On 2014/03/19 03:52:18, Ryan Sleevi wrote:
> > naming: private_key_info
> 
> Done.
> 
>
https://codereview.chromium.org/203753009/diff/20001/content/child/webcrypto/...
> content/child/webcrypto/platform_crypto_nss.cc:818: SECKEYPrivateKeyInfo pki =
> {};
> On 2014/03/19 04:04:21, wtc wrote:
> > 
> > I usually see {0} as this kind of initializer. Are you sure {} works?
> 
> Yes. In fact I cannot use {0} in C++ since it tries to apply that as the ctor
> for the SECItems and fails to compile.
> 
> {} is defined as default initializing everything, and for POD this means
zeros.
> 
>
https://codereview.chromium.org/203753009/diff/20001/content/child/webcrypto/...
> content/child/webcrypto/platform_crypto_nss.cc:823: return Status::Error();
> On 2014/03/19 03:52:18, Ryan Sleevi wrote:
> > LEAK: You fail to assign anything to free_private_key; as a result, when
this
> > fails, it'll leak - per your comment on line 612.
> 
> Good catch!
> 
>
https://codereview.chromium.org/203753009/diff/20001/content/child/webcrypto/...
> content/child/webcrypto/platform_crypto_nss.cc:843: NULL, NULL, &pki,
> SEC_ASN1_GET(SECKEY_PrivateKeyInfoTemplate)));
> On 2014/03/19 04:04:21, wtc wrote:
> > 
> > When I read this code, I finally remembered I wrote an NSS patch for this
> > before: https://bugzilla.mozilla.org/show_bug.cgi?id=519255
> > 
> > Maybe we should get that patch reviewed and checked into NSS now.
> 
> I think adding it to NSS would be great!
> 
> We will still need comparable code in Chrome though if we want things to work
on
> Linux.
> 
> And certainly my version is pretty incomplete.

One thing I don't quite understand about RSA are the private key data
requirements. Only the modulus ('n'), public exponent ('e'), and private
exponent ('d') are mandatory and the other 6 are optional, used for
optimization. So doing a pkcs8 import of a key with only the first three would
create a functional private key. In fact, I realize we are missing a test for
this case.

But what happens if you then export that key? Should the output ASN.1 blob also
only contain those 3 values with the rest missing (setting them to 0 does not
seem correct)? That will require special checks in the implementation.

Or does NSS compute the missing ones and fill them in? I haven't checked the
code yet. In that case we need special handling to make an import/export
roundtrip test pass.

[eroman, 2014-03-19 18:54:56]

@wtc: I looked into adapting your existing code, however ran into some issues.

For instance PK11_ReadAttribute() is not a public export. If I use
PK11_ReadRawAttribute() then I am back to my version which has to do the gross
memory management for SECItems (whereas your version more cleanly uses an arena
pool for those).

Since this code isn't likely to be long lived (2 Chrome releases is my
expectation) I think I prefer keeping it as C++. Although certainly happy to
replace if you want to do the work for it :)

@padolph: Good question; I have asked wtc since I don't know. Our current
mechanism for importing RSA public keys is to synthesize the DER-encoded key.
Presuming we adapt the same approach for RSA private keys, perhaps then it is
not even possible to import such keys into NSS without those fields specified.
Hmm.

[wtc, 2014-03-19 19:06:49]

On 2014/03/19 18:54:56, eroman wrote:
> @wtc: I looked into adapting your existing code, however ran into some issues.
> 
> For instance PK11_ReadAttribute() is not a public export.

Ah, I missed that. I will figure out a way to test my C code using your Chrome
code. You don't need to do anything.

[eroman, 2014-03-19 22:31:23]

@padolph: Just to close the loop on the JWK side of things...

I spoke with Ryan about this and he educated me. Here is my new
understanding of the situation:

For RSA private keys, JWK requires: n, e, d only.
The p, q, dp, dq, qi properties are optional. (And the "oth" is not
applicable for us since I don't believe NSS supports multiprime).

Given these values, "n", "e", and "d" are sufficient to calculate p and q,
and from that the rest follow easily. This process is described by
http://mxr.mozilla.org/nss/source/lib/freebl/rsa.c#330

Doing the inference directly is unnecessary. The correct way to create the
key is using PK11_CreateGenericObject, and passing it a template containing
values for CKA_MODULUS, CKA_PUBLIC_EXPONENT, and CKA_PRIVATE_EXPONENT. Then
NSS will fill in the rest for us.

(So our current way of importing RSA public keys should be switched over to
that method too).

As you observed, one consequence to consider is that rountrip import/export
through JWK may yield different results. Not only in the JSON attribute
ordering and padding of fields not fixed, but so too may the provided
fields differ. This is not a problem, and as far as I can tell the JOSE
spec does in fact expect this:

Section 6.3.2 says:

"""The others enable optimizations and SHOULD be included by producers of
JWKs representing RSA private key"

So I read that to mean that if the initial JWK import lacked p, q, dp, dq,
qi, it is desired that exporting this key to JWK will now yield a result
that now contains p,q,dp,dq,qi.

OK great. Since JOSE doesn't expect is to preserve the lack of those
fields, we are free to use PKCS#8 as the underlying serialized format.
(PrivateKeyInfo doesn't preserve the lack of those fields).

Hopefully I understood that correctly.

Lastly, an example showing how to import RSA private keys in this manner is:

http://mxr.mozilla.org/nss/source/cmd/rsapoptst/rsapoptst.c#211

If noone else takes it, I'll go ahead and switch over our import of public
keys to that method, and look into the JWK private key import.

Cheers


On Wed, Mar 19, 2014 at 12:06 PM, <wtc@chromium.org> wrote:

> On 2014/03/19 18:54:56, eroman wrote:
>
>> @wtc: I looked into adapting your existing code, however ran into some
>> issues.
>>
>
>  For instance PK11_ReadAttribute() is not a public export.
>>
>
> Ah, I missed that. I will figure out a way to test my C code using your
> Chrome
> code. You don't need to do anything.
>
>
>
> https://codereview.chromium.org/203753009/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[wtc, 2014-03-19 23:49:39]

Review comments on patch set 4:

Just two coding style nits, and a question for Ryan.

https://codereview.chromium.org/203753009/diff/80001/content/child/webcrypto/...
File content/child/webcrypto/platform_crypto_nss.cc (right):

https://codereview.chromium.org/203753009/diff/80001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:595: {0}};

Nit: indent the inners by two, not four.

Nit: put the final closing '}' on a separate line:

  {SEC_ASN1_INTEGER, offsetof(RSAPrivateKey, coefficient)},
  {0}
};

https://codereview.chromium.org/203753009/diff/80001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:829: const SECOidTag algorithm =
SEC_OID_PKCS1_RSA_ENCRYPTION;

Ryan: the NSS function PK11_ExportDERPrivateKeyInfo I wrote also hardcodes
the PrivateKeyInfo.privateKeyAlgorithm to be rsaEncryption
(SEC_OID_PKCS1_RSA_ENCRYPTION) for RSA private keys (keyType == rsaKey).

Do you think I should change PK11_ExportDERPrivateKeyInfo to take a
SECOidTag parameter so that the caller can specify the algorithm?
On the other hand, the NSS KeyType enum has not only rsaKey but also
rsaPssKey and rsaOaepKey, so NSS can figure out which algorithm to use
based on the keyType member of SECKEYPrivateKey alone.

https://codereview.chromium.org/203753009/diff/80001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:859: crypto::ScopedSECItem
encodedKey(

Nit: this variable should be named "encoded_key".

[eroman, 2014-03-19 23:55:17]

https://codereview.chromium.org/203753009/diff/80001/content/child/webcrypto/...
File content/child/webcrypto/platform_crypto_nss.cc (right):

https://codereview.chromium.org/203753009/diff/80001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:595: {0}};
On 2014/03/19 23:49:40, wtc wrote:
> 
> Nit: indent the inners by two, not four.
> 
> Nit: put the final closing '}' on a separate line:
> 
>   {SEC_ASN1_INTEGER, offsetof(RSAPrivateKey, coefficient)},
>   {0}
> };

We have decided to use clang-format exclusively for style decisions in
content/child/webcrypto.

This is what "git cl format" did, so I am inclined to leave it as-is. If it
doesn't match the desired chromium style we should file a bug against
clang-format

https://codereview.chromium.org/203753009/diff/80001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:859: crypto::ScopedSECItem
encodedKey(
On 2014/03/19 23:49:40, wtc wrote:
> 
> Nit: this variable should be named "encoded_key".

Done

[Ryan Sleevi, 2014-03-24 21:23:46]

lgtm

https://codereview.chromium.org/203753009/diff/100001/content/child/webcrypto...
File content/child/webcrypto/platform_crypto_nss.cc (right):

https://codereview.chromium.org/203753009/diff/100001/content/child/webcrypto...
content/child/webcrypto/platform_crypto_nss.cc:597: // On success |value| will
be filled with data which must be freed by
comment nit: "On success " -> "On success, "

[eroman, 2014-03-24 21:54:44]

https://codereview.chromium.org/203753009/diff/100001/content/child/webcrypto...
File content/child/webcrypto/platform_crypto_nss.cc (right):

https://codereview.chromium.org/203753009/diff/100001/content/child/webcrypto...
content/child/webcrypto/platform_crypto_nss.cc:597: // On success |value| will
be filled with data which must be freed by
On 2014/03/24 21:23:46, Ryan Sleevi wrote:
> comment nit: "On success " -> "On success, "

Done.

[eroman, 2014-03-24 21:54:49]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2014-03-24 21:55:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/eroman@chromium.org/203753009/120001

[commit-bot: I haz the power, 2014-03-25 00:46:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-25 00:46:05]

Retried try job too often on win_rel for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[eroman, 2014-03-25 00:51:13]

Committed patchset #6 manually as r259063 (presubmit successful).