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_framer.cc

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;
   }