Switch to the new, simpler WHATWG URL formulation. In light of https://groups.google.com/a/chromium.org/g/blink-dev/c/7QN5nxjwIfM/m/q9dw9MxoAwAJ, the WHATWG URL parser is now more restrictive about which strings are valid DNS names. The final component may not be numeric. Align the ECHConfig validator with this. Bug: 275 Change-Id: Iea2a3d9a7fee5bffc683da99274c54d60379be9e Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49225 Reviewed-by: Adam Langley <agl@google.com>
diff --git a/ssl/encrypted_client_hello.cc b/ssl/encrypted_client_hello.cc index 5192cc6..64fee3d 100644 --- a/ssl/encrypted_client_hello.cc +++ b/ssl/encrypted_client_hello.cc
@@ -310,103 +310,53 @@ return true; } -static bool parse_ipv4_number(Span<const uint8_t> in, uint32_t *out) { - // See https://url.spec.whatwg.org/#ipv4-number-parser. - uint32_t base = 10; - if (in.size() >= 2 && in[0] == '0' && (in[1] == 'x' || in[1] == 'X')) { - in = in.subspan(2); - base = 16; - } else if (in.size() >= 1 && in[0] == '0') { - in = in.subspan(1); - base = 8; +static bool is_hex_component(Span<const uint8_t> in) { + if (in.size() < 2 || in[0] != '0' || (in[1] != 'x' && in[1] != 'X')) { + return false; } - *out = 0; - for (uint8_t c : in) { - uint32_t d; - if ('0' <= c && c <= '9') { - d = c - '0'; - } else if ('a' <= c && c <= 'f') { - d = c - 'a' + 10; - } else if ('A' <= c && c <= 'F') { - d = c - 'A' + 10; - } else { + for (uint8_t b : in.subspan(2)) { + if (!('0' <= b && b <= '9') && !('a' <= b && b <= 'f') && + !('A' <= b && b <= 'F')) { return false; } - if (d >= base || - *out > UINT32_MAX / base) { - return false; - } - *out *= base; - if (*out > UINT32_MAX - d) { - return false; - } - *out += d; } return true; } -static bool is_ipv4_address(Span<const uint8_t> in) { - // See https://url.spec.whatwg.org/#concept-ipv4-parser - // - // TODO(https://crbug.com/boringssl/275): Revise this, and maybe the spec, per - // https://groups.google.com/a/chromium.org/g/blink-dev/c/7QN5nxjwIfM/m/q9dw9MxoAwAJ - uint32_t numbers[4]; - size_t num_numbers = 0; - while (!in.empty()) { - if (num_numbers == 4) { - // Too many components. - return false; - } - // Find the next dot-separated component. - auto dot = std::find(in.begin(), in.end(), '.'); - if (dot == in.begin()) { - // Empty components are not allowed. - return false; - } - Span<const uint8_t> component; - if (dot == in.end()) { - component = in; - in = Span<const uint8_t>(); - } else { - component = in.subspan(0, dot - in.begin()); - in = in.subspan(dot - in.begin() + 1); // Skip the dot. - } - if (!parse_ipv4_number(component, &numbers[num_numbers])) { - return false; - } - num_numbers++; - } - if (num_numbers == 0) { +static bool is_decimal_component(Span<const uint8_t> in) { + if (in.empty()) { return false; } - for (size_t i = 0; i < num_numbers - 1; i++) { - if (numbers[i] > 255) { + for (uint8_t b : in) { + if (!('0' <= b && b <= '9')) { return false; } } - return num_numbers == 1 || - numbers[num_numbers - 1] < 1u << (8 * (5 - num_numbers)); + return true; } bool ssl_is_valid_ech_public_name(Span<const uint8_t> public_name) { // See draft-ietf-tls-esni-13, Section 4 and RFC 5890, Section 2.3.1. The // public name must be a dot-separated sequence of LDH labels and not begin or // end with a dot. - auto copy = public_name; - if (copy.empty()) { + auto remaining = public_name; + if (remaining.empty()) { return false; } - while (!copy.empty()) { + Span<const uint8_t> last; + while (!remaining.empty()) { // Find the next dot-separated component. - auto dot = std::find(copy.begin(), copy.end(), '.'); + auto dot = std::find(remaining.begin(), remaining.end(), '.'); Span<const uint8_t> component; - if (dot == copy.end()) { - component = copy; - copy = Span<const uint8_t>(); + if (dot == remaining.end()) { + component = remaining; + last = component; + remaining = Span<const uint8_t>(); } else { - component = copy.subspan(0, dot - copy.begin()); - copy = copy.subspan(dot - copy.begin() + 1); // Skip the dot. - if (copy.empty()) { + component = remaining.subspan(0, dot - remaining.begin()); + // Skip the dot. + remaining = remaining.subspan(dot - remaining.begin() + 1); + if (remaining.empty()) { // Trailing dots are not allowed. return false; } @@ -425,7 +375,15 @@ } } - return !is_ipv4_address(public_name); + // The WHATWG URL parser additionally does not allow any DNS names that end in + // a numeric component. See: + // https://url.spec.whatwg.org/#concept-host-parser + // https://url.spec.whatwg.org/#ends-in-a-number-checker + // + // The WHATWG parser is formulated in terms of parsing decimal, octal, and + // hex, along with a separate ASCII digits check. The ASCII digits check + // subsumes the decimal and octal check, so we only need to check two cases. + return !is_hex_component(last) && !is_decimal_component(last); } static bool parse_ech_config(CBS *cbs, ECHConfig *out, bool *out_supported,
diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc index 60d820b..7ab5054 100644 --- a/ssl/ssl_test.cc +++ b/ssl/ssl_test.cc
@@ -2119,44 +2119,39 @@ EXPECT_FALSE(ssl_is_valid_ech_public_name(str_to_span( "abcdefhijklmnopqrstuvwxyz-ABCDEFGHIJKLMNOPQRSTUVWXYZ-01234567899"))); - // Inputs that parse as IPv4 addresses are rejected. + // Inputs with trailing numeric components are rejected. EXPECT_FALSE(ssl_is_valid_ech_public_name(str_to_span("127.0.0.1"))); - EXPECT_FALSE(ssl_is_valid_ech_public_name(str_to_span("0177.0.0.01"))); - EXPECT_FALSE( - ssl_is_valid_ech_public_name(str_to_span("0x7f.0x.0x.0x00000001"))); - EXPECT_FALSE( - ssl_is_valid_ech_public_name(str_to_span("0XAB.0XCD.0XEF.0X01"))); - EXPECT_FALSE(ssl_is_valid_ech_public_name(str_to_span("0.0.0.0"))); - EXPECT_FALSE(ssl_is_valid_ech_public_name(str_to_span("255.255.255.255"))); - // Out-of-bounds or overflowing components are not IP addresses. - EXPECT_TRUE(ssl_is_valid_ech_public_name(str_to_span("256.255.255.255"))); - EXPECT_TRUE(ssl_is_valid_ech_public_name(str_to_span("255.0x100.255.255"))); - EXPECT_TRUE(ssl_is_valid_ech_public_name(str_to_span("255.255.255.0400"))); + EXPECT_FALSE(ssl_is_valid_ech_public_name(str_to_span("example.1"))); + EXPECT_FALSE(ssl_is_valid_ech_public_name(str_to_span("example.01"))); + EXPECT_FALSE(ssl_is_valid_ech_public_name(str_to_span("example.0x01"))); + EXPECT_FALSE(ssl_is_valid_ech_public_name(str_to_span("example.0X01"))); + // Leading zeros and values that overflow |uint32_t| are still rejected. + EXPECT_FALSE(ssl_is_valid_ech_public_name( + str_to_span("example.123456789000000000000000"))); + EXPECT_FALSE(ssl_is_valid_ech_public_name( + str_to_span("example.012345678900000000000000"))); + EXPECT_FALSE(ssl_is_valid_ech_public_name( + str_to_span("example.0x123456789abcdefABCDEF0"))); + EXPECT_FALSE(ssl_is_valid_ech_public_name( + str_to_span("example.0x0123456789abcdefABCDEF"))); + // Adding a non-digit or non-hex character makes it a valid DNS name again. + // Single-component numbers are rejected. EXPECT_TRUE(ssl_is_valid_ech_public_name( - str_to_span("255.255.255.0x100000000"))); - // Invalid characters for the base are not IP addresses. - EXPECT_TRUE(ssl_is_valid_ech_public_name(str_to_span("12a.0.0.1"))); - EXPECT_TRUE(ssl_is_valid_ech_public_name(str_to_span("0xg.0.0.1"))); - EXPECT_TRUE(ssl_is_valid_ech_public_name(str_to_span("08.0.0.1"))); - // Trailing components can be merged into a single component. - EXPECT_FALSE(ssl_is_valid_ech_public_name(str_to_span("127.0.1"))); - EXPECT_FALSE(ssl_is_valid_ech_public_name(str_to_span("127.1"))); - EXPECT_FALSE(ssl_is_valid_ech_public_name(str_to_span("2130706433"))); - EXPECT_FALSE(ssl_is_valid_ech_public_name(str_to_span("0x7f000001"))); - // Merged components must respect their limits. - EXPECT_FALSE(ssl_is_valid_ech_public_name(str_to_span("0.0.0.0xff"))); - EXPECT_FALSE(ssl_is_valid_ech_public_name(str_to_span("0.0.0xffff"))); - EXPECT_FALSE(ssl_is_valid_ech_public_name(str_to_span("0.0xffffff"))); - EXPECT_FALSE(ssl_is_valid_ech_public_name(str_to_span("0xffffffff"))); - EXPECT_TRUE(ssl_is_valid_ech_public_name(str_to_span("0.0.0.0x100"))); - EXPECT_TRUE(ssl_is_valid_ech_public_name(str_to_span("0.0.0x10000"))); - EXPECT_TRUE(ssl_is_valid_ech_public_name(str_to_span("0.0x1000000"))); - EXPECT_TRUE(ssl_is_valid_ech_public_name(str_to_span("0x100000000"))); - // Correctly handle overflow in decimal and octal. - EXPECT_FALSE(ssl_is_valid_ech_public_name(str_to_span("037777777777"))); - EXPECT_TRUE(ssl_is_valid_ech_public_name(str_to_span("040000000000"))); - EXPECT_FALSE(ssl_is_valid_ech_public_name(str_to_span("4294967295"))); - EXPECT_TRUE(ssl_is_valid_ech_public_name(str_to_span("4294967296"))); + str_to_span("example.1234567890a"))); + EXPECT_TRUE(ssl_is_valid_ech_public_name( + str_to_span("example.01234567890a"))); + EXPECT_TRUE(ssl_is_valid_ech_public_name( + str_to_span("example.0x123456789abcdefg"))); + EXPECT_FALSE(ssl_is_valid_ech_public_name(str_to_span("1"))); + EXPECT_FALSE(ssl_is_valid_ech_public_name(str_to_span("01"))); + EXPECT_FALSE(ssl_is_valid_ech_public_name(str_to_span("0x01"))); + EXPECT_FALSE(ssl_is_valid_ech_public_name(str_to_span("0X01"))); + // Numbers with trailing dots are rejected. (They are already rejected by the + // LDH label rules, but the WHATWG URL parser additionally rejects them.) + EXPECT_FALSE(ssl_is_valid_ech_public_name(str_to_span("1."))); + EXPECT_FALSE(ssl_is_valid_ech_public_name(str_to_span("01."))); + EXPECT_FALSE(ssl_is_valid_ech_public_name(str_to_span("0x01."))); + EXPECT_FALSE(ssl_is_valid_ech_public_name(str_to_span("0X01."))); } // When using the built-in verifier, test that |SSL_get0_ech_name_override| is