crypto/tls: do not enforce legacy_record_version while reading TLS 1.3 records

[avened]

Go version

go version go1.21.6 linux/amd64

Output of go env in your module/workspace:

[...]

What did you do?

Called crypto/tls tls.Client() to connect to particular TLS server with TLS 1.3.

What did you see happen?

Error: "tls: received record with version 301 when expecting version 303".

What did you expect to see?

Successful connection, no error.

I believe this is a side effect of TLS version field enforcement while reading TLS records. See these particular lines in conn.go:

go/src/crypto/tls/conn.go

Line 655 in c83b1a7

if expectedVers == VersionTLS13 {

and

go/src/crypto/tls/conn.go

Line 661 in c83b1a7

if c.haveVers && vers != expectedVers {

Although legacy_record_version must indeed be set to 0x0303, - as by RFC 8446 Section 5.1, - "for all records generated by a TLS 1.3 implementation other than an initial ClientHello", the very same paragraph of RFC 8446 clearly requires to otherwise ignore the legacy_record_version value, as "[t]his field is deprecated". It seems that client reading TLS records for TLS 1.3 should tolerate "wrong value" discovered in record header, possibly by completely ignoring it. And this was the exact case before version enforcement, thus enabling successful TLS 1.3 connections to some "buggy" servers, which is no longer possible with newer crypto/tls. Please, consider reverting to former version-tolerant variant.

[gabyhelp]

Similar Issues

• crypto/tls: reject change_cipher_spec record after handshake in TLS 1.3 #58912
• crypto/tls: after switching to TLSv1.3+ Connect no longer returns errors #42656

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[seankhliao]

cc @golang/security

[rolandshoemaker]

RFC 8446 Appendix D also states:

The value of TLSCiphertext.legacy_record_version is
included in the additional data for deprotection but MAY otherwise be
ignored or MAY be validated to match the fixed constant value.

So I think our behavior here is compliant.

Are you encountering a lot of buggy TLS implementations that trigger this failure case? Given this was introduced in 1.21 and this is the first we are hearing of issues with this, it seems uncommon enough to keep enforcing.

[avened]

RFC 8446 Appendix D also states:

Thanks for clarification. This fragment of Appendix D describes protected records, but I now see the point. The issue with code appears to be a little bit more complicated.

There is a call to pickTLSVersion() in clientHandshake() (handshake_client.go) which sets internal TLS version (c.vers = vers; c.haveVers = true) before actual handshake completes. But for client and TLS 1.3 this means that TLS version from HelloRetryRequest will be set as a negotiated version (haveVers), which, I suppose, is incorrect. After HelloRetryRequest second ServerHello follows and it comes in plain-text record, as client has not yet negotiated crypto context. This is TLSPlaintext.legacy_record_version which must be ignored according to RFC 8446, and Appendix D just restates the same requirement, as I see it.

These are links to code:

Picking version in clientHandshake() prior HelloRetryRequest check:

go/src/crypto/tls/handshake_client.go

Line 338 in c83b1a7

if err := c.pickTLSVersion(serverHello); err != nil {

pickTLSVersion() setting internal versioning flags:

go/src/crypto/tls/handshake_client.go

Line 527 in c83b1a7

c.vers = vers

readRecordOrCCS() using internal versioning for terminating connection:

go/src/crypto/tls/conn.go

Line 654 in c83b1a7

expectedVers := c.vers

If possible, consider reverting to former relaxed variant or, maybe, change implementation steps to better suit HelloRetryRequest flow when talking to "buggy" servers.

Are you encountering a lot of buggy TLS implementations that trigger this failure case?

I've encountered it while investigating some peculiar discrepancy in internal go-enabled TLS tools (Go is marvellous for automating such chores). So, it is definitely special enough to be considered rare, but even rarest things are better with library mended. Nevertheless, browsers seem to handle the same connection without errors.