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.
This commit is contained in:
Frederic Lecaille 2025-09-05 15:41:27 +02:00
parent 71336bdd08
commit d7dea408c6
2 changed files with 9 additions and 8 deletions

View File

@ -145,9 +145,6 @@ enum quic_pkt_type {
#define QUIC_PACKET_PNL_BITMASK 0x03 #define QUIC_PACKET_PNL_BITMASK 0x03
#define QUIC_PACKET_PN_MAXLEN 4 #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
* 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 * 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

View File

@ -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 /* Add padding if packet is too small for HP sampling as specified
* above. QUIC TLS algos relies on 16 bytes sample extracted 4 bytes * above. QUIC TLS algos relies on 16 bytes sample extracted
* after PN offset. Thus, pn and payload must be at least 4 bytes long, * QUIC_PACKET_PN_MAXLEN(4) bytes after the PN offset Thus, pn and payload
* so that the sample will be extracted as the AEAD tag. * 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) { if (len < QUIC_PACKET_PN_MAXLEN + QUIC_TLS_TAG_LEN) {
padding_len = QUIC_PACKET_PN_MAXLEN + QUIC_HP_SAMPLE_LEN - (*pn_len + 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, TRACE_PRINTF(TRACE_LEVEL_DEVELOPER, QUIC_EV_CONN_PHPKTS, qc, 0, 0, 0,
"adding padding pn=%llu padding_len=%zu *pn_len=%zu" "adding padding pn=%llu padding_len=%zu *pn_len=%zu"
" len=%zu len_frms=%zu", " len=%zu len_frms=%zu",