From: Roland Shoemaker Date: Tue, 6 Jan 2026 22:36:01 +0000 (-0800) Subject: [release-branch.go1.24] crypto/tls: don't copy auto-rotated session ticket keys in... X-Git-Tag: go1.24.12~4 X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=d0754e6242e70e171a888b6c5e0336bbf014e538;p=gostls13.git [release-branch.go1.24] crypto/tls: don't copy auto-rotated session ticket keys in Config.Clone Once a tls.Config is used, it is not safe to mutate. We provide the Clone method in order to allow users to copy and modify a Config that is in use. If Config.SessionTicketKey is not populated, and if Config.SetSessionTicketKeys has not been called, we automatically populate and rotate session ticket keys. Clone was previously copying these keys into the new Config, meaning that two Configs could share the same auto-rotated session ticket keys. This could allow sessions to be resumed across different Configs, which may have completely different configurations. This change updates Clone to not copy the auto-rotated session ticket keys. Additionally, when resuming a session, check that not just that the leaf certificate is unexpired, but that the entire certificate chain is still unexpired. Fixes #77113 Fixes CVE-2025-68121 Change-Id: I011df7329de83068d11b3f0c793763692d018a98 Reviewed-on: https://go-internal-review.googlesource.com/c/go/+/3300 Reviewed-by: Damien Neil Reviewed-by: Nicholas Husin Reviewed-on: https://go-internal-review.googlesource.com/c/go/+/3322 Reviewed-by: Neal Patel Reviewed-on: https://go-review.googlesource.com/c/go/+/736700 TryBot-Bypass: Michael Pratt Reviewed-by: Junyang Shao Auto-Submit: Michael Pratt --- diff --git a/src/crypto/tls/common.go b/src/crypto/tls/common.go index d6942d2ef1..6b73796920 100644 --- a/src/crypto/tls/common.go +++ b/src/crypto/tls/common.go @@ -920,6 +920,10 @@ const maxSessionTicketLifetime = 7 * 24 * time.Hour // Clone returns a shallow clone of c or nil if c is nil. It is safe to clone a [Config] that is // being used concurrently by a TLS client or server. +// +// If Config.SessionTicketKey is unpopulated, and Config.SetSessionTicketKeys has not been +// called, the clone will not share the same auto-rotated session ticket keys as the original +// Config in order to prevent sessions from being resumed across Configs. func (c *Config) Clone() *Config { if c == nil { return nil @@ -959,7 +963,8 @@ func (c *Config) Clone() *Config { EncryptedClientHelloRejectionVerify: c.EncryptedClientHelloRejectionVerify, EncryptedClientHelloKeys: c.EncryptedClientHelloKeys, sessionTicketKeys: c.sessionTicketKeys, - autoSessionTicketKeys: c.autoSessionTicketKeys, + // We explicitly do not copy autoSessionTicketKeys, so that Configs do + // not share the same auto-rotated keys. } } diff --git a/src/crypto/tls/handshake_server.go b/src/crypto/tls/handshake_server.go index 6aebb74222..cdb15afc02 100644 --- a/src/crypto/tls/handshake_server.go +++ b/src/crypto/tls/handshake_server.go @@ -510,8 +510,13 @@ func (hs *serverHandshakeState) checkForResumption() error { if sessionHasClientCerts && c.config.ClientAuth == NoClientCert { return nil } - if sessionHasClientCerts && c.config.time().After(sessionState.peerCertificates[0].NotAfter) { - return nil + if sessionHasClientCerts { + now := c.config.time() + for _, c := range sessionState.peerCertificates { + if now.After(c.NotAfter) { + return nil + } + } } if sessionHasClientCerts && c.config.ClientAuth >= VerifyClientCertIfGiven && len(sessionState.verifiedChains) == 0 { diff --git a/src/crypto/tls/handshake_server_test.go b/src/crypto/tls/handshake_server_test.go index f533023afb..bd586dd7b7 100644 --- a/src/crypto/tls/handshake_server_test.go +++ b/src/crypto/tls/handshake_server_test.go @@ -13,6 +13,7 @@ import ( "crypto/rand" "crypto/tls/internal/fips140tls" "crypto/x509" + "crypto/x509/pkix" "encoding/pem" "errors" "fmt" @@ -2122,3 +2123,103 @@ func TestHandshakeContextHierarchy(t *testing.T) { t.Errorf("Unexpected client error: %v", err) } } + +func TestHandshakeChainExpiryResumptionTLS12(t *testing.T) { + t.Run("TLS1.2", func(t *testing.T) { + testHandshakeChainExpiryResumption(t, VersionTLS12) + }) + t.Run("TLS1.3", func(t *testing.T) { + testHandshakeChainExpiryResumption(t, VersionTLS13) + }) +} + +func testHandshakeChainExpiryResumption(t *testing.T, version uint16) { + now := time.Now() + createChain := func(leafNotAfter, rootNotAfter time.Time) (certDER []byte, root *x509.Certificate) { + tmpl := &x509.Certificate{ + Subject: pkix.Name{CommonName: "root"}, + NotBefore: rootNotAfter.Add(-time.Hour * 24), + NotAfter: rootNotAfter, + IsCA: true, + BasicConstraintsValid: true, + } + rootDER, err := x509.CreateCertificate(rand.Reader, tmpl, tmpl, &testECDSAPrivateKey.PublicKey, testECDSAPrivateKey) + if err != nil { + t.Fatalf("CreateCertificate: %v", err) + } + root, err = x509.ParseCertificate(rootDER) + if err != nil { + t.Fatalf("ParseCertificate: %v", err) + } + + tmpl = &x509.Certificate{ + Subject: pkix.Name{}, + DNSNames: []string{"expired-resume.example.com"}, + NotBefore: leafNotAfter.Add(-time.Hour * 24), + NotAfter: leafNotAfter, + KeyUsage: x509.KeyUsageDigitalSignature, + } + certDER, err = x509.CreateCertificate(rand.Reader, tmpl, root, &testECDSAPrivateKey.PublicKey, testECDSAPrivateKey) + if err != nil { + t.Fatalf("CreateCertificate: %v", err) + } + + return certDER, root + } + + initialLeafDER, initialRoot := createChain(now.Add(time.Hour), now.Add(2*time.Hour)) + + serverConfig := testConfig.Clone() + serverConfig.MaxVersion = version + serverConfig.Certificates = []Certificate{{ + Certificate: [][]byte{initialLeafDER}, + PrivateKey: testECDSAPrivateKey, + }} + serverConfig.ClientCAs = x509.NewCertPool() + serverConfig.ClientCAs.AddCert(initialRoot) + serverConfig.ClientAuth = RequireAndVerifyClientCert + serverConfig.Time = func() time.Time { + return now + } + + clientConfig := testConfig.Clone() + clientConfig.MaxVersion = version + clientConfig.Certificates = []Certificate{{ + Certificate: [][]byte{initialLeafDER}, + PrivateKey: testECDSAPrivateKey, + }} + clientConfig.RootCAs = x509.NewCertPool() + clientConfig.RootCAs.AddCert(initialRoot) + clientConfig.ServerName = "expired-resume.example.com" + clientConfig.ClientSessionCache = NewLRUClientSessionCache(32) + + testResume := func(t *testing.T, sc, cc *Config, expectResume bool) { + t.Helper() + ss, cs, err := testHandshake(t, cc, sc) + if err != nil { + t.Fatalf("handshake: %v", err) + } + if cs.DidResume != expectResume { + t.Fatalf("DidResume = %v; want %v", cs.DidResume, expectResume) + } + if ss.DidResume != expectResume { + t.Fatalf("DidResume = %v; want %v", cs.DidResume, expectResume) + } + } + + testResume(t, serverConfig, clientConfig, false) + testResume(t, serverConfig, clientConfig, true) + + freshLeafDER, freshRoot := createChain(now.Add(2*time.Hour), now.Add(3*time.Hour)) + clientConfig.Certificates = []Certificate{{ + Certificate: [][]byte{freshLeafDER}, + PrivateKey: testECDSAPrivateKey, + }} + serverConfig.Time = func() time.Time { + return now.Add(1*time.Hour + 30*time.Minute) + } + serverConfig.ClientCAs = x509.NewCertPool() + serverConfig.ClientCAs.AddCert(freshRoot) + + testResume(t, serverConfig, clientConfig, false) +} diff --git a/src/crypto/tls/handshake_server_tls13.go b/src/crypto/tls/handshake_server_tls13.go index a1a5d3101b..e9e5875511 100644 --- a/src/crypto/tls/handshake_server_tls13.go +++ b/src/crypto/tls/handshake_server_tls13.go @@ -352,6 +352,7 @@ func (hs *serverHandshakeStateTLS13) checkForResumption() error { return nil } +pskIdentityLoop: for i, identity := range hs.clientHello.pskIdentities { if i >= maxClientPSKIdentities { break @@ -404,8 +405,13 @@ func (hs *serverHandshakeStateTLS13) checkForResumption() error { if sessionHasClientCerts && c.config.ClientAuth == NoClientCert { continue } - if sessionHasClientCerts && c.config.time().After(sessionState.peerCertificates[0].NotAfter) { - continue + if sessionHasClientCerts { + now := c.config.time() + for _, c := range sessionState.peerCertificates { + if now.After(c.NotAfter) { + continue pskIdentityLoop + } + } } if sessionHasClientCerts && c.config.ClientAuth >= VerifyClientCertIfGiven && len(sessionState.verifiedChains) == 0 { diff --git a/src/crypto/tls/tls_test.go b/src/crypto/tls/tls_test.go index 76a9a222a9..c5bbeb9713 100644 --- a/src/crypto/tls/tls_test.go +++ b/src/crypto/tls/tls_test.go @@ -930,8 +930,8 @@ func TestCloneNonFuncFields(t *testing.T) { } } // Set the unexported fields related to session ticket keys, which are copied with Clone(). - c1.autoSessionTicketKeys = []ticketKey{c1.ticketKeyFromBytes(c1.SessionTicketKey)} c1.sessionTicketKeys = []ticketKey{c1.ticketKeyFromBytes(c1.SessionTicketKey)} + // We explicitly don't copy autoSessionTicketKeys in Clone, so don't set it. c2 := c1.Clone() if !reflect.DeepEqual(&c1, c2) { @@ -2249,3 +2249,12 @@ func TestECH(t *testing.T) { t.Fatal("unexpected certificate") } } + +func TestConfigCloneAutoSessionTicketKeys(t *testing.T) { + orig := &Config{} + orig.ticketKeys(nil) + clone := orig.Clone() + if slices.Equal(orig.autoSessionTicketKeys, clone.autoSessionTicketKeys) { + t.Fatal("autoSessionTicketKeys slice copied in Clone") + } +}