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

Message ID 20231231173431.31356-1-gert@greenie.muc.de
State Accepted
Headers show
Series [Openvpn-devel,v3] Fix unaligned access in macOS, FreeBSD, Solaris hwaddr | expand

Commit Message

Gert Doering Dec. 31, 2023, 5:34 p.m. UTC
From: Arne Schwabe <arne@rfc2549.org>

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 <arne@rfc2549.org>
Acked-by: Gert Doering <gert@greenie.muc.de>
---

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 <gert@greenie.muc.de>

Comments

Gert Doering Dec. 31, 2023, 6:12 p.m. UTC | #1
There's still room for improvement in the comments, but the current version
might be as good as it gets this year... and I want to be able to push to
GHA again without UBSAN errors :-)

Tested this on NetBSD, OpenBSD, FreeBSD, MacOS and OpenSolaris, and found
lots of interesting things - but nothing that was made worse by *this*
patch, and the UBSAN build stopped complaining on MacOS... also, I've
stared long and hard at the code, and as long as we can trust the kernel,
every variant of short/long length should be covered.

Did I say that this is a shitty API that MacOS copied from FreeBSD, 
FreeBSD actually fixed it (by increasing the structure size a lot, so 
if the buffer is aligned, nothing will overrun and alignment will also
be fine), and MacOS just stuck with what they have...

A followup patch that implements HWADDR for OpenSolaris happened by
accident while testing this :-) - will be on the list soon.

Your patch has been applied to the master and release/2.6 branch (bugfix).

commit f13331005d5a75f2788685485d46be1fe2f133a1 (master)
commit 5380fe02b9ef4f0f2b1f0eb52100b7922965dfdb (release/2.6)
Author: Arne Schwabe
Date:   Sun Dec 31 18:34:31 2023 +0100

     Fix unaligned access in macOS, FreeBSD, Solaris hwaddr

     Signed-off-by: Arne Schwabe <arne@rfc2549.org>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <20231231173431.31356-1-gert@greenie.muc.de>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg27885.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering

Patch

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)