From f501790ff20bf3896fff6299fd1f889106fab047 Mon Sep 17 00:00:00 2001 From: Waleed Elmelegy Date: Tue, 9 Jan 2024 14:18:34 +0000 Subject: [PATCH] Improve comments across record size limit changes Signed-off-by: Waleed Elmelegy --- library/ssl_misc.h | 1 + library/ssl_tls.c | 18 +++++++++--------- tests/ssl-opt.sh | 20 ++++++++++++-------- 3 files changed, 22 insertions(+), 17 deletions(-) diff --git a/library/ssl_misc.h b/library/ssl_misc.h index 7195d6343..2e621be89 100644 --- a/library/ssl_misc.h +++ b/library/ssl_misc.h @@ -2704,6 +2704,7 @@ int mbedtls_ssl_parse_server_name_ext(mbedtls_ssl_context *ssl, #if defined(MBEDTLS_SSL_RECORD_SIZE_LIMIT) #define MBEDTLS_SSL_RECORD_SIZE_LIMIT_EXTENSION_DATA_LENGTH (2) +/* This value is defined by RFC 8449 */ #define MBEDTLS_SSL_RECORD_SIZE_LIMIT_MIN (64) MBEDTLS_CHECK_RETURN_CRITICAL diff --git a/library/ssl_tls.c b/library/ssl_tls.c index f92e40ac7..517af785a 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3521,15 +3521,15 @@ int mbedtls_ssl_get_max_out_record_payload(const mbedtls_ssl_context *ssl) 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) + /* + * In TLS 1.3 case, when records are protected, `max_len` as computed + * above is the maximum length of the TLSInnerPlaintext structure that + * along the plaintext payload contains the inner content type (one byte) + * and some zero padding. Given the algorithm used for padding + * in mbedtls_ssl_encrypt_buf(), compute the maximum length for + * the plaintext payload. Round down to a multiple of + * MBEDTLS_SSL_CID_TLS1_3_PADDING_GRANULARITY and + * subtract 1. */ max_len = ((max_len / MBEDTLS_SSL_CID_TLS1_3_PADDING_GRANULARITY) * MBEDTLS_SSL_CID_TLS1_3_PADDING_GRANULARITY) - 1; diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 12605f5b0..30e6a725a 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -4862,16 +4862,18 @@ run_test "Record Size Limit: TLS 1.3: Client-side parsing and debug output" \ -c "EncryptedExtensions: record_size_limit(28) extension received." \ -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 -# 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 -# "%ALLOW_SMALL_RECORDS" priority string and not using the more recent function -# https://gnutls.org/reference/gnutls-gnutls.html#gnutls-record-set-max-recv-size. +# In the following tests, --recordsize is the value used by the G_NEXT_CLI (3.7.2) to configure the +# maximum record size using gnutls_record_set_max_size() +# (https://gnutls.org/reference/gnutls-gnutls.html#gnutls-record-set-max-size). +# There is currently a lower limit of 512, caused by gnutls_record_set_max_size() +# not respecting the "%ALLOW_SMALL_RECORDS" priority string and not using the +# more recent function gnutls_record_set_max_recv_size() +# (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. -# Moreover, the value sent in the extension is expected to be larger by one compared -# to the value passed on the cli: +# Thus, these tests are currently limited to the value range 512-4096. +# Also, the value sent in the extension will be one larger than the value +# set at the command line: # https://gitlab.com/gnutls/gnutls/-/blob/3.7.2/lib/ext/record_size_limit.c#L142 # Currently test certificates being used do not fit in 513 record size limit @@ -5050,6 +5052,8 @@ run_test "Record Size Limit: TLS 1.3 m->m: both peer comply with record size -s "Maximum outgoing record payload length is 16383" \ -s "Maximum incoming record payload length is 16384" +# End of Record size limit tests + # Tests for renegotiation # Renegotiation SCSV always added, regardless of SSL_RENEGOTIATION