NSS: {EC,RSA}PrivateKey shouldn't call crypto::GetPublicNSSKeySlot or GetPrivateNSSKeySlot.

crypto/ec_private_key_nss.cc

 // Copyright (c) 2012 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 "crypto/ec_private_key.h"
 
 extern "C" {
 // Work around NSS missing SEC_BEGIN_PROTOS in secmodt.h.  This must come before
 // other NSS headers.
 #include <secmodt.h>
 }
 
 #include <cryptohi.h>
 #include <keyhi.h>
 #include <pk11pub.h>
 #include <secmod.h>
 
 #include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "crypto/nss_util.h"
 #include "crypto/nss_util_internal.h"
 #include "crypto/scoped_nss_types.h"
 #include "crypto/third_party/nss/chromium-nss.h"
 
 namespace {
 
-PK11SlotInfo* GetKeySlot() {
-  return crypto::GetPublicNSSKeySlot();
-}
-
 class EllipticCurveSupportChecker {
  public:
   EllipticCurveSupportChecker() {
     // NOTE: we can do this check here only because we use the NSS internal
     // slot.  If we support other slots in the future, checking whether they
     // support ECDSA may block NSS, and the value may also change as devices are
     // inserted/removed, so we would need to re-check on every use.
     crypto::EnsureNSSInit();
-    crypto::ScopedPK11Slot slot(GetKeySlot());
+    crypto::ScopedPK11Slot slot(PK11_GetInternalKeySlot());

[wtc, 2013/11/11 20:56:25]

It may be a good idea to keep the GetKeySlot() function, so that we know the
slot we test for PKCS #11 mechanism support here will be the slot we use in the
rest of this file.

We can change GetKeySlot() to return PK11_GetInternalKeySlot() if
crypto::GetPublicNSSKeySlot() is not appropriate.

[mattm, 2013/11/12 02:42:44]

Done.

     supported_ = PK11_DoesMechanism(slot.get(), CKM_EC_KEY_PAIR_GEN) &&
         PK11_DoesMechanism(slot.get(), CKM_ECDSA);
   }
 
   bool Supported() {
     return supported_;
   }
 
  private:
   bool supported_;
@@ skipping 30 matching lines @@
     SECKEY_DestroyPublicKey(public_key_);
 }
 
 // static
 bool ECPrivateKey::IsSupported() {
   return g_elliptic_curve_supported.Get().Supported();
 }
 
 // static
 ECPrivateKey* ECPrivateKey::Create() {
-  return CreateWithParams(PR_FALSE /* not permanent */,
+  EnsureNSSInit();
+
+  return CreateWithParams(ScopedPK11Slot(PK11_GetInternalKeySlot()),

[wtc, 2013/11/11 20:56:25]

IMPORTANT: I think this should be PK11_GetInternalSlot().

I explained this issue in rsa_private_key_nss.cc. But I am less sure in this
file because the original code in this file uses crypto::GetPublicNSSKeySlot(),
which is defined as:

  PK11SlotInfo* GetPublicNSSKeySlot() {
    if (test_slot_)
      return PK11_ReferenceSlot(test_slot_);
    if (software_slot_)
      return PK11_ReferenceSlot(software_slot_);
    return PK11_GetInternalKeySlot();
  }

So PK11_GetInternalKeySlot() may be more appropriate in this file.

Also, why don't we need to the overrides by test_slot_ and software_slot_?

[mattm, 2013/11/12 02:42:44]

Done.
I believe that for temporary keys, it shouldn't matter which slot is used.

+                          PR_FALSE /* not permanent */,
                           PR_FALSE /* not sensitive */);

[wtc, 2013/11/11 20:56:25]

Nit: these should be 'false' instead of 'PR_FALSE' because they are of the C++
bool type.

[mattm, 2013/11/12 02:42:44]

Done.

 }
 
+#if defined(USE_NSS)
 // static
-ECPrivateKey* ECPrivateKey::CreateSensitive() {
-#if defined(USE_NSS)
-  return CreateWithParams(PR_TRUE /* permanent */,
-                          PR_TRUE /* sensitive */);
-#else
-  // If USE_NSS is not defined, we initialize NSS with no databases, so we can't
-  // create permanent keys.
-  NOTREACHED();
-  return NULL;
+ECPrivateKey* ECPrivateKey::CreateSensitive(ScopedPK11Slot slot) {
+  return CreateWithParams(
+      slot.Pass(), PR_TRUE /* permanent */, PR_TRUE /* sensitive */);

[wtc, 2013/11/11 20:56:25]

Nit: these should be 'true' instead of 'PR_TRUE'.

[mattm, 2013/11/12 02:42:44]

Done.

+}
 #endif
-}
 
 // static
 ECPrivateKey* ECPrivateKey::CreateFromEncryptedPrivateKeyInfo(
     const std::string& password,
     const std::vector<uint8>& encrypted_private_key_info,
     const std::vector<uint8>& subject_public_key_info) {
+  EnsureNSSInit();
+
   return CreateFromEncryptedPrivateKeyInfoWithParams(
+      ScopedPK11Slot(PK11_GetInternalKeySlot()),

[wtc, 2013/11/11 20:56:25]

IMPORTANT: I think this should be PK11_GetInternalSlot().

[mattm, 2013/11/12 02:42:44]

Done.

       password,
       encrypted_private_key_info,
       subject_public_key_info,
       PR_FALSE /* not permanent */,
       PR_FALSE /* not sensitive */);

[wtc, 2013/11/11 20:56:25]

Nit: these should be 'false' instead of 'PR_FALSE'.

[mattm, 2013/11/12 02:42:44]

Done.

 }
 
+#if defined(USE_NSS)
 // static
 ECPrivateKey* ECPrivateKey::CreateSensitiveFromEncryptedPrivateKeyInfo(
+    ScopedPK11Slot slot,
     const std::string& password,
     const std::vector<uint8>& encrypted_private_key_info,
     const std::vector<uint8>& subject_public_key_info) {
-#if defined(USE_NSS)
   return CreateFromEncryptedPrivateKeyInfoWithParams(
+      slot.Pass(),
       password,
       encrypted_private_key_info,
       subject_public_key_info,
       PR_TRUE /* permanent */,
       PR_TRUE /* sensitive */);

[wtc, 2013/11/11 20:56:25]

Nit: these should be 'true' instead of 'PR_TRUE'.

[mattm, 2013/11/12 02:42:44]

Done.

-#else
-  // If USE_NSS is not defined, we initialize NSS with no databases, so we can't
-  // create permanent keys.
-  NOTREACHED();
-  return NULL;
+}
 #endif
-}
 
 // static
 bool ECPrivateKey::ImportFromEncryptedPrivateKeyInfo(
+    ScopedPK11Slot slot,
     const std::string& password,
     const uint8* encrypted_private_key_info,
     size_t encrypted_private_key_info_len,
     CERTSubjectPublicKeyInfo* decoded_spki,
     bool permanent,
     bool sensitive,
     SECKEYPrivateKey** key,
     SECKEYPublicKey** public_key) {
-  ScopedPK11Slot slot(GetKeySlot());
   if (!slot.get())
     return false;
 
   *public_key = SECKEY_ExtractPublicKey(decoded_spki);
 
   if (!*public_key) {
     DLOG(ERROR) << "SECKEY_ExtractPublicKey: " << PORT_GetError();
     return false;
   }
 
@@ skipping 106 matching lines @@
   return ReadAttribute(key_, CKA_VALUE, output);
 }
 
 bool ECPrivateKey::ExportECParams(std::vector<uint8>* output) {
   return ReadAttribute(key_, CKA_EC_PARAMS, output);
 }
 
 ECPrivateKey::ECPrivateKey() : key_(NULL), public_key_(NULL) {}
 
 // static
-ECPrivateKey* ECPrivateKey::CreateWithParams(bool permanent,
+ECPrivateKey* ECPrivateKey::CreateWithParams(ScopedPK11Slot slot,
+                                             bool permanent,
                                              bool sensitive) {
-  EnsureNSSInit();
-
   scoped_ptr<ECPrivateKey> result(new ECPrivateKey);
 
-  ScopedPK11Slot slot(GetKeySlot());
   if (!slot.get())
     return NULL;
 
   SECOidData* oid_data = SECOID_FindOIDByTag(SEC_OID_SECG_EC_SECP256R1);
   if (!oid_data) {
     DLOG(ERROR) << "SECOID_FindOIDByTag: " << PORT_GetError();
     return NULL;
   }
 
   // SECKEYECParams is a SECItem containing the DER encoded ASN.1 ECParameters
@@ skipping 21 matching lines @@
   if (!result->key_) {
     DLOG(ERROR) << "PK11_GenerateKeyPair: " << PORT_GetError();
     return NULL;
   }
 
   return result.release();
 }
 
 // static
 ECPrivateKey* ECPrivateKey::CreateFromEncryptedPrivateKeyInfoWithParams(
+    ScopedPK11Slot slot,
     const std::string& password,
     const std::vector<uint8>& encrypted_private_key_info,
     const std::vector<uint8>& subject_public_key_info,
     bool permanent,
     bool sensitive) {
-  EnsureNSSInit();
-
   scoped_ptr<ECPrivateKey> result(new ECPrivateKey);
 
   SECItem encoded_spki = {
     siBuffer,
     const_cast<unsigned char*>(&subject_public_key_info[0]),
     static_cast<unsigned>(subject_public_key_info.size())
   };
   CERTSubjectPublicKeyInfo* decoded_spki = SECKEY_DecodeDERSubjectPublicKeyInfo(
       &encoded_spki);
   if (!decoded_spki) {
     DLOG(ERROR) << "SECKEY_DecodeDERSubjectPublicKeyInfo: " << PORT_GetError();
     return NULL;
   }
 
   bool success = ECPrivateKey::ImportFromEncryptedPrivateKeyInfo(

[wtc, 2013/11/11 22:18:54]

Could you take the opportunity to remove "ECPrivateKey::" from this line? It is
not necessary because we are inside a ECPrivateKey method.

[mattm, 2013/11/12 02:42:44]

Done.

+      slot.Pass(),
       password,
       &encrypted_private_key_info[0],
       encrypted_private_key_info.size(),
       decoded_spki,
       permanent,
       sensitive,
       &result->key_,
       &result->public_key_);
 
   SECKEY_DestroySubjectPublicKeyInfo(decoded_spki);
 
   if (success)
     return result.release();
 
   return NULL;
 }
 
 }  // namespace crypto