Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crypto/tls: TLS session resumption re-verifies the client's certificate chain #31641

Closed
marten-seemann opened this issue Apr 24, 2019 · 11 comments
Assignees
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@marten-seemann
Copy link
Contributor

What version of Go are you using (go version)?

$ go version
1.12.x

Does this issue reproduce with the latest release?

Yes

What did you do?

When using TLS session resumption with client certificates, the server reverifies the client's certificate chain.

What did you expect to see?

When using TLS session resumption the server sends an encrypted (and MACed) ticket to the client, which the client presents during the next handshake. According to the spec, the session ticket is an opaque blob. The Go implementation encodes the client's certificate chain into the session ticket. If the server is able to authenticate and decrypt the session ticket presented by the client, this should serve as proof that the server communicated with this client before, and allow it to skip the (potentially computationally expensive) verification of the certificate chain.

Re-verifying leads to unexpected consequences if the tls.Config.VerifyPeerCertificate callback was changed such that it rejects the certificate in between the original and the resumed connection . Since the server re-verifies the certificate chain, it will reject the connection. The client doesn't perform any verification (other than checking that the certificate didn't expire) and is happy to resume a connection even if its VerifyPeerCertificate callback would have rejected the server's certificate chain on a new connection.

@bradfitz bradfitz changed the title TLS session resumption re-verifies the client's certificate chain crypto/tls: TLS session resumption re-verifies the client's certificate chain Apr 24, 2019
@bradfitz bradfitz added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 24, 2019
@bradfitz bradfitz added this to the Go1.13 milestone Apr 24, 2019
@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@FiloSottile
Copy link
Contributor

I noticed this while implementing TLS 1.3, and it's not an easy decision. The discrepancy between client and server is bad. On the one hand, if the verification policy has changed, the application probably expects it to apply to all connections, so we should re-verify. On the other hand, verification is expensive and session tickets are a performance measure.

Usually, it's servers that have the most performance concern, though, so I am tempted to resolve the discrepancy towards safety, by making clients reverify as well. Opinions?

(Related to #25352)

@marten-seemann
Copy link
Contributor Author

Since we're encoding the whole certificate chain in the session ticket, there's no point whatsoever to use session tickets with TLS 1.3 at all: The amount of data transferred during the handshake is (modulo some framing overhead) exactly the same as during a normal handshake, the computational cost of re-verifying the certificate chain is the same, and we're not even saving any roundtrips.

Couldn't one argue that the security properties of a resumed session should be the same as those of the original session? If the session was kept alive (instead of being resumed), the new policy wouldn't apply to that session either, right?

@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@fiorix
Copy link

fiorix commented Mar 4, 2020

Landing here to +1 what @marten-seemann said. We've hit that exact issue while debugging slowness in Go servers using TLS 1.3 and ticket resumption. There's at least 140 KB of heap allocation on every handshake, largely due to parsing the certificate chain from the ticket.

We started trying to patch crypto/tls and make ticket generation and validation configurable, by adding the following to tls.Config:

GenerateClientTicket func(Certificate) (ticket []byte, err error)
ValidateClientTicket func(ticket []byte) error

We want to preserve the original behaviour, and only use custom tickets when these functions are provided by the user.

This needs a lot of testing still, and we're far from having anything concrete but I'd like to ask @FiloSottile if this is in the team's radar, and if what I describe above is potentially an acceptable solution to the problem.

@FiloSottile
Copy link
Contributor

Found what's almost certainly a bug related to this while reviewing CL 229122: if the mode is RequestClientCerts or NoClientCerts and the client doesn't provide certificates, VerifyPeerCertificate is not called on a new connection but is called on a resumed connection.

@rolandshoemaker
Copy link
Member

@FiloSottile what do you expect the behavior to be here for the VerifyPeerCertificate bug? (side note: it looks like the call is only gated by ClientAuth, if the client doesn't send anything you still get the callback)

It seems reasonable to me that if the server didn't request client auth that VerifyPeerCertificates wouldn't be called, even if the client did provide certificates (because the certificate processing code shouldn't be called) which matches what happens in new connections, but not resumed ones. Is the suggested behavior making resumed connections match new connections?

@FiloSottile FiloSottile modified the milestones: Backlog, Go1.16 Aug 18, 2020
@rolandshoemaker
Copy link
Member

This is the current state of chain verification:

New connection Resumed connection
Server certificates Verified Not verified
Client certificates Verified* Verified*

(*) For both TLS 1.2 and 1.3 the current behavior for client certificate verification is:

  • For new connections: client certificates are processed if ClientAuth >= RequestClientCert. VerifyPeerCertificates is always called if the number of certificates sent satisfies the ClientAuth level, including when ClientAuth == RequestClientCert and no certificates were sent.
  • For resumed connections: client certificates are always processed, regardless of ClientAuth level and if any certificates were sent. VerifyPeerCertificates is always called if the number of certificates sent satisfies the ClientAuth level, including when ClientAuth <= RequestClientCert and no certificates were sent.

There are two separate issues here to address:

  • server certificate and client certificate verification should be made consistent for resumed connections (either both reverifying or both skipping verification).
  • client certificate processing should be made consistent between new and resumed connections.

The performance/security trade-offs of the first issue have already been discussed. The second issue (which should probably be spun off into its own issue at this point) seems slightly clearer, although only applies if we keep reverification during resumption. During resumption we probably want to match the behavior of new connections, i.e. only process client certificates if ClientAuth >= RequestClientCert. This shouldn't degrade security of existing applications as processing/VerifyPeerCertificate would only be skipped when the level of ClientAuth was reduced since the initial connection. There is an additional question of if we want to continue calling VerifyPeerCertificates when ClientAuth == RequestClientCert and no certificates were sent, but I think that is less obvious.

@odeke-em
Copy link
Member

Howdy @marten-seemann, @fiorix, @rolandshoemaker, and @FiloSottile, given that this issue needs some more thoughts and work, plus processing. Shall we perhaps punt it to Go1.17? Thank you, and happy holidays!

@odeke-em odeke-em modified the milestones: Go1.16, Go1.17 Dec 25, 2020
@fiorix
Copy link

fiorix commented Dec 29, 2020

Let us share the crypto/tls patch we’ve been using since Go 1.14, only for TLS 1.3, to allow configuring a custom session ticket generator and validator. This is currently only used for internal traffic, no internet facing service is using this code.

The patch is experimental, and only serves for what is mentioned above. The idea is to add GenerateClientTicket and ValidateClientTicket to tls.Config for servers, and then adjust the server-side handshake of TLS 1.3 to use the custom code path when the Generator and Validator are set.

How did we get to this? A while ago @opsengine was profiling a Go service that was replacing a C++ service and noticed how the TLS handshake of the Go server was expensive. During the investigation we (mostly @opsengine and @slasher-) found that the crypto/tls session resumption code was using the client cert for ticket and parsing it on every handshake, including when resuming connections. We wanted to use a custom ticket and realised we couldn’t do so unless we patched the crypto/tls package, hence the patch we’re sharing here: https://pastebin.com/B7pVXHwj

@opsengine
Copy link

opsengine commented Jun 25, 2021

@fiorix @slasher- the patch has been battle tested in production for quite a while, should we start a PR?
@FiloSottile to give you more context, the change was inspired to fizz, which does something similar with tickets.
The main goal is to have the option of reducing CPU utilization for those servers with a high number of handshakes per second. This would be an "advanced" feature for users who supposedly know what they are doing. The default behavior would remain unchanged.
There are two reasons why having a custom ticket format helps performance:

  1. client certificates are parsed and verified only once per session, across multiple connections
  2. fewer heap objects allocated as a result of less ASN.1 parsing, which saves a non-negligible amount of work for the garbage collector

@FiloSottile FiloSottile modified the milestones: Backlog, Go1.19 Mar 2, 2022
@ianlancetaylor
Copy link
Contributor

CC @golang/security

Looks like this didn't make 1.19. Moving to backlog. Please recategorize as appropriate.

@ianlancetaylor ianlancetaylor removed this from the Go1.19 milestone Jun 24, 2022
@ianlancetaylor ianlancetaylor added this to the Backlog milestone Jun 24, 2022
@FiloSottile FiloSottile modified the milestones: Backlog, Go1.21 May 24, 2023
@gopherbot
Copy link

Change https://go.dev/cl/497895 mentions this issue: crypto/tls: don't reverify but check certificate expiration on resumption

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels May 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
Development

No branches or pull requests