Repository: quic-go/quic-go
Pull request #4031 (2023-08-16 02:51:21)

handshake: use the correct hash function for TLS_AES_256_GCM_SHA384
TLS_AES_128_GCM_SHA256 and TLS_CHACHA20_POLY1305_SHA256 work fine, but if TLS happens to select TLS_AES_256_GCM_SHA384 during the handshake, we're deriving the wrong keys, causing a handshake failure.

Thanks to @MarcoPolo for discovering this when interop-ing with zig-libp2p.


Patch:
diff --git a/internal/handshake/cipher_suite.go b/internal/handshake/cipher_suite.go
index 608d5ea0..265231f0 100644
--- a/internal/handshake/cipher_suite.go
+++ b/internal/handshake/cipher_suite.go
@@ -30,7 +30,7 @@ func getCipherSuite(id uint16) *cipherSuite {
 	case tls.TLS_CHACHA20_POLY1305_SHA256:
 		return &cipherSuite{ID: tls.TLS_CHACHA20_POLY1305_SHA256, Hash: crypto.SHA256, KeyLen: 32, AEAD: aeadChaCha20Poly1305}
 	case tls.TLS_AES_256_GCM_SHA384:
-		return &cipherSuite{ID: tls.TLS_AES_256_GCM_SHA384, Hash: crypto.SHA256, KeyLen: 32, AEAD: aeadAESGCMTLS13}
+		return &cipherSuite{ID: tls.TLS_AES_256_GCM_SHA384, Hash: crypto.SHA384, KeyLen: 32, AEAD: aeadAESGCMTLS13}
 	default:
 		panic(fmt.Sprintf("unknown cypher suite: %d", id))
 	}
diff --git a/internal/handshake/crypto_setup.go b/internal/handshake/crypto_setup.go
index 35d65b5e..a1179257 100644
--- a/internal/handshake/crypto_setup.go
+++ b/internal/handshake/crypto_setup.go
@@ -7,6 +7,7 @@ import (
 	"errors"
 	"fmt"
 	"net"
+	"strings"
 	"sync"
 	"sync/atomic"
 	"time"
@@ -356,10 +357,15 @@ func (h *cryptoSetup) getDataForSessionTicket() []byte {
 // Due to limitations in crypto/tls, it's only possible to generate a single session ticket per connection.
 // It is only valid for the server.
 func (h *cryptoSetup) GetSessionTicket() ([]byte, error) {
-	if h.tlsConf.SessionTicketsDisabled {
-		return nil, nil
-	}
 	if err := qtls.SendSessionTicket(h.conn, h.allow0RTT); err != nil {
+		// Session tickets might be disabled by tls.Config.SessionTicketsDisabled.
+		// We can't check h.tlsConfig here, since the actual config might have been obtained from
+		// the GetConfigForClient callback.
+		// See https://github.com/golang/go/issues/62032.
+		// Once that issue is resolved, this error assertion can be removed.
+		if strings.Contains(err.Error(), "session ticket keys unavailable") {
+			return nil, nil
+		}
 		return nil, err
 	}
 	ev := h.conn.NextEvent()


Test patch:
diff --git a/integrationtests/self/resumption_test.go b/integrationtests/self/resumption_test.go
index 264f832e..bedf0470 100644
--- a/integrationtests/self/resumption_test.go
+++ b/integrationtests/self/resumption_test.go
@@ -56,7 +56,7 @@ var _ = Describe("TLS session resumption", func() {
 			context.Background(),
 			fmt.Sprintf("localhost:%d", server.Addr().(*net.UDPAddr).Port),
 			tlsConf,
-			nil,
+			getQuicConfig(nil),
 		)
 		Expect(err).ToNot(HaveOccurred())
 		var sessionKey string
@@ -71,7 +71,7 @@ var _ = Describe("TLS session resumption", func() {
 			context.Background(),
 			fmt.Sprintf("localhost:%d", server.Addr().(*net.UDPAddr).Port),
 			tlsConf,
-			nil,
+			getQuicConfig(nil),
 		)
 		Expect(err).ToNot(HaveOccurred())
 		Expect(gets).To(Receive(Equal(sessionKey)))
@@ -85,7 +85,7 @@ var _ = Describe("TLS session resumption", func() {
 	It("doesn't use session resumption, if the config disables it", func() {
 		sConf := getTLSConfig()
 		sConf.SessionTicketsDisabled = true
-		server, err := quic.ListenAddr("localhost:0", sConf, nil)
+		server, err := quic.ListenAddr("localhost:0", sConf, getQuicConfig(nil))
 		Expect(err).ToNot(HaveOccurred())
 		defer server.Close()
 
@@ -98,7 +98,7 @@ var _ = Describe("TLS session resumption", func() {
 			context.Background(),
 			fmt.Sprintf("localhost:%d", server.Addr().(*net.UDPAddr).Port),
 			tlsConf,
-			nil,
+			getQuicConfig(nil),
 		)
 		Expect(err).ToNot(HaveOccurred())
 		Consistently(puts).ShouldNot(Receive())
@@ -114,7 +114,55 @@ var _ = Describe("TLS session resumption", func() {
 			context.Background(),
 			fmt.Sprintf("localhost:%d", server.Addr().(*net.UDPAddr).Port),
 			tlsConf,
-			nil,
+			getQuicConfig(nil),
+		)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(conn.ConnectionState().TLS.DidResume).To(BeFalse())
+
+		serverConn, err = server.Accept(context.Background())
+		Expect(err).ToNot(HaveOccurred())
+		Expect(serverConn.ConnectionState().TLS.DidResume).To(BeFalse())
+	})
+
+	It("doesn't use session resumption, if the config returned by GetConfigForClient disables it", func() {
+		sConf := &tls.Config{
+			GetConfigForClient: func(*tls.ClientHelloInfo) (*tls.Config, error) {
+				conf := getTLSConfig()
+				conf.SessionTicketsDisabled = true
+				return conf, nil
+			},
+		}
+
+		server, err := quic.ListenAddr("localhost:0", sConf, getQuicConfig(nil))
+		Expect(err).ToNot(HaveOccurred())
+		defer server.Close()
+
+		gets := make(chan string, 100)
+		puts := make(chan string, 100)
+		cache := newClientSessionCache(tls.NewLRUClientSessionCache(10), gets, puts)
+		tlsConf := getTLSClientConfig()
+		tlsConf.ClientSessionCache = cache
+		conn, err := quic.DialAddr(
+			context.Background(),
+			fmt.Sprintf("localhost:%d", server.Addr().(*net.UDPAddr).Port),
+			tlsConf,
+			getQuicConfig(nil),
+		)
+		Expect(err).ToNot(HaveOccurred())
+		Consistently(puts).ShouldNot(Receive())
+		Expect(conn.ConnectionState().TLS.DidResume).To(BeFalse())
+
+		ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
+		defer cancel()
+		serverConn, err := server.Accept(ctx)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(serverConn.ConnectionState().TLS.DidResume).To(BeFalse())
+
+		conn, err = quic.DialAddr(
+			context.Background(),
+			fmt.Sprintf("localhost:%d", server.Addr().(*net.UDPAddr).Port),
+			tlsConf,
+			getQuicConfig(nil),
 		)
 		Expect(err).ToNot(HaveOccurred())
 		Expect(conn.ConnectionState().TLS.DidResume).To(BeFalse())