backport PR 14749 to 1.1.1 (psk and key_share compliance fixes)#15252
Closed
kaduk wants to merge 4 commits intoOpenSSL_1_1_1-stablefrom
Closed
backport PR 14749 to 1.1.1 (psk and key_share compliance fixes)#15252kaduk wants to merge 4 commits intoOpenSSL_1_1_1-stablefrom
kaduk wants to merge 4 commits intoOpenSSL_1_1_1-stablefrom
Conversation
kaduk
added
branch: 1.1.1
t8m
approved these changes
May 12, 2021
t8m
added
approval: done
Contributor
Author
|
I think openssl-machine got confused about a line in the cherry-picked commit messages?? |
It's a MUST-level requirement that if the client sends a pre_shared_key extension not accompanied by a psk_key_exchange_modes extension, the server must abort the handshake. Prior to this commit the server would continue on. (cherry picked from commit efe0f31)
TLS 1.3 allows for the "psk_ke" and "psk_dhe_ke" key-exchange modes. Only the latter mode introduces a new ephemeral (Diffie-Hellman) key exchange, with the PSK being the only key material used in the former case. It's a compliance requirement of RFC 8446 that the server MUST NOT send a KeyShareEntry when using the "psk_ke" mode, but prior to this commit we would send a key-share based solely on whether the client sent one. This bug goes unnoticed in our internal test suite since openssl communicating with openssl can never negotiate the PSK-only key-exchange mode. However, we should still be compliant with the spec, so check whether the DHE mode was offered and don't send a key-share if it wasn't. (cherry picked from commit e776858)
One of the scenarios constructed in these tests was erroneously producing successful handshakes until the previous commits, but should have been failing. Update our expected behavior to match the specification requirements, and adjust the commentary slightly for a test case relevant for the other preceding commit. (cherry picked from commit 80c2561)
Contributor
Author
|
I renamed my pr14749 branch to pr14749-orig and force-pushed a rebased version that removed the "reviewed-by" and "merged from" lines, to try to forestall the machine getting confused again. I don't expect reapproval to be necessary, but I also don't mind waiting longer if needed. |
Contributor
Author
|
Oh, I see what's wrong; I'm pushing to the "wrong" github repo. Sigh. |
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
3 participants
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.
See #14749 for full description.
Needed to adapt to four-argument
SSLfinal()and correspondingmake updatebits. (This is not actually all the changes thatmake updatewants to make, but just the relevant ones.)