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

crypto/rsa_private_key_nss.cc

Index: crypto/rsa_private_key_nss.cc
diff --git a/crypto/rsa_private_key_nss.cc b/crypto/rsa_private_key_nss.cc
index 35697abb4e721ed7fe14f0022cab16469b2676f1..2eeb822afcd51af57ea43d10ea7cbea829a03406 100644
--- a/crypto/rsa_private_key_nss.cc
+++ b/crypto/rsa_private_key_nss.cc
@@ -51,7 +51,10 @@ RSAPrivateKey::~RSAPrivateKey() {
 
 // static
 RSAPrivateKey* RSAPrivateKey::Create(uint16 num_bits) {
-  return CreateWithParams(num_bits,
+  EnsureNSSInit();
+
+  return CreateWithParams(ScopedPK11Slot(PK11_GetInternalKeySlot()),

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

IMPORTANT: this should be PK11_GetInternalSlot().

See lines 217-218 in the original code.

Note: when NSS cert and key databases are used, PK11_GetInternalKeySlot() and
PK11_GetInternalSlot() are different slots. PK11_GetInternalKeySlot() is the
slot that is backed by the NSS cert and key databases, so it can create
permanent keys if necessary.

PK11_GetInternalSlot() is a slot that is usually NOT backed by the NSS cert and
key databases, so it should only be used for temporary keys.

When NSS cert and key databases are not used, PK11_GetInternalKeySlot() and
PK11_GetInternalSlot() are the same slot.

Please let me know if this is not clear. I know it is confusing.

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

Thanks, fixed.

+                          num_bits,
                           false /* not permanent */,
                           false /* not sensitive */);
 }
@@ -59,23 +62,31 @@ 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();
+
+  return CreateFromPrivateKeyInfoWithParams(
+      ScopedPK11Slot(PK11_GetInternalKeySlot()),

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

IMPORTANT: this should be PK11_GetInternalSlot().

See lines 255-256 in the original code.

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

Done.

+      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(ScopedPK11Slot slot,
+                                              uint16 num_bits) {
+  return CreateWithParams(slot.Pass(),
+                          num_bits,
                           true /* permanent */,
                           true /* sensitive */);
 }
 
 // static
 RSAPrivateKey* RSAPrivateKey::CreateSensitiveFromPrivateKeyInfo(
+    ScopedPK11Slot slot,
     const std::vector<uint8>& input) {
-  return CreateFromPrivateKeyInfoWithParams(input,
+  return CreateFromPrivateKeyInfoWithParams(slot.Pass(),
+                                            input,
                                             true /* permanent */,
                                             true /* sensitive */);
 }
@@ -200,25 +211,15 @@ RSAPrivateKey::RSAPrivateKey() : key_(NULL), public_key_(NULL) {
 }
 
 // static
-RSAPrivateKey* RSAPrivateKey::CreateWithParams(uint16 num_bits,
+RSAPrivateKey* RSAPrivateKey::CreateWithParams(ScopedPK11Slot slot,
+                                               uint16 num_bits,
                                                bool permanent,
                                                bool sensitive) {
-#if !defined(USE_NSS)
-  if (permanent) {
-    NOTIMPLEMENTED();
+  if (!slot.get())
     return NULL;
-  }
-#endif

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

BUG?: it seems wrong to remove this check.

In this file, !defined(USE_NSS) is equivalent to defined(OS_WIN) ||
defined(OS_MACOSX), where we use the NSS library but not the NSS cert and key
databases. On those two platforms, we should still check that |permanent| is
false.

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

Hm, yeah. I guess I was thinking that since CreateSensitive functions are only
being defined when USE_NSS is defined that it shouldn't be possible to trigger
the check. But it does seem good to leave the check since there could still be a
bug internal to the class, or to catch problems in future modifications. 
Re-added.

[wtc, 2013/11/12 18:49:42]

I see. Yes, you are right, we can conclude the check won't fail by code
inspection.
The NSS function (PK11_GenerateKeyPair, etc.) to create a permanent key will
fail if there is no NSS database, so this check isn't really necessary. It is
merely an advance warning for something that is expected to fail, which should
make debugging easier.

Now that I understand the background, I am fine with omitting the check.

[mattm, 2013/11/12 22:27:03]

Done.

-
-  EnsureNSSInit();
 
   scoped_ptr<RSAPrivateKey> result(new RSAPrivateKey);
 
-  ScopedPK11Slot slot(permanent ? GetPrivateNSSKeySlot() :
-                      PK11_GetInternalSlot());

[Ryan Sleevi, 2013/11/09 02:26:47]

Pretty sure ChromeOS is relying on this for TPM provisioning

[mattm, 2013/11/09 03:44:32]

Hm, I found "platform/login_manager" uses RSAPrivateKey, though not the
CreateSensitive functions:
https://chromium.googlesource.com/chromiumos/platform/login_manager/+/master/...

However, a few test related things do use CreateSensitive:
https://chromium.googlesource.com/chromiumos/platform/login_manager/+/master/...
https://chromium.googlesource.com/chromiumos/platform/login_manager/+/master/...

I guess a 3-sided patch would be needed to update those..

[wtc, 2013/11/11 20:58:20]

Good. You already found the files that need to be updated.

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

So I don't know what the procedure is for rolling the libchrome_crypto package
in chromeos... it looks like it's still way back on r180245.  Do you think it's
acceptable to leave it up to whoever rolls that package to update the few
chromeos-side callers at that time?

[wtc, 2013/11/12 18:49:42]

I don't know what the procedure is, either.
I think we should at least send a patch to the owner of the libchrome_crypto
package. Can you write the patch?

[mattm, 2013/11/12 22:27:03]

Checked with cmasone and sent him an example patch.  He said to go ahead and
he'll deal with it when he rolls the libchrome_crypto.

-  if (!slot.get())
-    return NULL;
-
   PK11RSAGenParams param;
   param.keySizeInBits = num_bits;
   param.pe = 65537L;
@@ -237,26 +238,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();
+    ScopedPK11Slot slot,
+    const std::vector<uint8>& input,
+    bool permanent,
+    bool sensitive) {
+  if (!slot.get())
     return NULL;
-  }
-#endif

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

BUG?: it seems wrong to remove this check.

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

Done.

-
-  // 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();