------------------------------------------------------------ revno: 13984 revision-id: squid3@treenet.co.nz-20160215112950-hpecdg920samwy8j parent: squid3@treenet.co.nz-20160215071414-pqrp5ww5y433z1ek author: Christos Tsantilas committer: Amos Jeffries branch nick: 3.5 timestamp: Tue 2016-02-16 00:29:50 +1300 message: 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 ------------------------------------------------------------ # Bazaar merge directive format 2 (Bazaar 0.90) # revision_id: squid3@treenet.co.nz-20160215112950-hpecdg920samwy8j # target_branch: http://bzr.squid-cache.org/bzr/squid3/3.5 # testament_sha1: d7f8c558e01b5197ff7a2689aeef7cef13a14a77 # timestamp: 2016-02-15 11:31:32 +0000 # source_branch: http://bzr.squid-cache.org/bzr/squid3/3.5 # base_revision_id: squid3@treenet.co.nz-20160215071414-\ # pqrp5ww5y433z1ek # # Begin patch === modified file 'src/acl/FilledChecklist.h' --- src/acl/FilledChecklist.h 2016-01-01 00:14:27 +0000 +++ src/acl/FilledChecklist.h 2016-02-15 11:29:50 +0000 @@ -79,7 +79,7 @@ #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 === modified file 'src/client_side.cc' --- src/client_side.cc 2016-01-01 00:14:27 +0000 +++ src/client_side.cc 2016-02-15 11:29:50 +0000 @@ -4032,7 +4032,7 @@ 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. === modified file 'src/ssl/PeerConnector.cc' --- src/ssl/PeerConnector.cc 2016-01-01 00:14:27 +0000 +++ src/ssl/PeerConnector.cc 2016-02-15 11:29:50 +0000 @@ -199,6 +199,9 @@ 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 @@ 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 @@ { Must(validationResponse != NULL); - Ssl::CertErrors *errs = NULL; Ssl::ErrorDetail *errDetails = NULL; bool validatorFailed = false; if (!Comm::IsConnOpen(serverConnection())) { @@ -479,9 +473,14 @@ 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 @@ 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 @@ 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). === modified file 'src/ssl/ServerBump.cc' --- src/ssl/ServerBump.cc 2016-01-01 00:14:27 +0000 +++ src/ssl/ServerBump.cc 2016-02-15 11:29:50 +0000 @@ -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 @@ 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 @@ 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; +} === modified file 'src/ssl/ServerBump.h' --- src/ssl/ServerBump.h 2016-01-01 00:14:27 +0000 +++ src/ssl/ServerBump.h 2016-02-15 11:29:50 +0000 @@ -30,12 +30,15 @@ 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 @@ } 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 === modified file 'src/ssl/gadgets.h' --- src/ssl/gadgets.h 2016-01-01 00:14:27 +0000 +++ src/ssl/gadgets.h 2016-02-15 11:29:50 +0000 @@ -113,7 +113,7 @@ 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;