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: set TLSPlaintext.version to MinVersion #31104

Closed
riraccuia opened this issue Mar 28, 2019 · 6 comments
Closed

crypto/tls: set TLSPlaintext.version to MinVersion #31104

riraccuia opened this issue Mar 28, 2019 · 6 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@riraccuia
Copy link

riraccuia commented Mar 28, 2019

In its current state, the crypto/tls package does not allow an implementer to choose whether a TLS 1.2 client should present the same TLS version (0x0303) in either record and handshake layers of the ClientHello message.

The current default behavior just sets the version in the record layer to 1.0 to maintain compatibility with legacy/buggy TLS server implementations.

This problem is described in RFC 5246 Appendix E.1

This may apply to TLS 1.1 as well

@FiloSottile @bradfitz

@FiloSottile
Copy link
Contributor

Apologies for responding to #31079 before seeing this.

I still don't see the compelling use case for this config option. I'm aware of no servers that require 0x0303 as the protocol version, which would be off-spec behavior.

When we have the option to select a default that works for everyone, we always prefer it to exposing a config option and delegating the complexity and learning requirement to the application.

@FiloSottile FiloSottile changed the title crypto/tls: add StrictVersionNegotiation config field for TLS 1.2 proposal: crypto/tls: add StrictVersionNegotiation config field for TLS 1.2 Mar 28, 2019
@gopherbot gopherbot added this to the Proposal milestone Mar 28, 2019
@FiloSottile FiloSottile added Proposal-Crypto Proposal related to crypto packages or other security issues and removed Proposal labels Mar 28, 2019
@dmagyar
Copy link

dmagyar commented Mar 28, 2019

In our use-case an IPS is blocking negotiation when attempted at <TLS1.2. Being a network appliance this decision is made by observing ClientHello and therefore even if I set MinVersion to 1.2 I'm getting blocked. Understand that the default works for most use-cases but there could be edge cases where a proper version in ClientHello is required. All modern browsers adhere to proper TLS versions, so far the only issue I had is with Golangs' TLS :(

@FiloSottile
Copy link
Contributor

If the IPS is abusing TLSPlaintext.version as a sort of minimum supported version, it is not compliant with RFC 5246. From Section E.1:

   Earlier versions of the TLS specification were not fully clear on
   what the record layer version number (TLSPlaintext.version) should
   contain when sending ClientHello (i.e., before it is known which
   version of the protocol will be employed).  Thus, TLS servers
   compliant with this specification MUST accept any value {03,XX} as
   the record layer version number for ClientHello.

   TLS clients that wish to negotiate with older servers MAY send any
   value {03,XX} as the record layer version number.  Typical values
   would be {03,00}, the lowest version number supported by the client,
   and the value of ClientHello.client_version.  No single value will
   guarantee interoperability with all old servers, but this is a
   complex topic beyond the scope of this document.

I don't know what you mean about browsers, but my Chrome instance sends 0x0301 as TLSPlaintext.version. I suppose we could set it to tls.Config.MinVersion, rather than make it configurable, if that's what you need.

@dmagyar
Copy link

dmagyar commented Mar 28, 2019

I would love that too. If MinVersion is not set then default to current behavior.

@FiloSottile FiloSottile changed the title proposal: crypto/tls: add StrictVersionNegotiation config field for TLS 1.2 crypto/tls: set TLSPlaintext.version to MinVersion Mar 28, 2019
@FiloSottile FiloSottile added NeedsFix The path to resolution is known, but the work has not been done. and removed Proposal Proposal-Crypto Proposal related to crypto packages or other security issues labels Mar 28, 2019
@FiloSottile FiloSottile modified the milestones: Proposal, Go1.13 Mar 28, 2019
@riraccuia
Copy link
Author

Thanks @FiloSottile ,
setting it to MinVersion is actually perfect and i don't see any downsides with this approach.
I should have thought of that before submitting a new API proposal.

@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@FiloSottile FiloSottile modified the milestones: Backlog, Go1.14 Oct 23, 2019
@FiloSottile
Copy link
Contributor

Actually found an authoritative answer in RFC 8446, Section 5.1: the ClientHello record layer version may only be 0x0303 or 0x0301.

   legacy_record_version:  MUST be set to 0x0303 for all records
      generated by a TLS 1.3 implementation other than an initial
      ClientHello (i.e., one not generated after a HelloRetryRequest),
      where it MAY also be 0x0301 for compatibility purposes.  This
      field is deprecated and MUST be ignored for all purposes.
      Previous versions of TLS would use other values in this field
      under some circumstances.

We already set it to 0x0301, so it looks like doing anything else would be off-spec.

@golang golang locked and limited conversation to collaborators Nov 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

6 participants