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.
 
       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
 -----------------
 
 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
         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. */
 #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
   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");
 #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
 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));
   #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 **);
                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);
 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"
 
 
 #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 */
 
 /* 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
   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",
 #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;
       }
        smtp_printf("554 Security failure\r\n", FALSE);
        break;
       }

-    tls_close(TRUE, TRUE);
+    tls_close(TRUE, TLS_SHUTDOWN_NOWAIT);

     break;
     #endif
 
     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).
 
 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
 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;
 
 {
 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)
   {
 
 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);
   }
 
 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;
   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 {
 #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->certificate = certificate;
 cbinfo->privatekey = privatekey;
 cbinfo->is_server = host==NULL;

+cbinfo->acceptable_certnames = NULL;

 #ifndef DISABLE_OCSP
 cbinfo->verify_stack = NULL;
 if (!host)
 #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;
 
 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);
 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
 
 Arguments:
   sctx          SSL_CTX* to initialise


@@ -1813,6 +1819,7 @@ if (expcerts && *expcerts)
        { file = NULL; dir = expcerts; }
       else
        {
        { 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
        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)
        {
       */
       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));
        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;
 
   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_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;
   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).
 
 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
 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;
 
 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)
   {
 
 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);
 SSL_free(*sslp);

+*ctxp = NULL;

 *sslp = NULL;
 *sslp = NULL;

-

 *fdp = -1;
 }
 
 *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
   (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
 #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;
     if ((rc = read(pfd[0], buf, bsize)) <= 0)
       {
       fd_bits = 0;

-      tls_close(FALSE, TRUE);
+      tls_close(FALSE, TLS_SHUTDOWN_NOWAIT);

       }
     else
       {
       }
     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. */
 
          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,
          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);
            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;
            (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
 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
 #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);
     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
       }
     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
              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);
 #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
       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);
 #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
   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);
 #endif
   HDEBUG(D_transport|D_acl|D_v) debug_printf_indent("  SMTP(close)>>\n");
   (void)close(fd);