From 7b872b6d955d3e749ea62dbfced68ab5c61eae91 Mon Sep 17 00:00:00 2001 From: Roland Shoemaker Date: Fri, 15 May 2020 12:49:04 -0700 Subject: [PATCH] crypto/tls: restore OCSP and SCTs during session resumption Restore previously sent SCTs and stapled OCSP response during session resumption for both TLS 1.2 and 1.3. This behavior is somewhat complicated for TLS 1.2 as SCTs are sent during the server hello, so they override what is saved in ClientSessionState. It is likely that if the server is sending a different set of SCTs there is probably a reason for doing so, such as a log being retired, or SCT validation requirements changing, so it makes sense to defer to the server in that case. Fixes #39075 Change-Id: I3c0fa2f69c6bf0247a447c48a1b4c733a882a233 Reviewed-on: https://go-review.googlesource.com/c/go/+/234237 Reviewed-by: Filippo Valsorda --- doc/go1.15.html | 6 ++ src/crypto/tls/common.go | 2 + src/crypto/tls/handshake_client.go | 11 +++- src/crypto/tls/handshake_client_test.go | 81 ++++++++++++++++++++++++ src/crypto/tls/handshake_client_tls13.go | 4 ++ 5 files changed, 103 insertions(+), 1 deletion(-) diff --git a/doc/go1.15.html b/doc/go1.15.html index 50f4fea5bc..ffe9d26dc7 100644 --- a/doc/go1.15.html +++ b/doc/go1.15.html @@ -478,6 +478,12 @@ Do not send CLs removing the interior tags from such phrases. ClientAuthType now implement fmt.Stringer.

+ +

+ The ConnectionState + fields OCSPResponse and SignedCertificateTimestamps + are now repopulated on client-side resumed connections. +

diff --git a/src/crypto/tls/common.go b/src/crypto/tls/common.go index fd21ae8fb1..3a5ca22613 100644 --- a/src/crypto/tls/common.go +++ b/src/crypto/tls/common.go @@ -278,6 +278,8 @@ type ClientSessionState struct { serverCertificates []*x509.Certificate // Certificate chain presented by the server verifiedChains [][]*x509.Certificate // Certificate chains we built for verification receivedAt time.Time // When the session ticket was received from the server + ocspResponse []byte // Stapled OCSP response presented by the server + scts [][]byte // SCTs presented by the server // TLS 1.3 fields. nonce []byte // Ticket nonce sent by the server, to derive PSK diff --git a/src/crypto/tls/handshake_client.go b/src/crypto/tls/handshake_client.go index 40c8e02c53..46b0a770d5 100644 --- a/src/crypto/tls/handshake_client.go +++ b/src/crypto/tls/handshake_client.go @@ -728,10 +728,17 @@ func (hs *clientHandshakeState) processServerHello() (bool, error) { return false, errors.New("tls: server resumed a session with a different cipher suite") } - // Restore masterSecret and peerCerts from previous state + // Restore masterSecret, peerCerts, and ocspResponse from previous state hs.masterSecret = hs.session.masterSecret c.peerCertificates = hs.session.serverCertificates c.verifiedChains = hs.session.verifiedChains + c.ocspResponse = hs.session.ocspResponse + // Let the ServerHello SCTs override the session SCTs from the original + // connection, if any are provided + if len(c.scts) == 0 && len(hs.session.scts) != 0 { + c.scts = hs.session.scts + } + return true, nil } @@ -788,6 +795,8 @@ func (hs *clientHandshakeState) readSessionTicket() error { serverCertificates: c.peerCertificates, verifiedChains: c.verifiedChains, receivedAt: c.config.time(), + ocspResponse: c.ocspResponse, + scts: c.scts, } return nil diff --git a/src/crypto/tls/handshake_client_test.go b/src/crypto/tls/handshake_client_test.go index 1cda90190c..12b0254123 100644 --- a/src/crypto/tls/handshake_client_test.go +++ b/src/crypto/tls/handshake_client_test.go @@ -19,6 +19,7 @@ import ( "os" "os/exec" "path/filepath" + "reflect" "strconv" "strings" "testing" @@ -2430,3 +2431,83 @@ func TestDowngradeCanary(t *testing.T) { t.Errorf("client unexpectedly reacted to a canary in TLS 1.0") } } + +func TestResumptionKeepsOCSPAndSCT(t *testing.T) { + t.Run("TLSv12", func(t *testing.T) { testResumptionKeepsOCSPAndSCT(t, VersionTLS12) }) + t.Run("TLSv13", func(t *testing.T) { testResumptionKeepsOCSPAndSCT(t, VersionTLS13) }) +} + +func testResumptionKeepsOCSPAndSCT(t *testing.T, ver uint16) { + issuer, err := x509.ParseCertificate(testRSACertificateIssuer) + if err != nil { + t.Fatalf("failed to parse test issuer") + } + roots := x509.NewCertPool() + roots.AddCert(issuer) + clientConfig := &Config{ + MaxVersion: ver, + ClientSessionCache: NewLRUClientSessionCache(32), + ServerName: "example.golang", + RootCAs: roots, + } + serverConfig := testConfig.Clone() + serverConfig.MaxVersion = ver + serverConfig.Certificates[0].OCSPStaple = []byte{1, 2, 3} + serverConfig.Certificates[0].SignedCertificateTimestamps = [][]byte{{4, 5, 6}} + + _, ccs, err := testHandshake(t, clientConfig, serverConfig) + if err != nil { + t.Fatalf("handshake failed: %s", err) + } + // after a new session we expect to see OCSPResponse and + // SignedCertificateTimestamps populated as usual + if !bytes.Equal(ccs.OCSPResponse, serverConfig.Certificates[0].OCSPStaple) { + t.Errorf("client ConnectionState contained unexpected OCSPResponse: wanted %v, got %v", + serverConfig.Certificates[0].OCSPStaple, ccs.OCSPResponse) + } + if !reflect.DeepEqual(ccs.SignedCertificateTimestamps, serverConfig.Certificates[0].SignedCertificateTimestamps) { + t.Errorf("client ConnectionState contained unexpected SignedCertificateTimestamps: wanted %v, got %v", + serverConfig.Certificates[0].SignedCertificateTimestamps, ccs.SignedCertificateTimestamps) + } + + // if the server doesn't send any SCTs, repopulate the old SCTs + oldSCTs := serverConfig.Certificates[0].SignedCertificateTimestamps + serverConfig.Certificates[0].SignedCertificateTimestamps = nil + _, ccs, err = testHandshake(t, clientConfig, serverConfig) + if err != nil { + t.Fatalf("handshake failed: %s", err) + } + if !ccs.DidResume { + t.Fatalf("expected session to be resumed") + } + // after a resumed session we also expect to see OCSPResponse + // and SignedCertificateTimestamps populated + if !bytes.Equal(ccs.OCSPResponse, serverConfig.Certificates[0].OCSPStaple) { + t.Errorf("client ConnectionState contained unexpected OCSPResponse after resumption: wanted %v, got %v", + serverConfig.Certificates[0].OCSPStaple, ccs.OCSPResponse) + } + if !reflect.DeepEqual(ccs.SignedCertificateTimestamps, oldSCTs) { + t.Errorf("client ConnectionState contained unexpected SignedCertificateTimestamps after resumption: wanted %v, got %v", + oldSCTs, ccs.SignedCertificateTimestamps) + } + + // Only test overriding the SCTs for TLS 1.2, since in 1.3 + // the server won't send the message containing them + if ver == VersionTLS13 { + return + } + + // if the server changes the SCTs it sends, they should override the saved SCTs + serverConfig.Certificates[0].SignedCertificateTimestamps = [][]byte{{7, 8, 9}} + _, ccs, err = testHandshake(t, clientConfig, serverConfig) + if err != nil { + t.Fatalf("handshake failed: %s", err) + } + if !ccs.DidResume { + t.Fatalf("expected session to be resumed") + } + if !reflect.DeepEqual(ccs.SignedCertificateTimestamps, serverConfig.Certificates[0].SignedCertificateTimestamps) { + t.Errorf("client ConnectionState contained unexpected SignedCertificateTimestamps after resumption: wanted %v, got %v", + serverConfig.Certificates[0].SignedCertificateTimestamps, ccs.SignedCertificateTimestamps) + } +} diff --git a/src/crypto/tls/handshake_client_tls13.go b/src/crypto/tls/handshake_client_tls13.go index 35a00f2f3a..9c61105cf7 100644 --- a/src/crypto/tls/handshake_client_tls13.go +++ b/src/crypto/tls/handshake_client_tls13.go @@ -334,6 +334,8 @@ func (hs *clientHandshakeStateTLS13) processServerHello() error { c.didResume = true c.peerCertificates = hs.session.serverCertificates c.verifiedChains = hs.session.verifiedChains + c.ocspResponse = hs.session.ocspResponse + c.scts = hs.session.scts return nil } @@ -666,6 +668,8 @@ func (c *Conn) handleNewSessionTicket(msg *newSessionTicketMsgTLS13) error { nonce: msg.nonce, useBy: c.config.time().Add(lifetime), ageAdd: msg.ageAdd, + ocspResponse: c.ocspResponse, + scts: c.scts, } cacheKey := clientSessionCacheKey(c.conn.RemoteAddr(), c.config) -- 2.50.0