diff mbox series

[Openvpn-devel,v3] mudp: fix unaligned 32-bit read when parsing peer ID

Message ID 20251210104839.8270-1-gert@greenie.muc.de
State New
Headers
Series [Openvpn-devel,v3] mudp: fix unaligned 32-bit read when parsing peer ID |

Commit Message

Gert Doering Dec. 10, 2025, 10:48 a.m. UTC 
  From: Gianmarco De Gregori <gianmarco@mandelbit.com>

The code previously read a 32-bit value from a uint8_t
buffer using a direct cast and dereference.
This can cause unaligned memory access and undefined
behavior on architectures that do not support unaligned
reads, potentially leading to a one-packet crash.

Fix this by reading the bytes individually and
combining them manually.

Reported-By: Joshua Rogers <contact@joshua.hu>
Found-By: ZeroPath (https://zeropath.com)

Change-Id: Id0bb4c45d373437ab8dbaff7a311745f9b538cbf
Signed-off-by: Gianmarco De Gregori <gianmarco@mandelbit.com>
Acked-by: Gert Doering <gert@greenie.muc.de>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1348
---

This change was reviewed on Gerrit and approved by at least one
developer. I request to merge it to master.

Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1348
This mail reflects revision 3 of this Change.

Acked-by according to Gerrit (reflected above):
Gert Doering <gert@greenie.muc.de>

Comments

Gert Doering Dec. 10, 2025, 11:02 a.m. UTC | #1 
Not sure the new code is less or more ugly than the old code... but it's
less ugly than v1 of the patch :-)

I have lightly tested this, and instrumented my t_server test bed to
add an ASSERT(old_peer_id == peer_id) check.  I do not have peer IDs > 255,
and the p2p mode (which uses random-high peer IDs) does not use *this*
code path, so "exposure to higher peer-id" has not been tested.

That said, the byte order handling is correct for peer-ids 1 and 2, and
stare-at-code suggests it should work fine for up to 24 bits too :-)

*That* said, I'm not sure if this is really a relevant "finding" - yes,
it could end up doing unaligned 32bit-reads, but on Intel, this is "just
slow" - which byte accesses are, too.  The only platform I'm aware that
will trap-and-die is SPARC32, and even that should be fine with the code
that has been in there for a loooong time...  but anyway.  I don't think
this makes a huge difference either way.

Your patch has been applied to the master branch.

commit 8bf8bead6d7cc3c743fdf2b915fbb2f3f24e7005
Author: Gianmarco De Gregori
Date:   Wed Dec 10 11:48:33 2025 +0100

     mudp: fix unaligned 32-bit read when parsing peer ID

     Signed-off-by: Gianmarco De Gregori <gianmarco@mandelbit.com>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1348
     Message-Id: <20251210104839.8270-1-gert@greenie.muc.de>
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/mudp.c b/src/openvpn/mudp.c
index b03e165..5de3af6 100644
--- a/src/openvpn/mudp.c
+++ b/src/openvpn/mudp.c
@@ -209,7 +209,7 @@ 
         /* make sure buffer has enough length to read opcode (1 byte) and peer-id (3 bytes) */
         if (v2)
         {
-            uint32_t peer_id = ntohl(*(uint32_t *)ptr) & 0xFFFFFF;
+            uint32_t peer_id = ((uint32_t)ptr[1] << 16) | ((uint32_t)ptr[2] << 8) | ((uint32_t)ptr[3]);
             peer_id_disabled = (peer_id == MAX_PEER_ID);
 
             if (!peer_id_disabled && (peer_id < m->max_clients) && (m->instances[peer_id]))