Incremental TLS handshake defragmentation #10011

gilles-peskine-arm · 2025-02-25T16:54:19Z

Defragment incoming TLS handshake messages incrementally, instead of all at the end. This makes the code simpler and a little more efficient.

Fixes defragmentation of encrypted messages with an explicit IV.

PR checklist

changelog not required because: fixes unreleased bug
development PR here
TF-PSA-Crypto PR not required because: TLS only
framework PR Incremental TLS handshake defragmentation tests mbedtls-framework#143
3.6 PR Backport 3.6: Incremental TLS handshake defragmentation #10030
2.28 PR not required because: feature not in 2.28
tests provided

minosgalanakis

Revied the range of commits specific to the change of fragment's re-assemly.

Looks good overall

minosgalanakis · 2025-03-04T17:48:32Z

library/ssl_msg.c

+#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) {


I assume that when MBEDTLS_SSL_VARIABLE_BUFFER_LENGTH is enabled, a buffer can only expand when resized?

Good question. A buffer can shrink when resized. But resizing only happens at the beginning and at the end of the handshake.

In any case, I don't think that matters here? We're checking whether the data fits now. If there's code that wants to resize the buffer, it would have to determine whether the data that's already in the buffer would still fit. Which, currently, is easy: resizing is only done at times when there's no data in the buffer.

gilles-peskine-arm · 2025-03-05T15:58:52Z

Here's a final rebase now that #10021 is merged.

Pacify `clang -Wdocumentation`. Signed-off-by: Gilles Peskine <[email protected]>

In preparation for reworking mbedtls_ssl_prepare_handshake_record(), tweak the "waiting for more handshake fragments" log message in ssl_consume_current_message(), and add a similar one in mbedtls_ssl_prepare_handshake_record(). Assert both. Signed-off-by: Gilles Peskine <[email protected]>

In preparation for reworking mbedtls_ssl_prepare_handshake_record(), tweak the "handshake fragment:" log message. This changes what information is displayed when a record contains data beyond the expected end of the handshake message. This case is currently untested and its handling will change in a subsequent commit. Signed-off-by: Gilles Peskine <[email protected]>

Minor refactoring of the initial checks and preparation when receiving the first fragment. Use `ssl->in_hsfraglen` to determine whether there is a pending handshake fragment, for consistency, and possibly for more robustness in case handshake fragments are mixed with non-handshake records (although this is not currently supported anyway). Signed-off-by: Gilles Peskine <[email protected]>

Signed-off-by: Gilles Peskine <[email protected]>

Reassemble handshake fragments incrementally instead of all at the end. That is, every time we receive a non-initial handshake fragment, append it to the initial fragment. Since we only have to deal with at most two handshake fragments at the same time, this simplifies the code (no re-parsing of a record) and is a little more memory-efficient (no need to store one record header per record). This commit also fixes a bug. The previous code did not calculate offsets correctly when records use an explicit IV, which is the case in TLS 1.2 with CBC (encrypt-then-MAC or not), GCM and CCM encryption (i.e. all but null and ChachaPoly). This led to the wrong data when an encrypted handshake message was fragmented (Finished or renegotiation). The new code handles this correctly. Signed-off-by: Gilles Peskine <[email protected]>

Pacify `clang -Wformat-pedantic`. Signed-off-by: Gilles Peskine <[email protected]>

minosgalanakis

~~LGTM~~

The pointer to the framework may need to be updated before merging, since we are failing component_test_tls13_only_psk because all tests are being skipped.

library/ssl_msg.c

Changed log messages and added more tests in `tests/opt-testcases/handshake-generated.sh`. Signed-off-by: Gilles Peskine <[email protected]>

gilles-peskine-arm · 2025-03-06T08:37:41Z

I had missed some commits when rebasing the framework. I've updated the framework branch and the pointer again, should be good now.

mpg

Looking good overall, but:

I'm afraid one of the bounds check is wrong (it's probably not needed anyway, but if we have it it should be correct).
Unless I'm mistaken there's an uncommented limitation compared to what the standard allows, which should have a comment.

mpg · 2025-03-06T11:30:23Z

library/ssl_msg.c

+                                   ssl->in_hslen));
+
+            const size_t hs_remain = ssl->in_hslen - ssl->in_hsfraglen;
+            if (ssl->in_msglen > hs_remain) {


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.)

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_message handles 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.

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.

mpg · 2025-03-06T11:40:39Z

library/ssl_msg.c

+#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) {


I think you mean payload_end + ssl->in_msglen here, to protect the call to memmove() below.

library/ssl_msg.c

Signed-off-by: Gilles Peskine <[email protected]>

A handshake record may contain multiple handshake messages, or multiple fragments (there can be the final fragment of a pending message, then zero or more whole messages, and an initial fragment of an incomplete message). This was previously untested, but supported, so don't break it. Signed-off-by: Gilles Peskine <[email protected]>

There is no longer any different processing at this point, just near-identical log messages. Signed-off-by: Gilles Peskine <[email protected]>

Signed-off-by: Gilles Peskine <[email protected]>

mpg

Looking pretty good! Just one issue in a debug string, and a suggestion to avoid potentially adding useless memory accesses even in the non-fragmented case.

library/ssl_msg.c

Signed-off-by: Gilles Peskine <[email protected]>

minosgalanakis

LGTM

mpg

LGTM, thanks!

gilles-peskine-arm added component-tls needs-ci Needs to pass CI tests size-s Estimated task size: small (~2d) priority-high High priority - will be reviewed soon labels Feb 25, 2025

minosgalanakis self-requested a review February 26, 2025 17:20

gilles-peskine-arm mentioned this pull request Feb 27, 2025

Add basic handshake defragmentation tests in ssl-opt #9989

Merged

6 tasks

mpg mentioned this pull request Feb 28, 2025

Defragment ext test 3.6 #9976

Merged

6 tasks

gilles-peskine-arm force-pushed the tls-defragment-incremental-dev branch 2 times, most recently from 06508ac to c75da8b Compare March 3, 2025 16:54

gilles-peskine-arm added the needs-preceding-pr Requires another PR to be merged first label Mar 3, 2025

gilles-peskine-arm mentioned this pull request Mar 3, 2025

Incremental TLS handshake defragmentation tests Mbed-TLS/mbedtls-framework#143

Merged

6 tasks

gilles-peskine-arm requested a review from mpg March 3, 2025 16:59

gilles-peskine-arm force-pushed the tls-defragment-incremental-dev branch 3 times, most recently from 129337e to 41e53cf Compare March 3, 2025 21:33

gilles-peskine-arm mentioned this pull request Mar 3, 2025

Backport 3.6: Incremental TLS handshake defragmentation #10030

Merged

6 tasks

gilles-peskine-arm force-pushed the tls-defragment-incremental-dev branch 3 times, most recently from ae8c65b to ce818da Compare March 4, 2025 09:37

minosgalanakis reviewed Mar 4, 2025

View reviewed changes

gilles-peskine-arm force-pushed the tls-defragment-incremental-dev branch from ce818da to e25976f Compare March 5, 2025 15:59

gilles-peskine-arm added 7 commits March 5, 2025 17:02

Fix Doxygen markup

2e5a7ea

Pacify `clang -Wdocumentation`. Signed-off-by: Gilles Peskine <[email protected]>

mbedtls_ssl_prepare_handshake_record(): log offsets after decryption

235eae9

Signed-off-by: Gilles Peskine <[email protected]>

Fix dodgy printf calls

90a9593

Pacify `clang -Wformat-pedantic`. Signed-off-by: Gilles Peskine <[email protected]>

gilles-peskine-arm force-pushed the tls-defragment-incremental-dev branch from e25976f to 1051a2e Compare March 5, 2025 16:02

gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, needs-backports Backports are missing or are pending review and approval. and removed needs-ci Needs to pass CI tests needs-preceding-pr Requires another PR to be merged first labels Mar 5, 2025

minosgalanakis previously approved these changes Mar 5, 2025

View reviewed changes

minosgalanakis reviewed Mar 5, 2025

View reviewed changes

library/ssl_msg.c Show resolved Hide resolved

Update framework

e4a3fc2

Changed log messages and added more tests in `tests/opt-testcases/handshake-generated.sh`. Signed-off-by: Gilles Peskine <[email protected]>

gilles-peskine-arm dismissed minosgalanakis’s stale review via e4a3fc2 March 6, 2025 08:35

gilles-peskine-arm force-pushed the tls-defragment-incremental-dev branch from 1051a2e to e4a3fc2 Compare March 6, 2025 08:35

mpg requested changes Mar 6, 2025

View reviewed changes

gilles-peskine-arm added 5 commits March 6, 2025 15:15

Fix end check before memmove

0851ec9

Signed-off-by: Gilles Peskine <[email protected]>

Unify handshake fragment log messages

afb254c

There is no longer any different processing at this point, just near-identical log messages. Signed-off-by: Gilles Peskine <[email protected]>

Pacify uncrustify

b8f1e4b

Signed-off-by: Gilles Peskine <[email protected]>

Note unused variables when debugging is disabled

dab1cb5

Signed-off-by: Gilles Peskine <[email protected]>

mpg requested changes Mar 7, 2025

View reviewed changes

library/ssl_msg.c Outdated Show resolved Hide resolved

library/ssl_msg.c Show resolved Hide resolved

Fix a log message

e34ec86

Signed-off-by: Gilles Peskine <[email protected]>

minosgalanakis approved these changes Mar 7, 2025

View reviewed changes

mpg approved these changes Mar 7, 2025

View reviewed changes

github-project-automation bot moved this from In Development to Has Approval in Roadmap pull requests (new board) Mar 7, 2025

mpg added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Mar 7, 2025

gilles-peskine-arm merged commit 723fec4 into Mbed-TLS:features/tls-defragmentation/development Mar 7, 2025
4 of 6 checks passed

github-project-automation bot moved this from Has Approval to Done in Roadmap pull requests (new board) Mar 7, 2025

Incremental TLS handshake defragmentation #10011

Incremental TLS handshake defragmentation #10011

Conversation

gilles-peskine-arm commented Feb 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Uh oh!

minosgalanakis left a comment

Choose a reason for hiding this comment

Uh oh!

minosgalanakis Mar 4, 2025

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm Mar 4, 2025

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm commented Mar 5, 2025

Uh oh!

minosgalanakis left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gilles-peskine-arm commented Mar 6, 2025

Uh oh!

mpg left a comment

Choose a reason for hiding this comment

Uh oh!

mpg Mar 6, 2025

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm Mar 6, 2025

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm Mar 6, 2025

Choose a reason for hiding this comment

Uh oh!

mpg Mar 6, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mpg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

minosgalanakis left a comment

Choose a reason for hiding this comment

Uh oh!

mpg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gilles-peskine-arm commented Feb 25, 2025 •

edited

Loading

minosgalanakis left a comment •

edited

Loading