From bba6baff306820a01ea66fddde5acbad11c601b6 Mon Sep 17 00:00:00 2001 From: Amaury Denoyelle Date: Mon, 29 Jul 2024 10:42:50 +0200 Subject: [PATCH] BUG/MEDIUM: quic: prevent conn freeze on 0RTT undeciphered content Received QUIC packets are stored in quic_conn Rx buffer after header protection removal in qc_rx_pkt_handle(). These packets are then removed after quic_conn IO handler via qc_treat_rx_pkts(). If HP cannot be removed, packets are still copied into quic_conn Rx buffer. This can happen if encryption level TLS keys are not yet available. The packet remains in the buffer until HP can be removed and its content processed. An issue occurs if client emits a 0-RTT packet but haproxy does not have the shared secret, for example after a haproxy process restart. In this case, the packet is copied in quic_conn Rx buffer but its HP won't ever be removed. This prevents the buffer to be purged. After some time, if the client has emitted enough packets, Rx buffer won't have any space left and received packets are dropped. This will cause the connection to freeze. To fix this, remove any 0-RTT buffered packets on handshake completion. At this stage, 0-RTT packets are unnecessary anymore. The client is expected to reemit its content in 1-RTT packet which are properly deciphered. This can easily reproduce with HTTP/3 POST requests or retrieving a big enough object, which will fill the Rx buffer with ACK frames. Here is a picoquic command to provoke the issue on haproxy startup : $ picoquicdemo -Q -v 00000001 -a h3 20443 "/?s=1g" Note that allow-0rtt must be present on the bind line to trigger the issue. Else haproxy will reject any 0-RTT packets. This must be backported up to 2.6. This could be one of the reason for github issue #2549 but it's unsure for now. --- src/quic_conn.c | 27 +++++++++++++++++++++++++++ src/quic_rx.c | 2 +- 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/src/quic_conn.c b/src/quic_conn.c index 9c6a09046..3019c848d 100644 --- a/src/quic_conn.c +++ b/src/quic_conn.c @@ -804,6 +804,33 @@ struct task *quic_conn_io_cb(struct task *t, void *context, unsigned int state) qc_el_rx_pkts_del(qc->hel); qc_release_pktns_frms(qc, qc->hel->pktns); } + + /* Release 0RTT packets still waiting for HP removal. These + * packets are considered unneeded after handshake completion. + * They will be freed later from Rx buf via quic_rx_pkts_del(). + */ + if (qc->eel && !LIST_ISEMPTY(&qc->eel->rx.pqpkts)) { + struct quic_rx_packet *pqpkt, *pkttmp; + list_for_each_entry_safe(pqpkt, pkttmp, &qc->eel->rx.pqpkts, list) { + LIST_DEL_INIT(&pqpkt->list); + quic_rx_packet_refdec(pqpkt); + } + + /* RFC 9001 4.9.3. Discarding 0-RTT Keys + * Additionally, a server MAY discard 0-RTT keys as soon as it receives + * a 1-RTT packet. However, due to packet reordering, a 0-RTT packet + * could arrive after a 1-RTT packet. Servers MAY temporarily retain 0- + * RTT keys to allow decrypting reordered packets without requiring + * their contents to be retransmitted with 1-RTT keys. After receiving a + * 1-RTT packet, servers MUST discard 0-RTT keys within a short time; + * the RECOMMENDED time period is three times the Probe Timeout (PTO, + * see [QUIC-RECOVERY]). A server MAY discard 0-RTT keys earlier if it + * determines that it has received all 0-RTT packets, which can be done + * by keeping track of missing packet numbers. + * + * TODO implement discarding of 0-RTT keys + */ + } } /* Insert each QEL into sending list if needed. */ diff --git a/src/quic_rx.c b/src/quic_rx.c index a2254e5e5..fd01c8f8b 100644 --- a/src/quic_rx.c +++ b/src/quic_rx.c @@ -1145,7 +1145,7 @@ static void qc_rm_hp_pkts(struct quic_conn *qc, struct quic_enc_level *el) quic_rx_packet_refinc(pqpkt); TRACE_PROTO("RX hp removed", QUIC_EV_CONN_ELRMHP, qc, pqpkt); } - LIST_DELETE(&pqpkt->list); + LIST_DEL_INIT(&pqpkt->list); quic_rx_packet_refdec(pqpkt); }