Require NSS 3.35 or later

This lets us remove a pile of dlsym-looked-up NSS functions, and an old
hack in the tests around importing EC keys.

See bug for details. 3.35 is set based on what's in Ubuntu 18.04,
which I believe is the limiting factor given our current Linux system
requirements.

We were previously blocked on https://crbug.com/1199405, but that should
have progressed enough now to allow this.

Bug: 1365414
Change-Id: I9e736e16f0d23b651b836f9095977a74ed86077c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4247537
Reviewed-by: Matt Mueller <mattm@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Pavol Marko <pmarko@chromium.org>
Commit-Queue: David Benjamin <davidben@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1105883}

chrome/browser/ash/platform_keys/platform_keys_service_nss.cc

@@ -1227,8 +1227,8 @@ void RemoveCertificateWithDB(std::unique_ptr<RemoveCertificateState> state,
   net::ScopedCERTCertificate nss_cert =
       net::x509_util::CreateCERTCertificateFromX509Certificate(
           state->certificate_.get());
-  if (!nss_cert || net::x509_util::GetCertIsPerm(
-                       nss_cert.get(), &certificate_found) != SECSuccess) {
+  if (!nss_cert ||
+      CERT_GetCertIsPerm(nss_cert.get(), &certificate_found) != SECSuccess) {
     state->OnError(FROM_HERE, Status::kNetErrorCertificateInvalid);
     return;
   }

chromeos/ash/components/network/onc/onc_certificate_importer_impl.cc

@@ -141,7 +141,7 @@ bool CertificateImporterImpl::StoreServerOrCaCertificateUserInitiated(
       net::x509_util::CreateCERTCertificateFromX509Certificate(
           certificate.certificate().get());
   if (!x509_cert ||
-      net::x509_util::GetCertIsPerm(x509_cert.get(), &is_perm) != SECSuccess) {
+      CERT_GetCertIsPerm(x509_cert.get(), &is_perm) != SECSuccess) {
     NET_LOG(ERROR) << "Unable to create certificate: " << certificate.guid();
     return false;
   }

crypto/ec_private_key.cc

@@ -11,7 +11,6 @@
 
 #include "base/check_op.h"
 #include "crypto/openssl_util.h"
-#include "third_party/boringssl/src/include/openssl/bn.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"
@@ -108,31 +107,6 @@ bool ECPrivateKey::ExportPrivateKey(std::vector<uint8_t>* output) const {
   return true;
 }
 
-bool ECPrivateKey::ExportEncryptedPrivateKey(
-    std::vector<uint8_t>* output) const {
-  OpenSSLErrStackTracer err_tracer(FROM_HERE);
-
-  // Encrypt the object.
-  // NOTE: NSS uses SEC_OID_PKCS12_V2_PBE_WITH_SHA1_AND_3KEY_TRIPLE_DES_CBC
-  // so use NID_pbe_WithSHA1And3_Key_TripleDES_CBC which should be the OpenSSL
-  // equivalent.
-  uint8_t* der;
-  size_t der_len;
-  bssl::ScopedCBB cbb;
-  if (!CBB_init(cbb.get(), 0) ||
-      !PKCS8_marshal_encrypted_private_key(
-          cbb.get(), NID_pbe_WithSHA1And3_Key_TripleDES_CBC,
-          nullptr /* cipher */, nullptr /* no password */, 0 /* pass_len */,
-          nullptr /* salt */, 0 /* salt_len */, 1 /* iterations */,
-          key_.get()) ||
-      !CBB_finish(cbb.get(), &der, &der_len)) {
-    return false;
-  }
-  output->assign(der, der + der_len);
-  OPENSSL_free(der);
-  return true;
-}
-
 bool ECPrivateKey::ExportPublicKey(std::vector<uint8_t>* output) const {
   OpenSSLErrStackTracer err_tracer(FROM_HERE);
   uint8_t* der;

crypto/ec_private_key.h

@@ -59,14 +59,6 @@ class CRYPTO_EXPORT ECPrivateKey {
   // Exports the private key to a PKCS #8 PrivateKeyInfo block.
   bool ExportPrivateKey(std::vector<uint8_t>* output) const;
 
-  // Exports the private key as an ASN.1-encoded PKCS #8 EncryptedPrivateKeyInfo
-  // block wth empty password. This was historically used as a workaround for
-  // NSS API deficiencies and does not provide security.
-  //
-  // This function is deprecated. Use ExportPrivateKey for new code. See
-  // https://crbug.com/603319.
-  bool ExportEncryptedPrivateKey(std::vector<uint8_t>* output) const;
-
   // Exports the public key to an X.509 SubjectPublicKeyInfo block.
   bool ExportPublicKey(std::vector<uint8_t>* output) const;
 

crypto/ec_private_key_unittest.cc

@@ -50,14 +50,6 @@ TEST(ECPrivateKeyUnitTest, InitRandomTest) {
       crypto::ECPrivateKey::CreateFromPrivateKeyInfo(privkey);
   ASSERT_TRUE(keypair_copy);
   ExpectKeysEqual(keypair.get(), keypair_copy.get());
-
-  // Re-import as an EncryptedPrivateKeyInfo with kPassword1.
-  std::vector<uint8_t> encrypted_privkey;
-  EXPECT_TRUE(keypair->ExportEncryptedPrivateKey(&encrypted_privkey));
-  keypair_copy = crypto::ECPrivateKey::CreateFromEncryptedPrivateKeyInfo(
-      encrypted_privkey);
-  ASSERT_TRUE(keypair_copy);
-  ExpectKeysEqual(keypair.get(), keypair_copy.get());
 }
 
 TEST(ECPrivateKeyUnitTest, Copy) {

crypto/nss_key_util.cc

@@ -11,6 +11,7 @@
 #include <stdint.h>
 
 #include <memory>
+#include <vector>
 
 #include "base/logging.h"
 #include "crypto/nss_util.h"
@@ -103,7 +104,7 @@ bool GenerateECKeyPairNSS(PK11SlotInfo* slot,
 
 ScopedSECKEYPrivateKey ImportNSSKeyFromPrivateKeyInfo(
     PK11SlotInfo* slot,
-    const std::vector<uint8_t>& input,
+    base::span<const uint8_t> input,
     bool permanent) {
   DCHECK(slot);
 

crypto/nss_key_util.h

@@ -8,8 +8,6 @@
 #include <secoidt.h>
 #include <stdint.h>
 
-#include <vector>
-
 #include "base/containers/span.h"
 #include "build/build_config.h"
 #include "crypto/crypto_export.h"
@@ -45,7 +43,7 @@ CRYPTO_EXPORT bool GenerateECKeyPairNSS(
 // plaintext form.
 CRYPTO_EXPORT ScopedSECKEYPrivateKey
 ImportNSSKeyFromPrivateKeyInfo(PK11SlotInfo* slot,
-                               const std::vector<uint8_t>& input,
+                               base::span<const uint8_t> input,
                                bool permanent);
 
 // Decodes |input| as a DER-encoded X.509 SubjectPublicKeyInfo and searches for

crypto/nss_util.cc

@@ -204,13 +204,13 @@ class NSSInitSingleton {
 
     EnsureNSPRInit();
 
-    // We *must* have NSS >= 3.26 at compile time.
-    static_assert((NSS_VMAJOR == 3 && NSS_VMINOR >= 26) || (NSS_VMAJOR > 3),
+    // We *must* have NSS >= 3.35 at compile time.
+    static_assert((NSS_VMAJOR == 3 && NSS_VMINOR >= 35) || (NSS_VMAJOR > 3),
                   "nss version check failed");
     // Also check the run-time NSS version.
     // NSS_VersionCheck is a >= check, not strict equality.
-    if (!NSS_VersionCheck("3.26")) {
-      LOG(FATAL) << "NSS_VersionCheck(\"3.26\") failed. NSS >= 3.26 is "
+    if (!NSS_VersionCheck("3.35")) {
+      LOG(FATAL) << "NSS_VersionCheck(\"3.35\") failed. NSS >= 3.35 is "
                     "required. Please upgrade to the latest NSS, and if you "
                     "still get this error, contact your distribution "
                     "maintainer.";

net/cert/known_roots_nss.cc

@@ -11,73 +11,47 @@
 
 #include <memory>
 
-#include "base/compiler_specific.h"
 #include "crypto/nss_util_internal.h"
-#include "net/base/hash_value.h"
 #include "net/cert/x509_util_nss.h"
 
 namespace net {
 
-namespace {
-
-// This can be removed once the minimum NSS version to build is >= 3.30.
-#if !defined(CKA_NSS_MOZILLA_CA_POLICY)
-#define CKA_NSS_MOZILLA_CA_POLICY (CKA_NSS + 34)
-#endif
-
-using PK11HasAttributeSetFunction = CK_BBOOL (*)(PK11SlotInfo* slot,
-                                                 CK_OBJECT_HANDLE id,
-                                                 CK_ATTRIBUTE_TYPE type,
-                                                 PRBool haslock);
-
-}  // namespace
-
 // IsKnownRoot returns true if the given certificate is one that we believe
 // is a standard (as opposed to user-installed) root.
-DISABLE_CFI_DLSYM
 bool IsKnownRoot(CERTCertificate* root) {
   if (!root || !root->slot)
     return false;
 
-  static PK11HasAttributeSetFunction pk11_has_attribute_set =
-      reinterpret_cast<PK11HasAttributeSetFunction>(
-          dlsym(RTLD_DEFAULT, "PK11_HasAttributeSet"));
-  if (pk11_has_attribute_set) {
-    // Historically, the set of root certs was determined based on whether or
-    // not it was part of nssckbi.[so,dll], the read-only PKCS#11 module that
-    // exported the certs with trust settings. However, some distributions,
-    // notably those in the Red Hat family, replace nssckbi with a redirect to
-    // their own store, such as from p11-kit, which can support more robust
-    // trust settings, like per-system trust, admin-defined, and user-defined
-    // trust.
-    //
-    // As a given certificate may exist in multiple modules and slots, scan
-    // through all of the available modules, all of the (connected) slots on
-    // those modules, and check to see if it has the CKA_NSS_MOZILLA_CA_POLICY
-    // attribute set. This attribute indicates it's from the upstream Mozilla
-    // trust store, and these distributions preserve the attribute as a flag.
-    crypto::AutoSECMODListReadLock lock_id;
-    for (const SECMODModuleList* item = SECMOD_GetDefaultModuleList();
-         item != nullptr; item = item->next) {
-      for (int i = 0; i < item->module->slotCount; ++i) {
-        PK11SlotInfo* slot = item->module->slots[i];
-        if (PK11_IsPresent(slot) && PK11_HasRootCerts(slot)) {
-          CK_OBJECT_HANDLE handle = PK11_FindCertInSlot(slot, root, nullptr);
-          if (handle != CK_INVALID_HANDLE &&
-              pk11_has_attribute_set(slot, handle, CKA_NSS_MOZILLA_CA_POLICY,
-                                     PR_FALSE) == CK_TRUE) {
-            return true;
-          }
+  // Historically, the set of root certs was determined based on whether or
+  // not it was part of nssckbi.[so,dll], the read-only PKCS#11 module that
+  // exported the certs with trust settings. However, some distributions,
+  // notably those in the Red Hat family, replace nssckbi with a redirect to
+  // their own store, such as from p11-kit, which can support more robust
+  // trust settings, like per-system trust, admin-defined, and user-defined
+  // trust.
+  //
+  // As a given certificate may exist in multiple modules and slots, scan
+  // through all of the available modules, all of the (connected) slots on
+  // those modules, and check to see if it has the CKA_NSS_MOZILLA_CA_POLICY
+  // attribute set. This attribute indicates it's from the upstream Mozilla
+  // trust store, and these distributions preserve the attribute as a flag.
+  crypto::AutoSECMODListReadLock lock_id;
+  for (const SECMODModuleList* item = SECMOD_GetDefaultModuleList();
+       item != nullptr; item = item->next) {
+    for (int i = 0; i < item->module->slotCount; ++i) {
+      PK11SlotInfo* slot = item->module->slots[i];
+      if (PK11_IsPresent(slot) && PK11_HasRootCerts(slot)) {
+        CK_OBJECT_HANDLE handle = PK11_FindCertInSlot(slot, root, nullptr);
+        if (handle != CK_INVALID_HANDLE &&
+            PK11_HasAttributeSet(slot, handle, CKA_NSS_MOZILLA_CA_POLICY,
+                                 PR_FALSE) == CK_TRUE) {
+          return true;
         }
       }
     }
-
-    return false;
   }
 
-  // This magic name is taken from
-  // http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ckfw/builtins/constants.c&rev=1.13&mark=86,89#79
-  return 0 == strcmp(PK11_GetSlotName(root->slot), "NSS Builtin Objects");
+  return false;
 }
 
 }  // namespace net

net/cert/nss_cert_database.cc

@@ -434,7 +434,6 @@ bool NSSCertDatabase::IsReadOnly(const CERTCertificate* cert) {
 }
 
 // static
-DISABLE_CFI_DLSYM
 bool NSSCertDatabase::IsHardwareBacked(const CERTCertificate* cert) {
   PK11SlotInfo* slot = cert->slot;
   if (!slot)
@@ -446,20 +445,15 @@ bool NSSCertDatabase::IsHardwareBacked(const CERTCertificate* cert) {
   // TPM does not support the key algorithm. Chaps sets a kKeyInSoftware
   // attribute to true for private keys that aren't 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;
-      }
+    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_HasAttributeSet(slot, private_key->pkcs11ID, kKeyInSoftware,
+                             /*haslock=*/PR_FALSE)) {
+      return false;
     }
     // All keys in chaps without the attribute are hardware backed.
     return true;

net/cert/nss_cert_database_unittest.cc

@@ -64,7 +64,7 @@ std::string GetSubjectCN(CERTCertificate* cert) {
 
 bool GetCertIsPerm(const CERTCertificate* cert) {
   PRBool is_perm;
-  CHECK_EQ(x509_util::GetCertIsPerm(cert, &is_perm), SECSuccess);
+  CHECK_EQ(CERT_GetCertIsPerm(cert, &is_perm), SECSuccess);
   return is_perm != PR_FALSE;
 }
 

net/cert/x509_util_nss.cc

@@ -18,7 +18,6 @@
 #include <secport.h>
 #include <string.h>
 
-#include "base/compiler_specific.h"
 #include "base/logging.h"
 #include "base/strings/stringprintf.h"
 #include "crypto/nss_util.h"
@@ -439,19 +438,4 @@ SHA256HashValue CalculateFingerprint256(CERTCertificate* cert) {
   return sha256;
 }
 
-DISABLE_CFI_DLSYM
-SECStatus GetCertIsPerm(const CERTCertificate* cert, PRBool* isperm) {
-  // TODO(https://crbug.com/1365414): When the minimum NSS version is raised to
-  // 3.31 or higher, replace this with calling CERT_GetCertIsPerm directly.
-  using GetCertIsPermFunction = SECStatus (*)(const CERTCertificate*, PRBool*);
-  static GetCertIsPermFunction get_cert_is_perm =
-      reinterpret_cast<GetCertIsPermFunction>(
-          dlsym(RTLD_DEFAULT, "CERT_GetCertIsPerm"));
-  if (get_cert_is_perm) {
-    return get_cert_is_perm(cert, isperm);
-  }
-  *isperm = cert->isperm;
-  return SECSuccess;
-}
-
 }  // namespace net::x509_util

net/cert/x509_util_nss.h

@@ -157,11 +157,6 @@ NET_EXPORT bool GetValidityTimes(CERTCertificate* cert,
 // (all zero) fingerprint on failure.
 NET_EXPORT SHA256HashValue CalculateFingerprint256(CERTCertificate* cert);
 
-// Behaves like `CERT_GetCertIsPerm` in NSS. This function's type signature
-// mirrors the NSS function so call sites can be easily replaced when
-// https://crbug.com/1365414 is resolved.
-NET_EXPORT SECStatus GetCertIsPerm(const CERTCertificate* cert, PRBool* isperm);
-
 }  // namespace net::x509_util
 
 #endif  // NET_CERT_X509_UTIL_NSS_H_

net/test/cert_test_util_nss.cc

@@ -13,17 +13,10 @@
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/logging.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 {
 
@@ -38,80 +31,12 @@ bool ImportSensitiveKeyFromFile(const base::FilePath& dir,
     return false;
   }
 
-  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,
-                                             true /* permanent */));
-  LOG_IF(ERROR, !private_key) << "Could not create key from file "
-                              << key_path.value();
+      crypto::ImportNSSKeyFromPrivateKeyInfo(
+          slot, base::as_bytes(base::make_span(key_pkcs8)),
+          /*permanent=*/true));
+  LOG_IF(ERROR, !private_key)
+      << "Could not create key from file " << key_path.value();
   return !!private_key;
 }
 

net/third_party/mozilla_security_manager/nsNSSCertificateDB.cpp

@@ -65,7 +65,7 @@ bool ImportCACerts(PK11SlotInfo* slot,
   // already and use that, but CERT_NewTempCertificate actually does that
   // itself, so we skip it here.
   PRBool root_is_perm;
-  if (net::x509_util::GetCertIsPerm(root, &root_is_perm) != SECSuccess) {
+  if (CERT_GetCertIsPerm(root, &root_is_perm) != SECSuccess) {
     LOG(ERROR) << "CERT_GetCertIsPerm failed with error " << PORT_GetError();
     return false;
   }
@@ -124,7 +124,7 @@ bool ImportCACerts(PK11SlotInfo* slot,
     }
 
     PRBool cert_is_perm;
-    if (net::x509_util::GetCertIsPerm(cert, &cert_is_perm) != SECSuccess) {
+    if (CERT_GetCertIsPerm(cert, &cert_is_perm) != SECSuccess) {
       LOG(ERROR) << "CERT_GetCertIsPerm failed with error " << PORT_GetError();
       return false;
     }