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ý <sashan@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.foundation>
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 d1ac3b88e9bf55bd99517086560fa2f96b71ce37..24acb8635cbb7685174482f5c3582282d2549607 100644 (file)
--- 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 730b0cf621bb0b83c94355ec1959eef9eb1c74df..74cfceed05897217bff56bac0cb3796e157bc8bf 100644 (file)
--- 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 a4b488fafcb3df232d1034b823a5f95eeef4a2e5..8598bf83986103dd8ea16cc034302a52f481f140 100644 (file)
--- a/test/quic_ackm_test.c
+++ b/test/quic_ackm_test.c
@@ -11,6 +11,7 @@
 #include <openssl/ssl.h>
 #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 de7ea27a490fe53de40e818e494e5336523d0056..4edd53e15fb9c2e5931202181d94ea274baf7987 100644 (file)
--- 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
 };