From: Filippo Valsorda Date: Fri, 30 Jan 2026 17:07:23 +0000 (+0100) Subject: [release-branch.go1.24] crypto/tls: revalidate whole chain on resumption on Windows... X-Git-Tag: go1.24.13~2 X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=1a75aadba8af453bf3d4ca05340554d046d37945;p=gostls13.git [release-branch.go1.24] crypto/tls: revalidate whole chain on resumption on Windows and macOS TestHandshakeChangeRootCAsResumption and TestHandshakeGetConfigForClientDifferentClientCAs changed because previously rootA and rootB shared Subject and SPKI, which made the new full-chain revalidation check succeed, as the same leaf would verify against both roots. Updates #77376 Fixes #77424 Cq-Include-Trybots: luci.golang.try:go1.24-darwin-arm64-longtest Change-Id: I60bed694bdc621c9e83f1bd8a8224c016a6a6964 Reviewed-on: https://go-review.googlesource.com/c/go/+/741361 Auto-Submit: Filippo Valsorda Reviewed-by: Dmitri Shuralyov Reviewed-by: Roland Shoemaker LUCI-TryBot-Result: Go LUCI Auto-Submit: Roland Shoemaker (cherry picked from commit b691a2edc7f5863f61a07c4a4f087eef1a15a704) Reviewed-on: https://go-review.googlesource.com/c/go/+/741245 Reviewed-by: Michael Pratt Auto-Submit: Michael Pratt --- diff --git a/src/crypto/tls/common.go b/src/crypto/tls/common.go index 623edcd258..46ed8b142d 100644 --- a/src/crypto/tls/common.go +++ b/src/crypto/tls/common.go @@ -22,6 +22,7 @@ import ( "internal/godebug" "io" "net" + "runtime" "slices" "strings" "sync" @@ -1756,15 +1757,27 @@ func anyValidVerifiedChain(verifiedChains [][]*x509.Certificate, opts x509.Verif }) { continue } - // Since we already validated the chain, we only care that it is - // rooted in a CA in CAs, or in the system pool. On platforms where - // we control chain validation (e.g. not Windows or macOS) this is a - // simple lookup in the CertPool internal hash map. On other - // platforms, this may be more expensive, depending on how they - // implement verification of just root certificates. - root := chain[len(chain)-1] - if _, err := root.Verify(opts); err == nil { - return true + // Since we already validated the chain, we only care that it is rooted + // in a CA in opts.Roots. On platforms where we control chain validation + // (e.g. not Windows or macOS) this is a simple lookup in the CertPool + // internal hash map, which we can simulate by running Verify on the + // root. On other platforms, we have to do full verification again, + // because EKU handling might differ. We will want to replace this with + // CertPool.Contains if/once that is available. See go.dev/issue/77376. + if runtime.GOOS == "windows" || runtime.GOOS == "darwin" || runtime.GOOS == "ios" { + opts.Intermediates = x509.NewCertPool() + for _, cert := range chain[1:max(1, len(chain)-1)] { + opts.Intermediates.AddCert(cert) + } + leaf := chain[0] + if _, err := leaf.Verify(opts); err == nil { + return true + } + } else { + root := chain[len(chain)-1] + if _, err := root.Verify(opts); err == nil { + return true + } } } return false diff --git a/src/crypto/tls/handshake_server_test.go b/src/crypto/tls/handshake_server_test.go index 2b6eb21301..d9d98e0b10 100644 --- a/src/crypto/tls/handshake_server_test.go +++ b/src/crypto/tls/handshake_server_test.go @@ -2271,7 +2271,7 @@ func testHandshakeGetConfigForClientDifferentClientCAs(t *testing.T, version uin if err != nil { t.Fatalf("ParseCertificate: %v", err) } - rootDER, err = x509.CreateCertificate(rand.Reader, tmpl, tmpl, &testECDSAPrivateKey.PublicKey, testECDSAPrivateKey) + rootDER, err = x509.CreateCertificate(rand.Reader, tmpl, tmpl, &testRSA2048PrivateKey.PublicKey, testRSA2048PrivateKey) if err != nil { t.Fatalf("CreateCertificate: %v", err) } @@ -2287,7 +2287,11 @@ func testHandshakeGetConfigForClientDifferentClientCAs(t *testing.T, version uin NotAfter: now.Add(time.Hour * 24), KeyUsage: x509.KeyUsageDigitalSignature, } - certDER, err := x509.CreateCertificate(rand.Reader, tmpl, rootA, &testECDSAPrivateKey.PublicKey, testECDSAPrivateKey) + certA, err := x509.CreateCertificate(rand.Reader, tmpl, rootA, &testECDSAPrivateKey.PublicKey, testECDSAPrivateKey) + if err != nil { + t.Fatalf("CreateCertificate: %v", err) + } + certB, err := x509.CreateCertificate(rand.Reader, tmpl, rootB, &testECDSAPrivateKey.PublicKey, testRSA2048PrivateKey) if err != nil { t.Fatalf("CreateCertificate: %v", err) } @@ -2295,7 +2299,7 @@ func testHandshakeGetConfigForClientDifferentClientCAs(t *testing.T, version uin serverConfig := testConfig.Clone() serverConfig.MaxVersion = version serverConfig.Certificates = []Certificate{{ - Certificate: [][]byte{certDER}, + Certificate: [][]byte{certA}, PrivateKey: testECDSAPrivateKey, }} serverConfig.Time = func() time.Time { @@ -2320,7 +2324,7 @@ func testHandshakeGetConfigForClientDifferentClientCAs(t *testing.T, version uin clientConfig := testConfig.Clone() clientConfig.MaxVersion = version clientConfig.Certificates = []Certificate{{ - Certificate: [][]byte{certDER}, + Certificate: [][]byte{certA}, PrivateKey: testECDSAPrivateKey, }} clientConfig.ClientSessionCache = NewLRUClientSessionCache(32) @@ -2349,6 +2353,8 @@ func testHandshakeGetConfigForClientDifferentClientCAs(t *testing.T, version uin testResume(t, serverConfig, clientConfig, false) testResume(t, serverConfig, clientConfig, true) + clientConfig.Certificates[0].Certificate = [][]byte{certB} + // Cause GetConfigForClient to return a config cloned from the base config, // but with a different ClientCAs pool. This should cause resumption to fail. switchConfig = true @@ -2383,7 +2389,7 @@ func testHandshakeChangeRootCAsResumption(t *testing.T, version uint16) { if err != nil { t.Fatalf("ParseCertificate: %v", err) } - rootDER, err = x509.CreateCertificate(rand.Reader, tmpl, tmpl, &testECDSAPrivateKey.PublicKey, testECDSAPrivateKey) + rootDER, err = x509.CreateCertificate(rand.Reader, tmpl, tmpl, &testRSA2048PrivateKey.PublicKey, testRSA2048PrivateKey) if err != nil { t.Fatalf("CreateCertificate: %v", err) } @@ -2399,7 +2405,11 @@ func testHandshakeChangeRootCAsResumption(t *testing.T, version uint16) { NotAfter: now.Add(time.Hour * 24), KeyUsage: x509.KeyUsageDigitalSignature, } - certDER, err := x509.CreateCertificate(rand.Reader, tmpl, rootA, &testECDSAPrivateKey.PublicKey, testECDSAPrivateKey) + certA, err := x509.CreateCertificate(rand.Reader, tmpl, rootA, &testECDSAPrivateKey.PublicKey, testECDSAPrivateKey) + if err != nil { + t.Fatalf("CreateCertificate: %v", err) + } + certB, err := x509.CreateCertificate(rand.Reader, tmpl, rootB, &testECDSAPrivateKey.PublicKey, testRSA2048PrivateKey) if err != nil { t.Fatalf("CreateCertificate: %v", err) } @@ -2407,7 +2417,7 @@ func testHandshakeChangeRootCAsResumption(t *testing.T, version uint16) { serverConfig := testConfig.Clone() serverConfig.MaxVersion = version serverConfig.Certificates = []Certificate{{ - Certificate: [][]byte{certDER}, + Certificate: [][]byte{certA}, PrivateKey: testECDSAPrivateKey, }} serverConfig.Time = func() time.Time { @@ -2422,7 +2432,7 @@ func testHandshakeChangeRootCAsResumption(t *testing.T, version uint16) { clientConfig := testConfig.Clone() clientConfig.MaxVersion = version clientConfig.Certificates = []Certificate{{ - Certificate: [][]byte{certDER}, + Certificate: [][]byte{certA}, PrivateKey: testECDSAPrivateKey, }} clientConfig.ClientSessionCache = NewLRUClientSessionCache(32) @@ -2455,6 +2465,8 @@ func testHandshakeChangeRootCAsResumption(t *testing.T, version uint16) { clientConfig.RootCAs = x509.NewCertPool() clientConfig.RootCAs.AddCert(rootB) + serverConfig.Certificates[0].Certificate = [][]byte{certB} + testResume(t, serverConfig, clientConfig, false) testResume(t, serverConfig, clientConfig, true) } diff --git a/src/crypto/tls/tls_test.go b/src/crypto/tls/tls_test.go index 76a9a222a9..f01c1e5f56 100644 --- a/src/crypto/tls/tls_test.go +++ b/src/crypto/tls/tls_test.go @@ -572,6 +572,41 @@ func TestVerifyHostname(t *testing.T) { } } +func TestRealResumption(t *testing.T) { + testenv.MustHaveExternalNetwork(t) + + config := &Config{ + ServerName: "yahoo.com", + ClientSessionCache: NewLRUClientSessionCache(0), + } + + for range 10 { + conn, err := Dial("tcp", "yahoo.com:443", config) + if err != nil { + t.Log("Dial error:", err) + continue + } + // Do a read to consume the NewSessionTicket messages. + fmt.Fprintf(conn, "GET / HTTP/1.1\r\nHost: yahoo.com\r\nConnection: close\r\n\r\n") + conn.Read(make([]byte, 4096)) + conn.Close() + + conn, err = Dial("tcp", "yahoo.com:443", config) + if err != nil { + t.Log("second Dial error:", err) + continue + } + state := conn.ConnectionState() + conn.Close() + + if state.DidResume { + return + } + } + + t.Fatal("no connection used session resumption") +} + func TestConnCloseBreakingWrite(t *testing.T) { ln := newLocalListener(t) defer ln.Close()