Fixing FindFromPublicKeyInfo so that it searches the "Public" NSS database

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 bac72814fe16d1cc168c637a019ebda082dcd492..03cc254a4ada284957a7ce04b0c46fab00ee995c 100644
--- a/crypto/rsa_private_key_nss.cc
+++ b/crypto/rsa_private_key_nss.cc
@@ -105,32 +105,41 @@ RSAPrivateKey* RSAPrivateKey::FindFromPublicKeyInfo(
     return NULL;
   }
 
-  // Now, look for the associated private key in the user's
-  // hardware-backed NSS DB.  If it's not there, consider that an
-  // error.
-  PK11SlotInfo *slot = GetPrivateNSSKeySlot();
-  if (!slot) {
-    NOTREACHED();
-    return NULL;
-  }
-
   // Make sure the key is an RSA key.  If not, that's an error
   if (result->public_key_->keyType != rsaKey) {
-    PK11_FreeSlot(slot);
     NOTREACHED();
     return NULL;
   }
 
   SECItem *ck_id = PK11_MakeIDFromPubKey(&(result->public_key_->u.rsa.modulus));

[Ryan Sleevi, 2011/05/25 05:58:25]

You can make this a ScopedSECItem (see src/crypto/scoped_nss_types.h), which
should make the error handling a lot easier for you.

See also ScopedPK11Slot for |slot| on line 120.

There are several other instances in this file where you can get away with the
Scoped classes (178/187, 203/212, 229/243), which you could update as well.

[Greg Spencer (Chromium), 2011/05/25 17:12:07]

Good point. I forgot about the scoped nss types.  Done.

   if (!ck_id) {
-    PK11_FreeSlot(slot);
     NOTREACHED();
     return NULL;
   }
 
+  PK11SlotInfo* slot = GetPrivateNSSKeySlot();
+  if (!slot) {
+    NOTREACHED();
+    SECITEM_FreeItem(ck_id, PR_TRUE);
+    return NULL;
+  }
+
   // Finally...Look for the key!
   result->key_ = PK11_FindKeyByKeyID(slot, ck_id, NULL);
 
+  // If we don't find the matching key in the private slot, then we
+  // look in the public slot.
+  if (!result->key_) {
+    PK11_FreeSlot(slot);
+    slot = GetPublicNSSKeySlot();

[Ryan Sleevi, 2011/05/25 05:58:25]

According to the header comments for GetPublicNSSKeySlot(), this slot is just
meant for public keys/non-sensitive data, not private keys. 

http://src.chromium.org/viewvc/chrome/trunk/src/crypto/nss_util_internal.h?re...

What it sounds like you're really after is the software token slot - since
that's what GetPublicNSSKeySlot() always returns, whereas GetPrivateNSSKeySlot()
conditionally returns the TPM when on CrOS.

Perhaps "GetPublicNSSKeySlot()" should be renamed to "GetSoftwareNSSKeySlot()",
and the header comments updated to highlight that it's not "just" for public
keys.



Finally, I'm a little nervous (long term, not short-term) about checking only
two slots, since it sounds like you're really desiring that it simply be in *a*
slot. If something like http://crbug.com/63542 is desired from CrOS, or if this
is used outside of CrOS, then this code would need to be re-written to satisfy
it. If not, or if you'd rather defer/postpone until that issue, you can ignore
the rest of this comment.

Perhaps something similar to CertDatabase::ListModules() or
PK11_KeyForCertExists() is what you want:
 - PK11_GetAllTokens
 - PK11_GetFirstSafe
 - PK11_GetNextSafe
 - PK11_FreeSlotList 

checking each token for PK11_FindKeyByKeyID to find *a* slot with the key.

[Greg Spencer (Chromium), 2011/05/25 17:12:07]

Yes, I wrote those comments.

The general policy for ChromeOS is to have all private key information stored in
the TPM by default, and all other data stored in the software backed slot.

For the device ownership key (not the same as TPM ownership, this is the
ChromeOS device "owner" user concept), however, we need it to be created before
cryptohomed has finished initializing the TPM, and the key itself is created in
the software NSS database by non-chrome code.  We deem this to be OK

This CL is making it so that when we search for a key, we look in both the
public and private sections.

When the feature you referenced is implemented, we can expand this to include
all slots, but for right now simply having it look in both of the slots that we
use is good enough.

+    if (!slot) {
+      NOTREACHED();
+      SECITEM_FreeItem(ck_id, PR_TRUE);
+      return NULL;
+    }
+    result->key_ = PK11_FindKeyByKeyID(slot, ck_id, NULL);
+  }
+
   // Cleanup...
   PK11_FreeSlot(slot);
   SECITEM_FreeItem(ck_id, PR_TRUE);