From dec766a1977250758eb7a3e127e079a9271afd89 Mon Sep 17 00:00:00 2001 From: Wolfgang Breyha Date: Mon, 19 Feb 2018 18:27:55 +0000 Subject: [PATCH] OpenSSL: Fix memory leak during multi-message connections using STARTTLS Reported-by: Wolfgang Breyha Fix-by: Wolfgang Breyha, with additions from Jeremy Harris --- doc/doc-txt/ChangeLog | 4 +++ src/src/daemon.c | 2 +- src/src/deliver.c | 2 +- src/src/exim.c | 2 +- src/src/functions.h | 2 +- src/src/macros.h | 5 +++ src/src/smtp_in.c | 4 +-- src/src/tls-gnu.c | 15 ++++++--- src/src/tls-openssl.c | 71 ++++++++++++++++++++++++++++++++++----- src/src/transports/smtp.c | 12 +++---- src/src/verify.c | 6 ++-- 11 files changed, 98 insertions(+), 27 deletions(-) diff --git a/doc/doc-txt/ChangeLog b/doc/doc-txt/ChangeLog index 4febdc93e..3fd3f384f 100644 --- a/doc/doc-txt/ChangeLog +++ b/doc/doc-txt/ChangeLog @@ -114,6 +114,10 @@ JH/22 Bug 2236: When a DKIM verification result is overridden by ACL, DMARC reported the original. Fix to report (as far as possible) the ACL result replacing the original. +JH/23 Fix memory leak during multi-message connections using STARTTLS under + OpenSSL. Certificate information is loaded for every new TLS startup, + and the resources needed to be freed. + Exim version 4.90 ----------------- diff --git a/src/src/daemon.c b/src/src/daemon.c index d032e467a..476ed296b 100644 --- a/src/src/daemon.c +++ b/src/src/daemon.c @@ -653,7 +653,7 @@ if (pid == 0) the data structures if necessary. */ #ifdef SUPPORT_TLS - tls_close(TRUE, FALSE); + tls_close(TRUE, TLS_NO_SHUTDOWN); #endif /* Reset SIGHUP and SIGCHLD in the child in both cases. */ diff --git a/src/src/deliver.c b/src/src/deliver.c index 255b4d9c9..34f36cd33 100644 --- a/src/src/deliver.c +++ b/src/src/deliver.c @@ -4988,7 +4988,7 @@ all pipes, so I do not see a reason to use non-blocking IO here if (cutthrough.fd >= 0 && cutthrough.callout_hold_only) { #ifdef SUPPORT_TLS - tls_close(FALSE, FALSE); + tls_close(FALSE, TLS_NO_SHUTDOWN); #endif (void) close(cutthrough.fd); release_cutthrough_connection(US"passed to transport proc"); diff --git a/src/src/exim.c b/src/src/exim.c index 870b33949..9fceaf524 100644 --- a/src/src/exim.c +++ b/src/src/exim.c @@ -543,7 +543,7 @@ close_unwanted(void) if (smtp_input) { #ifdef SUPPORT_TLS - tls_close(TRUE, FALSE); /* Shut down the TLS library */ + tls_close(TRUE, TLS_NO_SHUTDOWN); /* Shut down the TLS library */ #endif (void)close(fileno(smtp_in)); (void)close(fileno(smtp_out)); diff --git a/src/src/functions.h b/src/src/functions.h index 8a45ae48d..d537ac331 100644 --- a/src/src/functions.h +++ b/src/src/functions.h @@ -50,7 +50,7 @@ extern int tls_client_start(int, host_item *, address_item *, dns_answer *, # endif uschar **); -extern void tls_close(BOOL, BOOL); +extern void tls_close(BOOL, int); extern BOOL tls_could_read(void); extern int tls_export_cert(uschar *, size_t, void *); extern int tls_feof(void); diff --git a/src/src/macros.h b/src/src/macros.h index ec913e24e..beab72a28 100644 --- a/src/src/macros.h +++ b/src/src/macros.h @@ -1027,5 +1027,10 @@ enum { FILTER_UNSET, FILTER_FORWARD, FILTER_EXIM, FILTER_SIEVE }; #define UTF8_VERT_2DASH "\xE2\x95\x8E" +/* Options on tls_close */ +#define TLS_NO_SHUTDOWN 0 +#define TLS_SHUTDOWN_NOWAIT 1 +#define TLS_SHUTDOWN_WAIT 2 + /* End of macros.h */ diff --git a/src/src/smtp_in.c b/src/src/smtp_in.c index d804bc7d2..c45e7e26f 100644 --- a/src/src/smtp_in.c +++ b/src/src/smtp_in.c @@ -3724,7 +3724,7 @@ else smtp_printf("221 %s closing connection\r\n", FALSE, smtp_active_hostname); #ifdef SUPPORT_TLS -tls_close(TRUE, TRUE); +tls_close(TRUE, TLS_SHUTDOWN_NOWAIT); #endif log_write(L_smtp_connection, LOG_MAIN, "%s closed by QUIT", @@ -5413,7 +5413,7 @@ while (done <= 0) smtp_printf("554 Security failure\r\n", FALSE); break; } - tls_close(TRUE, TRUE); + tls_close(TRUE, TLS_SHUTDOWN_NOWAIT); break; #endif diff --git a/src/src/tls-gnu.c b/src/src/tls-gnu.c index 38e8eab09..e0ac6a546 100644 --- a/src/src/tls-gnu.c +++ b/src/src/tls-gnu.c @@ -2442,12 +2442,15 @@ return OK; daemon, to shut down the TLS library, without actually doing a shutdown (which would tamper with the TLS session in the parent process). -Arguments: TRUE if gnutls_bye is to be called +Arguments: + shutdown 1 if TLS close-alert is to be sent, + 2 if also response to be waited for + Returns: nothing */ void -tls_close(BOOL is_server, BOOL shutdown) +tls_close(BOOL is_server, int shutdown) { exim_gnutls_state_st *state = is_server ? &state_server : &state_client; @@ -2455,8 +2458,12 @@ if (!state->tlsp || state->tlsp->active < 0) return; /* TLS was not active */ if (shutdown) { - DEBUG(D_tls) debug_printf("tls_close(): shutting down TLS\n"); - gnutls_bye(state->session, GNUTLS_SHUT_WR); + DEBUG(D_tls) debug_printf("tls_close(): shutting down TLS%s\n", + shutdown > 1 ? " (with response-wait)" : ""); + + alarm(2); + gnutls_bye(state->session, shutdown > 1 ? GNUTLS_SHUT_RDWR : GNUTLS_SHUT_WR); + alarm(0); } gnutls_deinit(state->session); diff --git a/src/src/tls-openssl.c b/src/src/tls-openssl.c index 7a6e8bfdf..fd21adfa5 100644 --- a/src/src/tls-openssl.c +++ b/src/src/tls-openssl.c @@ -152,6 +152,7 @@ typedef struct tls_ext_ctx_cb { uschar *certificate; uschar *privatekey; BOOL is_server; + STACK_OF(X509_NAME) * acceptable_certnames; #ifndef DISABLE_OCSP STACK_OF(X509) *verify_stack; /* chain for verifying the proof */ union { @@ -1510,6 +1511,7 @@ cbinfo = store_malloc(sizeof(tls_ext_ctx_cb)); cbinfo->certificate = certificate; cbinfo->privatekey = privatekey; cbinfo->is_server = host==NULL; +cbinfo->acceptable_certnames = NULL; #ifndef DISABLE_OCSP cbinfo->verify_stack = NULL; if (!host) @@ -1754,6 +1756,9 @@ chain_from_pem_file(const uschar * file, STACK_OF(X509) * verify_stack) BIO * bp; X509 * x; +while (sk_X509_num(verify_stack) > 0) + X509_free(sk_X509_pop(verify_stack)); + if (!(bp = BIO_new_file(CS file, "r"))) return FALSE; while ((x = PEM_read_bio_X509(bp, NULL, 0, NULL))) sk_X509_push(verify_stack, x); @@ -1763,7 +1768,8 @@ return TRUE; -/* Called by both client and server startup +/* Called by both client and server startup; on the server possibly +repeated after a Server Name Indication. Arguments: sctx SSL_CTX* to initialise @@ -1813,6 +1819,7 @@ if (expcerts && *expcerts) { file = NULL; dir = expcerts; } else { + /*XXX somewhere down here we leak memory per-STARTTLS, on a multi-message conn, server-side */ file = expcerts; dir = NULL; #ifndef DISABLE_OCSP /* In the server if we will be offering an OCSP proof, load chain from @@ -1853,11 +1860,21 @@ if (expcerts && *expcerts) */ if (file) { - STACK_OF(X509_NAME) * names = SSL_load_client_CA_file(CS file); + tls_ext_ctx_cb * cbinfo = host + ? client_static_cbinfo : server_static_cbinfo; + STACK_OF(X509_NAME) * names; + if ((names = cbinfo->acceptable_certnames)) + { + sk_X509_NAME_pop_free(names, X509_NAME_free); + cbinfo->acceptable_certnames = NULL; + } + names = SSL_load_client_CA_file(CS file); + + SSL_CTX_set_client_CA_list(sctx, names); DEBUG(D_tls) debug_printf("Added %d certificate authorities.\n", sk_X509_NAME_num(names)); - SSL_CTX_set_client_CA_list(sctx, names); + cbinfo->acceptable_certnames = names; } } } @@ -2468,7 +2485,16 @@ if (error == SSL_ERROR_ZERO_RETURN) receive_ferror = smtp_ferror; receive_smtp_buffered = smtp_buffered; + if (SSL_get_shutdown(server_ssl) == SSL_RECEIVED_SHUTDOWN) + SSL_shutdown(server_ssl); + + sk_X509_pop_free(server_static_cbinfo->verify_stack, X509_free); + sk_X509_NAME_pop_free(server_static_cbinfo->acceptable_certnames, X509_NAME_free); SSL_free(server_ssl); + SSL_CTX_free(server_ctx); + server_static_cbinfo->verify_stack = NULL; + server_static_cbinfo->acceptable_certnames = NULL; + server_ctx = NULL; server_ssl = NULL; tls_in.active = -1; tls_in.bits = 0; @@ -2702,15 +2728,19 @@ return len; daemon, to shut down the TLS library, without actually doing a shutdown (which would tamper with the SSL session in the parent process). -Arguments: TRUE if SSL_shutdown is to be called +Arguments: + shutdown 1 if TLS close-alert is to be sent, + 2 if also response to be waited for + Returns: nothing Used by both server-side and client-side TLS. */ void -tls_close(BOOL is_server, BOOL shutdown) +tls_close(BOOL is_server, int shutdown) { +SSL_CTX **ctxp = is_server ? &server_ctx : &client_ctx; SSL **sslp = is_server ? &server_ssl : &client_ssl; int *fdp = is_server ? &tls_in.active : &tls_out.active; @@ -2718,13 +2748,38 @@ if (*fdp < 0) return; /* TLS was not active */ if (shutdown) { - DEBUG(D_tls) debug_printf("tls_close(): shutting down SSL\n"); - SSL_shutdown(*sslp); + int rc; + DEBUG(D_tls) debug_printf("tls_close(): shutting down TLS%s\n", + shutdown > 1 ? " (with response-wait)" : ""); + + if ( (rc = SSL_shutdown(*sslp)) == 0 /* send "close notify" alert */ + && shutdown > 1) + { + alarm(2); + rc = SSL_shutdown(*sslp); /* wait for response */ + alarm(0); + } + + if (rc < 0) DEBUG(D_tls) + { + ERR_error_string(ERR_get_error(), ssl_errstring); + debug_printf("SSL_shutdown: %s\n", ssl_errstring); + } + } + +if (is_server) + { + sk_X509_pop_free(server_static_cbinfo->verify_stack, X509_free); + sk_X509_NAME_pop_free(server_static_cbinfo->acceptable_certnames, + X509_NAME_free); + server_static_cbinfo->verify_stack = NULL; + server_static_cbinfo->acceptable_certnames = NULL; } +SSL_CTX_free(*ctxp); SSL_free(*sslp); +*ctxp = NULL; *sslp = NULL; - *fdp = -1; } diff --git a/src/src/transports/smtp.c b/src/src/transports/smtp.c index 38660f797..d3af04cc8 100644 --- a/src/src/transports/smtp.c +++ b/src/src/transports/smtp.c @@ -2234,7 +2234,7 @@ if (sx->send_quit) (void)smtp_write_command(&sx->outblock, SCMD_FLUSH, "QUIT\r\n"); #ifdef SUPPORT_TLS -tls_close(FALSE, TRUE); +tls_close(FALSE, TLS_SHUTDOWN_NOWAIT); #endif /* Close the socket, and return the appropriate value, first setting @@ -2668,7 +2668,7 @@ for (fd_bits = 3; fd_bits; ) if ((rc = read(pfd[0], buf, bsize)) <= 0) { fd_bits = 0; - tls_close(FALSE, TRUE); + tls_close(FALSE, TLS_SHUTDOWN_NOWAIT); } else { @@ -3458,7 +3458,7 @@ if (sx.completed_addr && sx.ok && sx.send_quit) a new EHLO. If we don't get a good response, we don't attempt to pass the socket on. */ - tls_close(FALSE, TRUE); + tls_close(FALSE, TLS_SHUTDOWN_WAIT); smtp_peer_options = smtp_peer_options_wrap; sx.ok = !sx.smtps && smtp_write_command(&sx.outblock, SCMD_FLUSH, @@ -3523,7 +3523,7 @@ propagate it from the initial close(pfd[0]); /* tidy the inter-proc to disconn the proxy proc */ waitpid(pid, NULL, 0); - tls_close(FALSE, FALSE); + tls_close(FALSE, TLS_NO_SHUTDOWN); (void)close(sx.inblock.sock); continue_transport = NULL; continue_hostname = NULL; @@ -3569,7 +3569,7 @@ if (sx.send_quit) (void)smtp_write_command(&sx.outblock, SCMD_FLUSH, "QUIT\r\n") END_OFF: #ifdef SUPPORT_TLS -tls_close(FALSE, TRUE); +tls_close(FALSE, TLS_SHUTDOWN_NOWAIT); #endif /* Close the socket, and return the appropriate value, first setting @@ -4541,7 +4541,7 @@ retry_non_continued: if (tls_out.active == fd) { (void) tls_write(FALSE, US"QUIT\r\n", 6, FALSE); - tls_close(FALSE, TRUE); + tls_close(FALSE, TLS_SHUTDOWN_NOWAIT); } else #else diff --git a/src/src/verify.c b/src/src/verify.c index 0d8c97097..9582fe5b7 100644 --- a/src/src/verify.c +++ b/src/src/verify.c @@ -821,7 +821,7 @@ tls_retry_connection: debug_printf_indent("problem after random/rset/mfrom; reopen conn\n"); random_local_part = NULL; #ifdef SUPPORT_TLS - tls_close(FALSE, TRUE); + tls_close(FALSE, TLS_SHUTDOWN_NOWAIT); #endif HDEBUG(D_transport|D_acl|D_v) debug_printf_indent(" SMTP(close)>>\n"); (void)close(sx.inblock.sock); @@ -1088,7 +1088,7 @@ no_conn: if (sx.inblock.sock >= 0) { #ifdef SUPPORT_TLS - tls_close(FALSE, TRUE); + tls_close(FALSE, TLS_SHUTDOWN_NOWAIT); #endif HDEBUG(D_transport|D_acl|D_v) debug_printf_indent(" SMTP(close)>>\n"); (void)close(sx.inblock.sock); @@ -1389,7 +1389,7 @@ if(fd >= 0) cutthrough_response(fd, '2', NULL, 1); #ifdef SUPPORT_TLS - tls_close(FALSE, TRUE); + tls_close(FALSE, TLS_SHUTDOWN_NOWAIT); #endif HDEBUG(D_transport|D_acl|D_v) debug_printf_indent(" SMTP(close)>>\n"); (void)close(fd); -- 2.30.2