From ae5692994fc31cc5fa25fb5681e59e326e6c5dbe Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 12 Sep 2018 11:02:59 -0400 Subject: [PATCH 1/2] Add a tor_tls_release_socket() function. This function tells the underlying TLS object that it shouldn't close the fd on exit. Mostly, we hope not to have to use it, since the NSS implementation is kludgey, but it should allow us to fix --- src/lib/tls/tortls.h | 1 + src/lib/tls/tortls_nss.c | 37 ++++++++++++++++++++++++++++++++++++ src/lib/tls/tortls_openssl.c | 24 ++++++++++++++++++++++- 3 files changed, 61 insertions(+), 1 deletion(-) diff --git a/src/lib/tls/tortls.h b/src/lib/tls/tortls.h index 4591927081..3f1098bbac 100644 --- a/src/lib/tls/tortls.h +++ b/src/lib/tls/tortls.h @@ -94,6 +94,7 @@ void tor_tls_set_renegotiate_callback(tor_tls_t *tls, void (*cb)(tor_tls_t *, void *arg), void *arg); int tor_tls_is_server(tor_tls_t *tls); +void tor_tls_release_socket(tor_tls_t *tls); void tor_tls_free_(tor_tls_t *tls); #define tor_tls_free(tls) FREE_AND_NULL(tor_tls_t, tor_tls_free_, (tls)) int tor_tls_peer_has_cert(tor_tls_t *tls); diff --git a/src/lib/tls/tortls_nss.c b/src/lib/tls/tortls_nss.c index 53adfedf32..1b2032764d 100644 --- a/src/lib/tls/tortls_nss.c +++ b/src/lib/tls/tortls_nss.c @@ -414,6 +414,43 @@ tor_tls_set_renegotiate_callback(tor_tls_t *tls, /* We don't support renegotiation-based TLS with NSS. */ } +/** + * Tell the TLS library that the underlying socket for tls has been + * closed, and the library should not attempt to free that socket itself. + */ +void +tor_tls_release_socket(tor_tls_t *tls) +{ + if (! tls) + return; + + /* NSS doesn't have the equivalent of BIO_NO_CLOSE. If you replace the + * fd with something that's invalid, it causes a memory leak in PR_Close. + * + * If there were a way to put the PRFileDesc into the CLOSED state, that + * would prevent it from closing its fd -- but there doesn't seem to be a + * supported way to do that either. + * + * So instead: we make a new sacrificial socket, and replace the original + * socket with that one. This seems to be the best we can do, until we + * redesign the mainloop code enough to make this function unnecessary. + */ + tor_socket_t sock = + tor_open_socket_nonblocking(AF_INET, SOCK_STREAM, IPPROTO_TCP); + if (!sock) { + log_warn(LD_NET, "Out of sockets when trying to shut down an NSS " + "connection"); + return; + } + + PRFileDesc *tcp = PR_GetIdentitiesLayer(tls->ssl, PR_NSPR_IO_LAYER); + if (BUG(! tcp)) { + return; + } + + PR_ChangeFileDescNativeHandle(tcp, sock); +} + void tor_tls_impl_free_(tor_tls_impl_t *tls) { diff --git a/src/lib/tls/tortls_openssl.c b/src/lib/tls/tortls_openssl.c index dc6c0bee9c..534a90de5d 100644 --- a/src/lib/tls/tortls_openssl.c +++ b/src/lib/tls/tortls_openssl.c @@ -1048,7 +1048,7 @@ tor_tls_new(tor_socket_t sock, int isServer) goto err; } result->socket = sock; - bio = BIO_new_socket(sock, BIO_NOCLOSE); + bio = BIO_new_socket(sock, BIO_CLOSE); if (! bio) { tls_log_errors(NULL, LOG_WARN, LD_NET, "opening BIO"); #ifdef SSL_set_tlsext_host_name @@ -1154,6 +1154,28 @@ tor_tls_assert_renegotiation_unblocked(tor_tls_t *tls) #endif /* defined(SSL_OP_ALLOW_UNSAFE_LEGACY_RENEGOTIATION) && ... */ } +/** + * Tell the TLS library that the underlying socket for tls has been + * closed, and the library should not attempt to free that socket itself. + */ +void +tor_tls_release_socket(tor_tls_t *tls) +{ + if (! tls) + return; + + BIO *rbio, *wbio; + rbio = SSL_get_rbio(tls->ssl); + wbio = SSL_get_wbio(tls->ssl); + + if (rbio) { + BIO_set_close(rbio, BIO_NOCLOSE); + } + if (wbio && wbio != rbio) { + BIO_set_close(wbio, BIO_NOCLOSE); + } +} + void tor_tls_impl_free_(tor_tls_impl_t *ssl) { From 9f5f67bda26979bb75e10a0ce0080997b1b72603 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 12 Sep 2018 11:32:15 -0400 Subject: [PATCH 2/2] Use tor_tls_release_socket() to avoid double-closed sockets on NSS Closes ticket 27451; bug not in any released Tor. --- src/core/mainloop/connection.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/core/mainloop/connection.c b/src/core/mainloop/connection.c index ffc9010fb8..16ce8b3f2a 100644 --- a/src/core/mainloop/connection.c +++ b/src/core/mainloop/connection.c @@ -638,8 +638,19 @@ connection_free_minimal(connection_t *conn) if (connection_speaks_cells(conn)) { or_connection_t *or_conn = TO_OR_CONN(conn); - tor_tls_free(or_conn->tls); - or_conn->tls = NULL; + if (or_conn->tls) { + if (! SOCKET_OK(conn->s)) { + /* The socket has been closed by somebody else; we must tell the + * TLS object not to close it. */ + tor_tls_release_socket(or_conn->tls); + } else { + /* The tor_tls_free() call below will close the socket; we must tell + * the code below not to close it a second time. */ + conn->s = TOR_INVALID_SOCKET; + } + tor_tls_free(or_conn->tls); + or_conn->tls = NULL; + } or_handshake_state_free(or_conn->handshake_state); or_conn->handshake_state = NULL; tor_free(or_conn->nickname);