[Openvpn-devel] [PATCH v3] Fix unaligned access in macOS, FreeBSD, Solaris hwaddr

[Gert D. <ge...@gr...>]

From: Arne Schwabe <ar...@rf...>

The undefined behaviour USAN clang checker found this.

This fix is a bit messy but so are the original structures.

Since the API on Solaris/Illuminos does not return the AF_LINK
sockaddr type we are interested in, there is little value in
fixing the code on that platform to iterate through a list
that does not contain the element we are looking for.

Add includes stddef.h for offsetof and integer.h for max_int.

Change-Id: Ia797c8801fa9a9bc10b6674efde5fdbd7132e4a8
Signed-off-by: Arne Schwabe <ar...@rf...>
Acked-by: Gert Doering <ge...@gr...>
---

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/+/454
This mail reflects revision 3 of this Change.
Acked-by according to Gerrit (reflected above):
Gert Doering <ge...@gr...>

        
diff --git a/src/openvpn/route.c b/src/openvpn/route.c
index 6cc112c..0e6667f 100644
--- a/src/openvpn/route.c
+++ b/src/openvpn/route.c
@@ -24,6 +24,7 @@
 /*
  * Support routines for adding/deleting network routes.
  */
+#include <stddef.h>
 
 #ifdef HAVE_CONFIG_H
 #include "config.h"
@@ -40,6 +41,7 @@
 #include "win32.h"
 #include "options.h"
 #include "networking.h"
+#include "integer.h"
 
 #include "memdbg.h"
 
@@ -3639,11 +3641,15 @@
         rgi->flags |= RGI_NETMASK_DEFINED;
     }
 
+#if !defined(TARGET_SOLARIS)
+    /* Illumos/Solaris does not provide AF_LINK entries when calling the
+     * SIOCGIFCONF API, so there is little sense to trying to figure out a
+     * MAC address from an API that does not provide that information */
+
     /* try to read MAC addr associated with interface that owns default gateway */
     if (rgi->flags & RGI_IFACE_DEFINED)
     {
         struct ifconf ifc;
-        struct ifreq *ifr;
         const int bufsize = 4096;
         char *buffer;
 
@@ -3668,22 +3674,50 @@
 
         for (cp = buffer; cp <= buffer + ifc.ifc_len - sizeof(struct ifreq); )
         {
-            ifr = (struct ifreq *)cp;
-#if defined(TARGET_SOLARIS)
-            const size_t len = sizeof(ifr->ifr_name) + sizeof(ifr->ifr_addr);
-#else
-            const size_t len = sizeof(ifr->ifr_name) + max(sizeof(ifr->ifr_addr), ifr->ifr_addr.sa_len);
-#endif
+            struct ifreq ifr = { 0 };
+            /* this is not always using an 8 byte alignment that struct ifr
+             * requires. Need to memcpy() to a strict ifr to force 8-byte
+             * alignment required for member access */
+            memcpy(&ifr, cp, sizeof(struct ifreq));
+            const size_t len = sizeof(ifr.ifr_name) + max(sizeof(ifr.ifr_addr), ifr.ifr_addr.sa_len);
 
-            if (!ifr->ifr_addr.sa_family)
+            if (!ifr.ifr_addr.sa_family)
             {
                 break;
             }
-            if (!strncmp(ifr->ifr_name, rgi->iface, IFNAMSIZ))
+            if (!strncmp(ifr.ifr_name, rgi->iface, IFNAMSIZ))
             {
-                if (ifr->ifr_addr.sa_family == AF_LINK)
+                if (ifr.ifr_addr.sa_family == AF_LINK)
                 {
-                    struct sockaddr_dl *sdl = (struct sockaddr_dl *)&ifr->ifr_addr;
+                    /* This is a confusing member access on multiple levels.
+                     *
+                     * struct sockaddr_dl is 20 bytes (on macOS and NetBSD,
+                     * larger on other BSDs) in size and has
+                     * 12 bytes space for the Ethernet interface name
+                     * (max 16 bytes) and  hw address (6 bytes)
+                     *
+                     * So if the interface name is more than 6 byte, the
+                     * location of hwaddr extends beyond the struct.
+                     *
+                     * This struct is embedded into ifreq that has
+                     * 16 bytes for a sockaddr and also expects this
+                     * struct to potentially extend beyond the bounds of
+                     * the struct.
+                     *
+                     * We only copied 32 bytes (size of ifr at least on macOS
+                     * might differ on other platforms again) from cp to ifr.
+                     *
+                     * But as hwaddr might extend but sdl might extend beyond
+                     * ifr's. So we need recalculate how large the actual size
+                     * of the embedded dl_sock actually is and then also need
+                     * to copy it since it also most likely does not have the
+                     * proper alignment required to access the struct.
+                     */
+                    const size_t sock_dl_len = max_int((int) (sizeof(struct sockaddr_dl)),
+                                                       (int) (ifr.ifr_addr.sa_len));
+
+                    struct sockaddr_dl *sdl = gc_malloc(sock_dl_len, true, &gc);
+                    memcpy(sdl, cp + offsetof(struct ifreq, ifr_addr), sock_dl_len);
                     memcpy(rgi->hwaddr, LLADDR(sdl), 6);
                     rgi->flags |= RGI_HWADDR_DEFINED;
                 }
@@ -3691,6 +3725,7 @@
             cp += len;
         }
     }
+#endif /* if !defined(TARGET_SOLARIS) */
 
 done:
     if (sockfd >= 0)