commit 0131a5ba98b361e2fb646d36eb071b4f86fb581b
author QUICHE team <quiche-dev@google.com> Wed Mar 20 15:23:27 2019 -0700
committer Copybara-Service <copybara-worker@google.com> Wed Mar 20 16:10:39 2019 -0700
tree 8d900cd29587c021028ce62d25721539f0c4cbc1
parent dc41bf1a71fa71ffd0b7882604a8b3e7b8df70ea

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

quic/core/quic_data_writer_test.cc

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