On 12/13/2015 10:11 AM, Amos Jeffries wrote:
On 11/12/2015 4:09 a.m., Christos Tsantilas wrote:
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.
+1.
The patch applied to trunk, the fixed bug is enough serious.
I am attaching the squid-3.5 patch.
Amos
Avoid memory leaks when a certificate validator is used with SslBump
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.
=== modified file 'src/ssl/PeerConnector.cc'
--- src/ssl/PeerConnector.cc 2015-09-29 10:50:52 +0000
+++ src/ssl/PeerConnector.cc 2015-11-30 16:30:14 +0000
@@ -325,41 +325,42 @@
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
// members of the Ssl::CertValidationRequest class. In this code the
// Ssl::CertValidationRequest object used only to pass data to
// Ssl::CertValidationHelper::submit method.
validationRequest.ssl = ssl;
validationRequest.domainName = request->GetHost();
if (Ssl::CertErrors *errs = static_cast<Ssl::CertErrors *>(SSL_get_ex_data(ssl, ssl_ex_index_ssl_errors)))
// validationRequest disappears on return so no need to cbdataReference
validationRequest.errors = errs;
else
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 " <<
"request for " << validationRequest.domainName <<
" certificate: " << e.what() << "; will now block to " <<
"validate that certificate.");
// fall through to do blocking in-process generation.
ErrorState *anErr = new ErrorState(ERR_GATEWAY_FAILURE, Http::scInternalServerError, request.getRaw());
bail(anErr);
if (serverConnection()->getPeer()) {
peerConnectFailed(serverConnection()->getPeer());
}
serverConn->close();
return true;
}
}
serverCertificateVerified();
return true;
}
@@ -448,61 +449,56 @@
}
Ssl::BumpMode
Ssl::PeerConnector::checkForPeekAndSpliceGuess() const
{
if (const ConnStateData *csd = request->clientConnectionManager.valid()) {
const Ssl::BumpMode currentMode = csd->sslBumpMode;
if (currentMode == Ssl::bumpStare) {
debugs(83,5, "default to bumping after staring");
return Ssl::bumpBump;
}
debugs(83,5, "default to splicing after " << currentMode);
} else {
debugs(83,3, "default to splicing due to missing info");
}
return Ssl::bumpSplice;
}
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;
if (!Comm::IsConnOpen(serverConnection())) {
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) {
serverCertificateVerified();
if (splice)
switchToTunnel(request.getRaw(), clientConn, serverConn);
else
callBack();
return;
}
ErrorState *anErr = NULL;
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()) {
=== modified file 'src/ssl/PeerConnector.h'
--- src/ssl/PeerConnector.h 2015-09-29 10:50:52 +0000
+++ src/ssl/PeerConnector.h 2015-11-30 16:35:27 +0000
@@ -1,45 +1,47 @@
/*
* Copyright (C) 1996-2015 The Squid Software Foundation and contributors
*
* Squid software is distributed under GPLv2+ license and includes
* contributions from numerous individuals and organizations.
* Please see the COPYING and CONTRIBUTORS files for details.
*/
#ifndef SQUID_SSL_PEER_CONNECTOR_H
#define SQUID_SSL_PEER_CONNECTOR_H
#include "acl/Acl.h"
#include "base/AsyncCbdataCalls.h"
#include "base/AsyncJob.h"
+#include "CommCalls.h"
#include "ssl/support.h"
#include <iosfwd>
class HttpRequest;
class ErrorState;
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.
class PeerConnectorAnswer
{
public:
~PeerConnectorAnswer(); ///< deletes error if it is still set
Comm::ConnectionPointer conn; ///< peer connection (secured on success)
/// answer recepients must clear the error member in order to keep its info
/// XXX: We should refcount ErrorState instead of cbdata-protecting it.
CbcPointer<ErrorState> error; ///< problem details (nil on success)
};
/**
\par
* Connects Squid client-side to an SSL peer (cache_peer ... ssl).
* Handles peer certificate validation.
* Used by TunnelStateData, FwdState, and PeerPoolMgr to start talking to an
* SSL peer.
@@ -137,56 +139,53 @@
/// Called when the SSL negotiation step aborted because data needs to
/// be transferred to/from SSL server or on error. In the first case
/// setups the appropriate Comm::SetSelect handler. In second case
/// fill an error and report to the PeerConnector caller.
void handleNegotiateError(const int result);
private:
PeerConnector(const PeerConnector &); // not implemented
PeerConnector &operator =(const PeerConnector &); // not implemented
/// mimics FwdState to minimize changes to FwdState::initiate/negotiateSsl
Comm::ConnectionPointer const &serverConnection() const { return serverConn; }
void bail(ErrorState *error); ///< Return an error to the PeerConnector caller
/// Callback the caller class, and pass the ready to communicate secure
/// connection or an error if PeerConnector failed.
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 *&);
/// Updates associated client connection manager members
/// if the server certificate was received from the server.
void handleServerCertificate();
/// Runs after the server certificate verified to update client
/// 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);
/// A wrapper function for checkForPeekAndSpliceDone for use with acl
static void cbCheckForPeekAndSpliceDone(allow_t answer, void *data);
HttpRequestPointer request; ///< peer connection trigger or cause
Comm::ConnectionPointer serverConn; ///< TCP connection to the peer
Comm::ConnectionPointer clientConn; ///< TCP connection to the client
AsyncCall::Pointer callback; ///< we call this with the results
AsyncCall::Pointer closeHandler; ///< we call this when the connection closed
time_t negotiationTimeout; ///< the ssl connection timeout to use
time_t startTime; ///< when the peer connector negotiation started
bool splice; ///< Whether we are going to splice or not
bool resumingSession; ///< whether it is an SSL resuming session connection
bool serverCertificateHandled; ///< whether handleServerCertificate() succeeded
CBDATA_CLASS2(PeerConnector);
};
=== modified file 'src/ssl/cert_validate_message.h'
--- src/ssl/cert_validate_message.h 2015-01-13 09:13:49 +0000
+++ src/ssl/cert_validate_message.h 2015-11-30 16:30:55 +0000
@@ -1,60 +1,63 @@
/*
* Copyright (C) 1996-2015 The Squid Software Foundation and contributors
*
* Squid software is distributed under GPLv2+ license and includes
* contributions from numerous individuals and organizations.
* Please see the COPYING and CONTRIBUTORS files for details.
*/
#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"
#include <vector>
namespace Ssl
{
/**
* This class is used to hold the required informations to build
* a request message for the certificate validator helper
*/
class CertValidationRequest
{
public:
SSL *ssl;
CertErrors *errors; ///< The list of errors detected
std::string domainName; ///< The server name
CertValidationRequest() : ssl(NULL), errors(NULL) {}
};
/**
* 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.
*/
class RecvdError
{
public:
RecvdError(): id(0), error_no(SSL_ERROR_NONE), cert(NULL) {}
RecvdError(const RecvdError &);
RecvdError & operator =(const RecvdError &);
void setCert(X509 *); ///< Sets cert to the given certificate
int id; ///< The id of the error
ssl_error_t error_no; ///< The OpenSSL error code
std::string error_reason; ///< A string describing the error
X509_Pointer cert; ///< The broken certificate
};
typedef std::vector<RecvdError> RecvdErrors;
/// Search in errors list for the error item with id=errorId.
=== modified file 'src/ssl/helper.cc'
--- src/ssl/helper.cc 2015-05-22 04:55:35 +0000
+++ src/ssl/helper.cc 2015-11-30 16:38:04 +0000
@@ -2,41 +2,41 @@
* Copyright (C) 1996-2015 The Squid Software Foundation and contributors
*
* Squid software is distributed under GPLv2+ license and includes
* contributions from numerous individuals and organizations.
* Please see the COPYING and CONTRIBUTORS files for details.
*/
#include "squid.h"
#include "../helper.h"
#include "anyp/PortCfg.h"
#include "helper/Reply.h"
#include "SquidConfig.h"
#include "SquidString.h"
#include "SquidTime.h"
#include "ssl/cert_validate_message.h"
#include "ssl/Config.h"
#include "ssl/helper.h"
#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()
{
static Ssl::Helper sslHelper;
return &sslHelper;
}
Ssl::Helper::Helper() : ssl_crtd(NULL)
{
}
Ssl::Helper::~Helper()
{
Shutdown();
}
void Ssl::Helper::Init()
{
assert(ssl_crtd == NULL);
@@ -169,124 +169,128 @@
bool parseParams = true;
while ((token = strwordtok(NULL, &tmp))) {
if (parseParams) {
if (strncmp(token, "ttl=", 4) == 0) {
ttl = atoi(token + 4);
continue;
} else if (strncmp(token, "cache=", 6) == 0) {
cache = atoi(token + 6);
continue;
} else
parseParams = false;
}
wordlistAdd(&ssl_crt_validator->cmdline, token);
}
xfree(tmp_begin);
}
helperOpenServers(ssl_crt_validator);
//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()
{
if (!ssl_crt_validator)
return;
helperShutdown(ssl_crt_validator);
wordlistDestroy(&ssl_crt_validator->cmdline);
delete ssl_crt_validator;
ssl_crt_validator = NULL;
// CertValidationHelper::HelperCache is a static member, it is not good policy to
// reset it here. Will work because the current Ssl::CertValidationHelper is
// always the same static object.
delete HelperCache;
HelperCache = NULL;
}
struct submitData {
std::string query;
- Ssl::CertValidationHelper::CVHCB *callback;
- void *data;
+ AsyncCall::Pointer callback;
SSL *ssl;
CBDATA_CLASS2(submitData);
};
CBDATA_CLASS_INIT(submitData);
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);
STACK_OF(X509) *peerCerts = SSL_get_peer_cert_chain(crtdvdData->ssl);
if (reply.result == ::Helper::BrokenHelper) {
debugs(83, DBG_IMPORTANT, "\"ssl_crtvd\" helper error response: " << reply.other().content());
validationResponse->resultCode = ::Helper::BrokenHelper;
} else if (replyMsg.parse(reply.other().content(), reply.other().contentSize()) != Ssl::CrtdMessage::OK ||
!replyMsg.parseResponse(*validationResponse, peerCerts, error) ) {
debugs(83, DBG_IMPORTANT, "WARNING: Reply from ssl_crtvd for " << " is incorrect");
debugs(83, DBG_IMPORTANT, "Certificate cannot be validated. ssl_crtvd response: " << replyMsg.getBody());
validationResponse->resultCode = ::Helper::BrokenHelper;
} 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);
if (ssl_crt_validator->stats.queue_size >= (int)(ssl_crt_validator->childs.n_running * 2)) {
if (first_warn == 0)
first_warn = squid_curtime;
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;
Ssl::CertValidationMsg message(Ssl::CrtdMessage::REQUEST);
message.setCode(Ssl::CertValidationMsg::code_cert_validate);
message.composeRequest(request);
debugs(83, 5, "SSL crtvd request: " << message.compose().c_str());
submitData *crtdvdData = new submitData;
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;
}
helperSubmit(ssl_crt_validator, crtdvdData->query.c_str(), sslCrtvdHandleReplyWrapper, crtdvdData);
}
=== modified file 'src/ssl/helper.h'
--- src/ssl/helper.h 2015-01-13 09:13:49 +0000
+++ src/ssl/helper.h 2015-11-30 17:19:03 +0000
@@ -1,63 +1,68 @@
/*
* Copyright (C) 1996-2015 The Squid Software Foundation and contributors
*
* Squid software is distributed under GPLv2+ license and includes
* contributions from numerous individuals and organizations.
* Please see the COPYING and CONTRIBUTORS files for details.
*/
#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"
#include "ssl/crtd_message.h"
namespace Ssl
{
/**
* Set of thread for ssl_crtd. This class is singleton. Use this class only
* over GetIntance() static method. This class use helper structure
* for threads management.
*/
#if USE_SSL_CRTD
class Helper
{
public:
static Helper * GetInstance(); ///< Instance class.
void Init(); ///< Init helper structure.
void Shutdown(); ///< Shutdown helper structure.
/// Submit crtd message to external crtd server.
void sslSubmit(CrtdMessage const & message, HLPCB * callback, void *data);
private:
Helper();
~Helper();
helper * ssl_crtd; ///< helper for management of ssl_crtd.
};
#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
#endif // SQUID_SSL_HELPER_H
_______________________________________________
squid-dev mailing list
[email protected]
http://lists.squid-cache.org/listinfo/squid-dev
