From 480416af9d4acb874d5f60bb725334f71103daf7 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 28 Jun 2018 19:04:07 +0200 Subject: [PATCH 1/6] Fix argument validation in asn1_write_10x 1 << bits doesn't work when bits is too large. Found by ASan. --- tests/suites/test_suite_psa_crypto.function | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/suites/test_suite_psa_crypto.function b/tests/suites/test_suite_psa_crypto.function index 0d1a25c82..5f705e3e3 100644 --- a/tests/suites/test_suite_psa_crypto.function +++ b/tests/suites/test_suite_psa_crypto.function @@ -46,7 +46,9 @@ static int asn1_write_10x( unsigned char **p, { int ret; int len = bits / 8 + 1; - if( x >= 1 << bits ) + if( bits == 0 ) + return( MBEDTLS_ERR_ASN1_INVALID_DATA ); + if( bits <= 8 && x >= 1 << ( bits - 1 ) ) return( MBEDTLS_ERR_ASN1_INVALID_DATA ); if( *p < start || *p - start < (ssize_t) len ) return( MBEDTLS_ERR_ASN1_BUF_TOO_SMALL ); From 46f1fd7afd4f17062883ebd6c0dbf915602212f5 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 28 Jun 2018 19:31:31 +0200 Subject: [PATCH 2/6] Handle null pointers safely when used as buffers of size 0 When the size of a buffer is 0, the corresponding pointer argument may be null. In such cases, library functions must not perform arithmetic on the pointer or call standard library functions such as memset and memcpy, since that would be undefined behavior in C. Protect such cases. Refactor the storage of a 0-sized raw data object to make it store a null pointer, rather than depending on the behavior of calloc(1,0). --- library/psa_crypto.c | 50 +++++++++++++++++++++++++++++++++++--------- 1 file changed, 40 insertions(+), 10 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 4a3363952..19db5a9ec 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -409,10 +409,17 @@ static psa_status_t prepare_raw_data_slot( psa_key_type_t type, switch( type ) { case PSA_KEY_TYPE_RAW_DATA: + if( bits == 0 ) + { + raw->bytes = 0; + raw->data = NULL; + return( PSA_SUCCESS ); + } + break; #if defined(MBEDTLS_MD_C) case PSA_KEY_TYPE_HMAC: -#endif break; +#endif #if defined(MBEDTLS_AES_C) case PSA_KEY_TYPE_AES: if( bits != 128 && bits != 192 && bits != 256 ) @@ -478,7 +485,8 @@ psa_status_t psa_import_key( psa_key_slot_t key, &slot->data.raw ); if( status != PSA_SUCCESS ) return( status ); - memcpy( slot->data.raw.data, data, data_length ); + if( data_length != 0 ) + memcpy( slot->data.raw.data, data, data_length ); } else #if defined(MBEDTLS_PK_PARSE_C) @@ -679,7 +687,8 @@ static psa_status_t psa_internal_export_key( psa_key_slot_t key, { if( slot->data.raw.bytes > data_size ) return( PSA_ERROR_BUFFER_TOO_SMALL ); - memcpy( data, slot->data.raw.data, slot->data.raw.bytes ); + if( slot->data.raw.bytes != 0 ) + memcpy( data, slot->data.raw.data, slot->data.raw.bytes ); *data_length = slot->data.raw.bytes; return( PSA_SUCCESS ); } @@ -710,7 +719,10 @@ static psa_status_t psa_internal_export_key( psa_key_slot_t key, ret = mbedtls_pk_write_key_der( &pk, data, data_size ); if( ret < 0 ) { - memset( data, 0, data_size ); + /* If data_size is 0 then data may be NULL and then the + * call to memset would have undefined behavior. */ + if( data_size != 0 ) + memset( data, 0, data_size ); return( mbedtls_to_psa_error( ret ) ); } /* The mbedtls_pk_xxx functions write to the end of the buffer. @@ -999,7 +1011,10 @@ psa_status_t psa_hash_finish( psa_hash_operation_t *operation, * (barring an attack on the hash and deliberately-crafted input), * in case the caller doesn't check the return status properly. */ *hash_length = actual_hash_length; - memset( hash, '!', hash_size ); + /* If hash_size is 0 then hash may be NULL and then the + * call to memset would have undefined behavior. */ + if( hash_size != 0 ) + memset( hash, '!', hash_size ); if( hash_size < actual_hash_length ) return( PSA_ERROR_BUFFER_TOO_SMALL ); @@ -1500,7 +1515,10 @@ static psa_status_t psa_mac_finish_internal( psa_mac_operation_t *operation, * (barring an attack on the mac and deliberately-crafted input), * in case the caller doesn't check the return status properly. */ *mac_length = operation->mac_size; - memset( mac, '!', mac_size ); + /* If mac_size is 0 then mac may be NULL and then the + * call to memset would have undefined behavior. */ + if( mac_size != 0 ) + memset( mac, '!', mac_size ); if( mac_size < operation->mac_size ) return( PSA_ERROR_BUFFER_TOO_SMALL ); @@ -1944,8 +1962,10 @@ exit: if( status == PSA_SUCCESS ) memset( signature + *signature_length, '!', signature_size - *signature_length ); - else + else if( signature_size != 0 ) memset( signature, '!', signature_size ); + /* If signature_size is 0 then we have nothing to do. We must not call + * memset because signature may be NULL in this case. */ return( status ); } @@ -2410,7 +2430,9 @@ psa_status_t psa_cipher_finish( psa_cipher_operation_t *operation, psa_cipher_abort( operation ); return( mbedtls_to_psa_error( ret ) ); } - if( output_size >= *output_length ) + if( *output_length == 0 ) + /* Nothing to copy. Note that output may be NULL in this case. */ ; + else if( output_size >= *output_length ) memcpy( output, temp_output_buffer, *output_length ); else { @@ -2684,7 +2706,10 @@ psa_status_t psa_aead_encrypt( psa_key_slot_t key, if( ret != 0 ) { - memset( ciphertext, 0, ciphertext_size ); + /* If ciphertext_size is 0 then ciphertext may be NULL and then the + * call to memset would have undefined behavior. */ + if( ciphertext_size != 0 ) + memset( ciphertext, 0, ciphertext_size ); return( mbedtls_to_psa_error( ret ) ); } @@ -2823,7 +2848,12 @@ psa_status_t psa_aead_decrypt( psa_key_slot_t key, } if( ret != 0 ) - memset( plaintext, 0, plaintext_size ); + { + /* If plaintext_size is 0 then plaintext may be NULL and then the + * call to memset has undefined behavior. */ + if( plaintext_size != 0 ) + memset( plaintext, 0, plaintext_size ); + } else *plaintext_length = ciphertext_length - tag_length; From 1ae051409f69d0db7c492fa59b57bc8755c71109 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Sat, 30 Jun 2018 17:46:59 +0200 Subject: [PATCH 3/6] Fix memory leak when importing an RSA key that is too large --- library/psa_crypto.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 19db5a9ec..1d5337bfb 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -514,7 +514,10 @@ psa_status_t psa_import_key( psa_key_slot_t key, mbedtls_rsa_context *rsa = mbedtls_pk_rsa( pk ); size_t bits = mbedtls_rsa_get_bitlen( rsa ); if( bits > PSA_VENDOR_RSA_MAX_KEY_BITS ) - return( PSA_ERROR_NOT_SUPPORTED ); + { + status = PSA_ERROR_NOT_SUPPORTED; + break; + } slot->data.rsa = rsa; } else From 02b750781f64f04c9c23f06245f39535ac4607bb Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Sun, 1 Jul 2018 22:31:34 +0200 Subject: [PATCH 4/6] Factor duplicated code into exercise_key Also fail the test if the test code lacks a way to exercise the key. --- tests/suites/test_suite_psa_crypto.function | 54 +++++++++++++-------- 1 file changed, 34 insertions(+), 20 deletions(-) diff --git a/tests/suites/test_suite_psa_crypto.function b/tests/suites/test_suite_psa_crypto.function index 5f705e3e3..1017e88c8 100644 --- a/tests/suites/test_suite_psa_crypto.function +++ b/tests/suites/test_suite_psa_crypto.function @@ -345,6 +345,36 @@ static int exercise_asymmetric_encryption_key( psa_key_slot_t key, exit: return( 0 ); } + +static int exercise_key( psa_key_slot_t slot, + psa_key_usage_t usage, + psa_algorithm_t alg ) +{ + int ok; + if( alg == 0 ) + ok = 1; /* If no algorihm, do nothing (used for raw data "keys"). */ + else if( PSA_ALG_IS_MAC( alg ) ) + ok = exercise_mac_key( slot, usage, alg ); + else if( PSA_ALG_IS_CIPHER( alg ) ) + ok = exercise_cipher_key( slot, usage, alg ); + else if( PSA_ALG_IS_AEAD( alg ) ) + ok = exercise_aead_key( slot, usage, alg ); + else if( PSA_ALG_IS_SIGN( alg ) ) + ok = exercise_signature_key( slot, usage, alg ); + else if( PSA_ALG_IS_ASYMMETRIC_ENCRYPTION( alg ) ) + ok = exercise_asymmetric_encryption_key( slot, usage, alg ); + else + { + char message[40]; + mbedtls_snprintf( message, sizeof( message ), + "No code to exercise alg=0x%08lx", + (unsigned long) alg ); + test_fail( message, __LINE__, __FILE__ ); + ok = 0; + } + return( ok ); +} + /* END_HEADER */ /* BEGIN_DEPENDENCIES @@ -640,16 +670,8 @@ void import_and_exercise_key( data_t *data, TEST_ASSERT( got_bits == bits ); /* Do something with the key according to its type and permitted usage. */ - if( PSA_ALG_IS_MAC( alg ) ) - exercise_mac_key( slot, usage, alg ); - else if( PSA_ALG_IS_CIPHER( alg ) ) - exercise_cipher_key( slot, usage, alg ); - else if( PSA_ALG_IS_AEAD( alg ) ) - exercise_aead_key( slot, usage, alg ); - else if( PSA_ALG_IS_SIGN( alg ) ) - exercise_signature_key( slot, usage, alg ); - else if( PSA_ALG_IS_ASYMMETRIC_ENCRYPTION( alg ) ) - exercise_asymmetric_encryption_key( slot, usage, alg ); + if( ! exercise_key( slot, usage, alg ) ) + goto exit; exit: psa_destroy_key( slot ); @@ -2260,16 +2282,8 @@ void generate_key( int type_arg, } /* Do something with the key according to its type and permitted usage. */ - if( PSA_ALG_IS_MAC( alg ) ) - exercise_mac_key( slot, usage, alg ); - else if( PSA_ALG_IS_CIPHER( alg ) ) - exercise_cipher_key( slot, usage, alg ); - else if( PSA_ALG_IS_AEAD( alg ) ) - exercise_aead_key( slot, usage, alg ); - else if( PSA_ALG_IS_SIGN( alg ) ) - exercise_signature_key( slot, usage, alg ); - else if( PSA_ALG_IS_ASYMMETRIC_ENCRYPTION( alg ) ) - exercise_asymmetric_encryption_key( slot, usage, alg ); + if( ! exercise_key( slot, usage, alg ) ) + goto exit; exit: psa_destroy_key( slot ); From aee13338b3a26045a7a8c8ff999312a26c30b6f1 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 2 Jul 2018 12:15:28 +0200 Subject: [PATCH 5/6] Fix safe output length in hash and mac finish In psa_hash_finish and psa_mac_finish_internal, set the fallback output length (which is reported on error) to the output buffer size, not to the _expected_ buffer size which could be larger. --- library/psa_crypto.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 1d5337bfb..a2f68975b 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -1013,7 +1013,7 @@ psa_status_t psa_hash_finish( psa_hash_operation_t *operation, /* Fill the output buffer with something that isn't a valid hash * (barring an attack on the hash and deliberately-crafted input), * in case the caller doesn't check the return status properly. */ - *hash_length = actual_hash_length; + *hash_length = hash_size; /* If hash_size is 0 then hash may be NULL and then the * call to memset would have undefined behavior. */ if( hash_size != 0 ) @@ -1068,6 +1068,7 @@ psa_status_t psa_hash_finish( psa_hash_operation_t *operation, if( ret == 0 ) { + *hash_length = actual_hash_length; return( psa_hash_abort( operation ) ); } else @@ -1517,7 +1518,7 @@ static psa_status_t psa_mac_finish_internal( psa_mac_operation_t *operation, /* Fill the output buffer with something that isn't a valid mac * (barring an attack on the mac and deliberately-crafted input), * in case the caller doesn't check the return status properly. */ - *mac_length = operation->mac_size; + *mac_length = mac_size; /* If mac_size is 0 then mac may be NULL and then the * call to memset would have undefined behavior. */ if( mac_size != 0 ) @@ -1583,6 +1584,7 @@ cleanup: if( ret == 0 && status == PSA_SUCCESS ) { + *mac_length = operation->mac_size; return( psa_mac_abort( operation ) ); } else From 1d96fff61a26090ea6f4c0ad4ee078b39a912aef Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 2 Jul 2018 12:15:39 +0200 Subject: [PATCH 6/6] In psa_mac_finish, write a safe output even in the BAD_STATE case --- library/psa_crypto.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index a2f68975b..ca461c20e 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -1510,10 +1510,6 @@ static psa_status_t psa_mac_finish_internal( psa_mac_operation_t *operation, { int ret = 0; psa_status_t status = PSA_SUCCESS; - if( ! operation->key_set ) - return( PSA_ERROR_BAD_STATE ); - if( operation->iv_required && ! operation->iv_set ) - return( PSA_ERROR_BAD_STATE ); /* Fill the output buffer with something that isn't a valid mac * (barring an attack on the mac and deliberately-crafted input), @@ -1524,6 +1520,11 @@ static psa_status_t psa_mac_finish_internal( psa_mac_operation_t *operation, if( mac_size != 0 ) memset( mac, '!', mac_size ); + if( ! operation->key_set ) + return( PSA_ERROR_BAD_STATE ); + if( operation->iv_required && ! operation->iv_set ) + return( PSA_ERROR_BAD_STATE ); + if( mac_size < operation->mac_size ) return( PSA_ERROR_BUFFER_TOO_SMALL );