sinjuginas/haproxy
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

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

Browse Source 
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.
This commit is contained in:
Frdric Lcaille 2022-11-18 18:15:28 +01:00 committed by Willy Tarreau
parent d9b7174d99
commit 814645f42f
2 changed files with 11 additions and 0 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
include/haproxy/quic_conn-t.h
View File

@ -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;

9
src/quic_conn.c
View File

@ -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;