From 16c0f4f787e7d4f52072b4ad211fdc6e04c2c98e Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 20 Jun 2018 16:05:20 +0200 Subject: [PATCH] Fix potential memory corruption on MAC/cipher setup failure When psa_mac_start(), psa_encrypt_setup() or psa_cipher_setup() failed, depending on when the failure happened, it was possible that psa_mac_abort() or psa_cipher_abort() would crash because it would try to call a free() function uninitialized data in the operation structure. Refactor the functions so that they initialize the operation structure before doing anything else. Add non-regression tests and a few more positive and negative unit tests for psa_mac_start() and psa_cipher_setup() (the latter via psa_encrypt_setip()). --- library/psa_crypto.c | 94 +++++++++++++++------ tests/suites/test_suite_psa_crypto.data | 47 +++++++++++ tests/suites/test_suite_psa_crypto.function | 85 +++++++++++++++++++ 3 files changed, 202 insertions(+), 24 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index d75226cc6..535384c42 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -1133,10 +1133,53 @@ static size_t psa_get_hash_block_size( psa_algorithm_t alg ) } } +/* Initialize the MAC operation structure. Once this function has been + * called, psa_mac_abort can run and will do the right thing. */ +static psa_status_t psa_mac_init( psa_mac_operation_t *operation, + psa_algorithm_t alg ) +{ + psa_status_t status = PSA_ERROR_NOT_SUPPORTED; + + operation->alg = alg; + operation->key_set = 0; + operation->iv_set = 0; + operation->iv_required = 0; + operation->has_input = 0; + operation->key_usage_sign = 0; + operation->key_usage_verify = 0; + +#if defined(MBEDTLS_CMAC_C) + if( alg == PSA_ALG_CMAC ) + { + operation->iv_required = 0; + mbedtls_cipher_init( &operation->ctx.cmac ); + status = PSA_SUCCESS; + } + else +#endif /* MBEDTLS_CMAC_C */ +#if defined(MBEDTLS_MD_C) + if( PSA_ALG_IS_HMAC( operation->alg ) ) + { + status = psa_hash_start( &operation->ctx.hmac.hash_ctx, + PSA_ALG_HMAC_HASH( alg ) ); + } + else +#endif /* MBEDTLS_MD_C */ + { + /* fall through with NOT_SUPPORTED */ + } + + if( status != PSA_SUCCESS ) + memset( operation, 0, sizeof( *operation ) ); + return( status ); +} + psa_status_t psa_mac_abort( psa_mac_operation_t *operation ) { switch( operation->alg ) { + case 0: + return( PSA_SUCCESS ); #if defined(MBEDTLS_CMAC_C) case PSA_ALG_CMAC: mbedtls_cipher_free( &operation->ctx.cmac ); @@ -1165,6 +1208,8 @@ psa_status_t psa_mac_abort( psa_mac_operation_t *operation ) operation->iv_set = 0; operation->iv_required = 0; operation->has_input = 0; + operation->key_usage_sign = 0; + operation->key_usage_verify = 0; return( PSA_SUCCESS ); } @@ -1178,8 +1223,6 @@ static int psa_cmac_start( psa_mac_operation_t *operation, int ret; operation->mac_size = cipher_info->block_size; - operation->iv_required = 0; - mbedtls_cipher_init( &operation->ctx.cmac ); ret = mbedtls_cipher_setup( &operation->ctx.cmac, cipher_info ); if( ret != 0 ) @@ -1213,14 +1256,9 @@ static int psa_hmac_start( psa_mac_operation_t *operation, if( key_type != PSA_KEY_TYPE_HMAC ) return( PSA_ERROR_INVALID_ARGUMENT ); - operation->iv_required = 0; operation->mac_size = digest_size; - status = psa_hash_start( &operation->ctx.hmac.hash_ctx, - PSA_ALG_HMAC_HASH( alg ) ); - if( status != PSA_SUCCESS ) - return( status ); - + /* The hash was started earlier in psa_mac_init. */ if( key_length > block_size ) { status = psa_hash_update( &operation->ctx.hmac.hash_ctx, @@ -1274,13 +1312,9 @@ psa_status_t psa_mac_start( psa_mac_operation_t *operation, size_t key_bits; const mbedtls_cipher_info_t *cipher_info = NULL; - operation->alg = 0; - operation->key_set = 0; - operation->iv_set = 0; - operation->iv_required = 1; - operation->has_input = 0; - operation->key_usage_sign = 0; - operation->key_usage_verify = 0; + status = psa_mac_init( operation, alg ); + if( status != PSA_SUCCESS ) + return( status ); status = psa_get_key_information( key, &key_type, &key_bits ); if( status != PSA_SUCCESS ) @@ -1332,7 +1366,6 @@ psa_status_t psa_mac_start( psa_mac_operation_t *operation, else { - operation->alg = alg; operation->key_set = 1; } return( status ); @@ -1872,6 +1905,21 @@ psa_status_t psa_asymmetric_decrypt( psa_key_slot_t key, /* Symmetric cryptography */ /****************************************************************/ +/* Initialize the cipher operation structure. Once this function has been + * called, psa_cipher_abort can run and will do the right thing. */ +static psa_status_t psa_cipher_init( psa_cipher_operation_t *operation, + psa_algorithm_t alg ) +{ + operation->alg = alg; + operation->key_set = 0; + operation->iv_set = 0; + operation->iv_required = 1; + operation->iv_size = 0; + operation->block_size = 0; + mbedtls_cipher_init( &operation->ctx.cipher ); + return( PSA_SUCCESS ); +} + static psa_status_t psa_cipher_setup( psa_cipher_operation_t *operation, psa_key_slot_t key, psa_algorithm_t alg, @@ -1884,12 +1932,9 @@ static psa_status_t psa_cipher_setup( psa_cipher_operation_t *operation, size_t key_bits; const mbedtls_cipher_info_t *cipher_info = NULL; - operation->alg = alg; - operation->key_set = 0; - operation->iv_set = 0; - operation->iv_required = 1; - operation->iv_size = 0; - operation->block_size = 0; + status = psa_cipher_init( operation, alg ); + if( status != PSA_SUCCESS ) + return( status ); status = psa_get_key_information( key, &key_type, &key_bits ); if( status != PSA_SUCCESS ) @@ -1900,7 +1945,6 @@ static psa_status_t psa_cipher_setup( psa_cipher_operation_t *operation, if( cipher_info == NULL ) return( PSA_ERROR_NOT_SUPPORTED ); - mbedtls_cipher_init( &operation->ctx.cipher ); ret = mbedtls_cipher_setup( &operation->ctx.cipher, cipher_info ); if( ret != 0 ) { @@ -1944,7 +1988,6 @@ static psa_status_t psa_cipher_setup( psa_cipher_operation_t *operation, #endif //MBEDTLS_CIPHER_MODE_WITH_PADDING operation->key_set = 1; - operation->alg = alg; operation->block_size = ( PSA_ALG_IS_BLOCK_CIPHER( alg ) ? PSA_BLOCK_CIPHER_BLOCK_SIZE( key_type ) : 1 ); @@ -2119,6 +2162,9 @@ psa_status_t psa_cipher_finish( psa_cipher_operation_t *operation, psa_status_t psa_cipher_abort( psa_cipher_operation_t *operation ) { + if( operation->alg == 0 ) + return( PSA_SUCCESS ); + mbedtls_cipher_free( &operation->ctx.cipher ); operation->alg = 0; diff --git a/tests/suites/test_suite_psa_crypto.data b/tests/suites/test_suite_psa_crypto.data index 9902a0ecb..552faf9c4 100644 --- a/tests/suites/test_suite_psa_crypto.data +++ b/tests/suites/test_suite_psa_crypto.data @@ -106,6 +106,14 @@ key_lifetime_set_fail:1:PSA_KEY_LIFETIME_WRITE_ONCE:PSA_ERROR_NOT_SUPPORTED PSA key lifetime set: invalid key lifetime value key_lifetime_set_fail:1:PSA_KEY_LIFETIME_PERSISTENT+1:PSA_ERROR_INVALID_ARGUMENT +PSA hash setup: good, SHA-256 +depends_on:MBEDTLS_SHA256_C +hash_setup:PSA_ALG_SHA_256:PSA_SUCCESS + +PSA hash setup: bad (unknown hash algorithm) +depends_on:MBEDTLS_MD_C:MBEDTLS_SHA256_C +hash_setup:0x80000000 | PSA_ALG_SHA_256:PSA_ERROR_NOT_SUPPORTED + PSA hash finish: SHA-256 depends_on:MBEDTLS_SHA256_C hash_finish:PSA_ALG_SHA_256:"bd":"68325720aabd7c82f30f554b313d0570c95accbb7dc4b5aae11204c08ffe732b" @@ -114,6 +122,27 @@ PSA hash verify: SHA-256 depends_on:MBEDTLS_SHA256_C hash_verify:PSA_ALG_SHA_256:"bd":"68325720aabd7c82f30f554b313d0570c95accbb7dc4b5aae11204c08ffe732b" +PSA MAC setup: good, HMAC-SHA-256 +depends_on:MBEDTLS_MD_C:MBEDTLS_SHA256_C +mac_setup:PSA_KEY_TYPE_HMAC:"000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f202122232425262728292a2b2c2d2e2f303132333435363738393a3b3c3d3e3f":PSA_ALG_HMAC(PSA_ALG_SHA_256):PSA_SUCCESS + +PSA MAC setup: good, AES-CMAC +depends_on:MBEDTLS_AES_C:MBEDTLS_CMAC_C +mac_setup:PSA_KEY_TYPE_AES:"000102030405060708090a0b0c0d0e0f":PSA_ALG_CMAC:PSA_SUCCESS + +PSA MAC setup: bad algorithm (unknown MAC algorithm) +depends_on:MBEDTLS_MD_C +mac_setup:PSA_KEY_TYPE_RAW_DATA:"000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f202122232425262728292a2b2c2d2e2f303132333435363738393a3b3c3d3e3f":PSA_ALG_HMAC(0):PSA_ERROR_NOT_SUPPORTED + +PSA MAC setup: invalid key type, HMAC-SHA-256 +depends_on:MBEDTLS_MD_C:MBEDTLS_SHA256_C +mac_setup:PSA_KEY_TYPE_RAW_DATA:"000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f202122232425262728292a2b2c2d2e2f303132333435363738393a3b3c3d3e3f":PSA_ALG_HMAC(PSA_ALG_SHA_256):PSA_ERROR_INVALID_ARGUMENT + +PSA MAC setup: incompatible key DES for CMAC +depends_on:MBEDTLS_DES_C:MBEDTLS_CMAC_C +# Either INVALID_ARGUMENT or NOT_SUPPORTED would be reasonable here +mac_setup:PSA_KEY_TYPE_DES:"000102030405060708090a0b0c0d0e0f":PSA_ALG_CMAC:PSA_ERROR_NOT_SUPPORTED + PSA MAC verify: HMAC-SHA-256 depends_on:MBEDTLS_MD_C:MBEDTLS_SHA256_C mac_verify:PSA_KEY_TYPE_HMAC:"000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f202122232425262728292a2b2c2d2e2f303132333435363738393a3b3c3d3e3f":PSA_ALG_HMAC(PSA_ALG_SHA_256):"53616d706c65206d65737361676520666f72206b65796c656e3d626c6f636b6c656e":"8bb9a1db9806f20df7f77b82138c7914d174d59e13dc4d0169c9057b133e1d62" @@ -218,6 +247,24 @@ PSA MAC verify: CMAC-AES-128 depends_on:MBEDTLS_CMAC_C:MBEDTLS_AES_C mac_verify:PSA_KEY_TYPE_AES:"2b7e151628aed2a6abf7158809cf4f3c":PSA_ALG_CMAC:"6bc1bee22e409f96e93d7e117393172aae2d8a571e03ac9c9eb76fac45af8e5130c81c46a35ce411":"dfa66747de9ae63030ca32611497c827" +PSA cipher setup: good, AES-CTR +depends_on:MBEDTLS_AES_C:MBEDTLS_CIPHER_MODE_CTR +cipher_setup:PSA_KEY_TYPE_AES:"000102030405060708090a0b0c0d0e0f":PSA_ALG_CTR:PSA_SUCCESS + +PSA cipher setup: bad algorithm (unknown cipher algorithm) +depends_on:MBEDTLS_AES_C +cipher_setup:PSA_KEY_TYPE_AES:"000102030405060708090a0b0c0d0e0f":PSA_ALG_CATEGORY_CIPHER:PSA_ERROR_NOT_SUPPORTED + +PSA cipher setup: invalid key type, CTR +depends_on:MBEDTLS_CIPHER_MODE_CTR +# Either INVALID_ARGUMENT or NOT_SUPPORTED would be reasonable here +cipher_setup:PSA_KEY_TYPE_RAW_DATA:"000102030405060708090a0b0c0d0e0f":PSA_ALG_CTR:PSA_ERROR_NOT_SUPPORTED + +PSA cipher setup: incompatible key ARC4 for CTR +depends_on:MBEDTLS_ARC4_C:MBEDTLS_CIPHER_MODE_CTR +# Either INVALID_ARGUMENT or NOT_SUPPORTED would be reasonable here +cipher_setup:PSA_KEY_TYPE_ARC4:"000102030405060708090a0b0c0d0e0f":PSA_ALG_CTR:PSA_ERROR_NOT_SUPPORTED + PSA symmetric encrypt: AES-CBC-nopad, 16 bytes, good depends_on:MBEDTLS_AES_C:MBEDTLS_CIPHER_MODE_CBC cipher_encrypt:PSA_ALG_CBC_BASE | PSA_ALG_BLOCK_CIPHER_PAD_NONE:PSA_KEY_TYPE_AES:"2b7e151628aed2a6abf7158809cf4f3c":"6bc1bee22e409f96e93d7e117393172a":"a076ec9dfbe47d52afc357336f20743b":PSA_SUCCESS diff --git a/tests/suites/test_suite_psa_crypto.function b/tests/suites/test_suite_psa_crypto.function index 1cd9c22a0..ee781326e 100644 --- a/tests/suites/test_suite_psa_crypto.function +++ b/tests/suites/test_suite_psa_crypto.function @@ -374,6 +374,25 @@ exit: } /* END_CASE */ +/* BEGIN_CASE */ +void hash_setup( int alg_arg, + int expected_status_arg ) +{ + psa_algorithm_t alg = alg_arg; + psa_hash_operation_t operation; + psa_status_t status; + + TEST_ASSERT( psa_crypto_init( ) == PSA_SUCCESS ); + + status = psa_hash_start( &operation, alg ); + psa_hash_abort( &operation ); + TEST_ASSERT( status == (psa_status_t) expected_status_arg ); + +exit: + mbedtls_psa_crypto_free( ); +} +/* END_CASE */ + /* BEGIN_CASE */ void hash_finish( int alg_arg, data_t *input, data_t *expected_hash ) { @@ -430,6 +449,40 @@ exit: } /* END_CASE */ +/* BEGIN_CASE */ +void mac_setup( int key_type_arg, + data_t *key, + int alg_arg, + int expected_status_arg ) +{ + int key_slot = 1; + psa_key_type_t key_type = key_type_arg; + psa_algorithm_t alg = alg_arg; + psa_mac_operation_t operation; + psa_key_policy_t policy; + 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_SIGN | PSA_KEY_USAGE_VERIFY, + alg ); + TEST_ASSERT( psa_set_key_policy( key_slot, &policy ) == PSA_SUCCESS ); + + TEST_ASSERT( psa_import_key( key_slot, key_type, + key->x, key->len ) == PSA_SUCCESS ); + + status = psa_mac_start( &operation, key_slot, alg ); + psa_mac_abort( &operation ); + TEST_ASSERT( status == (psa_status_t) expected_status_arg ); + +exit: + psa_destroy_key( key_slot ); + mbedtls_psa_crypto_free( ); +} +/* END_CASE */ + /* BEGIN_CASE */ void mac_verify( int key_type_arg, data_t *key, @@ -473,6 +526,38 @@ exit: } /* END_CASE */ +/* BEGIN_CASE */ +void cipher_setup( int key_type_arg, + data_t *key, + int alg_arg, + int expected_status_arg ) +{ + int key_slot = 1; + psa_key_type_t key_type = key_type_arg; + psa_algorithm_t alg = alg_arg; + psa_cipher_operation_t operation; + psa_key_policy_t policy; + 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_ENCRYPT, alg ); + TEST_ASSERT( psa_set_key_policy( key_slot, &policy ) == PSA_SUCCESS ); + + TEST_ASSERT( psa_import_key( key_slot, key_type, + key->x, key->len ) == PSA_SUCCESS ); + + status = psa_encrypt_setup( &operation, key_slot, alg ); + psa_cipher_abort( &operation ); + TEST_ASSERT( status == (psa_status_t) expected_status_arg ); + +exit: + psa_destroy_key( key_slot ); + mbedtls_psa_crypto_free( ); +} +/* END_CASE */ + /* BEGIN_CASE */ void cipher_encrypt( int alg_arg, int key_type_arg, data_t *key,