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