From 8f1908d7bd75bb08858e29486df616b56c0a115a Mon Sep 17 00:00:00 2001 From: Mark Hills Date: Wed, 3 Feb 2021 16:39:06 +0000 Subject: [PATCH] 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);