From: sashan Date: Tue, 15 Jul 2025 13:09:40 +0000 (+0200) Subject: ACK manager must avoid infinite probe time when waiting handshake confirmation X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=cdbfacead0d07ed47fa1087d633acf6f6399aa2c;p=thirdparty%2Fopenssl.git ACK manager must avoid infinite probe time when waiting handshake confirmation According to RFC 9002, section 6.2.2.1 the client the client must keep PTO (probe time out) armed if it has not seen HANDSHAKE_DONE quic message from server. Not following RFC spec here may cause the QUIC session to stale during TLS handshake. Fixes openssl/project#1266 Reviewed-by: Neil Horman Reviewed-by: Tomas Mraz (Merged from https://github.com/openssl/openssl/pull/28023) --- diff --git a/include/internal/quic_ackm.h b/include/internal/quic_ackm.h index c271dfca2e1..434bf1bd12d 100644 --- a/include/internal/quic_ackm.h +++ b/include/internal/quic_ackm.h @@ -23,7 +23,7 @@ OSSL_ACKM *ossl_ackm_new(OSSL_TIME (*now)(void *arg), void *now_arg, OSSL_STATM *statm, const OSSL_CC_METHOD *cc_method, - OSSL_CC_DATA *cc_data); + OSSL_CC_DATA *cc_data, char is_server); void ossl_ackm_free(OSSL_ACKM *ackm); void ossl_ackm_set_loss_detection_deadline_callback(OSSL_ACKM *ackm, diff --git a/ssl/quic/quic_ackm.c b/ssl/quic/quic_ackm.c index 75a1e5741a0..a16875ed599 100644 --- a/ssl/quic/quic_ackm.c +++ b/ssl/quic/quic_ackm.c @@ -536,6 +536,9 @@ struct ossl_ackm_st { /* Set to 1 when the handshake is confirmed. */ char handshake_confirmed; + /* Set to 1 when attached to server channel */ + char is_server; + /* Set to 1 when the peer has completed address validation. */ char peer_completed_addr_validation; @@ -855,7 +858,13 @@ static OSSL_TIME ackm_get_pto_time_and_space(OSSL_ACKM *ackm, int *space) } for (i = QUIC_PN_SPACE_INITIAL; i < QUIC_PN_SPACE_NUM; ++i) { - if (ackm->ack_eliciting_bytes_in_flight[i] == 0) + /* + * RFC 9002 section 5.2.2.1 keep probe timeout armed until + * handshake is confirmed (client sees HANDSHAKE_DONE message + * from server(. + */ + if (ackm->ack_eliciting_bytes_in_flight[i] == 0 && + (ackm->handshake_confirmed == 1 || ackm->is_server == 1)) continue; if (i == QUIC_PN_SPACE_APP) { @@ -875,10 +884,18 @@ static OSSL_TIME ackm_get_pto_time_and_space(OSSL_ACKM *ackm, int *space) } } - t = ossl_time_add(ackm->time_of_last_ack_eliciting_pkt[i], duration); - if (ossl_time_compare(t, pto_timeout) < 0) { - pto_timeout = t; - pto_space = i; + /* + * Only re-arm timer if stack has sent at least one ACK eliciting frame. + * If stack has sent no ACK eliciting at given encryption level then + * particular timer is zero and we must not attempt to set it. Timer time + * runs since epoch (Jan 1 1970 and we must not set timer to past. + */ + if (!ossl_time_is_zero(ackm->time_of_last_ack_eliciting_pkt[i])) { + t = ossl_time_add(ackm->time_of_last_ack_eliciting_pkt[i], duration); + if (ossl_time_compare(t, pto_timeout) < 0) { + pto_timeout = t; + pto_space = i; + } } } @@ -1021,7 +1038,8 @@ OSSL_ACKM *ossl_ackm_new(OSSL_TIME (*now)(void *arg), void *now_arg, OSSL_STATM *statm, const OSSL_CC_METHOD *cc_method, - OSSL_CC_DATA *cc_data) + OSSL_CC_DATA *cc_data, + char is_server) { OSSL_ACKM *ackm; int i; @@ -1045,6 +1063,7 @@ OSSL_ACKM *ossl_ackm_new(OSSL_TIME (*now)(void *arg), ackm->statm = statm; ackm->cc_method = cc_method; ackm->cc_data = cc_data; + ackm->is_server = is_server; ackm->rx_max_ack_delay = ossl_ms2time(QUIC_DEFAULT_MAX_ACK_DELAY); ackm->tx_max_ack_delay = DEFAULT_TX_MAX_ACK_DELAY; diff --git a/ssl/quic/quic_channel.c b/ssl/quic/quic_channel.c index 8fb651d9ceb..f8e4252ba48 100644 --- a/ssl/quic/quic_channel.c +++ b/ssl/quic/quic_channel.c @@ -242,7 +242,8 @@ static int ch_init(QUIC_CHANNEL *ch) goto err; if ((ch->ackm = ossl_ackm_new(get_time, ch, &ch->statm, - ch->cc_method, ch->cc_data)) == NULL) + ch->cc_method, ch->cc_data, + ch->is_server)) == NULL) goto err; if (!ossl_quic_stream_map_init(&ch->qsm, get_stream_limit, ch, diff --git a/test/quic_ackm_test.c b/test/quic_ackm_test.c index 2ffcf593359..07c45c219e3 100644 --- a/test/quic_ackm_test.c +++ b/test/quic_ackm_test.c @@ -104,7 +104,8 @@ static int helper_init(struct helper *h, size_t num_pkts) /* Initialise ACK manager. */ h->ackm = ossl_ackm_new(fake_now, NULL, &h->statm, - &ossl_cc_dummy_method, h->ccdata); + &ossl_cc_dummy_method, h->ccdata, + /* is_server */0); if (!TEST_ptr(h->ackm)) goto err; diff --git a/test/quic_fifd_test.c b/test/quic_fifd_test.c index cfa5a77745b..923642aabe8 100644 --- a/test/quic_fifd_test.c +++ b/test/quic_fifd_test.c @@ -329,7 +329,8 @@ static int test_fifd(int idx) || !TEST_ptr(info.ackm = ossl_ackm_new(fake_now, NULL, &info.statm, &ossl_cc_dummy_method, - info.ccdata)) + info.ccdata, + /* is_server */0)) || !TEST_true(ossl_ackm_on_handshake_confirmed(info.ackm)) || !TEST_ptr(info.cfq = ossl_quic_cfq_new()) || !TEST_ptr(info.txpim = ossl_quic_txpim_new()) diff --git a/test/quic_txp_test.c b/test/quic_txp_test.c index 7b6248cc15e..c9ac68ef391 100644 --- a/test/quic_txp_test.c +++ b/test/quic_txp_test.c @@ -182,7 +182,8 @@ static int helper_init(struct helper *h) if (!TEST_ptr(h->args.ackm = ossl_ackm_new(fake_now, NULL, &h->statm, h->cc_method, - h->cc_data))) + h->cc_data, + /* is_server */0))) goto err; if (!TEST_true(ossl_quic_stream_map_init(&h->qsm, NULL, NULL,