From da5cda77616fc85286d07ae5eaa858366e3d37fe Mon Sep 17 00:00:00 2001 From: Mark Hills Date: Wed, 3 Feb 2021 15:42:10 +0000 Subject: [PATCH] Do not take a copy of the SSL context When SSL certificates are renewed during runtime (via SIGUSR2), e->dtls_ctx is replaced with a context based on the new certificate. But this code continues to operate on its own borrowed pointer. This is clearly visible using valgrind, but the bug is subtle and not always noticed at runtime, possibly due to some fortunate re-use of memory. At the point of SSL_new(): ==28413== Thread 5: ==28413== Invalid read of size 8 ==28413== at 0x4F6198F: SSL_new (in /lib/libssl.so.1.1) ==28413== by 0x137A72: dtls_server_input_handler (dtls_listener.c:291) ==28413== by 0x137A72: handle_udp_packet (dtls_listener.c:443) ==28413== by 0x138153: udp_server_input_handler (dtls_listener.c:728) ==28413== by 0x4FC499E: ??? (in /usr/lib/libevent_core-2.1.so.7.0.0) ==28413== by 0x4FC50AF: event_base_loop (in /usr/lib/libevent_core-2.1.so.7.0.0) ==28413== by 0x121F34: run_events (netengine.c:1579) ==28413== by 0x121F34: run_general_relay_thread (netengine.c:1707) ==28413== by 0x40517B6: start (pthread_create.c:195) ==28413== by 0x40538EF: ??? (clone.s:22) ==28413== Address 0x49a75e0 is 0 bytes inside a block of size 1,024 free'd ==28413== at 0x48A074F: free (vg_replace_malloc.c:540) ==28413== by 0x4F5F6F1: SSL_CTX_free (in /lib/libssl.so.1.1) ==28413== by 0x11CEC4: set_ctx (mainrelay.c:3104) ==28413== by 0x11D233: openssl_load_certificates (mainrelay.c:3173) ==28413== by 0x11D328: reload_ssl_certs (mainrelay.c:3190) ==28413== by 0x4FC4601: ??? (in /usr/lib/libevent_core-2.1.so.7.0.0) ==28413== by 0x4FC50AF: event_base_loop (in /usr/lib/libevent_core-2.1.so.7.0.0) ==28413== by 0x122582: run_events (netengine.c:1579) ==28413== by 0x122582: run_listener_server (netengine.c:1603) ==28413== by 0x110BB8: main (mainrelay.c:2536) ==28413== Block was alloc'd at ==28413== at 0x489F72A: malloc (vg_replace_malloc.c:309) ==28413== by 0x4DFA2C6: CRYPTO_zalloc (in /lib/libcrypto.so.1.1) ==28413== by 0x4F5F79E: SSL_CTX_new (in /lib/libssl.so.1.1) ==28413== by 0x11CA80: set_ctx (mainrelay.c:2875) ==28413== by 0x11D233: openssl_load_certificates (mainrelay.c:3173) ==28413== by 0x110A19: openssl_setup (mainrelay.c:3139) ==28413== by 0x110A19: main (mainrelay.c:2396) ==28413== --- src/apps/relay/dtls_listener.c | 26 ++++++-------------------- 1 file changed, 6 insertions(+), 20 deletions(-) diff --git a/src/apps/relay/dtls_listener.c b/src/apps/relay/dtls_listener.c index 3105638..9a5698c 100644 --- a/src/apps/relay/dtls_listener.c +++ b/src/apps/relay/dtls_listener.c @@ -55,12 +55,6 @@ struct dtls_listener_relay_server_info { ioa_engine_handle e; turn_turnserver *ts; int verbose; -#if DTLS_SUPPORTED - SSL_CTX *dtls_ctx; -#if DTLSv1_2_SUPPORTED - SSL_CTX *dtls_ctx_v1_2; -#endif -#endif struct event *udp_listen_ev; ioa_socket_handle udp_listen_s; ur_addr_map *children_ss; /* map of socket children on remote addr */ @@ -288,13 +282,13 @@ static ioa_socket_handle dtls_server_input_handler(dtls_listener_relay_server_ty #if DTLSv1_2_SUPPORTED if(get_dtls_version(ioa_network_buffer_data(nbh), (int)ioa_network_buffer_get_size(nbh)) == 1) { - connecting_ssl = SSL_new(server->dtls_ctx_v1_2); + connecting_ssl = SSL_new(server->e->dtls_ctx_v1_2); } else { - connecting_ssl = SSL_new(server->dtls_ctx); + connecting_ssl = SSL_new(server->e->dtls_ctx); } #else { - connecting_ssl = SSL_new(server->dtls_ctx); + connecting_ssl = SSL_new(server->e->dtls_ctx); } #endif @@ -573,13 +567,13 @@ static int create_new_connected_udp_socket( #if DTLSv1_2_SUPPORTED if(get_dtls_version(ioa_network_buffer_data(server->sm.m.sm.nd.nbh), (int)ioa_network_buffer_get_size(server->sm.m.sm.nd.nbh)) == 1) { - connecting_ssl = SSL_new(server->dtls_ctx_v1_2); + connecting_ssl = SSL_new(server->e->dtls_ctx_v1_2); } else { - connecting_ssl = SSL_new(server->dtls_ctx); + connecting_ssl = SSL_new(server->e->dtls_ctx); } #else { - connecting_ssl = SSL_new(server->dtls_ctx); + connecting_ssl = SSL_new(server->e->dtls_ctx); } #endif @@ -912,14 +906,6 @@ static int init_server(dtls_listener_relay_server_type* server, if(!server) return -1; -#if DTLS_SUPPORTED - server->dtls_ctx = e->dtls_ctx; - -#if DTLSv1_2_SUPPORTED - server->dtls_ctx_v1_2 = e->dtls_ctx_v1_2; -#endif -#endif - server->ts = ts; server->connect_cb = send_socket;