From 2fdc7b3599f3eeb14391e925b6b859f9e3ab857c Mon Sep 17 00:00:00 2001 From: Tom Cosgrove Date: Wed, 21 Sep 2022 12:33:17 +0100 Subject: [PATCH 1/9] Return an error from mbedtls_ssl_handshake_step() if neither client nor server This prevents an infinite loop in mbedtls_ssl_handshake(). Fixes #6305. Signed-off-by: Tom Cosgrove --- library/ssl_tls.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 12e1c1b03d..5ea8afadfc 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3243,6 +3243,10 @@ int mbedtls_ssl_handshake_step( mbedtls_ssl_context *ssl ) if( ret != 0 ) goto cleanup; + /* If ssl->conf->endpoint is not one of MBEDTLS_SSL_IS_CLIENT or + * MBEDTLS_SSL_IS_SERVER, this is the return code we give */ + ret = MBEDTLS_ERR_SSL_BAD_INPUT_DATA; + #if defined(MBEDTLS_SSL_CLI_C) if( ssl->conf->endpoint == MBEDTLS_SSL_IS_CLIENT ) { From 87d9c6c4d879d4cf32a9fbc101cb3be3abf05f77 Mon Sep 17 00:00:00 2001 From: Tom Cosgrove Date: Thu, 22 Sep 2022 09:27:56 +0100 Subject: [PATCH 2/9] Ensure client mbedtls_ssl_handshake_step() returns success for HELLO_REQUEST Signed-off-by: Tom Cosgrove --- library/ssl_tls.c | 1 + 1 file changed, 1 insertion(+) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 5ea8afadfc..2d1ffbe040 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3257,6 +3257,7 @@ int mbedtls_ssl_handshake_step( mbedtls_ssl_context *ssl ) { case MBEDTLS_SSL_HELLO_REQUEST: ssl->state = MBEDTLS_SSL_CLIENT_HELLO; + ret = 0; break; case MBEDTLS_SSL_CLIENT_HELLO: From a4b4041219cf7cc273949ef560a3e81122d1a1f9 Mon Sep 17 00:00:00 2001 From: Glenn Strauss Date: Sun, 26 Jun 2022 19:32:09 -0400 Subject: [PATCH 3/9] Shared code to free x509 structs Signed-off-by: Glenn Strauss --- ChangeLog.d/mbedtls_asn1_type_free.txt | 2 + include/mbedtls/asn1.h | 9 +++ library/asn1parse.c | 10 +++ library/ssl_tls12_client.c | 10 +-- library/x509.c | 15 +--- library/x509_crl.c | 24 +------ library/x509_crt.c | 80 ++-------------------- library/x509_csr.c | 12 +--- tests/suites/test_suite_x509parse.function | 24 +------ 9 files changed, 37 insertions(+), 149 deletions(-) create mode 100644 ChangeLog.d/mbedtls_asn1_type_free.txt diff --git a/ChangeLog.d/mbedtls_asn1_type_free.txt b/ChangeLog.d/mbedtls_asn1_type_free.txt new file mode 100644 index 0000000000..87ac5ec5bb --- /dev/null +++ b/ChangeLog.d/mbedtls_asn1_type_free.txt @@ -0,0 +1,2 @@ +Features + * Shared code to free x509 structs like mbedtls_x509_named_data diff --git a/include/mbedtls/asn1.h b/include/mbedtls/asn1.h index be2cae7b5a..5d274950ae 100644 --- a/include/mbedtls/asn1.h +++ b/include/mbedtls/asn1.h @@ -625,6 +625,15 @@ void mbedtls_asn1_free_named_data( mbedtls_asn1_named_data *entry ); */ void mbedtls_asn1_free_named_data_list( mbedtls_asn1_named_data **head ); +/** + * \brief Free all shallow entries in a mbedtls_asn1_named_data list, + * but do not free internal pointer targets. + * + * \param name Head of the list of named data entries to free. + * This function calls mbedtls_free() on each list element. + */ +void mbedtls_asn1_free_named_data_list_shallow( mbedtls_asn1_named_data *name ); + /** \} name Functions to parse ASN.1 data structures */ /** \} addtogroup asn1_module */ diff --git a/library/asn1parse.c b/library/asn1parse.c index d874fff469..12a378cf31 100644 --- a/library/asn1parse.c +++ b/library/asn1parse.c @@ -455,6 +455,16 @@ void mbedtls_asn1_free_named_data_list( mbedtls_asn1_named_data **head ) } } +void mbedtls_asn1_free_named_data_list_shallow( mbedtls_asn1_named_data *name ) +{ + for( mbedtls_asn1_named_data *next; name != NULL; name = next ) + { + next = name->next; + mbedtls_platform_zeroize( name, sizeof( *name ) ); + mbedtls_free( name ); + } +} + const mbedtls_asn1_named_data *mbedtls_asn1_find_named_data( const mbedtls_asn1_named_data *list, const char *oid, size_t len ) { diff --git a/library/ssl_tls12_client.c b/library/ssl_tls12_client.c index 5360b3cb7f..1c53a09903 100644 --- a/library/ssl_tls12_client.c +++ b/library/ssl_tls12_client.c @@ -2680,7 +2680,6 @@ static int ssl_parse_certificate_request( mbedtls_ssl_context *ssl ) { unsigned char *p = dn + i + 2; mbedtls_x509_name name; - mbedtls_x509_name *name_cur, *name_prv; size_t asn1_len; char s[MBEDTLS_X509_MAX_DN_NAME_SIZE]; memset( &name, 0, sizeof( name ) ); @@ -2700,14 +2699,7 @@ static int ssl_parse_certificate_request( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_MSG( 3, ( "DN hint: %.*s", mbedtls_x509_dn_gets( s, sizeof(s), &name ), s ) ); - name_cur = name.next; - while( name_cur != NULL ) - { - name_prv = name_cur; - name_cur = name_cur->next; - mbedtls_platform_zeroize( name_prv, sizeof( mbedtls_x509_name ) ); - mbedtls_free( name_prv ); - } + mbedtls_asn1_free_named_data_list_shallow( name.next ); } #endif diff --git a/library/x509.c b/library/x509.c index c5b0161e71..362e036766 100644 --- a/library/x509.c +++ b/library/x509.c @@ -472,7 +472,6 @@ int mbedtls_x509_get_name( unsigned char **p, const unsigned char *end, size_t set_len; const unsigned char *end_set; mbedtls_x509_name *head = cur; - mbedtls_x509_name *prev, *allocated; /* don't use recursion, we'd risk stack overflow if not optimized */ while( 1 ) @@ -530,18 +529,8 @@ int mbedtls_x509_get_name( unsigned char **p, const unsigned char *end, error: /* Skip the first element as we did not allocate it */ - allocated = head->next; - - while( allocated != NULL ) - { - prev = allocated; - allocated = allocated->next; - - mbedtls_platform_zeroize( prev, sizeof( *prev ) ); - mbedtls_free( prev ); - } - - mbedtls_platform_zeroize( head, sizeof( *head ) ); + mbedtls_asn1_free_named_data_list_shallow( head->next ); + head->next = NULL; return( ret ); } diff --git a/library/x509_crl.c b/library/x509_crl.c index 2a3fac7900..d830fcd05f 100644 --- a/library/x509_crl.c +++ b/library/x509_crl.c @@ -705,28 +705,16 @@ void mbedtls_x509_crl_free( mbedtls_x509_crl *crl ) { mbedtls_x509_crl *crl_cur = crl; mbedtls_x509_crl *crl_prv; - mbedtls_x509_name *name_cur; - mbedtls_x509_name *name_prv; mbedtls_x509_crl_entry *entry_cur; mbedtls_x509_crl_entry *entry_prv; - if( crl == NULL ) - return; - - do + while( crl_cur != NULL ) { #if defined(MBEDTLS_X509_RSASSA_PSS_SUPPORT) mbedtls_free( crl_cur->sig_opts ); #endif - name_cur = crl_cur->issuer.next; - while( name_cur != NULL ) - { - name_prv = name_cur; - name_cur = name_cur->next; - mbedtls_platform_zeroize( name_prv, sizeof( mbedtls_x509_name ) ); - mbedtls_free( name_prv ); - } + mbedtls_asn1_free_named_data_list_shallow( crl_cur->issuer.next ); entry_cur = crl_cur->entry.next; while( entry_cur != NULL ) @@ -744,13 +732,6 @@ void mbedtls_x509_crl_free( mbedtls_x509_crl *crl ) mbedtls_free( crl_cur->raw.p ); } - crl_cur = crl_cur->next; - } - while( crl_cur != NULL ); - - crl_cur = crl; - do - { crl_prv = crl_cur; crl_cur = crl_cur->next; @@ -758,7 +739,6 @@ void mbedtls_x509_crl_free( mbedtls_x509_crl *crl ) if( crl_prv != crl ) mbedtls_free( crl_prv ); } - while( crl_cur != NULL ); } #endif /* MBEDTLS_X509_CRL_PARSE_C */ diff --git a/library/x509_crt.c b/library/x509_crt.c index c4f97bbe2b..81186fa0c0 100644 --- a/library/x509_crt.c +++ b/library/x509_crt.c @@ -685,16 +685,7 @@ static int x509_get_subject_alt_name( unsigned char **p, */ if( ret != 0 && ret != MBEDTLS_ERR_X509_FEATURE_UNAVAILABLE ) { - mbedtls_x509_sequence *seq_cur = subject_alt_name->next; - mbedtls_x509_sequence *seq_prv; - while( seq_cur != NULL ) - { - seq_prv = seq_cur; - seq_cur = seq_cur->next; - mbedtls_platform_zeroize( seq_prv, - sizeof( mbedtls_x509_sequence ) ); - mbedtls_free( seq_prv ); - } + mbedtls_asn1_sequence_free( subject_alt_name->next ); subject_alt_name->next = NULL; return( ret ); } @@ -3300,15 +3291,8 @@ void mbedtls_x509_crt_free( mbedtls_x509_crt *crt ) { mbedtls_x509_crt *cert_cur = crt; mbedtls_x509_crt *cert_prv; - mbedtls_x509_name *name_cur; - mbedtls_x509_name *name_prv; - mbedtls_x509_sequence *seq_cur; - mbedtls_x509_sequence *seq_prv; - if( crt == NULL ) - return; - - do + while( cert_cur != NULL ) { mbedtls_pk_free( &cert_cur->pk ); @@ -3316,53 +3300,11 @@ void mbedtls_x509_crt_free( mbedtls_x509_crt *crt ) mbedtls_free( cert_cur->sig_opts ); #endif - name_cur = cert_cur->issuer.next; - while( name_cur != NULL ) - { - name_prv = name_cur; - name_cur = name_cur->next; - mbedtls_platform_zeroize( name_prv, sizeof( mbedtls_x509_name ) ); - mbedtls_free( name_prv ); - } - - name_cur = cert_cur->subject.next; - while( name_cur != NULL ) - { - name_prv = name_cur; - name_cur = name_cur->next; - mbedtls_platform_zeroize( name_prv, sizeof( mbedtls_x509_name ) ); - mbedtls_free( name_prv ); - } - - seq_cur = cert_cur->ext_key_usage.next; - while( seq_cur != NULL ) - { - seq_prv = seq_cur; - seq_cur = seq_cur->next; - mbedtls_platform_zeroize( seq_prv, - sizeof( mbedtls_x509_sequence ) ); - mbedtls_free( seq_prv ); - } - - seq_cur = cert_cur->subject_alt_names.next; - while( seq_cur != NULL ) - { - seq_prv = seq_cur; - seq_cur = seq_cur->next; - mbedtls_platform_zeroize( seq_prv, - sizeof( mbedtls_x509_sequence ) ); - mbedtls_free( seq_prv ); - } - - seq_cur = cert_cur->certificate_policies.next; - while( seq_cur != NULL ) - { - seq_prv = seq_cur; - seq_cur = seq_cur->next; - mbedtls_platform_zeroize( seq_prv, - sizeof( mbedtls_x509_sequence ) ); - mbedtls_free( seq_prv ); - } + mbedtls_asn1_free_named_data_list_shallow( cert_cur->issuer.next ); + mbedtls_asn1_free_named_data_list_shallow( cert_cur->subject.next ); + mbedtls_asn1_sequence_free( cert_cur->ext_key_usage.next ); + mbedtls_asn1_sequence_free( cert_cur->subject_alt_names.next ); + mbedtls_asn1_sequence_free( cert_cur->certificate_policies.next ); if( cert_cur->raw.p != NULL && cert_cur->own_buffer ) { @@ -3370,13 +3312,6 @@ void mbedtls_x509_crt_free( mbedtls_x509_crt *crt ) mbedtls_free( cert_cur->raw.p ); } - cert_cur = cert_cur->next; - } - while( cert_cur != NULL ); - - cert_cur = crt; - do - { cert_prv = cert_cur; cert_cur = cert_cur->next; @@ -3384,7 +3319,6 @@ void mbedtls_x509_crt_free( mbedtls_x509_crt *crt ) if( cert_prv != crt ) mbedtls_free( cert_prv ); } - while( cert_cur != NULL ); } #if defined(MBEDTLS_ECDSA_C) && defined(MBEDTLS_ECP_RESTARTABLE) diff --git a/library/x509_csr.c b/library/x509_csr.c index dee0ea62d7..f9462added 100644 --- a/library/x509_csr.c +++ b/library/x509_csr.c @@ -375,9 +375,6 @@ void mbedtls_x509_csr_init( mbedtls_x509_csr *csr ) */ void mbedtls_x509_csr_free( mbedtls_x509_csr *csr ) { - mbedtls_x509_name *name_cur; - mbedtls_x509_name *name_prv; - if( csr == NULL ) return; @@ -387,14 +384,7 @@ void mbedtls_x509_csr_free( mbedtls_x509_csr *csr ) mbedtls_free( csr->sig_opts ); #endif - name_cur = csr->subject.next; - while( name_cur != NULL ) - { - name_prv = name_cur; - name_cur = name_cur->next; - mbedtls_platform_zeroize( name_prv, sizeof( mbedtls_x509_name ) ); - mbedtls_free( name_prv ); - } + mbedtls_asn1_free_named_data_list_shallow( csr->subject.next ); if( csr->raw.p != NULL ) { diff --git a/tests/suites/test_suite_x509parse.function b/tests/suites/test_suite_x509parse.function index a3606f29b5..3369a8a3f5 100644 --- a/tests/suites/test_suite_x509parse.function +++ b/tests/suites/test_suite_x509parse.function @@ -825,7 +825,6 @@ void mbedtls_x509_get_name( char * rdn_sequence, int exp_ret ) unsigned char *p; size_t name_len; mbedtls_x509_name head; - mbedtls_x509_name *allocated, *prev; int ret; memset( &head, 0, sizeof( head ) ); @@ -835,17 +834,7 @@ void mbedtls_x509_get_name( char * rdn_sequence, int exp_ret ) ret = mbedtls_x509_get_name( &p, ( name + name_len ), &head ); if( ret == 0 ) - { - allocated = head.next; - - while( allocated != NULL ) - { - prev = allocated; - allocated = allocated->next; - - mbedtls_free( prev ); - } - } + mbedtls_asn1_free_named_data_list_shallow( head.next ); TEST_EQUAL( ret, exp_ret ); @@ -859,7 +848,7 @@ void mbedtls_x509_dn_get_next( char * name_str, int next_merged, char * expected int ret = 0, i; size_t len = 0, out_size; mbedtls_asn1_named_data *names = NULL; - mbedtls_x509_name parsed, *parsed_cur, *parsed_prv; + mbedtls_x509_name parsed, *parsed_cur; // Size of buf is maximum required for test cases unsigned char buf[80], *out = NULL, *c; const char *short_name; @@ -913,14 +902,7 @@ void mbedtls_x509_dn_get_next( char * name_str, int next_merged, char * expected exit: mbedtls_free( out ); mbedtls_asn1_free_named_data_list( &names ); - - parsed_cur = parsed.next; - while( parsed_cur != 0 ) - { - parsed_prv = parsed_cur; - parsed_cur = parsed_cur->next; - mbedtls_free( parsed_prv ); - } + mbedtls_asn1_free_named_data_list_shallow( parsed.next ); } /* END_CASE */ From 7db3124c00afe5162c595c1e73eeec21438c1a23 Mon Sep 17 00:00:00 2001 From: Glenn Strauss Date: Fri, 1 Jul 2022 13:22:45 -0400 Subject: [PATCH 4/9] Skip asn1 zeroize if freeing shallow pointers This skips zeroizing additional pointers to data. (Note: actual sensitive data should still be zeroized when freed.) Signed-off-by: Glenn Strauss --- library/asn1parse.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/library/asn1parse.c b/library/asn1parse.c index 12a378cf31..4bc17710c0 100644 --- a/library/asn1parse.c +++ b/library/asn1parse.c @@ -314,7 +314,6 @@ void mbedtls_asn1_sequence_free( mbedtls_asn1_sequence *seq ) while( seq != NULL ) { mbedtls_asn1_sequence *next = seq->next; - mbedtls_platform_zeroize( seq, sizeof( *seq ) ); mbedtls_free( seq ); seq = next; } @@ -450,7 +449,8 @@ void mbedtls_asn1_free_named_data_list( mbedtls_asn1_named_data **head ) while( ( cur = *head ) != NULL ) { *head = cur->next; - mbedtls_asn1_free_named_data( cur ); + mbedtls_free( cur->oid.p ); + mbedtls_free( cur->val.p ); mbedtls_free( cur ); } } @@ -460,7 +460,6 @@ void mbedtls_asn1_free_named_data_list_shallow( mbedtls_asn1_named_data *name ) for( mbedtls_asn1_named_data *next; name != NULL; name = next ) { next = name->next; - mbedtls_platform_zeroize( name, sizeof( *name ) ); mbedtls_free( name ); } } From 7ba7b3aded7f51051213704edc6c6e710cdee9aa Mon Sep 17 00:00:00 2001 From: Gabor Mezei Date: Wed, 19 Oct 2022 17:22:15 +0200 Subject: [PATCH 5/9] Update tests to use mbedtls_test_read_mpi_core In conditional assign and swap tests use the mbedtls_test_read_mpi_core function for reading MPIs. Signed-off-by: Gabor Mezei --- tests/suites/test_suite_bignum_core.function | 54 +++++--------- .../suites/test_suite_bignum_mod_raw.function | 70 ++++++------------- 2 files changed, 39 insertions(+), 85 deletions(-) diff --git a/tests/suites/test_suite_bignum_core.function b/tests/suites/test_suite_bignum_core.function index f50fd07e41..021b7b31cf 100644 --- a/tests/suites/test_suite_bignum_core.function +++ b/tests/suites/test_suite_bignum_core.function @@ -345,14 +345,18 @@ exit: /* END_CASE */ /* BEGIN_CASE */ -void mpi_core_cond_assign( data_t * input_X, - data_t * input_Y, +void mpi_core_cond_assign( char * input_X, + char * input_Y, int input_bytes ) { mbedtls_mpi_uint *X = NULL; mbedtls_mpi_uint *Y = NULL; - size_t limbs_X = CHARS_TO_LIMBS( input_X->len ); - size_t limbs_Y = CHARS_TO_LIMBS( input_Y->len ); + size_t limbs_X; + size_t limbs_Y; + + TEST_EQUAL( mbedtls_test_read_mpi_core( &X, &limbs_X, input_X ), 0 ); + TEST_EQUAL( mbedtls_test_read_mpi_core( &Y, &limbs_Y, input_Y ), 0 ); + size_t limbs = limbs_X; size_t copy_limbs = CHARS_TO_LIMBS( input_bytes ); size_t bytes = limbs * sizeof( mbedtls_mpi_uint ); @@ -361,24 +365,12 @@ void mpi_core_cond_assign( data_t * input_X, TEST_EQUAL( limbs_X, limbs_Y ); TEST_ASSERT( copy_limbs <= limbs ); - ASSERT_ALLOC( X, limbs ); - ASSERT_ALLOC( Y, limbs ); - - TEST_ASSERT( mbedtls_mpi_core_read_be( X, limbs, input_X->x, input_X->len ) - == 0 ); - - TEST_ASSERT( mbedtls_mpi_core_read_be( Y, limbs, input_Y->x, input_Y->len ) - == 0 ); - /* condition is false */ TEST_CF_SECRET( X, bytes ); TEST_CF_SECRET( Y, bytes ); mbedtls_mpi_core_cond_assign( X, Y, copy_limbs, 0 ); - TEST_CF_PUBLIC( X, bytes ); - TEST_CF_PUBLIC( Y, bytes ); - TEST_ASSERT( memcmp( X, Y, bytes ) != 0 ); /* condition is true */ @@ -387,9 +379,6 @@ void mpi_core_cond_assign( data_t * input_X, mbedtls_mpi_core_cond_assign( X, Y, copy_limbs, 1 ); - TEST_CF_PUBLIC( X, bytes ); - TEST_CF_PUBLIC( Y, bytes ); - /* Check if the given length is copied even it is smaller than the length of the given MPIs. */ if( copy_limbs < limbs ) @@ -407,16 +396,20 @@ exit: /* END_CASE */ /* BEGIN_CASE */ -void mpi_core_cond_swap( data_t * input_X, - data_t * input_Y, +void mpi_core_cond_swap( char * input_X, + char * input_Y, int input_bytes ) { mbedtls_mpi_uint *tmp_X = NULL; mbedtls_mpi_uint *tmp_Y = NULL; mbedtls_mpi_uint *X = NULL; mbedtls_mpi_uint *Y = NULL; - size_t limbs_X = CHARS_TO_LIMBS( input_X->len ); - size_t limbs_Y = CHARS_TO_LIMBS( input_Y->len ); + size_t limbs_X; + size_t limbs_Y; + + TEST_EQUAL( mbedtls_test_read_mpi_core( &tmp_X, &limbs_X, input_X ), 0 ); + TEST_EQUAL( mbedtls_test_read_mpi_core( &tmp_Y, &limbs_Y, input_Y ), 0 ); + size_t limbs = limbs_X; size_t copy_limbs = CHARS_TO_LIMBS( input_bytes ); size_t bytes = limbs * sizeof( mbedtls_mpi_uint ); @@ -425,18 +418,9 @@ void mpi_core_cond_swap( data_t * input_X, TEST_EQUAL( limbs_X, limbs_Y ); TEST_ASSERT( copy_limbs <= limbs ); - ASSERT_ALLOC( tmp_X, limbs ); - ASSERT_ALLOC( tmp_Y, limbs ); - - TEST_ASSERT( mbedtls_mpi_core_read_be( tmp_X, limbs, - input_X->x, input_X->len ) - == 0 ); ASSERT_ALLOC( X, limbs ); memcpy( X, tmp_X, bytes ); - TEST_ASSERT( mbedtls_mpi_core_read_be( tmp_Y, limbs, - input_Y->x, input_Y->len ) - == 0 ); ASSERT_ALLOC( Y, limbs ); memcpy( Y, tmp_Y, bytes ); @@ -446,9 +430,6 @@ void mpi_core_cond_swap( data_t * input_X, mbedtls_mpi_core_cond_swap( X, Y, copy_limbs, 0 ); - TEST_CF_PUBLIC( X, bytes ); - TEST_CF_PUBLIC( Y, bytes ); - ASSERT_COMPARE( X, bytes, tmp_X, bytes ); ASSERT_COMPARE( Y, bytes, tmp_Y, bytes ); @@ -458,9 +439,6 @@ void mpi_core_cond_swap( data_t * input_X, mbedtls_mpi_core_cond_swap( X, Y, copy_limbs, 1 ); - TEST_CF_PUBLIC( X, bytes ); - TEST_CF_PUBLIC( Y, bytes ); - /* Check if the given length is copied even it is smaller than the length of the given MPIs. */ if( copy_limbs < limbs ) diff --git a/tests/suites/test_suite_bignum_mod_raw.function b/tests/suites/test_suite_bignum_mod_raw.function index 8ac1ef4977..556cca07f0 100644 --- a/tests/suites/test_suite_bignum_mod_raw.function +++ b/tests/suites/test_suite_bignum_mod_raw.function @@ -110,16 +110,20 @@ exit: /* END_CASE */ /* BEGIN_CASE */ -void mpi_mod_raw_cond_assign( data_t * input_X, - data_t * input_Y, +void mpi_mod_raw_cond_assign( char * input_X, + char * input_Y, int input_bytes ) { mbedtls_mpi_uint *X = NULL; mbedtls_mpi_uint *Y = NULL; mbedtls_mpi_uint *buff_m = NULL; mbedtls_mpi_mod_modulus m; - size_t limbs_X = CHARS_TO_LIMBS( input_X->len ); - size_t limbs_Y = CHARS_TO_LIMBS( input_Y->len ); + size_t limbs_X; + size_t limbs_Y; + + TEST_EQUAL( mbedtls_test_read_mpi_core( &X, &limbs_X, input_X ), 0 ); + TEST_EQUAL( mbedtls_test_read_mpi_core( &Y, &limbs_Y, input_Y ), 0 ); + size_t limbs = limbs_X; size_t copy_limbs = CHARS_TO_LIMBS( input_bytes ); size_t bytes = limbs * sizeof( mbedtls_mpi_uint ); @@ -130,24 +134,12 @@ void mpi_mod_raw_cond_assign( data_t * input_X, TEST_EQUAL( limbs_X, limbs_Y ); TEST_ASSERT( copy_limbs <= limbs ); - ASSERT_ALLOC( X, limbs ); - ASSERT_ALLOC( Y, limbs ); - - ASSERT_ALLOC( buff_m, limbs ); - memset( buff_m, 0xFF, copy_bytes ); - TEST_ASSERT( mbedtls_mpi_mod_modulus_setup( + ASSERT_ALLOC( buff_m, copy_limbs ); + memset( buff_m, 0xFF, copy_limbs ); + TEST_EQUAL( mbedtls_mpi_mod_modulus_setup( &m, buff_m, copy_limbs, MBEDTLS_MPI_MOD_EXT_REP_BE, - MBEDTLS_MPI_MOD_REP_MONTGOMERY ) - == 0 ); - - TEST_ASSERT( mbedtls_mpi_core_read_be( X, limbs, - input_X->x, input_X->len ) - == 0 ); - - TEST_ASSERT( mbedtls_mpi_core_read_be( Y, limbs, - input_Y->x, input_Y->len ) - == 0 ); + MBEDTLS_MPI_MOD_REP_MONTGOMERY ), 0 ); /* condition is false */ TEST_CF_SECRET( X, bytes ); @@ -155,9 +147,6 @@ void mpi_mod_raw_cond_assign( data_t * input_X, mbedtls_mpi_mod_raw_cond_assign( X, Y, &m, 0 ); - TEST_CF_PUBLIC( X, bytes ); - TEST_CF_PUBLIC( Y, bytes ); - TEST_ASSERT( memcmp( X, Y, bytes ) != 0 ); /* condition is true */ @@ -166,9 +155,6 @@ void mpi_mod_raw_cond_assign( data_t * input_X, mbedtls_mpi_mod_raw_cond_assign( X, Y, &m, 1 ); - TEST_CF_PUBLIC( X, bytes ); - TEST_CF_PUBLIC( Y, bytes ); - /* Check if the given length is copied even it is smaller than the length of the given MPIs. */ if( copy_limbs len ); - size_t limbs_Y = CHARS_TO_LIMBS( input_Y->len ); + size_t limbs_X; + size_t limbs_Y; + + TEST_EQUAL( mbedtls_test_read_mpi_core( &tmp_X, &limbs_X, input_X ), 0 ); + TEST_EQUAL( mbedtls_test_read_mpi_core( &tmp_Y, &limbs_Y, input_Y ), 0 ); + size_t limbs = limbs_X; size_t copy_limbs = CHARS_TO_LIMBS( input_bytes ); size_t bytes = limbs * sizeof( mbedtls_mpi_uint ); @@ -211,24 +201,16 @@ void mpi_mod_raw_cond_swap( data_t * input_X, TEST_EQUAL( limbs_X, limbs_Y ); TEST_ASSERT( copy_limbs <= limbs ); - ASSERT_ALLOC( tmp_X, limbs ); - ASSERT_ALLOC( tmp_Y, limbs ); - ASSERT_ALLOC( buff_m, copy_limbs ); - memset( buff_m, 0xFF, copy_bytes ); - TEST_ASSERT( mbedtls_mpi_mod_modulus_setup( + memset( buff_m, 0xFF, copy_limbs ); + TEST_EQUAL( mbedtls_mpi_mod_modulus_setup( &m, buff_m, copy_limbs, MBEDTLS_MPI_MOD_EXT_REP_BE, - MBEDTLS_MPI_MOD_REP_MONTGOMERY ) - == 0 ); + MBEDTLS_MPI_MOD_REP_MONTGOMERY ), 0 ); - TEST_ASSERT( mbedtls_mpi_core_read_be( tmp_X, limbs, input_X->x, input_X->len ) - == 0 ); ASSERT_ALLOC( X, limbs ); memcpy( X, tmp_X, bytes ); - TEST_ASSERT( mbedtls_mpi_core_read_be( tmp_Y, limbs, input_Y->x, input_Y->len ) - == 0 ); ASSERT_ALLOC( Y, bytes ); memcpy( Y, tmp_Y, bytes ); @@ -238,9 +220,6 @@ void mpi_mod_raw_cond_swap( data_t * input_X, mbedtls_mpi_mod_raw_cond_swap( X, Y, &m, 0 ); - TEST_CF_PUBLIC( X, bytes ); - TEST_CF_PUBLIC( Y, bytes ); - ASSERT_COMPARE( X, bytes, tmp_X, bytes ); ASSERT_COMPARE( Y, bytes, tmp_Y, bytes ); @@ -250,9 +229,6 @@ void mpi_mod_raw_cond_swap( data_t * input_X, mbedtls_mpi_mod_raw_cond_swap( X, Y, &m, 1 ); - TEST_CF_PUBLIC( X, bytes ); - TEST_CF_PUBLIC( Y, bytes ); - /* Check if the given length is copied even it is smaller than the length of the given MPIs. */ if( copy_limbs < limbs ) From a8cf998bc9569ce29dae28c3387a44882d226cdc Mon Sep 17 00:00:00 2001 From: Gabor Mezei Date: Thu, 20 Oct 2022 12:27:36 +0200 Subject: [PATCH 6/9] Let the allocated memory visible for the memory sanitizer Signed-off-by: Gabor Mezei --- tests/suites/test_suite_bignum_core.function | 15 +++++++++++++++ tests/suites/test_suite_bignum_mod_raw.function | 12 ++++++++++++ 2 files changed, 27 insertions(+) diff --git a/tests/suites/test_suite_bignum_core.function b/tests/suites/test_suite_bignum_core.function index 021b7b31cf..612a7c6bd4 100644 --- a/tests/suites/test_suite_bignum_core.function +++ b/tests/suites/test_suite_bignum_core.function @@ -371,6 +371,9 @@ void mpi_core_cond_assign( char * input_X, mbedtls_mpi_core_cond_assign( X, Y, copy_limbs, 0 ); + TEST_CF_PUBLIC( X, bytes ); + TEST_CF_PUBLIC( Y, bytes ); + TEST_ASSERT( memcmp( X, Y, bytes ) != 0 ); /* condition is true */ @@ -379,10 +382,16 @@ void mpi_core_cond_assign( char * input_X, mbedtls_mpi_core_cond_assign( X, Y, copy_limbs, 1 ); + TEST_CF_PUBLIC( X, bytes ); + TEST_CF_PUBLIC( Y, bytes ); + /* Check if the given length is copied even it is smaller than the length of the given MPIs. */ if( copy_limbs < limbs ) { + TEST_CF_PUBLIC( X, bytes ); + TEST_CF_PUBLIC( Y, bytes ); + ASSERT_COMPARE( X, copy_bytes, Y, copy_bytes ); TEST_ASSERT( memcmp( X, Y, bytes ) != 0 ); } @@ -430,6 +439,9 @@ void mpi_core_cond_swap( char * input_X, mbedtls_mpi_core_cond_swap( X, Y, copy_limbs, 0 ); + TEST_CF_PUBLIC( X, bytes ); + TEST_CF_PUBLIC( Y, bytes ); + ASSERT_COMPARE( X, bytes, tmp_X, bytes ); ASSERT_COMPARE( Y, bytes, tmp_Y, bytes ); @@ -439,6 +451,9 @@ void mpi_core_cond_swap( char * input_X, mbedtls_mpi_core_cond_swap( X, Y, copy_limbs, 1 ); + TEST_CF_PUBLIC( X, bytes ); + TEST_CF_PUBLIC( Y, bytes ); + /* Check if the given length is copied even it is smaller than the length of the given MPIs. */ if( copy_limbs < limbs ) diff --git a/tests/suites/test_suite_bignum_mod_raw.function b/tests/suites/test_suite_bignum_mod_raw.function index 556cca07f0..4b906751f2 100644 --- a/tests/suites/test_suite_bignum_mod_raw.function +++ b/tests/suites/test_suite_bignum_mod_raw.function @@ -147,6 +147,9 @@ void mpi_mod_raw_cond_assign( char * input_X, mbedtls_mpi_mod_raw_cond_assign( X, Y, &m, 0 ); + TEST_CF_PUBLIC( X, bytes ); + TEST_CF_PUBLIC( Y, bytes ); + TEST_ASSERT( memcmp( X, Y, bytes ) != 0 ); /* condition is true */ @@ -155,6 +158,9 @@ void mpi_mod_raw_cond_assign( char * input_X, mbedtls_mpi_mod_raw_cond_assign( X, Y, &m, 1 ); + TEST_CF_PUBLIC( X, bytes ); + TEST_CF_PUBLIC( Y, bytes ); + /* Check if the given length is copied even it is smaller than the length of the given MPIs. */ if( copy_limbs Date: Fri, 4 Nov 2022 04:01:23 -0400 Subject: [PATCH 7/9] Deprecate mbedtls_asn1_free_named_data() Signed-off-by: Glenn Strauss --- ChangeLog.d/mbedtls_asn1_type_free.txt | 4 ++++ include/mbedtls/asn1.h | 16 ++++++++++++---- library/asn1parse.c | 2 ++ tests/suites/test_suite_asn1parse.data | 5 +++++ tests/suites/test_suite_asn1parse.function | 4 ++-- 5 files changed, 25 insertions(+), 6 deletions(-) diff --git a/ChangeLog.d/mbedtls_asn1_type_free.txt b/ChangeLog.d/mbedtls_asn1_type_free.txt index 87ac5ec5bb..81f3a2007f 100644 --- a/ChangeLog.d/mbedtls_asn1_type_free.txt +++ b/ChangeLog.d/mbedtls_asn1_type_free.txt @@ -1,2 +1,6 @@ Features * Shared code to free x509 structs like mbedtls_x509_named_data +New deprecations + * Deprecate mbedtls_asn1_free_named_data(). + Use mbedtls_asn1_free_named_data_list() + or mbedtls_asn1_free_named_data_list_shallow() diff --git a/include/mbedtls/asn1.h b/include/mbedtls/asn1.h index 5d274950ae..8b66ee228c 100644 --- a/include/mbedtls/asn1.h +++ b/include/mbedtls/asn1.h @@ -24,6 +24,7 @@ #include "mbedtls/private_access.h" #include "mbedtls/build_info.h" +#include "mbedtls/platform_util.h" #include @@ -606,22 +607,29 @@ int mbedtls_asn1_get_alg_null( unsigned char **p, const mbedtls_asn1_named_data *mbedtls_asn1_find_named_data( const mbedtls_asn1_named_data *list, const char *oid, size_t len ); +#if !defined(MBEDTLS_DEPRECATED_REMOVED) /** * \brief Free a mbedtls_asn1_named_data entry * + * \deprecated This function is deprecated and will be removed in a + * future version of the library. + * Please use mbedtls_asn1_free_named_data_list() + * or mbedtls_asn1_free_named_data_list_shallow(). + * * \param entry The named data entry to free. * This function calls mbedtls_free() on * `entry->oid.p` and `entry->val.p`. */ -void mbedtls_asn1_free_named_data( mbedtls_asn1_named_data *entry ); +void MBEDTLS_DEPRECATED mbedtls_asn1_free_named_data( mbedtls_asn1_named_data *entry ); +#endif /* MBEDTLS_DEPRECATED_REMOVED */ /** * \brief Free all entries in a mbedtls_asn1_named_data list. * * \param head Pointer to the head of the list of named data entries to free. - * This function calls mbedtls_asn1_free_named_data() and - * mbedtls_free() on each list element and - * sets \c *head to \c NULL. + * This function calls mbedtls_free() on + * `entry->oid.p` and `entry->val.p` and then on `entry` + * for each list entry, and sets \c *head to \c NULL. */ void mbedtls_asn1_free_named_data_list( mbedtls_asn1_named_data **head ); diff --git a/library/asn1parse.c b/library/asn1parse.c index 4bc17710c0..28a3b144bf 100644 --- a/library/asn1parse.c +++ b/library/asn1parse.c @@ -431,6 +431,7 @@ int mbedtls_asn1_get_alg_null( unsigned char **p, return( 0 ); } +#if !defined(MBEDTLS_DEPRECATED_REMOVED) void mbedtls_asn1_free_named_data( mbedtls_asn1_named_data *cur ) { if( cur == NULL ) @@ -441,6 +442,7 @@ void mbedtls_asn1_free_named_data( mbedtls_asn1_named_data *cur ) mbedtls_platform_zeroize( cur, sizeof( mbedtls_asn1_named_data ) ); } +#endif /* MBEDTLS_DEPRECATED_REMOVED */ void mbedtls_asn1_free_named_data_list( mbedtls_asn1_named_data **head ) { diff --git a/tests/suites/test_suite_asn1parse.data b/tests/suites/test_suite_asn1parse.data index 36ab1e481c..c129e3c8f7 100644 --- a/tests/suites/test_suite_asn1parse.data +++ b/tests/suites/test_suite_asn1parse.data @@ -608,18 +608,23 @@ Find named data: first match find_named_data:"414141":"414141":"434343":"444444":"414141":0:0 Free named data: null pointer +depends_on:MBEDTLS_TEST_DEPRECATED free_named_data_null: Free named data: all null +depends_on:MBEDTLS_TEST_DEPRECATED free_named_data:0:0:0 Free named data: with oid +depends_on:MBEDTLS_TEST_DEPRECATED free_named_data:1:0:0 Free named data: with val +depends_on:MBEDTLS_TEST_DEPRECATED free_named_data:0:1:0 Free named data: with next +depends_on:MBEDTLS_TEST_DEPRECATED free_named_data:0:0:1 Free named data list (empty) diff --git a/tests/suites/test_suite_asn1parse.function b/tests/suites/test_suite_asn1parse.function index 002d8c4269..dac8e312b8 100644 --- a/tests/suites/test_suite_asn1parse.function +++ b/tests/suites/test_suite_asn1parse.function @@ -735,7 +735,7 @@ void find_named_data( data_t *oid0, data_t *oid1, data_t *oid2, data_t *oid3, } /* END_CASE */ -/* BEGIN_CASE */ +/* BEGIN_CASE depends_on:!MBEDTLS_DEPRECATED_REMOVED */ void free_named_data_null( ) { mbedtls_asn1_free_named_data( NULL ); @@ -743,7 +743,7 @@ void free_named_data_null( ) } /* END_CASE */ -/* BEGIN_CASE */ +/* BEGIN_CASE depends_on:!MBEDTLS_DEPRECATED_REMOVED */ void free_named_data( int with_oid, int with_val, int with_next ) { mbedtls_asn1_named_data next = From aa36c2a6f66a2db081238f3e202ca28a625fae3b Mon Sep 17 00:00:00 2001 From: Glenn Strauss Date: Mon, 7 Nov 2022 20:08:54 -0500 Subject: [PATCH 8/9] Update tests/suites/test_suite_asn1parse.function Co-authored-by: Andrzej Kurek Signed-off-by: Glenn Strauss --- tests/suites/test_suite_asn1parse.function | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/suites/test_suite_asn1parse.function b/tests/suites/test_suite_asn1parse.function index dac8e312b8..6d7e49ee47 100644 --- a/tests/suites/test_suite_asn1parse.function +++ b/tests/suites/test_suite_asn1parse.function @@ -743,7 +743,7 @@ void free_named_data_null( ) } /* END_CASE */ -/* BEGIN_CASE depends_on:!MBEDTLS_DEPRECATED_REMOVED */ +/* BEGIN_CASE depends_on:!MBEDTLS_DEPRECATED_REMOVED:!MBEDTLS_DEPRECATED_WARNING */ void free_named_data( int with_oid, int with_val, int with_next ) { mbedtls_asn1_named_data next = From 2a642996483a26166aa20284d46540a10a992aa7 Mon Sep 17 00:00:00 2001 From: Glenn Strauss Date: Mon, 7 Nov 2022 20:09:38 -0500 Subject: [PATCH 9/9] Update tests/suites/test_suite_asn1parse.function Co-authored-by: Andrzej Kurek Signed-off-by: Glenn Strauss --- tests/suites/test_suite_asn1parse.function | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/suites/test_suite_asn1parse.function b/tests/suites/test_suite_asn1parse.function index 6d7e49ee47..62669b35f1 100644 --- a/tests/suites/test_suite_asn1parse.function +++ b/tests/suites/test_suite_asn1parse.function @@ -735,7 +735,7 @@ void find_named_data( data_t *oid0, data_t *oid1, data_t *oid2, data_t *oid3, } /* END_CASE */ -/* BEGIN_CASE depends_on:!MBEDTLS_DEPRECATED_REMOVED */ +/* BEGIN_CASE depends_on:!MBEDTLS_DEPRECATED_REMOVED:!MBEDTLS_DEPRECATED_WARNING */ void free_named_data_null( ) { mbedtls_asn1_free_named_data( NULL );