From f24c7f80a07d37de80d277ea326f12df2bf510ae Mon Sep 17 00:00:00 2001 From: Jaeden Amero Date: Wed, 27 Jun 2018 17:20:43 +0100 Subject: [PATCH 1/3] psa_export_key: Always set a valid data length Make psa_export_key() always set a valid data_length when exporting, even when there are errors. This makes the API easier to use for buggy programs (like our test code). Our test code previously used exported_length uninitialized when checking to see that the buffer returned was all zero in import_export() in the case where an error was returned from psa_export_key(). Initialize exported_length to an invalid length, and check that it gets set properly by psa_export_key(), to avoid this using export_length uninitialized. Note that the mem_is_zero() check is still valid when psa_export_key() returns an error, e.g. where exported_length is 0, as we want to check that nothing was written to the buffer on error. Out test code also previous passed NULL for the data_length parameter of psa_export_key() when it expected a failure (in key_policy_fail()). However, data_length is not allowed to be NULL, especially now that we write to data_length from psa_export_key() even when there are errors. Update the test code to not pass in a NULL data_length. --- library/psa_crypto.c | 6 ++++++ tests/suites/test_suite_psa_crypto.function | 15 +++++++++++++-- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 2670e4139..de2bf40a9 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -636,6 +636,12 @@ static psa_status_t psa_internal_export_key( psa_key_slot_t key, { key_slot_t *slot; + /* Set the key to empty now, so that even when there are errors, we always + * set data_length to a value between 0 and data_size. On error, setting + * the key to empty is a good choice because an empty key representation is + * unlikely to be accepted anywhere. */ + *data_length = 0; + if( key == 0 || key > PSA_KEY_SLOT_COUNT ) return( PSA_ERROR_EMPTY_SLOT ); slot = &global_data.key_slots[key]; diff --git a/tests/suites/test_suite_psa_crypto.function b/tests/suites/test_suite_psa_crypto.function index 2d279fc38..c67725d70 100644 --- a/tests/suites/test_suite_psa_crypto.function +++ b/tests/suites/test_suite_psa_crypto.function @@ -8,6 +8,9 @@ #define PSA_CRYPTO_TEST_SIZE_T_RANGE( x ) 1 #endif +/** An invalid export length that will never be set by psa_export_key(). */ +static const size_t INVALID_EXPORT_LENGTH = ~0U; + /** Test if a buffer is not all-bits zero. * * \param buffer Pointer to the beginning of the buffer. @@ -320,7 +323,7 @@ void import_export( data_t *data, unsigned char *exported = NULL; unsigned char *reexported = NULL; size_t export_size; - size_t exported_length; + size_t exported_length = INVALID_EXPORT_LENGTH; size_t reexported_length; psa_key_type_t got_type; size_t got_bits; @@ -358,6 +361,13 @@ void import_export( data_t *data, exported, export_size, &exported_length ); TEST_ASSERT( status == expected_export_status ); + + /* The exported length must be set by psa_export_key() to a value between 0 + * and export_size. On errors, the exported length must be 0. */ + TEST_ASSERT( exported_length != INVALID_EXPORT_LENGTH ); + TEST_ASSERT( status == PSA_SUCCESS || exported_length == 0 ); + TEST_ASSERT( exported_length <= export_size ); + TEST_ASSERT( mem_is_zero( exported + exported_length, export_size - exported_length ) ); if( status != PSA_SUCCESS ) @@ -536,13 +546,14 @@ void key_policy_fail( int usage_arg, int alg_arg, int expected_status, if( usage & PSA_KEY_USAGE_SIGN ) { + size_t data_length; TEST_ASSERT( keypair != NULL ); TEST_ASSERT( PSA_CRYPTO_TEST_SIZE_T_RANGE( keypair->len ) ); TEST_ASSERT( psa_import_key( key_slot, PSA_KEY_TYPE_RSA_KEYPAIR, keypair->x, keypair->len ) == PSA_SUCCESS ); - actual_status = psa_export_key( key_slot, NULL, 0, NULL ); + actual_status = psa_export_key( key_slot, NULL, 0, &data_length ); } TEST_ASSERT( actual_status == expected_status ); From e7edf7bb20f9295f0e26cc09601d524aaf0377b0 Mon Sep 17 00:00:00 2001 From: Jaeden Amero Date: Wed, 27 Jun 2018 17:55:12 +0100 Subject: [PATCH 2/3] psa: Expect zero-length exported-public symmetric keys Because exporting-public a symmetric key fails, we have no reasonable expectation that the exported key length has any value at all other than something obviously incorrect or "empty", like a key with a length of 0. Our current implementation explicitly sets the exported key length to 0 on errors, so test for this. Fix the "PSA import/export-public: cannot export-public a symmetric key" test to expect a key length of 0 instead of 162. --- tests/suites/test_suite_psa_crypto.data | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/suites/test_suite_psa_crypto.data b/tests/suites/test_suite_psa_crypto.data index 265a6d5be..612a15220 100644 --- a/tests/suites/test_suite_psa_crypto.data +++ b/tests/suites/test_suite_psa_crypto.data @@ -103,7 +103,7 @@ import_export_public_key:"3082025e02010002818100af057d396ee84fb75fdbb5c2b13c7fe5 PSA import/export-public: cannot export-public a symmetric key depends_on:MBEDTLS_PK_C:MBEDTLS_PK_PARSE_C:MBEDTLS_RSA_C -import_export_public_key:"2b7e151628aed2a6abf7158809cf4f3c":PSA_KEY_TYPE_AES:PSA_ALG_CBC_BASE | PSA_ALG_BLOCK_CIPHER_PAD_NONE:128:162:PSA_ERROR_INVALID_ARGUMENT +import_export_public_key:"2b7e151628aed2a6abf7158809cf4f3c":PSA_KEY_TYPE_AES:PSA_ALG_CBC_BASE | PSA_ALG_BLOCK_CIPHER_PAD_NONE:128:0:PSA_ERROR_INVALID_ARGUMENT PSA import/export EC secp256r1: good depends_on:MBEDTLS_PK_C:MBEDTLS_PK_PARSE_C:MBEDTLS_ECP_C:MBEDTLS_ECP_DP_SECP256R1_ENABLED From 2a671e9031a1a760d4d4cfb8fcc6c469052ec7da Mon Sep 17 00:00:00 2001 From: Jaeden Amero Date: Wed, 27 Jun 2018 17:47:40 +0100 Subject: [PATCH 3/3] psa: export_public_key: Check for all zero on error --- tests/suites/test_suite_psa_crypto.function | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/suites/test_suite_psa_crypto.function b/tests/suites/test_suite_psa_crypto.function index c67725d70..0d056db2a 100644 --- a/tests/suites/test_suite_psa_crypto.function +++ b/tests/suites/test_suite_psa_crypto.function @@ -425,7 +425,7 @@ void import_export_public_key( data_t *data, psa_status_t status; unsigned char *exported = NULL; size_t export_size; - size_t exported_length; + size_t exported_length = INVALID_EXPORT_LENGTH; psa_key_type_t got_type; size_t got_bits; psa_key_policy_t policy; @@ -458,11 +458,12 @@ void import_export_public_key( data_t *data, exported, export_size, &exported_length ); TEST_ASSERT( status == expected_export_status ); + TEST_ASSERT( exported_length == (size_t) public_key_expected_length ); + TEST_ASSERT( mem_is_zero( exported + exported_length, + export_size - exported_length ) ); if( status != PSA_SUCCESS ) goto destroy; - TEST_ASSERT( exported_length == (size_t) public_key_expected_length ); - destroy: /* Destroy the key */ TEST_ASSERT( psa_destroy_key( slot ) == PSA_SUCCESS );