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

Make ECPrivateKey use PK11_GetInternalKeySlot for temporary keys.
Make ECPrivateKey and RSAPrivateKey "sensitive" functions take slot as parameter.

This avoids calling non-thread-safe functions in nss_util on arbitrary threads.

Also removes the ANNOTATE_SCOPED_MEMORY_LEAK from RSAPrivateKey which should no longer be necessary.

BUG=125848,34742

Review URL: https://codereview.chromium.org/66213002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@234726 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 103 insertions, 113 deletions

diff --git a/crypto/ec_private_key.h b/crypto/ec_private_key.h
index d3f5b73..2870163 100644
--- a/crypto/ec_private_key.h
+++ b/crypto/ec_private_key.h
@@ -18,6 +18,7 @@ typedef struct evp_pkey_st EVP_PKEY;
 #else
 // Forward declaration.
 typedef struct CERTSubjectPublicKeyInfoStr CERTSubjectPublicKeyInfo;
+typedef struct PK11SlotInfoStr PK11SlotInfo;
 typedef struct SECKEYPrivateKeyStr SECKEYPrivateKey;
 typedef struct SECKEYPublicKeyStr SECKEYPublicKey;
 #endif
@@ -41,11 +42,12 @@ class CRYPTO_EXPORT ECPrivateKey {
   // TODO(mattm): Add a curve parameter.
   static ECPrivateKey* Create();
 
-  // Creates a new random instance. Can return NULL if initialization fails.
-  // The created key is permanent and is not exportable in plaintext form.
-  //
-  // NOTE: Currently only available if USE_NSS is defined.
-  static ECPrivateKey* CreateSensitive();
+#if defined(USE_NSS)
+  // Creates a new random instance in |slot|. Can return NULL if initialization
+  // fails.  The created key is permanent and is not exportable in plaintext
+  // form.
+  static ECPrivateKey* CreateSensitive(PK11SlotInfo* slot);
+#endif
 
   // Creates a new instance by importing an existing key pair.
   // The key pair is given as an ASN.1-encoded PKCS #8 EncryptedPrivateKeyInfo
@@ -56,24 +58,26 @@ class CRYPTO_EXPORT ECPrivateKey {
       const std::vector<uint8>& encrypted_private_key_info,
       const std::vector<uint8>& subject_public_key_info);
 
-  // Creates a new instance by importing an existing key pair.
+#if defined(USE_NSS)
+  // Creates a new instance in |slot| by importing an existing key pair.
   // The key pair is given as an ASN.1-encoded PKCS #8 EncryptedPrivateKeyInfo
   // block and an X.509 SubjectPublicKeyInfo block.
   // This can return NULL if initialization fails.  The created key is permanent
   // and is not exportable in plaintext form.
-  //
-  // NOTE: Currently only available if USE_NSS is defined.
   static ECPrivateKey* CreateSensitiveFromEncryptedPrivateKeyInfo(
+      PK11SlotInfo* slot,
       const std::string& password,
       const std::vector<uint8>& encrypted_private_key_info,
       const std::vector<uint8>& subject_public_key_info);
+#endif
 
 #if !defined(USE_OPENSSL)
-  // Imports the key pair and returns in |public_key| and |key|.
+  // Imports the key pair into |slot| and returns in |public_key| and |key|.
   // Shortcut for code that needs to keep a reference directly to NSS types
   // without having to create a ECPrivateKey object and make a copy of them.
   // TODO(mattm): move this function to some NSS util file.
   static bool ImportFromEncryptedPrivateKeyInfo(
+      PK11SlotInfo* slot,
       const std::string& password,
       const uint8* encrypted_private_key_info,
       size_t encrypted_private_key_info_len,
@@ -112,20 +116,24 @@ class CRYPTO_EXPORT ECPrivateKey {
   // Constructor is private. Use one of the Create*() methods above instead.
   ECPrivateKey();
 
+#if !defined(USE_OPENSSL)
   // Shared helper for Create() and CreateSensitive().
   // TODO(cmasone): consider replacing |permanent| and |sensitive| with a
   //                flags arg created by ORing together some enumerated values.
-  static ECPrivateKey* CreateWithParams(bool permanent,
+  static ECPrivateKey* CreateWithParams(PK11SlotInfo* slot,
+                                        bool permanent,
                                         bool sensitive);
 
   // Shared helper for CreateFromEncryptedPrivateKeyInfo() and
   // CreateSensitiveFromEncryptedPrivateKeyInfo().
   static ECPrivateKey* CreateFromEncryptedPrivateKeyInfoWithParams(
+      PK11SlotInfo* 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);
+#endif
 
 #if defined(USE_OPENSSL)
   EVP_PKEY* key_;
diff --git a/crypto/ec_private_key_nss.cc b/crypto/ec_private_key_nss.cc
index 9bb9df1..1d3bf88 100644
--- a/crypto/ec_private_key_nss.cc
+++ b/crypto/ec_private_key_nss.cc
@@ -25,8 +25,8 @@ extern "C" {
 
 namespace {
 
-PK11SlotInfo* GetKeySlot() {
-  return crypto::GetPublicNSSKeySlot();
+PK11SlotInfo* GetTempKeySlot() {
+  return PK11_GetInternalSlot();
 }
 
 class EllipticCurveSupportChecker {
@@ -37,7 +37,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(GetTempKeySlot());
     supported_ = PK11_DoesMechanism(slot.get(), CKM_EC_KEY_PAIR_GEN) &&
         PK11_DoesMechanism(slot.get(), CKM_ECDSA);
   }
@@ -88,58 +88,59 @@ bool ECPrivateKey::IsSupported() {
 
 // static
 ECPrivateKey* ECPrivateKey::Create() {
-  return CreateWithParams(PR_FALSE /* not permanent */,
-                          PR_FALSE /* not sensitive */);
+  EnsureNSSInit();
+
+  ScopedPK11Slot slot(GetTempKeySlot());
+  return CreateWithParams(slot.get(),
+                          false /* not permanent */,
+                          false /* not sensitive */);
 }
 
-// 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(PK11SlotInfo* slot) {
+  return CreateWithParams(
+      slot, true /* permanent */, true /* sensitive */);
 }
+#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();
+
+  ScopedPK11Slot slot(GetTempKeySlot());
   return CreateFromEncryptedPrivateKeyInfoWithParams(
+      slot.get(),
       password,
       encrypted_private_key_info,
       subject_public_key_info,
-      PR_FALSE /* not permanent */,
-      PR_FALSE /* not sensitive */);
+      false /* not permanent */,
+      false /* not sensitive */);
 }
 
+#if defined(USE_NSS)
 // static
 ECPrivateKey* ECPrivateKey::CreateSensitiveFromEncryptedPrivateKeyInfo(
+    PK11SlotInfo* 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,
       password,
       encrypted_private_key_info,
       subject_public_key_info,
-      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
+      true /* permanent */,
+      true /* sensitive */);
 }
+#endif
 
 // static
 bool ECPrivateKey::ImportFromEncryptedPrivateKeyInfo(
+    PK11SlotInfo* slot,
     const std::string& password,
     const uint8* encrypted_private_key_info,
     size_t encrypted_private_key_info_len,
@@ -148,8 +149,7 @@ bool ECPrivateKey::ImportFromEncryptedPrivateKeyInfo(
     bool sensitive,
     SECKEYPrivateKey** key,
     SECKEYPublicKey** public_key) {
-  ScopedPK11Slot slot(GetKeySlot());
-  if (!slot.get())
+  if (!slot)
     return false;
 
   *public_key = SECKEY_ExtractPublicKey(decoded_spki);
@@ -188,7 +188,7 @@ bool ECPrivateKey::ImportFromEncryptedPrivateKeyInfo(
   };
 
   rv = ImportEncryptedECPrivateKeyInfoAndReturnKey(
-      slot.get(),
+      slot,
       &epki,
       &password_item,
       NULL,  // nickname
@@ -275,16 +275,14 @@ bool ECPrivateKey::ExportECParams(std::vector<uint8>* output) {
 ECPrivateKey::ECPrivateKey() : key_(NULL), public_key_(NULL) {}
 
 // static
-ECPrivateKey* ECPrivateKey::CreateWithParams(bool permanent,
+ECPrivateKey* ECPrivateKey::CreateWithParams(PK11SlotInfo* slot,
+                                             bool permanent,
                                              bool sensitive) {
-  EnsureNSSInit();
+  if (!slot)
+    return NULL;
 
   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();
@@ -306,7 +304,7 @@ ECPrivateKey* ECPrivateKey::CreateWithParams(bool permanent,
   ec_parameters.data[1] = oid_data->oid.len;
   memcpy(ec_parameters.data + 2, oid_data->oid.data, oid_data->oid.len);
 
-  result->key_ = PK11_GenerateKeyPair(slot.get(),
+  result->key_ = PK11_GenerateKeyPair(slot,
                                       CKM_EC_KEY_PAIR_GEN,
                                       &ec_parameters,
                                       &result->public_key_,
@@ -323,13 +321,12 @@ ECPrivateKey* ECPrivateKey::CreateWithParams(bool permanent,
 
 // static
 ECPrivateKey* ECPrivateKey::CreateFromEncryptedPrivateKeyInfoWithParams(
+    PK11SlotInfo* 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 = {
@@ -344,7 +341,8 @@ ECPrivateKey* ECPrivateKey::CreateFromEncryptedPrivateKeyInfoWithParams(
     return NULL;
   }
 
-  bool success = ECPrivateKey::ImportFromEncryptedPrivateKeyInfo(
+  bool success = ImportFromEncryptedPrivateKeyInfo(
+      slot,
       password,
       &encrypted_private_key_info[0],
       encrypted_private_key_info.size(),
diff --git a/crypto/ec_private_key_openssl.cc b/crypto/ec_private_key_openssl.cc
index c8d9c25..ce921dd 100644
--- a/crypto/ec_private_key_openssl.cc
+++ b/crypto/ec_private_key_openssl.cc
@@ -101,12 +101,6 @@ ECPrivateKey* ECPrivateKey::Create() {
 }
 
 // static
-ECPrivateKey* ECPrivateKey::CreateSensitive() {
-  NOTIMPLEMENTED();
-  return NULL;
-}
-
-// static
 ECPrivateKey* ECPrivateKey::CreateFromEncryptedPrivateKeyInfo(
     const std::string& password,
     const std::vector<uint8>& encrypted_private_key_info,
@@ -150,15 +144,6 @@ ECPrivateKey* ECPrivateKey::CreateFromEncryptedPrivateKeyInfo(
   return result.release();
 }
 
-// static
-ECPrivateKey* ECPrivateKey::CreateSensitiveFromEncryptedPrivateKeyInfo(
-    const std::string& password,
-    const std::vector<uint8>& encrypted_private_key_info,
-    const std::vector<uint8>& subject_public_key_info) {
-  NOTIMPLEMENTED();
-  return NULL;
-}
-
 bool ECPrivateKey::ExportEncryptedPrivateKey(
     const std::string& password,
     int iterations,
diff --git a/crypto/rsa_private_key.h b/crypto/rsa_private_key.h
index ad82148..4453845 100644
--- a/crypto/rsa_private_key.h
+++ b/crypto/rsa_private_key.h
@@ -20,8 +20,9 @@
 #if defined(USE_OPENSSL)
 // Forward declaration for openssl/*.h
 typedef struct evp_pkey_st EVP_PKEY;
-#elif defined(USE_NSS) || defined(OS_WIN) || defined(OS_MACOSX)
+#else
 // Forward declaration.
+typedef struct PK11SlotInfoStr PK11SlotInfo;
 typedef struct SECKEYPrivateKeyStr SECKEYPrivateKey;
 typedef struct SECKEYPublicKeyStr SECKEYPublicKey;
 #endif
@@ -180,15 +181,17 @@ class CRYPTO_EXPORT RSAPrivateKey {
       const std::vector<uint8>& input);
 
 #if defined(USE_NSS)
-  // Create a new random instance. Can return NULL if initialization fails.
-  // The created key is permanent and is not exportable in plaintext form.
-  static RSAPrivateKey* CreateSensitive(uint16 num_bits);
-
-  // Create a new instance by importing an existing private key. The format is
-  // an ASN.1-encoded PrivateKeyInfo block from PKCS #8. This can return NULL if
-  // initialization fails.
+  // Create a new random instance in |slot|. Can return NULL if initialization
+  // fails.  The created key is permanent and is not exportable in plaintext
+  // form.
+  static RSAPrivateKey* CreateSensitive(PK11SlotInfo* slot, uint16 num_bits);
+
+  // Create a new instance in |slot| by importing an existing private key. The
+  // format is an ASN.1-encoded PrivateKeyInfo block from PKCS #8. This can
+  // return NULL if initialization fails.
   // The created key is permanent and is not exportable in plaintext form.
   static RSAPrivateKey* CreateSensitiveFromPrivateKeyInfo(
+      PK11SlotInfo* slot,
       const std::vector<uint8>& input);
 
   // Create a new instance by referencing an existing private key
@@ -207,7 +210,7 @@ class CRYPTO_EXPORT RSAPrivateKey {
 
 #if defined(USE_OPENSSL)
   EVP_PKEY* key() { return key_; }
-#elif defined(USE_NSS) || defined(OS_WIN) || defined(OS_MACOSX)
+#else
   SECKEYPrivateKey* key() { return key_; }
   SECKEYPublicKey* public_key() { return public_key_; }
 #endif
@@ -231,11 +234,13 @@ class CRYPTO_EXPORT RSAPrivateKey {
   // methods above instead.
   RSAPrivateKey();
 
+#if !defined(USE_OPENSSL)
   // Shared helper for Create() and CreateSensitive().
   // TODO(cmasone): consider replacing |permanent| and |sensitive| with a
   //                flags arg created by ORing together some enumerated values.
   // Note: |permanent| is only supported when USE_NSS is defined.
-  static RSAPrivateKey* CreateWithParams(uint16 num_bits,
+  static RSAPrivateKey* CreateWithParams(PK11SlotInfo* slot,
+                                         uint16 num_bits,
                                          bool permanent,
                                          bool sensitive);
 
@@ -243,13 +248,15 @@ class CRYPTO_EXPORT RSAPrivateKey {
   // CreateSensitiveFromPrivateKeyInfo().
   // Note: |permanent| is only supported when USE_NSS is defined.
   static RSAPrivateKey* CreateFromPrivateKeyInfoWithParams(
+      PK11SlotInfo* slot,
       const std::vector<uint8>& input,
       bool permanent,
       bool sensitive);
+#endif
 
 #if defined(USE_OPENSSL)
   EVP_PKEY* key_;
-#elif defined(USE_NSS) || defined(OS_WIN) || defined(OS_MACOSX)
+#else
   SECKEYPrivateKey* key_;
   SECKEYPublicKey* public_key_;
 #endif
diff --git a/crypto/rsa_private_key_nss.cc b/crypto/rsa_private_key_nss.cc
index 35697ab..bd54c2e 100644
--- a/crypto/rsa_private_key_nss.cc
+++ b/crypto/rsa_private_key_nss.cc
@@ -51,7 +51,11 @@ RSAPrivateKey::~RSAPrivateKey() {
 
 // static
 RSAPrivateKey* RSAPrivateKey::Create(uint16 num_bits) {
-  return CreateWithParams(num_bits,
+  EnsureNSSInit();
+
+  ScopedPK11Slot slot(PK11_GetInternalSlot());
+  return CreateWithParams(slot.get(),
+                          num_bits,
                           false /* not permanent */,
                           false /* not sensitive */);
 }
@@ -59,23 +63,32 @@ RSAPrivateKey* RSAPrivateKey::Create(uint16 num_bits) {
 // static
 RSAPrivateKey* RSAPrivateKey::CreateFromPrivateKeyInfo(
     const std::vector<uint8>& input) {
-  return CreateFromPrivateKeyInfoWithParams(input,
-                                            false /* not permanent */,
-                                            false /* not sensitive */);
+  EnsureNSSInit();
+
+  ScopedPK11Slot slot(PK11_GetInternalSlot());
+  return CreateFromPrivateKeyInfoWithParams(
+      slot.get(),
+      input,
+      false /* not permanent */,
+      false /* not sensitive */);
 }
 
 #if defined(USE_NSS)
 // static
-RSAPrivateKey* RSAPrivateKey::CreateSensitive(uint16 num_bits) {
-  return CreateWithParams(num_bits,
+RSAPrivateKey* RSAPrivateKey::CreateSensitive(PK11SlotInfo* slot,
+                                              uint16 num_bits) {
+  return CreateWithParams(slot,
+                          num_bits,
                           true /* permanent */,
                           true /* sensitive */);
 }
 
 // static
 RSAPrivateKey* RSAPrivateKey::CreateSensitiveFromPrivateKeyInfo(
+    PK11SlotInfo* slot,
     const std::vector<uint8>& input) {
-  return CreateFromPrivateKeyInfoWithParams(input,
+  return CreateFromPrivateKeyInfoWithParams(slot,
+                                            input,
                                             true /* permanent */,
                                             true /* sensitive */);
 }
@@ -200,29 +213,19 @@ RSAPrivateKey::RSAPrivateKey() : key_(NULL), public_key_(NULL) {
 }
 
 // static
-RSAPrivateKey* RSAPrivateKey::CreateWithParams(uint16 num_bits,
+RSAPrivateKey* RSAPrivateKey::CreateWithParams(PK11SlotInfo* slot,
+                                               uint16 num_bits,
                                                bool permanent,
                                                bool sensitive) {
-#if !defined(USE_NSS)
-  if (permanent) {
-    NOTIMPLEMENTED();
+  if (!slot)
     return NULL;
-  }
-#endif
-
-  EnsureNSSInit();
 
   scoped_ptr<RSAPrivateKey> result(new RSAPrivateKey);
 
-  ScopedPK11Slot slot(permanent ? GetPrivateNSSKeySlot() :
-                      PK11_GetInternalSlot());
-  if (!slot.get())
-    return NULL;
-
   PK11RSAGenParams param;
   param.keySizeInBits = num_bits;
   param.pe = 65537L;
-  result->key_ = PK11_GenerateKeyPair(slot.get(),
+  result->key_ = PK11_GenerateKeyPair(slot,
                                       CKM_RSA_PKCS_KEY_PAIR_GEN,
                                       &param,
                                       &result->public_key_,
@@ -237,26 +240,15 @@ RSAPrivateKey* RSAPrivateKey::CreateWithParams(uint16 num_bits,
 
 // static
 RSAPrivateKey* RSAPrivateKey::CreateFromPrivateKeyInfoWithParams(
-    const std::vector<uint8>& input, bool permanent, bool sensitive) {
-#if !defined(USE_NSS)
-  if (permanent) {
-    NOTIMPLEMENTED();
+    PK11SlotInfo* slot,
+    const std::vector<uint8>& input,
+    bool permanent,
+    bool sensitive) {
+  if (!slot)
     return NULL;
-  }
-#endif
-
-  // This method currently leaks some memory.
-  // See http://crbug.com/34742.
-  ANNOTATE_SCOPED_MEMORY_LEAK;
-  EnsureNSSInit();
 
   scoped_ptr<RSAPrivateKey> result(new RSAPrivateKey);
 
-  ScopedPK11Slot slot(permanent ? GetPrivateNSSKeySlot() :
-                      PK11_GetInternalSlot());
-  if (!slot.get())
-    return NULL;
-
   SECItem der_private_key_info;
   der_private_key_info.data = const_cast<unsigned char*>(&input.front());
   der_private_key_info.len = input.size();
@@ -265,7 +257,7 @@ RSAPrivateKey* RSAPrivateKey::CreateFromPrivateKeyInfoWithParams(
   const unsigned int key_usage = KU_KEY_ENCIPHERMENT | KU_DATA_ENCIPHERMENT |
                                  KU_DIGITAL_SIGNATURE;
   SECStatus rv =  PK11_ImportDERPrivateKeyInfoAndReturnKey(
-      slot.get(), &der_private_key_info, NULL, NULL, permanent, sensitive,
+      slot, &der_private_key_info, NULL, NULL, permanent, sensitive,
       key_usage, &result->key_, NULL);
   if (rv != SECSuccess) {
     NOTREACHED();