[webcrypto] Add JWK import of RSA public key for NSS.

content/renderer/webcrypto/webcrypto_impl_nss.cc

Index: content/renderer/webcrypto/webcrypto_impl_nss.cc
diff --git a/content/renderer/webcrypto/webcrypto_impl_nss.cc b/content/renderer/webcrypto/webcrypto_impl_nss.cc
index 23e446bdff7052d5174de891a46e552da145f541..366f9daf88862ff147cc167411f353e61a2b7b73 100644
--- a/content/renderer/webcrypto/webcrypto_impl_nss.cc
+++ b/content/renderer/webcrypto/webcrypto_impl_nss.cc
@@ -239,6 +239,11 @@ bool BigIntegerToLong(const uint8* data,
   return true;
 }
 
+typedef scoped_ptr_malloc<

[Ryan Sleevi, 2013/11/21 01:41:34]

scoped_ptr_malloc is DEPRECATED, as scoped_ptr supports the case of passing
custom deleters now.

See
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/scoped...

[padolph, 2013/11/21 03:38:13]

This typedef is no longer needed because of the change below.

+    PRArenaPool, crypto::NSSDestroyer1<PRArenaPool,
+                                       PORT_FreeArena,
+                                       PR_FALSE> > ScopedPRArenaPool;
+
 }  // namespace
 
 void WebCryptoImpl::Init() {
@@ -649,4 +654,70 @@ bool WebCryptoImpl::VerifySignatureInternal(
   return true;
 }
 
+bool WebCryptoImpl::ImportRsaPublicKeyInternal(

[eroman, 2013/11/20 23:37:12]

@rsleevi, dark lord of NSS: Can you review this function?

+    const unsigned char* modulus_data,
+    unsigned modulus_size,
+    const unsigned char* exponent_data,
+    unsigned exponent_size,
+    const blink::WebCryptoAlgorithm& algorithm,
+    bool extractable,
+    blink::WebCryptoKeyUsageMask usage_mask,
+    blink::WebCryptoKey* key) {
+
+  DCHECK(modulus_data);
+  DCHECK(modulus_size);
+  DCHECK(exponent_data);
+  DCHECK(exponent_size);

[Ryan Sleevi, 2013/11/21 01:41:34]

Is Blink going to require these four fields be valid? I just want to confirm
they're fine as a DCHECK (eg: Blink guarantees it), and not something we should
be doing as actual checks.

[eroman, 2013/11/21 01:55:41]

The caller here is the JWK parsing code, which validates that none of these are
empty. That said it could be more future proof to fail on empty data instead.

[padolph, 2013/11/21 03:38:13]

Done.

+
+  // NSS does not provide a way to create an RSA public key directly from the
+  // modulus and exponent values. But it can import an DER-encoded ASN.1 blob

[Ryan Sleevi, 2013/11/21 01:41:34]

"values. But" -> "values, but"

[padolph, 2013/11/21 03:38:13]

Done.

+  // with the values and create the key from that. The code below follows the
+  // recommendation described in
+  // https://developer.mozilla.org/en-US/docs/NSS/NSS_Tech_Notes/nss_tech_note7
+
+  ScopedPRArenaPool arena(PORT_NewArena(DER_DEFAULT_CHUNKSIZE));
+  if (!arena.get())
+    return false;
+
+  // Pack the input values into struct compatible with NSS ASN.1 encoding, and
+  // set up the ASN.1 encoder template for it.
+  struct RsaPublicKeyData {
+    SECItem modulus;
+    SECItem exponent;
+  };
+  const RsaPublicKeyData pubkey_in = {
+      {siUnsignedInteger, const_cast<unsigned char*>(modulus_data),
+       modulus_size},
+      {siUnsignedInteger, const_cast<unsigned char*>(exponent_data),
+       exponent_size}};
+  const SEC_ASN1Template RsaPublicKeyTemplate[] = {
+      {SEC_ASN1_SEQUENCE, 0, NULL, sizeof(RsaPublicKeyData)},
+      {SEC_ASN1_INTEGER, offsetof(RsaPublicKeyData, modulus), },
+      {SEC_ASN1_INTEGER, offsetof(RsaPublicKeyData, exponent), },
+      {0, }};

[Ryan Sleevi, 2013/11/21 01:41:34]

nit: I seem to recall that clang-format/the style guide expects spaces following
the leading {

[padolph, 2013/11/21 03:38:13]

The current format is what clang-format -style=Chromium produced. I could not
find info either way in the Chromium style guide. Should I change this?

+
+  // Do the ASN.1 encoding to produce the DER-formatted public key. In this
+  // usage, SEC_ASN1EncodeItem() returns a pointer to the provided output
+  // SECItem (pubkey_der) on success, or NULL on failure.

[Ryan Sleevi, 2013/11/21 01:41:34]

A subtle element of this is the fact that the SECItem is allocated from the
PRArenaPool.

// DER-encode the public key. The returned SECItem* is allocated from
// the specified arena, and thus does not need to be explicitly freed.


I'm not a fan of documenting the function being called (eg: "on success, ... or
NULL of failure". Also, the "Do the ASN.1 encoding" is a bit redundant with the
function call; comments that start with "Do" can usually be reworded like I
tried.

That said, I don't understand why you manually create the arena. The
SEC_ASN1EncodeItem API permits you to supply NULL for the pool, and use the
'easier' APIs like ScopedSECItem - see
http://mxr.mozilla.org/nss/source/lib/util/secasn1.h#118

[padolph, 2013/11/21 03:38:13]

Done.
 
Copy/paste from the NSS tech note. Using the NULL calling method does make
things simpler. Thank you.

+  SECItem pubkey_der = {siBuffer, NULL, 0};
+  SECItem* const check_der_ptr = SEC_ASN1EncodeItem(
+      arena.get(), &pubkey_der, &pubkey_in, RsaPublicKeyTemplate);
+  if (!check_der_ptr)
+    return false;
+  DCHECK_EQ(&pubkey_der, check_der_ptr);
+
+  // Import the DER to create an RSA SECKEYPublicKey.

[Ryan Sleevi, 2013/11/21 01:41:34]

nit: // Import the DER-encoded public key to...

[padolph, 2013/11/21 03:38:13]

Done.

+  crypto::ScopedSECKEYPublicKey pubkey(
+      SECKEY_ImportDERPublicKey(&pubkey_der, CKK_RSA));
+  if (!pubkey.get())
+    return false;
+
+  *key = blink::WebCryptoKey::create(new PublicKeyHandle(pubkey.Pass()),
+                                     blink::WebCryptoKeyTypePublic,
+                                     extractable,
+                                     algorithm,
+                                     usage_mask);
+  return true;
+}
+
 }  // namespace content