From 165342beac61a5573c8eb422cb5bc7001adbf0c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= Date: Sun, 25 Sep 2022 12:34:05 +0000 Subject: [PATCH] inspector: harden IP address validation again Use inet_pton() to parse IP addresses, which restricts IP addresses to a small number of well-defined formats. In particular, octal and hexadecimal number formats are not allowed, and neither are leading zeros. Also explicitly reject 0.0.0.0/8 and ::/128 as non-routable. Refs: https://hackerone.com/reports/1710652 CVE-ID: CVE-2022-43548 PR-URL: https://github.com/nodejs-private/node-private/pull/354 Reviewed-by: Michael Dawson Reviewed-by: Rafael Gonzaga Reviewed-by: Rich Trott --- src/inspector_socket.cc | 77 ++++++++++++++++++++------ test/cctest/test_inspector_socket.cc | 80 ++++++++++++++++++++++++++++ 2 files changed, 141 insertions(+), 16 deletions(-) diff --git a/src/inspector_socket.cc b/src/inspector_socket.cc index ab1cdf1fa5bdb6..8001d893e1fdcc 100644 --- a/src/inspector_socket.cc +++ b/src/inspector_socket.cc @@ -162,25 +162,70 @@ static std::string TrimPort(const std::string& host) { } static bool IsIPAddress(const std::string& host) { - if (host.length() >= 4 && host.front() == '[' && host.back() == ']') - return true; - if (host.front() == '0') return false; - uint_fast16_t accum = 0; - uint_fast8_t quads = 0; - bool empty = true; - auto endOctet = [&accum, &quads, &empty](bool final = false) { - return !empty && accum <= 0xff && ++quads <= 4 && final == (quads == 4) && - (empty = true) && !(accum = 0); - }; - for (char c : host) { - if (isdigit(c)) { - if ((accum = (accum * 10) + (c - '0')) > 0xff) return false; - empty = false; - } else if (c != '.' || !endOctet()) { + // TODO(tniessen): add CVEs to the following bullet points + // To avoid DNS rebinding attacks, we are aware of the following requirements: + // * the host name must be an IP address, + // * the IP address must be routable, and + // * the IP address must be formatted unambiguously. + + // The logic below assumes that the string is null-terminated, so ensure that + // we did not somehow end up with null characters within the string. + if (host.find('\0') != std::string::npos) return false; + + // All IPv6 addresses must be enclosed in square brackets, and anything + // enclosed in square brackets must be an IPv6 address. + if (host.length() >= 4 && host.front() == '[' && host.back() == ']') { + // INET6_ADDRSTRLEN is the maximum length of the dual format (including the + // terminating null character), which is the longest possible representation + // of an IPv6 address: xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:ddd.ddd.ddd.ddd + if (host.length() - 2 >= INET6_ADDRSTRLEN) return false; + + // Annoyingly, libuv's implementation of inet_pton() deviates from other + // implementations of the function in that it allows '%' in IPv6 addresses. + if (host.find('%') != std::string::npos) return false; + + // Parse the IPv6 address to ensure it is syntactically valid. + char ipv6_str[INET6_ADDRSTRLEN]; + std::copy(host.begin() + 1, host.end() - 1, ipv6_str); + ipv6_str[host.length()] = '\0'; + unsigned char ipv6[sizeof(struct in6_addr)]; + if (uv_inet_pton(AF_INET6, ipv6_str, ipv6) != 0) return false; + + // The only non-routable IPv6 address is ::/128. It should not be necessary + // to explicitly reject it because it will still be enclosed in square + // brackets and not even macOS should make DNS requests in that case, but + // history has taught us that we cannot be careful enough. + // Note that RFC 4291 defines both "IPv4-Compatible IPv6 Addresses" and + // "IPv4-Mapped IPv6 Addresses", which means that there are IPv6 addresses + // (other than ::/128) that represent non-routable IPv4 addresses. However, + // this translation assumes that the host is interpreted as an IPv6 address + // in the first place, at which point DNS rebinding should not be an issue. + if (std::all_of(ipv6, ipv6 + sizeof(ipv6), [](auto b) { return b == 0; })) { return false; } + + // It is a syntactically valid and routable IPv6 address enclosed in square + // brackets. No client should be able to misinterpret this. + return true; } - return endOctet(true); + + // Anything not enclosed in square brackets must be an IPv4 address. It is + // important here that inet_pton() accepts only the so-called dotted-decimal + // notation, which is a strict subset of the so-called numbers-and-dots + // notation that is allowed by inet_aton() and inet_addr(). This subset does + // not allow hexadecimal or octal number formats. + unsigned char ipv4[sizeof(struct in_addr)]; + if (uv_inet_pton(AF_INET, host.c_str(), ipv4) != 0) return false; + + // The only strictly non-routable IPv4 address is 0.0.0.0, and macOS will make + // DNS requests for this IP address, so we need to explicitly reject it. In + // fact, we can safely reject all of 0.0.0.0/8 (see Section 3.2 of RFC 791 and + // Section 3.2.1.3 of RFC 1122). + // Note that inet_pton() stores the IPv4 address in network byte order. + if (ipv4[0] == 0) return false; + + // It is a routable IPv4 address in dotted-decimal notation. + return true; } // Constants for hybi-10 frame format. diff --git a/test/cctest/test_inspector_socket.cc b/test/cctest/test_inspector_socket.cc index 6ae92c4b27e232..b351a23002c9ca 100644 --- a/test/cctest/test_inspector_socket.cc +++ b/test/cctest/test_inspector_socket.cc @@ -925,6 +925,54 @@ TEST_F(InspectorSocketTest, HostIpTooManyOctetsChecked) { expect_handshake_failure(); } +TEST_F(InspectorSocketTest, HostIpInvalidOctalOctetStartChecked) { + const std::string INVALID_HOST_IP_REQUEST = "GET /json HTTP/1.1\r\n" + "Host: 08.1.1.1:9229\r\n\r\n"; + send_in_chunks(INVALID_HOST_IP_REQUEST.c_str(), + INVALID_HOST_IP_REQUEST.length()); + expect_handshake_failure(); +} + +TEST_F(InspectorSocketTest, HostIpInvalidOctalOctetMidChecked) { + const std::string INVALID_HOST_IP_REQUEST = "GET /json HTTP/1.1\r\n" + "Host: 1.09.1.1:9229\r\n\r\n"; + send_in_chunks(INVALID_HOST_IP_REQUEST.c_str(), + INVALID_HOST_IP_REQUEST.length()); + expect_handshake_failure(); +} + +TEST_F(InspectorSocketTest, HostIpInvalidOctalOctetEndChecked) { + const std::string INVALID_HOST_IP_REQUEST = "GET /json HTTP/1.1\r\n" + "Host: 1.1.1.009:9229\r\n\r\n"; + send_in_chunks(INVALID_HOST_IP_REQUEST.c_str(), + INVALID_HOST_IP_REQUEST.length()); + expect_handshake_failure(); +} + +TEST_F(InspectorSocketTest, HostIpLeadingZeroStartChecked) { + const std::string INVALID_HOST_IP_REQUEST = "GET /json HTTP/1.1\r\n" + "Host: 01.1.1.1:9229\r\n\r\n"; + send_in_chunks(INVALID_HOST_IP_REQUEST.c_str(), + INVALID_HOST_IP_REQUEST.length()); + expect_handshake_failure(); +} + +TEST_F(InspectorSocketTest, HostIpLeadingZeroMidChecked) { + const std::string INVALID_HOST_IP_REQUEST = "GET /json HTTP/1.1\r\n" + "Host: 1.1.001.1:9229\r\n\r\n"; + send_in_chunks(INVALID_HOST_IP_REQUEST.c_str(), + INVALID_HOST_IP_REQUEST.length()); + expect_handshake_failure(); +} + +TEST_F(InspectorSocketTest, HostIpLeadingZeroEndChecked) { + const std::string INVALID_HOST_IP_REQUEST = "GET /json HTTP/1.1\r\n" + "Host: 1.1.1.01:9229\r\n\r\n"; + send_in_chunks(INVALID_HOST_IP_REQUEST.c_str(), + INVALID_HOST_IP_REQUEST.length()); + expect_handshake_failure(); +} + TEST_F(InspectorSocketTest, HostIPNonRoutable) { const std::string INVALID_HOST_IP_REQUEST = "GET /json HTTP/1.1\r\n" "Host: 0.0.0.0:9229\r\n\r\n"; @@ -933,4 +981,36 @@ TEST_F(InspectorSocketTest, HostIPNonRoutable) { expect_handshake_failure(); } +TEST_F(InspectorSocketTest, HostIPv6NonRoutable) { + const std::string INVALID_HOST_IP_REQUEST = "GET /json HTTP/1.1\r\n" + "Host: [::]:9229\r\n\r\n"; + send_in_chunks(INVALID_HOST_IP_REQUEST.c_str(), + INVALID_HOST_IP_REQUEST.length()); + expect_handshake_failure(); +} + +TEST_F(InspectorSocketTest, HostIPv6NonRoutableDual) { + const std::string INVALID_HOST_IP_REQUEST = "GET /json HTTP/1.1\r\n" + "Host: [::0.0.0.0]:9229\r\n\r\n"; + send_in_chunks(INVALID_HOST_IP_REQUEST.c_str(), + INVALID_HOST_IP_REQUEST.length()); + expect_handshake_failure(); +} + +TEST_F(InspectorSocketTest, HostIPv4InSquareBrackets) { + const std::string INVALID_HOST_IP_REQUEST = "GET /json HTTP/1.1\r\n" + "Host: [127.0.0.1]:9229\r\n\r\n"; + send_in_chunks(INVALID_HOST_IP_REQUEST.c_str(), + INVALID_HOST_IP_REQUEST.length()); + expect_handshake_failure(); +} + +TEST_F(InspectorSocketTest, HostIPv6InvalidAbbreviation) { + const std::string INVALID_HOST_IP_REQUEST = "GET /json HTTP/1.1\r\n" + "Host: [:::1]:9229\r\n\r\n"; + send_in_chunks(INVALID_HOST_IP_REQUEST.c_str(), + INVALID_HOST_IP_REQUEST.length()); + expect_handshake_failure(); +} + } // anonymous namespace