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: clients don't delete tickets that fail handshakes from ClientSessionCache #24919

Closed
santoshankr opened this issue Apr 18, 2018 · 4 comments

Comments

@santoshankr
Copy link

@santoshankr santoshankr commented Apr 18, 2018

Please answer these questions before submitting your issue. Thanks!

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

1.9.2

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go version go1.9.2 linux/amd64
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/ubuntu/src/go"
GORACE=""
GOROOT="/usr/lib/go-1.9"
GOTOOLDIR="/usr/lib/go-1.9/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build198947136=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOROOT/bin/go version: go version go1.9.2 linux/amd64
GOROOT/bin/go tool compile -V: compile version go1.9.2
uname -sr: Linux 4.13.0-38-generic
Distributor ID:	Ubuntu
Description:	Ubuntu 16.04.3 LTS
Release:	16.04
Codename:	xenial
/lib/x86_64-linux-gnu/libc.so.6: GNU C Library (Ubuntu GLIBC 2.23-0ubuntu10) stable release version 2.23, by Roland McGrath et al.
gdb --version: GNU gdb (Ubuntu 7.11.1-0ubuntu1~16.5) 7.11.1

What did you do?

https://play.golang.org/p/2qH4SFg2Gjn

Created a TLS client and a server, and tested TLS session resumption behavior.

What did you expect to see?

If a server accepts a ticket sent by the client but the handshake fails, the client SHOULD delete the ticket from its cache, according to Section 3.2 of RFC 5077. https://tools.ietf.org/html/rfc5077. The next connection to the server should fallback to a full TLS handshake.

What did you see instead?

If a client and server attempt to resume a TLS session past the expiry of the client certificate embedded in the ticket, the client gets wedged. The server accepts the ticket, but handshake fails because of the expired certs. The ticket is not deleted from the cache however, so subsequent attempts by the client to connect to the server fail in the same way.

@agnivade agnivade changed the title TLS clients don't delete tickets that fail handshakes from ClientSessionCache crypto/tls: clients don't delete tickets that fail handshakes from ClientSessionCache Apr 18, 2018
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Apr 18, 2018

@ianlancetaylor ianlancetaylor added this to the Go1.11 milestone Apr 18, 2018
@santoshankr

This comment has been minimized.

Copy link
Author

@santoshankr santoshankr commented Jun 14, 2018

The correct fix is to add a Delete method to the ClientSessionCache interface and have the client call it if the handshake fails when trying to resume a session. This is backwards incompatible though, anyone implementing ClientSessionCache will have to change their code.

A hack that could work is to replace the value stored in the cache with nil on failure. In the LRU implementation in crypto/tls, we can special case a nil Put by deleting the entry altogether. This will only break anyone manually stuffing a nil into a cache they got from NewLRUClientSessionCache but this was always dangerous anyway, because clientHandshake accesses attributes of candidateSession without checking for nil.

Thoughts, @FiloSottile ?

santoshankr added a commit to santoshankr/go that referenced this issue Jun 14, 2018
When a server accepts a session ticket presented by a client, but
the TLS handshake fails, RFC 5077 recommends that the client delete
the ticket. Because adding a full Delete method to the interface for
ClientSessionCache would break existing implementations, we have the
handshake implementation put a nil value instead.

Fixes golang#24919
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Aug 8, 2018

Change https://golang.org/cl/128477 mentions this issue: crypto/tls: delete session tickets on TLS handshake failure

@andybons andybons modified the milestones: Go1.11, Go1.11.1 Aug 25, 2018
@FiloSottile FiloSottile modified the milestones: Go1.11.1, Go1.12 Aug 31, 2018
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 5, 2018

Change https://golang.org/cl/147420 mentions this issue: crypto/tls: implement TLS 1.3 PSK authentication (client side)

@gopherbot gopherbot closed this in d669cc4 Nov 12, 2018
@golang golang locked and limited conversation to collaborators Nov 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
6 participants
You can’t perform that action at this time.