Comply with the received Record Size Limit extension #7455

KloolK · 2023-04-18T14:59:15Z

Gatekeeper checklist

changelog no, feature is not complete for production use
backport not required
tests provided

KloolK · 2023-04-18T16:18:51Z

@yanesca I have two open questions regarding my implementation and would like to get some feedback on these:

1:
This part https://github.com/Mbed-TLS/mbedtls/pull/7455/files#diff-f60739d09b65e7b43187688f40e19d5c9825972f8690c0f6f9611431f2cf738eR1692-R1698 is currently untested.
I did some manual tests and it seems fine. But there are no automated tests at the moment, since I don't see a way to trigger this codepath with the GnuTLS client.
Is that fine for now?

2:
Regarding what I describe here: https://github.com/Mbed-TLS/mbedtls/pull/7455/files#diff-f60739d09b65e7b43187688f40e19d5c9825972f8690c0f6f9611431f2cf738eR1700-R1714.
Where do you think is the correct place to perform this subtraction? Ideally some place where the total application data length that is currently attempted to be written via ssl_write_real() (len) is available (to be able to consider the actual padding, which depends on the data length).

Thanks!

tests/scripts/all.sh

KloolK · 2023-04-19T12:17:36Z

I removed MBEDTLS_SSL_RECORD_SIZE_LIMIT from the exclude list in config.py: https://github.com/Mbed-TLS/mbedtls/pull/7455/files#diff-e9ffa9c36082d5a0b2ae4098b1b77d020a8d57d159bc89255fcad17287bc3ed9L218 since enabling it should not break unrelated tests anymore.

This made the changes in all.sh necessary: https://github.com/Mbed-TLS/mbedtls/pull/7455/files#diff-e683396c93d3af1ea2db5a3d94788da3916ed084f3eb8be5e5f7c0e429badf39

However, all_u16-test_depends_py_hashes and all_u16-test_depends_py_hashes_psa still fail, due to MBEDTLS_SSL_RECORD_SIZE_LIMIT being enabled without MBEDTLS_SSL_PROTO_TLS1_3.

Now I'm wondering how to fix this properly. Any hints?

yanesca · 2023-04-19T12:20:25Z

I did some manual tests and it seems fine. But there are no automated tests at the moment, since I don't see a way to trigger this codepath with the GnuTLS client.
Is that fine for now?

Yes, it is. For now we just want to do some smoke testing. Adding unit tests and extensive testing is out of scope for this PR and will be done in #7027.

Where do you think is the correct place to perform this subtraction?

We can calculate it right here rounding down to a multiple of MBEDTLS_SSL_CID_TLS1_3_PADDING_GRANULARITY. (Messages will be padded to a multiple of MBEDTLS_SSL_CID_TLS1_3_PADDING_GRANULARITY, anything longer than that would be padded to something longer than the record size limit.)

yanesca · 2023-04-19T12:35:37Z

Now I'm wondering how to fix this properly. Any hints?

I suggest we don't remove MBEDTLS_SSL_RECORD_SIZE_LIMIT from the exclude list in config.py just yet. We can do that later after the feature is complete and we have added extensive unit tests.

KloolK · 2023-04-19T15:56:21Z

We can calculate it right here rounding down to a multiple of MBEDTLS_SSL_CID_TLS1_3_PADDING_GRANULARITY. (Messages will be padded to a multiple of MBEDTLS_SSL_CID_TLS1_3_PADDING_GRANULARITY, anything longer than that would be padded to something longer than the record size limit.)

Done, with subtracting 1 byte after rounding to account for the content type that will be added afterwards.

I suggest we don't remove MBEDTLS_SSL_RECORD_SIZE_LIMIT from the exclude list in config.py just yet. We can do that later after the feature is complete and we have added extensive unit tests.

Reverted both the change to config.py and the related changes to all.sh.

yanesca · 2023-07-10T10:55:34Z

@KloolK This PR is something that we still need and a contribution we want to take, we are just struggling to find the review bandwidth for it at the moment. I just wanted to give you a heads up that it looks like it will take some time until we will be able to schedule the review. Thanks again for your contribution and thank you for your patience.

daverodgman · 2023-07-14T13:24:53Z

@KloolK This PR is something that we still need and a contribution we want to take, we are just struggling to find the review bandwidth for it at the moment. I just wanted to give you a heads up that it looks like it will take some time until we will be able to schedule the review. Thanks again for your contribution and thank you for your patience.

We are aiming to finish up some outstanding TLS 1.3 work in Q4, including this PR, for release in Mbed TLS 3.6. So we do expect to pick this up in the near future.

include/mbedtls/ssl.h

library/ssl_misc.h

library/ssl_tls13_generic.c

tests/ssl-opt.sh

tom-cosgrove-arm · 2023-11-07T10:27:56Z

Hi @KloolK, can I check if you will have time to make the changes requested, or would you like us to finish off this PR?

tom-cosgrove-arm · 2023-11-07T10:28:43Z

@waleed-elmelegy-arm

KloolK · 2023-11-07T10:40:05Z

Hi @KloolK, can I check if you will have time to make the changes requested, or would you like us to finish off this PR?

Hi @tom-cosgrove-arm I don't expect to have much time in the coming weeks unfortunately. Feel free to finish this off.

yanrayw · 2023-12-07T05:11:51Z

library/ssl_msg.c

@@ -917,6 +917,7 @@ int mbedtls_ssl_encrypt_buf(mbedtls_ssl_context *ssl,
 #endif
    size_t add_data_len;
    size_t post_avail;
+    int max_out_record_len = mbedtls_ssl_get_max_out_record_payload(ssl);


There are many failures in CI that's because, in mbedtls_ssl_get_max_out_record_payload, we call mbedtls_ssl_get_output_max_frag_len which tries to access ssl->conf-mfl_code.

However, in some test cases, we only test specific functionality so we don't call mbedtls_ssl_setup(). ssl_crypt_record in test_suite_ssl.function is an example.

Edit: This commit is a workaround to fix aforementioned issue.

Follow-up:
I found another issue is that we have test cases which set max_fragment_length to 512 and which, in DTLS, set mtu = 512. It has same problem as RecordLimitSize=512 due to size of Certificate.

@yanrayw could you please point me to a test that has this problem?

@yanesca There are many failed components in CI. Take ./tests/scripts/all.sh --seed 4 --keep-going test_tls13 as an example. We see this log
[2023-12-06T22:50:06.787Z] Record crypt, AES-128-CBC, 1.2, SHA-384 ........................... /var/lib/build/library/ssl_tls.c:3430:47: runtime error: member access within null pointer of type 'const struct mbedtls_ssl_config'
That's because we try to access in NULL pointer in the test case, which matches this comment.

As for the problem mentioned in this comment, we see this error log

[2023-12-06T22:50:06.786Z] Sending app data via TLS, MFL=512 without fragmentation ........... FAILED [2023-12-06T22:50:06.786Z] mbedtls_test_move_handshake_to_state(&(client.ssl), &(server.ssl), MBEDTLS_SSL_HANDSHAKE_OVER) == expected_handshake_result [2023-12-06T22:50:06.786Z] at line 2100, /var/lib/build/tests/src/test_helpers/ssl_helpers.c [2023-12-06T22:50:06.786Z] Sending app data via TLS, MFL=512 with fragmentation .............. FAILED [2023-12-06T22:50:06.786Z] mbedtls_test_move_handshake_to_state(&(client.ssl), &(server.ssl), MBEDTLS_SSL_HANDSHAKE_OVER) == expected_handshake_result [2023-12-06T22:50:06.786Z] at line 2100, /var/lib/build/tests/src/test_helpers/ssl_helpers.c

and log from ssl-opt.sh

[2023-12-06T23:12:25.052Z] DTLS fragmenting: both (MTU=512) ....................................... RETRY(client-timeout) RETRY(client-timeout) FAIL [2023-12-06T23:12:25.052Z] ! bad client exit code (expected 0, got 143)

which is caused by this check since max_out_record_len is smaller than certificate.

Please let me know if anything is not correct from my understanding.

I think the problem is checking on maximum fragment length according to this: https://github.com/Mbed-TLS/mbedtls/blob/development/include/mbedtls/ssl.h#L4262-L4264 we do not support currently maximum fragment length during handshake so I added a check for if we are in handshake or not.

Thank you @yanrayw, I think @waleed-elmelegy-arm 's solution should resolve the issue.

Thanks! CI is still not happy, might be related to mtu in DTLS. I haven't looked into it.

I looked into the failure and the reason is the certificate being to big while doing handshake and this is in DTLS so we now check the MTU in case of DTLS so the only solutions are to use smaller certificate or disable 512 bytes tests, in this case the good thing is this is a test that is using mbedtls as a server and a client so we might be able to use secp192 certificate since GnuTLS is not involved but I will check other failing tests to confirm.

That doesn't add up, we fragment handshake messages in DTLS. How can the certificate be too big?

MbedTLS currently does not support maximum fragment length during handshake so we skip it for now. Signed-off-by: Waleed Elmelegy <[email protected]>

This reverts commit 419f841. Signed-off-by: Waleed Elmelegy <[email protected]>

Revert back checking on handshake messages length due to limitation on our fragmentation support of handshake messages. Signed-off-by: Waleed Elmelegy <[email protected]>

ronald-cron-arm

I've reviewed the library code. Please find my comments below. I have started to look at the tests as well but not completely.

include/mbedtls/ssl.h

library/ssl_tls13_generic.c

library/ssl_tls13_client.c

library/ssl_tls.c

tests/ssl-opt.sh

Signed-off-by: Waleed Elmelegy <[email protected]>

ronald-cron-arm

I've re-reviewed the library code and reviewed the tests.

library/ssl_tls.c

library/ssl_tls13_client.c

library/ssl_tls.c

tests/ssl-opt.sh

Signed-off-by: Waleed Elmelegy <[email protected]>

ronald-cron-arm

One comment not addressed yet it seems, otherwise this looks good to me. Otherwise, as you have experienced with the tests, currently we can not reach the maximum payload of 16384 in TLS 1.3 records. I have created #8671 to track that.

Signed-off-by: Waleed Elmelegy <[email protected]>

ronald-cron-arm

The description of mbedtls_ssl_get_output_record_size_limit() is outdated. Otherwise this looks good to me.

library/ssl_misc.h

Co-authored-by: Ronald Cron <[email protected]> Signed-off-by: Waleed-Ziad Maamoun-Elmelegy <[email protected]>

ronald-cron-arm

LGTM, thanks.

tom-cosgrove-arm

Looks good to me; nice work.

Some minor comments; I'm not sure if they need to be addressed as part of this PR, or could be handled (if necessary) in the other PR

tom-cosgrove-arm · 2024-01-08T17:01:05Z

library/ssl_tls.c

@@ -2610,6 +2618,13 @@ static int ssl_tls13_session_load(mbedtls_ssl_session *session,
    session->max_early_data_size = MBEDTLS_GET_UINT32_BE(p, 0);
    p += 4;
 #endif
+#if defined(MBEDTLS_SSL_RECORD_SIZE_LIMIT)
+    if (end - p < 2) {
+        return MBEDTLS_ERR_SSL_BAD_INPUT_DATA;


I'd like to think that if someone was using Mbed TLS just before this PR, and had a saved session, then recompiled with this PR, and tried to resume, they'd get an MBEDTLS_ERR_SSL_VERSION_MISMATCH (considering it the version of the data, rather than just the version of the library) error, not a MBEDTLS_ERR_SSL_BAD_INPUT_DATA error, although I suspect the caller would treat both identically.

That would suggest that MBEDTLS_SSL_RECORD_SIZE_LIMIT should be incorporated in SSL_SERIALIZED_SESSION_CONFIG_BITFLAG

However, the same reasoning applies to MBEDTLS_SSL_EARLY_DATA, so this looks like pre-existing, rather than anything else, and I'm mainly making the observation here, rather than asking for a change.

That would suggest that MBEDTLS_SSL_RECORD_SIZE_LIMIT should be incorporated in SSL_SERIALIZED_SESSION_CONFIG_BITFLAG

That's a good point. I think we overlooked this aspect of serialized sessions while working on new session tickets, early data and now record size limit. I am creating an issue to address that: #8686.

tom-cosgrove-arm · 2024-01-08T17:05:03Z

library/ssl_tls.c

+    size_t record_size_limit = max_len;
+
+    if (ssl->session != NULL &&
+        ssl->session->record_size_limit >= MBEDTLS_SSL_RECORD_SIZE_LIMIT_MIN &&


It's not clear to me under what circumstances we would put a value < MBEDTLS_SSL_RECORD_SIZE_LIMIT_MIN into the session. Would it make more sense to verify when populating ssl->session->record_size_limit and then be able to skip the check here?

If we do not receive any record size limit extension then ssl->session->record_size_limit is equal to zero. Thus to me this check is to detect if the peer sent a record size limit or not.

Ah - if this test had been for > 0 then I might have thought that.

Would (in the future, not this PR) it make more sense to set this to the value to use for record size and use it unconditionally?

Yes that would be an improvement.

tom-cosgrove-arm · 2024-01-08T17:11:30Z

tests/src/test_helpers/ssl_helpers.c

@@ -1776,6 +1776,10 @@ int mbedtls_test_ssl_tls13_populate_session(mbedtls_ssl_session *session,
    }
 #endif /* MBEDTLS_SSL_CLI_C */

+#if defined(MBEDTLS_SSL_RECORD_SIZE_LIMIT)
+    session->record_size_limit = 2048;


What if MBEDTLS_SSL_RECORD_SIZE_LIMIT_MIN > 2048?

MBEDTLS_SSL_RECORD_SIZE_LIMIT_MIN is defined by RFC 8449 to be equal to 64. We should probably add a comment where MBEDTLS_SSL_RECORD_SIZE_LIMIT_MIN is defined to make that clear.

tom-cosgrove-arm · 2024-01-08T17:13:09Z

tests/ssl-opt.sh

-#            -s "Received alert \[110]: An unsupported extension was sent"
+#            -c "RecordSizeLimit: 16385 Bytes"
+
+# In the following (9) tests, --recordsize is the value used by the G_NEXT_CLI (3.7.2) to configure the


(minor; only if making other changes) 9 seems to be a number that won't be maintained (as tests are added and removed in this section, it's likely that at some point we'll forget to update this number) so best removed. Would it be better to have an "end of record size limit tests" comment?

tom-cosgrove-arm · 2024-01-08T17:15:05Z

tests/ssl-opt.sh

+
+# In the following (9) tests, --recordsize is the value used by the G_NEXT_CLI (3.7.2) to configure the
+# maximum record size using "https://gnutls.org/reference/gnutls-gnutls.html#gnutls-record-set-max-size".
+# There is currently a lower limit of 512, caused by this function not respecting the


(minor; not sure if worth an update to the PR on its own)

On first (and second!) reading, it wasn't clear to me that "this function" referred to the function documented in the URL on the line above, mainly because it wasn't clear that the object of "using" was a function.

I wonder if it would be better to be explicit about gnutls_record_set_max_size() rather than saying "this function", or if "that function" would work just as well.

tom-cosgrove-arm · 2024-01-08T17:16:27Z

tests/ssl-opt.sh

+# https://gnutls.org/reference/gnutls-gnutls.html#gnutls-record-set-max-recv-size.
+# There is currently an upper limit of 4096, caused by the cli arg parser:
+# https://gitlab.com/gnutls/gnutls/-/blob/3.7.2/src/cli-args.def#L395.
+# Thus, these tests are currently limit to that value range.


(minor) Best be explicit. Also, should be limited

Suggested change

# Thus, these tests are currently limit to that value range.

# Thus, these tests are currently limited to the value range 512-4096.

tom-cosgrove-arm · 2024-01-08T17:18:40Z

tests/ssl-opt.sh

+# Thus, these tests are currently limit to that value range.
+# Moreover, the value sent in the extension is expected to be larger by one compared
+# to the value passed on the cli:
+# https://gitlab.com/gnutls/gnutls/-/blob/3.7.2/lib/ext/record_size_limit.c#L142


Is it is expected to be or just will be?

Also, the value sent in the extension will be one larger than the value set at the command line:?

tom-cosgrove-arm · 2024-01-08T17:42:28Z

library/ssl_tls.c

+
+    if (ssl->transform_out != NULL &&
+        ssl->transform_out->tls_version == MBEDTLS_SSL_VERSION_TLS1_3) {
+        /* RFC 8449, section 4:


It's not immediately clear why this is not within the MBEDTLS_SSL_RECORD_SIZE_LIMIT block. Some of the text in Ronald's comment might help here

tom-cosgrove-arm · 2024-01-09T11:31:29Z

Do we still need to be able to craft a small certificate for future testing, as mentioned in Janos' comment?

If so, removing curves smaller than 255 bits (issue 8136) might be a problem!

ronald-cron-arm · 2024-01-09T11:38:32Z

Do we still need to be able to craft a small certificate for future testing, as mentioned in Janos' comment?

If so, removing curves smaller than 255 bits (issue 8136) might be a problem!

I do not think that this would improve much the testing of the record size limit code as now we have tests with a record size limit of 512 using a pre-shared key.

KloolK force-pushed the record-size-limit/comply-with-limit branch 2 times, most recently from 2561fff to 7ad693c Compare April 18, 2023 16:09

KloolK force-pushed the record-size-limit/comply-with-limit branch from 7ad693c to d7d5c10 Compare April 19, 2023 06:49

KloolK commented Apr 19, 2023

View reviewed changes

tests/scripts/all.sh Outdated Show resolved Hide resolved

yanesca added enhancement needs-work component-tls13 size-s Estimated task size: small (~2d) priority-medium Medium priority - this can be reviewed as time permits labels Apr 19, 2023

KloolK force-pushed the record-size-limit/comply-with-limit branch from d7d5c10 to 38d0046 Compare April 19, 2023 15:52

KloolK marked this pull request as ready for review April 19, 2023 19:20

KloolK marked this pull request as draft April 19, 2023 19:21

KloolK force-pushed the record-size-limit/comply-with-limit branch from 38d0046 to cacf1dc Compare April 20, 2023 06:05

KloolK marked this pull request as ready for review April 20, 2023 08:03

KloolK force-pushed the record-size-limit/comply-with-limit branch from cacf1dc to e900345 Compare April 20, 2023 08:37

xkqian self-requested a review July 17, 2023 08:30

xkqian suggested changes Jul 20, 2023

View reviewed changes

include/mbedtls/ssl.h Outdated Show resolved Hide resolved

library/ssl_misc.h Outdated Show resolved Hide resolved

library/ssl_misc.h Show resolved Hide resolved

library/ssl_tls13_generic.c Outdated Show resolved Hide resolved

tests/ssl-opt.sh Show resolved Hide resolved

davidhorstmann-arm self-requested a review July 21, 2023 13:46

yuhaoth added priority-high High priority - will be reviewed soon and removed priority-medium Medium priority - this can be reviewed as time permits labels Oct 12, 2023

waleed-elmelegy-arm force-pushed the record-size-limit/comply-with-limit branch from c1578c2 to 9aec1c7 Compare December 6, 2023 15:19

yanrayw reviewed Dec 7, 2023

View reviewed changes

waleed-elmelegy-arm added 3 commits December 7, 2023 18:33

Skip checking on maximum fragment length during handshake

419f841

MbedTLS currently does not support maximum fragment length during handshake so we skip it for now. Signed-off-by: Waleed Elmelegy <[email protected]>

Revert "Skip checking on maximum fragment length during handshake"

05d670b

This reverts commit 419f841. Signed-off-by: Waleed Elmelegy <[email protected]>

Revert back checking on handshake messages length

26e3698

Revert back checking on handshake messages length due to limitation on our fragmentation support of handshake messages. Signed-off-by: Waleed Elmelegy <[email protected]>

waleed-elmelegy-arm force-pushed the record-size-limit/comply-with-limit branch from 2cf8e7b to 26e3698 Compare December 14, 2023 16:23

ronald-cron-arm requested changes Dec 18, 2023

View reviewed changes

include/mbedtls/ssl.h Outdated Show resolved Hide resolved

library/ssl_tls13_generic.c Outdated Show resolved Hide resolved

library/ssl_tls13_client.c Show resolved Hide resolved

library/ssl_tls.c Outdated Show resolved Hide resolved

tests/ssl-opt.sh Outdated Show resolved Hide resolved

waleed-elmelegy-arm added 2 commits December 20, 2023 17:28

Refactor record size limit extension handling

049cd30

Signed-off-by: Waleed Elmelegy <[email protected]>

Fix code style in ssl_tls.c

65e3046

Signed-off-by: Waleed Elmelegy <[email protected]>

ronald-cron-arm requested changes Dec 21, 2023

View reviewed changes

waleed-elmelegy-arm added 5 commits December 28, 2023 17:48

Refactor and improve Record size limit handling

6a971fd

Signed-off-by: Waleed Elmelegy <[email protected]>

Improve Record size limit testing

87a373e

Signed-off-by: Waleed Elmelegy <[email protected]>

Adjust TLS 1.3 tests to new maximum output changes

ea03183

Signed-off-by: Waleed Elmelegy <[email protected]>

Fix TLS 1.2 test to use TLS 1.2 maximum output size

bae705c

Signed-off-by: Waleed Elmelegy <[email protected]>

Fix Max fragmen length test to use TLS 1.2 maximum output size

3d46b7f

Signed-off-by: Waleed Elmelegy <[email protected]>

ronald-cron-arm requested changes Jan 4, 2024

View reviewed changes

Add config dependencies to record size tests

60f0f72

Signed-off-by: Waleed Elmelegy <[email protected]>

ronald-cron-arm requested changes Jan 5, 2024

View reviewed changes

library/ssl_misc.h Outdated Show resolved Hide resolved

ronald-cron-arm requested review from tom-cosgrove-arm and removed request for davidhorstmann-arm January 5, 2024 10:09

ronald-cron-arm added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Jan 5, 2024

Update mbedtls_ssl_get_output_record_size_limit signature

e2d3db5

Co-authored-by: Ronald Cron <[email protected]> Signed-off-by: Waleed-Ziad Maamoun-Elmelegy <[email protected]>

ronald-cron-arm approved these changes Jan 5, 2024

View reviewed changes

tom-cosgrove-arm approved these changes Jan 8, 2024

View reviewed changes

tom-cosgrove-arm added this pull request to the merge queue Jan 9, 2024

Merged via the queue into Mbed-TLS:development with commit 3a6059b Jan 9, 2024

	# Thus, these tests are currently limit to that value range.
	# Thus, these tests are currently limited to the value range 512-4096.

Comply with the received Record Size Limit extension #7455

Comply with the received Record Size Limit extension #7455

Conversation

KloolK commented Apr 18, 2023 • edited by minosgalanakis Loading

Gatekeeper checklist

KloolK commented Apr 18, 2023

KloolK commented Apr 19, 2023

yanesca commented Apr 19, 2023

yanesca commented Apr 19, 2023

KloolK commented Apr 19, 2023

yanesca commented Jul 10, 2023

daverodgman commented Jul 14, 2023

tom-cosgrove-arm commented Nov 7, 2023

tom-cosgrove-arm commented Nov 7, 2023

KloolK commented Nov 7, 2023

yanrayw Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronald-cron-arm left a comment

Choose a reason for hiding this comment

ronald-cron-arm left a comment

Choose a reason for hiding this comment

ronald-cron-arm left a comment

Choose a reason for hiding this comment

ronald-cron-arm left a comment

Choose a reason for hiding this comment

ronald-cron-arm left a comment

Choose a reason for hiding this comment

tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tom-cosgrove-arm commented Jan 9, 2024 • edited Loading

ronald-cron-arm commented Jan 9, 2024

KloolK commented Apr 18, 2023 •

edited by minosgalanakis

Loading

yanrayw Dec 7, 2023 •

edited

Loading

tom-cosgrove-arm commented Jan 9, 2024 •

edited

Loading