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.

diff --git a/src/quic_tx.c b/src/quic_tx.c
index 48979aa00953e277e5aaec2ee292ee310f554d7f..99a51d92c6bef7642165e4af293f7f858b266d34 100644 (file)
--- 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:
-                        * <paddding> 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 <probe> and LIST_EMPTY(<frms>)
-                        * conditions are verified (see qc_do_build_pkt()).
-                        *
-                        * For clients:
-                        * <padding> 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. <pn_len> will also have the packet number
  * length as value.
  *
+ * Caller must set <padding> 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, <frms> 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 <padding> 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 <padding> 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 <frms>
- * and when probing with only a PING frame.
- * Finally, if <frms> was empty, if <probe> 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 <probe> argument instead of using QEL <pto_probe> 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 <probe> argument instead of using QEL <pto_probe> 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 <probe> argument instead of using QEL <pto_probe> 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))