From 8bc339a6ad4702f2c39b2a78aaaff665d85c762b Mon Sep 17 00:00:00 2001 From: Amaury Denoyelle Date: Mon, 11 Aug 2025 17:54:39 +0200 Subject: [PATCH] BUG/MAJOR: quic: fix INITIAL padding with probing packet only A QUIC datagram that contains an INITIAL packet must be padded to 1.200 bytes to prevent any deadlock due to anti-amplification protection. This is implemented by encoding a PADDING frame on the last packet of the datagram if necessary. Previously, qc_prep_pkts() was responsible to activate padding when calling qc_do_build_pkt(), as it knows which packet is the last to encode. However, this has the side-effect of preventing PING emission for probing with no data as this case was handled in an else-if branch after padding. This was fixed by the below commit 217e467e89d15f3c22e11fe144458afbf718c8a8 BUG/MINOR: quic: fix malformed probing packet building Above logic was altered to fix the PING case : padding was set to false explicitely in qc_prep_pkts(). Padding was then added in a specific block dedicated to the PING case in qc_do_build_pkt() itself for INITIAL packets. However, the fix is incorrect if the last QEL used to built a packet is not the initial one and probing is used with PING frame only. In this case, specific block in qc_do_build_pkt() does not add padding. This causes a BUG_ON() crash in qc_txb_store() which catches these packets as irregularly formed. To fix this while also properly handling PING emission, revert to the original padding logic : qc_prep_pkts() is responsible to activate INITIAL padding. To not interfere with PING emission, qc_do_build_pkt() body is adjusted so that PING block is moved up in the function and detached from the padding condition. The main benefit from this patch is that INITIAL padding decision in qc_prep_pkts() is clearer now. Note that padding can also be activated by qc_do_build_pkt(), as packets should be big enough for header protection decipher. However, this case is different from INITIAL padding, so it is not covered by this patch. This should be backported up to 2.6. --- src/quic_tx.c | 92 ++++++++++++++++++--------------------------------- 1 file changed, 33 insertions(+), 59 deletions(-) diff --git a/src/quic_tx.c b/src/quic_tx.c index 48979aa00..99a51d92c 100644 --- a/src/quic_tx.c +++ b/src/quic_tx.c @@ -689,6 +689,12 @@ static int qc_prep_pkts(struct quic_conn *qc, struct buffer *buf, * datagrams carrying ack-eliciting Initial packets to at least the * smallest allowed maximum datagram size of 1200 bytes. */ + + /* Padding is activated as soon as Initial data is + * present in the datagram. The only exception is on + * server side if Initial content is not ack-eliciting + * (e.g. a single ACK frame sent as Initial). + */ if (qel == qc->iel && (qc_is_back(qc) || !LIST_ISEMPTY(frms) || probe)) { /* Ensure that no Initial packets are sent into too small datagrams */ if (end - pos < QUIC_INITIAL_PACKET_MINLEN) { @@ -697,32 +703,13 @@ static int qc_prep_pkts(struct quic_conn *qc, struct buffer *buf, break; } - /* padding MUST ALWAYS be set for the last QEL, except: - * - for a listener, when probing, that is to say - * 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); - /* For listeners: - * 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()). - * - * For clients: - * must be set to 1 only the current packet cannot be coalesced, - * i.e. if the next qel is not present or empty. - */ cur_pkt = qc_build_pkt(&pos, end, qel, tls_ctx, frms, qc, ver, dglen, pkt_type, must_ack, - padding && - ((qc_is_back(qc) && (!next_qel || LIST_ISEMPTY(next_qel->send_frms))) || - (!qc_is_back(qc) && !next_qel && (!probe || !LIST_ISEMPTY(frms)))), + padding && !next_qel, probe, cc, &err); if (!cur_pkt) { switch (err) { @@ -1755,19 +1742,14 @@ 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. * + * Caller must set when building the last packet of a datagram which + * must be at least 1.200 bytes long. Note that padding can also be added + * automatically to ensure packet size is big enough for header protection + * sampling. + * * 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. */ @@ -1952,6 +1934,22 @@ static int qc_do_build_pkt(unsigned char *pos, const unsigned char *end, dglen += len_sz; } + /* TODO qc_do_build_pkt() must rely on its argument instead of using QEL field. */ + if (qel->pktns->tx.pto_probe && LIST_ISEMPTY(&frm_list) && !cc) { + /* Send PING if probing required but no frame avail for sending. */ + add_ping_frm = 1; + len += 1; + dglen += 1; + + /* Ensure packet is big enough so that header protection sample + * decryption can be performed. Note that +1 is for the PING + * frame. + */ + if (!padding && *pn_len + 1 < QUIC_PACKET_PN_MAXLEN) + len += padding_len = QUIC_PACKET_PN_MAXLEN - *pn_len - 1; + } + + /* Handle Initial packet padding if necessary. */ if (padding && dglen < QUIC_INITIAL_PACKET_MINLEN) { padding_len = QUIC_INITIAL_PACKET_MINLEN - dglen; @@ -1969,35 +1967,11 @@ static int qc_do_build_pkt(unsigned char *pos, const unsigned char *end, else if (len_frms && len_frms < QUIC_PACKET_PN_MAXLEN) { len += padding_len = QUIC_PACKET_PN_MAXLEN - len_frms; } - else if (LIST_ISEMPTY(&frm_list) && !cc) { - /* TODO qc_do_build_pkt() must rely on its argument instead of using QEL field. */ - if (qel->pktns->tx.pto_probe) { - /* If we cannot send a frame, we send a PING frame. */ - add_ping_frm = 1; - len += 1; - dglen += 1; - /* Note that only we are in the case where this Initial packet - * is not coalesced to an Handshake packet. We must directly - * pad the datragram. - */ - if (pkt->type == QUIC_PACKET_TYPE_INITIAL) { - if (dglen < QUIC_INITIAL_PACKET_MINLEN) { - padding_len = QUIC_INITIAL_PACKET_MINLEN - dglen; - padding_len -= quic_int_getsize(len + padding_len) - len_sz; - len += padding_len; - } - } - else { - /* Note that +1 is for the PING frame */ - if (*pn_len + 1 < QUIC_PACKET_PN_MAXLEN) - len += padding_len = QUIC_PACKET_PN_MAXLEN - *pn_len - 1; - } - } - else { - /* If there is no frame at all to follow, add at least a PADDING frame. */ - if (!ack_frm_len) - len += padding_len = QUIC_PACKET_PN_MAXLEN - *pn_len; - } + /* TODO qc_do_build_pkt() must rely on its argument instead of using QEL field. */ + else if (LIST_ISEMPTY(&frm_list) && !cc && !qel->pktns->tx.pto_probe) { + /* If there is no frame at all to follow, add at least a PADDING frame. */ + if (!ack_frm_len) + len += padding_len = QUIC_PACKET_PN_MAXLEN - *pn_len; } if (pkt->type != QUIC_PACKET_TYPE_SHORT && !quic_enc_int(&pos, end, len))