From 5902cd64e2aa1e3e6be6fde498fa5b3d01fbe4bf Mon Sep 17 00:00:00 2001 From: Andrzej Kurek Date: Tue, 28 Sep 2021 10:00:32 -0400 Subject: [PATCH 1/3] Remove MBEDTLS_SSL_EXPORT_KEYS, making it always on This option only gated an ability to set a callback, but was deemed unnecessary as it was yet another define to remember when writing tests, or test configurations. Fixes #4653. Signed-off-by: Andrzej Kurek --- ChangeLog.d/remove-ssl-export-keys.txt | 5 +++++ configs/config-thread.h | 1 - include/mbedtls/mbedtls_config.h | 10 ---------- include/mbedtls/ssl.h | 6 ------ library/ssl_tls.c | 16 +++++----------- programs/ssl/ssl_client2.c | 13 ------------- programs/ssl/ssl_server2.c | 13 ------------- programs/ssl/ssl_test_common_source.c | 3 --- programs/ssl/ssl_test_lib.h | 4 ---- tests/ssl-opt.sh | 1 - 10 files changed, 10 insertions(+), 62 deletions(-) create mode 100644 ChangeLog.d/remove-ssl-export-keys.txt diff --git a/ChangeLog.d/remove-ssl-export-keys.txt b/ChangeLog.d/remove-ssl-export-keys.txt new file mode 100644 index 000000000..1a4b31dca --- /dev/null +++ b/ChangeLog.d/remove-ssl-export-keys.txt @@ -0,0 +1,5 @@ +Changes + * Remove MBEDTLS_SSL_EXPORT_KEYS, making it always on and increasing the + code size by about 80B on an M0 build. This option only gated an ability + to set a callback, but was deemed unnecessary as it was yet another define + to remember when writing tests, or test configurations. Fixes #4653. diff --git a/configs/config-thread.h b/configs/config-thread.h index be889a187..36d824510 100644 --- a/configs/config-thread.h +++ b/configs/config-thread.h @@ -45,7 +45,6 @@ #define MBEDTLS_SSL_PROTO_DTLS #define MBEDTLS_SSL_DTLS_ANTI_REPLAY #define MBEDTLS_SSL_DTLS_HELLO_VERIFY -#define MBEDTLS_SSL_EXPORT_KEYS /* mbed TLS modules */ #define MBEDTLS_AES_C diff --git a/include/mbedtls/mbedtls_config.h b/include/mbedtls/mbedtls_config.h index adc317dfe..fc42dfbd0 100644 --- a/include/mbedtls/mbedtls_config.h +++ b/include/mbedtls/mbedtls_config.h @@ -1598,16 +1598,6 @@ */ #define MBEDTLS_SSL_SESSION_TICKETS -/** - * \def MBEDTLS_SSL_EXPORT_KEYS - * - * Enable support for exporting key block and master secret. - * This is required for certain users of TLS, e.g. EAP-TLS. - * - * Comment this macro to disable support for key export - */ -#define MBEDTLS_SSL_EXPORT_KEYS - /** * \def MBEDTLS_SSL_SERVER_NAME_INDICATION * diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index 725b156d5..956afc684 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -1139,7 +1139,6 @@ typedef enum } mbedtls_tls_prf_types; -#if defined(MBEDTLS_SSL_EXPORT_KEYS) typedef enum { MBEDTLS_SSL_KEY_EXPORT_TLS12_MASTER_SECRET = 0, @@ -1175,7 +1174,6 @@ typedef void mbedtls_ssl_export_keys_t( void *p_expkey, const unsigned char client_random[32], const unsigned char server_random[32], mbedtls_tls_prf_types tls_prf_type ); -#endif /* MBEDTLS_SSL_EXPORT_KEYS */ /** * SSL/TLS configuration to be shared between mbedtls_ssl_context structures. @@ -1617,11 +1615,9 @@ struct mbedtls_ssl_context * and #MBEDTLS_SSL_CID_DISABLED. */ #endif /* MBEDTLS_SSL_DTLS_CONNECTION_ID */ -#if defined(MBEDTLS_SSL_EXPORT_KEYS) /** Callback to export key block and master secret */ mbedtls_ssl_export_keys_t *MBEDTLS_PRIVATE(f_export_keys); void *MBEDTLS_PRIVATE(p_export_keys); /*!< context for key export callback */ -#endif }; /** @@ -2194,7 +2190,6 @@ void mbedtls_ssl_conf_session_tickets_cb( mbedtls_ssl_config *conf, void *p_ticket ); #endif /* MBEDTLS_SSL_SESSION_TICKETS && MBEDTLS_SSL_SRV_C */ -#if defined(MBEDTLS_SSL_EXPORT_KEYS) /** * \brief Configure a key export callback. * (Default: none.) @@ -2216,7 +2211,6 @@ void mbedtls_ssl_conf_session_tickets_cb( mbedtls_ssl_config *conf, void mbedtls_ssl_set_export_keys_cb( mbedtls_ssl_context *ssl, mbedtls_ssl_export_keys_t *f_export_keys, void *p_export_keys ); -#endif /* MBEDTLS_SSL_EXPORT_KEYS */ #if defined(MBEDTLS_SSL_ASYNC_PRIVATE) /** diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 360419240..f16157a52 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -587,7 +587,6 @@ static int ssl_use_opaque_psk( mbedtls_ssl_context const *ssl ) #endif /* MBEDTLS_USE_PSA_CRYPTO && MBEDTLS_KEY_EXCHANGE_PSK_ENABLED */ -#if defined(MBEDTLS_SSL_EXPORT_KEYS) static mbedtls_tls_prf_types tls_prf_get_type( mbedtls_ssl_tls_prf_cb *tls_prf ) { #if defined(MBEDTLS_SSL_PROTO_TLS1_2) @@ -608,7 +607,6 @@ static mbedtls_tls_prf_types tls_prf_get_type( mbedtls_ssl_tls_prf_cb *tls_prf ) #endif /* MBEDTLS_SSL_PROTO_TLS1_2 */ return( MBEDTLS_SSL_TLS_PRF_NONE ); } -#endif /* MBEDTLS_SSL_EXPORT_KEYS */ int mbedtls_ssl_tls_prf( const mbedtls_tls_prf_types prf, const unsigned char *secret, size_t slen, @@ -660,8 +658,9 @@ typedef int ssl_tls_prf_t(const unsigned char *, size_t, const char *, * - [in] randbytes: buffer holding ServerHello.random + ClientHello.random * - [in] minor_ver: SSL/TLS minor version * - [in] endpoint: client or server - * - [in] ssl: optionally used for: - * - MBEDTLS_SSL_EXPORT_KEYS: ssl->conf->{f,p}_export_keys + * - [in] ssl: used for: + * - ssl->conf->{f,p}_export_keys + * [in] optionally used for: * - MBEDTLS_DEBUG_C: ssl->conf->{f,p}_dbg */ static int ssl_tls12_populate_transform( mbedtls_ssl_transform *transform, @@ -694,9 +693,8 @@ static int ssl_tls12_populate_transform( mbedtls_ssl_transform *transform, const mbedtls_cipher_info_t *cipher_info; const mbedtls_md_info_t *md_info; -#if !defined(MBEDTLS_SSL_EXPORT_KEYS) && \ - !defined(MBEDTLS_DEBUG_C) - ssl = NULL; /* make sure we don't use it except for those cases */ +#if !defined(MBEDTLS_DEBUG_C) + ssl = NULL; /* make sure we don't use it except for this case */ (void) ssl; #endif @@ -960,7 +958,6 @@ static int ssl_tls12_populate_transform( mbedtls_ssl_transform *transform, ((void) mac_dec); ((void) mac_enc); -#if defined(MBEDTLS_SSL_EXPORT_KEYS) if( ssl->f_export_keys != NULL ) { ssl->f_export_keys( ssl->p_export_keys, @@ -970,7 +967,6 @@ static int ssl_tls12_populate_transform( mbedtls_ssl_transform *transform, randbytes, tls_prf_get_type( tls_prf ) ); } -#endif #if defined(MBEDTLS_USE_PSA_CRYPTO) @@ -4229,7 +4225,6 @@ void mbedtls_ssl_conf_session_tickets_cb( mbedtls_ssl_config *conf, #endif #endif /* MBEDTLS_SSL_SESSION_TICKETS */ -#if defined(MBEDTLS_SSL_EXPORT_KEYS) void mbedtls_ssl_set_export_keys_cb( mbedtls_ssl_context *ssl, mbedtls_ssl_export_keys_t *f_export_keys, void *p_export_keys ) @@ -4237,7 +4232,6 @@ void mbedtls_ssl_set_export_keys_cb( mbedtls_ssl_context *ssl, ssl->f_export_keys = f_export_keys; ssl->p_export_keys = p_export_keys; } -#endif #if defined(MBEDTLS_SSL_ASYNC_PRIVATE) void mbedtls_ssl_conf_async_private_cb( diff --git a/programs/ssl/ssl_client2.c b/programs/ssl/ssl_client2.c index a970503c8..a02d977ec 100644 --- a/programs/ssl/ssl_client2.c +++ b/programs/ssl/ssl_client2.c @@ -207,7 +207,6 @@ int main( void ) #define USAGE_TICKETS "" #endif /* MBEDTLS_SSL_SESSION_TICKETS */ -#if defined(MBEDTLS_SSL_EXPORT_KEYS) #define USAGE_EAP_TLS \ " eap_tls=%%d default: 0 (disabled)\n" #define USAGE_NSS_KEYLOG \ @@ -230,12 +229,6 @@ int main( void ) #else /* MBEDTLS_SSL_DTLS_SRTP */ #define USAGE_SRTP "" #endif -#else /* MBEDTLS_SSL_EXPORT_KEYS */ -#define USAGE_EAP_TLS "" -#define USAGE_NSS_KEYLOG "" -#define USAGE_NSS_KEYLOG_FILE "" -#define USAGE_SRTP "" -#endif /* MBEDTLS_SSL_EXPORT_KEYS */ #if defined(MBEDTLS_SSL_MAX_FRAGMENT_LENGTH) #define USAGE_MAX_FRAG_LEN \ @@ -729,7 +722,6 @@ int main( int argc, char *argv[] ) unsigned char *context_buf = NULL; size_t context_buf_len; #endif -#if defined(MBEDTLS_SSL_EXPORT_KEYS) unsigned char eap_tls_keymaterial[16]; unsigned char eap_tls_iv[8]; const char* eap_tls_label = "client EAP encryption"; @@ -747,7 +739,6 @@ int main( int argc, char *argv[] ) MBEDTLS_TLS_SRTP_UNSET }; #endif /* MBEDTLS_SSL_DTLS_SRTP */ -#endif /* MBEDTLS_SSL_EXPORT_KEYS */ #if defined(MBEDTLS_MEMORY_BUFFER_ALLOC_C) mbedtls_memory_buffer_alloc_init( alloc_buf, sizeof(alloc_buf) ); @@ -1962,7 +1953,6 @@ int main( int argc, char *argv[] ) goto exit; } -#if defined(MBEDTLS_SSL_EXPORT_KEYS) if( opt.eap_tls != 0 ) { mbedtls_ssl_set_export_keys_cb( &ssl, eap_tls_key_derivation, @@ -1981,7 +1971,6 @@ int main( int argc, char *argv[] ) &dtls_srtp_keying ); } #endif /* MBEDTLS_SSL_DTLS_SRTP */ -#endif /* MBEDTLS_SSL_EXPORT_KEYS */ #if defined(MBEDTLS_X509_CRT_PARSE_C) if( ( ret = mbedtls_ssl_set_hostname( &ssl, opt.server_name ) ) != 0 ) @@ -2169,7 +2158,6 @@ int main( int argc, char *argv[] ) } #endif -#if defined(MBEDTLS_SSL_EXPORT_KEYS) if( opt.eap_tls != 0 ) { size_t j = 0; @@ -2286,7 +2274,6 @@ int main( int argc, char *argv[] ) } } #endif /* MBEDTLS_SSL_DTLS_SRTP */ -#endif /* MBEDTLS_SSL_EXPORT_KEYS */ if( opt.reconnect != 0 ) { mbedtls_printf(" . Saving session for reuse..." ); diff --git a/programs/ssl/ssl_server2.c b/programs/ssl/ssl_server2.c index e8e4ed8ae..40a690229 100644 --- a/programs/ssl/ssl_server2.c +++ b/programs/ssl/ssl_server2.c @@ -278,7 +278,6 @@ int main( void ) #define USAGE_TICKETS "" #endif /* MBEDTLS_SSL_SESSION_TICKETS */ -#if defined(MBEDTLS_SSL_EXPORT_KEYS) #define USAGE_EAP_TLS \ " eap_tls=%%d default: 0 (disabled)\n" #define USAGE_NSS_KEYLOG \ @@ -299,12 +298,6 @@ int main( void ) #else /* MBEDTLS_SSL_DTLS_SRTP */ #define USAGE_SRTP "" #endif -#else /* MBEDTLS_SSL_EXPORT_KEYS */ -#define USAGE_EAP_TLS "" -#define USAGE_NSS_KEYLOG "" -#define USAGE_NSS_KEYLOG_FILE "" -#define USAGE_SRTP "" -#endif /* MBEDTLS_SSL_EXPORT_KEYS */ #if defined(MBEDTLS_SSL_CACHE_C) #define USAGE_CACHE \ @@ -1365,7 +1358,6 @@ int main( int argc, char *argv[] ) #if defined(MBEDTLS_USE_PSA_CRYPTO) psa_status_t status; #endif -#if defined(MBEDTLS_SSL_EXPORT_KEYS) unsigned char eap_tls_keymaterial[16]; unsigned char eap_tls_iv[8]; const char* eap_tls_label = "client EAP encryption"; @@ -1383,7 +1375,6 @@ int main( int argc, char *argv[] ) MBEDTLS_TLS_SRTP_UNSET }; #endif /* MBEDTLS_SSL_DTLS_SRTP */ -#endif /* MBEDTLS_SSL_EXPORT_KEYS */ #if defined(MBEDTLS_MEMORY_BUFFER_ALLOC_C) mbedtls_memory_buffer_alloc_init( alloc_buf, sizeof(alloc_buf) ); @@ -2966,7 +2957,6 @@ int main( int argc, char *argv[] ) goto exit; } -#if defined(MBEDTLS_SSL_EXPORT_KEYS) if( opt.eap_tls != 0 ) { mbedtls_ssl_set_export_keys_cb( &ssl, eap_tls_key_derivation, @@ -2985,7 +2975,6 @@ int main( int argc, char *argv[] ) &dtls_srtp_keying ); } #endif /* MBEDTLS_SSL_DTLS_SRTP */ -#endif /* MBEDTLS_SSL_EXPORT_KEYS */ io_ctx.ssl = &ssl; io_ctx.net = &client_fd; @@ -3251,7 +3240,6 @@ handshake: #endif /* MBEDTLS_X509_REMOVE_INFO */ #endif /* MBEDTLS_X509_CRT_PARSE_C */ -#if defined(MBEDTLS_SSL_EXPORT_KEYS) if( opt.eap_tls != 0 ) { size_t j = 0; @@ -3369,7 +3357,6 @@ handshake: } } #endif /* MBEDTLS_SSL_DTLS_SRTP */ -#endif /* MBEDTLS_SSL_EXPORT_KEYS */ #if defined(MBEDTLS_SSL_DTLS_CONNECTION_ID) ret = report_cid_usage( &ssl, "initial handshake" ); diff --git a/programs/ssl/ssl_test_common_source.c b/programs/ssl/ssl_test_common_source.c index 6ec4171a8..62cd35de8 100644 --- a/programs/ssl/ssl_test_common_source.c +++ b/programs/ssl/ssl_test_common_source.c @@ -24,7 +24,6 @@ * limitations under the License. */ -#if defined(MBEDTLS_SSL_EXPORT_KEYS) void eap_tls_key_derivation( void *p_expkey, mbedtls_ssl_key_export_type secret_type, const unsigned char *secret, @@ -140,8 +139,6 @@ void dtls_srtp_key_derivation( void *p_expkey, } #endif /* MBEDTLS_SSL_DTLS_SRTP */ -#endif /* MBEDTLS_SSL_EXPORT_KEYS */ - int ssl_check_record( mbedtls_ssl_context const *ssl, unsigned char const *buf, size_t len ) { diff --git a/programs/ssl/ssl_test_lib.h b/programs/ssl/ssl_test_lib.h index f9e031b58..6b9e7b8da 100644 --- a/programs/ssl/ssl_test_lib.h +++ b/programs/ssl/ssl_test_lib.h @@ -95,8 +95,6 @@ #include "../test/query_config.h" -#if defined(MBEDTLS_SSL_EXPORT_KEYS) - typedef struct eap_tls_keys { unsigned char master_secret[48]; @@ -122,8 +120,6 @@ typedef struct dtls_srtp_keys #endif /* MBEDTLS_SSL_DTLS_SRTP */ -#endif /* MBEDTLS_SSL_EXPORT_KEYS */ - typedef struct { mbedtls_ssl_context *ssl; diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 39499d441..e641396aa 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -8613,7 +8613,6 @@ run_test "DTLS proxy: 3d, gnutls server, fragmentation, nbio" \ -s "Extra-header:" \ -c "Extra-header:" -requires_config_enabled MBEDTLS_SSL_EXPORT_KEYS run_test "export keys functionality" \ "$P_SRV eap_tls=1 debug_level=3" \ "$P_CLI eap_tls=1 debug_level=3" \ From 324f72ec9c6e77ec4bd215f6d19cd7b7c6e57c58 Mon Sep 17 00:00:00 2001 From: Andrzej Kurek Date: Wed, 29 Sep 2021 04:21:21 -0400 Subject: [PATCH 2/3] Fix a bug where the ssl context is used after it's nullified When not using DEBUG_C, but using the DTLS CID feature - a null pointer was accessed in ssl_tls.c. Signed-off-by: Andrzej Kurek --- library/ssl_tls.c | 5 +++-- tests/scripts/all.sh | 12 ++++++++++++ 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index f16157a52..821506ff7 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -693,8 +693,9 @@ static int ssl_tls12_populate_transform( mbedtls_ssl_transform *transform, const mbedtls_cipher_info_t *cipher_info; const mbedtls_md_info_t *md_info; -#if !defined(MBEDTLS_DEBUG_C) - ssl = NULL; /* make sure we don't use it except for this case */ +#if !defined(MBEDTLS_DEBUG_C) && \ + !defined(MBEDTLS_SSL_DTLS_CONNECTION_ID) + ssl = NULL; /* make sure we don't use it except for these cases */ (void) ssl; #endif diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index f30795c22..00939a738 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -2083,6 +2083,18 @@ component_test_variable_ssl_in_out_buffer_len_CID () { tests/compat.sh } +component_test_CID_no_debug() { + msg "build: Connection ID enabled, debug disabled" + scripts/config.py unset MBEDTLS_DEBUG_C + scripts/config.py set MBEDTLS_SSL_DTLS_CONNECTION_ID + + CC=gcc cmake . + make + + msg "test: Connection ID enabled, debug disabled" + make test +} + component_test_ssl_alloc_buffer_and_mfl () { msg "build: default config with memory buffer allocator and MFL extension" scripts/config.py set MBEDTLS_MEMORY_BUFFER_ALLOC_C From a72fe641cc44c320b1ed0e61f936f5161731411f Mon Sep 17 00:00:00 2001 From: Andrzej Kurek Date: Wed, 29 Sep 2021 15:57:30 -0400 Subject: [PATCH 3/3] Do not zeroize the ssl context if a key exporting function is set Signed-off-by: Andrzej Kurek --- library/ssl_tls.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 821506ff7..11ccf274c 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -695,8 +695,11 @@ static int ssl_tls12_populate_transform( mbedtls_ssl_transform *transform, #if !defined(MBEDTLS_DEBUG_C) && \ !defined(MBEDTLS_SSL_DTLS_CONNECTION_ID) - ssl = NULL; /* make sure we don't use it except for these cases */ - (void) ssl; + if( ssl->f_export_keys == NULL ) + { + ssl = NULL; /* make sure we don't use it except for these cases */ + (void) ssl; + } #endif /* @@ -959,7 +962,7 @@ static int ssl_tls12_populate_transform( mbedtls_ssl_transform *transform, ((void) mac_dec); ((void) mac_enc); - if( ssl->f_export_keys != NULL ) + if( ssl != NULL && ssl->f_export_keys != NULL ) { ssl->f_export_keys( ssl->p_export_keys, MBEDTLS_SSL_KEY_EXPORT_TLS12_MASTER_SECRET,