From 049cd302ed13a516eef412bffa9753f77edb763f Mon Sep 17 00:00:00 2001 From: Waleed Elmelegy Date: Wed, 20 Dec 2023 17:28:31 +0000 Subject: [PATCH] Refactor record size limit extension handling Signed-off-by: Waleed Elmelegy --- include/mbedtls/ssl.h | 2 +- library/ssl_tls.c | 45 ++++++++++++++++++++++++++-- library/ssl_tls13_client.c | 13 ++++++++ library/ssl_tls13_generic.c | 37 ----------------------- tests/src/test_helpers/ssl_helpers.c | 4 +++ tests/suites/test_suite_ssl.function | 4 +++ 6 files changed, 65 insertions(+), 40 deletions(-) diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index 85ec7ab364..3192e2a826 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -1188,7 +1188,7 @@ struct mbedtls_ssl_session { unsigned char MBEDTLS_PRIVATE(mfl_code); /*!< MaxFragmentLength negotiated by peer */ #endif /* MBEDTLS_SSL_MAX_FRAGMENT_LENGTH */ -/*!< RecordSizeLimit received by peer */ +/*!< RecordSizeLimit received from the peer */ #if defined(MBEDTLS_SSL_RECORD_SIZE_LIMIT) uint16_t MBEDTLS_PRIVATE(record_size_limit); #endif /* MBEDTLS_SSL_RECORD_SIZE_LIMIT */ diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 7a8c759fa3..914eec3299 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -2492,7 +2492,7 @@ static int ssl_tls13_session_save(const mbedtls_ssl_session *session, needed += 4; /* max_early_data_size */ #endif #if defined(MBEDTLS_SSL_RECORD_SIZE_LIMIT) - needed += 2; /* record_size_limit */ + needed += 2; /* record_size_limit */ #endif /* MBEDTLS_SSL_RECORD_SIZE_LIMIT */ #if defined(MBEDTLS_HAVE_TIME) @@ -3420,6 +3420,31 @@ size_t mbedtls_ssl_get_input_max_frag_len(const mbedtls_ssl_context *ssl) return max_len; } +#if defined(MBEDTLS_SSL_RECORD_SIZE_LIMIT) + +size_t mbedtls_ssl_get_output_record_size_limit(const mbedtls_ssl_context *ssl) +{ + const size_t max_len = MBEDTLS_SSL_OUT_CONTENT_LEN; + size_t record_size_limit = max_len; + + if (ssl->session != NULL && + ssl->session->record_size_limit >= MBEDTLS_SSL_RECORD_SIZE_LIMIT_MIN && + ssl->session->record_size_limit < max_len) { + record_size_limit = ssl->session->record_size_limit; + } + + // TODO: this is currently untested + /* During a handshake, use the value being negotiated */ + if (ssl->session_negotiate != NULL && + ssl->session_negotiate->record_size_limit >= MBEDTLS_SSL_RECORD_SIZE_LIMIT_MIN && + ssl->session_negotiate->record_size_limit < max_len) { + record_size_limit = ssl->session_negotiate->record_size_limit; + } + + return record_size_limit; +} +#endif /* MBEDTLS_SSL_RECORD_SIZE_LIMIT */ + size_t mbedtls_ssl_get_output_max_frag_len(const mbedtls_ssl_context *ssl) { size_t max_len; @@ -3491,6 +3516,21 @@ int mbedtls_ssl_get_max_out_record_payload(const mbedtls_ssl_context *ssl) if (max_len > record_size_limit) { max_len = record_size_limit; + if (ssl->transform_out != NULL && + ssl->transform_out->tls_version == MBEDTLS_SSL_VERSION_TLS1_3) { + /* RFC 8449, section 4: + * + * This value [record_size_limit] is the length of the plaintext + * of a protected record. + * The value includes the content type and padding added in TLS 1.3 + * (that is, the complete length of TLSInnerPlaintext). + * + * Thus, round down to a multiple of MBEDTLS_SSL_CID_TLS1_3_PADDING_GRANULARITY + * and subtract 1 (for the content type that will be added later) + */ + max_len = ((max_len / MBEDTLS_SSL_CID_TLS1_3_PADDING_GRANULARITY) * + MBEDTLS_SSL_CID_TLS1_3_PADDING_GRANULARITY) - 1; + } } #endif @@ -3516,7 +3556,8 @@ int mbedtls_ssl_get_max_out_record_payload(const mbedtls_ssl_context *ssl) #endif /* MBEDTLS_SSL_PROTO_DTLS */ #if !defined(MBEDTLS_SSL_MAX_FRAGMENT_LENGTH) && \ - !defined(MBEDTLS_SSL_PROTO_DTLS) + !defined(MBEDTLS_SSL_PROTO_DTLS) && \ + !defined(MBEDTLS_SSL_RECORD_SIZE_LIMIT) ((void) ssl); #endif diff --git a/library/ssl_tls13_client.c b/library/ssl_tls13_client.c index 1a246c4dfc..503db5862a 100644 --- a/library/ssl_tls13_client.c +++ b/library/ssl_tls13_client.c @@ -2131,6 +2131,19 @@ static int ssl_tls13_parse_encrypted_extensions(mbedtls_ssl_context *ssl, p += extension_data_len; } + if ((handshake->received_extensions & MBEDTLS_SSL_EXT_MASK(RECORD_SIZE_LIMIT)) && + (handshake->received_extensions & MBEDTLS_SSL_EXT_MASK(MAX_FRAGMENT_LENGTH))) { + mbedtls_debug_print_msg(ssl, + 3, + __FILE__, + __LINE__, + "Record size limit extension cannot be used with max fragment length extension"); + MBEDTLS_SSL_PEND_FATAL_ALERT( + MBEDTLS_SSL_ALERT_MSG_ILLEGAL_PARAMETER, + MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER); + return MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER; + } + MBEDTLS_SSL_PRINT_EXTS(3, MBEDTLS_SSL_HS_ENCRYPTED_EXTENSIONS, handshake->received_extensions); diff --git a/library/ssl_tls13_generic.c b/library/ssl_tls13_generic.c index 7c7aac80e4..326811a601 100644 --- a/library/ssl_tls13_generic.c +++ b/library/ssl_tls13_generic.c @@ -1732,43 +1732,6 @@ int mbedtls_ssl_tls13_parse_record_size_limit_ext(mbedtls_ssl_context *ssl, return 0; } -static inline size_t ssl_compute_internal_record_size_limit(size_t record_size_limit) -{ - /* RFC 8449, section 4: - * - * This value [record_size_limit] is the length of the plaintext of a protected record. - * The value includes the content type and padding added in TLS 1.3 (that is, the complete - * length of TLSInnerPlaintext). - * - * Thus, round down to a multiple of MBEDTLS_SSL_CID_TLS1_3_PADDING_GRANULARITY - * and subtract 1 (for the content type that will be added later) - */ - return ((record_size_limit / MBEDTLS_SSL_CID_TLS1_3_PADDING_GRANULARITY) * - MBEDTLS_SSL_CID_TLS1_3_PADDING_GRANULARITY) - 1; -} - -size_t mbedtls_ssl_get_output_record_size_limit(const mbedtls_ssl_context *ssl) -{ - const size_t max_len = MBEDTLS_SSL_OUT_CONTENT_LEN; - size_t record_size_limit = max_len; - - if (ssl->session != NULL && - ssl->session->record_size_limit >= MBEDTLS_SSL_RECORD_SIZE_LIMIT_MIN && - ssl->session->record_size_limit < max_len) { - record_size_limit = ssl_compute_internal_record_size_limit(ssl->session->record_size_limit); - } - - // TODO: this is currently untested - /* During a handshake, use the value being negotiated */ - if (ssl->session_negotiate != NULL && - ssl->session_negotiate->record_size_limit >= MBEDTLS_SSL_RECORD_SIZE_LIMIT_MIN && - ssl->session_negotiate->record_size_limit < max_len) { - record_size_limit = ssl_compute_internal_record_size_limit( - ssl->session_negotiate->record_size_limit); - } - - return record_size_limit; -} #endif /* MBEDTLS_SSL_RECORD_SIZE_LIMIT */ #endif /* MBEDTLS_SSL_TLS_C && MBEDTLS_SSL_PROTO_TLS1_3 */ diff --git a/tests/src/test_helpers/ssl_helpers.c b/tests/src/test_helpers/ssl_helpers.c index d02d305394..3d8937da6d 100644 --- a/tests/src/test_helpers/ssl_helpers.c +++ b/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; +#endif + return 0; } #endif /* MBEDTLS_SSL_PROTO_TLS1_3 */ diff --git a/tests/suites/test_suite_ssl.function b/tests/suites/test_suite_ssl.function index 05571a1dc8..8a03d1b970 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -2096,6 +2096,10 @@ void ssl_serialize_session_save_load(int ticket_len, char *crt_file, } #endif /* MBEDTLS_SSL_PROTO_TLS1_3 */ +#if defined(MBEDTLS_SSL_RECORD_SIZE_LIMIT) + TEST_ASSERT(original.record_size_limit == restored.record_size_limit); +#endif + exit: mbedtls_ssl_session_free(&original); mbedtls_ssl_session_free(&restored);