Skip to content

Commit b691a2e

Browse files
FiloSottilegopherbot
FiloSottile
authored and
gopherbot
committed
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 <[email protected]> Reviewed-by: Dmitri Shuralyov <[email protected]> Reviewed-by: Roland Shoemaker <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]> Auto-Submit: Roland Shoemaker <[email protected]>
1 parent 31c9bcb commit b691a2e

3 files changed

Lines changed: 77 additions & 17 deletions

File tree

‎src/crypto/tls/common.go‎

Lines changed: 22 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import (
2222
"internal/godebug"
2323
"io"
2424
"net"
25+
"runtime"
2526
"slices"
2627
"strings"
2728
"sync"
@@ -1873,15 +1874,27 @@ func anyValidVerifiedChain(verifiedChains [][]*x509.Certificate, opts x509.Verif
18731874
}) {
18741875
continue
18751876
}
1876-
// Since we already validated the chain, we only care that it is
1877-
// rooted in a CA in CAs, or in the system pool. On platforms where
1878-
// we control chain validation (e.g. not Windows or macOS) this is a
1879-
// simple lookup in the CertPool internal hash map. On other
1880-
// platforms, this may be more expensive, depending on how they
1881-
// implement verification of just root certificates.
1882-
root := chain[len(chain)-1]
1883-
if _, err := root.Verify(opts); err == nil {
1884-
return true
1877+
// Since we already validated the chain, we only care that it is rooted
1878+
// in a CA in opts.Roots. On platforms where we control chain validation
1879+
// (e.g. not Windows or macOS) this is a simple lookup in the CertPool
1880+
// internal hash map, which we can simulate by running Verify on the
1881+
// root. On other platforms, we have to do full verification again,
1882+
// because EKU handling might differ. We will want to replace this with
1883+
// CertPool.Contains if/once that is available. See go.dev/issue/77376.
1884+
if runtime.GOOS == "windows" || runtime.GOOS == "darwin" || runtime.GOOS == "ios" {
1885+
opts.Intermediates = x509.NewCertPool()
1886+
for _, cert := range chain[1:max(1, len(chain)-1)] {
1887+
opts.Intermediates.AddCert(cert)
1888+
}
1889+
leaf := chain[0]
1890+
if _, err := leaf.Verify(opts); err == nil {
1891+
return true
1892+
}
1893+
} else {
1894+
root := chain[len(chain)-1]
1895+
if _, err := root.Verify(opts); err == nil {
1896+
return true
1897+
}
18851898
}
18861899
}
18871900
return false

‎src/crypto/tls/handshake_server_test.go‎

Lines changed: 20 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2302,7 +2302,7 @@ func testHandshakeGetConfigForClientDifferentClientCAs(t *testing.T, version uin
23022302
if err != nil {
23032303
t.Fatalf("ParseCertificate: %v", err)
23042304
}
2305-
rootDER, err = x509.CreateCertificate(rand.Reader, tmpl, tmpl, &testECDSAPrivateKey.PublicKey, testECDSAPrivateKey)
2305+
rootDER, err = x509.CreateCertificate(rand.Reader, tmpl, tmpl, &testRSA2048PrivateKey.PublicKey, testRSA2048PrivateKey)
23062306
if err != nil {
23072307
t.Fatalf("CreateCertificate: %v", err)
23082308
}
@@ -2318,15 +2318,19 @@ func testHandshakeGetConfigForClientDifferentClientCAs(t *testing.T, version uin
23182318
NotAfter: now.Add(time.Hour * 24),
23192319
KeyUsage: x509.KeyUsageDigitalSignature,
23202320
}
2321-
certDER, err := x509.CreateCertificate(rand.Reader, tmpl, rootA, &testECDSAPrivateKey.PublicKey, testECDSAPrivateKey)
2321+
certA, err := x509.CreateCertificate(rand.Reader, tmpl, rootA, &testECDSAPrivateKey.PublicKey, testECDSAPrivateKey)
2322+
if err != nil {
2323+
t.Fatalf("CreateCertificate: %v", err)
2324+
}
2325+
certB, err := x509.CreateCertificate(rand.Reader, tmpl, rootB, &testECDSAPrivateKey.PublicKey, testRSA2048PrivateKey)
23222326
if err != nil {
23232327
t.Fatalf("CreateCertificate: %v", err)
23242328
}
23252329

23262330
serverConfig := testConfig.Clone()
23272331
serverConfig.MaxVersion = version
23282332
serverConfig.Certificates = []Certificate{{
2329-
Certificate: [][]byte{certDER},
2333+
Certificate: [][]byte{certA},
23302334
PrivateKey: testECDSAPrivateKey,
23312335
}}
23322336
serverConfig.Time = func() time.Time {
@@ -2351,7 +2355,7 @@ func testHandshakeGetConfigForClientDifferentClientCAs(t *testing.T, version uin
23512355
clientConfig := testConfig.Clone()
23522356
clientConfig.MaxVersion = version
23532357
clientConfig.Certificates = []Certificate{{
2354-
Certificate: [][]byte{certDER},
2358+
Certificate: [][]byte{certA},
23552359
PrivateKey: testECDSAPrivateKey,
23562360
}}
23572361
clientConfig.ClientSessionCache = NewLRUClientSessionCache(32)
@@ -2380,6 +2384,8 @@ func testHandshakeGetConfigForClientDifferentClientCAs(t *testing.T, version uin
23802384
testResume(t, serverConfig, clientConfig, false)
23812385
testResume(t, serverConfig, clientConfig, true)
23822386

2387+
clientConfig.Certificates[0].Certificate = [][]byte{certB}
2388+
23832389
// Cause GetConfigForClient to return a config cloned from the base config,
23842390
// but with a different ClientCAs pool. This should cause resumption to fail.
23852391
switchConfig = true
@@ -2414,7 +2420,7 @@ func testHandshakeChangeRootCAsResumption(t *testing.T, version uint16) {
24142420
if err != nil {
24152421
t.Fatalf("ParseCertificate: %v", err)
24162422
}
2417-
rootDER, err = x509.CreateCertificate(rand.Reader, tmpl, tmpl, &testECDSAPrivateKey.PublicKey, testECDSAPrivateKey)
2423+
rootDER, err = x509.CreateCertificate(rand.Reader, tmpl, tmpl, &testRSA2048PrivateKey.PublicKey, testRSA2048PrivateKey)
24182424
if err != nil {
24192425
t.Fatalf("CreateCertificate: %v", err)
24202426
}
@@ -2430,15 +2436,19 @@ func testHandshakeChangeRootCAsResumption(t *testing.T, version uint16) {
24302436
NotAfter: now.Add(time.Hour * 24),
24312437
KeyUsage: x509.KeyUsageDigitalSignature,
24322438
}
2433-
certDER, err := x509.CreateCertificate(rand.Reader, tmpl, rootA, &testECDSAPrivateKey.PublicKey, testECDSAPrivateKey)
2439+
certA, err := x509.CreateCertificate(rand.Reader, tmpl, rootA, &testECDSAPrivateKey.PublicKey, testECDSAPrivateKey)
2440+
if err != nil {
2441+
t.Fatalf("CreateCertificate: %v", err)
2442+
}
2443+
certB, err := x509.CreateCertificate(rand.Reader, tmpl, rootB, &testECDSAPrivateKey.PublicKey, testRSA2048PrivateKey)
24342444
if err != nil {
24352445
t.Fatalf("CreateCertificate: %v", err)
24362446
}
24372447

24382448
serverConfig := testConfig.Clone()
24392449
serverConfig.MaxVersion = version
24402450
serverConfig.Certificates = []Certificate{{
2441-
Certificate: [][]byte{certDER},
2451+
Certificate: [][]byte{certA},
24422452
PrivateKey: testECDSAPrivateKey,
24432453
}}
24442454
serverConfig.Time = func() time.Time {
@@ -2453,7 +2463,7 @@ func testHandshakeChangeRootCAsResumption(t *testing.T, version uint16) {
24532463
clientConfig := testConfig.Clone()
24542464
clientConfig.MaxVersion = version
24552465
clientConfig.Certificates = []Certificate{{
2456-
Certificate: [][]byte{certDER},
2466+
Certificate: [][]byte{certA},
24572467
PrivateKey: testECDSAPrivateKey,
24582468
}}
24592469
clientConfig.ClientSessionCache = NewLRUClientSessionCache(32)
@@ -2486,6 +2496,8 @@ func testHandshakeChangeRootCAsResumption(t *testing.T, version uint16) {
24862496
clientConfig.RootCAs = x509.NewCertPool()
24872497
clientConfig.RootCAs.AddCert(rootB)
24882498

2499+
serverConfig.Certificates[0].Certificate = [][]byte{certB}
2500+
24892501
testResume(t, serverConfig, clientConfig, false)
24902502
testResume(t, serverConfig, clientConfig, true)
24912503
}

‎src/crypto/tls/tls_test.go‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -572,6 +572,41 @@ func TestVerifyHostname(t *testing.T) {
572572
}
573573
}
574574

575+
func TestRealResumption(t *testing.T) {
576+
testenv.MustHaveExternalNetwork(t)
577+
578+
config := &Config{
579+
ServerName: "yahoo.com",
580+
ClientSessionCache: NewLRUClientSessionCache(0),
581+
}
582+
583+
for range 10 {
584+
conn, err := Dial("tcp", "yahoo.com:443", config)
585+
if err != nil {
586+
t.Log("Dial error:", err)
587+
continue
588+
}
589+
// Do a read to consume the NewSessionTicket messages.
590+
fmt.Fprintf(conn, "GET / HTTP/1.1\r\nHost: yahoo.com\r\nConnection: close\r\n\r\n")
591+
conn.Read(make([]byte, 4096))
592+
conn.Close()
593+
594+
conn, err = Dial("tcp", "yahoo.com:443", config)
595+
if err != nil {
596+
t.Log("second Dial error:", err)
597+
continue
598+
}
599+
state := conn.ConnectionState()
600+
conn.Close()
601+
602+
if state.DidResume {
603+
return
604+
}
605+
}
606+
607+
t.Fatal("no connection used session resumption")
608+
}
609+
575610
func TestConnCloseBreakingWrite(t *testing.T) {
576611
ln := newLocalListener(t)
577612
defer ln.Close()

0 commit comments

Comments
 (0)