TLS handshake fragmentation support #8981

rojer · 2024-03-22T00:20:20Z

Description

Adds support for max_frag_len extension in TLS mode.

Consists of two parts:

The change itself - fragmenting records on the way out, defragmenting on the way in
A fix for max_frag_len negotiation on the client side: if the server ignores the extension, we must fall back to the maximum value, irrespective of what was configured.

This approach was extensively tested in production in a big fleet of devices. However, this exact code is new (i'm forward-porting patches to 2.16 which was our previous base version).

Fixes #1840

PR checklist

Note: this is not a finished product. If there's agreement in principle, i will add tests, changelog, etc.

Please tick as appropriate and edit the reasons (e.g.: "backport: not needed because this is a new feature")

changelog - TODO
3.6 backport no: ABI break (also, new feature)
2.28 backport no: ABI break (also, new feature)
tests - TODO

Notes for the submitter

Please refer to the contributing guidelines, especially the
checklist for PR contributors.

Help make review efficient:

Multiple simple commits
- please structure your PR into a series of small commits, each of which does one thing
Avoid force-push
- please do not force-push to update your PR - just add new commit(s)
See our Guidelines for Contributors for more details about the review process.

paul-elliott-arm · 2024-03-22T16:26:52Z

CI Failed, as under some configs:

ssl_msg.c:3330:22: error: unused variable 'msg_hslen' [-Werror=unused-variable]
          const size_t msg_hslen = (hs_remain <= ssl->in_msglen ? hs_remain : ssl->in_msglen);

(You can see full test results by clicking on the Details link for the OpenCI PR Tests below)

rojer · 2024-03-23T00:47:24Z

yes, looks like a temporary variable was unused when building without debug. fixed.

but unused variables aside, does the overall approach look good?

library/ssl_msg.c

library/ssl_tls12_client.c

Support for MBEDTLS_SSL_MAX_FRAGMENT_LENGTH in stream TLS mode Signed-off-by: Deomid rojer Ryabkov <rojer@rojer.me>

If the server does not acknowledge our fragment length proposal we should revert back to not fragmenting. Signed-off-by: Deomid rojer Ryabkov <rojer@rojer.me>

rojer · 2024-03-28T18:25:10Z

addressed comments, rebased

cryi · 2024-05-16T05:25:24Z

Hi, any idea when this is going to land? This prevents us from moving to 3.6.0.

gilles-peskine-arm · 2024-05-16T07:01:03Z

@cryi Due to the size of the feature, I'm afraid we can't add it to the 3.6 long-time support branch. Since we are not planning any new 3.6 release, I'm afraid this won't be released before Mbed TLS 4.0, which is planned for in Q2 2025.

gilles-peskine-arm · 2024-05-16T07:28:10Z

include/mbedtls/ssl.h

@@ -1811,6 +1811,10 @@ struct mbedtls_ssl_context {

    size_t MBEDTLS_PRIVATE(in_hslen);            /*!< current handshake message length,
                                                    including the handshake header   */


Please add a changelog entry.

gilles-peskine-arm · 2024-05-16T07:29:19Z

include/mbedtls/ssl.h

@@ -1811,6 +1811,10 @@ struct mbedtls_ssl_context {

    size_t MBEDTLS_PRIVATE(in_hslen);            /*!< current handshake message length,
                                                    including the handshake header   */


Please add some tests, i.e. at least one test case in tests/ssl-opt.sh that fails before adding this feature and passes after adding it. This will probably require adding things to programs/ssl/ssl_{client,server}2.c.

cryi · 2024-05-16T07:38:04Z

@cryi Due to the size of the feature, I'm afraid we can't add it to the 3.6 long-time support branch. Since we are not planning any new 3.6 release, I'm afraid this won't be released before Mbed TLS 4.0, which is planned for in Q2 2025.

Thank you @gilles-peskine-arm . ~~Is there any workaround without disabling tls1.3? Right now we experience failed handshakes with GitHub servers when downloading files.~~

Nvm I realized that solution is just not to use mbedtls_ssl_conf_max_frag_len at this point. We do not need it technically so we should be ok with 3.6.0. Thank you 👍🏻

mpg · 2024-05-16T08:23:11Z

library/ssl_msg.c

+            /*
+             * Make room for the next fragment's header.
+             * Note: out_ctr points into previous record's payload but since in practice
+             * we only process unencrypted handshake messages here, like certificates),


Note: in TLS 1.3 certificates are encrypted. Does this need updating for 1.3?

mpg · 2024-05-16T08:25:01Z

library/ssl_msg.c

@@ -3292,11 +3323,74 @@ int mbedtls_ssl_prepare_handshake_record(mbedtls_ssl_context *ssl)
        }
    } else
 #endif /* MBEDTLS_SSL_PROTO_DTLS */
-    /* With TLS we don't handle fragmentation (for now) */
+#if defined(MBEDTLS_SSL_MAX_FRAGMENT_LENGTH)


I'm not sure re-assembly should depend on MAX_FRAGMENT_LENGTH being enabled: even if we didn't send the extension, the peer is always free to fragment messages on its own initiative.

mpg · 2024-05-16T08:37:13Z

Consists of two parts:

The change itself - fragmenting records on the way out, defragmenting on the way in

I'd say that's already two parts, and arguably we should support defragmenting on the way in unconditionally. Could almost be two distinct PRs, except that for testing it's convenient to have both.

A fix for max_frag_len negotiation on the client side: if the server ignores the extension, we must fall back to the maximum value, irrespective of what was configured.

I'm sorry, I'm not sure I understand here: are you talking about messages we send or messages we receive? Indeed if the server ignores the extension, then we're likely to receive messages larger than we asked for, but I don't see why that would prevent us from still sending messages in smaller fragments if we want to.

This approach was extensively tested in production in a big fleet of devices.

That's great to know! Unfortunately I don't have time to do a full review now, but this is good news, and the size of the patch is smaller than I expected for this feature, which is encouraging.

rojer · 2024-05-16T14:10:31Z

ok, so i take it that there is overall agreement on the approach - good to know. i will work on tests and TLS 1.3 compatibility (that i completely ignored until now, since we don't have it enabled in our build).

@mpg, i agree that defragmentation should be performed irrespective of whether fragment length is specified or not. somehow, this did not occur to me when i was working on it.

rojer · 2024-06-16T12:44:43Z

@brewkon ssl_tls13_process_server_hello - as mentioned, this was not tested at all with TLS 1.3, so i'm not surprised. try disabling MBEDTLS_SSL_PROTO_TLS1_3 for now. also make sure MBEDTLS_SSL_MAX_FRAGMENT_LENGTH is enabled and mbedtls_ssl_conf_max_frag_len is set.

rojer · 2024-06-17T14:17:17Z

yes, rebasing, making sure it works with 1.3 and adding automated tests is what needs to be done here to get this ready.
this PR was kind of an rfc about the overall approach, now it needs more work to get to a mergeable state.
no eta on that, as i'm pretty busy with other stuff atm.

mpg · 2024-12-23T10:11:13Z

The change itself - fragmenting records on the way out, defragmenting on the way in

I'd say that's already two parts, and arguably we should support defragmenting on the way in unconditionally. Could almost be two distinct PRs, except that for testing it's convenient to have both.

Actually on second thought, we don't need to have both for testing: if we support defragmenting incoming messages, we can use another implementation for testing - for example, openssl s_server -mtu <low_value> will fragment in TLS as well (see https://github.com/Mbed-TLS/mbedtls/pull/3817/files#diff-54a2261aca14ebb2491a1584cc3351a458487c23c25f90df08de2573cd705e32R9806 for example).

So, I think this PR could indeed be split into multiple parts:

Support defragmenting (reassembly) of incoming handshake messages (testing against OpenSSL).
Support fragmenting outgoing handshake messages (can now test against ourselves too).
Adjust max_frag_len negotiation if necessary.

The reasons I'm suggesting this are:

in general, review bandwidth tends to be a bottleneck and experience shows the smallest a PR is, the most likely it is to get in, and
in this specific case, I think part 1 above (handling incoming fragmented messages) is the most important: it's what was originally reported in TLS Handshake record layer fragmentation not working #1840, again recently in Header files should have different names #9862, and AFAICS there's no work-around for affected users. So we don't want it delayed by the other parts.

@rojer Would you be interested in updating this PR (or creating a new one) focusing just on support for incoming message, including TLS 1.3 and tests?

rojer · 2024-12-23T10:26:36Z

@mpg it's on my list of TODOs, but way, way down in the "fun" section. at the moment there's no business requirement for TLS1.3 and it'd be a bigger change. meanwhile, it's being used in prod for 1.2 without issues (i think there's one fix i made after this PR). so, i can commit to get 1.2 support out but not 1.3, unfortunately.

rojer · 2024-12-25T12:38:44Z

as suggested, i'm splitting this PR into multiple.
incoming message defragmentation, updated to work with TLS 1.3: #9872

rojer mentioned this pull request Mar 22, 2024

TLS Handshake record layer fragmentation not working #1840

Open

rojer force-pushed the tls_hs_frag branch 2 times, most recently from dc99fe4 to 469afe8 Compare March 22, 2024 00:44

paul-elliott-arm added enhancement needs-review Every commit must be reviewed by at least two team members, component-tls needs-reviewer This PR needs someone to pick it up for review size-m Estimated task size: medium (~1w) needs-design-approval labels Mar 22, 2024

paul-elliott-arm added needs-work needs-ci Needs to pass CI tests labels Mar 22, 2024

tom-cosgrove-arm reviewed Mar 26, 2024

View reviewed changes

library/ssl_msg.c Show resolved Hide resolved

tom-cosgrove-arm reviewed Mar 26, 2024

View reviewed changes

library/ssl_msg.c Outdated Show resolved Hide resolved

tom-cosgrove-arm reviewed Mar 26, 2024

View reviewed changes

library/ssl_tls12_client.c Outdated Show resolved Hide resolved

rojer added 2 commits March 28, 2024 18:17

TLS handshake fragmentation support

8e1937d

Support for MBEDTLS_SSL_MAX_FRAGMENT_LENGTH in stream TLS mode Signed-off-by: Deomid rojer Ryabkov <rojer@rojer.me>

Fix client-side max fragment length negotiation

fb804e5

If the server does not acknowledge our fragment length proposal we should revert back to not fragmenting. Signed-off-by: Deomid rojer Ryabkov <rojer@rojer.me>

rojer force-pushed the tls_hs_frag branch from 42f4c86 to fb804e5 Compare March 28, 2024 18:24

gilles-peskine-arm added the priority-medium Medium priority - this can be reviewed as time permits label May 16, 2024

gilles-peskine-arm reviewed May 16, 2024

View reviewed changes

gilles-peskine-arm removed needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels May 16, 2024

mpg reviewed May 16, 2024

View reviewed changes

mpg mentioned this pull request Dec 23, 2024

RTMP_Connect1, TLS_Connect failed: -0x7080 #9861

Closed

hauke mentioned this pull request Dec 23, 2024

mbedtls: unable to access Meta websites openwrt/openwrt#17350

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TLS handshake fragmentation support #8981

TLS handshake fragmentation support #8981

rojer commented Mar 22, 2024 •

edited by mpg

Loading

paul-elliott-arm commented Mar 22, 2024

rojer commented Mar 23, 2024 •

edited

Loading

rojer commented Mar 28, 2024

cryi commented May 16, 2024

gilles-peskine-arm commented May 16, 2024

gilles-peskine-arm May 16, 2024

gilles-peskine-arm May 16, 2024

cryi commented May 16, 2024 •

edited

Loading

mpg May 16, 2024 •

edited

Loading

mpg May 16, 2024 •

edited

Loading

mpg commented May 16, 2024

rojer commented May 16, 2024

rojer commented Jun 16, 2024

rojer commented Jun 17, 2024

mpg commented Dec 23, 2024

rojer commented Dec 23, 2024

rojer commented Dec 25, 2024

		@@ -1811,6 +1811,10 @@ struct mbedtls_ssl_context {

		size_t MBEDTLS_PRIVATE(in_hslen); /*!< current handshake message length,
		including the handshake header */

TLS handshake fragmentation support #8981

Are you sure you want to change the base?

TLS handshake fragmentation support #8981

Conversation

rojer commented Mar 22, 2024 • edited by mpg Loading

Description

PR checklist

Notes for the submitter

paul-elliott-arm commented Mar 22, 2024

rojer commented Mar 23, 2024 • edited Loading

rojer commented Mar 28, 2024

cryi commented May 16, 2024

gilles-peskine-arm commented May 16, 2024

gilles-peskine-arm May 16, 2024

Choose a reason for hiding this comment

gilles-peskine-arm May 16, 2024

Choose a reason for hiding this comment

cryi commented May 16, 2024 • edited Loading

mpg May 16, 2024 • edited Loading

Choose a reason for hiding this comment

mpg May 16, 2024 • edited Loading

Choose a reason for hiding this comment

mpg commented May 16, 2024

rojer commented May 16, 2024

rojer commented Jun 16, 2024

rojer commented Jun 17, 2024

mpg commented Dec 23, 2024

rojer commented Dec 23, 2024

rojer commented Dec 25, 2024

rojer commented Mar 22, 2024 •

edited by mpg

Loading

rojer commented Mar 23, 2024 •

edited

Loading

cryi commented May 16, 2024 •

edited

Loading

mpg May 16, 2024 •

edited

Loading

mpg May 16, 2024 •

edited

Loading