[release-branch.go1.24] crypto/tls: revalidate whole chain on resumption on Windows...

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 <filippo@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Roland Shoemaker <roland@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Roland Shoemaker <roland@golang.org>
(cherry picked from commit b691a2edc7f5863f61a07c4a4f087eef1a15a704)
Reviewed-on: https://go-review.googlesource.com/c/go/+/741245
Reviewed-by: Michael Pratt <mpratt@google.com>
Auto-Submit: Michael Pratt <mpratt@google.com>

diff --git a/src/crypto/tls/common.go b/src/crypto/tls/common.go
index 623edcd2583c457fc986fe77bc2636fa1cc57699..46ed8b142d661d6df88e187e919b8e269916fb09 100644 (file)
--- 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 2b6eb2130194573da9d5bd0f67f8e1c52b6842f9..d9d98e0b10b60ec6b08cf8792ad2476774149ccb 100644 (file)
--- 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 76a9a222a94bd74b5251e08fa72e0e0f28dfd3f6..f01c1e5f561af370c6c84e705e6c3f367ece8d82 100644 (file)
--- 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()