ChromeOS: Fix crash if login profile triggers client auth.

ChromeOS: Fix crash if login profile triggers client auth.

The login profile (which is identified with an empty username_hash) does not have an entry in the chromeos_user_map_, which would cause a crash (or DCHECK) when GetPrivateSlotForChromeOSUser was called. GetPrivateSlotForChromeOSUser is changed to return a NULL slot handle for this case.

Updates NSSProfileFilterChromeOS to allow NULL slot handles, which it will now receive due to the above change. 

BUG=331945, 302125
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=244690

[Ryan Sleevi, 2014-01-11 02:56:12]

I'm having a bit of trouble following the why of this CL.

Is it because the username hash is empty? Is it because no slots match the
filter? The changes are not entirely clear to me why/how this fixes the issue -
or what the root issue is.

I would encourage a better commit description for this purpose (rather than as a
draft note)

https://codereview.chromium.org/123633002/diff/30001/net/cert/nss_profile_fil...
File net/cert/nss_profile_filter_chromeos.cc (right):

https://codereview.chromium.org/123633002/diff/30001/net/cert/nss_profile_fil...
net/cert/nss_profile_filter_chromeos.cc:52: slot ==
crypto::ScopedPK11Slot(PK11_GetInternalSlot()))
Why not PK11_IsInternal?

https://codereview.chromium.org/123633002/diff/30001/net/cert/nss_profile_fil...
net/cert/nss_profile_filter_chromeos.cc:60: return true;
This is so that roots display in the UI, right?

Why not white list this first - PK11_IsInternalKeySlot(slot)

[mattm, 2014-01-11 03:23:23]

updated description.

https://codereview.chromium.org/123633002/diff/30001/net/cert/nss_profile_fil...
File net/cert/nss_profile_filter_chromeos.cc (right):

https://codereview.chromium.org/123633002/diff/30001/net/cert/nss_profile_fil...
net/cert/nss_profile_filter_chromeos.cc:52: slot ==
crypto::ScopedPK11Slot(PK11_GetInternalSlot()))
On 2014/01/11 02:56:13, Ryan Sleevi wrote:
> Why not PK11_IsInternal?

PK11_IsInternal matches any of the nss software slots.

https://codereview.chromium.org/123633002/diff/30001/net/cert/nss_profile_fil...
net/cert/nss_profile_filter_chromeos.cc:60: return true;
On 2014/01/11 02:56:13, Ryan Sleevi wrote:
> This is so that roots display in the UI, right?

Yeah.

> Why not white list this first - PK11_IsInternalKeySlot(slot) 

Just that it normally already gets handled by the final test, so this avoids a
bit of redundancy. I could move it there if it seems cleaner.

[mattm, 2014-01-11 03:23:23]

updated description.

https://codereview.chromium.org/123633002/diff/30001/net/cert/nss_profile_fil...
File net/cert/nss_profile_filter_chromeos.cc (right):

https://codereview.chromium.org/123633002/diff/30001/net/cert/nss_profile_fil...
net/cert/nss_profile_filter_chromeos.cc:52: slot ==
crypto::ScopedPK11Slot(PK11_GetInternalSlot()))
On 2014/01/11 02:56:13, Ryan Sleevi wrote:
> Why not PK11_IsInternal?

PK11_IsInternal matches any of the nss software slots.

https://codereview.chromium.org/123633002/diff/30001/net/cert/nss_profile_fil...
net/cert/nss_profile_filter_chromeos.cc:60: return true;
On 2014/01/11 02:56:13, Ryan Sleevi wrote:
> This is so that roots display in the UI, right?

Yeah.

> Why not white list this first - PK11_IsInternalKeySlot(slot) 

Just that it normally already gets handled by the final test, so this avoids a
bit of redundancy. I could move it there if it seems cleaner.

[Ryan Sleevi, 2014-01-11 03:36:14]

https://codereview.chromium.org/123633002/diff/30001/net/cert/nss_profile_fil...
File net/cert/nss_profile_filter_chromeos.cc (right):

https://codereview.chromium.org/123633002/diff/30001/net/cert/nss_profile_fil...
net/cert/nss_profile_filter_chromeos.cc:52: slot ==
crypto::ScopedPK11Slot(PK11_GetInternalSlot()))
On 2014/01/11 03:23:23, mattm wrote:
> On 2014/01/11 02:56:13, Ryan Sleevi wrote:
> > Why not PK11_IsInternal?
> 
> PK11_IsInternal matches any of the nss software slots.

Yeah, but I thought they showed up as removable slots (PK11_IsRemovable)

https://codereview.chromium.org/123633002/diff/30001/net/cert/nss_profile_fil...
net/cert/nss_profile_filter_chromeos.cc:60: return true;
On 2014/01/11 03:23:23, mattm wrote:
> On 2014/01/11 02:56:13, Ryan Sleevi wrote:
> > This is so that roots display in the UI, right?
> 
> Yeah.
> 
> > Why not white list this first - PK11_IsInternalKeySlot(slot) 
> 
> Just that it normally already gets handled by the final test, so this avoids a
> bit of redundancy. I could move it there if it seems cleaner.

I was going to suggest moving it before / outside of the if (eg: line 54). It's
redundant to the check on 66/67, but it doesn't hurt to short-circuit and be
explicit

https://codereview.chromium.org/123633002/diff/30001/net/cert/nss_profile_fil...
File net/cert/nss_profile_filter_chromeos_unittest.cc (right):

https://codereview.chromium.org/123633002/diff/30001/net/cert/nss_profile_fil...
net/cert/nss_profile_filter_chromeos_unittest.cc:22: int slot_count =
item->module->loaded ? item->module->slotCount : 0;
when would ->loaded be 0?

I just compare with
http://mxr.mozilla.org/nss/source/lib/pk11wrap/pk11slot.c#471 which doesn't seem
to be doing the same test. And is using PK11_IsPresent

https://codereview.chromium.org/123633002/diff/30001/net/cert/nss_profile_fil...
net/cert/nss_profile_filter_chromeos_unittest.cc:25: if
(PK11_HasRootCerts(slot))
Note, > 1 slot can have root certs, fwiw.

[mattm, 2014-01-14 02:29:05]

https://codereview.chromium.org/123633002/diff/30001/net/cert/nss_profile_fil...
File net/cert/nss_profile_filter_chromeos.cc (right):

https://codereview.chromium.org/123633002/diff/30001/net/cert/nss_profile_fil...
net/cert/nss_profile_filter_chromeos.cc:52: slot ==
crypto::ScopedPK11Slot(PK11_GetInternalSlot()))
On 2014/01/11 03:36:14, Ryan Sleevi wrote:
> On 2014/01/11 03:23:23, mattm wrote:
> > On 2014/01/11 02:56:13, Ryan Sleevi wrote:
> > > Why not PK11_IsInternal?
> > 
> > PK11_IsInternal matches any of the nss software slots.
> 
> Yeah, but I thought they showed up as removable slots (PK11_IsRemovable)

Ok, changed to use PK11_IsInternal && !PK11_IsRemovable

https://codereview.chromium.org/123633002/diff/30001/net/cert/nss_profile_fil...
net/cert/nss_profile_filter_chromeos.cc:60: return true;
On 2014/01/11 03:36:14, Ryan Sleevi wrote:
> On 2014/01/11 03:23:23, mattm wrote:
> > On 2014/01/11 02:56:13, Ryan Sleevi wrote:
> > > This is so that roots display in the UI, right?
> > 
> > Yeah.
> > 
> > > Why not white list this first - PK11_IsInternalKeySlot(slot) 
> > 
> > Just that it normally already gets handled by the final test, so this avoids
a
> > bit of redundancy. I could move it there if it seems cleaner.
> 
> I was going to suggest moving it before / outside of the if (eg: line 54).
It's
> redundant to the check on 66/67, but it doesn't hurt to short-circuit and be
> explicit

Done.

https://codereview.chromium.org/123633002/diff/30001/net/cert/nss_profile_fil...
File net/cert/nss_profile_filter_chromeos_unittest.cc (right):

https://codereview.chromium.org/123633002/diff/30001/net/cert/nss_profile_fil...
net/cert/nss_profile_filter_chromeos_unittest.cc:22: int slot_count =
item->module->loaded ? item->module->slotCount : 0;
On 2014/01/11 03:36:14, Ryan Sleevi wrote:
> when would ->loaded be 0?
> 
> I just compare with
> http://mxr.mozilla.org/nss/source/lib/pk11wrap/pk11slot.c#471 which doesn't
seem
> to be doing the same test.

I was just copying that from somewhere.. Ex:
https://code.google.com/p/chromium/codesearch#chromium/src/crypto/rsa_private...


> And is using PK11_IsPresent

Added

https://codereview.chromium.org/123633002/diff/30001/net/cert/nss_profile_fil...
net/cert/nss_profile_filter_chromeos_unittest.cc:25: if
(PK11_HasRootCerts(slot))
On 2014/01/11 03:36:14, Ryan Sleevi wrote:
> Note, > 1 slot can have root certs, fwiw.

Should be okay for the test's purposes, though, right?

[Ryan Sleevi, 2014-01-14 03:45:42]

lgtm

[commit-bot: I haz the power, 2014-01-14 04:53:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mattm@chromium.org/123633002/180001

[commit-bot: I haz the power, 2014-01-14 07:35:24]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2014-01-14 08:31:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mattm@chromium.org/123633002/380001

[commit-bot: I haz the power, 2014-01-14 14:00:48]

Change committed as 244690