search

golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
124.08k stars 17.68k forks source link

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

Open avened opened 5 months ago

avened commented 5 months ago

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:

https://github.com/golang/go/blob/c83b1a7013784098c2061ae7be832b2ab7241424/src/crypto/tls/conn.go#L655

and

https://github.com/golang/go/blob/c83b1a7013784098c2061ae7be832b2ab7241424/src/crypto/tls/conn.go#L661

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 commented 5 months ago

Similar Issues

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

seankhliao commented 5 months ago

cc @golang/security

rolandshoemaker commented 5 months ago

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 commented 5 months ago

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:

https://github.com/golang/go/blob/c83b1a7013784098c2061ae7be832b2ab7241424/src/crypto/tls/handshake_client.go#L338

pickTLSVersion() setting internal versioning flags:

https://github.com/golang/go/blob/c83b1a7013784098c2061ae7be832b2ab7241424/src/crypto/tls/handshake_client.go#L527

readRecordOrCCS() using internal versioning for terminating connection:

https://github.com/golang/go/blob/c83b1a7013784098c2061ae7be832b2ab7241424/src/crypto/tls/conn.go#L654

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.