Reland "Don't use RSAPrivateKey in NSS integration code."

Reland "Don't use RSAPrivateKey in NSS integration code."

This is a reland of https://codereview.chromium.org/1106103003/ with fixes to
ensure callers never pass in a null slot.

Currently some NSS platform integration logic transits private keys through
RSAPrivateKey on CrOS. This prevents incrementally switching RSAPrivateKey to
BoringSSL while keeping platform integrations on NSS.

The intent of this change is to clarify RSAPrivateKey as a BoringSSL vs NSS
internal crypto library (use_openssl=0 vs use_openssl=1) abstraction. It's
primarily to be used with SignatureCreator. Code which uses NSS based on
use_nss_certs rather than use_openssl because the underlying platform is NSS
should call NSS routines directly, or introduce different abstractions.

Remove the problematic RSAPrivateKey methods and instead add
crypto/nss_key_util.h which contains some helper functions for manipulating NSS
keys. This is sufficient to allow consumers of the removed methods to use NSS
directly with about as much code. (This should not set back migrating that
logic to NSS as that code was already very NSS-specific; those APIs assumed
PK11SlotInfo.)

nss_key_util.h, like nss_util.h, is built whenever NSS is used either
internally or for platform integrations. This is so rsa_private_key_nss.cc can
continue to use the helper functions to implement the NSS-agnostic interface.

With this, the chimera CrOS configuration should build. The RSAPrivateKey logic
is functional with the exception of some logic in components/ownership. That
will be resolved in a future CL.

BUG=478777, 483606
Committed: https://crrev.com/85bad9e7d11461b7599967c9a504da9ac96f11b3
Cr-Commit-Position: refs/heads/master@{#329227}

[davidben, 2015-05-06 18:40:19]

Patchset #1 (id:1) has been deleted

[davidben, 2015-05-06 18:48:38]

davidben@chromium.org changed reviewers:
+ dpolukhin@chromium.org, rsleevi@chromium.org

[davidben, 2015-05-06 18:48:39]

rsleevi for crypto and net, dpolukhin for chromeos and ownership.

The main difference is that callers of the new NSS functions now account for
null slots, where needed, before calling them. The old RSAPrivateKey functions
silently returned null on null input, but I think this makes the fact that some
slots are optional less clear and masks issues (see
OwnerSettingsService::IsOwnerAsync comment below).

https://codereview.chromium.org/1128153003/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc
(right):

https://codereview.chromium.org/1128153003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc:74: }
I switched this to the slotless variant in case any existing code passed in a
null slot and to avoid behavior differences.

https://codereview.chromium.org/1128153003/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/ownership/owner_settings_service_chromeos.h
(right):

https://codereview.chromium.org/1128153003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos.h:140: //
|private_key->key()| may be null.
This is really weird and, tbh, I don't think it makes much sense. But
OwnerSettingsService::IsOwnerAsync actually assumes this with the private_key_
check. I'm somewhat puzzled by that code but have left it alone for now. It
seems a boolean keypair_loaded_ would make much more sense, but I didn't want to
make this CL grow in scope that much.

Plus there's failure paths where ReloadKeypairImpl gives back a null
scoped_refptr<PrivateKey> so that gets more confusing...

dpolukhin: I imagine this should get a TODO of some sort. What would you like me
to write in there.

https://codereview.chromium.org/1128153003/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/platform_keys/platform_keys_nss.cc (right):

https://codereview.chromium.org/1128153003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys_nss.cc:404: if
(!state->slot_) {
This shouldn't be possible, but if token_id is empty, DidGetCertDBOnIOThread
intentionally leaves slot_ null. The only caller I found checks the token_id
parameter early, but that's not locally clear.

[Ryan Sleevi, 2015-05-06 23:46:02]

rsleevi@chromium.org changed reviewers:
+ pneubeck@chromium.org

[Ryan Sleevi, 2015-05-06 23:46:03]

Tagging in pneubeck to help me make sense of this. I definitely need another set
of ChromeOS eyes on it.

And for completeness, cc'ing Matt as well in case he knows.

https://codereview.chromium.org/1128153003/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc
(right):

https://codereview.chromium.org/1128153003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:87:
public_slot.get());
This shouldn't have a private key. I have no clue what's going on here.
pneubeck@, please tell me you can make sense of this :)

[Dmitry Polukhin, 2015-05-07 10:59:15]

dpolukhin@chromium.org changed reviewers:
+ cmasone@chromium.org

[Dmitry Polukhin, 2015-05-07 10:59:16]

+ cmasone@

https://codereview.chromium.org/1128153003/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc
(right):

https://codereview.chromium.org/1128153003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:87:
public_slot.get());
On 2015/05/06 23:46:03, Ryan Sleevi wrote:
> This shouldn't have a private key. I have no clue what's going on here.
> pneubeck@, please tell me you can make sense of this :)

The code was added here https://codereview.chromium.org/660343002

https://codereview.chromium.org/1128153003/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/ownership/owner_settings_service_chromeos.h
(right):

https://codereview.chromium.org/1128153003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos.h:140: //
|private_key->key()| may be null.
On 2015/05/06 18:48:39, David Benjamin wrote:
> This is really weird and, tbh, I don't think it makes much sense. But
> OwnerSettingsService::IsOwnerAsync actually assumes this with the private_key_
> check. I'm somewhat puzzled by that code but have left it alone for now. It
> seems a boolean keypair_loaded_ would make much more sense, but I didn't want
to
> make this CL grow in scope that much.
> 
> Plus there's failure paths where ReloadKeypairImpl gives back a null
> scoped_refptr<PrivateKey> so that gets more confusing...
> 
> dpolukhin: I imagine this should get a TODO of some sort. What would you like
me
> to write in there.

NULL private key means that active user is not an owner. But public key should
be available. I'm not an original author of the code.

[pneubeck (no reviews), 2015-05-07 12:19:16]

https://codereview.chromium.org/1128153003/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc
(right):

https://codereview.chromium.org/1128153003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc:74: }
On 2015/05/06 18:48:38, David Benjamin wrote:
> I switched this to the slotless variant in case any existing code passed in a
> null slot and to avoid behavior differences.

I don't understand why all slots should be searched for the key?

Reading the above documentation, this function (GetPrivateKey...) should search
in a specific slot.
If that slot is null, it's an error (handled in  SignDataOnWorkerThread) and
should not silently return a slotless key.

I don't know if NSS can have key pairs without assigned slot.
As that is not obvious, please don't rely on it.

Instead, please do an early return instead at the beginning of the function.

(If something like slotless keys exist at all and we find the owner key to be
slotless, that should mean that we have a severe bug in the owner
implementation. The idea was that the owner key is stored in the user's key DB
on the user's encrypted partition.)

https://codereview.chromium.org/1128153003/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc
(right):

https://codereview.chromium.org/1128153003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:87:
public_slot.get());
On 2015/05/06 23:46:03, Ryan Sleevi wrote:
> This shouldn't have a private key. I have no clue what's going on here.
> pneubeck@, please tell me you can make sense of this :)

This was added by Chris Masone here https://codereview.chromium.org/660343002,
explicitly to find both legacy owner keys (stored in the software NSSDB) and new
owner keys (stored in Chaps/TPM).

Rational behind this was discussed in this thread:
https://code.google.com/p/chromium/issues/detail?id=396439

So, I think this is fine.

https://codereview.chromium.org/1128153003/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/platform_keys/platform_keys_nss.cc (right):

https://codereview.chromium.org/1128153003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys_nss.cc:404: if
(!state->slot_) {
On 2015/05/06 18:48:39, David Benjamin wrote:
> This shouldn't be possible, but if token_id is empty, DidGetCertDBOnIOThread
> intentionally leaves slot_ null. The only caller I found checks the token_id
> parameter early, but that's not locally clear.

That's right.
I'll create a separate CL that adds an additional sanity check of token_id in
GenerateRSAKey.

[davidben, 2015-05-07 20:51:44]

https://codereview.chromium.org/1128153003/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc
(right):

https://codereview.chromium.org/1128153003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc:74: }
On 2015/05/07 12:19:16, pneubeck wrote:
> On 2015/05/06 18:48:38, David Benjamin wrote:
> > I switched this to the slotless variant in case any existing code passed in
a
> > null slot and to avoid behavior differences.
> 
> I don't understand why all slots should be searched for the key?
> 
> Reading the above documentation, this function (GetPrivateKey...) should
search
> in a specific slot.
> If that slot is null, it's an error (handled in  SignDataOnWorkerThread) and
> should not silently return a slotless key.

Yeah, looking through the callers, there is a CHECK so we're fine.

> I don't know if NSS can have key pairs without assigned slot.
> As that is not obvious, please don't rely on it.
> 
> Instead, please do an early return instead at the beginning of the function.
> 
> (If something like slotless keys exist at all and we find the owner key to be
> slotless, that should mean that we have a severe bug in the owner
> implementation. The idea was that the owner key is stored in the user's key DB
> on the user's encrypted partition.)

I intentionally want to keep this logic as similar to the original as possible
for this CL (also why there are rsaKey checks everywhere). Note that the
original does FindFromPublicKeyInfo and then checks pkcs11Slot. I'm happy to do
a follow-up to switch that to the slotted version. But it should be separate so
it can be reverted separately.

https://codereview.chromium.org/1128153003/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc
(right):

https://codereview.chromium.org/1128153003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:87:
public_slot.get());
On 2015/05/07 12:19:16, pneubeck wrote:
> On 2015/05/06 23:46:03, Ryan Sleevi wrote:
> > This shouldn't have a private key. I have no clue what's going on here.
> > pneubeck@, please tell me you can make sense of this :)
> 
> This was added by Chris Masone here https://codereview.chromium.org/660343002,
> explicitly to find both legacy owner keys (stored in the software NSSDB) and
new
> owner keys (stored in Chaps/TPM).
> 
> Rational behind this was discussed in this thread:
> https://code.google.com/p/chromium/issues/detail?id=396439
> 
> So, I think this is fine.

Unfortunately, I had to switch this back anyway because the tests depended on it
being called with null slot. I left a TODO and moved the null check to
OwnerKeyUtilImpl.

https://codereview.chromium.org/1128153003/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/ownership/owner_settings_service_chromeos.h
(right):

https://codereview.chromium.org/1128153003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos.h:140: //
|private_key->key()| may be null.
On 2015/05/07 10:59:16, Dmitry Polukhin wrote:
> On 2015/05/06 18:48:39, David Benjamin wrote:
> > This is really weird and, tbh, I don't think it makes much sense. But
> > OwnerSettingsService::IsOwnerAsync actually assumes this with the
private_key_
> > check. I'm somewhat puzzled by that code but have left it alone for now. It
> > seems a boolean keypair_loaded_ would make much more sense, but I didn't
want
> to
> > make this CL grow in scope that much.
> > 
> > Plus there's failure paths where ReloadKeypairImpl gives back a null
> > scoped_refptr<PrivateKey> so that gets more confusing...
> > 
> > dpolukhin: I imagine this should get a TODO of some sort. What would you
like
> me
> > to write in there.
> 
> NULL private key means that active user is not an owner. But public key should
> be available. I'm not an original author of the code.

Right. But the question is whether this is signaled by |private_key| being null
or |private_key->key()| being null. Right now it's the latter. I think it's
overly confusing that you can have an existent PrivateKey with non-existent
RSAPrivateKey.

[Ryan Sleevi, 2015-05-08 00:11:52]

Thanks all for reminding me of the code that I suggested :P

While it's possible to have slotless SECKEYs, I don't think this code will touch
on it, and it certainly doesn't intend do (so as pneubeck said, if it does, it's
a bug)

The easy unlock stuff is sketchy - it sounds like Easy Unlock doesn't have a
good definition of where it's storing its keys, and that will be a challenge to
getting the Chimera build going. I think it's suitable to address that in
follow-up, because this 'simply' fixes it for the case of nullptr slots.

Overall, LGTM

https://codereview.chromium.org/1128153003/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc
(right):

https://codereview.chromium.org/1128153003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc:74: }
Seems like there should be a TODO here - |slot| should never be NULL, always
well-defined.

[pneubeck (no reviews), 2015-05-08 08:08:41]

lgtm

[Dmitry Polukhin, 2015-05-08 09:27:53]

lgtm

[davidben, 2015-05-09 00:41:54]

davidben@chromium.org changed reviewers:
+ erik@erikwright.com

[davidben, 2015-05-09 00:41:56]

+erikwright for components/ownership.gypi

> The easy unlock stuff is sketchy - it sounds like Easy Unlock doesn't have a
> good definition of where it's storing its keys, and that will be a challenge
to
> getting the Chimera build going. I think it's suitable to address that in
> follow-up, because this 'simply' fixes it for the case of nullptr slots.

Hrm? Slots and stuff don't really change for the chimera or anything.

https://codereview.chromium.org/1128153003/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc
(right):

https://codereview.chromium.org/1128153003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc:74: }
On 2015/05/08 00:11:52, Ryan Sleevi wrote:
> Seems like there should be a TODO here - |slot| should never be NULL, always
> well-defined.

Done. It's kinda a silly TODO, but I don't want to do it in the same CL, lest
NSS have some bizarre edge case I'm not aware of and I get the whole thing
reverted again, rather than just a part of it. :-) (The current code does this
bizarre pkcs11Slot != slot check, so this way the trace of calls into NSS can
stay the same.)

I'll upload the follow-up right after this.

[erikwright (departed), 2015-05-11 14:08:56]

erikwright@chromium.org changed reviewers:
+ erikwright@chromium.org
- erik@erikwright.com

[erikwright (departed), 2015-05-11 14:08:57]

Please add a new entry for this gypi file in components/OWNERS

LGTM.

[davidben, 2015-05-11 18:18:38]

On 2015/05/11 14:08:57, erikwright wrote:
> Please add a new entry for this gypi file in components/OWNERS

Did this in a separate CL: https://codereview.chromium.org/1123343006/

[davidben, 2015-05-11 18:18:58]

The CQ bit was checked by davidben@chromium.org

[davidben, 2015-05-11 18:18:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpolukhin@chromium.org, rsleevi@chromium.org, pneubeck@chromium.org
Link to the patchset: https://codereview.chromium.org/1128153003/#ps80001
(title: "Add TODO")

[commit-bot: I haz the power, 2015-05-11 18:24:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1128153003/80001

[commit-bot: I haz the power, 2015-05-11 18:43:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-11 18:43:36]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[davidben, 2015-05-11 19:26:46]

On 2015/05/11 18:43:36, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

Looks unrelated. Trying again.

[davidben, 2015-05-11 19:26:53]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2015-05-11 19:27:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1128153003/80001

[commit-bot: I haz the power, 2015-05-11 20:20:20]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2015-05-11 20:21:22]

Patchset 4 (id:??) landed as
https://crrev.com/85bad9e7d11461b7599967c9a504da9ac96f11b3
Cr-Commit-Position: refs/heads/master@{#329227}