ChromeOS: Fix crash if login profile triggers client auth.

net/cert/nss_profile_filter_chromeos.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/cert/nss_profile_filter_chromeos.h"
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/strings/stringprintf.h"
 
@@ skipping 29 matching lines @@
 void NSSProfileFilterChromeOS::Init(crypto::ScopedPK11Slot public_slot,
                                     crypto::ScopedPK11Slot private_slot) {
   public_slot_ = public_slot.Pass();
   private_slot_ = private_slot.Pass();
 }
 
 bool NSSProfileFilterChromeOS::IsModuleAllowed(PK11SlotInfo* slot) const {
   // If this is one of the public/private slots for this profile, allow it.
   if (slot == public_slot_.get() || slot == private_slot_.get())
     return true;
-  // If it's from the read-only slot, allow it.
-  if (PK11_IsInternalKeySlot(slot))
+  // If it's from the read-only slots, allow it.
+  if (PK11_IsInternalKeySlot(slot) ||
+      slot == crypto::ScopedPK11Slot(PK11_GetInternalSlot()))

[Ryan Sleevi, 2014/01/11 02:56:13]

Why not PK11_IsInternal?

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

PK11_IsInternal matches any of the nss software slots.

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

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

[mattm, 2014/01/14 02:29:06]

Ok, changed to use PK11_IsInternal && !PK11_IsRemovable

     return true;
+  // If |public_slot_| or |private_slot_| is null, there isn't a way to get the
+  // modules to use in the final test.
+  if (!public_slot_.get() || !private_slot_.get()) {
+    // Allow the root certs module which would normally be allowed by the final
+    // test.
+    if (PK11_HasRootCerts(slot))
+      return true;

[Ryan Sleevi, 2014/01/11 02:56:13]

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]

Yeah.
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]

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

[mattm, 2014/01/14 02:29:06]

Done.

+    return false;
+  }
   // If this is not the internal (file-system) module or the TPM module, allow
   // it.
   SECMODModule* module_for_slot = PK11_GetModule(slot);
   if (module_for_slot != PK11_GetModule(public_slot_.get()) &&
       module_for_slot != PK11_GetModule(private_slot_.get()))
     return true;
   return false;
 }
 
 bool NSSProfileFilterChromeOS::IsCertAllowed(
@@ skipping 35 matching lines @@
     ModuleNotAllowedForProfilePredicate(const NSSProfileFilterChromeOS& filter)
     : filter_(filter) {}
 
 bool NSSProfileFilterChromeOS::ModuleNotAllowedForProfilePredicate::operator()(
     const scoped_refptr<CryptoModule>& module) const {
   return !filter_.IsModuleAllowed(module->os_module_handle());
 }
 
 }  // namespace net