commit ea45be222fc0e0544cb5b34d4e03e5e9cbabb419 parent e458ba3e1e13a7ccc0b257afe827fc7317eda01f Author: Cristina Horotan <chorotan@mozilla.com> Date: Wed, 15 Oct 2025 02:25:01 +0300 Revert "Bug 1994034 - make TLS handshake certificates always available r=jschanck,necko-reviewers,extension-reviewers,kershaw,robwu" for causing mochitest failures on test_cache_padding.html This reverts commit ed038d676033bf17672b92c44e08ef4776e7e484. Diffstat:
19 files changed, 201 insertions(+), 159 deletions(-)
diff --git a/browser/base/content/pageinfo/security.js b/browser/base/content/pageinfo/security.js @@ -92,7 +92,7 @@ var security = { if (secInfo.succeededCertChain.length) { certChainArray = secInfo.succeededCertChain; } else { - certChainArray = secInfo.handshakeCertificates; + certChainArray = secInfo.failedCertChain; } retval = { diff --git a/browser/base/content/test/about/browser_aboutCertError.js b/browser/base/content/test/about/browser_aboutCertError.js @@ -225,14 +225,14 @@ add_task(async function checkAdvancedDetails() { "none", "Debug information is visible" ); - let handshakeCertificates = - content.docShell.failedChannel.securityInfo.handshakeCertificates.map( - cert => cert.getBase64DERString() + let failedCertChain = + content.docShell.failedChannel.securityInfo.failedCertChain.map(cert => + cert.getBase64DERString() ); return { divDisplay: content.getComputedStyle(div).display, text: text.textContent, - handshakeCertificates, + failedCertChain, }; }); isnot(message.divDisplay, "none", "Debug information is visible"); @@ -249,7 +249,7 @@ add_task(async function checkAdvancedDetails() { message.text.includes("HTTP Public Key Pinning: false"), "Correct HPKP value found" ); - let certChain = getCertChainAsString(message.handshakeCertificates); + let certChain = getCertChainAsString(message.failedCertChain); ok(message.text.includes(certChain), "Found certificate chain"); BrowserTestUtils.removeTab(gBrowser.selectedTab); @@ -307,14 +307,14 @@ add_task(async function checkAdvancedDetailsForHSTS() { errorCode.click(); let div = doc.getElementById("certificateErrorDebugInformation"); let text = doc.getElementById("certificateErrorText"); - let handshakeCertificates = - content.docShell.failedChannel.securityInfo.handshakeCertificates.map( - cert => cert.getBase64DERString() + let failedCertChain = + content.docShell.failedChannel.securityInfo.failedCertChain.map(cert => + cert.getBase64DERString() ); return { divDisplay: content.getComputedStyle(div).display, text: text.textContent, - handshakeCertificates, + failedCertChain, }; }); isnot(message.divDisplay, "none", "Debug information is visible"); @@ -333,7 +333,7 @@ add_task(async function checkAdvancedDetailsForHSTS() { message.text.includes("HTTP Public Key Pinning: true"), "Correct HPKP value found" ); - let certChain = getCertChainAsString(message.handshakeCertificates); + let certChain = getCertChainAsString(message.failedCertChain); ok(message.text.includes(certChain), "Found certificate chain"); BrowserTestUtils.removeTab(gBrowser.selectedTab); diff --git a/dom/base/Document.cpp b/dom/base/Document.cpp @@ -1916,19 +1916,19 @@ void Document::GetFailedCertSecurityInfo(FailedCertSecurityInfo& aInfo, int64_t maxValidity = std::numeric_limits<int64_t>::max(); int64_t minValidity = 0; PRTime notBefore, notAfter; - nsTArray<RefPtr<nsIX509Cert>> handshakeCertificates; - rv = tsi->GetHandshakeCertificates(handshakeCertificates); + nsTArray<RefPtr<nsIX509Cert>> failedCertArray; + rv = tsi->GetFailedCertChain(failedCertArray); if (NS_WARN_IF(NS_FAILED(rv))) { aRv.Throw(rv); return; } - if (NS_WARN_IF(handshakeCertificates.IsEmpty())) { + if (NS_WARN_IF(failedCertArray.IsEmpty())) { aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR); return; } - for (const auto& certificate : handshakeCertificates) { + for (const auto& certificate : failedCertArray) { rv = certificate->GetIssuerCommonName(issuerCommonName); if (NS_WARN_IF(NS_FAILED(rv))) { aRv.Throw(rv); diff --git a/netwerk/base/FuzzySecurityInfo.cpp b/netwerk/base/FuzzySecurityInfo.cpp @@ -37,8 +37,8 @@ FuzzySecurityInfo::GetErrorCodeString(nsAString& aErrorString) { } NS_IMETHODIMP -FuzzySecurityInfo::GetHandshakeCertificates( - nsTArray<RefPtr<nsIX509Cert>>& aHandshakeCertificates) { +FuzzySecurityInfo::GetFailedCertChain( + nsTArray<RefPtr<nsIX509Cert>>& aFailedCertChain) { MOZ_CRASH("Unused"); return NS_OK; } diff --git a/netwerk/base/SSLTokensCache.cpp b/netwerk/base/SSLTokensCache.cpp @@ -49,10 +49,10 @@ SessionCacheInfo SessionCacheInfo::Clone() const { : Nothing(); result.mIsBuiltCertChainRootBuiltInRoot = mIsBuiltCertChainRootBuiltInRoot; result.mOverridableErrorCategory = mOverridableErrorCategory; - result.mHandshakeCertificatesBytes = - mHandshakeCertificatesBytes + result.mFailedCertChainBytes = + mFailedCertChainBytes ? Some(TransformIntoNewArray( - *mHandshakeCertificatesBytes, + *mFailedCertChainBytes, [](const auto& element) { return element.Clone(); })) : Nothing(); return result; @@ -81,9 +81,8 @@ uint32_t SSLTokensCache::TokenCacheRecord::Size() const { size += cert.Length(); } } - if (mSessionCacheInfo.mHandshakeCertificatesBytes) { - for (const auto& cert : - mSessionCacheInfo.mHandshakeCertificatesBytes.ref()) { + if (mSessionCacheInfo.mFailedCertChainBytes) { + for (const auto& cert : mSessionCacheInfo.mFailedCertChainBytes.ref()) { size += cert.Length(); } } @@ -101,7 +100,7 @@ void SSLTokensCache::TokenCacheRecord::Reset() { mSessionCacheInfo.mIsBuiltCertChainRootBuiltInRoot.reset(); mSessionCacheInfo.mOverridableErrorCategory = nsITransportSecurityInfo::OverridableErrorCategory::ERROR_UNSET; - mSessionCacheInfo.mHandshakeCertificatesBytes.reset(); + mSessionCacheInfo.mFailedCertChainBytes.reset(); } uint32_t SSLTokensCache::TokenCacheEntry::Size() const { @@ -292,21 +291,21 @@ nsresult SSLTokensCache::Put(const nsACString& aKey, const uint8_t* aToken, return rv; } - Maybe<nsTArray<nsTArray<uint8_t>>> handshakeCertificatesBytes; - nsTArray<RefPtr<nsIX509Cert>> handshakeCertificates; - rv = securityInfo->GetHandshakeCertificates(handshakeCertificates); + Maybe<nsTArray<nsTArray<uint8_t>>> failedCertChainBytes; + nsTArray<RefPtr<nsIX509Cert>> failedCertArray; + rv = securityInfo->GetFailedCertChain(failedCertArray); if (NS_FAILED(rv)) { return rv; } - if (!handshakeCertificates.IsEmpty()) { - handshakeCertificatesBytes.emplace(); - for (const auto& cert : handshakeCertificates) { + if (!failedCertArray.IsEmpty()) { + failedCertChainBytes.emplace(); + for (const auto& cert : failedCertArray) { nsTArray<uint8_t> rawCert; nsresult rv = cert->GetRawDER(rawCert); if (NS_FAILED(rv)) { return rv; } - handshakeCertificatesBytes->AppendElement(std::move(rawCert)); + failedCertChainBytes->AppendElement(std::move(rawCert)); } } @@ -328,8 +327,8 @@ nsresult SSLTokensCache::Put(const nsACString& aKey, const uint8_t* aToken, rec->mSessionCacheInfo.mIsBuiltCertChainRootBuiltInRoot = std::move(isBuiltCertChainRootBuiltInRoot); rec->mSessionCacheInfo.mOverridableErrorCategory = overridableErrorCategory; - rec->mSessionCacheInfo.mHandshakeCertificatesBytes = - std::move(handshakeCertificatesBytes); + rec->mSessionCacheInfo.mFailedCertChainBytes = + std::move(failedCertChainBytes); return rec; }; diff --git a/netwerk/base/SSLTokensCache.h b/netwerk/base/SSLTokensCache.h @@ -32,7 +32,7 @@ struct SessionCacheInfo { Maybe<nsTArray<nsTArray<uint8_t>>> mSucceededCertChainBytes; Maybe<bool> mIsBuiltCertChainRootBuiltInRoot; nsITransportSecurityInfo::OverridableErrorCategory mOverridableErrorCategory; - Maybe<nsTArray<nsTArray<uint8_t>>> mHandshakeCertificatesBytes; + Maybe<nsTArray<nsTArray<uint8_t>>> mFailedCertChainBytes; }; class SSLTokensCache : public nsIMemoryReporter { diff --git a/security/manager/ssl/CommonSocketControl.cpp b/security/manager/ssl/CommonSocketControl.cpp @@ -83,10 +83,10 @@ void CommonSocketControl::SetSucceededCertChain( return CreateCertChain(mSucceededCertChain, std::move(aCertList)); } -void CommonSocketControl::SetHandshakeCertificates( +void CommonSocketControl::SetFailedCertChain( nsTArray<nsTArray<uint8_t>>&& aCertList) { COMMON_SOCKET_CONTROL_ASSERT_ON_OWNING_THREAD(); - return CreateCertChain(mHandshakeCertificates, std::move(aCertList)); + return CreateCertChain(mFailedCertChain, std::move(aCertList)); } void CommonSocketControl::SetCanceled(PRErrorCode errorCode) { @@ -319,8 +319,8 @@ void CommonSocketControl::RebuildCertificateInfoFromSSLTokenCache() { SetIsBuiltCertChainRootBuiltInRoot(*info.mIsBuiltCertChainRootBuiltInRoot); } - if (info.mHandshakeCertificatesBytes) { - SetHandshakeCertificates(std::move(*info.mHandshakeCertificatesBytes)); + if (info.mFailedCertChainBytes) { + SetFailedCertChain(std::move(*info.mFailedCertChainBytes)); } } @@ -451,8 +451,8 @@ CommonSocketControl::GetSecurityInfo(nsITransportSecurityInfo** aSecurityInfo) { } nsCOMPtr<nsITransportSecurityInfo> securityInfo( new psm::TransportSecurityInfo( - mSecurityState, mErrorCode, mHandshakeCertificates.Clone(), - mServerCert, mSucceededCertChain.Clone(), mCipherSuite, mKeaGroupName, + mSecurityState, mErrorCode, mFailedCertChain.Clone(), mServerCert, + mSucceededCertChain.Clone(), mCipherSuite, mKeaGroupName, mSignatureSchemeName, mProtocolVersion, mCertificateTransparencyStatus, mIsAcceptedEch, mIsDelegatedCredential, mOverridableErrorCategory, mMadeOCSPRequests, diff --git a/security/manager/ssl/CommonSocketControl.h b/security/manager/ssl/CommonSocketControl.h @@ -87,7 +87,7 @@ class CommonSocketControl : public nsITLSSocketControl { nsITransportSecurityInfo::OverridableErrorCategory::ERROR_UNSET; } void SetSucceededCertChain(nsTArray<nsTArray<uint8_t>>&& certList); - void SetHandshakeCertificates(nsTArray<nsTArray<uint8_t>>&& certList); + void SetFailedCertChain(nsTArray<nsTArray<uint8_t>>&& certList); void SetIsBuiltCertChainRootBuiltInRoot( bool aIsBuiltCertChainRootBuiltInRoot) { COMMON_SOCKET_CONTROL_ASSERT_ON_OWNING_THREAD(); @@ -156,11 +156,9 @@ class CommonSocketControl : public nsITLSSocketControl { // Fields used to build a TransportSecurityInfo uint32_t mSecurityState; PRErrorCode mErrorCode; - // Certificates provided in the TLS handshake by the server. - nsTArray<RefPtr<nsIX509Cert>> mHandshakeCertificates; - // The server end-entity certificate. + // Peer cert chain for failed connections. + nsTArray<RefPtr<nsIX509Cert>> mFailedCertChain; nsCOMPtr<nsIX509Cert> mServerCert; - // The chain built during certificate validation, if successful. nsTArray<RefPtr<nsIX509Cert>> mSucceededCertChain; mozilla::Maybe<uint16_t> mCipherSuite; mozilla::Maybe<nsCString> mKeaGroupName; diff --git a/security/manager/ssl/SSLServerCertVerification.cpp b/security/manager/ssl/SSLServerCertVerification.cpp @@ -1168,13 +1168,13 @@ SSLServerCertVerificationResult::Run() { nsTArray<uint8_t> certBytes(mPeerCertChain.ElementAt(0).Clone()); nsCOMPtr<nsIX509Cert> cert(new nsNSSCertificate(std::move(certBytes))); mSocketControl->SetServerCert(cert, EVStatus::NotEV); + mSocketControl->SetFailedCertChain(std::move(mPeerCertChain)); if (mOverridableErrorCategory != nsITransportSecurityInfo::OverridableErrorCategory::ERROR_UNSET) { mSocketControl->SetStatusErrorBits(mOverridableErrorCategory); } } - mSocketControl->SetHandshakeCertificates(std::move(mPeerCertChain)); mSocketControl->SetCertVerificationResult(mFinalError); // Release this reference to the socket control so that it will be freed on // the socket thread. diff --git a/security/manager/ssl/TransportSecurityInfo.cpp b/security/manager/ssl/TransportSecurityInfo.cpp @@ -39,7 +39,7 @@ namespace psm { TransportSecurityInfo::TransportSecurityInfo( uint32_t aSecurityState, PRErrorCode aErrorCode, - nsTArray<RefPtr<nsIX509Cert>>&& aHandshakeCertificates, + nsTArray<RefPtr<nsIX509Cert>>&& aFailedCertChain, nsCOMPtr<nsIX509Cert>& aServerCert, nsTArray<RefPtr<nsIX509Cert>>&& aSucceededCertChain, Maybe<uint16_t> aCipherSuite, Maybe<nsCString> aKeaGroupName, @@ -52,7 +52,7 @@ TransportSecurityInfo::TransportSecurityInfo( bool aIsBuiltCertChainRootBuiltInRoot, const nsCString& aPeerId) : mSecurityState(aSecurityState), mErrorCode(aErrorCode), - mHandshakeCertificates(std::move(aHandshakeCertificates)), + mFailedCertChain(std::move(aFailedCertChain)), mServerCert(aServerCert), mSucceededCertChain(std::move(aSucceededCertChain)), mCipherSuite(aCipherSuite), @@ -201,9 +201,9 @@ TransportSecurityInfo::ToString(nsACString& aResult) { NS_ENSURE_SUCCESS(rv, rv); } // END moved from nsISSLStatus - rv = objStream->Write16(mHandshakeCertificates.Length()); + rv = objStream->Write16(mFailedCertChain.Length()); NS_ENSURE_SUCCESS(rv, rv); - for (const auto& cert : mHandshakeCertificates) { + for (const auto& cert : mFailedCertChain) { rv = objStream->WriteCompoundObject(cert, NS_GET_IID(nsIX509Cert), true); NS_ENSURE_SUCCESS(rv, rv); } @@ -409,8 +409,8 @@ nsresult TransportSecurityInfo::ReadSSLStatus( } // Read only to consume bytes from the stream. - nsTArray<RefPtr<nsIX509Cert>> handshakeCertificates; - rv = ReadCertList(aStream, handshakeCertificates); + nsTArray<RefPtr<nsIX509Cert>> failedCertChain; + rv = ReadCertList(aStream, failedCertChain); if (NS_FAILED(rv)) { return rv; } @@ -540,7 +540,7 @@ nsresult TransportSecurityInfo::Read(const nsCString& aSerializedSecurityInfo, uint32_t aSecurityState = 0; PRErrorCode aErrorCode = 0; - nsTArray<RefPtr<nsIX509Cert>> aHandshakeCertificates; + nsTArray<RefPtr<nsIX509Cert>> aFailedCertChain; nsCOMPtr<nsIX509Cert> aServerCert; nsTArray<RefPtr<nsIX509Cert>> aSucceededCertChain; Maybe<uint16_t> aCipherSuite; @@ -703,15 +703,14 @@ nsresult TransportSecurityInfo::Read(const nsCString& aSerializedSecurityInfo, // END moved from nsISSLStatus if (serVersionParsedToInt < 3) { // The old data structure of certList(nsIX509CertList) presents - rv = ReadCertList(objStream, aHandshakeCertificates); + rv = ReadCertList(objStream, aFailedCertChain); NS_ENSURE_SUCCESS(rv, rv); } else { uint16_t certCount; rv = objStream->Read16(&certCount); NS_ENSURE_SUCCESS(rv, rv); - rv = ReadCertificatesFromStream(objStream, certCount, - aHandshakeCertificates); + rv = ReadCertificatesFromStream(objStream, certCount, aFailedCertChain); NS_ENSURE_SUCCESS(rv, rv); } @@ -792,8 +791,8 @@ nsresult TransportSecurityInfo::Read(const nsCString& aSerializedSecurityInfo, } RefPtr<nsITransportSecurityInfo> securityInfo(new TransportSecurityInfo( - aSecurityState, aErrorCode, std::move(aHandshakeCertificates), - aServerCert, std::move(aSucceededCertChain), aCipherSuite, aKeaGroupName, + aSecurityState, aErrorCode, std::move(aFailedCertChain), aServerCert, + std::move(aSucceededCertChain), aCipherSuite, aKeaGroupName, aSignatureSchemeName, aProtocolVersion, aCertificateTransparencyStatus, aIsAcceptedEch, aIsDelegatedCredential, aOverridableErrorCategory, aMadeOCSPRequests, aUsedPrivateDNS, aIsEV, aNPNCompleted, aNegotiatedNPN, @@ -805,7 +804,7 @@ nsresult TransportSecurityInfo::Read(const nsCString& aSerializedSecurityInfo, void TransportSecurityInfo::SerializeToIPC(IPC::MessageWriter* aWriter) { WriteParam(aWriter, mSecurityState); WriteParam(aWriter, mErrorCode); - WriteParam(aWriter, mHandshakeCertificates); + WriteParam(aWriter, mFailedCertChain); WriteParam(aWriter, mServerCert); WriteParam(aWriter, mSucceededCertChain); WriteParam(aWriter, mCipherSuite); @@ -830,7 +829,7 @@ bool TransportSecurityInfo::DeserializeFromIPC( IPC::MessageReader* aReader, RefPtr<nsITransportSecurityInfo>* aResult) { uint32_t aSecurityState; PRErrorCode aErrorCode; - nsTArray<RefPtr<nsIX509Cert>> aHandshakeCertificates; + nsTArray<RefPtr<nsIX509Cert>> aFailedCertChain; nsCOMPtr<nsIX509Cert> aServerCert; nsTArray<RefPtr<nsIX509Cert>> aSucceededCertChain; Maybe<uint16_t> aCipherSuite; @@ -852,7 +851,7 @@ bool TransportSecurityInfo::DeserializeFromIPC( if (!ReadParam(aReader, &aSecurityState) || !ReadParam(aReader, &aErrorCode) || - !ReadParam(aReader, &aHandshakeCertificates) || + !ReadParam(aReader, &aFailedCertChain) || !ReadParam(aReader, &aServerCert) || !ReadParam(aReader, &aSucceededCertChain) || !ReadParam(aReader, &aCipherSuite) || @@ -873,8 +872,8 @@ bool TransportSecurityInfo::DeserializeFromIPC( } RefPtr<nsITransportSecurityInfo> securityInfo(new TransportSecurityInfo( - aSecurityState, aErrorCode, std::move(aHandshakeCertificates), - aServerCert, std::move(aSucceededCertChain), aCipherSuite, aKeaGroupName, + aSecurityState, aErrorCode, std::move(aFailedCertChain), aServerCert, + std::move(aSucceededCertChain), aCipherSuite, aKeaGroupName, aSignatureSchemeName, aProtocolVersion, aCertificateTransparencyStatus, aIsAcceptedEch, aIsDelegatedCredential, aOverridableErrorCategory, aMadeOCSPRequests, aUsedPrivateDNS, aIsEV, aNPNCompleted, aNegotiatedNPN, @@ -884,13 +883,13 @@ bool TransportSecurityInfo::DeserializeFromIPC( } NS_IMETHODIMP -TransportSecurityInfo::GetHandshakeCertificates( - nsTArray<RefPtr<nsIX509Cert>>& aHandshakeCertificates) { - MOZ_ASSERT(aHandshakeCertificates.IsEmpty()); - if (!aHandshakeCertificates.IsEmpty()) { +TransportSecurityInfo::GetFailedCertChain( + nsTArray<RefPtr<nsIX509Cert>>& aFailedCertChain) { + MOZ_ASSERT(aFailedCertChain.IsEmpty()); + if (!aFailedCertChain.IsEmpty()) { return NS_ERROR_INVALID_ARG; } - aHandshakeCertificates.AppendElements(mHandshakeCertificates); + aFailedCertChain.AppendElements(mFailedCertChain); return NS_OK; } diff --git a/security/manager/ssl/TransportSecurityInfo.h b/security/manager/ssl/TransportSecurityInfo.h @@ -33,7 +33,7 @@ class TransportSecurityInfo : public nsITransportSecurityInfo { public: TransportSecurityInfo( uint32_t aSecurityState, PRErrorCode aErrorCode, - nsTArray<RefPtr<nsIX509Cert>>&& aHandshakeCertificates, + nsTArray<RefPtr<nsIX509Cert>>&& aFailedCertChain, nsCOMPtr<nsIX509Cert>& aServerCert, nsTArray<RefPtr<nsIX509Cert>>&& aSucceededCertChain, Maybe<uint16_t> aCipherSuite, Maybe<nsCString> aKeaGroupName, @@ -60,11 +60,9 @@ class TransportSecurityInfo : public nsITransportSecurityInfo { const uint32_t mSecurityState; const PRErrorCode mErrorCode; - // Certificates provided in the TLS handshake by the server. - const nsTArray<RefPtr<nsIX509Cert>> mHandshakeCertificates; - // The server end-entity certificate. + // Peer cert chain for failed connections. + const nsTArray<RefPtr<nsIX509Cert>> mFailedCertChain; const nsCOMPtr<nsIX509Cert> mServerCert; - // The chain built during certificate validation, if successful. const nsTArray<RefPtr<nsIX509Cert>> mSucceededCertChain; const mozilla::Maybe<uint16_t> mCipherSuite; const mozilla::Maybe<nsCString> mKeaGroupName; diff --git a/security/manager/ssl/nsITransportSecurityInfo.idl b/security/manager/ssl/nsITransportSecurityInfo.idl @@ -37,20 +37,13 @@ interface nsITransportSecurityInfo : nsISupports { */ /** - * The list of certificates provided by the server during the TLS - * handshake. + * If certificate verification failed, this will be the peer certificate + * chain provided in the handshake, so it can be used for error reporting. + * If verification succeeded, this will be empty. */ - readonly attribute Array<nsIX509Cert> handshakeCertificates; + readonly attribute Array<nsIX509Cert> failedCertChain; - /** - * The server certificate (also called "end-entity certificate"). - */ readonly attribute nsIX509Cert serverCert; - - /** - * If the server certificate verified successfully, this will consist of - * the verified path from the end-entity to the trust-anchor. - */ readonly attribute Array<nsIX509Cert> succeededCertChain; [must_use] diff --git a/security/manager/ssl/tests/gtest/DeserializeCertTest.cpp b/security/manager/ssl/tests/gtest/DeserializeCertTest.cpp @@ -32,7 +32,7 @@ using namespace mozilla::psm; // We would like to move away from this binary compatibility requirement // in service workers. See bug 1248628. void deserializeAndVerify(const nsCString& serializedSecInfo, - Maybe<size_t> handshakeCertificatesLength = Nothing(), + Maybe<size_t> failedCertChainLength = Nothing(), Maybe<size_t> succeededCertChainLength = Nothing()) { nsCOMPtr<nsITransportSecurityInfo> securityInfo; nsresult rv = TransportSecurityInfo::Read(serializedSecInfo, @@ -45,19 +45,18 @@ void deserializeAndVerify(const nsCString& serializedSecInfo, ASSERT_EQ(NS_OK, rv); ASSERT_TRUE(cert); - nsTArray<RefPtr<nsIX509Cert>> handshakeCertificatesArray; - rv = securityInfo->GetHandshakeCertificates(handshakeCertificatesArray); + nsTArray<RefPtr<nsIX509Cert>> failedCertArray; + rv = securityInfo->GetFailedCertChain(failedCertArray); ASSERT_EQ(NS_OK, rv); - if (handshakeCertificatesLength) { - ASSERT_FALSE(handshakeCertificatesArray.IsEmpty()); - for (const auto& cert : handshakeCertificatesArray) { + if (failedCertChainLength) { + ASSERT_FALSE(failedCertArray.IsEmpty()); + for (const auto& cert : failedCertArray) { ASSERT_TRUE(cert); } - ASSERT_EQ(*handshakeCertificatesLength, - handshakeCertificatesArray.Length()); + ASSERT_EQ(*failedCertChainLength, failedCertArray.Length()); } else { - ASSERT_TRUE(handshakeCertificatesArray.IsEmpty()); + ASSERT_TRUE(failedCertArray.IsEmpty()); } nsTArray<RefPtr<nsIX509Cert>> succeededCertArray; @@ -194,7 +193,7 @@ TEST(psm_DeserializeCert, preSSLStatusConsolidation) deserializeAndVerify(base64Serialization, Nothing(), Some(2)); } -TEST(psm_DeserializeCert, preSSLStatusConsolidationHandshakeCertificates) +TEST(psm_DeserializeCert, preSSLStatusConsolidationFailedCertChain) { // clang-format off // Generated using serialized output of test "expired.example.com" diff --git a/security/manager/ssl/tests/unit/test_cert_chains.js b/security/manager/ssl/tests/unit/test_cert_chains.js @@ -297,7 +297,98 @@ function test_cert_pkcs7_empty_array() { function run_test() { do_get_profile(); + add_tls_server_setup("BadCertAndPinningServer", "bad_certs"); - test_cert_pkcs7_export(); - test_cert_pkcs7_empty_array(); + add_test(function () { + test_cert_pkcs7_export(); + run_next_test(); + }); + + add_test(function () { + test_cert_pkcs7_empty_array(); + run_next_test(); + }); + + // Test successful connection (failedCertChain should be null) + add_connection_test( + // re-use pinning certs (keeler) + "good.include-subdomains.pinning.example.com", + PRErrorCodeSuccess, + null, + function withSecurityInfo(aTransportSecurityInfo) { + equal( + aTransportSecurityInfo.failedCertChain.length, + 0, + "failedCertChain for a successful connection should be null" + ); + } + ); + + // Test overrideable connection failure (failedCertChain should be non-null) + add_connection_test( + "expired.example.com", + SEC_ERROR_EXPIRED_CERTIFICATE, + null, + function withSecurityInfo(securityInfo) { + notEqual( + securityInfo.failedCertChain, + null, + "failedCertChain should not be null for an overrideable" + + " connection failure" + ); + let originalCertChain = build_cert_chain(["expired-ee", "test-ca"]); + ok( + areCertArraysEqual(originalCertChain, securityInfo.failedCertChain), + "failedCertChain should equal the original cert chain for an" + + " overrideable connection failure" + ); + } + ); + + // Test overrideable connection failure (failedCertChain should be non-null) + add_connection_test( + "unknownissuer.example.com", + SEC_ERROR_UNKNOWN_ISSUER, + null, + function withSecurityInfo(securityInfo) { + notEqual( + securityInfo.failedCertChain, + null, + "failedCertChain should not be null for an overrideable" + + " connection failure" + ); + let originalCertChain = build_cert_chain(["unknownissuer"]); + ok( + areCertArraysEqual(originalCertChain, securityInfo.failedCertChain), + "failedCertChain should equal the original cert chain for an" + + " overrideable connection failure" + ); + } + ); + + // Test non-overrideable error (failedCertChain should be non-null) + add_connection_test( + "inadequatekeyusage.example.com", + SEC_ERROR_INADEQUATE_KEY_USAGE, + null, + function withSecurityInfo(securityInfo) { + notEqual( + securityInfo.failedCertChain, + null, + "failedCertChain should not be null for a non-overrideable" + + " connection failure" + ); + let originalCertChain = build_cert_chain([ + "inadequatekeyusage-ee", + "test-ca", + ]); + ok( + areCertArraysEqual(originalCertChain, securityInfo.failedCertChain), + "failedCertChain should equal the original cert chain for a" + + " non-overrideable connection failure" + ); + } + ); + + run_next_test(); } diff --git a/security/manager/ssl/tests/unit/test_session_resumption.js b/security/manager/ssl/tests/unit/test_session_resumption.js @@ -52,9 +52,9 @@ function add_resume_non_ev_with_override_test() { "expired.example.com should not have succeededCertChain set" ); equal( - transportSecurityInfo.handshakeCertificates.length, + transportSecurityInfo.failedCertChain.length, 2, - "expired.example.com should have handshakeCertificates set" + "expired.example.com should have failedCertChain set" ); equal( transportSecurityInfo.overridableErrorCategory, @@ -106,9 +106,9 @@ function add_one_ev_test(resumed) { "ev-test.example.com should have succeededCertChain set" ); equal( - transportSecurityInfo.handshakeCertificates.length, - 2, - "ev-test.example.com should have handshakeCertificates set" + transportSecurityInfo.failedCertChain.length, + 0, + "ev-test.example.com should not have failedCertChain set" ); equal( transportSecurityInfo.overridableErrorCategory, diff --git a/security/manager/ssl/tests/unit/test_ssl_status.js b/security/manager/ssl/tests/unit/test_ssl_status.js @@ -16,19 +16,17 @@ function run_test() { }); fakeOCSPResponder.start(8888); - // Test successful connection (both handshakeCertificates and - // succeededCertChain should be set as expected). + // Test successful connection (failedCertChain should be null, + // succeededCertChain should be set as expected) add_connection_test( "good.include-subdomains.pinning.example.com", PRErrorCodeSuccess, null, function withSecurityInfo(aSecInfo) { - ok( - areCertArraysEqual( - aSecInfo.handshakeCertificates, - build_cert_chain(["default-ee", "test-ca"]) - ), - "handshakeCertificates for a successful connection should be as expected" + equal( + aSecInfo.failedCertChain.length, + 0, + "failedCertChain for a successful connection should be empty" ); ok( areCertArraysEqual( @@ -40,7 +38,7 @@ function run_test() { } ); - // Test failed connection (handshakeCertificates should be set as expected, + // Test failed connection (failedCertChain should be set as expected, // succeededCertChain should be null) add_connection_test( "expired.example.com", @@ -54,52 +52,23 @@ function run_test() { ); ok( areCertArraysEqual( - aSecInfo.handshakeCertificates, + aSecInfo.failedCertChain, build_cert_chain(["expired-ee", "test-ca"]) ), - "handshakeCertificates for a failed connection should be as expected" - ); - } - ); - - // Test non-overrideable error (handshakeCertificates should be non-null). - add_connection_test( - "inadequatekeyusage.example.com", - SEC_ERROR_INADEQUATE_KEY_USAGE, - null, - function withSecurityInfo(securityInfo) { - ok( - areCertArraysEqual( - securityInfo.handshakeCertificates, - build_cert_chain(["inadequatekeyusage-ee", "test-ca"]) - ), - "handshakeCertificates for a non-overridable error should be as expected" + "failedCertChain for a failed connection should be as expected" ); } ); - // Ensure the correct handshakeCertificates is set on cert error override. - // First, add a certificate error override. - add_cert_override_test("expired.example.com", SEC_ERROR_EXPIRED_CERTIFICATE); - // Then, connect again and validate handshakeCertificates. - add_connection_test( + // Ensure the correct failed cert chain is set on cert override + let overrideStatus = { + failedCertChain: build_cert_chain(["expired-ee", "test-ca"]), + }; + add_cert_override_test( "expired.example.com", - PRErrorCodeSuccess, - null, - function withSecurityInfo(aSecInfo) { - equal( - aSecInfo.succeededCertChain.length, - 0, - "succeededCertChain for a connection with a certificate error override should be null" - ); - ok( - areCertArraysEqual( - aSecInfo.handshakeCertificates, - build_cert_chain(["expired-ee", "test-ca"]) - ), - "handshakeCertificates for a connection with a certificate error override should be as expected" - ); - } + SEC_ERROR_EXPIRED_CERTIFICATE, + undefined, + overrideStatus ); run_next_test(); diff --git a/toolkit/actors/NetErrorChild.sys.mjs b/toolkit/actors/NetErrorChild.sys.mjs @@ -37,15 +37,13 @@ export class NetErrorChild extends RemotePageChild { this.exportFunctions(exportableFunctions); } - getHandshakeCertificates(docShell) { + getFailedCertChain(docShell) { let securityInfo = docShell.failedChannel && docShell.failedChannel.securityInfo; if (!securityInfo) { return []; } - return securityInfo.handshakeCertificates.map(cert => - cert.getBase64DERString() - ); + return securityInfo.failedCertChain.map(cert => cert.getBase64DERString()); } handleEvent(aEvent) { @@ -60,9 +58,7 @@ export class NetErrorChild extends RemotePageChild { this.sendAsyncMessage("Browser:CertExceptionError", { location: doc.location.href, elementId: elem.id, - handshakeCertificates: this.getHandshakeCertificates( - doc.defaultView.docShell - ), + failedCertChain: this.getFailedCertChain(doc.defaultView.docShell), }); } break; diff --git a/toolkit/actors/NetErrorParent.sys.mjs b/toolkit/actors/NetErrorParent.sys.mjs @@ -239,7 +239,7 @@ export class NetErrorParent extends EscapablePageParent { case "Browser:CertExceptionError": switch (message.data.elementId) { case "viewCertificate": { - let certs = message.data.handshakeCertificates.map(certBase64 => + let certs = message.data.failedCertChain.map(certBase64 => encodeURIComponent(certBase64) ); let certsStringURL = certs.map(elem => `cert=${elem}`); diff --git a/toolkit/components/extensions/webrequest/SecurityInfo.sys.mjs b/toolkit/components/extensions/webrequest/SecurityInfo.sys.mjs @@ -102,9 +102,9 @@ export const SecurityInfo = { // The connection failed. info.state = "broken"; info.errorMessage = securityInfo.errorMessage; - if (options.certificateChain && securityInfo.handshakeCertificates) { + if (options.certificateChain && securityInfo.failedCertChain) { info.certificates = this.getCertificateChain( - securityInfo.handshakeCertificates, + securityInfo.failedCertChain, false, options );