From 25c4fa8fb0537fbb9d5a4af42d1b4643e9ee9aca Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Fri, 6 Jul 2018 16:23:25 +0100 Subject: [PATCH 1/3] Fix copy paste error PSA test suite At this point it fixes memory leaks as well. These memory leaks are the fault of the 'psa_cipher_finish()' function and the calls fixed in this commit (among with many others in the test suite) will become obsolete after fixing 'psa_cipher_finish()'. --- tests/suites/test_suite_psa_crypto.function | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/suites/test_suite_psa_crypto.function b/tests/suites/test_suite_psa_crypto.function index c90447f81..715236896 100644 --- a/tests/suites/test_suite_psa_crypto.function +++ b/tests/suites/test_suite_psa_crypto.function @@ -1385,7 +1385,7 @@ void cipher_verify_output( int alg_arg, int key_type_arg, output2_length += function_output_length; - TEST_ASSERT( psa_cipher_abort( &operation1 ) == PSA_SUCCESS ); + TEST_ASSERT( psa_cipher_abort( &operation2 ) == PSA_SUCCESS ); TEST_ASSERT( input->len == output2_length ); TEST_ASSERT( memcmp( input->x, output2, input->len ) == 0 ); @@ -1491,7 +1491,7 @@ void cipher_verify_output_multipart( int alg_arg, &function_output_length ) == PSA_SUCCESS ); output2_length += function_output_length; - TEST_ASSERT( psa_cipher_abort( &operation1 ) == PSA_SUCCESS ); + TEST_ASSERT( psa_cipher_abort( &operation2 ) == PSA_SUCCESS ); TEST_ASSERT( input->len == output2_length ); TEST_ASSERT( memcmp( input->x, output2, input->len ) == 0 ); From 315b51c22d8356db9bd550a4b71239b19faa7420 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 9 Jul 2018 16:04:51 +0100 Subject: [PATCH 2/3] Fix memory leak in psa_cipher_finish() --- library/psa_crypto.c | 49 ++++++++++++++++++++++++++++---------------- 1 file changed, 31 insertions(+), 18 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 6dff2f532..e10ca30ed 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -2410,18 +2410,19 @@ psa_status_t psa_cipher_finish( psa_cipher_operation_t *operation, size_t output_size, size_t *output_length ) { - int ret = MBEDTLS_ERR_CIPHER_FEATURE_UNAVAILABLE; + psa_status_t status = PSA_ERROR_UNKNOWN_ERROR; + int cipher_ret = MBEDTLS_ERR_CIPHER_FEATURE_UNAVAILABLE; uint8_t temp_output_buffer[MBEDTLS_MAX_BLOCK_LENGTH]; if( ! operation->key_set ) { - psa_cipher_abort( operation ); - return( PSA_ERROR_BAD_STATE ); + status = PSA_ERROR_BAD_STATE; + goto error; } if( operation->iv_required && ! operation->iv_set ) { - psa_cipher_abort( operation ); - return( PSA_ERROR_BAD_STATE ); + status = PSA_ERROR_BAD_STATE; + goto error; } if( operation->ctx.cipher.operation == MBEDTLS_ENCRYPT && PSA_ALG_IS_BLOCK_CIPHER( operation->alg ) ) @@ -2430,37 +2431,49 @@ psa_status_t psa_cipher_finish( psa_cipher_operation_t *operation, operation->alg & PSA_ALG_BLOCK_CIPHER_PADDING_MASK; if( operation->ctx.cipher.unprocessed_len >= operation->block_size ) { - psa_cipher_abort( operation ); - return( PSA_ERROR_TAMPERING_DETECTED ); + status = PSA_ERROR_TAMPERING_DETECTED; + goto error; } if( padding_mode == PSA_ALG_BLOCK_CIPHER_PAD_NONE ) { if( operation->ctx.cipher.unprocessed_len != 0 ) { - psa_cipher_abort( operation ); - return( PSA_ERROR_INVALID_ARGUMENT ); + status = PSA_ERROR_INVALID_ARGUMENT; + goto error; } } } - ret = mbedtls_cipher_finish( &operation->ctx.cipher, temp_output_buffer, - output_length ); - if( ret != 0 ) + cipher_ret = mbedtls_cipher_finish( &operation->ctx.cipher, + temp_output_buffer, + output_length ); + if( cipher_ret != 0 ) { - psa_cipher_abort( operation ); - return( mbedtls_to_psa_error( ret ) ); + status = mbedtls_to_psa_error( cipher_ret ); + goto error; } + if( *output_length == 0 ) - /* Nothing to copy. Note that output may be NULL in this case. */ ; + ; /* 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 { - psa_cipher_abort( operation ); - return( PSA_ERROR_BUFFER_TOO_SMALL ); + status = PSA_ERROR_BUFFER_TOO_SMALL; + goto error; } - return( PSA_SUCCESS ); + status = psa_cipher_abort( operation ); + + return( status ); + +error: + + *output_length = 0; + + (void) psa_cipher_abort( operation ); + + return( status ); } psa_status_t psa_cipher_abort( psa_cipher_operation_t *operation ) From 279ab8e69be6c05105f5b422507886dd112733fd Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 9 Jul 2018 16:13:21 +0100 Subject: [PATCH 3/3] Prevent leaking plaintext in psa_cipher_finish() --- library/psa_crypto.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index e10ca30ed..e5833ce22 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -2463,6 +2463,7 @@ psa_status_t psa_cipher_finish( psa_cipher_operation_t *operation, goto error; } + mbedtls_zeroize( temp_output_buffer, sizeof( temp_output_buffer ) ); status = psa_cipher_abort( operation ); return( status ); @@ -2471,6 +2472,7 @@ error: *output_length = 0; + mbedtls_zeroize( temp_output_buffer, sizeof( temp_output_buffer ) ); (void) psa_cipher_abort( operation ); return( status );