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

quic/core/tls_client_handshaker.cc

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;
   }

quic/core/tls_client_handshaker.h

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_;
 

quic/core/tls_client_handshaker_test.cc

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

quic/core/tls_server_handshaker.cc

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";
   }

quic/core/tls_server_handshaker_test.cc

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();