Make macOS gateway detection IPv6 aware and use actual server address

This also move the building IV_HWADDR peer info variable to the point
that the server address is actually available.

This also avoids failing to connect when push-peer-info is enabled and
there is no IPv4 default gateway. The new code will always pick the device
that holds the route to the current remote.

Signed-off-by: Arne Schwabe <arne@openvpn.net>

openvpn/addr/ip.hpp

@@ -335,9 +335,9 @@ class Addr
     static Addr from_sockaddr(const struct sockaddr *sa)
     {
         if (sa->sa_family == AF_INET)
-            return from_ipv4(IPv4::Addr::from_sockaddr((struct sockaddr_in *)sa));
+            return from_ipv4(IPv4::Addr::from_sockaddr(reinterpret_cast<const struct sockaddr_in *>(sa)));
         else if (sa->sa_family == AF_INET6)
-            return from_ipv6(IPv6::Addr::from_sockaddr((struct sockaddr_in6 *)sa));
+            return from_ipv6(IPv6::Addr::from_sockaddr(reinterpret_cast<const struct sockaddr_in6 *>(sa)));
         else
             return Addr();
     }

openvpn/addr/ipv6.hpp

@@ -109,6 +109,10 @@ class Addr // NOTE: must be union-legal, so default constructor does not initial
         ret.sin6_port = htons(port);
         host_to_network_order((union ipv6addr *)&ret.sin6_addr.s6_addr, &u);
         ret.sin6_scope_id = scope_id_;
+#ifdef SIN6_LEN
+        /* This is defined on both macOS and FreeBSD that have the sin6_len member */
+        ret.sin6_len = sizeof(sockaddr_in6);
+#endif
         return ret;
     }
 

openvpn/client/cliopt.hpp

@@ -1040,14 +1040,24 @@ class ClientOptions : public RC<thread_unsafe_refcount>
         if (!config.sso_methods.empty())
             pi->emplace_back("IV_SSO", config.sso_methods);
 
+        return pi;
+    }
+
+    PeerInfo::Set::Ptr build_peer_info_transport(const Config &config, const ParseClientConfig &pcc)
+    {
+        PeerInfo::Set::Ptr pi(new PeerInfo::Set);
+
         // MAC address
         if (pcc.pushPeerInfo())
         {
-            std::string hwaddr = get_hwaddr();
+            /* If we override the HWADDR, we add it at this time statically. If we need to
+             * dynamically discover it from the transport it will be added in
+             * \c build_connect_time_peer_info_string instead */
             if (!config.hw_addr_override.empty())
+            {
                 pi->emplace_back("IV_HWADDR", config.hw_addr_override);
-            else if (!hwaddr.empty())
-                pi->emplace_back("IV_HWADDR", hwaddr);
+            }
+
             pi->emplace_back("IV_SSL", get_ssl_library_version());
 
             if (!config.platform_version.empty())
@@ -1056,6 +1066,7 @@ class ClientOptions : public RC<thread_unsafe_refcount>
         return pi;
     }
 
+
     void next(RemoteList::Advance type)
     {
         bool omit_next = false;
@@ -1253,6 +1264,7 @@ class ClientOptions : public RC<thread_unsafe_refcount>
         cp->load(opt, *proto_context_options, config.default_key_direction, false);
         cp->set_xmit_creds(!autologin || pcc.hasEmbeddedPassword() || autologin_sessions);
         cp->extra_peer_info = build_peer_info(config, pcc, autologin_sessions);
+        cp->extra_peer_info_push_peerinfo = pcc.pushPeerInfo();
         cp->frame = frame;
         cp->now = &now_;
         cp->rng = rng;

openvpn/client/cliproto.hpp

@@ -494,6 +494,7 @@ class Session : ProtoContext,
     {
         try
         {
+            Base::conf().build_connect_time_peer_info_string(transport);
             OPENVPN_LOG("Connecting to " << server_endpoint_render());
             Base::set_protocol(transport->transport_protocol());
             Base::start();

openvpn/netconf/hwaddr.hpp

@@ -32,13 +32,13 @@
 #if defined(OPENVPN_PLATFORM_WIN) && !defined(OPENVPN_PLATFORM_UWP)
 #include <openvpn/tun/win/tunutil.hpp>
 #elif defined(OPENVPN_PLATFORM_MAC)
-#include <openvpn/tun/mac/gwv4.hpp>
+#include <openvpn/tun/mac/gw.hpp>
 #elif defined(TARGET_OS_IPHONE)
 #include <UIKit/UIKit.h>
 #endif
 
 namespace openvpn {
-inline std::string get_hwaddr()
+inline std::string get_hwaddr([[maybe_unused]] IP::Addr server_addr)
 {
 #if defined(OPENVPN_PLATFORM_WIN) && !defined(OPENVPN_PLATFORM_UWP)
     const TunWin::Util::BestGateway dg{AF_INET};
@@ -53,7 +53,7 @@ inline std::string get_hwaddr()
         }
     }
 #elif defined(OPENVPN_PLATFORM_MAC)
-    const MacGatewayInfoV4 gw;
+    const MacGatewayInfo gw{server_addr};
     if (gw.hwaddr_defined())
     {
         const MACAddr &mac = gw.hwaddr();

openvpn/ssl/peerinfo.hpp

@@ -90,6 +90,13 @@ struct Set : public std::vector<KeyValue>, public RCCopyable<thread_unsafe_refco
             emplace_back(kv.key, kv.value);
     }
 
+    [[nodiscard]] bool contains_key(const std::string &key)
+    {
+        return std::find_if(begin(), end(), [&](const PeerInfo::KeyValue &kv)
+                            { return kv.key == key; })
+               != end();
+    }
+
     Ptr copy() const
     {
         return new Set(*this);

openvpn/ssl/proto.hpp

@@ -75,6 +75,7 @@
 #include <openvpn/ssl/mssparms.hpp>
 #include <openvpn/transport/mssfix.hpp>
 #include <openvpn/transport/protocol.hpp>
+#include <openvpn/transport/client/transbase.hpp>
 #include <openvpn/tun/layer.hpp>
 #include <openvpn/tun/tunmtu.hpp>
 #include <openvpn/compress/compress.hpp>
@@ -82,6 +83,7 @@
 #include <openvpn/ssl/peerinfo.hpp>
 #include <openvpn/ssl/ssllog.hpp>
 #include <openvpn/crypto/crypto_aead.hpp>
+#include <openvpn/netconf/hwaddr.hpp>
 
 
 #if OPENVPN_DEBUG_PROTO >= 1
@@ -369,9 +371,17 @@ class ProtoContext
         Time::Duration keepalive_timeout;       // timeout period after primary KeyContext reaches ACTIVE state
         Time::Duration keepalive_timeout_early; // timeout period before primary KeyContext reaches ACTIVE state
 
-        // extra peer info key/value pairs generated by client app
+        //! extra peer info key/value pairs generated by client app
         PeerInfo::Set::Ptr extra_peer_info;
 
+        /** extra peer information that depends on the state of the underlying transport and needs to be initialised
+         * after the transport is initialised but before the IV variables are sent */
+        PeerInfo::Set::Ptr extra_peer_info_transport;
+
+        /** When the extra_peer_info_transport is being built, we need to remember if it should include the more
+         * sensitive information that push-peer-info includes */
+        bool extra_peer_info_push_peerinfo = false;
+
         // op header
         bool enable_op32 = false;
         int remote_peer_id = -1; // -1 to disable
@@ -934,6 +944,26 @@ class ProtoContext
             return initial_options;
         }
 
+        /**
+         * This method adds the parts of the peer info string that depend on the state of the
+         * connection, especially the remote that we are connecting to.
+         */
+        void build_connect_time_peer_info_string(TransportClient::Ptr transport)
+        {
+            extra_peer_info_transport.reset(new PeerInfo::Set{});
+            if (extra_peer_info_push_peerinfo)
+            {
+                /* check if the IV_HWADDR is already present in the extra_peer_info set as it has then been
+                 * statically been overridden */
+                if (!extra_peer_info->contains_key("IV_HWADDR"))
+                {
+                    std::string hwaddr = get_hwaddr(transport->server_endpoint_addr());
+                    if (!hwaddr.empty())
+                        extra_peer_info_transport->emplace_back("IV_HWADDR", hwaddr);
+                }
+            }
+        }
+
         // generate a string summarizing information about the client
         // including capabilities
         std::string peer_info_string() const
@@ -992,6 +1022,8 @@ class ProtoContext
                 out << compstr;
             if (extra_peer_info)
                 out << extra_peer_info->to_string();
+            if (extra_peer_info_transport)
+                out << extra_peer_info_transport->to_string();
             if (is_bs64_cipher(dc.cipher()))
                 out << "IV_BS64DL=1\n"; // indicate support for data limits when using 64-bit block-size ciphers, version 1 (CVE-2016-6329)
             if (relay_mode)

openvpn/tun/mac/gw.hpp

@@ -31,6 +31,7 @@
 #include <net/route.h>
 #include <net/if.h>
 #include <net/if_dl.h>
+#include <netinet6/in6_var.h>
 
 #include <cstring>
 #include <string>
@@ -47,9 +48,10 @@
 #include <openvpn/addr/ip.hpp>
 #include <openvpn/addr/addrpair.hpp>
 #include <openvpn/addr/macaddr.hpp>
+#include <ifaddrs.h>
 
 namespace openvpn {
-class MacGatewayInfoV4
+class MacGatewayInfo
 {
     struct rtmsg
     {
@@ -60,12 +62,21 @@ class MacGatewayInfoV4
 #define OPENVPN_ROUNDUP(a) \
     ((a) > 0 ? (1 + (((a)-1) | (sizeof(std::uint32_t) - 1))) : sizeof(std::uint32_t))
 
-#define OPENVPN_NEXTADDR(w, u)         \
-    if (rtm_addrs & (w))               \
-    {                                  \
-        l = OPENVPN_ROUNDUP(u.sa_len); \
-        std::memmove(cp, &(u), l);     \
-        cp += l;                       \
+#define OPENVPN_NEXTADDR(w, u)          \
+    if (rtm_addrs & (w))                \
+    {                                   \
+        l = OPENVPN_ROUNDUP(u.sin_len); \
+        std::memmove(cp, &(u), l);      \
+        cp += l;                        \
+    }
+
+
+#define OPENVPN_NEXTADDR6(w, u)          \
+    if (rtm_addrs & (w))                 \
+    {                                    \
+        l = OPENVPN_ROUNDUP(u.sin6_len); \
+        std::memmove(cp, &(u), l);       \
+        cp += l;                         \
     }
 
 #define OPENVPN_ADVANCE(x, n) \
@@ -82,13 +93,13 @@ class MacGatewayInfoV4
         IFACE_DEFINED = (1 << 3),   /* set if iface is defined */
     };
 
-    MacGatewayInfoV4()
-        : flags_(0)
+    MacGatewayInfo(IP::Addr dest)
     {
         struct rtmsg m_rtmsg;
         ScopedFD sockfd;
         int seq, l, pid, rtm_addrs;
-        struct sockaddr so_dst, so_mask;
+        sockaddr_in so_dst{}, so_mask{};
+        sockaddr_in6 so_dst6{};
         char *cp = m_rtmsg.m_space;
         struct sockaddr *gate = nullptr, *ifp = nullptr, *sa;
         struct rt_msghdr *rtm_aux;
@@ -99,8 +110,6 @@ class MacGatewayInfoV4
         rtm_addrs = RTA_DST | RTA_NETMASK | RTA_IFP;
 
         std::memset(&m_rtmsg, 0, sizeof(m_rtmsg));
-        std::memset(&so_dst, 0, sizeof(so_dst));
-        std::memset(&so_mask, 0, sizeof(so_mask));
         std::memset(&m_rtmsg.m_rtm, 0, sizeof(struct rt_msghdr));
 
         m_rtmsg.m_rtm.rtm_type = RTM_GET;
@@ -109,13 +118,24 @@ class MacGatewayInfoV4
         m_rtmsg.m_rtm.rtm_seq = ++seq;
         m_rtmsg.m_rtm.rtm_addrs = rtm_addrs;
 
-        so_dst.sa_family = AF_INET;
-        so_dst.sa_len = sizeof(struct sockaddr_in);
-        so_mask.sa_family = AF_INET;
-        so_mask.sa_len = sizeof(struct sockaddr_in);
+        const bool ipv6 = dest.is_ipv6();
 
-        OPENVPN_NEXTADDR(RTA_DST, so_dst);
-        OPENVPN_NEXTADDR(RTA_NETMASK, so_mask);
+        if (!ipv6)
+        {
+            so_dst = dest.to_ipv4().to_sockaddr();
+            // 32 netmask to lookup the route to the destination
+            so_mask.sin_family = AF_INET;
+            so_mask.sin_addr.s_addr = 0xffffffff;
+            so_mask.sin_len = sizeof(struct sockaddr_in);
+
+            OPENVPN_NEXTADDR(RTA_DST, so_dst);
+            OPENVPN_NEXTADDR(RTA_NETMASK, so_mask);
+        }
+        else
+        {
+            so_dst6 = dest.to_ipv6().to_sockaddr();
+            OPENVPN_NEXTADDR6(RTA_DST, so_dst6);
+        }
 
         m_rtmsg.m_rtm.rtm_msglen = l = cp - (char *)&m_rtmsg;
 
@@ -123,8 +143,12 @@ class MacGatewayInfoV4
         sockfd.reset(socket(PF_ROUTE, SOCK_RAW, 0));
         if (!sockfd.defined())
             throw route_gateway_error("GDG: socket #1 failed");
-        if (::write(sockfd(), (char *)&m_rtmsg, l) < 0)
-            throw route_gateway_error("GDG: problem writing to routing socket");
+
+        auto ret = ::write(sockfd(), (char *)&m_rtmsg, l);
+        if (ret < 0)
+            throw route_gateway_error("GDG: problem writing to routing socket: " + std::to_string(ret)
+                                      + " errno: " + std::to_string(errno) + " msg: " + ::strerror(errno));
+
         do
         {
             l = ::read(sockfd(), (char *)&m_rtmsg, sizeof(m_rtmsg));
@@ -156,7 +180,7 @@ class MacGatewayInfoV4
         if (gate != nullptr)
         {
             /* get default gateway addr */
-            gateway_.addr.reset_ipv4_from_uint32(ntohl(((struct sockaddr_in *)gate)->sin_addr.s_addr));
+            gateway_.addr = IP::Addr::from_sockaddr(gate);
             if (!gateway_.addr.unspecified())
                 flags_ |= ADDR_DEFINED;
 
@@ -174,94 +198,64 @@ class MacGatewayInfoV4
             }
         }
 
-        /* get netmask of interface that owns default gateway */
-        if (flags_ & IFACE_DEFINED)
+        /* get netmask of interface that owns default gateway. Querying the IPv6 netmask does not
+         * seem to work on my system (Arne), so it is disabled for now until we can figure out why it
+         * doesn't work */
+        if (flags_ & IFACE_DEFINED && gateway_.addr.version() == IP::Addr::V4)
         {
-            struct ifreq ifr;
+            ifreq ifr{};
+            sa_family_t sa_family;
 
-            sockfd.reset(socket(AF_INET, SOCK_DGRAM, 0));
+            sa_family = AF_INET;
+            ifr.ifr_addr.sa_family = sa_family;
+            string::strncpynt(ifr.ifr_name, iface_, IFNAMSIZ);
+
+            sockfd.reset(socket(sa_family, SOCK_DGRAM, 0));
             if (!sockfd.defined())
                 throw route_gateway_error("GDG: socket #2 failed");
 
-            std::memset(&ifr, 0, sizeof(ifr));
-            ifr.ifr_addr.sa_family = AF_INET;
-            string::strncpynt(ifr.ifr_name, iface_, IFNAMSIZ);
-
             if (::ioctl(sockfd(), SIOCGIFNETMASK, (char *)&ifr) < 0)
-                throw route_gateway_error("GDG: ioctl #1 failed");
-            sockfd.close();
+                throw route_gateway_error("GDG: ioctl SIOCGIFNETMASK failed");
 
-            gateway_.netmask.reset_ipv4_from_uint32(ntohl(((struct sockaddr_in *)&ifr.ifr_addr)->sin_addr.s_addr));
+            gateway_.netmask = IP::Addr::from_sockaddr(&ifr.ifr_addr));
             flags_ |= NETMASK_DEFINED;
+
+            sockfd.close();
         }
 
         /* try to read MAC addr associated with interface that owns default gateway */
         if (flags_ & IFACE_DEFINED)
         {
-            struct ifconf ifc;
-            const int bufsize = 4096;
+            struct ifaddrs *ifaddrp, *ifa;
 
-            const std::unique_ptr<char[]> buffer(new char[bufsize]);
-            std::memset(buffer.get(), 0, bufsize);
-            sockfd.reset(socket(AF_INET, SOCK_DGRAM, 0));
-            if (!sockfd.defined())
-                throw route_gateway_error("GDG: socket #3 failed");
-
-            ifc.ifc_len = bufsize;
-            ifc.ifc_buf = buffer.get();
-
-            if (::ioctl(sockfd(), SIOCGIFCONF, (char *)&ifc) < 0)
-                throw route_gateway_error("GDG: ioctl #2 failed");
-            sockfd.close();
-
-            for (cp = buffer.get(); cp <= buffer.get() + ifc.ifc_len - sizeof(struct ifreq);)
+            if (getifaddrs(&ifaddrp) != 0)
             {
-                ifreq ifr = {};
-                std::memcpy(&ifr, cp, sizeof(ifr));
-                const size_t len = sizeof(ifr.ifr_name) + std::max(sizeof(ifr.ifr_addr), size_t{ifr.ifr_addr.sa_len});
+                throw route_gateway_error("GDG: getifaddrs failed errno: " + std::to_string(errno) + " msg: " + ::strerror(errno));
+            }
 
-                if (!ifr.ifr_addr.sa_family)
-                    break;
-                if (!::strncmp(ifr.ifr_name, iface_, IFNAMSIZ))
+            /* put the pointer into a unique_ptr to have RAII (allow throwing etc) */
+            std::unique_ptr<::ifaddrs, decltype(&::freeifaddrs)> ifap{ifaddrp, &::freeifaddrs};
+
+            for (ifa = ifap.get(); ifa != nullptr; ifa = ifa->ifa_next)
+            {
+                if (ifa->ifa_addr == nullptr)
+                    continue;
+
+                if (flags_ & IFACE_DEFINED
+                    && ifa->ifa_addr->sa_family == AF_LINK
+                    && !strncmp(ifa->ifa_name, iface_, IFNAMSIZ))
                 {
-                    if (ifr.ifr_addr.sa_family == AF_LINK)
-                    {
-                        /* This is a confusing member access on multiple levels.
-                         *
-                         * struct sockaddr_dl is 20 bytes in size and has
-                         * 12 bytes space for the hw address (6 bytes)
-                         * and Ethernet interface name (max 16 bytes)
-                         *
-                         * So if the interface name is more than 6 byte, it
-                         * 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.
-                         *
-                         * Since we only copied 32 bytes from cp to ifr but sdl
-                         * might extend after ifr's end, we  need to copy from
-                         * cp directly to avoid missing out on extra bytes
-                         * behind the struct
-                         */
-                        const size_t sock_dl_len = std::max(sizeof(sockaddr_dl), size_t{ifr.ifr_addr.sa_len});
+                    const struct sockaddr_dl *sockaddr_dl = reinterpret_cast<struct sockaddr_dl *>(ifa->ifa_addr);
 
-                        const std::unique_ptr<char[]> sock_dl_buf(new char[sock_dl_len]);
-                        std::memcpy(sock_dl_buf.get(), cp + offsetof(struct ifreq, ifr_addr), sock_dl_len);
-
-                        const struct sockaddr_dl *sockaddr_dl = reinterpret_cast<struct sockaddr_dl *>(sock_dl_buf.get());
-
-                        hwaddr_.reset(reinterpret_cast<unsigned char *>(LLADDR(sockaddr_dl)));
-                        flags_ |= HWADDR_DEFINED;
-                    }
+                    hwaddr_.reset(reinterpret_cast<unsigned char *>(LLADDR(sockaddr_dl)));
+                    flags_ |= HWADDR_DEFINED;
                 }
-                cp += len;
             }
         }
     }
 #undef OPENVPN_ROUNDUP
 #undef OPENVPN_NEXTADDR
+#undef OPENVPN_NEXTADDR6
 #undef OPENVPN_ADVANCE
 
     std::string info() const
@@ -323,7 +317,7 @@ class MacGatewayInfoV4
     }
 
   private:
-    unsigned int flags_;
+    unsigned int flags_ = 0;
     IP::AddrMaskPair gateway_;
     char iface_[16];
     MACAddr hwaddr_;

test/unittests/test_ip.cpp

@@ -8,6 +8,7 @@
 #include <openvpn/addr/ip.hpp>
 #include <openvpn/addr/pool.hpp>
 #include <openvpn/addr/ipv6.hpp>
+#include <sys/socket.h>
 
 using namespace openvpn;
 
@@ -134,6 +135,11 @@ void do_shift_tests(std::vector<test_case> test_vectors, bool leftshift)
         auto ret = shifted_addr.to_sockaddr();
 
         sockaddr_in6 cmp{};
+#if defined(SIN6_LEN) || defined(__APPLE__) || defined(__FreeBSD__)
+        /* Enable this test on the platforms that we know to have sin6_len
+         * to not only depend on SIN6_LEN */
+        cmp.sin6_len = sizeof(sockaddr_in6);
+#endif
         cmp.sin6_family = AF_INET6;
         for (int i = 0; i < 16; i++)
         {