From 46f1fd7afd4f17062883ebd6c0dbf915602212f5 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 28 Jun 2018 19:31:31 +0200 Subject: [PATCH] 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;