commit 7d066ca9008f93bbe0a1edfeac4af8f8ce4d8814
author dschinazi <dschinazi@google.com> Wed May 15 17:59:49 2019 -0700
committer Copybara-Service <copybara-worker@google.com> Wed May 15 18:03:10 2019 -0700
tree 956d3ccebef4b8ff1355765241ff17b22508a1f3
parent d362a7a77038fbe8cf6df4ffe972dac903250921

Replace a DCHECK with a parse failure in connection ID parsing

This issue was found by ClusterFuzz:
https://bugs.chromium.org/p/chromium/issues/detail?id=962900

On the client, when parsing an IETF long header, we do not expect there to be a destination connection ID as we do not support client connection IDs yet. Instead of having a DCHECK to verify that, we should fail parsing with QUIC_INVALID_PACKET_HEADER.

gfe-relnote: change how client reacts to a type of invalid packet, client-only.
PiperOrigin-RevId: 248442467
Change-Id: Ie13868e5c45c0868c0f0d8655546ea927d705753

quic/core/quic_framer.cc

diff --git a/quic/core/quic_framer.cc b/quic/core/quic_framer.cc
index f0954d1..b686068 100644
--- a/quic/core/quic_framer.cc
+++ b/quic/core/quic_framer.cc
@@ -2699,8 +2699,13 @@
 
   if (!GetQuicRestartFlag(quic_do_not_override_connection_id)) {
     if (header->source_connection_id_included == CONNECTION_ID_PRESENT) {
+      DCHECK_EQ(Perspective::IS_CLIENT, perspective_);
+      DCHECK_EQ(IETF_QUIC_LONG_HEADER_PACKET, header->form);
+      if (!header->destination_connection_id.IsEmpty()) {
+        set_detailed_error("Client connection ID not supported yet.");
+        return false;
+      }
       // Set destination connection ID to source connection ID.
-      DCHECK_EQ(EmptyQuicConnectionId(), header->destination_connection_id);
       header->destination_connection_id = header->source_connection_id;
     } else if (header->destination_connection_id_included ==
                CONNECTION_ID_ABSENT) {

quic/core/quic_framer_test.cc

diff --git a/quic/core/quic_framer_test.cc b/quic/core/quic_framer_test.cc
index f196e5d..347df84 100644
--- a/quic/core/quic_framer_test.cc
+++ b/quic/core/quic_framer_test.cc
@@ -13701,6 +13701,49 @@
       parsed_probe_payload_bytes, parsed_probe_payload_length);
 }
 
+TEST_P(QuicFramerTest, ClientConnectionIdNotSupportedYet) {
+  if (GetQuicRestartFlag(quic_do_not_override_connection_id)) {
+    // This check is currently only performed when this flag is disabled.
+    return;
+  }
+  if (framer_.transport_version() <= QUIC_VERSION_43) {
+    // This test requires an IETF long header.
+    return;
+  }
+  QuicFramerPeer::SetPerspective(&framer_, Perspective::IS_CLIENT);
+  const unsigned char type_byte =
+      framer_.transport_version() == QUIC_VERSION_44 ? 0xFC : 0xD3;
+  // clang-format off
+  unsigned char packet[] = {
+    // public flags (long header with packet type ZERO_RTT_PROTECTED and
+    // 4-byte packet number)
+    type_byte,
+    // version
+    QUIC_VERSION_BYTES,
+    // destination connection ID length
+    0x50,
+    // destination connection ID
+    0xFE, 0xDC, 0xBA, 0x98, 0x76, 0x54, 0x32, 0x10,
+    // long header packet length
+    0x05,
+    // packet number
+    0x12, 0x34, 0x56, 0x00,
+    // padding frame
+    0x00,
+  };
+  // clang-format on
+  EXPECT_FALSE(framer_.ProcessPacket(
+      QuicEncryptedPacket(AsChars(packet), QUIC_ARRAYSIZE(packet), false)));
+  EXPECT_EQ(QUIC_INVALID_PACKET_HEADER, framer_.error());
+  if (!QuicUtils::VariableLengthConnectionIdAllowedForVersion(
+          framer_.transport_version())) {
+    EXPECT_EQ("Invalid ConnectionId length.", framer_.detailed_error());
+  } else {
+    EXPECT_EQ("Client connection ID not supported yet.",
+              framer_.detailed_error());
+  }
+}
+
 }  // namespace
 }  // namespace test
 }  // namespace quic