Require on-the-wire SNI to pass IsValidSNI check
This requirement existed in QUIC Crypto; it should exist when we run QUIC
with TLS.
Restrict sni in ietf quic draft versions. protected by reloadable flag quic_tls_enforce_valid_sni.
PiperOrigin-RevId: 310054163
Change-Id: I9ffdea55c350e9c1592a71debb3fbb271eca7750
diff --git a/quic/core/tls_client_handshaker.cc b/quic/core/tls_client_handshaker.cc
index 324c378..09ab71e 100644
--- a/quic/core/tls_client_handshaker.cc
+++ b/quic/core/tls_client_handshaker.cc
@@ -12,6 +12,7 @@
#include "net/third_party/quiche/src/quic/core/crypto/quic_encrypter.h"
#include "net/third_party/quiche/src/quic/core/crypto/transport_parameters.h"
#include "net/third_party/quiche/src/quic/core/quic_session.h"
+#include "net/third_party/quiche/src/quic/platform/api/quic_hostname_utils.h"
#include "net/third_party/quiche/src/common/platform/api/quiche_string_piece.h"
#include "net/third_party/quiche/src/common/platform/api/quiche_text_utils.h"
@@ -87,7 +88,14 @@
// Set the SNI to send, if any.
SSL_set_connect_state(ssl());
+ if (QUIC_DLOG_INFO_IS_ON() &&
+ !QuicHostnameUtils::IsValidSNI(server_id_.host())) {
+ QUIC_DLOG(INFO) << "Client configured with invalid hostname \""
+ << server_id_.host() << "\", not sending as SNI";
+ }
if (!server_id_.host().empty() &&
+ (QuicHostnameUtils::IsValidSNI(server_id_.host()) ||
+ allow_invalid_sni_for_tests_) &&
SSL_set_tlsext_host_name(ssl(), server_id_.host().c_str()) != 1) {
return false;
}
diff --git a/quic/core/tls_client_handshaker.h b/quic/core/tls_client_handshaker.h
index fdd68c2..cc314b7 100644
--- a/quic/core/tls_client_handshaker.h
+++ b/quic/core/tls_client_handshaker.h
@@ -75,6 +75,7 @@
std::unique_ptr<ApplicationState> application_state) override;
void AllowEmptyAlpnForTests() { allow_empty_alpn_for_tests_ = true; }
+ void AllowInvalidSNIForTests() { allow_invalid_sni_for_tests_ = true; }
protected:
const TlsConnection* tls_connection() const override {
@@ -169,6 +170,7 @@
crypto_negotiated_params_;
bool allow_empty_alpn_for_tests_ = false;
+ bool allow_invalid_sni_for_tests_ = false;
const bool has_application_state_;
diff --git a/quic/core/tls_client_handshaker_test.cc b/quic/core/tls_client_handshaker_test.cc
index fe757b4..2629bd4 100644
--- a/quic/core/tls_client_handshaker_test.cc
+++ b/quic/core/tls_client_handshaker_test.cc
@@ -392,6 +392,26 @@
EXPECT_TRUE(server_stream()->encryption_established());
}
+TEST_P(TlsClientHandshakerTest, InvalidSNI) {
+ // Test that a client will skip sending SNI if configured to send an invalid
+ // hostname. In this case, the inclusion of '!' is invalid.
+ server_id_ = QuicServerId("invalid!.example.com", 443);
+ crypto_config_.reset(new QuicCryptoClientConfig(
+ std::make_unique<FakeProofVerifier>(), nullptr));
+ CreateConnection();
+ InitializeFakeServer();
+
+ stream()->CryptoConnect();
+ crypto_test_utils::CommunicateHandshakeMessages(
+ connection_, stream(), server_connection_, server_stream());
+
+ EXPECT_EQ(PROTOCOL_TLS1_3, stream()->handshake_protocol());
+ EXPECT_TRUE(stream()->encryption_established());
+ EXPECT_TRUE(stream()->one_rtt_keys_available());
+
+ EXPECT_EQ(server_stream()->crypto_negotiated_params().sni, "");
+}
+
} // namespace
} // namespace test
} // namespace quic
diff --git a/quic/core/tls_server_handshaker.cc b/quic/core/tls_server_handshaker.cc
index 890abae..54a066f 100644
--- a/quic/core/tls_server_handshaker.cc
+++ b/quic/core/tls_server_handshaker.cc
@@ -484,6 +484,15 @@
hostname_ = hostname;
crypto_negotiated_params_->sni =
QuicHostnameUtils::NormalizeHostname(hostname_);
+ if (GetQuicReloadableFlag(quic_tls_enforce_valid_sni)) {
+ QUIC_RELOADABLE_FLAG_COUNT(quic_tls_enforce_valid_sni);
+ if (!QuicHostnameUtils::IsValidSNI(hostname_)) {
+ // TODO(b/151676147): Include this error string in the CONNECTION_CLOSE
+ // frame.
+ QUIC_LOG(ERROR) << "Invalid SNI provided: \"" << hostname_ << "\"";
+ return SSL_TLSEXT_ERR_ALERT_FATAL;
+ }
+ }
} else {
QUIC_LOG(INFO) << "No hostname indicated in SNI";
}
diff --git a/quic/core/tls_server_handshaker_test.cc b/quic/core/tls_server_handshaker_test.cc
index 28f13c6..89fb325 100644
--- a/quic/core/tls_server_handshaker_test.cc
+++ b/quic/core/tls_server_handshaker_test.cc
@@ -356,6 +356,20 @@
ExpectHandshakeSuccessful();
}
+TEST_F(TlsServerHandshakerTest, RejectInvalidSNI) {
+ SetQuicReloadableFlag(quic_tls_enforce_valid_sni, true);
+ server_id_ = QuicServerId("invalid!.example.com", kServerPort, false);
+ InitializeFakeClient();
+ static_cast<TlsClientHandshaker*>(
+ QuicCryptoClientStreamPeer::GetHandshaker(client_stream()))
+ ->AllowInvalidSNIForTests();
+
+ // Run the handshake and expect it to fail.
+ AdvanceHandshakeWithFakeClient();
+ EXPECT_FALSE(server_stream()->encryption_established());
+ EXPECT_FALSE(server_stream()->one_rtt_keys_available());
+}
+
TEST_F(TlsServerHandshakerTest, Resumption) {
// Do the first handshake
InitializeFakeClient();
