commit e0df3f7007a2d0bc68d3bcde617936a652377c75
author dschinazi <dschinazi@google.com> Mon May 06 16:37:51 2019 -0700
committer Copybara-Service <copybara-worker@google.com> Tue May 07 07:21:42 2019 -0700
tree 28d83099d11f582bdd3c2d9171ee7d1e315f5b42
parent 244f6dcc1f007cb0517b5f659ec8a9f6dce2b562

Make client drop packets with wrong versions

In Google QUIC, packets with the version flag set and a different version number were version negotiation packets. In IETF QUIC, version negotiation packets have their own format, and it is now possible for a client to receive a packet that is not a version negotiation packet but still has a version different from what it expects. Those packets must be dropped.

This issue was found by Chromium clusterfuzz:
https://bugs.chromium.org/p/chromium/issues/detail?id=959143
I was able to reproduce the fuzzer DCHECK failure and verify that the fix prevents it.

gfe-relnote: client-only change to handling of packets with invalid versions, not flag protected
PiperOrigin-RevId: 246921247
Change-Id: I6610c9cd8c667bfad62fd335cc7f45fc425d3d2e

quic/core/quic_framer.cc

diff --git a/quic/core/quic_framer.cc b/quic/core/quic_framer.cc
index 55208e9..0e026c6 100644
--- a/quic/core/quic_framer.cc
+++ b/quic/core/quic_framer.cc
@@ -1498,19 +1498,30 @@
     return true;
   }
 
-  if (perspective_ == Perspective::IS_SERVER && header.version_flag &&
-      header.version != version_) {
-    if (!visitor_->OnProtocolVersionMismatch(header.version, header.form)) {
-      RecordDroppedPacketReason(DroppedPacketReason::VERSION_MISMATCH);
-      return true;
+  if (IsVersionNegotiation(header, packet_has_ietf_packet_header)) {
+    QUIC_DVLOG(1) << ENDPOINT << "Received version negotiation packet";
+    return ProcessVersionNegotiationPacket(&reader, header);
+  }
+
+  if (header.version_flag && header.version != version_) {
+    if (perspective_ == Perspective::IS_SERVER) {
+      if (!visitor_->OnProtocolVersionMismatch(header.version, header.form)) {
+        RecordDroppedPacketReason(DroppedPacketReason::VERSION_MISMATCH);
+        return true;
+      }
+    } else {
+      // A client received a packet of a different version but that packet is
+      // not a version negotiation packet. It is therefore invalid and dropped.
+      QUIC_DLOG(ERROR) << "Client received unexpected version "
+                       << ParsedQuicVersionToString(header.version)
+                       << " instead of " << ParsedQuicVersionToString(version_);
+      set_detailed_error("Client received unexpected version.");
+      return RaiseError(QUIC_INVALID_VERSION);
     }
   }
 
   bool rv;
-  if (IsVersionNegotiation(header, packet_has_ietf_packet_header)) {
-    QUIC_DVLOG(1) << ENDPOINT << "Received version negotiation packet";
-    rv = ProcessVersionNegotiationPacket(&reader, header);
-  } else if (header.long_packet_type == RETRY) {
+  if (header.long_packet_type == RETRY) {
     rv = ProcessRetryPacket(&reader, header);
   } else if (header.reset_flag) {
     rv = ProcessPublicResetPacket(&reader, header);

quic/core/quic_framer_test.cc

diff --git a/quic/core/quic_framer_test.cc b/quic/core/quic_framer_test.cc
index 56665df..0b5dca5 100644
--- a/quic/core/quic_framer_test.cc
+++ b/quic/core/quic_framer_test.cc
@@ -12938,6 +12938,36 @@
   ASSERT_EQ(visitor_.coalesced_packets_.size(), 0u);
 }
 
+TEST_P(QuicFramerTest, ClientReceivesInvalidVersion) {
+  if (framer_.transport_version() < QUIC_VERSION_44) {
+    return;
+  }
+  QuicFramerPeer::SetPerspective(&framer_, Perspective::IS_CLIENT);
+
+  // clang-format off
+  unsigned char packet[] = {
+       // public flags (long header with packet type INITIAL)
+       0xFF,
+       // version that is different from the framer's version
+       'Q', '0', '4', '3',
+       // connection ID lengths
+       0x05,
+       // source connection ID
+       0xFE, 0xDC, 0xBA, 0x98, 0x76, 0x54, 0x32, 0x10,
+       // packet number
+       0x01,
+       // padding frame
+       0x00,
+  };
+  // clang-format on
+
+  QuicEncryptedPacket encrypted(AsChars(packet), QUIC_ARRAYSIZE(packet), false);
+  EXPECT_FALSE(framer_.ProcessPacket(encrypted));
+
+  EXPECT_EQ(QUIC_INVALID_VERSION, framer_.error());
+  EXPECT_EQ("Client received unexpected version.", framer_.detailed_error());
+}
+
 TEST_P(QuicFramerTest, PacketHeaderWithVariableLengthConnectionId) {
   if (framer_.transport_version() < QUIC_VERSION_46) {
     return;