Skip to content

Commit bba2471

Browse files
rolandshoemakergopherbot
rolandshoemaker
authored and
gopherbot
committed
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 <[email protected]> Reviewed-by: Nicholas Husin <[email protected]> Reviewed-on: https://go-review.googlesource.com/c/go/+/736709 Auto-Submit: Michael Pratt <[email protected]> Reviewed-by: Junyang Shao <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]>
1 parent 9ef26e9 commit bba2471

5 files changed

Lines changed: 132 additions & 6 deletions

File tree

‎src/crypto/tls/common.go‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -980,6 +980,10 @@ const maxSessionTicketLifetime = 7 * 24 * time.Hour
980980

981981
// Clone returns a shallow clone of c or nil if c is nil. It is safe to clone a [Config] that is
982982
// being used concurrently by a TLS client or server.
983+
//
984+
// If Config.SessionTicketKey is unpopulated, and Config.SetSessionTicketKeys has not been
985+
// called, the clone will not share the same auto-rotated session ticket keys as the original
986+
// Config in order to prevent sessions from being resumed across Configs.
983987
func (c *Config) Clone() *Config {
984988
if c == nil {
985989
return nil
@@ -1020,7 +1024,8 @@ func (c *Config) Clone() *Config {
10201024
EncryptedClientHelloRejectionVerify: c.EncryptedClientHelloRejectionVerify,
10211025
EncryptedClientHelloKeys: c.EncryptedClientHelloKeys,
10221026
sessionTicketKeys: c.sessionTicketKeys,
1023-
autoSessionTicketKeys: c.autoSessionTicketKeys,
1027+
// We explicitly do not copy autoSessionTicketKeys, so that Configs do
1028+
// not share the same auto-rotated keys.
10241029
}
10251030
}
10261031

‎src/crypto/tls/handshake_server.go‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -520,8 +520,13 @@ func (hs *serverHandshakeState) checkForResumption() error {
520520
if sessionHasClientCerts && c.config.ClientAuth == NoClientCert {
521521
return nil
522522
}
523-
if sessionHasClientCerts && c.config.time().After(sessionState.peerCertificates[0].NotAfter) {
524-
return nil
523+
if sessionHasClientCerts {
524+
now := c.config.time()
525+
for _, c := range sessionState.peerCertificates {
526+
if now.After(c.NotAfter) {
527+
return nil
528+
}
529+
}
525530
}
526531
if sessionHasClientCerts && c.config.ClientAuth >= VerifyClientCertIfGiven &&
527532
len(sessionState.verifiedChains) == 0 {

‎src/crypto/tls/handshake_server_test.go‎

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import (
1313
"crypto/rand"
1414
"crypto/tls/internal/fips140tls"
1515
"crypto/x509"
16+
"crypto/x509/pkix"
1617
"encoding/pem"
1718
"errors"
1819
"fmt"
@@ -2153,3 +2154,103 @@ func TestHandshakeContextHierarchy(t *testing.T) {
21532154
t.Errorf("Unexpected client error: %v", err)
21542155
}
21552156
}
2157+
2158+
func TestHandshakeChainExpiryResumptionTLS12(t *testing.T) {
2159+
t.Run("TLS1.2", func(t *testing.T) {
2160+
testHandshakeChainExpiryResumption(t, VersionTLS12)
2161+
})
2162+
t.Run("TLS1.3", func(t *testing.T) {
2163+
testHandshakeChainExpiryResumption(t, VersionTLS13)
2164+
})
2165+
}
2166+
2167+
func testHandshakeChainExpiryResumption(t *testing.T, version uint16) {
2168+
now := time.Now()
2169+
createChain := func(leafNotAfter, rootNotAfter time.Time) (certDER []byte, root *x509.Certificate) {
2170+
tmpl := &x509.Certificate{
2171+
Subject: pkix.Name{CommonName: "root"},
2172+
NotBefore: rootNotAfter.Add(-time.Hour * 24),
2173+
NotAfter: rootNotAfter,
2174+
IsCA: true,
2175+
BasicConstraintsValid: true,
2176+
}
2177+
rootDER, err := x509.CreateCertificate(rand.Reader, tmpl, tmpl, &testECDSAPrivateKey.PublicKey, testECDSAPrivateKey)
2178+
if err != nil {
2179+
t.Fatalf("CreateCertificate: %v", err)
2180+
}
2181+
root, err = x509.ParseCertificate(rootDER)
2182+
if err != nil {
2183+
t.Fatalf("ParseCertificate: %v", err)
2184+
}
2185+
2186+
tmpl = &x509.Certificate{
2187+
Subject: pkix.Name{},
2188+
DNSNames: []string{"expired-resume.example.com"},
2189+
NotBefore: leafNotAfter.Add(-time.Hour * 24),
2190+
NotAfter: leafNotAfter,
2191+
KeyUsage: x509.KeyUsageDigitalSignature,
2192+
}
2193+
certDER, err = x509.CreateCertificate(rand.Reader, tmpl, root, &testECDSAPrivateKey.PublicKey, testECDSAPrivateKey)
2194+
if err != nil {
2195+
t.Fatalf("CreateCertificate: %v", err)
2196+
}
2197+
2198+
return certDER, root
2199+
}
2200+
2201+
initialLeafDER, initialRoot := createChain(now.Add(time.Hour), now.Add(2*time.Hour))
2202+
2203+
serverConfig := testConfig.Clone()
2204+
serverConfig.MaxVersion = version
2205+
serverConfig.Certificates = []Certificate{{
2206+
Certificate: [][]byte{initialLeafDER},
2207+
PrivateKey: testECDSAPrivateKey,
2208+
}}
2209+
serverConfig.ClientCAs = x509.NewCertPool()
2210+
serverConfig.ClientCAs.AddCert(initialRoot)
2211+
serverConfig.ClientAuth = RequireAndVerifyClientCert
2212+
serverConfig.Time = func() time.Time {
2213+
return now
2214+
}
2215+
2216+
clientConfig := testConfig.Clone()
2217+
clientConfig.MaxVersion = version
2218+
clientConfig.Certificates = []Certificate{{
2219+
Certificate: [][]byte{initialLeafDER},
2220+
PrivateKey: testECDSAPrivateKey,
2221+
}}
2222+
clientConfig.RootCAs = x509.NewCertPool()
2223+
clientConfig.RootCAs.AddCert(initialRoot)
2224+
clientConfig.ServerName = "expired-resume.example.com"
2225+
clientConfig.ClientSessionCache = NewLRUClientSessionCache(32)
2226+
2227+
testResume := func(t *testing.T, sc, cc *Config, expectResume bool) {
2228+
t.Helper()
2229+
ss, cs, err := testHandshake(t, cc, sc)
2230+
if err != nil {
2231+
t.Fatalf("handshake: %v", err)
2232+
}
2233+
if cs.DidResume != expectResume {
2234+
t.Fatalf("DidResume = %v; want %v", cs.DidResume, expectResume)
2235+
}
2236+
if ss.DidResume != expectResume {
2237+
t.Fatalf("DidResume = %v; want %v", cs.DidResume, expectResume)
2238+
}
2239+
}
2240+
2241+
testResume(t, serverConfig, clientConfig, false)
2242+
testResume(t, serverConfig, clientConfig, true)
2243+
2244+
freshLeafDER, freshRoot := createChain(now.Add(2*time.Hour), now.Add(3*time.Hour))
2245+
clientConfig.Certificates = []Certificate{{
2246+
Certificate: [][]byte{freshLeafDER},
2247+
PrivateKey: testECDSAPrivateKey,
2248+
}}
2249+
serverConfig.Time = func() time.Time {
2250+
return now.Add(1*time.Hour + 30*time.Minute)
2251+
}
2252+
serverConfig.ClientCAs = x509.NewCertPool()
2253+
serverConfig.ClientCAs.AddCert(freshRoot)
2254+
2255+
testResume(t, serverConfig, clientConfig, false)
2256+
}

‎src/crypto/tls/handshake_server_tls13.go‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -314,6 +314,7 @@ func (hs *serverHandshakeStateTLS13) checkForResumption() error {
314314
return nil
315315
}
316316

317+
pskIdentityLoop:
317318
for i, identity := range hs.clientHello.pskIdentities {
318319
if i >= maxClientPSKIdentities {
319320
break
@@ -366,8 +367,13 @@ func (hs *serverHandshakeStateTLS13) checkForResumption() error {
366367
if sessionHasClientCerts && c.config.ClientAuth == NoClientCert {
367368
continue
368369
}
369-
if sessionHasClientCerts && c.config.time().After(sessionState.peerCertificates[0].NotAfter) {
370-
continue
370+
if sessionHasClientCerts {
371+
now := c.config.time()
372+
for _, c := range sessionState.peerCertificates {
373+
if now.After(c.NotAfter) {
374+
continue pskIdentityLoop
375+
}
376+
}
371377
}
372378
if sessionHasClientCerts && c.config.ClientAuth >= VerifyClientCertIfGiven &&
373379
len(sessionState.verifiedChains) == 0 {

‎src/crypto/tls/tls_test.go‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -935,8 +935,8 @@ func TestCloneNonFuncFields(t *testing.T) {
935935
}
936936
}
937937
// Set the unexported fields related to session ticket keys, which are copied with Clone().
938-
c1.autoSessionTicketKeys = []ticketKey{c1.ticketKeyFromBytes(c1.SessionTicketKey)}
939938
c1.sessionTicketKeys = []ticketKey{c1.ticketKeyFromBytes(c1.SessionTicketKey)}
939+
// We explicitly don't copy autoSessionTicketKeys in Clone, so don't set it.
940940

941941
c2 := c1.Clone()
942942
if !reflect.DeepEqual(&c1, c2) {
@@ -2461,3 +2461,12 @@ func (s messageOnlySigner) SignMessage(rand io.Reader, msg []byte, opts crypto.S
24612461
digest := h.Sum(nil)
24622462
return s.Signer.Sign(rand, digest, opts)
24632463
}
2464+
2465+
func TestConfigCloneAutoSessionTicketKeys(t *testing.T) {
2466+
orig := &Config{}
2467+
orig.ticketKeys(nil)
2468+
clone := orig.Clone()
2469+
if slices.Equal(orig.autoSessionTicketKeys, clone.autoSessionTicketKeys) {
2470+
t.Fatal("autoSessionTicketKeys slice copied in Clone")
2471+
}
2472+
}

0 commit comments

Comments
 (0)