From ab1d3ece026844e682676673b8a461964a5b3ce4 Mon Sep 17 00:00:00 2001
From: laanwj <126646+laanwj@users.noreply.github.com>
Date: Tue, 1 Oct 2024 16:28:28 +0200
Subject: [PATCH] net: Add optional length checking to CService::SetSockAddr

In almost all cases (the only exception is `getifaddrs`), we know the
size of the data passed into SetSockAddr, so we can check this to be
what is expected.
---
 src/common/netif.cpp | 9 +--------
 src/common/pcp.cpp   | 2 +-
 src/net.cpp          | 4 ++--
 src/netaddress.cpp   | 4 +++-
 src/netaddress.h     | 9 ++++++++-
 5 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/src/common/netif.cpp b/src/common/netif.cpp
index 08f034a412a..7424f977c7e 100644
--- a/src/common/netif.cpp
+++ b/src/common/netif.cpp
@@ -169,16 +169,9 @@ std::optional<CNetAddr> QueryDefaultGatewayImpl(sa_family_t family)
 
 std::optional<CNetAddr> FromSockAddr(const struct sockaddr* addr)
 {
-    // Check valid length. Note that sa_len is not part of POSIX, and exists on MacOS and some BSDs only, so we can't
-    // do this check in SetSockAddr.
-    if (!(addr->sa_family == AF_INET && addr->sa_len == sizeof(struct sockaddr_in)) &&
-        !(addr->sa_family == AF_INET6 && addr->sa_len == sizeof(struct sockaddr_in6))) {
-        return std::nullopt;
-    }
-
     // Fill in a CService from the sockaddr, then drop the port part.
     CService service;
-    if (service.SetSockAddr(addr)) {
+    if (service.SetSockAddr(addr, addr->sa_len)) {
         return (CNetAddr)service;
     }
     return std::nullopt;
diff --git a/src/common/pcp.cpp b/src/common/pcp.cpp
index 3cc1cba9242..7fbc1472b4e 100644
--- a/src/common/pcp.cpp
+++ b/src/common/pcp.cpp
@@ -426,7 +426,7 @@ std::variant<MappingResult, MappingError> PCPRequestPortMap(const PCPMappingNonc
         return MappingError::NETWORK_ERROR;
     }
     CService internal;
-    if (!internal.SetSockAddr((struct sockaddr*)&internal_addr)) return MappingError::NETWORK_ERROR;
+    if (!internal.SetSockAddr((struct sockaddr*)&internal_addr, internal_addrlen)) return MappingError::NETWORK_ERROR;
     LogPrintLevel(BCLog::NET, BCLog::Level::Debug, "pcp: Internal address after connect: %s\n", internal.ToStringAddr());
 
     // Build request packet. Make sure the packet is zeroed so that reserved fields are zero
diff --git a/src/net.cpp b/src/net.cpp
index 8ea7f6ce445..3ff466c1531 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -383,7 +383,7 @@ static CAddress GetBindAddress(const Sock& sock)
     struct sockaddr_storage sockaddr_bind;
     socklen_t sockaddr_bind_len = sizeof(sockaddr_bind);
     if (!sock.GetSockName((struct sockaddr*)&sockaddr_bind, &sockaddr_bind_len)) {
-        addr_bind.SetSockAddr((const struct sockaddr*)&sockaddr_bind);
+        addr_bind.SetSockAddr((const struct sockaddr*)&sockaddr_bind, sockaddr_bind_len);
     } else {
         LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "getsockname failed\n");
     }
@@ -1728,7 +1728,7 @@ void CConnman::AcceptConnection(const ListenSocket& hListenSocket) {
         return;
     }
 
-    if (!addr.SetSockAddr((const struct sockaddr*)&sockaddr)) {
+    if (!addr.SetSockAddr((const struct sockaddr*)&sockaddr, len)) {
         LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "Unknown socket family\n");
     } else {
         addr = CAddress{MaybeFlipIPv6toCJDNS(addr), NODE_NONE};
diff --git a/src/netaddress.cpp b/src/netaddress.cpp
index bd2353a7121..5ad0da7792e 100644
--- a/src/netaddress.cpp
+++ b/src/netaddress.cpp
@@ -807,13 +807,15 @@ CService::CService(const struct sockaddr_in6 &addr) : CNetAddr(addr.sin6_addr, a
    assert(addr.sin6_family == AF_INET6);
 }
 
-bool CService::SetSockAddr(const struct sockaddr *paddr)
+bool CService::SetSockAddr(const struct sockaddr *paddr, socklen_t addrlen)
 {
     switch (paddr->sa_family) {
     case AF_INET:
+        if (addrlen != sizeof(struct sockaddr_in)) return false;
         *this = CService(*(const struct sockaddr_in*)paddr);
         return true;
     case AF_INET6:
+        if (addrlen != sizeof(struct sockaddr_in6)) return false;
         *this = CService(*(const struct sockaddr_in6*)paddr);
         return true;
     default:
diff --git a/src/netaddress.h b/src/netaddress.h
index 24f5c3fb962..ad83c5381c3 100644
--- a/src/netaddress.h
+++ b/src/netaddress.h
@@ -539,7 +539,14 @@ public:
     explicit CService(const struct sockaddr_in& addr);
     uint16_t GetPort() const;
     bool GetSockAddr(struct sockaddr* paddr, socklen_t* addrlen) const;
-    bool SetSockAddr(const struct sockaddr* paddr);
+    /**
+     * Set CService from a network sockaddr.
+     * @param[in] paddr Pointer to sockaddr structure
+     * @param[in] addrlen Length of sockaddr structure in bytes. This will be checked to exactly match the length of
+     * a socket address of the provided family, unless std::nullopt is passed
+     * @returns true on success
+     */
+    bool SetSockAddr(const struct sockaddr* paddr, socklen_t addrlen);
     /**
      * Get the address family
      * @returns AF_UNSPEC if unspecified