]> 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:37 +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 396cbe8461da2a0e08079a2291b45f244cdb132e..88e86b83df0ad58c1ccb5fd5c4c792184f1ea407 100644 (file)
@@ -474,8 +474,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)
@@ -489,8 +488,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;
 }
 
@@ -3344,7 +3347,9 @@ int ossl_quic_channel_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 cda684245d77d12c5cde310c4bbb2e88c3b2e7e7..3f3688f134c5d1cea5d5271887fba4301cd42187 100644 (file)
@@ -842,15 +842,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_mutate_write(qtx, pkt, txe, enc_level);
index bbbb6a7045208059f79fd7c41daec9d8e8b09652..8bc6a7d38ef33eff3e7993351171fe56b2d1cb82 100644 (file)
@@ -613,8 +613,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,