From: Christos Tsantilas Date: Mon, 15 Feb 2016 11:29:50 +0000 (+1300) Subject: Cert Validation memory leaks X-Git-Tag: SQUID_3_5_14~5 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=6cc1de1106f531979776ae857d06028eb370c26b;p=thirdparty%2Fsquid.git Cert Validation memory leaks In the case SSL errors detected by certificate validator helper the objects stored in Ssl::ServerBump::sslErrors member and will never released. This member normally points to an Ssl::CertErrors list attached to the related SSL object which is responsible to release this list. When the cert validator detects errors a new errors list created and attached to the related Ssl::ServerBump::sslErrors member but the SSL objects still hold the old one. The old list released but not the new one. This patch also fixes the case the cbdata protected Ssl::CertErrors list, still is used through the related Ssl::ServerBump object but it is not valid any more, because the SSL object which hold it gone. This patch instead of storing the Ssl::CertErrors list to Ssl::ServerBump object stores the SSL object and increases its reference to avoid be released This is a Measurement Factory project --- diff --git a/src/acl/FilledChecklist.h b/src/acl/FilledChecklist.h index 3a46c70b27..1d3f09378c 100644 --- a/src/acl/FilledChecklist.h +++ b/src/acl/FilledChecklist.h @@ -79,7 +79,7 @@ public: #if USE_OPENSSL /// SSL [certificate validation] errors, in undefined order - Ssl::CertErrors *sslErrors; + const Ssl::CertErrors *sslErrors; /// The peer certificate Ssl::X509_Pointer serverCert; #endif diff --git a/src/client_side.cc b/src/client_side.cc index d363fbe495..3412f42dbd 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -4032,7 +4032,7 @@ void ConnStateData::buildSslCertGenerationParams(Ssl::CertificateProperties &cer ACLFilledChecklist checklist(NULL, sslServerBump->request.getRaw(), clientConnection != NULL ? clientConnection->rfc931 : dash_str); - checklist.sslErrors = cbdataReference(sslServerBump->sslErrors); + checklist.sslErrors = cbdataReference(sslServerBump->sslErrors()); for (sslproxy_cert_adapt *ca = Config.ssl_client.cert_adapt; ca != NULL; ca = ca->next) { // If the algorithm already set, then ignore it. diff --git a/src/ssl/PeerConnector.cc b/src/ssl/PeerConnector.cc index 1fde91ce40..377a9fc708 100644 --- a/src/ssl/PeerConnector.cc +++ b/src/ssl/PeerConnector.cc @@ -199,6 +199,9 @@ Ssl::PeerConnector::initializeSsl() if (sniServer) Ssl::setClientSNI(ssl, sniServer); } + + if (Ssl::ServerBump *serverBump = csd->serverBump()) + serverBump->attachServerSSL(ssl); } // If CertValidation Helper used do not lookup checklist for errors, @@ -319,14 +322,6 @@ Ssl::PeerConnector::sslFinalized() handleServerCertificate(); - if (ConnStateData *csd = request->clientConnectionManager.valid()) { - if (Ssl::ServerBump *serverBump = csd->serverBump()) { - // remember validation errors, if any - if (Ssl::CertErrors *errs = static_cast(SSL_get_ex_data(ssl, ssl_ex_index_ssl_errors))) - serverBump->sslErrors = cbdataReference(errs); - } - } - if (Ssl::TheConfig.ssl_crt_validator) { Ssl::CertValidationRequest validationRequest; // WARNING: Currently we do not use any locking for any of the @@ -470,7 +465,6 @@ Ssl::PeerConnector::sslCrtvdHandleReply(Ssl::CertValidationResponse::Pointer val { Must(validationResponse != NULL); - Ssl::CertErrors *errs = NULL; Ssl::ErrorDetail *errDetails = NULL; bool validatorFailed = false; if (!Comm::IsConnOpen(serverConnection())) { @@ -479,9 +473,14 @@ Ssl::PeerConnector::sslCrtvdHandleReply(Ssl::CertValidationResponse::Pointer val debugs(83,5, request->GetHost() << " cert validation result: " << validationResponse->resultCode); - if (validationResponse->resultCode == ::Helper::Error) - errs = sslCrtvdCheckForErrors(*validationResponse, errDetails); - else if (validationResponse->resultCode != ::Helper::Okay) + if (validationResponse->resultCode == ::Helper::Error) { + if (Ssl::CertErrors *errs = sslCrtvdCheckForErrors(*validationResponse, errDetails)) { + SSL *ssl = fd_table[serverConnection()->fd].ssl; + Ssl::CertErrors *oldErrs = static_cast(SSL_get_ex_data(ssl, ssl_ex_index_ssl_errors)); + SSL_set_ex_data(ssl, ssl_ex_index_ssl_errors, (void *)errs); + delete oldErrs; + } + } else if (validationResponse->resultCode != ::Helper::Okay) validatorFailed = true; if (!errDetails && !validatorFailed) { @@ -497,20 +496,6 @@ Ssl::PeerConnector::sslCrtvdHandleReply(Ssl::CertValidationResponse::Pointer val if (validatorFailed) { anErr = new ErrorState(ERR_GATEWAY_FAILURE, Http::scInternalServerError, request.getRaw()); } else { - - // Check the list error with - if (errDetails && request->clientConnectionManager.valid()) { - // remember the server certificate from the ErrorDetail object - if (Ssl::ServerBump *serverBump = request->clientConnectionManager->serverBump()) { - // remember validation errors, if any - if (errs) { - if (serverBump->sslErrors) - cbdataReferenceDone(serverBump->sslErrors); - serverBump->sslErrors = cbdataReference(errs); - } - } - } - anErr = new ErrorState(ERR_SECURE_CONNECT_FAIL, Http::scServiceUnavailable, request.getRaw()); anErr->detail = errDetails; /*anErr->xerrno= Should preserved*/ @@ -693,14 +678,9 @@ Ssl::PeerConnector::handleNegotiateError(const int ret) if (request->clientConnectionManager.valid()) { // remember the server certificate from the ErrorDetail object - if (Ssl::ServerBump *serverBump = request->clientConnectionManager->serverBump()) { + if (Ssl::ServerBump *serverBump = request->clientConnectionManager->serverBump()) serverBump->serverCert.resetAndLock(anErr->detail->peerCert()); - // remember validation errors, if any - if (Ssl::CertErrors *errs = static_cast(SSL_get_ex_data(ssl, ssl_ex_index_ssl_errors))) - serverBump->sslErrors = cbdataReference(errs); - } - // For intercepted connections, set the host name to the server // certificate CN. Otherwise, we just hope that CONNECT is using // a user-entered address (a host name or a user-entered IP). diff --git a/src/ssl/ServerBump.cc b/src/ssl/ServerBump.cc index b7ab5870ee..e0ae44d4a2 100644 --- a/src/ssl/ServerBump.cc +++ b/src/ssl/ServerBump.cc @@ -11,6 +11,7 @@ #include "squid.h" #include "client_side.h" +#include "globals.h" #include "FwdState.h" #include "ssl/ServerBump.h" #include "Store.h" @@ -21,7 +22,6 @@ CBDATA_NAMESPACED_CLASS_INIT(Ssl, ServerBump); Ssl::ServerBump::ServerBump(HttpRequest *fakeRequest, StoreEntry *e, Ssl::BumpMode md): request(fakeRequest), - sslErrors(NULL), step(bumpStep1) { debugs(33, 4, HERE << "will peek at " << request->GetHost() << ':' << request->port); @@ -47,6 +47,23 @@ Ssl::ServerBump::~ServerBump() storeUnregister(sc, entry, this); entry->unlock("Ssl::ServerBump"); } - cbdataReferenceDone(sslErrors); } +void +Ssl::ServerBump::attachServerSSL(SSL *ssl) +{ + if (serverSSL.get()) + return; + + serverSSL.resetAndLock(ssl); +} + +const Ssl::CertErrors * +Ssl::ServerBump::sslErrors() const +{ + if (!serverSSL.get()) + return NULL; + + const Ssl::CertErrors *errs = static_cast(SSL_get_ex_data(serverSSL.get(), ssl_ex_index_ssl_errors)); + return errs; +} diff --git a/src/ssl/ServerBump.h b/src/ssl/ServerBump.h index 737663df95..68dc9963df 100644 --- a/src/ssl/ServerBump.h +++ b/src/ssl/ServerBump.h @@ -30,12 +30,15 @@ class ServerBump public: explicit ServerBump(HttpRequest *fakeRequest, StoreEntry *e = NULL, Ssl::BumpMode mode = Ssl::bumpServerFirst); ~ServerBump(); + void attachServerSSL(SSL *); ///< Sets the server SSL object + const Ssl::CertErrors *sslErrors() const; ///< SSL [certificate validation] errors /// faked, minimal request; required by Client API HttpRequest::Pointer request; StoreEntry *entry; ///< for receiving Squid-generated error messages - Ssl::X509_Pointer serverCert; ///< HTTPS server certificate - Ssl::CertErrors *sslErrors; ///< SSL [certificate validation] errors + /// HTTPS server certificate. Maybe it is different than the one + /// it is stored in serverSSL object (error SQUID_X509_V_ERR_CERT_CHANGE) + Ssl::X509_Pointer serverCert; struct { Ssl::BumpMode step1; ///< The SSL bump mode at step1 Ssl::BumpMode step2; ///< The SSL bump mode at step2 @@ -43,6 +46,7 @@ public: } act; ///< bumping actions at various bumping steps Ssl::BumpStep step; ///< The SSL bumping step SBuf clientSni; ///< the SSL client SNI name + Ssl::SSL_Pointer serverSSL; ///< The SSL object on server side. private: store_client *sc; ///< dummy client to prevent entry trimming diff --git a/src/ssl/gadgets.h b/src/ssl/gadgets.h index f01990698c..6ab027d4dd 100644 --- a/src/ssl/gadgets.h +++ b/src/ssl/gadgets.h @@ -113,7 +113,7 @@ CtoCpp1(SSL_CTX_free, SSL_CTX *) typedef TidyPointer SSL_CTX_Pointer; CtoCpp1(SSL_free, SSL *) -typedef TidyPointer SSL_Pointer; +typedef LockingPointer SSL_Pointer; CtoCpp1(DH_free, DH *); typedef TidyPointer DH_Pointer;