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

Fixing FindFromPublicKeyInfo so that it searches the "Public" NSS database
if it doesn't find the requested key in the "Private" NSS database.

This fixes the ownership process because the ownership key is created
in the public database because that needs to happen before the TPM is
owned and available (and it's not really all that sensitive to begin
with).

BUG=chromium-os:15645
TEST=Built a new recovery image, wiped a device with it and verified
that I was able to sign in as a new user and add users and forget
networks.  It also showed me as the owner of the device.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=86654

[Chris Masone, 2011-05-24 22:03:56]

LGTM

[Greg Spencer (Chromium), 2011-05-25 05:17:06]

Adding OWNERS to review.

[Ryan Sleevi, 2011-05-25 05:58:24]

Functionally LGTM, but some nits/comments for possible cleanup tasks, either in
this or future CLs.

http://codereview.chromium.org/7066032/diff/1/crypto/rsa_private_key_nss.cc
File crypto/rsa_private_key_nss.cc (right):

http://codereview.chromium.org/7066032/diff/1/crypto/rsa_private_key_nss.cc#n...
crypto/rsa_private_key_nss.cc:94: CERTSubjectPublicKeyInfo *spki =
nit: CERTSubjectPublicKeyInfo *spki -> CERTSubjectPublicKeyInfo* spki

http://codereview.chromium.org/7066032/diff/1/crypto/rsa_private_key_nss.cc#n...
crypto/rsa_private_key_nss.cc:114: SECItem *ck_id =
PK11_MakeIDFromPubKey(&(result->public_key_->u.rsa.modulus));
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.

http://codereview.chromium.org/7066032/diff/1/crypto/rsa_private_key_nss.cc#n...
crypto/rsa_private_key_nss.cc:134: slot = GetPublicNSSKeySlot();
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]

http://codereview.chromium.org/7066032/diff/1/crypto/rsa_private_key_nss.cc
File crypto/rsa_private_key_nss.cc (right):

http://codereview.chromium.org/7066032/diff/1/crypto/rsa_private_key_nss.cc#n...
crypto/rsa_private_key_nss.cc:94: CERTSubjectPublicKeyInfo *spki =
On 2011/05/25 05:58:25, Ryan Sleevi wrote:
> nit: CERTSubjectPublicKeyInfo *spki -> CERTSubjectPublicKeyInfo* spki

Done.

http://codereview.chromium.org/7066032/diff/1/crypto/rsa_private_key_nss.cc#n...
crypto/rsa_private_key_nss.cc:114: SECItem *ck_id =
PK11_MakeIDFromPubKey(&(result->public_key_->u.rsa.modulus));
On 2011/05/25 05:58:25, Ryan Sleevi wrote:
> You can make this a ScopedSECItem (see src/crypto/scoped_nss_types.h), which
> should make the error handling a lot easier for you.

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

http://codereview.chromium.org/7066032/diff/1/crypto/rsa_private_key_nss.cc#n...
crypto/rsa_private_key_nss.cc:134: slot = GetPublicNSSKeySlot();
On 2011/05/25 05:58:25, Ryan Sleevi wrote:
> According to the header comments for GetPublicNSSKeySlot(), this slot is just
> meant for public keys/non-sensitive data, not private keys.

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.

[wtc, 2011-05-25 23:34:00]

LGTM.

http://codereview.chromium.org/7066032/diff/1002/crypto/rsa_private_key_nss.cc
File crypto/rsa_private_key_nss.cc (right):

http://codereview.chromium.org/7066032/diff/1002/crypto/rsa_private_key_nss.c...
crypto/rsa_private_key_nss.cc:134: slot.reset(GetPublicNSSKeySlot());
If the computer doesn't have a TPM, GetPrivateNSSKeySlot()
and GetPublicNSSKeySlot() are the same slot, and we may
search in that slot twice.  This only happens when the RSA
private key doesn't exist, so the inefficiency is acceptable.

BUT, it seems that this function should search in all the
slots.  (I found that rsleevi said the same thing.)