ClientCertStoreMac: Don't use CSSM to check keyUsage and extendedKeyU…

…sage

CSSM on macOS has been deprecated for a long time. Switch to parsing the
certs ourselves when checking if a cert supports client auth.
(Other uses of CSSM still remain though.)

Change-Id: Ic9ee9990a7b79f6c7d77fc79cd9e6805ff6c649a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3500343
Reviewed-by: David Benjamin <davidben@chromium.org>
Commit-Queue: Matt Mueller <mattm@chromium.org>
Cr-Commit-Position: refs/heads/main@{#977674}

net/ssl/client_cert_store_mac.cc

@@ -28,6 +28,8 @@
 #include "base/task/task_runner_util.h"
 #include "crypto/mac_security_services_lock.h"
 #include "net/base/host_port_pair.h"
+#include "net/cert/internal/extended_key_usage.h"
+#include "net/cert/internal/parse_certificate.h"
 #include "net/cert/x509_util.h"
 #include "net/cert/x509_util_ios_and_mac.h"
 #include "net/cert/x509_util_mac.h"
@@ -147,55 +149,64 @@ bool IsIssuedByInKeychain(const std::vector<std::string>& valid_issuers,
   return true;
 }
 
-// Returns true if |purpose| is listed as allowed in |usage|. This
-// function also considers the "Any" purpose. If the attribute is
-// present and empty, we return false.
-bool ExtendedKeyUsageAllows(const CE_ExtendedKeyUsage* usage,
-                            const CSSM_OID* purpose) {
-  for (unsigned p = 0; p < usage->numPurposes; ++p) {
-    if (x509_util::CSSMOIDEqual(&usage->purposes[p], purpose))
-      return true;
-    if (x509_util::CSSMOIDEqual(&usage->purposes[p],
-                                &CSSMOID_ExtendedKeyUsageAny))
-      return true;
+// Does |cert|'s usage allow SSL client authentication?
+bool SupportsSSLClientAuth(CRYPTO_BUFFER* cert) {
+  DCHECK(cert);
+
+  ParseCertificateOptions options;
+  options.allow_invalid_serial_numbers = true;
+  der::Input tbs_certificate_tlv;
+  der::Input signature_algorithm_tlv;
+  der::BitString signature_value;
+  ParsedTbsCertificate tbs;
+  if (!ParseCertificate(
+          der::Input(CRYPTO_BUFFER_data(cert), CRYPTO_BUFFER_len(cert)),
+          &tbs_certificate_tlv, &signature_algorithm_tlv, &signature_value,
+          nullptr /* errors*/) ||
+      !ParseTbsCertificate(tbs_certificate_tlv, options, &tbs,
+                           nullptr /*errors*/)) {
+    return false;
   }
-  return false;
-}
 
-// Does |cert|'s usage allow SSL client authentication?
-bool SupportsSSLClientAuth(SecCertificateRef cert) {
-  x509_util::CSSMCachedCertificate cached_cert;
-  OSStatus status = cached_cert.Init(cert);
-  if (status)
+  if (!tbs.has_extensions)
+    return true;
+
+  std::map<der::Input, ParsedExtension> extensions;
+  if (!ParseExtensions(tbs.extensions_tlv, &extensions))
     return false;
 
   // RFC5280 says to take the intersection of the two extensions.
   //
-  // Our underlying crypto libraries don't expose
-  // ClientCertificateType, so for now we will not support fixed
-  // Diffie-Hellman mechanisms. For rsa_sign, we need the
+  // We only support signature-based client certificates, so we need the
   // digitalSignature bit.
   //
   // In particular, if a key has the nonRepudiation bit and not the
   // digitalSignature one, we will not offer it to the user.
-  x509_util::CSSMFieldValue key_usage;
-  status = cached_cert.GetField(&CSSMOID_KeyUsage, &key_usage);
-  if (status == CSSM_OK && key_usage.field()) {
-    const CSSM_X509_EXTENSION* ext = key_usage.GetAs<CSSM_X509_EXTENSION>();
-    const CE_KeyUsage* key_usage_value =
-        reinterpret_cast<const CE_KeyUsage*>(ext->value.parsedValue);
-    if (!((*key_usage_value) & CE_KU_DigitalSignature))
+  if (auto it = extensions.find(der::Input(kKeyUsageOid));
+      it != extensions.end()) {
+    der::BitString key_usage;
+    if (!ParseKeyUsage(it->second.value, &key_usage) ||
+        !key_usage.AssertsBit(KEY_USAGE_BIT_DIGITAL_SIGNATURE)) {
       return false;
+    }
   }
 
-  status = cached_cert.GetField(&CSSMOID_ExtendedKeyUsage, &key_usage);
-  if (status == CSSM_OK && key_usage.field()) {
-    const CSSM_X509_EXTENSION* ext = key_usage.GetAs<CSSM_X509_EXTENSION>();
-    const CE_ExtendedKeyUsage* ext_key_usage =
-        reinterpret_cast<const CE_ExtendedKeyUsage*>(ext->value.parsedValue);
-    if (!ExtendedKeyUsageAllows(ext_key_usage, &CSSMOID_ClientAuth))
+  if (auto it = extensions.find(der::Input(kExtKeyUsageOid));
+      it != extensions.end()) {
+    std::vector<der::Input> extended_key_usage;
+    if (!ParseEKUExtension(it->second.value, &extended_key_usage))
+      return false;
+    bool found_acceptable_eku = false;
+    for (const auto& oid : extended_key_usage) {
+      if (oid == der::Input(kAnyEKU) || oid == der::Input(kClientAuth)) {
+        found_acceptable_eku = true;
+        break;
+      }
+    }
+    if (!found_acceptable_eku)
       return false;
   }
+
   return true;
 }
 
@@ -223,8 +234,10 @@ void GetClientCertsImpl(
   selected_identities->clear();
   for (size_t i = 0; i < preliminary_list.size(); ++i) {
     std::unique_ptr<ClientCertIdentityMac>& cert = preliminary_list[i];
-    if (cert->certificate()->HasExpired())
+    if (cert->certificate()->HasExpired() ||
+        !SupportsSSLClientAuth(cert->certificate()->cert_buffer())) {
       continue;
+    }
 
     // Skip duplicates (a cert may be in multiple keychains).
     auto cert_iter = std::find_if(
@@ -275,9 +288,6 @@ void AddIdentity(ScopedCFTypeRef<SecIdentityRef> sec_identity,
   if (err != noErr)
     return;
 
-  if (!SupportsSSLClientAuth(cert_handle.get()))
-    return;
-
   // Allow UTF-8 inside PrintableStrings in client certificates. See
   // crbug.com/770323.
   X509Certificate::UnsafeCreateOptions options;

net/ssl/client_cert_store_mac.h

@@ -28,6 +28,8 @@ class NET_EXPORT ClientCertStoreMac : public ClientCertStore {
                       ClientCertListCallback callback) override;
 
  private:
+  // TODO(https://crbug.com/1302761): Improve test coverage and remove/reduce
+  // the friend tests and ForTesting methods.
   friend class ClientCertStoreMacTest;
   friend class ClientCertStoreMacTestDelegate;
 

net/ssl/client_cert_store_mac_unittest.cc

@@ -6,13 +6,20 @@
 
 #include <memory>
 
+#include "base/strings/string_number_conversions.h"
+#include "net/cert/internal/extended_key_usage.h"
+#include "net/cert/internal/parse_certificate.h"
+#include "net/cert/x509_util_ios_and_mac.h"
 #include "net/ssl/client_cert_identity_mac.h"
 #include "net/ssl/client_cert_identity_test_util.h"
 #include "net/ssl/client_cert_store_unittest-inl.h"
 #include "net/ssl/ssl_private_key.h"
+#include "net/test/cert_builder.h"
 
 namespace net {
 
+namespace {
+
 std::vector<std::unique_ptr<ClientCertIdentityMac>>
 ClientCertIdentityMacListFromCertificateList(const CertificateList& certs) {
   // This doesn't quite construct a real `ClientCertIdentityMac` the
@@ -29,6 +36,32 @@ ClientCertIdentityMacListFromCertificateList(const CertificateList& certs) {
   return identities;
 }
 
+std::string InputVectorToString(std::vector<der::Input> vec) {
+  std::string r = "{";
+  std::string sep;
+  for (const auto& element : vec) {
+    r += sep;
+    r += base::HexEncode(element.AsSpan());
+    sep = ',';
+  }
+  r += '}';
+  return r;
+}
+
+std::string KeyUsageVectorToString(std::vector<KeyUsageBit> vec) {
+  std::string r = "{";
+  std::string sep;
+  for (const auto& element : vec) {
+    r += sep;
+    r += base::NumberToString(static_cast<int>(element));
+    sep = ',';
+  }
+  r += '}';
+  return r;
+}
+
+}  // namespace
+
 class ClientCertStoreMacTestDelegate {
  public:
   bool SelectClientCerts(const CertificateList& input_certs,
@@ -49,6 +82,14 @@ INSTANTIATE_TYPED_TEST_SUITE_P(Mac,
 
 class ClientCertStoreMacTest : public ::testing::Test {
  protected:
+  bool SelectClientCerts(const CertificateList& input_certs,
+                         const SSLCertRequestInfo& cert_request_info,
+                         ClientCertIdentityList* selected_certs) {
+    return store_.SelectClientCertsForTesting(
+        ClientCertIdentityMacListFromCertificateList(input_certs),
+        cert_request_info, selected_certs);
+  }
+
   bool SelectClientCertsGivenPreferred(
       const scoped_refptr<X509Certificate>& preferred_cert,
       const CertificateList& regular_certs,
@@ -115,4 +156,68 @@ TEST_F(ClientCertStoreMacTest, PreferredCertGoesFirst) {
       selected_certs[1]->certificate()->EqualsExcludingChain(cert_2.get()));
 }
 
+TEST_F(ClientCertStoreMacTest, CertSupportsClientAuth) {
+  base::FilePath certs_dir = GetTestCertsDirectory();
+  std::unique_ptr<CertBuilder> builder =
+      CertBuilder::FromFile(certs_dir.AppendASCII("ok_cert.pem"), nullptr);
+  ASSERT_TRUE(builder);
+
+  struct {
+    bool expected_result;
+    std::vector<KeyUsageBit> key_usages;
+    std::vector<der::Input> ekus;
+  } cases[] = {
+      {true, {}, {}},
+      {true, {KEY_USAGE_BIT_DIGITAL_SIGNATURE}, {}},
+      {true,
+       {KEY_USAGE_BIT_DIGITAL_SIGNATURE, KEY_USAGE_BIT_KEY_CERT_SIGN},
+       {}},
+      {false, {KEY_USAGE_BIT_NON_REPUDIATION}, {}},
+      {false, {KEY_USAGE_BIT_KEY_ENCIPHERMENT}, {}},
+      {false, {KEY_USAGE_BIT_DATA_ENCIPHERMENT}, {}},
+      {false, {KEY_USAGE_BIT_KEY_AGREEMENT}, {}},
+      {false, {KEY_USAGE_BIT_KEY_CERT_SIGN}, {}},
+      {false, {KEY_USAGE_BIT_CRL_SIGN}, {}},
+      {false, {KEY_USAGE_BIT_ENCIPHER_ONLY}, {}},
+      {false, {KEY_USAGE_BIT_DECIPHER_ONLY}, {}},
+      {true, {}, {der::Input(kAnyEKU)}},
+      {true, {}, {der::Input(kClientAuth)}},
+      {true, {}, {der::Input(kServerAuth), der::Input(kClientAuth)}},
+      {true, {}, {der::Input(kClientAuth), der::Input(kServerAuth)}},
+      {false, {}, {der::Input(kServerAuth)}},
+      {true, {KEY_USAGE_BIT_DIGITAL_SIGNATURE}, {der::Input(kClientAuth)}},
+      {false, {KEY_USAGE_BIT_KEY_CERT_SIGN}, {der::Input(kClientAuth)}},
+      {false, {KEY_USAGE_BIT_DIGITAL_SIGNATURE}, {der::Input(kServerAuth)}},
+  };
+
+  for (const auto& testcase : cases) {
+    SCOPED_TRACE(testcase.expected_result);
+    SCOPED_TRACE(KeyUsageVectorToString(testcase.key_usages));
+    SCOPED_TRACE(InputVectorToString(testcase.ekus));
+
+    if (testcase.key_usages.empty())
+      builder->EraseExtension(der::Input(kKeyUsageOid));
+    else
+      builder->SetKeyUsages(testcase.key_usages);
+
+    if (testcase.ekus.empty())
+      builder->EraseExtension(der::Input(kExtKeyUsageOid));
+    else
+      builder->SetExtendedKeyUsages(testcase.ekus);
+
+    auto request = base::MakeRefCounted<SSLCertRequestInfo>();
+    ClientCertIdentityList selected_certs;
+    bool rv = SelectClientCerts({builder->GetX509Certificate()}, *request.get(),
+                                &selected_certs);
+    EXPECT_TRUE(rv);
+    if (testcase.expected_result) {
+      ASSERT_EQ(1U, selected_certs.size());
+      EXPECT_TRUE(selected_certs[0]->certificate()->EqualsExcludingChain(
+          builder->GetX509Certificate().get()));
+    } else {
+      EXPECT_TRUE(selected_certs.empty());
+    }
+  }
+}
+
 }  // namespace net

net/test/cert_builder.cc

@@ -11,7 +11,6 @@
 #include "crypto/openssl_util.h"
 #include "crypto/rsa_private_key.h"
 #include "net/cert/asn1_util.h"
-#include "net/cert/internal/parse_certificate.h"
 #include "net/cert/x509_util.h"
 #include "net/der/encode_values.h"
 #include "net/der/input.h"
@@ -384,6 +383,45 @@ void CertBuilder::SetSubjectAltNames(
   SetExtension(der::Input(kSubjectAltNameOid), FinishCBB(cbb.get()));
 }
 
+void CertBuilder::SetKeyUsages(const std::vector<KeyUsageBit>& usages) {
+  ASSERT_GT(usages.size(), 0U);
+  int number_of_unused_bits = 0;
+  std::vector<uint8_t> bytes;
+  for (auto usage : usages) {
+    int bit_index = static_cast<int>(usage);
+
+    // Index of the byte that contains the bit.
+    size_t byte_index = bit_index / 8;
+
+    if (byte_index + 1 > bytes.size()) {
+      bytes.resize(byte_index + 1);
+      number_of_unused_bits = 8;
+    }
+
+    // Within a byte, bits are ordered from most significant to least
+    // significant. Convert |bit_index| to an index within the |byte_index|
+    // byte, measured from its least significant bit.
+    uint8_t bit_index_in_byte = 7 - (bit_index - byte_index * 8);
+
+    if (byte_index + 1 == bytes.size() &&
+        bit_index_in_byte < number_of_unused_bits) {
+      number_of_unused_bits = bit_index_in_byte;
+    }
+
+    bytes[byte_index] |= (1 << bit_index_in_byte);
+  }
+
+  // From RFC 5290:
+  //   KeyUsage ::= BIT STRING {...}
+  bssl::ScopedCBB cbb;
+  CBB ku_cbb;
+  ASSERT_TRUE(CBB_init(cbb.get(), bytes.size() + 1));
+  ASSERT_TRUE(CBB_add_asn1(cbb.get(), &ku_cbb, CBS_ASN1_BITSTRING));
+  ASSERT_TRUE(CBB_add_u8(&ku_cbb, number_of_unused_bits));
+  ASSERT_TRUE(CBB_add_bytes(&ku_cbb, bytes.data(), bytes.size()));
+  SetExtension(der::Input(kKeyUsageOid), FinishCBB(cbb.get()));
+}
+
 void CertBuilder::SetExtendedKeyUsages(
     const std::vector<der::Input>& purpose_oids) {
   // From RFC 5280:

net/test/cert_builder.h

@@ -11,6 +11,7 @@
 #include "base/memory/raw_ptr.h"
 #include "base/rand_util.h"
 #include "net/base/ip_address.h"
+#include "net/cert/internal/parse_certificate.h"
 #include "net/cert/internal/signature_algorithm.h"
 #include "net/cert/x509_certificate.h"
 #include "third_party/boringssl/src/include/openssl/base.h"
@@ -123,6 +124,10 @@ class CertBuilder {
   void SetSubjectAltNames(const std::vector<std::string>& dns_names,
                           const std::vector<IPAddress>& ip_addresses);
 
+  // Sets the keyUsage extension. |usages| should contain the KeyUsageBit
+  // values of the usages to set, and must not be empty.
+  void SetKeyUsages(const std::vector<KeyUsageBit>& usages);
+
   // Sets the extendedKeyUsage extension. |usages| should contain the DER OIDs
   // of the usage purposes to set, and must not be empty.
   void SetExtendedKeyUsages(const std::vector<der::Input>& purpose_oids);