Bug 1289186 - wait for the server certificate to verify successfully before asking for a client auth certificate r=jschanck

If a TLS server asks for a client authentication certificate, no dialog asking
the user to select one should be shown until the server's certificate verifies
successfully.

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

build/pgo/server-locations.txt

@@ -125,6 +125,7 @@ https://expired.example.com:443        privileged,cert=expired
 https://requestclientcert.example.com:443         privileged,clientauth=request
 https://requireclientcert.example.com:443         privileged,clientauth=require
 https://requireclientcert-2.example.com:443         privileged,clientauth=require
+https://requireclientcert-untrusted.example.com:443         privileged,clientauth=require,cert=untrusted
 https://mismatch.expired.example.com:443	privileged,cert=expired
 https://mismatch.untrusted.example.com:443	privileged,cert=untrusted
 https://untrusted-expired.example.com:443	privileged,cert=untrustedandexpired

security/manager/ssl/NSSSocketControl.cpp

@@ -24,7 +24,7 @@ NSSSocketControl::NSSSocketControl(const nsCString& aHostName, int32_t aPort,
                                    uint32_t providerTlsFlags)
     : CommonSocketControl(aHostName, aPort, providerFlags),
       mFd(nullptr),
-      mCertVerificationState(before_cert_verification),
+      mCertVerificationState(BeforeCertVerification),
       mSharedState(aState),
       mForSTARTTLS(false),
       mTLSVersionRange{0, 0},
@@ -45,7 +45,8 @@ NSSSocketControl::NSSSocketControl(const nsCString& aHostName, int32_t aPort,
       mMACAlgorithmUsed(nsITLSSocketControl::SSL_MAC_UNKNOWN),
       mProviderTlsFlags(providerTlsFlags),
       mSocketCreationTimestamp(TimeStamp::Now()),
-      mPlaintextBytesRead(0) {}
+      mPlaintextBytesRead(0),
+      mPendingSelectClientAuthCertificate(nullptr) {}
 
 NS_IMETHODIMP
 NSSSocketControl::GetKEAUsed(int16_t* aKea) {
@@ -336,12 +337,12 @@ nsresult NSSSocketControl::SetFileDescPtr(PRFileDesc* aFilePtr) {
 
 void NSSSocketControl::SetCertVerificationWaiting() {
   COMMON_SOCKET_CONTROL_ASSERT_ON_OWNING_THREAD();
-  // mCertVerificationState may be before_cert_verification for the first
-  // handshake on the connection, or after_cert_verification for subsequent
+  // mCertVerificationState may be BeforeCertVerification for the first
+  // handshake on the connection, or AfterCertVerification for subsequent
   // renegotiation handshakes.
-  MOZ_ASSERT(mCertVerificationState != waiting_for_cert_verification,
-             "Invalid state transition to waiting_for_cert_verification");
-  mCertVerificationState = waiting_for_cert_verification;
+  MOZ_ASSERT(mCertVerificationState != WaitingForCertVerification,
+             "Invalid state transition to WaitingForCertVerification");
+  mCertVerificationState = WaitingForCertVerification;
 }
 
 // Be careful that SetCertVerificationResult does NOT get called while we are
@@ -351,8 +352,8 @@ void NSSSocketControl::SetCertVerificationWaiting() {
 void NSSSocketControl::SetCertVerificationResult(PRErrorCode errorCode) {
   COMMON_SOCKET_CONTROL_ASSERT_ON_OWNING_THREAD();
   SetUsedPrivateDNS(GetProviderFlags() & nsISocketProvider::USED_PRIVATE_DNS);
-  MOZ_ASSERT(mCertVerificationState == waiting_for_cert_verification,
-             "Invalid state transition to cert_verification_finished");
+  MOZ_ASSERT(mCertVerificationState == WaitingForCertVerification,
+             "Invalid state transition to AfterCertVerification");
 
   if (mFd) {
     SECStatus rv = SSL_AuthCertificateComplete(mFd, errorCode);
@@ -380,7 +381,7 @@ void NSSSocketControl::SetCertVerificationResult(PRErrorCode errorCode) {
                           AssertedCast<uint32_t>(mPlaintextBytesRead));
   }
 
-  mCertVerificationState = after_cert_verification;
+  mCertVerificationState = AfterCertVerification;
 }
 
 void NSSSocketControl::ClientAuthCertificateSelected(
@@ -470,6 +471,9 @@ NSSSocketControl::SetHandshakeCallbackListener(
 
 PRStatus NSSSocketControl::CloseSocketAndDestroy() {
   COMMON_SOCKET_CONTROL_ASSERT_ON_OWNING_THREAD();
+
+  mPendingSelectClientAuthCertificate = nullptr;
+
   PRFileDesc* popped = PR_PopIOLayer(mFd, PR_TOP_IO_LAYER);
   MOZ_ASSERT(
       popped && popped->identity == nsSSLIOLayerHelpers::nsSSLIOLayerIdentity,

security/manager/ssl/NSSSocketControl.h

@@ -9,6 +9,11 @@
 
 #include "CommonSocketControl.h"
 #include "SharedSSLState.h"
+#include "TLSClientAuthCertSelection.h"
+
+extern mozilla::LazyLogModule gPIPNSSLog;
+
+class SelectClientAuthCertificate;
 
 class NSSSocketControl final : public CommonSocketControl {
  public:
@@ -118,13 +123,14 @@ class NSSSocketControl final : public CommonSocketControl {
 
   mozilla::psm::SharedSSLState& SharedState();
 
-  // XXX: These are only used on for diagnostic purposes
   enum CertVerificationState {
-    before_cert_verification,
-    waiting_for_cert_verification,
-    after_cert_verification
+    BeforeCertVerification,
+    WaitingForCertVerification,
+    AfterCertVerification
   };
+
   void SetCertVerificationWaiting();
+
   // Use errorCode == 0 to indicate success;
   void SetCertVerificationResult(PRErrorCode errorCode) override;
 
@@ -132,10 +138,9 @@ class NSSSocketControl final : public CommonSocketControl {
       nsTArray<uint8_t>& certBytes,
       nsTArray<nsTArray<uint8_t>>& certChainBytes);
 
-  // for logging only
-  PRBool IsWaitingForCertVerification() const {
+  bool IsWaitingForCertVerification() const {
     COMMON_SOCKET_CONTROL_ASSERT_ON_OWNING_THREAD();
-    return mCertVerificationState == waiting_for_cert_verification;
+    return mCertVerificationState == WaitingForCertVerification;
   }
   void AddPlaintextBytesRead(uint64_t val) {
     COMMON_SOCKET_CONTROL_ASSERT_ON_OWNING_THREAD();
@@ -218,8 +223,32 @@ class NSSSocketControl final : public CommonSocketControl {
   void SetPreliminaryHandshakeInfo(const SSLChannelInfo& channelInfo,
                                    const SSLCipherSuiteInfo& cipherInfo);
 
+  void SetPendingSelectClientAuthCertificate(
+      nsCOMPtr<nsIRunnable>&& selectClientAuthCertificate) {
+    COMMON_SOCKET_CONTROL_ASSERT_ON_OWNING_THREAD();
+    MOZ_LOG(
+        gPIPNSSLog, mozilla::LogLevel::Debug,
+        ("[%p] setting pending select client auth certificate", (void*)mFd));
+    mPendingSelectClientAuthCertificate =
+        std::move(selectClientAuthCertificate);
+  }
+
+  void MaybeDispatchSelectClientAuthCertificate() {
+    COMMON_SOCKET_CONTROL_ASSERT_ON_OWNING_THREAD();
+    if (!IsWaitingForCertVerification() &&
+        mPendingSelectClientAuthCertificate) {
+      MOZ_LOG(gPIPNSSLog, mozilla::LogLevel::Debug,
+              ("[%p] dispatching pending select client auth certificate",
+               (void*)mFd));
+      mozilla::Unused << NS_DispatchToMainThread(
+          mPendingSelectClientAuthCertificate);
+      mPendingSelectClientAuthCertificate = nullptr;
+    }
+  }
+
  private:
   ~NSSSocketControl() = default;
+
   PRFileDesc* mFd;
 
   CertVerificationState mCertVerificationState;
@@ -270,6 +299,8 @@ class NSSSocketControl final : public CommonSocketControl {
   mozilla::TimeStamp mSocketCreationTimestamp;
   uint64_t mPlaintextBytesRead;
 
+  nsCOMPtr<nsIRunnable> mPendingSelectClientAuthCertificate;
+
   // Regarding the client certificate message in the TLS handshake, RFC 5246
   // (TLS 1.2) says:
   //   If the certificate_authorities list in the certificate request

security/manager/ssl/TLSClientAuthCertSelection.cpp

@@ -98,34 +98,6 @@ static bool hasExplicitKeyUsageNonRepudiation(CERTCertificate* cert) {
   return !!(keyUsage & KU_NON_REPUDIATION);
 }
 
-// This class is used to store the needed information for invoking the client
-// cert selection UI.
-class ClientAuthInfo final {
- public:
-  explicit ClientAuthInfo(const nsACString& hostName,
-                          const mozilla::OriginAttributes& originAttributes,
-                          int32_t port, uint32_t providerFlags,
-                          uint32_t providerTlsFlags);
-  ~ClientAuthInfo() = default;
-  ClientAuthInfo(ClientAuthInfo&& aOther) noexcept;
-
-  const nsACString& HostName() const;
-  const mozilla::OriginAttributes& OriginAttributesRef() const;
-  int32_t Port() const;
-  uint32_t ProviderFlags() const;
-  uint32_t ProviderTlsFlags() const;
-
-  ClientAuthInfo(const ClientAuthInfo&) = delete;
-  void operator=(const ClientAuthInfo&) = delete;
-
- private:
-  nsCString mHostName;
-  mozilla::OriginAttributes mOriginAttributes;
-  int32_t mPort;
-  uint32_t mProviderFlags;
-  uint32_t mProviderTlsFlags;
-};
-
 ClientAuthInfo::ClientAuthInfo(const nsACString& hostName,
                                const OriginAttributes& originAttributes,
                                int32_t port, uint32_t providerFlags,
@@ -435,41 +407,6 @@ ClientAuthCertificateSelected::Run() {
   return NS_OK;
 }
 
-// Helper runnable to select a client authentication certificate. Gets created
-// on the socket thread or an IPC thread, runs on the main thread, and then runs
-// its continuation on the socket thread.
-class SelectClientAuthCertificate : public Runnable {
- public:
-  SelectClientAuthCertificate(ClientAuthInfo&& info,
-                              UniqueCERTCertificate&& serverCert,
-                              nsTArray<nsTArray<uint8_t>>&& caNames,
-                              UniqueCERTCertList&& potentialClientCertificates,
-                              ClientAuthCertificateSelectedBase* continuation)
-      : Runnable("SelectClientAuthCertificate"),
-        mInfo(std::move(info)),
-        mServerCert(std::move(serverCert)),
-        mCANames(std::move(caNames)),
-        mPotentialClientCertificates(std::move(potentialClientCertificates)),
-        mContinuation(continuation) {}
-
-  NS_IMETHOD Run() override;
-
- private:
-  mozilla::pkix::Result BuildChainForCertificate(
-      nsTArray<uint8_t>& certBytes,
-      nsTArray<nsTArray<uint8_t>>& certChainBytes);
-  void DoSelectClientAuthCertificate();
-
-  ClientAuthInfo mInfo;
-  UniqueCERTCertificate mServerCert;
-  nsTArray<nsTArray<uint8_t>> mCANames;
-  UniqueCERTCertList mPotentialClientCertificates;
-  RefPtr<ClientAuthCertificateSelectedBase> mContinuation;
-
-  nsTArray<nsTArray<uint8_t>> mEnterpriseCertificates;
-  nsTArray<uint8_t> mSelectedCertBytes;
-};
-
 NS_IMETHODIMP
 SelectClientAuthCertificate::Run() {
   DoSelectClientAuthCertificate();
@@ -793,12 +730,6 @@ SECStatus SSLGetClientAuthDataHook(void* arg, PRFileDesc* socket,
   // appropriate information to the NSSSocketControl, which then calls
   // SSL_ClientCertCallbackComplete to continue the connection.
   if (XRE_IsSocketProcess()) {
-    mozilla::ipc::PBackgroundChild* actorChild = mozilla::ipc::BackgroundChild::
-        GetOrCreateForSocketParentBridgeForCurrentThread();
-    if (!actorChild) {
-      PR_SetError(SEC_ERROR_LIBRARY_FAILURE, 0);
-      return SECFailure;
-    }
     RefPtr<SelectTLSClientAuthCertChild> selectClientAuthCertificate(
         new SelectTLSClientAuthCertChild(continuation));
     nsAutoCString hostname(info->GetHostName());
@@ -809,26 +740,41 @@ SECStatus SSLGetClientAuthDataHook(void* arg, PRFileDesc* socket,
     }
     serverCertBytes.AppendElements(serverCert->derCert.data,
                                    serverCert->derCert.len);
-    if (!actorChild->SendPSelectTLSClientAuthCertConstructor(
-            selectClientAuthCertificate, hostname, info->GetOriginAttributes(),
-            info->GetPort(), info->GetProviderFlags(),
-            info->GetProviderTlsFlags(), ByteArray(serverCertBytes),
-            caNamesBytes)) {
-      PR_SetError(SEC_ERROR_LIBRARY_FAILURE, 0);
-      return SECFailure;
-    }
+    OriginAttributes originAttributes(info->GetOriginAttributes());
+    int32_t port(info->GetPort());
+    uint32_t providerFlags(info->GetProviderFlags());
+    uint32_t providerTlsFlags(info->GetProviderTlsFlags());
+    nsCOMPtr<nsIRunnable> remoteSelectClientAuthCertificate(
+        NS_NewRunnableFunction(
+            "RemoteSelectClientAuthCertificate",
+            [selectClientAuthCertificate(
+                 std::move(selectClientAuthCertificate)),
+             hostname(std::move(hostname)),
+             originAttributes(std::move(originAttributes)), port, providerFlags,
+             providerTlsFlags, serverCertBytes(std::move(serverCertBytes)),
+             caNamesBytes(std::move(caNamesBytes))]() {
+              mozilla::ipc::PBackgroundChild* actorChild =
+                  mozilla::ipc::BackgroundChild::
+                      GetOrCreateForSocketParentBridgeForCurrentThread();
+              if (actorChild) {
+                Unused << actorChild->SendPSelectTLSClientAuthCertConstructor(
+                    selectClientAuthCertificate, hostname, originAttributes,
+                    port, providerFlags, providerTlsFlags,
+                    ByteArray(serverCertBytes), caNamesBytes);
+              }
+            }));
+    info->SetPendingSelectClientAuthCertificate(
+        std::move(remoteSelectClientAuthCertificate));
   } else {
     ClientAuthInfo authInfo(info->GetHostName(), info->GetOriginAttributes(),
                             info->GetPort(), info->GetProviderFlags(),
                             info->GetProviderTlsFlags());
-    RefPtr<SelectClientAuthCertificate> selectClientAuthCertificate(
+    nsCOMPtr<nsIRunnable> selectClientAuthCertificate(
         new SelectClientAuthCertificate(
             std::move(authInfo), std::move(serverCert), std::move(caNames),
             std::move(potentialClientCertificates), continuation));
-    if (NS_FAILED(NS_DispatchToMainThread(selectClientAuthCertificate))) {
-      PR_SetError(SEC_ERROR_LIBRARY_FAILURE, 0);
-      return SECFailure;
-    }
+    info->SetPendingSelectClientAuthCertificate(
+        std::move(selectClientAuthCertificate));
   }
 
   // Meanwhile, tell NSS this connection is blocking for now.

security/manager/ssl/TLSClientAuthCertSelection.h

@@ -13,6 +13,8 @@
 #include "nsThreadUtils.h"
 #include "ssl.h"
 
+class NSSSocketControl;
+
 // NSS callback to select a client authentication certificate. See documentation
 // at the top of TLSClientAuthCertSelection.cpp.
 SECStatus SSLGetClientAuthDataHook(void* arg, PRFileDesc* socket,
@@ -52,4 +54,67 @@ class ClientAuthCertificateSelected : public ClientAuthCertificateSelectedBase {
   RefPtr<NSSSocketControl> mSocketInfo;
 };
 
+// This class is used to store the needed information for invoking the client
+// cert selection UI.
+class ClientAuthInfo final {
+ public:
+  explicit ClientAuthInfo(const nsACString& hostName,
+                          const mozilla::OriginAttributes& originAttributes,
+                          int32_t port, uint32_t providerFlags,
+                          uint32_t providerTlsFlags);
+  ~ClientAuthInfo() = default;
+  ClientAuthInfo(ClientAuthInfo&& aOther) noexcept;
+
+  const nsACString& HostName() const;
+  const mozilla::OriginAttributes& OriginAttributesRef() const;
+  int32_t Port() const;
+  uint32_t ProviderFlags() const;
+  uint32_t ProviderTlsFlags() const;
+
+  ClientAuthInfo(const ClientAuthInfo&) = delete;
+  void operator=(const ClientAuthInfo&) = delete;
+
+ private:
+  nsCString mHostName;
+  mozilla::OriginAttributes mOriginAttributes;
+  int32_t mPort;
+  uint32_t mProviderFlags;
+  uint32_t mProviderTlsFlags;
+};
+
+// Helper runnable to select a client authentication certificate. Gets created
+// on the socket thread or an IPC thread, runs on the main thread, and then runs
+// its continuation on the socket thread.
+class SelectClientAuthCertificate : public mozilla::Runnable {
+ public:
+  SelectClientAuthCertificate(
+      ClientAuthInfo&& info, mozilla::UniqueCERTCertificate&& serverCert,
+      nsTArray<nsTArray<uint8_t>>&& caNames,
+      mozilla::UniqueCERTCertList&& potentialClientCertificates,
+      ClientAuthCertificateSelectedBase* continuation)
+      : Runnable("SelectClientAuthCertificate"),
+        mInfo(std::move(info)),
+        mServerCert(std::move(serverCert)),
+        mCANames(std::move(caNames)),
+        mPotentialClientCertificates(std::move(potentialClientCertificates)),
+        mContinuation(continuation) {}
+
+  NS_IMETHOD Run() override;
+
+ private:
+  mozilla::pkix::Result BuildChainForCertificate(
+      nsTArray<uint8_t>& certBytes,
+      nsTArray<nsTArray<uint8_t>>& certChainBytes);
+  void DoSelectClientAuthCertificate();
+
+  ClientAuthInfo mInfo;
+  mozilla::UniqueCERTCertificate mServerCert;
+  nsTArray<nsTArray<uint8_t>> mCANames;
+  mozilla::UniqueCERTCertList mPotentialClientCertificates;
+  RefPtr<ClientAuthCertificateSelectedBase> mContinuation;
+
+  nsTArray<nsTArray<uint8_t>> mEnterpriseCertificates;
+  nsTArray<uint8_t> mSelectedCertBytes;
+};
+
 #endif  // SECURITY_MANAGER_SSL_TLSCLIENTAUTHCERTSELECTION_H_

security/manager/ssl/nsNSSIOLayer.cpp

@@ -735,6 +735,8 @@ static int16_t nsSSLIOLayerPoll(PRFileDesc* fd, int16_t in_flags,
                : "[%p] poll SSL socket using lower %d\n",
            fd, (int)in_flags));
 
+  socketInfo->MaybeDispatchSelectClientAuthCertificate();
+
   // We want the handshake to continue during certificate validation, so we
   // don't need to do anything special here. libssl automatically blocks when
   // it reaches any point that would be unsafe to send/receive something before

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

@@ -159,6 +159,8 @@ add_setup(async function() {
  *
  * @param {string} prefValue
  *        Value to set the "security.default_personal_cert" pref to.
+ * @param {string} urlToNavigate
+ *        The URL to navigate to.
  * @param {string} expectedURL
  *        If the connection is expected to load successfully, the URL that
  *        should load. If the connection is expected to fail and result in an
@@ -173,6 +175,7 @@ add_setup(async function() {
  */
 async function testHelper(
   prefValue,
+  urlToNavigate,
   expectedURL,
   expectCallingChooseCertificate,
   options = undefined,
@@ -185,10 +188,7 @@ async function testHelper(
 
   let win = await BrowserTestUtils.openNewBrowserWindow(options);
 
-  BrowserTestUtils.loadURIString(
-    win.gBrowser.selectedBrowser,
-    "https://requireclientcert.example.com:443"
-  );
+  BrowserTestUtils.loadURIString(win.gBrowser.selectedBrowser, urlToNavigate);
   if (expectedURL) {
     await BrowserTestUtils.browserLoaded(
       win.gBrowser.selectedBrowser,
@@ -250,6 +250,7 @@ add_task(async function testCertChosenAutomatically() {
   await testHelper(
     "Select Automatically",
     "https://requireclientcert.example.com/",
+    "https://requireclientcert.example.com/",
     false
   );
   // This clears all saved client auth certificate state so we don't influence
@@ -263,6 +264,7 @@ add_task(async function testCertNotChosenByUser() {
   gClientAuthDialogs.state = DialogState.RETURN_CERT_NOT_SELECTED;
   await testHelper(
     "Ask Every Time",
+    "https://requireclientcert.example.com/",
     undefined,
     true,
     undefined,
@@ -280,6 +282,7 @@ add_task(async function testCertChosenByUser() {
   await testHelper(
     "Ask Every Time",
     "https://requireclientcert.example.com/",
+    "https://requireclientcert.example.com/",
     true
   );
   cars.clearRememberedDecisions();
@@ -289,8 +292,18 @@ add_task(async function testCertChosenByUser() {
 add_task(async function testEmptyCertChosenByUser() {
   gClientAuthDialogs.state = DialogState.RETURN_CERT_NOT_SELECTED;
   gClientAuthDialogs.rememberClientAuthCertificate = true;
-  await testHelper("Ask Every Time", undefined, true);
-  await testHelper("Ask Every Time", undefined, false);
+  await testHelper(
+    "Ask Every Time",
+    "https://requireclientcert.example.com/",
+    undefined,
+    true
+  );
+  await testHelper(
+    "Ask Every Time",
+    "https://requireclientcert.example.com/",
+    undefined,
+    false
+  );
   cars.clearRememberedDecisions();
 });
 
@@ -308,13 +321,6 @@ add_task(async function testClearPrivateBrowsingState() {
   await testHelper(
     "Ask Every Time",
     "https://requireclientcert.example.com/",
-    true,
-    {
-      private: true,
-    }
-  );
-  await testHelper(
-    "Ask Every Time",
     "https://requireclientcert.example.com/",
     true,
     {
@@ -324,6 +330,16 @@ add_task(async function testClearPrivateBrowsingState() {
   await testHelper(
     "Ask Every Time",
     "https://requireclientcert.example.com/",
+    "https://requireclientcert.example.com/",
+    true,
+    {
+      private: true,
+    }
+  );
+  await testHelper(
+    "Ask Every Time",
+    "https://requireclientcert.example.com/",
+    "https://requireclientcert.example.com/",
     true
   );
   // NB: we don't `cars.clearRememberedDecisions()` in between the two calls to
@@ -359,6 +375,7 @@ add_task(async function testCertFilteringWithIntermediate() {
   await testHelper(
     "Ask Every Time",
     "https://requireclientcert.example.com/",
+    "https://requireclientcert.example.com/",
     true
   );
   cars.clearRememberedDecisions();
@@ -367,3 +384,18 @@ add_task(async function testCertFilteringWithIntermediate() {
     set: [["security.enterprise_roots.enabled", true]],
   });
 });
+
+// Test that if the server certificate does not validate successfully,
+// nsIClientAuthDialogs.chooseCertificate() is never called.
+add_task(async function testNoDialogForUntrustedServerCertificate() {
+  gClientAuthDialogs.state = DialogState.ASSERT_NOT_CALLED;
+  await testHelper(
+    "Ask Every Time",
+    "https://requireclientcert-untrusted.example.com/",
+    undefined,
+    false
+  );
+  // This clears all saved client auth certificate state so we don't influence
+  // subsequent tests.
+  cars.clearRememberedDecisions();
+});