socket: assert buffer length before reading prepended sockaddr family

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
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>

diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c
index 624ce4f53fd8f41cbb2d8db1ec07ca98954b46c2..df2cc9e0326aad53acb829e78dd64988f0715a4a 100644 (file)
--- a/src/openvpn/socket.c
+++ b/src/openvpn/socket.c
@@ -2813,38 +2813,31 @@ read_sockaddr_from_packet(struct buffer *buf, struct sockaddr *dst)
 {
     int sa_len = 0;
 
 {
     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);
     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);
             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:
             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;
 }
 
     return sa_len;
 }