From d74d5e91d628187de3b7338aa134aefe571d49c6 Mon Sep 17 00:00:00 2001 From: Dana Keeler Date: Fri, 10 Dec 2021 21:14:23 +0000 Subject: [PATCH] Bug 1712972 - only call IsCertBuiltInRoot on the socket thread during certificate verification r=jschanck Based on a patch authored by R. Martinho Fernandes . Differential Revision: https://phabricator.services.mozilla.com/D116505 --- security/certverifier/CertVerifier.cpp | 89 +++++++++---------- security/certverifier/CertVerifier.h | 5 +- .../certverifier/NSSCertDBTrustDomain.cpp | 47 ++++++++-- security/certverifier/NSSCertDBTrustDomain.h | 3 + .../manager/ssl/SSLServerCertVerification.cpp | 53 +++++------ 5 files changed, 110 insertions(+), 87 deletions(-) diff --git a/security/certverifier/CertVerifier.cpp b/security/certverifier/CertVerifier.cpp index 5a15aea209da..559581d0a202 100644 --- a/security/certverifier/CertVerifier.cpp +++ b/security/certverifier/CertVerifier.cpp @@ -22,6 +22,7 @@ #include "mozilla/IntegerPrintfMacros.h" #include "mozilla/Logging.h" #include "nsNSSComponent.h" +#include "mozilla/SyncRunnable.h" #include "nsPromiseFlatString.h" #include "nsServiceManagerUtils.h" #include "pk11pub.h" @@ -30,6 +31,7 @@ #include "mozpkix/pkixnss.h" #include "mozpkix/pkixutil.h" #include "secmod.h" +#include "nsNetCID.h" using namespace mozilla::ct; using namespace mozilla::pkix; @@ -145,20 +147,6 @@ CertVerifier::CertVerifier(OcspDownloadConfig odc, OcspStrictConfig osc, CertVerifier::~CertVerifier() = default; -Result IsCertChainRootBuiltInRoot(const nsTArray>& chain, - bool& result) { - if (chain.IsEmpty()) { - return Result::FATAL_ERROR_LIBRARY_FAILURE; - } - const nsTArray& rootBytes = chain.LastElement(); - Input rootInput; - Result rv = rootInput.Init(rootBytes.Elements(), rootBytes.Length()); - if (rv != Result::Success) { - return rv; - } - return IsCertBuiltInRoot(rootInput, result); -} - Result IsDelegatedCredentialAcceptable(const DelegatedCredentialInfo& dcInfo) { bool isEcdsa = dcInfo.scheme == ssl_sig_ecdsa_secp256r1_sha256 || dcInfo.scheme == ssl_sig_ecdsa_secp384r1_sha384 || @@ -179,6 +167,12 @@ Result IsDelegatedCredentialAcceptable(const DelegatedCredentialInfo& dcInfo) { // has been added to the NSS trust store, because it has been approved // for inclusion according to the Mozilla CA policy, and might be accepted // by Mozilla applications as an issuer for certificates seen on the public web. +// Ideally this would only be called from the socket thread. In the context of +// TLS server certificate verification, IsCertBuiltInRootWithSyncDispatch +// ensures this function is only called from the socket thread. However, there +// are other code paths that reach this function that do not run on the socket +// thread. None of these code paths should be reachable during TLS server +// certificate verification. Result IsCertBuiltInRoot(Input certInput, bool& result) { if (NS_FAILED(BlockUntilLoadableCertsLoaded())) { return Result::FATAL_ERROR_LIBRARY_FAILURE; @@ -496,7 +490,8 @@ Result CertVerifier::VerifyCert( /*optional out*/ KeySizeStatus* keySizeStatus, /*optional out*/ SHA1ModeResult* sha1ModeResult, /*optional out*/ PinningTelemetryInfo* pinningTelemetryInfo, - /*optional out*/ CertificateTransparencyInfo* ctInfo) { + /*optional out*/ CertificateTransparencyInfo* ctInfo, + /*optional out*/ bool* isBuiltChainRootBuiltInRoot) { MOZ_LOG(gCertVerifierLog, LogLevel::Debug, ("Top of VerifyCert\n")); MOZ_ASSERT(usage == certificateUsageSSLServer || !(flags & FLAG_MUST_BE_EV)); @@ -538,6 +533,10 @@ Result CertVerifier::VerifyCert( return Result::FATAL_ERROR_INVALID_ARGS; } + if (isBuiltChainRootBuiltInRoot) { + *isBuiltChainRootBuiltInRoot = false; + } + Input certDER; Result rv = certDER.Init(certBytes.Elements(), certBytes.Length()); if (rv != Success) { @@ -665,15 +664,9 @@ Result CertVerifier::VerifyCert( KeyPurposeId::id_kp_serverAuth, evPolicy, stapledOCSPResponse, ocspStaplingStatus); if (rv == Success && - sha1ModeConfigurations[i] == SHA1Mode::ImportedRoot) { - bool isBuiltInRoot = false; - rv = IsCertChainRootBuiltInRoot(builtChain, isBuiltInRoot); - if (rv != Success) { - break; - } - if (isBuiltInRoot) { - rv = Result::ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED; - } + sha1ModeConfigurations[i] == SHA1Mode::ImportedRoot && + trustDomain.GetIsBuiltChainRootBuiltInRoot()) { + rv = Result::ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED; } if (rv == Success) { MOZ_LOG(gCertVerifierLog, LogLevel::Debug, @@ -690,6 +683,10 @@ Result CertVerifier::VerifyCert( if (rv != Success) { break; } + if (isBuiltChainRootBuiltInRoot) { + *isBuiltChainRootBuiltInRoot = + trustDomain.GetIsBuiltChainRootBuiltInRoot(); + } } } if (rv == Success) { @@ -756,15 +753,9 @@ Result CertVerifier::VerifyCert( rv = Result::ERROR_ADDITIONAL_POLICY_CONSTRAINT_FAILED; } if (rv == Success && - sha1ModeConfigurations[j] == SHA1Mode::ImportedRoot) { - bool isBuiltInRoot = false; - rv = IsCertChainRootBuiltInRoot(builtChain, isBuiltInRoot); - if (rv != Success) { - break; - } - if (isBuiltInRoot) { - rv = Result::ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED; - } + sha1ModeConfigurations[j] == SHA1Mode::ImportedRoot && + trustDomain.GetIsBuiltChainRootBuiltInRoot()) { + rv = Result::ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED; } if (rv == Success) { if (keySizeStatus) { @@ -778,6 +769,10 @@ Result CertVerifier::VerifyCert( if (rv != Success) { break; } + if (isBuiltChainRootBuiltInRoot) { + *isBuiltChainRootBuiltInRoot = + trustDomain.GetIsBuiltChainRootBuiltInRoot(); + } } } } @@ -904,12 +899,12 @@ Result CertVerifier::VerifySSLServerCert( /*optional out*/ SHA1ModeResult* sha1ModeResult, /*optional out*/ PinningTelemetryInfo* pinningTelemetryInfo, /*optional out*/ CertificateTransparencyInfo* ctInfo, - /*optional out*/ bool* isBuiltCertChainRootBuiltInRoot) { + /*optional out*/ bool* isBuiltChainRootBuiltInRoot) { // XXX: MOZ_ASSERT(pinarg); MOZ_ASSERT(!hostname.IsEmpty()); - if (isBuiltCertChainRootBuiltInRoot) { - *isBuiltCertChainRootBuiltInRoot = false; + if (isBuiltChainRootBuiltInRoot) { + *isBuiltChainRootBuiltInRoot = false; } if (evStatus) { @@ -928,11 +923,13 @@ Result CertVerifier::VerifySSLServerCert( if (rv != Success) { return rv; } + bool isBuiltChainRootBuiltInRootLocal; rv = VerifyCert(peerCertBytes, certificateUsageSSLServer, time, pinarg, PromiseFlatCString(hostname).get(), builtChain, flags, extraCertificates, stapledOCSPResponse, sctsFromTLS, originAttributes, evStatus, ocspStaplingStatus, keySizeStatus, - sha1ModeResult, pinningTelemetryInfo, ctInfo); + sha1ModeResult, pinningTelemetryInfo, ctInfo, + &isBuiltChainRootBuiltInRootLocal); if (rv != Success) { // we don't use the certificate for path building, so this parameter doesn't // matter @@ -1007,19 +1004,11 @@ Result CertVerifier::VerifySSLServerCert( if (rv != Success) { return Result::FATAL_ERROR_INVALID_ARGS; } - bool isBuiltInRoot; - rv = IsCertChainRootBuiltInRoot(builtChain, isBuiltInRoot); - if (rv != Success) { - return rv; - } - - if (isBuiltCertChainRootBuiltInRoot) { - *isBuiltCertChainRootBuiltInRoot = isBuiltInRoot; - } BRNameMatchingPolicy nameMatchingPolicy( - isBuiltInRoot ? mNameMatchingMode - : BRNameMatchingPolicy::Mode::DoNotEnforce); + isBuiltChainRootBuiltInRootLocal + ? mNameMatchingMode + : BRNameMatchingPolicy::Mode::DoNotEnforce); rv = CheckCertHostname(peerCertInput, hostnameInput, nameMatchingPolicy); if (rv != Success) { // Treat malformed name information as a domain mismatch. @@ -1030,6 +1019,10 @@ Result CertVerifier::VerifySSLServerCert( return rv; } + if (isBuiltChainRootBuiltInRoot) { + *isBuiltChainRootBuiltInRoot = isBuiltChainRootBuiltInRootLocal; + } + return Success; } diff --git a/security/certverifier/CertVerifier.h b/security/certverifier/CertVerifier.h index cdb3eb1628fe..25545bf77953 100644 --- a/security/certverifier/CertVerifier.h +++ b/security/certverifier/CertVerifier.h @@ -172,7 +172,8 @@ class CertVerifier { /*optional out*/ KeySizeStatus* keySizeStatus = nullptr, /*optional out*/ SHA1ModeResult* sha1ModeResult = nullptr, /*optional out*/ PinningTelemetryInfo* pinningTelemetryInfo = nullptr, - /*optional out*/ CertificateTransparencyInfo* ctInfo = nullptr); + /*optional out*/ CertificateTransparencyInfo* ctInfo = nullptr, + /*optional out*/ bool* isBuiltChainRootBuiltInRoot = nullptr); mozilla::pkix::Result VerifySSLServerCert( const nsTArray& peerCert, mozilla::pkix::Time time, void* pinarg, @@ -193,7 +194,7 @@ class CertVerifier { /*optional out*/ SHA1ModeResult* sha1ModeResult = nullptr, /*optional out*/ PinningTelemetryInfo* pinningTelemetryInfo = nullptr, /*optional out*/ CertificateTransparencyInfo* ctInfo = nullptr, - /*optional out*/ bool* isBuiltCertChainRootBuiltInRoot = nullptr); + /*optional out*/ bool* isBuiltChainRootBuiltInRoot = nullptr); enum class SHA1Mode { Allowed = 0, diff --git a/security/certverifier/NSSCertDBTrustDomain.cpp b/security/certverifier/NSSCertDBTrustDomain.cpp index a26121054c5a..bac3b6b874f7 100644 --- a/security/certverifier/NSSCertDBTrustDomain.cpp +++ b/security/certverifier/NSSCertDBTrustDomain.cpp @@ -97,6 +97,7 @@ NSSCertDBTrustDomain::NSSCertDBTrustDomain( mThirdPartyIntermediateInputs(thirdPartyIntermediateInputs), mExtraCertificates(extraCertificates), mBuiltChain(builtChain), + mIsBuiltChainRootBuiltInRoot(false), mPinningTelemetryInfo(pinningTelemetryInfo), mHostname(hostname), mCertStorage(do_GetService(NS_CERT_STORAGE_CID)), @@ -1260,6 +1261,36 @@ nsresult isDistrustedCertificateChain( return NS_OK; } +// This is used by NSSCertDBTrustDomain to ensure IsCertBuiltInRoot is only +// called from the socket thread during TLS server certificate verification. +Result IsCertBuiltInRootWithSyncDispatch(Input certInput, bool& result) { + nsCOMPtr socketThread( + do_GetService(NS_SOCKETTRANSPORTSERVICE_CONTRACTID)); + if (!socketThread) { + return Result::FATAL_ERROR_LIBRARY_FAILURE; + } + bool onSocketThread = true; + nsresult rv = socketThread->IsOnCurrentThread(&onSocketThread); + if (NS_FAILED(rv) || onSocketThread) { + return Result::FATAL_ERROR_LIBRARY_FAILURE; + } + + result = false; + Result runnableRV = Result::FATAL_ERROR_LIBRARY_FAILURE; + + RefPtr isBuiltInRootTask = NS_NewRunnableFunction( + "IsCertBuiltInRoot", + [&]() { runnableRV = IsCertBuiltInRoot(certInput, result); }); + rv = SyncRunnable::DispatchToThread(socketThread, isBuiltInRootTask); + if (NS_FAILED(rv)) { + return Result::FATAL_ERROR_LIBRARY_FAILURE; + } + if (runnableRV != Success) { + return runnableRV; + } + return Success; +} + Result NSSCertDBTrustDomain::IsChainValid(const DERArray& reversedDERArray, Time time, const CertPolicyId& requiredPolicy) { @@ -1276,15 +1307,14 @@ Result NSSCertDBTrustDomain::IsChainValid(const DERArray& reversedDERArray, certArray.EmplaceBack(derInput->UnsafeGetData(), derInput->GetLength()); } - bool isBuiltInRoot = false; - const nsTArray& rootBytes = certArray.LastElement(); Input rootInput; Result rv = rootInput.Init(rootBytes.Elements(), rootBytes.Length()); if (rv != Success) { return rv; } - rv = IsCertBuiltInRoot(rootInput, isBuiltInRoot); + rv = IsCertBuiltInRootWithSyncDispatch(rootInput, + mIsBuiltChainRootBuiltInRoot); if (rv != Result::Success) { return rv; } @@ -1299,8 +1329,8 @@ Result NSSCertDBTrustDomain::IsChainValid(const DERArray& reversedDERArray, bool chainHasValidPins; nsrv = PublicKeyPinningService::ChainHasValidPins( - derCertSpanList, mHostname, time, isBuiltInRoot, chainHasValidPins, - mPinningTelemetryInfo); + derCertSpanList, mHostname, time, mIsBuiltChainRootBuiltInRoot, + chainHasValidPins, mPinningTelemetryInfo); if (NS_FAILED(nsrv)) { return Result::FATAL_ERROR_LIBRARY_FAILURE; } @@ -1311,7 +1341,7 @@ Result NSSCertDBTrustDomain::IsChainValid(const DERArray& reversedDERArray, // Check that the childs' certificate NotBefore date is anterior to // the NotAfter value of the parent when the root is a builtin. - if (isBuiltInRoot) { + if (mIsBuiltChainRootBuiltInRoot) { bool isDistrusted; nsrv = isDistrustedCertificateChain(certArray, mCertDBTrustType, isDistrusted); @@ -1501,6 +1531,7 @@ void NSSCertDBTrustDomain::ResetAccumulatedState() { mSCTListFromOCSPStapling = nullptr; mSCTListFromCertificate = nullptr; mSawDistrustedCAByPolicyError = false; + mIsBuiltChainRootBuiltInRoot = false; } static Input SECItemToInput(const UniqueSECItem& item) { @@ -1524,6 +1555,10 @@ Input NSSCertDBTrustDomain::GetSCTListFromOCSPStapling() const { return SECItemToInput(mSCTListFromOCSPStapling); } +bool NSSCertDBTrustDomain::GetIsBuiltChainRootBuiltInRoot() const { + return mIsBuiltChainRootBuiltInRoot; +} + bool NSSCertDBTrustDomain::GetIsErrorDueToDistrustedCAPolicy() const { return mSawDistrustedCAByPolicyError; } diff --git a/security/certverifier/NSSCertDBTrustDomain.h b/security/certverifier/NSSCertDBTrustDomain.h index fa24b85f2e24..c3ac82420c96 100644 --- a/security/certverifier/NSSCertDBTrustDomain.h +++ b/security/certverifier/NSSCertDBTrustDomain.h @@ -212,6 +212,8 @@ class NSSCertDBTrustDomain : public mozilla::pkix::TrustDomain { mozilla::pkix::Input GetSCTListFromCertificate() const; mozilla::pkix::Input GetSCTListFromOCSPStapling() const; + bool GetIsBuiltChainRootBuiltInRoot() const; + bool GetIsErrorDueToDistrustedCAPolicy() const; private: @@ -263,6 +265,7 @@ class NSSCertDBTrustDomain : public mozilla::pkix::TrustDomain { mThirdPartyIntermediateInputs; // non-owning const Maybe>>& mExtraCertificates; // non-owning nsTArray>& mBuiltChain; // non-owning + bool mIsBuiltChainRootBuiltInRoot; PinningTelemetryInfo* mPinningTelemetryInfo; const char* mHostname; // non-owning - only used for pinning checks nsCOMPtr mCertStorage; diff --git a/security/manager/ssl/SSLServerCertVerification.cpp b/security/manager/ssl/SSLServerCertVerification.cpp index a2103903b9f3..36cf17ec8c90 100644 --- a/security/manager/ssl/SSLServerCertVerification.cpp +++ b/security/manager/ssl/SSLServerCertVerification.cpp @@ -522,12 +522,13 @@ static SECStatus BlockServerCertChangeForSpdy( // Gather telemetry on whether the end-entity cert for a server has the // required TLS Server Authentication EKU, or any others void GatherEKUTelemetry(const UniqueCERTCertList& certList) { + MOZ_ASSERT(!CERT_LIST_EMPTY(certList)); + if (CERT_LIST_EMPTY(certList)) { + return; + } CERTCertListNode* endEntityNode = CERT_LIST_HEAD(certList); - CERTCertListNode* rootNode = CERT_LIST_TAIL(certList); - MOZ_ASSERT(!(CERT_LIST_END(endEntityNode, certList) || - CERT_LIST_END(rootNode, certList))); - if (CERT_LIST_END(endEntityNode, certList) || - CERT_LIST_END(rootNode, certList)) { + MOZ_ASSERT(endEntityNode); + if (!endEntityNode) { return; } CERTCertificate* endEntityCert = endEntityNode->cert; @@ -536,23 +537,6 @@ void GatherEKUTelemetry(const UniqueCERTCertList& certList) { return; } - // Only log telemetry if the root CA is built-in - CERTCertificate* rootCert = rootNode->cert; - MOZ_ASSERT(rootCert); - if (!rootCert) { - return; - } - bool isBuiltIn = false; - Input rootInput; - Result rv = rootInput.Init(rootCert->derCert.data, rootCert->derCert.len); - if (rv != Result::Success) { - return; - } - rv = IsCertBuiltInRoot(rootInput, isBuiltIn); - if (rv != Success || !isBuiltIn) { - return; - } - // Find the EKU extension, if present bool foundEKU = false; SECOidTag oidTag; @@ -628,8 +612,12 @@ void GatherRootCATelemetry(const UniqueCERTCertList& certList) { // There are various things that we want to measure about certificate // chains that we accept. This is a single entry point for all of them. -void GatherSuccessfulValidationTelemetry(const UniqueCERTCertList& certList) { - GatherEKUTelemetry(certList); +void GatherSuccessfulValidationTelemetry(const UniqueCERTCertList& certList, + bool isCertListRootBuiltInRoot) { + if (isCertListRootBuiltInRoot) { + // Only gather this telemetry if the root CA is built-in + GatherEKUTelemetry(certList); + } GatherRootCATelemetry(certList); } @@ -765,6 +753,7 @@ static void CollectCertTelemetry( KeySizeStatus aKeySizeStatus, SHA1ModeResult aSha1ModeResult, const PinningTelemetryInfo& aPinningTelemetryInfo, const nsTArray>& aBuiltCertChain, + bool aIsBuiltCertChainRootBuiltInRoot, const CertificateTransparencyInfo& aCertificateTransparencyInfo) { UniqueCERTCertList builtCertChainList(CERT_NewCertList()); if (!builtCertChainList) { @@ -818,7 +807,8 @@ static void CollectCertTelemetry( } if (aCertVerificationResult == Success) { - GatherSuccessfulValidationTelemetry(builtCertChainList); + GatherSuccessfulValidationTelemetry(builtCertChainList, + aIsBuiltCertChainRootBuiltInRoot); GatherCertificateTransparencyTelemetry(builtCertChainList, aEVStatus == EVStatus::EV, aCertificateTransparencyInfo); @@ -830,7 +820,7 @@ static void AuthCertificateSetResults( nsTArray>&& aBuiltCertChain, nsTArray>&& aPeerCertChain, uint16_t aCertificateTransparencyStatus, EVStatus aEvStatus, - bool aSucceeded, bool aIsCertChainRootBuiltInRoot) { + bool aSucceeded, bool aIsBuiltCertChainRootBuiltInRoot) { MOZ_ASSERT(aInfoObject); if (aSucceeded) { // Certificate verification succeeded. Delete any potential record of @@ -844,7 +834,7 @@ static void AuthCertificateSetResults( ("AuthCertificate setting NEW cert %p", aCert)); aInfoObject->SetIsBuiltCertChainRootBuiltInRoot( - aIsCertChainRootBuiltInRoot); + aIsBuiltCertChainRootBuiltInRoot); aInfoObject->SetCertificateTransparencyStatus( aCertificateTransparencyStatus); } else { @@ -867,7 +857,7 @@ Result AuthCertificate( /*out*/ nsTArray>& builtCertChain, /*out*/ EVStatus& evStatus, /*out*/ CertificateTransparencyInfo& certificateTransparencyInfo, - /*out*/ bool& aIsCertChainRootBuiltInRoot) { + /*out*/ bool& aIsBuiltCertChainRootBuiltInRoot) { CertVerifier::OCSPStaplingStatus ocspStaplingStatus = CertVerifier::OCSP_STAPLING_NEVER_CHECKED; KeySizeStatus keySizeStatus = KeySizeStatus::NeverChecked; @@ -889,10 +879,11 @@ Result AuthCertificate( sctsFromTLSExtension, dcInfo, aOriginAttributes, &evStatus, &ocspStaplingStatus, &keySizeStatus, &sha1ModeResult, &pinningTelemetryInfo, &certificateTransparencyInfo, - &aIsCertChainRootBuiltInRoot); + &aIsBuiltCertChainRootBuiltInRoot); CollectCertTelemetry(rv, evStatus, ocspStaplingStatus, keySizeStatus, sha1ModeResult, pinningTelemetryInfo, builtCertChain, + aIsBuiltCertChainRootBuiltInRoot, certificateTransparencyInfo); return rv; @@ -1368,7 +1359,7 @@ void SSLServerCertVerificationResult::Dispatch( nsTArray>&& aPeerCertChain, uint16_t aCertificateTransparencyStatus, EVStatus aEVStatus, bool aSucceeded, PRErrorCode aFinalError, uint32_t aCollectedErrors, - bool aIsCertChainRootBuiltInRoot, uint32_t aProviderFlags) { + bool aIsBuiltCertChainRootBuiltInRoot, uint32_t aProviderFlags) { mCert = aCert; mBuiltChain = std::move(aBuiltChain); mPeerCertChain = std::move(aPeerCertChain); @@ -1377,7 +1368,7 @@ void SSLServerCertVerificationResult::Dispatch( mSucceeded = aSucceeded; mFinalError = aFinalError; mCollectedErrors = aCollectedErrors; - mIsBuiltCertChainRootBuiltInRoot = aIsCertChainRootBuiltInRoot; + mIsBuiltCertChainRootBuiltInRoot = aIsBuiltCertChainRootBuiltInRoot; mProviderFlags = aProviderFlags; nsresult rv;