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 3a46c70b270c3ce042d74ffbcb7f0b86e790176b..1d3f09378c2066b39b346e075f63f9393636925c 100644 (file)
--- 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 d363fbe4954599e3e67436ec4bebd240e03bec74..3412f42dbd40a0e63440c846cb838f7ab319819f 100644 (file)
--- 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 1fde91ce4095ff01d58551d8899b1e4695a0b56d..377a9fc708df7a23b36040b8886fd65bd5e19060 100644 (file)
--- 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::CertErrors *>(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::CertErrors*>(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::CertErrors*>(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 b7ab5870eee073f9933477a578f2af8242e222fa..e0ae44d4a2122a72d2f8979f53a3f29423df2408 100644 (file)
--- 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<const Ssl::CertErrors*>(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 737663df957160660bd41057f06a8150ef9e537a..68dc9963dfb52dd2a7d76ef7d3dc37f2227bb6b2 100644 (file)
--- 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 f01990698c92e69d886bec50197303261858eac6..6ab027d4dd2aa0c3c76ce2821964664e55a2581e 100644 (file)
--- a/src/ssl/gadgets.h
+++ b/src/ssl/gadgets.h
@@ -113,7 +113,7 @@ CtoCpp1(SSL_CTX_free, SSL_CTX *)
 typedef TidyPointer<SSL_CTX, SSL_CTX_free_cpp> SSL_CTX_Pointer;
 
 CtoCpp1(SSL_free, SSL *)
-typedef TidyPointer<SSL, SSL_free_cpp> SSL_Pointer;
+typedef LockingPointer<SSL, SSL_free_cpp, CRYPTO_LOCK_SSL> SSL_Pointer;
 
 CtoCpp1(DH_free, DH *);
 typedef TidyPointer<DH, DH_free_cpp> DH_Pointer;