Shut down SSL sessions semi-gracefully.

Before we shut down the socket, send a shutdown alert.  That should
prevent some cases where errors are reported when they shouldn't be (it
was happening if I did a --list-remote-interfaces in tcpdump).

While we're at it, do the SSL shutdown *before* closing the main active
socket; we were doing it *after*.  Also, fix a comment.

diff --git a/pcap-rpcap.c b/pcap-rpcap.c
index d4ff3f9925918c48b806bb1c5dc065cd6a3cc6b8..f6cf8172b72b95e7ceb12b0641612f3f05dab7bb 100644 (file)
--- a/pcap-rpcap.c
+++ b/pcap-rpcap.c
@@ -694,7 +694,9 @@ static int pcap_read_rpcap(pcap_t *p, int cnt, pcap_handler callback, u_char *us
 }
 
 /*
- * This function sends a CLOSE command to the capture server.
+ * This function sends a CLOSE command to the capture server if we're in
+ * passive mode and an ENDCAP command to the capture server if we're in
+ * active mode.
  *
  * It is called when the user calls pcap_close().  It sends a command
  * to our peer that says 'ok, let's stop capturing'.
@@ -766,7 +768,9 @@ static void pcap_cleanup_rpcap(pcap_t *fp)
 #ifdef HAVE_OPENSSL
                if (pr->data_ssl)
                {
-                       SSL_free(pr->data_ssl); // Has to be done before the socket is closed
+                       // Finish using the SSL handle for the data socket.
+                       // This must be done *before* the socket is closed.
+                       ssl_finish(pr->data_ssl);
                        pr->data_ssl = NULL;
                }
 #endif
@@ -779,7 +783,9 @@ static void pcap_cleanup_rpcap(pcap_t *fp)
 #ifdef HAVE_OPENSSL
                if (pr->ctrl_ssl)
                {
-                       SSL_free(pr->ctrl_ssl);
+                       // Finish using the SSL handle for the control socket.
+                       // This must be done *before* the socket is closed.
+                       ssl_finish(pr->ctrl_ssl);
                        pr->ctrl_ssl = NULL;
                }
 #endif
@@ -1426,7 +1432,9 @@ error_nodiscard:
 #ifdef HAVE_OPENSSL
        if (pr->data_ssl)
        {
-               SSL_free(pr->data_ssl);  // Have to be done before the socket is closed
+               // Finish using the SSL handle for the data socket.
+               // This must be done *before* the socket is closed.
+               ssl_finish(pr->data_ssl);
                pr->data_ssl = NULL;
        }
 #endif
@@ -1439,7 +1447,9 @@ error_nodiscard:
 #ifdef HAVE_OPENSSL
                if (pr->ctrl_ssl)
                {
-                       SSL_free(pr->ctrl_ssl);
+                       // Finish using the SSL handle for the control socket.
+                       // This must be done *before* the socket is closed.
+                       ssl_finish(pr->ctrl_ssl);
                        pr->ctrl_ssl = NULL;
                }
 #endif
@@ -2414,7 +2424,12 @@ error_nodiscard:
        if (!active)
        {
 #ifdef HAVE_OPENSSL
-               if (ssl) SSL_free(ssl);  // Have to be done before the socket is closed
+               if (ssl)
+               {
+                       // Finish using the SSL handle for the socket.
+                       // This must be done *before* the socket is closed.
+                       ssl_finish(ssl);
+               }
 #endif
                sock_close(sockctrl, NULL, 0);
        }
@@ -2543,7 +2558,13 @@ pcap_findalldevs_ex_remote(const char *source, struct pcap_rmtauth *auth, pcap_i
                if (rpcap_doauth(sockctrl, ssl, &protocol_version, auth, errbuf) == -1)
                {
 #ifdef HAVE_OPENSSL
-                       if (ssl) SSL_free(ssl); // Must be done before the socket is closed
+                       if (ssl)
+                       {
+                               // Finish using the SSL handle for the socket.
+                               // This must be done *before* the socket is
+                               // closed.
+                               ssl_finish(ssl);
+                       }
 #endif
                        sock_close(sockctrl, NULL, 0);
                        return -1;
@@ -2776,7 +2797,12 @@ pcap_findalldevs_ex_remote(const char *source, struct pcap_rmtauth *auth, pcap_i
        {
                /* DO not send RPCAP_CLOSE, since we did not open a pcap_t; no need to free resources */
 #ifdef HAVE_OPENSSL
-               if (ssl) SSL_free(ssl); // Has to be done before the socket is closed
+               if (ssl)
+               {
+                       // Finish using the SSL handle for the socket.
+                       // This must be done *before* the socket is closed.
+                       ssl_finish(ssl);
+               }
 #endif
                if (sock_close(sockctrl, errbuf, PCAP_ERRBUF_SIZE))
                        return -1;
@@ -2808,7 +2834,12 @@ error_nodiscard:
        if (!active)
        {
 #ifdef HAVE_OPENSSL
-               if (ssl) SSL_free(ssl); // Has to be done before the socket is closed
+               if (ssl)
+               {
+                       // Finish using the SSL handle for the socket.
+                       // This must be done *before* the socket is closed.
+                       ssl_finish(ssl);
+               }
 #endif
                sock_close(sockctrl, NULL, 0);
        }
@@ -2918,7 +2949,12 @@ SOCKET pcap_remoteact_accept_ex(const char *address, const char *port, const cha
                sock_geterror("getnameinfo(): ", errbuf, PCAP_ERRBUF_SIZE);
                rpcap_senderror(sockctrl, ssl, 0, PCAP_ERR_REMOTEACCEPT, errbuf, NULL);
 #ifdef HAVE_OPENSSL
-               if (ssl) SSL_free(ssl);
+               if (ssl)
+               {
+                       // Finish using the SSL handle for the socket.
+                       // This must be done *before* the socket is closed.
+                       ssl_finish(ssl);
+               }
 #endif
                sock_close(sockctrl, NULL, 0);
                return (SOCKET)-1;
@@ -2929,7 +2965,12 @@ SOCKET pcap_remoteact_accept_ex(const char *address, const char *port, const cha
        {
                rpcap_senderror(sockctrl, ssl, 0, PCAP_ERR_REMOTEACCEPT, errbuf, NULL);
 #ifdef HAVE_OPENSSL
-               if (ssl) SSL_free(ssl);
+               if (ssl)
+               {
+                       // Finish using the SSL handle for the socket.
+                       // This must be done *before* the socket is closed.
+                       ssl_finish(ssl);
+               }
 #endif
                sock_close(sockctrl, NULL, 0);
                return (SOCKET)-1;
@@ -2943,7 +2984,12 @@ SOCKET pcap_remoteact_accept_ex(const char *address, const char *port, const cha
                /* Unrecoverable error. */
                rpcap_senderror(sockctrl, ssl, 0, PCAP_ERR_REMOTEACCEPT, errbuf, NULL);
 #ifdef HAVE_OPENSSL
-               if (ssl) SSL_free(ssl);
+               if (ssl)
+               {
+                       // Finish using the SSL handle for the socket.
+                       // This must be done *before* the socket is closed.
+                       ssl_finish(ssl);
+               }
 #endif
                sock_close(sockctrl, NULL, 0);
                return (SOCKET)-3;
@@ -2983,7 +3029,12 @@ SOCKET pcap_remoteact_accept_ex(const char *address, const char *port, const cha
                    errno, "malloc() failed");
                rpcap_senderror(sockctrl, ssl, protocol_version, PCAP_ERR_REMOTEACCEPT, errbuf, NULL);
 #ifdef HAVE_OPENSSL
-               if (ssl) SSL_free(ssl);
+               if (ssl)
+               {
+                       // Finish using the SSL handle for the socket.
+                       // This must be done *before* the socket is closed.
+                       ssl_finish(ssl);
+               }
 #endif
                sock_close(sockctrl, NULL, 0);
                return (SOCKET)-1;
@@ -3053,7 +3104,14 @@ int pcap_remoteact_close(const char *host, char *errbuf)
                                         * report.
                                         */
 #ifdef HAVE_OPENSSL
-                                       if (temp->ssl) SSL_free(temp->ssl);
+                                       if (temp->ssl)
+                                       {
+                                               // Finish using the SSL handle
+                                               // for the socket.
+                                               // This must be done *before*
+                                               // the socket is closed.
+                                               ssl_finish(temp->ssl);
+                                       }
 #endif
                                        (void)sock_close(temp->sockctrl, NULL,
                                           0);
@@ -3062,7 +3120,14 @@ int pcap_remoteact_close(const char *host, char *errbuf)
                                else
                                {
 #ifdef HAVE_OPENSSL
-                                       if (temp->ssl) SSL_free(temp->ssl);
+                                       if (temp->ssl)
+                                       {
+                                               // Finish using the SSL handle
+                                               // for the socket.
+                                               // This must be done *before*
+                                               // the socket is closed.
+                                               ssl_finish(temp->ssl);
+                                       }
 #endif
                                        if (sock_close(temp->sockctrl, errbuf,
                                           PCAP_ERRBUF_SIZE) == -1)
@@ -3106,6 +3171,16 @@ int pcap_remoteact_close(const char *host, char *errbuf)
 
 void pcap_remoteact_cleanup(void)
 {
+#      ifdef HAVE_OPENSSL
+       if (ssl_main)
+       {
+               // Finish using the SSL handle for the main active socket.
+               // This must be done *before* the socket is closed.
+               ssl_finish(ssl_main);
+               ssl_main = NULL;
+       }
+#      endif
+
        /* Very dirty, but it works */
        if (sockmain)
        {
@@ -3114,14 +3189,6 @@ void pcap_remoteact_cleanup(void)
                /* To avoid inconsistencies in the number of sock_init() */
                sock_cleanup();
        }
-
-#      ifdef HAVE_OPENSSL
-       if (ssl_main)
-       {
-               SSL_free(ssl_main);
-               ssl_main = NULL;
-       }
-#      endif
 }
 
 int pcap_remoteact_list(char *hostlist, char sep, int size, char *errbuf)

diff --git a/rpcapd/daemon.c b/rpcapd/daemon.c
index 459fdf7224d288a88ade90100a75fba74f755f18..780173b18d69e39f25200ddb0f588619d177aefe 100644 (file)
--- a/rpcapd/daemon.c
+++ b/rpcapd/daemon.c
@@ -1103,13 +1103,15 @@ end:
        }
 
        //
-       // Free the SSL handle for the control socket, if we have one,
-       // and close the control socket.
+       // Finish using the SSL handle for the control socket, if we
+       // have an SSL connection, and close the control socket.
        //
 #ifdef HAVE_OPENSSL
        if (ssl)
        {
-               SSL_free(ssl);
+               // Finish using the SSL handle for the socket.
+               // This must be done *before* the socket is closed.
+               ssl_finish(ssl);
        }
 #endif
        sock_close(sockctrl, NULL, 0);
@@ -2844,7 +2846,9 @@ static void session_close(struct session *session)
 #ifdef HAVE_OPENSSL
        if (session->data_ssl)
        {
-               SSL_free(session->data_ssl); // Must happen *before* the socket is closed
+               // Finish using the SSL handle for the socket.
+               // This must be done *before* the socket is closed.
+               ssl_finish(session->data_ssl);
                session->data_ssl = NULL;
        }
 #endif

diff --git a/sslutils.c b/sslutils.c
index fba34603747f5770f2e62875e28d0175e611c100..21cb3f905a2fd3c6df1afab63336064e6ef8c00c 100644 (file)
--- a/sslutils.c
+++ b/sslutils.c
@@ -159,6 +159,25 @@ SSL *ssl_promotion(int is_server, SOCKET s, char *errbuf, size_t errbuflen)
        return ssl;
 }
 
+// Finish using an SSL handle; shut down the connection and free the
+// handle.
+void ssl_finish(SSL *ssl)
+{
+       //
+       // We won't be using this again, so we can just send the
+       // shutdown alert and free up the handle, and have our
+       // caller close the socket.
+       //
+       // XXX - presumably, if the connection is shut down on
+       // our side, either our peer won't have a problem sending
+       // their shutdown alert or will not treat such a problem
+       // as an error.  If this causes errors to be reported,
+       // fix that as appropriate.
+       //
+       SSL_shutdown(ssl);
+       SSL_free(ssl);
+}
+
 // Same return value as sock_send:
 // 0 on OK, -1 on error but closed connection (-2).
 int ssl_send(SSL *ssl, char const *buffer, int size, char *errbuf, size_t errbuflen)

diff --git a/sslutils.h b/sslutils.h
index 3661a3ce92a09407c1397f8ea2de06822f3b404a..6316364ecfc298536e1d2b606a4bc450ae55f782 100644 (file)
--- a/sslutils.h
+++ b/sslutils.h
@@ -46,6 +46,7 @@ void ssl_set_certfile(const char *certfile);
 void ssl_set_keyfile(const char *keyfile);
 int ssl_init_once(int is_server, int enable_compression, char *errbuf, size_t errbuflen);
 SSL *ssl_promotion(int is_server, SOCKET s, char *errbuf, size_t errbuflen);
+void ssl_finish(SSL *ssl);
 int ssl_send(SSL *, char const *buffer, int size, char *errbuf, size_t errbuflen);
 int ssl_recv(SSL *, char *buffer, int size, char *errbuf, size_t errbuflen);