[webcrypto] Add RSA public key SPKI import/export for NSS.

content/renderer/webcrypto/webcrypto_impl_nss.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/renderer/webcrypto/webcrypto_impl.h"
 
 #include <cryptohi.h>
 #include <pk11pub.h>
 #include <sechash.h>
 
+#include <algorithm>
 #include <vector>
 
 #include "base/logging.h"
 #include "crypto/nss_util.h"
 #include "crypto/scoped_nss_types.h"
 #include "crypto/secure_util.h"
 #include "third_party/WebKit/public/platform/WebArrayBuffer.h"
 #include "third_party/WebKit/public/platform/WebCryptoAlgorithm.h"
 #include "third_party/WebKit/public/platform/WebCryptoAlgorithmParams.h"
 
@@ skipping 210 matching lines @@
     size_t reverse_i = data_size - i - 1;
 
     if (reverse_i >= sizeof(unsigned long) && data[i])
       return false;  // Too large for a long.
 
     *result |= data[i] << 8 * reverse_i;
   }
   return true;
 }
 
+bool IsAlgorithmRsa(const WebKit::WebCryptoAlgorithm& algorithm) {
+  return algorithm.id() == WebKit::WebCryptoAlgorithmIdRsaEsPkcs1v1_5 ||
+         algorithm.id() == WebKit::WebCryptoAlgorithmIdRsaOaep ||
+         algorithm.id() == WebKit::WebCryptoAlgorithmIdRsaSsaPkcs1v1_5;
+}
+
+bool ImportKeyInternalRaw(
+    const unsigned char* key_data,
+    unsigned key_data_size,
+    const WebKit::WebCryptoAlgorithm& algorithm_or_null,
+    bool extractable,
+    WebKit::WebCryptoKeyUsageMask usage_mask,
+    WebKit::WebCryptoKey* key) {
+
+  // TODO(eroman): Currently expects algorithm to always be specified, as it is

[eroman, 2013/11/06 23:48:40]

Not sure this needs a TODO anymore, since for "raw" format we need to have the
algorithm. I propose deleting this TODO. You could even rename the parameter
"algorithm" and do the nullity check in the caller.

[padolph, 2013/11/07 00:23:50]

Done.

+  //               required for raw format.
+  if (algorithm_or_null.isNull())
+    return false;
+  const WebKit::WebCryptoAlgorithm& algorithm = algorithm_or_null;
+
+  WebKit::WebCryptoKeyType type;
+  switch (algorithm.id()) {
+    case WebKit::WebCryptoAlgorithmIdHmac:
+    case WebKit::WebCryptoAlgorithmIdAesCbc:
+      type = WebKit::WebCryptoKeyTypeSecret;
+      break;
+    // TODO(bryaneyler): Support more key types.
+    default:
+      return false;
+  }
+
+  // TODO(bryaneyler): Need to split handling for symmetric and asymmetric keys.
+  // Currently only supporting symmetric.

[eroman, 2013/11/06 23:48:40]

I presume nothing else changed in this function? (I will do a verification pass
before final approval)

[padolph, 2013/11/07 00:23:50]

Yes, I made sure this was a strict copy-paste (aside from the function rename).
But there will be a minor change now from your comment above.

+  CK_MECHANISM_TYPE mechanism = CKM_INVALID_MECHANISM;
+  // Flags are verified at the Blink layer; here the flags are set to all
+  // possible operations for this key type.
+  CK_FLAGS flags = 0;
+
+  switch (algorithm.id()) {
+    case WebKit::WebCryptoAlgorithmIdHmac: {
+      const WebKit::WebCryptoHmacParams* params = algorithm.hmacParams();
+      if (!params) {
+        return false;
+      }
+
+      mechanism = WebCryptoAlgorithmToHMACMechanism(params->hash());
+      if (mechanism == CKM_INVALID_MECHANISM) {
+        return false;
+      }
+
+      flags |= CKF_SIGN | CKF_VERIFY;
+
+      break;
+    }
+    case WebKit::WebCryptoAlgorithmIdAesCbc: {
+      mechanism = CKM_AES_CBC;
+      flags |= CKF_ENCRYPT | CKF_DECRYPT;
+      break;
+    }
+    default:
+      return false;
+  }
+
+  DCHECK_NE(CKM_INVALID_MECHANISM, mechanism);
+  DCHECK_NE(0ul, flags);
+
+  SECItem key_item = {
+      siBuffer,
+      const_cast<unsigned char*>(key_data),
+      key_data_size
+  };
+
+  crypto::ScopedPK11SymKey pk11_sym_key(
+      PK11_ImportSymKeyWithFlags(PK11_GetInternalSlot(),
+                                 mechanism,
+                                 PK11_OriginUnwrap,
+                                 CKA_FLAGS_ONLY,
+                                 &key_item,
+                                 flags,
+                                 false,
+                                 NULL));
+  if (!pk11_sym_key.get()) {
+    return false;
+  }
+
+  *key = WebKit::WebCryptoKey::create(new SymKeyHandle(pk11_sym_key.Pass()),
+                                      type, extractable, algorithm, usage_mask);
+  return true;
+}
+
+typedef scoped_ptr_malloc<
+    CERTSubjectPublicKeyInfo,
+    crypto::NSSDestroyer<CERTSubjectPublicKeyInfo,
+                         SECKEY_DestroySubjectPublicKeyInfo> >
+    ScopedCERTSubjectPublicKeyInfo;
+
+bool ImportKeyInternalSpki(
+    const unsigned char* key_data,
+    unsigned key_data_size,
+    const WebKit::WebCryptoAlgorithm& algorithm_or_null,
+    bool extractable,
+    WebKit::WebCryptoKeyUsageMask usage_mask,
+    WebKit::WebCryptoKey* key) {
+
+  DCHECK(key_data);
+  DCHECK(key_data_size);

[eroman, 2013/11/06 23:48:40]

These shouldn't be DCHECKs --> it is possible for Blink to feed an empty buffer
for the key data.

[padolph, 2013/11/07 00:23:50]

Done.

+  DCHECK(key);
+
+  const WebKit::WebCryptoAlgorithm& algorithm = algorithm_or_null;

[eroman, 2013/11/06 23:48:40]

The same thing could be accomplished by just renaming the parameter. Please
reconsider how to handle this: either leave the parameter named
algorithm_or_null, or do the nullity checking in the caller.

[padolph, 2013/11/07 00:23:50]

Can't do it in the caller, since fail for null might only apply to RSA. I
changed the logic to make this clearer, please take a look.

+
+  // The binary blob 'key_data' is expected to be a DER-encoded ASN.1 Subject
+  // Public Key Info. Decode this to a CERTSubjectPublicKeyInfo.
+  SECItem spki_item = {
+      siBuffer,
+      const_cast<uint8*>(key_data),
+      key_data_size
+  };
+  const ScopedCERTSubjectPublicKeyInfo spki(
+      SECKEY_DecodeDERSubjectPublicKeyInfo(&spki_item));
+  if (!spki)
+    return false;
+
+  crypto::ScopedSECKEYPublicKey sec_public_key(
+      SECKEY_ExtractPublicKey(spki.get()));
+  if (!sec_public_key)
+    return false;
+
+  // Validate the key type.
+  const KeyType sec_key_type = SECKEY_GetPublicKeyType(sec_public_key.get());

[eroman, 2013/11/06 23:48:40]

FYI: I am not familiar with these specifics; I will be asking Sleevi to review
the NSS crypto particulars of this CL later.

+  switch (sec_key_type) {
+    case rsaKey:
+      // NSS's rsaKey KeyType maps to keys with SEC_OID_PKCS1_RSA_ENCRYPTION and
+      // according to RFC 4055 this can be used for both encryption and
+      // signatures. However, this is not specific enough to build a compatible
+      // Web Crypto algorithm, since in Web Crypto RSA encryption and signature
+      // algorithms are distinct. So if the input algorithm is NULL here, we

[eroman, 2013/11/06 23:48:40]

Useful comment. Small wording nit: rather than "NULL" perhaps refer to this as
"null" or "isNull()".

[padolph, 2013/11/07 00:23:50]

Done.

+      // have to fail.
+      if (algorithm.isNull() || !IsAlgorithmRsa(algorithm))
+        return false;
+      break;
+    case dsaKey:
+    case ecKey:
+    case rsaPssKey:
+    case rsaOaepKey:
+      // TODO(padolph): Handle other key types
+      return false;
+    default:
+      NOTREACHED();

[eroman, 2013/11/06 23:48:40]

Only use NOTREACHED() for code which really can't be reached unless there is a
bug in the code.

[padolph, 2013/11/07 00:23:50]

Done.

+      return false;
+  }
+
+  *key = WebKit::WebCryptoKey::create(
+      new PublicKeyHandle(sec_public_key.Pass()),
+      WebKit::WebCryptoKeyTypePublic,
+      extractable,
+      algorithm,
+      usage_mask);
+
+  return true;
+}
+
+bool ExportKeyInternalSpki(
+    const WebKit::WebCryptoKey& key,
+    WebKit::WebArrayBuffer* buffer) {
+
+  DCHECK(key.handle());
+  DCHECK(buffer);
+
+  if (key.type() != WebKit::WebCryptoKeyTypePublic || !key.extractable())
+    return false;
+
+  PublicKeyHandle* const pub_key =
+      reinterpret_cast<PublicKeyHandle*>(key.handle());
+
+  const crypto::ScopedSECItem spki_der(
+      SECKEY_EncodeDERSubjectPublicKeyInfo(pub_key->key()));
+  if (!spki_der)
+    return false;
+
+  DCHECK(spki_der->data);
+  DCHECK(spki_der->len);
+
+  *buffer = WebKit::WebArrayBuffer::create(spki_der->len, 1);
+  std::copy(spki_der->data,
+            spki_der->data + spki_der->len,
+            static_cast<unsigned char*>(buffer->data()));
+
+  return true;
+}
+
 }  // namespace
 
 void WebCryptoImpl::Init() {
   crypto::EnsureNSSInit();
 }
 
 bool WebCryptoImpl::EncryptInternal(
     const WebKit::WebCryptoAlgorithm& algorithm,
     const WebKit::WebCryptoKey& key,
     const unsigned char* data,
@@ skipping 186 matching lines @@
 
       // One extractable input parameter is provided, and the Web Crypto API
       // spec at this time says it applies to both members of the key pair.
       // This is probably not correct: it makes more operational sense to have
       // extractable apply only to the private key and make the public key
       // always extractable. For now implement what the spec says and track the
       // spec bug here: https://www.w3.org/Bugs/Public/show_bug.cgi?id=23695
       *public_key = WebKit::WebCryptoKey::create(
           new PublicKeyHandle(crypto::ScopedSECKEYPublicKey(sec_public_key)),
           WebKit::WebCryptoKeyTypePublic,
-          extractable,   // probably should be 'true' always
+          extractable,  // probably should be 'true' always
           algorithm,
           usage_mask);
       *private_key = WebKit::WebCryptoKey::create(
           new PrivateKeyHandle(scoped_sec_private_key.Pass()),
           WebKit::WebCryptoKeyTypePrivate,
           extractable,
           algorithm,
           usage_mask);
 
       return true;
     }
     default:
       return false;
   }
 }
 
 bool WebCryptoImpl::ImportKeyInternal(
     WebKit::WebCryptoKeyFormat format,
     const unsigned char* key_data,
     unsigned key_data_size,
     const WebKit::WebCryptoAlgorithm& algorithm_or_null,
     bool extractable,
     WebKit::WebCryptoKeyUsageMask usage_mask,
     WebKit::WebCryptoKey* key) {
-  // TODO(eroman): Currently expects algorithm to always be specified, as it is
-  //               required for raw format.
-  if (algorithm_or_null.isNull())
-    return false;
-  const WebKit::WebCryptoAlgorithm& algorithm = algorithm_or_null;
-
-  WebKit::WebCryptoKeyType type;
-  switch (algorithm.id()) {
-    case WebKit::WebCryptoAlgorithmIdHmac:
-    case WebKit::WebCryptoAlgorithmIdAesCbc:
-      type = WebKit::WebCryptoKeyTypeSecret;
-      break;
-    // TODO(bryaneyler): Support more key types.
-    default:
-      return false;
-  }
-
-  // TODO(bryaneyler): Need to split handling for symmetric and asymmetric keys.
-  // Currently only supporting symmetric.
-  CK_MECHANISM_TYPE mechanism = CKM_INVALID_MECHANISM;
-  // Flags are verified at the Blink layer; here the flags are set to all
-  // possible operations for this key type.
-  CK_FLAGS flags = 0;
-
-  switch(algorithm.id()) {
-    case WebKit::WebCryptoAlgorithmIdHmac: {
-      const WebKit::WebCryptoHmacParams* params = algorithm.hmacParams();
-      if (!params) {
-        return false;
-      }
-
-      mechanism = WebCryptoAlgorithmToHMACMechanism(params->hash());
-      if (mechanism == CKM_INVALID_MECHANISM) {
-        return false;
-      }
-
-      flags |= CKF_SIGN | CKF_VERIFY;
-
-      break;
-    }
-    case WebKit::WebCryptoAlgorithmIdAesCbc: {
-      mechanism = CKM_AES_CBC;
-      flags |= CKF_ENCRYPT | CKF_DECRYPT;
-      break;
-    }
-    default:
-      return false;
-  }
-
-  DCHECK_NE(CKM_INVALID_MECHANISM, mechanism);
-  DCHECK_NE(0ul, flags);
-
-  SECItem key_item = { siBuffer, NULL, 0 };
 
   switch (format) {
     case WebKit::WebCryptoKeyFormatRaw:
-      key_item.data = const_cast<unsigned char*>(key_data);
-      key_item.len = key_data_size;
-      break;
-    // TODO(bryaneyler): Handle additional formats.
+      return ImportKeyInternalRaw(key_data,
+                                  key_data_size,
+                                  algorithm_or_null,
+                                  extractable,
+                                  usage_mask,
+                                  key);
+    case WebKit::WebCryptoKeyFormatSpki:
+      return ImportKeyInternalSpki(key_data,
+                                   key_data_size,
+                                   algorithm_or_null,
+                                   extractable,
+                                   usage_mask,
+                                   key);
+    case WebKit::WebCryptoKeyFormatPkcs8:
+      // TODO(padolph): Handle PKCS#8 private key import
+      return false;
     default:
+      NOTREACHED();

[eroman, 2013/11/06 23:48:40]

Please leave this off since it is reachable (i.e. if javascript requests JWK
import). Don't want that to trigger an assertion in debug builds.

[padolph, 2013/11/07 00:23:50]

Done.

       return false;
   }
+}
 
-  crypto::ScopedPK11SymKey pk11_sym_key(
-      PK11_ImportSymKeyWithFlags(PK11_GetInternalSlot(),
-                                 mechanism,
-                                 PK11_OriginUnwrap,
-                                 CKA_FLAGS_ONLY,
-                                 &key_item,
-                                 flags,
-                                 false,
-                                 NULL));
-  if (!pk11_sym_key.get()) {
-    return false;
+bool WebCryptoImpl::ExportKeyInternal(
+    WebKit::WebCryptoKeyFormat format,
+    const WebKit::WebCryptoKey& key,
+    WebKit::WebArrayBuffer* buffer) {
+  switch (format) {
+    case WebKit::WebCryptoKeyFormatRaw:
+      // TODO(padolph): Implement raw export
+      NOTREACHED();

[eroman, 2013/11/06 23:48:40]

Same comment here. Use NOTREACHED() only for code which is logically impossible
to reach somewhere. Since these codepaths controlled are controlled by user
inputs we just return false.

[padolph, 2013/11/07 00:23:50]

Done.

+      return false;
+    case WebKit::WebCryptoKeyFormatSpki:
+      return ExportKeyInternalSpki(key, buffer);
+    case WebKit::WebCryptoKeyFormatPkcs8:
+      // TODO(padolph): Implement pkcs8 export
+      NOTREACHED();
+      return false;
+    default:
+      NOTREACHED();
+      return false;
   }
-
-  *key = WebKit::WebCryptoKey::create(new SymKeyHandle(pk11_sym_key.Pass()),
-                                      type, extractable, algorithm, usage_mask);
-  return true;
 }
 
 bool WebCryptoImpl::SignInternal(
     const WebKit::WebCryptoAlgorithm& algorithm,
     const WebKit::WebCryptoKey& key,
     const unsigned char* data,
     unsigned data_size,
     WebKit::WebArrayBuffer* buffer) {
   WebKit::WebArrayBuffer result;
 
@@ skipping 80 matching lines @@
       break;
     }
     default:
       return false;
   }
 
   return true;
 }
 
 }  // namespace content