From f5f88a1225b2c1b597828d1dccb58b4e77e3a01b Mon Sep 17 00:00:00 2001 From: Alexandr Nedvedicky Date: Thu, 26 Mar 2026 14:24:32 +0100 Subject: [PATCH] QUIC stack must limit the number of PATH_CHALLENGE frames processed in RX Currently local QUIC stack allocates PATH_RESPONSE frame for every PATH_CHALLENGE frame it receives in single packet from its remote peer. The memory with PATH_RESPONSE frame is released after local QUIC stack receives an ACK which confirms reception of PATH_RESPONSE by remote peer. This gives remote peer too much control over memory resources local QUIC stack may consume. Quoting RFC 9000 section 9.2.1: ...an endpoint SHOULD NOT send multiple PATH_CHALLENGE frames in a single packet. Limiting the number of PATCH_CHALLENGE frames to 1 per QUIC packet received helps to reduce heap memory overhead required to process PATH_CHALLENGE frame. Currently QUIC ACKM (ACK-manager) keeps all frames in retransmission buffer until ACK is received. It can be changed such frames which don't need to be ACKed don't need to be kept in retrans buffer, those can be released right after transmission. Fixes CVE-2026-34183 Reviewed-by: Nikola Pajkovsky Reviewed-by: Neil Horman Reviewed-by: Tomas Mraz MergeDate: Mon Jun 8 14:43:13 2026 --- include/internal/quic_cfq.h | 1 + include/internal/quic_channel.h | 1 + include/internal/quic_fifd.h | 1 + ssl/quic/quic_cfq.c | 15 +++++++++ ssl/quic/quic_channel.c | 6 ++++ ssl/quic/quic_channel_local.h | 40 ++++++++++++++++++++++ ssl/quic/quic_fifd.c | 43 +++++++++++++++++++++++ ssl/quic/quic_rx_depack.c | 60 ++++++++++++++++++++------------- ssl/quic/quic_txp.c | 2 ++ 9 files changed, 145 insertions(+), 24 deletions(-) diff --git a/include/internal/quic_cfq.h b/include/internal/quic_cfq.h index 0b2a3a4cb2..96c8d89eb6 100644 --- a/include/internal/quic_cfq.h +++ b/include/internal/quic_cfq.h @@ -149,6 +149,7 @@ QUIC_CFQ_ITEM *ossl_quic_cfq_get_priority_head(const QUIC_CFQ *cfq, QUIC_CFQ_ITEM *ossl_quic_cfq_item_get_priority_next(const QUIC_CFQ_ITEM *item, uint32_t pn_space); +int ossl_quic_cfq_discard_unreliable(QUIC_CFQ *cfq, QUIC_CFQ_ITEM *item); #endif #endif diff --git a/include/internal/quic_channel.h b/include/internal/quic_channel.h index fdf72b81f7..b4724627e7 100644 --- a/include/internal/quic_channel.h +++ b/include/internal/quic_channel.h @@ -444,6 +444,7 @@ uint64_t ossl_quic_channel_get_max_idle_timeout_peer_request(const QUIC_CHANNEL /* Get the idle timeout actually negotiated. */ uint64_t ossl_quic_channel_get_max_idle_timeout_actual(const QUIC_CHANNEL *ch); +void ossl_ch_reset_rx_state(QUIC_CHANNEL *ch); #endif #endif diff --git a/include/internal/quic_fifd.h b/include/internal/quic_fifd.h index 4ea7a2e0d2..afa330cbc4 100644 --- a/include/internal/quic_fifd.h +++ b/include/internal/quic_fifd.h @@ -83,6 +83,7 @@ int ossl_quic_fifd_pkt_commit(QUIC_FIFD *fifd, QUIC_TXPIM_PKT *pkt); void ossl_quic_fifd_set_qlog_cb(QUIC_FIFD *fifd, QLOG *(*get_qlog_cb)(void *arg), void *arg); +void ossl_quic_fifd_pkt_discard_unreliable(QUIC_FIFD *fifd, QUIC_TXPIM_PKT *tpkt); #endif #endif diff --git a/ssl/quic/quic_cfq.c b/ssl/quic/quic_cfq.c index 8fbed16d34..a88b56eae0 100644 --- a/ssl/quic/quic_cfq.c +++ b/ssl/quic/quic_cfq.c @@ -7,6 +7,7 @@ * https://www.openssl.org/source/license.html */ +#include "internal/quic_channel.h" #include "internal/quic_cfq.h" #include "internal/numbers.h" @@ -307,6 +308,20 @@ void ossl_quic_cfq_mark_lost(QUIC_CFQ *cfq, QUIC_CFQ_ITEM *item, } } +int ossl_quic_cfq_discard_unreliable(QUIC_CFQ *cfq, QUIC_CFQ_ITEM *item) +{ + int discarded; + + if (ossl_quic_cfq_item_is_unreliable(item)) { + ossl_quic_cfq_release(cfq, item); + discarded = 1; + } else { + discarded = 0; + } + + return discarded; +} + /* * Releases a CFQ item. The item may be in either state (NEW or TX) prior to the * call. The QUIC_CFQ_ITEM pointer must not be used following this call. diff --git a/ssl/quic/quic_channel.c b/ssl/quic/quic_channel.c index 8517a1a818..f629da58d5 100644 --- a/ssl/quic/quic_channel.c +++ b/ssl/quic/quic_channel.c @@ -2035,6 +2035,12 @@ static void ch_rx_check_forged_pkt_limit(QUIC_CHANNEL *ch) "forgery limit"); } +void ossl_ch_reset_rx_state(QUIC_CHANNEL *ch) +{ + ch->did_crypto_frame = 0; + ch->seen_path_challenge = 0; +} + /* Process queued incoming packets and handle frames, if any. */ static int ch_rx(QUIC_CHANNEL *ch, int channel_only) { diff --git a/ssl/quic/quic_channel_local.h b/ssl/quic/quic_channel_local.h index 0bb1b4465e..bbcbd1160e 100644 --- a/ssl/quic/quic_channel_local.h +++ b/ssl/quic/quic_channel_local.h @@ -11,6 +11,28 @@ #include "internal/quic_fc.h" #include "internal/quic_stream_map.h" +/* + * This is a part of PATH_CHALLENGE flood [1] mitigation. This limits the + * number of PATH_CHALLENGE frames QUIC stack is willing to process for + * connection. Local QUIC stack creates PATH_RESPONSE frame for PATH_CHALLENGE + * frame it receives from remote peer. The response frame is put Control Frame + * Queue waiting to be dispatched. The PATH_RESPONSE frame is removed from CFQ + * after it is dispatched. The QUIC_PATH_RESPONSE_QLEN limits the number of + * PATH_RESPONSE frames waiting to be dispatched. No new PATH_RESPONSE frames + * are inserted into CFQ if queue limit is exceeded. + * + * QUIC implementations use different limits for PATH_RESPONSE queue lengths: + * quic-go defines maxPathResponses as 256 + * quiche from cloadflare sets DEFAULT_MAX_PATH_CHALLENGE_RX_QUEUE_LEN to 3 + * t-quic from tencent chooses MAX_PATH_CHALS_RECV to be 8 + * + * OpenSSL here introduces QUIC_PATH_RESPONSE_QLEN as 32. + * + * [1] https://www.ietf.org/archive/id/draft-chen-quic-logical-vuln-mitigations-00.txt + * (section 4.2) + */ +#define QUIC_PATH_RESPONSE_QLEN 32 + /* * QUIC Channel Structure * ====================== @@ -437,6 +459,19 @@ struct quic_channel_st { /* Has qlog been requested? */ unsigned int use_qlog : 1; + /* + * RFC 9000 Section 9.2.1 says: + * However, an endpoint SHOULD NOT send multiple + * PATH_CHALLENGE frames in a single packet. + * The counter here allows us to detect multiple presence + * of PATH_CHALLENGE frame in packet. We process only the + * first PATH_CHALLENGE frame found in packet. Remaining PATH_CHALLENGE + * frames are ignored. + * seen_path_challenge flag is always reset before + * ossl_quic_handle_frames() gets called. + */ + unsigned int seen_path_challenge : 1; + /* Saved error stack in case permanent error was encountered */ ERR_STATE *err_state; @@ -446,6 +481,11 @@ struct quic_channel_st { /* Title for qlog purposes. We own this copy. */ char *qlog_title; + /* + * number of path responses waiting to be dispatched + * from control frame queue (CFQ) + */ + unsigned int path_response_limit; }; #endif diff --git a/ssl/quic/quic_fifd.c b/ssl/quic/quic_fifd.c index 03b8cebd30..e80483b501 100644 --- a/ssl/quic/quic_fifd.c +++ b/ssl/quic/quic_fifd.c @@ -310,3 +310,46 @@ void ossl_quic_fifd_set_qlog_cb(QUIC_FIFD *fifd, QLOG *(*get_qlog_cb)(void *arg) fifd->get_qlog_cb = get_qlog_cb; fifd->get_qlog_cb_arg = get_qlog_cb_arg; } + +static void txpim_pkt_remove_cfq_item(QUIC_TXPIM_PKT *pkt, QUIC_CFQ_ITEM *cfq_item) +{ + QUIC_CFQ_ITEM *prev = cfq_item->pkt_prev; + + if (prev != NULL) { + prev->pkt_next = cfq_item->pkt_next; + } else { + pkt->retx_head = cfq_item->pkt_next; + } + + if (cfq_item->pkt_next != NULL) + cfq_item->pkt_next->pkt_prev = prev; + + cfq_item->pkt_prev = NULL; + cfq_item->pkt_next = NULL; +} + +void ossl_quic_fifd_pkt_discard_unreliable(QUIC_FIFD *fifd, QUIC_TXPIM_PKT *pkt) +{ + QUIC_CFQ_ITEM *cfq_item, *cfq_next; + + /* + * The packet has been written to network. We can discard frames we don't + * retransmit when loss is detected. + */ + cfq_item = pkt->retx_head; + while (cfq_item != NULL) { + /* + * Discarded items are moved to free list. If item + * got moved to free list we must also remove it from + * cfq list kept in pkt, so ACKM does not find it when + * receives an ACK for pkt. + */ + if (ossl_quic_cfq_discard_unreliable(fifd->cfq, cfq_item)) { + cfq_next = cfq_item->pkt_next; + txpim_pkt_remove_cfq_item(pkt, cfq_item); + cfq_item = cfq_next; + } else { + cfq_item = cfq_item->pkt_next; + } + } +} diff --git a/ssl/quic/quic_rx_depack.c b/ssl/quic/quic_rx_depack.c index 6a67e44dc3..eee572da4a 100644 --- a/ssl/quic/quic_rx_depack.c +++ b/ssl/quic/quic_rx_depack.c @@ -928,6 +928,12 @@ static int depack_do_frame_retire_conn_id(PACKET *pkt, static void free_path_response(unsigned char *buf, size_t buf_len, void *arg) { + QUIC_CHANNEL *ch = (QUIC_CHANNEL *)arg; + + assert(ch->path_response_limit > 0); + + ch->path_response_limit--; + OPENSSL_free(buf); } @@ -948,33 +954,39 @@ static int depack_do_frame_path_challenge(PACKET *pkt, return 0; } - /* - * RFC 9000 s. 8.2.2: On receiving a PATH_CHALLENGE frame, an endpoint MUST - * respond by echoing the data contained in the PATH_CHALLENGE frame in a - * PATH_RESPONSE frame. - * - * TODO(QUIC FUTURE): We should try to avoid allocation here in the future. - */ - encoded_len = sizeof(uint64_t) + 1; - if ((encoded = OPENSSL_malloc(encoded_len)) == NULL) - goto err; + if (ch->seen_path_challenge == 0 + && ch->path_response_limit < QUIC_PATH_RESPONSE_QLEN) { + /* + * RFC 9000 s. 8.2.2: On receiving a PATH_CHALLENGE frame, an endpoint + * MUST respond by echoing the data contained in the PATH_CHALLENGE + * frame in a PATH_RESPONSE frame. + * + * TODO(QUIC FUTURE): We should try to avoid allocation here in the + * future. + */ + encoded_len = sizeof(uint64_t) + 1; + if ((encoded = OPENSSL_malloc(encoded_len)) == NULL) + goto err; - if (!WPACKET_init_static_len(&wpkt, encoded, encoded_len, 0)) - goto err; + if (!WPACKET_init_static_len(&wpkt, encoded, encoded_len, 0)) + goto err; - if (!ossl_quic_wire_encode_frame_path_response(&wpkt, frame_data)) { - WPACKET_cleanup(&wpkt); - goto err; - } + if (!ossl_quic_wire_encode_frame_path_response(&wpkt, frame_data)) { + WPACKET_cleanup(&wpkt); + goto err; + } - WPACKET_finish(&wpkt); + WPACKET_finish(&wpkt); - if (!ossl_quic_cfq_add_frame(ch->cfq, 0, QUIC_PN_SPACE_APP, - OSSL_QUIC_FRAME_TYPE_PATH_RESPONSE, - QUIC_CFQ_ITEM_FLAG_UNRELIABLE, - encoded, encoded_len, - free_path_response, NULL)) - goto err; + if (!ossl_quic_cfq_add_frame(ch->cfq, 0, QUIC_PN_SPACE_APP, + OSSL_QUIC_FRAME_TYPE_PATH_RESPONSE, + QUIC_CFQ_ITEM_FLAG_UNRELIABLE, + encoded, encoded_len, + free_path_response, ch)) + goto err; + ch->seen_path_challenge = 1; + ch->path_response_limit++; + } return 1; @@ -1415,7 +1427,7 @@ int ossl_quic_handle_frames(QUIC_CHANNEL *ch, OSSL_QRX_PKT *qpacket) if (ch == NULL) return 0; - ch->did_crypto_frame = 0; + ossl_ch_reset_rx_state(ch); /* Initialize |ackm_data| (and reinitialize |ok|)*/ memset(&ackm_data, 0, sizeof(ackm_data)); diff --git a/ssl/quic/quic_txp.c b/ssl/quic/quic_txp.c index 6997dc9ee3..5acc3fe0c1 100644 --- a/ssl/quic/quic_txp.c +++ b/ssl/quic/quic_txp.c @@ -3033,6 +3033,8 @@ static int txp_pkt_commit(OSSL_QUIC_TX_PACKETISER *txp, --probe_info->pto[pn_space]; } + ossl_quic_fifd_pkt_discard_unreliable(&txp->fifd, tpkt); + return rc; } -- 2.55.0