Repository navigation
Fix brainpool group and version downgrade handling for TLS 1.3, DTLS 1.3 and ECH #11857
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
Open
Frauschi
wants to merge
13
commits into
wolfSSL:master
Choose a base branch
from
Frauschi:fix-brainpool-downgrade
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
279a8df
Offer the RFC 7027 brainpool groups only when TLS 1.2 or older is rea…
Frauschi ee871a2
Downgrade a DTLS 1.3 SSL when TLS 1.3 is masked off
Frauschi c39d83b
Keep the RFC 7027 brainpool groups out of the ECH inner ClientHello
Frauschi 4fcd339
Address review on DTLS 1.3 downgrade and ECH outer hello
Frauschi a289daa
Check the DTLS version mask when the handshake starts
Frauschi 8a6a321
Rebuild suites and cover all entry points in the DTLS mask check
Frauschi 1f1c30e
Apply the DTLS version mask in set_options and on every ClientHello
Frauschi a19568c
Start the DTLS mask walk from the method version when resuming
Frauschi 885727e
Pick only listed DTLS versions and share the server mask check
Frauschi 5cd7874
Apply the DTLS mask when a session is set, not mid-ClientHello
Frauschi b3068dd
Add SSL_OP_NO_DTLSv1 and apply DTLS min and max proto versions
Frauschi a74729c
Hand pre-1.3 DTLS connections to the legacy handshake
Frauschi a6a5205
Let DTLS max proto and options lift an earlier cap
Frauschi File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Openssl doesn't use
SSL_OP_NO_TLSv1_3for DTLS. Why are we allowing this?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OpenSSL does now. 4.1, the first release with DTLS 1.3, defines
SSL_OP_NO_DTLSv1_3asSSL_OP_BIT(29), the same bit asSSL_OP_NO_TLSv1_3, and uses it as the mask of the DTLS 1.3 methods (ssl/methods.c).DTLS_method()picks its version inssl_get_min_max_version()when the handshake starts, skipping masked versions, soSSL_OP_NO_DTLSv1_3gives DTLS 1.2 there.wolfSSL master already did half of this:
TLSX_SupportedVersions_GetSize()drops DTLS 1.3 from supported_versions underWOLFSSL_OP_NO_TLSv1_3, but the SSL stayed on DTLS 1.3 and still sent a TLS 1.3 key_share and the TLS 1.3-only groups. The PR now follows OpenSSL.wolfSSL_set_options()andwolfSSL_set_session()move the version straight away.wolfSSL_connect()/wolfSSL_accept()and the_TLSv13entry points check the accumulated mask again when the handshake starts (DtlsCheckVersionMask()), so the order of the calls doesn't matter. On the server, the supported_versions parse only picks versions the mask allows. I also addedSSL_OP_NO_DTLSv1_2andSSL_OP_NO_DTLSv1_3, which use the same bits as in OpenSSL. I left outSSL_OP_NO_DTLSv1: OpenSSL defines it as theSSL_OP_NO_TLSv1bit, while wolfSSL masks DTLS 1.0 withWOLFSSL_OP_NO_TLSv1_1, so the alias would behave differently.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, the OpenSSL 4.1 behavior checks out.
On SSL_OP_NO_DTLSv1: WOLFSSL_OP_NO_TLSv1_1 is 0x04000000, bit 26. That’s the same value OpenSSL uses for SSL_OP_NO_DTLSv1. So #define SSL_OP_NO_DTLSv1 WOLFSSL_OP_NO_TLSv1_1 would match OpenSSL’s value and mask DTLS 1.0 the way wolfSSL does. The only difference is on a TLS context, where it would mask TLS 1.1 rather than TLS 1.0. Code that sets a DTLS option on a TLS context should be rare.
Without the alias, code ported from OpenSSL that uses SSL_OP_NO_DTLSv1 won’t compile. I’d add the alias and document the TLS-context difference. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, I had the bits wrong. wolfSSL's
WOLFSSL_OP_NO_TLSv1_1is OpenSSL'sSSL_OP_NO_TLSv1/SSL_OP_NO_DTLSv1value, so the alias matches OpenSSL's value and wolfSSL's DTLS 1.0 masking. AddedSSL_OP_NO_DTLSv1asWOLFSSL_OP_NO_TLSv1_1, and thewolfSSL_CTX_set_options()doc table now lists the three DTLS options, with the TLS 1.1 effect on a TLS object called out.test_dtls13_no_tls13_optionuses the new name.