From c225299c844142710d7ca2f3b43d128a17ed91a1 Mon Sep 17 00:00:00 2001
From: Marten Seemann <martenseemann@gmail.com>
Date: Thu, 19 May 2022 12:29:12 +0200
Subject: [PATCH] handle TLS errors that occur before the ClientHello has been
 written

---
 internal/handshake/crypto_setup.go      | 32 +++++++++++++++-----
 internal/handshake/crypto_setup_test.go | 40 ++++++++++++++++++++++++-
 2 files changed, 64 insertions(+), 8 deletions(-)

diff --git a/internal/handshake/crypto_setup.go b/internal/handshake/crypto_setup.go
index 51bd1a44a..25b35cffc 100644
--- a/internal/handshake/crypto_setup.go
+++ b/internal/handshake/crypto_setup.go
@@ -113,7 +113,8 @@ type cryptoSetup struct {
 
 	zeroRTTParameters      *wire.TransportParameters
 	clientHelloWritten     bool
-	clientHelloWrittenChan chan<- *wire.TransportParameters
+	clientHelloWrittenChan chan struct{} // is closed as soon as the ClientHello is written
+	zeroRTTParametersChan  chan<- *wire.TransportParameters
 
 	rttStats *utils.RTTStats
 
@@ -238,7 +239,7 @@ func newCryptoSetup(
 		tracer.UpdatedKeyFromTLS(protocol.EncryptionInitial, protocol.PerspectiveServer)
 	}
 	extHandler := newExtensionHandler(tp.Marshal(perspective), perspective, version)
-	clientHelloWrittenChan := make(chan *wire.TransportParameters, 1)
+	zeroRTTParametersChan := make(chan *wire.TransportParameters, 1)
 	cs := &cryptoSetup{
 		tlsConf:                   tlsConf,
 		initialStream:             initialStream,
@@ -257,7 +258,8 @@ func newCryptoSetup(
 		perspective:               perspective,
 		handshakeDone:             make(chan struct{}),
 		alertChan:                 make(chan uint8),
-		clientHelloWrittenChan:    clientHelloWrittenChan,
+		clientHelloWrittenChan:    make(chan struct{}),
+		zeroRTTParametersChan:     zeroRTTParametersChan,
 		messageChan:               make(chan []byte, 100),
 		isReadingHandshakeMessage: make(chan struct{}),
 		closeChan:                 make(chan struct{}),
@@ -279,7 +281,7 @@ func newCryptoSetup(
 		GetAppDataForSessionState:  cs.marshalDataForSessionState,
 		SetAppDataFromSessionState: cs.handleDataFromSessionState,
 	}
-	return cs, clientHelloWrittenChan
+	return cs, zeroRTTParametersChan
 }
 
 func (h *cryptoSetup) ChangeConnectionID(id protocol.ConnectionID) {
@@ -309,6 +311,15 @@ func (h *cryptoSetup) RunHandshake() {
 		close(handshakeComplete)
 	}()
 
+	if h.perspective == protocol.PerspectiveClient {
+		select {
+		case err := <-handshakeErrChan:
+			h.onError(0, err.Error())
+			return
+		case <-h.clientHelloWrittenChan:
+		}
+	}
+
 	select {
 	case <-handshakeComplete: // return when the handshake is done
 		h.mutex.Lock()
@@ -325,7 +336,13 @@ func (h *cryptoSetup) RunHandshake() {
 }
 
 func (h *cryptoSetup) onError(alert uint8, message string) {
-	h.runner.OnError(qerr.NewCryptoError(alert, message))
+	var err error
+	if alert == 0 {
+		err = &qerr.TransportError{ErrorCode: qerr.InternalError, ErrorMessage: message}
+	} else {
+		err = qerr.NewCryptoError(alert, message)
+	}
+	h.runner.OnError(err)
 }
 
 // Close closes the crypto setup.
@@ -646,12 +663,13 @@ func (h *cryptoSetup) WriteRecord(p []byte) (int, error) {
 		n, err := h.initialStream.Write(p)
 		if !h.clientHelloWritten && h.perspective == protocol.PerspectiveClient {
 			h.clientHelloWritten = true
+			close(h.clientHelloWrittenChan)
 			if h.zeroRTTSealer != nil && h.zeroRTTParameters != nil {
 				h.logger.Debugf("Doing 0-RTT.")
-				h.clientHelloWrittenChan <- h.zeroRTTParameters
+				h.zeroRTTParametersChan <- h.zeroRTTParameters
 			} else {
 				h.logger.Debugf("Not doing 0-RTT.")
-				h.clientHelloWrittenChan <- nil
+				h.zeroRTTParametersChan <- nil
 			}
 		}
 		return n, err
diff --git a/internal/handshake/crypto_setup_test.go b/internal/handshake/crypto_setup_test.go
index 5720321a8..4717c1815 100644
--- a/internal/handshake/crypto_setup_test.go
+++ b/internal/handshake/crypto_setup_test.go
@@ -107,7 +107,7 @@ var _ = Describe("Crypto Setup TLS", func() {
 			defer GinkgoRecover()
 			server.RunHandshake()
 			Expect(sErrChan).To(Receive(MatchError(&qerr.TransportError{
-				ErrorCode:    0x10a,
+				ErrorCode:    0x100 + qerr.TransportErrorCode(alertUnexpectedMessage),
 				ErrorMessage: "local error: tls: unexpected message",
 			})))
 			close(done)
@@ -124,6 +124,44 @@ var _ = Describe("Crypto Setup TLS", func() {
 		Eventually(done).Should(BeClosed())
 	})
 
+	It("handles qtls errors occurring before during ClientHello generation", func() {
+		sErrChan := make(chan error, 1)
+		runner := NewMockHandshakeRunner(mockCtrl)
+		runner.EXPECT().OnError(gomock.Any()).Do(func(e error) { sErrChan <- e })
+		_, sInitialStream, sHandshakeStream := initStreams()
+		tlsConf := testdata.GetTLSConfig()
+		tlsConf.InsecureSkipVerify = true
+		tlsConf.NextProtos = []string{""}
+		cl, _ := NewCryptoSetupClient(
+			sInitialStream,
+			sHandshakeStream,
+			protocol.ConnectionID{},
+			nil,
+			nil,
+			&wire.TransportParameters{},
+			runner,
+			tlsConf,
+			false,
+			&utils.RTTStats{},
+			nil,
+			utils.DefaultLogger.WithPrefix("client"),
+			protocol.VersionTLS,
+		)
+
+		done := make(chan struct{})
+		go func() {
+			defer GinkgoRecover()
+			cl.RunHandshake()
+			close(done)
+		}()
+
+		Eventually(done).Should(BeClosed())
+		Expect(sErrChan).To(Receive(MatchError(&qerr.TransportError{
+			ErrorCode:    qerr.InternalError,
+			ErrorMessage: "tls: invalid NextProtos value",
+		})))
+	})
+
 	It("errors when a message is received at the wrong encryption level", func() {
 		sErrChan := make(chan error, 1)
 		_, sInitialStream, sHandshakeStream := initStreams()