> On Oct 17, 2016, at 3:53 PM, shinr...@apache.org wrote:
>
> This is an automated email from the ASF dual-hosted git repository.
>
> shinrich pushed a commit to branch master
> in repository https://git-dual.apache.org/repos/asf/trafficserver.git
>
> The following commit(s) were added to refs/heads/master by this push:
> new 34b5535 TS-4858: fix memory leak problem of
> global_default_keyblock
> 34b5535 is described below
>
> commit 34b55356d1cfb38744bb4b4c080fc758e96c6381
> Author: Persia Aziz <per...@yahoo-inc.com>
> AuthorDate: Tue Sep 20 14:40:28 2016 -0500
>
> TS-4858: fix memory leak problem of global_default_keyblock
> ---
> iocore/net/P_SSLCertLookup.h | 4 +--
> iocore/net/P_SSLConfig.h | 6 +++-
> iocore/net/SSLCertLookup.cc | 70 ++++++++++++++++++++++++++++++++++++++++
> iocore/net/SSLConfig.cc | 23 ++++++++++++--
> iocore/net/SSLUtils.cc | 76 +++-----------------------------------------
> proxy/InkAPI.cc | 1 -
> 6 files changed, 103 insertions(+), 77 deletions(-)
>
> diff --git a/iocore/net/P_SSLCertLookup.h b/iocore/net/P_SSLCertLookup.h
> index b408626..97c11ff 100644
> --- a/iocore/net/P_SSLCertLookup.h
> +++ b/iocore/net/P_SSLCertLookup.h
> @@ -40,7 +40,6 @@ struct ssl_ticket_key_block {
> unsigned num_keys;
> ssl_ticket_key_t keys[];
> };
> -
> /** A certificate context.
>
> This holds data about a certificate and how it is used by the SSL logic.
> Current this is mainly
> @@ -110,5 +109,6 @@ struct SSLCertLookup : public ConfigInfo {
>
> void ticket_block_free(void *ptr);
> ssl_ticket_key_block *ticket_block_alloc(unsigned count);
> -
> +ssl_ticket_key_block *ticket_block_create(char *ticket_key_data, int
> ticket_key_len);
> +ssl_ticket_key_block *ssl_create_ticket_keyblock(const char
> *ticket_key_path);
As noted in the original review, this should be called ticket_block_read() to
be consistent with the other APIs above.
> #endif /* __P_SSLCERTLOOKUP_H__ */
> diff --git a/iocore/net/P_SSLConfig.h b/iocore/net/P_SSLConfig.h
> index 35fdff1..6adc2b5 100644
> --- a/iocore/net/P_SSLConfig.h
> +++ b/iocore/net/P_SSLConfig.h
> @@ -34,9 +34,11 @@
> #include "ProxyConfig.h"
> #include "SSLSessionCache.h"
> #include "ts/ink_inet.h"
> +#include <openssl/rand.h>
> +#include "P_SSLCertLookup.h"
>
> struct SSLCertLookup;
> -
> +struct ssl_ticket_key_block;
> /////////////////////////////////////////////////////////////
> //
> // struct SSLConfigParams
> @@ -67,6 +69,8 @@ struct SSLConfigParams : public ConfigInfo {
> char *dhparamsFile;
> char *cipherSuite;
> char *client_cipherSuite;
> + char *ticket_key_filename;
> + ssl_ticket_key_block *default_global_keyblock;
> int configExitOnLoadError;
> int clientCertLevel;
> int verify_depth;
> diff --git a/iocore/net/SSLCertLookup.cc b/iocore/net/SSLCertLookup.cc
> index b9d7381..972071d 100644
> --- a/iocore/net/SSLCertLookup.cc
> +++ b/iocore/net/SSLCertLookup.cc
> @@ -28,10 +28,18 @@
> #include "P_SSLConfig.h"
> #include "I_EventSystem.h"
> #include "ts/I_Layout.h"
> +#include "ts/MatcherUtils.h"
> #include "ts/Regex.h"
> #include "ts/Trie.h"
> #include "ts/TestBox.h"
>
> +// Check if the ticket_key callback #define is available, and if so, enable
> session tickets.
> +#ifdef SSL_CTX_set_tlsext_ticket_key_cb
> +
> +#define HAVE_OPENSSL_SESSION_TICKETS 1
> +
> +#endif /* SSL_CTX_set_tlsext_ticket_key_cb */
> +
> struct SSLAddressLookupKey {
> explicit SSLAddressLookupKey(const IpEndpoint &ip) : sep(0)
> {
> @@ -160,7 +168,69 @@ ticket_block_alloc(unsigned count)
>
> return ptr;
> }
> +ssl_ticket_key_block *
> +ticket_block_create(char *ticket_key_data, int ticket_key_len)
> +{
> + ssl_ticket_key_block *keyblock = NULL;
> + unsigned num_ticket_keys = ticket_key_len / sizeof(ssl_ticket_key_t);
> + if (num_ticket_keys == 0) {
> + Error("SSL session ticket key is too short (>= 48 bytes are required)");
> + goto fail;
> + }
> +
> + keyblock = ticket_block_alloc(num_ticket_keys);
>
> + // Slurp all the keys in the ticket key file. We will encrypt with the
> first key, and decrypt
> + // with any key (for rotation purposes).
> + for (unsigned i = 0; i < num_ticket_keys; ++i) {
> + const char *data = (const char *)ticket_key_data + (i *
> sizeof(ssl_ticket_key_t));
> +
> + memcpy(keyblock->keys[i].key_name, data,
> sizeof(keyblock->keys[i].key_name));
> + memcpy(keyblock->keys[i].hmac_secret, data +
> sizeof(keyblock->keys[i].key_name), sizeof(keyblock->keys[i].hmac_secret));
> + memcpy(keyblock->keys[i].aes_key, data +
> sizeof(keyblock->keys[i].key_name) + sizeof(keyblock->keys[i].hmac_secret),
> + sizeof(keyblock->keys[i].aes_key));
> + }
> +
> + return keyblock;
> +
> +fail:
> + ticket_block_free(keyblock);
> + return NULL;
> +}
> +
> +ssl_ticket_key_block *
> +ssl_create_ticket_keyblock(const char *ticket_key_path)
> +{
> +#if HAVE_OPENSSL_SESSION_TICKETS
> + ats_scoped_str ticket_key_data;
> + int ticket_key_len;
> + ssl_ticket_key_block *keyblock = NULL;
> +
> + if (ticket_key_path != NULL) {
> + ticket_key_data = readIntoBuffer(ticket_key_path, __func__,
> &ticket_key_len);
> + if (!ticket_key_data) {
> + Error("failed to read SSL session ticket key from %s", (const char
> *)ticket_key_path);
> + goto fail;
> + }
> + keyblock = ticket_block_create(ticket_key_data, ticket_key_len);
> + } else {
> + // Generate a random ticket key
> + ssl_ticket_key_t key;
> + RAND_bytes(reinterpret_cast<unsigned char *>(&key), sizeof(key));
> + keyblock = ticket_block_create(reinterpret_cast<char *>(&key),
> sizeof(key));
> + }
> +
> + return keyblock;
> +
> +fail:
> + ticket_block_free(keyblock);
> + return NULL;
> +
> +#else /* !HAVE_OPENSSL_SESSION_TICKETS */
> + (void)ticket_key_path;
> + return NULL;
> +#endif /* HAVE_OPENSSL_SESSION_TICKETS */
> +}
Please leave a newline between functions.
> void
> SSLCertContext::release()
> {
> diff --git a/iocore/net/SSLConfig.cc b/iocore/net/SSLConfig.cc
> index 13b6921..f52a1b0 100644
> --- a/iocore/net/SSLConfig.cc
> +++ b/iocore/net/SSLConfig.cc
> @@ -64,12 +64,19 @@ char *SSLConfigParams::ssl_wire_trace_server_name = NULL;
>
> static ConfigUpdateHandler<SSLCertificateConfig> *sslCertUpdate;
>
> +// Check if the ticket_key callback #define is available, and if so, enable
> session tickets.
> +#ifdef SSL_CTX_set_tlsext_ticket_key_cb
> +
> +#define HAVE_OPENSSL_SESSION_TICKETS 1
> +
> +#endif /* SSL_CTX_set_tlsext_ticket_key_cb */
> +
> SSLConfigParams::SSLConfigParams()
> {
> serverCertPathOnly = serverCertChainFilename = configFilePath =
> serverCACertFilename = serverCACertPath = clientCertPath =
> clientKeyPath = clientCACertFilename = clientCACertPath = cipherSuite =
> client_cipherSuite = dhparamsFile = serverKeyPathOnly =
> - NULL;
> -
> + ticket_key_filename
> = NULL;
> + default_global_keyblock
> = NULL;
> clientCertLevel = client_verify_depth = verify_depth = clientVerify = 0;
>
> ssl_ctx_options = SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3;
> @@ -105,6 +112,8 @@ SSLConfigParams::cleanup()
> ats_free_null(client_cipherSuite);
> ats_free_null(dhparamsFile);
> ats_free_null(ssl_wire_trace_ip);
> + ats_free_null(ticket_key_filename);
> + ticket_block_free(default_global_keyblock);
>
> clientCertLevel = client_verify_depth = verify_depth = clientVerify = 0;
> }
> @@ -243,6 +252,16 @@ SSLConfigParams::initialize()
> ats_free(ssl_server_ca_cert_filename);
> ats_free(CACertRelativePath);
>
> +#if HAVE_OPENSSL_SESSION_TICKETS
> + REC_ReadConfigStringAlloc(ticket_key_filename,
> "proxy.config.ssl.server.ticket_key.filename");
> + if (this->ticket_key_filename != NULL) {
> + ats_scoped_str
> ticket_key_path(Layout::relative_to(this->serverCertPathOnly,
> this->ticket_key_filename));
> + default_global_keyblock = ssl_create_ticket_keyblock(ticket_key_path);
> + } else {
> + default_global_keyblock = ssl_create_ticket_keyblock(NULL);
> + }
> +#endif
> +
> // SSL session cache configurations
> REC_ReadConfigInteger(ssl_session_cache, "proxy.config.ssl.session_cache");
> REC_ReadConfigInteger(ssl_session_cache_size,
> "proxy.config.ssl.session_cache.size");
> diff --git a/iocore/net/SSLUtils.cc b/iocore/net/SSLUtils.cc
> index ef44181..ce96222 100644
> --- a/iocore/net/SSLUtils.cc
> +++ b/iocore/net/SSLUtils.cc
> @@ -133,8 +133,7 @@ static int ssl_callback_session_ticket(SSL *, unsigned
> char *, unsigned char *,
> #endif /* SSL_CTX_set_tlsext_ticket_key_cb */
>
> #if HAVE_OPENSSL_SESSION_TICKETS
> -static int ssl_session_ticket_index = -1;
> -static ssl_ticket_key_block *global_default_keyblock = NULL;
> +static int ssl_session_ticket_index = -1;
> #endif
>
> static int ssl_vc_index = -1;
> @@ -562,70 +561,15 @@ ssl_context_enable_ecdh(SSL_CTX *ctx)
> }
>
> static ssl_ticket_key_block *
> -ssl_create_ticket_keyblock(const char *ticket_key_path)
> +ssl_context_enable_tickets(SSL_CTX *ctx, const char *ticket_key_path)
> {
> #if HAVE_OPENSSL_SESSION_TICKETS
> - ats_scoped_str ticket_key_data;
> - int ticket_key_len;
> - unsigned num_ticket_keys;
> ssl_ticket_key_block *keyblock = NULL;
> -
> - if (ticket_key_path != NULL) {
> - ticket_key_data = readIntoBuffer(ticket_key_path, __func__,
> &ticket_key_len);
> - if (!ticket_key_data) {
> - Error("failed to read SSL session ticket key from %s", (const char
> *)ticket_key_path);
> - goto fail;
> - }
> - } else {
> - // Generate a random ticket key
> - ticket_key_len = 48;
> - ticket_key_data = (char *)ats_malloc(ticket_key_len);
> - char *tmp_ptr = ticket_key_data;
> - RAND_bytes(reinterpret_cast<unsigned char *>(tmp_ptr), ticket_key_len);
> - }
> -
> - num_ticket_keys = ticket_key_len / sizeof(ssl_ticket_key_t);
> - if (num_ticket_keys == 0) {
> - Error("SSL session ticket key from %s is too short (>= 48 bytes are
> required)", (const char *)ticket_key_path);
> - goto fail;
> - }
> -
> + keyblock =
> ssl_create_ticket_keyblock(ticket_key_path);
> // Increase the stats.
> if (ssl_rsb != NULL) { // ssl_rsb is not initialized during the first run.
> SSL_INCREMENT_DYN_STAT(ssl_total_ticket_keys_renewed_stat);
> }
> -
> - keyblock = ticket_block_alloc(num_ticket_keys);
> -
> - // Slurp all the keys in the ticket key file. We will encrypt with the
> first key, and decrypt
> - // with any key (for rotation purposes).
> - for (unsigned i = 0; i < num_ticket_keys; ++i) {
> - const char *data = (const char *)ticket_key_data + (i *
> sizeof(ssl_ticket_key_t));
> -
> - memcpy(keyblock->keys[i].key_name, data,
> sizeof(keyblock->keys[i].key_name));
> - memcpy(keyblock->keys[i].hmac_secret, data +
> sizeof(keyblock->keys[i].key_name), sizeof(keyblock->keys[i].hmac_secret));
> - memcpy(keyblock->keys[i].aes_key, data +
> sizeof(keyblock->keys[i].key_name) + sizeof(keyblock->keys[i].hmac_secret),
> - sizeof(keyblock->keys[i].aes_key));
> - }
> -
> - return keyblock;
> -
> -fail:
> - ticket_block_free(keyblock);
> - return NULL;
> -
> -#else /* !HAVE_OPENSSL_SESSION_TICKETS */
> - (void)ticket_key_path;
> - return NULL;
> -#endif /* HAVE_OPENSSL_SESSION_TICKETS */
> -}
> -
> -static ssl_ticket_key_block *
> -ssl_context_enable_tickets(SSL_CTX *ctx, const char *ticket_key_path)
> -{
> -#if HAVE_OPENSSL_SESSION_TICKETS
> - ssl_ticket_key_block *keyblock = NULL;
> - keyblock =
> ssl_create_ticket_keyblock(ticket_key_path);
> // Setting the callback can only fail if OpenSSL does not recognize the
> // SSL_CTRL_SET_TLSEXT_TICKET_KEY_CB constant. we set the callback first
> // so that we don't leave a ticket_key pointer attached if it fails.
> @@ -2054,22 +1998,11 @@ SSLParseCertificateConfiguration(const
> SSLConfigParams *params, SSLCertLookup *l
> char *tok_state = NULL;
> char *line = NULL;
> ats_scoped_str file_buf;
> - ats_scoped_str ticket_key_filename;
> unsigned line_num = 0;
> matcher_line line_info;
>
> const matcher_tags sslCertTags = {NULL, NULL, NULL, NULL, NULL, NULL,
> false};
>
> - // Load the global ticket key for later use.
> - REC_ReadConfigStringAlloc(ticket_key_filename,
> "proxy.config.ssl.server.ticket_key.filename");
> -
> - if (ticket_key_filename != NULL) {
> - ats_scoped_str
> ticket_key_path(Layout::relative_to(params->serverCertPathOnly,
> ticket_key_filename));
> - global_default_keyblock = ssl_create_ticket_keyblock(ticket_key_path);
> - } else {
> - global_default_keyblock = ssl_create_ticket_keyblock(NULL);
> - }
> -
> Note("loading SSL certificate configuration from %s",
> params->configFilePath);
>
> if (params->configFilePath) {
> @@ -2153,6 +2086,7 @@ ssl_callback_session_ticket(SSL *ssl, unsigned char
> *keyname, unsigned char *iv,
> int enc)
> {
> SSLCertificateConfig::scoped_config lookup;
> + SSLConfig::scoped_config params;
> SSLNetVConnection *netvc = SSLNetVCAccess(ssl);
>
> // Get the IP address to look up the keyblock
> @@ -2163,7 +2097,7 @@ ssl_callback_session_ticket(SSL *ssl, unsigned char
> *keyname, unsigned char *iv,
> ssl_ticket_key_block *keyblock = NULL;
> if (cc == NULL || cc->keyblock == NULL) {
> // Try the default
> - keyblock = global_default_keyblock;
> + keyblock = params->default_global_keyblock;
> } else {
> keyblock = cc->keyblock;
> }
> diff --git a/proxy/InkAPI.cc b/proxy/InkAPI.cc
> index 52f8c00..2c92c42 100644
> --- a/proxy/InkAPI.cc
> +++ b/proxy/InkAPI.cc
> @@ -5440,7 +5440,6 @@ TSHttpSsnIncomingAddrGet(TSHttpSsn ssnp)
> if (vc == NULL) {
> return 0;
> }
> -
> return vc->get_local_addr();
> }
> sockaddr const *
>
> --
> To stop receiving notification emails like this one, please contact
> ['"comm...@trafficserver.apache.org" <comm...@trafficserver.apache.org>'].
