From 996deb18cca397d5be2728bde8fd27e57ea11052 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 1 Aug 2018 15:45:45 +0200 Subject: [PATCH 1/2] Fix buffer overflow in the slot array Slots are numbered from 1, but the slot array is a C array so it's numbered from 0. Add a non-regression test. --- library/psa_crypto.c | 9 +++- tests/suites/test_suite_psa_crypto.data | 3 ++ tests/suites/test_suite_psa_crypto.function | 59 +++++++++++++++++++++ 3 files changed, 69 insertions(+), 2 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 210fa5ff4..fe3072935 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -82,6 +82,8 @@ +#define ARRAY_LENGTH( array ) ( sizeof( array ) / sizeof( *( array ) ) ) + /* Implementation that should never be optimized out by the compiler */ static void mbedtls_zeroize( void *v, size_t n ) { @@ -343,10 +345,13 @@ static psa_status_t mbedtls_to_psa_error( int ret ) static psa_status_t psa_get_key_slot( psa_key_slot_t key, key_slot_t **p_slot ) { - if( key == 0 || key > PSA_KEY_SLOT_COUNT ) + /* 0 is not a valid slot number under any circumstance. This + * implementation provides slots number 1 to N where N is the + * number of available slots. */ + if( key == 0 || key > ARRAY_LENGTH( global_data.key_slots ) ) return( PSA_ERROR_INVALID_ARGUMENT ); - *p_slot = &global_data.key_slots[key]; + *p_slot = &global_data.key_slots[key - 1]; return( PSA_SUCCESS ); } diff --git a/tests/suites/test_suite_psa_crypto.data b/tests/suites/test_suite_psa_crypto.data index 894317e32..0f7c8a97f 100644 --- a/tests/suites/test_suite_psa_crypto.data +++ b/tests/suites/test_suite_psa_crypto.data @@ -1,6 +1,9 @@ PSA init/deinit init_deinit: +PSA fill 250 slots +fill_slots:250 + PSA import/export raw: 0 bytes import_export:"":PSA_KEY_TYPE_RAW_DATA:PSA_ALG_CBC_BASE | PSA_ALG_BLOCK_CIPHER_PAD_NONE:PSA_KEY_USAGE_EXPORT:0:0:PSA_SUCCESS:1 diff --git a/tests/suites/test_suite_psa_crypto.function b/tests/suites/test_suite_psa_crypto.function index 2e0804bf5..88ef27fbb 100644 --- a/tests/suites/test_suite_psa_crypto.function +++ b/tests/suites/test_suite_psa_crypto.function @@ -447,6 +447,65 @@ void init_deinit( ) } /* END_CASE */ +/* BEGIN_CASE */ +void fill_slots( int max_arg ) +{ + /* Fill all the slots until we run out of memory or out of slots, + * or until some limit specified in the test data for the sake of + * implementations with an essentially unlimited number of slots. + * This test assumes that available slots are numbered from 1. */ + + psa_key_slot_t slot; + psa_key_slot_t max = 0; + psa_key_policy_t policy; + uint8_t exported[sizeof( max )]; + size_t exported_size; + psa_status_t status; + + TEST_ASSERT( psa_crypto_init( ) == PSA_SUCCESS ); + + psa_key_policy_init( &policy ); + psa_key_policy_set_usage( &policy, PSA_KEY_USAGE_EXPORT, 0 ); + + for( max = 1; max <= (size_t) max_arg; max++ ) + { + status = psa_set_key_policy( max, &policy ); + /* Stop filling slots if we run out of memory or out of + * available slots. */ + TEST_ASSERT( status == PSA_SUCCESS || + status == PSA_ERROR_INSUFFICIENT_MEMORY || + status == PSA_ERROR_INVALID_ARGUMENT ); + if( status != PSA_SUCCESS ) + break; + status = psa_import_key( max, PSA_KEY_TYPE_RAW_DATA, + (uint8_t*) &max, sizeof( max ) ); + /* Since psa_set_key_policy succeeded, we know that the slot + * number is valid. But we may legitimately run out of memory. */ + TEST_ASSERT( status == PSA_SUCCESS || + status == PSA_ERROR_INSUFFICIENT_MEMORY ); + if( status != PSA_SUCCESS ) + break; + } + /* `max` is now the first slot number that wasn't filled. */ + max -= 1; + + for( slot = 1; slot <= max; slot++ ) + { + TEST_ASSERT( psa_export_key( slot, + exported, sizeof( exported ), + &exported_size ) == PSA_SUCCESS ); + TEST_ASSERT( exported_size == sizeof( slot ) ); + TEST_ASSERT( memcmp( exported, &slot, sizeof( slot ) ) == 0 ); + TEST_ASSERT( psa_destroy_key( slot ) == PSA_SUCCESS ); + } + +exit: + for( slot = 1; slot <= max; slot++ ) + psa_destroy_key( slot ); + mbedtls_psa_crypto_free( ); +} +/* END_CASE */ + /* BEGIN_CASE */ void import( data_t *data, int type, int expected_status_arg ) { From 9a05634558ccd2f8cbb8cf62a22555a7085f4d42 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 1 Aug 2018 15:46:54 +0200 Subject: [PATCH 2/2] psa_crypto_free: destroy the last slot The last slot in the array was not freed due to an off-by-one error. Amend the fill_slots test to serve as a non-regression test for this issue: without this bug fix, it would cause a memory leak. --- library/psa_crypto.c | 2 +- tests/suites/test_suite_psa_crypto.function | 4 +--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index fe3072935..8b25dac1a 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -3453,7 +3453,7 @@ psa_status_t psa_generate_key( psa_key_slot_t key, void mbedtls_psa_crypto_free( void ) { psa_key_slot_t key; - for( key = 1; key < PSA_KEY_SLOT_COUNT; key++ ) + for( key = 1; key <= PSA_KEY_SLOT_COUNT; key++ ) psa_destroy_key( key ); mbedtls_ctr_drbg_free( &global_data.ctr_drbg ); mbedtls_entropy_free( &global_data.entropy ); diff --git a/tests/suites/test_suite_psa_crypto.function b/tests/suites/test_suite_psa_crypto.function index 88ef27fbb..43e479470 100644 --- a/tests/suites/test_suite_psa_crypto.function +++ b/tests/suites/test_suite_psa_crypto.function @@ -496,12 +496,10 @@ void fill_slots( int max_arg ) &exported_size ) == PSA_SUCCESS ); TEST_ASSERT( exported_size == sizeof( slot ) ); TEST_ASSERT( memcmp( exported, &slot, sizeof( slot ) ) == 0 ); - TEST_ASSERT( psa_destroy_key( slot ) == PSA_SUCCESS ); } exit: - for( slot = 1; slot <= max; slot++ ) - psa_destroy_key( slot ); + /* Do not destroy the keys. mbedtls_psa_crypto_free() should do it. */ mbedtls_psa_crypto_free( ); } /* END_CASE */