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
