fix deadlock when receiving two packets with diversification nonces

internal/handshake/crypto_setup_client.go

@@ -38,7 +38,7 @@ type cryptoSetupClient struct {
 	lastSentCHLO     []byte
 	certManager      crypto.CertManager
 
-	divNonceChan         <-chan []byte
+	divNonceChan         chan struct{}
 	diversificationNonce []byte
 
 	clientHelloCounter int
@@ -79,12 +79,12 @@ func NewCryptoSetupClient(
 	initialVersion protocol.VersionNumber,
 	negotiatedVersions []protocol.VersionNumber,
 	logger utils.Logger,
-) (CryptoSetup, chan<- []byte, error) {
+) (CryptoSetup, error) {
 	nullAEAD, err := crypto.NewNullAEAD(protocol.PerspectiveClient, connID, version)
 	if err != nil {
-		return nil, nil, err
+		return nil, err
 	}
-	divNonceChan := make(chan []byte)
+	divNonceChan := make(chan struct{})
 	cs := &cryptoSetupClient{
 		cryptoStream:       cryptoStream,
 		hostname:           hostname,
@@ -101,7 +101,7 @@ func NewCryptoSetupClient(
 		divNonceChan:       divNonceChan,
 		logger:             logger,
 	}
-	return cs, divNonceChan, nil
+	return cs, nil
 }
 
 func (h *cryptoSetupClient) HandleCryptoStream() error {
@@ -120,33 +120,26 @@ func (h *cryptoSetupClient) HandleCryptoStream() error {
 	}()
 
 	for {
-		err := h.maybeUpgradeCrypto()
-		if err != nil {
+		if err := h.maybeUpgradeCrypto(); err != nil {
 			return err
 		}
 
 		h.mutex.RLock()
 		sendCHLO := h.secureAEAD == nil
 		h.mutex.RUnlock()
-
 		if sendCHLO {
-			err = h.sendCHLO()
-			if err != nil {
+			if err := h.sendCHLO(); err != nil {
 				return err
 			}
 		}
 
 		var message HandshakeMessage
 		select {
-		case divNonce := <-h.divNonceChan:
-			if len(h.diversificationNonce) != 0 && !bytes.Equal(h.diversificationNonce, divNonce) {
-				return errConflictingDiversificationNonces
-			}
-			h.diversificationNonce = divNonce
+		case <-h.divNonceChan:
 			// there's no message to process, but we should try upgrading the crypto again
 			continue
 		case message = <-messageChan:
-		case err = <-errorChan:
+		case err := <-errorChan:
 			return err
 		}
 
@@ -386,6 +379,21 @@ func (h *cryptoSetupClient) ConnectionState() ConnectionState {
 	}
 }
 
+func (h *cryptoSetupClient) SetDiversificationNonce(divNonce []byte) error {
+	h.mutex.Lock()
+	if len(h.diversificationNonce) > 0 {
+		defer h.mutex.Unlock()
+		if !bytes.Equal(h.diversificationNonce, divNonce) {
+			return errConflictingDiversificationNonces
+		}
+		return nil
+	}
+	h.diversificationNonce = divNonce
+	h.mutex.Unlock()
+	h.divNonceChan <- struct{}{}
+	return nil
+}
+
 func (h *cryptoSetupClient) sendCHLO() error {
 	h.clientHelloCounter++
 	if h.clientHelloCounter > protocol.MaxClientHellos {

internal/handshake/crypto_setup_client_test.go

@@ -91,7 +91,6 @@ var _ = Describe("Client Crypto Setup", func() {
 		shloMap                 map[Tag][]byte
 		handshakeEvent          chan struct{}
 		paramsChan              chan TransportParameters
-		divNonceChan            chan<- []byte
 	)
 
 	BeforeEach(func() {
@@ -120,7 +119,7 @@ var _ = Describe("Client Crypto Setup", func() {
 		// use a buffered channel here, so that we can parse a SHLO without having to receive the TransportParameters to avoid blocking
 		paramsChan = make(chan TransportParameters, 1)
 		handshakeEvent = make(chan struct{}, 2)
-		csInt, dnc, err := NewCryptoSetupClient(
+		csInt, err := NewCryptoSetupClient(
 			stream,
 			"hostname",
 			protocol.ConnectionID{1, 2, 3, 4, 5, 6, 7, 8},
@@ -139,7 +138,6 @@ var _ = Describe("Client Crypto Setup", func() {
 		cs.keyDerivation = keyDerivation
 		cs.nullAEAD = mockcrypto.NewMockAEAD(mockCtrl)
 		cs.cryptoStream = stream
-		divNonceChan = dnc
 	})
 
 	Context("Reading REJ", func() {
@@ -717,16 +715,16 @@ var _ = Describe("Client Crypto Setup", func() {
 
 		It("tries to escalate the crypto after receiving a diversification nonce", func() {
 			done := make(chan struct{})
+			cs.diversificationNonce = nil
+			cs.serverVerified = true
 			go func() {
 				defer GinkgoRecover()
 				err := cs.HandleCryptoStream()
 				Expect(err).To(MatchError(qerr.Error(qerr.HandshakeFailed, errMockStreamClosing.Error())))
 				close(done)
 			}()
-			cs.diversificationNonce = nil
-			cs.serverVerified = true
 			Expect(cs.secureAEAD).To(BeNil())
-			divNonceChan <- []byte("div")
+			Expect(cs.SetDiversificationNonce([]byte("div"))).To(Succeed())
 			Eventually(handshakeEvent).Should(Receive())
 			Expect(cs.secureAEAD).ToNot(BeNil())
 			Expect(handshakeEvent).ToNot(Receive())
@@ -926,7 +924,7 @@ var _ = Describe("Client Crypto Setup", func() {
 				close(done)
 			}()
 			nonce := []byte("foobar")
-			divNonceChan <- nonce
+			Expect(cs.SetDiversificationNonce(nonce)).To(Succeed())
 			Eventually(func() []byte { return cs.diversificationNonce }).Should(Equal(nonce))
 			// make the go routine return
 			stream.close()
@@ -942,8 +940,8 @@ var _ = Describe("Client Crypto Setup", func() {
 				close(done)
 			}()
 			nonce := []byte("foobar")
-			divNonceChan <- nonce
-			divNonceChan <- nonce
+			Expect(cs.SetDiversificationNonce(nonce)).To(Succeed())
+			Expect(cs.SetDiversificationNonce(nonce)).To(Succeed())
 			Eventually(func() []byte { return cs.diversificationNonce }).Should(Equal(nonce))
 			// make the go routine return
 			stream.close()
@@ -955,13 +953,17 @@ var _ = Describe("Client Crypto Setup", func() {
 			go func() {
 				defer GinkgoRecover()
 				err := cs.HandleCryptoStream()
-				Expect(err).To(MatchError(errConflictingDiversificationNonces))
+				Expect(err).To(MatchError(qerr.Error(qerr.HandshakeFailed, errMockStreamClosing.Error())))
 				close(done)
 			}()
 			nonce1 := []byte("foobar")
 			nonce2 := []byte("raboof")
-			divNonceChan <- nonce1
-			divNonceChan <- nonce2
+			err := cs.SetDiversificationNonce(nonce1)
+			Expect(err).ToNot(HaveOccurred())
+			err = cs.SetDiversificationNonce(nonce2)
+			Expect(err).To(MatchError(errConflictingDiversificationNonces))
+			// make the go routine return
+			stream.close()
 			Eventually(done).Should(BeClosed())
 		})
 	})

packet_packer_test.go

@@ -27,6 +27,7 @@ type mockCryptoSetup struct {
 	handleErr          error
 	encLevelSeal       protocol.EncryptionLevel
 	encLevelSealCrypto protocol.EncryptionLevel
+	divNonce           []byte
 }
 
 var _ handshake.CryptoSetup = &mockCryptoSetup{}
@@ -46,6 +47,10 @@ func (m *mockCryptoSetup) GetSealerForCryptoStream() (protocol.EncryptionLevel,
 func (m *mockCryptoSetup) GetSealerWithEncryptionLevel(protocol.EncryptionLevel) (handshake.Sealer, error) {
 	return &mockSealer{}, nil
 }
+func (m *mockCryptoSetup) SetDiversificationNonce(divNonce []byte) error {
+	m.divNonce = divNonce
+	return nil
+}
 func (m *mockCryptoSetup) ConnectionState() ConnectionState { panic("not implemented") }
 
 var _ = Describe("Packet packer", func() {

session.go

@@ -50,6 +50,10 @@ type cryptoStreamHandler interface {
 	ConnectionState() handshake.ConnectionState
 }
 
+type divNonceSetter interface {
+	SetDiversificationNonce([]byte) error
+}
+
 type receivedPacket struct {
 	remoteAddr net.Addr
 	header     *wire.Header
@@ -93,7 +97,6 @@ type session struct {
 	packer   *packetPacker
 
 	cryptoStreamHandler cryptoStreamHandler
-	divNonceChan        chan<- []byte // only set for the client
 
 	receivedPackets  chan *receivedPacket
 	sendingScheduled chan struct{}
@@ -248,7 +251,7 @@ var newClientSession = func(
 		IdleTimeout:                 s.config.IdleTimeout,
 		OmitConnectionID:            s.config.RequestConnectionIDOmission,
 	}
-	cs, divNonceChan, err := newCryptoSetupClient(
+	cs, err := newCryptoSetupClient(
 		s.cryptoStream,
 		hostname,
 		connectionID,
@@ -265,7 +268,6 @@ var newClientSession = func(
 		return nil, err
 	}
 	s.cryptoStreamHandler = cs
-	s.divNonceChan = divNonceChan
 	s.unpacker = newPacketUnpackerGQUIC(cs, s.version)
 	s.streamsMap = newStreamsMapLegacy(s.newStream, s.config.MaxIncomingStreams, s.perspective)
 	s.streamFramer = newStreamFramer(s.cryptoStream, s.streamsMap, s.version)
@@ -449,6 +451,8 @@ runLoop:
 		case _, ok := <-s.handshakeEvent:
 			// when the handshake is completed, the channel will be closed
 			s.handleHandshakeEvent(!ok)
+		// case p := <-s.paramsChan:
+		// 	s.processTransportParameters(&p)
 		default:
 		}
 
@@ -589,7 +593,9 @@ func (s *session) handleHandshakeEvent(completed bool) {
 func (s *session) handlePacketImpl(p *receivedPacket) error {
 	if s.perspective == protocol.PerspectiveClient {
 		if divNonce := p.header.DiversificationNonce; len(divNonce) > 0 {
-			s.divNonceChan <- divNonce
+			if err := s.cryptoStreamHandler.(divNonceSetter).SetDiversificationNonce(divNonce); err != nil {
+				return err
+			}
 		}
 	}
 

session_test.go

@@ -1713,7 +1713,6 @@ var _ = Describe("Client Session", func() {
 		sess          *session
 		mconn         *mockConnection
 		handshakeChan chan<- struct{}
-		divNonceChan  chan []byte
 
 		cryptoSetup *mockCryptoSetup
 	)
@@ -1722,7 +1721,6 @@ var _ = Describe("Client Session", func() {
 		Eventually(areSessionsRunning).Should(BeFalse())
 
 		cryptoSetup = &mockCryptoSetup{}
-		divNonceChan = make(chan []byte, 1)
 		newCryptoSetupClient = func(
 			_ io.ReadWriter,
 			_ string,
@@ -1735,9 +1733,9 @@ var _ = Describe("Client Session", func() {
 			_ protocol.VersionNumber,
 			_ []protocol.VersionNumber,
 			_ utils.Logger,
-		) (handshake.CryptoSetup, chan<- []byte, error) {
+		) (handshake.CryptoSetup, error) {
 			handshakeChan = handshakeChanP
-			return cryptoSetup, divNonceChan, nil
+			return cryptoSetup, nil
 		}
 
 		mconn = newMockConnection()
@@ -1784,6 +1782,8 @@ var _ = Describe("Client Session", func() {
 		})
 
 		It("passes the diversification nonce to the crypto setup", func() {
+			cryptoSetup := &mockCryptoSetup{}
+			sess.cryptoStreamHandler = cryptoSetup
 			unpacker := NewMockUnpacker(mockCtrl)
 			unpacker.EXPECT().Unpack(gomock.Any(), gomock.Any(), gomock.Any()).Return(&unpackedPacket{}, nil)
 			sess.unpacker = unpacker
@@ -1798,7 +1798,7 @@ var _ = Describe("Client Session", func() {
 			hdr.DiversificationNonce = []byte("foobar")
 			err := sess.handlePacketImpl(&receivedPacket{header: hdr})
 			Expect(err).ToNot(HaveOccurred())
-			Expect(divNonceChan).To(Receive(Equal(hdr.DiversificationNonce)))
+			Expect(cryptoSetup.divNonce).To(Equal(hdr.DiversificationNonce))
 			Expect(sess.Close(nil)).To(Succeed())
 			Eventually(done).Should(BeClosed())
 		})