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: fix PSK binder calculation #59425

Closed
wants to merge 1 commit into from

Conversation

tsaarni
Copy link
Contributor

@tsaarni tsaarni commented Apr 4, 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 #59424

@gopherbot
Copy link

This PR (HEAD: ccf1b7e) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/481955 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link

Message from Gopher Robot:

Patch Set 1:

Congratulations on opening your first change. Thank you for your contribution!

Next steps:
A maintainer will review your change and provide feedback. See
https://go.dev/doc/contribute#review for more info and tips to get your
patch through code review.

Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.

During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11 or adds a tag like "wait-release", it means that this CL will be
reviewed as part of the next development cycle. See https://go.dev/s/release
for more details.


Please don’t reply on this GitHub thread. Visit golang.org/cl/481955.
After addressing review feedback, remember to publish your drafts!

@tsaarni
Copy link
Contributor Author

tsaarni commented Apr 4, 2023

The bug that this PR attempts to fix is on the TLSv1.3 client handshake code. Only TLSv1.3 clients are impacted. Servers are not impacted. TLSv1.2 implementation is not impacted.

The bug was introduced by #58001

The bug impacts TLS clients compiled with with Go1.19.6 or later or Go1.20.1 or later.

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 triggers resumption

The bug happens in PSK resumption when 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}

but client uses the Go default list: client will send ClientHello with x25519. Since server preferences does not include x25519, server will respond with HelloRetryRequest and set secp512r1 from CurvePreferences according to chapter "2.1. Incorrect DHE Share" https://www.rfc-editor.org/rfc/rfc8446#page-14.

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.

@gopherbot
Copy link

Message from Roland Shoemaker:

Patch Set 1: Code-Review+2 Run-TryBot+1

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/481955.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

Message from Gopher Robot:

Patch Set 1:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/481955.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

Message from Gopher Robot:

Patch Set 1: TryBot-Result+1

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/481955.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

Message from Tero Saarni:

Patch Set 1:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/481955.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

Message from Roland Shoemaker:

Patch Set 1:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/481955.
After addressing review feedback, remember to publish your drafts!

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
@gopherbot
Copy link

This PR (HEAD: 1aad9bc) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/481955 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link

Message from Tero Saarni:

Patch Set 2:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/481955.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

Message from Roland Shoemaker:

Patch Set 2: Auto-Submit+1 Code-Review+2 Run-TryBot+1


Please don’t reply on this GitHub thread. Visit golang.org/cl/481955.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

Message from Gopher Robot:

Patch Set 2:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/481955.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

Message from Gopher Robot:

Patch Set 2: TryBot-Result+1

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/481955.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

Message from Dmitri Shuralyov:

Patch Set 1: Run-TryBot+1


Please don’t reply on this GitHub thread. Visit golang.org/cl/488055.
After addressing review feedback, remember to publish your drafts!

gopherbot pushed a commit that referenced this pull request 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.

Fixes #59424

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>
@gopherbot
Copy link

Message from Gopher Robot:

Patch Set 1:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/488075.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

Message from Dmitri Shuralyov:

Patch Set 1: Code-Review+1


Please don’t reply on this GitHub thread. Visit golang.org/cl/488075.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

Message from Gopher Robot:

Patch Set 1:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/488075.
After addressing review feedback, remember to publish your drafts!

@dmitshur
Copy link
Contributor

Merged as CL 481955 (commit 2c70690).

@dmitshur dmitshur closed this Apr 24, 2023
@gopherbot
Copy link

Message from Gopher Robot:

Patch Set 1: TryBot-Result-1

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/488075.
After addressing review feedback, remember to publish your drafts!

gopherbot pushed a commit that referenced this pull request 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 pull request 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 pull request 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 pull request 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>
bassosimone pushed a commit to ooni/oocrypto that referenced this pull request May 29, 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: 1aad9bcf27f563449c1a7ed6d0dd1d247cc65713
GitHub-Pull-Request: golang/go#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 2c70690451f1484607a9172a4c24f78ae832dcb0)
Reviewed-on: https://go-review.googlesource.com/c/go/+/488055
Auto-Submit: Dmitri Shuralyov <dmitshur@google.com>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
bassosimone pushed a commit to ooni/oocrypto that referenced this pull request Apr 8, 2024
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 #59424

Change-Id: I2ca8948474275740a36d991c057b62a13392dbb9
GitHub-Last-Rev: 1aad9bcf27f563449c1a7ed6d0dd1d247cc65713
GitHub-Pull-Request: golang/go#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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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