Remove ClientCertStoreChromeOS::CertFilter indirection

Historically, ClientCertStoreChromeOS lived in //net and depended on the
CrOS NSS profile logic in //crypto. To reduce the dependency on that
mess, https://codereview.chromium.org/663583006 extracted the filtering
logic into a separate ClientCertFilterChromeOS in //chrome, indirected
via a CertFilter interface.

This was done in preparation for
https://codereview.chromium.org/419013003/, which was since abandoned
(although the initialization still needs cleanup, per
https://crbug.com/1018972). It also left the ClientCertFilterChromeOS
logic entirely untested.

Since then, ClientCertStoreChromeOS has moved into //chrome, so we no
longer need the indirection. Fold it back in and test the user profile
logic directly to restore ClientCertFilterChromeOS's test coverage. This
is done preparation for work on https://crbug.com/1011576, which will
affect this code somewhat.

This moves the pre-NSS-3.30 workaround for importing ECDSA keys from
ssl_platform_key_nss_unittest.cc to cert_test_util_nss.cc so it works
for all callers of ImportSensitiveKeyFromFile.

Bug: 1011576
Change-Id: I26ae5d610b8b7e443c18ca1314d16129a8f52350
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1888312
Commit-Queue: David Benjamin <davidben@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Reviewed-by: Matt Mueller <mattm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#711301}

chrome/browser/chromeos/net/client_cert_filter_chromeos.h

@@ -8,8 +8,8 @@
 #include <memory>
 #include <string>
 
+#include "base/callback.h"
 #include "base/memory/weak_ptr.h"
-#include "chrome/browser/chromeos/net/client_cert_store_chromeos.h"
 #include "crypto/scoped_nss_types.h"
 #include "net/cert/nss_profile_filter_chromeos.h"
 
@@ -17,7 +17,9 @@ namespace chromeos {
 
 // A client certificate filter that filters by applying a
 // NSSProfileFilterChromeOS.
-class ClientCertFilterChromeOS : public ClientCertStoreChromeOS::CertFilter {
+//
+// TODO(davidben): Fold this class back into ClientCertStoreChromeOS.
+class ClientCertFilterChromeOS {
  public:
   // The internal NSSProfileFilterChromeOS will be initialized with the public
   // and private slot of the user with |username_hash| and with the system slot
@@ -25,11 +27,18 @@ class ClientCertFilterChromeOS : public ClientCertStoreChromeOS::CertFilter {
   // If |username_hash| is empty, no public and no private slot will be used.
   ClientCertFilterChromeOS(bool use_system_slot,
                            const std::string& username_hash);
-  ~ClientCertFilterChromeOS() override;
-
-  // ClientCertStoreChromeOS::CertFilter:
-  bool Init(const base::Closure& callback) override;
-  bool IsCertAllowed(CERTCertificate* cert) const override;
+  ~ClientCertFilterChromeOS();
+
+  // Initializes this filter. Returns true if it finished initialization,
+  // otherwise returns false and calls |callback| once the initialization is
+  // completed.
+  // Must be called at most once.
+  bool Init(const base::Closure& callback);
+
+  // Returns true if |cert| is allowed to be used as a client certificate (e.g.
+  // for a certain browser context or user). This is only called once
+  // initialization is finished, see Init().
+  bool IsCertAllowed(CERTCertificate* cert) const;
 
  private:
   // Called back if the system slot was retrieved asynchronously. Continues the

chrome/browser/chromeos/net/client_cert_store_chromeos.cc

@@ -17,6 +17,7 @@
 #include "base/task/post_task.h"
 #include "base/threading/scoped_blocking_call.h"
 #include "chrome/browser/chromeos/certificate_provider/certificate_provider.h"
+#include "chrome/browser/chromeos/net/client_cert_filter_chromeos.h"
 #include "crypto/nss_crypto_module_delegate.h"
 #include "net/ssl/ssl_cert_request_info.h"
 #include "net/ssl/ssl_private_key.h"
@@ -25,10 +26,11 @@ namespace chromeos {
 
 ClientCertStoreChromeOS::ClientCertStoreChromeOS(
     std::unique_ptr<CertificateProvider> cert_provider,
-    std::unique_ptr<CertFilter> cert_filter,
+    bool use_system_slot,
+    const std::string& username_hash,
     const PasswordDelegateFactory& password_delegate_factory)
     : cert_provider_(std::move(cert_provider)),
-      cert_filter_(std::move(cert_filter)) {}
+      cert_filter_(use_system_slot, username_hash) {}
 
 ClientCertStoreChromeOS::~ClientCertStoreChromeOS() {}
 
@@ -56,7 +58,7 @@ void ClientCertStoreChromeOS::GetClientCerts(
 
   auto repeating_callback = base::AdaptCallbackForRepeating(
       std::move(get_additional_certs_and_continue));
-  if (cert_filter_->Init(repeating_callback))
+  if (cert_filter_.Init(repeating_callback))
     repeating_callback.Run();
 }
 
@@ -93,8 +95,8 @@ ClientCertStoreChromeOS::GetAndFilterCertsOnWorkerThread(
   net::ClientCertIdentityList client_certs;
   net::ClientCertStoreNSS::GetPlatformCertsOnWorkerThread(
       std::move(password_delegate),
-      base::BindRepeating(&CertFilter::IsCertAllowed,
-                          base::Unretained(cert_filter_.get())),
+      base::BindRepeating(&ClientCertFilterChromeOS::IsCertAllowed,
+                          base::Unretained(&cert_filter_)),
       &client_certs);
 
   client_certs.reserve(client_certs.size() + additional_certs.size());

chrome/browser/chromeos/net/client_cert_store_chromeos.h

@@ -11,6 +11,7 @@
 
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
+#include "chrome/browser/chromeos/net/client_cert_filter_chromeos.h"
 #include "net/ssl/client_cert_store_nss.h"
 
 namespace chromeos {
@@ -22,28 +23,15 @@ class ClientCertStoreChromeOS : public net::ClientCertStore {
   using PasswordDelegateFactory =
       net::ClientCertStoreNSS::PasswordDelegateFactory;
 
-  class CertFilter {
-   public:
-    virtual ~CertFilter() {}
-
-    // Initializes this filter. Returns true if it finished initialization,
-    // otherwise returns false and calls |callback| once the initialization is
-    // completed.
-    // Must be called at most once.
-    virtual bool Init(const base::Closure& callback) = 0;
-
-    // Returns true if |cert| is allowed to be used as a client certificate
-    // (e.g. for a certain browser context or user).
-    // This is only called once initialization is finished, see Init().
-    virtual bool IsCertAllowed(CERTCertificate* cert) const = 0;
-  };
-
-  // This ClientCertStore will return client certs from NSS certificate
-  // databases that pass the filter |cert_filter| and additionally return
-  // certificates provided by |cert_provider|.
+  // This ClientCertStore will return client certs from the public
+  // and private slot of the user with |username_hash| and with the system slot
+  // if |use_system_slot| is true. If |username_hash| is empty, no public and no
+  // private slot will be used. It will additionally return certificates
+  // provided by |cert_provider|.
   ClientCertStoreChromeOS(
       std::unique_ptr<CertificateProvider> cert_provider,
-      std::unique_ptr<CertFilter> cert_filter,
+      bool use_system_slot,
+      const std::string& username_hash,
       const PasswordDelegateFactory& password_delegate_factory);
   ~ClientCertStoreChromeOS() override;
 
@@ -63,7 +51,7 @@ class ClientCertStoreChromeOS : public net::ClientCertStore {
       net::ClientCertIdentityList additional_certs);
 
   std::unique_ptr<CertificateProvider> cert_provider_;
-  std::unique_ptr<CertFilter> cert_filter_;
+  ClientCertFilterChromeOS cert_filter_;
 
   // The factory for creating the delegate for requesting a password to a
   // PKCS#11 token. May be null.

chrome/browser/chromeos/platform_keys/platform_keys_nss.cc

@@ -24,7 +24,6 @@
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/browser_process_platform_part_chromeos.h"
 #include "chrome/browser/chromeos/certificate_provider/certificate_provider.h"
-#include "chrome/browser/chromeos/net/client_cert_filter_chromeos.h"
 #include "chrome/browser/chromeos/net/client_cert_store_chromeos.h"
 #include "chrome/browser/chromeos/platform_keys/platform_keys.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
@@ -597,8 +596,7 @@ void SelectCertificatesOnIOThread(
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   state->cert_store_.reset(new ClientCertStoreChromeOS(
       nullptr,  // no additional provider
-      std::make_unique<ClientCertFilterChromeOS>(state->use_system_key_slot_,
-                                                 state->username_hash_),
+      state->use_system_key_slot_, state->username_hash_,
       ClientCertStoreChromeOS::PasswordDelegateFactory()));
 
   SelectCertificatesState* state_ptr = state.get();

chrome/browser/profiles/profile_io_data.cc

@@ -82,7 +82,6 @@
 #include "chrome/browser/chromeos/certificate_provider/certificate_provider_service.h"
 #include "chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.h"
 #include "chrome/browser/chromeos/login/startup_utils.h"
-#include "chrome/browser/chromeos/net/client_cert_filter_chromeos.h"
 #include "chrome/browser/chromeos/net/client_cert_store_chromeos.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
@@ -424,8 +423,7 @@ std::unique_ptr<net::ClientCertStore> ProfileIOData::CreateClientCertStore() {
   return std::unique_ptr<net::ClientCertStore>(
       new chromeos::ClientCertStoreChromeOS(
           certificate_provider_ ? certificate_provider_->Copy() : nullptr,
-          std::make_unique<chromeos::ClientCertFilterChromeOS>(
-              use_system_key_slot, username_hash_),
+          use_system_key_slot, username_hash_,
           base::Bind(&CreateCryptoModuleBlockingPasswordDelegate,
                      kCryptoModulePasswordClientAuth)));
 #elif defined(USE_NSS_CERTS)

net/ssl/ssl_platform_key_nss_unittest.cc

@@ -4,19 +4,11 @@
 
 #include "net/ssl/ssl_platform_key_nss.h"
 
-#include <keyhi.h>
-#include <pk11pub.h>
-#include <stdint.h>
-#include <string.h>
-
-#include <memory>
 #include <string>
-#include <vector>
 
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/memory/ref_counted.h"
-#include "crypto/ec_private_key.h"
 #include "crypto/nss_crypto_module_delegate.h"
 #include "crypto/scoped_nss_types.h"
 #include "crypto/scoped_test_nss_db.h"
@@ -27,12 +19,7 @@
 #include "net/test/test_data_directory.h"
 #include "net/test/test_with_task_environment.h"
 #include "testing/gtest/include/gtest/gtest.h"
-#include "third_party/boringssl/src/include/openssl/bytestring.h"
-#include "third_party/boringssl/src/include/openssl/ec.h"
-#include "third_party/boringssl/src/include/openssl/ec_key.h"
 #include "third_party/boringssl/src/include/openssl/evp.h"
-#include "third_party/boringssl/src/include/openssl/mem.h"
-#include "third_party/boringssl/src/include/openssl/ssl.h"
 
 namespace net {
 
@@ -71,62 +58,12 @@ TEST_P(SSLPlatformKeyNSSTest, KeyMatches) {
 
   // Import the key into a test NSS database.
   crypto::ScopedTestNSSDB test_db;
-  scoped_refptr<X509Certificate> cert;
   ScopedCERTCertificate nss_cert;
-  if (test_key.type == EVP_PKEY_EC) {
-    // NSS cannot import unencrypted ECDSA keys, so we encrypt it with an empty
-    // password and import manually.
-    std::vector<uint8_t> pkcs8_vector(pkcs8.begin(), pkcs8.end());
-    std::unique_ptr<crypto::ECPrivateKey> ec_private_key =
-        crypto::ECPrivateKey::CreateFromPrivateKeyInfo(pkcs8_vector);
-    ASSERT_TRUE(ec_private_key);
-    std::vector<uint8_t> encrypted;
-    ASSERT_TRUE(ec_private_key->ExportEncryptedPrivateKey(&encrypted));
-
-    SECItem encrypted_item = {siBuffer, encrypted.data(),
-                              static_cast<unsigned>(encrypted.size())};
-    SECKEYEncryptedPrivateKeyInfo epki;
-    memset(&epki, 0, sizeof(epki));
-    crypto::ScopedPLArenaPool arena(PORT_NewArena(DER_DEFAULT_CHUNKSIZE));
-    ASSERT_EQ(SECSuccess,
-              SEC_QuickDERDecodeItem(
-                  arena.get(), &epki,
-                  SEC_ASN1_GET(SECKEY_EncryptedPrivateKeyInfoTemplate),
-                  &encrypted_item));
-
-    // NSS uses the serialized public key in X9.62 form as the "public value"
-    // for key ID purposes.
-    bssl::ScopedCBB cbb;
-    ASSERT_TRUE(CBB_init(cbb.get(), 0));
-    EC_KEY* ec_key = EVP_PKEY_get0_EC_KEY(ec_private_key->key());
-    ASSERT_TRUE(EC_POINT_point2cbb(cbb.get(), EC_KEY_get0_group(ec_key),
-                                   EC_KEY_get0_public_key(ec_key),
-                                   POINT_CONVERSION_UNCOMPRESSED, nullptr));
-    uint8_t* public_value;
-    size_t public_value_len;
-    ASSERT_TRUE(CBB_finish(cbb.get(), &public_value, &public_value_len));
-    bssl::UniquePtr<uint8_t> scoped_public_value(public_value);
-    SECItem public_item = {siBuffer, public_value,
-                           static_cast<unsigned>(public_value_len)};
-
-    SECItem password_item = {siBuffer, nullptr, 0};
-    ASSERT_EQ(SECSuccess,
-              PK11_ImportEncryptedPrivateKeyInfo(
-                  test_db.slot(), &epki, &password_item, nullptr /* nickname */,
-                  &public_item, PR_TRUE /* permanent */, PR_TRUE /* private */,
-                  ecKey, KU_DIGITAL_SIGNATURE, nullptr /* wincx */));
-
-    cert = ImportCertFromFile(GetTestCertsDirectory(), test_key.cert_file);
-    ASSERT_TRUE(cert);
-    nss_cert = ImportClientCertToSlot(cert, test_db.slot());
-    ASSERT_TRUE(nss_cert);
-  } else {
-    cert = ImportClientCertAndKeyFromFile(GetTestCertsDirectory(),
-                                          test_key.cert_file, test_key.key_file,
-                                          test_db.slot(), &nss_cert);
-    ASSERT_TRUE(cert);
-    ASSERT_TRUE(nss_cert);
-  }
+  scoped_refptr<X509Certificate> cert = ImportClientCertAndKeyFromFile(
+      GetTestCertsDirectory(), test_key.cert_file, test_key.key_file,
+      test_db.slot(), &nss_cert);
+  ASSERT_TRUE(cert);
+  ASSERT_TRUE(nss_cert);
 
   // Look up the key.
   scoped_refptr<SSLPrivateKey> key =

net/test/cert_test_util.h

@@ -6,6 +6,7 @@
 #define NET_TEST_CERT_TEST_UTIL_H_
 
 #include <string>
+#include <vector>
 
 #include "base/memory/ref_counted.h"
 #include "net/cert/x509_cert_types.h"

net/test/cert_test_util_nss.cc

@@ -6,14 +6,23 @@
 
 #include <pk11pub.h>
 #include <secmodt.h>
+#include <string.h>
+
+#include <memory>
 
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
+#include "crypto/ec_private_key.h"
 #include "crypto/nss_key_util.h"
 #include "crypto/nss_util.h"
 #include "crypto/scoped_nss_types.h"
 #include "net/cert/cert_type.h"
 #include "net/cert/x509_util_nss.h"
+#include "third_party/boringssl/src/include/openssl/bytestring.h"
+#include "third_party/boringssl/src/include/openssl/ec.h"
+#include "third_party/boringssl/src/include/openssl/ec_key.h"
+#include "third_party/boringssl/src/include/openssl/evp.h"
+#include "third_party/boringssl/src/include/openssl/mem.h"
 
 namespace net {
 
@@ -28,10 +37,74 @@ bool ImportSensitiveKeyFromFile(const base::FilePath& dir,
     return false;
   }
 
-  const uint8_t* key_pkcs8_begin =
-      reinterpret_cast<const uint8_t*>(key_pkcs8.data());
-  std::vector<uint8_t> key_vector(key_pkcs8_begin,
-                                  key_pkcs8_begin + key_pkcs8.length());
+  std::vector<uint8_t> key_vector(key_pkcs8.begin(), key_pkcs8.end());
+
+  // Prior to NSS 3.30, NSS cannot import unencrypted ECDSA private keys. Detect
+  // such keys and encrypt with an empty password before importing. Once our
+  // minimum version is raised to NSS 3.30, this logic can be removed. See
+  // https://bugzilla.mozilla.org/show_bug.cgi?id=1295121
+  CBS cbs;
+  CBS_init(&cbs, key_vector.data(), key_vector.size());
+  bssl::UniquePtr<EVP_PKEY> evp_pkey(EVP_parse_private_key(&cbs));
+  if (!evp_pkey) {
+    LOG(ERROR) << "Could not parse private key from file " << key_path.value();
+    return false;
+  }
+  if (EVP_PKEY_id(evp_pkey.get()) == EVP_PKEY_EC) {
+    std::unique_ptr<crypto::ECPrivateKey> ec_private_key =
+        crypto::ECPrivateKey::CreateFromPrivateKeyInfo(key_vector);
+    std::vector<uint8_t> encrypted;
+    if (!ec_private_key ||
+        !ec_private_key->ExportEncryptedPrivateKey(&encrypted)) {
+      LOG(ERROR) << "Error importing private key from file "
+                 << key_path.value();
+      return false;
+    }
+
+    SECItem encrypted_item = {siBuffer, encrypted.data(),
+                              static_cast<unsigned>(encrypted.size())};
+    SECKEYEncryptedPrivateKeyInfo epki;
+    memset(&epki, 0, sizeof(epki));
+    crypto::ScopedPLArenaPool arena(PORT_NewArena(DER_DEFAULT_CHUNKSIZE));
+    if (SEC_QuickDERDecodeItem(
+            arena.get(), &epki,
+            SEC_ASN1_GET(SECKEY_EncryptedPrivateKeyInfoTemplate),
+            &encrypted_item) != SECSuccess) {
+      LOG(ERROR) << "Error importing private key from file "
+                 << key_path.value();
+      return false;
+    }
+
+    // NSS uses the serialized public key in X9.62 form as the "public value"
+    // for key ID purposes.
+    EC_KEY* ec_key = EVP_PKEY_get0_EC_KEY(ec_private_key->key());
+    bssl::ScopedCBB cbb;
+    uint8_t* public_value;
+    size_t public_value_len;
+    if (!CBB_init(cbb.get(), 0) ||
+        !EC_POINT_point2cbb(cbb.get(), EC_KEY_get0_group(ec_key),
+                            EC_KEY_get0_public_key(ec_key),
+                            POINT_CONVERSION_UNCOMPRESSED, nullptr) ||
+        !CBB_finish(cbb.get(), &public_value, &public_value_len)) {
+      LOG(ERROR) << "Error importing private key from file "
+                 << key_path.value();
+      return false;
+    }
+    bssl::UniquePtr<uint8_t> scoped_public_value(public_value);
+    SECItem public_item = {siBuffer, public_value,
+                           static_cast<unsigned>(public_value_len)};
+
+    SECItem password_item = {siBuffer, nullptr, 0};
+    if (PK11_ImportEncryptedPrivateKeyInfo(
+            slot, &epki, &password_item, nullptr /* nickname */, &public_item,
+            PR_TRUE /* permanent */, PR_TRUE /* private */, ecKey,
+            KU_DIGITAL_SIGNATURE, nullptr /* wincx */) != SECSuccess) {
+      LOG(ERROR) << "Error importing private key from file "
+                 << key_path.value();
+      return false;
+    }
+    return true;
+  }
 
   crypto::ScopedSECKEYPrivateKey private_key(
       crypto::ImportNSSKeyFromPrivateKeyInfo(slot, key_vector,