Incremental TLS handshake defragmentation #10011
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2962,23 +2962,34 @@ static uint32_t ssl_get_hs_total_len(mbedtls_ssl_context const *ssl) | |
|
|
||
| int mbedtls_ssl_prepare_handshake_record(mbedtls_ssl_context *ssl) | ||
| { | ||
| if (ssl->in_hsfraglen == 0) { | ||
| /* The handshake message must at least include the header. | ||
| * We may not have the full message yet in case of fragmentation. | ||
| * To simplify the code, we insist on having the header (and in | ||
| * particular the handshake message length) in the first | ||
| * fragment. */ | ||
| if (ssl->in_msglen < mbedtls_ssl_hs_hdr_len(ssl)) { | ||
| MBEDTLS_SSL_DEBUG_MSG(1, ("handshake message too short: %" MBEDTLS_PRINTF_SIZET, | ||
| ssl->in_msglen)); | ||
| return MBEDTLS_ERR_SSL_INVALID_RECORD; | ||
| } | ||
|
|
||
| ssl->in_hslen = mbedtls_ssl_hs_hdr_len(ssl) + ssl_get_hs_total_len(ssl); | ||
| } | ||
|
|
||
| MBEDTLS_SSL_DEBUG_MSG(3, ("handshake message: msglen =" | ||
| " %" MBEDTLS_PRINTF_SIZET ", type = %u, hslen = %" | ||
| MBEDTLS_PRINTF_SIZET, | ||
| ssl->in_msglen, ssl->in_msg[0], ssl->in_hslen)); | ||
|
|
||
| if (ssl->transform_in != NULL) { | ||
| MBEDTLS_SSL_DEBUG_MSG(4, ("decrypted handshake message:" | ||
| " iv-buf=%d hdr-buf=%d hdr-buf=%d", | ||
| (int) (ssl->in_iv - ssl->in_buf), | ||
| (int) (ssl->in_hdr - ssl->in_buf), | ||
| (int) (ssl->in_msg - ssl->in_buf))); | ||
| } | ||
|
|
||
| #if defined(MBEDTLS_SSL_PROTO_DTLS) | ||
| if (ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM) { | ||
| int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; | ||
|
|
@@ -3038,60 +3049,122 @@ int mbedtls_ssl_prepare_handshake_record(mbedtls_ssl_context *ssl) | |
| } | ||
| } else | ||
| #endif /* MBEDTLS_SSL_PROTO_DTLS */ | ||
| { | ||
| unsigned char *const reassembled_record_start = | ||
| ssl->in_buf + MBEDTLS_SSL_SEQUENCE_NUMBER_LEN; | ||
| unsigned char *const payload_start = | ||
| reassembled_record_start + mbedtls_ssl_in_hdr_len(ssl); | ||
| unsigned char *payload_end = payload_start + ssl->in_hsfraglen; | ||
|
|
||
| if (ssl->in_hsfraglen != 0) { | ||
| /* We already had a handshake fragment. Prepare to append | ||
| * to the initial segment. */ | ||
| MBEDTLS_SSL_DEBUG_MSG(3, | ||
| ("subsequent handshake fragment: %" MBEDTLS_PRINTF_SIZET | ||
| ", %" MBEDTLS_PRINTF_SIZET | ||
| "..%" MBEDTLS_PRINTF_SIZET | ||
| " of %" MBEDTLS_PRINTF_SIZET, | ||
| ssl->in_msglen, | ||
| ssl->in_hsfraglen, | ||
| ssl->in_hsfraglen + ssl->in_msglen, | ||
| ssl->in_hslen)); | ||
|
|
||
| const size_t hs_remain = ssl->in_hslen - ssl->in_hsfraglen; | ||
| if (ssl->in_msglen > hs_remain) { | ||
| MBEDTLS_SSL_DEBUG_MSG(1, ("Handshake fragment too long: %" | ||
| MBEDTLS_PRINTF_SIZET " but only %" | ||
| MBEDTLS_PRINTF_SIZET " of %" | ||
| MBEDTLS_PRINTF_SIZET " remain", | ||
| ssl->in_msglen, | ||
| hs_remain, | ||
| ssl->in_hslen)); | ||
| return MBEDTLS_ERR_SSL_INVALID_RECORD; | ||
| } | ||
| } else if (ssl->in_msglen == ssl->in_hslen) { | ||
| /* This is the sole fragment. */ | ||
| /* Emit a log message in the same format as when there are | ||
| * multiple fragments, for ease of matching. */ | ||
| MBEDTLS_SSL_DEBUG_MSG(3, | ||
| ("sole handshake fragment: %" MBEDTLS_PRINTF_SIZET | ||
| ", %" MBEDTLS_PRINTF_SIZET | ||
| "..%" MBEDTLS_PRINTF_SIZET | ||
| " of %" MBEDTLS_PRINTF_SIZET, | ||
| ssl->in_msglen, | ||
| ssl->in_hsfraglen, | ||
| ssl->in_hsfraglen + ssl->in_msglen, | ||
| ssl->in_hslen)); | ||
| } else { | ||
| /* This is the first fragment of many. */ | ||
| MBEDTLS_SSL_DEBUG_MSG(3, | ||
| ("initial handshake fragment: %" MBEDTLS_PRINTF_SIZET | ||
| ", %" MBEDTLS_PRINTF_SIZET | ||
| "..%" MBEDTLS_PRINTF_SIZET | ||
| " of %" MBEDTLS_PRINTF_SIZET, | ||
| ssl->in_msglen, | ||
| ssl->in_hsfraglen, | ||
| ssl->in_hsfraglen + ssl->in_msglen, | ||
| ssl->in_hslen)); | ||
| } | ||
|
|
||
| /* Move the received handshake fragment to have the whole message | ||
| * (at least the part received so far) in a single segment at a | ||
| * known offset in the input buffer. | ||
| * - When receiving a non-initial handshake fragment, append it to | ||
| * the initial segment. | ||
| * - Even the initial handshake fragment is moved, if it was | ||
mpg marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| * encrypted with an explicit IV: decryption leaves the payload | ||
| * after the explicit IV, but here we move it to start where the | ||
| * IV was. | ||
| */ | ||
| #if defined(MBEDTLS_SSL_VARIABLE_BUFFER_LENGTH) | ||
| size_t const in_buf_len = ssl->in_buf_len; | ||
| #else | ||
| size_t const in_buf_len = MBEDTLS_SSL_IN_BUFFER_LEN; | ||
| #endif | ||
| if (payload_end + ssl->in_hsfraglen > ssl->in_buf + in_buf_len) { | ||
|
||
| MBEDTLS_SSL_DEBUG_MSG(1, | ||
| ("Shouldn't happen: no room to move handshake fragment %" | ||
| MBEDTLS_PRINTF_SIZET " from %p to %p (buf=%p len=%" | ||
| MBEDTLS_PRINTF_SIZET ")", | ||
| ssl->in_msglen, | ||
| (void *) ssl->in_msg, (void *) payload_end, | ||
| (void *) ssl->in_buf, in_buf_len)); | ||
| return MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; | ||
| } | ||
| memmove(payload_end, ssl->in_msg, ssl->in_msglen); | ||
|
|
||
| ssl->in_hsfraglen += ssl->in_msglen; | ||
minosgalanakis marked this conversation as resolved.
Show resolved
Hide resolved
mpg marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| payload_end += ssl->in_msglen; | ||
|
|
||
| if (ssl->in_hsfraglen < ssl->in_hslen) { | ||
| MBEDTLS_SSL_DEBUG_MSG(3, ("Prepare: waiting for more handshake fragments %" | ||
| MBEDTLS_PRINTF_SIZET "/%" | ||
| MBEDTLS_PRINTF_SIZET, | ||
| ssl->in_hsfraglen, ssl->in_hslen)); | ||
| ssl->in_hdr = payload_end; | ||
| ssl->in_msglen = 0; | ||
| mbedtls_ssl_update_in_pointers(ssl); | ||
| return MBEDTLS_ERR_SSL_CONTINUE_PROCESSING; | ||
| } else { | ||
| ssl->in_msglen = ssl->in_hsfraglen; | ||
| ssl->in_hsfraglen = 0; | ||
| ssl->in_hdr = reassembled_record_start; | ||
| mbedtls_ssl_update_in_pointers(ssl); | ||
|
|
||
| /* Update the record length in the fully reassembled record */ | ||
| if (ssl->in_msglen > 0xffff) { | ||
mpg marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| MBEDTLS_SSL_DEBUG_MSG(1, | ||
| ("Shouldn't happen: in_msglen=%" | ||
| MBEDTLS_PRINTF_SIZET " > 0xffff", | ||
| ssl->in_msglen)); | ||
| return MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; | ||
| } | ||
| MBEDTLS_PUT_UINT16_BE(ssl->in_msglen, ssl->in_len, 0); | ||
|
|
||
| MBEDTLS_SSL_DEBUG_BUF(4, "reassembled record", | ||
| ssl->in_hdr, | ||
| mbedtls_ssl_in_hdr_len(ssl) + ssl->in_msglen); | ||
| } | ||
| } | ||
|
|
||
| return 0; | ||
|
|
@@ -4438,11 +4511,9 @@ static int ssl_consume_current_message(mbedtls_ssl_context *ssl) | |
|
|
||
| if (ssl->in_hsfraglen != 0) { | ||
| /* Not all handshake fragments have arrived, do not consume. */ | ||
| MBEDTLS_SSL_DEBUG_MSG(3, ("Consume: waiting for more handshake fragments %" | ||
| MBEDTLS_PRINTF_SIZET "/%" MBEDTLS_PRINTF_SIZET, | ||
| ssl->in_hsfraglen, ssl->in_hslen)); | ||
| return 0; | ||
| } | ||
|
|
||
|
|
||
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
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.
IIUC we're being stricter than the protocol here: this condition means that there's more data in the record after the rest of this handshake message, presumably the (beginning of) the next handshake message, which is allowed by the standard unless I'm mistaken.
I don't have an issue with rejecting it anyway, but I'd like a comment explaining that we are rejecting something the standard allows and why we're doing that. Also, that's a limitation we should document. And I find the current debug message a bit confusing.
(Unless I'm mistaken about what's happening here.)
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.
Right.
I added this check because I believed that the old code silently ignored subsequent handshake messages if there is more than one in the same record. However, revisiting this, I'm not sure that was the case: here we do nothing, but
ssl_consume_current_messagehandles it. So now I'm worried that I've broken some existing untested behavior. But if I remove the check here, I'm worried that the defragmentation code may be creating an unexpected state. I'll keep digging today.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.
Leveraging #9976, I've crafted a test case with ServerHello and ServerCertificate in the same message, and that test case passes before incremental defragmentation. So this check removes functionality, and I'm going to remove it.