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 <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(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 a36b02d5dcb4f76547638433aaf6d769f12cc360..f800d8984193dbe74c47bd1a6435eebb7efa1cfb 100644 (file)
--- 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;
 }