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 <nhorman@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/28023)

(cherry picked from commit cdbfacead0d07ed47fa1087d633acf6f6399aa2c)

diff --git a/include/internal/quic_ackm.h b/include/internal/quic_ackm.h
index 69b862d9c55a7b89b0e84f0f66621001d7740632..320b49ca6b0bec05ddd6c04856c0119287bfdab7 100644 (file)
--- 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 75a1e5741a030b5a426a83e6e73c62cffbdae3a3..a16875ed599b977a20e8eed915b336a3b89b07bf 100644 (file)
--- 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 64aded4253661422cec95eb4041a2d1ca02d303f..ffa46bead282be8d598dab364e7b4d3b576faca7 100644 (file)
--- a/ssl/quic/quic_channel.c
+++ b/ssl/quic/quic_channel.c
@@ -244,7 +244,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 0f26e9d38a0e8a587dff689f8a9672ff7298db24..44d47c2d7a84b61ba6f2d7bc9fa016d924947c10 100644 (file)
--- 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 cfa5a77745b72c674d93346502e0f2e148b9e51e..923642aabe8c3e28feccf412706db9d7c9d72314 100644 (file)
--- 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 f234fb683ac9199d4d3c449ae849a838d5357b62..a5d34bdfe75533196b82338616a6948a9ab8883d 100644 (file)
--- 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,