BUG/MINOR: quic: too short PADDING frame for too short packets

This bug arrvived with this commit:

    MINOR: quic: centralize padding for HP sampling on packet building

What was missed is the fact that at the centralization point for the
PADDING frame to add for too short packet, <len> payload length  already includes
<*pn_len> the packet number field length value.

So when computing the length of the PADDING frame, the packet field length must
not be considered and added to the payload length (<len>).

This bug leaded too short PADDING frame to too short packets. This was the case,
most of times with Application level packets with a 1-byte packet number field
followed by a 1-byte PING frame. A 1-byte PADDING frame was added in this case
in place of a correct 2-bytes PADDINF frame. The header packet protection of
such packet could not be removed by the clients as for instance for ngtcp2 with
such traces:

    I00001828 0x5a135c81e803f092c74bac64a85513b657 pkt could not decrypt packet number

As the header protection could no be removed, the header keyupdate bit could also
not be read by packet analyzers such as pyshark used during the keyupdate tests.

No need to backport.

diff --git a/include/haproxy/quic_conn-t.h b/include/haproxy/quic_conn-t.h
index 75493915d5345e6121aea0afd25a21f0ef031fe1..4100e3061f30695a2faef37a35fbe8703aacb150 100644 (file)
--- a/include/haproxy/quic_conn-t.h
+++ b/include/haproxy/quic_conn-t.h
@@ -145,9 +145,6 @@ enum quic_pkt_type {
 #define QUIC_PACKET_PNL_BITMASK      0x03
 #define QUIC_PACKET_PN_MAXLEN        4
 
-/* TLS algo supported by QUIC uses a 16-bytes sample for HP. */
-#define QUIC_HP_SAMPLE_LEN           16
-
 /*
  *  0                   1                   2                   3
  *  0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1

diff --git a/src/quic_tx.c b/src/quic_tx.c
index 8a8f05dc37da1c55aa8a8058725eab0eec9b837c..7b57e0135f36ef2b2016b2a11a3c3f839a265c60 100644 (file)
--- a/src/quic_tx.c
+++ b/src/quic_tx.c
@@ -1990,12 +1990,16 @@ static int qc_do_build_pkt(unsigned char *pos, const unsigned char *end,
         */
 
        /* Add padding if packet is too small for HP sampling as specified
-        * above. QUIC TLS algos relies on 16 bytes sample extracted 4 bytes
-        * after PN offset. Thus, pn and payload must be at least 4 bytes long,
-        * so that the sample will be extracted as the AEAD tag.
+        * above. QUIC TLS algos relies on 16 bytes sample extracted
+        * QUIC_PACKET_PN_MAXLEN(4) bytes after the PN offset Thus, pn and payload
+        * must be at least QUIC_PACKET_PN_MAXLEN(4) bytes long, so that the sample
+        * will be extracted as the AEAD tag.
+        *
+        * Note that from here, <len> includes <*pn_len>, the total frame lenghts,
+        * and QUIC_TLS_TAG_LEN(16).
         */
-       if (*pn_len + len < QUIC_PACKET_PN_MAXLEN + QUIC_HP_SAMPLE_LEN) {
-               padding_len = QUIC_PACKET_PN_MAXLEN + QUIC_HP_SAMPLE_LEN - (*pn_len + len);
+       if (len < QUIC_PACKET_PN_MAXLEN + QUIC_TLS_TAG_LEN) {
+               padding_len = QUIC_PACKET_PN_MAXLEN + QUIC_TLS_TAG_LEN - len;
                TRACE_PRINTF(TRACE_LEVEL_DEVELOPER, QUIC_EV_CONN_PHPKTS, qc, 0, 0, 0,
                             "adding padding pn=%llu padding_len=%zu *pn_len=%zu"
                             " len=%zu len_frms=%zu",