bug 1590888 - reinstate filtering of client certificate selection during the TLS handshake r=kjacobs

Bug 1267643 removed filtering of client certificates based on the
"certificate_authorities" list sent in the client certificate request from the
server in TLS handshakes because it is impossible to implement as specified
without false negatives (i.e. excluding certificates that could be usable but
don't seem to be according to the certificates the client is aware of). In
practice, however, it seems enough users rely on this behavior[0] that we
should add it back until the platform can save client certificate selections
across restarts and the "select one automatically" option is removed (see also
bug 634697).

[0] See e.g. bug 1588703, bug 1590297, bug 1590596, bug 1074195 comment 27,
and any other duplicates of this bug.

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

--HG--
extra : moz-landing-system : lando

browser/components/urlbar/tests/browser/browser_urlbar_speculative_connect_not_with_client_cert.js

@@ -40,7 +40,7 @@ const clientAuthDialogs = {
         expectingChooseCertificate ? "" : "not "
       }expecting chooseCertificate to be called`
     );
-    ok(!!certList.length, "Should have at least one certificate");
+    is(certList.length, 1, "should have only one client certificate available");
     selectedIndex.value = 0;
     rememberClientAuthCertificate.value = false;
     chooseCertificateCalled = true;

security/manager/ssl/nsNSSIOLayer.cpp

@@ -1656,13 +1656,15 @@ class ClientAuthDataRunnable : public SyncRunnableBase {
  public:
   ClientAuthDataRunnable(CERTCertificate** pRetCert, SECKEYPrivateKey** pRetKey,
                          nsNSSSocketInfo* info,
-                         const UniqueCERTCertificate& serverCert)
+                         const UniqueCERTCertificate& serverCert,
+                         nsTArray<nsCString>& caNamesStrings)
       : mRV(SECFailure),
         mErrorCodeToReport(SEC_ERROR_NO_MEMORY),
         mPRetCert(pRetCert),
         mPRetKey(pRetKey),
         mSocketInfo(info),
-        mServerCert(serverCert.get()) {}
+        mServerCert(serverCert.get()),
+        mCANamesStrings(std::move(caNamesStrings)) {}
 
   SECStatus mRV;                      // out
   PRErrorCode mErrorCodeToReport;     // out
@@ -1672,16 +1674,49 @@ class ClientAuthDataRunnable : public SyncRunnableBase {
   virtual void RunOnTargetThread() override;
 
  private:
-  nsNSSSocketInfo* const mSocketInfo;  // in
-  CERTCertificate* const mServerCert;  // in
+  nsNSSSocketInfo* const mSocketInfo;   // in
+  CERTCertificate* const mServerCert;   // in
+  nsTArray<nsCString> mCANamesStrings;  // in
 };
 
+nsTArray<nsCString> DecodeCANames(CERTDistNames* caNames) {
+  MOZ_ASSERT(caNames);
+
+  nsTArray<nsCString> caNamesStrings;
+  if (!caNames) {
+    return caNamesStrings;
+  }
+
+  for (int i = 0; i < caNames->nnames; i++) {
+    char* caName = CERT_DerNameToAscii(&caNames->names[i]);
+    // Ignore failures
+    if (caName) {
+      caNamesStrings.AppendElement(nsCString(caName));
+      PORT_Free(caName);  // CERT_DerNameToAscii uses PORT_Alloc
+    }
+  }
+  return caNamesStrings;
+}
+
+nsTArray<char*> GetDependentStringPointers(nsTArray<nsCString>& strings) {
+  nsTArray<char*> pointers;
+  for (auto& string : strings) {
+    // We're not actually writing to the string. The API we're going to use this
+    // data with takes an array of non-const char pointers. To avoid the
+    // undefined behavior of reinterpreting a const pointer as a non-const
+    // pointer, everything we do here has to be non-const.
+    pointers.AppendElement(string.BeginWriting());
+  }
+  return pointers;
+}
+
 // This callback function is used to pull client certificate
 // information upon server request
 //
 // - arg: SSL data connection
 // - socket: SSL socket we're dealing with
-// - caNames: list of CA names, we ignore this argument in this function
+// - caNames: list of CA names to use as a hint for selecting potential client
+//            certificates (may be empty)
 // - pRetCert: returns a pointer to a pointer to a valid certificate if
 //             successful; otherwise nullptr
 // - pRetKey: returns a pointer to a pointer to the corresponding key if
@@ -1690,7 +1725,7 @@ SECStatus nsNSS_SSLGetClientAuthData(void* arg, PRFileDesc* socket,
                                      CERTDistNames* caNames,
                                      CERTCertificate** pRetCert,
                                      SECKEYPrivateKey** pRetKey) {
-  if (!socket || !pRetCert || !pRetKey) {
+  if (!socket || !caNames || !pRetCert || !pRetKey) {
     PR_SetError(PR_INVALID_ARGUMENT_ERROR, 0);
     return SECFailure;
   }
@@ -1731,9 +1766,10 @@ SECStatus nsNSS_SSLGetClientAuthData(void* arg, PRFileDesc* socket,
     return SECSuccess;
   }
 
+  nsTArray<nsCString> caNamesStrings(DecodeCANames(caNames));
   // XXX: This should be done asynchronously; see bug 696976
-  RefPtr<ClientAuthDataRunnable> runnable(
-      new ClientAuthDataRunnable(pRetCert, pRetKey, info, serverCert));
+  RefPtr<ClientAuthDataRunnable> runnable(new ClientAuthDataRunnable(
+      pRetCert, pRetKey, info, serverCert, caNamesStrings));
   nsresult rv = runnable->DispatchToMainThreadAndWait();
   if (NS_FAILED(rv)) {
     PR_SetError(SEC_ERROR_NO_MEMORY, 0);
@@ -1798,7 +1834,17 @@ void ClientAuthDataRunnable::RunOnTargetThread() {
     return;
   }
 
-  mRV = SECSuccess;
+  nsTArray<char*> caNamesStringPointers(
+      GetDependentStringPointers(mCANamesStrings));
+  mRV = CERT_FilterCertListByCANames(
+      certList.get(), caNamesStringPointers.Length(),
+      caNamesStringPointers.Elements(), certUsageSSLClient);
+  if (mRV != SECSuccess) {
+    return;
+  }
+  if (CERT_LIST_EMPTY(certList)) {
+    return;
+  }
 
   // find valid user cert and key pair
   if (nsGetUserCertChoice() == UserCertChoice::Auto) {

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

@@ -92,21 +92,17 @@ const gClientAuthDialogs = {
     // For mochitests, only the cert at build/pgo/certs/mochitest.client should
     // be selectable, so we do some brief checks to confirm this.
     Assert.notEqual(certList, null, "Cert list should not be null");
-    Assert.ok(certList.length > 0, "Should have at least one certificate");
-    let index = -1;
-    for (let i = 0; i < certList.length; i++) {
-      let cert = certList.queryElementAt(i, Ci.nsIX509Cert);
-      Assert.notEqual(cert, null, "Cert list should contain an nsIX509Cert");
-      if (cert.commonName === "Mochitest client") {
-        index = i;
-        break;
-      }
-    }
-
-    Assert.notEqual(index, -1, "Should have found 'Mochitest client'");
+    Assert.equal(certList.length, 1, "Only 1 certificate should be available");
+    let cert = certList.queryElementAt(0, Ci.nsIX509Cert);
+    Assert.notEqual(cert, null, "Cert list should contain an nsIX509Cert");
+    Assert.equal(
+      cert.commonName,
+      "Mochitest client",
+      "Cert CN should be 'Mochitest client'"
+    );
 
     if (this.state == DialogState.RETURN_CERT_SELECTED) {
-      selectedIndex.value = index;
+      selectedIndex.value = 0;
       return true;
     }
     return false;

testing/mochitest/ssltunnel/ssltunnel.cpp

@@ -31,6 +31,7 @@
 #include "sslproto.h"
 #include "plhash.h"
 #include "mozilla/Sprintf.h"
+#include "mozilla/Unused.h"
 
 using namespace mozilla;
 using namespace mozilla::psm;
@@ -382,6 +383,32 @@ bool ConfigureSSLServerSocket(PRFileDesc* socket, server_info_t* si,
   SSL_OptionSet(ssl_socket, SSL_HANDSHAKE_AS_SERVER, true);
 
   if (clientAuth != caNone) {
+    // If we're requesting or requiring a client certificate, we should
+    // configure NSS to include the "certificate_authorities" field in the
+    // certificate request message. That way we can test that gecko properly
+    // takes note of it.
+    UniqueCERTCertificate issuer(
+        CERT_FindCertIssuer(cert.get(), PR_Now(), certUsageAnyCA));
+    if (!issuer) {
+      LOG_DEBUG(("Failed to find issuer for %s\n", certnick));
+      return false;
+    }
+    UniqueCERTCertList issuerList(CERT_NewCertList());
+    if (!issuerList) {
+      LOG_ERROR(("Failed to allocate new CERTCertList\n"));
+      return false;
+    }
+    if (CERT_AddCertToListTail(issuerList.get(), issuer.get()) != SECSuccess) {
+      LOG_ERROR(("Failed to add issuer to issuerList\n"));
+      return false;
+    }
+    Unused << issuer.release();  // Ownership transferred to issuerList.
+    if (SSL_SetTrustAnchors(ssl_socket, issuerList.get()) != SECSuccess) {
+      LOG_ERROR(
+          ("Failed to set certificate_authorities list for client "
+           "authentication\n"));
+      return false;
+    }
     SSL_OptionSet(ssl_socket, SSL_REQUEST_CERTIFICATE, true);
     SSL_OptionSet(ssl_socket, SSL_REQUIRE_CERTIFICATE, clientAuth == caRequire);
   }