corysanin/tubestation
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Bug 1915008 - leak mResultTask if dispatch fails in certificate verification r=jschanck

Browse Source 
Differential Revision: https://phabricator.services.mozilla.com/D221381
This commit is contained in:
Dana Keeler
2024-09-09 20:55:21 +00:00
parent b6a880c1f7
commit c554bb14cd
4 changed files with 67 additions and 50 deletions
Download Patch File Download Diff File Expand all files Collapse all files

42
security/manager/ssl/SSLServerCertVerification.cpp
View File

@@ -741,17 +741,15 @@ SSLServerCertVerificationJob::Run() {
// Runs on a cert verification thread and only on parent process. // Runs on a cert verification thread and only on parent process.
MOZ_ASSERT(XRE_IsParentProcess()); MOZ_ASSERT(XRE_IsParentProcess());
MOZ_LOG( MOZ_LOG(gPIPNSSLog, LogLevel::Debug,
gPIPNSSLog, LogLevel::Debug, ("[%" PRIx64 "] SSLServerCertVerificationJob::Run", mAddrForLogging));
("[%" PRIx64 "] SSLServerCertVerificationJob::Run\n", mAddrForLogging));
RefPtr<SharedCertVerifier> certVerifier(GetDefaultCertVerifier()); RefPtr<SharedCertVerifier> certVerifier(GetDefaultCertVerifier());
if (!certVerifier) { if (!certVerifier) {
PR_SetError(SEC_ERROR_NOT_INITIALIZED, 0);
// We can't release this off the STS thread because some parts of it // We can't release this off the STS thread because some parts of it
// are not threadsafe. Just leak the mResultTask // are not threadsafe. Just leak mResultTask.
Unused << mResultTask.forget(); Unused << mResultTask.forget();
return NS_OK; return NS_ERROR_FAILURE;
} }
TimeStamp jobStartTime = TimeStamp::Now(); TimeStamp jobStartTime = TimeStamp::Now();
@@ -761,7 +759,7 @@ SSLServerCertVerificationJob::Run() {
bool madeOCSPRequests = false; bool madeOCSPRequests = false;
nsTArray<nsTArray<uint8_t>> builtChainBytesArray; nsTArray<nsTArray<uint8_t>> builtChainBytesArray;
nsTArray<uint8_t> certBytes(mPeerCertChain.ElementAt(0).Clone()); nsTArray<uint8_t> certBytes(mPeerCertChain.ElementAt(0).Clone());
Result rv = AuthCertificate( Result result = AuthCertificate(
*certVerifier, mPinArg, certBytes, mPeerCertChain, mHostName, *certVerifier, mPinArg, certBytes, mPeerCertChain, mHostName,
mOriginAttributes, mStapledOCSPResponse, mSCTsFromTLSExtension, mDCInfo, mOriginAttributes, mStapledOCSPResponse, mSCTsFromTLSExtension, mDCInfo,
mProviderFlags, mTime, mCertVerifierFlags, builtChainBytesArray, evStatus, mProviderFlags, mTime, mCertVerifierFlags, builtChainBytesArray, evStatus,
@@ -769,25 +767,30 @@ SSLServerCertVerificationJob::Run() {
madeOCSPRequests); madeOCSPRequests);
TimeDuration elapsed = TimeStamp::Now() - jobStartTime; TimeDuration elapsed = TimeStamp::Now() - jobStartTime;
if (rv == Success) { if (result == Success) {
mozilla::glean::cert_verification_time::success.AccumulateRawDuration( mozilla::glean::cert_verification_time::success.AccumulateRawDuration(
elapsed); elapsed);
Telemetry::Accumulate(Telemetry::SSL_CERT_ERROR_OVERRIDES, 1); Telemetry::Accumulate(Telemetry::SSL_CERT_ERROR_OVERRIDES, 1);
mResultTask->Dispatch( nsresult rv = mResultTask->Dispatch(
std::move(builtChainBytesArray), std::move(mPeerCertChain), std::move(builtChainBytesArray), std::move(mPeerCertChain),
TransportSecurityInfo::ConvertCertificateTransparencyInfoToStatus( TransportSecurityInfo::ConvertCertificateTransparencyInfoToStatus(
certificateTransparencyInfo), certificateTransparencyInfo),
evStatus, true, 0, evStatus, true, 0,
nsITransportSecurityInfo::OverridableErrorCategory::ERROR_UNSET, nsITransportSecurityInfo::OverridableErrorCategory::ERROR_UNSET,
isCertChainRootBuiltInRoot, mProviderFlags, madeOCSPRequests); isCertChainRootBuiltInRoot, mProviderFlags, madeOCSPRequests);
return NS_OK; if (NS_FAILED(rv)) {
// We can't release this off the STS thread because some parts of it
// are not threadsafe. Just leak mResultTask.
Unused << mResultTask.forget();
}
return rv;
} }
mozilla::glean::cert_verification_time::failure.AccumulateRawDuration( mozilla::glean::cert_verification_time::failure.AccumulateRawDuration(
elapsed); elapsed);
PRErrorCode error = MapResultToPRErrorCode(rv); PRErrorCode error = MapResultToPRErrorCode(result);
nsITransportSecurityInfo::OverridableErrorCategory overridableErrorCategory = nsITransportSecurityInfo::OverridableErrorCategory overridableErrorCategory =
nsITransportSecurityInfo::OverridableErrorCategory::ERROR_UNSET; nsITransportSecurityInfo::OverridableErrorCategory::ERROR_UNSET;
nsCOMPtr<nsIX509Cert> cert(new nsNSSCertificate(std::move(certBytes))); nsCOMPtr<nsIX509Cert> cert(new nsNSSCertificate(std::move(certBytes)));
@@ -796,16 +799,22 @@ SSLServerCertVerificationJob::Run() {
overridableErrorCategory); overridableErrorCategory);
// NB: finalError may be 0 here, in which the connection will continue. // NB: finalError may be 0 here, in which the connection will continue.
mResultTask->Dispatch( nsresult rv = mResultTask->Dispatch(
std::move(builtChainBytesArray), std::move(mPeerCertChain), std::move(builtChainBytesArray), std::move(mPeerCertChain),
nsITransportSecurityInfo::CERTIFICATE_TRANSPARENCY_NOT_APPLICABLE, nsITransportSecurityInfo::CERTIFICATE_TRANSPARENCY_NOT_APPLICABLE,
EVStatus::NotEV, false, finalError, overridableErrorCategory, EVStatus::NotEV, false, finalError, overridableErrorCategory,
// If the certificate verifier returned Result::ERROR_BAD_CERT_DOMAIN, a // If the certificate verifier returned Result::ERROR_BAD_CERT_DOMAIN, a
// chain was built, so isCertChainRootBuiltInRoot is valid and // chain was built, so isCertChainRootBuiltInRoot is valid and
// potentially useful. Otherwise, assume no chain was built. // potentially useful. Otherwise, assume no chain was built.
rv == Result::ERROR_BAD_CERT_DOMAIN ? isCertChainRootBuiltInRoot : false, result == Result::ERROR_BAD_CERT_DOMAIN ? isCertChainRootBuiltInRoot
: false,
mProviderFlags, madeOCSPRequests); mProviderFlags, madeOCSPRequests);
return NS_OK; if (NS_FAILED(rv)) {
// We can't release this off the STS thread because some parts of it
// are not threadsafe. Just leak mResultTask.
Unused << mResultTask.forget();
}
return rv;
} }
// Takes information needed for cert verification, does some consistency // Takes information needed for cert verification, does some consistency
@@ -1029,7 +1038,7 @@ SSLServerCertVerificationResult::SSLServerCertVerificationResult(
nsITransportSecurityInfo::OverridableErrorCategory::ERROR_UNSET), nsITransportSecurityInfo::OverridableErrorCategory::ERROR_UNSET),
mProviderFlags(0) {} mProviderFlags(0) {}
void SSLServerCertVerificationResult::Dispatch( nsresult SSLServerCertVerificationResult::Dispatch(
nsTArray<nsTArray<uint8_t>>&& aBuiltChain, nsTArray<nsTArray<uint8_t>>&& aBuiltChain,
nsTArray<nsTArray<uint8_t>>&& aPeerCertChain, nsTArray<nsTArray<uint8_t>>&& aPeerCertChain,
uint16_t aCertificateTransparencyStatus, EVStatus aEVStatus, uint16_t aCertificateTransparencyStatus, EVStatus aEVStatus,
@@ -1075,11 +1084,12 @@ void SSLServerCertVerificationResult::Dispatch(
if (!stsTarget) { if (!stsTarget) {
// This has to be released on STS; just leak it // This has to be released on STS; just leak it
Unused << mSocketControl.forget(); Unused << mSocketControl.forget();
return; return NS_ERROR_FAILURE;
} }
rv = stsTarget->Dispatch(this, NS_DISPATCH_NORMAL); rv = stsTarget->Dispatch(this, NS_DISPATCH_NORMAL);
MOZ_ASSERT(NS_SUCCEEDED(rv), MOZ_ASSERT(NS_SUCCEEDED(rv),
"Failed to dispatch SSLServerCertVerificationResult"); "Failed to dispatch SSLServerCertVerificationResult");
return rv;
} }
NS_IMETHODIMP NS_IMETHODIMP

35
security/manager/ssl/SSLServerCertVerification.h
View File

@@ -44,15 +44,15 @@ class BaseSSLServerCertVerificationResult {
public: public:
NS_INLINE_DECL_PURE_VIRTUAL_REFCOUNTING NS_INLINE_DECL_PURE_VIRTUAL_REFCOUNTING
virtual void Dispatch(nsTArray<nsTArray<uint8_t>>&& aBuiltChain, [[nodiscard]] virtual nsresult Dispatch(
nsTArray<nsTArray<uint8_t>>&& aPeerCertChain, nsTArray<nsTArray<uint8_t>>&& aBuiltChain,
uint16_t aCertificateTransparencyStatus, nsTArray<nsTArray<uint8_t>>&& aPeerCertChain,
EVStatus aEVStatus, bool aSucceeded, uint16_t aCertificateTransparencyStatus, EVStatus aEVStatus,
PRErrorCode aFinalError, bool aSucceeded, PRErrorCode aFinalError,
nsITransportSecurityInfo::OverridableErrorCategory nsITransportSecurityInfo::OverridableErrorCategory
aOverridableErrorCategory, aOverridableErrorCategory,
bool aIsBuiltCertChainRootBuiltInRoot, bool aIsBuiltCertChainRootBuiltInRoot, uint32_t aProviderFlags,
uint32_t aProviderFlags, bool aMadeOCSPRequests) = 0; bool aMadeOCSPRequests) = 0;
}; };
// Dispatched to the STS thread to notify the infoObject of the verification // Dispatched to the STS thread to notify the infoObject of the verification
@@ -70,14 +70,15 @@ class SSLServerCertVerificationResult final
explicit SSLServerCertVerificationResult(CommonSocketControl* socketControl); explicit SSLServerCertVerificationResult(CommonSocketControl* socketControl);
void Dispatch(nsTArray<nsTArray<uint8_t>>&& aBuiltChain, [[nodiscard]] nsresult Dispatch(
nsTArray<nsTArray<uint8_t>>&& aPeerCertChain, nsTArray<nsTArray<uint8_t>>&& aBuiltChain,
uint16_t aCertificateTransparencyStatus, EVStatus aEVStatus, nsTArray<nsTArray<uint8_t>>&& aPeerCertChain,
bool aSucceeded, PRErrorCode aFinalError, uint16_t aCertificateTransparencyStatus, EVStatus aEVStatus,
nsITransportSecurityInfo::OverridableErrorCategory bool aSucceeded, PRErrorCode aFinalError,
aOverridableErrorCategory, nsITransportSecurityInfo::OverridableErrorCategory
bool aIsBuiltCertChainRootBuiltInRoot, uint32_t aProviderFlags, aOverridableErrorCategory,
bool aMadeOCSPRequests) override; bool aIsBuiltCertChainRootBuiltInRoot, uint32_t aProviderFlags,
bool aMadeOCSPRequests) override;
private: private:
~SSLServerCertVerificationResult() = default; ~SSLServerCertVerificationResult() = default;

15
security/manager/ssl/VerifySSLServerCertChild.cpp
View File

@@ -46,11 +46,16 @@ ipc::IPCResult VerifySSLServerCertChild::RecvOnVerifySSLServerCertFinished(
certBytesArray.AppendElement(std::move(cert.data())); certBytesArray.AppendElement(std::move(cert.data()));
} }
mResultTask->Dispatch(std::move(certBytesArray), std::move(mPeerCertChain), nsresult rv = mResultTask->Dispatch(
aCertTransparencyStatus, aEVStatus, aSucceeded, std::move(certBytesArray), std::move(mPeerCertChain),
aFinalError, aOverridableErrorCategory, aCertTransparencyStatus, aEVStatus, aSucceeded, aFinalError,
aIsBuiltCertChainRootBuiltInRoot, mProviderFlags, aOverridableErrorCategory, aIsBuiltCertChainRootBuiltInRoot,
aMadeOCSPRequests); mProviderFlags, aMadeOCSPRequests);
if (NS_FAILED(rv)) {
// We can't release this off the STS thread because some parts of it are
// not threadsafe. Just leak mResultTask.
Unused << mResultTask.forget();
}
return IPC_OK(); return IPC_OK();
} }

25
security/manager/ssl/VerifySSLServerCertParent.cpp
View File

@@ -57,14 +57,15 @@ class IPCServerCertVerificationResult final
VerifySSLServerCertParent* aParent) VerifySSLServerCertParent* aParent)
: mTarget(aTarget), mParent(aParent) {} : mTarget(aTarget), mParent(aParent) {}
void Dispatch(nsTArray<nsTArray<uint8_t>>&& aBuiltChain, [[nodiscard]] nsresult Dispatch(
nsTArray<nsTArray<uint8_t>>&& aPeerCertChain, nsTArray<nsTArray<uint8_t>>&& aBuiltChain,
uint16_t aCertificateTransparencyStatus, EVStatus aEVStatus, nsTArray<nsTArray<uint8_t>>&& aPeerCertChain,
bool aSucceeded, PRErrorCode aFinalError, uint16_t aCertificateTransparencyStatus, EVStatus aEVStatus,
nsITransportSecurityInfo::OverridableErrorCategory bool aSucceeded, PRErrorCode aFinalError,
aOverridableErrorCategory, nsITransportSecurityInfo::OverridableErrorCategory
bool aIsBuiltCertChainRootBuiltInRoot, uint32_t aProviderFlags, aOverridableErrorCategory,
bool aMadeOCSPRequests) override; bool aIsBuiltCertChainRootBuiltInRoot, uint32_t aProviderFlags,
bool aMadeOCSPRequests) override;
private: private:
~IPCServerCertVerificationResult() = default; ~IPCServerCertVerificationResult() = default;
@@ -73,7 +74,7 @@ class IPCServerCertVerificationResult final
RefPtr<VerifySSLServerCertParent> mParent; RefPtr<VerifySSLServerCertParent> mParent;
}; };
void IPCServerCertVerificationResult::Dispatch( nsresult IPCServerCertVerificationResult::Dispatch(
nsTArray<nsTArray<uint8_t>>&& aBuiltChain, nsTArray<nsTArray<uint8_t>>&& aBuiltChain,
nsTArray<nsTArray<uint8_t>>&& aPeerCertChain, nsTArray<nsTArray<uint8_t>>&& aPeerCertChain,
uint16_t aCertificateTransparencyStatus, EVStatus aEVStatus, uint16_t aCertificateTransparencyStatus, EVStatus aEVStatus,
@@ -89,7 +90,7 @@ void IPCServerCertVerificationResult::Dispatch(
} }
} }
nsresult nrv = mTarget->Dispatch( nsresult rv = mTarget->Dispatch(
NS_NewRunnableFunction( NS_NewRunnableFunction(
"psm::VerifySSLServerCertParent::OnVerifiedSSLServerCert", "psm::VerifySSLServerCertParent::OnVerifiedSSLServerCert",
[parent(mParent), builtCertChain{std::move(builtCertChain)}, [parent(mParent), builtCertChain{std::move(builtCertChain)},
@@ -102,8 +103,8 @@ void IPCServerCertVerificationResult::Dispatch(
aIsBuiltCertChainRootBuiltInRoot, aMadeOCSPRequests); aIsBuiltCertChainRootBuiltInRoot, aMadeOCSPRequests);
}), }),
NS_DISPATCH_NORMAL); NS_DISPATCH_NORMAL);
MOZ_DIAGNOSTIC_ASSERT(NS_SUCCEEDED(nrv)); MOZ_DIAGNOSTIC_ASSERT(NS_SUCCEEDED(rv));
Unused << nrv; return rv;
} }
} // anonymous namespace } // anonymous namespace