]> git.ipfire.org Git - thirdparty/openssl.git/commitdiff
Fix read out of buffer bounds when dealing with BIO_ADDR
authorAlexandr Nedvedicky <sashan@openssl.org>
Mon, 23 Dec 2024 16:03:32 +0000 (17:03 +0100)
committerTomas Mraz <tomas@openssl.org>
Tue, 25 Feb 2025 14:56:43 +0000 (15:56 +0100)
This issue was discoevered while I was testing SSL_new_from_listener()
using a newly created unit test. It has turned out the QUIC stack
at few places contain pattern as follows:
foo(QUIC_WHATEVER *q, BIO_ADDR *a)
{
   q->a = *a;
}

The problem is that derefencning a that way is risky. If the address `a`
comes from BIO_lookup_ex() it may actually be shorter than sizeof(BIO_ADDR).
Using BIO_ADDR_copy() is the right thing to do here.

Fixes #26241

Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/26252)

(cherry picked from commit 395a83a617a09c1ae02e8040386f9acb356d13c1)

crypto/bio/bio_addr.c
ssl/quic/quic_channel.c
ssl/quic/quic_record_tx.c
ssl/quic/quic_txp.c

index 3e4e850596ce37f3b9948ce1a9ff33052bf7f941..2ba60ea9f7f417b1a6e49b9b882a2bdb5c3df509 100644 (file)
@@ -104,6 +104,7 @@ void BIO_ADDR_clear(BIO_ADDR *ap)
  */
 int BIO_ADDR_make(BIO_ADDR *ap, const struct sockaddr *sa)
 {
+    memset(ap, 0, sizeof(BIO_ADDR));
     if (sa->sa_family == AF_INET) {
         memcpy(&(ap->s_in), sa, sizeof(struct sockaddr_in));
         return 1;
index d3b7947fb462587200512c9463a16599eb40ec76..8d66bddf40f2964e788f431e5a7629208f021d8c 100644 (file)
@@ -591,8 +591,7 @@ int ossl_quic_channel_get_peer_addr(QUIC_CHANNEL *ch, BIO_ADDR *peer_addr)
     if (!ch->addressed_mode)
         return 0;
 
-    *peer_addr = ch->cur_peer_addr;
-    return 1;
+    return BIO_ADDR_copy(peer_addr, &ch->cur_peer_addr);
 }
 
 int ossl_quic_channel_set_peer_addr(QUIC_CHANNEL *ch, const BIO_ADDR *peer_addr)
@@ -606,8 +605,12 @@ int ossl_quic_channel_set_peer_addr(QUIC_CHANNEL *ch, const BIO_ADDR *peer_addr)
         return 1;
     }
 
-    ch->cur_peer_addr   = *peer_addr;
-    ch->addressed_mode  = 1;
+    if (!BIO_ADDR_copy(&ch->cur_peer_addr, peer_addr)) {
+        ch->addressed_mode = 0;
+        return 0;
+    }
+    ch->addressed_mode = 1;
+
     return 1;
 }
 
@@ -3500,7 +3503,9 @@ static int ch_server_on_new_conn(QUIC_CHANNEL *ch, const BIO_ADDR *peer,
         return 0;
 
     /* Note our newly learnt peer address and CIDs. */
-    ch->cur_peer_addr   = *peer;
+    if (!BIO_ADDR_copy(&ch->cur_peer_addr, peer))
+        return 0;
+
     ch->init_dcid       = *peer_dcid;
     ch->cur_remote_dcid = *peer_scid;
 
index c01abed0d66a4d42d60f6bcb533a610185b43a1e..3e72409fe390d6cdc0b803ece59deef02dc5cd64 100644 (file)
@@ -803,15 +803,19 @@ int ossl_qtx_write_pkt(OSSL_QTX *qtx, const OSSL_QTX_PKT *pkt)
 
         if (!was_coalescing) {
             /* Set addresses in TXE. */
-            if (pkt->peer != NULL)
-                txe->peer = *pkt->peer;
-            else
+            if (pkt->peer != NULL) {
+                if (!BIO_ADDR_copy(&txe->peer, pkt->peer))
+                    return 0;
+            } else {
                 BIO_ADDR_clear(&txe->peer);
+            }
 
-            if (pkt->local != NULL)
-                txe->local = *pkt->local;
-            else
+            if (pkt->local != NULL) {
+                if (!BIO_ADDR_copy(&txe->local, pkt->local))
+                    return 0;
+            } else {
                 BIO_ADDR_clear(&txe->local);
+            }
         }
 
         ret = qtx_write(qtx, pkt, txe, enc_level);
index 246be1c3477e94375b986c6e0c01a416641048bd..2e43e6d24be9a3ab1c91a5a0aec2779eb3fd1db4 100644 (file)
@@ -611,8 +611,7 @@ int ossl_quic_tx_packetiser_set_peer(OSSL_QUIC_TX_PACKETISER *txp,
         return 1;
     }
 
-    txp->args.peer = *peer;
-    return 1;
+    return BIO_ADDR_copy(&txp->args.peer, peer);
 }
 
 void ossl_quic_tx_packetiser_set_ack_tx_cb(OSSL_QUIC_TX_PACKETISER *txp,