]> git.ipfire.org Git - thirdparty/squid.git/commitdiff
Avoid memory leaks when a certificate validator is used with SslBump
authorChristos Tsantilas <chtsanti@users.sourceforge.net>
Tue, 22 Dec 2015 08:11:59 +0000 (21:11 +1300)
committerAmos Jeffries <squid3@treenet.co.nz>
Tue, 22 Dec 2015 08:11:59 +0000 (21:11 +1300)
When a certificate validator was used, sslCrtvdHandleReplyWrapper delivered
validator response directly to the Ssl::PeerConnector job using job's
Ssl::CertValidationHelper::CVHCB callback. If that synchronous call happened
to be the last job call, then Ssl::PeerConnector::done() would become true
for the job, as it should, but nobody would notice that the PeerConnector
job object should be deleted, and the object would leak.

This fix converts CVHCB into an async job call to avoid direct, unprotected
job calls in this context.

This is a Measurement Factory project.

src/ssl/PeerConnector.cc
src/ssl/PeerConnector.h
src/ssl/cert_validate_message.h
src/ssl/helper.cc
src/ssl/helper.h

index bc88cf6b8745dfa356befc0b88c5ddc29e32767e..df375c4cb0d98871ba2bdf4b1e8cf6ab883aa43c 100644 (file)
@@ -342,7 +342,8 @@ Ssl::PeerConnector::sslFinalized()
             validationRequest.errors = NULL;
         try {
             debugs(83, 5, "Sending SSL certificate for validation to ssl_crtvd.");
-            Ssl::CertValidationHelper::GetInstance()->sslSubmit(validationRequest, sslCrtvdHandleReplyWrapper, this);
+            AsyncCall::Pointer call = asyncCall(83,5, "Ssl::PeerConnector::sslCrtvdHandleReply", Ssl::CertValidationHelper::CbDialer(this, &Ssl::PeerConnector::sslCrtvdHandleReply, NULL));
+            Ssl::CertValidationHelper::GetInstance()->sslSubmit(validationRequest, call);
             return false;
         } catch (const std::exception &e) {
             debugs(83, DBG_IMPORTANT, "ERROR: Failed to compose ssl_crtvd " <<
@@ -465,15 +466,10 @@ Ssl::PeerConnector::checkForPeekAndSpliceGuess() const
 }
 
 void
-Ssl::PeerConnector::sslCrtvdHandleReplyWrapper(void *data, Ssl::CertValidationResponse const &validationResponse)
+Ssl::PeerConnector::sslCrtvdHandleReply(Ssl::CertValidationResponse::Pointer validationResponse)
 {
-    Ssl::PeerConnector *connector = (Ssl::PeerConnector *)(data);
-    connector->sslCrtvdHandleReply(validationResponse);
-}
+    Must(validationResponse != NULL);
 
-void
-Ssl::PeerConnector::sslCrtvdHandleReply(Ssl::CertValidationResponse const &validationResponse)
-{
     Ssl::CertErrors *errs = NULL;
     Ssl::ErrorDetail *errDetails = NULL;
     bool validatorFailed = false;
@@ -481,11 +477,11 @@ Ssl::PeerConnector::sslCrtvdHandleReply(Ssl::CertValidationResponse const &valid
         return;
     }
 
-    debugs(83,5, request->GetHost() << " cert validation result: " << validationResponse.resultCode);
+    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)
+        errs = sslCrtvdCheckForErrors(*validationResponse, errDetails);
+    else if (validationResponse->resultCode != ::Helper::Okay)
         validatorFailed = true;
 
     if (!errDetails && !validatorFailed) {
index 44e95c08d1fc7cd60648ae78bb981b2d2445d814..ffde4265a9e7f3f8014c03c2d3d4cb1a65eac66f 100644 (file)
@@ -12,6 +12,7 @@
 #include "acl/Acl.h"
 #include "base/AsyncCbdataCalls.h"
 #include "base/AsyncJob.h"
+#include "CommCalls.h"
 #include "ssl/support.h"
 #include <iosfwd>
 
@@ -23,6 +24,7 @@ namespace Ssl
 
 class ErrorDetail;
 class CertValidationResponse;
+typedef RefCount<CertValidationResponse> CertValidationResponsePointer;
 
 /// PeerConnector results (supplied via a callback).
 /// The connection to peer was secured if and only if the error member is nil.
@@ -154,7 +156,7 @@ private:
     void callBack();
 
     /// Process response from cert validator helper
-    void sslCrtvdHandleReply(Ssl::CertValidationResponse const &);
+    void sslCrtvdHandleReply(Ssl::CertValidationResponsePointer);
 
     /// Check SSL errors returned from cert validator against sslproxy_cert_error access list
     Ssl::CertErrors *sslCrtvdCheckForErrors(Ssl::CertValidationResponse const &, Ssl::ErrorDetail *&);
@@ -167,9 +169,6 @@ private:
     /// connection manager members
     void serverCertificateVerified();
 
-    /// Callback function called when squid receive message from cert validator helper
-    static void sslCrtvdHandleReplyWrapper(void *data, Ssl::CertValidationResponse const &);
-
     /// A wrapper function for negotiateSsl for use with Comm::SetSelect
     static void NegotiateSsl(int fd, void *data);
 
index 79edea4ab5d2533a6333c35bec0c91e961b20375..8e13e2333d908323524b174dcafae4216fb961dc 100644 (file)
@@ -9,6 +9,7 @@
 #ifndef SQUID_SSL_CERT_VALIDATE_MESSAGE_H
 #define SQUID_SSL_CERT_VALIDATE_MESSAGE_H
 
+#include "base/RefCount.h"
 #include "helper/ResultCode.h"
 #include "ssl/crtd_message.h"
 #include "ssl/support.h"
@@ -35,9 +36,11 @@ public:
  * This class is used to store informations found in certificate validation
  * response messages read from certificate validator helper
  */
-class CertValidationResponse
+class CertValidationResponse: public RefCountable
 {
 public:
+    typedef RefCount<CertValidationResponse> Pointer;
+
     /**
      * This class used to hold error informations returned from
      * cert validator helper.
index 30892d5aa8fce5c3dc6223ec08c332952aee0621..5d2e377c4a671dcbf3a5154b72bf7e4d87261801 100644 (file)
@@ -19,7 +19,7 @@
 #include "SwapDir.h"
 #include "wordlist.h"
 
-LruMap<Ssl::CertValidationResponse> *Ssl::CertValidationHelper::HelperCache = NULL;
+Ssl::CertValidationHelper::LruCache *Ssl::CertValidationHelper::HelperCache = NULL;
 
 #if USE_SSL_CRTD
 Ssl::Helper * Ssl::Helper::GetInstance()
@@ -186,7 +186,7 @@ void Ssl::CertValidationHelper::Init()
 
     //WARNING: initializing static member in an object initialization method
     assert(HelperCache == NULL);
-    HelperCache = new LruMap<Ssl::CertValidationResponse>(ttl, cache);
+    HelperCache = new Ssl::CertValidationHelper::LruCache(ttl, cache);
 }
 
 void Ssl::CertValidationHelper::Shutdown()
@@ -207,8 +207,7 @@ void Ssl::CertValidationHelper::Shutdown()
 
 struct submitData {
     std::string query;
-    Ssl::CertValidationHelper::CVHCB *callback;
-    void *data;
+    AsyncCall::Pointer callback;
     SSL *ssl;
     CBDATA_CLASS2(submitData);
 };
@@ -218,7 +217,7 @@ static void
 sslCrtvdHandleReplyWrapper(void *data, const ::Helper::Reply &reply)
 {
     Ssl::CertValidationMsg replyMsg(Ssl::CrtdMessage::REPLY);
-    Ssl::CertValidationResponse *validationResponse = new Ssl::CertValidationResponse;
+    Ssl::CertValidationResponse::Pointer validationResponse = new Ssl::CertValidationResponse;
     std::string error;
 
     submitData *crtdvdData = static_cast<submitData *>(data);
@@ -234,20 +233,22 @@ sslCrtvdHandleReplyWrapper(void *data, const ::Helper::Reply &reply)
     } else
         validationResponse->resultCode = reply.result;
 
-    crtdvdData->callback(crtdvdData->data, *validationResponse);
+    Ssl::CertValidationHelper::CbDialer *dialer = dynamic_cast<Ssl::CertValidationHelper::CbDialer*>(crtdvdData->callback->getDialer());
+    Must(dialer);
+    dialer->arg1 = validationResponse;
+    ScheduleCallHere(crtdvdData->callback);
 
     if (Ssl::CertValidationHelper::HelperCache &&
             (validationResponse->resultCode == ::Helper::Okay || validationResponse->resultCode == ::Helper::Error)) {
-        Ssl::CertValidationHelper::HelperCache->add(crtdvdData->query.c_str(), validationResponse);
-    } else
-        delete validationResponse;
+        Ssl::CertValidationResponse::Pointer *item = new Ssl::CertValidationResponse::Pointer(validationResponse);
+        Ssl::CertValidationHelper::HelperCache->add(crtdvdData->query.c_str(), item);
+    }
 
-    cbdataReferenceDone(crtdvdData->data);
     SSL_free(crtdvdData->ssl);
     delete crtdvdData;
 }
 
-void Ssl::CertValidationHelper::sslSubmit(Ssl::CertValidationRequest const &request, Ssl::CertValidationHelper::CVHCB * callback, void * data)
+void Ssl::CertValidationHelper::sslSubmit(Ssl::CertValidationRequest const &request, AsyncCall::Pointer &callback)
 {
     static time_t first_warn = 0;
     assert(ssl_crt_validator);
@@ -258,9 +259,11 @@ void Ssl::CertValidationHelper::sslSubmit(Ssl::CertValidationRequest const &requ
         if (squid_curtime - first_warn > 3 * 60)
             fatal("ssl_crtvd queue being overloaded for long time");
         debugs(83, DBG_IMPORTANT, "WARNING: ssl_crtvd queue overload, rejecting");
-        Ssl::CertValidationResponse resp;
-        resp.resultCode = ::Helper::BrokenHelper;
-        callback(data, resp);
+        Ssl::CertValidationResponse::Pointer resp = new Ssl::CertValidationResponse;
+        resp->resultCode = ::Helper::BrokenHelper;
+        Ssl::CertValidationHelper::CbDialer *dialer = dynamic_cast<Ssl::CertValidationHelper::CbDialer*>(callback->getDialer());
+        dialer->arg1 = resp;
+        ScheduleCallHere(callback);
         return;
     }
     first_warn = 0;
@@ -274,15 +277,16 @@ void Ssl::CertValidationHelper::sslSubmit(Ssl::CertValidationRequest const &requ
     crtdvdData->query = message.compose();
     crtdvdData->query += '\n';
     crtdvdData->callback = callback;
-    crtdvdData->data = cbdataReference(data);
     crtdvdData->ssl = request.ssl;
     CRYPTO_add(&crtdvdData->ssl->references,1,CRYPTO_LOCK_SSL);
-    Ssl::CertValidationResponse const*validationResponse;
+    Ssl::CertValidationResponse::Pointer const*validationResponse;
 
     if (CertValidationHelper::HelperCache &&
             (validationResponse = CertValidationHelper::HelperCache->get(crtdvdData->query.c_str()))) {
-        callback(data, *validationResponse);
-        cbdataReferenceDone(crtdvdData->data);
+
+        CertValidationHelper::CbDialer *dialer = dynamic_cast<CertValidationHelper::CbDialer*>(callback->getDialer());
+        dialer->arg1 = *validationResponse;
+        ScheduleCallHere(callback);
         SSL_free(crtdvdData->ssl);
         delete crtdvdData;
         return;
index 69cb15c1998f205bef812ba20c1c0199d9e3f2d0..140f2e78b98cd445fb18dbca7f8a0610c2ec81a5 100644 (file)
@@ -9,6 +9,7 @@
 #ifndef SQUID_SSL_HELPER_H
 #define SQUID_SSL_HELPER_H
 
+#include "base/AsyncJobCalls.h"
 #include "base/LruMap.h"
 #include "helper/forward.h"
 #include "ssl/cert_validate_message.h"
@@ -38,24 +39,28 @@ private:
 };
 #endif
 
+class PeerConnector;
 class CertValidationRequest;
 class CertValidationResponse;
 class CertValidationHelper
 {
 public:
+    typedef UnaryMemFunT<Ssl::PeerConnector, CertValidationResponse::Pointer> CbDialer;
+
     typedef void CVHCB(void *, Ssl::CertValidationResponse const &);
     static CertValidationHelper * GetInstance(); ///< Instance class.
     void Init(); ///< Init helper structure.
     void Shutdown(); ///< Shutdown helper structure.
     /// Submit crtd request message to external crtd server.
-    void sslSubmit(Ssl::CertValidationRequest const & request, CVHCB * callback, void *data);
+    void sslSubmit(Ssl::CertValidationRequest const & request, AsyncCall::Pointer &);
 private:
     CertValidationHelper();
     ~CertValidationHelper();
 
     helper * ssl_crt_validator; ///< helper for management of ssl_crtd.
 public:
-    static LruMap<Ssl::CertValidationResponse> *HelperCache; ///< cache for cert validation helper
+    typedef LruMap<Ssl::CertValidationResponse::Pointer, sizeof(Ssl::CertValidationResponse::Pointer) + sizeof(Ssl::CertValidationResponse)> LruCache;
+    static LruCache *HelperCache; ///< cache for cert validation helper
 };
 
 } //namespace Ssl