From bdf27616ba0ef19a5896f2dd9f7727799930225c Mon Sep 17 00:00:00 2001 From: Mark Hills Date: Wed, 3 Feb 2021 15:37:43 +0000 Subject: [PATCH 1/3] Do not mutate something which the DTLS listener server does not own Multiple DTLS listener servers are created, and server->dtls_ctx is the same object shared between them. Set these callbacks once, and logically this is at the point where the SSL context is created. --- src/apps/relay/dtls_listener.c | 47 ++++++++++++---------------------- src/apps/relay/dtls_listener.h | 4 +++ src/apps/relay/mainrelay.c | 2 ++ 3 files changed, 23 insertions(+), 30 deletions(-) diff --git a/src/apps/relay/dtls_listener.c b/src/apps/relay/dtls_listener.c index 7689a13..3105638 100644 --- a/src/apps/relay/dtls_listener.c +++ b/src/apps/relay/dtls_listener.c @@ -935,36 +935,6 @@ static int init_server(dtls_listener_relay_server_type* server, server->verbose=verbose; server->e = e; - -#if DTLS_SUPPORTED - if(server->dtls_ctx) { - -#if defined(REQUEST_CLIENT_CERT) - /* If client has to authenticate, then */ - SSL_CTX_set_verify(server->dtls_ctx, SSL_VERIFY_PEER | SSL_VERIFY_CLIENT_ONCE, dtls_verify_callback); -#endif - - SSL_CTX_set_read_ahead(server->dtls_ctx, 1); - - SSL_CTX_set_cookie_generate_cb(server->dtls_ctx, generate_cookie); - SSL_CTX_set_cookie_verify_cb(server->dtls_ctx, verify_cookie); - } - -#if DTLSv1_2_SUPPORTED - if(server->dtls_ctx_v1_2) { - - #if defined(REQUEST_CLIENT_CERT) - /* If client has to authenticate, then */ - SSL_CTX_set_verify(server->dtls_ctx_v1_2, SSL_VERIFY_PEER | SSL_VERIFY_CLIENT_ONCE, dtls_verify_callback); - #endif - - SSL_CTX_set_read_ahead(server->dtls_ctx_v1_2, 1); - - SSL_CTX_set_cookie_generate_cb(server->dtls_ctx_v1_2, generate_cookie); - SSL_CTX_set_cookie_verify_cb(server->dtls_ctx_v1_2, verify_cookie); - } -#endif -#endif return create_server_socket(server, report_creation); } @@ -980,6 +950,23 @@ static int clean_server(dtls_listener_relay_server_type* server) { /////////////////////////////////////////////////////////// +#if DTLS_SUPPORTED +void setup_dtls_callbacks(SSL_CTX *ctx) { + if (!ctx) + return; + +#if defined(REQUEST_CLIENT_CERT) + /* If client has to authenticate, then */ + SSL_CTX_set_verify(ctx, SSL_VERIFY_PEER | SSL_VERIFY_CLIENT_ONCE, dtls_verify_callback); +#endif + + SSL_CTX_set_read_ahead(ctx, 1); + + SSL_CTX_set_cookie_generate_cb(ctx, generate_cookie); + SSL_CTX_set_cookie_verify_cb(ctx, verify_cookie); +} +#endif + dtls_listener_relay_server_type* create_dtls_listener_server(const char* ifname, const char *local_address, int port, diff --git a/src/apps/relay/dtls_listener.h b/src/apps/relay/dtls_listener.h index 9d7cab6..5ca4ec9 100644 --- a/src/apps/relay/dtls_listener.h +++ b/src/apps/relay/dtls_listener.h @@ -50,6 +50,10 @@ typedef struct dtls_listener_relay_server_info dtls_listener_relay_server_type; /////////////////////////////////////////// +#if DTLS_SUPPORTED +void setup_dtls_callbacks(SSL_CTX *ctx); +#endif + dtls_listener_relay_server_type* create_dtls_listener_server(const char* ifname, const char *local_address, int port, diff --git a/src/apps/relay/mainrelay.c b/src/apps/relay/mainrelay.c index d11a2cd..9d95186 100644 --- a/src/apps/relay/mainrelay.c +++ b/src/apps/relay/mainrelay.c @@ -3198,10 +3198,12 @@ static void openssl_load_certificates(void) set_ctx(&turn_params.dtls_ctx,"DTLS",DTLS_server_method()); set_ctx(&turn_params.dtls_ctx_v1_2,"DTLS1.2",DTLSv1_2_server_method()); SSL_CTX_set_read_ahead(turn_params.dtls_ctx_v1_2, 1); + setup_dtls_callbacks(turn_params.dtls_ctx_v1_2); #else set_ctx(&turn_params.dtls_ctx,"DTLS",DTLSv1_server_method()); #endif SSL_CTX_set_read_ahead(turn_params.dtls_ctx, 1); + setup_dtls_callbacks(turn_params.dtls_ctx); TURN_LOG_FUNC(TURN_LOG_LEVEL_INFO, "DTLS cipher suite: %s\n",turn_params.cipher_list); From da5cda77616fc85286d07ae5eaa858366e3d37fe Mon Sep 17 00:00:00 2001 From: Mark Hills Date: Wed, 3 Feb 2021 15:42:10 +0000 Subject: [PATCH 2/3] 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; From 8f1908d7bd75bb08858e29486df616b56c0a115a Mon Sep 17 00:00:00 2001 From: Mark Hills Date: Wed, 3 Feb 2021 16:39:06 +0000 Subject: [PATCH 3/3] A use-after-free can occur on the SSL_ctx on a busy system When openssl_load_certificates() is called as a result of USR2 signal, it has the effect of SSL_free() on certificates. But pointers to these certificates are borrowed by the ioa_engines where they are used for new connections. The tls_mutex when loading the certificates does not prevent this use because it's released before despatching asynchronous events to each ioa_engine asking them to pick up the new SSL context. So there is a race; if a new connection arrives quickly after openssl_load_certificates() but before the tls_ctx_update_ev. This patch resolves this using OpenSSL's own fine grained locking. The ioa_engines now 'copy' the SSL context (actually a refcounted copy) --- src/apps/relay/netengine.c | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/src/apps/relay/netengine.c b/src/apps/relay/netengine.c index fa2be3f..e118c27 100644 --- a/src/apps/relay/netengine.c +++ b/src/apps/relay/netengine.c @@ -304,25 +304,38 @@ typedef struct update_ssl_ctx_cb_args { struct event *next; } update_ssl_ctx_cb_args_t; +static void replace_one_ssl_ctx(SSL_CTX **to, SSL_CTX *from) +{ + if (*to) + SSL_CTX_free(*to); + + SSL_CTX_up_ref(from); + *to = from; +} + +/* + * Synchronise the ioa_engine's SSL certificates with the global ones + */ static void update_ssl_ctx(evutil_socket_t sock, short events, update_ssl_ctx_cb_args_t *args) { ioa_engine_handle e = args->engine; turn_params_t *params = args->params; + /* No mutex with "e" as these are only used in the same event loop */ pthread_mutex_lock(&turn_params.tls_mutex); - e->tls_ctx_ssl23 = params->tls_ctx_ssl23; - e->tls_ctx_v1_0 = params->tls_ctx_v1_0; + replace_one_ssl_ctx(&e->tls_ctx_ssl23, params->tls_ctx_ssl23); + replace_one_ssl_ctx(&e->tls_ctx_v1_0, params->tls_ctx_v1_0); #if TLSv1_1_SUPPORTED - e->tls_ctx_v1_1 = params->tls_ctx_v1_1; + replace_one_ssl_ctx(&e->tls_ctx_v1_1, params->tls_ctx_v1_1); #if TLSv1_2_SUPPORTED - e->tls_ctx_v1_2 = params->tls_ctx_v1_2; + replace_one_ssl_ctx(&e->tls_ctx_v1_2, params->tls_ctx_v1_2); #endif #endif #if DTLS_SUPPORTED - e->dtls_ctx = params->dtls_ctx; + replace_one_ssl_ctx(&e->dtls_ctx, params->dtls_ctx); #endif #if DTLSv1_2_SUPPORTED - e->dtls_ctx_v1_2 = params->dtls_ctx_v1_2; + replace_one_ssl_ctx(&e->dtls_ctx_v1_2, params->dtls_ctx_v1_2); #endif struct event *next = args->next; pthread_mutex_unlock(&turn_params.tls_mutex);