From a0e440195f78d237d3ecacef8fd5cbf7654454ab Mon Sep 17 00:00:00 2001 From: Dennis Jackson Date: Fri, 2 Sep 2022 20:59:34 +0000 Subject: [PATCH] Bug 1788290 - Record whether OCSP requests were made whilst making a TLS connection. r=keeler,necko-reviewers. Differential Revision: https://phabricator.services.mozilla.com/D156105 --- netwerk/base/FuzzySecurityInfo.cpp | 12 +++++++ netwerk/socket/nsITransportSecurityInfo.idl | 7 ++++ security/certverifier/CertVerifier.cpp | 36 +++++++++++++++++-- security/certverifier/CertVerifier.h | 6 ++-- .../certverifier/NSSCertDBTrustDomain.cpp | 4 ++- security/certverifier/NSSCertDBTrustDomain.h | 9 +++++ .../manager/ssl/PVerifySSLServerCert.ipdl | 4 +-- .../manager/ssl/SSLServerCertVerification.cpp | 20 +++++++---- .../manager/ssl/SSLServerCertVerification.h | 7 ++-- .../manager/ssl/TransportSecurityInfo.cpp | 29 +++++++++++++-- security/manager/ssl/TransportSecurityInfo.h | 6 ++++ .../manager/ssl/VerifySSLServerCertChild.cpp | 10 +++--- .../manager/ssl/VerifySSLServerCertChild.h | 6 ++-- .../manager/ssl/VerifySSLServerCertParent.cpp | 19 +++++----- .../manager/ssl/VerifySSLServerCertParent.h | 3 +- .../ssl/tests/unit/test_ocsp_required.js | 27 ++++++++++++-- 16 files changed, 167 insertions(+), 38 deletions(-) diff --git a/netwerk/base/FuzzySecurityInfo.cpp b/netwerk/base/FuzzySecurityInfo.cpp index 7409afe87507..3dfa4f911188 100644 --- a/netwerk/base/FuzzySecurityInfo.cpp +++ b/netwerk/base/FuzzySecurityInfo.cpp @@ -339,6 +339,18 @@ NS_IMETHODIMP FuzzySecurityInfo::GetIsBuiltCertChainRootBuiltInRoot( return NS_OK; } +NS_IMETHODIMP +FuzzySecurityInfo::GetUsedPrivateDNS(bool* aUsedPrivateDNS) { + *aUsedPrivateDNS = false; + return NS_OK; +} + +NS_IMETHODIMP +FuzzySecurityInfo::GetMadeOCSPRequests(bool* aMadeOCSPRequests) { + *aMadeOCSPRequests = false; + return NS_OK; +} + NS_IMETHODIMP FuzzySecurityInfo::DisableEarlyData(void) { return NS_OK; } NS_IMETHODIMP FuzzySecurityInfo::SetHandshakeCallbackListener( diff --git a/netwerk/socket/nsITransportSecurityInfo.idl b/netwerk/socket/nsITransportSecurityInfo.idl index cd9e9ddaf968..232dfbef50f7 100644 --- a/netwerk/socket/nsITransportSecurityInfo.idl +++ b/netwerk/socket/nsITransportSecurityInfo.idl @@ -80,6 +80,13 @@ interface nsITransportSecurityInfo : nsISupports { [must_use] readonly attribute nsITransportSecurityInfo_OverridableErrorCategory overridableErrorCategory; + /** + * True if OCSP requests were made to query the status of certificates + * used in this connection. + */ + [must_use] + readonly attribute boolean madeOCSPRequests; + /** * True only if (and after) serverCert was successfully validated as * Extended Validation (EV). diff --git a/security/certverifier/CertVerifier.cpp b/security/certverifier/CertVerifier.cpp index eb1c07ead75f..f279560fbdcb 100644 --- a/security/certverifier/CertVerifier.cpp +++ b/security/certverifier/CertVerifier.cpp @@ -459,7 +459,8 @@ Result CertVerifier::VerifyCert( /*optional out*/ KeySizeStatus* keySizeStatus, /*optional out*/ PinningTelemetryInfo* pinningTelemetryInfo, /*optional out*/ CertificateTransparencyInfo* ctInfo, - /*optional out*/ bool* isBuiltChainRootBuiltInRoot) { + /*optional out*/ bool* isBuiltChainRootBuiltInRoot, + /*optional out*/ bool* madeOCSPRequests) { MOZ_LOG(gCertVerifierLog, LogLevel::Debug, ("Top of VerifyCert\n")); MOZ_ASSERT(usage == certificateUsageSSLServer || !(flags & FLAG_MUST_BE_EV)); @@ -497,6 +498,10 @@ Result CertVerifier::VerifyCert( *isBuiltChainRootBuiltInRoot = false; } + if (madeOCSPRequests) { + *madeOCSPRequests = false; + } + Input certDER; Result rv = certDER.Init(certBytes.Elements(), certBytes.Length()); if (rv != Success) { @@ -547,6 +552,10 @@ Result CertVerifier::VerifyCert( trustDomain, certDER, time, EndEntityOrCA::MustBeEndEntity, KeyUsage::digitalSignature, KeyPurposeId::id_kp_clientAuth, CertPolicyId::anyPolicy, stapledOCSPResponse); + if (madeOCSPRequests) { + *madeOCSPRequests |= + trustDomain.GetOCSPFetchStatus() == OCSPFetchStatus::Fetched; + } break; } @@ -579,6 +588,10 @@ Result CertVerifier::VerifyCert( KeyUsage::keyAgreement, // (EC)DH KeyPurposeId::id_kp_serverAuth, evPolicy, stapledOCSPResponse, ocspStaplingStatus); + if (madeOCSPRequests) { + *madeOCSPRequests |= + trustDomain.GetOCSPFetchStatus() == OCSPFetchStatus::Fetched; + } if (rv == Success) { rv = VerifyCertificateTransparencyPolicy( trustDomain, builtChain, sctsFromTLSInput, time, ctInfo); @@ -634,6 +647,10 @@ Result CertVerifier::VerifyCert( KeyUsage::keyAgreement, //(EC)DH KeyPurposeId::id_kp_serverAuth, CertPolicyId::anyPolicy, stapledOCSPResponse, ocspStaplingStatus); + if (madeOCSPRequests) { + *madeOCSPRequests |= + trustDomain.GetOCSPFetchStatus() == OCSPFetchStatus::Fetched; + } if (rv != Success && !IsFatalError(rv) && rv != Result::ERROR_REVOKED_CERTIFICATE && trustDomain.GetIsErrorDueToDistrustedCAPolicy()) { @@ -677,6 +694,10 @@ Result CertVerifier::VerifyCert( rv = BuildCertChain(trustDomain, certDER, time, EndEntityOrCA::MustBeCA, KeyUsage::keyCertSign, KeyPurposeId::id_kp_serverAuth, CertPolicyId::anyPolicy, stapledOCSPResponse); + if (madeOCSPRequests) { + *madeOCSPRequests |= + trustDomain.GetOCSPFetchStatus() == OCSPFetchStatus::Fetched; + } break; } @@ -698,6 +719,10 @@ Result CertVerifier::VerifyCert( KeyUsage::nonRepudiation, KeyPurposeId::id_kp_emailProtection, CertPolicyId::anyPolicy, stapledOCSPResponse); } + if (madeOCSPRequests) { + *madeOCSPRequests |= + trustDomain.GetOCSPFetchStatus() == OCSPFetchStatus::Fetched; + } break; } @@ -724,6 +749,10 @@ Result CertVerifier::VerifyCert( KeyPurposeId::id_kp_emailProtection, CertPolicyId::anyPolicy, stapledOCSPResponse); } + if (madeOCSPRequests) { + *madeOCSPRequests |= + trustDomain.GetOCSPFetchStatus() == OCSPFetchStatus::Fetched; + } break; } @@ -785,7 +814,8 @@ Result CertVerifier::VerifySSLServerCert( /*optional out*/ KeySizeStatus* keySizeStatus, /*optional out*/ PinningTelemetryInfo* pinningTelemetryInfo, /*optional out*/ CertificateTransparencyInfo* ctInfo, - /*optional out*/ bool* isBuiltChainRootBuiltInRoot) { + /*optional out*/ bool* isBuiltChainRootBuiltInRoot, + /*optional out*/ bool* madeOCSPRequests) { // XXX: MOZ_ASSERT(pinarg); MOZ_ASSERT(!hostname.IsEmpty()); @@ -815,7 +845,7 @@ Result CertVerifier::VerifySSLServerCert( extraCertificates, stapledOCSPResponse, sctsFromTLS, originAttributes, evStatus, ocspStaplingStatus, keySizeStatus, pinningTelemetryInfo, ctInfo, - &isBuiltChainRootBuiltInRootLocal); + &isBuiltChainRootBuiltInRootLocal, madeOCSPRequests); if (rv != Success) { // we don't use the certificate for path building, so this parameter doesn't // matter diff --git a/security/certverifier/CertVerifier.h b/security/certverifier/CertVerifier.h index 76d6bf3ff84b..d03f81b62d72 100644 --- a/security/certverifier/CertVerifier.h +++ b/security/certverifier/CertVerifier.h @@ -162,7 +162,8 @@ class CertVerifier { /*optional out*/ KeySizeStatus* keySizeStatus = nullptr, /*optional out*/ PinningTelemetryInfo* pinningTelemetryInfo = nullptr, /*optional out*/ CertificateTransparencyInfo* ctInfo = nullptr, - /*optional out*/ bool* isBuiltChainRootBuiltInRoot = nullptr); + /*optional out*/ bool* isBuiltChainRootBuiltInRoot = nullptr, + /*optional out*/ bool* madeOCSPRequests = nullptr); mozilla::pkix::Result VerifySSLServerCert( const nsTArray& peerCert, mozilla::pkix::Time time, void* pinarg, @@ -182,7 +183,8 @@ class CertVerifier { /*optional out*/ KeySizeStatus* keySizeStatus = nullptr, /*optional out*/ PinningTelemetryInfo* pinningTelemetryInfo = nullptr, /*optional out*/ CertificateTransparencyInfo* ctInfo = nullptr, - /*optional out*/ bool* isBuiltChainRootBuiltInRoot = nullptr); + /*optional out*/ bool* isBuiltChainRootBuiltInRoot = nullptr, + /*optional out*/ bool* madeOCSPRequests = nullptr); enum OcspDownloadConfig { ocspOff = 0, ocspOn = 1, ocspEVOnly = 2 }; enum OcspStrictConfig { ocspRelaxed = 0, ocspStrict }; diff --git a/security/certverifier/NSSCertDBTrustDomain.cpp b/security/certverifier/NSSCertDBTrustDomain.cpp index 940f43578d90..ee71c21102b1 100644 --- a/security/certverifier/NSSCertDBTrustDomain.cpp +++ b/security/certverifier/NSSCertDBTrustDomain.cpp @@ -103,7 +103,8 @@ NSSCertDBTrustDomain::NSSCertDBTrustDomain( mOCSPStaplingStatus(CertVerifier::OCSP_STAPLING_NEVER_CHECKED), mSCTListFromCertificate(), mSCTListFromOCSPStapling(), - mBuiltInRootsModule(SECMOD_FindModule(kRootModuleName)) {} + mBuiltInRootsModule(SECMOD_FindModule(kRootModuleName)), + mOCSPFetchStatus(OCSPFetchStatus::NotFetched) {} static void FindRootsWithSubject(UniqueSECMODModule& rootsModule, SECItem subject, @@ -1012,6 +1013,7 @@ Result NSSCertDBTrustDomain::SynchronousCheckRevocationWithServer( Vector ocspResponse; Input response; + mOCSPFetchStatus = OCSPFetchStatus::Fetched; rv = DoOCSPRequest(aiaLocation, mOriginAttributes, ocspRequestBytes, ocspRequestLength, GetOCSPTimeout(), ocspResponse); if (rv == Success && diff --git a/security/certverifier/NSSCertDBTrustDomain.h b/security/certverifier/NSSCertDBTrustDomain.h index 6eddb7093d4a..1afd065f7fca 100644 --- a/security/certverifier/NSSCertDBTrustDomain.h +++ b/security/certverifier/NSSCertDBTrustDomain.h @@ -49,6 +49,11 @@ enum class NetscapeStepUpPolicy : uint32_t { NeverMatch = 3, }; +enum class OCSPFetchStatus : uint16_t { + NotFetched = 0, + Fetched = 1, +}; + SECStatus InitializeNSS(const nsACString& dir, NSSDBConfig nssDbConfig, PKCS11DBConfig pkcs11DbConfig); @@ -239,6 +244,8 @@ class NSSCertDBTrustDomain : public mozilla::pkix::TrustDomain { bool GetIsErrorDueToDistrustedCAPolicy() const; + OCSPFetchStatus GetOCSPFetchStatus() { return mOCSPFetchStatus; } + private: Result CheckCRLiteStash( const nsTArray& issuerSubjectPublicKeyInfoBytes, @@ -311,6 +318,8 @@ class NSSCertDBTrustDomain : public mozilla::pkix::TrustDomain { // The built-in roots module, if available. UniqueSECMODModule mBuiltInRootsModule; + + OCSPFetchStatus mOCSPFetchStatus; }; } // namespace psm diff --git a/security/manager/ssl/PVerifySSLServerCert.ipdl b/security/manager/ssl/PVerifySSLServerCert.ipdl index 14f56ac28ece..5277d00ac665 100644 --- a/security/manager/ssl/PVerifySSLServerCert.ipdl +++ b/security/manager/ssl/PVerifySSLServerCert.ipdl @@ -19,10 +19,10 @@ child: async OnVerifiedSSLServerCertSuccess(ByteArray[] aBuiltCertChain, uint16_t aCertTransparencyStatus, uint8_t aEVStatus, - bool isBuiltCertChainRootBuiltInRoot); + bool isBuiltCertChainRootBuiltInRoot, bool aMadeOCSPRequests); async OnVerifiedSSLServerCertFailure(int32_t aFinalError, - uint32_t aOverridableErrorCategory); + uint32_t aOverridableErrorCategory, bool aMadeOCSPRequests); async __delete__(); }; diff --git a/security/manager/ssl/SSLServerCertVerification.cpp b/security/manager/ssl/SSLServerCertVerification.cpp index ef13b542029c..c182006d7673 100644 --- a/security/manager/ssl/SSLServerCertVerification.cpp +++ b/security/manager/ssl/SSLServerCertVerification.cpp @@ -594,7 +594,8 @@ Result AuthCertificate( /*out*/ nsTArray>& builtCertChain, /*out*/ EVStatus& evStatus, /*out*/ CertificateTransparencyInfo& certificateTransparencyInfo, - /*out*/ bool& aIsBuiltCertChainRootBuiltInRoot) { + /*out*/ bool& aIsBuiltCertChainRootBuiltInRoot, + /*out*/ bool& aMadeOCSPRequests) { CertVerifier::OCSPStaplingStatus ocspStaplingStatus = CertVerifier::OCSP_STAPLING_NEVER_CHECKED; KeySizeStatus keySizeStatus = KeySizeStatus::NeverChecked; @@ -614,7 +615,8 @@ Result AuthCertificate( Some(std::move(peerCertsBytes)), stapledOCSPResponse, sctsFromTLSExtension, dcInfo, aOriginAttributes, &evStatus, &ocspStaplingStatus, &keySizeStatus, &pinningTelemetryInfo, - &certificateTransparencyInfo, &aIsBuiltCertChainRootBuiltInRoot); + &certificateTransparencyInfo, &aIsBuiltCertChainRootBuiltInRoot, + &aMadeOCSPRequests); CollectCertTelemetry(rv, evStatus, ocspStaplingStatus, keySizeStatus, pinningTelemetryInfo, builtCertChain, @@ -761,13 +763,15 @@ SSLServerCertVerificationJob::Run() { EVStatus evStatus; CertificateTransparencyInfo certificateTransparencyInfo; bool isCertChainRootBuiltInRoot = false; + bool madeOCSPRequests = false; nsTArray> builtChainBytesArray; nsTArray certBytes(mPeerCertChain.ElementAt(0).Clone()); Result rv = AuthCertificate( *certVerifier, mPinArg, certBytes, mPeerCertChain, mHostName, mOriginAttributes, mStapledOCSPResponse, mSCTsFromTLSExtension, mDCInfo, mProviderFlags, mTime, mCertVerifierFlags, builtChainBytesArray, evStatus, - certificateTransparencyInfo, isCertChainRootBuiltInRoot); + certificateTransparencyInfo, isCertChainRootBuiltInRoot, + madeOCSPRequests); if (rv == Success) { Telemetry::AccumulateTimeDelta( @@ -781,7 +785,7 @@ SSLServerCertVerificationJob::Run() { certificateTransparencyInfo), evStatus, true, 0, nsITransportSecurityInfo::OverridableErrorCategory::ERROR_UNSET, - isCertChainRootBuiltInRoot, mProviderFlags); + isCertChainRootBuiltInRoot, mProviderFlags, madeOCSPRequests); return NS_OK; } @@ -802,7 +806,7 @@ SSLServerCertVerificationJob::Run() { std::move(builtChainBytesArray), std::move(mPeerCertChain), nsITransportSecurityInfo::CERTIFICATE_TRANSPARENCY_NOT_APPLICABLE, EVStatus::NotEV, false, finalError, overridableErrorCategory, false, - mProviderFlags); + mProviderFlags, madeOCSPRequests); return NS_OK; } @@ -1033,7 +1037,8 @@ void SSLServerCertVerificationResult::Dispatch( bool aSucceeded, PRErrorCode aFinalError, nsITransportSecurityInfo::OverridableErrorCategory aOverridableErrorCategory, - bool aIsBuiltCertChainRootBuiltInRoot, uint32_t aProviderFlags) { + bool aIsBuiltCertChainRootBuiltInRoot, uint32_t aProviderFlags, + bool aMadeOCSPRequests) { mBuiltChain = std::move(aBuiltChain); mPeerCertChain = std::move(aPeerCertChain); mCertificateTransparencyStatus = aCertificateTransparencyStatus; @@ -1043,6 +1048,7 @@ void SSLServerCertVerificationResult::Dispatch( mOverridableErrorCategory = aOverridableErrorCategory; mIsBuiltCertChainRootBuiltInRoot = aIsBuiltCertChainRootBuiltInRoot; mProviderFlags = aProviderFlags; + mMadeOCSPRequests = aMadeOCSPRequests; if (mSucceeded && mBuiltChain.IsEmpty()) { MOZ_ASSERT_UNREACHABLE( @@ -1085,6 +1091,8 @@ SSLServerCertVerificationResult::Run() { SaveIntermediateCerts(mBuiltChain); } + mInfoObject->SetMadeOCSPRequest(mMadeOCSPRequests); + if (mSucceeded) { // Certificate verification succeeded. Delete any potential record of // certificate error bits. diff --git a/security/manager/ssl/SSLServerCertVerification.h b/security/manager/ssl/SSLServerCertVerification.h index 05cb39b19224..8e5257545ec4 100644 --- a/security/manager/ssl/SSLServerCertVerification.h +++ b/security/manager/ssl/SSLServerCertVerification.h @@ -52,7 +52,7 @@ class BaseSSLServerCertVerificationResult { nsITransportSecurityInfo::OverridableErrorCategory aOverridableErrorCategory, bool aIsBuiltCertChainRootBuiltInRoot, - uint32_t aProviderFlags) = 0; + uint32_t aProviderFlags, bool aMadeOCSPRequests) = 0; }; // Dispatched to the STS thread to notify the infoObject of the verification @@ -76,8 +76,8 @@ class SSLServerCertVerificationResult final bool aSucceeded, PRErrorCode aFinalError, nsITransportSecurityInfo::OverridableErrorCategory aOverridableErrorCategory, - bool aIsBuiltCertChainRootBuiltInRoot, - uint32_t aProviderFlags) override; + bool aIsBuiltCertChainRootBuiltInRoot, uint32_t aProviderFlags, + bool aMadeOCSPRequests) override; private: ~SSLServerCertVerificationResult() = default; @@ -92,6 +92,7 @@ class SSLServerCertVerificationResult final nsITransportSecurityInfo::OverridableErrorCategory mOverridableErrorCategory; bool mIsBuiltCertChainRootBuiltInRoot; uint32_t mProviderFlags; + bool mMadeOCSPRequests; }; class SSLServerCertVerificationJob : public Runnable { diff --git a/security/manager/ssl/TransportSecurityInfo.cpp b/security/manager/ssl/TransportSecurityInfo.cpp index 5dd1649a21c6..5173ce9a0ba5 100644 --- a/security/manager/ssl/TransportSecurityInfo.cpp +++ b/security/manager/ssl/TransportSecurityInfo.cpp @@ -54,6 +54,7 @@ TransportSecurityInfo::TransportSecurityInfo() mSignatureSchemeName(), mIsAcceptedEch(false), mIsDelegatedCredential(false), + mMadeOCSPRequests(false), mNPNCompleted(false), mResumed(false), mIsBuiltCertChainRootBuiltInRoot(false), @@ -205,7 +206,7 @@ TransportSecurityInfo::Write(nsIObjectOutputStream* aStream) { // Re-purpose mErrorMessageCached to represent serialization version // If string doesn't match exact version it will be treated as older // serialization. - rv = aStream->WriteWStringZ(NS_ConvertUTF8toUTF16("8").get()); + rv = aStream->WriteWStringZ(NS_ConvertUTF8toUTF16("9").get()); if (NS_FAILED(rv)) { return rv; } @@ -292,6 +293,11 @@ TransportSecurityInfo::Write(nsIObjectOutputStream* aStream) { return rv; } + rv = aStream->WriteBoolean(mMadeOCSPRequests); + if (NS_FAILED(rv)) { + return rv; + } + return NS_OK; } @@ -790,6 +796,15 @@ TransportSecurityInfo::Read(nsIObjectInputStream* aStream) { } } + if (serVersionParsedToInt >= 9) { + rv = aStream->ReadBoolean(&mMadeOCSPRequests); + CHILD_DIAGNOSTIC_ASSERT(NS_SUCCEEDED(rv), + "Deserialization should not fail"); + if (NS_FAILED(rv)) { + return rv; + } + } + return NS_OK; } @@ -822,6 +837,7 @@ void TransportSecurityInfo::SerializeToIPC(IPC::MessageWriter* aWriter) { WriteParam(aWriter, mIsBuiltCertChainRootBuiltInRoot); WriteParam(aWriter, mIsAcceptedEch); WriteParam(aWriter, mPeerId); + WriteParam(aWriter, mMadeOCSPRequests); } bool TransportSecurityInfo::DeserializeFromIPC(IPC::MessageReader* aReader) { @@ -848,7 +864,8 @@ bool TransportSecurityInfo::DeserializeFromIPC(IPC::MessageReader* aReader) { !ReadParam(aReader, &mNPNCompleted) || !ReadParam(aReader, &mNegotiatedNPN) || !ReadParam(aReader, &mResumed) || !ReadParam(aReader, &mIsBuiltCertChainRootBuiltInRoot) || - !ReadParam(aReader, &mIsAcceptedEch) || !ReadParam(aReader, &mPeerId)) { + !ReadParam(aReader, &mIsAcceptedEch) || !ReadParam(aReader, &mPeerId) || + !ReadParam(aReader, &mMadeOCSPRequests)) { return false; } @@ -1179,6 +1196,14 @@ TransportSecurityInfo::GetCertificateTransparencyStatus( return NS_OK; } +NS_IMETHODIMP +TransportSecurityInfo::GetMadeOCSPRequests(bool* aMadeOCSPRequests) { + MutexAutoLock lock(mMutex); + + *aMadeOCSPRequests = mMadeOCSPRequests; + return NS_OK; +} + // static uint16_t TransportSecurityInfo::ConvertCertificateTransparencyInfoToStatus( const mozilla::psm::CertificateTransparencyInfo& info) { diff --git a/security/manager/ssl/TransportSecurityInfo.h b/security/manager/ssl/TransportSecurityInfo.h index 477be5b57131..30854c5d9bbd 100644 --- a/security/manager/ssl/TransportSecurityInfo.h +++ b/security/manager/ssl/TransportSecurityInfo.h @@ -105,6 +105,11 @@ class TransportSecurityInfo : public nsITransportSecurityInfo, mCertificateTransparencyStatus = aCertificateTransparencyStatus; } + void SetMadeOCSPRequest(bool aMadeOCSPRequests) { + MutexAutoLock lock(mMutex); + mMadeOCSPRequests = aMadeOCSPRequests; + } + void SetResumed(bool aResumed); Atomic mOverridableErrorCategory; @@ -134,6 +139,7 @@ class TransportSecurityInfo : public nsITransportSecurityInfo, bool mIsAcceptedEch MOZ_GUARDED_BY(mMutex); bool mIsDelegatedCredential MOZ_GUARDED_BY(mMutex); + bool mMadeOCSPRequests MOZ_GUARDED_BY(mMutex); nsCOMPtr mCallbacks MOZ_GUARDED_BY(mMutex); nsTArray> mSucceededCertChain MOZ_GUARDED_BY(mMutex); diff --git a/security/manager/ssl/VerifySSLServerCertChild.cpp b/security/manager/ssl/VerifySSLServerCertChild.cpp index dff7760c10d3..17ea0e4cfb90 100644 --- a/security/manager/ssl/VerifySSLServerCertChild.cpp +++ b/security/manager/ssl/VerifySSLServerCertChild.cpp @@ -28,7 +28,8 @@ VerifySSLServerCertChild::VerifySSLServerCertChild( ipc::IPCResult VerifySSLServerCertChild::RecvOnVerifiedSSLServerCertSuccess( nsTArray&& aBuiltCertChain, const uint16_t& aCertTransparencyStatus, const uint8_t& aEVStatus, - const bool& aIsBuiltCertChainRootBuiltInRoot) { + const bool& aIsBuiltCertChainRootBuiltInRoot, + const bool& aMadeOCSPRequests) { MOZ_LOG(gPIPNSSLog, LogLevel::Debug, ("[%p] VerifySSLServerCertChild::RecvOnVerifiedSSLServerCertSuccess", this)); @@ -42,19 +43,20 @@ ipc::IPCResult VerifySSLServerCertChild::RecvOnVerifiedSSLServerCertSuccess( std::move(certBytesArray), std::move(mPeerCertChain), aCertTransparencyStatus, static_cast(aEVStatus), true, 0, nsITransportSecurityInfo::OverridableErrorCategory::ERROR_UNSET, - aIsBuiltCertChainRootBuiltInRoot, mProviderFlags); + aIsBuiltCertChainRootBuiltInRoot, mProviderFlags, aMadeOCSPRequests); return IPC_OK(); } ipc::IPCResult VerifySSLServerCertChild::RecvOnVerifiedSSLServerCertFailure( - const int32_t& aFinalError, const uint32_t& aOverridableErrorCategory) { + 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); + false, mProviderFlags, aMadeOCSPRequests); return IPC_OK(); } diff --git a/security/manager/ssl/VerifySSLServerCertChild.h b/security/manager/ssl/VerifySSLServerCertChild.h index e393080083e3..94fd5d48d0b9 100644 --- a/security/manager/ssl/VerifySSLServerCertChild.h +++ b/security/manager/ssl/VerifySSLServerCertChild.h @@ -33,10 +33,12 @@ class VerifySSLServerCertChild : public PVerifySSLServerCertChild { ipc::IPCResult RecvOnVerifiedSSLServerCertSuccess( nsTArray&& aBuiltCertChain, const uint16_t& aCertTransparencyStatus, const uint8_t& aEVStatus, - const bool& aIsBuiltCertChainRootBuiltInRoot); + const bool& aIsBuiltCertChainRootBuiltInRoot, + const bool& aMadeOCSPRequests); ipc::IPCResult RecvOnVerifiedSSLServerCertFailure( - const int32_t& aFinalError, const uint32_t& aOverridableErrorCategory); + 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 a56224d295a3..297d3eabd19c 100644 --- a/security/manager/ssl/VerifySSLServerCertParent.cpp +++ b/security/manager/ssl/VerifySSLServerCertParent.cpp @@ -32,7 +32,7 @@ void VerifySSLServerCertParent::OnVerifiedSSLServerCert( const nsTArray& aBuiltCertChain, uint16_t aCertificateTransparencyStatus, uint8_t aEVStatus, bool aSucceeded, PRErrorCode aFinalError, uint32_t aOverridableErrorCategory, - bool aIsBuiltCertChainRootBuiltInRoot) { + bool aIsBuiltCertChainRootBuiltInRoot, bool aMadeOCSPRequests) { AssertIsOnBackgroundThread(); if (!CanSend()) { @@ -42,10 +42,10 @@ void VerifySSLServerCertParent::OnVerifiedSSLServerCert( if (aSucceeded) { Unused << SendOnVerifiedSSLServerCertSuccess( aBuiltCertChain, aCertificateTransparencyStatus, aEVStatus, - aIsBuiltCertChainRootBuiltInRoot); + aIsBuiltCertChainRootBuiltInRoot, aMadeOCSPRequests); } else { - Unused << SendOnVerifiedSSLServerCertFailure(aFinalError, - aOverridableErrorCategory); + Unused << SendOnVerifiedSSLServerCertFailure( + aFinalError, aOverridableErrorCategory, aMadeOCSPRequests); } Unused << Send__delete__(this); } @@ -68,8 +68,8 @@ class IPCServerCertVerificationResult final bool aSucceeded, PRErrorCode aFinalError, nsITransportSecurityInfo::OverridableErrorCategory aOverridableErrorCategory, - bool aIsBuiltCertChainRootBuiltInRoot, - uint32_t aProviderFlags) override; + bool aIsBuiltCertChainRootBuiltInRoot, uint32_t aProviderFlags, + bool aMadeOCSPRequests) override; private: ~IPCServerCertVerificationResult() = default; @@ -85,7 +85,8 @@ void IPCServerCertVerificationResult::Dispatch( bool aSucceeded, PRErrorCode aFinalError, nsITransportSecurityInfo::OverridableErrorCategory aOverridableErrorCategory, - bool aIsBuiltCertChainRootBuiltInRoot, uint32_t aProviderFlags) { + bool aIsBuiltCertChainRootBuiltInRoot, uint32_t aProviderFlags, + bool aMadeOCSPRequests) { nsTArray builtCertChain; if (aSucceeded) { for (auto& cert : aBuiltChain) { @@ -99,7 +100,7 @@ void IPCServerCertVerificationResult::Dispatch( [parent(mParent), builtCertChain{std::move(builtCertChain)}, aCertificateTransparencyStatus, aEVStatus, aSucceeded, aFinalError, aOverridableErrorCategory, aIsBuiltCertChainRootBuiltInRoot, - aProviderFlags]() { + aMadeOCSPRequests, aProviderFlags]() { if (aSucceeded && !(aProviderFlags & nsISocketProvider::NO_PERMANENT_STORAGE)) { nsTArray> certBytesArray; @@ -114,7 +115,7 @@ void IPCServerCertVerificationResult::Dispatch( builtCertChain, aCertificateTransparencyStatus, static_cast(aEVStatus), aSucceeded, aFinalError, static_cast(aOverridableErrorCategory), - aIsBuiltCertChainRootBuiltInRoot); + aIsBuiltCertChainRootBuiltInRoot, aMadeOCSPRequests); }), NS_DISPATCH_NORMAL); MOZ_DIAGNOSTIC_ASSERT(NS_SUCCEEDED(nrv)); diff --git a/security/manager/ssl/VerifySSLServerCertParent.h b/security/manager/ssl/VerifySSLServerCertParent.h index d3e8f2d1e7c8..6c79b7c2c14e 100644 --- a/security/manager/ssl/VerifySSLServerCertParent.h +++ b/security/manager/ssl/VerifySSLServerCertParent.h @@ -44,7 +44,8 @@ class VerifySSLServerCertParent : public PVerifySSLServerCertParent { uint8_t aEVStatus, bool aSucceeded, PRErrorCode aFinalError, uint32_t aOverridableErrorCategory, - bool aIsBuiltCertChainRootBuiltInRoot); + bool aIsBuiltCertChainRootBuiltInRoot, + bool aMadeOCSPRequests); private: virtual ~VerifySSLServerCertParent(); diff --git a/security/manager/ssl/tests/unit/test_ocsp_required.js b/security/manager/ssl/tests/unit/test_ocsp_required.js index 6e1c52a6eba4..17f471d66c66 100644 --- a/security/manager/ssl/tests/unit/test_ocsp_required.js +++ b/security/manager/ssl/tests/unit/test_ocsp_required.js @@ -48,11 +48,25 @@ function run_test() { function add_tests() { add_connection_test( "ocsp-stapling-none.example.com", - SEC_ERROR_OCSP_BAD_SIGNATURE + SEC_ERROR_OCSP_BAD_SIGNATURE, + function() {}, + function(aTransportSecurityInfo) { + Assert.ok( + aTransportSecurityInfo.madeOCSPRequests, + "An OCSP Request should have been made." + ); + } ); add_connection_test( "ocsp-stapling-none.example.com", - SEC_ERROR_OCSP_BAD_SIGNATURE + SEC_ERROR_OCSP_BAD_SIGNATURE, + function() {}, + function(aTransportSecurityInfo) { + Assert.ok( + !aTransportSecurityInfo.madeOCSPRequests, + "An OCSP Request should not have been made." + ); + } ); add_test(function() { equal( @@ -69,6 +83,13 @@ function add_tests() { add_connection_test( "ocsp-stapling-none.example.com", - SEC_ERROR_OCSP_MALFORMED_RESPONSE + SEC_ERROR_OCSP_MALFORMED_RESPONSE, + function() {}, + function(aTransportSecurityInfo) { + Assert.ok( + aTransportSecurityInfo.madeOCSPRequests, + "An OCSP Request should have been made." + ); + } ); }