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.
