diff --git a/CHANGELOG.md b/CHANGELOG.md index 819a741b73..f69380bcf4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,9 @@ Increment the: * [CODE HEALTH] Move remaining API test helpers into anonymous namespaces [#4301](https://github.com/open-telemetry/opentelemetry-cpp/pull/4301) +* [BUG] Make SocketAddr string parsing safe and reject malformed addresses + [#4292](https://github.com/open-telemetry/opentelemetry-cpp/pull/4292) + * [CODE HEALTH] Move metrics storage test fixtures into anonymous namespace [#4286](https://github.com/open-telemetry/opentelemetry-cpp/pull/4286) diff --git a/ext/BUILD b/ext/BUILD index 52ed22afb5..82ff180a0e 100644 --- a/ext/BUILD +++ b/ext/BUILD @@ -8,5 +8,10 @@ package(default_visibility = ["//visibility:public"]) cc_library( name = "headers", hdrs = glob(["include/**/*.h"]), + linkopts = select({ + # socket_tools.h calls Winsock; declare it on the interface so consumers link it too. + "//bazel:windows": ["-DEFAULTLIB:Ws2_32.lib"], + "//conditions:default": [], + }), strip_include_prefix = "include", ) diff --git a/ext/CMakeLists.txt b/ext/CMakeLists.txt index d50cf3a759..39a97c4dea 100644 --- a/ext/CMakeLists.txt +++ b/ext/CMakeLists.txt @@ -10,6 +10,14 @@ target_include_directories( set_target_properties(opentelemetry_ext PROPERTIES EXPORT_NAME "ext") target_link_libraries(opentelemetry_ext INTERFACE opentelemetry_api) +# The embedded HTTP server headers (socket_tools.h) call Winsock, so declare the +# dependency on the interface target. MSVC also autolinks via #pragma +# comment(lib), but MinGW/GCC ignore that, so consumers there would otherwise +# fail to resolve inet_pton/WSAStartup. +if(WIN32) + target_link_libraries(opentelemetry_ext INTERFACE ws2_32) +endif() + otel_add_component( COMPONENT ext_common diff --git a/ext/include/opentelemetry/ext/http/server/socket_tools.h b/ext/include/opentelemetry/ext/http/server/socket_tools.h index 6a5b830f91..5b0a1ab487 100644 --- a/ext/include/opentelemetry/ext/http/server/socket_tools.h +++ b/ext/include/opentelemetry/ext/http/server/socket_tools.h @@ -22,6 +22,7 @@ // # include # include +# include // inet_pton // TODO: consider NOMINMAX # undef min @@ -181,46 +182,85 @@ struct SocketAddr inet4.sin_addr.s_addr = htonl(addr); } + /// Parses an IPv4 address in "host" or "host:port" form. Host parsing follows the platform's + /// inet_pton(AF_INET), which requires four decimal components; a port, when present, must be + /// decimal digits only in the range 0..65535, and an omitted port is represented as 0. Invalid + /// input leaves the address at AF_UNSPEC, for which port() returns -1. SocketAddr(char const *addr) { -#ifdef _WIN32 - INT addrlen = sizeof(m_data); - WCHAR buf[200]; - for (int i = 0; i < sizeof(buf) && addr[i]; i++) + // One parser for every platform: inet_pton (Winsock provides it since Vista) plus a strict + // decimal port. This avoids WSAStringToAddress, whose grammar and default-component filling + // differ from the POSIX path. Parse into a local sockaddr_in and commit with memcpy only on + // success, which keeps m_data at AF_UNSPEC on failure and avoids accessing the sockaddr + // storage through a sockaddr_in glvalue (an alignment/type-access issue tracked in #4307). + if (addr == nullptr) { - buf[i] = addr[i]; + LOG_WARN("SocketAddr: cannot parse a null address"); + return; // m_data is already AF_UNSPEC, so port() reports -1. } - buf[199] = L'\0'; - ::WSAStringToAddressW(buf, AF_INET, nullptr, &m_data, &addrlen); -#else - sockaddr_in &inet4 = reinterpret_cast(m_data); - inet4.sin_family = AF_INET; - char const *colon = strchr(addr, ':'); - if (colon) + + sockaddr_in parsed{}; + parsed.sin_family = AF_INET; + + char const *colon = strchr(addr, ':'); + char const *hostEnd = colon ? colon : addr + strlen(addr); + ptrdiff_t const hostLength = hostEnd - addr; + + // Reject a host that would not fit rather than truncating it into a different valid address + // (for example "255.255.255.2559" would otherwise become "255.255.255.255"). No dotted-quad + // exceeds 15 characters. + bool ok = (hostLength >= 1 && hostLength <= 15); + if (ok) { - char *portEnd = nullptr; - errno = 0; - auto const parsed = std::strtol(colon + 1, &portEnd, 10); - // Accept only a converted, in-range port value; fall back to 0 otherwise. - if (errno == 0 && portEnd != colon + 1 && parsed >= 0 && parsed <= 65535) + char host[16]; + memcpy(host, addr, static_cast(hostLength)); + host[hostLength] = '\0'; + ok = (::inet_pton(AF_INET, host, &parsed.sin_addr) == 1); + } + + // Port: decimal digits only, with overflow checked before it can wrap. strtol would also + // accept a leading sign or whitespace and depend on the locale. + if (ok && colon) + { + char const *p = colon + 1; + unsigned int port = 0; + if (*p == '\0') { - inet4.sin_port = htons(static_cast(parsed)); + ok = false; // empty port, e.g. "127.0.0.1:" } - else + while (ok && *p != '\0') + { + if (*p < '0' || *p > '9') + { + ok = false; + break; + } + unsigned int const digit = static_cast(*p - '0'); + if (port > (65535u - digit) / 10u) + { + ok = false; // would exceed 65535 + break; + } + port = port * 10u + digit; + ++p; + } + if (ok) { - inet4.sin_port = 0; + parsed.sin_port = htons(static_cast(port)); } - char buf[16]; - memcpy(buf, addr, (std::min)(15, colon - addr)); - buf[15] = '\0'; - ::inet_pton(AF_INET, buf, &inet4.sin_addr); + } + + if (ok) + { + memcpy(&m_data, &parsed, sizeof(parsed)); } else { - inet4.sin_port = 0; - ::inet_pton(AF_INET, addr, &inet4.sin_addr); + // Leave m_data at AF_UNSPEC; port() returns -1 so callers can tell a parse failure from a + // real endpoint, including the legitimate ":0". Do not echo the raw input, which may be + // arbitrarily long. + LOG_WARN("SocketAddr: cannot parse address"); } -#endif } operator sockaddr *() { return &m_data; } @@ -232,7 +272,10 @@ struct SocketAddr switch (m_data.sa_family) { case AF_INET: { - sockaddr_in const &inet4 = reinterpret_cast(m_data); + // Copy out rather than binding a sockaddr_in glvalue to sockaddr storage, which is an + // alignment/type-access issue (see the constructor and #4307). + sockaddr_in inet4{}; + memcpy(&inet4, &m_data, sizeof(inet4)); return ntohs(inet4.sin_port); } @@ -248,8 +291,9 @@ struct SocketAddr switch (m_data.sa_family) { case AF_INET: { - sockaddr_in const &inet4 = reinterpret_cast(m_data); - u_long addr = ntohl(inet4.sin_addr.s_addr); + sockaddr_in inet4{}; + memcpy(&inet4, &m_data, sizeof(inet4)); + u_long addr = ntohl(inet4.sin_addr.s_addr); os << (addr >> 24) << '.' << ((addr >> 16) & 255) << '.' << ((addr >> 8) & 255) << '.' << (addr & 255); os << ':' << ntohs(inet4.sin_port); @@ -263,6 +307,18 @@ struct SocketAddr } }; +// The parser memcpys a sockaddr_in into m_data, and the socket syscalls pass sizeof(SocketAddr) +// as the address length. This wrapper is IPv4-only, so require sockaddr and sockaddr_in to have +// the exact same size rather than trusting every ABI: passing an address length that is too large +// for the family is a documented EINVAL for connect()/bind(). Exact equality also keeps the memcpy +// safe. Together with the assertion below, sizeof(SocketAddr) == sizeof(sockaddr_in). +static_assert(sizeof(sockaddr) == sizeof(sockaddr_in), + "SocketAddr is IPv4-only: sockaddr and sockaddr_in must have identical size"); +static_assert(offsetof(sockaddr, sa_family) == offsetof(sockaddr_in, sin_family), + "sockaddr and sockaddr_in must place the address family at the same offset"); +static_assert(sizeof(SocketAddr) == sizeof(sockaddr), + "SocketAddr must add no storage beyond its sockaddr, since syscalls use its size"); + /// /// Encapsulation of a socket (non-exclusive ownership) /// diff --git a/ext/test/http/BUILD b/ext/test/http/BUILD index 2e6a07e688..a94a2d19e3 100644 --- a/ext/test/http/BUILD +++ b/ext/test/http/BUILD @@ -3,6 +3,20 @@ load("@rules_cc//cc:cc_test.bzl", "cc_test") +cc_test( + name = "socket_tools_test", + srcs = [ + "socket_tools_test.cc", + ], + # ws2_32 is an interface linkopt of //ext:headers, so it is inherited here rather than + # relinked, which also lets the test verify the public target's usage requirements. + tags = ["test"], + deps = [ + "//ext:headers", + "@com_google_googletest//:gtest_main", + ], +) + cc_test( name = "curl_http_test", srcs = [ diff --git a/ext/test/http/CMakeLists.txt b/ext/test/http/CMakeLists.txt index b7707bd8c2..14d6aee613 100644 --- a/ext/test/http/CMakeLists.txt +++ b/ext/test/http/CMakeLists.txt @@ -14,6 +14,18 @@ if(WITH_HTTP_CLIENT_CURL) TEST_LIST ${FILENAME}) endif() +set(SOCKET_TOOLS_FILENAME socket_tools_test) +add_executable(${SOCKET_TOOLS_FILENAME} ${SOCKET_TOOLS_FILENAME}.cc) +target_link_libraries(${SOCKET_TOOLS_FILENAME} opentelemetry_ext ${GMOCK_LIB} + ${GTEST_BOTH_LIBRARIES} ${CMAKE_THREAD_LIBS_INIT}) +# ws2_32 is now an interface dependency of opentelemetry_ext, so it is inherited +# here; the test thus verifies the public target's usage requirements rather +# than papering over them locally. +gtest_add_tests( + TARGET ${SOCKET_TOOLS_FILENAME} + TEST_PREFIX ext.http.sockettools. + TEST_LIST ${SOCKET_TOOLS_FILENAME}) + set(URL_PARSER_FILENAME url_parser_test) add_executable(${URL_PARSER_FILENAME} ${URL_PARSER_FILENAME}.cc) target_link_libraries(${URL_PARSER_FILENAME} opentelemetry_ext ${GMOCK_LIB} diff --git a/ext/test/http/socket_tools_test.cc b/ext/test/http/socket_tools_test.cc new file mode 100644 index 0000000000..34f9dac51b --- /dev/null +++ b/ext/test/http/socket_tools_test.cc @@ -0,0 +1,151 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +#include +#include +#ifndef _WIN32 +# include // for sockaddr, AF_UNSPEC, AF_INET +#endif + +#include "opentelemetry/ext/http/server/socket_tools.h" + +namespace +{ + +// A parsed address reports AF_INET and a non-negative port; a rejected one is left at AF_UNSPEC +// with port() == -1. Checking the family too, not just the port, catches a family byte being +// corrupted while port() happens to still return -1. +void ExpectInvalid(const SocketTools::SocketAddr &addr) +{ + EXPECT_EQ(addr.m_data.sa_family, AF_UNSPEC); + EXPECT_EQ(addr.port(), -1); +} + +TEST(SocketAddrTest, ParsesHostAndPort) +{ + SocketTools::SocketAddr addr("127.0.0.1:8800"); + EXPECT_EQ(addr.m_data.sa_family, AF_INET); + EXPECT_EQ(addr.port(), 8800); + EXPECT_EQ(addr.toString(), "127.0.0.1:8800"); +} + +// A host shorter than the buffer must be null-terminated at its actual length; terminating at a +// fixed index would leave inet_pton reading indeterminate bytes for any host under 15 characters. +TEST(SocketAddrTest, ParsesHostShorterThanTheBuffer) +{ + SocketTools::SocketAddr addr("1.2.3.4:80"); + EXPECT_EQ(addr.port(), 80); + EXPECT_EQ(addr.toString(), "1.2.3.4:80"); +} + +TEST(SocketAddrTest, ParsesLongestRepresentableHost) +{ + SocketTools::SocketAddr addr("255.255.255.255:65535"); + EXPECT_EQ(addr.port(), 65535); + EXPECT_EQ(addr.toString(), "255.255.255.255:65535"); +} + +TEST(SocketAddrTest, ParsesHostWithoutPort) +{ + SocketTools::SocketAddr addr("10.0.0.1"); + EXPECT_EQ(addr.port(), 0); + EXPECT_EQ(addr.toString(), "10.0.0.1:0"); +} + +// A legitimate port 0 must be distinguishable from a parse failure, so this must parse rather +// than collapse to the same state the rejections use. +TEST(SocketAddrTest, ParsesLegitimateZeroPort) +{ + SocketTools::SocketAddr addr("127.0.0.1:0"); + EXPECT_EQ(addr.m_data.sa_family, AF_INET); + EXPECT_EQ(addr.port(), 0); + EXPECT_EQ(addr.toString(), "127.0.0.1:0"); +} + +// A decimal port may carry leading zeros; the value is what matters, so "080" is port 80. The +// pre-multiply overflow check still bounds the value at 65535 no matter how many zeros precede it. +TEST(SocketAddrTest, AcceptsLeadingZeroPort) +{ + SocketTools::SocketAddr addr("127.0.0.1:080"); + EXPECT_EQ(addr.m_data.sa_family, AF_INET); + EXPECT_EQ(addr.port(), 80); +} + +TEST(SocketAddrTest, RejectsOutOfRangePort) +{ + SocketTools::SocketAddr addr("127.0.0.1:99999"); + ExpectInvalid(addr); +} + +// A host longer than the buffer must be rejected, not truncated into a different valid address: +// "255.255.255.2559" must not become "255.255.255.255". +TEST(SocketAddrTest, RejectsOverlongHostInsteadOfTruncating) +{ + SocketTools::SocketAddr addr("255.255.255.2559:80"); + ExpectInvalid(addr); +} + +TEST(SocketAddrTest, RejectsTrailingGarbageInPort) +{ + ExpectInvalid(SocketTools::SocketAddr("127.0.0.1:80junk")); + ExpectInvalid(SocketTools::SocketAddr("127.0.0.1:80:90")); +} + +TEST(SocketAddrTest, RejectsInvalidHost) +{ + ExpectInvalid(SocketTools::SocketAddr("999.999.999.999:4318")); + ExpectInvalid(SocketTools::SocketAddr("garbage")); +} + +// A host far longer than the 15-character dotted-quad maximum must be rejected by the length +// bound before it can drive an out-of-bounds access on the fixed host buffer. +TEST(SocketAddrTest, RejectsExtremelyOverlongHost) +{ + const std::string overlong(512, '9'); + SocketTools::SocketAddr addr(overlong.c_str()); + ExpectInvalid(addr); +} + +TEST(SocketAddrTest, HandlesEmptyInput) +{ + SocketTools::SocketAddr addr(""); + ExpectInvalid(addr); +} + +TEST(SocketAddrTest, RejectsNullInput) +{ + SocketTools::SocketAddr addr(nullptr); + ExpectInvalid(addr); +} + +TEST(SocketAddrTest, RejectsEmptyHostOrPort) +{ + ExpectInvalid(SocketTools::SocketAddr(":80")); + ExpectInvalid(SocketTools::SocketAddr("127.0.0.1:")); +} + +// The port grammar is decimal digits only: a sign or whitespace that strtol would have accepted +// must be rejected so both platforms agree. +TEST(SocketAddrTest, RejectsSignAndWhitespaceInPort) +{ + ExpectInvalid(SocketTools::SocketAddr("127.0.0.1:+80")); + ExpectInvalid(SocketTools::SocketAddr("127.0.0.1:-0")); + ExpectInvalid(SocketTools::SocketAddr("127.0.0.1: 80")); +} + +TEST(SocketAddrTest, RejectsPortOverflow) +{ + ExpectInvalid(SocketTools::SocketAddr("127.0.0.1:65536")); + ExpectInvalid(SocketTools::SocketAddr("127.0.0.1:4294967377")); +} + +// inet_pton() requires four decimal components, so it rejects the shorthand form ("127.1") that the +// legacy inet_aton()/inet_addr() resolvers accepted. This holds on every platform (verified on +// glibc and macOS). Leading-zero components ("01.02.03.004") are deliberately not asserted: glibc +// rejects them but macOS inet_pton accepts them, so that outcome is platform-dependent. +TEST(SocketAddrTest, RejectsShorthandHost) +{ + ExpectInvalid(SocketTools::SocketAddr("127.1:80")); +} + +} // namespace