Do not leak [SSL] objects tied to http_port and https_port on reconfigure.
PortCfg objects were not destroyed at all (no delete call) and were
incorrectly stored (excessive cbdata locking). This change adds
destruction and removes excessive locking to allow the destructed
object to be freed. It also cleans up forgotten(?) clientca and crlfile
PortCfg members.
This change fixes a serious leak but also carries an elevated risk:
There is a lot of code throughout Squid that does not check the pointers
to the objects that are now properly destroyed. It is possible that some
of that code will crash some time after reconfigure. It is not possible
to ensure that this does not happen without rewriting/fixing the
offending code to use refcounting. Such a rewrite would be a relatively
large change outside this patch scope. We may decide that it is better
to leak than to take this additional risk.
Alex.
Do not leak [SSL] objects tied to http_port and https_port on reconfigure
PortCfg objects were not destroyed at all (no delete call) and were
incorrectly stored (excessive cbdata locking). This change adds destruction
and removes excessive locking to allow the destructed object to be freed.
It also cleans up forgotten(?) clientca and crlfile PortCfg members.
This change fixes a serious leak but also carries an elevated risk:
There is a lot of code throughout Squid that does not check the pointers to
the objects that are now properly destroyed. It is possible that some of
that code will crash some time after reconfigure. It is not possible to
ensure that this does not happen without rewriting/fixing the offending
code to use refcounting. Such a rewrite would be a relatively large
change outside this patch scope. We may decide that it is better to leak
than to take this additional risk.
=== modified file 'src/anyp/PortCfg.cc'
--- src/anyp/PortCfg.cc 2014-04-16 21:58:08 +0000
+++ src/anyp/PortCfg.cc 2014-04-24 20:19:11 +0000
@@ -57,44 +57,46 @@ AnyP::PortCfg::PortCfg() :
sslContextFlags(0),
sslOptions(0)
#endif
{
memset(&tcp_keepalive, 0, sizeof(tcp_keepalive));
}
AnyP::PortCfg::~PortCfg()
{
if (Comm::IsConnOpen(listenConn)) {
listenConn->close();
listenConn = NULL;
}
safe_free(name);
safe_free(defaultsite);
#if USE_OPENSSL
safe_free(cert);
safe_free(key);
- safe_free(options);
safe_free(cipher);
+ safe_free(options);
+ safe_free(clientca);
safe_free(cafile);
safe_free(capath);
+ safe_free(crlfile);
safe_free(dhfile);
safe_free(sslflags);
safe_free(sslContextSessionId);
#endif
}
AnyP::PortCfg *
AnyP::PortCfg::clone() const
{
AnyP::PortCfg *b = new AnyP::PortCfg();
b->s = s;
if (name)
b->name = xstrdup(name);
if (defaultsite)
b->defaultsite = xstrdup(defaultsite);
b->transport = transport;
b->flags = flags;
b->allow_direct = allow_direct;
b->vhost = vhost;
=== modified file 'src/cache_cf.cc'
--- src/cache_cf.cc 2014-04-12 17:32:34 +0000
+++ src/cache_cf.cc 2014-04-24 20:23:43 +0000
@@ -3761,43 +3766,43 @@ parse_port_option(AnyP::PortCfg * s, cha
s->generateHostCertificates = true;
} else if (strcmp(token, "generate-host-certificates=off") == 0) {
s->generateHostCertificates = false;
} else if (strncmp(token, "dynamic_cert_mem_cache_size=", 28) == 0) {
parseBytesOptionValue(&s->dynamicCertMemCacheSize, B_BYTES_STR, token + 28);
#endif
} else {
debugs(3, DBG_CRITICAL, "FATAL: Unknown http(s)_port option '" << token << "'.");
self_destruct();
}
}
void
add_http_port(char *portspec)
{
AnyP::PortCfg *s = new AnyP::PortCfg();
s->setTransport("HTTP");
parsePortSpecification(s, portspec);
// we may need to merge better if the above returns a list with clones
assert(s->next == NULL);
- s->next = cbdataReference(Config.Sockaddr.http);
- cbdataReferenceDone(Config.Sockaddr.http);
- Config.Sockaddr.http = cbdataReference(s);
+ // no cbdata locking here: Config owns the ports (deleted in free_PortCfg)
+ s->next = Config.Sockaddr.http;
+ Config.Sockaddr.http = s;
}
static void
parsePortCfg(AnyP::PortCfg ** head, const char *optionName)
{
const char *protocol = NULL;
if (strcmp(optionName, "http_port") == 0 ||
strcmp(optionName, "ascii_port") == 0)
protocol = "http";
else if (strcmp(optionName, "https_port") == 0)
protocol = "https";
if (!protocol) {
self_destruct();
return;
}
char *token = ConfigParser::NextToken();
if (!token) {
self_destruct();
@@ -3820,42 +3825,42 @@ parsePortCfg(AnyP::PortCfg ** head, cons
if (s->flags.tunnelSslBumping && !hijacked) {
debugs(3, DBG_CRITICAL, "FATAL: ssl-bump on https_port requires tproxy/intercept which is missing.");
self_destruct();
}
if (hijacked && !s->flags.tunnelSslBumping) {
debugs(3, DBG_CRITICAL, "FATAL: tproxy/intercept on https_port requires ssl-bump which is missing.");
self_destruct();
}
}
#endif
if (Ip::EnableIpv6&IPV6_SPECIAL_SPLITSTACK && s->s.isAnyAddr()) {
// clone the port options from *s to *(s->next)
s->next = cbdataReference(s->clone());
s->next->s.setIPv4();
debugs(3, 3, AnyP::UriScheme(s->transport.protocol).c_str() << "_port: clone wildcard address for split-stack: " << s->s << " and " << s->next->s);
}
while (*head)
head = &(*head)->next;
-
- *head = cbdataReference(s);
+ // no cbdata locking here: Config owns the ports (deleted in free_PortCfg)
+ *head = s;
}
static void
dump_generic_port(StoreEntry * e, const char *n, const AnyP::PortCfg * s)
{
char buf[MAX_IPSTRLEN];
storeAppendPrintf(e, "%s %s",
n,
s->s.toUrl(buf,MAX_IPSTRLEN));
// MODES and specific sub-options.
if (s->flags.natIntercept)
storeAppendPrintf(e, " intercept");
else if (s->flags.tproxyIntercept)
storeAppendPrintf(e, " tproxy");
else if (s->flags.accelSurrogate) {
storeAppendPrintf(e, " accel");
@@ -3965,50 +3970,51 @@ dump_generic_port(StoreEntry * e, const
#endif
}
static void
dump_PortCfg(StoreEntry * e, const char *n, const AnyP::PortCfg * s)
{
while (s) {
dump_generic_port(e, n, s);
storeAppendPrintf(e, "\n");
s = s->next;
}
}
static void
free_PortCfg(AnyP::PortCfg ** head)
{
AnyP::PortCfg *s;
while ((s = *head) != NULL) {
*head = s->next;
- cbdataReferenceDone(s);
+ delete s;
}
}
void
configFreeMemory(void)
{
free_all();
#if USE_OPENSSL
SSL_CTX_free(Config.ssl_client.sslContext);
+ Config.ssl_client.sslContext = NULL;
#endif
}
void
requirePathnameExists(const char *name, const char *path)
{
struct stat sb;
char pathbuf[BUFSIZ];
assert(path != NULL);
if (Config.chroot_dir && (geteuid() == 0)) {
snprintf(pathbuf, BUFSIZ, "%s/%s", Config.chroot_dir, path);
path = pathbuf;
}
if (stat(path, &sb) < 0) {
debugs(0, DBG_CRITICAL, (opt_parse_cfg_only?"FATAL ":"") << "ERROR: " << name << " " << path << ": " << xstrerror());
// keep going to find more issues if we are only checking the config file with "-k parse"
if (opt_parse_cfg_only)
=== modified file 'src/client_side.cc'
--- src/client_side.cc 2014-03-31 06:57:27 +0000
+++ src/client_side.cc 2014-04-24 20:12:30 +0000
@@ -3704,43 +3704,48 @@ ConnStateData::sslCrtdHandleReplyWrapper
ConnStateData * state_data = (ConnStateData *)(data);
state_data->sslCrtdHandleReply(reply);
}
void
ConnStateData::sslCrtdHandleReply(const HelperReply &reply)
{
if (reply.result == HelperReply::BrokenHelper) {
debugs(33, 5, HERE << "Certificate for " << sslConnectHostOrIp << " cannot be generated. ssl_crtd response: " << reply);
} else if (!reply.other().hasContent()) {
debugs(1, DBG_IMPORTANT, HERE << "\"ssl_crtd\" helper returned <NULL> reply.");
} else {
Ssl::CrtdMessage reply_message(Ssl::CrtdMessage::REPLY);
if (reply_message.parse(reply.other().content(), reply.other().contentSize()) != Ssl::CrtdMessage::OK) {
debugs(33, 5, HERE << "Reply from ssl_crtd for " << sslConnectHostOrIp << " is incorrect");
} else {
if (reply.result != HelperReply::Okay) {
debugs(33, 5, HERE << "Certificate for " << sslConnectHostOrIp << " cannot be generated. ssl_crtd response: " << reply_message.getBody());
} else {
debugs(33, 5, HERE << "Certificate for " << sslConnectHostOrIp << " was successfully recieved from ssl_crtd");
- SSL_CTX *ctx = Ssl::generateSslContextUsingPkeyAndCertFromMemory(reply_message.getBody().c_str(), *port);
- getSslContextDone(ctx, true);
- return;
+ if (cbdataReferenceValid(port)) {
+ SSL_CTX *ctx = Ssl::generateSslContextUsingPkeyAndCertFromMemory(reply_message.getBody().c_str(), *port);
+ getSslContextDone(ctx, true);
+ return;
+ } else {
+ debugs(33, 3, "but our port is gone");
+ // getSslContextDone() will cleanup
+ }
}
}
}
getSslContextDone(NULL);
}
void ConnStateData::buildSslCertGenerationParams(Ssl::CertificateProperties &certProperties)
{
certProperties.commonName = sslCommonName.size() > 0 ? sslCommonName.termedBuf() : sslConnectHostOrIp.termedBuf();
// fake certificate adaptation requires bump-server-first mode
if (!sslServerBump) {
assert(port->signingCert.get());
certProperties.signWithX509.resetAndLock(port->signingCert.get());
if (port->signPkey.get())
certProperties.signWithPkey.resetAndLock(port->signPkey.get());
certProperties.signAlgorithm = Ssl::algSignTrusted;
return;
}
@@ -3806,41 +3811,41 @@ void ConnStateData::buildSslCertGenerati
assert(port->untrustedSigningCert.get());
certProperties.signWithX509.resetAndLock(port->untrustedSigningCert.get());
certProperties.signWithPkey.resetAndLock(port->untrustedSignPkey.get());
} else {
assert(port->signingCert.get());
certProperties.signWithX509.resetAndLock(port->signingCert.get());
if (port->signPkey.get())
certProperties.signWithPkey.resetAndLock(port->signPkey.get());
}
signAlgorithm = certProperties.signAlgorithm;
}
void
ConnStateData::getSslContextStart()
{
assert(areAllContextsForThisConnection());
freeAllContexts();
/* careful: freeAllContexts() above frees request, host, etc. */
- if (port->generateHostCertificates) {
+ if (cbdataReferenceValid(port) && port->generateHostCertificates) {
Ssl::CertificateProperties certProperties;
buildSslCertGenerationParams(certProperties);
sslBumpCertKey = certProperties.dbKey().c_str();
assert(sslBumpCertKey.size() > 0 && sslBumpCertKey[0] != '\0');
debugs(33, 5, HERE << "Finding SSL certificate for " << sslBumpCertKey << " in cache");
Ssl::LocalContextStorage *ssl_ctx_cache = Ssl::TheGlobalContextStorage.getLocalStorage(port->s);
SSL_CTX * dynCtx = NULL;
Ssl::SSL_CTX_Pointer *cachedCtx = ssl_ctx_cache ? ssl_ctx_cache->get(sslBumpCertKey.termedBuf()) : NULL;
if (cachedCtx && (dynCtx = cachedCtx->get())) {
debugs(33, 5, HERE << "SSL certificate for " << sslBumpCertKey << " have found in cache");
if (Ssl::verifySslCertificate(dynCtx, certProperties)) {
debugs(33, 5, HERE << "Cached SSL certificate for " << sslBumpCertKey << " is valid");
getSslContextDone(dynCtx);
return;
} else {
debugs(33, 5, HERE << "Cached SSL certificate for " << sslBumpCertKey << " is out of date. Delete this certificate from cache");
if (ssl_ctx_cache)
ssl_ctx_cache->del(sslBumpCertKey.termedBuf());
}
@@ -3860,40 +3865,48 @@ ConnStateData::getSslContextStart()
} catch (const std::exception &e) {
debugs(33, DBG_IMPORTANT, "ERROR: Failed to compose ssl_crtd " <<
"request for " << certProperties.commonName <<
" certificate: " << e.what() << "; will now block to " <<
"generate that certificate.");
// fall through to do blocking in-process generation.
}
#endif // USE_SSL_CRTD
debugs(33, 5, HERE << "Generating SSL certificate for " << certProperties.commonName);
dynCtx = Ssl::generateSslContext(certProperties, *port);
getSslContextDone(dynCtx, true);
return;
}
getSslContextDone(NULL);
}
void
ConnStateData::getSslContextDone(SSL_CTX * sslContext, bool isNew)
{
+ if (!cbdataReferenceValid(port)) { // reconfigure may delete our port
+ debugs(83, 2, "Closing SSL " << clientConnection->remote << " for lost port");
+ if (sslContext && Comm::IsConnOpen(clientConnection))
+ fd_table[clientConnection->fd].dynamicSslContext = sslContext;
+ clientConnection->close();
+ return;
+ }
+
// Try to add generated ssl context to storage.
if (port->generateHostCertificates && isNew) {
if (signAlgorithm == Ssl::algSignTrusted) {
// Add signing certificate to the certificates chain
X509 *cert = port->signingCert.get();
if (SSL_CTX_add_extra_chain_cert(sslContext, cert)) {
// increase the certificate lock
CRYPTO_add(&(cert->references),1,CRYPTO_LOCK_X509);
} else {
const int ssl_error = ERR_get_error();
debugs(33, DBG_IMPORTANT, "WARNING: can not add signing certificate to SSL context chain: " << ERR_error_string(ssl_error, NULL));
}
Ssl::addChainToSslContext(sslContext, port->certsToChain.get());
}
//else it is self-signed or untrusted do not attrach any certificate
Ssl::LocalContextStorage *ssl_ctx_cache = Ssl::TheGlobalContextStorage.getLocalStorage(port->s);
assert(sslBumpCertKey.size() > 0 && sslBumpCertKey[0] != '\0');
if (sslContext) {
