From: Arran Cudbard-Bell Date: Thu, 17 Feb 2022 00:50:27 +0000 (-0500) Subject: Fix multiple issues with clearing failed TLS sessions X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=b37912dc4f3eb020a724a400c39483af9ee1d83a;p=thirdparty%2Ffreeradius-server.git Fix multiple issues with clearing failed TLS sessions --- diff --git a/src/lib/eap/tls.c b/src/lib/eap/tls.c index b5ea1ede070..436c3675c99 100644 --- a/src/lib/eap/tls.c +++ b/src/lib/eap/tls.c @@ -329,7 +329,7 @@ int eap_tls_fail(request_t *request, eap_session_t *eap_session) /* * Destroy any cached session data */ - fr_tls_cache_deny(tls_session); + fr_tls_cache_deny(request, tls_session); if (eap_tls_compose(request, eap_session, EAP_TLS_FAIL, eap_tls_session->base_flags, NULL, 0, 0) < 0) return -1; @@ -812,7 +812,7 @@ static unlang_action_t eap_tls_handshake_resume(UNUSED rlm_rcode_t *p_result, UN case FR_TLS_RESULT_ERROR: REDEBUG("TLS receive handshake failed during operation"); - fr_tls_cache_deny(tls_session); + fr_tls_cache_deny(request, tls_session); eap_tls_session->state = EAP_TLS_FAIL; goto finish; @@ -858,7 +858,7 @@ static unlang_action_t eap_tls_handshake_resume(UNUSED rlm_rcode_t *p_result, UN (void) fr_tls_session_async_handshake_push(request, tls_session); if (tls_session->result != FR_TLS_RESULT_SUCCESS) { REDEBUG("TLS receive handshake failed during operation"); - fr_tls_cache_deny(tls_session); + fr_tls_cache_deny(request, tls_session); eap_tls_session->state = EAP_TLS_FAIL; return UNLANG_ACTION_CALCULATE_RESULT; } diff --git a/src/lib/tls/cache.c b/src/lib/tls/cache.c index b1b34957cb6..0c59068ba0f 100644 --- a/src/lib/tls/cache.c +++ b/src/lib/tls/cache.c @@ -313,10 +313,28 @@ static void tls_cache_delete_request(SSL_SESSION *sess) tls_cache->clear.state = FR_TLS_CACHE_CLEAR_REQUESTED; /* - * Go and do the delete now, instead of at some - * indeterminate point in the future. + * We store a copy of the pointer for the session + * in tls_session->session. If the session is + * being freed then this pointer must be invalid + * so clear it to prevent crashes in other areas + * of the code. */ - ASYNC_pause_job(); /* Jumps back to SSL_read() in session.c */ + if (tls_session->session == sess) tls_session->session = NULL; + + /* + * Previously the code called ASYNC_pause_job(); + * assuming this callback would always be called + * from SSL_read() or another SSL function. + * + * Unfortunately it appears that the call path + * can also be triggered with SSL_CTX_remove_session + * if the reference count on the SSL_SESSION + * drops to zero. + * + * We now check the 'can_pause' flag to determine + * if we're inside a yieldable SSL_read call. + */ + if (tls_session->can_pause) ASYNC_pause_job(); } /** Process the result of `session load { ... }` @@ -792,7 +810,27 @@ again: MEM(tls_cache->load.id = talloc_typed_memdup(tls_cache, (uint8_t const *)key, key_len)); RDEBUG3("Requested session load - ID %pV", fr_box_octets_buffer(tls_cache->load.id)); - ASYNC_pause_job(); /* Jumps back to SSL_read() in session.c */ + + /* + * Cache functions are only allowed during the handshake + * FIXME: With TLS 1.3 session tickets can be sent + * later... Technically every point where we call + * SSL_read() may need to be a yield point. + */ + if (unlikely(!tls_session->can_pause)) { + cant_pause: + fr_assert_msg("Unexpected call to %s. " + "tls_session_async_handshake_cont must be in call stack", __FUNCTION__); + return NULL; + } + /* + * Jumps back to SSL_read() in session.c + * + * Be aware that if the request is cancelled + * whatever was meant to be done during the + * time we yielded may not have been completed. + */ + ASYNC_pause_job(); /* * load cache { ... } returned, but the parent @@ -852,7 +890,15 @@ again: */ fr_tls_verify_cert_request(tls_session, true); - ASYNC_pause_job(); /* Jumps back to SSL_read() in session.c */ + if (unlikely(!tls_session->can_pause)) goto cant_pause; + /* + * Jumps back to SSL_read() in session.c + * + * Be aware that if the request is cancelled + * whatever was meant to be done during the + * time we yielded may not have been completed. + */ + ASYNC_pause_job(); /* * Certificate validation returned but the request @@ -912,8 +958,6 @@ again: */ static void tls_cache_delete_cb(UNUSED SSL_CTX *ctx, SSL_SESSION *sess) { - - /* * Not sure why this happens, but sometimes SSL_SESSION *s * make it here without the correct ex data. @@ -995,26 +1039,69 @@ int fr_tls_cache_disable_cb(SSL *ssl, int is_forward_secure) return 0; } -/** Prevent a TLS session from being cached +/** Prevent a pending TLS session being persisted, and clear any resumed sessions * - * Usually called if the session has failed for some reason. + * Usually called if authentication has failed for some reason. * * Will clear any serialized data out of the tls_session structure * and should result in tls_cache_delete_cb being called. * - * @param[in] tls_session on which to prevent resumption. + * @note Calling this function will immediately free the memory used + * by the session, but not the external persisted copy of the + * session. To clear the persisted copy #fr_tls_cache_pending_push + * must be called in a place where the caller is prepared to yield. + * In most cases this means whether the handshake is a success or + * failure, the last thing the caller of the TLS code should do + * is set the result, and call #fr_tls_cache_pending_push. + * + * @param[in] request to use for running any async cache actions. + * @param[in] tls_session on which to prevent resumption. */ -void fr_tls_cache_deny(fr_tls_session_t *tls_session) +void fr_tls_cache_deny(request_t *request, fr_tls_session_t *tls_session) { fr_tls_cache_t *tls_cache = tls_session->cache; + bool tmp_bind = !fr_tls_session_request_bound(tls_session->ssl); /* - * Even for 1.1.0 we don't know when this function - * will be called, so better to remove the session - * directly. + * This is necessary to allow this function to + * be called inside and outside of OpenSSL handshake + * code. + */ + if (tmp_bind) { + fr_tls_session_request_bind(tls_session->ssl, request); + /* + * If there's already a request bound, it better be + * the one passed to this function. + */ + } else { + fr_assert(fr_tls_session_request(tls_session->ssl) == request); + } + + /* + * SSL_CTX_remove_session frees the previously loaded + * session in tls_session. If the reference count reaches zero + * the SSL_CTX_sess_remove_cb is called, which in our code is + * tls_cache_delete_cb. + * + * tls_cache_delete_cb calls tls_cache_delete_request + * to record the ID of tls_session->session + * in our pending cache state structure. + * + * tls_cache_delete_request does NOT immediately call the + * `cache clear {}` section as that must be done in a code area + * which is prepared to yield. + * + * #fr_tls_cache_pending_push MUST be called to actually + * clear external data. */ if (tls_session->session) SSL_CTX_remove_session(tls_session->ctx, tls_session->session); + if (tmp_bind) fr_tls_session_request_unbind(tls_session->ssl); + tls_session->allow_session_resumption = false; + + /* + * Clear any pending store requests. + */ tls_cache_store_state_reset(fr_tls_session_request(tls_session->ssl), tls_cache); } @@ -1216,7 +1303,26 @@ static SSL_TICKET_RETURN tls_cache_session_ticket_app_data_get(SSL *ssl, SSL_SES */ fr_tls_verify_cert_request(tls_session, true); - ASYNC_pause_job(); /* Jumps back to SSL_read() in session.c */ + /* + * Cache functions are only allowed during the handshake + * FIXME: With TLS 1.3 session tickets can be sent + * later... Technically every point where we call + * SSL_read() may need to be a yield point. + */ + if (unlikely(!tls_session->can_pause)) { + fr_assert_msg("Unexpected call to %s. " + "tls_session_async_handshake_cont must be in call stack", __FUNCTION__); + return SSL_TICKET_RETURN_IGNORE_RENEW; + } + + /* + * Jumps back to SSL_read() in session.c + * + * Be aware that if the request is cancelled + * whatever was meant to be done during the + * time we yielded may not have been completed. + */ + ASYNC_pause_job(); /* * If the request was cancelled get everything back into diff --git a/src/lib/tls/cache.h b/src/lib/tls/cache.h index d0a191abf9f..0c925ca90e4 100644 --- a/src/lib/tls/cache.h +++ b/src/lib/tls/cache.h @@ -105,7 +105,7 @@ uint8_t *fr_tls_cache_id(TALLOC_CTX *ctx, SSL_SESSION *sess); unlang_action_t fr_tls_cache_pending_push(request_t *request, fr_tls_session_t *tls_session); -void fr_tls_cache_deny(fr_tls_session_t *tls_session); +void fr_tls_cache_deny(request_t *request, fr_tls_session_t *tls_session); int fr_tls_cache_disable_cb(SSL *ssl, int is_forward_secure); diff --git a/src/lib/tls/session.c b/src/lib/tls/session.c index 7eb2b967c82..38c8fcdfb77 100644 --- a/src/lib/tls/session.c +++ b/src/lib/tls/session.c @@ -1291,6 +1291,12 @@ static void tls_session_async_handshake_signal(UNUSED request_t *request, fr_sta if (action != FR_SIGNAL_CANCEL) return; + /* + * We might want to set can_pause = false here + * but that would trigger asserts in the + * cache code. + */ + /* * If SSL_get_error returns SSL_ERROR_WANT_ASYNC * it means we're yielded in the middle of a @@ -1354,8 +1360,10 @@ static unlang_action_t tls_session_async_handshake_cont(rlm_rcode_t *p_result, i * If acting as a server SSL_set_accept_state must have * been called before this function. */ + tls_session->can_pause = true; tls_session->last_ret = SSL_read(tls_session->ssl, tls_session->clean_out.data + tls_session->clean_out.used, sizeof(tls_session->clean_out.data) - tls_session->clean_out.used); + tls_session->can_pause = false; if (tls_session->last_ret > 0) { tls_session->clean_out.used += tls_session->last_ret; @@ -1365,6 +1373,9 @@ static unlang_action_t tls_session_async_handshake_cont(rlm_rcode_t *p_result, i */ tls_session->result = FR_TLS_RESULT_SUCCESS; finish: + /* + * Was bound by caller + */ fr_tls_session_request_unbind(tls_session->ssl); return UNLANG_ACTION_CALCULATE_RESULT; } @@ -1502,7 +1513,7 @@ static unlang_action_t tls_session_async_handshake(rlm_rcode_t *p_result, int *p tls_session->result = FR_TLS_RESULT_IN_PROGRESS; - fr_tls_session_request_bind(tls_session->ssl, request); + fr_tls_session_request_bind(tls_session->ssl, request); /* May be unbound in this function or asynchronously */ /* * This is a logic error. fr_tls_session_async_handshake @@ -1514,7 +1525,7 @@ static unlang_action_t tls_session_async_handshake(rlm_rcode_t *p_result, int *p REDEBUG("Attempted to continue TLS handshake, but handshake has completed"); error: tls_session->result = FR_TLS_RESULT_ERROR; - fr_tls_session_request_unbind(tls_session->ssl); + fr_tls_session_request_unbind(tls_session->ssl); /* Was bound in this function */ return UNLANG_ACTION_CALCULATE_RESULT; } @@ -1538,7 +1549,7 @@ static unlang_action_t tls_session_async_handshake(rlm_rcode_t *p_result, int *p record_init(&tls_session->dirty_in); } - return tls_session_async_handshake_cont(p_result, priority, request, uctx); + return tls_session_async_handshake_cont(p_result, priority, request, uctx); /* Must unbind request, possibly asynchronously */ } /** Push a handshake call onto the stack @@ -1629,7 +1640,7 @@ fr_tls_session_t *fr_tls_session_alloc_client(TALLOC_CTX *ctx, SSL_CTX *ssl_ctx) request = request_alloc_internal(tls_session, NULL); - fr_tls_session_request_bind(tls_session->ssl, request); + fr_tls_session_request_bind(tls_session->ssl, request); /* Is unbound in this function */ /* * Add the message callback to identify what type of @@ -1653,7 +1664,7 @@ fr_tls_session_t *fr_tls_session_alloc_client(TALLOC_CTX *ctx, SSL_CTX *ssl_ctx) ret = SSL_connect(tls_session->ssl); if (ret <= 0) { fr_tls_log_io_error(NULL, SSL_get_error(tls_session->ssl, ret), "SSL_connect (%s)", __FUNCTION__); - fr_tls_session_request_unbind(tls_session->ssl); + fr_tls_session_request_unbind(tls_session->ssl); /* Was bound in this function */ talloc_free(tls_session); return NULL; @@ -1661,7 +1672,7 @@ fr_tls_session_t *fr_tls_session_alloc_client(TALLOC_CTX *ctx, SSL_CTX *ssl_ctx) tls_session->mtu = conf->fragment_size; - fr_tls_session_request_unbind(tls_session->ssl); + fr_tls_session_request_unbind(tls_session->ssl); /* Was bound in this function */ return tls_session; } @@ -1704,7 +1715,7 @@ fr_tls_session_t *fr_tls_session_alloc_server(TALLOC_CTX *ctx, SSL_CTX *ssl_ctx, tls_session->ssl = ssl; talloc_set_destructor(tls_session, _fr_tls_session_free); - fr_tls_session_request_bind(tls_session->ssl, request); + fr_tls_session_request_bind(tls_session->ssl, request); /* Is unbound in this function */ /* * Initialize callbacks @@ -1765,7 +1776,7 @@ fr_tls_session_t *fr_tls_session_alloc_server(TALLOC_CTX *ctx, SSL_CTX *ssl_ctx, if (tmpl_aexpand(tls_session, &context_id, request, conf->cache.id_name, NULL, NULL) < 0) { RPEDEBUG("Failed expanding session ID"); error: - fr_tls_session_request_unbind(tls_session->ssl); + fr_tls_session_request_unbind(tls_session->ssl); /* Was bound in this function */ talloc_free(tls_session); return NULL; } @@ -1872,11 +1883,11 @@ fr_tls_session_t *fr_tls_session_alloc_server(TALLOC_CTX *ctx, SSL_CTX *ssl_ctx, } if (conf->cache.mode != FR_TLS_CACHE_DISABLED) { - tls_session->allow_session_resumption = true; /* otherwise it's false */ + tls_session->allow_session_resumption = true; /* otherwise it's false */ fr_tls_cache_session_alloc(tls_session); } - fr_tls_session_request_unbind(tls_session->ssl); + fr_tls_session_request_unbind(tls_session->ssl); /* Was bound in this function */ return tls_session; } diff --git a/src/lib/tls/session.h b/src/lib/tls/session.h index a511b24c44c..366d06f500e 100644 --- a/src/lib/tls/session.h +++ b/src/lib/tls/session.h @@ -134,6 +134,8 @@ struct fr_tls_session_s { bool invalid; //!< Whether heartbleed attack was detected. bool client_cert_ok; //!< whether or not the client certificate was validated + bool can_pause; //!< If true, it's ok to pause the request + ///< using the OpenSSL async API. uint8_t alerts_sent; bool pending_alert; diff --git a/src/lib/tls/verify.c b/src/lib/tls/verify.c index 3b78ded25db..be8ce12c3b5 100644 --- a/src/lib/tls/verify.c +++ b/src/lib/tls/verify.c @@ -157,6 +157,17 @@ int fr_tls_verify_cert_cb(int ok, X509_STORE_CTX *x509_ctx) tls_session = talloc_get_type_abort(SSL_get_ex_data(ssl, FR_TLS_EX_INDEX_TLS_SESSION), fr_tls_session_t); request = fr_tls_session_request(tls_session->ssl); + /* + * If this error appears it suggests + * that OpenSSL is trying to perform post-handshake + * certificate validation which we don't support. + */ + if (!tls_session->can_pause) { + fr_assert_msg("Unexpected call to %s. " + "tls_session_async_handshake_cont must be in call stack", __FUNCTION__); + return 0; + } + /* * Bail out as quickly as possible, producing * as few errors as possible. @@ -281,7 +292,14 @@ done: */ fr_tls_verify_cert_request(tls_session, SSL_session_reused(tls_session->ssl)); - ASYNC_pause_job(); /* Jumps back to SSL_read() in session.c */ + /* + * Jumps back to SSL_read() in session.c + * + * Be aware that if the request is cancelled + * whatever was meant to be done during the + * time we yielded may not have been completed. + */ + ASYNC_pause_job(); /* * Just try and bail out as quickly as possible. diff --git a/src/modules/rlm_eap/types/rlm_eap_tls/rlm_eap_tls.c b/src/modules/rlm_eap/types/rlm_eap_tls/rlm_eap_tls.c index 83d48d80b3f..989d9d155df 100644 --- a/src/modules/rlm_eap/types/rlm_eap_tls/rlm_eap_tls.c +++ b/src/modules/rlm_eap/types/rlm_eap_tls/rlm_eap_tls.c @@ -150,9 +150,15 @@ static unlang_action_t mod_handshake_resume(rlm_rcode_t *p_result, module_ctx_t * the client can't re-use it. */ default: - fr_tls_cache_deny(tls_session); + fr_tls_cache_deny(request, tls_session); + *p_result = RLM_MODULE_REJECT; - RETURN_MODULE_REJECT; + /* + * We'll jump back to the caller + * in the unlang stack if this + * fails. + */ + return fr_tls_cache_pending_push(request, tls_session); /* Run any pending cache clear operations */ } }