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: support ECDHE key exchanges when ec_point_formats is missing in ClientHello extension #49126

Closed
yang-wei opened this issue Oct 23, 2021 · 16 comments
Assignees
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@yang-wei
Copy link

yang-wei commented Oct 23, 2021

What did you do?

Per rfc8422#section-5.1.2,

For backwards compatibility purposes, the point format list extension MAY still be included and contain exactly one value: the uncompressed point format (0). RFC 4492 specified that if this extension is missing, it means that only the uncompressed point format is supported, so interoperability with implementations that support the uncompressed format should work with or without the extension

We are seeing TLS handshake failure (client and server failed to agree on ECDHE_ECDSA key exchange algorithem) when ec_point_formats is missing because we expect it to be listed in tls/handshake_server.go

// supportsECDHE returns whether ECDHE key exchanges can be used with this
// pre-TLS 1.3 client.
func supportsECDHE(c *Config, supportedCurves []CurveID, supportedPoints []uint8) bool {
...
	supportsPointFormat := false
	for _, pointFormat := range supportedPoints {
		if pointFormat == pointFormatUncompressed {
			supportsPointFormat = true
			break
		}
	}

	return supportsCurve && supportsPointFormat
}

What did you expect to see?

If ec_point_formats is missing in ClientHello, we will allow ECDHE key exchanges because it means that only the uncompressed point format is supported

yang-wei added a commit to yang-wei/go that referenced this issue Oct 23, 2021
As describe in rfc8422 5.1.2, we will support ECDHE in the case client does not
include ec_point_formats extension in ClientHello extension. This make sure ECDHE
will work with (uncompressed point format is listed explicitly) or without extension.

rfc8422 5.1.2: https://datatracker.ietf.org/doc/html/rfc8422#section-5.1.2.

Fixes golang#49126
@gopherbot
Copy link

Change https://golang.org/cl/358116 mentions this issue: crypto: support ECDHE when ec_point_formats is missing in ClientHello

@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 24, 2021
@seankhliao
Copy link
Member

cc @FiloSottile

@yang-wei
Copy link
Author

Hey, how is everything? Any updates on this?

@wqweto
Copy link

wqweto commented Nov 18, 2021

This issue is affecting Windows 11 and Windows Server 2022 built-in Schannel library latest TLS 1.3 implementation.

It seems TLS1_3_CLIENT sends supported_groups (10) extension but does not send ec_point_formats (11) on ClientHello message.

Sites unreachable by TLS1_3_CLIENT:

@wqweto
Copy link

wqweto commented Nov 18, 2021

@yang-wei Did see this part from @gopherbot reply on PR #49127?

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://golang.org/s/release for more details.

@FiloSottile FiloSottile self-assigned this Mar 2, 2022
@FiloSottile FiloSottile added this to the Go1.19 milestone 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.

@cipherboy
Copy link
Contributor

cipherboy commented Jul 28, 2022

@ianlancetaylor Any movement on this? Hashicorp is also affected, per duplicate ticket. Though, in this case it is TLSv1.2-only client, not a Microsoft TLSv1.3 client (unlike this OP).

@wqweto
Copy link

wqweto commented Jul 28, 2022

@cipherboy Btw, the PR #49127 above is from Oct 23, 2021 and modifies a single line.

@rolandshoemaker
Copy link
Member

We're planning on landing a fix for this in 1.20.

@torntrousers
Copy link

So am i right in thinking 1.20 will be about early 2023? Is there really no way to get it out before then? Go is clearly broken and doesn't support the TLS spec properly and there is a one line fix...

@FiloSottile
Copy link
Contributor

Interestingly, this is only hit when using the Windows TLS1_3_CLIENT (or a different client that omits the extension) to reach a TLS 1.2-only Go server, because the extension is ignored entirely when negotiating TLS 1.3. This probably explains why it was only noticed for test sites (#49126 (comment)) which might disable TLS 1.3 to test for vulnerabilities, and why it didn't cause widespread breakage.

Still, considering the duplicates that were filed, the fact that we're the odd one out in getting this wrong, the fact that we're off-spec, the fact that this is not something applications can workaround (unless they can upgrade to TLS 1.3), how this kind of issue tends to add complexity to the ecosystem in the form of workarounds, and the simplicity of the fix, I think we should backport it.

PR #49127 / CL 358116 is an incomplete fix because we should not send the extension back when the client didn't send it. I'll send a new fix in a second.

@gopherbot please open backport issues for both supported releases.

@gopherbot
Copy link

Backport issue(s) opened: #54642 (for 1.18), #54643 (for 1.19).

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

@gopherbot
Copy link

Change https://go.dev/cl/425295 mentions this issue: crypto/tls: support ECDHE when ec_point_formats is missing

@gopherbot
Copy link

Change https://go.dev/cl/425635 mentions this issue: [release-branch.go1.19] crypto/tls: support ECDHE when ec_point_formats is missing

@gopherbot
Copy link

Change https://go.dev/cl/425636 mentions this issue: [release-branch.go1.18] crypto/tls: support ECDHE when ec_point_formats is missing

gopherbot pushed a commit that referenced this issue Aug 29, 2022
…ts is missing

Updates #49126
Fixes #54642

Change-Id: I9d6f6392b1a6748bdac1d2c6371b22d75829a2b6
Reviewed-on: https://go-review.googlesource.com/c/go/+/425295
Run-TryBot: Filippo Valsorda <filippo@golang.org>
Auto-Submit: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Alex Scheel <alex.scheel@hashicorp.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Roland Shoemaker <roland@golang.org>
Reviewed-by: David Chase <drchase@google.com>
(cherry picked from commit 1df2a03)
Reviewed-on: https://go-review.googlesource.com/c/go/+/425636
gopherbot pushed a commit that referenced this issue Aug 29, 2022
…ts is missing

Updates #49126
Fixes #54643

Change-Id: I9d6f6392b1a6748bdac1d2c6371b22d75829a2b6
Reviewed-on: https://go-review.googlesource.com/c/go/+/425295
Run-TryBot: Filippo Valsorda <filippo@golang.org>
Auto-Submit: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Alex Scheel <alex.scheel@hashicorp.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Roland Shoemaker <roland@golang.org>
Reviewed-by: David Chase <drchase@google.com>
(cherry picked from commit 1df2a03)
Reviewed-on: https://go-review.googlesource.com/c/go/+/425635
rajbarik pushed a commit to rajbarik/go that referenced this issue Sep 1, 2022
Fixes golang#49126

Change-Id: I9d6f6392b1a6748bdac1d2c6371b22d75829a2b6
Reviewed-on: https://go-review.googlesource.com/c/go/+/425295
Run-TryBot: Filippo Valsorda <filippo@golang.org>
Auto-Submit: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Alex Scheel <alex.scheel@hashicorp.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Roland Shoemaker <roland@golang.org>
Reviewed-by: David Chase <drchase@google.com>
bradfitz pushed a commit to tailscale/go that referenced this issue Sep 8, 2022
…ts is missing

Updates golang#49126
Fixes golang#54643

Change-Id: I9d6f6392b1a6748bdac1d2c6371b22d75829a2b6
Reviewed-on: https://go-review.googlesource.com/c/go/+/425295
Run-TryBot: Filippo Valsorda <filippo@golang.org>
Auto-Submit: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Alex Scheel <alex.scheel@hashicorp.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Roland Shoemaker <roland@golang.org>
Reviewed-by: David Chase <drchase@google.com>
(cherry picked from commit 1df2a03)
Reviewed-on: https://go-review.googlesource.com/c/go/+/425635
marten-seemann added a commit to quic-go/quic-go that referenced this issue Oct 9, 2022
The new qtls versions contain the standard library fix for
golang/go#49126.
marten-seemann added a commit to quic-go/quic-go that referenced this issue Oct 9, 2022
The new qtls versions contain the standard library fix for
golang/go#49126.
marten-seemann added a commit to quic-go/quic-go that referenced this issue Oct 11, 2022
The new qtls versions contain the standard library fix for
golang/go#49126.
rod-hynes pushed a commit to Psiphon-Labs/quic-go that referenced this issue Oct 13, 2022
@golang golang locked and limited conversation to collaborators Aug 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
Development

Successfully merging a pull request may close this issue.

11 participants