From: Alexandr Nedvedicky Date: Wed, 9 Jul 2025 09:24:38 +0000 (+0200) Subject: QUIC receiver may accidentally ACK packet it fails to process X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=e6c20588efa755c246f52d56891a889b201a015a;p=thirdparty%2Fopenssl.git QUIC receiver may accidentally ACK packet it fails to process we set ok to -1 as we enter ossl_quic_handle_frames(). If we set ok to 0 here we effectively assume successful processing of all frames found in packet. We do this just before we return from function: ``` 1479 1480 /* Now that special cases are out of the way, parse frames */ 1481 if (!PACKET_buf_init(&pkt, qpacket->hdr->data, qpacket->hdr->len) 1482 || !depack_process_frames(ch, &pkt, qpacket, 1483 enc_level, 1484 qpacket->time, 1485 &ackm_data)) 1486 goto end; 1487 1488 ok = 1; 1489 end: 1490 /* 1491 * ASSUMPTION: If this function is called at all, |qpacket| is 1492 * a legitimate packet, even if its contents aren't. 1493 * Therefore, we call ossl_ackm_on_rx_packet() unconditionally, as long as 1494 * |ackm_data| has at least been initialized. 1495 */ 1496 if (ok >= 0) 1497 ossl_ackm_on_rx_packet(ch->ackm, &ackm_data); 1498 1499 return ok > 0; ``` if the call to `depack_process_frames()` at line 1492 fails, because barticualr frame in packet is corrupted/invalid we take a branch to `end:` goto target. In this case we must avoid the call to `ossl_ackm_on_rx_packet()`. Packet with malformed/invalid frame must not be accepted. See RFC 9000 section 13.1: Once the packet has been fully processed, a receiver acknowledges receipt by sending one or more ACK frames containing the packet number of the received packet. Reviewed-by: Matt Caswell Reviewed-by: Tomas Mraz (Merged from https://github.com/openssl/openssl/pull/28002) --- diff --git a/ssl/quic/quic_rx_depack.c b/ssl/quic/quic_rx_depack.c index a36b02d5dcb..f800d898419 100644 --- a/ssl/quic/quic_rx_depack.c +++ b/ssl/quic/quic_rx_depack.c @@ -1429,16 +1429,8 @@ int ossl_quic_handle_frames(QUIC_CHANNEL *ch, OSSL_QRX_PKT *qpacket) uint32_t enc_level; size_t dgram_len = qpacket->datagram_len; - /* - * ok has three states: - * -1 error with ackm_data uninitialized - * 0 error with ackm_data initialized - * 1 success (ackm_data initialized) - */ - int ok = -1; /* Assume the worst */ - if (ch == NULL) - goto end; + return 0; ch->did_crypto_frame = 0; @@ -1456,9 +1448,8 @@ int ossl_quic_handle_frames(QUIC_CHANNEL *ch, OSSL_QRX_PKT *qpacket) * Retry and Version Negotiation packets should not be passed to this * function. */ - goto end; + return 0; - ok = 0; /* Still assume the worst */ ackm_data.pkt_space = ossl_quic_enc_level_to_pn_space(enc_level); /* @@ -1480,18 +1471,9 @@ int ossl_quic_handle_frames(QUIC_CHANNEL *ch, OSSL_QRX_PKT *qpacket) enc_level, qpacket->time, &ackm_data)) - goto end; + return 0; - ok = 1; - end: - /* - * ASSUMPTION: If this function is called at all, |qpacket| is - * a legitimate packet, even if its contents aren't. - * Therefore, we call ossl_ackm_on_rx_packet() unconditionally, as long as - * |ackm_data| has at least been initialized. - */ - if (ok >= 0) - ossl_ackm_on_rx_packet(ch->ackm, &ackm_data); + ossl_ackm_on_rx_packet(ch->ackm, &ackm_data); - return ok > 0; + return 1; }