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 373061 from http://svn.asterisk.org/svn/asterisk/branches/1.8
........

Merged revisions 373062 from http://svn.asterisk.org/svn/asterisk/branches/10

git-svn-id: https://origsvn.digium.com/svn/asterisk/branches/11@373079 65c4cc65-6c06-0410-ace0-fbb531ad65f3

diff --git a/channels/chan_sip.c b/channels/chan_sip.c
index bbd5942b0ead08399247e09e4c001dea4547b4ee..1a77dd4939a9924bc2c4a37dace5b1dd25e263be 100644 (file)
--- a/channels/chan_sip.c
+++ b/channels/chan_sip.c
@@ -2448,6 +2448,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);

diff --git a/main/libasteriskssl.c b/main/libasteriskssl.c
index 361875aa1276c1f2bf78acff624a5aeda07f4922..ca3fb569c38a9eb4991ee49507d7c4d17b6acb22 100644 (file)
--- a/main/libasteriskssl.c
+++ b/main/libasteriskssl.c
@@ -158,7 +158,6 @@ int ast_ssl_init(void)
        void (*real_CRYPTO_set_locking_callback)(void (*)(int, int, const char *, int));
        void (*real_SSL_load_error_strings)(void);
        void (*real_ERR_load_SSL_strings)(void);
-       void (*real_ERR_load_crypto_strings)(void);
        void (*real_ERR_load_BIO_strings)(void);
        const char *errstr;
 
@@ -220,17 +219,9 @@ int ast_ssl_init(void)
        get_OpenSSL_function(ERR_load_SSL_strings);
        real_ERR_load_SSL_strings();
 
-       get_OpenSSL_function(ERR_load_crypto_strings);
-       real_ERR_load_crypto_strings();
-
        get_OpenSSL_function(ERR_load_BIO_strings);
        real_ERR_load_BIO_strings();
 
-#if 0
-       /* currently this is just another call to SSL_library_init, so we don't call it */
-       OpenSSL_add_all_algorithms();
-#endif
-
        startup_complete = 1;
 
 #endif /* HAVE_OPENSSL */

diff --git a/main/tcptls.c b/main/tcptls.c
index 2ad3a10e481d8813bf05812e154975c648b1fb7e..2e5cbf4c6af039b1d9494179391fbc16d8825ada 100644 (file)
--- a/main/tcptls.c
+++ b/main/tcptls.c
@@ -84,6 +84,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
@@ -93,6 +94,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)) {
@@ -320,9 +327,6 @@ static int __ssl_setup(struct ast_tls_config *cfg, int client)
                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
         */
@@ -364,7 +368,6 @@ 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>\n", cfg->certfile);
-                               sleep(2);
                                cfg->enabled = 0;
                                SSL_CTX_free(cfg->ssl_ctx);
                                cfg->ssl_ctx = NULL;
@@ -375,7 +378,6 @@ 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>\n", tmpprivate);
-                               sleep(2);
                                cfg->enabled = 0;
                                SSL_CTX_free(cfg->ssl_ctx);
                                cfg->ssl_ctx = NULL;
@@ -387,7 +389,6 @@ 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>\n", cfg->cipher);
-                               sleep(2);
                                cfg->enabled = 0;
                                SSL_CTX_free(cfg->ssl_ctx);
                                cfg->ssl_ctx = NULL;