Stop padding QUIC_CRYPTO CHLOs
Before 2016, the QUIC code relied on the size of CHLO messages to dictate maximum response sizes (to avoid amplification attacks). However since cl/129160718 and cl/133633175 landed in 2016, we do not rely on that anymore. Instead we use the size of the packet that contained the CHLO. This is also what is mandated by the IETF QUIC spec. This CL removes unnecessary padding from CHLO messages which could lead to more efficient handshakes. It also adds a check in QuicDispatcher to validate the length of Q043 CHLO packets since the length check only applied to subsequent versions.
Stop padding chlo, protected by gfe2_reloadable_flag_quic_dont_pad_chlo
PiperOrigin-RevId: 317320920
Change-Id: I17d60625886648105905a9e811c9b1def5856fd5
diff --git a/quic/core/crypto/crypto_server_test.cc b/quic/core/crypto/crypto_server_test.cc
index ee9dc53..58d39b8 100644
--- a/quic/core/crypto/crypto_server_test.cc
+++ b/quic/core/crypto/crypto_server_test.cc
@@ -508,6 +508,12 @@
}
TEST_P(CryptoServerTest, TooSmall) {
+ if (GetQuicReloadableFlag(quic_dont_pad_chlo)) {
+ // This test validates that non-padded CHLOs are rejected, it cannot pass
+ // when the padding is no longer required.
+ // TODO(dschinazi) remove this test when we deprecate quic_dont_pad_chlo.
+ return;
+ }
ShouldFailMentioning(
"too small", crypto_test_utils::CreateCHLO(
{{"PDMD", "X509"}, {"VER\0", client_version_string_}}));
diff --git a/quic/core/crypto/quic_crypto_client_config.cc b/quic/core/crypto/quic_crypto_client_config.cc
index fd1d41c..09c259e 100644
--- a/quic/core/crypto/quic_crypto_client_config.cc
+++ b/quic/core/crypto/quic_crypto_client_config.cc
@@ -449,7 +449,7 @@
CryptoHandshakeMessage* out) const {
out->set_tag(kCHLO);
// TODO(rch): Remove this when we remove quic_use_chlo_packet_size flag.
- if (pad_inchoate_hello_) {
+ if (pad_inchoate_hello_ && !GetQuicReloadableFlag(quic_dont_pad_chlo)) {
out->set_minimum_size(kClientHelloMinimumSize);
} else {
out->set_minimum_size(1);
@@ -538,7 +538,7 @@
FillInchoateClientHello(server_id, preferred_version, cached, rand,
/* demand_x509_proof= */ true, out_params, out);
- if (pad_full_hello_) {
+ if (pad_full_hello_ && !GetQuicReloadableFlag(quic_dont_pad_chlo)) {
out->set_minimum_size(kClientHelloMinimumSize);
} else {
out->set_minimum_size(1);
diff --git a/quic/core/crypto/quic_crypto_client_config_test.cc b/quic/core/crypto/quic_crypto_client_config_test.cc
index 4b6dcd7..5eaa31f 100644
--- a/quic/core/crypto/quic_crypto_client_config_test.cc
+++ b/quic/core/crypto/quic_crypto_client_config_test.cc
@@ -200,7 +200,11 @@
EXPECT_TRUE(msg.GetStringPiece(kALPN, &alpn));
EXPECT_EQ("hq", alpn);
- EXPECT_EQ(msg.minimum_size(), 1024u);
+ if (GetQuicReloadableFlag(quic_dont_pad_chlo)) {
+ EXPECT_EQ(msg.minimum_size(), 1u);
+ } else {
+ EXPECT_EQ(msg.minimum_size(), 1024u);
+ }
}
TEST_F(QuicCryptoClientConfigTest, InchoateChloIsNotPadded) {
diff --git a/quic/core/crypto/quic_crypto_server_config.cc b/quic/core/crypto/quic_crypto_server_config.cc
index 9cb089b..c07b8d3 100644
--- a/quic/core/crypto/quic_crypto_server_config.cc
+++ b/quic/core/crypto/quic_crypto_server_config.cc
@@ -1211,10 +1211,14 @@
const CryptoHandshakeMessage& client_hello = client_hello_state->client_hello;
ClientHelloInfo* info = &(client_hello_state->info);
- if (validate_chlo_size_ && client_hello.size() < kClientHelloMinimumSize) {
- helper.ValidationComplete(QUIC_CRYPTO_INVALID_VALUE_LENGTH,
- "Client hello too small", nullptr);
- return;
+ if (GetQuicReloadableFlag(quic_dont_pad_chlo)) {
+ QUIC_RELOADABLE_FLAG_COUNT_N(quic_dont_pad_chlo, 1, 2);
+ } else {
+ if (validate_chlo_size_ && client_hello.size() < kClientHelloMinimumSize) {
+ helper.ValidationComplete(QUIC_CRYPTO_INVALID_VALUE_LENGTH,
+ "Client hello too small", nullptr);
+ return;
+ }
}
if (client_hello.GetStringPiece(kSNI, &info->sni) &&
diff --git a/quic/core/quic_dispatcher.cc b/quic/core/quic_dispatcher.cc
index d26431d..ca3f50d 100644
--- a/quic/core/quic_dispatcher.cc
+++ b/quic/core/quic_dispatcher.cc
@@ -545,6 +545,21 @@
BufferEarlyPacket(*packet_info);
break;
}
+ if (GetQuicReloadableFlag(quic_dont_pad_chlo)) {
+ QUIC_RELOADABLE_FLAG_COUNT_N(quic_dont_pad_chlo, 2, 2);
+ // We only apply this check for versions that do not use the IETF
+ // invariant header because those versions are already checked in
+ // QuicDispatcher::MaybeDispatchPacket.
+ if (packet_info->version_flag &&
+ !packet_info->version.HasIetfInvariantHeader() &&
+ crypto_config()->validate_chlo_size() &&
+ packet_info->packet.length() < kMinClientInitialPacketLength) {
+ QUIC_DVLOG(1) << "Dropping CHLO packet which is too short, length: "
+ << packet_info->packet.length();
+ QUIC_CODE_COUNT(quic_drop_small_chlo_packets);
+ break;
+ }
+ }
ProcessChlo({alpn_extractor.ConsumeAlpn()}, packet_info);
} break;
case kFateTimeWait:
diff --git a/quic/core/quic_dispatcher_test.cc b/quic/core/quic_dispatcher_test.cc
index 12008c4..fc8fd4a 100644
--- a/quic/core/quic_dispatcher_test.cc
+++ b/quic/core/quic_dispatcher_test.cc
@@ -1287,8 +1287,12 @@
}
TEST_P(QuicDispatcherTestAllVersions, DoNotProcessSmallPacket) {
- if (!version_.HasIetfInvariantHeader()) {
- // We only drop small packets when using IETF_QUIC_LONG_HEADER_PACKET.
+ if (!version_.HasIetfInvariantHeader() &&
+ !GetQuicReloadableFlag(quic_dont_pad_chlo)) {
+ // When quic_dont_pad_chlo is false, we only drop small packets when using
+ // IETF_QUIC_LONG_HEADER_PACKET. When quic_dont_pad_chlo is true, we drop
+ // small packets for all versions.
+ // TODO(dschinazi) remove this early return when we deprecate the flag.
return;
}
CreateTimeWaitListManager();
