QUIC RX: Refactor unsafe DCID consistency checking

Previously, we enforced the requirement that the DCIDs be the same for
all packets in a datagram by keeping a pointer to the first RXE
generated from a datagram. This is unsafe and could lead to a UAF if the
first packet is malformed, meaning that no RXE ended up being generated
from it. Keep track of the DCID directly instead, as we should enforce
this correctly even if the first packet in a datagram is malformed (but
has an intelligible header with a DCID and length).

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/19703)

2 files changed, 30 insertions, 25 deletions

diff --git a/ssl/quic/quic_record_rx.c b/ssl/quic/quic_record_rx.c
index 1de3758af6..18a2428b6b 100644
--- a/ssl/quic/quic_record_rx.c
+++ b/ssl/quic/quic_record_rx.c
@@ -376,6 +376,10 @@ static RXE *qrx_resize_rxe(RXE_LIST *rxl, RXE *rxe, size_t n)
     p = ossl_list_rxe_prev(rxe);
     ossl_list_rxe_remove(rxl, rxe);
 
+    /* Should never resize an RXE which has been handed out. */
+    if (!ossl_assert(rxe->refcount == 0))
+        return NULL;
+
     /*
      * NOTE: We do not clear old memory, although it does contain decrypted
      * data.
@@ -480,7 +484,7 @@ static uint32_t rxe_determine_pn_space(RXE *rxe)
 }
 
 static int qrx_validate_hdr_early(OSSL_QRX *qrx, RXE *rxe,
-                                  RXE *first_rxe)
+                                  const QUIC_CONN_ID *first_dcid)
 {
     /* Ensure version is what we want. */
     if (rxe->hdr.version != QUIC_VERSION_1
@@ -492,17 +496,19 @@ static int qrx_validate_hdr_early(OSSL_QRX *qrx, RXE *rxe,
         return 0;
 
     /* Version negotiation and retry packets must be the first packet. */
-    if (first_rxe != NULL && !ossl_quic_pkt_type_can_share_dgram(rxe->hdr.type))
+    if (first_dcid != NULL && !ossl_quic_pkt_type_can_share_dgram(rxe->hdr.type))
         return 0;
 
     /*
      * If this is not the first packet in a datagram, the destination connection
      * ID must match the one in that packet.
      */
-    if (first_rxe != NULL &&
-        !ossl_quic_conn_id_eq(&first_rxe->hdr.dst_conn_id,
-                              &rxe->hdr.dst_conn_id))
+    if (first_dcid != NULL) {
+        if (!ossl_assert(first_dcid->id_len < QUIC_MAX_CONN_ID_LEN)
+            || !ossl_quic_conn_id_eq(first_dcid,
+                                     &rxe->hdr.dst_conn_id))
         return 0;
+    }
 
     return 1;
 }
@@ -668,7 +674,7 @@ static void qrx_key_update_initiated(OSSL_QRX *qrx)
 /* Process a single packet in a datagram. */
 static int qrx_process_pkt(OSSL_QRX *qrx, QUIC_URXE *urxe,
                            PACKET *pkt, size_t pkt_idx,
-                           RXE **first_rxe,
+                           QUIC_CONN_ID *first_dcid,
                            size_t datagram_len)
 {
     RXE *rxe;
@@ -712,18 +718,18 @@ static int qrx_process_pkt(OSSL_QRX *qrx, QUIC_URXE *urxe,
     eop = PACKET_data(pkt);
 
     /*
-     * Make a note of the first RXE so we can later ensure the destination
-     * connection IDs of all packets in a datagram match.
+     * Make a note of the first packet's DCID so we can later ensure the
+     * destination connection IDs of all packets in a datagram match.
      */
     if (pkt_idx == 0)
-        *first_rxe = rxe;
+        *first_dcid = rxe->hdr.dst_conn_id;
 
     /*
      * Early header validation. Since we now know the packet length, we can also
      * now skip over it if we already processed it.
      */
     if (already_processed
-        || !qrx_validate_hdr_early(qrx, rxe, pkt_idx == 0 ? NULL : *first_rxe))
+        || !qrx_validate_hdr_early(qrx, rxe, pkt_idx == 0 ? NULL : first_dcid))
         /*
          * Already processed packets are handled identically to malformed
          * packets; i.e., they are ignored.
@@ -962,7 +968,7 @@ static int qrx_process_datagram(OSSL_QRX *qrx, QUIC_URXE *e,
     int have_deferred = 0;
     PACKET pkt;
     size_t pkt_idx = 0;
-    RXE *first_rxe = NULL;
+    QUIC_CONN_ID first_dcid = { 255 };
 
     qrx->bytes_received += data_len;
 
@@ -992,7 +998,7 @@ static int qrx_process_datagram(OSSL_QRX *qrx, QUIC_URXE *e,
          * length, qrx_process_pkt will take care of advancing to the end of
          * the packet, so we will exit the loop automatically in this case.
          */
-        if (qrx_process_pkt(qrx, e, &pkt, pkt_idx, &first_rxe, data_len))
+        if (qrx_process_pkt(qrx, e, &pkt, pkt_idx, &first_dcid, data_len))
             have_deferred = 1;
     }
 
diff --git a/test/quic_record_test.c b/test/quic_record_test.c
index 1664cd2afc..cd83f6652d 100644
--- a/test/quic_record_test.c
+++ b/test/quic_record_test.c
@@ -1710,12 +1710,6 @@ static void rx_state_teardown(struct rx_state *s)
         s->ccdata = NULL;
     }
 
-    if (s->qrx != NULL) {
-        ossl_qrx_free(s->qrx);
-        ossl_quic_conn_set_qrx(s->quic_conn, NULL);
-        s->qrx = NULL;
-    }
-
     if (s->quic_conn != NULL) {
         SSL_free((SSL *)s->quic_conn);
         s->quic_conn = NULL;
@@ -1725,6 +1719,11 @@ static void rx_state_teardown(struct rx_state *s)
         s->quic_ssl_ctx = NULL;
     }
 
+    if (s->qrx != NULL) {
+        ossl_qrx_free(s->qrx);
+        s->qrx = NULL;
+    }
+
     if (s->demux != NULL) {
         ossl_quic_demux_free(s->demux);
         s->demux = NULL;
@@ -1800,7 +1799,7 @@ static int rx_state_ensure_for_frames(struct rx_state *s)
 
 static int rx_run_script(const struct rx_test_op *script)
 {
-    int testresult = 0, pkt_outstanding = 0;
+    int testresult = 0;
     struct rx_state s = {0};
     size_t i;
     OSSL_QRX_PKT *pkt = NULL;
@@ -1861,7 +1860,6 @@ static int rx_run_script(const struct rx_test_op *script)
                 if (!TEST_true(ossl_qrx_read_pkt(s.qrx, &pkt)))
                     goto err;
 
-                pkt_outstanding = 1;
                 if (!TEST_ptr(pkt) || !TEST_ptr(pkt->hdr))
                     goto err;
 
@@ -1877,20 +1875,22 @@ static int rx_run_script(const struct rx_test_op *script)
                 case RX_TEST_OP_CHECK_PKT_FRAMES_OK:
                     if (!TEST_true(rx_state_ensure_for_frames(&s)))
                         goto err;
-                    pkt_outstanding = 0;
                     if (!TEST_true(ossl_quic_handle_frames(s.quic_conn, pkt)))
                         goto err;
+                    ossl_qrx_pkt_release(pkt);
+                    pkt = NULL;
                     break;
                 case RX_TEST_OP_CHECK_PKT_FRAMES_INVALID:
                     if (!TEST_true(rx_state_ensure_for_frames(&s)))
                         goto err;
-                    pkt_outstanding = 0;
                     if (!TEST_false(ossl_quic_handle_frames(s.quic_conn, pkt)))
                         goto err;
+                    ossl_qrx_pkt_release(pkt);
+                    pkt = NULL;
                     break;
                 default:
-                    pkt_outstanding = 0;
                     ossl_qrx_pkt_release(pkt);
+                    pkt = NULL;
                     break;
                 }
                 break;
@@ -1931,8 +1931,7 @@ static int rx_run_script(const struct rx_test_op *script)
 
     testresult = 1;
 err:
-    if (pkt_outstanding)
-        ossl_qrx_pkt_release(pkt);
+    ossl_qrx_pkt_release(pkt);
     rx_state_teardown(&s);
     return testresult;
 }