From: Wolfgang Breyha <wbreyha@gmx.net>
Date: Mon, 19 Feb 2018 18:27:55 +0000 (+0000)
Subject: OpenSSL: Fix memory leak during multi-message connections using STARTTLS
X-Git-Url: https://git.exim.org/users/jgh/exim.git/commitdiff_plain/dec766a1977250758eb7a3e127e079a9271afd89

OpenSSL: Fix memory leak during multi-message connections using STARTTLS

Reported-by: Wolfgang Breyha
Fix-by: Wolfgang Breyha, with additions from Jeremy Harris
---

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);