Prevent TLS1.2 handshake fragments from entering TLS1.3 post-handshak… - #6039
Open
FreezB11 wants to merge 2 commits into
Open
Prevent TLS1.2 handshake fragments from entering TLS1.3 post-handshak…#6039FreezB11 wants to merge 2 commits into
FreezB11 wants to merge 2 commits into
Conversation
Contributor
Author
|
@kaukabrizvi @jmayclin, please do review when free thanks |
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
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.
Description
Fixes #5624.
s2n_post_handshake_recv()treated anyTLS_HANDSHAKE-typed record as a post-handshake message, even if the handshake hadn't actually finished. If a caller's completion tracking diverges from s2n-tls's internal state (e.g. the peer's finalFinishedmessage wasn't fully consumed yet), leftover handshake bytes get misread as a post-handshake message header — resulting in eitherS2N_ERR_BAD_MESSAGE, or worse, silent success if the misread bytes happened to look like aTLS_HELLO_REQUEST(which is silently ignored by default).Fix
s2n_post_handshake_recv()now requiress2n_handshake_is_complete(conn)before processing anything, returning the existingS2N_ERR_HANDSHAKE_NOT_COMPLETEotherwise.I deliberately checked handshake completeness rather than gating on
actual_protocol_version < S2N_TLS13(as suggested in the issue). TLS1.2HelloRequestis a legitimate message this same function handles, so a blanket version check would make that unreachable.s2n_handshake_is_complete()correctly distinguishes leftover/unconsumed bytes (the bug) from a genuinely-finished TLS1.2 connection receivingHelloRequestor a finished TLS1.3 connection receivingKeyUpdate/NewSessionTicket(which already have their own version checks).Testing
s2n_post_handshake_test.c) reproducing the exact scenario: negotiates a real TLS1.2 handshake, stalls the client right before it reads the server'sFinished, then asserts the leftover bytes are rejected withS2N_ERR_HANDSHAKE_NOT_COMPLETE.s2n_skip_handshake()helper, matching real-world state.s2n_early_data_io_test.calready covered a variant of this bug class; it now correctly getsS2N_ERR_HANDSHAKE_NOT_COMPLETEinstead ofS2N_ERR_BAD_MESSAGE.HelloRequestwas accepted on a TLS1.3 connection — only passing because the handshake was never markedNEGOTIATED, bypassing a pre-existing version check. Updated to assert the correct rejection.ctest -j$(nproc)).