From 84d465d6713c9609012b493d699d4cfd4554bc15 Mon Sep 17 00:00:00 2001 From: yuhan6665 <1588741+yuhan6665@users.noreply.github.com> Date: Sat, 14 Oct 2023 09:37:11 -0400 Subject: [PATCH] crypto/tls: don't reverify but check certificate expiration on resumption We used to inconsistently run certificate verification on the server on resumption, but not on the client. This made TLS 1.3 resumption pretty much useless, as it didn't save bytes, CPU, or round-trips. This requires serializing the verified chains into the session ticket, so it's a tradeoff making the ticket bigger to save computation (and for consistency). The previous behavior also had a "stickyness" issue: if a ticket contained invalid certificates, they would be used even if the client had in the meantime configured valid certificates for a full handshake. We also didn't check expiration on the client side on resumption if InsecureSkipVerify was set. Again for consistency, we do that now. Also, we used to run VerifyPeerCertificates on resumption even if NoClientCerts was set. Fixes #31641 Change-Id: Icc88269ea4adb544fa81158114aae76f3c91a15f Reviewed-on: https://go-review.googlesource.com/c/go/+/497895 Reviewed-by: Damien Neil TryBot-Result: Gopher Robot Run-TryBot: Filippo Valsorda Reviewed-by: Roland Shoemaker Auto-Submit: Filippo Valsorda --- common.go | 19 ++++-- handshake_client.go | 13 ++--- handshake_server.go | 18 ++++-- handshake_server_tls13.go | 14 ++++- ticket.go | 118 +++++++++++++++++--------------------- 5 files changed, 96 insertions(+), 86 deletions(-) diff --git a/common.go b/common.go index c801535..211fa39 100644 --- a/common.go +++ b/common.go @@ -618,10 +618,16 @@ type Config struct { // non-nil error, the handshake is aborted and that error results. // // If normal verification fails then the handshake will abort before - // considering this callback. If normal verification is disabled by - // setting InsecureSkipVerify, or (for a server) when ClientAuth is - // RequestClientCert or RequireAnyClientCert, then this callback will - // be considered but the verifiedChains argument will always be nil. + // considering this callback. If normal verification is disabled (on the + // client when InsecureSkipVerify is set, or on a server when ClientAuth is + // RequestClientCert or RequireAnyClientCert), then this callback will be + // considered but the verifiedChains argument will always be nil. When + // ClientAuth is NoClientCert, this callback is not called on the server. + // rawCerts may be empty on the server if ClientAuth is RequestClientCert or + // VerifyClientCertIfGiven. + // + // This callback is not invoked on resumed connections, as certificates are + // not re-verified on resumption. // // verifiedChains and its contents should not be modified. VerifyPeerCertificate func(rawCerts [][]byte, verifiedChains [][]*x509.Certificate) error @@ -632,8 +638,9 @@ type Config struct { // and that error results. // // If normal verification fails then the handshake will abort before - // considering this callback. This callback will run for all connections - // regardless of InsecureSkipVerify or ClientAuth settings. + // considering this callback. This callback will run for all connections, + // including resumptions, regardless of InsecureSkipVerify or ClientAuth + // settings. VerifyConnection func(ConnectionState) error // RootCAs defines the set of root certificate authorities diff --git a/handshake_client.go b/handshake_client.go index 5b42d30..daed95b 100644 --- a/handshake_client.go +++ b/handshake_client.go @@ -319,18 +319,17 @@ func (c *Conn) loadSession(hello *clientHelloMsg) ( // Check that the cached server certificate is not expired, and that it's // valid for the ServerName. This should be ensured by the cache key, but // protect the application from a faulty ClientSessionCache implementation. + if c.config.time().After(session.peerCertificates[0].NotAfter) { + // Expired certificate, delete the entry. + c.config.ClientSessionCache.Put(cacheKey, nil) + return nil, nil, nil, nil + } if !c.config.InsecureSkipVerify { if len(session.verifiedChains) == 0 { // The original connection had InsecureSkipVerify, while this doesn't. return nil, nil, nil, nil } - serverCert := session.peerCertificates[0] - if c.config.time().After(serverCert.NotAfter) { - // Expired certificate, delete the entry. - c.config.ClientSessionCache.Put(cacheKey, nil) - return nil, nil, nil, nil - } - if err := serverCert.VerifyHostname(c.config.ServerName); err != nil { + if err := session.peerCertificates[0].VerifyHostname(c.config.ServerName); err != nil { return nil, nil, nil, nil } } diff --git a/handshake_server.go b/handshake_server.go index 5fa4281..1c3d74a 100644 --- a/handshake_server.go +++ b/handshake_server.go @@ -471,6 +471,13 @@ func (hs *serverHandshakeState) checkForResumption() error { if sessionHasClientCerts && c.config.ClientAuth == NoClientCert { return nil } + if sessionHasClientCerts && c.config.time().After(sessionState.peerCertificates[0].NotAfter) { + return nil + } + if sessionHasClientCerts && c.config.ClientAuth >= VerifyClientCertIfGiven && + len(sessionState.verifiedChains) == 0 { + return nil + } // RFC 7627, Section 5.3 if !sessionState.extMasterSecret && hs.clientHello.extendedMasterSecret { @@ -482,6 +489,10 @@ func (hs *serverHandshakeState) checkForResumption() error { return errors.New("tls: session supported extended_master_secret but client does not") } + c.peerCertificates = sessionState.peerCertificates + c.ocspResponse = sessionState.ocspResponse + c.scts = sessionState.scts + c.verifiedChains = sessionState.verifiedChains c.extMasterSecret = sessionState.extMasterSecret hs.sessionState = sessionState hs.suite = suite @@ -510,10 +521,6 @@ func (hs *serverHandshakeState) doResumeHandshake() error { return err } - if err := c.processCertsFromClient(hs.sessionState.certificate()); err != nil { - return err - } - if c.config.VerifyConnection != nil { if err := c.config.VerifyConnection(c.connectionStateLocked()); err != nil { c.sendAlert(alertBadCertificate) @@ -847,8 +854,7 @@ func (hs *serverHandshakeState) sendFinished(out []byte) error { } // processCertsFromClient takes a chain of client certificates either from a -// Certificates message or from a sessionState and verifies them. It returns -// the public key of the leaf certificate. +// Certificates message and verifies them. func (c *Conn) processCertsFromClient(certificate Certificate) error { certificates := certificate.Certificate certs := make([]*x509.Certificate, len(certificates)) diff --git a/handshake_server_tls13.go b/handshake_server_tls13.go index 1abf770..f79ce04 100644 --- a/handshake_server_tls13.go +++ b/handshake_server_tls13.go @@ -387,6 +387,13 @@ func (hs *serverHandshakeStateTLS13) checkForResumption() error { if sessionHasClientCerts && c.config.ClientAuth == NoClientCert { continue } + if sessionHasClientCerts && c.config.time().After(sessionState.peerCertificates[0].NotAfter) { + continue + } + if sessionHasClientCerts && c.config.ClientAuth >= VerifyClientCertIfGiven && + len(sessionState.verifiedChains) == 0 { + continue + } hs.earlySecret = hs.suite.extract(sessionState.secret, nil) binderKey := hs.suite.deriveSecret(hs.earlySecret, resumptionBinderLabel, nil) @@ -422,9 +429,10 @@ func (hs *serverHandshakeStateTLS13) checkForResumption() error { } c.didResume = true - if err := c.processCertsFromClient(sessionState.certificate()); err != nil { - return err - } + c.peerCertificates = sessionState.peerCertificates + c.ocspResponse = sessionState.ocspResponse + c.scts = sessionState.scts + c.verifiedChains = sessionState.verifiedChains hs.hello.selectedIdentityPresent = true hs.hello.selectedIdentity = uint16(i) diff --git a/ticket.go b/ticket.go index adb6c3c..e72f4d6 100644 --- a/ticket.go +++ b/ticket.go @@ -37,16 +37,16 @@ type SessionState struct { // uint8 ext_master_secret = { 0, 1 }; // uint8 early_data = { 0, 1 }; // CertificateEntry certificate_list<0..2^24-1>; + // CertificateChain verified_chains<0..2^24-1>; /* excluding leaf */ // select (SessionState.early_data) { // case 0: Empty; // case 1: opaque alpn<1..2^8-1>; // }; // select (SessionState.type) { - // case server: /* empty */; + // case server: Empty; // case client: struct { - // CertificateChain verified_chains<0..2^24-1>; /* excluding leaf */ // select (SessionState.version) { - // case VersionTLS10..VersionTLS12: /* empty */; + // case VersionTLS10..VersionTLS12: Empty; // case VersionTLS13: struct { // uint64 use_by; // uint32 age_add; @@ -87,10 +87,8 @@ type SessionState struct { activeCertHandles []*activeCert ocspResponse []byte scts [][]byte - alpnProtocol string // only set if EarlyData is true - - // Client-side fields. verifiedChains [][]*x509.Certificate + alpnProtocol string // only set if EarlyData is true // Client-side TLS 1.3-only fields. useBy uint64 // seconds since UNIX epoch @@ -129,29 +127,33 @@ func (s *SessionState) Bytes() ([]byte, error) { } else { b.AddUint8(0) } - marshalCertificate(&b, s.certificate()) + marshalCertificate(&b, Certificate{ + Certificate: certificatesToBytesSlice(s.peerCertificates), + OCSPStaple: s.ocspResponse, + SignedCertificateTimestamps: s.scts, + }) + b.AddUint24LengthPrefixed(func(b *cryptobyte.Builder) { + for _, chain := range s.verifiedChains { + b.AddUint24LengthPrefixed(func(b *cryptobyte.Builder) { + // We elide the first certificate because it's always the leaf. + if len(chain) == 0 { + b.SetError(errors.New("tls: internal error: empty verified chain")) + return + } + for _, cert := range chain[1:] { + b.AddUint24LengthPrefixed(func(b *cryptobyte.Builder) { + b.AddBytes(cert.Raw) + }) + } + }) + } + }) if s.EarlyData { b.AddUint8LengthPrefixed(func(b *cryptobyte.Builder) { b.AddBytes([]byte(s.alpnProtocol)) }) } if s.isClient { - b.AddUint24LengthPrefixed(func(b *cryptobyte.Builder) { - for _, chain := range s.verifiedChains { - b.AddUint24LengthPrefixed(func(b *cryptobyte.Builder) { - // We elide the first certificate because it's always the leaf. - if len(chain) == 0 { - b.SetError(errors.New("tls: internal error: empty verified chain")) - return - } - for _, cert := range chain[1:] { - b.AddUint24LengthPrefixed(func(b *cryptobyte.Builder) { - b.AddBytes(cert.Raw) - }) - } - }) - } - }) if s.version >= VersionTLS13 { addUint64(&b, s.useBy) b.AddUint32(s.ageAdd) @@ -160,14 +162,6 @@ func (s *SessionState) Bytes() ([]byte, error) { return b.Bytes() } -func (s *SessionState) certificate() Certificate { - return Certificate{ - Certificate: certificatesToBytesSlice(s.peerCertificates), - OCSPStaple: s.ocspResponse, - SignedCertificateTimestamps: s.scts, - } -} - func certificatesToBytesSlice(certs []*x509.Certificate) [][]byte { s := make([][]byte, 0, len(certs)) for _, c := range certs { @@ -221,6 +215,34 @@ func ParseSessionState(data []byte) (*SessionState, error) { } ss.ocspResponse = cert.OCSPStaple ss.scts = cert.SignedCertificateTimestamps + var chainList cryptobyte.String + if !s.ReadUint24LengthPrefixed(&chainList) { + return nil, errors.New("tls: invalid session encoding") + } + for !chainList.Empty() { + var certList cryptobyte.String + if !chainList.ReadUint24LengthPrefixed(&certList) { + return nil, errors.New("tls: invalid session encoding") + } + var chain []*x509.Certificate + if len(ss.peerCertificates) == 0 { + return nil, errors.New("tls: invalid session encoding") + } + chain = append(chain, ss.peerCertificates[0]) + for !certList.Empty() { + var cert []byte + if !readUint24LengthPrefixed(&certList, &cert) { + return nil, errors.New("tls: invalid session encoding") + } + c, err := globalCertCache.newCert(cert) + if err != nil { + return nil, err + } + ss.activeCertHandles = append(ss.activeCertHandles, c) + chain = append(chain, c.cert) + } + ss.verifiedChains = append(ss.verifiedChains, chain) + } if ss.EarlyData { var alpn []byte if !readUint8LengthPrefixed(&s, &alpn) { @@ -238,31 +260,6 @@ func ParseSessionState(data []byte) (*SessionState, error) { if len(ss.peerCertificates) == 0 { return nil, errors.New("tls: no server certificates in client session") } - var chainList cryptobyte.String - if !s.ReadUint24LengthPrefixed(&chainList) { - return nil, errors.New("tls: invalid session encoding") - } - for !chainList.Empty() { - var certList cryptobyte.String - if !chainList.ReadUint24LengthPrefixed(&certList) { - return nil, errors.New("tls: invalid session encoding") - } - var chain []*x509.Certificate - chain = append(chain, ss.peerCertificates[0]) - for !certList.Empty() { - var cert []byte - if !readUint24LengthPrefixed(&certList, &cert) { - return nil, errors.New("tls: invalid session encoding") - } - c, err := globalCertCache.newCert(cert) - if err != nil { - return nil, err - } - ss.activeCertHandles = append(ss.activeCertHandles, c) - chain = append(chain, c.cert) - } - ss.verifiedChains = append(ss.verifiedChains, chain) - } if ss.version < VersionTLS13 { if !s.Empty() { return nil, errors.New("tls: invalid session encoding") @@ -278,13 +275,6 @@ func ParseSessionState(data []byte) (*SessionState, error) { // sessionState returns a partially filled-out [SessionState] with information // from the current connection. func (c *Conn) sessionState() (*SessionState, error) { - var verifiedChains [][]*x509.Certificate - if c.isClient { - verifiedChains = c.verifiedChains - if len(c.peerCertificates) == 0 { - return nil, errors.New("tls: internal error: empty peer certificates") - } - } return &SessionState{ version: c.vers, cipherSuite: c.cipherSuite, @@ -296,7 +286,7 @@ func (c *Conn) sessionState() (*SessionState, error) { scts: c.scts, isClient: c.isClient, extMasterSecret: c.extMasterSecret, - verifiedChains: verifiedChains, + verifiedChains: c.verifiedChains, }, nil }