diff mbox series

[Openvpn-devel,v1] socket: assert buffer length before reading prepended sockaddr family

Message ID 20260605141808.14028-1-gert@greenie.muc.de
State New
Headers
Series [Openvpn-devel,v1] socket: assert buffer length before reading prepended sockaddr family |

Commit Message

Gert Doering June 5, 2026, 2:18 p.m. UTC 
  From: Lev Stipakov <lev@openvpn.net>

read_sockaddr_from_packet() inspected sa->sa_family before any check
on buf->len, so a short delivery from the dco-win driver would have
produced a garbage peer address from uninitialized buffer memory.
The driver always prepends a full sockaddr and validates the family
before writing, so reaching any of the size/family checks would mean
something is severely wrong on the driver side - assert the three
preconditions instead of M_FATAL'ing on them.

GitHub: https://github.com/OpenVPN/openvpn-private-issues/issues/105

Change-Id: I2ce954aa5b74002be5e38d53783435736625bb2f
Signed-off-by: Lev Stipakov <lev@openvpn.net>
Acked-by: Gert Doering <gert@greenie.muc.de>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1706
---

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/+/1706
This mail reflects revision 1 of this Change.

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

Comments

Gert Doering June 5, 2026, 6:07 p.m. UTC | #1 
As discussed, we keep the ASSERT() on "too short buffer" (because it would
be a breach of contract between *our* driver in the kernel) - but it gets
moved so even in that case we avoid the "access out of buffer memory first,
ASSERT() then" situation.

Also, code cleanup :-) - no real change, just move the common parts after
the switch().

Stared-at-code, test compiled on ubuntu/mingw.  Build errors in buildbot
are unrelated "infrastructure" issues (as the unix code paths are fully
untouched by this patch).

Your patch has been applied to the master and release/2.7 branch (bugfix),
code is not in 2.6 or earlier.

commit 71b271d9a168b7c43f3b78bdc4768edadfa25e86 (master)
commit d849dbd395fff72ffc10e0161e5a4237f72f5be3 (release/2.7)
Author: Lev Stipakov
Date:   Fri Jun 5 16:18:02 2026 +0200

     socket: assert buffer length before reading prepended sockaddr family

     Signed-off-by: Lev Stipakov <lev@openvpn.net>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1706
     Message-Id: <20260605141808.14028-1-gert@greenie.muc.de>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg37065.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c
index 624ce4f..df2cc9e 100644
--- a/src/openvpn/socket.c
+++ b/src/openvpn/socket.c
@@ -2813,38 +2813,31 @@ 
 {
     int sa_len = 0;
 
+    /* In dco-win multipeer mode the kernel driver always prepends a full
+     * sockaddr_in or sockaddr_in6 in front of the control-packet payload,
+     * so the buffer must hold at least sizeof(struct sockaddr_in) bytes
+     * before we may inspect sa_family. */
+    ASSERT(buf_len(buf) >= (int)sizeof(struct sockaddr_in));
+
     const struct sockaddr *sa = (const struct sockaddr *)BPTR(buf);
     switch (sa->sa_family)
     {
         case AF_INET:
             sa_len = sizeof(struct sockaddr_in);
-            if (buf_len(buf) < sa_len)
-            {
-                msg(M_FATAL,
-                    "ERROR: received incoming packet with too short length of %d -- must be at least %d.",
-                    buf_len(buf), sa_len);
-            }
-            memcpy(dst, sa, sa_len);
-            buf_advance(buf, sa_len);
             break;
 
         case AF_INET6:
             sa_len = sizeof(struct sockaddr_in6);
-            if (buf_len(buf) < sa_len)
-            {
-                msg(M_FATAL,
-                    "ERROR: received incoming packet with too short length of %d -- must be at least %d.",
-                    buf_len(buf), sa_len);
-            }
-            memcpy(dst, sa, sa_len);
-            buf_advance(buf, sa_len);
+            ASSERT(buf_len(buf) >= sa_len);
             break;
 
         default:
-            msg(M_FATAL, "ERROR: received incoming packet with invalid address family %d.",
-                sa->sa_family);
+            ASSERT(0); /* driver validates the family before writing */
     }
 
+    memcpy(dst, sa, sa_len);
+    buf_advance(buf, sa_len);
+
     return sa_len;
 }