From 217e467e89d15f3c22e11fe144458afbf718c8a8 Mon Sep 17 00:00:00 2001 From: Frederic Lecaille Date: Mon, 4 Nov 2024 18:50:10 +0100 Subject: [PATCH] BUG/MINOR: quic: fix malformed probing packet building This bug arrived with this commit: cdfceb10a MINOR: quic: refactor qc_prep_pkts() loop which prevents haproxy from sending PING only packets/datagrams (some packets/datagrams with only PING frame as ack-eliciting frames inside). Such packets/datagrams are useful in rare cases during retransmissions when one wants to probe the peer without exceeding the anti-amplification limit. Modify the condition passed to qc_build_pkt() to add padding to the current datagram. One does not want to do that when probing the peer without ack-eliciting frames passed as parameter. Indeed qc_build_pkt() calls qc_do_build_pkt() which supports this case: if is true (probing required), qc_do_build_pkt() handles the case where some padding must be added to a PING only packet/datagram. This is the case when probing with an empty frame list of ack-eliciting frames without exceeding the anti-amplification limit from qc_dgrams_retransmit(). Add some comments to qc_build_pkt() and qc_do_build_pkt() to clarify this as this code is easy to break! Thank you for @Tristan971 for having reported this issue in GH #2709. Must be backported to 3.0. --- src/quic_tx.c | 30 +++++++++++++++++++++++++++--- 1 file changed, 27 insertions(+), 3 deletions(-) diff --git a/src/quic_tx.c b/src/quic_tx.c index 4a72e9fe94396..fc89ca66b99f2 100644 --- a/src/quic_tx.c +++ b/src/quic_tx.c @@ -623,14 +623,24 @@ static int qc_prep_pkts(struct quic_conn *qc, struct buffer *buf, break; } - /* padding will be set for last QEL */ + /* padding will be set for last QEL, except when probing: + * to build a PING only non coalesced Initial datagram for + * instance when blocked by the anti-amplification limit, + * this datagram MUST be padded. + */ padding = 1; } pkt_type = quic_enc_level_pkt_type(qc, qel); + /* parameter for qc_build_pkt() must not be set to 1 when + * building PING only Initial datagram (a datagram with an Initial + * packet inside containing only a PING frame as ack-eliciting + * frame). This is the case when both and LIST_EMPTY() + * conditions are verified (see qc_do_build_pkt()). + */ cur_pkt = qc_build_pkt(&pos, end, qel, tls_ctx, frms, - qc, ver, dglen, pkt_type, - must_ack, padding && !next_qel, + qc, ver, dglen, pkt_type, must_ack, + padding && !next_qel && (!probe || !LIST_ISEMPTY(frms)), probe, cc, &err); if (!cur_pkt) { switch (err) { @@ -1812,6 +1822,20 @@ static inline uint64_t quic_compute_ack_delay_us(unsigned int time_received, * number field in this packet. will also have the packet number * length as value. * + * NOTE: This function does not build all the possible combinations of packets + * depending on its list of parameters. In most cases, frame list is + * not empty. So, this function first tries to build this list of frames. + * Then some padding is added to this packet if boolean is set true. + * The unique case one wants to do that is when a first Initial packet was + * previously built into the same datagram as the currently built one and when + * this packet is supposed to pad the datagram, if needed, to build an at + * least 1200 bytes long Initial datagram. + * If is not true, if the packet is too short, the packet is also + * padded. This is very often the case when no frames are provided by + * and when probing with only a PING frame. + * Finally, if was empty, if boolean is true this function builds + * a PING only packet handling also the cases where it must be padded. + * * Return 1 if succeeded (enough room to buile this packet), O if not. */ static int qc_do_build_pkt(unsigned char *pos, const unsigned char *end,