Check new connection ID length before reading it This CL fixes a buffer overflow in the NEW_CONNECTION_ID read path, and adds sanity checks to prevent similar issues from reoccuring. The issue was found by clusterfuzz: https://bugs.chromium.org/p/chromium/issues/detail?id=943951#c4 gfe-relnote: trivial security fix when parsing invalid frame, not flag protected PiperOrigin-RevId: 239486794 Change-Id: I70b8e7b4adfd52afbbcb3308ba7dded0416c884e

diff --git a/quic/core/quic_data_reader.cc b/quic/core/quic_data_reader.cc
index 4200329..0488fd3 100644
--- a/quic/core/quic_data_reader.cc
+++ b/quic/core/quic_data_reader.cc

@@ -133,7 +133,11 @@
 
 bool QuicDataReader::ReadConnectionId(QuicConnectionId* connection_id,
                                       uint8_t length) {
-  DCHECK_LE(length, kQuicMaxConnectionIdLength);
+  if (length > kQuicMaxConnectionIdLength) {
+    QUIC_BUG << "Attempted to read connection ID with length too high "
+             << static_cast<int>(length);
+    return false;
+  }
   if (length == 0) {
     connection_id->set_length(0);
     return true;

diff --git a/quic/core/quic_data_writer_test.cc b/quic/core/quic_data_writer_test.cc
index aed14c1..031620d 100644
--- a/quic/core/quic_data_writer_test.cc
+++ b/quic/core/quic_data_writer_test.cc

@@ -10,6 +10,7 @@
 #include "net/third_party/quiche/src/quic/core/quic_data_reader.h"
 #include "net/third_party/quiche/src/quic/core/quic_utils.h"
 #include "net/third_party/quiche/src/quic/platform/api/quic_arraysize.h"
+#include "net/third_party/quiche/src/quic/platform/api/quic_expect_bug.h"
 #include "net/third_party/quiche/src/quic/platform/api/quic_flags.h"
 #include "net/third_party/quiche/src/quic/platform/api/quic_test.h"
 #include "net/third_party/quiche/src/quic/test_tools/quic_test_utils.h"
@@ -38,7 +39,8 @@
 
 class QuicDataWriterTest : public QuicTestWithParam<TestParams> {};
 
-INSTANTIATE_TEST_SUITE_P(QuicDataWriterTests, QuicDataWriterTest,
+INSTANTIATE_TEST_SUITE_P(QuicDataWriterTests,
+                         QuicDataWriterTest,
                          ::testing::ValuesIn(GetTestParams()));
 
 TEST_P(QuicDataWriterTest, SanityCheckUFloat16Consts) {
@@ -1097,6 +1099,21 @@
   EXPECT_EQ(8, reader4.PeekVarInt62Length());
 }
 
+TEST_P(QuicDataWriterTest, InvalidConnectionIdLengthRead) {
+  static const uint8_t bad_connection_id_length = 19;
+  static_assert(bad_connection_id_length > kQuicMaxConnectionIdLength,
+                "bad lengths");
+  char buffer[20] = {};
+  QuicDataReader reader(buffer, 20);
+  QuicConnectionId connection_id;
+  bool ok;
+  EXPECT_QUIC_BUG(
+      ok = reader.ReadConnectionId(&connection_id, bad_connection_id_length),
+      QuicStrCat("Attempted to read connection ID with length too high ",
+                 static_cast<int>(bad_connection_id_length)));
+  EXPECT_FALSE(ok);
+}
+
 }  // namespace
 }  // namespace test
 }  // namespace quic

diff --git a/quic/core/quic_framer.cc b/quic/core/quic_framer.cc
index 333fe59..3e4f921 100644
--- a/quic/core/quic_framer.cc
+++ b/quic/core/quic_framer.cc

@@ -2579,6 +2579,9 @@
     source_connection_id_length = scil;
   }
 
+  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)) {
@@ -5597,10 +5600,15 @@
     return false;
   }
 
+  if (connection_id_length > kQuicMaxConnectionIdLength) {
+    set_detailed_error("New connection ID length too high.");
+    return false;
+  }
+
   if (connection_id_length != kQuicDefaultConnectionIdLength &&
       !QuicUtils::VariableLengthConnectionIdAllowedForVersion(
           transport_version())) {
-    set_detailed_error("Invalid new connection ID length.");
+    set_detailed_error("Invalid new connection ID length for version.");
     return false;
   }
 

diff --git a/quic/core/quic_framer_test.cc b/quic/core/quic_framer_test.cc
index 4604a08..ea362eb 100644
--- a/quic/core/quic_framer_test.cc
+++ b/quic/core/quic_framer_test.cc

@@ -11201,6 +11201,49 @@
   CheckFramingBoundaries(packet99, QUIC_INVALID_NEW_CONNECTION_ID_DATA);
 }
 
+// Verifies that parsing a NEW_CONNECTION_ID frame with a length above the
+// specified maximum fails.
+TEST_P(QuicFramerTest, InvalidLongNewConnectionIdFrame) {
+  if (framer_.transport_version() != QUIC_VERSION_99) {
+    // The NEW_CONNECTION_ID frame is only for version 99.
+    return;
+  }
+  // clang-format off
+  PacketFragments packet99 = {
+      // type (short header, 4 byte packet number)
+      {"",
+       {0x43}},
+      // connection_id
+      {"",
+       {0xFE, 0xDC, 0xBA, 0x98, 0x76, 0x54, 0x32, 0x10}},
+      // packet number
+      {"",
+       {0x12, 0x34, 0x56, 0x78}},
+      // frame type (IETF_NEW_CONNECTION_ID frame)
+      {"",
+       {0x18}},
+      // error code
+      {"Unable to read new connection ID frame sequence number.",
+       {kVarInt62OneByte + 0x11}},
+      {"Unable to read new connection ID frame connection id length.",
+       {0x13}},  // 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}},
+      {"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}}
+  };
+  // clang-format on
+
+  std::unique_ptr<QuicEncryptedPacket> encrypted(
+      AssemblePacketFromFragments(packet99));
+  EXPECT_FALSE(framer_.ProcessPacket(*encrypted));
+  EXPECT_EQ(QUIC_INVALID_NEW_CONNECTION_ID_DATA, framer_.error());
+  EXPECT_EQ("New connection ID length too high.", framer_.detailed_error());
+}
+
 TEST_P(QuicFramerTest, BuildNewConnectionIdFramePacket) {
   if (framer_.transport_version() != QUIC_VERSION_99) {
     // This frame is only for version 99.