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 8b8156b41720dc7e7c87aea70c9cb4ae2b3c7a60..b7b0c99705e8841622900a955aa50f0c6d62fb9c 100644 (file)
--- 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 cc1aef0638a2733a37a828f8867c8e46cf0702b9..e2c0f6082d1d3ba1f2aac5e29de562553aa12f9c 100644 (file)
--- 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 80bd029e2f7a3d50eba5e649d8d9af9adf4d0384..bfcc133fb07399e39c65efc11e15c7a9364273f8 100644 (file)
--- 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 f095fbac67b0357ba712002d32a41d14a3dfd4a4..d01fb70a148ce380043609f4255499681dabbc12 100644 (file)
--- 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<SSL, Security::SSL_free_cpp> SessionPointer;
+typedef LockingPointer<SSL, Security::SSL_free_cpp, CRYPTO_LOCK_SSL> 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<struct gnutls_session_int, Security::gnutls_deinit_cpp> SessionPointer;
 
 #else

diff --git a/src/ssl/PeekingPeerConnector.cc b/src/ssl/PeekingPeerConnector.cc
index 18773407cba8f8450f79ed37c7a45670f3bf0806..88ae4ed4d407b6783f945c6dd3f3c38faf16165e 100644 (file)
--- 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 94de1d16048311ae293c2a06d2450c84794c7e7c..695abc8bd83af55f3a4474494bc3496664d1cb8f 100644 (file)
--- 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::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) {
@@ -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::CertErrors*>(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 fc50f57b7fd6bdea2af6cc82042f5399e215e407..7a8f481d715289996d148fb62fcb935c4d974a55 100644 (file)
--- 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 9230b88702624837f4425811beb1cfa90c7c5f89..75dcd3846cb1e9ffe1e1475864c0c97de5c6cf4e 100644 (file)
--- 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<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 6d518cf133f136ecbaf2bf9ef3582e96ac04f472..9f22c3b6fd632684cd3e381b08d82314dd50068f 100644 (file)
--- 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