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.

4 files changed, 118 insertions, 27 deletions

diff --git a/pcap-rpcap.c b/pcap-rpcap.c
index d4ff3f99..f6cf8172 100644
--- 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 459fdf72..780173b1 100644
--- 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 fba34603..21cb3f90 100644
--- 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 3661a3ce..6316364e 100644
--- 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);