OpenSSL: Fix memory leak during multi-message connections using STARTTLS
authorWolfgang Breyha <wbreyha@gmx.net>
Mon, 19 Feb 2018 18:27:55 +0000 (18:27 +0000)
committerJeremy Harris <jgh146exb@wizmail.org>
Tue, 20 Feb 2018 12:16:36 +0000 (12:16 +0000)
Reported-by: Wolfgang Breyha
Fix-by: Wolfgang Breyha, with additions from Jeremy Harris
doc/doc-txt/ChangeLog
src/src/daemon.c
src/src/deliver.c
src/src/exim.c
src/src/functions.h
src/src/macros.h
src/src/smtp_in.c
src/src/tls-gnu.c
src/src/tls-openssl.c
src/src/transports/smtp.c
src/src/verify.c

index 4febdc9..3fd3f38 100644 (file)
@@ -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
 -----------------
index d032e46..476ed29 100644 (file)
@@ -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. */
index 255b4d9..34f36cd 100644 (file)
@@ -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");
index 870b339..9fceaf5 100644 (file)
@@ -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));
index 8a45ae4..d537ac3 100644 (file)
@@ -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);
index ec913e2..beab72a 100644 (file)
@@ -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 */
index d804bc7..c45e7e2 100644 (file)
@@ -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
 
index 38e8eab..e0ac6a5 100644 (file)
@@ -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);
index 7a6e8bf..fd21adf 100644 (file)
@@ -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;
 }
 
index 38660f7..d3af04c 100644 (file)
@@ -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
index 0d8c970..9582fe5 100644 (file)
@@ -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);