From 6af7df436a4a35366acbfc449ddf02d2fe7e8c2a Mon Sep 17 00:00:00 2001
From: Marten Seemann <martenseemann@gmail.com>
Date: Sun, 30 Jun 2019 18:35:30 +0700
Subject: [PATCH] don't close the session when unpacking a packet fails

---
 session.go      |  8 ++++----
 session_test.go | 32 ++++++++++++++++++++++++++++----
 2 files changed, 32 insertions(+), 8 deletions(-)

diff --git a/session.go b/session.go
index 9204e4ba4..018127161 100644
--- a/session.go
+++ b/session.go
@@ -598,12 +598,12 @@ func (s *session) handleSinglePacket(p *receivedPacket, hdr *wire.Header) bool /
 			// Try again later.
 			wasQueued = true
 			s.tryQueueingUndecryptablePacket(p)
-		case handshake.ErrDecryptionFailed:
+		case wire.ErrInvalidReservedBits:
+			s.closeLocal(qerr.Error(qerr.ProtocolViolation, err.Error()))
+		default:
 			// This might be a packet injected by an attacker.
 			// Drop it.
-			s.logger.Debugf("Dropping packet that could not be unpacked.")
-		default:
-			s.closeLocal(err)
+			s.logger.Debugf("Dropping packet that could not be unpacked. Error: %s", err)
 		}
 		return false
 	}
diff --git a/session_test.go b/session_test.go
index d89d6f4aa..e8dccafaa 100644
--- a/session_test.go
+++ b/session_test.go
@@ -565,9 +565,8 @@ var _ = Describe("Session", func() {
 			Eventually(sess.Context().Done()).Should(BeClosed())
 		})
 
-		It("closes the session when unpacking fails for any other reason than a decryption error", func() {
-			testErr := errors.New("test error")
-			unpacker.EXPECT().Unpack(gomock.Any(), gomock.Any()).Return(nil, testErr)
+		It("closes the session when unpacking fails because the reserved bits were incorrect", func() {
+			unpacker.EXPECT().Unpack(gomock.Any(), gomock.Any()).Return(nil, wire.ErrInvalidReservedBits)
 			streamManager.EXPECT().CloseWithError(gomock.Any())
 			cryptoSetup.EXPECT().Close()
 			packer.EXPECT().PackConnectionClose(gomock.Any()).Return(&packedPacket{}, nil)
@@ -575,7 +574,9 @@ var _ = Describe("Session", func() {
 			go func() {
 				defer GinkgoRecover()
 				cryptoSetup.EXPECT().RunHandshake().MaxTimes(1)
-				Expect(sess.run()).To(MatchError(testErr))
+				err := sess.run()
+				Expect(err).To(HaveOccurred())
+				Expect(err.(*qerr.QuicError).ErrorCode).To(Equal(qerr.ProtocolViolation))
 				close(done)
 			}()
 			sessionRunner.EXPECT().Retire(gomock.Any())
@@ -586,6 +587,29 @@ var _ = Describe("Session", func() {
 			Eventually(sess.Context().Done()).Should(BeClosed())
 		})
 
+		It("ignores packets when unpacking fails for any other reason", func() {
+			testErr := errors.New("test err")
+			unpacker.EXPECT().Unpack(gomock.Any(), gomock.Any()).Return(nil, testErr)
+			streamManager.EXPECT().CloseWithError(gomock.Any())
+			cryptoSetup.EXPECT().Close()
+			packer.EXPECT().PackConnectionClose(gomock.Any()).Return(&packedPacket{}, nil)
+			runErr := make(chan error)
+			go func() {
+				defer GinkgoRecover()
+				cryptoSetup.EXPECT().RunHandshake().MaxTimes(1)
+				runErr <- sess.run()
+			}()
+			sessionRunner.EXPECT().Retire(gomock.Any())
+			sess.handlePacket(getPacket(&wire.ExtendedHeader{
+				Header:          wire.Header{DestConnectionID: sess.srcConnID},
+				PacketNumberLen: protocol.PacketNumberLen1,
+			}, nil))
+			Consistently(runErr).ShouldNot(Receive())
+			// make the go routine return
+			sess.Close()
+			Eventually(sess.Context().Done()).Should(BeClosed())
+		})
+
 		It("rejects packets with empty payload", func() {
 			unpacker.EXPECT().Unpack(gomock.Any(), gomock.Any()).Return(&unpackedPacket{
 				hdr:  &wire.ExtendedHeader{},