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

crypto/ec_private_key_nss.cc

Index: crypto/ec_private_key_nss.cc
diff --git a/crypto/ec_private_key_nss.cc b/crypto/ec_private_key_nss.cc
index 9bb9df124e497ca4a3c6077bbc88575bd6f16729..cea19f5f513550cc4d13c45ef103c7247174ba0b 100644
--- a/crypto/ec_private_key_nss.cc
+++ b/crypto/ec_private_key_nss.cc
@@ -25,10 +25,6 @@ extern "C" {
 
 namespace {
 
-PK11SlotInfo* GetKeySlot() {
-  return crypto::GetPublicNSSKeySlot();
-}
-
 class EllipticCurveSupportChecker {
  public:
   EllipticCurveSupportChecker() {
@@ -37,7 +33,7 @@ class EllipticCurveSupportChecker {
     // 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);
   }
@@ -88,29 +84,30 @@ bool ECPrivateKey::IsSupported() {
 
 // 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.

 }
 
-// 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;
-#endif
+// static
+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,
@@ -118,28 +115,26 @@ ECPrivateKey* ECPrivateKey::CreateFromEncryptedPrivateKeyInfo(
       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
 }
+#endif
 
 // static
 bool ECPrivateKey::ImportFromEncryptedPrivateKeyInfo(
+    ScopedPK11Slot slot,
     const std::string& password,
     const uint8* encrypted_private_key_info,
     size_t encrypted_private_key_info_len,
@@ -148,7 +143,6 @@ bool ECPrivateKey::ImportFromEncryptedPrivateKeyInfo(
     bool sensitive,
     SECKEYPrivateKey** key,
     SECKEYPublicKey** public_key) {
-  ScopedPK11Slot slot(GetKeySlot());
   if (!slot.get())
     return false;
 
@@ -275,13 +269,11 @@ bool ECPrivateKey::ExportECParams(std::vector<uint8>* 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;
 
@@ -323,13 +315,12 @@ ECPrivateKey* ECPrivateKey::CreateWithParams(bool permanent,
 
 // 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 = {
@@ -345,6 +336,7 @@ ECPrivateKey* ECPrivateKey::CreateFromEncryptedPrivateKeyInfoWithParams(
   }
 
   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(),