From 5b7b9c84d4c1f26e970191c97e2bc134414a8379 Mon Sep 17 00:00:00 2001
From: Marten Seemann <martenseemann@gmail.com>
Date: Tue, 14 Apr 2020 17:39:13 +0700
Subject: [PATCH] qlog dropped version negotiation packets

---
 client.go      |  9 ++++++
 client_test.go | 79 ++++++++++++++++++++++++++++++--------------------
 2 files changed, 57 insertions(+), 31 deletions(-)

diff --git a/client.go b/client.go
index 37e050063..6ddb87c1b 100644
--- a/client.go
+++ b/client.go
@@ -330,18 +330,27 @@ func (c *client) handleVersionNegotiationPacket(p *receivedPacket) {
 
 	hdr, _, _, err := wire.ParsePacket(p.data, 0)
 	if err != nil {
+		if c.qlogger != nil {
+			c.qlogger.DroppedPacket(qlog.PacketTypeVersionNegotiation, protocol.ByteCount(len(p.data)), qlog.PacketDropHeaderParseError)
+		}
 		c.logger.Debugf("Error parsing Version Negotiation packet: %s", err)
 		return
 	}
 
 	// ignore delayed / duplicated version negotiation packets
 	if c.receivedVersionNegotiationPacket || c.versionNegotiated.Get() {
+		if c.qlogger != nil {
+			c.qlogger.DroppedPacket(qlog.PacketTypeVersionNegotiation, protocol.ByteCount(len(p.data)), qlog.PacketDropUnexpectedPacket)
+		}
 		c.logger.Debugf("Received a delayed Version Negotiation packet.")
 		return
 	}
 
 	for _, v := range hdr.SupportedVersions {
 		if v == c.version {
+			if c.qlogger != nil {
+				c.qlogger.DroppedPacket(qlog.PacketTypeVersionNegotiation, protocol.ByteCount(len(p.data)), qlog.PacketDropUnexpectedVersion)
+			}
 			// The Version Negotiation packet contains the version that we offered.
 			// This might be a packet sent by an attacker (or by a terribly broken server implementation).
 			return
diff --git a/client_test.go b/client_test.go
index 1ca145a8c..e15848306 100644
--- a/client_test.go
+++ b/client_test.go
@@ -714,10 +714,57 @@ var _ = Describe("Client", func() {
 				Expect(cl.version).To(Equal(protocol.VersionNumber(1234)))
 			})
 
+			It("drops unparseable version negotiation packets", func() {
+				cl.config = config
+				ver := cl.version
+				p := composeVersionNegotiationPacket(connID, []protocol.VersionNumber{ver})
+				p.data = p.data[:len(p.data)-1]
+				done := make(chan struct{})
+				qlogger.EXPECT().DroppedPacket(qlog.PacketTypeVersionNegotiation, protocol.ByteCount(len(p.data)), qlog.PacketDropHeaderParseError).Do(func(qlog.PacketType, protocol.ByteCount, qlog.PacketDropReason) {
+					close(done)
+				})
+				cl.handlePacket(p)
+				Eventually(done).Should(BeClosed())
+				Expect(cl.version).To(Equal(ver))
+			})
+
+			It("drops version negotiation packets if any other packet was received before", func() {
+				sess := NewMockQuicSession(mockCtrl)
+				sess.EXPECT().handlePacket(gomock.Any())
+				cl.session = sess
+				cl.config = config
+				buf := &bytes.Buffer{}
+				Expect((&wire.ExtendedHeader{
+					Header: wire.Header{
+						DestConnectionID: connID,
+						SrcConnectionID:  connID,
+						Version:          cl.version,
+					},
+					PacketNumberLen: protocol.PacketNumberLen3,
+				}).Write(buf, protocol.VersionTLS)).To(Succeed())
+				cl.handlePacket(&receivedPacket{data: buf.Bytes()})
+
+				ver := cl.version
+				p := composeVersionNegotiationPacket(connID, []protocol.VersionNumber{1234})
+				done := make(chan struct{})
+				qlogger.EXPECT().DroppedPacket(qlog.PacketTypeVersionNegotiation, protocol.ByteCount(len(p.data)), qlog.PacketDropUnexpectedPacket).Do(func(qlog.PacketType, protocol.ByteCount, qlog.PacketDropReason) {
+					close(done)
+				})
+				cl.handlePacket(p)
+				Eventually(done).Should(BeClosed())
+				Expect(cl.version).To(Equal(ver))
+			})
+
 			It("drops version negotiation packets that contain the offered version", func() {
 				cl.config = config
 				ver := cl.version
-				cl.handlePacket(composeVersionNegotiationPacket(connID, []protocol.VersionNumber{ver}))
+				p := composeVersionNegotiationPacket(connID, []protocol.VersionNumber{ver})
+				done := make(chan struct{})
+				qlogger.EXPECT().DroppedPacket(qlog.PacketTypeVersionNegotiation, protocol.ByteCount(len(p.data)), qlog.PacketDropUnexpectedVersion).Do(func(qlog.PacketType, protocol.ByteCount, qlog.PacketDropReason) {
+					close(done)
+				})
+				cl.handlePacket(p)
+				Eventually(done).Should(BeClosed())
 				Expect(cl.version).To(Equal(ver))
 			})
 		})
@@ -727,34 +774,4 @@ var _ = Describe("Client", func() {
 		Expect(cl.version).ToNot(BeZero())
 		Expect(cl.GetVersion()).To(Equal(cl.version))
 	})
-
-	Context("handling potentially injected packets", func() {
-		// NOTE: We hope these tests as written will fail once mitigations for injection adversaries are put in place.
-
-		// Illustrates that adversary who injects any packet quickly can
-		// cause a real version negotiation packet to be ignored.
-		It("version negotiation packets ignored if any other packet is received", func() {
-			// Copy of existing test "recognizes that a non Version Negotiation packet means that the server accepted the suggested version"
-			sess := NewMockQuicSession(mockCtrl)
-			sess.EXPECT().handlePacket(gomock.Any())
-			cl.session = sess
-			cl.config = config
-			buf := &bytes.Buffer{}
-			Expect((&wire.ExtendedHeader{
-				Header: wire.Header{
-					DestConnectionID: connID,
-					SrcConnectionID:  connID,
-					Version:          cl.version,
-				},
-				PacketNumberLen: protocol.PacketNumberLen3,
-			}).Write(buf, protocol.VersionTLS)).To(Succeed())
-			cl.handlePacket(&receivedPacket{data: buf.Bytes()})
-
-			// Version negotiation is now ignored
-			cl.config = config
-			ver := cl.version
-			cl.handlePacket(composeVersionNegotiationPacket(connID, []protocol.VersionNumber{1234}))
-			Expect(cl.version).To(Equal(ver))
-		})
-	})
 })