From 102c89ed65c2ada2b43fb40cd2b6f08b86cc89b7 Mon Sep 17 00:00:00 2001 From: TRodziewicz Date: Wed, 12 May 2021 13:28:59 +0200 Subject: [PATCH 1/5] Remove the MBEDTLS_SSL_RECORD_CHECKING option Signed-off-by: TRodziewicz --- ChangeLog.d/issue4361.txt | 2 ++ include/mbedtls/config.h | 14 -------------- include/mbedtls/ssl.h | 2 -- library/ssl_msg.c | 2 -- library/version_features.c | 3 --- programs/ssl/ssl_test_common_source.c | 6 ------ programs/test/query_config.c | 8 -------- 7 files changed, 2 insertions(+), 35 deletions(-) create mode 100644 ChangeLog.d/issue4361.txt diff --git a/ChangeLog.d/issue4361.txt b/ChangeLog.d/issue4361.txt new file mode 100644 index 0000000000..670c8a6580 --- /dev/null +++ b/ChangeLog.d/issue4361.txt @@ -0,0 +1,2 @@ +Removals + * Remove the MBEDTLS_SSL_RECORD_CHECKING option. Fixes #4361. diff --git a/include/mbedtls/config.h b/include/mbedtls/config.h index 603d985ae0..aa69848c78 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -1467,20 +1467,6 @@ */ #define MBEDTLS_SSL_ALL_ALERT_MESSAGES -/** - * \def MBEDTLS_SSL_RECORD_CHECKING - * - * Enable the function mbedtls_ssl_check_record() which can be used to check - * the validity and authenticity of an incoming record, to verify that it has - * not been seen before. These checks are performed without modifying the - * externally visible state of the SSL context. - * - * See mbedtls_ssl_check_record() for more information. - * - * Uncomment to enable support for record checking. - */ -#define MBEDTLS_SSL_RECORD_CHECKING - /** * \def MBEDTLS_SSL_DTLS_CONNECTION_ID * diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index 40814e660d..a47631c94f 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -1795,7 +1795,6 @@ void mbedtls_ssl_set_verify( mbedtls_ssl_context *ssl, */ void mbedtls_ssl_conf_read_timeout( mbedtls_ssl_config *conf, uint32_t timeout ); -#if defined(MBEDTLS_SSL_RECORD_CHECKING) /** * \brief Check whether a buffer contains a valid and authentic record * that has not been seen before. (DTLS only). @@ -1843,7 +1842,6 @@ void mbedtls_ssl_conf_read_timeout( mbedtls_ssl_config *conf, uint32_t timeout ) int mbedtls_ssl_check_record( mbedtls_ssl_context const *ssl, unsigned char *buf, size_t buflen ); -#endif /* MBEDTLS_SSL_RECORD_CHECKING */ /** * \brief Set the timer callbacks (Mandatory for DTLS.) diff --git a/library/ssl_msg.c b/library/ssl_msg.c index 3956a67d27..c2fcdcbfd2 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -86,7 +86,6 @@ int mbedtls_ssl_check_timer( mbedtls_ssl_context *ssl ) return( 0 ); } -#if defined(MBEDTLS_SSL_RECORD_CHECKING) static int ssl_parse_record_header( mbedtls_ssl_context const *ssl, unsigned char *buf, size_t len, @@ -150,7 +149,6 @@ exit: MBEDTLS_SSL_DEBUG_MSG( 1, ( "<= mbedtls_ssl_check_record" ) ); return( ret ); } -#endif /* MBEDTLS_SSL_RECORD_CHECKING */ #define SSL_DONT_FORCE_FLUSH 0 #define SSL_FORCE_FLUSH 1 diff --git a/library/version_features.c b/library/version_features.c index d2de8957d2..b42fb29aa1 100644 --- a/library/version_features.c +++ b/library/version_features.c @@ -468,9 +468,6 @@ static const char * const features[] = { #if defined(MBEDTLS_SSL_ALL_ALERT_MESSAGES) "MBEDTLS_SSL_ALL_ALERT_MESSAGES", #endif /* MBEDTLS_SSL_ALL_ALERT_MESSAGES */ -#if defined(MBEDTLS_SSL_RECORD_CHECKING) - "MBEDTLS_SSL_RECORD_CHECKING", -#endif /* MBEDTLS_SSL_RECORD_CHECKING */ #if defined(MBEDTLS_SSL_DTLS_CONNECTION_ID) "MBEDTLS_SSL_DTLS_CONNECTION_ID", #endif /* MBEDTLS_SSL_DTLS_CONNECTION_ID */ diff --git a/programs/ssl/ssl_test_common_source.c b/programs/ssl/ssl_test_common_source.c index 73457a1390..fd7eacf6d8 100644 --- a/programs/ssl/ssl_test_common_source.c +++ b/programs/ssl/ssl_test_common_source.c @@ -159,7 +159,6 @@ int dtls_srtp_key_derivation( void *p_expkey, #endif /* MBEDTLS_SSL_EXPORT_KEYS */ -#if defined(MBEDTLS_SSL_RECORD_CHECKING) int ssl_check_record( mbedtls_ssl_context const *ssl, unsigned char const *buf, size_t len ) { @@ -220,7 +219,6 @@ int ssl_check_record( mbedtls_ssl_context const *ssl, return( 0 ); } -#endif /* MBEDTLS_SSL_RECORD_CHECKING */ int recv_cb( void *ctx, unsigned char *buf, size_t len ) { @@ -241,10 +239,8 @@ int recv_cb( void *ctx, unsigned char *buf, size_t len ) /* Here's the place to do any datagram/record checking * in between receiving the packet from the underlying * transport and passing it on to the TLS stack. */ -#if defined(MBEDTLS_SSL_RECORD_CHECKING) if( ssl_check_record( io_ctx->ssl, buf, recv_len ) != 0 ) return( -1 ); -#endif /* MBEDTLS_SSL_RECORD_CHECKING */ } return( (int) recv_len ); @@ -267,10 +263,8 @@ int recv_timeout_cb( void *ctx, unsigned char *buf, size_t len, /* Here's the place to do any datagram/record checking * in between receiving the packet from the underlying * transport and passing it on to the TLS stack. */ -#if defined(MBEDTLS_SSL_RECORD_CHECKING) if( ssl_check_record( io_ctx->ssl, buf, recv_len ) != 0 ) return( -1 ); -#endif /* MBEDTLS_SSL_RECORD_CHECKING */ } return( (int) recv_len ); diff --git a/programs/test/query_config.c b/programs/test/query_config.c index 450e2fbbf0..cf7b3032f6 100644 --- a/programs/test/query_config.c +++ b/programs/test/query_config.c @@ -1299,14 +1299,6 @@ int query_config( const char *config ) } #endif /* MBEDTLS_SSL_ALL_ALERT_MESSAGES */ -#if defined(MBEDTLS_SSL_RECORD_CHECKING) - if( strcmp( "MBEDTLS_SSL_RECORD_CHECKING", config ) == 0 ) - { - MACRO_EXPANSION_TO_STR( MBEDTLS_SSL_RECORD_CHECKING ); - return( 0 ); - } -#endif /* MBEDTLS_SSL_RECORD_CHECKING */ - #if defined(MBEDTLS_SSL_DTLS_CONNECTION_ID) if( strcmp( "MBEDTLS_SSL_DTLS_CONNECTION_ID", config ) == 0 ) { From 95f8f22c2701218c1d24ef568c8e1632f19dcd41 Mon Sep 17 00:00:00 2001 From: TRodziewicz Date: Fri, 14 May 2021 14:07:51 +0200 Subject: [PATCH 2/5] Migration guide added and ChangeLog clarified Signed-off-by: TRodziewicz --- ChangeLog.d/issue4361.txt | 3 ++- .../remove_ssl_record_checking.md | 13 +++++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) create mode 100644 docs/3.0-migration-guide.d/remove_ssl_record_checking.md diff --git a/ChangeLog.d/issue4361.txt b/ChangeLog.d/issue4361.txt index 670c8a6580..f1dbb3f195 100644 --- a/ChangeLog.d/issue4361.txt +++ b/ChangeLog.d/issue4361.txt @@ -1,2 +1,3 @@ Removals - * Remove the MBEDTLS_SSL_RECORD_CHECKING option. Fixes #4361. + * Remove the MBEDTLS_SSL_RECORD_CHECKING option and enable by default its + previous action. Fixes #4361. diff --git a/docs/3.0-migration-guide.d/remove_ssl_record_checking.md b/docs/3.0-migration-guide.d/remove_ssl_record_checking.md new file mode 100644 index 0000000000..a1b8a5757b --- /dev/null +++ b/docs/3.0-migration-guide.d/remove_ssl_record_checking.md @@ -0,0 +1,13 @@ +Remove MBEDTLS_SSL_RECORD_CHECKING option and enable its action by default +-------------------------------------------------------------------------- + +This change does not affects users who use the default config.h, as the +option MBEDTLS_SSL_RECORD_CHECKING was already on by default. + +This option was added only to controls compilation of one function +(mbedtls_ssl_check_record()) used in DTLS to check a buffer's validity and +authenticity. Switching it off poses a security risk. + +For users who changed the default setting of the option there is no real path +of migration. + From 1cf33bf94d173343da7e94a4ee56eb08c6e8f936 Mon Sep 17 00:00:00 2001 From: TRodziewicz Date: Fri, 14 May 2021 14:35:26 +0200 Subject: [PATCH 3/5] Corrections o the migration guide Signed-off-by: TRodziewicz --- docs/3.0-migration-guide.d/remove_ssl_record_checking.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/3.0-migration-guide.d/remove_ssl_record_checking.md b/docs/3.0-migration-guide.d/remove_ssl_record_checking.md index a1b8a5757b..91f6f7e88b 100644 --- a/docs/3.0-migration-guide.d/remove_ssl_record_checking.md +++ b/docs/3.0-migration-guide.d/remove_ssl_record_checking.md @@ -1,10 +1,10 @@ Remove MBEDTLS_SSL_RECORD_CHECKING option and enable its action by default -------------------------------------------------------------------------- -This change does not affects users who use the default config.h, as the +This change does not affect users who use the default config.h, as the option MBEDTLS_SSL_RECORD_CHECKING was already on by default. -This option was added only to controls compilation of one function +This option was added only to control compilation of one function (mbedtls_ssl_check_record()) used in DTLS to check a buffer's validity and authenticity. Switching it off poses a security risk. From 57d7ab72fb9952a86bf88c5730ac8ef0534a2e4f Mon Sep 17 00:00:00 2001 From: TRodziewicz Date: Mon, 17 May 2021 10:43:41 +0200 Subject: [PATCH 4/5] Correction to migration guide entry wording Signed-off-by: TRodziewicz --- .../remove_ssl_record_checking.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/docs/3.0-migration-guide.d/remove_ssl_record_checking.md b/docs/3.0-migration-guide.d/remove_ssl_record_checking.md index 91f6f7e88b..203e740240 100644 --- a/docs/3.0-migration-guide.d/remove_ssl_record_checking.md +++ b/docs/3.0-migration-guide.d/remove_ssl_record_checking.md @@ -4,10 +4,10 @@ Remove MBEDTLS_SSL_RECORD_CHECKING option and enable its action by default This change does not affect users who use the default config.h, as the option MBEDTLS_SSL_RECORD_CHECKING was already on by default. -This option was added only to control compilation of one function -(mbedtls_ssl_check_record()) used in DTLS to check a buffer's validity and -authenticity. Switching it off poses a security risk. - -For users who changed the default setting of the option there is no real path -of migration. +This option was added only to control compilation of one function, +mbedtls_ssl_check_record(), which is only useful in some specific cases, so it +was made optional to allow users who don't need it to save some code space. +However, the same effect can be achieve by using link-time garbage collection. +Users who changed the default setting of the option need to change the config/ +build system to remove that change. \ No newline at end of file From e13a23b4394454943605ddeaeea892c420178290 Mon Sep 17 00:00:00 2001 From: TRodziewicz Date: Mon, 17 May 2021 11:16:52 +0200 Subject: [PATCH 5/5] New line added at the end of the migration guide entry Signed-off-by: TRodziewicz --- docs/3.0-migration-guide.d/remove_ssl_record_checking.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/3.0-migration-guide.d/remove_ssl_record_checking.md b/docs/3.0-migration-guide.d/remove_ssl_record_checking.md index 203e740240..7550f7b5a5 100644 --- a/docs/3.0-migration-guide.d/remove_ssl_record_checking.md +++ b/docs/3.0-migration-guide.d/remove_ssl_record_checking.md @@ -10,4 +10,4 @@ was made optional to allow users who don't need it to save some code space. However, the same effect can be achieve by using link-time garbage collection. Users who changed the default setting of the option need to change the config/ -build system to remove that change. \ No newline at end of file +build system to remove that change.