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

This reverts CL 736709 (commit bba24719a4cad5cc8d771fc9cfff5a38019d554a).

Updates #77113
Updates #77355
Updates CVE-2025-68121

Change-Id: I0261cb75e9adf9d0ac9890dc91ae8476b8988ba0
Reviewed-on: https://go-review.googlesource.com/c/go/+/739320
Reviewed-by: Coia Prant <coiaprant@gmail.com>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-on: https://go-review.googlesource.com/c/go/+/740060
Reviewed-by: Damien Neil <dneil@google.com>
Auto-Submit: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Nicholas Husin <nsh@golang.org>
Reviewed-by: Nicholas Husin <husin@google.com>

diff --git a/src/crypto/tls/common.go b/src/crypto/tls/common.go
index 6b73796920882eaa35854769116bbe572f52e3e8..d6942d2ef14a27b532dee6d1b502f5632e156ea0 100644 (file)
--- a/src/crypto/tls/common.go
+++ b/src/crypto/tls/common.go
@@ -920,10 +920,6 @@ 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
@@ -963,8 +959,7 @@ func (c *Config) Clone() *Config {
                EncryptedClientHelloRejectionVerify: c.EncryptedClientHelloRejectionVerify,
                EncryptedClientHelloKeys:            c.EncryptedClientHelloKeys,
                sessionTicketKeys:                   c.sessionTicketKeys,
-               // We explicitly do not copy autoSessionTicketKeys, so that Configs do
-               // not share the same auto-rotated keys.
+               autoSessionTicketKeys:               c.autoSessionTicketKeys,
        }
 }
 

diff --git a/src/crypto/tls/handshake_server.go b/src/crypto/tls/handshake_server.go
index cdb15afc02ff6aaaf87529a1a08c9d1a66382b6e..6aebb742229a50b0172b4a0210b3f4c021530b20 100644 (file)
--- a/src/crypto/tls/handshake_server.go
+++ b/src/crypto/tls/handshake_server.go
@@ -510,13 +510,8 @@ func (hs *serverHandshakeState) checkForResumption() error {
        if sessionHasClientCerts && c.config.ClientAuth == NoClientCert {
                return nil
        }
-       if sessionHasClientCerts {
-               now := c.config.time()
-               for _, c := range sessionState.peerCertificates {
-                       if now.After(c.NotAfter) {
-                               return nil
-                       }
-               }
+       if sessionHasClientCerts && c.config.time().After(sessionState.peerCertificates[0].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 bd586dd7b7af9d0afc15392746a9cbaabef4e273..f533023afba5b991e68eb3c15c9a70046c0faf98 100644 (file)
--- a/src/crypto/tls/handshake_server_test.go
+++ b/src/crypto/tls/handshake_server_test.go
@@ -13,7 +13,6 @@ import (
        "crypto/rand"
        "crypto/tls/internal/fips140tls"
        "crypto/x509"
-       "crypto/x509/pkix"
        "encoding/pem"
        "errors"
        "fmt"
@@ -2123,103 +2122,3 @@ 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 e9e58755110ebe513fb3b185d2b06d0cdae86c83..a1a5d3101b9853cac3999e88eefccf6b2dbeaa21 100644 (file)
--- a/src/crypto/tls/handshake_server_tls13.go
+++ b/src/crypto/tls/handshake_server_tls13.go
@@ -352,7 +352,6 @@ func (hs *serverHandshakeStateTLS13) checkForResumption() error {
                return nil
        }
 
-pskIdentityLoop:
        for i, identity := range hs.clientHello.pskIdentities {
                if i >= maxClientPSKIdentities {
                        break
@@ -405,13 +404,8 @@ pskIdentityLoop:
                if sessionHasClientCerts && c.config.ClientAuth == NoClientCert {
                        continue
                }
-               if sessionHasClientCerts {
-                       now := c.config.time()
-                       for _, c := range sessionState.peerCertificates {
-                               if now.After(c.NotAfter) {
-                                       continue pskIdentityLoop
-                               }
-                       }
+               if sessionHasClientCerts && c.config.time().After(sessionState.peerCertificates[0].NotAfter) {
+                       continue
                }
                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 c5bbeb971366660faf4354b768a6be61c34f30ce..76a9a222a94bd74b5251e08fa72e0e0f28dfd3f6 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,12 +2249,3 @@ 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")
-       }
-}