Skip to content

Commit 7308946

Browse files
nhormanjogme
nhorman
authored and
jogme
committed
Don't store ACK-only frames in TX history for QUIC.
When QUIC sends an ACK-only frame, there is no expectation that the peer will ack that ack (i.e. it is itself not ack-eliciting). However, our implementation stores these frames in the TX history regardless. In and of itself thats ok, but if a malicious client establishes a connection, and then drives the connection such that ack-only frames are forced from the peer (i.e. by sending numerous ping frames), and then withholding any subseqent acks for ack-eliciting data, like legitimate data, said malicious client can force inappropriate memory growth on the server, leading to potential DOS attacks. Don't store any ACK-only frames in the TX history to address this. Record it in our TX history so that the send window moves forward appropriately, but for ack-only frames, immediately remove it, since we don't expect to get an ack for them anyway. Initially authored by Opal Wright <opal.wright@trailofbits.com> The initial proposal had some shortcommings in which the highest pn acked value was not accounted for which I have fixed with the assistance of Claude Assisted-by: Anthopic Sonnet 5 Fixes CVE-2026-63075 Reviewed-by: Saša Nedvědický <sashan@openssl.org> Reviewed-by: Bob Beck <beck@openssl.org> Merge-date: Mon Aug 24 12:09:01 2026
1 parent a17cc8d commit 7308946

3 files changed

Lines changed: 71 additions & 12 deletions

File tree

‎include/internal/quic_ackm.h‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,11 @@ struct ossl_ackm_tx_pkt_st {
129129
};
130130

131131
int ossl_ackm_on_tx_packet(OSSL_ACKM *ackm, OSSL_ACKM_TX_PKT *pkt);
132+
133+
/*
134+
* Records transmission of a packet containing only ACK frames.
135+
*/
136+
int ossl_ackm_on_tx_ack_only_packet(OSSL_ACKM *ackm, OSSL_ACKM_TX_PKT *pkt);
132137
int ossl_ackm_on_rx_datagram(OSSL_ACKM *ackm, size_t num_bytes);
133138

134139
#define OSSL_ACKM_ECN_NONE 0

‎ssl/quic/quic_ackm.c‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1135,6 +1135,38 @@ int ossl_ackm_on_tx_packet(OSSL_ACKM *ackm, OSSL_ACKM_TX_PKT *pkt)
11351135
return 1;
11361136
}
11371137

1138+
int ossl_ackm_on_tx_ack_only_packet(OSSL_ACKM *ackm, OSSL_ACKM_TX_PKT *pkt)
1139+
{
1140+
struct tx_pkt_history_st *h;
1141+
unsigned int pkt_space;
1142+
1143+
if (pkt == NULL || pkt->pkt_space >= QUIC_PN_SPACE_NUM)
1144+
return 0;
1145+
1146+
/*
1147+
* A packet containing only an ACK frame must not be treated as
1148+
* in-flight or ack-eliciting; if it were, ossl_ackm_on_tx_packet()
1149+
* below would (correctly) perform bytes-in-flight/timer/CC bookkeeping
1150+
* for a packet we are about to discard from history, which would be
1151+
* incorrect.
1152+
*/
1153+
if (pkt->is_inflight || pkt->is_ack_eliciting)
1154+
return 0;
1155+
1156+
pkt_space = pkt->pkt_space;
1157+
1158+
/*
1159+
* No one can expect ACK for packet which carries ACK frames only
1160+
* (ack_only packet). The ACKM does not need to keep record for ack_only
1161+
* packet. For ack_only packet the ACKM manager must be updated by the
1162+
* highest packet number which got sent.
1163+
*/
1164+
h = get_tx_history(ackm, pkt_space);
1165+
h->highest_sent = pkt->pkt_num;
1166+
1167+
return 1;
1168+
}
1169+
11381170
int ossl_ackm_on_rx_datagram(OSSL_ACKM *ackm, size_t num_bytes)
11391171
{
11401172
/* No-op on the client. */

‎ssl/quic/quic_txp.c‎

Lines changed: 34 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2935,6 +2935,20 @@ static int txp_generate_for_el(OSSL_QUIC_TX_PACKETISER *txp,
29352935
return TXP_ERR_INTERNAL;
29362936
}
29372937

2938+
static int txp_pkt_is_ack_only(const QUIC_TXPIM_PKT *tpkt)
2939+
{
2940+
return tpkt->had_ack_frame
2941+
&& !tpkt->ackm_pkt.is_inflight
2942+
&& !tpkt->ackm_pkt.is_ack_eliciting
2943+
&& !tpkt->had_handshake_done_frame
2944+
&& !tpkt->had_max_data_frame
2945+
&& !tpkt->had_max_streams_bidi_frame
2946+
&& !tpkt->had_max_streams_uni_frame
2947+
&& !tpkt->had_conn_close
2948+
&& tpkt->retx_head == NULL
2949+
&& ossl_quic_txpim_pkt_get_num_chunks(tpkt) == 0;
2950+
}
2951+
29382952
/*
29392953
* Commits and queues a packet for transmission. There is no backing out after
29402954
* this.
@@ -2943,8 +2957,9 @@ static int txp_generate_for_el(OSSL_QUIC_TX_PACKETISER *txp,
29432957
*
29442958
* - Sends the packet to the QTX for encryption and transmission;
29452959
*
2946-
* - Records the packet as having been transmitted in FIFM. ACKM is informed,
2947-
* etc. and the TXPIM record is filed.
2960+
* - Records non-ACK-only packets as having been transmitted in FIFM. ACKM is
2961+
* informed, etc. and the TXPIM record is filed only when later callbacks
2962+
* need it.
29482963
*
29492964
* - Informs various subsystems of frames that were sent and clears frame
29502965
* wanted flags so that we do not generate the same frames again.
@@ -2971,7 +2986,7 @@ static int txp_pkt_commit(OSSL_QUIC_TX_PACKETISER *txp,
29712986
uint32_t archetype,
29722987
int *txpim_pkt_reffed)
29732988
{
2974-
int rc = 1;
2989+
int ack_only, rc = 1;
29752990
uint32_t enc_level = pkt->h.enc_level;
29762991
uint32_t pn_space = ossl_quic_enc_level_to_pn_space(enc_level);
29772992
QUIC_TXPIM_PKT *tpkt = pkt->tpkt;
@@ -3015,28 +3030,35 @@ static int txp_pkt_commit(OSSL_QUIC_TX_PACKETISER *txp,
30153030
return 0; /* alloc error */
30163031
}
30173032

3018-
/* Dispatch to FIFD. */
3019-
if (!ossl_quic_fifd_pkt_commit(&txp->fifd, tpkt))
3033+
ack_only = txp_pkt_is_ack_only(tpkt);
3034+
3035+
/* Dispatch packets that need loss/retransmit callbacks to FIFD. */
3036+
if (!ack_only && !ossl_quic_fifd_pkt_commit(&txp->fifd, tpkt))
30203037
return 0;
30213038

30223039
/*
30233040
* Transmission and Post-Packet Generation Bookkeeping
30243041
* ===================================================
30253042
*
3026-
* No backing out anymore - at this point the ACKM has recorded the packet
3027-
* as having been sent, so we need to increment our next PN counter, or
3028-
* the ACKM will complain when we try to record a duplicate packet with
3029-
* the same PN later. At this point actually sending the packet may still
3030-
* fail. In this unlikely event it will simply be handled as though it
3031-
* were a lost packet.
3043+
* No backing out anymore - at this point we need to increment our next PN
3044+
* counter, or the ACKM will complain when we try to record a duplicate
3045+
* packet with the same PN later. Non-ACK-only packets have also been
3046+
* recorded in ACKM, so if QTX write fails they are handled as though they
3047+
* were lost. ACK-only packets are not recorded and will be cleaned up by
3048+
* the caller.
30323049
*/
30333050
++txp->next_pn[pn_space];
3034-
*txpim_pkt_reffed = 1;
3051+
if (!ack_only)
3052+
*txpim_pkt_reffed = 1;
30353053

30363054
/* Send the packet. */
30373055
if (!ossl_qtx_write_pkt(txp->args.qtx, &txpkt))
30383056
return 0;
30393057

3058+
if (ack_only
3059+
&& !ossl_ackm_on_tx_ack_only_packet(txp->args.ackm, &tpkt->ackm_pkt))
3060+
rc = 0;
3061+
30403062
/*
30413063
* Record FC and stream abort frames as sent; deactivate streams which no
30423064
* longer have anything to do.

0 commit comments

Comments
 (0)