[release-branch.go1.24] crypto/tls: don't copy auto-rotated session ticket keys in...

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 <dneil@google.com>
Reviewed-by: Nicholas Husin <husin@google.com>
Reviewed-on: https://go-internal-review.googlesource.com/c/go/+/3322
Reviewed-by: Neal Patel <nealpatel@google.com>
Reviewed-on: https://go-review.googlesource.com/c/go/+/736700
TryBot-Bypass: Michael Pratt <mpratt@google.com>
Reviewed-by: Junyang Shao <shaojunyang@google.com>
Auto-Submit: Michael Pratt <mpratt@google.com>

diff --git a/src/crypto/tls/common.go b/src/crypto/tls/common.go
index d6942d2ef14a27b532dee6d1b502f5632e156ea0..6b73796920882eaa35854769116bbe572f52e3e8 100644 (file)
--- 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 6aebb742229a50b0172b4a0210b3f4c021530b20..cdb15afc02ff6aaaf87529a1a08c9d1a66382b6e 100644 (file)
--- 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 f533023afba5b991e68eb3c15c9a70046c0faf98..bd586dd7b7af9d0afc15392746a9cbaabef4e273 100644 (file)
--- 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 a1a5d3101b9853cac3999e88eefccf6b2dbeaa21..e9e58755110ebe513fb3b185d2b06d0cdae86c83 100644 (file)
--- 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 76a9a222a94bd74b5251e08fa72e0e0f28dfd3f6..c5bbeb971366660faf4354b768a6be61c34f30ce 100644 (file)
--- 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")
+       }
+}