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

content/child/webcrypto/platform_crypto_nss.cc

 // Copyright 2014 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/child/webcrypto/platform_crypto.h"
 
 #include <cryptohi.h>
 #include <pk11pub.h>
 #include <secerr.h>
 #include <sechash.h>
 
 #include <vector>
 
 #include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "content/child/webcrypto/crypto_data.h"
 #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"
 #include "third_party/WebKit/public/platform/WebCryptoKeyAlgorithm.h"
 
 #if defined(USE_NSS)
 #include <dlfcn.h>
 #include <secoid.h>
 #endif
 
@@ skipping 1679 matching lines @@
   AddAttribute(CKA_KEY_TYPE, &key_type, sizeof(key_type), &key_template);
   AddAttribute(CKA_TOKEN, &ck_false, sizeof(ck_false), &key_template);
   AddAttribute(CKA_SENSITIVE, &ck_false, sizeof(ck_false), &key_template);
   AddAttribute(CKA_PRIVATE, &ck_false, sizeof(ck_false), &key_template);
 
   // Required properties.
   AddOptionalAttribute(CKA_MODULUS, modulus, &key_template);
   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);
   AddOptionalAttribute(CKA_EXPONENT_1, exponent1, &key_template);
   AddOptionalAttribute(CKA_EXPONENT_2, exponent2, &key_template);
   AddOptionalAttribute(CKA_COEFFICIENT, coefficient, &key_template);
 
   crypto::ScopedPK11Slot slot(PK11_GetInternalSlot());
 
   ScopedPK11GenericObject key_object(PK11_CreateGenericObject(
       slot.get(), &key_template[0], key_template.size(), PR_FALSE));
 
   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();
 
   blink::WebCryptoKeyAlgorithm key_algorithm;
   if (!CreatePrivateKeyAlgorithm(algorithm, private_key.get(), &key_algorithm))
     return Status::ErrorUnexpected();
 
   scoped_ptr<PrivateKey> key_handle;
   Status status =
       PrivateKey::Create(private_key.Pass(), key_algorithm, &key_handle);
@@ skipping 106 matching lines @@
   buffer->assign(key_data->data, key_data->data + key_data->len);
 
   return Status::Success();
 }
 
 }  // namespace platform
 
 }  // namespace webcrypto
 
 }  // namespace content