Bug 1631404 - work around mozilla::pkix forbidding id-kp-OCSPSigning unless specifically required r=bbeurdouche

mozilla::pkix treats the id-kp-OCSPSigning extended key usage as forbidden
unless specifically required. Client authentication certificate filtering in
gecko uses mozilla::pkix, so before this patch, certificates with this EKU would
be filtered out. Normally this is correct, because client authentication
certificates should never have this EKU. However, there is at least one private
PKI where client certificates have this EKU. For interoperability, this patch
works around this restriction by falling back to requiring id-kp-OCSPSigning if
path building initially fails.

Differential Revision: https://phabricator.services.mozilla.com/D72760

security/manager/ssl/nsNSSIOLayer.cpp

@@ -1796,7 +1796,7 @@ class ClientAuthDataRunnable : public SyncRunnableBase {
   ClientAuthInfo mInfo;
   CERTCertificate* const mServerCert;
   nsTArray<nsTArray<uint8_t>> mCollectedCANames;
-  nsTArray<nsTArray<uint8_t>> mEnterpriseIntermediates;
+  nsTArray<nsTArray<uint8_t>> mEnterpriseCertificates;
   UniqueCERTCertificate mSelectedCertificate;
   UniqueSECKEYPrivateKey mSelectedKey;
 };
@@ -1967,12 +1967,12 @@ class ClientAuthCertNonverifyingTrustDomain final : public TrustDomain {
  public:
   ClientAuthCertNonverifyingTrustDomain(
       nsTArray<nsTArray<uint8_t>>& collectedCANames,
-      nsTArray<nsTArray<uint8_t>>& thirdPartyIntermediates)
+      nsTArray<nsTArray<uint8_t>>& thirdPartyCertificates)
       : mCollectedCANames(collectedCANames),
 #ifdef MOZ_NEW_CERT_STORAGE
         mCertStorage(do_GetService(NS_CERT_STORAGE_CID)),
 #endif
-        mThirdPartyIntermediates(thirdPartyIntermediates) {
+        mThirdPartyCertificates(thirdPartyCertificates) {
   }
 
   virtual mozilla::pkix::Result GetCertTrust(
@@ -2042,7 +2042,7 @@ class ClientAuthCertNonverifyingTrustDomain final : public TrustDomain {
 #ifdef MOZ_NEW_CERT_STORAGE
   nsCOMPtr<nsICertStorage> mCertStorage;
 #endif
-  nsTArray<nsTArray<uint8_t>>& mThirdPartyIntermediates;  // non-owning
+  nsTArray<nsTArray<uint8_t>>& mThirdPartyCertificates;  // non-owning
   UniqueCERTCertList mBuiltChain;
 };
 
@@ -2064,14 +2064,20 @@ mozilla::pkix::Result ClientAuthCertNonverifyingTrustDomain::GetCertTrust(
   // If this certificate's issuer distinguished name is in the set of acceptable
   // CA names, we say this is a trust anchor so that the client certificate
   // issued from this certificate will be presented as an option for the user.
+  // We also check the certificate's subject distinguished name to account for
+  // the case where client certificates that have the id-kp-OCSPSigning EKU
+  // can't be trust anchors according to mozilla::pkix, and thus we may be
+  // looking directly at the issuer.
   Input issuer(cert.GetIssuer());
+  Input subject(cert.GetSubject());
   for (const auto& caName : mCollectedCANames) {
     Input caNameInput;
     rv = caNameInput.Init(caName.Elements(), caName.Length());
     if (rv != Success) {
       continue;  // probably too big
     }
-    if (InputsAreEqual(issuer, caNameInput)) {
+    if (InputsAreEqual(issuer, caNameInput) ||
+        InputsAreEqual(subject, caNameInput)) {
       trustLevel = TrustLevel::TrustAnchor;
       return Success;
     }
@@ -2080,11 +2086,20 @@ mozilla::pkix::Result ClientAuthCertNonverifyingTrustDomain::GetCertTrust(
   return Success;
 }
 
+// In theory this implementation should only need to consider intermediate
+// certificates, since in theory it should only need to look at the issuer
+// distinguished name of each certificate to determine if the client
+// certificate is considered acceptable to the server.
+// However, because we need to account for client certificates with the
+// id-kp-OCSPSigning EKU, and because mozilla::pkix doesn't allow such
+// certificates to be trust anchors, we need to consider the issuers of such
+// certificates directly. These issuers could be roots, so we have to consider
+// roots here.
 mozilla::pkix::Result ClientAuthCertNonverifyingTrustDomain::FindIssuer(
     Input encodedIssuerName, IssuerChecker& checker, Time time) {
   // First try all relevant certificates known to Gecko, which avoids calling
   // CERT_CreateSubjectCertList, because that can be expensive.
-  Vector<Input> geckoIntermediateCandidates;
+  Vector<Input> geckoCandidates;
 #ifdef MOZ_NEW_CERT_STORAGE
   if (!mCertStorage) {
     return mozilla::pkix::Result::FATAL_ERROR_LIBRARY_FAILURE;
@@ -2103,27 +2118,26 @@ mozilla::pkix::Result ClientAuthCertNonverifyingTrustDomain::FindIssuer(
     if (rv != Success) {
       continue;  // probably too big
     }
-    // Currently we're only expecting intermediate certificates in cert storage.
+    if (!geckoCandidates.append(certDER)) {
-    if (!geckoIntermediateCandidates.append(certDER)) {
       return mozilla::pkix::Result::FATAL_ERROR_NO_MEMORY;
     }
   }
 #endif
 
-  for (const auto& thirdPartyIntermediate : mThirdPartyIntermediates) {
+  for (const auto& thirdPartyCertificate : mThirdPartyCertificates) {
-    Input thirdPartyIntermediateInput;
+    Input thirdPartyCertificateInput;
-    mozilla::pkix::Result rv = thirdPartyIntermediateInput.Init(
+    mozilla::pkix::Result rv = thirdPartyCertificateInput.Init(
-        thirdPartyIntermediate.Elements(), thirdPartyIntermediate.Length());
+        thirdPartyCertificate.Elements(), thirdPartyCertificate.Length());
     if (rv != Success) {
       continue;  // probably too big
     }
-    if (!geckoIntermediateCandidates.append(thirdPartyIntermediateInput)) {
+    if (!geckoCandidates.append(thirdPartyCertificateInput)) {
       return mozilla::pkix::Result::FATAL_ERROR_NO_MEMORY;
     }
   }
 
   bool keepGoing = true;
-  for (Input candidate : geckoIntermediateCandidates) {
+  for (Input candidate : geckoCandidates) {
     mozilla::pkix::Result rv = checker.Check(candidate, nullptr, keepGoing);
     if (rv != Success) {
       return rv;
@@ -2139,7 +2153,7 @@ mozilla::pkix::Result ClientAuthCertNonverifyingTrustDomain::FindIssuer(
   // there was no error if CERT_CreateSubjectCertList returns nullptr.
   UniqueCERTCertList candidates(CERT_CreateSubjectCertList(
       nullptr, CERT_GetDefaultCertDB(), &encodedIssuerNameItem, 0, false));
-  Vector<Input> nssIntermediateCandidates;
+  Vector<Input> nssCandidates;
   if (candidates) {
     for (CERTCertListNode* n = CERT_LIST_HEAD(candidates);
          !CERT_LIST_END(n, candidates); n = CERT_LIST_NEXT(n)) {
@@ -2149,15 +2163,13 @@ mozilla::pkix::Result ClientAuthCertNonverifyingTrustDomain::FindIssuer(
       if (rv != Success) {
         continue;  // probably too big
       }
-      if (!n->cert->isRoot) {
+      if (!nssCandidates.append(certDER)) {
-        if (!nssIntermediateCandidates.append(certDER)) {
+        return mozilla::pkix::Result::FATAL_ERROR_NO_MEMORY;
-          return mozilla::pkix::Result::FATAL_ERROR_NO_MEMORY;
-        }
       }
     }
   }
 
-  for (Input candidate : nssIntermediateCandidates) {
+  for (Input candidate : nssCandidates) {
     mozilla::pkix::Result rv = checker.Check(candidate, nullptr, keepGoing);
     if (rv != Success) {
       return rv;
@@ -2181,33 +2193,43 @@ mozilla::pkix::Result ClientAuthCertNonverifyingTrustDomain::IsChainValid(
 mozilla::pkix::Result ClientAuthDataRunnable::BuildChainForCertificate(
     CERTCertificate* cert, UniqueCERTCertList& builtChain) {
   ClientAuthCertNonverifyingTrustDomain trustDomain(mCollectedCANames,
-                                                    mEnterpriseIntermediates);
+                                                    mEnterpriseCertificates);
   Input certDER;
   mozilla::pkix::Result result =
       certDER.Init(cert->derCert.data, cert->derCert.len);
   if (result != Success) {
     return result;
   }
-  mozilla::pkix::Result eeResult = BuildCertChain(
+  // Client certificates shouldn't be CAs, but for interoperability reasons we
-      trustDomain, certDER, Now(), EndEntityOrCA::MustBeEndEntity,
+  // attempt to build a path with each certificate as an end entity and then as
-      KeyUsage::noParticularKeyUsageRequired, KeyPurposeId::anyExtendedKeyUsage,
+  // a CA if that fails.
-      CertPolicyId::anyPolicy, nullptr);
+  const EndEntityOrCA kEndEntityOrCAParams[] = {EndEntityOrCA::MustBeEndEntity,
-  if (eeResult == Success) {
+                                                EndEntityOrCA::MustBeCA};
-    builtChain = trustDomain.TakeBuiltChain();
+  // mozilla::pkix rejects certificates with id-kp-OCSPSigning unless it is
-    return Success;
+  // specifically required. A client certificate should never have this EKU.
+  // Unfortunately, there are some client certificates in private PKIs that
+  // have this EKU. For interoperability, we attempt to work around this
+  // restriction in mozilla::pkix by first building the certificate chain with
+  // no particular EKU required and then again with id-kp-OCSPSigning required
+  // if that fails.
+  const KeyPurposeId kKeyPurposeIdParams[] = {KeyPurposeId::anyExtendedKeyUsage,
+                                              KeyPurposeId::id_kp_OCSPSigning};
+  for (const auto& endEntityOrCAParam : kEndEntityOrCAParams) {
+    for (const auto& keyPurposeIdParam : kKeyPurposeIdParams) {
+      mozilla::pkix::Result result =
+          BuildCertChain(trustDomain, certDER, Now(), endEntityOrCAParam,
+                         KeyUsage::noParticularKeyUsageRequired,
+                         keyPurposeIdParam, CertPolicyId::anyPolicy, nullptr);
+      if (result == Success) {
+        builtChain = trustDomain.TakeBuiltChain();
+        return Success;
+      }
+      MOZ_LOG(gPIPNSSLog, LogLevel::Debug,
+              ("client cert non-validation returned %d for '%s'",
+               static_cast<int>(result), cert->subjectName));
+    }
   }
-  mozilla::pkix::Result caResult = BuildCertChain(
+  return mozilla::pkix::Result::ERROR_UNKNOWN_ISSUER;
-      trustDomain, certDER, Now(), EndEntityOrCA::MustBeCA,
-      KeyUsage::noParticularKeyUsageRequired, KeyPurposeId::anyExtendedKeyUsage,
-      CertPolicyId::anyPolicy, nullptr);
-  if (caResult == Success) {
-    builtChain = trustDomain.TakeBuiltChain();
-    return Success;
-  }
-  MOZ_LOG(gPIPNSSLog, LogLevel::Debug,
-          ("client cert non-validation returned %d %d\n",
-           static_cast<int>(eeResult), static_cast<int>(caResult)));
-  return eeResult;
 }
 
 void ClientAuthDataRunnable::RunOnTargetThread() {
@@ -2219,10 +2241,16 @@ void ClientAuthDataRunnable::RunOnTargetThread() {
   if (NS_WARN_IF(!component)) {
     return;
   }
-  nsresult rv = component->GetEnterpriseIntermediates(mEnterpriseIntermediates);
+  nsresult rv = component->GetEnterpriseIntermediates(mEnterpriseCertificates);
   if (NS_WARN_IF(NS_FAILED(rv))) {
     return;
   }
+  nsTArray<nsTArray<uint8_t>> enterpriseRoots;
+  rv = component->GetEnterpriseRoots(enterpriseRoots);
+  if (NS_WARN_IF(NS_FAILED(rv))) {
+    return;
+  }
+  mEnterpriseCertificates.AppendElements(enterpriseRoots);
 
   if (NS_WARN_IF(NS_FAILED(CheckForSmartCardChanges()))) {
     return;
@@ -2253,8 +2281,7 @@ void ClientAuthDataRunnable::RunOnTargetThread() {
         BuildChainForCertificate(n->cert, unusedBuiltChain);
     if (result != Success) {
       MOZ_LOG(gPIPNSSLog, LogLevel::Debug,
-              ("removing cert '%s' (result=%d)", n->cert->subjectName,
+              ("removing cert '%s'", n->cert->subjectName));
-               static_cast<int>(result)));
       CERTCertListNode* toRemove = n;
       n = CERT_LIST_NEXT(n);
       CERT_RemoveCertListNode(toRemove);

security/manager/ssl/tests/mochitest/browser/browser_clientAuth_connection.js

@@ -144,7 +144,11 @@ add_task(async function setup() {
   // the client is not aware of the intermediate, and so it is not available in
   // the callback.
   await readCertificate("client-cert-via-intermediate.pem", ",,");
-  gExpectedClientCertificateChoices = 2;
+  // This certificate has an id-kp-OCSPSigning EKU. Client certificates
+  // shouldn't have this EKU, but there is at least one private PKI where they
+  // do. For interoperability, such certificates will be presented for use.
+  await readCertificate("client-cert-with-ocsp-signing.pem", ",,");
+  gExpectedClientCertificateChoices = 3;
 });
 
 /**
@@ -276,7 +280,7 @@ add_task(async function testCertFilteringWithIntermediate() {
   );
   let nssComponent = Cc["@mozilla.org/psm;1"].getService(Ci.nsINSSComponent);
   nssComponent.addEnterpriseIntermediate(intermediateBytes);
-  gExpectedClientCertificateChoices = 3;
+  gExpectedClientCertificateChoices = 4;
   gClientAuthDialogs.state = DialogState.RETURN_CERT_SELECTED;
   await testHelper("Ask Every Time", "https://requireclientcert.example.com/");
   sdr.logoutAndTeardown();

security/manager/ssl/tests/mochitest/browser/client-cert-with-ocsp-signing.pem

@@ -0,0 +1,20 @@
+-----BEGIN CERTIFICATE-----
+MIIDSDCCAjCgAwIBAgIUd5MV+Q6LETVXiTtwniwUg+gEe10wDQYJKoZIhvcNAQEL
+BQAwajEoMCYGA1UEAxMfVGVtcG9yYXJ5IENlcnRpZmljYXRlIEF1dGhvcml0eTEY
+MBYGA1UEChMPTW96aWxsYSBUZXN0aW5nMSQwIgYDVQQLExtQcm9maWxlIEd1aWRl
+ZCBPcHRpbWl6YXRpb24wIhgPMjAxODExMjcwMDAwMDBaGA8yMDIxMDIwNDAwMDAw
+MFowKzEpMCcGA1UEAwwgY2xpZW50IGNlcnQgd2l0aCBPQ1NQU2lnbmluZyBla3Uw
+ggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQC6iFGoRI4W1kH9braIBjYQ
+PTwT2erkNUq07PVoV2wke8HHJajg2B+9sZwGm24ahvJr4q9adWtqZHEIeqVap0WH
+9xzVJJwCfs1D/B5p0DggKZOrIMNJ5Nu5TMJrbA7tFYIP8X6taRqx0wI6iypB7qdw
+4A8Njf1mCyuwJJKkfbmIYXmQsVeQPdI7xeC4SB+oN9OIQ+8nFthVt2Zaqn4CkC86
+exCABiTMHGyXrZZhW7filhLAdTGjDJHdtMr3/K0dJdMJ77kXDqdo4bN7LyJvaeO0
+ipVhHe4m1iWdq5EITjbLHCQELL8Wiy/l8Y+ZFzG4s/5JI/pyUcQx1QOs2hgKNe2N
+AgMBAAGjITAfMB0GA1UdJQQWMBQGCCsGAQUFBwMCBggrBgEFBQcDCTANBgkqhkiG
+9w0BAQsFAAOCAQEAN/H/gQYh+0NblEL7OJYq37OtVgP9x9ljikDRe30BEN9n9r4r
+UvFLcDQFpH1Pxknqg7VWOq7rTHCzwxDf6V33B9081JQ/yOmShfxzJgFxMkdEw37D
+4V5cX3EDAyED1/9HOVf/BuiV6by05JuPU9a38ovUOfDdi3snZzMM0EMyWPppe0De
+sxQ1fBqzdoOiaST1N20BO0Ba+HHSQJd6Fy2wnchp7uknp1QcyM8W8B0x8egFnGY3
+orBML5zhIl9667QT+gdzmSCnz67p6cT5cZDjpzB5DCu/g5l7n/YoDqPyju9DGL2e
+JVYT6gFxI5W24HN1Id5xiXKvS8Ap3C6Vb6W4fA==
+-----END CERTIFICATE-----

security/manager/ssl/tests/mochitest/browser/client-cert-with-ocsp-signing.pem.certspec

@@ -0,0 +1,3 @@
+issuer:printableString/CN=Temporary Certificate Authority/O=Mozilla Testing/OU=Profile Guided Optimization
+subject:client cert with OCSPSigning eku
+extension:extKeyUsage:clientAuth,OCSPSigning

security/manager/ssl/tests/mochitest/browser/moz.build

@@ -14,6 +14,7 @@ BROWSER_CHROME_MANIFESTS += ['browser.ini']
 #test_certificates = (
 #    'ca.pem',
 #    'client-cert-via-intermediate.pem',
+#    'client-cert-with-ocsp-signing.pem',
 #    'code-ee.pem',
 #    'ee-from-expired-ca.pem',
 #    'ee-from-untrusted-ca.pem',