Forces TPM slot to be "Friendly", allowing NSS to avoid locking

Forces TPM slot to be "Friendly", allowing NSS to avoid locking

Also added VLOG(1) logging for PKCS11 slot info.

BUG=chromium:118206
TEST=Ran on device, tried to repro bug, and was unable to.  Confirmed that friendly bit was set (based on log output).

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=130474

[Ryan Sleevi, 2012-03-30 18:29:26]

https://chromiumcodereview.appspot.com/9969019/diff/1/crypto/nss_util.cc
File crypto/nss_util.cc (right):

https://chromiumcodereview.appspot.com/9969019/diff/1/crypto/nss_util.cc#newc...
crypto/nss_util.cc:570: "trustOrder=100
slotParams=(1={slotFlags=[RSA,PublicCerts] "
I believe you should update the 1= to 0=, based on your PK11_GetSlotID results

[Ryan Sleevi, 2012-03-30 21:05:47]

http://codereview.chromium.org/9969019/diff/1/crypto/nss_util.cc
File crypto/nss_util.cc (right):

http://codereview.chromium.org/9969019/diff/1/crypto/nss_util.cc#newcode571
crypto/nss_util.cc:571: "askpw=only})");
One more thing to try here:

This will help determine how/why these flags may not be getting passed. This is
a short-term diagnostic step but should make it easier to debug.

if (chaps_module_ && chaps_module_->loaded) {
  int size = 0;
  PK11DefaultArrayEntry* entries = PK11_GetDefaultArrayEntry(&size);
  PK11DefaultArrayEntry* friendly_entry = NULL;
  for (int i = 0; i < size; ++i) {
    if (entries[i]->flag == SECMOD_FRIENDLY_FLAG) {
      friendly_entry = entries[i];
      break;
    }
  }
  for (int i = 0; i < chaps_module_->slotCount; ++i) {
    PK11SlotInfo* slot = chaps_module_->slots[i];
    // Force all slots in the TPM module to be friendly
    if (slot)
      PK11_UpdateSlotAttribute(slot, friendly_entry, PR_TRUE);
  }
}
LogSlotInfo();

[Ryan Sleevi, 2012-03-30 21:07:13]

http://codereview.chromium.org/9969019/diff/1/crypto/nss_util.cc
File crypto/nss_util.cc (right):

http://codereview.chromium.org/9969019/diff/1/crypto/nss_util.cc#newcode571
crypto/nss_util.cc:571: "askpw=only})");
On 2012/03/30 21:05:48, Ryan Sleevi wrote:
>   for (int i = 0; i < chaps_module_->slotCount; ++i) {

Sorry, that should be:
for (int i = 0; friendly_entry && i < chaps_module_->slotCount; ++i) {

Although even a crash would be nice here :)

[Greg Spencer (Chromium), 2012-03-31 00:09:15]

On 2012/03/30 21:07:13, Ryan Sleevi wrote:
> http://codereview.chromium.org/9969019/diff/1/crypto/nss_util.cc
> File crypto/nss_util.cc (right):
> 
> http://codereview.chromium.org/9969019/diff/1/crypto/nss_util.cc#newcode571
> crypto/nss_util.cc:571: "askpw=only})");
> On 2012/03/30 21:05:48, Ryan Sleevi wrote:
> >   for (int i = 0; i < chaps_module_->slotCount; ++i) {
> 
> Sorry, that should be:
> for (int i = 0; friendly_entry && i < chaps_module_->slotCount; ++i) {
> 
> Although even a crash would be nice here :)

OK, I was able to successfully turn on the friendly flag using that snippet. 
Now I need to know how to repro so we can call it a "fix". No idea why it's
ignoring the options, but we can tackle that after we have a fix.

[oshima, 2012-03-31 05:07:08]

great, can you upload new patch? I'll try it on monday.

- oshima

On Fri, Mar 30, 2012 at 5:09 PM, <gspencer@chromium.org> wrote:

> On 2012/03/30 21:07:13, Ryan Sleevi wrote:
>
>>
http://codereview.chromium.**org/9969019/diff/1/crypto/nss_**util.cc<http://c...
>> File crypto/nss_util.cc (right):
>>
>
>  http://codereview.chromium.**org/9969019/diff/1/crypto/nss_**
>>
util.cc#newcode571<http://codereview.chromium.org/9969019/diff/1/crypto/nss_util.cc#newcode571>
>> crypto/nss_util.cc:571: "askpw=only})");
>> On 2012/03/30 21:05:48, Ryan Sleevi wrote:
>> >   for (int i = 0; i < chaps_module_->slotCount; ++i) {
>>
>
>  Sorry, that should be:
>> for (int i = 0; friendly_entry && i < chaps_module_->slotCount; ++i) {
>>
>
>  Although even a crash would be nice here :)
>>
>
> OK, I was able to successfully turn on the friendly flag using that
> snippet.
> Now I need to know how to repro so we can call it a "fix". No idea why it's
> ignoring the options, but we can tackle that after we have a fix.
>
>
https://chromiumcodereview.**appspot.com/9969019/<https://chromiumcodereview....
>

[Greg Spencer (Chromium), 2012-04-02 14:24:51]

On 2012/03/31 05:07:08, oshima wrote:
> great, can you upload new patch? I'll try it on monday.

OK, I uploaded it to this CL:

https://chromiumcodereview.appspot.com/9969019

[Greg Spencer (Chromium), 2012-04-02 21:45:39]

OK, this should be ready to submit for a fix.  I've updated the original patch
to use VLOG(1) instead of LOG(ERROR), and narrowed the bit setting to just set
it on the slot that we think is the TPM instead of iterating through all of
them.

Oshima, could you please test this again on the device for me?  I'm WAH today
and won't be able to test it directly until tomorrow (but it can wait until then
if you're busy).

[oshima, 2012-04-02 21:50:30]

On 2012/04/02 21:45:39, Greg Spencer (Chromium) wrote:
> OK, this should be ready to submit for a fix.  I've updated the original patch
> to use VLOG(1) instead of LOG(ERROR), and narrowed the bit setting to just set
> it on the slot that we think is the TPM instead of iterating through all of
> them.
> 
> Oshima, could you please test this again on the device for me?  I'm WAH today
> and won't be able to test it directly until tomorrow (but it can wait until
then
> if you're busy).

sure, testing it now.

[Greg Spencer (Chromium), 2012-04-02 23:22:56]

On 2012/04/02 21:50:30, oshima wrote:
> sure, testing it now.

How'd it go?  I'd like to try and get this in the CQ today.

[oshima, 2012-04-02 23:47:27]

LGTM

Sorry I had local build issue and had to rebuild a few times.

On 2012/04/02 23:22:56, Greg Spencer (Chromium) wrote:
> On 2012/04/02 21:50:30, oshima wrote:
> > sure, testing it now.
> 
> How'd it go?  I'd like to try and get this in the CQ today.

[wtc, 2012-04-03 00:25:49]

Review comments on patch set 3:

I only reviewed the NSS PKCS #11 module spec string.

http://codereview.chromium.org/9969019/diff/5001/crypto/nss_util.cc
File crypto/nss_util.cc (right):

http://codereview.chromium.org/9969019/diff/5001/crypto/nss_util.cc#newcode574
crypto/nss_util.cc:574: "trustOrder=100
slotParams=(0={slotFlags=[RSA,PublicCerts],"

Please try replacing the last comma (right before the closing
quote) with a space.  The NSS PKCS #11 Module Specs page says:
  
  slotParams - space separated list of name/value pairs
  where the name is a slotID and the value is a space
                                                ^^^^^
  separated list of parameters related to that slotID.
  ^^^^^^^^^

So we should use a space instead of a comma there.

Why did you change the slotID from 1= to 0= ?

[Greg Spencer (Chromium), 2012-04-03 00:28:50]

On 2012/04/03 00:25:49, wtc wrote:
> So we should use a space instead of a comma there.

So, it was originally  a space, and I tried adding PublicCerts before switching
it to a comma to see if it worked.

> Why did you change the slotID from 1= to 0= ?

Because the output from the logging showed the TPM slot as slot #0.

[Greg Spencer (Chromium), 2012-04-03 00:30:11]

On 2012/04/03 00:28:50, Greg Spencer (Chromium) wrote:
> On 2012/04/03 00:25:49, wtc wrote:
> > So we should use a space instead of a comma there.
> 
> So, it was originally  a space, and I tried adding PublicCerts before
switching
> it to a comma to see if it worked.

...and it didn't work with the space, and I thought "Hmm, I wonder if they want
a comma there?" and tried that.  I'll switch it back to a space, however, since
you say that's the right syntax.

[Greg Spencer (Chromium), 2012-04-03 02:21:13]

rsleevi and/or wtc, can one of you give an OWNERS review for this CL so I can
commit it?

I'm not concerned with getting the option string right at the moment (and if you
want, I'd be happy just reverting those changes, since they seem to be ignored
anyhow).  I just want to get the fix in, and then we can figure out why the
options aren't working.

[Ryan Sleevi, 2012-04-03 02:33:28]

Two nits:
- I'd be inclined to say revert the options string, just to be sure there are no
(other) side-effects.
- Is the logging aspect still necessary? I don't feel strongly about this, but
it seems like it doesn't need to land now.

However, LGTM whether you address those nits or not.

[Greg Spencer (Chromium), 2012-04-03 02:49:48]

On 2012/04/03 02:33:28, Ryan Sleevi wrote:
> - I'd be inclined to say revert the options string, just to be sure there are
no
> (other) side-effects.

OK, I'll revert that bit.

> - Is the logging aspect still necessary? I don't feel strongly about this, but
> it seems like it doesn't need to land now.

Yeah, I think I'll leave this in because it only fires when VLOG is 1 (which it
isn't by default: you have to give a flag to chrome), and because we'll need it
again when we test to make sure the options are working after we fix them.

[commit-bot: I haz the power, 2012-04-03 17:13:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gspencer@chromium.org/9969019/8

[commit-bot: I haz the power, 2012-04-03 18:03:43]

Try job failure for 9969019-8 (retry) on win for step "compile" (clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[wtc, 2012-04-03 19:27:26]

Patch set 5 LGTM.  Thanks!

http://codereview.chromium.org/9969019/diff/8/crypto/nss_util.cc
File crypto/nss_util.cc (right):

http://codereview.chromium.org/9969019/diff/8/crypto/nss_util.cc#newcode576
crypto/nss_util.cc:576: PK11DefaultArrayEntry* entries =
PK11_GetDefaultArray(&size);

It would be a good idea to document where this cryptic code
comes from.  (I think it comes from the NSS command-line tool
'modutil'.)

http://codereview.chromium.org/9969019/diff/8/crypto/nss_util.cc#newcode591
crypto/nss_util.cc:591: // "PublicCerts" above, and otherwise NSS does some
unnecessary locking,

Nit: this comment still references "PublicCerts".  I think
you can clarify it by mentioning the LoadModule() call.

[wtc, 2012-04-03 19:56:55]

Patch set 5 LGTM.

gspencer: I have a question for you.

http://codereview.chromium.org/9969019/diff/8/crypto/nss_util.cc
File crypto/nss_util.cc (right):

http://codereview.chromium.org/9969019/diff/8/crypto/nss_util.cc#newcode572
crypto/nss_util.cc:572: "trustOrder=100 slotParams=(1={slotFlags=[RSA]
askpw=only})");

I can now prove that adding the SECMOD_LoadUserModule function
called by LoadModule won't add SECMOD_FRIENDLY_FLAG to the NSS
slot structure.  I don't know how to fix it yet, but this
confirms the code that rsleevi gave you is necessary.

Did you open a new bug report for this issue?  I can enter
more info there.

[Greg Spencer (Chromium), 2012-04-03 20:46:35]

On 2012/04/03 19:56:55, wtc wrote:
> Did you open a new bug report for this issue?  I can enter
> more info there.

Yes, this is the bug: http://crosbug.com/28842
I've CC'd you on it.

[wtc, 2012-04-03 20:47:27]

http://codereview.chromium.org/9969019/diff/8/crypto/nss_util.cc
File crypto/nss_util.cc (right):

http://codereview.chromium.org/9969019/diff/8/crypto/nss_util.cc#newcode572
crypto/nss_util.cc:572: "trustOrder=100 slotParams=(1={slotFlags=[RSA]
askpw=only})");

On 2012/04/03 19:56:55, wtc wrote:
> 
> I can now prove that adding the SECMOD_LoadUserModule function
> called by LoadModule won't add SECMOD_FRIENDLY_FLAG to the NSS
> slot structure.

rsleevi showed that my proof is wrong because I missed one
code path.  So please ignore my declaration of victory.
We still don't know why this doesn't work.

[Ryan Sleevi, 2012-04-03 21:57:14]

I believe I've identified the root cause, see below.

Subtlety, thy name is NSS

http://codereview.chromium.org/9969019/diff/8/crypto/nss_util.cc
File crypto/nss_util.cc (right):

http://codereview.chromium.org/9969019/diff/8/crypto/nss_util.cc#newcode572
crypto/nss_util.cc:572: "trustOrder=100 slotParams=(1={slotFlags=[RSA]
askpw=only})");
This should be
NSS=\"trustOrder=100 slotParams=(1={slotFlags=[RSA] askpw=only})\"

As per https://developer.mozilla.org/en/PKCS11_Module_Specs, the NSS parameters
should be wrapped in a NSS=quoted-string.

Doing this properly causes SECMOD_LoadUserModule to recognize these flags.

[wtc, 2012-04-03 22:17:24]

http://codereview.chromium.org/9969019/diff/8/crypto/nss_util.cc
File crypto/nss_util.cc (right):

http://codereview.chromium.org/9969019/diff/8/crypto/nss_util.cc#newcode572
crypto/nss_util.cc:572: "trustOrder=100 slotParams=(1={slotFlags=[RSA]
askpw=only})");

rsleevi: good job!  Thank you for tracking this down.

I also noticed the lack of NSS= in our module spec
string, but I wasn't sure if the NSS documentation was
talking about some higher-level string that required NSS=.

[Greg Spencer (Chromium), 2012-04-03 22:21:50]

Great!  It seems pretty non-intuitive to need to have this, but hey, if it
works...

rsleevi, Since this change has already landed, do you want to make the patch to
switch us back to the options string?  Or shall I?

(My plan is to merge this "workaround" patch to R19, and leave the "real fix"
patch on ToT for R20.)

[Ryan Sleevi, 2012-04-03 22:28:36]

On 2012/04/03 22:21:50, Greg Spencer (Chromium) wrote:
> Great!  It seems pretty non-intuitive to need to have this, but hey, if it
> works...
> 
> rsleevi, Since this change has already landed, do you want to make the patch
to
> switch us back to the options string?  Or shall I?

I suppose it doesn't matter one way or the other, since I guess oshima's been
testing the CLs, so I'll send one up. 

> 
> (My plan is to merge this "workaround" patch to R19, and leave the "real fix"
> patch on ToT for R20.)

I'd like to get the real fix in R19, unless you know of a compelling reason why
we wouldn't want to?

[Greg Spencer (Chromium), 2012-04-03 22:36:44]

On Tue, Apr 3, 2012 at 3:28 PM, <rsleevi@chromium.org> wrote:
>
> I suppose it doesn't matter one way or the other, since I guess oshima's
> been
> testing the CLs, so I'll send one up.


Actually, I went ahead and have a straw man here, but I haven't tested it
yet (building now).
https://chromiumcodereview.appspot.com/9969132/


> I'd like to get the real fix in R19, unless you know of a compelling
> reason why
> we wouldn't want to?
>
>
Only because getting changes in has been problematic today, and the bug
that needs fixing is ReleaseBlock-Beta, which they want done by 7pm
tonight, and in order to get it in, you'd have to merge both mine and yours
into R19 because of the conflict.

[Greg Spencer (Chromium), 2012-04-03 22:38:33]

On 2012/04/03 22:36:44, Greg Spencer (Chromium) wrote:
> On Tue, Apr 3, 2012 at 3:28 PM, <mailto:rsleevi@chromium.org> wrote:
> >
> > I suppose it doesn't matter one way or the other, since I guess oshima's
> > been
> > testing the CLs, so I'll send one up.
> 
> 
> Actually, I went ahead and have a straw man here, but I haven't tested it
> yet (building now).
> https://chromiumcodereview.appspot.com/9969132/
> 
> 
> > I'd like to get the real fix in R19, unless you know of a compelling
> > reason why
> > we wouldn't want to?
> >
> >
> Only because getting changes in has been problematic today, and the bug
> that needs fixing is ReleaseBlock-Beta, which they want done by 7pm
> tonight, and in order to get it in, you'd have to merge both mine and yours
> into R19 because of the conflict.

Actually, I guess there wouldn't *have* to be a conflict.  I was thinking we'd
have to remove the other code, but you wouldn't need to do that in your change.

[wtc, 2012-04-03 23:00:58]

I recommend using only slotFlags=[PublicCerts] askpw=only
(formatted properly).  I explain why trustOrder=100 and
slotFlags=RSA should be removed below.

http://codereview.chromium.org/9969019/diff/8/crypto/nss_util.cc
File crypto/nss_util.cc (right):

http://codereview.chromium.org/9969019/diff/8/crypto/nss_util.cc#newcode568
crypto/nss_util.cc:568: //   trusted slot for the mechanisms it provides.

This comment is incorrect.  'trustOrder' is defined as:
  trustOrder - integer value specifying the order in
  which the trust information for certificates specified
  by tokens on this PKCS #11 library should be rolled up.
  '0' means that tokens on this library should not supply
  trust information (default). The relative order of two
  pkcs#11 libraries which have the same trustOrder value
  is undefined.

I doubt the CHAPS module provides certificate trust
information, so I recommend removing trustOrder=100.

I suspect what you intended may be cipherOrder.  In
any case, I recommend not specifying cipherOrder unless
we know it's necessary.

slotFlags=RSA means "This token should be used for all RSA
operations (other than Private key operations where the key
lives in another token)".  This doesn't seem appropriate for
the CHAPS module.