From 2fa45cc17272cb6850a85501e8c5a25421fdce16 Mon Sep 17 00:00:00 2001 From: Sean Feng Date: Tue, 15 Oct 2019 19:57:23 +0000 Subject: [PATCH] Bug 1580315 - Change certList internal representation to Array r=keeler This patch intends to change the internal reprensentation of certList from nsIX509CertList to Array for TransportSecurityInfo. Differential Revision: https://phabricator.services.mozilla.com/D48744 --HG-- extra : moz-landing-system : lando --- netwerk/protocol/http/QuicSocketControl.cpp | 13 +- .../manager/ssl/TransportSecurityInfo.cpp | 131 +++++++++++++++--- security/manager/ssl/TransportSecurityInfo.h | 11 +- 3 files changed, 129 insertions(+), 26 deletions(-) diff --git a/netwerk/protocol/http/QuicSocketControl.cpp b/netwerk/protocol/http/QuicSocketControl.cpp index 87665342af46..44b7482151e5 100644 --- a/netwerk/protocol/http/QuicSocketControl.cpp +++ b/netwerk/protocol/http/QuicSocketControl.cpp @@ -64,9 +64,16 @@ void QuicSocketControl::HandshakeCompleted() { uint32_t state = nsIWebProgressListener::STATE_IS_SECURE; bool distrustImminent; - nsresult srv = - IsCertificateDistrustImminent(mSucceededCertChain, distrustImminent); - if (NS_SUCCEEDED(srv) && distrustImminent) { + nsCOMPtr succeededCertChain; + nsresult rv = TransportSecurityInfo::ConvertCertArrayToCertList( + mSucceededCertChain, getter_AddRefs(succeededCertChain)); + + nsresult srv; + if (NS_SUCCEEDED(rv)) { + srv = IsCertificateDistrustImminent(succeededCertChain, distrustImminent); + } + + if (NS_SUCCEEDED(rv) && NS_SUCCEEDED(srv) && distrustImminent) { state |= nsIWebProgressListener::STATE_CERT_DISTRUST_IMMINENT; } diff --git a/security/manager/ssl/TransportSecurityInfo.cpp b/security/manager/ssl/TransportSecurityInfo.cpp index 192913dbd7b8..61aececcad0e 100644 --- a/security/manager/ssl/TransportSecurityInfo.cpp +++ b/security/manager/ssl/TransportSecurityInfo.cpp @@ -234,14 +234,27 @@ TransportSecurityInfo::Write(nsIObjectOutputStream* aStream) { rv = aStream->WriteStringZ(mSignatureSchemeName.get()); NS_ENSURE_SUCCESS(rv, rv); - rv = NS_WriteOptionalCompoundObject(aStream, mSucceededCertChain, + nsCOMPtr succeededCertChain; + rv = TransportSecurityInfo::ConvertCertArrayToCertList( + mSucceededCertChain, getter_AddRefs(succeededCertChain)); + if (NS_FAILED(rv)) { + return rv; + } + rv = NS_WriteOptionalCompoundObject(aStream, succeededCertChain, NS_GET_IID(nsIX509CertList), true); if (NS_FAILED(rv)) { return rv; } // END moved from nsISSLStatus - rv = NS_WriteOptionalCompoundObject(aStream, mFailedCertChain, + nsCOMPtr failedCertChain; + rv = TransportSecurityInfo::ConvertCertArrayToCertList( + mFailedCertChain, getter_AddRefs(failedCertChain)); + if (NS_FAILED(rv)) { + return rv; + } + + rv = NS_WriteOptionalCompoundObject(aStream, failedCertChain, NS_GET_IID(nsIX509CertList), true); if (NS_FAILED(rv)) { return rv; @@ -259,7 +272,7 @@ TransportSecurityInfo::Write(nsIObjectOutputStream* aStream) { MOZ_DIAGNOSTIC_ASSERT(condition, message); \ } -// This is for backward compatability to be able to read nsISSLStatus +// This is for backward compatibility to be able to read nsISSLStatus // serialized object. nsresult TransportSecurityInfo::ReadSSLStatus(nsIObjectInputStream* aStream) { bool nsISSLStatusPresent; @@ -375,7 +388,15 @@ nsresult TransportSecurityInfo::ReadSSLStatus(nsIObjectInputStream* aStream) { if (NS_FAILED(rv)) { return rv; } - mSucceededCertChain = do_QueryInterface(succeededCertChainSupports); + nsCOMPtr certList = + do_QueryInterface(succeededCertChainSupports); + if (certList) { + rv = TransportSecurityInfo::ConvertCertListToCertArray( + certList, mSucceededCertChain); + if (NS_FAILED(rv)) { + return rv; + } + } // Read only to consume bytes from the stream. nsCOMPtr failedCertChainSupports; @@ -390,6 +411,51 @@ nsresult TransportSecurityInfo::ReadSSLStatus(nsIObjectInputStream* aStream) { return rv; } +nsresult TransportSecurityInfo::ConvertCertArrayToCertList( + const nsTArray>& aCertArray, + nsIX509CertList** aCertList) { + NS_ENSURE_ARG_POINTER(aCertList); + *aCertList = nullptr; + + // aCertList will be null if aCertArray is empty, this also matches + // the original certList behaviour + if (aCertArray.IsEmpty()) { + return NS_OK; + } + + nsCOMPtr certList = new nsNSSCertList(); + for (const auto& cert : aCertArray) { + nsresult rv = certList->AddCert(cert); + if (NS_FAILED(rv)) { + return rv; + } + } + + certList.forget(aCertList); + + return NS_OK; +} + +nsresult TransportSecurityInfo::ConvertCertListToCertArray( + const nsCOMPtr& aCertList, + nsTArray>& aCertArray) { + MOZ_ASSERT(aCertList); + if (!aCertList) { + return NS_ERROR_INVALID_ARG; + } + + aCertArray.Clear(); + RefPtr certList = aCertList->GetCertList(); + + return certList->ForEachCertificateInChain( + [&aCertArray](nsCOMPtr& aCert, bool aHasMore, + bool& aContinue) { + RefPtr cert(aCert.get()); + aCertArray.AppendElement(cert); + return NS_OK; + }); +} + // NB: Any updates (except disk-only fields) must be kept in sync with // |DeserializeFromIPC|. NS_IMETHODIMP @@ -535,7 +601,16 @@ TransportSecurityInfo::Read(nsIObjectInputStream* aStream) { if (NS_FAILED(rv)) { return rv; } - mSucceededCertChain = do_QueryInterface(succeededCertChainSupports); + nsCOMPtr succeededCertChain = + do_QueryInterface(succeededCertChainSupports); + MOZ_ASSERT(mSucceededCertChain.IsEmpty()); + if (succeededCertChain) { + rv = TransportSecurityInfo::ConvertCertListToCertArray( + succeededCertChain, mSucceededCertChain); + if (NS_FAILED(rv)) { + return rv; + } + } } // END moved from nsISSLStatus @@ -546,8 +621,17 @@ TransportSecurityInfo::Read(nsIObjectInputStream* aStream) { if (NS_FAILED(rv)) { return rv; } - mFailedCertChain = do_QueryInterface(failedCertChainSupports); + nsCOMPtr failedCertChain = + do_QueryInterface(failedCertChainSupports); + MOZ_ASSERT(mFailedCertChain.IsEmpty()); + if (failedCertChain) { + rv = TransportSecurityInfo::ConvertCertListToCertArray(failedCertChain, + mFailedCertChain); + if (NS_FAILED(rv)) { + return rv; + } + } // mIsDelegatedCredential added in bug 1562773 if (serVersion.EqualsASCII("2")) { rv = aStream->ReadBoolean(&mIsDelegatedCredential); @@ -769,18 +853,18 @@ void TransportSecurityInfo::SetStatusErrorBits(nsNSSCertificate* cert, NS_IMETHODIMP TransportSecurityInfo::GetFailedCertChain(nsIX509CertList** _result) { MOZ_ASSERT(_result); - - *_result = mFailedCertChain; - NS_IF_ADDREF(*_result); - - return NS_OK; + return TransportSecurityInfo::ConvertCertArrayToCertList(mFailedCertChain, + _result); } nsresult TransportSecurityInfo::SetFailedCertChain( - UniqueCERTCertList certList) { - // nsNSSCertList takes ownership of certList - mFailedCertChain = new nsNSSCertList(std::move(certList)); - + UniqueCERTCertList aCertList) { + mFailedCertChain.Clear(); + for (CERTCertListNode* node = CERT_LIST_HEAD(aCertList); + !CERT_LIST_END(node, aCertList); node = CERT_LIST_NEXT(node)) { + RefPtr cert = nsNSSCertificate::Create(node->cert); + mFailedCertChain.AppendElement(cert); + } return NS_OK; } @@ -806,16 +890,21 @@ NS_IMETHODIMP TransportSecurityInfo::GetSucceededCertChain(nsIX509CertList** _result) { NS_ENSURE_ARG_POINTER(_result); - nsCOMPtr tmpList = mSucceededCertChain; - tmpList.forget(_result); - - return NS_OK; + return TransportSecurityInfo::ConvertCertArrayToCertList(mSucceededCertChain, + _result); } nsresult TransportSecurityInfo::SetSucceededCertChain( UniqueCERTCertList aCertList) { - // nsNSSCertList takes ownership of certList - mSucceededCertChain = new nsNSSCertList(std::move(aCertList)); + // This function effectively takes ownership of aCertList by consuming its + // elements and then releasing the original aCertList when it goes out of + // scope. + mSucceededCertChain.Clear(); + for (CERTCertListNode* node = CERT_LIST_HEAD(aCertList); + !CERT_LIST_END(node, aCertList); node = CERT_LIST_NEXT(node)) { + RefPtr cert = nsNSSCertificate::Create(node->cert); + mSucceededCertChain.AppendElement(cert); + } return NS_OK; } diff --git a/security/manager/ssl/TransportSecurityInfo.h b/security/manager/ssl/TransportSecurityInfo.h index 8a4fbf3695f5..a53ad6a0e1e5 100644 --- a/security/manager/ssl/TransportSecurityInfo.h +++ b/security/manager/ssl/TransportSecurityInfo.h @@ -121,7 +121,11 @@ class TransportSecurityInfo : public nsITransportSecurityInfo, protected: nsCOMPtr mCallbacks; - nsCOMPtr mSucceededCertChain; + nsTArray> mSucceededCertChain; + + static nsresult ConvertCertArrayToCertList( + const nsTArray>& aCertArray, + nsIX509CertList** aCertList); private: uint32_t mSecurityState; @@ -135,9 +139,12 @@ class TransportSecurityInfo : public nsITransportSecurityInfo, nsCOMPtr mServerCert; /* Peer cert chain for failed connections (for error reporting) */ - nsCOMPtr mFailedCertChain; + nsTArray> mFailedCertChain; nsresult ReadSSLStatus(nsIObjectInputStream* aStream); + static nsresult ConvertCertListToCertArray( + const nsCOMPtr& aCertList, + nsTArray>& aCertArray); }; class RememberCertErrorsTable {