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 4febdc93ec5c53c37ca5238b19a7d54a61ecc301..3fd3f384f1263d5bd8555ecd7f7b9f2411c155bf 100644 (file)
--- 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 d032e467a9f7828e4d5777a665f1d52b28f2cefa..476ed296b61e57f62b7a801c1f18f788a34c8246 100644 (file)
--- 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 255b4d9c9476db82df36c610fce50e0087e5540a..34f36cd334c0c8e477e70cfdcd85815a909243a1 100644 (file)
--- 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 870b33949b357fbcde9e3400ecd4246a78bf3107..9fceaf524d73ae2da75ca1f1fd5ebe97d0066c28 100644 (file)
--- 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 8a45ae48d7489569b277557caace5f9dc476e587..d537ac33134c4e381672947e82e1d8798391b443 100644 (file)
--- 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 ec913e24e1f7086e384dddefddac9888eb8090c3..beab72a28433023ffa057372e70482a802531825 100644 (file)
--- 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 d804bc7d262c918d3741d577d5856619f15884f2..c45e7e26f5ef19f1453fed32dedc9ffa89f0e3e0 100644 (file)
--- 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 38e8eab09a3efc451a0579aa56dac61cd326846b..e0ac6a546dc858561a73a51f82612a95b6adce23 100644 (file)
--- 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 7a6e8bfdf79c57809aa9dde9f0b966877e67d86b..fd21adfa5b6718f6815df9689ee77c82698bf439 100644 (file)
--- 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 38660f79718aeb867e9eae9c299c6a1d627be0a8..d3af04cc8ffa8820aa17381a68227096c9fa4b23 100644 (file)
--- 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 0d8c97097641772baff13d3d6d83b2c778e956a4..9582fe5b7407b1f1002c0a7f7625bb137148dd70 100644 (file)
--- 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);