From: Haiyang Huang Date: Thu, 18 Jun 2026 01:19:42 +0000 (+0800) Subject: quic: reject ACK of an unsent packet number X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=24137d12a2e65db94118e01bf370ccce1d68c80f;p=thirdparty%2Fopenssl.git quic: reject ACK of an unsent packet number ossl_ackm_on_rx_ack_frame() stored ack_ranges[0].end into largest_acked_pkt[pkt_space] without checking it against the highest packet number actually sent in that space. Because largest_acked_pkt only ever increases and drives loss detection, an ACK acknowledging a packet number that was never sent (up to 2**62 - 1) pins the value and causes every in-flight and subsequently-sent packet to be declared lost, permanently corrupting loss detection for the connection. RFC 9000 s. 13.1 recommends treating an acknowledgment for a packet the endpoint did not send as a connection error of type PROTOCOL_VIOLATION, where it can be detected. Reject any ACK whose largest acknowledged packet number exceeds the highest packet number sent in that space; the bound, highest_sent, is already tracked. The depacketiser raises PROTOCOL_VIOLATION when the ACK manager rejects the frame. Update the QUIC tests for the new behaviour: cases 7 and 8 now assert rejection, case 14 covers the 2**62 - 1 boundary, two pre-existing fixtures that acknowledged one packet past the highest sent are corrected, and the "fictional PN" script now expects a PROTOCOL_VIOLATION close. Fixes: fa4e92a70a5f "QUIC ACK Manager, Statistics Manager and Congestion Control API" Assisted-by: Claude:claude-opus-4.6 Reviewed-by: Saša Nedvědický Reviewed-by: Tomas Mraz MergeDate: Tue Jun 23 16:36:27 2026 (Merged from https://github.com/openssl/openssl/pull/31582) --- diff --git a/ssl/quic/quic_ackm.c b/ssl/quic/quic_ackm.c index d1ac3b88e9b..24acb8635cb 100644 --- a/ssl/quic/quic_ackm.c +++ b/ssl/quic/quic_ackm.c @@ -1169,8 +1169,21 @@ int ossl_ackm_on_rx_ack_frame(OSSL_ACKM *ackm, const OSSL_QUIC_FRAME_ACK *ack, int pkt_space, OSSL_TIME rx_time) { OSSL_ACKM_TX_PKT *na_pkts, *lost_pkts; + struct tx_pkt_history_st *h = get_tx_history(ackm, pkt_space); int must_set_timer = 0; + /* + * RFC 9000 s. 13.1 recommends treating an acknowledgment for a packet we + * did not send as a PROTOCOL_VIOLATION, where detectable. The largest + * acknowledged PN is ack_ranges[0].end; if it exceeds the highest PN we have + * sent in this space, reject the ACK. Otherwise the peer-controlled value is + * stored into largest_acked_pkt below, which only ever increases and drives + * loss detection, so a single such ACK would permanently force every + * in-flight and subsequently-sent packet to be declared lost. + */ + if (ack->ack_ranges[0].end > h->highest_sent) + return 0; + if (ackm->largest_acked_pkt[pkt_space] == QUIC_PN_INVALID) ackm->largest_acked_pkt[pkt_space] = ack->ack_ranges[0].end; else diff --git a/ssl/quic/quic_rx_depack.c b/ssl/quic/quic_rx_depack.c index 730b0cf621b..74cfceed058 100644 --- a/ssl/quic/quic_rx_depack.c +++ b/ssl/quic/quic_rx_depack.c @@ -125,8 +125,19 @@ static int depack_do_frame_ack(PACKET *pkt, QUIC_CHANNEL *ch, } if (!ossl_ackm_on_rx_ack_frame(ch->ackm, &ack, - packet_space, received)) - goto malformed; + packet_space, received)) { + /* + * The ACK manager rejects the frame if it acknowledges a packet number + * we have not sent. RFC 9000 s. 13.1 recommends treating this as a + * PROTOCOL_VIOLATION connection error (distinct from a frame decoding + * error, which is handled at the malformed label below). + */ + ossl_quic_channel_raise_protocol_error(ch, + OSSL_QUIC_ERR_PROTOCOL_VIOLATION, + frame_type, + "ACK for unsent packet number"); + return 0; + } ++ch->diag_num_rx_ack; return 1; diff --git a/test/quic_ackm_test.c b/test/quic_ackm_test.c index a4b488fafcb..8598bf83986 100644 --- a/test/quic_ackm_test.c +++ b/test/quic_ackm_test.c @@ -11,6 +11,7 @@ #include #include "internal/quic_ackm.h" #include "internal/quic_cc.h" +#include "internal/quic_vlint.h" static OSSL_TIME fake_time = { 0 }; @@ -147,13 +148,26 @@ struct tx_ack_test_case { const OSSL_QUIC_ACK_RANGE *ack_ranges; size_t num_ack_ranges; const char *expect_ack; /* 1=ack, 2=lost, 4=discarded */ + int expect_reject; /* if nonzero the ACK must be rejected (returns 0) */ }; #define DEFINE_TX_ACK_CASE(n, pntable) \ static const struct tx_ack_test_case tx_ack_case_##n = { \ (pntable), OSSL_NELEM(pntable), \ tx_ack_range_##n, OSSL_NELEM(tx_ack_range_##n), \ - tx_ack_expect_##n \ + tx_ack_expect_##n, 0 \ + } + +/* + * As DEFINE_TX_ACK_CASE, but the ACK acknowledges a packet number that was + * never sent and so must be rejected by ossl_ackm_on_rx_ack_frame() + * (RFC 9000 s. 13.1). + */ +#define DEFINE_TX_ACK_CASE_REJECT(n, pntable) \ + static const struct tx_ack_test_case tx_ack_case_##n = { \ + (pntable), OSSL_NELEM(pntable), \ + tx_ack_range_##n, OSSL_NELEM(tx_ack_range_##n), \ + tx_ack_expect_##n, 1 \ } /* One range, partial coverage of space */ @@ -207,32 +221,32 @@ static const char tx_ack_expect_5[] = { }; DEFINE_TX_ACK_CASE(5, linear_20); -/* One range, covering entire space */ +/* One range covering the whole space (0..19, highest sent PN is 19): all acked */ static const OSSL_QUIC_ACK_RANGE tx_ack_range_6[] = { - { 0, 20 }, + { 0, 19 }, }; static const char tx_ack_expect_6[] = { 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1 }; DEFINE_TX_ACK_CASE(6, linear_20); -/* One range, covering more space than exists */ +/* One range above the highest sent PN (30 > 19): ACK rejected */ static const OSSL_QUIC_ACK_RANGE tx_ack_range_7[] = { { 0, 30 }, }; static const char tx_ack_expect_7[] = { - 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1 + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 }; -DEFINE_TX_ACK_CASE(7, linear_20); +DEFINE_TX_ACK_CASE_REJECT(7, linear_20); -/* One range, covering nothing (too high) */ +/* One range entirely above the sent PNs (21..30): ACK rejected */ static const OSSL_QUIC_ACK_RANGE tx_ack_range_8[] = { { 21, 30 }, }; static const char tx_ack_expect_8[] = { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 }; -DEFINE_TX_ACK_CASE(8, linear_20); +DEFINE_TX_ACK_CASE_REJECT(8, linear_20); /* One range, covering nothing (too low) */ static const OSSL_QUIC_ACK_RANGE tx_ack_range_9[] = { @@ -289,6 +303,20 @@ static const char tx_ack_expect_13[] = { }; DEFINE_TX_ACK_CASE(13, high_linear_20); +/* + * Largest range claims the maximum PN (2**62 - 1, never sent) plus a second + * range over real packets so loss detection would otherwise run. ACK rejected; + * otherwise largest_acked_pkt pins at the maximum and every in-flight packet is + * declared lost. + */ +static const OSSL_QUIC_ACK_RANGE tx_ack_range_14[] = { + { OSSL_QUIC_VLINT_MAX, OSSL_QUIC_VLINT_MAX }, { 15, 19 } +}; +static const char tx_ack_expect_14[] = { + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 +}; +DEFINE_TX_ACK_CASE_REJECT(14, linear_20); + static const struct tx_ack_test_case *const tx_ack_cases[] = { &tx_ack_case_1, &tx_ack_case_2, @@ -303,6 +331,7 @@ static const struct tx_ack_test_case *const tx_ack_cases[] = { &tx_ack_case_11, &tx_ack_case_12, &tx_ack_case_13, + &tx_ack_case_14, }; enum { @@ -402,6 +431,25 @@ static int test_tx_ack_case_actual(int tidx, int space, int mode) /* Try acknowledging. */ ack.ack_ranges = (OSSL_QUIC_ACK_RANGE *)c->ack_ranges; ack.num_ack_ranges = c->num_ack_ranges; + + if (c->expect_reject) { + /* ACK of an unsent PN: rejected without touching loss detection. */ + if (!TEST_int_eq(ossl_ackm_on_rx_ack_frame(h.ackm, &ack, space, + fake_time), + 0)) + goto err; + + for (i = 0; i < c->pn_table_len; ++i) { + if (!TEST_int_eq(h.pkts[i].acked, 0) + || !TEST_int_eq(h.pkts[i].lost, 0) + || !TEST_int_eq(h.pkts[i].discarded, 0)) + goto err; + } + + testresult = 1; + goto err; + } + if (!TEST_int_eq(ossl_ackm_on_rx_ack_frame(h.ackm, &ack, space, fake_time), 1)) goto err; @@ -577,7 +625,7 @@ static int test_tx_ack_time_script(int tidx) ack.num_ack_ranges = 1; ack_range.start = s->pn; - ack_range.end = s->pn + s->num_pn; + ack_range.end = s->pn + s->num_pn - 1; fake_time = ossl_time_add(fake_time, ossl_ticks2time(s->time_advance)); diff --git a/test/quic_multistream_test.c b/test/quic_multistream_test.c index de7ea27a490..4edd53e15fb 100644 --- a/test/quic_multistream_test.c +++ b/test/quic_multistream_test.c @@ -3886,7 +3886,12 @@ static const struct script_op script_49[] = { OP_SET_INJECT_WORD(4, 0), OP_S_WRITE(a, "Strawberry", 10), - OP_C_READ_EXPECT(DEFAULT, "Strawberry", 10), + /* + * The injected ACK acknowledges a packet number we have not sent, which the + * peer is expected to treat as a PROTOCOL_VIOLATION, so the connection is + * closed rather than the stream data being delivered. + */ + OP_C_EXPECT_CONN_CLOSE_INFO(OSSL_QUIC_ERR_PROTOCOL_VIOLATION, 0, 0), OP_END };