Fix returning keys belonging to daemons

Before this CL, PlatformKeysService::GetAllKeys might return keys that
belong to the arc-oemcrypto daemon. These are now being explicitly left
out.

Bug: 1135081
Change-Id: Ie3f321ee789e0dc4d18476d2316897cb4a6514bb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2448494
Commit-Queue: Omar Morsi <omorsi@google.com>
Reviewed-by: Pavol Marko <pmarko@chromium.org>
Reviewed-by: Ryan Sleevi <rsleevi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#815549}

chrome/browser/chromeos/platform_keys/platform_keys_service.h

@@ -192,10 +192,11 @@ class PlatformKeysService : public KeyedService {
   virtual void GetCertificates(TokenId token_id,
                                const GetCertificatesCallback callback) = 0;
 
-  // Returns the list of all keys available from the given |token_id| as a list
-  // of der-encoded SubjectPublicKeyInfo strings. |callback| will be invoked on
-  // the UI thread with the list of available public keys, possibly with an
-  // error status.
+  // Returns the list of all public keys available from the given |token_id|
+  // that have corresponding private keys on the same token as a list of
+  // DER-encoded SubjectPublicKeyInfo strings. |callback| will be invoked on the
+  // UI thread with the list of available public keys, possibly with an error
+  // status.
   virtual void GetAllKeys(TokenId token_id, GetAllKeysCallback callback) = 0;
 
   // Imports |certificate| to the given token if the certified key is already

chrome/browser/chromeos/platform_keys/platform_keys_service_nss.cc

@@ -1112,6 +1112,33 @@ void GetCertificatesWithDB(std::unique_ptr<GetCertificatesState> state,
       base::BindOnce(&DidGetCertificates, std::move(state)), slot);
 }
 
+// Returns true if |public_key| is relevant as a "platform key" that should be
+// visible to chrome extensions / subsystems.
+bool ShouldIncludePublicKey(SECKEYPublicKey* public_key) {
+  crypto::ScopedSECItem cka_id(SECITEM_AllocItem(/*arena=*/nullptr,
+                                                 /*item=*/nullptr,
+                                                 /*len=*/0));
+  if (PK11_ReadRawAttribute(
+          /*objType=*/PK11_TypePubKey, public_key, CKA_ID, cka_id.get()) !=
+      SECSuccess) {
+    return false;
+  }
+
+  base::StringPiece cka_id_str(reinterpret_cast<char*>(cka_id->data),
+                               cka_id->len);
+
+  // Only keys generated/stored by extensions/Chrome should be visible to
+  // extensions. Oemcrypto stores its key in the TPM, but that should not
+  // be exposed. Look at exposing additional attributes or changing the slot
+  // that Oemcrypto stores keys, so that it can be done based on properties
+  // of the key. See https://crbug/com/1136396
+  if (cka_id_str == "arc-oemcrypto") {
+    VLOG(0) << "Filtered out arc-oemcrypto public key.";
+    return false;
+  }
+  return true;
+}
+
 // Does the actual retrieval of the SubjectPublicKeyInfo string on a worker
 // thread. Used by GetAllKeysWithDb().
 void GetAllKeysOnWorkerThread(std::unique_ptr<GetAllKeysState> state) {
@@ -1123,8 +1150,8 @@ void GetAllKeysOnWorkerThread(std::unique_ptr<GetAllKeysState> state) {
   // private + public keys, so it's sufficient to get the public keys (and also
   // not necessary to check that a private key for that public key really
   // exists).
-  SECKEYPublicKeyList* public_keys =
-      PK11_ListPublicKeysInSlot(state->slot_.get(), /*nickname=*/nullptr);
+  crypto::ScopedSECKEYPublicKeyList public_keys(
+      PK11_ListPublicKeysInSlot(state->slot_.get(), /*nickname=*/nullptr));
 
   if (!public_keys) {
     state->OnSuccess(FROM_HERE, std::move(public_key_spki_der_list));
@@ -1133,6 +1160,10 @@ void GetAllKeysOnWorkerThread(std::unique_ptr<GetAllKeysState> state) {
 
   for (SECKEYPublicKeyListNode* node = PUBKEY_LIST_HEAD(public_keys);
        !PUBKEY_LIST_END(node, public_keys); node = PUBKEY_LIST_NEXT(node)) {
+    if (!ShouldIncludePublicKey(node->key)) {
+      continue;
+    }
+
     crypto::ScopedSECItem subject_public_key_info(
         SECKEY_EncodeDERSubjectPublicKeyInfo(node->key));
     if (!subject_public_key_info) {
@@ -1147,7 +1178,6 @@ void GetAllKeysOnWorkerThread(std::unique_ptr<GetAllKeysState> state) {
     }
   }
 
-  SECKEY_DestroyPublicKeyList(public_keys);
   state->OnSuccess(FROM_HERE, std::move(public_key_spki_der_list));
 }
 

crypto/nss_key_util.h

@@ -51,12 +51,18 @@ ImportNSSKeyFromPrivateKeyInfo(PK11SlotInfo* slot,
 // Decodes |input| as a DER-encoded X.509 SubjectPublicKeyInfo and searches for
 // the private key half in the key database. Returns the private key on success
 // or nullptr on error.
+// Note: This function assumes the CKA_ID for public/private key pairs is
+// derived from the public key. NSS does this, but this is not guaranteed by
+// PKCS#11, so keys generated outside of NSS may not be found.
 CRYPTO_EXPORT ScopedSECKEYPrivateKey
 FindNSSKeyFromPublicKeyInfo(base::span<const uint8_t> input);
 
 // Decodes |input| as a DER-encoded X.509 SubjectPublicKeyInfo and searches for
 // the private key half in the slot specified by |slot|. Returns the private key
 // on success or nullptr on error.
+// Note: This function assumes the CKA_ID for public/private key pairs is
+// derived from the public key. NSS does this, but this is not guaranteed by
+// PKCS#11, so keys generated outside of NSS may not be found.
 CRYPTO_EXPORT ScopedSECKEYPrivateKey
 FindNSSKeyFromPublicKeyInfoInSlot(base::span<const uint8_t> input,
                                   PK11SlotInfo* slot);

crypto/scoped_nss_types.h

@@ -17,16 +17,12 @@ namespace crypto {
 
 template <typename Type, void (*Destroyer)(Type*)>
 struct NSSDestroyer {
-  void operator()(Type* ptr) const {
-    Destroyer(ptr);
-  }
+  void operator()(Type* ptr) const { Destroyer(ptr); }
 };
 
 template <typename Type, void (*Destroyer)(Type*, PRBool), PRBool freeit>
 struct NSSDestroyer1 {
-  void operator()(Type* ptr) const {
-    Destroyer(ptr, freeit);
-  }
+  void operator()(Type* ptr) const { Destroyer(ptr, freeit); }
 };
 
 // Define some convenient scopers around NSS pointers.
@@ -39,6 +35,10 @@ typedef std::unique_ptr<PK11SlotInfo, NSSDestroyer<PK11SlotInfo, PK11_FreeSlot>>
 typedef std::unique_ptr<PK11SlotList,
                         NSSDestroyer<PK11SlotList, PK11_FreeSlotList>>
     ScopedPK11SlotList;
+typedef std::unique_ptr<
+    SECKEYPublicKeyList,
+    NSSDestroyer<SECKEYPublicKeyList, SECKEY_DestroyPublicKeyList>>
+    ScopedSECKEYPublicKeyList;
 typedef std::unique_ptr<PK11SymKey, NSSDestroyer<PK11SymKey, PK11_FreeSymKey>>
     ScopedPK11SymKey;
 typedef std::unique_ptr<SECKEYPublicKey,