Check kKeyInSoftware attribute for certs keys

chrome://settings/certificates should display hardware-backed for
a certificate if it is saved on a hardware-backed slot and its private
key is stored in the TPM.

CL:1967781 introduced kKeyInSoftware attribute for private key which can
be used to check whether the key is wrapped by the TPM or not.

This CL introduces:
1- Taking advantage of the kKeyInSoftware attribute to check if a
   private key is hardware-backed or not.
2- Adds support for requesting certificate information list from NSS
   certificate database. Gathering certificates information is done
   asynchronously.

1- net_unittests --gtest_filter=NSSCertDatabase*
2- unittests --gtest_filter=CertificateManagerModel*
3- Manually by checking that when importing and binding a client certificate with EC key on a TPM version 1.2, it doesn't display (hardware-backed) in chrome://certificate-manager beside the certificate anymore.

Bug: chromium:1043083
Test: 
Change-Id: I3b2551ae04d5ddadbee28cab823bcbb4278480be
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2022945
Commit-Queue: Omar Morsi <omorsi@google.com>
Reviewed-by: Matt Mueller <mattm@chromium.org>
Reviewed-by: Ryan Sleevi <rsleevi@chromium.org>
Reviewed-by: Pavol Marko <pmarko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#743043}

chrome/browser/certificate_manager_model.cc

@@ -220,35 +220,30 @@ class CertsSourcePlatformNSS : public CertificateManagerModel::CertsSource {
   void RefreshSlotsUnlocked() {
     DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
     DVLOG(1) << "refresh listing certs...";
-    cert_db_->ListCerts(base::BindOnce(&CertsSourcePlatformNSS::DidGetCerts,
-                                       weak_ptr_factory_.GetWeakPtr()));
+    cert_db_->ListCertsInfo(base::BindOnce(&CertsSourcePlatformNSS::DidGetCerts,
+                                           weak_ptr_factory_.GetWeakPtr()));
   }
 
-  void DidGetCerts(net::ScopedCERTCertificateList certs) {
+  void DidGetCerts(net::NSSCertDatabase::CertInfoList cert_info_list) {
     DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
     DVLOG(1) << "refresh finished for platform provided certificates";
+
     std::vector<std::unique_ptr<CertificateManagerModel::CertInfo>> cert_infos;
+    cert_infos.reserve(cert_info_list.size());
 
-    cert_infos.reserve(certs.size());
-    for (auto& cert : certs) {
-      net::CertType type = x509_certificate_model::GetType(cert.get());
-      bool can_be_deleted = !net::NSSCertDatabase::IsReadOnly(cert.get());
-      bool untrusted = net::NSSCertDatabase::IsUntrusted(cert.get());
-      bool hardware_backed = net::NSSCertDatabase::IsHardwareBacked(cert.get());
-      bool web_trust_anchor =
-          net::NSSCertDatabase::IsWebTrustAnchor(cert.get());
-      bool device_wide = false;
-#if defined(OS_CHROMEOS)
+    for (auto& cert_info : cert_info_list) {
+      net::CertType type =
+          x509_certificate_model::GetType(cert_info.cert.get());
+      bool can_be_deleted = !cert_info.on_read_only_slot;
+      bool hardware_backed = cert_info.hardware_backed;
+      base::string16 name = GetName(cert_info.cert.get(), hardware_backed);
 
-      device_wide = net::NSSCertDatabase::IsCertificateOnSlot(
-          /*cert=*/cert.get(),
-          /*slot=*/cert_db_->GetSystemSlot().get());
-#endif
-      base::string16 name = GetName(cert.get(), hardware_backed);
       cert_infos.push_back(std::make_unique<CertificateManagerModel::CertInfo>(
-          std::move(cert), type, name, can_be_deleted, untrusted,
-          CertificateManagerModel::CertInfo::Source::kPlatform,
-          web_trust_anchor, hardware_backed, device_wide));
+          /*cert=*/std::move(cert_info.cert), type, name, can_be_deleted,
+          /*untrusted=*/cert_info.untrusted,
+          /*source=*/CertificateManagerModel::CertInfo::Source::kPlatform,
+          /*web_trust_anchor=*/cert_info.web_trust_anchor, hardware_backed,
+          /*device_wide=*/cert_info.device_wide));
     }
 
     SetCertInfos(std::move(cert_infos));

crypto/nss_util_chromeos.cc

@@ -26,6 +26,7 @@
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/path_service.h"
+#include "base/strings/string_piece.h"
 #include "base/strings/stringprintf.h"
 #include "base/task/post_task.h"
 #include "base/threading/scoped_blocking_call.h"
@@ -608,4 +609,13 @@ void SetPrivateSoftwareSlotForChromeOSUserForTesting(ScopedPK11Slot slot) {
       std::move(slot));
 }
 
+bool IsSlotProvidedByChaps(PK11SlotInfo* slot) {
+  if (!slot)
+    return false;
+
+  SECMODModule* pk11_module = PK11_GetModule(slot);
+  return pk11_module && base::StringPiece(pk11_module->commonName) ==
+                            base::StringPiece(kChapsModuleName);
+}
+
 }  // namespace crypto

crypto/nss_util_internal.h

@@ -121,6 +121,9 @@ CRYPTO_EXPORT void CloseChromeOSUserForTesting(
 CRYPTO_EXPORT void SetPrivateSoftwareSlotForChromeOSUserForTesting(
     ScopedPK11Slot slot);
 
+// Returns true if chaps is the module to which |slot| is attached.
+CRYPTO_EXPORT bool IsSlotProvidedByChaps(PK11SlotInfo* slot);
+
 #endif  // defined(OS_CHROMEOS)
 
 // Loads the given module for this NSS session.

net/cert/nss_cert_database.cc

@@ -6,6 +6,7 @@
 
 #include <cert.h>
 #include <certdb.h>
+#include <dlfcn.h>
 #include <keyhi.h>
 #include <pk11pub.h>
 #include <secmod.h>
@@ -20,6 +21,7 @@
 #include "base/observer_list_threadsafe.h"
 #include "base/task/post_task.h"
 #include "base/threading/scoped_blocking_call.h"
+#include "crypto/nss_util_internal.h"
 #include "crypto/scoped_nss_types.h"
 #include "net/base/net_errors.h"
 #include "net/cert/cert_database.h"
@@ -35,6 +37,11 @@ namespace net {
 
 namespace {
 
+using PK11HasAttributeSetFunction = CK_BBOOL (*)(PK11SlotInfo* slot,
+                                                 CK_OBJECT_HANDLE id,
+                                                 CK_ATTRIBUTE_TYPE type,
+                                                 PRBool haslock);
+
 // TODO(pneubeck): Move this class out of NSSCertDatabase and to the caller of
 // the c'tor of NSSCertDatabase, see https://crbug.com/395983 .
 // Helper that observes events from the NSSCertDatabase and forwards them to
@@ -56,6 +63,12 @@ class CertNotificationForwarder : public NSSCertDatabase::Observer {
 
 }  // namespace
 
+NSSCertDatabase::CertInfo::CertInfo() = default;
+NSSCertDatabase::CertInfo::CertInfo(CertInfo&& other) = default;
+NSSCertDatabase::CertInfo::~CertInfo() = default;
+NSSCertDatabase::CertInfo& NSSCertDatabase::CertInfo::operator=(
+    NSSCertDatabase::CertInfo&& other) = default;
+
 NSSCertDatabase::ImportCertFailure::ImportCertFailure(
     ScopedCERTCertificate cert,
     int err)
@@ -103,6 +116,17 @@ void NSSCertDatabase::ListCertsInSlot(ListCertsCallback callback,
       std::move(callback));
 }
 
+void NSSCertDatabase::ListCertsInfo(ListCertsInfoCallback callback) {
+  base::PostTaskAndReplyWithResult(
+      FROM_HERE,
+      {base::ThreadPool(), base::MayBlock(),
+       base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN},
+      base::BindOnce(&NSSCertDatabase::ListCertsInfoImpl,
+                     /*slot=*/nullptr,
+                     /*add_certs_info=*/true),
+      std::move(callback));
+}
+
 #if defined(OS_CHROMEOS)
 crypto::ScopedPK11Slot NSSCertDatabase::GetSystemSlot() const {
   return crypto::ScopedPK11Slot();
@@ -411,7 +435,34 @@ bool NSSCertDatabase::IsReadOnly(const CERTCertificate* cert) {
 // static
 bool NSSCertDatabase::IsHardwareBacked(const CERTCertificate* cert) {
   PK11SlotInfo* slot = cert->slot;
-  return slot && PK11_IsHW(slot);
+  if (!slot || !PK11_IsHW(slot))
+    return false;
+
+#if defined(OS_CHROMEOS)
+  // Chaps announces PK11_IsHW(slot) for all slots. However, it is possible for
+  // a key in chaps to be not truly hardware-backed, either because it has been
+  // requested to be software-backed, or because the TPM does not support the
+  // key algorithm. Chaps sets kKeyInSoftware attribute to true for private keys
+  // not wrapped by the TPM.
+  if (crypto::IsSlotProvidedByChaps(slot)) {
+    static PK11HasAttributeSetFunction pk11_has_attribute_set =
+        reinterpret_cast<PK11HasAttributeSetFunction>(
+            dlsym(RTLD_DEFAULT, "PK11_HasAttributeSet"));
+    if (pk11_has_attribute_set) {
+      constexpr CK_ATTRIBUTE_TYPE kKeyInSoftware = CKA_VENDOR_DEFINED + 5;
+      SECKEYPrivateKey* private_key = PK11_FindPrivateKeyFromCert(
+          slot, const_cast<CERTCertificate*>(cert), nullptr);
+      // PK11_HasAttributeSet returns true if the object in the given slot has
+      // the attribute set to true. Otherwise it returns false.
+      if (private_key &&
+          pk11_has_attribute_set(slot, private_key->pkcs11ID, kKeyInSoftware,
+                                 /*haslock=*/PR_FALSE)) {
+        return false;
+      }
+    }
+  }
+#endif
+  return true;
 }
 
 void NSSCertDatabase::AddObserver(Observer* observer) {
@@ -422,17 +473,39 @@ void NSSCertDatabase::RemoveObserver(Observer* observer) {
   observer_list_->RemoveObserver(observer);
 }
 
+// static
+ScopedCERTCertificateList NSSCertDatabase::ExtractCertificates(
+    CertInfoList certs_info) {
+  ScopedCERTCertificateList certs;
+  certs.reserve(certs_info.size());
+
+  for (auto& cert_info : certs_info)
+    certs.push_back(std::move(cert_info.cert));
+
+  return certs;
+}
+
 // static
 ScopedCERTCertificateList NSSCertDatabase::ListCertsImpl(
     crypto::ScopedPK11Slot slot) {
+  CertInfoList certs_info =
+      ListCertsInfoImpl(std::move(slot), /*add_certs_info=*/false);
+
+  return ExtractCertificates(std::move(certs_info));
+}
+
+// static
+NSSCertDatabase::CertInfoList NSSCertDatabase::ListCertsInfoImpl(
+    crypto::ScopedPK11Slot slot,
+    bool add_certs_info) {
   // This method may acquire the NSS lock or reenter this code via extension
   // hooks (such as smart card UI). To ensure threads are not starved or
   // deadlocked, the base::ScopedBlockingCall below increments the thread pool
   // capacity if this method takes too much time to run.
   base::ScopedBlockingCall scoped_blocking_call(FROM_HERE,
                                                 base::BlockingType::MAY_BLOCK);
 
-  ScopedCERTCertificateList certs;
+  CertInfoList certs_info;
   CERTCertList* cert_list = nullptr;
   if (slot)
     cert_list = PK11_ListCertsInSlot(slot.get());
@@ -442,10 +515,20 @@ ScopedCERTCertificateList NSSCertDatabase::ListCertsImpl(
   CERTCertListNode* node;
   for (node = CERT_LIST_HEAD(cert_list); !CERT_LIST_END(node, cert_list);
        node = CERT_LIST_NEXT(node)) {
-    certs.push_back(x509_util::DupCERTCertificate(node->cert));
+    CertInfo cert_info;
+    cert_info.cert = x509_util::DupCERTCertificate(node->cert);
+
+    if (add_certs_info) {
+      cert_info.on_read_only_slot = IsReadOnly(cert_info.cert.get());
+      cert_info.untrusted = IsUntrusted(cert_info.cert.get());
+      cert_info.web_trust_anchor = IsWebTrustAnchor(cert_info.cert.get());
+      cert_info.hardware_backed = IsHardwareBacked(cert_info.cert.get());
+    }
+
+    certs_info.push_back(std::move(cert_info));
   }
   CERT_DestroyCertList(cert_list);
-  return certs;
+  return certs_info;
 }
 
 void NSSCertDatabase::NotifyCertRemovalAndCallBack(DeleteCertCallback callback,

net/cert/nss_cert_database.h

@@ -50,6 +50,34 @@ class NET_EXPORT NSSCertDatabase {
     DISALLOW_COPY_AND_ASSIGN(Observer);
   };
 
+  // Holds an NSS certificate along with additional information.
+  struct CertInfo {
+    CertInfo();
+    CertInfo(CertInfo&& other);
+    ~CertInfo();
+    CertInfo& operator=(CertInfo&& other);
+
+    // The certificate itself.
+    ScopedCERTCertificate cert;
+
+    // The certificate is stored on a read-only slot.
+    bool on_read_only_slot = false;
+
+    // The certificate is untrusted.
+    bool untrusted = false;
+
+    // The certificate is trusted for web navigations according to the trust
+    // bits stored in the database.
+    bool web_trust_anchor = false;
+
+    // The certificate is hardware-backed.
+    bool hardware_backed = false;
+
+    // The certificate is device-wide.
+    // Note: can be true only on Chrome OS.
+    bool device_wide = false;
+  };
+
   // Stores per-certificate error codes for import failures.
   struct NET_EXPORT ImportCertFailure {
    public:
@@ -88,6 +116,11 @@ class NET_EXPORT NSSCertDatabase {
     DISTRUSTED_OBJ_SIGN   = 1 << 5,
   };
 
+  using CertInfoList = std::vector<CertInfo>;
+
+  using ListCertsInfoCallback =
+      base::OnceCallback<void(CertInfoList certs_info)>;
+
   using ListCertsCallback =
       base::OnceCallback<void(ScopedCERTCertificateList certs)>;
 
@@ -112,12 +145,16 @@ class NET_EXPORT NSSCertDatabase {
   virtual void ListCerts(ListCertsCallback callback);
 
   // Get a list of certificates in the certificate database of the given slot.
-  // Note that the callback may be run even after the database is deleted.
-  // Must be called on the IO thread and it calls |callback| on the IO thread.
-  // This does not block by retrieving the certs asynchronously on a worker
-  // thread. Never calls |callback| synchronously.
+  // Note that the callback may be run even after the database is deleted. Must
+  // be called on the IO thread. This does not block by retrieving the certs
+  // asynchronously on a worker thread.
   virtual void ListCertsInSlot(ListCertsCallback callback, PK11SlotInfo* slot);
 
+  // Asynchronously get a list of certificates along with additional
+  // information. Note that the callback may be run even after the database is
+  // deleted.
+  virtual void ListCertsInfo(ListCertsInfoCallback callback);
+
 #if defined(OS_CHROMEOS)
   // Get the slot for system-wide key data. May be NULL if the system token was
   // not explicitly set.
@@ -231,14 +268,30 @@ class NET_EXPORT NSSCertDatabase {
   static bool IsReadOnly(const CERTCertificate* cert);
 
   // Check whether cert is stored in a hardware slot.
+  // This should only be invoked on a worker thread due to expensive operations
+  // behind it.
   static bool IsHardwareBacked(const CERTCertificate* cert);
 
  protected:
+  // Returns a list of certificates extracted from |certs_info| list ignoring
+  // additional information.
+  static ScopedCERTCertificateList ExtractCertificates(CertInfoList certs_info);
+
   // Certificate listing implementation used by |ListCerts*|. Static so it may
   // safely be used on the worker thread. If |slot| is nullptr, obtains the
   // certs of all slots, otherwise only of |slot|.
   static ScopedCERTCertificateList ListCertsImpl(crypto::ScopedPK11Slot slot);
 
+  // Implements the logic behind returning a list of certificates along with
+  // additional information about every certificate.
+  // If |add_certs_info| is false, doesn't compute the certificate additional
+  // information, the corresponding CertInfo struct fields will be left on their
+  // default values.
+  // Static so it may safely be used on the worker thread. If |slot| is nullptr,
+  // obtains the certs of all slots, otherwise only of |slot|.
+  static CertInfoList ListCertsInfoImpl(crypto::ScopedPK11Slot slot,
+                                        bool add_certs_info);
+
   // Broadcasts notifications to all registered observers.
   void NotifyObserversCertDBChanged();