Add connection ID length checks
These changes only impact behavior for versions that support variable length connection IDs, and all of those versions are disabled by flags, so we don't need extra flag protection.
gfe-relnote: add connection ID length checks, protected by disabled quic_enable_v47 flag
PiperOrigin-RevId: 261237221
Change-Id: I89e7bec58644b7ec18e3c7ce3ecbd6d93c9c0fc3
diff --git a/quic/core/http/quic_spdy_client_session_test.cc b/quic/core/http/quic_spdy_client_session_test.cc
index be06984..fc42d52 100644
--- a/quic/core/http/quic_spdy_client_session_test.cc
+++ b/quic/core/http/quic_spdy_client_session_test.cc
@@ -31,6 +31,7 @@
using spdy::SpdyHeaderBlock;
using testing::_;
using testing::AnyNumber;
+using testing::AtMost;
using testing::Invoke;
using testing::Truly;
@@ -503,10 +504,7 @@
QuicReceivedPacket valid_packet(buf, 2, QuicTime::Zero(), false);
// Close connection shouldn't be called.
EXPECT_CALL(*connection_, CloseConnection(_, _, _)).Times(0);
- if (connection_->transport_version() > QUIC_VERSION_44) {
- // Illegal fixed bit value.
- EXPECT_CALL(*connection_, OnError(_)).Times(1);
- }
+ EXPECT_CALL(*connection_, OnError(_)).Times(AtMost(1));
session_->ProcessUdpPacket(client_address, server_address, valid_packet);
// Verify that a non-decryptable packet doesn't close the connection.
diff --git a/quic/core/quic_connection_id.cc b/quic/core/quic_connection_id.cc
index 3ad13a3..7d0f8d7 100644
--- a/quic/core/quic_connection_id.cc
+++ b/quic/core/quic_connection_id.cc
@@ -57,7 +57,8 @@
kQuicMaxConnectionIdLength <= std::numeric_limits<uint8_t>::max(),
"kQuicMaxConnectionIdLength too high");
if (length > kQuicMaxConnectionIdLength) {
- QUIC_BUG << "Attempted to create connection ID of length " << length;
+ QUIC_BUG << "Attempted to create connection ID of length "
+ << static_cast<int>(length);
length = kQuicMaxConnectionIdLength;
}
length_ = length;
@@ -126,7 +127,8 @@
void QuicConnectionId::set_length(uint8_t length) {
if (length > kQuicMaxConnectionIdLength) {
- QUIC_BUG << "Attempted to set connection ID length to " << length;
+ QUIC_BUG << "Attempted to set connection ID length to "
+ << static_cast<int>(length);
length = kQuicMaxConnectionIdLength;
}
if (GetQuicRestartFlag(quic_use_allocated_connection_ids)) {
diff --git a/quic/core/quic_connection_id.h b/quic/core/quic_connection_id.h
index 6b1b0bc..f1861e0 100644
--- a/quic/core/quic_connection_id.h
+++ b/quic/core/quic_connection_id.h
@@ -25,7 +25,7 @@
CONNECTION_ID_ABSENT = 2,
};
-// Connection IDs can be 0-18 bytes per IETF specifications.
+// Maximum connection ID length that we support in any packet or version.
const uint8_t kQuicMaxConnectionIdLength = 18;
// kQuicDefaultConnectionIdLength is the only supported length for QUIC
diff --git a/quic/core/quic_connection_test.cc b/quic/core/quic_connection_test.cc
index 176e386..a3ab06f 100644
--- a/quic/core/quic_connection_test.cc
+++ b/quic/core/quic_connection_test.cc
@@ -341,7 +341,10 @@
clock_(clock),
write_pause_time_delta_(QuicTime::Delta::Zero()),
max_packet_size_(kMaxOutgoingPacketSize),
- supports_release_time_(false) {}
+ supports_release_time_(false) {
+ QuicFramerPeer::SetLastSerializedServerConnectionId(framer_.framer(),
+ TestConnectionId());
+ }
TestPacketWriter(const TestPacketWriter&) = delete;
TestPacketWriter& operator=(const TestPacketWriter&) = delete;
diff --git a/quic/core/quic_data_reader.cc b/quic/core/quic_data_reader.cc
index e2871d7..71395dd 100644
--- a/quic/core/quic_data_reader.cc
+++ b/quic/core/quic_data_reader.cc
@@ -160,7 +160,8 @@
}
connection_id->set_length(length);
- const bool ok = ReadBytes(connection_id->mutable_data(), length);
+ const bool ok =
+ ReadBytes(connection_id->mutable_data(), connection_id->length());
DCHECK(ok);
return ok;
}
diff --git a/quic/core/quic_data_writer_test.cc b/quic/core/quic_data_writer_test.cc
index 727c813..352f27c 100644
--- a/quic/core/quic_data_writer_test.cc
+++ b/quic/core/quic_data_writer_test.cc
@@ -1149,11 +1149,11 @@
}
TEST_P(QuicDataWriterTest, InvalidConnectionIdLengthRead) {
- static const uint8_t bad_connection_id_length = 19;
+ static const uint8_t bad_connection_id_length = 200;
static_assert(bad_connection_id_length > kQuicMaxConnectionIdLength,
"bad lengths");
- char buffer[20] = {};
- QuicDataReader reader(buffer, 20);
+ char buffer[255] = {};
+ QuicDataReader reader(buffer, sizeof(buffer));
QuicConnectionId connection_id;
bool ok;
EXPECT_QUIC_BUG(
diff --git a/quic/core/quic_dispatcher.cc b/quic/core/quic_dispatcher.cc
index 7522703..1782e4a 100644
--- a/quic/core/quic_dispatcher.cc
+++ b/quic/core/quic_dispatcher.cc
@@ -263,6 +263,26 @@
QUIC_DLOG(ERROR) << "Invalid Connection Id Length";
return;
}
+
+ if (packet_info.version_flag && IsSupportedVersion(packet_info.version)) {
+ if (!QuicUtils::IsConnectionIdValidForVersion(
+ packet_info.destination_connection_id,
+ packet_info.version.transport_version)) {
+ SetLastError(QUIC_INVALID_PACKET_HEADER);
+ QUIC_DLOG(ERROR)
+ << "Invalid destination connection ID length for version";
+ return;
+ }
+ if (packet_info.version.SupportsClientConnectionIds() &&
+ !QuicUtils::IsConnectionIdValidForVersion(
+ packet_info.source_connection_id,
+ packet_info.version.transport_version)) {
+ SetLastError(QUIC_INVALID_PACKET_HEADER);
+ QUIC_DLOG(ERROR) << "Invalid source connection ID length for version";
+ return;
+ }
+ }
+
if (should_update_expected_server_connection_id_length_) {
expected_server_connection_id_length_ =
packet_info.destination_connection_id.length();
diff --git a/quic/core/quic_framer.cc b/quic/core/quic_framer.cc
index ecad6a1..bd0a724 100644
--- a/quic/core/quic_framer.cc
+++ b/quic/core/quic_framer.cc
@@ -24,6 +24,7 @@
#include "net/third_party/quiche/src/quic/core/quic_data_reader.h"
#include "net/third_party/quiche/src/quic/core/quic_data_writer.h"
#include "net/third_party/quiche/src/quic/core/quic_error_codes.h"
+#include "net/third_party/quiche/src/quic/core/quic_packets.h"
#include "net/third_party/quiche/src/quic/core/quic_socket_address_coder.h"
#include "net/third_party/quiche/src/quic/core/quic_stream_frame_data_producer.h"
#include "net/third_party/quiche/src/quic/core/quic_types.h"
@@ -1667,6 +1668,13 @@
}
}
+ if (!QuicUtils::IsConnectionIdValidForVersion(
+ original_destination_connection_id, transport_version())) {
+ set_detailed_error(
+ "Received Original Destination ConnectionId with invalid length.");
+ return false;
+ }
+
QuicStringPiece retry_token = reader->ReadRemainingPayload();
visitor_->OnRetryPacket(original_destination_connection_id,
header.source_connection_id, retry_token);
@@ -2683,6 +2691,24 @@
return true;
}
+bool QuicFramer::ValidateReceivedConnectionIds(const QuicPacketHeader& header) {
+ if (!QuicUtils::IsConnectionIdValidForVersion(
+ GetServerConnectionIdAsRecipient(header, perspective_),
+ transport_version())) {
+ set_detailed_error("Received server connection ID with invalid length.");
+ return false;
+ }
+
+ if (version_.SupportsClientConnectionIds() &&
+ !QuicUtils::IsConnectionIdValidForVersion(
+ GetClientConnectionIdAsRecipient(header, perspective_),
+ transport_version())) {
+ set_detailed_error("Received client connection ID with invalid length.");
+ return false;
+ }
+ return true;
+}
+
bool QuicFramer::ProcessIetfPacketHeader(QuicDataReader* reader,
QuicPacketHeader* header) {
if (version_.HasLengthPrefixedConnectionIds()) {
@@ -2716,6 +2742,11 @@
header->source_connection_id = last_serialized_client_connection_id_;
}
}
+
+ if (!ValidateReceivedConnectionIds(*header)) {
+ return false;
+ }
+
if (header->version_flag &&
header->version.transport_version > QUIC_VERSION_44 &&
!(header->type_byte & FLAGS_FIXED_BIT)) {
@@ -2784,9 +2815,6 @@
}
}
- DCHECK_LE(destination_connection_id_length, kQuicMaxConnectionIdLength);
- DCHECK_LE(source_connection_id_length, kQuicMaxConnectionIdLength);
-
// Read connection ID.
if (!reader->ReadConnectionId(&header->destination_connection_id,
destination_connection_id_length)) {
@@ -2813,7 +2841,7 @@
}
}
- return true;
+ return ValidateReceivedConnectionIds(*header);
}
bool QuicFramer::ProcessAndCalculatePacketNumber(
@@ -6122,11 +6150,6 @@
return false;
}
- if (frame->connection_id.length() > kQuicMaxConnectionIdLength) {
- set_detailed_error("New connection ID length too high.");
- return false;
- }
-
if (!QuicUtils::IsConnectionIdValidForVersion(frame->connection_id,
transport_version())) {
set_detailed_error("Invalid new connection ID length for version.");
diff --git a/quic/core/quic_framer.h b/quic/core/quic_framer.h
index a05ad8f..94632c6 100644
--- a/quic/core/quic_framer.h
+++ b/quic/core/quic_framer.h
@@ -873,6 +873,8 @@
QuicConnectionId* destination_connection_id,
std::string* detailed_error);
+ bool ValidateReceivedConnectionIds(const QuicPacketHeader& header);
+
// The Append* methods attempt to write the provided header or frame using the
// |writer|, and return true if successful.
diff --git a/quic/core/quic_framer_test.cc b/quic/core/quic_framer_test.cc
index bdb9019..f6f5eda 100644
--- a/quic/core/quic_framer_test.cc
+++ b/quic/core/quic_framer_test.cc
@@ -5641,8 +5641,6 @@
unsigned char packet[] = {
// type (short packet, 1 byte packet number)
0x50,
- // connection_id
- 0xFE, 0xDC, 0xBA, 0x98, 0x76, 0x54, 0x32, 0x10,
// Random bytes
0x01, 0x11, 0x02, 0x22, 0x03, 0x33, 0x04, 0x44,
0x01, 0x11, 0x02, 0x22, 0x03, 0x33, 0x04, 0x44,
@@ -5657,6 +5655,8 @@
return;
}
QuicFramerPeer::SetPerspective(&framer_, Perspective::IS_CLIENT);
+ QuicFramerPeer::SetLastSerializedServerConnectionId(&framer_,
+ TestConnectionId(0x33));
decrypter_ = new test::TestDecrypter();
if (framer_.version().KnowsWhichDecrypterToUse()) {
framer_.InstallDecrypter(ENCRYPTION_INITIAL, QuicMakeUnique<NullDecrypter>(
@@ -5683,8 +5683,11 @@
unsigned char packet[] = {
// type (short packet, 1 byte packet number)
0x50,
- // connection_id
- 0xFE, 0xDC, 0xBA, 0x98, 0x76, 0x54, 0x32, 0x10,
+ // Random bytes
+ 0x01, 0x11, 0x02, 0x22, 0x03, 0x33, 0x04, 0x44,
+ 0x01, 0x11, 0x02, 0x22, 0x03, 0x33, 0x04, 0x44,
+ 0x01, 0x11, 0x02, 0x22, 0x03, 0x33, 0x04, 0x44,
+ 0x01, 0x11, 0x02, 0x22, 0x03, 0x33, 0x04, 0x44,
// stateless reset token
0xB6, 0x69, 0x0F, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
@@ -5693,6 +5696,8 @@
if (framer_.transport_version() <= QUIC_VERSION_43) {
return;
}
+ QuicFramerPeer::SetLastSerializedServerConnectionId(&framer_,
+ TestConnectionId(0x33));
QuicFramerPeer::SetPerspective(&framer_, Perspective::IS_CLIENT);
decrypter_ = new test::TestDecrypter();
if (framer_.version().KnowsWhichDecrypterToUse()) {
@@ -11566,11 +11571,16 @@
{"Unable to read new connection ID frame retire_prior_to.",
{kVarInt62OneByte + 0x0b}},
{"Unable to read new connection ID frame connection id.",
- {0x13}}, // connection ID length
+ {0x40}}, // connection ID length
{"Unable to read new connection ID frame connection id.",
{0xFE, 0xDC, 0xBA, 0x98, 0x76, 0x54, 0x32, 0x10,
0xF0, 0xD2, 0xB4, 0x96, 0x78, 0x5A, 0x3C, 0x1E,
- 0x42, 0x33, 0x42}},
+ 0xFE, 0xDC, 0xBA, 0x98, 0x76, 0x54, 0x32, 0x10,
+ 0xF0, 0xD2, 0xB4, 0x96, 0x78, 0x5A, 0x3C, 0x1E,
+ 0xFE, 0xDC, 0xBA, 0x98, 0x76, 0x54, 0x32, 0x10,
+ 0xF0, 0xD2, 0xB4, 0x96, 0x78, 0x5A, 0x3C, 0x1E,
+ 0xFE, 0xDC, 0xBA, 0x98, 0x76, 0x54, 0x32, 0x10,
+ 0xF0, 0xD2, 0xB4, 0x96, 0x78, 0x5A, 0x3C, 0x1E}},
{"Can not read new connection ID frame reset token.",
{0xb5, 0x69, 0x0f, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}}
@@ -13523,7 +13533,8 @@
}
TEST_P(QuicFramerTest, PacketHeaderWithVariableLengthConnectionId) {
- if (framer_.transport_version() < QUIC_VERSION_46) {
+ if (!QuicUtils::VariableLengthConnectionIdAllowedForVersion(
+ framer_.transport_version())) {
return;
}
SetDecrypterLevel(ENCRYPTION_FORWARD_SECURE);
diff --git a/quic/core/quic_utils.cc b/quic/core/quic_utils.cc
index a9de14a..4ff7489 100644
--- a/quic/core/quic_utils.cc
+++ b/quic/core/quic_utils.cc
@@ -505,18 +505,12 @@
QuicConnectionId QuicUtils::CreateRandomConnectionId(
uint8_t connection_id_length,
QuicRandom* random) {
- if (connection_id_length == 0) {
- return EmptyQuicConnectionId();
+ QuicConnectionId connection_id;
+ connection_id.set_length(connection_id_length);
+ if (connection_id.length() > 0) {
+ random->RandBytes(connection_id.mutable_data(), connection_id.length());
}
- if (connection_id_length > kQuicMaxConnectionIdLength) {
- QUIC_BUG << "Tried to CreateRandomConnectionId of invalid length "
- << static_cast<int>(connection_id_length);
- connection_id_length = kQuicMaxConnectionIdLength;
- }
- char connection_id_bytes[kQuicMaxConnectionIdLength];
- random->RandBytes(connection_id_bytes, connection_id_length);
- return QuicConnectionId(static_cast<char*>(connection_id_bytes),
- connection_id_length);
+ return connection_id;
}
// static
@@ -554,7 +548,14 @@
if (!VariableLengthConnectionIdAllowedForVersion(transport_version)) {
return connection_id_length8 == kQuicDefaultConnectionIdLength;
}
- // Currently all other versions require the length to be at most 18 bytes.
+ // Versions that do support variable length but do not have length-prefixed
+ // connection IDs use the 4-bit connection ID length encoding which can
+ // only encode values 0 and 4-18.
+ if (!VersionHasLengthPrefixedConnectionIds(transport_version)) {
+ return connection_id_length8 == 0 ||
+ (connection_id_length8 >= 4 &&
+ connection_id_length8 <= kQuicMaxConnectionIdLength);
+ }
return connection_id_length8 <= kQuicMaxConnectionIdLength;
}
diff --git a/quic/test_tools/crypto_test_utils.cc b/quic/test_tools/crypto_test_utils.cc
index 095472a..3fb896a 100644
--- a/quic/test_tools/crypto_test_utils.cc
+++ b/quic/test_tools/crypto_test_utils.cc
@@ -679,9 +679,13 @@
PacketSavingConnection* dest_conn,
Perspective dest_perspective) {
SimpleQuicFramer framer(source_conn->supported_versions(), dest_perspective);
+ QuicFramerPeer::SetLastSerializedServerConnectionId(framer.framer(),
+ TestConnectionId());
SimpleQuicFramer null_encryption_framer(source_conn->supported_versions(),
dest_perspective);
+ QuicFramerPeer::SetLastSerializedServerConnectionId(
+ null_encryption_framer.framer(), TestConnectionId());
size_t index = *inout_packet_index;
for (; index < source_conn->encrypted_packets_.size(); index++) {
