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: ECH decodeInnerClientHello incorrectly rejects ClientHello with GREASE values in supportedVersions #71642

Open
gdy666 opened this issue Feb 10, 2025 · 5 comments
Labels
FixPending Issues that have a fix which has not yet been reviewed or submitted. NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@gdy666
Copy link

gdy666 commented Feb 10, 2025

Problem Description

In Golang's crypto/tls implementation, the function decodeInnerClientHello contains the following code:

if len(inner.supportedVersions) != 1 || (len(inner.supportedVersions) >= 1 && inner.supportedVersions[0] != VersionTLS13) {
    return nil, errors.New("tls: client sent encrypted_client_hello extension and offered incompatible versions")
}

This logic requires supportedVersions to contain exactly one version, which must be TLS 1.3, otherwise, the connection is rejected.

However, Chrome and Edge use the GREASE mechanism, which means supportedVersions may contain multiple values, such as:

supported_versions: [GREASE, TLS 1.3]

Since len(inner.supportedVersions) != 1, Golang incorrectly rejects these connections, causing Chrome and Edge to fail when accessing a Golang Web server with ECH enabled, while Firefox works fine.

Steps to Reproduce

Start a Golang crypto/tls server with ECH enabled.
Try accessing the server using Chrome or Edge.
The connection fails with a TLS error in Chrome.
Try accessing the server using Firefox.
The connection works fine.

Expected Behavior

The server should accept connections as long as supportedVersions contains TLS 1.3, rather than rejecting them due to the GREASE mechanism.

Suggested Fix

Modify decodeInnerClientHello logic to:

Ignore GREASE values (i.e., values with the format 0xXAXA).
Allow multiple versions in supportedVersions, as long as TLS 1.3 is included.
Proposed Code Change

hasTLS13 := false
  for _, v := range inner.supportedVersions {
      if v == VersionTLS13 {
          hasTLS13 = true
          break
      }
  }

  if !hasTLS13 {
      return nil, errors.New("tls: client sent encrypted_client_hello extension but did not offer TLS 1.3")
  }

This ensures compatibility with Chrome and Edge while maintaining ECH security.

Related Fix Proposal
🔗 Gerrit CL: https://go-review.googlesource.com/c/go/+/648015

@gdy666 gdy666 closed this as completed Feb 10, 2025
@gdy666 gdy666 reopened this Feb 10, 2025
@gdy666
Copy link
Author

gdy666 commented Feb 10, 2025

@rolandshoemaker

@gdy666
Copy link
Author

gdy666 commented Feb 10, 2025

Ensure ECH processing allows multiple versions while enforcing TLS 1.3+ requirement

	hasTLS13 := false
	for _, v := range inner.supportedVersions {
		if v == VersionTLS13 {
			hasTLS13 = true
		} else if v < VersionTLS13 {
			return nil, errors.New("tls: client sent encrypted_client_hello extension with unsupported versions")
		}
	}

	if !hasTLS13 {
		return nil, errors.New("tls: client sent encrypted_client_hello extension but did not offer TLS 1.3")
	}

@rolandshoemaker
Copy link
Member

Ah, right, GREASE.

Checking for < VersionTLS13 is probably fine, although I guess skipping values > 0x0A0A would be more explicit. Either way we should have a comment indicating why we do this.

@gdy666
Copy link
Author

gdy666 commented Feb 10, 2025

啊,对,GREASE。

检查 可能没问题,尽管我想跳过值会更明确。无论哪种方式,我们都应该有一个注释来说明我们为什么要这样做。< VersionTLS13``> 0x0A0A

Thanks for the feedback!
I’ve updated the code to explicitly skip GREASE values (0x?A0A) and added a detailed comment explaining why we do this.
Let me know if any further refinements are needed!

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/648015 mentions this issue: crypto/tls: fix ECH compatibility

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. FixPending Issues that have a fix which has not yet been reviewed or submitted. labels Feb 10, 2025
@dmitshur dmitshur added this to the Go1.25 milestone Feb 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FixPending Issues that have a fix which has not yet been reviewed or submitted. NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

4 participants