From 777066906aeacd1afb860393d296081bf6cb420b Mon Sep 17 00:00:00 2001 From: Dana Keeler Date: Wed, 5 Jun 2024 23:58:02 +0000 Subject: [PATCH] Bug 1900132 - streamline PVerifySSLServerCert protocol r=jschanck Previously the PVerifySSLServerCert protocol consisted of two functions: one to call when certificate verification succeeded, and another to call upon failure. This was unnecessary, as the code before and after this protocol didn't have the same split. This patch unifies the protocol to better match the surrounding code. It also takes the opportunity to make use of some IPC helpers to serialize enums rather than manually casting to and from basic integer types. Differential Revision: https://phabricator.services.mozilla.com/D212594 --- ipc/glue/TransportSecurityInfoUtils.h | 7 ++++ .../manager/ssl/PVerifySSLServerCert.ipdl | 23 +++++++++---- .../manager/ssl/SSLServerCertVerification.cpp | 13 ++++++-- .../manager/ssl/VerifySSLServerCertChild.cpp | 32 +++++++------------ .../manager/ssl/VerifySSLServerCertChild.h | 11 +++---- .../manager/ssl/VerifySSLServerCertParent.cpp | 23 ++++++------- .../manager/ssl/VerifySSLServerCertParent.h | 14 ++++---- 7 files changed, 66 insertions(+), 57 deletions(-) diff --git a/ipc/glue/TransportSecurityInfoUtils.h b/ipc/glue/TransportSecurityInfoUtils.h index 009db8e16265..0228f9bae9eb 100644 --- a/ipc/glue/TransportSecurityInfoUtils.h +++ b/ipc/glue/TransportSecurityInfoUtils.h @@ -5,6 +5,7 @@ #ifndef mozilla_ipc_TransportSecurityInfoUtils_h #define mozilla_ipc_TransportSecurityInfoUtils_h +#include "CertVerifier.h" #include "ipc/EnumSerializer.h" #include "mozilla/RefPtr.h" #include "nsITransportSecurityInfo.h" @@ -38,6 +39,12 @@ struct ParamTraits nsITransportSecurityInfo::OverridableErrorCategory::ERROR_UNSET, nsITransportSecurityInfo::OverridableErrorCategory::ERROR_TIME> {}; +template <> +struct ParamTraits + : public ContiguousEnumSerializerInclusive {}; + } // namespace IPC #endif // mozilla_ipc_TransportSecurityInfoUtils_h diff --git a/security/manager/ssl/PVerifySSLServerCert.ipdl b/security/manager/ssl/PVerifySSLServerCert.ipdl index 92a14160cef8..029a5ece86c9 100644 --- a/security/manager/ssl/PVerifySSLServerCert.ipdl +++ b/security/manager/ssl/PVerifySSLServerCert.ipdl @@ -8,6 +8,8 @@ include PSMIPCTypes; using mozilla::OriginAttributes from "mozilla/ipc/BackgroundUtils.h"; +using mozilla::psm::EVStatus from "mozilla/ipc/TransportSecurityInfoUtils.h"; +using nsITransportSecurityInfo::OverridableErrorCategory from "mozilla/ipc/TransportSecurityInfoUtils.h"; namespace mozilla { namespace psm { @@ -16,13 +18,20 @@ namespace psm { protocol PVerifySSLServerCert { child: - async OnVerifiedSSLServerCertSuccess(ByteArray[] aBuiltCertChain, - uint16_t aCertTransparencyStatus, - uint8_t aEVStatus, - bool isBuiltCertChainRootBuiltInRoot, bool aMadeOCSPRequests); - - async OnVerifiedSSLServerCertFailure(int32_t aFinalError, - uint32_t aOverridableErrorCategory, bool aMadeOCSPRequests); + // This is not an incredibly intuitive order for these arguments, as + // `aFinalError` is probably the most salient argument (if it is 0, the + // connection will proceed, regardless of the value of, e.g. `aSucceeded`). + // However, this ordering matches the order of the related functions in + // SSLServerCertVerification.{h,cpp} (minus unnecessary arguments such as + // `aPeerCertChain`). + async OnVerifySSLServerCertFinished(ByteArray[] aBuiltCertChain, + uint16_t aCertTransparencyStatus, + EVStatus aEVStatus, + bool aSucceeded, + int32_t aFinalError, + OverridableErrorCategory aOverridableErrorCategory, + bool aIsBuiltCertChainRootBuiltInRoot, + bool aMadeOCSPRequests); async __delete__(); }; diff --git a/security/manager/ssl/SSLServerCertVerification.cpp b/security/manager/ssl/SSLServerCertVerification.cpp index bf2f56afcb9c..d964c448aadd 100644 --- a/security/manager/ssl/SSLServerCertVerification.cpp +++ b/security/manager/ssl/SSLServerCertVerification.cpp @@ -1063,15 +1063,22 @@ void SSLServerCertVerificationResult::Dispatch( mProviderFlags = aProviderFlags; mMadeOCSPRequests = aMadeOCSPRequests; - if (mSucceeded && mBuiltChain.IsEmpty()) { + if (mSucceeded && + (mBuiltChain.IsEmpty() || mFinalError != 0 || + mOverridableErrorCategory != + nsITransportSecurityInfo::OverridableErrorCategory::ERROR_UNSET)) { MOZ_ASSERT_UNREACHABLE( - "if the handshake succeeded, the built chain shouldn't be empty"); + "if certificate verification succeeded without overridden errors, the " + "built chain shouldn't be empty and any error bits should be unset"); mSucceeded = false; mFinalError = SEC_ERROR_LIBRARY_FAILURE; } + // Note that mSucceeded can be false while mFinalError is 0, in which case + // the connection will proceed. if (!mSucceeded && mPeerCertChain.IsEmpty()) { MOZ_ASSERT_UNREACHABLE( - "if the handshake failed, the peer chain shouldn't be empty"); + "if certificate verification failed, the peer chain shouldn't be " + "empty"); mFinalError = SEC_ERROR_LIBRARY_FAILURE; } diff --git a/security/manager/ssl/VerifySSLServerCertChild.cpp b/security/manager/ssl/VerifySSLServerCertChild.cpp index 6c9795486e76..3ac477df3b21 100644 --- a/security/manager/ssl/VerifySSLServerCertChild.cpp +++ b/security/manager/ssl/VerifySSLServerCertChild.cpp @@ -29,13 +29,16 @@ VerifySSLServerCertChild::VerifySSLServerCertChild( mPeerCertChain(std::move(aPeerCertChain)), mProviderFlags(aProviderFlags) {} -ipc::IPCResult VerifySSLServerCertChild::RecvOnVerifiedSSLServerCertSuccess( +ipc::IPCResult VerifySSLServerCertChild::RecvOnVerifySSLServerCertFinished( nsTArray&& aBuiltCertChain, - const uint16_t& aCertTransparencyStatus, const uint8_t& aEVStatus, + const uint16_t& aCertTransparencyStatus, const EVStatus& aEVStatus, + const bool& aSucceeded, int32_t aFinalError, + const nsITransportSecurityInfo::OverridableErrorCategory& + aOverridableErrorCategory, const bool& aIsBuiltCertChainRootBuiltInRoot, const bool& aMadeOCSPRequests) { MOZ_LOG(gPIPNSSLog, LogLevel::Debug, - ("[%p] VerifySSLServerCertChild::RecvOnVerifiedSSLServerCertSuccess", + ("[%p] VerifySSLServerCertChild::RecvOnVerifySSLServerCertFinished", this)); nsTArray> certBytesArray; @@ -43,24 +46,11 @@ ipc::IPCResult VerifySSLServerCertChild::RecvOnVerifiedSSLServerCertSuccess( certBytesArray.AppendElement(std::move(cert.data())); } - mResultTask->Dispatch( - std::move(certBytesArray), std::move(mPeerCertChain), - aCertTransparencyStatus, static_cast(aEVStatus), true, 0, - nsITransportSecurityInfo::OverridableErrorCategory::ERROR_UNSET, - aIsBuiltCertChainRootBuiltInRoot, mProviderFlags, aMadeOCSPRequests); - return IPC_OK(); -} - -ipc::IPCResult VerifySSLServerCertChild::RecvOnVerifiedSSLServerCertFailure( - const int32_t& aFinalError, const uint32_t& aOverridableErrorCategory, - const bool& aMadeOCSPRequests) { - mResultTask->Dispatch( - nsTArray>(), std::move(mPeerCertChain), - nsITransportSecurityInfo::CERTIFICATE_TRANSPARENCY_NOT_APPLICABLE, - EVStatus::NotEV, false, aFinalError, - static_cast( - aOverridableErrorCategory), - false, mProviderFlags, aMadeOCSPRequests); + mResultTask->Dispatch(std::move(certBytesArray), std::move(mPeerCertChain), + aCertTransparencyStatus, aEVStatus, aSucceeded, + aFinalError, aOverridableErrorCategory, + aIsBuiltCertChainRootBuiltInRoot, mProviderFlags, + aMadeOCSPRequests); return IPC_OK(); } diff --git a/security/manager/ssl/VerifySSLServerCertChild.h b/security/manager/ssl/VerifySSLServerCertChild.h index 94fd5d48d0b9..04046919efb5 100644 --- a/security/manager/ssl/VerifySSLServerCertChild.h +++ b/security/manager/ssl/VerifySSLServerCertChild.h @@ -30,16 +30,15 @@ class VerifySSLServerCertChild : public PVerifySSLServerCertChild { SSLServerCertVerificationResult* aResultTask, nsTArray>&& aPeerCertChain, uint32_t aProviderFlags); - ipc::IPCResult RecvOnVerifiedSSLServerCertSuccess( + ipc::IPCResult RecvOnVerifySSLServerCertFinished( nsTArray&& aBuiltCertChain, - const uint16_t& aCertTransparencyStatus, const uint8_t& aEVStatus, + const uint16_t& aCertTransparencyStatus, const EVStatus& aEVStatus, + const bool& aSucceeded, int32_t aFinalError, + const nsITransportSecurityInfo::OverridableErrorCategory& + aOverridableErrorCategory, const bool& aIsBuiltCertChainRootBuiltInRoot, const bool& aMadeOCSPRequests); - ipc::IPCResult RecvOnVerifiedSSLServerCertFailure( - const int32_t& aFinalError, const uint32_t& aOverridableErrorCategory, - const bool& aMadeOCSPRequests); - private: ~VerifySSLServerCertChild() = default; diff --git a/security/manager/ssl/VerifySSLServerCertParent.cpp b/security/manager/ssl/VerifySSLServerCertParent.cpp index 5aaaba8197c8..310dcc7a06ce 100644 --- a/security/manager/ssl/VerifySSLServerCertParent.cpp +++ b/security/manager/ssl/VerifySSLServerCertParent.cpp @@ -28,21 +28,19 @@ VerifySSLServerCertParent::VerifySSLServerCertParent() {} void VerifySSLServerCertParent::OnVerifiedSSLServerCert( const nsTArray& aBuiltCertChain, - uint16_t aCertificateTransparencyStatus, uint8_t aEVStatus, bool aSucceeded, - PRErrorCode aFinalError, uint32_t aOverridableErrorCategory, + uint16_t aCertificateTransparencyStatus, EVStatus aEVStatus, + bool aSucceeded, PRErrorCode aFinalError, + nsITransportSecurityInfo::OverridableErrorCategory + aOverridableErrorCategory, bool aIsBuiltCertChainRootBuiltInRoot, bool aMadeOCSPRequests) { if (!CanSend()) { return; } - if (aSucceeded) { - Unused << SendOnVerifiedSSLServerCertSuccess( - aBuiltCertChain, aCertificateTransparencyStatus, aEVStatus, - aIsBuiltCertChainRootBuiltInRoot, aMadeOCSPRequests); - } else { - Unused << SendOnVerifiedSSLServerCertFailure( - aFinalError, aOverridableErrorCategory, aMadeOCSPRequests); - } + Unused << SendOnVerifySSLServerCertFinished( + aBuiltCertChain, aCertificateTransparencyStatus, aEVStatus, aSucceeded, + aFinalError, aOverridableErrorCategory, aIsBuiltCertChainRootBuiltInRoot, + aMadeOCSPRequests); Close(); } @@ -109,9 +107,8 @@ void IPCServerCertVerificationResult::Dispatch( SaveIntermediateCerts(certBytesArray); } parent->OnVerifiedSSLServerCert( - builtCertChain, aCertificateTransparencyStatus, - static_cast(aEVStatus), aSucceeded, aFinalError, - static_cast(aOverridableErrorCategory), + builtCertChain, aCertificateTransparencyStatus, aEVStatus, + aSucceeded, aFinalError, aOverridableErrorCategory, aIsBuiltCertChainRootBuiltInRoot, aMadeOCSPRequests); }), NS_DISPATCH_NORMAL); diff --git a/security/manager/ssl/VerifySSLServerCertParent.h b/security/manager/ssl/VerifySSLServerCertParent.h index de2c062935e8..84245a59f0e5 100644 --- a/security/manager/ssl/VerifySSLServerCertParent.h +++ b/security/manager/ssl/VerifySSLServerCertParent.h @@ -37,13 +37,13 @@ class VerifySSLServerCertParent : public PVerifySSLServerCertParent { const uint32_t& aProviderFlags, const uint32_t& aCertVerifierFlags); - void OnVerifiedSSLServerCert(const nsTArray& aBuiltCertChain, - uint16_t aCertificateTransparencyStatus, - uint8_t aEVStatus, bool aSucceeded, - PRErrorCode aFinalError, - uint32_t aOverridableErrorCategory, - bool aIsBuiltCertChainRootBuiltInRoot, - bool aMadeOCSPRequests); + void OnVerifiedSSLServerCert( + const nsTArray& aBuiltCertChain, + uint16_t aCertificateTransparencyStatus, EVStatus aEVStatus, + bool aSucceeded, PRErrorCode aFinalError, + nsITransportSecurityInfo::OverridableErrorCategory + aOverridableErrorCategory, + bool aIsBuiltCertChainRootBuiltInRoot, bool aMadeOCSPRequests); private: virtual ~VerifySSLServerCertParent();