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: TLSv1.3 connection fails with invalid PSK binder #59424

Closed
tsaarni opened this issue Apr 4, 2023 · 9 comments
Closed

crypto/tls: TLSv1.3 connection fails with invalid PSK binder #59424

tsaarni opened this issue Apr 4, 2023 · 9 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@tsaarni
Copy link
Contributor

tsaarni commented Apr 4, 2023

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

The bug impacts following versions

  • Go1.19.6 or later
  • Go1.20.1 or later

Does this issue reproduce with the latest release?

Yes

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

The bug is not OS dependent

What did you do?

  1. I have written TLSv1.3 client and server written in go.
  2. In the server I have set CurvePreferences with non-default list of curves.
  3. In the client I'm using default list of curves.
  4. In the client I have enabled TLS session resumption by setting ClientSessionCache

What did you expect to see?

When the client establishes second TLS session, I expected the TLS handshake to succeed.

What did you see instead?

When the client establishes second TLS session, the server will reject it with alert invalid PSK binder.

@tsaarni
Copy link
Contributor Author

tsaarni commented Apr 4, 2023

I'm attaching reproduction case. Here is the failure when running it

$ go run reproduce-tls-resumption-failure.go
Client: sending request to https://localhost:8443
Server: received request from 127.0.0.1:56926
Client: received response from server with status: 200 OK
Client: sleeping 5 sec...
Client: sending request to https://localhost:8443
2023/04/04 11:10:13 http: TLS handshake error from 127.0.0.1:56934: tls: invalid PSK binder
panic: Get "https://localhost:8443": remote error: tls: error decrypting message

The example code is here:

reproduce-tls-resumption-failure.go.txt

Same code is uploaded to https://goplay.tools/snippet/ur5TpD7hXe4

@gopherbot
Copy link

Change https://go.dev/cl/481955 mentions this issue: crypto/tls: fix PSK binder calculation

@tsaarni
Copy link
Contributor Author

tsaarni commented Apr 4, 2023

Hi @rolandshoemaker 👋 I may have bumped into a bug that was introduced with #58001. I submitted a proposal for a fix here #59425. I'd greatly appreciate it if you had an opportunity to take a look. Thank you!

@mknyszek
Copy link
Contributor

mknyszek commented Apr 4, 2023

CC @golang/security

@mknyszek mknyszek added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 4, 2023
@mknyszek mknyszek added this to the Backlog milestone Apr 4, 2023
@tsaarni
Copy link
Contributor Author

tsaarni commented Apr 4, 2023

Here is a summary how the bug gets triggered:

The bug is in TLSv1.3 handshake code and only impacts TLSv1.3 clients. Servers are not impacted. TLSv1.2 implementation is not impacted.

The bug was introduced by #58001

For the client to be impacted, client must have explicitly enabled TLS resumption by setting e.g. ClientSessionCache: tls.NewLRUClientSessionCache(num) in client's tls.Config structure. Client also needs to hold the SSL context and re-use it for multiple requests to trigger resumption

The bug happens in PSK resumption if client and server having incompatible preferred EC curve. For example, it can happen when the server has set CurvePreferences: []tls.CurveID{tls.CurveP521, tls.CurveP384, tls.CurveP256} in tls.Config but client uses the Go default list. Client will send ClientHello with x25519 but if server preferences does not include x25519, server will respond with HelloRetryRequest according to the spec (link) and set secp512r1 from server's CurvePreferences.

The reception of HelloRetryRequest response from the server will trigger the bug in client when above conditions are true. There is a typo in processHelloRetryRequest() which causes ClientHello message to be sent to server with invalid PSK binders hash. When server will process the new ClientHello with invalid PSK binders field, it will abort the handshake with invalid PSK binder alert.

@rolandshoemaker
Copy link
Member

Sorry, I was out on vacation. Thanks for catching this!

@gopherbot please open backport issues for this, it's a regression from a security fix.

@gopherbot
Copy link

Backport issue(s) opened: #59539 (for 1.19), #59540 (for 1.20).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

tsaarni added a commit to Nordix/go that referenced this issue Apr 12, 2023
When server and client have mismatch in curve preference, the server will
send HelloRetryRequest during TLSv1.3 PSK resumption. There was a bug
introduced by Go1.19.6 or later and Go1.20.1 or later, that makes the client
calculate the PSK binder hash incorrectly. Server will reject the TLS
handshake by sending alert: invalid PSK binder.

Fixes golang#59424
@dmitshur dmitshur modified the milestones: Backlog, Go1.21 Apr 22, 2023
@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 Apr 22, 2023
@gopherbot
Copy link

Change https://go.dev/cl/488055 mentions this issue: [release-branch.go1.20] crypto/tls: fix PSK binder calculation

@gopherbot
Copy link

Change https://go.dev/cl/488075 mentions this issue: [release-branch.go1.19] crypto/tls: fix PSK binder calculation

gopherbot pushed a commit that referenced this issue Apr 24, 2023
When server and client have mismatch in curve preference, the server will
send HelloRetryRequest during TLSv1.3 PSK resumption. There was a bug
introduced by Go1.19.6 or later and Go1.20.1 or later, that makes the client
calculate the PSK binder hash incorrectly. Server will reject the TLS
handshake by sending alert: invalid PSK binder.

For #59424.
Fixes #59539.

Change-Id: I2ca8948474275740a36d991c057b62a13392dbb9
GitHub-Last-Rev: 1aad9bc
GitHub-Pull-Request: #59425
Reviewed-on: https://go-review.googlesource.com/c/go/+/481955
Reviewed-by: Roland Shoemaker <roland@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Run-TryBot: Roland Shoemaker <roland@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Roland Shoemaker <roland@golang.org>
(cherry picked from commit 2c70690)
Reviewed-on: https://go-review.googlesource.com/c/go/+/488075
Auto-Submit: Dmitri Shuralyov <dmitshur@google.com>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
gopherbot pushed a commit that referenced this issue Apr 24, 2023
When server and client have mismatch in curve preference, the server will
send HelloRetryRequest during TLSv1.3 PSK resumption. There was a bug
introduced by Go1.19.6 or later and Go1.20.1 or later, that makes the client
calculate the PSK binder hash incorrectly. Server will reject the TLS
handshake by sending alert: invalid PSK binder.

For #59424.
Fixes #59540.

Change-Id: I2ca8948474275740a36d991c057b62a13392dbb9
GitHub-Last-Rev: 1aad9bc
GitHub-Pull-Request: #59425
Reviewed-on: https://go-review.googlesource.com/c/go/+/481955
Reviewed-by: Roland Shoemaker <roland@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Run-TryBot: Roland Shoemaker <roland@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Roland Shoemaker <roland@golang.org>
(cherry picked from commit 2c70690)
Reviewed-on: https://go-review.googlesource.com/c/go/+/488055
Auto-Submit: Dmitri Shuralyov <dmitshur@google.com>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
bradfitz pushed a commit to tailscale/go that referenced this issue May 25, 2023
When server and client have mismatch in curve preference, the server will
send HelloRetryRequest during TLSv1.3 PSK resumption. There was a bug
introduced by Go1.19.6 or later and Go1.20.1 or later, that makes the client
calculate the PSK binder hash incorrectly. Server will reject the TLS
handshake by sending alert: invalid PSK binder.

For golang#59424.
Fixes golang#59540.

Change-Id: I2ca8948474275740a36d991c057b62a13392dbb9
GitHub-Last-Rev: 1aad9bc
GitHub-Pull-Request: golang#59425
Reviewed-on: https://go-review.googlesource.com/c/go/+/481955
Reviewed-by: Roland Shoemaker <roland@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Run-TryBot: Roland Shoemaker <roland@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Roland Shoemaker <roland@golang.org>
(cherry picked from commit 2c70690)
Reviewed-on: https://go-review.googlesource.com/c/go/+/488055
Auto-Submit: Dmitri Shuralyov <dmitshur@google.com>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
bradfitz pushed a commit to tailscale/go that referenced this issue May 25, 2023
When server and client have mismatch in curve preference, the server will
send HelloRetryRequest during TLSv1.3 PSK resumption. There was a bug
introduced by Go1.19.6 or later and Go1.20.1 or later, that makes the client
calculate the PSK binder hash incorrectly. Server will reject the TLS
handshake by sending alert: invalid PSK binder.

For golang#59424.
Fixes golang#59540.

Change-Id: I2ca8948474275740a36d991c057b62a13392dbb9
GitHub-Last-Rev: 1aad9bc
GitHub-Pull-Request: golang#59425
Reviewed-on: https://go-review.googlesource.com/c/go/+/481955
Reviewed-by: Roland Shoemaker <roland@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Run-TryBot: Roland Shoemaker <roland@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Roland Shoemaker <roland@golang.org>
(cherry picked from commit 2c70690)
Reviewed-on: https://go-review.googlesource.com/c/go/+/488055
Auto-Submit: Dmitri Shuralyov <dmitshur@google.com>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
@golang golang locked and limited conversation to collaborators Apr 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants