Skip to content

crypto/tls: ECH decodeInnerClientHello incorrectly rejects ClientHello with GREASE values in supportedVersions [1.24 backport] #73118

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.

Sign up for GitHub

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

Jump to bottom
Closed
gopherbot opened this issue Apr 1, 2025 · 8 comments
Closed

crypto/tls: ECH decodeInnerClientHello incorrectly rejects ClientHello with GREASE values in supportedVersions [1.24 backport] #73118

gopherbot opened this issue Apr 1, 2025 · 8 comments
Labels
CherryPickApproved Used during the release process for point releases
Milestone
Go1.24.3

Comments

@gopherbot
Copy link
Contributor

@gdy666 requested issue #71642 to be considered for backport to the next 1.24 minor release.

@gopherbot please backport to go 1.24
@gopherbot gopherbot added the CherryPickCandidate Used during the release process for point releases label Apr 1, 2025
@gopherbot gopherbot mentioned this issue Apr 1, 2025
Closed
@gopherbot gopherbot added this to the Go1.24.2 milestone Apr 1, 2025
@gopherbot
Copy link
Contributor Author

Change https://go.dev/cl/661936 mentions this issue: [release-branch.go1.24] crypto/tls: fix ECH compatibility

@gopherbot gopherbot modified the milestones: Go1.24.2, Go1.24.3 Apr 1, 2025
@cagedmantis
Copy link
Contributor

@gdy666 @rolandshoemaker It would be helpful to provide a reason for why a backport is requested.

@cagedmantis cagedmantis added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Apr 2, 2025
@gdy666
Copy link
Contributor

gdy666 commented Apr 3, 2025

It would be helpful to provide a reason for why a backport is requested.

I would like to request a backport to the 1.24 branch because the fix addresses a critical compatibility issue specifically related to the use of Encrypted Client Hello (ECH) with Chromium-based browsers like Chrome and Edge. These browsers have significant market shares, and the current implementation fails to properly negotiate TLS 1.3 with ECH enabled, which could disrupt a substantial number of users and applications.

This modification only involves an improvement in the version checking logic and does not introduce any new features, making the risk of backporting extremely low. By addressing this issue, we improve reliability and user trust, crucial for environments where Go is used in secure communications.

@rolandshoemaker
Copy link
Member

I agree it makes sense to backport this. The change is minimal, and only makes the implementation more permissive, so is unlikely to break any users, just remove some existing incompatibility.

@dmitshur
Copy link
Contributor

dmitshur commented Apr 3, 2025 •
edited
Loading

Can you please clarify if the request to backport to the 1.24 branch is because the problem only exists in 1.24 but not 1.23? Or does this problem affect 1.23 too? We need this information because the release policy is to support last two releases equally. Thanks.

@rolandshoemaker
Copy link
Member

ECH server support was added in 1.24, this functionality is not present in 1.23.

@dmitshur dmitshur removed the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Apr 3, 2025
@prattmic prattmic added the CherryPickApproved Used during the release process for point releases label Apr 16, 2025
@gopherbot gopherbot removed the CherryPickCandidate Used during the release process for point releases label Apr 16, 2025
@christaikobo christaikobo mentioned this issue Apr 19, 2025
Closed
@tomfrenzel
Copy link

Hi @rolandshoemaker! Is there any update on this? I saw that this fix is supposed to be shipped with 1.24.3 but its change for the release branch is still unreviewd

gopherbot pushed a commit that referenced this issue Apr 28, 2025
@gdy666 @cagedmantis
[release-branch.go1.24] crypto/tls: fix ECH compatibility
b2c005e 
Previously, the code only checked supportedVersions[0] for TLS 1.3
However, Chromium-based
browsers may list TLS 1.3 at different positions, causing ECH failures.
This fix:
    Iterates through supportedVersions to accept connections as long as TLS 1.3 is present.
    Improves ECH compatibility, ensuring Chrome, Edge, and other browsers work properly.

Fixes #73118

Change-Id: I32f4219fb6654d5cc22c7f33497c6142c0acb4f2
Reviewed-on: https://go-review.googlesource.com/c/go/+/648015
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Roland Shoemaker <[email protected]>
Reviewed-by: David Chase <[email protected]>
Reviewed-by: Daniel McCarney <[email protected]>
(cherry picked from commit cd2f347)
Reviewed-on: https://go-review.googlesource.com/c/go/+/661936
Reviewed-by: Carlos Amedee <[email protected]>
Reviewed-by: 古大羊 <[email protected]>
@gopherbot
Copy link
Contributor Author

Closed by merging CL 661936 (commit b2c005e) to release-branch.go1.24.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
CherryPickApproved Used during the release process for point releases
Projects
None yet
Milestone
Go1.24.3
Development

No branches or pull requests
7 participants
@cagedmantis @prattmic @dmitshur @gdy666 @rolandshoemaker @gopherbot @tomfrenzel