commit 71a99f8c272bfc0253db898105f5571f58dfb848 parent e0b79a5ae9fbb7d3b68c251ffdcbb9937e3ce2e7 Author: Dana Keeler <dkeeler@mozilla.com> Date: Tue, 21 Oct 2025 16:21:07 +0000 Bug 1994034 - make TLS handshake certificates always available r=jschanck,necko-reviewers,extension-reviewers,kershaw,robwu,dom-worker-reviewers,edenchuang This patch renames nsITransportSecurityInfo.failedCertChain to handshakeCertificates and makes the property always available (after the handshake has completed, that is). Differential Revision: https://phabricator.services.mozilla.com/D268467 Diffstat:
20 files changed, 181 insertions(+), 204 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.failedCertChain; + certChainArray = secInfo.handshakeCertificates; } 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 failedCertChain = - content.docShell.failedChannel.securityInfo.failedCertChain.map(cert => - cert.getBase64DERString() + let handshakeCertificates = + content.docShell.failedChannel.securityInfo.handshakeCertificates.map( + cert => cert.getBase64DERString() ); return { divDisplay: content.getComputedStyle(div).display, text: text.textContent, - failedCertChain, + handshakeCertificates, }; }); 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.failedCertChain); + let certChain = getCertChainAsString(message.handshakeCertificates); 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 failedCertChain = - content.docShell.failedChannel.securityInfo.failedCertChain.map(cert => - cert.getBase64DERString() + let handshakeCertificates = + content.docShell.failedChannel.securityInfo.handshakeCertificates.map( + cert => cert.getBase64DERString() ); return { divDisplay: content.getComputedStyle(div).display, text: text.textContent, - failedCertChain, + handshakeCertificates, }; }); 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.failedCertChain); + let certChain = getCertChainAsString(message.handshakeCertificates); 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>> failedCertArray; - rv = tsi->GetFailedCertChain(failedCertArray); + nsTArray<RefPtr<nsIX509Cert>> handshakeCertificates; + rv = tsi->GetHandshakeCertificates(handshakeCertificates); if (NS_WARN_IF(NS_FAILED(rv))) { aRv.Throw(rv); return; } - if (NS_WARN_IF(failedCertArray.IsEmpty())) { + if (NS_WARN_IF(handshakeCertificates.IsEmpty())) { aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR); return; } - for (const auto& certificate : failedCertArray) { + for (const auto& certificate : handshakeCertificates) { rv = certificate->GetIssuerCommonName(issuerCommonName); if (NS_WARN_IF(NS_FAILED(rv))) { aRv.Throw(rv); diff --git a/dom/cache/test/mochitest/test_cache_padding.html b/dom/cache/test/mochitest/test_cache_padding.html @@ -92,6 +92,25 @@ function fetchOpaqueResponse(url) { return fetch(url, { mode: "no-cors" }); } +// This test (and the underlying implementation being tested) relies on the +// implementation details of sqlite. In particular, when using the obfuscating +// VFS in private contexts [0][1], reserving 32 bytes per page [2] for the +// encryption IV causes sqlite to grow the database under some conditions when +// the serialized security info [3] exceeds 8119 bytes. The growth size is +// 32KiB [4], which means that in private contexts, the given values that are +// supposed to be equal here will be off by that amount. +// [0] https://searchfox.org/firefox-main/rev/3aff965e839b7872bf48a9803b80669ca49276ac/dom/cache/Manager.cpp#79 +// [1] https://searchfox.org/firefox-main/rev/3aff965e839b7872bf48a9803b80669ca49276ac/storage/mozStorageConnection.cpp#1175 +// [2] https://searchfox.org/firefox-main/rev/1b3787c361452ef9c9e37d8ffd4c414135d547f5/storage/ObfuscatingVFS.cpp#309 +// [3] https://searchfox.org/firefox-main/rev/3aff965e839b7872bf48a9803b80669ca49276ac/security/manager/ssl/TransportSecurityInfo.cpp#114 +// [4] https://searchfox.org/firefox-main/rev/3aff965e839b7872bf48a9803b80669ca49276ac/dom/cache/DBSchema.cpp#255 +function equalOrOffByOneGrowthChunk(a, b, message) { + if (SpecialPowers.getBoolPref("browser.privatebrowsing.autostart") && a != b) { + b += 32768; + } + is(a, b, message); +} + SimpleTest.waitForExplicitFinish(); SpecialPowers.pushPrefEnv({ "set": [["dom.caches.enabled", true], @@ -162,7 +181,7 @@ SpecialPowers.pushPrefEnv({ let usage4 = await verifyUsage(); // Since we put the same request and response again, the size should be the // same (DOM Cache removes the previous cached request and response) - ok(usage4 == usage3, + equalOrOffByOneGrowthChunk(usage4, usage3, "We won't generate different padding for cloned response"); info("Stage 4: Verify the cached response has the same size."); @@ -173,7 +192,7 @@ SpecialPowers.pushPrefEnv({ await cache.put(cors_base + url, opaqueResponse); await waitForIOToComplete(cache, url); let usage5 = await verifyUsage(); - ok(usage5 == usage3, + equalOrOffByOneGrowthChunk(usage5, usage3, "We won't generate different padding for cached response"); info("Stage 5: Verify padding size may changes in different fetch()s."); @@ -210,7 +229,7 @@ SpecialPowers.pushPrefEnv({ await cache.delete(cors_base + url); await waitForIOToComplete(cache, url); let usage7 = await verifyUsage(); - ok(usage7 == usage2, + equalOrOffByOneGrowthChunk(usage7, usage2, "The opaque response should be removed by caches.delete() and " + "cache.delete()"); diff --git a/netwerk/base/FuzzySecurityInfo.cpp b/netwerk/base/FuzzySecurityInfo.cpp @@ -37,8 +37,8 @@ FuzzySecurityInfo::GetErrorCodeString(nsAString& aErrorString) { } NS_IMETHODIMP -FuzzySecurityInfo::GetFailedCertChain( - nsTArray<RefPtr<nsIX509Cert>>& aFailedCertChain) { +FuzzySecurityInfo::GetHandshakeCertificates( + nsTArray<RefPtr<nsIX509Cert>>& aHandshakeCertificates) { 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.mFailedCertChainBytes = - mFailedCertChainBytes + result.mHandshakeCertificatesBytes = + mHandshakeCertificatesBytes ? Some(TransformIntoNewArray( - *mFailedCertChainBytes, + *mHandshakeCertificatesBytes, [](const auto& element) { return element.Clone(); })) : Nothing(); return result; @@ -81,8 +81,9 @@ uint32_t SSLTokensCache::TokenCacheRecord::Size() const { size += cert.Length(); } } - if (mSessionCacheInfo.mFailedCertChainBytes) { - for (const auto& cert : mSessionCacheInfo.mFailedCertChainBytes.ref()) { + if (mSessionCacheInfo.mHandshakeCertificatesBytes) { + for (const auto& cert : + mSessionCacheInfo.mHandshakeCertificatesBytes.ref()) { size += cert.Length(); } } @@ -100,7 +101,7 @@ void SSLTokensCache::TokenCacheRecord::Reset() { mSessionCacheInfo.mIsBuiltCertChainRootBuiltInRoot.reset(); mSessionCacheInfo.mOverridableErrorCategory = nsITransportSecurityInfo::OverridableErrorCategory::ERROR_UNSET; - mSessionCacheInfo.mFailedCertChainBytes.reset(); + mSessionCacheInfo.mHandshakeCertificatesBytes.reset(); } uint32_t SSLTokensCache::TokenCacheEntry::Size() const { @@ -291,21 +292,21 @@ nsresult SSLTokensCache::Put(const nsACString& aKey, const uint8_t* aToken, return rv; } - Maybe<nsTArray<nsTArray<uint8_t>>> failedCertChainBytes; - nsTArray<RefPtr<nsIX509Cert>> failedCertArray; - rv = securityInfo->GetFailedCertChain(failedCertArray); + Maybe<nsTArray<nsTArray<uint8_t>>> handshakeCertificatesBytes; + nsTArray<RefPtr<nsIX509Cert>> handshakeCertificates; + rv = securityInfo->GetHandshakeCertificates(handshakeCertificates); if (NS_FAILED(rv)) { return rv; } - if (!failedCertArray.IsEmpty()) { - failedCertChainBytes.emplace(); - for (const auto& cert : failedCertArray) { + if (!handshakeCertificates.IsEmpty()) { + handshakeCertificatesBytes.emplace(); + for (const auto& cert : handshakeCertificates) { nsTArray<uint8_t> rawCert; nsresult rv = cert->GetRawDER(rawCert); if (NS_FAILED(rv)) { return rv; } - failedCertChainBytes->AppendElement(std::move(rawCert)); + handshakeCertificatesBytes->AppendElement(std::move(rawCert)); } } @@ -327,8 +328,8 @@ nsresult SSLTokensCache::Put(const nsACString& aKey, const uint8_t* aToken, rec->mSessionCacheInfo.mIsBuiltCertChainRootBuiltInRoot = std::move(isBuiltCertChainRootBuiltInRoot); rec->mSessionCacheInfo.mOverridableErrorCategory = overridableErrorCategory; - rec->mSessionCacheInfo.mFailedCertChainBytes = - std::move(failedCertChainBytes); + rec->mSessionCacheInfo.mHandshakeCertificatesBytes = + std::move(handshakeCertificatesBytes); 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>>> mFailedCertChainBytes; + Maybe<nsTArray<nsTArray<uint8_t>>> mHandshakeCertificatesBytes; }; 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::SetFailedCertChain( +void CommonSocketControl::SetHandshakeCertificates( nsTArray<nsTArray<uint8_t>>&& aCertList) { COMMON_SOCKET_CONTROL_ASSERT_ON_OWNING_THREAD(); - return CreateCertChain(mFailedCertChain, std::move(aCertList)); + return CreateCertChain(mHandshakeCertificates, std::move(aCertList)); } void CommonSocketControl::SetCanceled(PRErrorCode errorCode) { @@ -319,8 +319,8 @@ void CommonSocketControl::RebuildCertificateInfoFromSSLTokenCache() { SetIsBuiltCertChainRootBuiltInRoot(*info.mIsBuiltCertChainRootBuiltInRoot); } - if (info.mFailedCertChainBytes) { - SetFailedCertChain(std::move(*info.mFailedCertChainBytes)); + if (info.mHandshakeCertificatesBytes) { + SetHandshakeCertificates(std::move(*info.mHandshakeCertificatesBytes)); } } @@ -451,8 +451,8 @@ CommonSocketControl::GetSecurityInfo(nsITransportSecurityInfo** aSecurityInfo) { } nsCOMPtr<nsITransportSecurityInfo> securityInfo( new psm::TransportSecurityInfo( - mSecurityState, mErrorCode, mFailedCertChain.Clone(), mServerCert, - mSucceededCertChain.Clone(), mCipherSuite, mKeaGroupName, + mSecurityState, mErrorCode, mHandshakeCertificates.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 SetFailedCertChain(nsTArray<nsTArray<uint8_t>>&& certList); + void SetHandshakeCertificates(nsTArray<nsTArray<uint8_t>>&& certList); void SetIsBuiltCertChainRootBuiltInRoot( bool aIsBuiltCertChainRootBuiltInRoot) { COMMON_SOCKET_CONTROL_ASSERT_ON_OWNING_THREAD(); @@ -156,9 +156,11 @@ class CommonSocketControl : public nsITLSSocketControl { // Fields used to build a TransportSecurityInfo uint32_t mSecurityState; PRErrorCode mErrorCode; - // Peer cert chain for failed connections. - nsTArray<RefPtr<nsIX509Cert>> mFailedCertChain; + // Certificates provided in the TLS handshake by the server. + nsTArray<RefPtr<nsIX509Cert>> mHandshakeCertificates; + // The server end-entity certificate. 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>>&& aFailedCertChain, + nsTArray<RefPtr<nsIX509Cert>>&& aHandshakeCertificates, 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), - mFailedCertChain(std::move(aFailedCertChain)), + mHandshakeCertificates(std::move(aHandshakeCertificates)), 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(mFailedCertChain.Length()); + rv = objStream->Write16(mHandshakeCertificates.Length()); NS_ENSURE_SUCCESS(rv, rv); - for (const auto& cert : mFailedCertChain) { + for (const auto& cert : mHandshakeCertificates) { 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>> failedCertChain; - rv = ReadCertList(aStream, failedCertChain); + nsTArray<RefPtr<nsIX509Cert>> handshakeCertificates; + rv = ReadCertList(aStream, handshakeCertificates); 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>> aFailedCertChain; + nsTArray<RefPtr<nsIX509Cert>> aHandshakeCertificates; nsCOMPtr<nsIX509Cert> aServerCert; nsTArray<RefPtr<nsIX509Cert>> aSucceededCertChain; Maybe<uint16_t> aCipherSuite; @@ -703,14 +703,15 @@ nsresult TransportSecurityInfo::Read(const nsCString& aSerializedSecurityInfo, // END moved from nsISSLStatus if (serVersionParsedToInt < 3) { // The old data structure of certList(nsIX509CertList) presents - rv = ReadCertList(objStream, aFailedCertChain); + rv = ReadCertList(objStream, aHandshakeCertificates); NS_ENSURE_SUCCESS(rv, rv); } else { uint16_t certCount; rv = objStream->Read16(&certCount); NS_ENSURE_SUCCESS(rv, rv); - rv = ReadCertificatesFromStream(objStream, certCount, aFailedCertChain); + rv = ReadCertificatesFromStream(objStream, certCount, + aHandshakeCertificates); NS_ENSURE_SUCCESS(rv, rv); } @@ -791,8 +792,8 @@ nsresult TransportSecurityInfo::Read(const nsCString& aSerializedSecurityInfo, } RefPtr<nsITransportSecurityInfo> securityInfo(new TransportSecurityInfo( - aSecurityState, aErrorCode, std::move(aFailedCertChain), aServerCert, - std::move(aSucceededCertChain), aCipherSuite, aKeaGroupName, + aSecurityState, aErrorCode, std::move(aHandshakeCertificates), + aServerCert, std::move(aSucceededCertChain), aCipherSuite, aKeaGroupName, aSignatureSchemeName, aProtocolVersion, aCertificateTransparencyStatus, aIsAcceptedEch, aIsDelegatedCredential, aOverridableErrorCategory, aMadeOCSPRequests, aUsedPrivateDNS, aIsEV, aNPNCompleted, aNegotiatedNPN, @@ -804,7 +805,7 @@ nsresult TransportSecurityInfo::Read(const nsCString& aSerializedSecurityInfo, void TransportSecurityInfo::SerializeToIPC(IPC::MessageWriter* aWriter) { WriteParam(aWriter, mSecurityState); WriteParam(aWriter, mErrorCode); - WriteParam(aWriter, mFailedCertChain); + WriteParam(aWriter, mHandshakeCertificates); WriteParam(aWriter, mServerCert); WriteParam(aWriter, mSucceededCertChain); WriteParam(aWriter, mCipherSuite); @@ -829,7 +830,7 @@ bool TransportSecurityInfo::DeserializeFromIPC( IPC::MessageReader* aReader, RefPtr<nsITransportSecurityInfo>* aResult) { uint32_t aSecurityState; PRErrorCode aErrorCode; - nsTArray<RefPtr<nsIX509Cert>> aFailedCertChain; + nsTArray<RefPtr<nsIX509Cert>> aHandshakeCertificates; nsCOMPtr<nsIX509Cert> aServerCert; nsTArray<RefPtr<nsIX509Cert>> aSucceededCertChain; Maybe<uint16_t> aCipherSuite; @@ -851,7 +852,7 @@ bool TransportSecurityInfo::DeserializeFromIPC( if (!ReadParam(aReader, &aSecurityState) || !ReadParam(aReader, &aErrorCode) || - !ReadParam(aReader, &aFailedCertChain) || + !ReadParam(aReader, &aHandshakeCertificates) || !ReadParam(aReader, &aServerCert) || !ReadParam(aReader, &aSucceededCertChain) || !ReadParam(aReader, &aCipherSuite) || @@ -872,8 +873,8 @@ bool TransportSecurityInfo::DeserializeFromIPC( } RefPtr<nsITransportSecurityInfo> securityInfo(new TransportSecurityInfo( - aSecurityState, aErrorCode, std::move(aFailedCertChain), aServerCert, - std::move(aSucceededCertChain), aCipherSuite, aKeaGroupName, + aSecurityState, aErrorCode, std::move(aHandshakeCertificates), + aServerCert, std::move(aSucceededCertChain), aCipherSuite, aKeaGroupName, aSignatureSchemeName, aProtocolVersion, aCertificateTransparencyStatus, aIsAcceptedEch, aIsDelegatedCredential, aOverridableErrorCategory, aMadeOCSPRequests, aUsedPrivateDNS, aIsEV, aNPNCompleted, aNegotiatedNPN, @@ -883,13 +884,13 @@ bool TransportSecurityInfo::DeserializeFromIPC( } NS_IMETHODIMP -TransportSecurityInfo::GetFailedCertChain( - nsTArray<RefPtr<nsIX509Cert>>& aFailedCertChain) { - MOZ_ASSERT(aFailedCertChain.IsEmpty()); - if (!aFailedCertChain.IsEmpty()) { +TransportSecurityInfo::GetHandshakeCertificates( + nsTArray<RefPtr<nsIX509Cert>>& aHandshakeCertificates) { + MOZ_ASSERT(aHandshakeCertificates.IsEmpty()); + if (!aHandshakeCertificates.IsEmpty()) { return NS_ERROR_INVALID_ARG; } - aFailedCertChain.AppendElements(mFailedCertChain); + aHandshakeCertificates.AppendElements(mHandshakeCertificates); 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>>&& aFailedCertChain, + nsTArray<RefPtr<nsIX509Cert>>&& aHandshakeCertificates, nsCOMPtr<nsIX509Cert>& aServerCert, nsTArray<RefPtr<nsIX509Cert>>&& aSucceededCertChain, Maybe<uint16_t> aCipherSuite, Maybe<nsCString> aKeaGroupName, @@ -60,9 +60,11 @@ class TransportSecurityInfo : public nsITransportSecurityInfo { const uint32_t mSecurityState; const PRErrorCode mErrorCode; - // Peer cert chain for failed connections. - const nsTArray<RefPtr<nsIX509Cert>> mFailedCertChain; + // Certificates provided in the TLS handshake by the server. + const nsTArray<RefPtr<nsIX509Cert>> mHandshakeCertificates; + // The server end-entity certificate. 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,13 +37,20 @@ interface nsITransportSecurityInfo : nsISupports { */ /** - * 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. + * The list of certificates provided by the server during the TLS + * handshake. */ - readonly attribute Array<nsIX509Cert> failedCertChain; + readonly attribute Array<nsIX509Cert> handshakeCertificates; + /** + * 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> failedCertChainLength = Nothing(), + Maybe<size_t> handshakeCertificatesLength = Nothing(), Maybe<size_t> succeededCertChainLength = Nothing()) { nsCOMPtr<nsITransportSecurityInfo> securityInfo; nsresult rv = TransportSecurityInfo::Read(serializedSecInfo, @@ -45,18 +45,19 @@ void deserializeAndVerify(const nsCString& serializedSecInfo, ASSERT_EQ(NS_OK, rv); ASSERT_TRUE(cert); - nsTArray<RefPtr<nsIX509Cert>> failedCertArray; - rv = securityInfo->GetFailedCertChain(failedCertArray); + nsTArray<RefPtr<nsIX509Cert>> handshakeCertificatesArray; + rv = securityInfo->GetHandshakeCertificates(handshakeCertificatesArray); ASSERT_EQ(NS_OK, rv); - if (failedCertChainLength) { - ASSERT_FALSE(failedCertArray.IsEmpty()); - for (const auto& cert : failedCertArray) { + if (handshakeCertificatesLength) { + ASSERT_FALSE(handshakeCertificatesArray.IsEmpty()); + for (const auto& cert : handshakeCertificatesArray) { ASSERT_TRUE(cert); } - ASSERT_EQ(*failedCertChainLength, failedCertArray.Length()); + ASSERT_EQ(*handshakeCertificatesLength, + handshakeCertificatesArray.Length()); } else { - ASSERT_TRUE(failedCertArray.IsEmpty()); + ASSERT_TRUE(handshakeCertificatesArray.IsEmpty()); } nsTArray<RefPtr<nsIX509Cert>> succeededCertArray; @@ -193,7 +194,7 @@ TEST(psm_DeserializeCert, preSSLStatusConsolidation) deserializeAndVerify(base64Serialization, Nothing(), Some(2)); } -TEST(psm_DeserializeCert, preSSLStatusConsolidationFailedCertChain) +TEST(psm_DeserializeCert, preSSLStatusConsolidationHandshakeCertificates) { // 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,98 +297,7 @@ function test_cert_pkcs7_empty_array() { function run_test() { do_get_profile(); - add_tls_server_setup("BadCertAndPinningServer", "bad_certs"); - 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(); + test_cert_pkcs7_export(); + test_cert_pkcs7_empty_array(); } 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.failedCertChain.length, + transportSecurityInfo.handshakeCertificates.length, 2, - "expired.example.com should have failedCertChain set" + "expired.example.com should have handshakeCertificates set" ); equal( transportSecurityInfo.overridableErrorCategory, @@ -106,9 +106,9 @@ function add_one_ev_test(resumed) { "ev-test.example.com should have succeededCertChain set" ); equal( - transportSecurityInfo.failedCertChain.length, - 0, - "ev-test.example.com should not have failedCertChain set" + transportSecurityInfo.handshakeCertificates.length, + 2, + "ev-test.example.com should have handshakeCertificates 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,17 +16,19 @@ function run_test() { }); fakeOCSPResponder.start(8888); - // Test successful connection (failedCertChain should be null, - // succeededCertChain should be set as expected) + // Test successful connection (both handshakeCertificates and + // succeededCertChain should be set as expected). add_connection_test( "good.include-subdomains.pinning.example.com", PRErrorCodeSuccess, null, function withSecurityInfo(aSecInfo) { - equal( - aSecInfo.failedCertChain.length, - 0, - "failedCertChain for a successful connection should be empty" + ok( + areCertArraysEqual( + aSecInfo.handshakeCertificates, + build_cert_chain(["default-ee", "test-ca"]) + ), + "handshakeCertificates for a successful connection should be as expected" ); ok( areCertArraysEqual( @@ -38,7 +40,7 @@ function run_test() { } ); - // Test failed connection (failedCertChain should be set as expected, + // Test failed connection (handshakeCertificates should be set as expected, // succeededCertChain should be null) add_connection_test( "expired.example.com", @@ -52,23 +54,52 @@ function run_test() { ); ok( areCertArraysEqual( - aSecInfo.failedCertChain, + aSecInfo.handshakeCertificates, build_cert_chain(["expired-ee", "test-ca"]) ), - "failedCertChain for a failed connection should be as expected" + "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" ); } ); - // 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( + // 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( "expired.example.com", - SEC_ERROR_EXPIRED_CERTIFICATE, - undefined, - overrideStatus + 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" + ); + } ); run_next_test(); diff --git a/toolkit/actors/NetErrorChild.sys.mjs b/toolkit/actors/NetErrorChild.sys.mjs @@ -37,13 +37,15 @@ export class NetErrorChild extends RemotePageChild { this.exportFunctions(exportableFunctions); } - getFailedCertChain(docShell) { + getHandshakeCertificates(docShell) { let securityInfo = docShell.failedChannel && docShell.failedChannel.securityInfo; if (!securityInfo) { return []; } - return securityInfo.failedCertChain.map(cert => cert.getBase64DERString()); + return securityInfo.handshakeCertificates.map(cert => + cert.getBase64DERString() + ); } handleEvent(aEvent) { @@ -58,7 +60,9 @@ export class NetErrorChild extends RemotePageChild { this.sendAsyncMessage("Browser:CertExceptionError", { location: doc.location.href, elementId: elem.id, - failedCertChain: this.getFailedCertChain(doc.defaultView.docShell), + handshakeCertificates: this.getHandshakeCertificates( + 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.failedCertChain.map(certBase64 => + let certs = message.data.handshakeCertificates.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.failedCertChain) { + if (options.certificateChain && securityInfo.handshakeCertificates) { info.certificates = this.getCertificateChain( - securityInfo.failedCertChain, + securityInfo.handshakeCertificates, false, options );