From 5a3629b6132c7dd4a517ebb6e572bb6dbaeb9888 Mon Sep 17 00:00:00 2001 From: Jan Bruckner Date: Thu, 23 Feb 2023 12:08:09 +0100 Subject: [PATCH 1/4] Fix debug print of encrypted extensions Perform debug print of encrypted extensions buffer only after the buffer length was checked successfully Signed-off-by: Jan Bruckner --- library/ssl_tls13_client.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/library/ssl_tls13_client.c b/library/ssl_tls13_client.c index a72f770b3..8697c5386 100644 --- a/library/ssl_tls13_client.c +++ b/library/ssl_tls13_client.c @@ -2117,10 +2117,11 @@ static int ssl_tls13_parse_encrypted_extensions(mbedtls_ssl_context *ssl, extensions_len = MBEDTLS_GET_UINT16_BE(p, 0); p += 2; - MBEDTLS_SSL_DEBUG_BUF(3, "encrypted extensions", p, extensions_len); MBEDTLS_SSL_CHK_BUF_READ_PTR(p, end, extensions_len); extensions_end = p + extensions_len; + MBEDTLS_SSL_DEBUG_BUF(3, "encrypted extensions", p, extensions_len); + handshake->received_extensions = MBEDTLS_SSL_EXT_MASK_NONE; while (p < extensions_end) { From 151f64283ffc91d2c5d3ee7d122cd36ded42cec0 Mon Sep 17 00:00:00 2001 From: Jan Bruckner Date: Fri, 10 Feb 2023 12:45:19 +0100 Subject: [PATCH 2/4] Add parsing for Record Size Limit extension in TLS 1.3 Fixes #7007 Signed-off-by: Jan Bruckner --- include/mbedtls/check_config.h | 4 +++ include/mbedtls/mbedtls_config.h | 14 ++++++++ include/mbedtls/ssl.h | 2 ++ library/ssl_misc.h | 12 ++++++- library/ssl_tls.c | 9 ++++-- library/ssl_tls13_client.c | 11 +++++++ library/ssl_tls13_generic.c | 55 ++++++++++++++++++++++++++++++++ library/ssl_tls13_server.c | 11 +++++++ scripts/config.py | 1 + tests/scripts/all.sh | 15 +++++++++ tests/ssl-opt.sh | 49 +++++++++++++++------------- 11 files changed, 158 insertions(+), 25 deletions(-) diff --git a/include/mbedtls/check_config.h b/include/mbedtls/check_config.h index ca60a9d92..3550ca034 100644 --- a/include/mbedtls/check_config.h +++ b/include/mbedtls/check_config.h @@ -1020,6 +1020,10 @@ #error "MBEDTLS_SSL_VARIABLE_BUFFER_LENGTH defined, but not all prerequisites" #endif +#if defined(MBEDTLS_SSL_RECORD_SIZE_LIMIT) && ( !defined(MBEDTLS_SSL_PROTO_TLS1_3) ) +#error "MBEDTLS_SSL_RECORD_SIZE_LIMIT defined, but not all prerequisites" +#endif + #if defined(MBEDTLS_SSL_CONTEXT_SERIALIZATION) && !( defined(MBEDTLS_GCM_C) || defined(MBEDTLS_CCM_C) || defined(MBEDTLS_CHACHAPOLY_C) ) #error "MBEDTLS_SSL_CONTEXT_SERIALIZATION defined, but not all prerequisites" #endif diff --git a/include/mbedtls/mbedtls_config.h b/include/mbedtls/mbedtls_config.h index 7daba3740..509eeab84 100644 --- a/include/mbedtls/mbedtls_config.h +++ b/include/mbedtls/mbedtls_config.h @@ -1547,6 +1547,20 @@ */ #define MBEDTLS_SSL_MAX_FRAGMENT_LENGTH +/** + * \def MBEDTLS_SSL_RECORD_SIZE_LIMIT + * + * Enable support for RFC 8449 record_size_limit extension in SSL. + * + * \warning This extension is currently in development and must NOT be used except + * for testing purposes. + * + * Requires: MBEDTLS_SSL_PROTO_TLS1_3 + * + * Uncomment this macro to enable support for the record_size_limit extension + */ +//#define MBEDTLS_SSL_RECORD_SIZE_LIMIT + /** * \def MBEDTLS_SSL_PROTO_TLS1_2 * diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index 29ba85a39..efe08305f 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -571,6 +571,8 @@ #define MBEDTLS_TLS_EXT_ENCRYPT_THEN_MAC 22 /* 0x16 */ #define MBEDTLS_TLS_EXT_EXTENDED_MASTER_SECRET 0x0017 /* 23 */ +#define MBEDTLS_TLS_EXT_RECORD_SIZE_LIMIT 28 /* RFC 8449 (implemented for TLS 1.3 only) */ + #define MBEDTLS_TLS_EXT_SESSION_TICKET 35 #define MBEDTLS_TLS_EXT_PRE_SHARED_KEY 41 /* RFC 8446 TLS 1.3 */ diff --git a/library/ssl_misc.h b/library/ssl_misc.h index 4cdd87eca..ccf382e56 100644 --- a/library/ssl_misc.h +++ b/library/ssl_misc.h @@ -107,6 +107,7 @@ #define MBEDTLS_SSL_EXT_ID_ENCRYPT_THEN_MAC 25 #define MBEDTLS_SSL_EXT_ID_EXTENDED_MASTER_SECRET 26 #define MBEDTLS_SSL_EXT_ID_SESSION_TICKET 27 +#define MBEDTLS_SSL_EXT_ID_RECORD_SIZE_LIMIT 28 /* Utility for translating IANA extension type. */ uint32_t mbedtls_ssl_get_extension_id(unsigned int extension_type); @@ -167,6 +168,7 @@ uint32_t mbedtls_ssl_get_extension_mask(unsigned int extension_type); MBEDTLS_SSL_EXT_MASK(CERT_AUTH) | \ MBEDTLS_SSL_EXT_MASK(POST_HANDSHAKE_AUTH) | \ MBEDTLS_SSL_EXT_MASK(SIG_ALG_CERT) | \ + MBEDTLS_SSL_EXT_MASK(RECORD_SIZE_LIMIT) | \ MBEDTLS_SSL_TLS1_3_EXT_MASK_UNRECOGNIZED) /* RFC 8446 section 4.2. Allowed extensions for EncryptedExtensions */ @@ -179,7 +181,8 @@ uint32_t mbedtls_ssl_get_extension_mask(unsigned int extension_type); MBEDTLS_SSL_EXT_MASK(ALPN) | \ MBEDTLS_SSL_EXT_MASK(CLI_CERT_TYPE) | \ MBEDTLS_SSL_EXT_MASK(SERV_CERT_TYPE) | \ - MBEDTLS_SSL_EXT_MASK(EARLY_DATA)) + MBEDTLS_SSL_EXT_MASK(EARLY_DATA) | \ + MBEDTLS_SSL_EXT_MASK(RECORD_SIZE_LIMIT)) /* RFC 8446 section 4.2. Allowed extensions for CertificateRequest */ #define MBEDTLS_SSL_TLS1_3_ALLOWED_EXTS_OF_CR \ @@ -2662,6 +2665,13 @@ int mbedtls_ssl_parse_server_name_ext(mbedtls_ssl_context *ssl, const unsigned char *end); #endif /* MBEDTLS_SSL_SERVER_NAME_INDICATION */ +#if defined(MBEDTLS_SSL_RECORD_SIZE_LIMIT) +MBEDTLS_CHECK_RETURN_CRITICAL +int mbedtls_ssl_tls13_parse_record_size_limit_ext(mbedtls_ssl_context *ssl, + const unsigned char *buf, + const unsigned char *end); +#endif /* MBEDTLS_SSL_RECORD_SIZE_LIMIT */ + #if defined(MBEDTLS_SSL_ALPN) MBEDTLS_CHECK_RETURN_CRITICAL int mbedtls_ssl_parse_alpn_ext(mbedtls_ssl_context *ssl, diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 5d8a761db..e96ce06f1 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -593,6 +593,9 @@ uint32_t mbedtls_ssl_get_extension_id(unsigned int extension_type) case MBEDTLS_TLS_EXT_EXTENDED_MASTER_SECRET: return MBEDTLS_SSL_EXT_ID_EXTENDED_MASTER_SECRET; + case MBEDTLS_TLS_EXT_RECORD_SIZE_LIMIT: + return MBEDTLS_SSL_EXT_ID_RECORD_SIZE_LIMIT; + case MBEDTLS_TLS_EXT_SESSION_TICKET: return MBEDTLS_SSL_EXT_ID_SESSION_TICKET; @@ -635,7 +638,8 @@ static const char *extension_name_table[] = { [MBEDTLS_SSL_EXT_ID_SUPPORTED_POINT_FORMATS] = "supported_point_formats", [MBEDTLS_SSL_EXT_ID_ENCRYPT_THEN_MAC] = "encrypt_then_mac", [MBEDTLS_SSL_EXT_ID_EXTENDED_MASTER_SECRET] = "extended_master_secret", - [MBEDTLS_SSL_EXT_ID_SESSION_TICKET] = "session_ticket" + [MBEDTLS_SSL_EXT_ID_SESSION_TICKET] = "session_ticket", + [MBEDTLS_SSL_EXT_ID_RECORD_SIZE_LIMIT] = "record_size_limit" }; static unsigned int extension_type_table[] = { @@ -666,7 +670,8 @@ static unsigned int extension_type_table[] = { [MBEDTLS_SSL_EXT_ID_SUPPORTED_POINT_FORMATS] = MBEDTLS_TLS_EXT_SUPPORTED_POINT_FORMATS, [MBEDTLS_SSL_EXT_ID_ENCRYPT_THEN_MAC] = MBEDTLS_TLS_EXT_ENCRYPT_THEN_MAC, [MBEDTLS_SSL_EXT_ID_EXTENDED_MASTER_SECRET] = MBEDTLS_TLS_EXT_EXTENDED_MASTER_SECRET, - [MBEDTLS_SSL_EXT_ID_SESSION_TICKET] = MBEDTLS_TLS_EXT_SESSION_TICKET + [MBEDTLS_SSL_EXT_ID_SESSION_TICKET] = MBEDTLS_TLS_EXT_SESSION_TICKET, + [MBEDTLS_SSL_EXT_ID_RECORD_SIZE_LIMIT] = MBEDTLS_TLS_EXT_RECORD_SIZE_LIMIT }; const char *mbedtls_ssl_get_extension_name(unsigned int extension_type) diff --git a/library/ssl_tls13_client.c b/library/ssl_tls13_client.c index 8697c5386..17c1baecf 100644 --- a/library/ssl_tls13_client.c +++ b/library/ssl_tls13_client.c @@ -2173,6 +2173,17 @@ static int ssl_tls13_parse_encrypted_extensions(mbedtls_ssl_context *ssl, break; #endif /* MBEDTLS_SSL_EARLY_DATA */ +#if defined(MBEDTLS_SSL_RECORD_SIZE_LIMIT) + case MBEDTLS_TLS_EXT_RECORD_SIZE_LIMIT: + MBEDTLS_SSL_DEBUG_MSG(3, ("found record_size_limit extension")); + + ret = mbedtls_ssl_tls13_parse_record_size_limit_ext(ssl, p, p + extension_data_len); + + return ret; + + break; +#endif /* MBEDTLS_SSL_RECORD_SIZE_LIMIT */ + default: MBEDTLS_SSL_PRINT_EXT( 3, MBEDTLS_SSL_HS_ENCRYPTED_EXTENSIONS, diff --git a/library/ssl_tls13_generic.c b/library/ssl_tls13_generic.c index 512656e34..0e7aa3a74 100644 --- a/library/ssl_tls13_generic.c +++ b/library/ssl_tls13_generic.c @@ -1567,4 +1567,59 @@ int mbedtls_ssl_tls13_check_received_extension( return MBEDTLS_ERR_SSL_UNSUPPORTED_EXTENSION; } +#if defined(MBEDTLS_SSL_RECORD_SIZE_LIMIT) +/* From RFC 8449: + * The ExtensionData of the "record_size_limit" extension is + * RecordSizeLimit: + * uint16 RecordSizeLimit; + */ +MBEDTLS_CHECK_RETURN_CRITICAL +int mbedtls_ssl_tls13_parse_record_size_limit_ext(mbedtls_ssl_context *ssl, + const unsigned char *buf, + const unsigned char *end) +{ + const ptrdiff_t extension_data_len = end - buf; + if (extension_data_len != 2) { + MBEDTLS_SSL_DEBUG_MSG(2, + ("record_size_limit extension has invalid length: %td Bytes", + extension_data_len)); + + MBEDTLS_SSL_PEND_FATAL_ALERT( + MBEDTLS_SSL_ALERT_MSG_ILLEGAL_PARAMETER, + MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER); + return MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER; + } + + const unsigned char *p = buf; + uint16_t record_size_limit; + + MBEDTLS_SSL_CHK_BUF_READ_PTR(p, end, 2); + record_size_limit = MBEDTLS_GET_UINT16_BE(p, 0); + + MBEDTLS_SSL_DEBUG_MSG(2, ("RecordSizeLimit: %u Bytes", record_size_limit)); + + /* RFC 8449 section 4 + * + * Endpoints MUST NOT send a "record_size_limit" extension with a value + * smaller than 64. An endpoint MUST treat receipt of a smaller value + * as a fatal error and generate an "illegal_parameter" alert. + */ + if (record_size_limit < 64) { + MBEDTLS_SSL_PEND_FATAL_ALERT( + MBEDTLS_SSL_ALERT_MSG_ILLEGAL_PARAMETER, + MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER); + return MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER; + } + + MBEDTLS_SSL_DEBUG_MSG(2, + ( + "record_size_limit extension is still in development. Aborting handshake.")); + + MBEDTLS_SSL_PEND_FATAL_ALERT( + MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_EXT, + MBEDTLS_ERR_SSL_UNSUPPORTED_EXTENSION); + return MBEDTLS_ERR_SSL_UNSUPPORTED_EXTENSION; +} +#endif /* MBEDTLS_SSL_RECORD_SIZE_LIMIT */ + #endif /* MBEDTLS_SSL_TLS_C && MBEDTLS_SSL_PROTO_TLS1_3 */ diff --git a/library/ssl_tls13_server.c b/library/ssl_tls13_server.c index 6b1c4c5e6..de60a7ae8 100644 --- a/library/ssl_tls13_server.c +++ b/library/ssl_tls13_server.c @@ -1585,6 +1585,17 @@ static int ssl_tls13_parse_client_hello(mbedtls_ssl_context *ssl, break; #endif /* MBEDTLS_SSL_TLS1_3_KEY_EXCHANGE_MODE_EPHEMERAL_ENABLED */ +#if defined(MBEDTLS_SSL_RECORD_SIZE_LIMIT) + case MBEDTLS_TLS_EXT_RECORD_SIZE_LIMIT: + MBEDTLS_SSL_DEBUG_MSG(3, ("found record_size_limit extension")); + + ret = mbedtls_ssl_tls13_parse_record_size_limit_ext(ssl, p, extension_data_end); + + return ret; + + break; +#endif /* MBEDTLS_SSL_RECORD_SIZE_LIMIT */ + default: MBEDTLS_SSL_PRINT_EXT( 3, MBEDTLS_SSL_HS_CLIENT_HELLO, diff --git a/scripts/config.py b/scripts/config.py index a53c470f0..404a5eff2 100755 --- a/scripts/config.py +++ b/scripts/config.py @@ -215,6 +215,7 @@ EXCLUDE_FROM_FULL = frozenset([ 'MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN', # build dependency (clang+memsan) 'MBEDTLS_TEST_CONSTANT_FLOW_VALGRIND', # build dependency (valgrind headers) 'MBEDTLS_X509_REMOVE_INFO', # removes a feature + 'MBEDTLS_SSL_RECORD_SIZE_LIMIT', # in development, currently breaks other tests ]) def is_seamless_alt(name): diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index 30185ad65..4294382d7 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -3825,6 +3825,21 @@ component_test_tls13_no_compatibility_mode () { tests/ssl-opt.sh } +component_test_tls13_only_record_size_limit () { + msg "build: TLS 1.3 only from default, record size limit extension enabled" + scripts/config.py set MBEDTLS_SSL_RECORD_SIZE_LIMIT + make CFLAGS="'-DMBEDTLS_USER_CONFIG_FILE=\"../tests/configs/tls13-only.h\"'" + + msg "test_suite_ssl: TLS 1.3 only, record size limit extension enabled" + cd tests; ./test_suite_ssl; cd .. + + msg "ssl-opt.sh: (TLS 1.3 only, record size limit extension tests only)" + # Both the server and the client will currently abort the handshake when they encounter the + # record size limit extension. There is no way to prevent gnutls-cli from sending the extension + # which makes all G_NEXT_CLI + P_SRV tests fail. Thus, run only the tests for the this extension. + tests/ssl-opt.sh -f "Record Size Limit" +} + component_build_mingw () { msg "build: Windows cross build - mingw64, make (Link Library)" # ~ 30s make CC=i686-w64-mingw32-gcc AR=i686-w64-mingw32-ar LD=i686-w64-minggw32-ld CFLAGS='-Werror -Wall -Wextra' WINDOWS_BUILD=1 lib programs diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index d73ef0f87..f60edc575 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -4727,32 +4727,37 @@ run_test "Max fragment length: DTLS client, larger message" \ # Tests for Record Size Limit extension -# gnutls feature tests: check if the record size limit extension is supported with TLS 1.2. -requires_gnutls_record_size_limit -run_test "Record Size Limit: Test gnutls record size limit feature" \ - "$G_NEXT_SRV --priority=NORMAL:-VERS-ALL:+VERS-TLS1.2:+CIPHER-ALL --disable-client-cert -d 4" \ - "$G_NEXT_CLI localhost --priority=NORMAL:-VERS-ALL:+VERS-TLS1.2 -V -d 4" \ - 0 \ - -c "Preparing extension (Record Size Limit/28) for 'client hello'"\ - -s "Parsing extension 'Record Size Limit/28' (2 bytes)" \ - -s "Preparing extension (Record Size Limit/28) for 'TLS 1.2 server hello'" \ - -c "Parsing extension 'Record Size Limit/28' (2 bytes)" \ - -s "Version: TLS1.2" \ - -c "Version: TLS1.2" - -# gnutls feature tests: check if the record size limit extension is supported with TLS 1.3. requires_gnutls_tls1_3 requires_gnutls_record_size_limit -run_test "Record Size Limit: TLS 1.3: Test gnutls record size limit feature" \ - "$G_NEXT_SRV --priority=NORMAL:-VERS-ALL:+VERS-TLS1.3:+CIPHER-ALL --disable-client-cert -d 4" \ +requires_config_enabled MBEDTLS_SSL_RECORD_SIZE_LIMIT +run_test "Record Size Limit: TLS 1.3: Server-side parsing, debug output and fatal alert" \ + "$P_SRV debug_level=3 force_version=tls13" \ "$G_NEXT_CLI localhost --priority=NORMAL:-VERS-ALL:+VERS-TLS1.3 -V -d 4" \ + 1 \ + -c "Preparing extension (Record Size Limit/28) for 'client hello'" \ + -c "Sending extension Record Size Limit/28 (2 bytes)" \ + -s "ClientHello: record_size_limit(28) extension received."\ + -s "found record_size_limit extension" \ + -s "RecordSizeLimit: 16385 Bytes" \ + -c "Received alert \[110]: An unsupported extension was sent" + +requires_gnutls_tls1_3 +requires_gnutls_record_size_limit +requires_gnutls_next_disable_tls13_compat +requires_config_enabled MBEDTLS_SSL_RECORD_SIZE_LIMIT +run_test "Record Size Limit: TLS 1.3: Client-side parsing, debug output and fatal alert" \ + "$G_NEXT_SRV --priority=NORMAL:-VERS-ALL:+VERS-TLS1.3:+CIPHER-ALL:%DISABLE_TLS13_COMPAT_MODE --disable-client-cert -d 4" \ + "$P_CLI debug_level=4 force_version=tls13" \ 0 \ - -c "Preparing extension (Record Size Limit/28) for 'client hello'"\ - -s "Parsing extension 'Record Size Limit/28' (2 bytes)" \ - -s "Preparing extension (Record Size Limit/28) for 'encrypted extensions'" \ - -c "Parsing extension 'Record Size Limit/28' (2 bytes)" \ - -s "Version: TLS1.3" \ - -c "Version: TLS1.3" + -s "Preparing extension (Record Size Limit/28) for 'encrypted extensions'" +# The P_CLI can not yet send the Record Size Limit extension. Thus, the G_NEXT_SRV does not send +# a response in its EncryptedExtensions record. +# -s "Parsing extension 'Record Size Limit/28 (2 bytes)" \ +# -s "Sending extension Record Size Limit/28 (2 bytes)" \ +# -c "EncryptedExtensions: record_size_limit(28) extension received."\ +# -c "found record_size_limit extension" \ +# -c "RecordSizeLimit: 16385 Bytes" \ +# -s "Received alert \[110]: An unsupported extension was sent" # Tests for renegotiation From a0589e75a087f126a9584c4e91632e084cf8d0dd Mon Sep 17 00:00:00 2001 From: Jan Bruckner Date: Wed, 15 Mar 2023 11:04:45 +0100 Subject: [PATCH 3/4] Changes from review Signed-off-by: Jan Bruckner --- include/mbedtls/mbedtls_config.h | 2 +- library/ssl_misc.h | 3 +++ library/ssl_tls13_client.c | 2 ++ library/ssl_tls13_generic.c | 8 ++++---- library/ssl_tls13_server.c | 2 ++ 5 files changed, 12 insertions(+), 5 deletions(-) diff --git a/include/mbedtls/mbedtls_config.h b/include/mbedtls/mbedtls_config.h index 509eeab84..c3597e8eb 100644 --- a/include/mbedtls/mbedtls_config.h +++ b/include/mbedtls/mbedtls_config.h @@ -1550,7 +1550,7 @@ /** * \def MBEDTLS_SSL_RECORD_SIZE_LIMIT * - * Enable support for RFC 8449 record_size_limit extension in SSL. + * Enable support for RFC 8449 record_size_limit extension in SSL (TLS 1.3 only). * * \warning This extension is currently in development and must NOT be used except * for testing purposes. diff --git a/library/ssl_misc.h b/library/ssl_misc.h index ccf382e56..6fe0414f9 100644 --- a/library/ssl_misc.h +++ b/library/ssl_misc.h @@ -2666,6 +2666,9 @@ int mbedtls_ssl_parse_server_name_ext(mbedtls_ssl_context *ssl, #endif /* MBEDTLS_SSL_SERVER_NAME_INDICATION */ #if defined(MBEDTLS_SSL_RECORD_SIZE_LIMIT) +#define MBEDTLS_SSL_RECORD_SIZE_LIMIT_EXTENSION_DATA_LENGTH (2) +#define MBEDTLS_SSL_RECORD_SIZE_LIMIT_MIN (64) + MBEDTLS_CHECK_RETURN_CRITICAL int mbedtls_ssl_tls13_parse_record_size_limit_ext(mbedtls_ssl_context *ssl, const unsigned char *buf, diff --git a/library/ssl_tls13_client.c b/library/ssl_tls13_client.c index 17c1baecf..143056f6a 100644 --- a/library/ssl_tls13_client.c +++ b/library/ssl_tls13_client.c @@ -2179,6 +2179,8 @@ static int ssl_tls13_parse_encrypted_extensions(mbedtls_ssl_context *ssl, ret = mbedtls_ssl_tls13_parse_record_size_limit_ext(ssl, p, p + extension_data_len); + // Return unconditionally here until we handle the record size limit correctly. + // Once handled correctly, only return in case of errors. return ret; break; diff --git a/library/ssl_tls13_generic.c b/library/ssl_tls13_generic.c index 0e7aa3a74..75854fc6a 100644 --- a/library/ssl_tls13_generic.c +++ b/library/ssl_tls13_generic.c @@ -1578,10 +1578,10 @@ int mbedtls_ssl_tls13_parse_record_size_limit_ext(mbedtls_ssl_context *ssl, const unsigned char *buf, const unsigned char *end) { - const ptrdiff_t extension_data_len = end - buf; - if (extension_data_len != 2) { + const size_t extension_data_len = end - buf; + if (extension_data_len != MBEDTLS_SSL_RECORD_SIZE_LIMIT_EXTENSION_DATA_LENGTH) { MBEDTLS_SSL_DEBUG_MSG(2, - ("record_size_limit extension has invalid length: %td Bytes", + ("record_size_limit extension has invalid length: %zu Bytes", extension_data_len)); MBEDTLS_SSL_PEND_FATAL_ALERT( @@ -1604,7 +1604,7 @@ int mbedtls_ssl_tls13_parse_record_size_limit_ext(mbedtls_ssl_context *ssl, * smaller than 64. An endpoint MUST treat receipt of a smaller value * as a fatal error and generate an "illegal_parameter" alert. */ - if (record_size_limit < 64) { + if (record_size_limit < MBEDTLS_SSL_RECORD_SIZE_LIMIT_MIN) { MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_ILLEGAL_PARAMETER, MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER); diff --git a/library/ssl_tls13_server.c b/library/ssl_tls13_server.c index de60a7ae8..1e1462c30 100644 --- a/library/ssl_tls13_server.c +++ b/library/ssl_tls13_server.c @@ -1591,6 +1591,8 @@ static int ssl_tls13_parse_client_hello(mbedtls_ssl_context *ssl, ret = mbedtls_ssl_tls13_parse_record_size_limit_ext(ssl, p, extension_data_end); + // Return unconditionally here until we handle the record size limit correctly. + // Once handled correctly, only return in case of errors. return ret; break; From 1a38e54436229967b1be61e35bdc1fa36e20ae0c Mon Sep 17 00:00:00 2001 From: Jan Bruckner Date: Wed, 15 Mar 2023 14:15:11 +0100 Subject: [PATCH 4/4] Changes from 2nd review Signed-off-by: Jan Bruckner --- library/ssl_tls13_client.c | 4 ++-- library/ssl_tls13_generic.c | 14 ++++++++------ library/ssl_tls13_server.c | 4 ++-- 3 files changed, 12 insertions(+), 10 deletions(-) diff --git a/library/ssl_tls13_client.c b/library/ssl_tls13_client.c index 143056f6a..a7fecedfd 100644 --- a/library/ssl_tls13_client.c +++ b/library/ssl_tls13_client.c @@ -2179,8 +2179,8 @@ static int ssl_tls13_parse_encrypted_extensions(mbedtls_ssl_context *ssl, ret = mbedtls_ssl_tls13_parse_record_size_limit_ext(ssl, p, p + extension_data_len); - // Return unconditionally here until we handle the record size limit correctly. - // Once handled correctly, only return in case of errors. + /* TODO: Return unconditionally here until we handle the record size limit correctly. + * Once handled correctly, only return in case of errors. */ return ret; break; diff --git a/library/ssl_tls13_generic.c b/library/ssl_tls13_generic.c index 75854fc6a..669a90a9b 100644 --- a/library/ssl_tls13_generic.c +++ b/library/ssl_tls13_generic.c @@ -1568,7 +1568,8 @@ int mbedtls_ssl_tls13_check_received_extension( } #if defined(MBEDTLS_SSL_RECORD_SIZE_LIMIT) -/* From RFC 8449: +/* RFC 8449, section 4: + * * The ExtensionData of the "record_size_limit" extension is * RecordSizeLimit: * uint16 RecordSizeLimit; @@ -1578,10 +1579,14 @@ int mbedtls_ssl_tls13_parse_record_size_limit_ext(mbedtls_ssl_context *ssl, const unsigned char *buf, const unsigned char *end) { + const unsigned char *p = buf; + uint16_t record_size_limit; const size_t extension_data_len = end - buf; + if (extension_data_len != MBEDTLS_SSL_RECORD_SIZE_LIMIT_EXTENSION_DATA_LENGTH) { MBEDTLS_SSL_DEBUG_MSG(2, - ("record_size_limit extension has invalid length: %zu Bytes", + ("record_size_limit extension has invalid length: %" + MBEDTLS_PRINTF_SIZET " Bytes", extension_data_len)); MBEDTLS_SSL_PEND_FATAL_ALERT( @@ -1590,15 +1595,12 @@ int mbedtls_ssl_tls13_parse_record_size_limit_ext(mbedtls_ssl_context *ssl, return MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER; } - const unsigned char *p = buf; - uint16_t record_size_limit; - MBEDTLS_SSL_CHK_BUF_READ_PTR(p, end, 2); record_size_limit = MBEDTLS_GET_UINT16_BE(p, 0); MBEDTLS_SSL_DEBUG_MSG(2, ("RecordSizeLimit: %u Bytes", record_size_limit)); - /* RFC 8449 section 4 + /* RFC 8449, section 4 * * Endpoints MUST NOT send a "record_size_limit" extension with a value * smaller than 64. An endpoint MUST treat receipt of a smaller value diff --git a/library/ssl_tls13_server.c b/library/ssl_tls13_server.c index 1e1462c30..bccf9abec 100644 --- a/library/ssl_tls13_server.c +++ b/library/ssl_tls13_server.c @@ -1591,8 +1591,8 @@ static int ssl_tls13_parse_client_hello(mbedtls_ssl_context *ssl, ret = mbedtls_ssl_tls13_parse_record_size_limit_ext(ssl, p, extension_data_end); - // Return unconditionally here until we handle the record size limit correctly. - // Once handled correctly, only return in case of errors. + /* TODO: Return unconditionally here until we handle the record size limit correctly. + * Once handled correctly, only return in case of errors. */ return ret; break;