From: Christos Tsantilas Date: Sat, 13 Feb 2016 07:51:20 +0000 (+0200) Subject: Cert Validation memory leaks X-Git-Tag: SQUID_4_0_6~6 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=088f0761d7e31cdb6e1c15fbc328e7790468d05e;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 8b8156b417..b7b0c99705 100644 --- a/src/acl/FilledChecklist.h +++ b/src/acl/FilledChecklist.h @@ -85,7 +85,7 @@ public: #if USE_OPENSSL /// SSL [certificate validation] errors, in undefined order - Ssl::CertErrors *sslErrors; + const Ssl::CertErrors *sslErrors; /// The peer certificate Security::CertPointer serverCert; #endif diff --git a/src/client_side.cc b/src/client_side.cc index cc1aef0638..e2c0f6082d 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -2899,7 +2899,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/format/Format.cc b/src/format/Format.cc index 80bd029e2f..bfcc133fb0 100644 --- a/src/format/Format.cc +++ b/src/format/Format.cc @@ -1251,7 +1251,7 @@ Format::Format::assemble(MemBuf &mb, const AccessLogEntry::Pointer &al, int logS if (al->request && al->request->clientConnectionManager.valid()) { if (Ssl::ServerBump * srvBump = al->request->clientConnectionManager->serverBump()) { const char *separator = fmt->data.string ? fmt->data.string : ":"; - for (Ssl::CertErrors *sslError = srvBump->sslErrors; sslError != NULL; sslError = sslError->next) { + for (Ssl::CertErrors const *sslError = srvBump->sslErrors(); sslError != NULL; sslError = sslError->next) { if (sb.size()) sb.append(separator); if (const char *errorName = Ssl::GetErrorName(sslError->element.code)) diff --git a/src/security/Session.h b/src/security/Session.h index f095fbac67..d01fb70a14 100644 --- a/src/security/Session.h +++ b/src/security/Session.h @@ -24,25 +24,20 @@ #endif #endif -/* - * NOTE: we use TidyPointer for sessions. OpenSSL provides explicit reference - * locking mechanisms, but GnuTLS only provides init/deinit. To ensure matching - * behaviour we cannot use LockingPointer (yet) and must ensure that there is - * no possibility of double-free being used on the raw pointers. That is - * currently done by using a TidyPointer in the global fde table so its - * lifetime matched the connection. - */ - namespace Security { #if USE_OPENSSL typedef SSL* SessionPtr; CtoCpp1(SSL_free, SSL *); -typedef TidyPointer SessionPointer; +typedef LockingPointer SessionPointer; #elif USE_GNUTLS typedef gnutls_session_t SessionPtr; CtoCpp1(gnutls_deinit, gnutls_session_t); +// TODO: Convert to Locking pointer. +// Locks can be implemented attaching locks counter to gnutls_session_t +// objects using the gnutls_session_set_ptr()/gnutls_session_get_ptr () +// library functions typedef TidyPointer SessionPointer; #else diff --git a/src/ssl/PeekingPeerConnector.cc b/src/ssl/PeekingPeerConnector.cc index 18773407cb..88ae4ed4d4 100644 --- a/src/ssl/PeekingPeerConnector.cc +++ b/src/ssl/PeekingPeerConnector.cc @@ -205,12 +205,13 @@ Ssl::PeekingPeerConnector::initializeSsl() Ssl::setClientSNI(ssl, sniServer); } - // store peeked cert to check SQUID_X509_V_ERR_CERT_CHANGE - X509 *peeked_cert; - if (csd->serverBump() && - (peeked_cert = csd->serverBump()->serverCert.get())) { - CRYPTO_add(&(peeked_cert->references),1,CRYPTO_LOCK_X509); - SSL_set_ex_data(ssl, ssl_ex_index_ssl_peeked_cert, peeked_cert); + if (Ssl::ServerBump *serverBump = csd->serverBump()) { + serverBump->attachServerSSL(ssl); + // store peeked cert to check SQUID_X509_V_ERR_CERT_CHANGE + if (X509 *peeked_cert = serverBump->serverCert.get()) { + CRYPTO_add(&(peeked_cert->references),1,CRYPTO_LOCK_X509); + SSL_set_ex_data(ssl, ssl_ex_index_ssl_peeked_cert, peeked_cert); + } } } @@ -228,13 +229,6 @@ Ssl::PeekingPeerConnector::noteNegotiationDone(ErrorState *error) // remember the server certificate from the ErrorDetail object if (Ssl::ServerBump *serverBump = request->clientConnectionManager->serverBump()) { - // remember validation errors, if any - if (certErrors) { - if (serverBump->sslErrors) - cbdataReferenceDone(serverBump->sslErrors); - serverBump->sslErrors = cbdataReference(certErrors); - } - if (!serverBump->serverCert.get()) { // remember the server certificate from the ErrorDetail object if (error && error->detail && error->detail->peerCert()) diff --git a/src/ssl/PeerConnector.cc b/src/ssl/PeerConnector.cc index 94de1d1604..695abc8bd8 100644 --- a/src/ssl/PeerConnector.cc +++ b/src/ssl/PeerConnector.cc @@ -25,7 +25,6 @@ CBDATA_NAMESPACED_CLASS_INIT(Ssl, PeerConnector); Ssl::PeerConnector::PeerConnector(const Comm::ConnectionPointer &aServerConn, AsyncCall::Pointer &aCallback, const AccessLogEntryPointer &alp, const time_t timeout) : AsyncJob("Ssl::PeerConnector"), serverConn(aServerConn), - certErrors(NULL), al(alp), callback(aCallback), negotiationTimeout(timeout), @@ -38,7 +37,6 @@ Ssl::PeerConnector::PeerConnector(const Comm::ConnectionPointer &aServerConn, As Ssl::PeerConnector::~PeerConnector() { - cbdataReferenceDone(certErrors); debugs(83, 5, "Peer connector " << this << " gone"); } @@ -203,7 +201,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())) { @@ -212,9 +209,14 @@ Ssl::PeerConnector::sslCrtvdHandleReply(Ssl::CertValidationResponse::Pointer val debugs(83,5, request->url.host() << " 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)) { + Security::SessionPtr ssl = fd_table[serverConnection()->fd].ssl.get(); + 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) { @@ -223,12 +225,6 @@ Ssl::PeerConnector::sslCrtvdHandleReply(Ssl::CertValidationResponse::Pointer val return; } - if (errs) { - if (certErrors) - cbdataReferenceDone(certErrors); - certErrors = cbdataReference(errs); - } - ErrorState *anErr = NULL; if (validatorFailed) { anErr = new ErrorState(ERR_GATEWAY_FAILURE, Http::scInternalServerError, request.getRaw()); @@ -397,11 +393,6 @@ Ssl::PeerConnector::noteSslNegotiationError(const int ret, const int ssl_error, if (ssl_lib_error != SSL_ERROR_NONE) anErr->detail->setLibError(ssl_lib_error); - assert(certErrors == NULL); - // remember validation errors, if any - if (Ssl::CertErrors *errs = static_cast(SSL_get_ex_data(ssl, ssl_ex_index_ssl_errors))) - certErrors = cbdataReference(errs); - noteNegotiationDone(anErr); bail(anErr); } diff --git a/src/ssl/PeerConnector.h b/src/ssl/PeerConnector.h index fc50f57b7f..7a8f481d71 100644 --- a/src/ssl/PeerConnector.h +++ b/src/ssl/PeerConnector.h @@ -157,9 +157,6 @@ protected: HttpRequestPointer request; ///< peer connection trigger or cause Comm::ConnectionPointer serverConn; ///< TCP connection to the peer - /// Certificate errors found from SSL validation procedure or from cert - /// validator - Ssl::CertErrors *certErrors; AccessLogEntryPointer al; ///< info for the future access.log entry AsyncCall::Pointer callback; ///< we call this with the results private: diff --git a/src/ssl/ServerBump.cc b/src/ssl/ServerBump.cc index 9230b88702..75dcd3846c 100644 --- a/src/ssl/ServerBump.cc +++ b/src/ssl/ServerBump.cc @@ -22,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, "will peek at " << request->url.authority(true)); @@ -50,6 +49,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 6d518cf133..9f22c3b6fd 100644 --- a/src/ssl/ServerBump.h +++ b/src/ssl/ServerBump.h @@ -32,12 +32,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 - Security::CertPointer 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) + Security::CertPointer serverCert; struct { Ssl::BumpMode step1; ///< The SSL bump mode at step1 Ssl::BumpMode step2; ///< The SSL bump mode at step2 @@ -45,6 +48,7 @@ public: } act; ///< bumping actions at various bumping steps Ssl::BumpStep step; ///< The SSL bumping step SBuf clientSni; ///< the SSL client SNI name + Security::SessionPointer serverSSL; ///< The SSL object on server side. private: store_client *sc; ///< dummy client to prevent entry trimming