Make QuicDispatcher reject packets with invalid short connection IDs This CLs enforces a MUST in the IETF spec that dictates that clients cannot send initial connection IDs under 8 bytes. The QuicDispatcher will reject (and close the connection of) any packet whose connection ID is shorter than 8 (or what it was configured for). The behavior is disabled by quartc. This only impacts v99 because connection IDs of any length other than 8 are already currently dropped when using versions < 99. gfe-relnote: v99 only, not flag protected PiperOrigin-RevId: 239629063 Change-Id: I85cee11d84566073e8cbb3569ba3e88e91192f2a
diff --git a/quic/core/quic_connection_id.h b/quic/core/quic_connection_id.h index 262a658..d51366f 100644 --- a/quic/core/quic_connection_id.h +++ b/quic/core/quic_connection_id.h
@@ -31,6 +31,10 @@ // versions < v99, and is the default picked for all versions. const uint8_t kQuicDefaultConnectionIdLength = 8; +// According to the IETF spec, the initial server connection ID generated by +// the client must be at least this long. +const uint8_t kQuicMinimumInitialConnectionIdLength = 8; + class QUIC_EXPORT_PRIVATE QuicConnectionId { public: // Creates a connection ID of length zero.
diff --git a/quic/core/quic_dispatcher.cc b/quic/core/quic_dispatcher.cc index 7ef2869..d1200a9 100644 --- a/quic/core/quic_dispatcher.cc +++ b/quic/core/quic_dispatcher.cc
@@ -296,7 +296,8 @@ expected_connection_id_length), last_error_(QUIC_NO_ERROR), new_sessions_allowed_per_event_loop_(0u), - accept_new_connections_(true) { + accept_new_connections_(true), + allow_short_initial_connection_ids_(false) { framer_.set_visitor(this); } @@ -371,6 +372,26 @@ } QuicConnectionId connection_id = header.destination_connection_id; + // The IETF spec requires the client to generate an initial server + // connection ID that is at least 64 bits long. After that initial + // connection ID, the dispatcher picks a new one of its expected length. + // Therefore we should never receive a connection ID that is smaller + // than 64 bits and smaller than what we expect. + if (connection_id.length() < kQuicMinimumInitialConnectionIdLength && + connection_id.length() < framer_.GetExpectedConnectionIdLength() && + !allow_short_initial_connection_ids_) { + DCHECK(header.version_flag); + DCHECK(QuicUtils::VariableLengthConnectionIdAllowedForVersion( + header.version.transport_version)); + QUIC_DLOG(INFO) << "Packet with short destination connection ID " + << connection_id << " expected " + << static_cast<int>( + framer_.GetExpectedConnectionIdLength()); + ProcessUnauthenticatedHeaderFate(kFateTimeWait, connection_id, header.form, + header.version); + return false; + } + // Packets with connection IDs for active connections are processed // immediately. auto it = session_map_.find(connection_id);
diff --git a/quic/core/quic_dispatcher.h b/quic/core/quic_dispatcher.h index cdd1196..f00880b 100644 --- a/quic/core/quic_dispatcher.h +++ b/quic/core/quic_dispatcher.h
@@ -364,6 +364,13 @@ should_update_expected_connection_id_length); } + // If true, the dispatcher will allow incoming initial packets that have + // connection IDs shorter than 64 bits. + void SetAllowShortInitialConnectionIds( + bool allow_short_initial_connection_ids) { + allow_short_initial_connection_ids_ = allow_short_initial_connection_ids; + } + private: friend class test::QuicDispatcherPeer; friend class StatelessRejectorProcessDoneCallback; @@ -495,6 +502,10 @@ // True if this dispatcher is not draining. bool accept_new_connections_; + + // If false, the dispatcher follows the IETF spec and rejects packets with + // invalid connection IDs lengths below 64 bits. If true they are allowed. + bool allow_short_initial_connection_ids_; }; } // namespace quic
diff --git a/quic/core/quic_dispatcher_test.cc b/quic/core/quic_dispatcher_test.cc index 3f4ef0e..1e646af 100644 --- a/quic/core/quic_dispatcher_test.cc +++ b/quic/core/quic_dispatcher_test.cc
@@ -158,6 +158,7 @@ using QuicDispatcher::current_client_address; using QuicDispatcher::current_peer_address; using QuicDispatcher::current_self_address; + using QuicDispatcher::SetAllowShortInitialConnectionIds; using QuicDispatcher::writer; }; @@ -639,7 +640,7 @@ } // Makes sure nine-byte connection IDs are replaced by 8-byte ones. -TEST_F(QuicDispatcherTest, BadConnectionIdLengthReplaced) { +TEST_F(QuicDispatcherTest, LongConnectionIdLengthReplaced) { if (!QuicUtils::VariableLengthConnectionIdAllowedForVersion( CurrentSupportedVersions()[0].transport_version)) { // When variable length connection IDs are not supported, the connection @@ -672,6 +673,45 @@ EXPECT_EQ(server_address_, dispatcher_->current_self_address()); } +// Makes sure zero-byte connection IDs are replaced by 8-byte ones. +TEST_F(QuicDispatcherTest, InvalidShortConnectionIdLengthReplaced) { + if (!QuicUtils::VariableLengthConnectionIdAllowedForVersion( + CurrentSupportedVersions()[0].transport_version)) { + // When variable length connection IDs are not supported, the connection + // fails. See StrayPacketTruncatedConnectionId. + return; + } + QuicSocketAddress client_address(QuicIpAddress::Loopback4(), 1); + + QuicConnectionId bad_connection_id = EmptyQuicConnectionId(); + QuicConnectionId fixed_connection_id = + QuicUtils::CreateRandomConnectionId(mock_helper_.GetRandomGenerator()); + + // Disable validation of invalid short connection IDs. + dispatcher_->SetAllowShortInitialConnectionIds(true); + // Note that StrayPacketTruncatedConnectionId covers the case where the + // validation is still enabled. + + EXPECT_CALL(*dispatcher_, + CreateQuicSession(fixed_connection_id, client_address, + QuicStringPiece("hq"), _)) + .WillOnce(testing::Return(CreateSession( + dispatcher_.get(), config_, fixed_connection_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, bad_connection_id](const QuicEncryptedPacket& packet) { + ValidatePacket(bad_connection_id, packet); + }))); + EXPECT_CALL(*dispatcher_, + ShouldCreateOrBufferPacketForConnection(bad_connection_id, _)); + ProcessPacket(client_address, bad_connection_id, true, SerializeCHLO()); + EXPECT_EQ(client_address, dispatcher_->current_peer_address()); + EXPECT_EQ(server_address_, dispatcher_->current_self_address()); +} + // Makes sure TestConnectionId(1) creates a new connection and // TestConnectionIdNineBytesLong(2) gets replaced. TEST_F(QuicDispatcherTest, MixGoodAndBadConnectionIdLengthPackets) { @@ -1323,20 +1363,15 @@ // Packets with truncated connection IDs should be dropped. TEST_F(QuicDispatcherTestStrayPacketConnectionId, StrayPacketTruncatedConnectionId) { - if (QuicUtils::VariableLengthConnectionIdAllowedForVersion( - CurrentSupportedVersions()[0].transport_version)) { - // When variable length connection IDs are supported, the server - // transparently replaces empty connection IDs with one it chooses. - // See BadConnectionIdLengthReplaced. - return; - } CreateTimeWaitListManager(); QuicSocketAddress client_address(QuicIpAddress::Loopback4(), 1); QuicConnectionId connection_id = TestConnectionId(1); EXPECT_CALL(*dispatcher_, CreateQuicSession(_, _, QuicStringPiece("hq"), _)) .Times(0); - if (CurrentSupportedVersions()[0].transport_version > QUIC_VERSION_43) { + if (CurrentSupportedVersions()[0].transport_version > QUIC_VERSION_43 && + !QuicUtils::VariableLengthConnectionIdAllowedForVersion( + CurrentSupportedVersions()[0].transport_version)) { // This IETF packet has invalid connection ID length. EXPECT_CALL(*time_wait_list_manager_, ProcessPacket(_, _, _, _, _)) .Times(0);
diff --git a/quic/quartc/quartc_dispatcher.cc b/quic/quartc/quartc_dispatcher.cc index 4c35c54..fd4f289 100644 --- a/quic/quartc/quartc_dispatcher.cc +++ b/quic/quartc/quartc_dispatcher.cc
@@ -38,6 +38,8 @@ packet_writer_(packet_writer.get()) { // Allow incoming packets to set our expected connection ID length. SetShouldUpdateExpectedConnectionIdLength(true); + // Allow incoming packets with connection ID lengths shorter than allowed. + SetAllowShortInitialConnectionIds(true); // QuicDispatcher takes ownership of the writer. QuicDispatcher::InitializeWithWriter(packet_writer.release()); // NB: This must happen *after* InitializeWithWriter. It can call us back