From 23231c1e64438f1139c798a13f7d3b9b1e09b6c8 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 1/2] 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 08f034a412..7424f977c7 100644 --- a/src/common/netif.cpp +++ b/src/common/netif.cpp @@ -169,16 +169,9 @@ std::optional QueryDefaultGatewayImpl(sa_family_t family) std::optional 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 3cc1cba924..7fbc1472b4 100644 --- a/src/common/pcp.cpp +++ b/src/common/pcp.cpp @@ -426,7 +426,7 @@ std::variant 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 8ea7f6ce44..3ff466c153 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 bd2353a712..5ad0da7792 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 24f5c3fb96..ad83c5381c 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 From 219872fc75e552c87c20b5c1ce7e15fa887eec20 Mon Sep 17 00:00:00 2001 From: laanwj <126646+laanwj@users.noreply.github.com> Date: Tue, 1 Oct 2024 16:32:40 +0200 Subject: [PATCH 2/2] net: Use GetAdaptersAddresses to get local addresses on Windows Instead of a `gethostname` hack, use the official way of calling `GetAdaptersAddresses` to get local network addresses on Windows. As additional cleanup, move out `FromSockAddr` from MacOS and use it everywhere appropriate. Suggested by Ava Chow. --- src/common/netif.cpp | 93 ++++++++++++++++++++++++++++++++++---------- 1 file changed, 72 insertions(+), 21 deletions(-) diff --git a/src/common/netif.cpp b/src/common/netif.cpp index 7424f977c7..b31bb179d8 100644 --- a/src/common/netif.cpp +++ b/src/common/netif.cpp @@ -31,6 +31,30 @@ namespace { +//! Return CNetAddr for the specified OS-level network address. If a length is provided, it is checked for validity. +//! If a length is not given, it is taken to be sizeof(struct sockaddr_*) for the family. +std::optional FromSockAddr(const struct sockaddr* addr, std::optional sa_len_opt) +{ + socklen_t sa_len = 0; + if (sa_len_opt.has_value()) { + sa_len = *sa_len_opt; + } else { + // If sockaddr length was not specified, determine it from the family. + switch (addr->sa_family) { + case AF_INET: sa_len = sizeof(struct sockaddr_in); break; + case AF_INET6: sa_len = sizeof(struct sockaddr_in6); break; + default: + return std::nullopt; + } + } + // Fill in a CService from the sockaddr, then drop the port part. + CService service; + if (service.SetSockAddr(addr, sa_len)) { + return (CNetAddr)service; + } + return std::nullopt; +} + // Linux and FreeBSD 14.0+. For FreeBSD 13.2 the code can be compiled but // running it requires loading a special kernel module, otherwise socket(AF_NETLINK,...) // will fail, so we skip that. @@ -167,16 +191,6 @@ std::optional QueryDefaultGatewayImpl(sa_family_t family) #define ROUNDUP32(a) \ ((a) > 0 ? (1 + (((a) - 1) | (sizeof(uint32_t) - 1))) : sizeof(uint32_t)) -std::optional FromSockAddr(const struct sockaddr* addr) -{ - // Fill in a CService from the sockaddr, then drop the port part. - CService service; - if (service.SetSockAddr(addr, addr->sa_len)) { - return (CNetAddr)service; - } - return std::nullopt; -} - //! MacOS: Get default gateway from route table. See route(4) for the format. std::optional QueryDefaultGatewayImpl(sa_family_t family) { @@ -210,9 +224,9 @@ std::optional QueryDefaultGatewayImpl(sa_family_t family) const struct sockaddr* sa = (const struct sockaddr*)(buf.data() + sa_pos); if ((sa_pos + sa->sa_len) > next_msg_pos) return std::nullopt; if (i == RTAX_DST) { - dst = FromSockAddr(sa); + dst = FromSockAddr(sa, sa->sa_len); } else if (i == RTAX_GATEWAY) { - gateway = FromSockAddr(sa); + gateway = FromSockAddr(sa, sa->sa_len); } // Skip sockaddr entries for bit flags we're not interested in, // move cursor. @@ -269,9 +283,49 @@ std::vector GetLocalAddresses() { std::vector addresses; #ifdef WIN32 - char pszHostName[256] = ""; - if (gethostname(pszHostName, sizeof(pszHostName)) != SOCKET_ERROR) { - addresses = LookupHost(pszHostName, 0, true); + DWORD status = 0; + constexpr size_t MAX_ADAPTER_ADDR_SIZE = 4 * 1000 * 1000; // Absolute maximum size of adapter addresses structure we're willing to handle, as a precaution. + std::vector out_buf(15000, {}); // Start with 15KB allocation as recommended in GetAdaptersAddresses documentation. + while (true) { + ULONG out_buf_len = out_buf.size(); + status = GetAdaptersAddresses(AF_UNSPEC, GAA_FLAG_SKIP_ANYCAST | GAA_FLAG_SKIP_MULTICAST | GAA_FLAG_SKIP_DNS_SERVER | GAA_FLAG_SKIP_FRIENDLY_NAME, + nullptr, reinterpret_cast(out_buf.data()), &out_buf_len); + if (status == ERROR_BUFFER_OVERFLOW && out_buf.size() < MAX_ADAPTER_ADDR_SIZE) { + // If status == ERROR_BUFFER_OVERFLOW, out_buf_len will contain the needed size. + // Unfortunately, this cannot be fully relied on, because another process may have added interfaces. + // So to avoid getting stuck due to a race condition, double the buffer size at least + // once before retrying (but only up to the maximum allowed size). + do { + out_buf.resize(std::min(out_buf.size() * 2, MAX_ADAPTER_ADDR_SIZE)); + } while (out_buf.size() < out_buf_len && out_buf.size() < MAX_ADAPTER_ADDR_SIZE); + } else { + break; + } + } + + if (status != NO_ERROR) { + // This includes ERROR_NO_DATA if there are no addresses and thus there's not even one PIP_ADAPTER_ADDRESSES + // record in the returned structure. + LogPrintLevel(BCLog::NET, BCLog::Level::Error, "Could not get local adapter addreses: %s\n", NetworkErrorString(status)); + return addresses; + } + + // Iterate over network adapters. + for (PIP_ADAPTER_ADDRESSES cur_adapter = reinterpret_cast(out_buf.data()); + cur_adapter != nullptr; cur_adapter = cur_adapter->Next) { + if (cur_adapter->OperStatus != IfOperStatusUp) continue; + if (cur_adapter->IfType == IF_TYPE_SOFTWARE_LOOPBACK) continue; + + // Iterate over unicast addresses for adapter, the only address type we're interested in. + for (PIP_ADAPTER_UNICAST_ADDRESS cur_address = cur_adapter->FirstUnicastAddress; + cur_address != nullptr; cur_address = cur_address->Next) { + // "The IP address is a cluster address and should not be used by most applications." + if ((cur_address->Flags & IP_ADAPTER_ADDRESS_TRANSIENT) != 0) continue; + + if (std::optional addr = FromSockAddr(cur_address->Address.lpSockaddr, static_cast(cur_address->Address.iSockaddrLength))) { + addresses.push_back(*addr); + } + } } #elif (HAVE_DECL_GETIFADDRS && HAVE_DECL_FREEIFADDRS) struct ifaddrs* myaddrs; @@ -281,12 +335,9 @@ std::vector GetLocalAddresses() if (ifa->ifa_addr == nullptr) continue; if ((ifa->ifa_flags & IFF_UP) == 0) continue; if ((ifa->ifa_flags & IFF_LOOPBACK) != 0) continue; - if (ifa->ifa_addr->sa_family == AF_INET) { - struct sockaddr_in* s4 = (struct sockaddr_in*)(ifa->ifa_addr); - addresses.emplace_back(s4->sin_addr); - } else if (ifa->ifa_addr->sa_family == AF_INET6) { - struct sockaddr_in6* s6 = (struct sockaddr_in6*)(ifa->ifa_addr); - addresses.emplace_back(s6->sin6_addr); + + if (std::optional addr = FromSockAddr(ifa->ifa_addr, std::nullopt)) { + addresses.push_back(*addr); } } freeifaddrs(myaddrs);