From: Arran Cudbard-Bell Date: Thu, 24 Jun 2021 18:05:45 +0000 (-0500) Subject: Deal with request cancellations when we're yielded in an OpenSSL callback X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=7313c2bdcf839ce8364accc5de2c1ea3cef3c25f;p=thirdparty%2Ffreeradius-server.git Deal with request cancellations when we're yielded in an OpenSSL callback --- diff --git a/src/lib/server/request.h b/src/lib/server/request.h index 5ec78c62660..a1bb17fa211 100644 --- a/src/lib/server/request.h +++ b/src/lib/server/request.h @@ -63,7 +63,7 @@ typedef enum { REQUEST_ACTIVE = 1, REQUEST_STOP_PROCESSING, REQUEST_COUNTED -} rad_master_state_t; +} request_master_state_t; #define REQUEST_MASTER_NUM_STATES (REQUEST_COUNTED + 1) typedef enum request_state_t { @@ -215,7 +215,7 @@ struct request_s { RADCLIENT *client; //!< The client that originally sent us the request. - rad_master_state_t master_state; //!< Set by the master thread to signal the child that's currently + request_master_state_t master_state; //!< Set by the master thread to signal the child that's currently //!< working with the request, to do something. rlm_rcode_t rcode; //!< Last rcode returned by a module diff --git a/src/lib/tls/cache.c b/src/lib/tls/cache.c index 61404f10ccb..c49e350f151 100644 --- a/src/lib/tls/cache.c +++ b/src/lib/tls/cache.c @@ -35,6 +35,7 @@ USES_APPLE_DEPRECATED_API /* OpenSSL API has been deprecated by Apple */ #include #include #include +#include #include #include "attrs.h" @@ -226,6 +227,11 @@ static void tls_cache_delete_request(SSL_SESSION *sess) request = fr_tls_session_request(tls_session->ssl); tls_cache = tls_session->cache; + /* + * Request was cancelled just return without doing any work. + */ + if (unlang_request_is_cancelled(request)) return; + fr_assert(tls_cache->clear.state == FR_TLS_CACHE_CLEAR_INIT); /* @@ -241,7 +247,11 @@ static void tls_cache_delete_request(SSL_SESSION *sess) tls_cache->clear.state = FR_TLS_CACHE_CLEAR_REQUESTED; - ASYNC_pause_job(); /* Go do the delete _now_ */ + /* + * Go and do the delete now, instead of at some + * indeterminate point in the future. + */ + ASYNC_pause_job(); /* Jumps back to SSL_read() in session.c */ } /** Process the result of `session load { ... }` @@ -615,6 +625,12 @@ static int tls_cache_store_cb(SSL *ssl, SSL_SESSION *sess) unsigned int id_len; uint8_t const *id; + /* + * Request was cancelled, just get OpenSSL to + * free the session data, and don't do any work. + */ + if (unlang_request_is_cancelled(request)) return 0; + /* * This functions should only be called once during the lifetime * of the tls_session, as the fields aren't re-populated on @@ -660,6 +676,12 @@ static SSL_SESSION *tls_cache_load_cb(SSL *ssl, request = fr_tls_session_request(tls_session->ssl); tls_cache = tls_session->cache; + /* + * Request was cancelled, don't return any session and hopefully + * OpenSSL will return back to SSL_read() soon. + */ + if (unlang_request_is_cancelled(request)) return NULL; + /* * Ensure if session resumption is disallowed this callback * will never return session data. @@ -687,7 +709,19 @@ 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(); + ASYNC_pause_job(); /* Jumps back to SSL_read() in session.c */ + + /* + * load cache { ... } returned, but the parent + * request was cancelled, try and get everything + * back into a consistent state and tell OpenSSL + * we failed to load the session. + */ + if (unlang_request_is_cancelled(request)) { + tls_cache_load_state_reset(tls_cache); /* Clears any loaded session data */ + return NULL; + + } goto again; case FR_TLS_CACHE_LOAD_REQUESTED: @@ -733,7 +767,20 @@ again: */ fr_tls_verify_client_cert_request(tls_session, true); - ASYNC_pause_job(); + ASYNC_pause_job(); /* Jumps back to SSL_read() in session.c */ + + /* + * Certificate validation returned but the request + * was cancelled. Free any data we have so far + * and reset the states, then let OpenSSL know + * we failed to load the session. + */ + if (unlang_request_is_cancelled(request)) { + tls_cache_load_state_reset(tls_cache); /* Clears any loaded session data */ + fr_tls_verify_client_cert_reset(tls_session); + return NULL; + + } /* * If we couldn't validate the client certificate @@ -766,6 +813,8 @@ 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. @@ -797,6 +846,12 @@ int fr_tls_cache_disable_cb(SSL *ssl, int is_forward_secure) tls_session = fr_tls_session(ssl); request = fr_tls_session_request(tls_session->ssl); + /* + * Request was cancelled, try and get OpenSSL to + * do as little work as possible. + */ + if (unlang_request_is_cancelled(request)) return 1; + { fr_tls_conf_t *conf; @@ -964,6 +1019,11 @@ static int tls_cache_session_ticket_app_data_set(SSL *ssl, void *arg) */ request = fr_tls_session_request(ssl); + /* + * Request was cancelled, don't do anything. + */ + if (unlang_request_is_cancelled(request)) return 0; + /* * Fatal error - We definitely should be * attempting to generate session tickets @@ -1001,7 +1061,10 @@ static SSL_TICKET_RETURN tls_cache_session_ticket_app_data_get(SSL *ssl, SSL_SES fr_tls_cache_conf_t *tls_cache_conf = arg; /* Not talloced */ request_t *request = NULL; - if (fr_tls_session_request_bound(ssl)) request = fr_tls_session_request(ssl); + if (fr_tls_session_request_bound(ssl)) { + request = fr_tls_session_request(ssl); + if (unlang_request_is_cancelled(request)) return SSL_TICKET_RETURN_ABORT; + } if (!tls_session->allow_session_resumption || (!(tls_cache_conf->mode & FR_TLS_CACHE_STATELESS))) { @@ -1054,7 +1117,16 @@ static SSL_TICKET_RETURN tls_cache_session_ticket_app_data_get(SSL *ssl, SSL_SES */ fr_tls_verify_client_cert_request(tls_session, true); - ASYNC_pause_job(); + ASYNC_pause_job(); /* Jumps back to SSL_read() in session.c */ + + /* + * If the request was cancelled get everything back into + * a known state. + */ + if (unlang_request_is_cancelled(request)) { + fr_tls_verify_client_cert_reset(tls_session); + return SSL_TICKET_RETURN_ABORT; + } /* * If we couldn't validate the client certificate diff --git a/src/lib/tls/session.c b/src/lib/tls/session.c index d0a59491619..6a1f3d88dc8 100644 --- a/src/lib/tls/session.c +++ b/src/lib/tls/session.c @@ -1176,6 +1176,45 @@ static unlang_action_t tls_session_async_handshake_done_round(UNUSED rlm_rcode_t return UNLANG_ACTION_CALCULATE_RESULT; } +/** Try very hard to get the SSL * into a consistent state where it's not yielded + * + * ...because if it's yielded, we'll probably leak thread contexts and all kinds of memory. + * + * @param[in] request being cancelled. + * @param[in] action we're being signalled with. + * @param[in] uctx the SSL * to cancell. + */ +static void tls_session_async_handshake_signal(UNUSED request_t *request, fr_state_signal_t action, void *uctx) +{ + SSL *ssl = uctx; + fr_tls_session_t *tls_session = fr_tls_session(ssl); + int ret; + + if (action != FR_SIGNAL_CANCEL) return; + + /* + * If SSL_get_error returns SSL_ERROR_WANT_ASYNC + * it means we're yielded in the middle of a + * callback. + * + * Keep calling SSL_read() in a loop until we + * no longer get SSL_ERROR_WANT_ASYNC, then + * shut it down so it's in a consistent state. + * + * It'll get freed later when the request is + * freed. + */ + for (ret = tls_session->last_ret; + SSL_get_error(tls_session->ssl, ret) == SSL_ERROR_WANT_ASYNC; + 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)); + + /* + * Unbind the cancelled request from the SSL * + */ + fr_tls_session_request_unbind(tls_session->ssl); +} + /** Call SSL_read() to continue the TLS state machine * * This function may be called multiple times, once after every asynchronous request. @@ -1192,7 +1231,6 @@ static unlang_action_t tls_session_async_handshake_cont(rlm_rcode_t *p_result, i request_t *request, void *uctx) { fr_tls_session_t *tls_session = talloc_get_type_abort(uctx, fr_tls_session_t); - int ret; RDEBUG3("(re-)entered state %s", __FUNCTION__); @@ -1216,10 +1254,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. */ - 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); - if (ret > 0) { - tls_session->clean_out.used += ret; + 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); + if (tls_session->last_ret > 0) { + tls_session->clean_out.used += tls_session->last_ret; /* * Round successful, and we don't need to do any @@ -1238,7 +1276,7 @@ static unlang_action_t tls_session_async_handshake_cont(rlm_rcode_t *p_result, i * it'd like to perform the operation * asynchronously. */ - switch (SSL_get_error(tls_session->ssl, ret)) { + switch (SSL_get_error(tls_session->ssl, tls_session->last_ret)) { case SSL_ERROR_WANT_ASYNC: /* Certification validation or cache loads */ { unlang_action_t ua; @@ -1249,7 +1287,7 @@ static unlang_action_t tls_session_async_handshake_cont(rlm_rcode_t *p_result, i * Call this function again once we're done * asynchronously satisfying the load request. */ - if (unlikely(unlang_function_repeat(request, tls_session_async_handshake_cont) < 0)) { + if (unlikely(unlang_function_repeat_set(request, tls_session_async_handshake_cont) < 0)) { error: tls_session->result = FR_TLS_RESULT_ERROR; goto finish; @@ -1291,7 +1329,8 @@ static unlang_action_t tls_session_async_handshake_cont(rlm_rcode_t *p_result, i * Returns 0 if we can continue processing the handshake * Returns -1 if we encountered a fatal error. */ - if (fr_tls_log_io_error(request, tls_session, ret, "Failed in SSL_read") < 0) goto error; + if (fr_tls_log_io_error(request, tls_session, + tls_session->last_ret, "Failed in SSL_read") < 0) goto error; return tls_session_async_handshake_done_round(p_result, priority, request, uctx); } } @@ -1362,16 +1401,6 @@ static unlang_action_t tls_session_async_handshake(rlm_rcode_t *p_result, int *p return tls_session_async_handshake_cont(p_result, priority, request, uctx); } -/** Unbind the request from the SSL session if the request is cancelled - * - */ -static void tls_session_async_handshake_signal(UNUSED request_t *request, fr_state_signal_t action, void *uctx) -{ - fr_tls_session_t *tls_session = talloc_get_type_abort(uctx, fr_tls_session_t); - - if (action == FR_SIGNAL_CANCEL) fr_tls_session_request_unbind(tls_session->ssl); -} - /** Push a handshake call onto the stack * * We push the handshake frame (as opposed to having the caller do it), diff --git a/src/lib/tls/session.h b/src/lib/tls/session.h index e57c4d6ee08..176d0dbd38d 100644 --- a/src/lib/tls/session.h +++ b/src/lib/tls/session.h @@ -112,6 +112,7 @@ struct fr_tls_session_s { fr_tls_record_t clean_out; //!< Cleartext data that's been encrypted. fr_tls_record_t dirty_in; //!< Encrypted data to decrypt. fr_tls_record_t dirty_out; //!< Encrypted data that's been decrypted. + int last_ret; //!< Last result returned by SSL_read(). void (*record_init)(fr_tls_record_t *buf); void (*record_close)(fr_tls_record_t *buf); diff --git a/src/lib/tls/verify.c b/src/lib/tls/verify.c index ef84b576f7c..166dc020e5c 100644 --- a/src/lib/tls/verify.c +++ b/src/lib/tls/verify.c @@ -31,6 +31,7 @@ #include #include #include +#include #include #include #include @@ -117,6 +118,15 @@ 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); + /* + * Bail out as quickly as possible, producing + * as few errors as possible. + */ + if (unlang_request_is_cancelled(request)) { + X509_STORE_CTX_set_error(x509_ctx, 0); + return 1; + } + if (RDEBUG_ENABLED3) { char subject[2048]; STACK_OF(X509) *our_chain = X509_STORE_CTX_get_chain(x509_ctx); @@ -216,7 +226,17 @@ done: */ fr_tls_verify_client_cert_request(tls_session, SSL_session_reused(tls_session->ssl)); - ASYNC_pause_job(); + ASYNC_pause_job(); /* Jumps back to SSL_read() in session.c */ + + /* + * Just try and bail out as quickly as possible. + */ + if (unlang_request_is_cancelled(request)) { + X509_STORE_CTX_set_error(x509_ctx, 0); + fr_tls_verify_client_cert_reset(tls_session); + return 1; + } + /* * If we couldn't validate the client certificate @@ -387,7 +407,16 @@ bool fr_tls_verify_client_cert_result(fr_tls_session_t *tls_session) return result; } -/** Setup a validation request +/** Reset the verification state + * + */ +void fr_tls_verify_client_cert_reset(fr_tls_session_t *tls_session) +{ + tls_session->validate.state = FR_TLS_VALIDATION_INIT; + tls_session->validate.resumed = false; +} + +/** Setup a verification request * */ void fr_tls_verify_client_cert_request(fr_tls_session_t *tls_session, bool session_resumed) diff --git a/src/lib/tls/verify.h b/src/lib/tls/verify.h index 309099473cf..a4da02556c0 100644 --- a/src/lib/tls/verify.h +++ b/src/lib/tls/verify.h @@ -80,6 +80,8 @@ int fr_tls_verify_client_cert_chain(request_t *request, SSL *ssl); bool fr_tls_verify_client_cert_result(fr_tls_session_t *tls_session); +void fr_tls_verify_client_cert_reset(fr_tls_session_t *tls_session); + void fr_tls_verify_client_cert_request(fr_tls_session_t *tls_session, bool resumed); unlang_action_t fr_tls_verify_client_cert_pending_push(request_t *request, fr_tls_session_t *tls_session); diff --git a/src/lib/unlang/interpret.c b/src/lib/unlang/interpret.c index e09bdc69aa9..fc5d7ee7d09 100644 --- a/src/lib/unlang/interpret.c +++ b/src/lib/unlang/interpret.c @@ -1004,6 +1004,13 @@ bool unlang_request_is_scheduled(request_t const *request) return intp->funcs.scheduled(request, intp->uctx); } +/** Return whether a request has been cancelled + */ +bool unlang_request_is_cancelled(request_t const *request) +{ + return (request->master_state == REQUEST_STOP_PROCESSING); +} + /** Check if a request as resumable. * * @param[in] request The current request. diff --git a/src/lib/unlang/interpret.h b/src/lib/unlang/interpret.h index cd103c3bd20..43c682d9d07 100644 --- a/src/lib/unlang/interpret.h +++ b/src/lib/unlang/interpret.h @@ -147,6 +147,8 @@ void *unlang_interpret_stack_alloc(TALLOC_CTX *ctx); bool unlang_request_is_scheduled(request_t const *request); +bool unlang_request_is_cancelled(request_t const *request); + void unlang_interpret_request_done(request_t *request); void unlang_interpret_mark_runnable(request_t *request);