diff --git a/build/pgo/certs/cert9.db b/build/pgo/certs/cert9.db index 316cd8292384..0a3572c10a64 100644 Binary files a/build/pgo/certs/cert9.db and b/build/pgo/certs/cert9.db differ diff --git a/build/pgo/certs/key4.db b/build/pgo/certs/key4.db index fe14fa2eecca..189a620bbc41 100644 Binary files a/build/pgo/certs/key4.db and b/build/pgo/certs/key4.db differ diff --git a/build/pgo/certs/mochitest.client b/build/pgo/certs/mochitest.client index ce71cc6ff261..eb96da60e259 100644 Binary files a/build/pgo/certs/mochitest.client and b/build/pgo/certs/mochitest.client differ diff --git a/build/pgo/server-locations.txt b/build/pgo/server-locations.txt index dc5db0d81d70..be5394ac1c6b 100644 --- a/build/pgo/server-locations.txt +++ b/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 diff --git a/security/manager/ssl/NSSSocketControl.cpp b/security/manager/ssl/NSSSocketControl.cpp index 7573085fcd0a..94674e0b75a4 100644 --- a/security/manager/ssl/NSSSocketControl.cpp +++ b/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(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, diff --git a/security/manager/ssl/NSSSocketControl.h b/security/manager/ssl/NSSSocketControl.h index 7c87ff6305fc..69f5a3fa43f1 100644 --- a/security/manager/ssl/NSSSocketControl.h +++ b/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& certBytes, nsTArray>& 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&& 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 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 diff --git a/security/manager/ssl/TLSClientAuthCertSelection.cpp b/security/manager/ssl/TLSClientAuthCertSelection.cpp index 818c214d7f97..5d9066d069b8 100644 --- a/security/manager/ssl/TLSClientAuthCertSelection.cpp +++ b/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>&& 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& certBytes, - nsTArray>& certChainBytes); - void DoSelectClientAuthCertificate(); - - ClientAuthInfo mInfo; - UniqueCERTCertificate mServerCert; - nsTArray> mCANames; - UniqueCERTCertList mPotentialClientCertificates; - RefPtr mContinuation; - - nsTArray> mEnterpriseCertificates; - nsTArray 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 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 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( + nsCOMPtr 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. diff --git a/security/manager/ssl/TLSClientAuthCertSelection.h b/security/manager/ssl/TLSClientAuthCertSelection.h index a0315b9c69f6..c1e90fc77597 100644 --- a/security/manager/ssl/TLSClientAuthCertSelection.h +++ b/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 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>&& 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& certBytes, + nsTArray>& certChainBytes); + void DoSelectClientAuthCertificate(); + + ClientAuthInfo mInfo; + mozilla::UniqueCERTCertificate mServerCert; + nsTArray> mCANames; + mozilla::UniqueCERTCertList mPotentialClientCertificates; + RefPtr mContinuation; + + nsTArray> mEnterpriseCertificates; + nsTArray mSelectedCertBytes; +}; + #endif // SECURITY_MANAGER_SSL_TLSCLIENTAUTHCERTSELECTION_H_ diff --git a/security/manager/ssl/nsNSSIOLayer.cpp b/security/manager/ssl/nsNSSIOLayer.cpp index 756d73617ffe..952e8f6a4d4f 100644 --- a/security/manager/ssl/nsNSSIOLayer.cpp +++ b/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 diff --git a/security/manager/ssl/tests/mochitest/browser/browser_clientAuth_connection.js b/security/manager/ssl/tests/mochitest/browser/browser_clientAuth_connection.js index 4a93dc1c4e50..367343c2fe2b 100644 --- a/security/manager/ssl/tests/mochitest/browser/browser_clientAuth_connection.js +++ b/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(); +});