[webcrypto] Fix a bug with JWK private key import.

content/child/webcrypto/platform_crypto_nss.cc

Index: content/child/webcrypto/platform_crypto_nss.cc
diff --git a/content/child/webcrypto/platform_crypto_nss.cc b/content/child/webcrypto/platform_crypto_nss.cc
index 3d0e9923a69f4185b2bdb2e4919fa2ff61e25204..80fa2663c73e1ca2d0d05ece371b9d65d0be9492 100644
--- a/content/child/webcrypto/platform_crypto_nss.cc
+++ b/content/child/webcrypto/platform_crypto_nss.cc
@@ -18,6 +18,7 @@
 #include "content/child/webcrypto/status.h"
 #include "content/child/webcrypto/webcrypto_util.h"
 #include "crypto/nss_util.h"
+#include "crypto/random.h"
 #include "crypto/scoped_nss_types.h"
 #include "third_party/WebKit/public/platform/WebCryptoAlgorithm.h"
 #include "third_party/WebKit/public/platform/WebCryptoAlgorithmParams.h"
@@ -1717,6 +1718,39 @@ Status ImportRsaPrivateKey(const blink::WebCryptoAlgorithm& algorithm,
   AddOptionalAttribute(CKA_PUBLIC_EXPONENT, public_exponent, &key_template);
   AddOptionalAttribute(CKA_PRIVATE_EXPONENT, private_exponent, &key_template);
 
+  // Manufacture a CKA_ID so the created key can be retrieved later as a
+  // SECKEYPrivateKey using FindKeyByKeyID(). Unfortunately there isn't a more
+  // direct way to do this in NSS.
+  //
+  // A randomly generated value is used for the ID.
+  //
+  // NSS assigns the CKA_ID to PK11_MakeIDFromPubKey() for the other key
+  // creation paths. However it is incorrect to try and use that as the CKA_ID
+  // here because:
+  //
+  //  (1) CreateGenericObject does not validate its parameters. It is therefore
+  //      possible to construct a key using the known public modulus, and all
+  //      the other parameters are bogus. FindKeyByKeyID() returns the first
+  //      key matching the ID. So this would effectively allow an attacker to
+  //      retrieve a private key of their choice.
+  //      NSS 3.16.2 does validate parameters.
+  //
+  //  (2) The ID space is shared by different key types. So even if parameters
+  //      were validated by NSS, FindKeyByKeyID() could still return the wrong
+  //      key. For instance it might return a DH key that happens to have same
+  //      values for its public data.
+  //      Ryan says this isn't possible in practice, because matching values for

[Ryan Sleevi, 2014/06/02 23:55:57]

We don't do this in comments :)


But I'm glad Ryan Hamilton approves ;)


// This is not an issue in practice, since it requires generating a valid RSA
public key that is also a valid DH/EC key.

+  //      valid keys can't be easily guessed.
+  //
+  //  (3) Even if the above two problems were non-issues, there is no guarantee
+  //      that FindKeyByKeyID() returns the object just created. If the same
+  //      private key were imported multiple times, with different flags, using
+  //      the wrong one could lead to problems.
+  std::vector<uint8> cka_id(20);
+  crypto::RandBytes(&cka_id[0], cka_id.size());
+  AddOptionalAttribute(CKA_ID, CryptoData(cka_id), &key_template);
+  SECItem object_id = MakeSECItemForBuffer(CryptoData(cka_id));
+
   // Optional properties (all of these will have been specified or none).
   AddOptionalAttribute(CKA_PRIME_1, prime1, &key_template);
   AddOptionalAttribute(CKA_PRIME_2, prime2, &key_template);
@@ -1732,18 +1766,9 @@ Status ImportRsaPrivateKey(const blink::WebCryptoAlgorithm& algorithm,
   if (!key_object)
     return Status::OperationError();
 
-  // The ID isn't guaranteed to be set by PKCS#11. However it is by softtoken so
-  // this should work.
-  SECItem object_id = {};
-  if (PK11_ReadRawAttribute(
-          PK11_TypeGeneric, key_object.get(), CKA_ID, &object_id) != SECSuccess)
-    return Status::OperationError();
-
   crypto::ScopedSECKEYPrivateKey private_key(
       PK11_FindKeyByKeyID(slot.get(), &object_id, NULL));
 
-  SECITEM_FreeItem(&object_id, PR_FALSE);
-
   if (!private_key)
     return Status::OperationError();