From: Alex Rousskov Date: Wed, 10 May 2023 20:45:27 +0000 (+0000) Subject: Do not leak Security::CertErrors created in X509_verify_cert() (#1346) X-Git-Tag: SQUID_7_0_1~444 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=27a1c6de519dc8a5af0cb0ad080ee4b962a03d3f;p=thirdparty%2Fsquid.git Do not leak Security::CertErrors created in X509_verify_cert() (#1346) ACLFilledChecklist was using a raw C pointer for handling cbdata-managed Security::CertErrors. Some sslErrors users knew about hidden cbdata requirements, some did not, resulting in inconsistent locking/unlocking and associated memory leaks. Upgrading ACLFilledChecklist::sslErrors to smart CbcPointer fixes those leaks (and simplifies code). --- diff --git a/src/acl/FilledChecklist.cc b/src/acl/FilledChecklist.cc index edd603b778..285540162a 100644 --- a/src/acl/FilledChecklist.cc +++ b/src/acl/FilledChecklist.cc @@ -33,9 +33,6 @@ ACLFilledChecklist::ACLFilledChecklist() : #endif #if SQUID_SNMP snmp_community(nullptr), -#endif -#if USE_OPENSSL - sslErrors(nullptr), #endif requestErrorType(ERR_MAX), conn_(nullptr), @@ -61,10 +58,6 @@ ACLFilledChecklist::~ACLFilledChecklist() cbdataReferenceDone(conn_); -#if USE_OPENSSL - cbdataReferenceDone(sslErrors); -#endif - debugs(28, 4, "ACLFilledChecklist destroyed " << this); } @@ -227,9 +220,6 @@ ACLFilledChecklist::ACLFilledChecklist(const acl_access *A, HttpRequest *http_re #endif #if SQUID_SNMP snmp_community(nullptr), -#endif -#if USE_OPENSSL - sslErrors(nullptr), #endif requestErrorType(ERR_MAX), conn_(nullptr), diff --git a/src/acl/FilledChecklist.h b/src/acl/FilledChecklist.h index c600ebba48..0e71be360a 100644 --- a/src/acl/FilledChecklist.h +++ b/src/acl/FilledChecklist.h @@ -87,11 +87,12 @@ public: char *snmp_community; #endif + // TODO: RefCount errors; do not ignore them because their "owner" is gone! /// TLS server [certificate validation] errors, in undefined order. /// The errors are accumulated as Squid goes through validation steps /// and server certificates. They are cleared on connection retries. /// For sslproxy_cert_error checks, contains just the current/last error. - const Security::CertErrors *sslErrors; + CbcPointer sslErrors; /// Peer certificate being checked by ssl_verify_cb() and by /// Security::PeerConnector class. In other contexts, the peer diff --git a/src/acl/SslError.cc b/src/acl/SslError.cc index f06f24df27..4bea9f7349 100644 --- a/src/acl/SslError.cc +++ b/src/acl/SslError.cc @@ -14,6 +14,6 @@ int ACLSslErrorStrategy::match (ACLData * &data, ACLFilledChecklist *checklist) { - return data->match (checklist->sslErrors); + return data->match(checklist->sslErrors.get()); } diff --git a/src/client_side.cc b/src/client_side.cc index 00d49faab9..3d6f0a5727 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -1524,11 +1524,10 @@ bool ConnStateData::serveDelayedError(Http::Stream *context) bool allowDomainMismatch = false; if (Config.ssl_client.cert_error) { ACLFilledChecklist check(Config.ssl_client.cert_error, nullptr); - check.sslErrors = new Security::CertErrors(Security::CertError(SQUID_X509_V_ERR_DOMAIN_MISMATCH, srvCert)); + const auto sslErrors = std::make_unique(Security::CertError(SQUID_X509_V_ERR_DOMAIN_MISMATCH, srvCert)); + check.sslErrors = sslErrors.get(); clientAclChecklistFill(check, http); allowDomainMismatch = check.fastCheck().allowed(); - delete check.sslErrors; - check.sslErrors = nullptr; } if (!allowDomainMismatch) { @@ -3586,7 +3585,7 @@ ConnStateData::fillConnectionLevelDetails(ACLFilledChecklist &checklist) const #if USE_OPENSSL if (!checklist.sslErrors && sslServerBump) - checklist.sslErrors = cbdataReference(sslServerBump->sslErrors()); + checklist.sslErrors = sslServerBump->sslErrors(); #endif if (!checklist.rfc931[0]) // checklist creator may have supplied it already diff --git a/src/security/PeerConnector.cc b/src/security/PeerConnector.cc index b6db325b1a..f12229403e 100644 --- a/src/security/PeerConnector.cc +++ b/src/security/PeerConnector.cc @@ -402,9 +402,11 @@ Security::PeerConnector::sslCrtvdCheckForErrors(Ssl::CertValidationResponse cons if (!errDetails) { bool allowed = false; if (check) { - check->sslErrors = new Security::CertErrors(Security::CertError(i->error_no, i->cert, i->error_depth)); + const auto sslErrors = std::make_unique(Security::CertError(i->error_no, i->cert, i->error_depth)); + check->sslErrors = sslErrors.get(); if (check->fastCheck().allowed()) allowed = true; + check->sslErrors.clear(); } // else the Config.ssl_client.cert_error access list is not defined // and the first error will cause the error page @@ -418,10 +420,6 @@ Security::PeerConnector::sslCrtvdCheckForErrors(Ssl::CertValidationResponse cons const char *aReason = i->error_reason.empty() ? nullptr : i->error_reason.c_str(); errDetails = new ErrorDetail(i->error_no, peerCert, brokenCert, aReason); } - if (check) { - delete check->sslErrors; - check->sslErrors = nullptr; - } } if (!errs) diff --git a/src/ssl/ServerBump.cc b/src/ssl/ServerBump.cc index 76715da3f3..ffa85b7efc 100644 --- a/src/ssl/ServerBump.cc +++ b/src/ssl/ServerBump.cc @@ -62,13 +62,12 @@ Ssl::ServerBump::attachServerSession(const Security::SessionPointer &s) serverSession = s; } -const Security::CertErrors * +Security::CertErrors * Ssl::ServerBump::sslErrors() const { if (!serverSession) return nullptr; - const Security::CertErrors *errs = static_cast(SSL_get_ex_data(serverSession.get(), ssl_ex_index_ssl_errors)); - return errs; + return static_cast(SSL_get_ex_data(serverSession.get(), ssl_ex_index_ssl_errors)); } diff --git a/src/ssl/ServerBump.h b/src/ssl/ServerBump.h index f60983ec95..c8af4d0208 100644 --- a/src/ssl/ServerBump.h +++ b/src/ssl/ServerBump.h @@ -38,7 +38,7 @@ public: explicit ServerBump(ClientHttpRequest *http, StoreEntry *e = nullptr, Ssl::BumpMode mode = Ssl::bumpServerFirst); ~ServerBump(); void attachServerSession(const Security::SessionPointer &); ///< Sets the server TLS session object - const Security::CertErrors *sslErrors() const; ///< SSL [certificate validation] errors + Security::CertErrors *sslErrors() const; ///< SSL [certificate validation] errors /// whether there was a successful connection to (and peeking at) the origin server bool connectedOk() const {return entry && entry->isEmpty();} diff --git a/src/ssl/support.cc b/src/ssl/support.cc index ce7896ead6..28e88136d8 100644 --- a/src/ssl/support.cc +++ b/src/ssl/support.cc @@ -351,7 +351,8 @@ ssl_verify_cb(int ok, X509_STORE_CTX * ctx) if (check) { ACLFilledChecklist *filledCheck = Filled(check); const auto savedErrors = filledCheck->sslErrors; - filledCheck->sslErrors = new Security::CertErrors(Security::CertError(error_no, broken_cert)); + const auto sslErrors = std::make_unique(Security::CertError(error_no, broken_cert)); + filledCheck->sslErrors = sslErrors.get(); filledCheck->serverCert = peer_cert; if (check->fastCheck().allowed()) { debugs(83, 3, "bypassing SSL error " << error_no << " in " << *peer_cert); @@ -359,7 +360,6 @@ ssl_verify_cb(int ok, X509_STORE_CTX * ctx) } else { debugs(83, 5, "confirming SSL error " << error_no); } - delete filledCheck->sslErrors; filledCheck->sslErrors = savedErrors; filledCheck->serverCert.reset(); }