Fix inverted MinTlsVersion protocol version mapping - #438
Merged
Conversation
MinTlsVersion::V13 mapped to a ProtocolVersions value with both TLS 1.2 and 1.3 enabled, so it did not exclude TLS 1.2 and would not fail the handshake against a peer that only supports TLS 1.2. MinTlsVersion::V12 mapped to v12_only(), disabling TLS 1.3 entirely. Swap the arms so each matches its documented meaning: V12 allows TLS 1.2 and 1.3, V13 allows only TLS 1.3. Add tests asserting the configured version set for each variant, clarify the enum and FFI schema documentation, and correct the TLS section of the guide. Fixes #437
The guide referred to Tls1_2 and Tls1_3, which are not the variant names in any binding. Use V12 and V13, matching the convention already used for CertificateMode elsewhere in the same document, and attribute the default to the bindings rather than implying Rust has one.
jadamcrain
added
bug
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
From<MinTlsVersion> for ProtocolVersionshad its match arms swapped, so neither variant did what its name and documentation said:V12V13The security-relevant half is
V13: because TLS 1.2 remained enabled, the setting did not fail closed against a peer that only supports TLS 1.2, despite the guide stating it "will force the usage of TLSv1.3".V12— the default in the bindings — pinned connections to TLS 1.2 and made TLS 1.3 unavailable.This is not an exploitable weakness: rustls only offers hardened TLS 1.2 suites, and active downgrade is still caught by the TLS 1.2
ServerHellosentinel. The defect is that a stated security control did not enforce, and gave no signal that it hadn't.Introduced in 25da741 (#333), first shipped in 1.6.0.
Changes:
MinTlsVersiondoc comments and thev13FFI schema description to state the fail-closed behavior.Tls1_2,Tls1_3) that exist in no binding.:bell:note:V13users become stricter, and bindings users on the defaultV12gain TLS 1.3 negotiation.Known gap: the tests cover the configured
ProtocolVersions, which is what regressed, but not a live handshake — that needs certificate fixtures the repo doesn't currently have. Worth a follow-up.Fixes #437