Validate ECH public names.
This was added in draft-11, which I'll update to more broadly in a
follow-up CL. This is an easily separable component: we don't want to
allow the DNS to arbitrarily insert strings in the ClientHello, so
invalid public names are rejected.
Unfortunately, we have a bit of a mess because DNS syntax does not
exclude IPv4 literals, yet everyone sticks DNS and IP literals in the
same string. The RFC3986 rules are alright, but don't match reality.
Reality is (probably?) the WHATWG rules, which are a mess.
The load-bearing bit of the spec is that, at certificate verification,
you should reject whatever strings your application refuses to represent
as a DNS name. I'll have Chromium call into its URL parser.
https://www.ietf.org/archive/id/draft-ietf-tls-esni-11.html#section-6.1.4.3-3
But there's still a bit at the validation step where clients "SHOULD"
run the IPv4 parser. In case downstream logic forgets, I've gone ahead
and implemented the WHATWG IPv4 parser.
https://www.ietf.org/archive/id/draft-ietf-tls-esni-11.html#section-4-6.6.1
Bug: 275
Change-Id: I15aa1ac0391df9c3859c44b8a259296e1907b7d4
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48085
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/ssl/encrypted_client_hello.cc b/ssl/encrypted_client_hello.cc
index b6a79a2..041c8de 100644
--- a/ssl/encrypted_client_hello.cc
+++ b/ssl/encrypted_client_hello.cc
@@ -17,6 +17,7 @@
#include <assert.h>
#include <string.h>
+#include <algorithm>
#include <utility>
#include <openssl/aead.h>
@@ -358,6 +359,121 @@
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;
+ }
+ *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 {
+ 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
+ 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) {
+ return false;
+ }
+ for (size_t i = 0; i < num_numbers - 1; i++) {
+ if (numbers[i] > 255) {
+ return false;
+ }
+ }
+ return num_numbers == 1 ||
+ numbers[num_numbers - 1] < 1u << (8 * (5 - num_numbers));
+}
+
+bool ssl_is_valid_ech_public_name(Span<const uint8_t> public_name) {
+ // See draft-ietf-tls-esni-11, Section 4 and RFC5890, 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()) {
+ return false;
+ }
+ while (!copy.empty()) {
+ // Find the next dot-separated component.
+ auto dot = std::find(copy.begin(), copy.end(), '.');
+ Span<const uint8_t> component;
+ if (dot == copy.end()) {
+ component = copy;
+ copy = 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()) {
+ // Trailing dots are not allowed.
+ return false;
+ }
+ }
+ // |component| must be a valid LDH label. Checking for empty components also
+ // rejects leading dots.
+ if (component.empty() || component.size() > 63 ||
+ component.front() == '-' || component.back() == '-') {
+ return false;
+ }
+ for (uint8_t c : component) {
+ if (!('a' <= c && c <= 'z') && !('A' <= c && c <= 'Z') &&
+ !('0' <= c && c <= '9') && c != '-') {
+ return false;
+ }
+ }
+ }
+
+ return !is_ipv4_address(public_name);
+}
+
static bool parse_ech_config(CBS *cbs, ECHConfig *out, bool *out_supported,
bool all_extensions_mandatory) {
uint16_t version;
@@ -400,7 +516,13 @@
return false;
}
- // TODO(https://crbug.com/boringssl/275): Check validity of |public_name|.
+ if (!ssl_is_valid_ech_public_name(public_name)) {
+ // TODO(https://crbug.com/boringssl/275): The draft says ECHConfigs with
+ // invalid public names should be ignored, but LDH syntax failures are
+ // unambiguously invalid.
+ *out_supported = false;
+ return true;
+ }
out->public_key = public_key;
out->public_name = public_name;
@@ -874,10 +996,9 @@
int SSL_marshal_ech_config(uint8_t **out, size_t *out_len, uint8_t config_id,
const EVP_HPKE_KEY *key, const char *public_name,
size_t max_name_len) {
- size_t public_name_len = strlen(public_name);
- if (public_name_len == 0) {
- // TODO(https://crbug.com/boringssl/275): Check |public_name| is a valid DNS
- // name.
+ Span<const uint8_t> public_name_u8 = MakeConstSpan(
+ reinterpret_cast<const uint8_t *>(public_name), strlen(public_name));
+ if (!ssl_is_valid_ech_public_name(public_name_u8)) {
OPENSSL_PUT_ERROR(SSL, SSL_R_INVALID_ECH_PUBLIC_NAME);
return 0;
}
@@ -905,8 +1026,7 @@
!CBB_add_u16(&child, EVP_HPKE_CHACHA20_POLY1305) ||
!CBB_add_u16(&contents, max_name_len) ||
!CBB_add_u16_length_prefixed(&contents, &child) ||
- !CBB_add_bytes(&child, reinterpret_cast<const uint8_t *>(public_name),
- public_name_len) ||
+ !CBB_add_bytes(&child, public_name_u8.data(), public_name_u8.size()) ||
// TODO(https://crbug.com/boringssl/275): Reserve some GREASE extensions
// and include some.
!CBB_add_u16(&contents, 0 /* no extensions */) ||
diff --git a/ssl/internal.h b/ssl/internal.h
index cb5bf75..ac01a61 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -1525,6 +1525,11 @@
const SSLTranscript &transcript,
Span<const uint8_t> server_hello);
+// ssl_is_valid_ech_public_name returns true if |public_name| is a valid ECH
+// public name and false otherwise. It is exported for testing.
+OPENSSL_EXPORT bool ssl_is_valid_ech_public_name(
+ Span<const uint8_t> public_name);
+
// ssl_is_valid_ech_config_list returns true if |ech_config_list| is a valid
// ECHConfigList structure and false otherwise.
bool ssl_is_valid_ech_config_list(Span<const uint8_t> ech_config_list);
diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc
index ceb52de..d6edcfe 100644
--- a/ssl/ssl_test.cc
+++ b/ssl/ssl_test.cc
@@ -1949,6 +1949,15 @@
EXPECT_FALSE(SSL_ECH_KEYS_add(keys.get(), /*is_retry_config=*/1,
ech_config.data(), ech_config.size(),
key.get()));
+
+ // Invalid public names are rejected.
+ ECHConfigParams invalid_public_name;
+ invalid_public_name.key = key.get();
+ invalid_public_name.public_name = "dns_names_have_no_underscores.example";
+ ASSERT_TRUE(MakeECHConfig(&ech_config, invalid_public_name));
+ EXPECT_FALSE(SSL_ECH_KEYS_add(keys.get(), /*is_retry_config=*/1,
+ ech_config.data(), ech_config.size(),
+ key.get()));
}
// Test that |SSL_get_client_random| reports the correct value on both client
@@ -2078,6 +2087,73 @@
}
}
+TEST(SSLTest, ECHPublicName) {
+ auto str_to_span = [](const char *str) -> Span<const uint8_t> {
+ return MakeConstSpan(reinterpret_cast<const uint8_t *>(str), strlen(str));
+ };
+
+ EXPECT_FALSE(ssl_is_valid_ech_public_name(str_to_span("")));
+ EXPECT_TRUE(ssl_is_valid_ech_public_name(str_to_span("example.com")));
+ EXPECT_FALSE(ssl_is_valid_ech_public_name(str_to_span(".example.com")));
+ EXPECT_FALSE(ssl_is_valid_ech_public_name(str_to_span("example.com.")));
+ EXPECT_FALSE(ssl_is_valid_ech_public_name(str_to_span("example..com")));
+ EXPECT_FALSE(ssl_is_valid_ech_public_name(str_to_span("www.-example.com")));
+ EXPECT_FALSE(ssl_is_valid_ech_public_name(str_to_span("www.example-.com")));
+ EXPECT_FALSE(
+ ssl_is_valid_ech_public_name(str_to_span("no_underscores.example")));
+ EXPECT_FALSE(ssl_is_valid_ech_public_name(
+ str_to_span("invalid_chars.\x01.example")));
+ EXPECT_FALSE(ssl_is_valid_ech_public_name(
+ str_to_span("invalid_chars.\xff.example")));
+ static const uint8_t kWithNUL[] = {'t', 'e', 's', 't', 0};
+ EXPECT_FALSE(ssl_is_valid_ech_public_name(kWithNUL));
+
+ // Test an LDH label with every character and the maximum length.
+ EXPECT_TRUE(ssl_is_valid_ech_public_name(str_to_span(
+ "abcdefhijklmnopqrstuvwxyz-ABCDEFGHIJKLMNOPQRSTUVWXYZ-0123456789")));
+ EXPECT_FALSE(ssl_is_valid_ech_public_name(str_to_span(
+ "abcdefhijklmnopqrstuvwxyz-ABCDEFGHIJKLMNOPQRSTUVWXYZ-01234567899")));
+
+ // Inputs that parse as IPv4 addresses 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_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")));
+}
+
#if defined(OPENSSL_THREADS)
// Test that the server ECH config can be swapped out while the |SSL_CTX| is
// in use on other threads. This test is intended to be run with TSan.
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index fded062..092b8f6 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -17807,6 +17807,45 @@
expectedError: ":WRONG_VERSION_ON_EARLY_DATA:",
})
}
+
+ // Test that the client ignores ECHConfigs with invalid public names.
+ invalidPublicName := generateServerECHConfig(&ECHConfig{PublicName: "dns_names_have_no_underscores.example"})
+ testCases = append(testCases, testCase{
+ testType: clientTest,
+ protocol: protocol,
+ name: prefix + "ECH-Client-SkipInvalidPublicName",
+ config: Config{
+ Bugs: ProtocolBugs{
+ // No ECHConfigs are supported, so the client should fall
+ // back to cleartext.
+ ExpectNoClientECH: true,
+ ExpectServerName: "secret.example",
+ },
+ },
+ flags: []string{
+ "-ech-config-list", base64.StdEncoding.EncodeToString(CreateECHConfigList(invalidPublicName.ECHConfig.Raw)),
+ "-host-name", "secret.example",
+ },
+ })
+ testCases = append(testCases, testCase{
+ testType: clientTest,
+ protocol: protocol,
+ name: prefix + "ECH-Client-SkipInvalidPublicName-2",
+ config: Config{
+ // The client should skip |invalidPublicName| and use |echConfig|.
+ ServerECHConfigs: []ServerECHConfig{echConfig},
+ Bugs: ProtocolBugs{
+ ExpectOuterServerName: "public.example",
+ ExpectServerName: "secret.example",
+ },
+ },
+ flags: []string{
+ "-ech-config-list", base64.StdEncoding.EncodeToString(CreateECHConfigList(invalidPublicName.ECHConfig.Raw, echConfig.ECHConfig.Raw)),
+ "-host-name", "secret.example",
+ "-expect-ech-accept",
+ },
+ expectations: connectionExpectations{echAccepted: true},
+ })
}
}
