From 82b7a0eee90e3280bd0e2dd4a9812b3873a7f462 Mon Sep 17 00:00:00 2001 From: Hugo Landau Date: Fri, 20 Oct 2023 16:52:40 +0100 Subject: QUIC TLS: Ensure QUIC_TLS is ticked between each processed RX packet Reviewed-by: Tomas Mraz Reviewed-by: Matt Caswell (Merged from https://github.com/openssl/openssl/pull/22476) --- ssl/quic/quic_channel.c | 67 ++++++++++++++++++++++++------------------- ssl/quic/quic_channel_local.h | 5 ++++ ssl/quic/quic_rx_depack.c | 3 ++ test/quicapitest.c | 17 ++++++----- 4 files changed, 56 insertions(+), 36 deletions(-) diff --git a/ssl/quic/quic_channel.c b/ssl/quic/quic_channel.c index ef6ad15087..fe2a924433 100644 --- a/ssl/quic/quic_channel.c +++ b/ssl/quic/quic_channel.c @@ -46,10 +46,11 @@ static void ch_save_err_state(QUIC_CHANNEL *ch); static void ch_rx_pre(QUIC_CHANNEL *ch); -static int ch_rx(QUIC_CHANNEL *ch); +static int ch_rx(QUIC_CHANNEL *ch, int channel_only); static int ch_tx(QUIC_CHANNEL *ch); static void ch_tick(QUIC_TICK_RESULT *res, void *arg, uint32_t flags); -static void ch_rx_handle_packet(QUIC_CHANNEL *ch); +static int ch_tick_tls(QUIC_CHANNEL *ch, int channel_only); +static void ch_rx_handle_packet(QUIC_CHANNEL *ch, int channel_only); static OSSL_TIME ch_determine_next_tick_deadline(QUIC_CHANNEL *ch); static int ch_retry(QUIC_CHANNEL *ch, const unsigned char *retry_token, @@ -1850,9 +1851,6 @@ static void ch_tick(QUIC_TICK_RESULT *res, void *arg, uint32_t flags) OSSL_TIME now, deadline; QUIC_CHANNEL *ch = arg; int channel_only = (flags & QUIC_REACTOR_TICK_FLAG_CHANNEL_ONLY) != 0; - uint64_t error_code; - const char *error_msg; - ERR_STATE *error_state = NULL; /* * When we tick the QUIC connection, we do everything we need to do @@ -1898,21 +1896,16 @@ static void ch_tick(QUIC_TICK_RESULT *res, void *arg, uint32_t flags) do { /* Process queued incoming packets. */ - ch_rx(ch); + ch->did_tls_tick = 0; + ch->have_new_rx_secret = 0; + ch_rx(ch, channel_only); /* * Allow the handshake layer to check for any new incoming data and * generate new outgoing data. */ - ch->have_new_rx_secret = 0; - if (!channel_only) { - ossl_quic_tls_tick(ch->qtls); - - if (ossl_quic_tls_get_error(ch->qtls, &error_code, &error_msg, - &error_state)) - ossl_quic_channel_raise_protocol_error_state(ch, error_code, 0, - error_msg, error_state); - } + if (!ch->did_tls_tick) + ch_tick_tls(ch, channel_only); /* * If the handshake layer gave us a new secret, we need to do RX @@ -1983,6 +1976,28 @@ static void ch_tick(QUIC_TICK_RESULT *res, void *arg, uint32_t flags) && ossl_qtx_get_queue_len_datagrams(ch->qtx) > 0); } +static int ch_tick_tls(QUIC_CHANNEL *ch, int channel_only) +{ + uint64_t error_code; + const char *error_msg; + ERR_STATE *error_state = NULL; + + if (channel_only) + return 1; + + ch->did_tls_tick = 1; + ossl_quic_tls_tick(ch->qtls); + + if (ossl_quic_tls_get_error(ch->qtls, &error_code, &error_msg, + &error_state)) { + ossl_quic_channel_raise_protocol_error_state(ch, error_code, 0, + error_msg, error_state); + return 0; + } + + return 1; +} + /* Process incoming datagrams, if any. */ static void ch_rx_pre(QUIC_CHANNEL *ch) { @@ -2042,7 +2057,7 @@ static void ch_rx_check_forged_pkt_limit(QUIC_CHANNEL *ch) } /* Process queued incoming packets and handle frames, if any. */ -static int ch_rx(QUIC_CHANNEL *ch) +static int ch_rx(QUIC_CHANNEL *ch, int channel_only) { int handled_any = 0; const int closing = ossl_quic_channel_is_closing(ch); @@ -2070,7 +2085,7 @@ static int ch_rx(QUIC_CHANNEL *ch) ch_update_ping_deadline(ch); } - ch_rx_handle_packet(ch); /* best effort */ + ch_rx_handle_packet(ch, channel_only); /* best effort */ /* * Regardless of the outcome of frame handling, unref the packet. @@ -2122,7 +2137,7 @@ static int bio_addr_eq(const BIO_ADDR *a, const BIO_ADDR *b) } /* Handles the packet currently in ch->qrx_pkt->hdr. */ -static void ch_rx_handle_packet(QUIC_CHANNEL *ch) +static void ch_rx_handle_packet(QUIC_CHANNEL *ch, int channel_only) { uint32_t enc_level; int old_have_processed_any_pkt = ch->have_processed_any_pkt; @@ -2323,6 +2338,10 @@ static void ch_rx_handle_packet(QUIC_CHANNEL *ch) /* This packet contains frames, pass to the RXDP. */ ossl_quic_handle_frames(ch, ch->qrx_pkt); /* best effort */ + + if (ch->did_crypto_frame) + ch_tick_tls(ch, channel_only); + break; case QUIC_PKT_TYPE_VERSION_NEG: @@ -2726,10 +2745,6 @@ int ossl_quic_channel_set_net_wbio(QUIC_CHANNEL *ch, BIO *net_wbio) */ int ossl_quic_channel_start(QUIC_CHANNEL *ch) { - uint64_t error_code; - const char *error_msg; - ERR_STATE *error_state = NULL; - if (ch->is_server) /* * This is not used by the server. The server moves to active @@ -2758,14 +2773,8 @@ int ossl_quic_channel_start(QUIC_CHANNEL *ch) ch->doing_proactive_ver_neg = 0; /* not currently supported */ /* Handshake layer: start (e.g. send CH). */ - ossl_quic_tls_tick(ch->qtls); - - if (ossl_quic_tls_get_error(ch->qtls, &error_code, &error_msg, - &error_state)) { - ossl_quic_channel_raise_protocol_error_state(ch, error_code, 0, - error_msg, error_state); + if (!ch_tick_tls(ch, /*channel_only=*/0)) return 0; - } ossl_quic_reactor_tick(&ch->rtor, 0); /* best effort */ return 1; diff --git a/ssl/quic/quic_channel_local.h b/ssl/quic/quic_channel_local.h index 77dc5dd7bc..f0ac742420 100644 --- a/ssl/quic/quic_channel_local.h +++ b/ssl/quic/quic_channel_local.h @@ -388,6 +388,11 @@ struct quic_channel_st { */ unsigned int have_new_rx_secret : 1; + /* Have we ever called QUIC_TLS yet during RX processing? */ + unsigned int did_tls_tick : 1; + /* Has any CRYPTO frame been processed during this tick? */ + unsigned int did_crypto_frame : 1; + /* * Have we sent an ack-eliciting packet since the last successful packet * reception? Used to determine when to bump idle timer (see RFC 9000 s. diff --git a/ssl/quic/quic_rx_depack.c b/ssl/quic/quic_rx_depack.c index f2a564862b..97c6d6095d 100644 --- a/ssl/quic/quic_rx_depack.c +++ b/ssl/quic/quic_rx_depack.c @@ -317,6 +317,7 @@ static int depack_do_frame_crypto(PACKET *pkt, QUIC_CHANNEL *ch, return 0; } + ch->did_crypto_frame = 1; *datalen = f.len; return 1; @@ -1422,6 +1423,8 @@ int ossl_quic_handle_frames(QUIC_CHANNEL *ch, OSSL_QRX_PKT *qpacket) if (ch == NULL) goto end; + ch->did_crypto_frame = 0; + /* Initialize |ackm_data| (and reinitialize |ok|)*/ memset(&ackm_data, 0, sizeof(ackm_data)); /* diff --git a/test/quicapitest.c b/test/quicapitest.c index 83221885bc..51d6eea245 100644 --- a/test/quicapitest.c +++ b/test/quicapitest.c @@ -1057,15 +1057,15 @@ static int non_io_retry_cert_verify_cb(X509_STORE_CTX *ctx, void *arg) { int idx = SSL_get_ex_data_X509_STORE_CTX_idx(); SSL *ssl; - int *ctr = (int *)arg; + const int *allow = (int *)arg; /* this should not happen but check anyway */ if (idx < 0 || (ssl = X509_STORE_CTX_get_ex_data(ctx, idx)) == NULL) return 0; - /* If this is the first time we've been called then retry */ - if (((*ctr)++) == 0) + /* If this is our first attempt then retry */ + if (*allow == 0) return SSL_set_retry_verify(ssl); /* Otherwise do nothing - verification succeeds. Continue as normal */ @@ -1082,7 +1082,7 @@ static int test_non_io_retry(int idx) SSL *clientquic = NULL; QUIC_TSERVER *qtserv = NULL; int testresult = 0; - int flags = 0, ctr = 0; + int flags = 0, allow = 0; if (idx >= 1 && !qtest_supports_blocking()) return TEST_skip("Blocking tests not supported in this build"); @@ -1091,7 +1091,7 @@ static int test_non_io_retry(int idx) if (!TEST_ptr(cctx)) goto err; - SSL_CTX_set_cert_verify_callback(cctx, non_io_retry_cert_verify_cb, &ctr); + SSL_CTX_set_cert_verify_callback(cctx, non_io_retry_cert_verify_cb, &allow); flags = (idx >= 1) ? QTEST_FLAG_BLOCK : 0; if (!TEST_true(qtest_create_quic_objects(libctx, cctx, NULL, cert, privkey, @@ -1099,8 +1099,11 @@ static int test_non_io_retry(int idx) NULL)) || !TEST_true(qtest_create_quic_connection_ex(qtserv, clientquic, SSL_ERROR_WANT_RETRY_VERIFY)) - || !TEST_int_eq(SSL_want(clientquic), SSL_RETRY_VERIFY) - || !TEST_true(qtest_create_quic_connection(qtserv, clientquic))) + || !TEST_int_eq(SSL_want(clientquic), SSL_RETRY_VERIFY)) + goto err; + + allow = 1; + if (!TEST_true(qtest_create_quic_connection(qtserv, clientquic))) goto err; testresult = 1; -- cgit v1.2.3