From: Guy Harris Date: Tue, 29 Jan 2019 22:37:21 +0000 (-0800) Subject: Shut down SSL sessions semi-gracefully. X-Git-Tag: libpcap-1.10-bp~597 X-Git-Url: https://git.tcpdump.org/libpcap/commitdiff_plain/45579c945ae1294260130c18c6892e1b95ce64a9 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 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);