From: Matthew Jordan Date: Fri, 14 Sep 2012 20:25:44 +0000 (+0000) Subject: Multiple revisions 366052,367002,367416,369731,373061 X-Git-Tag: certified/1.8.11-cert8~14 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=8fb6cbc65e78ba75c80098522240eef765382416;p=thirdparty%2Fasterisk.git Multiple revisions 366052,367002,367416,369731,373061 ........ r366052 | mmichelson | 2012-05-10 11:10:18 -0500 (Thu, 10 May 2012) | 7 lines Close the proper tcptls_session when session creation fails. (issue AST-998) Reported by: Thomas Arimont Tested by: Thomas Arimont ........ r367002 | mmichelson | 2012-05-18 11:53:47 -0500 (Fri, 18 May 2012) | 17 lines Fix memory leak of SSL_CTX structures in TLS core. SSL_CTX structures were allocated but never freed. This was a bigger issue for clients than servers since new SSL_CTX structures could be allocated for each connection. Servers, on the other hand, typically set up a single SSL_CTX for their lifetime. This is solved in two ways: 1. In __ssl_setup(), if a tcptls_cfg has an ssl_ctx on it, it is freed so that a new one can take its place. 2. A companion to ast_ssl_setup() called ast_ssl_teardown() has been added so that servers can properly free their SSL_CTXs. (issue ASTERISK-19278) ........ r367416 | mmichelson | 2012-05-23 15:27:47 -0500 (Wed, 23 May 2012) | 5 lines Only call SSL_CTX_free if DO_SSL is defined. Thanks to Paul Belanger for pointing out this error. ........ r369731 | mmichelson | 2012-07-06 13:40:06 -0500 (Fri, 06 Jul 2012) | 19 lines Remove a superfluous and dangerous freeing of an SSL_CTX. The problem here is that multiple server sessions share a SSL_CTX. When one session ended, the SSL_CTX would be freed and set NULL, leaving the other sessions unable to function. The code being removed is superfluous because the SSL_CTX structures for servers will be properly freed when ast_ssl_teardown is called. (closes issue ASTERISK-20074) Reported by Trevor Helmsley Patches: ASTERISK-20074.diff uploaded by Mark Michelson (license #5049) Testers: Trevor Helmsley ........ r373061 | mjordan | 2012-09-14 14:07:20 -0500 (Fri, 14 Sep 2012) | 28 lines Resolve memory leaks in TLS initialization and TLS client connections This patch resolves two sources of memory leaks when using TLS in Asterisk: 1) It removes improper initialization (and multiple re-initializations) of portions of the SSL library. Asterisk calls SSL_library_init and SSL_load_error_strings during SSL initialization; collectively this obviates the need for calling any of the following during initialization or client connection handling: * ERR_load_crypto_strings (handled by SSL_load_error_strings) * OpenSSL_add_all_algorithms (synonym for SSL_library_init) * SSLeay_add_ssl_algorithms (synonym for SSL_library_init) 2) Failure to completely clean up all memory allocated by Asterisk and by the SSL library for TLS clients. This included not freeing the SSL_CTX object in the SIP channel driver, as well as not clearing the error stack when the TLS client exited. Note that these memory leaks were found by Thomas Arimont, and this patch was essentially written by him with some minor tweaks. (closes issue AST-889) Reported by: Thomas Arimont Tested by: Thomas Arimont patches: (bugAST-889.patch) by Thomas Arimont (license 5525) Review: https://reviewboard.asterisk.org/r/2105 ........ Merged revisions 366052,367002,367416,369731,373061 from http://svn.asterisk.org/svn/asterisk/branches/1.8 git-svn-id: https://origsvn.digium.com/svn/asterisk/certified/branches/1.8.11@373088 65c4cc65-6c06-0410-ace0-fbb531ad65f3 --- diff --git a/channels/chan_sip.c b/channels/chan_sip.c index 456d5fa7e7..d1939b73ef 100644 --- a/channels/chan_sip.c +++ b/channels/chan_sip.c @@ -2381,6 +2381,8 @@ static void sip_tcptls_client_args_destructor(void *obj) ast_free(args->tls_cfg->cipher); ast_free(args->tls_cfg->cafile); ast_free(args->tls_cfg->capath); + + ast_ssl_teardown(args->tls_cfg); } ast_free(args->tls_cfg); ast_free((char *) args->name); @@ -26112,7 +26114,7 @@ create_tcptls_session_fail: ao2_t_ref(ca, -1, "failed to create client, getting rid of client tcptls_session arguments"); } if (s->tcptls_session) { - ast_tcptls_close_session_file(tcptls_session); + ast_tcptls_close_session_file(s->tcptls_session); s->fd = -1; ao2_ref(s->tcptls_session, -1); s->tcptls_session = NULL; @@ -31020,6 +31022,7 @@ static int unload_module(void) if (sip_tls_desc.master) { ast_tcptls_server_stop(&sip_tls_desc); } + ast_ssl_teardown(sip_tls_desc.tls_cfg); /* Kill all existing TCP/TLS threads */ i = ao2_iterator_init(threadt, 0); diff --git a/include/asterisk/tcptls.h b/include/asterisk/tcptls.h index f4b79f4318..e4894f7bbc 100644 --- a/include/asterisk/tcptls.h +++ b/include/asterisk/tcptls.h @@ -190,8 +190,25 @@ void ast_tcptls_server_start(struct ast_tcptls_session_args *desc); * \version 1.6.1 changed desc parameter to be of ast_tcptls_session_args type */ void ast_tcptls_server_stop(struct ast_tcptls_session_args *desc); + +/*! + * \brief Set up an SSL server + * + * \param cfg Configuration for the SSL server + * \retval 1 Success + * \retval 0 Failure + */ int ast_ssl_setup(struct ast_tls_config *cfg); +/*! + * \brief free resources used by an SSL server + * + * \note This only needs to be called if ast_ssl_setup() was + * directly called first. + * \param cfg Configuration for the SSL server + */ +void ast_ssl_teardown(struct ast_tls_config *cfg); + /*! * \brief Used to parse conf files containing tls/ssl options. */ diff --git a/main/ssl.c b/main/ssl.c index 40129f84f5..d6ecdb0028 100644 --- a/main/ssl.c +++ b/main/ssl.c @@ -77,9 +77,7 @@ int ast_ssl_init(void) SSL_library_init(); SSL_load_error_strings(); - ERR_load_crypto_strings(); ERR_load_BIO_strings(); - OpenSSL_add_all_algorithms(); /* Make OpenSSL thread-safe. */ diff --git a/main/tcptls.c b/main/tcptls.c index 7063b0ced1..761518c84f 100644 --- a/main/tcptls.c +++ b/main/tcptls.c @@ -78,6 +78,7 @@ static int ssl_close(void *cookie) { int cookie_fd = SSL_get_fd(cookie); int ret; + if (cookie_fd > -1) { /* * According to the TLS standard, it is acceptable for an application to only send its shutdown @@ -87,6 +88,12 @@ static int ssl_close(void *cookie) if ((ret = SSL_shutdown(cookie)) < 0) { ast_log(LOG_ERROR, "SSL_shutdown() failed: %d\n", SSL_get_error(cookie, ret)); } + + if (!((SSL*)cookie)->server) { + /* For client threads, ensure that the error stack is cleared */ + ERR_remove_state(0); + } + SSL_free(cookie); /* adding shutdown(2) here has no added benefit */ if (close(cookie_fd)) { @@ -308,8 +315,13 @@ static int __ssl_setup(struct ast_tls_config *cfg, int client) if (!cfg->enabled) return 0; - SSL_load_error_strings(); - SSLeay_add_ssl_algorithms(); + /* Get rid of an old SSL_CTX since we're about to + * allocate a new one + */ + if (cfg->ssl_ctx) { + SSL_CTX_free(cfg->ssl_ctx); + cfg->ssl_ctx = NULL; + } if (client) { #ifndef OPENSSL_NO_SSL2 @@ -344,8 +356,9 @@ static int __ssl_setup(struct ast_tls_config *cfg, int client) if (!client) { /* Clients don't need a certificate, but if its setup we can use it */ ast_verb(0, "SSL error loading cert file. <%s>", cfg->certfile); - sleep(2); cfg->enabled = 0; + SSL_CTX_free(cfg->ssl_ctx); + cfg->ssl_ctx = NULL; return 0; } } @@ -353,8 +366,9 @@ static int __ssl_setup(struct ast_tls_config *cfg, int client) if (!client) { /* Clients don't need a private key, but if its setup we can use it */ ast_verb(0, "SSL error loading private key file. <%s>", tmpprivate); - sleep(2); cfg->enabled = 0; + SSL_CTX_free(cfg->ssl_ctx); + cfg->ssl_ctx = NULL; return 0; } } @@ -363,8 +377,9 @@ static int __ssl_setup(struct ast_tls_config *cfg, int client) if (SSL_CTX_set_cipher_list(cfg->ssl_ctx, cfg->cipher) == 0 ) { if (!client) { ast_verb(0, "SSL cipher error <%s>", cfg->cipher); - sleep(2); cfg->enabled = 0; + SSL_CTX_free(cfg->ssl_ctx); + cfg->ssl_ctx = NULL; return 0; } } @@ -384,6 +399,16 @@ int ast_ssl_setup(struct ast_tls_config *cfg) return __ssl_setup(cfg, 0); } +void ast_ssl_teardown(struct ast_tls_config *cfg) +{ +#ifdef DO_SSL + if (cfg->ssl_ctx) { + SSL_CTX_free(cfg->ssl_ctx); + cfg->ssl_ctx = NULL; + } +#endif +} + struct ast_tcptls_session_instance *ast_tcptls_client_start(struct ast_tcptls_session_instance *tcptls_session) { struct ast_tcptls_session_args *desc;