From: Frédéric Lécaille <flecaille@haproxy.com>
Date: Fri, 18 Nov 2022 17:15:28 +0000 (+0100)
Subject: BUG/MAJOR: quic: Crash upon retransmission of dgrams with several packets
X-Git-Tag: v2.7-dev10~52
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=814645f42fae3e6dea994f88aa3b67cf43958dcf;p=thirdparty%2Fhaproxy.git

BUG/MAJOR: quic: Crash upon retransmission of dgrams with several packets

As revealed by some traces provided by @gabrieltz in GH #1903 issue,
there are clients (chrome I guess) which acknowledge only one packet among others
in the same datagram. This is the case for the first datagram sent by a QUIC haproxy
listener made an Initial packet followed by an Handshake one. In this identified
case, this is the Handshake packet only which is acknowledged. But if the
client is able to respond with an Handshake packet (ACK frame) this is because
it has successfully parsed the Initial packet. So, why not also acknowledging it?
AFAIK, this is mandatory. On our side, when restransmitting this datagram, the
Handshake packet was accessed from the Initial packet after having being released.

Anyway. There is an issue on our side. Obviously, we must not expect an
implementation to respect the RFC especially when it want to build an attack ;)

With this simple patch for each TX packet we send, we also set the previous one
in addition to the next one. When a packet is acknowledged, we detach the next one
and the next one in the same datagram from this packet, so that it cannot be
resent when resending these packets (the previous one, in our case).

Thank you to @gabrieltz for having reported this issue.

Must be backported to 2.6.
---

diff --git a/include/haproxy/quic_conn-t.h b/include/haproxy/quic_conn-t.h
index 65a4ab0225..6c3948b7d0 100644
--- a/include/haproxy/quic_conn-t.h
+++ b/include/haproxy/quic_conn-t.h
@@ -483,6 +483,8 @@ struct quic_tx_packet {
 	int refcnt;
 	/* Next packet in the same datagram */
 	struct quic_tx_packet *next;
+	/* Previous packet in the same datagram */
+	struct quic_tx_packet *prev;
 	/* Largest acknowledged packet number if this packet contains an ACK frame */
 	int64_t largest_acked_pn;
 	unsigned char type;
diff --git a/src/quic_conn.c b/src/quic_conn.c
index 496aa69899..1d67248c67 100644
--- a/src/quic_conn.c
+++ b/src/quic_conn.c
@@ -1756,6 +1756,13 @@ static inline struct eb64_node *qc_ackrng_pkts(struct quic_conn *qc,
 		TRACE_DEVEL("Removing packet #", QUIC_EV_CONN_PRSAFRM, qc, NULL, &pkt->pn_node.key);
 		list_for_each_entry_safe(frm, frmbak, &pkt->frms, list)
 			qc_treat_acked_tx_frm(qc, frm);
+		/* If there are others packet in the same datagram <pkt> is attached to,
+		 * detach the previous one and the next one from <pkt>.
+		 */
+		if (pkt->prev)
+			pkt->prev->next = pkt->next;
+		if (pkt->next)
+			pkt->next->prev = pkt->prev;
 		node = eb64_prev(node);
 		eb64_delete(&pkt->pn_node);
 	}
@@ -3233,6 +3240,7 @@ static int qc_prep_pkts(struct quic_conn *qc, struct buffer *buf,
 		/* Attach the current one to the previous one */
 		if (prv_pkt) {
 			prv_pkt->next = cur_pkt;
+			cur_pkt->prev = prv_pkt;
 			cur_pkt->flags |= QUIC_FL_TX_PACKET_COALESCED;
 		}
 		/* Let's say we have to build a new dgram */
@@ -7110,6 +7118,7 @@ static inline void quic_tx_packet_init(struct quic_tx_packet *pkt, int type)
 	LIST_INIT(&pkt->frms);
 	pkt->time_sent = TICK_ETERNITY;
 	pkt->next = NULL;
+	pkt->prev = NULL;
 	pkt->largest_acked_pn = -1;
 	pkt->flags = 0;
 	pkt->refcnt = 0;