From 6888eb859306b13605c43fd7586c086934ea33ec Mon Sep 17 00:00:00 2001
From: Marten Seemann <martenseemann@gmail.com>
Date: Sat, 1 Jun 2019 13:01:49 +0800
Subject: [PATCH] return an error when handling the NewSessionTicket failed

---
 internal/handshake/crypto_setup.go      | 24 ++++++++++-
 internal/handshake/crypto_setup_test.go | 53 +++++++++++++++++++++++++
 2 files changed, 76 insertions(+), 1 deletion(-)

diff --git a/internal/handshake/crypto_setup.go b/internal/handshake/crypto_setup.go
index d1e91ab5..91b076a9 100644
--- a/internal/handshake/crypto_setup.go
+++ b/internal/handshake/crypto_setup.go
@@ -434,7 +434,7 @@ func (h *cryptoSetup) handleMessageForClient(msgType messageType) bool {
 		return true
 	case typeNewSessionTicket:
 		<-h.handshakeDone // don't process session tickets before the handshake has completed
-		h.conn.HandlePostHandshakeMessage()
+		h.handleNewSessionTicket()
 		return false
 	default:
 		h.messageErrChan <- qerr.CryptoError(alertUnexpectedMessage, fmt.Sprintf("unexpected handshake message: %d", msgType))
@@ -442,6 +442,28 @@ func (h *cryptoSetup) handleMessageForClient(msgType messageType) bool {
 	}
 }
 
+func (h *cryptoSetup) handleNewSessionTicket() {
+	done := make(chan struct{})
+	defer close(done)
+
+	// h.alertChan is an unbuffered channel.
+	// If an error occurs during conn.HandlePostHandshakeMessage,
+	// it will be sent on this channel.
+	// Read it from a go-routine so that HandlePostHandshakeMessage doesn't deadlock.
+	alertChan := make(chan uint8, 1)
+	go func() {
+		select {
+		case alert := <-h.alertChan:
+			alertChan <- alert
+		case <-done:
+		}
+	}()
+
+	if err := h.conn.HandlePostHandshakeMessage(); err != nil {
+		h.runner.OnError(qerr.CryptoError(<-alertChan, err.Error()))
+	}
+}
+
 // ReadHandshakeMessage is called by TLS.
 // It blocks until a new handshake message is available.
 func (h *cryptoSetup) ReadHandshakeMessage() ([]byte, error) {
diff --git a/internal/handshake/crypto_setup_test.go b/internal/handshake/crypto_setup_test.go
index 05adec5f..f5549e67 100644
--- a/internal/handshake/crypto_setup_test.go
+++ b/internal/handshake/crypto_setup_test.go
@@ -466,5 +466,58 @@ var _ = Describe("Crypto Setup TLS", func() {
 			Expect(srvTP.Unmarshal(sTransportParametersRcvd, protocol.PerspectiveServer)).To(Succeed())
 			Expect(srvTP.IdleTimeout).To(Equal(sTransportParameters.IdleTimeout))
 		})
+
+		It("errors when handling the NewSessionTicket fails", func() {
+			cChunkChan, cInitialStream, cHandshakeStream := initStreams()
+			cRunner := NewMockHandshakeRunner(mockCtrl)
+			cRunner.EXPECT().OnReceivedParams(gomock.Any())
+			cRunner.EXPECT().OnHandshakeComplete()
+			client, _, err := NewCryptoSetupClient(
+				cInitialStream,
+				cHandshakeStream,
+				ioutil.Discard,
+				protocol.ConnectionID{},
+				nil,
+				&TransportParameters{},
+				cRunner,
+				clientConf,
+				utils.DefaultLogger.WithPrefix("client"),
+			)
+			Expect(err).ToNot(HaveOccurred())
+
+			sChunkChan, sInitialStream, sHandshakeStream := initStreams()
+			sRunner := NewMockHandshakeRunner(mockCtrl)
+			sRunner.EXPECT().OnReceivedParams(gomock.Any())
+			sRunner.EXPECT().OnHandshakeComplete()
+			server, err := NewCryptoSetupServer(
+				sInitialStream,
+				sHandshakeStream,
+				ioutil.Discard,
+				protocol.ConnectionID{},
+				nil,
+				&TransportParameters{},
+				sRunner,
+				testdata.GetTLSConfig(),
+				utils.DefaultLogger.WithPrefix("server"),
+			)
+			Expect(err).ToNot(HaveOccurred())
+
+			done := make(chan struct{})
+			go func() {
+				defer GinkgoRecover()
+				handshake(client, cChunkChan, server, sChunkChan)
+				close(done)
+			}()
+			Eventually(done).Should(BeClosed())
+
+			// inject an invalid session ticket
+			cRunner.EXPECT().OnError(gomock.Any()).Do(func(err error) {
+				Expect(err).To(BeAssignableToTypeOf(&qerr.QuicError{}))
+				qerr := err.(*qerr.QuicError)
+				Expect(qerr.IsCryptoError()).To(BeTrue())
+			})
+			b := append([]byte{uint8(typeNewSessionTicket), 0, 0, 6}, []byte("foobar")...)
+			client.HandleMessage(b, protocol.Encryption1RTT)
+		})
 	})
 })