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.

Fixes #77376

Cq-Include-Trybots: luci.golang.try:gotip-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>

diff --git a/src/crypto/tls/common.go b/src/crypto/tls/common.go
index c7a35d0bb520144371ea146cfe46ed10a9c9c2ba..f6926f1aa5a4d215767f611aa81baf66395bb617 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"
@@ -1873,15 +1874,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 cbcf0129748f418395ecba5bd580249ab418f3c5..736bb831ab64a2bb59c5b4b96400466ef2dac26e 100644 (file)
--- a/src/crypto/tls/handshake_server_test.go
+++ b/src/crypto/tls/handshake_server_test.go
@@ -2302,7 +2302,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)
        }
@@ -2318,7 +2318,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)
        }
@@ -2326,7 +2330,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 {
@@ -2351,7 +2355,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)
@@ -2380,6 +2384,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
@@ -2414,7 +2420,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)
        }
@@ -2430,7 +2436,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)
        }
@@ -2438,7 +2448,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 {
@@ -2453,7 +2463,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)
@@ -2486,6 +2496,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 39ebb9d2f1edd39d30fc6035840b700019bcce82..4513008a5f930e0fc008682956bc3dd27d938122 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()