Repository: trafficserver Updated Branches: refs/heads/master 87efe7937 -> 064126785
TS-3375: Memory leak and error handling in SSL loading. Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo Commit: http://git-wip-us.apache.org/repos/asf/trafficserver/commit/5fb742d6 Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/5fb742d6 Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/5fb742d6 Branch: refs/heads/master Commit: 5fb742d63742abbc0e441b6b9b8a74673097cf81 Parents: 2dbdd9c Author: shinrich <shinr...@yahoo-inc.com> Authored: Mon Feb 9 21:56:08 2015 -0600 Committer: shinrich <shinr...@yahoo-inc.com> Committed: Mon Feb 9 21:56:08 2015 -0600 ---------------------------------------------------------------------- CHANGES | 2 ++ iocore/net/SSLNetProcessor.cc | 2 +- iocore/net/SSLUtils.cc | 44 +++++++++++++++++++++++++++----------- proxy/Main.cc | 5 ++++- 4 files changed, 38 insertions(+), 15 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/trafficserver/blob/5fb742d6/CHANGES ---------------------------------------------------------------------- diff --git a/CHANGES b/CHANGES index 4a80d57..8a58d47 100644 --- a/CHANGES +++ b/CHANGES @@ -1,6 +1,8 @@ -*- coding: utf-8 -*- Changes with Apache Traffic Server 5.3.0 + *) [TS-3375] Memory leak and error handling in SSL loading. + *) [TS-3380] Fix the detection of new openssl calls to enable compilation against libressl. *) [TS-3364] Add command line config validation support to traffic_server http://git-wip-us.apache.org/repos/asf/trafficserver/blob/5fb742d6/iocore/net/SSLNetProcessor.cc ---------------------------------------------------------------------- diff --git a/iocore/net/SSLNetProcessor.cc b/iocore/net/SSLNetProcessor.cc index 9547617..761487c 100644 --- a/iocore/net/SSLNetProcessor.cc +++ b/iocore/net/SSLNetProcessor.cc @@ -68,7 +68,7 @@ SSLNetProcessor::start(int number_of_ssl_threads, size_t stacksize) SSLInitializeLibrary(); SSLConfig::startup(); - (void) SSLCertificateConfig::startup(); + if (!SSLCertificateConfig::startup()) return -1; // Acquire a SSLConfigParams instance *after* we start SSL up. SSLConfig::scoped_config params; http://git-wip-us.apache.org/repos/asf/trafficserver/blob/5fb742d6/iocore/net/SSLUtils.cc ---------------------------------------------------------------------- diff --git a/iocore/net/SSLUtils.cc b/iocore/net/SSLUtils.cc index 638b7ab..86c9149 100644 --- a/iocore/net/SSLUtils.cc +++ b/iocore/net/SSLUtils.cc @@ -1584,16 +1584,17 @@ asn1_strdup(ASN1_STRING * s) // Given a certificate and it's corresponding SSL_CTX context, insert hash // table aliases for subject CN and subjectAltNames DNS without wildcard, // insert trie aliases for those with wildcard. -static void +static bool ssl_index_certificate(SSLCertLookup * lookup, SSLCertContext const& cc, const char * certfile) { X509_NAME * subject = NULL; X509 * cert; scoped_BIO bio(BIO_new_file(certfile, "r")); + bool inserted = false; cert = PEM_read_bio_X509_AUX(bio.get(), NULL, NULL, NULL); if (NULL == cert) { - return; + return inserted; } // Insert a key for the subject CN. @@ -1612,7 +1613,7 @@ ssl_index_certificate(SSLCertLookup * lookup, SSLCertContext const& cc, const ch subj_name = asn1_strdup(cn); Debug("ssl", "mapping '%s' to certificate %s", (const char *) subj_name, certfile); - lookup->insert(subj_name, cc); + if (lookup->insert(subj_name, cc) >= 0) inserted = true; } } @@ -1630,7 +1631,7 @@ ssl_index_certificate(SSLCertLookup * lookup, SSLCertContext const& cc, const ch // only try to insert if the alternate name is not the main name if (strcmp(dns, subj_name) != 0) { Debug("ssl", "mapping '%s' to certificate %s", (const char *) dns, certfile); - lookup->insert(dns, cc); + if (lookup->insert(dns, cc) >= 0) inserted = true; } } } @@ -1639,6 +1640,7 @@ ssl_index_certificate(SSLCertLookup * lookup, SSLCertContext const& cc, const ch } #endif // HAVE_OPENSSL_TS_H X509_free(cert); + return inserted; } // This callback function is executed while OpenSSL processes the SSL @@ -1697,6 +1699,8 @@ ssl_store_ssl_context( ssl_ticket_key_block *keyblock = NULL; bool inserted = false; + if (!ctx) return ctx; + // The certificate callbacks are set by the caller only // for the default certificate @@ -1740,6 +1744,7 @@ ssl_store_ssl_context( } } else { Error("'%s' is not a valid IPv4 or IPv6 address", (const char *)sslMultCertSettings.addr); + goto end; } } } @@ -1757,14 +1762,14 @@ ssl_store_ssl_context( Debug("ssl", "ssl ocsp stapling is enabled"); SSL_CTX_set_tlsext_status_cb(ctx, ssl_callback_ocsp_stapling); if (!ssl_stapling_init_cert(ctx, (const char *)certpath)) { - Error("fail to configure SSL_CTX for OCSP Stapling info"); + Warning("fail to configure SSL_CTX for OCSP Stapling info for certificate at %s", (const char *)certpath); } } else { Debug("ssl", "ssl ocsp stapling is disabled"); } #else if (SSLConfigParams::ssl_ocsp_enabled) { - Error("fail to enable ssl ocsp stapling, this openssl version does not support it"); + Warning("fail to enable ssl ocsp stapling, this openssl version does not support it"); } #endif /* HAVE_OPENSSL_OCSP_STAPLING */ @@ -1772,16 +1777,25 @@ ssl_store_ssl_context( // this code is updated to reconfigure the SSL certificates, it will need some sort of // refcounting or alternate way of avoiding double frees. Debug("ssl", "importing SNI names from %s", (const char *)certpath); - ssl_index_certificate(lookup, SSLCertContext(ctx, sslMultCertSettings.opt), certpath); + inserted = inserted || ssl_index_certificate(lookup, SSLCertContext(ctx, sslMultCertSettings.opt), certpath); - if (SSLConfigParams::init_ssl_ctx_cb) { - SSLConfigParams::init_ssl_ctx_cb(ctx, true); + if (inserted) { + if (SSLConfigParams::init_ssl_ctx_cb) { + SSLConfigParams::init_ssl_ctx_cb(ctx, true); + } } +end: + if (!inserted) { #if HAVE_OPENSSL_SESSION_TICKETS - if (!inserted && keyblock != NULL) { - ticket_block_free(keyblock); - } + if (keyblock != NULL) { + ticket_block_free(keyblock); + } #endif + if (ctx != NULL) { + SSL_CTX_free(ctx); + ctx = NULL; + } + } return ctx; } @@ -1905,6 +1919,7 @@ SSLParseCertificateConfiguration(const SSLConfigParams * params, SSLCertLookup * if (ssl_store_ssl_context(params, lookup, sslMultiCertSettings) == NULL) { Error("failed to load SSL certificate specification from %s line %u", params->configFilePath, line_num); + return false; } } else { RecSignalWarning(REC_SIGNAL_CONFIG_ERROR, "%s: discarding invalid %s entry at line %u", @@ -1923,7 +1938,10 @@ SSLParseCertificateConfiguration(const SSLConfigParams * params, SSLCertLookup * if (lookup->ssl_default == NULL) { ssl_user_config sslMultiCertSettings; sslMultiCertSettings.addr = ats_strdup("*"); - ssl_store_ssl_context(params, lookup, sslMultiCertSettings); + if (ssl_store_ssl_context(params, lookup, sslMultiCertSettings) == NULL) { + Error("failed set default context"); + return false; + } } return true; } http://git-wip-us.apache.org/repos/asf/trafficserver/blob/5fb742d6/proxy/Main.cc ---------------------------------------------------------------------- diff --git a/proxy/Main.cc b/proxy/Main.cc index 7e9a433..c4991d3 100644 --- a/proxy/Main.cc +++ b/proxy/Main.cc @@ -1697,7 +1697,10 @@ main(int /* argc ATS_UNUSED */, char **argv) (void) plugin_init(); // plugin.config SSLConfigParams::init_ssl_ctx_cb = init_ssl_ctx_callback; - sslNetProcessor.start(getNumSSLThreads(), stacksize); + if (sslNetProcessor.start(getNumSSLThreads(), stacksize) < 0) { + // Failure to load SSL state + exit(1); + } pmgmt->registerPluginCallbacks(global_config_cbs); cacheProcessor.set_after_init_callback(&CB_After_Cache_Init);
