QuicDispatcher asks the ConnectionIdGenerator how long short-header connection IDs should be.
Tested against all 4 permutations of quic_connection_uses_abstract_connection_id_generator, and quic_ask_for_short_header_connection_id_length.
(The test passes by expecting connection failure when these flags are not in alignment -- the point of the last flag is to fix the bug.)
Protected by FLAGS_quic_reloadable_flag_quic_ask_for_short_header_connection_id_length.
PiperOrigin-RevId: 481258788
diff --git a/quiche/quic/core/quic_buffered_packet_store.cc b/quiche/quic/core/quic_buffered_packet_store.cc
index a84a293..4c729b6 100644
--- a/quiche/quic/core/quic_buffered_packet_store.cc
+++ b/quiche/quic/core/quic_buffered_packet_store.cc
@@ -194,12 +194,19 @@
absl::optional<absl::string_view> unused_retry_token;
std::string unused_detailed_error;
+ // We don't need to pass |generator| because we already got the correct
+ // connection ID length when we buffered the packet and indexed by
+ // connection ID.
QuicErrorCode error_code = QuicFramer::ParsePublicHeaderDispatcher(
- *packet.packet, kQuicDefaultConnectionIdLength, &unused_format,
- &long_packet_type, &unused_version_flag, &unused_use_length_prefix,
- &unused_version_label, &unused_parsed_version,
- &unused_destination_connection_id, &unused_source_connection_id,
- &unused_retry_token, &unused_detailed_error);
+ *packet.packet,
+ GetQuicReloadableFlag(quic_ask_for_short_header_connection_id_length)
+ ? connection_id.length()
+ : kQuicDefaultConnectionIdLength,
+ &unused_format, &long_packet_type, &unused_version_flag,
+ &unused_use_length_prefix, &unused_version_label,
+ &unused_parsed_version, &unused_destination_connection_id,
+ &unused_source_connection_id, &unused_retry_token,
+ &unused_detailed_error);
if (error_code == QUIC_NO_ERROR && long_packet_type == INITIAL) {
initial_packets.push_back(std::move(packet));
@@ -208,8 +215,8 @@
}
}
- initial_packets.splice(initial_packets.end(), other_packets);
- packets_to_deliver.buffered_packets = std::move(initial_packets);
+ initial_packets.splice(initial_packets.end(), other_packets);
+ packets_to_deliver.buffered_packets = std::move(initial_packets);
}
return packets_to_deliver;
}
diff --git a/quiche/quic/core/quic_dispatcher.cc b/quiche/quic/core/quic_dispatcher.cc
index da0af71..0ae3a64 100644
--- a/quiche/quic/core/quic_dispatcher.cc
+++ b/quiche/quic/core/quic_dispatcher.cc
@@ -41,6 +41,13 @@
// Minimal INITIAL packet length sent by clients is 1200.
const QuicPacketLength kMinClientInitialPacketLength = 1200;
+bool VariableShortHeaderConnectionIdLengths() {
+ return GetQuicReloadableFlag(
+ quic_ask_for_short_header_connection_id_length) &&
+ GetQuicReloadableFlag(
+ quic_connection_uses_abstract_connection_id_generator);
+}
+
// An alarm that informs the QuicDispatcher to delete old sessions.
class DeleteSessionsAlarm : public QuicAlarm::DelegateWithoutContext {
public:
@@ -305,13 +312,24 @@
absl::string_view(packet.data(), packet.length()));
ReceivedPacketInfo packet_info(self_address, peer_address, packet);
std::string detailed_error;
- const QuicErrorCode error = QuicFramer::ParsePublicHeaderDispatcher(
- packet, expected_server_connection_id_length_, &packet_info.form,
- &packet_info.long_packet_type, &packet_info.version_flag,
- &packet_info.use_length_prefix, &packet_info.version_label,
- &packet_info.version, &packet_info.destination_connection_id,
- &packet_info.source_connection_id, &packet_info.retry_token,
- &detailed_error);
+ QuicErrorCode error;
+ if (GetQuicReloadableFlag(quic_ask_for_short_header_connection_id_length)) {
+ error = QuicFramer::ParsePublicHeaderDispatcherShortHeaderLengthUnknown(
+ packet, &packet_info.form, &packet_info.long_packet_type,
+ &packet_info.version_flag, &packet_info.use_length_prefix,
+ &packet_info.version_label, &packet_info.version,
+ &packet_info.destination_connection_id,
+ &packet_info.source_connection_id, &packet_info.retry_token,
+ &detailed_error, connection_id_generator_);
+ } else {
+ error = QuicFramer::ParsePublicHeaderDispatcher(
+ packet, expected_server_connection_id_length_, &packet_info.form,
+ &packet_info.long_packet_type, &packet_info.version_flag,
+ &packet_info.use_length_prefix, &packet_info.version_label,
+ &packet_info.version, &packet_info.destination_connection_id,
+ &packet_info.source_connection_id, &packet_info.retry_token,
+ &detailed_error);
+ }
if (error != QUIC_NO_ERROR) {
// Packet has framing error.
SetLastError(error);
@@ -347,7 +365,16 @@
}
}
- if (should_update_expected_server_connection_id_length_) {
+ // Before introducing the flag, it was impossible for a short header to
+ // update |expected_server_connection_id_length_|.
+ QUIC_BUG_IF(
+ quic_bug_480483284_01,
+ !GetQuicReloadableFlag(quic_ask_for_short_header_connection_id_length) &&
+ !packet_info.version_flag &&
+ packet_info.destination_connection_id.length() !=
+ expected_server_connection_id_length_);
+ if (should_update_expected_server_connection_id_length_ &&
+ packet_info.version_flag) {
expected_server_connection_id_length_ =
packet_info.destination_connection_id.length();
}
@@ -356,6 +383,38 @@
// Packet has been dropped or successfully dispatched, stop processing.
return;
}
+ // The framer might have extracted the incorrect Connection ID length from a
+ // short header. |packet| could be gQUIC; if Q043, the connection ID has been
+ // parsed correctly thanks to the fixed bit. If a Q046 or Q050 short header,
+ // the dispatcher might have assumed it was a long connection ID when (because
+ // it was gQUIC) it actually issued or kept an 8-byte ID. The other case is
+ // where NEW_CONNECTION_IDs are not using the generator, and the dispatcher
+ // is, due to flag misconfiguration.
+ if (!packet_info.version_flag &&
+ GetQuicReloadableFlag(quic_ask_for_short_header_connection_id_length) &&
+ (IsSupportedVersion(ParsedQuicVersion::Q046()) ||
+ IsSupportedVersion(ParsedQuicVersion::Q050()) ||
+ !GetQuicReloadableFlag(
+ quic_connection_uses_abstract_connection_id_generator))) {
+ ReceivedPacketInfo gquic_packet_info(self_address, peer_address, packet);
+ // Try again without asking |connection_id_generator_| for the length.
+ const QuicErrorCode gquic_error = QuicFramer::ParsePublicHeaderDispatcher(
+ packet, expected_server_connection_id_length_, &gquic_packet_info.form,
+ &gquic_packet_info.long_packet_type, &gquic_packet_info.version_flag,
+ &gquic_packet_info.use_length_prefix, &gquic_packet_info.version_label,
+ &gquic_packet_info.version,
+ &gquic_packet_info.destination_connection_id,
+ &gquic_packet_info.source_connection_id, &gquic_packet_info.retry_token,
+ &detailed_error);
+ if (gquic_error == QUIC_NO_ERROR) {
+ if (MaybeDispatchPacket(gquic_packet_info)) {
+ return;
+ }
+ } else {
+ QUICHE_VLOG(1) << "Tried to parse short header as gQUIC packet: "
+ << detailed_error;
+ }
+ }
ProcessHeader(&packet_info);
}
@@ -416,15 +475,20 @@
// than 64 bits and smaller than what we expect. Unless the version is
// unknown, in which case we allow short connection IDs for version
// negotiation because that version could allow those.
+ uint8_t expected_connection_id_length =
+ VariableShortHeaderConnectionIdLengths()
+ ? connection_id_generator_.ConnectionIdLength(
+ static_cast<uint8_t>(*server_connection_id.data()))
+ : expected_server_connection_id_length_;
if (packet_info.version_flag && packet_info.version.IsKnown() &&
server_connection_id.length() < kQuicMinimumInitialConnectionIdLength &&
- server_connection_id.length() < expected_server_connection_id_length_ &&
+ server_connection_id.length() < expected_connection_id_length &&
!allow_short_initial_server_connection_ids_) {
QUICHE_DCHECK(packet_info.version_flag);
QUICHE_DCHECK(packet_info.version.AllowsVariableLengthConnectionIds());
QUIC_DLOG(INFO) << "Packet with short destination connection ID "
<< server_connection_id << " expected "
- << static_cast<int>(expected_server_connection_id_length_);
+ << static_cast<int>(expected_connection_id_length);
// Drop the packet silently.
QUIC_CODE_COUNT(quic_dropped_invalid_small_initial_connection_id);
return true;
@@ -1294,8 +1358,13 @@
return;
}
} else {
+ uint8_t min_connection_id_length =
+ VariableShortHeaderConnectionIdLengths()
+ ? connection_id_generator_.ConnectionIdLength(static_cast<uint8_t>(
+ *packet_info.destination_connection_id.data()))
+ : expected_server_connection_id_length_;
const size_t MinValidPacketLength =
- kPacketHeaderTypeSize + expected_server_connection_id_length_ +
+ kPacketHeaderTypeSize + min_connection_id_length +
PACKET_1BYTE_PACKET_NUMBER + /*payload size=*/1 + /*tag size=*/12;
if (packet_info.packet.length() < MinValidPacketLength) {
// The packet size is too small.
diff --git a/quiche/quic/core/quic_dispatcher_test.cc b/quiche/quic/core/quic_dispatcher_test.cc
index ffa0b22..ce5e23f 100644
--- a/quiche/quic/core/quic_dispatcher_test.cc
+++ b/quiche/quic/core/quic_dispatcher_test.cc
@@ -584,6 +584,58 @@
ProcessFirstFlight(client_address, TestConnectionId(1));
}
+TEST_P(QuicDispatcherTestAllVersions, VariableServerConnectionIdLength) {
+ QuicConnectionId old_id = TestConnectionId(1);
+ // Return a connection ID that is not expected_server_connection_id_length_
+ // bytes long.
+ generated_connection_id_ = QuicConnectionId(
+ {0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0a, 0x0b});
+ expect_generator_is_called_ = version_.HasIetfQuicFrames();
+ QuicConnectionId new_id =
+ expect_generator_is_called_ ? *generated_connection_id_ : old_id;
+ // This will not return the correct value for long headers that might use a
+ // first byte other than 0x00, but long header replies don't matter.
+ if (GetQuicReloadableFlag(quic_ask_for_short_header_connection_id_length)) {
+ EXPECT_CALL(connection_id_generator_, ConnectionIdLength(0x00))
+ .WillRepeatedly(Return(generated_connection_id_->length()));
+ } else {
+ EXPECT_CALL(connection_id_generator_, ConnectionIdLength(_)).Times(0);
+ }
+ QuicSocketAddress client_address(QuicIpAddress::Loopback4(), 1);
+ EXPECT_CALL(*dispatcher_,
+ CreateQuicSession(new_id, _, client_address, Eq(ExpectedAlpn()),
+ _, Eq(ParsedClientHelloForTest())))
+ .WillOnce(Return(ByMove(CreateSession(
+ dispatcher_.get(), config_, new_id, client_address, &mock_helper_,
+ &mock_alarm_factory_, &crypto_config_,
+ QuicDispatcherPeer::GetCache(dispatcher_.get()), &session1_))));
+ EXPECT_CALL(*reinterpret_cast<MockQuicConnection*>(session1_->connection()),
+ ProcessUdpPacket(_, _, _))
+ .WillOnce(WithArg<2>(Invoke([this](const QuicEncryptedPacket& packet) {
+ ValidatePacket(TestConnectionId(1), packet);
+ })));
+ ProcessFirstFlight(client_address, old_id);
+
+ // Send short header packets with the new length and verify they are parsed
+ // correctly. If flag is set, all versions should succeed. If not, it should
+ // fail (this is the bugfix!). gQUIC never gets a new connection ID, so it's
+ // not affected by asking.
+ if (version_.HasIetfQuicFrames() &&
+ !GetQuicReloadableFlag(quic_ask_for_short_header_connection_id_length)) {
+ // Dispatcher issued a longer connection ID if IETF QUIC, but won't ask for
+ // that length when processing a short header. Thus dispatch will fail.
+ EXPECT_CALL(*reinterpret_cast<MockQuicConnection*>(session1_->connection()),
+ ProcessUdpPacket(_, _, _))
+ .Times(0);
+ } else {
+ // Dispatch succeeds, because it's gQUIC or all the flags are aligned.
+ EXPECT_CALL(*reinterpret_cast<MockQuicConnection*>(session1_->connection()),
+ ProcessUdpPacket(_, _, _))
+ .Times(1);
+ }
+ ProcessPacket(client_address, new_id, false, "foo");
+}
+
void QuicDispatcherTestBase::TestTlsMultiPacketClientHello(
bool add_reordering, bool long_connection_id) {
if (!version_.UsesTls()) {
diff --git a/quiche/quic/core/quic_flags_list.h b/quiche/quic/core/quic_flags_list.h
index 6078d31..d54304f 100644
--- a/quiche/quic/core/quic_flags_list.h
+++ b/quiche/quic/core/quic_flags_list.h
@@ -83,6 +83,8 @@
QUIC_FLAG(quic_reloadable_flag_quic_connection_migration_use_new_cid_v2, true)
// If true, uses conservative cwnd gain and pacing gain when cwnd gets bootstrapped.
QUIC_FLAG(quic_reloadable_flag_quic_conservative_cwnd_and_pacing_gains, false)
+// Instead of assuming an incoming connection ID length for short headers, ask each time, if both quic_abstract_connection_id_generator and quic_connection_uses_abstract_connection_id_generator are true.
+QUIC_FLAG(quic_reloadable_flag_quic_ask_for_short_header_connection_id_length, false)
// QuicConnection uses a library to generate connection IDs
QUIC_FLAG(quic_reloadable_flag_quic_connection_uses_abstract_connection_id_generator, true)
// When true, defaults to BBR congestion control instead of Cubic.
diff --git a/quiche/quic/core/quic_framer.cc b/quiche/quic/core/quic_framer.cc
index 2c7860f..29a0789 100644
--- a/quiche/quic/core/quic_framer.cc
+++ b/quiche/quic/core/quic_framer.cc
@@ -4,6 +4,8 @@
#include "quiche/quic/core/quic_framer.h"
+#include <sys/types.h>
+
#include <cstddef>
#include <cstdint>
#include <limits>
@@ -6863,6 +6865,35 @@
}
// static
+QuicErrorCode QuicFramer::ParsePublicHeaderDispatcherShortHeaderLengthUnknown(
+ const QuicEncryptedPacket& packet, PacketHeaderFormat* format,
+ QuicLongHeaderType* long_packet_type, bool* version_present,
+ bool* has_length_prefix, QuicVersionLabel* version_label,
+ ParsedQuicVersion* parsed_version,
+ QuicConnectionId* destination_connection_id,
+ QuicConnectionId* source_connection_id,
+ absl::optional<absl::string_view>* retry_token, std::string* detailed_error,
+ ConnectionIdGeneratorInterface& generator) {
+ QuicDataReader reader(packet.data(), packet.length());
+ // Get the first two bytes.
+ if (reader.BytesRemaining() < 2) {
+ *detailed_error = "Unable to read first two bytes.";
+ return QUIC_INVALID_PACKET_HEADER;
+ }
+ uint8_t two_bytes[2];
+ reader.ReadBytes(two_bytes, 2);
+ uint8_t expected_destination_connection_id_length =
+ (two_bytes[0] & FLAGS_LONG_HEADER)
+ ? 0
+ : generator.ConnectionIdLength(two_bytes[1]);
+ return ParsePublicHeaderDispatcher(
+ packet, expected_destination_connection_id_length, format,
+ long_packet_type, version_present, has_length_prefix, version_label,
+ parsed_version, destination_connection_id, source_connection_id,
+ retry_token, detailed_error);
+}
+
+// static
QuicErrorCode QuicFramer::ParsePublicHeaderGoogleQuic(
QuicDataReader* reader, uint8_t* first_byte, PacketHeaderFormat* format,
bool* version_present, QuicVersionLabel* version_label,
@@ -7021,8 +7052,6 @@
*format = GetIetfPacketHeaderFormat(*first_byte);
if (*format == IETF_QUIC_SHORT_HEADER_PACKET) {
- // Read destination connection ID using
- // expected_destination_connection_id_length to determine its length.
if (!reader->ReadConnectionId(destination_connection_id,
expected_destination_connection_id_length)) {
*detailed_error = "Unable to read destination connection ID.";
diff --git a/quiche/quic/core/quic_framer.h b/quiche/quic/core/quic_framer.h
index 4552693..484d7c1 100644
--- a/quiche/quic/core/quic_framer.h
+++ b/quiche/quic/core/quic_framer.h
@@ -11,6 +11,7 @@
#include <string>
#include "absl/strings/string_view.h"
+#include "quiche/quic/core/connection_id_generator.h"
#include "quiche/quic/core/crypto/quic_decrypter.h"
#include "quiche/quic/core/crypto/quic_encrypter.h"
#include "quiche/quic/core/crypto/quic_random.h"
@@ -432,6 +433,10 @@
// Parses the unencrypted fields in a QUIC header using |reader| as input,
// stores the result in the other parameters.
// |expected_destination_connection_id_length| is only used for short headers.
+ // When server connection IDs are generated by a
+ // ConnectionIdGeneartor interface, and callers need an accurate
+ // Destination Connection ID for short header packets, call
+ // ParsePublicHeaderDispatcherShortHeaderLengthUnknown() instead.
static QuicErrorCode ParsePublicHeader(
QuicDataReader* reader, uint8_t expected_destination_connection_id_length,
bool ietf_format, uint8_t* first_byte, PacketHeaderFormat* format,
@@ -445,7 +450,10 @@
// Parses the unencrypted fields in |packet| and stores them in the other
// parameters. This can only be called on the server.
- // |expected_destination_connection_id_length| is only used for short headers.
+ // |expected_destination_connection_id_length| is only used
+ // for short headers. When callers need an accurate Destination Connection ID
+ // specifically for short header packets, call
+ // ParsePublicHeaderDispatcherShortHeaderLengthUnknown() instead.
static QuicErrorCode ParsePublicHeaderDispatcher(
const QuicEncryptedPacket& packet,
uint8_t expected_destination_connection_id_length,
@@ -457,6 +465,24 @@
absl::optional<absl::string_view>* retry_token,
std::string* detailed_error);
+ // Parses the unencrypted fields in |packet| and stores them in the other
+ // parameters. The only callers that should use this method are ones where
+ // (1) the short-header connection ID length is only known by looking at the
+ // connection ID itself (and |generator| can provide the answer), and (2)
+ // the caller is interested in the parsed contents even if the packet has a
+ // short header. Some callers are only interested in parsing long header
+ // packets to peer into the handshake, and should use
+ // ParsePublicHeaderDispatcher instead.
+ static QuicErrorCode ParsePublicHeaderDispatcherShortHeaderLengthUnknown(
+ const QuicEncryptedPacket& packet, PacketHeaderFormat* format,
+ QuicLongHeaderType* long_packet_type, bool* version_present,
+ bool* has_length_prefix, QuicVersionLabel* version_label,
+ ParsedQuicVersion* parsed_version,
+ QuicConnectionId* destination_connection_id,
+ QuicConnectionId* source_connection_id,
+ absl::optional<absl::string_view>* retry_token,
+ std::string* detailed_error, ConnectionIdGeneratorInterface& generator);
+
// Serializes a packet containing |frames| into |buffer|.
// Returns the length of the packet, which must not be longer than
// |packet_length|. Returns 0 if it fails to serialize.
diff --git a/quiche/quic/core/quic_framer_test.cc b/quiche/quic/core/quic_framer_test.cc
index 16f44a5..1293834 100644
--- a/quiche/quic/core/quic_framer_test.cc
+++ b/quiche/quic/core/quic_framer_test.cc
@@ -16416,6 +16416,65 @@
framer_.detailed_error());
}
+TEST_P(QuicFramerTest, ShortHeaderWithNonDefaultConnectionIdLength) {
+ SetDecrypterLevel(ENCRYPTION_FORWARD_SECURE);
+ // clang-format off
+ unsigned char packet[kMaxIncomingPacketSize + 1] = {
+ // type (short header, 4 byte packet number)
+ 0x43,
+ // connection_id
+ 0x28, 0xDC, 0xBA, 0x98, 0x76, 0x54, 0x32, 0x10, 0x48,
+ // packet number
+ 0x12, 0x34, 0x56, 0x78,
+
+ // frame type (padding frame)
+ 0x00,
+ 0x00, 0x00, 0x00, 0x00
+ };
+ unsigned char* p = packet;
+ size_t p_size = ABSL_ARRAYSIZE(packet);
+
+ const size_t header_size = GetPacketHeaderSize(
+ framer_.transport_version(), kPacket8ByteConnectionId + 1,
+ kPacket0ByteConnectionId, !kIncludeVersion,
+ !kIncludeDiversificationNonce, PACKET_4BYTE_PACKET_NUMBER,
+ quiche::VARIABLE_LENGTH_INTEGER_LENGTH_0, 0,
+ quiche::VARIABLE_LENGTH_INTEGER_LENGTH_0) + 1;
+ // Add one because it's a 9 byte connection ID.
+
+ memset(p + header_size, 0, kMaxIncomingPacketSize - header_size);
+
+ QuicEncryptedPacket encrypted(AsChars(p), p_size, false);
+ MockConnectionIdGenerator generator;
+ PacketHeaderFormat format;
+ QuicLongHeaderType long_packet_type = INVALID_PACKET_TYPE;
+ bool version_flag;
+ QuicConnectionId destination_connection_id, source_connection_id;
+ QuicVersionLabel version_label;
+ std::string detailed_error;
+ bool use_length_prefix;
+ absl::optional<absl::string_view> retry_token;
+ ParsedQuicVersion parsed_version = UnsupportedQuicVersion();
+ EXPECT_CALL(generator, ConnectionIdLength(0x28))
+ .WillOnce(Return(9));
+ EXPECT_EQ(QUIC_NO_ERROR,
+ QuicFramer::ParsePublicHeaderDispatcherShortHeaderLengthUnknown(
+ encrypted, &format, &long_packet_type, &version_flag,
+ &use_length_prefix, &version_label, &parsed_version,
+ &destination_connection_id, &source_connection_id, &retry_token,
+ &detailed_error, generator));
+ EXPECT_EQ(format, IETF_QUIC_SHORT_HEADER_PACKET);
+ EXPECT_EQ(long_packet_type, INVALID_PACKET_TYPE);
+ EXPECT_FALSE(version_flag);
+ EXPECT_FALSE(use_length_prefix);
+ EXPECT_EQ(version_label, 0);
+ EXPECT_EQ(parsed_version, UnsupportedQuicVersion());
+ EXPECT_EQ(destination_connection_id.length(), 9);
+ EXPECT_EQ(source_connection_id.length(), 0);
+ EXPECT_FALSE(retry_token.has_value());
+ EXPECT_EQ(detailed_error, "");
+}
+
} // namespace
} // namespace test
} // namespace quic
