Ensure that the key share group is allowed for our protocol version #19317
Conversation
This file contains 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
mattcaswell
added
branch: master
github-actions
bot
added
the
severity: fips change
|
fixes #8369? |
Yes - although that issue says "When openssl client is configured to support only one of the curves that is forbidden in TLS 1.3, it does send that curve in supported_groups extensions and it does also put appropriate key share into the key_share extension.". However the behaviour in master/3.0 at the moment is that we do not send the curve in supported_groups, but we do still send a key_share for it! This is obviously illegal. So, it looks to me like we already fixed the issue about sending illegal supported groups, but didn't fix they key_share aspect at the same time. This PR addresses the key_share problem. |
We should never send or accept a key share group that is not in the supported groups list or a group that isn't suitable for use in TLSv1.3
This should not happen but we should tolerate and send an HRR
Make sure that a TLSv1.3 only client does not send a TLSv1.3 key_share.
mattcaswell
force-pushed
the
fix-key-share
branch
from
94033fb
to
f6dda4e
Compare
|
I have rebased this PR now that #19315 has been merged. I also fixed a typo in the test which caused it to not operate correctly and pass on an unpatched version of OpenSSL! I have taken this out of "draft" state and it is ready for review. |
mattcaswell
added
approval: review pending
github-actions
bot
removed
the
severity: fips change
paulidale
approved these changes
Oct 10, 2022
paulidale
removed
the
approval: otc review pending
|
Ping for second review? |
beldmit
added
approval: done
openssl-machine
added
approval: ready to merge
|
This pull request is ready to merge |
openssl-machine
pushed a commit
that referenced
this pull request
Oct 12, 2022
We should never send or accept a key share group that is not in the supported groups list or a group that isn't suitable for use in TLSv1.3 Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> (Merged from #19317)
openssl-machine
pushed a commit
that referenced
this pull request
Oct 12, 2022
This should not happen but we should tolerate and send an HRR Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> (Merged from #19317)
openssl-machine
pushed a commit
that referenced
this pull request
Oct 12, 2022
Make sure that a TLSv1.3 only client does not send a TLSv1.3 key_share. Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> (Merged from #19317)
|
Pushed. Thanks. There was a conflict while cherry-picking to 3.0, so I raised a backport PR in #19404. |
beldmit
pushed a commit
to beldmit/openssl
that referenced
this pull request
Dec 26, 2022
We should never send or accept a key share group that is not in the supported groups list or a group that isn't suitable for use in TLSv1.3 Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> (Merged from openssl#19317)
beldmit
pushed a commit
to beldmit/openssl
that referenced
this pull request
Dec 26, 2022
This should not happen but we should tolerate and send an HRR Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> (Merged from openssl#19317)
beldmit
pushed a commit
to beldmit/openssl
that referenced
this pull request
Dec 26, 2022
Make sure that a TLSv1.3 only client does not send a TLSv1.3 key_share. Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> (Merged from openssl#19317)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
We should never send or accept a key share group that is not in the
supported groups list or a group that isn't suitable for use in TLSv1.3.
Currently if we put a TLSv1.2 group at the top of the groups list, but we only request TLSv1.3, then the supported groups list will only include TLSv1.3 groups, but the key_share will use a TLSv1.2 group. This results in the server failing with an invalid parameter alert.
server:
client:
This PR is based on #19315 and includes those commits - hence this PR is draft. I will take it out of draft once #19315 has been merged.