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