From 71fd80d279ab46934534c8e8f4a76e4f1b2e9ea3 Mon Sep 17 00:00:00 2001 From: Steven Cooreman Date: Tue, 7 Jul 2020 21:12:27 +0200 Subject: [PATCH 01/16] Re-define members of psa_key_slot_t In preparation for the implementation of the accelerator APIs. This is ramping up to the goal of only storing the export representation in the key slot, and not keeping the crypto implementation-specific representations around. Signed-off-by: Steven Cooreman --- library/psa_crypto.c | 60 +++++++++++++++++++-------------------- library/psa_crypto_core.h | 7 +++-- 2 files changed, 34 insertions(+), 33 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 79bc9c9db..54980730c 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -443,7 +443,7 @@ mbedtls_ecp_group_id mbedtls_ecc_group_of_psa( psa_ecc_family_t curve, static psa_status_t prepare_raw_data_slot( psa_key_type_t type, size_t bits, - struct raw_data *raw ) + struct key_data *key ) { /* Check that the bit size is acceptable for the key type */ switch( type ) @@ -491,11 +491,11 @@ static psa_status_t prepare_raw_data_slot( psa_key_type_t type, return( PSA_ERROR_INVALID_ARGUMENT ); /* Allocate memory for the key */ - raw->bytes = PSA_BITS_TO_BYTES( bits ); - raw->data = mbedtls_calloc( 1, raw->bytes ); - if( raw->data == NULL ) + key->bytes = PSA_BITS_TO_BYTES( bits ); + key->data = mbedtls_calloc( 1, key->bytes ); + if( key->data == NULL ) { - raw->bytes = 0; + key->bytes = 0; return( PSA_ERROR_INSUFFICIENT_MEMORY ); } return( PSA_SUCCESS ); @@ -716,7 +716,7 @@ static psa_key_bits_t psa_calculate_key_bits( const psa_key_slot_t *slot ) size_t bits = 0; /* return 0 on an empty slot */ if( key_type_is_raw_bytes( slot->attr.type ) ) - bits = PSA_BYTES_TO_BITS( slot->data.raw.bytes ); + bits = PSA_BYTES_TO_BITS( slot->data.key.bytes ); #if defined(MBEDTLS_RSA_C) else if( PSA_KEY_TYPE_IS_RSA( slot->attr.type ) ) bits = PSA_BYTES_TO_BITS( mbedtls_rsa_get_len( slot->data.rsa ) ); @@ -751,11 +751,11 @@ psa_status_t psa_import_key_into_slot( psa_key_slot_t *slot, if( bit_size > PSA_MAX_KEY_BITS ) return( PSA_ERROR_NOT_SUPPORTED ); status = prepare_raw_data_slot( slot->attr.type, bit_size, - &slot->data.raw ); + &slot->data.key ); if( status != PSA_SUCCESS ) return( status ); if( data_length != 0 ) - memcpy( slot->data.raw.data, data, data_length ); + memcpy( slot->data.key.data, data, data_length ); } else #if defined(MBEDTLS_ECP_C) @@ -963,7 +963,7 @@ static psa_status_t psa_remove_key_data_from_memory( psa_key_slot_t *slot ) } else if( key_type_is_raw_bytes( slot->attr.type ) ) { - mbedtls_free( slot->data.raw.data ); + mbedtls_free( slot->data.key.data ); } else #if defined(MBEDTLS_RSA_C) @@ -1306,12 +1306,12 @@ static psa_status_t psa_internal_export_key( const psa_key_slot_t *slot, if( key_type_is_raw_bytes( slot->attr.type ) ) { - if( slot->data.raw.bytes > data_size ) + if( slot->data.key.bytes > data_size ) return( PSA_ERROR_BUFFER_TOO_SMALL ); - memcpy( data, slot->data.raw.data, slot->data.raw.bytes ); - memset( data + slot->data.raw.bytes, 0, - data_size - slot->data.raw.bytes ); - *data_length = slot->data.raw.bytes; + memcpy( data, slot->data.key.data, slot->data.key.bytes ); + memset( data + slot->data.key.bytes, 0, + data_size - slot->data.key.bytes ); + *data_length = slot->data.key.bytes; return( PSA_SUCCESS ); } #if defined(MBEDTLS_ECP_C) @@ -2718,7 +2718,7 @@ static int psa_cmac_setup( psa_mac_operation_t *operation, return( ret ); ret = mbedtls_cipher_cmac_starts( &operation->ctx.cmac, - slot->data.raw.data, + slot->data.key.data, key_bits ); return( ret ); } @@ -2862,8 +2862,8 @@ static psa_status_t psa_mac_setup( psa_mac_operation_t *operation, } status = psa_hmac_setup_internal( &operation->ctx.hmac, - slot->data.raw.data, - slot->data.raw.bytes, + slot->data.key.data, + slot->data.key.bytes, hash_alg ); } else @@ -3795,8 +3795,8 @@ static psa_status_t psa_cipher_setup( psa_cipher_operation_t *operation, { /* Two-key Triple-DES is 3-key Triple-DES with K1=K3 */ uint8_t keys[24]; - memcpy( keys, slot->data.raw.data, 16 ); - memcpy( keys + 16, slot->data.raw.data, 8 ); + memcpy( keys, slot->data.key.data, 16 ); + memcpy( keys + 16, slot->data.key.data, 8 ); ret = mbedtls_cipher_setkey( &operation->ctx.cipher, keys, 192, cipher_operation ); @@ -3805,7 +3805,7 @@ static psa_status_t psa_cipher_setup( psa_cipher_operation_t *operation, #endif { ret = mbedtls_cipher_setkey( &operation->ctx.cipher, - slot->data.raw.data, + slot->data.key.data, (int) key_bits, cipher_operation ); } if( ret != 0 ) @@ -4137,7 +4137,7 @@ static psa_status_t psa_aead_setup( aead_operation_t *operation, mbedtls_ccm_init( &operation->ctx.ccm ); status = mbedtls_to_psa_error( mbedtls_ccm_setkey( &operation->ctx.ccm, cipher_id, - operation->slot->data.raw.data, + operation->slot->data.key.data, (unsigned int) key_bits ) ); if( status != 0 ) goto cleanup; @@ -4156,7 +4156,7 @@ static psa_status_t psa_aead_setup( aead_operation_t *operation, mbedtls_gcm_init( &operation->ctx.gcm ); status = mbedtls_to_psa_error( mbedtls_gcm_setkey( &operation->ctx.gcm, cipher_id, - operation->slot->data.raw.data, + operation->slot->data.key.data, (unsigned int) key_bits ) ); if( status != 0 ) goto cleanup; @@ -4173,7 +4173,7 @@ static psa_status_t psa_aead_setup( aead_operation_t *operation, mbedtls_chachapoly_init( &operation->ctx.chachapoly ); status = mbedtls_to_psa_error( mbedtls_chachapoly_setkey( &operation->ctx.chachapoly, - operation->slot->data.raw.data ) ); + operation->slot->data.key.data ) ); if( status != 0 ) goto cleanup; break; @@ -5246,8 +5246,8 @@ psa_status_t psa_key_derivation_input_key( return( psa_key_derivation_input_internal( operation, step, slot->attr.type, - slot->data.raw.data, - slot->data.raw.bytes ) ); + slot->data.key.data, + slot->data.key.bytes ) ); } @@ -5525,17 +5525,17 @@ static psa_status_t psa_generate_key_internal( if( key_type_is_raw_bytes( type ) ) { psa_status_t status; - status = prepare_raw_data_slot( type, bits, &slot->data.raw ); + status = prepare_raw_data_slot( type, bits, &slot->data.key ); if( status != PSA_SUCCESS ) return( status ); - status = psa_generate_random( slot->data.raw.data, - slot->data.raw.bytes ); + status = psa_generate_random( slot->data.key.data, + slot->data.key.bytes ); if( status != PSA_SUCCESS ) return( status ); #if defined(MBEDTLS_DES_C) if( type == PSA_KEY_TYPE_DES ) - psa_des_set_key_parity( slot->data.raw.data, - slot->data.raw.bytes ); + psa_des_set_key_parity( slot->data.key.data, + slot->data.key.bytes ); #endif /* MBEDTLS_DES_C */ } else diff --git a/library/psa_crypto_core.h b/library/psa_crypto_core.h index ef40f7994..8af45a17d 100644 --- a/library/psa_crypto_core.h +++ b/library/psa_crypto_core.h @@ -43,12 +43,13 @@ typedef struct psa_core_key_attributes_t attr; union { - /* Raw-data key (key_type_is_raw_bytes() in psa_crypto.c) */ - struct raw_data + /* Dynamically allocated key data buffer. + * Format as specified in psa_export_key(). */ + struct key_data { uint8_t *data; size_t bytes; - } raw; + } key; #if defined(MBEDTLS_RSA_C) /* RSA public key or key pair */ mbedtls_rsa_context *rsa; From 81be2fa0b24a9aaa3b5b5344fdba5857cdb13b7e Mon Sep 17 00:00:00 2001 From: Steven Cooreman Date: Fri, 24 Jul 2020 22:04:59 +0200 Subject: [PATCH 02/16] Pull apart slot memory allocation from key validation. Signed-off-by: Steven Cooreman --- library/psa_crypto.c | 64 ++++++++++++++++++++++++++++++-------------- 1 file changed, 44 insertions(+), 20 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 54980730c..e2e99d7d1 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -441,9 +441,8 @@ mbedtls_ecp_group_id mbedtls_ecc_group_of_psa( psa_ecc_family_t curve, } #endif /* defined(MBEDTLS_ECP_C) */ -static psa_status_t prepare_raw_data_slot( psa_key_type_t type, - size_t bits, - struct key_data *key ) +static psa_status_t validate_unstructured_key_bit_size( psa_key_type_t type, + size_t bits ) { /* Check that the bit size is acceptable for the key type */ switch( type ) @@ -490,14 +489,6 @@ static psa_status_t prepare_raw_data_slot( psa_key_type_t type, if( bits % 8 != 0 ) return( PSA_ERROR_INVALID_ARGUMENT ); - /* Allocate memory for the key */ - key->bytes = PSA_BITS_TO_BYTES( bits ); - key->data = mbedtls_calloc( 1, key->bytes ); - if( key->data == NULL ) - { - key->bytes = 0; - return( PSA_ERROR_INSUFFICIENT_MEMORY ); - } return( PSA_SUCCESS ); } @@ -740,22 +731,42 @@ psa_status_t psa_import_key_into_slot( psa_key_slot_t *slot, { psa_status_t status = PSA_SUCCESS; + /* zero-length keys are never supported. */ + if( data_length == 0 ) + return( PSA_ERROR_NOT_SUPPORTED ); + + /* Ensure that the bytes-to-bit conversion never overflows. */ + if( data_length > SIZE_MAX / 8 ) + return( PSA_ERROR_NOT_SUPPORTED ); + if( key_type_is_raw_bytes( slot->attr.type ) ) { size_t bit_size = PSA_BYTES_TO_BITS( data_length ); - /* Ensure that the bytes-to-bit conversion didn't overflow. */ - if( data_length > SIZE_MAX / 8 ) - return( PSA_ERROR_NOT_SUPPORTED ); + /* Enforce a size limit, and in particular ensure that the bit * size fits in its representation type. */ if( bit_size > PSA_MAX_KEY_BITS ) return( PSA_ERROR_NOT_SUPPORTED ); - status = prepare_raw_data_slot( slot->attr.type, bit_size, - &slot->data.key ); + + status = validate_unstructured_key_bit_size( slot->attr.type, bit_size ); if( status != PSA_SUCCESS ) - return( status ); - if( data_length != 0 ) - memcpy( slot->data.key.data, data, data_length ); + return status; + + /* Allocate memory for the key */ + slot->data.key.data = mbedtls_calloc( 1, data_length ); + if( slot->data.key.data == NULL ) + { + return( PSA_ERROR_INSUFFICIENT_MEMORY ); + } + slot->data.key.bytes = data_length; + + /* copy key into allocated buffer */ + memcpy(slot->data.key.data, data, data_length); + + /* Write the actual key size to the slot. + * psa_start_key_creation() wrote the size declared by the + * caller, which may be 0 (meaning unspecified) or wrong. */ + slot->attr.bits = (psa_key_bits_t) bit_size; } else #if defined(MBEDTLS_ECP_C) @@ -5525,13 +5536,26 @@ static psa_status_t psa_generate_key_internal( if( key_type_is_raw_bytes( type ) ) { psa_status_t status; - status = prepare_raw_data_slot( type, bits, &slot->data.key ); + + status = validate_unstructured_key_bit_size( slot->attr.type, bits ); if( status != PSA_SUCCESS ) return( status ); + + /* Allocate memory for the key */ + slot->data.key.bytes = PSA_BITS_TO_BYTES( bits ); + slot->data.key.data = mbedtls_calloc( 1, slot->data.key.bytes ); + if( slot->data.key.data == NULL ) + { + slot->data.key.bytes = 0; + return( PSA_ERROR_INSUFFICIENT_MEMORY ); + } + status = psa_generate_random( slot->data.key.data, slot->data.key.bytes ); if( status != PSA_SUCCESS ) return( status ); + + slot->attr.bits = (psa_key_bits_t) bits; #if defined(MBEDTLS_DES_C) if( type == PSA_KEY_TYPE_DES ) psa_des_set_key_parity( slot->data.key.data, From a01795d6090361e53afbafd731ec0357a95d0cba Mon Sep 17 00:00:00 2001 From: Steven Cooreman Date: Fri, 24 Jul 2020 22:48:15 +0200 Subject: [PATCH 03/16] Remove RSA internal representation from key slot Change to on-demand loading of the internal representation when required in order to call an mbed TLS cryptography API. Signed-off-by: Steven Cooreman --- library/psa_crypto.c | 380 +++++++++++++++++++++++++++++++------- library/psa_crypto_core.h | 5 - 2 files changed, 315 insertions(+), 70 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index e2e99d7d1..6f374b12b 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -492,7 +492,9 @@ static psa_status_t validate_unstructured_key_bit_size( psa_key_type_t type, return( PSA_SUCCESS ); } -#if defined(MBEDTLS_RSA_C) && defined(MBEDTLS_PK_PARSE_C) +#if defined(MBEDTLS_RSA_C) + +#if defined(MBEDTLS_PK_PARSE_C) /* Mbed TLS doesn't support non-byte-aligned key sizes (i.e. key sizes * that are not a multiple of 8) well. For example, there is only * mbedtls_rsa_get_len(), which returns a number of bytes, and no @@ -514,63 +516,201 @@ static psa_status_t psa_check_rsa_key_byte_aligned( mbedtls_mpi_free( &n ); return( status ); } +#endif /* MBEDTLS_PK_PARSE_C */ -static psa_status_t psa_import_rsa_key( psa_key_type_t type, - const uint8_t *data, - size_t data_length, - mbedtls_rsa_context **p_rsa ) +/** Load the contents of a key slot into an internal RSA representation + * + * \param[in] slot The slot from which to load the representation + * \param[out] rsa The internal RSA representation to hold the key. Must be + * allocated and initialized. If it already holds a + * different key, it will be overwritten and cause a memory + * leak. + */ +static psa_status_t psa_load_rsa_representation( const psa_key_slot_t *slot, + mbedtls_rsa_context *rsa ) { +#if defined(MBEDTLS_PK_PARSE_C) psa_status_t status; - mbedtls_pk_context pk; - mbedtls_rsa_context *rsa; + mbedtls_pk_context ctx; size_t bits; - - mbedtls_pk_init( &pk ); + mbedtls_pk_init( &ctx ); /* Parse the data. */ - if( PSA_KEY_TYPE_IS_KEY_PAIR( type ) ) + if( PSA_KEY_TYPE_IS_KEY_PAIR( slot->attr.type ) ) status = mbedtls_to_psa_error( - mbedtls_pk_parse_key( &pk, data, data_length, NULL, 0 ) ); + mbedtls_pk_parse_key( &ctx, slot->data.key.data, slot->data.key.bytes, NULL, 0 ) ); else status = mbedtls_to_psa_error( - mbedtls_pk_parse_public_key( &pk, data, data_length ) ); + mbedtls_pk_parse_public_key( &ctx, slot->data.key.data, slot->data.key.bytes ) ); if( status != PSA_SUCCESS ) goto exit; /* We have something that the pkparse module recognizes. If it is a * valid RSA key, store it. */ - if( mbedtls_pk_get_type( &pk ) != MBEDTLS_PK_RSA ) + if( mbedtls_pk_get_type( &ctx ) != MBEDTLS_PK_RSA ) { status = PSA_ERROR_INVALID_ARGUMENT; goto exit; } - rsa = mbedtls_pk_rsa( pk ); /* The size of an RSA key doesn't have to be a multiple of 8. Mbed TLS * supports non-byte-aligned key sizes, but not well. For example, * mbedtls_rsa_get_len() returns the key size in bytes, not in bits. */ - bits = PSA_BYTES_TO_BITS( mbedtls_rsa_get_len( rsa ) ); + bits = PSA_BYTES_TO_BITS( mbedtls_rsa_get_len( mbedtls_pk_rsa( ctx ) ) ); if( bits > PSA_VENDOR_RSA_MAX_KEY_BITS ) { status = PSA_ERROR_NOT_SUPPORTED; goto exit; } - status = psa_check_rsa_key_byte_aligned( rsa ); + status = psa_check_rsa_key_byte_aligned( mbedtls_pk_rsa( ctx ) ); + + if( status != PSA_SUCCESS ) + goto exit; + + /* Copy the PK-contained RSA context to the one provided as function input */ + status = mbedtls_to_psa_error( + mbedtls_rsa_copy( rsa, mbedtls_pk_rsa( ctx ) ) ); exit: - /* Free the content of the pk object only on error. */ + mbedtls_pk_free( &ctx ); + return( status ); +#else + (void) slot; + (void) rsa; + return( PSA_ERROR_NOT_SUPPORTED ); +#endif /* MBEDTLS_PK_PARSE_C */ +} + +/** Export an RSA key to export representation + * + * \param[in] type The type of key (public/private) to export + * \param[in] rsa The internal RSA representation from which to export + * \param[out] data The buffer to export to + * \param[in] data_size The length of the buffer to export to + * \param[out] data_length The amount of bytes written to \p data + */ +static psa_status_t psa_export_rsa_key( psa_key_type_t type, + mbedtls_rsa_context *rsa, + uint8_t *data, + size_t data_size, + size_t *data_length ) +{ +#if defined(MBEDTLS_PK_WRITE_C) + int ret; + mbedtls_pk_context pk; + uint8_t *pos = data + data_size; + + mbedtls_pk_init( &pk ); + pk.pk_info = &mbedtls_rsa_info; + pk.pk_ctx = rsa; + + /* PSA Crypto API defines the format of an RSA key as a DER-encoded + * representation of respectively the non-encrypted PKCS#1 RSAPrivateKey + * or the RFC3279 RSAPublicKey for a private key or a public key. */ + if( PSA_KEY_TYPE_IS_KEY_PAIR( type ) ) + ret = mbedtls_pk_write_key_der( &pk, data, data_size ); + else + ret = mbedtls_pk_write_pubkey( &pos, data, &pk ); + + if( ret < 0 ) + return mbedtls_to_psa_error( ret ); + + /* The mbedtls_pk_xxx functions write to the end of the buffer. + * Move the data to the beginning and erase remaining data + * at the original location. */ + if( 2 * (size_t) ret <= data_size ) + { + memcpy( data, data + data_size - ret, ret ); + memset( data + data_size - ret, 0, ret ); + } + else if( (size_t) ret < data_size ) + { + memmove( data, data + data_size - ret, ret ); + memset( data + ret, 0, data_size - ret ); + } + + *data_length = ret; + return( PSA_SUCCESS ); +#else + (void) type; + (void) rsa; + (void) data; + (void) data_size; + (void) data_length; + return( PSA_ERROR_NOT_SUPPORTED ); +#endif /* MBEDTLS_PK_WRITE_C */ +} + +/** Import an RSA key from import representation to a slot + * + * \param[in,out] slot The slot where to store the export representation to + * \param[in] data The buffer containing the import representation + * \param[in] data_length The amount of bytes in \p data + */ +static psa_status_t psa_import_rsa_key( psa_key_slot_t *slot, + const uint8_t *data, + size_t data_length ) +{ + psa_status_t status; + uint8_t* output = NULL; + mbedtls_rsa_context rsa; + mbedtls_rsa_init( &rsa, MBEDTLS_RSA_PKCS_V15, MBEDTLS_MD_NONE ); + + /* Temporarily load input into slot. The cast here is safe since it'll + * only be used for load_rsa_representation, which doesn't modify the + * buffer. */ + slot->data.key.data = (uint8_t *)data; + slot->data.key.bytes = data_length; + + /* Parse input */ + status = psa_load_rsa_representation( slot, &rsa ); + if( status != PSA_SUCCESS ) + goto exit; + + slot->attr.bits = (psa_key_bits_t) PSA_BYTES_TO_BITS( + mbedtls_rsa_get_len( &rsa ) ); + + /* Re-export the data to PSA export format, which in case of RSA is the + * smallest representation we can parse. */ + output = mbedtls_calloc( 1, data_length ); + + if( output == NULL ) + { + status = PSA_ERROR_INSUFFICIENT_MEMORY; + goto exit; + } + + /* PSA Crypto API defines the format of an RSA key as a DER-encoded + * representation of respectively the non-encrypted PKCS#1 RSAPrivateKey + * or the RFC3279 RSAPublicKey for a private key or a public key. That + * means we have no other choice then to run an import to verify the key + * size. */ + status = psa_export_rsa_key( slot->attr.type, + &rsa, + output, + data_length, + &data_length); + +exit: + /* Always free the RSA object */ + mbedtls_rsa_free( &rsa ); + + /* Free the allocated buffer only on error. */ if( status != PSA_SUCCESS ) { - mbedtls_pk_free( &pk ); + mbedtls_free( output ); + slot->data.key.data = NULL; + slot->data.key.bytes = 0; return( status ); } - /* On success, store the content of the object in the RSA context. */ - *p_rsa = rsa; + /* On success, store the allocated export-formatted key. */ + slot->data.key.data = output; + slot->data.key.bytes = data_length; return( PSA_SUCCESS ); } -#endif /* defined(MBEDTLS_RSA_C) && defined(MBEDTLS_PK_PARSE_C) */ +#endif /* defined(MBEDTLS_RSA_C) */ #if defined(MBEDTLS_ECP_C) static psa_status_t psa_prepare_import_ec_key( psa_ecc_family_t curve, @@ -708,10 +848,6 @@ static psa_key_bits_t psa_calculate_key_bits( const psa_key_slot_t *slot ) if( key_type_is_raw_bytes( slot->attr.type ) ) bits = PSA_BYTES_TO_BITS( slot->data.key.bytes ); -#if defined(MBEDTLS_RSA_C) - else if( PSA_KEY_TYPE_IS_RSA( slot->attr.type ) ) - bits = PSA_BYTES_TO_BITS( mbedtls_rsa_get_len( slot->data.rsa ) ); -#endif /* defined(MBEDTLS_RSA_C) */ #if defined(MBEDTLS_ECP_C) else if( PSA_KEY_TYPE_IS_ECC( slot->attr.type ) ) bits = slot->data.ecp->grp.pbits; @@ -788,9 +924,7 @@ psa_status_t psa_import_key_into_slot( psa_key_slot_t *slot, #if defined(MBEDTLS_RSA_C) && defined(MBEDTLS_PK_PARSE_C) if( PSA_KEY_TYPE_IS_RSA( slot->attr.type ) ) { - status = psa_import_rsa_key( slot->attr.type, - data, data_length, - &slot->data.rsa ); + status = psa_import_rsa_key( slot, data, data_length ); } else #endif /* defined(MBEDTLS_RSA_C) && defined(MBEDTLS_PK_PARSE_C) */ @@ -800,10 +934,13 @@ psa_status_t psa_import_key_into_slot( psa_key_slot_t *slot, if( status == PSA_SUCCESS ) { - /* Write the actual key size to the slot. - * psa_start_key_creation() wrote the size declared by the - * caller, which may be 0 (meaning unspecified) or wrong. */ - slot->attr.bits = psa_calculate_key_bits( slot ); + if( !PSA_KEY_TYPE_IS_RSA( slot->attr.type ) ) + { + /* Write the actual key size to the slot. + * psa_start_key_creation() wrote the size declared by the + * caller, which may be 0 (meaning unspecified) or wrong. */ + slot->attr.bits = psa_calculate_key_bits( slot ); + } } return( status ); } @@ -980,8 +1117,9 @@ static psa_status_t psa_remove_key_data_from_memory( psa_key_slot_t *slot ) #if defined(MBEDTLS_RSA_C) if( PSA_KEY_TYPE_IS_RSA( slot->attr.type ) ) { - mbedtls_rsa_free( slot->data.rsa ); - mbedtls_free( slot->data.rsa ); + mbedtls_free( slot->data.key.data ); + slot->data.key.data = NULL; + slot->data.key.bytes = 0; } else #endif /* defined(MBEDTLS_RSA_C) */ @@ -1232,7 +1370,18 @@ psa_status_t psa_get_key_attributes( psa_key_handle_t handle, if( psa_key_slot_is_external( slot ) ) break; #endif /* MBEDTLS_PSA_CRYPTO_SE_C */ - status = psa_get_rsa_public_exponent( slot->data.rsa, attributes ); + { + mbedtls_rsa_context rsa; + mbedtls_rsa_init( &rsa, MBEDTLS_RSA_PKCS_V15, MBEDTLS_MD_NONE ); + + status = psa_load_rsa_representation( slot, &rsa ); + if( status != PSA_SUCCESS ) + break; + + status = psa_get_rsa_public_exponent( &rsa, + attributes ); + mbedtls_rsa_free( &rsa ); + } break; #endif /* MBEDTLS_RSA_C */ default: @@ -1276,6 +1425,20 @@ static int pk_write_pubkey_simple( mbedtls_pk_context *key, } #endif /* defined(MBEDTLS_RSA_C) || defined(MBEDTLS_ECP_C) */ +static psa_status_t psa_internal_export_key_buffer( const psa_key_slot_t *slot, + uint8_t *data, + size_t data_size, + size_t *data_length ) +{ + if( slot->data.key.bytes > data_size ) + return( PSA_ERROR_BUFFER_TOO_SMALL ); + memcpy( data, slot->data.key.data, slot->data.key.bytes ); + memset( data + slot->data.key.bytes, 0, + data_size - slot->data.key.bytes ); + *data_length = slot->data.key.bytes; + return( PSA_SUCCESS ); +} + static psa_status_t psa_internal_export_key( const psa_key_slot_t *slot, uint8_t *data, size_t data_size, @@ -1354,10 +1517,36 @@ static psa_status_t psa_internal_export_key( const psa_key_slot_t *slot, if( PSA_KEY_TYPE_IS_RSA( slot->attr.type ) ) { #if defined(MBEDTLS_RSA_C) - mbedtls_pk_init( &pk ); - pk.pk_info = &mbedtls_rsa_info; - pk.pk_ctx = slot->data.rsa; + if( PSA_KEY_TYPE_IS_PUBLIC_KEY( slot->attr.type ) ) + { + /* Exporting public -> public */ + return( psa_internal_export_key_buffer( slot, data, data_size, data_length ) ); + } + else if( !export_public_key ) + { + /* Exporting private -> private */ + return( psa_internal_export_key_buffer( slot, data, data_size, data_length ) ); + } + + /* Exporting private -> public */ + mbedtls_rsa_context rsa; + mbedtls_rsa_init( &rsa, MBEDTLS_RSA_PKCS_V15, MBEDTLS_MD_NONE ); + + psa_status_t status = psa_load_rsa_representation( slot, &rsa ); + if( status != PSA_SUCCESS ) + return status; + + status = psa_export_rsa_key( PSA_KEY_TYPE_RSA_PUBLIC_KEY, + &rsa, + data, + data_size, + data_length ); + + mbedtls_rsa_free( &rsa ); + + return( status ); #else + /* We don't know how to convert a private RSA key to public. */ return( PSA_ERROR_NOT_SUPPORTED ); #endif } @@ -1805,12 +1994,19 @@ static psa_status_t psa_validate_optional_attributes( #if defined(MBEDTLS_RSA_C) if( PSA_KEY_TYPE_IS_RSA( slot->attr.type ) ) { + mbedtls_rsa_context rsa; + mbedtls_rsa_init( &rsa, MBEDTLS_RSA_PKCS_V15, MBEDTLS_MD_NONE ); + + psa_status_t status = psa_load_rsa_representation( slot, &rsa ); + if( status != PSA_SUCCESS ) + return status; mbedtls_mpi actual, required; int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; mbedtls_mpi_init( &actual ); mbedtls_mpi_init( &required ); - ret = mbedtls_rsa_export( slot->data.rsa, + ret = mbedtls_rsa_export( &rsa, NULL, NULL, NULL, NULL, &actual ); + mbedtls_rsa_free( &rsa ); if( ret != 0 ) goto rsa_exit; ret = mbedtls_mpi_read_binary( &required, @@ -3447,11 +3643,21 @@ psa_status_t psa_sign_hash( psa_key_handle_t handle, #if defined(MBEDTLS_RSA_C) if( slot->attr.type == PSA_KEY_TYPE_RSA_KEY_PAIR ) { - status = psa_rsa_sign( slot->data.rsa, + mbedtls_rsa_context rsa; + mbedtls_rsa_init( &rsa, MBEDTLS_RSA_PKCS_V15, MBEDTLS_MD_NONE ); + + status = psa_load_rsa_representation( slot, + &rsa ); + if( status != PSA_SUCCESS ) + goto exit; + + status = psa_rsa_sign( &rsa, alg, hash, hash_length, signature, signature_size, signature_length ); + + mbedtls_rsa_free( &rsa ); } else #endif /* defined(MBEDTLS_RSA_C) */ @@ -3533,10 +3739,19 @@ psa_status_t psa_verify_hash( psa_key_handle_t handle, #if defined(MBEDTLS_RSA_C) if( PSA_KEY_TYPE_IS_RSA( slot->attr.type ) ) { - return( psa_rsa_verify( slot->data.rsa, - alg, - hash, hash_length, - signature, signature_length ) ); + mbedtls_rsa_context rsa; + mbedtls_rsa_init( &rsa, MBEDTLS_RSA_PKCS_V15, MBEDTLS_MD_NONE ); + + status = psa_load_rsa_representation( slot, &rsa ); + if( status != PSA_SUCCESS ) + return status; + + status = psa_rsa_verify( &rsa, + alg, + hash, hash_length, + signature, signature_length ); + mbedtls_rsa_free( &rsa ); + return( status ); } else #endif /* defined(MBEDTLS_RSA_C) */ @@ -3606,14 +3821,22 @@ psa_status_t psa_asymmetric_encrypt( psa_key_handle_t handle, #if defined(MBEDTLS_RSA_C) if( PSA_KEY_TYPE_IS_RSA( slot->attr.type ) ) { - mbedtls_rsa_context *rsa = slot->data.rsa; + mbedtls_rsa_context rsa; + mbedtls_rsa_init( &rsa, MBEDTLS_RSA_PKCS_V15, MBEDTLS_MD_NONE ); + + status = psa_load_rsa_representation( slot, &rsa ); + if( status != PSA_SUCCESS ) + return status; int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; - if( output_size < mbedtls_rsa_get_len( rsa ) ) + if( output_size < mbedtls_rsa_get_len( &rsa ) ) + { + mbedtls_rsa_free( &rsa ); return( PSA_ERROR_BUFFER_TOO_SMALL ); + } #if defined(MBEDTLS_PKCS1_V15) if( alg == PSA_ALG_RSA_PKCS1V15_CRYPT ) { - ret = mbedtls_rsa_pkcs1_encrypt( rsa, + ret = mbedtls_rsa_pkcs1_encrypt( &rsa, mbedtls_ctr_drbg_random, &global_data.ctr_drbg, MBEDTLS_RSA_PUBLIC, @@ -3626,8 +3849,8 @@ psa_status_t psa_asymmetric_encrypt( psa_key_handle_t handle, #if defined(MBEDTLS_PKCS1_V21) if( PSA_ALG_IS_RSA_OAEP( alg ) ) { - psa_rsa_oaep_set_padding_mode( alg, rsa ); - ret = mbedtls_rsa_rsaes_oaep_encrypt( rsa, + psa_rsa_oaep_set_padding_mode( alg, &rsa ); + ret = mbedtls_rsa_rsaes_oaep_encrypt( &rsa, mbedtls_ctr_drbg_random, &global_data.ctr_drbg, MBEDTLS_RSA_PUBLIC, @@ -3639,10 +3862,13 @@ psa_status_t psa_asymmetric_encrypt( psa_key_handle_t handle, else #endif /* MBEDTLS_PKCS1_V21 */ { + mbedtls_rsa_free( &rsa ); return( PSA_ERROR_INVALID_ARGUMENT ); } if( ret == 0 ) - *output_length = mbedtls_rsa_get_len( rsa ); + *output_length = mbedtls_rsa_get_len( &rsa ); + + mbedtls_rsa_free( &rsa ); return( mbedtls_to_psa_error( ret ) ); } else @@ -3685,16 +3911,24 @@ psa_status_t psa_asymmetric_decrypt( psa_key_handle_t handle, #if defined(MBEDTLS_RSA_C) if( slot->attr.type == PSA_KEY_TYPE_RSA_KEY_PAIR ) { - mbedtls_rsa_context *rsa = slot->data.rsa; + mbedtls_rsa_context rsa; + mbedtls_rsa_init( &rsa, MBEDTLS_RSA_PKCS_V15, MBEDTLS_MD_NONE ); + + status = psa_load_rsa_representation( slot, &rsa ); + if( status != PSA_SUCCESS ) + return status; int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; - if( input_length != mbedtls_rsa_get_len( rsa ) ) + if( input_length != mbedtls_rsa_get_len( &rsa ) ) + { + mbedtls_rsa_free( &rsa ); return( PSA_ERROR_INVALID_ARGUMENT ); + } #if defined(MBEDTLS_PKCS1_V15) if( alg == PSA_ALG_RSA_PKCS1V15_CRYPT ) { - ret = mbedtls_rsa_pkcs1_decrypt( rsa, + ret = mbedtls_rsa_pkcs1_decrypt( &rsa, mbedtls_ctr_drbg_random, &global_data.ctr_drbg, MBEDTLS_RSA_PRIVATE, @@ -3708,8 +3942,8 @@ psa_status_t psa_asymmetric_decrypt( psa_key_handle_t handle, #if defined(MBEDTLS_PKCS1_V21) if( PSA_ALG_IS_RSA_OAEP( alg ) ) { - psa_rsa_oaep_set_padding_mode( alg, rsa ); - ret = mbedtls_rsa_rsaes_oaep_decrypt( rsa, + psa_rsa_oaep_set_padding_mode( alg, &rsa ); + ret = mbedtls_rsa_rsaes_oaep_decrypt( &rsa, mbedtls_ctr_drbg_random, &global_data.ctr_drbg, MBEDTLS_RSA_PRIVATE, @@ -3722,9 +3956,11 @@ psa_status_t psa_asymmetric_decrypt( psa_key_handle_t handle, else #endif /* MBEDTLS_PKCS1_V21 */ { + mbedtls_rsa_free( &rsa ); return( PSA_ERROR_INVALID_ARGUMENT ); } + mbedtls_rsa_free( &rsa ); return( mbedtls_to_psa_error( ret ) ); } else @@ -5567,7 +5803,7 @@ static psa_status_t psa_generate_key_internal( #if defined(MBEDTLS_RSA_C) && defined(MBEDTLS_GENPRIME) if ( type == PSA_KEY_TYPE_RSA_KEY_PAIR ) { - mbedtls_rsa_context *rsa; + mbedtls_rsa_context rsa; int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; int exponent; psa_status_t status; @@ -5582,22 +5818,36 @@ static psa_status_t psa_generate_key_internal( &exponent ); if( status != PSA_SUCCESS ) return( status ); - rsa = mbedtls_calloc( 1, sizeof( *rsa ) ); - if( rsa == NULL ) - return( PSA_ERROR_INSUFFICIENT_MEMORY ); - mbedtls_rsa_init( rsa, MBEDTLS_RSA_PKCS_V15, MBEDTLS_MD_NONE ); - ret = mbedtls_rsa_gen_key( rsa, + mbedtls_rsa_init( &rsa, MBEDTLS_RSA_PKCS_V15, MBEDTLS_MD_NONE ); + ret = mbedtls_rsa_gen_key( &rsa, mbedtls_ctr_drbg_random, &global_data.ctr_drbg, (unsigned int) bits, exponent ); if( ret != 0 ) - { - mbedtls_rsa_free( rsa ); - mbedtls_free( rsa ); return( mbedtls_to_psa_error( ret ) ); + + /* Make sure to always have an export representation available */ + size_t bytes = PSA_KEY_EXPORT_RSA_KEY_PAIR_MAX_SIZE( bits ); + + slot->data.key.data = mbedtls_calloc( 1, bytes ); + if( slot->data.key.data == NULL ) + { + mbedtls_rsa_free( &rsa ); + return( PSA_ERROR_INSUFFICIENT_MEMORY ); + } + + status = psa_export_rsa_key( type, + &rsa, + slot->data.key.data, + bytes, + &slot->data.key.bytes ); + mbedtls_rsa_free( &rsa ); + if( status != PSA_SUCCESS ) + { + psa_remove_key_data_from_memory( slot ); + return( status ); } - slot->data.rsa = rsa; } else #endif /* MBEDTLS_RSA_C && MBEDTLS_GENPRIME */ diff --git a/library/psa_crypto_core.h b/library/psa_crypto_core.h index 8af45a17d..c90d7375a 100644 --- a/library/psa_crypto_core.h +++ b/library/psa_crypto_core.h @@ -33,7 +33,6 @@ #include "psa/crypto_se_driver.h" #include "mbedtls/ecp.h" -#include "mbedtls/rsa.h" /** The data structure representing a key slot, containing key material * and metadata for one key. @@ -50,10 +49,6 @@ typedef struct uint8_t *data; size_t bytes; } key; -#if defined(MBEDTLS_RSA_C) - /* RSA public key or key pair */ - mbedtls_rsa_context *rsa; -#endif /* MBEDTLS_RSA_C */ #if defined(MBEDTLS_ECP_C) /* EC public key or key pair */ mbedtls_ecp_keypair *ecp; From acda8346bf3ab9dbffbf8a56b09f45e0ed8f795d Mon Sep 17 00:00:00 2001 From: Steven Cooreman Date: Fri, 24 Jul 2020 23:09:52 +0200 Subject: [PATCH 04/16] Remove ECP internal representation from key slot Change to on-demand loading of the internal representation when required in order to call an mbed TLS cryptography API. Signed-off-by: Steven Cooreman --- library/psa_crypto.c | 423 ++++++++++++++++++++++---------------- library/psa_crypto_core.h | 6 - 2 files changed, 242 insertions(+), 187 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 6f374b12b..ee616956a 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -713,18 +713,17 @@ exit: #endif /* defined(MBEDTLS_RSA_C) */ #if defined(MBEDTLS_ECP_C) -static psa_status_t psa_prepare_import_ec_key( psa_ecc_family_t curve, - size_t data_length, - int is_public, - mbedtls_ecp_keypair **p_ecp ) +/* Load the key slot contents into an mbedTLS internal representation object. + * Note: caller is responsible for freeing the object properly */ +static psa_status_t psa_load_ecp_representation( const psa_key_slot_t *slot, + mbedtls_ecp_keypair *ecp ) { mbedtls_ecp_group_id grp_id = MBEDTLS_ECP_DP_NONE; - *p_ecp = mbedtls_calloc( 1, sizeof( mbedtls_ecp_keypair ) ); - if( *p_ecp == NULL ) - return( PSA_ERROR_INSUFFICIENT_MEMORY ); - mbedtls_ecp_keypair_init( *p_ecp ); + size_t data_length = slot->data.key.bytes; + psa_status_t status; + mbedtls_ecp_keypair_init( ecp ); - if( is_public ) + if( PSA_KEY_TYPE_IS_PUBLIC_KEY( slot->attr.type ) ) { /* A public key is represented as: * - The byte 0x04; @@ -732,99 +731,169 @@ static psa_status_t psa_prepare_import_ec_key( psa_ecc_family_t curve, * - `y_P` as a `ceiling(m/8)`-byte string, big-endian. * So its data length is 2m+1 where n is the key size in bits. */ - if( ( data_length & 1 ) == 0 ) + if( ( slot->data.key.bytes & 1 ) == 0 ) return( PSA_ERROR_INVALID_ARGUMENT ); - data_length = data_length / 2; + data_length = slot->data.key.bytes / 2; } /* Load the group. */ - grp_id = mbedtls_ecc_group_of_psa( curve, data_length ); + grp_id = mbedtls_ecc_group_of_psa( PSA_KEY_TYPE_ECC_GET_FAMILY( slot->attr.type), + data_length ); if( grp_id == MBEDTLS_ECP_DP_NONE ) return( PSA_ERROR_INVALID_ARGUMENT ); - return( mbedtls_to_psa_error( - mbedtls_ecp_group_load( &( *p_ecp )->grp, grp_id ) ) ); + status = mbedtls_to_psa_error( + mbedtls_ecp_group_load( &ecp->grp, grp_id ) ); + if( status != PSA_SUCCESS ) + return( status ); + + /* Load the key material */ + if( PSA_KEY_TYPE_IS_PUBLIC_KEY( slot->attr.type ) ) + { + /* Load the public value. */ + status = mbedtls_to_psa_error( + mbedtls_ecp_point_read_binary( &ecp->grp, &ecp->Q, + slot->data.key.data, + slot->data.key.bytes ) ); + if( status != PSA_SUCCESS ) + goto exit; + + /* Check that the point is on the curve. */ + status = mbedtls_to_psa_error( + mbedtls_ecp_check_pubkey( &ecp->grp, &ecp->Q ) ); + if( status != PSA_SUCCESS ) + goto exit; + } + else + { + /* Load the secret value. */ + status = mbedtls_to_psa_error( + mbedtls_ecp_read_key( ecp->grp.id, + ecp, + slot->data.key.data, + slot->data.key.bytes ) ); + + if( status != PSA_SUCCESS ) + goto exit; + /* Validate the private key. */ + status = mbedtls_to_psa_error( + mbedtls_ecp_check_privkey( &ecp->grp, &ecp->d ) ); + if( status != PSA_SUCCESS ) + goto exit; + } +exit: + if( status != PSA_SUCCESS ) + mbedtls_ecp_keypair_free( ecp ); + return status; } -/* Import a public key given as the uncompressed representation defined by SEC1 - * 2.3.3 as the content of an ECPoint. */ -static psa_status_t psa_import_ec_public_key( psa_ecc_family_t curve, - const uint8_t *data, - size_t data_length, - mbedtls_ecp_keypair **p_ecp ) +static psa_status_t psa_export_ecp_key( psa_key_type_t type, + mbedtls_ecp_keypair *ecp, + uint8_t *data, + size_t data_size, + size_t *data_length ) { - psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED; - mbedtls_ecp_keypair *ecp = NULL; + psa_status_t status; - status = psa_prepare_import_ec_key( curve, data_length, 1, &ecp ); - if( status != PSA_SUCCESS ) - goto exit; - - /* Load the public value. */ - status = mbedtls_to_psa_error( - mbedtls_ecp_point_read_binary( &ecp->grp, &ecp->Q, - data, data_length ) ); - if( status != PSA_SUCCESS ) - goto exit; - - /* Check that the point is on the curve. */ - status = mbedtls_to_psa_error( - mbedtls_ecp_check_pubkey( &ecp->grp, &ecp->Q ) ); - if( status != PSA_SUCCESS ) - goto exit; - - *p_ecp = ecp; - return( PSA_SUCCESS ); - -exit: - if( ecp != NULL ) + if( PSA_KEY_TYPE_IS_PUBLIC_KEY( type ) ) { - mbedtls_ecp_keypair_free( ecp ); - mbedtls_free( ecp ); + /* Check whether the public part is loaded */ + if( mbedtls_ecp_is_zero( &ecp->Q ) ) + { + /* Calculate the public key */ + status = mbedtls_to_psa_error( + mbedtls_ecp_mul( &ecp->grp, &ecp->Q, &ecp->d, &ecp->grp.G, + mbedtls_ctr_drbg_random, &global_data.ctr_drbg ) ); + if( status != PSA_SUCCESS ) + return status; + } + + return( mbedtls_to_psa_error( + mbedtls_ecp_point_write_binary( &ecp->grp, &ecp->Q, + MBEDTLS_ECP_PF_UNCOMPRESSED, + data_length, + data, + data_size ) ) ); + } + else + { + if( data_size < PSA_BITS_TO_BYTES(ecp->grp.nbits) ) + return( PSA_ERROR_BUFFER_TOO_SMALL ); + + status = mbedtls_to_psa_error( + mbedtls_ecp_write_key( ecp, + data, + PSA_BITS_TO_BYTES(ecp->grp.nbits) ) ); + if( status == PSA_SUCCESS ) + { + *data_length = PSA_BITS_TO_BYTES(ecp->grp.nbits); + } + + return( status ); } - return( status ); } -/* Import a private key given as a byte string which is the private value - * in big-endian order. */ -static psa_status_t psa_import_ec_private_key( psa_ecc_family_t curve, - const uint8_t *data, - size_t data_length, - mbedtls_ecp_keypair **p_ecp ) +static psa_status_t psa_import_ecp_key( psa_key_slot_t *slot, + const uint8_t *data, + size_t data_length ) { - psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED; - mbedtls_ecp_keypair *ecp = NULL; + psa_status_t status; + uint8_t* output = NULL; + mbedtls_ecp_keypair ecp; - status = psa_prepare_import_ec_key( curve, data_length, 0, &ecp ); + /* Temporarily load input into slot. The cast here is safe since it'll + * only be used for load_ecp_representation, which doesn't modify the + * buffer. */ + slot->data.key.data = (uint8_t *)data; + slot->data.key.bytes = data_length; + + /* Parse input */ + status = psa_load_ecp_representation( slot, &ecp ); if( status != PSA_SUCCESS ) goto exit; - /* Load and validate the secret key */ - status = mbedtls_to_psa_error( - mbedtls_ecp_read_key( ecp->grp.id, ecp, data, data_length ) ); - if( status != PSA_SUCCESS ) - goto exit; + if( PSA_KEY_TYPE_ECC_GET_FAMILY( slot->attr.type ) == PSA_ECC_FAMILY_MONTGOMERY) + slot->attr.bits = (psa_key_bits_t) ecp.grp.nbits + 1; + else + slot->attr.bits = (psa_key_bits_t) ecp.grp.nbits; - /* Calculate the public key from the private key. */ - status = mbedtls_to_psa_error( - mbedtls_ecp_mul( &ecp->grp, &ecp->Q, &ecp->d, &ecp->grp.G, - mbedtls_ctr_drbg_random, &global_data.ctr_drbg ) ); - if( status != PSA_SUCCESS ) - goto exit; + /* Re-export the data to PSA export format. There is currently no support + * for other input formats then the export format, so this is a 1-1 + * copy operation. */ + output = mbedtls_calloc( 1, data_length ); - *p_ecp = ecp; - return( PSA_SUCCESS ); + if( output == NULL ) + { + status = PSA_ERROR_INSUFFICIENT_MEMORY; + goto exit; + } + + status = psa_export_ecp_key( slot->attr.type, + &ecp, + output, + data_length, + &data_length); exit: - if( ecp != NULL ) + /* Always free the PK object (will also free contained RSA context) */ + mbedtls_ecp_keypair_free( &ecp ); + + /* Free the allocated buffer only on error. */ + if( status != PSA_SUCCESS ) { - mbedtls_ecp_keypair_free( ecp ); - mbedtls_free( ecp ); + mbedtls_free( output ); + slot->data.key.data = NULL; + slot->data.key.bytes = 0; + return( status ); } - return( status ); + + /* On success, store the allocated export-formatted key. */ + slot->data.key.data = output; + slot->data.key.bytes = data_length; + + return( PSA_SUCCESS ); } #endif /* defined(MBEDTLS_ECP_C) */ - /** Return the size of the key in the given slot, in bits. * * \param[in] slot A key slot. @@ -848,10 +917,6 @@ static psa_key_bits_t psa_calculate_key_bits( const psa_key_slot_t *slot ) if( key_type_is_raw_bytes( slot->attr.type ) ) bits = PSA_BYTES_TO_BITS( slot->data.key.bytes ); -#if defined(MBEDTLS_ECP_C) - else if( PSA_KEY_TYPE_IS_ECC( slot->attr.type ) ) - bits = slot->data.ecp->grp.pbits; -#endif /* defined(MBEDTLS_ECP_C) */ /* We know that the size fits in psa_key_bits_t thanks to checks * when the key was created. */ @@ -906,18 +971,9 @@ psa_status_t psa_import_key_into_slot( psa_key_slot_t *slot, } else #if defined(MBEDTLS_ECP_C) - if( PSA_KEY_TYPE_IS_ECC_KEY_PAIR( slot->attr.type ) ) + if( PSA_KEY_TYPE_IS_ECC( slot->attr.type ) ) { - status = psa_import_ec_private_key( PSA_KEY_TYPE_ECC_GET_FAMILY( slot->attr.type ), - data, data_length, - &slot->data.ecp ); - } - else if( PSA_KEY_TYPE_IS_ECC_PUBLIC_KEY( slot->attr.type ) ) - { - status = psa_import_ec_public_key( - PSA_KEY_TYPE_ECC_GET_FAMILY( slot->attr.type ), - data, data_length, - &slot->data.ecp ); + status = psa_import_ecp_key( slot, data, data_length ); } else #endif /* MBEDTLS_ECP_C */ @@ -934,7 +990,8 @@ psa_status_t psa_import_key_into_slot( psa_key_slot_t *slot, if( status == PSA_SUCCESS ) { - if( !PSA_KEY_TYPE_IS_RSA( slot->attr.type ) ) + if( !PSA_KEY_TYPE_IS_RSA( slot->attr.type ) && + !PSA_KEY_TYPE_IS_ECC( slot->attr.type ) ) { /* Write the actual key size to the slot. * psa_start_key_creation() wrote the size declared by the @@ -1126,8 +1183,9 @@ static psa_status_t psa_remove_key_data_from_memory( psa_key_slot_t *slot ) #if defined(MBEDTLS_ECP_C) if( PSA_KEY_TYPE_IS_ECC( slot->attr.type ) ) { - mbedtls_ecp_keypair_free( slot->data.ecp ); - mbedtls_free( slot->data.ecp ); + mbedtls_free( slot->data.key.data ); + slot->data.key.data = NULL; + slot->data.key.bytes = 0; } else #endif /* defined(MBEDTLS_ECP_C) */ @@ -1409,22 +1467,6 @@ psa_status_t psa_get_key_slot_number( } #endif /* MBEDTLS_PSA_CRYPTO_SE_C */ -#if defined(MBEDTLS_RSA_C) || defined(MBEDTLS_ECP_C) -static int pk_write_pubkey_simple( mbedtls_pk_context *key, - unsigned char *buf, size_t size ) -{ - int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; - unsigned char *c; - size_t len = 0; - - c = buf + size; - - MBEDTLS_ASN1_CHK_ADD( len, mbedtls_pk_write_pubkey( &c, buf, key ) ); - - return( (int) len ); -} -#endif /* defined(MBEDTLS_RSA_C) || defined(MBEDTLS_ECP_C) */ - static psa_status_t psa_internal_export_key_buffer( const psa_key_slot_t *slot, uint8_t *data, size_t data_size, @@ -1491,29 +1533,15 @@ static psa_status_t psa_internal_export_key( const psa_key_slot_t *slot, #if defined(MBEDTLS_ECP_C) if( PSA_KEY_TYPE_IS_ECC_KEY_PAIR( slot->attr.type ) && !export_public_key ) { - psa_status_t status; - - size_t bytes = PSA_BITS_TO_BYTES( slot->attr.bits ); - if( bytes > data_size ) - return( PSA_ERROR_BUFFER_TOO_SMALL ); - status = mbedtls_to_psa_error( - mbedtls_ecp_write_key( slot->data.ecp, - data, bytes ) ); - if( status != PSA_SUCCESS ) - return( status ); - memset( data + bytes, 0, data_size - bytes ); - *data_length = bytes; - return( PSA_SUCCESS ); + /* Exporting private -> private */ + return( psa_internal_export_key_buffer( slot, data, data_size, data_length ) ); } #endif else { -#if defined(MBEDTLS_PK_WRITE_C) if( PSA_KEY_TYPE_IS_RSA( slot->attr.type ) || PSA_KEY_TYPE_IS_ECC( slot->attr.type ) ) { - mbedtls_pk_context pk; - int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; if( PSA_KEY_TYPE_IS_RSA( slot->attr.type ) ) { #if defined(MBEDTLS_RSA_C) @@ -1553,44 +1581,28 @@ static psa_status_t psa_internal_export_key( const psa_key_slot_t *slot, else { #if defined(MBEDTLS_ECP_C) - mbedtls_pk_init( &pk ); - pk.pk_info = &mbedtls_eckey_info; - pk.pk_ctx = slot->data.ecp; + mbedtls_ecp_keypair ecp; + psa_status_t status = psa_load_ecp_representation( slot, &ecp ); + if( status != PSA_SUCCESS ) + return status; + + status = psa_export_ecp_key( PSA_KEY_TYPE_ECC_PUBLIC_KEY( + PSA_KEY_TYPE_ECC_GET_FAMILY( + slot->attr.type ) ), + &ecp, + data, + data_size, + data_length ); + + mbedtls_ecp_keypair_free( &ecp ); + return( status ); #else + /* We don't know how to convert a private ECC key to public. */ return( PSA_ERROR_NOT_SUPPORTED ); -#endif +#endif /* defined(MBEDTLS_ECP_C) */ } - if( export_public_key || PSA_KEY_TYPE_IS_PUBLIC_KEY( slot->attr.type ) ) - { - ret = pk_write_pubkey_simple( &pk, data, data_size ); - } - else - { - ret = mbedtls_pk_write_key_der( &pk, data, data_size ); - } - if( ret < 0 ) - { - memset( data, 0, data_size ); - return( mbedtls_to_psa_error( ret ) ); - } - /* The mbedtls_pk_xxx functions write to the end of the buffer. - * Move the data to the beginning and erase remaining data - * at the original location. */ - if( 2 * (size_t) ret <= data_size ) - { - memcpy( data, data + data_size - ret, ret ); - memset( data + data_size - ret, 0, ret ); - } - else if( (size_t) ret < data_size ) - { - memmove( data, data + data_size - ret, ret ); - memset( data + ret, 0, data_size - ret ); - } - *data_length = ret; - return( PSA_SUCCESS ); } else -#endif /* defined(MBEDTLS_PK_WRITE_C) */ { /* This shouldn't happen in the reference implementation, but it is valid for a special-purpose implementation to omit @@ -3580,6 +3592,14 @@ static psa_status_t psa_ecdsa_verify( mbedtls_ecp_keypair *ecp, signature + curve_bytes, curve_bytes ) ); + /* Check whether the public part is loaded. If not, load it. */ + if( mbedtls_ecp_is_zero( &ecp->Q ) ) + { + MBEDTLS_MPI_CHK( + mbedtls_ecp_mul( &ecp->grp, &ecp->Q, &ecp->d, &ecp->grp.G, + mbedtls_ctr_drbg_random, &global_data.ctr_drbg ) ); + } + ret = mbedtls_ecdsa_verify( &ecp->grp, hash, hash_length, &ecp->Q, &r, &s ); @@ -3672,11 +3692,18 @@ psa_status_t psa_sign_hash( psa_key_handle_t handle, PSA_ALG_IS_RANDOMIZED_ECDSA( alg ) #endif ) - status = psa_ecdsa_sign( slot->data.ecp, + { + mbedtls_ecp_keypair ecp; + status = psa_load_ecp_representation( slot, &ecp ); + if( status != PSA_SUCCESS ) + goto exit; + status = psa_ecdsa_sign( &ecp, alg, hash, hash_length, signature, signature_size, signature_length ); + mbedtls_ecp_keypair_free( &ecp ); + } else #endif /* defined(MBEDTLS_ECDSA_C) */ { @@ -3760,9 +3787,17 @@ psa_status_t psa_verify_hash( psa_key_handle_t handle, { #if defined(MBEDTLS_ECDSA_C) if( PSA_ALG_IS_ECDSA( alg ) ) - return( psa_ecdsa_verify( slot->data.ecp, - hash, hash_length, - signature, signature_length ) ); + { + mbedtls_ecp_keypair ecp; + status = psa_load_ecp_representation( slot, &ecp ); + if( status != PSA_SUCCESS ) + return status; + status = psa_ecdsa_verify( &ecp, + hash, hash_length, + signature, signature_length ); + mbedtls_ecp_keypair_free( &ecp ); + return status; + } else #endif /* defined(MBEDTLS_ECDSA_C) */ { @@ -5511,21 +5546,26 @@ static psa_status_t psa_key_agreement_ecdh( const uint8_t *peer_key, size_t shared_secret_size, size_t *shared_secret_length ) { - mbedtls_ecp_keypair *their_key = NULL; + mbedtls_ecp_keypair their_key; + psa_key_slot_t their_key_slot; mbedtls_ecdh_context ecdh; psa_status_t status; size_t bits = 0; psa_ecc_family_t curve = mbedtls_ecc_group_to_psa( our_key->grp.id, &bits ); mbedtls_ecdh_init( &ecdh ); + memset(&their_key_slot, 0, sizeof(their_key_slot)); - status = psa_import_ec_public_key( curve, - peer_key, peer_key_length, - &their_key ); + /* Creating ephemeral key slot for import purposes */ + their_key_slot.attr.type = PSA_KEY_TYPE_ECC_PUBLIC_KEY(curve); + their_key_slot.data.key.data = (uint8_t*) peer_key; + their_key_slot.data.key.bytes = peer_key_length; + + status = psa_load_ecp_representation( &their_key_slot, &their_key ); if( status != PSA_SUCCESS ) goto exit; status = mbedtls_to_psa_error( - mbedtls_ecdh_get_params( &ecdh, their_key, MBEDTLS_ECDH_THEIRS ) ); + mbedtls_ecdh_get_params( &ecdh, &their_key, MBEDTLS_ECDH_THEIRS ) ); if( status != PSA_SUCCESS ) goto exit; status = mbedtls_to_psa_error( @@ -5548,8 +5588,7 @@ exit: if( status != PSA_SUCCESS ) mbedtls_platform_zeroize( shared_secret, shared_secret_size ); mbedtls_ecdh_free( &ecdh ); - mbedtls_ecp_keypair_free( their_key ); - mbedtls_free( their_key ); + mbedtls_ecp_keypair_free( &their_key ); return( status ); } #endif /* MBEDTLS_ECDH_C */ @@ -5570,10 +5609,16 @@ static psa_status_t psa_key_agreement_raw_internal( psa_algorithm_t alg, case PSA_ALG_ECDH: if( ! PSA_KEY_TYPE_IS_ECC_KEY_PAIR( private_key->attr.type ) ) return( PSA_ERROR_INVALID_ARGUMENT ); - return( psa_key_agreement_ecdh( peer_key, peer_key_length, - private_key->data.ecp, - shared_secret, shared_secret_size, - shared_secret_length ) ); + mbedtls_ecp_keypair ecp; + psa_status_t status = psa_load_ecp_representation( private_key, &ecp ); + if( status != PSA_SUCCESS ) + return status; + status = psa_key_agreement_ecdh( peer_key, peer_key_length, + &ecp, + shared_secret, shared_secret_size, + shared_secret_length ); + mbedtls_ecp_keypair_free( &ecp ); + return status; #endif /* MBEDTLS_ECDH_C */ default: (void) private_key; @@ -5860,7 +5905,7 @@ static psa_status_t psa_generate_key_internal( mbedtls_ecc_group_of_psa( curve, PSA_BITS_TO_BYTES( bits ) ); const mbedtls_ecp_curve_info *curve_info = mbedtls_ecp_curve_info_from_grp_id( grp_id ); - mbedtls_ecp_keypair *ecp; + mbedtls_ecp_keypair ecp; int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; if( domain_parameters_size != 0 ) return( PSA_ERROR_NOT_SUPPORTED ); @@ -5868,20 +5913,36 @@ static psa_status_t psa_generate_key_internal( return( PSA_ERROR_NOT_SUPPORTED ); if( curve_info->bit_size != bits ) return( PSA_ERROR_INVALID_ARGUMENT ); - ecp = mbedtls_calloc( 1, sizeof( *ecp ) ); - if( ecp == NULL ) - return( PSA_ERROR_INSUFFICIENT_MEMORY ); - mbedtls_ecp_keypair_init( ecp ); - ret = mbedtls_ecp_gen_key( grp_id, ecp, + mbedtls_ecp_keypair_init( &ecp ); + ret = mbedtls_ecp_gen_key( grp_id, &ecp, mbedtls_ctr_drbg_random, &global_data.ctr_drbg ); if( ret != 0 ) { - mbedtls_ecp_keypair_free( ecp ); - mbedtls_free( ecp ); + mbedtls_ecp_keypair_free( &ecp ); return( mbedtls_to_psa_error( ret ) ); } - slot->data.ecp = ecp; + + + /* Make sure to always have an export representation available */ + size_t bytes = PSA_BITS_TO_BYTES( bits ); + slot->data.key.data = mbedtls_calloc( 1, bytes ); + if( slot->data.key.data == NULL ) + { + mbedtls_ecp_keypair_free( &ecp ); + return( PSA_ERROR_INSUFFICIENT_MEMORY ); + } + slot->data.key.bytes = bytes; + psa_status_t status = mbedtls_to_psa_error( + mbedtls_ecp_write_key( &ecp, slot->data.key.data, bytes ) ); + + mbedtls_ecp_keypair_free( &ecp ); + if( status != PSA_SUCCESS ) + { + psa_remove_key_data_from_memory( slot ); + return( status ); + } + return( PSA_SUCCESS ); } else #endif /* MBEDTLS_ECP_C */ diff --git a/library/psa_crypto_core.h b/library/psa_crypto_core.h index c90d7375a..53fb61a71 100644 --- a/library/psa_crypto_core.h +++ b/library/psa_crypto_core.h @@ -32,8 +32,6 @@ #include "psa/crypto.h" #include "psa/crypto_se_driver.h" -#include "mbedtls/ecp.h" - /** The data structure representing a key slot, containing key material * and metadata for one key. */ @@ -49,10 +47,6 @@ typedef struct uint8_t *data; size_t bytes; } key; -#if defined(MBEDTLS_ECP_C) - /* EC public key or key pair */ - mbedtls_ecp_keypair *ecp; -#endif /* MBEDTLS_ECP_C */ #if defined(MBEDTLS_PSA_CRYPTO_SE_C) /* Any key type in a secure element */ struct se From 560c28a1acfd8bc83cc90cd69cc2f709575cfa11 Mon Sep 17 00:00:00 2001 From: Steven Cooreman Date: Fri, 24 Jul 2020 23:20:24 +0200 Subject: [PATCH 05/16] Unify key handling logic Now that both ECP and RSA keys are represented in export representation, they can be treated more uniformly. Signed-off-by: Steven Cooreman --- library/psa_crypto.c | 184 ++++++++++++++----------------------------- 1 file changed, 60 insertions(+), 124 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index ee616956a..de5e8588d 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -905,24 +905,6 @@ static inline size_t psa_get_key_slot_bits( const psa_key_slot_t *slot ) return( slot->attr.bits ); } -/** Calculate the size of the key in the given slot, in bits. - * - * \param[in] slot A key slot containing a transparent key. - * - * \return The key size in bits, calculated from the key data. - */ -static psa_key_bits_t psa_calculate_key_bits( const psa_key_slot_t *slot ) -{ - size_t bits = 0; /* return 0 on an empty slot */ - - if( key_type_is_raw_bytes( slot->attr.type ) ) - bits = PSA_BYTES_TO_BITS( slot->data.key.bytes ); - - /* We know that the size fits in psa_key_bits_t thanks to checks - * when the key was created. */ - return( (psa_key_bits_t) bits ); -} - /** Import key data into a slot. `slot->attr.type` must have been set * previously. This function assumes that the slot does not contain * any key material yet. On failure, the slot content is unchanged. */ @@ -988,17 +970,6 @@ psa_status_t psa_import_key_into_slot( psa_key_slot_t *slot, return( PSA_ERROR_NOT_SUPPORTED ); } - if( status == PSA_SUCCESS ) - { - if( !PSA_KEY_TYPE_IS_RSA( slot->attr.type ) && - !PSA_KEY_TYPE_IS_ECC( slot->attr.type ) ) - { - /* Write the actual key size to the slot. - * psa_start_key_creation() wrote the size declared by the - * caller, which may be 0 (meaning unspecified) or wrong. */ - slot->attr.bits = psa_calculate_key_bits( slot ); - } - } return( status ); } @@ -1166,29 +1137,15 @@ static psa_status_t psa_remove_key_data_from_memory( psa_key_slot_t *slot ) { /* No key material to clean. */ } - else if( key_type_is_raw_bytes( slot->attr.type ) ) - { - mbedtls_free( slot->data.key.data ); - } - else -#if defined(MBEDTLS_RSA_C) - if( PSA_KEY_TYPE_IS_RSA( slot->attr.type ) ) + else if( key_type_is_raw_bytes( slot->attr.type ) || + PSA_KEY_TYPE_IS_RSA( slot->attr.type ) || + PSA_KEY_TYPE_IS_ECC( slot->attr.type ) ) { mbedtls_free( slot->data.key.data ); slot->data.key.data = NULL; slot->data.key.bytes = 0; } else -#endif /* defined(MBEDTLS_RSA_C) */ -#if defined(MBEDTLS_ECP_C) - if( PSA_KEY_TYPE_IS_ECC( slot->attr.type ) ) - { - mbedtls_free( slot->data.key.data ); - slot->data.key.data = NULL; - slot->data.key.bytes = 0; - } - else -#endif /* defined(MBEDTLS_ECP_C) */ { /* Shouldn't happen: the key type is not any type that we * put in. */ @@ -1522,94 +1479,78 @@ static psa_status_t psa_internal_export_key( const psa_key_slot_t *slot, if( key_type_is_raw_bytes( slot->attr.type ) ) { - if( slot->data.key.bytes > data_size ) - return( PSA_ERROR_BUFFER_TOO_SMALL ); - memcpy( data, slot->data.key.data, slot->data.key.bytes ); - memset( data + slot->data.key.bytes, 0, - data_size - slot->data.key.bytes ); - *data_length = slot->data.key.bytes; - return( PSA_SUCCESS ); - } -#if defined(MBEDTLS_ECP_C) - if( PSA_KEY_TYPE_IS_ECC_KEY_PAIR( slot->attr.type ) && !export_public_key ) - { - /* Exporting private -> private */ return( psa_internal_export_key_buffer( slot, data, data_size, data_length ) ); } -#endif - else + else if( PSA_KEY_TYPE_IS_RSA( slot->attr.type ) || + PSA_KEY_TYPE_IS_ECC( slot->attr.type ) ) { - if( PSA_KEY_TYPE_IS_RSA( slot->attr.type ) || - PSA_KEY_TYPE_IS_ECC( slot->attr.type ) ) + if( PSA_KEY_TYPE_IS_PUBLIC_KEY( slot->attr.type ) ) + { + /* Exporting public -> public */ + return( psa_internal_export_key_buffer( slot, data, data_size, data_length ) ); + } + else if( !export_public_key ) + { + /* Exporting private -> private */ + return( psa_internal_export_key_buffer( slot, data, data_size, data_length ) ); + } + /* Need to export the public part of a private key, + * so conversion is needed */ + if( PSA_KEY_TYPE_IS_RSA( slot->attr.type ) ) { - if( PSA_KEY_TYPE_IS_RSA( slot->attr.type ) ) - { #if defined(MBEDTLS_RSA_C) - if( PSA_KEY_TYPE_IS_PUBLIC_KEY( slot->attr.type ) ) - { - /* Exporting public -> public */ - return( psa_internal_export_key_buffer( slot, data, data_size, data_length ) ); - } - else if( !export_public_key ) - { - /* Exporting private -> private */ - return( psa_internal_export_key_buffer( slot, data, data_size, data_length ) ); - } + mbedtls_rsa_context rsa; + mbedtls_rsa_init( &rsa, MBEDTLS_RSA_PKCS_V15, MBEDTLS_MD_NONE ); - /* Exporting private -> public */ - mbedtls_rsa_context rsa; - mbedtls_rsa_init( &rsa, MBEDTLS_RSA_PKCS_V15, MBEDTLS_MD_NONE ); + psa_status_t status = psa_load_rsa_representation( slot, &rsa ); + if( status != PSA_SUCCESS ) + return status; - psa_status_t status = psa_load_rsa_representation( slot, &rsa ); - if( status != PSA_SUCCESS ) - return status; + status = psa_export_rsa_key( PSA_KEY_TYPE_RSA_PUBLIC_KEY, + &rsa, + data, + data_size, + data_length ); - status = psa_export_rsa_key( PSA_KEY_TYPE_RSA_PUBLIC_KEY, - &rsa, - data, - data_size, - data_length ); + mbedtls_rsa_free( &rsa ); - mbedtls_rsa_free( &rsa ); - - return( status ); + return( status ); #else - /* We don't know how to convert a private RSA key to public. */ - return( PSA_ERROR_NOT_SUPPORTED ); + /* We don't know how to convert a private RSA key to public. */ + return( PSA_ERROR_NOT_SUPPORTED ); #endif - } - else - { -#if defined(MBEDTLS_ECP_C) - mbedtls_ecp_keypair ecp; - psa_status_t status = psa_load_ecp_representation( slot, &ecp ); - if( status != PSA_SUCCESS ) - return status; - - status = psa_export_ecp_key( PSA_KEY_TYPE_ECC_PUBLIC_KEY( - PSA_KEY_TYPE_ECC_GET_FAMILY( - slot->attr.type ) ), - &ecp, - data, - data_size, - data_length ); - - mbedtls_ecp_keypair_free( &ecp ); - return( status ); -#else - /* We don't know how to convert a private ECC key to public. */ - return( PSA_ERROR_NOT_SUPPORTED ); -#endif /* defined(MBEDTLS_ECP_C) */ - } } else { - /* This shouldn't happen in the reference implementation, but - it is valid for a special-purpose implementation to omit - support for exporting certain key types. */ +#if defined(MBEDTLS_ECP_C) + mbedtls_ecp_keypair ecp; + psa_status_t status = psa_load_ecp_representation( slot, &ecp ); + if( status != PSA_SUCCESS ) + return status; + + status = psa_export_ecp_key( PSA_KEY_TYPE_ECC_PUBLIC_KEY( + PSA_KEY_TYPE_ECC_GET_FAMILY( + slot->attr.type ) ), + &ecp, + data, + data_size, + data_length ); + + mbedtls_ecp_keypair_free( &ecp ); + return( status ); +#else + /* We don't know how to convert a private ECC key to public */ return( PSA_ERROR_NOT_SUPPORTED ); +#endif } } + else + { + /* This shouldn't happen in the reference implementation, but + it is valid for a special-purpose implementation to omit + support for exporting certain key types. */ + return( PSA_ERROR_NOT_SUPPORTED ); + } } psa_status_t psa_export_key( psa_key_handle_t handle, @@ -5889,10 +5830,8 @@ static psa_status_t psa_generate_key_internal( &slot->data.key.bytes ); mbedtls_rsa_free( &rsa ); if( status != PSA_SUCCESS ) - { psa_remove_key_data_from_memory( slot ); - return( status ); - } + return( status ); } else #endif /* MBEDTLS_RSA_C && MBEDTLS_GENPRIME */ @@ -5938,11 +5877,8 @@ static psa_status_t psa_generate_key_internal( mbedtls_ecp_keypair_free( &ecp ); if( status != PSA_SUCCESS ) - { psa_remove_key_data_from_memory( slot ); - return( status ); - } - return( PSA_SUCCESS ); + return( status ); } else #endif /* MBEDTLS_ECP_C */ From 19fd574b3ac40819d8db6b0201e69f0f41661251 Mon Sep 17 00:00:00 2001 From: Steven Cooreman Date: Fri, 24 Jul 2020 23:31:01 +0200 Subject: [PATCH 06/16] Disconnect knowing about a PSA key type from knowing the mbedTLS API Signed-off-by: Steven Cooreman --- library/psa_crypto.c | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index de5e8588d..4a3877cc1 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -951,22 +951,31 @@ psa_status_t psa_import_key_into_slot( psa_key_slot_t *slot, * caller, which may be 0 (meaning unspecified) or wrong. */ slot->attr.bits = (psa_key_bits_t) bit_size; } - else + else if( PSA_KEY_TYPE_IS_ECC( slot->attr.type ) ) + { #if defined(MBEDTLS_ECP_C) - if( PSA_KEY_TYPE_IS_ECC( slot->attr.type ) ) + status = psa_import_ecp_key( slot, + data, data_length ); +#else + /* No drivers have been implemented yet, so without mbed TLS backing + * there's no way to do ECP with the current library. */ + return( PSA_ERROR_NOT_SUPPORTED ); +#endif /* defined(MBEDTLS_ECP_C) */ + } + else if( PSA_KEY_TYPE_IS_RSA( slot->attr.type ) ) { - status = psa_import_ecp_key( slot, data, data_length ); +#if defined(MBEDTLS_RSA_C) + status = psa_import_rsa_key( slot, + data, data_length ); +#else + /* No drivers have been implemented yet, so without mbed TLS backing + * there's no way to do RSA with the current library. */ + status = PSA_ERROR_NOT_SUPPORTED; +#endif /* defined(MBEDTLS_RSA_C) */ } else -#endif /* MBEDTLS_ECP_C */ -#if defined(MBEDTLS_RSA_C) && defined(MBEDTLS_PK_PARSE_C) - if( PSA_KEY_TYPE_IS_RSA( slot->attr.type ) ) - { - status = psa_import_rsa_key( slot, data, data_length ); - } - else -#endif /* defined(MBEDTLS_RSA_C) && defined(MBEDTLS_PK_PARSE_C) */ { + /* Unknown key type */ return( PSA_ERROR_NOT_SUPPORTED ); } From 75b743666eb13739720b5e36dcffeed6316ed22c Mon Sep 17 00:00:00 2001 From: Steven Cooreman Date: Tue, 28 Jul 2020 14:30:13 +0200 Subject: [PATCH 07/16] Update after feedback on #3492 * Updated wording * Split out buffer allocation to a convenience function * Moved variable declarations to beginning of their code block Signed-off-by: Steven Cooreman --- library/psa_crypto.c | 95 +++++++++++++++++++++++++++----------------- 1 file changed, 58 insertions(+), 37 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 4a3877cc1..34d48950f 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -605,8 +605,8 @@ static psa_status_t psa_export_rsa_key( psa_key_type_t type, pk.pk_ctx = rsa; /* PSA Crypto API defines the format of an RSA key as a DER-encoded - * representation of respectively the non-encrypted PKCS#1 RSAPrivateKey - * or the RFC3279 RSAPublicKey for a private key or a public key. */ + * representation of the non-encrypted PKCS#1 RSAPrivateKey for a + * private key and of the RFC3279 RSAPublicKey for a public key. */ if( PSA_KEY_TYPE_IS_KEY_PAIR( type ) ) ret = mbedtls_pk_write_key_der( &pk, data, data_size ); else @@ -670,8 +670,10 @@ static psa_status_t psa_import_rsa_key( psa_key_slot_t *slot, slot->attr.bits = (psa_key_bits_t) PSA_BYTES_TO_BITS( mbedtls_rsa_get_len( &rsa ) ); - /* Re-export the data to PSA export format, which in case of RSA is the - * smallest representation we can parse. */ + /* Re-export the data to PSA export format, such that we can store export + * representation in the key slot. Export representation in case of RSA is + * the smallest representation that's allowed as input, so a straight-up + * allocation of the same size as the input buffer will be large enough. */ output = mbedtls_calloc( 1, data_length ); if( output == NULL ) @@ -680,11 +682,6 @@ static psa_status_t psa_import_rsa_key( psa_key_slot_t *slot, goto exit; } - /* PSA Crypto API defines the format of an RSA key as a DER-encoded - * representation of respectively the non-encrypted PKCS#1 RSAPrivateKey - * or the RFC3279 RSAPublicKey for a private key or a public key. That - * means we have no other choice then to run an import to verify the key - * size. */ status = psa_export_rsa_key( slot->attr.type, &rsa, output, @@ -905,6 +902,32 @@ static inline size_t psa_get_key_slot_bits( const psa_key_slot_t *slot ) return( slot->attr.bits ); } +/** Try to allocate a buffer to an empty key slot. + * + * \param[in,out] slot Key slot to attach buffer to. + * \param[in] buffer_length Requested size of the buffer. + * + * \retval #PSA_SUCCESS + * The buffer has been successfully allocated. + * \retval #PSA_ERROR_INSUFFICIENT_MEMORY + * Not enough memory was available for allocation. + * \retval #PSA_ERROR_ALREADY_EXISTS + * Trying to allocate a buffer to a non-empty key slot. + */ +static psa_status_t psa_allocate_buffer_to_slot( psa_key_slot_t *slot, + size_t buffer_length ) +{ + if( slot->data.key.data != NULL ) + return PSA_ERROR_ALREADY_EXISTS; + + slot->data.key.data = mbedtls_calloc( 1, buffer_length ); + if( slot->data.key.data == NULL ) + return PSA_ERROR_INSUFFICIENT_MEMORY; + + slot->data.key.bytes = buffer_length; + return PSA_SUCCESS; +} + /** Import key data into a slot. `slot->attr.type` must have been set * previously. This function assumes that the slot does not contain * any key material yet. On failure, the slot content is unchanged. */ @@ -918,14 +941,14 @@ psa_status_t psa_import_key_into_slot( psa_key_slot_t *slot, if( data_length == 0 ) return( PSA_ERROR_NOT_SUPPORTED ); - /* Ensure that the bytes-to-bit conversion never overflows. */ - if( data_length > SIZE_MAX / 8 ) - return( PSA_ERROR_NOT_SUPPORTED ); - if( key_type_is_raw_bytes( slot->attr.type ) ) { size_t bit_size = PSA_BYTES_TO_BITS( data_length ); + /* Ensure that the bytes-to-bits conversion hasn't overflown. */ + if( data_length > SIZE_MAX / 8 ) + return( PSA_ERROR_NOT_SUPPORTED ); + /* Enforce a size limit, and in particular ensure that the bit * size fits in its representation type. */ if( bit_size > PSA_MAX_KEY_BITS ) @@ -936,12 +959,9 @@ psa_status_t psa_import_key_into_slot( psa_key_slot_t *slot, return status; /* Allocate memory for the key */ - slot->data.key.data = mbedtls_calloc( 1, data_length ); - if( slot->data.key.data == NULL ) - { - return( PSA_ERROR_INSUFFICIENT_MEMORY ); - } - slot->data.key.bytes = data_length; + status = psa_allocate_buffer_to_slot( slot, data_length ); + if( status != PSA_SUCCESS ) + return status; /* copy key into allocated buffer */ memcpy(slot->data.key.data, data, data_length); @@ -1135,6 +1155,10 @@ static psa_status_t psa_get_transparent_key( psa_key_handle_t handle, /** Wipe key data from a slot. Preserve metadata such as the policy. */ static psa_status_t psa_remove_key_data_from_memory( psa_key_slot_t *slot ) { + /* Check whether key is already clean */ + if( slot->data.key.data == NULL ) + return PSA_SUCCESS; + #if defined(MBEDTLS_PSA_CRYPTO_SE_C) if( psa_key_slot_is_external( slot ) ) { @@ -1958,11 +1982,12 @@ static psa_status_t psa_validate_optional_attributes( { mbedtls_rsa_context rsa; mbedtls_rsa_init( &rsa, MBEDTLS_RSA_PKCS_V15, MBEDTLS_MD_NONE ); + mbedtls_mpi actual, required; psa_status_t status = psa_load_rsa_representation( slot, &rsa ); if( status != PSA_SUCCESS ) return status; - mbedtls_mpi actual, required; + int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; mbedtls_mpi_init( &actual ); mbedtls_mpi_init( &required ); @@ -3808,11 +3833,11 @@ psa_status_t psa_asymmetric_encrypt( psa_key_handle_t handle, { mbedtls_rsa_context rsa; mbedtls_rsa_init( &rsa, MBEDTLS_RSA_PKCS_V15, MBEDTLS_MD_NONE ); + int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; status = psa_load_rsa_representation( slot, &rsa ); if( status != PSA_SUCCESS ) return status; - int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; if( output_size < mbedtls_rsa_get_len( &rsa ) ) { mbedtls_rsa_free( &rsa ); @@ -3898,11 +3923,11 @@ psa_status_t psa_asymmetric_decrypt( psa_key_handle_t handle, { mbedtls_rsa_context rsa; mbedtls_rsa_init( &rsa, MBEDTLS_RSA_PKCS_V15, MBEDTLS_MD_NONE ); + int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; status = psa_load_rsa_representation( slot, &rsa ); if( status != PSA_SUCCESS ) return status; - int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; if( input_length != mbedtls_rsa_get_len( &rsa ) ) { @@ -5773,13 +5798,9 @@ static psa_status_t psa_generate_key_internal( return( status ); /* Allocate memory for the key */ - slot->data.key.bytes = PSA_BITS_TO_BYTES( bits ); - slot->data.key.data = mbedtls_calloc( 1, slot->data.key.bytes ); - if( slot->data.key.data == NULL ) - { - slot->data.key.bytes = 0; - return( PSA_ERROR_INSUFFICIENT_MEMORY ); - } + status = psa_allocate_buffer_to_slot( slot, PSA_BITS_TO_BYTES( bits ) ); + if( status != PSA_SUCCESS ) + return status; status = psa_generate_random( slot->data.key.data, slot->data.key.bytes ); @@ -5825,11 +5846,11 @@ static psa_status_t psa_generate_key_internal( /* Make sure to always have an export representation available */ size_t bytes = PSA_KEY_EXPORT_RSA_KEY_PAIR_MAX_SIZE( bits ); - slot->data.key.data = mbedtls_calloc( 1, bytes ); - if( slot->data.key.data == NULL ) + status = psa_allocate_buffer_to_slot( slot, bytes ); + if( status != PSA_SUCCESS ) { mbedtls_rsa_free( &rsa ); - return( PSA_ERROR_INSUFFICIENT_MEMORY ); + return status; } status = psa_export_rsa_key( type, @@ -5874,14 +5895,14 @@ static psa_status_t psa_generate_key_internal( /* Make sure to always have an export representation available */ size_t bytes = PSA_BITS_TO_BYTES( bits ); - slot->data.key.data = mbedtls_calloc( 1, bytes ); - if( slot->data.key.data == NULL ) + psa_status_t status = psa_allocate_buffer_to_slot( slot, bytes ); + if( status != PSA_SUCCESS ) { mbedtls_ecp_keypair_free( &ecp ); - return( PSA_ERROR_INSUFFICIENT_MEMORY ); + return status; } - slot->data.key.bytes = bytes; - psa_status_t status = mbedtls_to_psa_error( + + status = mbedtls_to_psa_error( mbedtls_ecp_write_key( &ecp, slot->data.key.data, bytes ) ); mbedtls_ecp_keypair_free( &ecp ); From a2371e53e4146828de28855929b144fe89d47ed1 Mon Sep 17 00:00:00 2001 From: Steven Cooreman Date: Tue, 28 Jul 2020 14:30:39 +0200 Subject: [PATCH 08/16] Update after feedback from #3492 * Allocate internal representation contexts on the heap (i.e. don't change where they're being allocated) * Unify load_xxx_representation in terms of allocation and init behaviour Signed-off-by: Steven Cooreman --- library/psa_crypto.c | 190 +++++++++++++++++++++++++------------------ 1 file changed, 111 insertions(+), 79 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 34d48950f..15a612b68 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -520,14 +520,14 @@ static psa_status_t psa_check_rsa_key_byte_aligned( /** Load the contents of a key slot into an internal RSA representation * - * \param[in] slot The slot from which to load the representation - * \param[out] rsa The internal RSA representation to hold the key. Must be - * allocated and initialized. If it already holds a - * different key, it will be overwritten and cause a memory - * leak. + * \param[in] slot The slot from which to load the representation + * \param[out] p_rsa Returns a pointer to an RSA context on success. + * The caller is responsible for freeing both the + * contents of the context and the context itself + * when done. */ static psa_status_t psa_load_rsa_representation( const psa_key_slot_t *slot, - mbedtls_rsa_context *rsa ) + mbedtls_rsa_context **p_rsa ) { #if defined(MBEDTLS_PK_PARSE_C) psa_status_t status; @@ -567,9 +567,10 @@ static psa_status_t psa_load_rsa_representation( const psa_key_slot_t *slot, if( status != PSA_SUCCESS ) goto exit; - /* Copy the PK-contained RSA context to the one provided as function input */ - status = mbedtls_to_psa_error( - mbedtls_rsa_copy( rsa, mbedtls_pk_rsa( ctx ) ) ); + /* Copy out the pointer to the RSA context, and reset the PK context + * such that pk_free doesn't free the RSA context we just grabbed. */ + *p_rsa = mbedtls_pk_rsa( ctx ); + ctx.pk_info = NULL; exit: mbedtls_pk_free( &ctx ); @@ -653,8 +654,7 @@ static psa_status_t psa_import_rsa_key( psa_key_slot_t *slot, { psa_status_t status; uint8_t* output = NULL; - mbedtls_rsa_context rsa; - mbedtls_rsa_init( &rsa, MBEDTLS_RSA_PKCS_V15, MBEDTLS_MD_NONE ); + mbedtls_rsa_context *rsa = NULL; /* Temporarily load input into slot. The cast here is safe since it'll * only be used for load_rsa_representation, which doesn't modify the @@ -668,7 +668,7 @@ static psa_status_t psa_import_rsa_key( psa_key_slot_t *slot, goto exit; slot->attr.bits = (psa_key_bits_t) PSA_BYTES_TO_BITS( - mbedtls_rsa_get_len( &rsa ) ); + mbedtls_rsa_get_len( rsa ) ); /* Re-export the data to PSA export format, such that we can store export * representation in the key slot. Export representation in case of RSA is @@ -683,14 +683,16 @@ static psa_status_t psa_import_rsa_key( psa_key_slot_t *slot, } status = psa_export_rsa_key( slot->attr.type, - &rsa, + rsa, output, data_length, &data_length); exit: /* Always free the RSA object */ - mbedtls_rsa_free( &rsa ); + mbedtls_rsa_free( rsa ); + if( rsa != NULL ) + mbedtls_free( rsa ); /* Free the allocated buffer only on error. */ if( status != PSA_SUCCESS ) @@ -710,15 +712,24 @@ exit: #endif /* defined(MBEDTLS_RSA_C) */ #if defined(MBEDTLS_ECP_C) -/* Load the key slot contents into an mbedTLS internal representation object. - * Note: caller is responsible for freeing the object properly */ +/** Load the contents of a key slot into an internal ECP representation + * + * \param[in] slot The slot from which to load the representation + * \param[out] p_ecp Returns a pointer to an ECP context on success. + * The caller is responsible for freeing both the + * contents of the context and the context itself + * when done. + */ static psa_status_t psa_load_ecp_representation( const psa_key_slot_t *slot, - mbedtls_ecp_keypair *ecp ) + mbedtls_ecp_keypair **p_ecp ) { mbedtls_ecp_group_id grp_id = MBEDTLS_ECP_DP_NONE; size_t data_length = slot->data.key.bytes; psa_status_t status; - mbedtls_ecp_keypair_init( ecp ); + mbedtls_ecp_keypair *ecp = mbedtls_calloc(1, sizeof(mbedtls_ecp_keypair)); + + if( ecp == NULL ) + return PSA_ERROR_INSUFFICIENT_MEMORY; if( PSA_KEY_TYPE_IS_PUBLIC_KEY( slot->attr.type ) ) { @@ -733,6 +744,8 @@ static psa_status_t psa_load_ecp_representation( const psa_key_slot_t *slot, data_length = slot->data.key.bytes / 2; } + mbedtls_ecp_keypair_init( ecp ); + /* Load the group. */ grp_id = mbedtls_ecc_group_of_psa( PSA_KEY_TYPE_ECC_GET_FAMILY( slot->attr.type), data_length ); @@ -777,9 +790,15 @@ static psa_status_t psa_load_ecp_representation( const psa_key_slot_t *slot, if( status != PSA_SUCCESS ) goto exit; } + + *p_ecp = ecp; exit: if( status != PSA_SUCCESS ) + { mbedtls_ecp_keypair_free( ecp ); + mbedtls_free( ecp ); + } + return status; } @@ -835,7 +854,7 @@ static psa_status_t psa_import_ecp_key( psa_key_slot_t *slot, { psa_status_t status; uint8_t* output = NULL; - mbedtls_ecp_keypair ecp; + mbedtls_ecp_keypair *ecp = NULL; /* Temporarily load input into slot. The cast here is safe since it'll * only be used for load_ecp_representation, which doesn't modify the @@ -849,9 +868,9 @@ static psa_status_t psa_import_ecp_key( psa_key_slot_t *slot, goto exit; if( PSA_KEY_TYPE_ECC_GET_FAMILY( slot->attr.type ) == PSA_ECC_FAMILY_MONTGOMERY) - slot->attr.bits = (psa_key_bits_t) ecp.grp.nbits + 1; + slot->attr.bits = (psa_key_bits_t) ecp->grp.nbits + 1; else - slot->attr.bits = (psa_key_bits_t) ecp.grp.nbits; + slot->attr.bits = (psa_key_bits_t) ecp->grp.nbits; /* Re-export the data to PSA export format. There is currently no support * for other input formats then the export format, so this is a 1-1 @@ -865,14 +884,16 @@ static psa_status_t psa_import_ecp_key( psa_key_slot_t *slot, } status = psa_export_ecp_key( slot->attr.type, - &ecp, + ecp, output, data_length, &data_length); exit: - /* Always free the PK object (will also free contained RSA context) */ - mbedtls_ecp_keypair_free( &ecp ); + /* Always free the PK object (will also free contained ECP context) */ + mbedtls_ecp_keypair_free( ecp ); + if( ecp != NULL ) + mbedtls_free( ecp ); /* Free the allocated buffer only on error. */ if( status != PSA_SUCCESS ) @@ -1419,16 +1440,16 @@ psa_status_t psa_get_key_attributes( psa_key_handle_t handle, break; #endif /* MBEDTLS_PSA_CRYPTO_SE_C */ { - mbedtls_rsa_context rsa; - mbedtls_rsa_init( &rsa, MBEDTLS_RSA_PKCS_V15, MBEDTLS_MD_NONE ); + mbedtls_rsa_context *rsa = NULL; status = psa_load_rsa_representation( slot, &rsa ); if( status != PSA_SUCCESS ) break; - status = psa_get_rsa_public_exponent( &rsa, + status = psa_get_rsa_public_exponent( rsa, attributes ); - mbedtls_rsa_free( &rsa ); + mbedtls_rsa_free( rsa ); + mbedtls_free( rsa ); } break; #endif /* MBEDTLS_RSA_C */ @@ -1532,20 +1553,19 @@ static psa_status_t psa_internal_export_key( const psa_key_slot_t *slot, if( PSA_KEY_TYPE_IS_RSA( slot->attr.type ) ) { #if defined(MBEDTLS_RSA_C) - mbedtls_rsa_context rsa; - mbedtls_rsa_init( &rsa, MBEDTLS_RSA_PKCS_V15, MBEDTLS_MD_NONE ); - + mbedtls_rsa_context *rsa = NULL; psa_status_t status = psa_load_rsa_representation( slot, &rsa ); if( status != PSA_SUCCESS ) return status; status = psa_export_rsa_key( PSA_KEY_TYPE_RSA_PUBLIC_KEY, - &rsa, + rsa, data, data_size, data_length ); - mbedtls_rsa_free( &rsa ); + mbedtls_rsa_free( rsa ); + mbedtls_free( rsa ); return( status ); #else @@ -1556,7 +1576,7 @@ static psa_status_t psa_internal_export_key( const psa_key_slot_t *slot, else { #if defined(MBEDTLS_ECP_C) - mbedtls_ecp_keypair ecp; + mbedtls_ecp_keypair *ecp = NULL; psa_status_t status = psa_load_ecp_representation( slot, &ecp ); if( status != PSA_SUCCESS ) return status; @@ -1564,12 +1584,13 @@ static psa_status_t psa_internal_export_key( const psa_key_slot_t *slot, status = psa_export_ecp_key( PSA_KEY_TYPE_ECC_PUBLIC_KEY( PSA_KEY_TYPE_ECC_GET_FAMILY( slot->attr.type ) ), - &ecp, + ecp, data, data_size, data_length ); - mbedtls_ecp_keypair_free( &ecp ); + mbedtls_ecp_keypair_free( ecp ); + mbedtls_free( ecp ); return( status ); #else /* We don't know how to convert a private ECC key to public */ @@ -1980,8 +2001,7 @@ static psa_status_t psa_validate_optional_attributes( #if defined(MBEDTLS_RSA_C) if( PSA_KEY_TYPE_IS_RSA( slot->attr.type ) ) { - mbedtls_rsa_context rsa; - mbedtls_rsa_init( &rsa, MBEDTLS_RSA_PKCS_V15, MBEDTLS_MD_NONE ); + mbedtls_rsa_context *rsa = NULL; mbedtls_mpi actual, required; psa_status_t status = psa_load_rsa_representation( slot, &rsa ); @@ -1991,9 +2011,10 @@ static psa_status_t psa_validate_optional_attributes( int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; mbedtls_mpi_init( &actual ); mbedtls_mpi_init( &required ); - ret = mbedtls_rsa_export( &rsa, + ret = mbedtls_rsa_export( rsa, NULL, NULL, NULL, NULL, &actual ); - mbedtls_rsa_free( &rsa ); + mbedtls_rsa_free( rsa ); + mbedtls_free( rsa ); if( ret != 0 ) goto rsa_exit; ret = mbedtls_mpi_read_binary( &required, @@ -3638,21 +3659,21 @@ psa_status_t psa_sign_hash( psa_key_handle_t handle, #if defined(MBEDTLS_RSA_C) if( slot->attr.type == PSA_KEY_TYPE_RSA_KEY_PAIR ) { - mbedtls_rsa_context rsa; - mbedtls_rsa_init( &rsa, MBEDTLS_RSA_PKCS_V15, MBEDTLS_MD_NONE ); + mbedtls_rsa_context *rsa = NULL; status = psa_load_rsa_representation( slot, &rsa ); if( status != PSA_SUCCESS ) goto exit; - status = psa_rsa_sign( &rsa, + status = psa_rsa_sign( rsa, alg, hash, hash_length, signature, signature_size, signature_length ); - mbedtls_rsa_free( &rsa ); + mbedtls_rsa_free( rsa ); + mbedtls_free( rsa ); } else #endif /* defined(MBEDTLS_RSA_C) */ @@ -3668,16 +3689,17 @@ psa_status_t psa_sign_hash( psa_key_handle_t handle, #endif ) { - mbedtls_ecp_keypair ecp; + mbedtls_ecp_keypair *ecp = NULL; status = psa_load_ecp_representation( slot, &ecp ); if( status != PSA_SUCCESS ) goto exit; - status = psa_ecdsa_sign( &ecp, + status = psa_ecdsa_sign( ecp, alg, hash, hash_length, signature, signature_size, signature_length ); - mbedtls_ecp_keypair_free( &ecp ); + mbedtls_ecp_keypair_free( ecp ); + mbedtls_free( ecp ); } else #endif /* defined(MBEDTLS_ECDSA_C) */ @@ -3741,18 +3763,18 @@ psa_status_t psa_verify_hash( psa_key_handle_t handle, #if defined(MBEDTLS_RSA_C) if( PSA_KEY_TYPE_IS_RSA( slot->attr.type ) ) { - mbedtls_rsa_context rsa; - mbedtls_rsa_init( &rsa, MBEDTLS_RSA_PKCS_V15, MBEDTLS_MD_NONE ); + mbedtls_rsa_context *rsa = NULL; status = psa_load_rsa_representation( slot, &rsa ); if( status != PSA_SUCCESS ) return status; - status = psa_rsa_verify( &rsa, + status = psa_rsa_verify( rsa, alg, hash, hash_length, signature, signature_length ); - mbedtls_rsa_free( &rsa ); + mbedtls_rsa_free( rsa ); + mbedtls_free( rsa ); return( status ); } else @@ -3763,14 +3785,15 @@ psa_status_t psa_verify_hash( psa_key_handle_t handle, #if defined(MBEDTLS_ECDSA_C) if( PSA_ALG_IS_ECDSA( alg ) ) { - mbedtls_ecp_keypair ecp; + mbedtls_ecp_keypair *ecp = NULL; status = psa_load_ecp_representation( slot, &ecp ); if( status != PSA_SUCCESS ) return status; - status = psa_ecdsa_verify( &ecp, + status = psa_ecdsa_verify( ecp, hash, hash_length, signature, signature_length ); - mbedtls_ecp_keypair_free( &ecp ); + mbedtls_ecp_keypair_free( ecp ); + mbedtls_free( ecp ); return status; } else @@ -3831,22 +3854,23 @@ psa_status_t psa_asymmetric_encrypt( psa_key_handle_t handle, #if defined(MBEDTLS_RSA_C) if( PSA_KEY_TYPE_IS_RSA( slot->attr.type ) ) { - mbedtls_rsa_context rsa; - mbedtls_rsa_init( &rsa, MBEDTLS_RSA_PKCS_V15, MBEDTLS_MD_NONE ); + mbedtls_rsa_context *rsa = NULL; int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; status = psa_load_rsa_representation( slot, &rsa ); if( status != PSA_SUCCESS ) return status; - if( output_size < mbedtls_rsa_get_len( &rsa ) ) + + if( output_size < mbedtls_rsa_get_len( rsa ) ) { - mbedtls_rsa_free( &rsa ); + mbedtls_rsa_free( rsa ); + mbedtls_free( rsa ); return( PSA_ERROR_BUFFER_TOO_SMALL ); } #if defined(MBEDTLS_PKCS1_V15) if( alg == PSA_ALG_RSA_PKCS1V15_CRYPT ) { - ret = mbedtls_rsa_pkcs1_encrypt( &rsa, + ret = mbedtls_rsa_pkcs1_encrypt( rsa, mbedtls_ctr_drbg_random, &global_data.ctr_drbg, MBEDTLS_RSA_PUBLIC, @@ -3859,8 +3883,8 @@ psa_status_t psa_asymmetric_encrypt( psa_key_handle_t handle, #if defined(MBEDTLS_PKCS1_V21) if( PSA_ALG_IS_RSA_OAEP( alg ) ) { - psa_rsa_oaep_set_padding_mode( alg, &rsa ); - ret = mbedtls_rsa_rsaes_oaep_encrypt( &rsa, + psa_rsa_oaep_set_padding_mode( alg, rsa ); + ret = mbedtls_rsa_rsaes_oaep_encrypt( rsa, mbedtls_ctr_drbg_random, &global_data.ctr_drbg, MBEDTLS_RSA_PUBLIC, @@ -3872,13 +3896,15 @@ psa_status_t psa_asymmetric_encrypt( psa_key_handle_t handle, else #endif /* MBEDTLS_PKCS1_V21 */ { - mbedtls_rsa_free( &rsa ); + mbedtls_rsa_free( rsa ); + mbedtls_free( rsa ); return( PSA_ERROR_INVALID_ARGUMENT ); } if( ret == 0 ) - *output_length = mbedtls_rsa_get_len( &rsa ); + *output_length = mbedtls_rsa_get_len( rsa ); - mbedtls_rsa_free( &rsa ); + mbedtls_rsa_free( rsa ); + mbedtls_free( rsa ); return( mbedtls_to_psa_error( ret ) ); } else @@ -3921,24 +3947,24 @@ psa_status_t psa_asymmetric_decrypt( psa_key_handle_t handle, #if defined(MBEDTLS_RSA_C) if( slot->attr.type == PSA_KEY_TYPE_RSA_KEY_PAIR ) { - mbedtls_rsa_context rsa; - mbedtls_rsa_init( &rsa, MBEDTLS_RSA_PKCS_V15, MBEDTLS_MD_NONE ); + mbedtls_rsa_context *rsa; int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; status = psa_load_rsa_representation( slot, &rsa ); if( status != PSA_SUCCESS ) return status; - if( input_length != mbedtls_rsa_get_len( &rsa ) ) + if( input_length != mbedtls_rsa_get_len( rsa ) ) { - mbedtls_rsa_free( &rsa ); + mbedtls_rsa_free( rsa ); + mbedtls_free( rsa ); return( PSA_ERROR_INVALID_ARGUMENT ); } #if defined(MBEDTLS_PKCS1_V15) if( alg == PSA_ALG_RSA_PKCS1V15_CRYPT ) { - ret = mbedtls_rsa_pkcs1_decrypt( &rsa, + ret = mbedtls_rsa_pkcs1_decrypt( rsa, mbedtls_ctr_drbg_random, &global_data.ctr_drbg, MBEDTLS_RSA_PRIVATE, @@ -3952,8 +3978,8 @@ psa_status_t psa_asymmetric_decrypt( psa_key_handle_t handle, #if defined(MBEDTLS_PKCS1_V21) if( PSA_ALG_IS_RSA_OAEP( alg ) ) { - psa_rsa_oaep_set_padding_mode( alg, &rsa ); - ret = mbedtls_rsa_rsaes_oaep_decrypt( &rsa, + psa_rsa_oaep_set_padding_mode( alg, rsa ); + ret = mbedtls_rsa_rsaes_oaep_decrypt( rsa, mbedtls_ctr_drbg_random, &global_data.ctr_drbg, MBEDTLS_RSA_PRIVATE, @@ -3966,11 +3992,13 @@ psa_status_t psa_asymmetric_decrypt( psa_key_handle_t handle, else #endif /* MBEDTLS_PKCS1_V21 */ { - mbedtls_rsa_free( &rsa ); + mbedtls_rsa_free( rsa ); + mbedtls_free( rsa ); return( PSA_ERROR_INVALID_ARGUMENT ); } - mbedtls_rsa_free( &rsa ); + mbedtls_rsa_free( rsa ); + mbedtls_free( rsa ); return( mbedtls_to_psa_error( ret ) ); } else @@ -5521,7 +5549,7 @@ static psa_status_t psa_key_agreement_ecdh( const uint8_t *peer_key, size_t shared_secret_size, size_t *shared_secret_length ) { - mbedtls_ecp_keypair their_key; + mbedtls_ecp_keypair *their_key; psa_key_slot_t their_key_slot; mbedtls_ecdh_context ecdh; psa_status_t status; @@ -5540,7 +5568,7 @@ static psa_status_t psa_key_agreement_ecdh( const uint8_t *peer_key, goto exit; status = mbedtls_to_psa_error( - mbedtls_ecdh_get_params( &ecdh, &their_key, MBEDTLS_ECDH_THEIRS ) ); + mbedtls_ecdh_get_params( &ecdh, their_key, MBEDTLS_ECDH_THEIRS ) ); if( status != PSA_SUCCESS ) goto exit; status = mbedtls_to_psa_error( @@ -5563,7 +5591,10 @@ exit: if( status != PSA_SUCCESS ) mbedtls_platform_zeroize( shared_secret, shared_secret_size ); mbedtls_ecdh_free( &ecdh ); - mbedtls_ecp_keypair_free( &their_key ); + mbedtls_ecp_keypair_free( their_key ); + if( their_key != NULL) + mbedtls_free( their_key ); + return( status ); } #endif /* MBEDTLS_ECDH_C */ @@ -5584,15 +5615,16 @@ static psa_status_t psa_key_agreement_raw_internal( psa_algorithm_t alg, case PSA_ALG_ECDH: if( ! PSA_KEY_TYPE_IS_ECC_KEY_PAIR( private_key->attr.type ) ) return( PSA_ERROR_INVALID_ARGUMENT ); - mbedtls_ecp_keypair ecp; + mbedtls_ecp_keypair *ecp = NULL; psa_status_t status = psa_load_ecp_representation( private_key, &ecp ); if( status != PSA_SUCCESS ) return status; status = psa_key_agreement_ecdh( peer_key, peer_key_length, - &ecp, + ecp, shared_secret, shared_secret_size, shared_secret_length ); - mbedtls_ecp_keypair_free( &ecp ); + mbedtls_ecp_keypair_free( ecp ); + mbedtls_free( ecp ); return status; #endif /* MBEDTLS_ECDH_C */ default: From 6d839f05bf1404c8d424bdf9a858e7d521f262f3 Mon Sep 17 00:00:00 2001 From: Steven Cooreman Date: Thu, 30 Jul 2020 11:36:45 +0200 Subject: [PATCH 09/16] Cleanup * No null-check before calling free * Close memory leak * No need for double check of privkey validity Signed-off-by: Steven Cooreman --- library/psa_crypto.c | 39 ++++++++++++++++++--------------------- 1 file changed, 18 insertions(+), 21 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 15a612b68..b6a34dfd3 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -691,8 +691,7 @@ static psa_status_t psa_import_rsa_key( psa_key_slot_t *slot, exit: /* Always free the RSA object */ mbedtls_rsa_free( rsa ); - if( rsa != NULL ) - mbedtls_free( rsa ); + mbedtls_free( rsa ); /* Free the allocated buffer only on error. */ if( status != PSA_SUCCESS ) @@ -726,10 +725,7 @@ static psa_status_t psa_load_ecp_representation( const psa_key_slot_t *slot, mbedtls_ecp_group_id grp_id = MBEDTLS_ECP_DP_NONE; size_t data_length = slot->data.key.bytes; psa_status_t status; - mbedtls_ecp_keypair *ecp = mbedtls_calloc(1, sizeof(mbedtls_ecp_keypair)); - - if( ecp == NULL ) - return PSA_ERROR_INSUFFICIENT_MEMORY; + mbedtls_ecp_keypair *ecp = NULL; if( PSA_KEY_TYPE_IS_PUBLIC_KEY( slot->attr.type ) ) { @@ -744,19 +740,27 @@ static psa_status_t psa_load_ecp_representation( const psa_key_slot_t *slot, data_length = slot->data.key.bytes / 2; } + /* Allocate and initialize a key representation. */ + ecp = mbedtls_calloc(1, sizeof(mbedtls_ecp_keypair)); + if( ecp == NULL ) + return PSA_ERROR_INSUFFICIENT_MEMORY; mbedtls_ecp_keypair_init( ecp ); /* Load the group. */ - grp_id = mbedtls_ecc_group_of_psa( PSA_KEY_TYPE_ECC_GET_FAMILY( slot->attr.type), + grp_id = mbedtls_ecc_group_of_psa( PSA_KEY_TYPE_ECC_GET_FAMILY( slot->attr.type ), data_length ); if( grp_id == MBEDTLS_ECP_DP_NONE ) - return( PSA_ERROR_INVALID_ARGUMENT ); + { + status = PSA_ERROR_INVALID_ARGUMENT; + goto exit; + } + status = mbedtls_to_psa_error( mbedtls_ecp_group_load( &ecp->grp, grp_id ) ); if( status != PSA_SUCCESS ) - return( status ); + goto exit; - /* Load the key material */ + /* Load the key material. */ if( PSA_KEY_TYPE_IS_PUBLIC_KEY( slot->attr.type ) ) { /* Load the public value. */ @@ -775,7 +779,7 @@ static psa_status_t psa_load_ecp_representation( const psa_key_slot_t *slot, } else { - /* Load the secret value. */ + /* Load and validate the secret value. */ status = mbedtls_to_psa_error( mbedtls_ecp_read_key( ecp->grp.id, ecp, @@ -784,11 +788,6 @@ static psa_status_t psa_load_ecp_representation( const psa_key_slot_t *slot, if( status != PSA_SUCCESS ) goto exit; - /* Validate the private key. */ - status = mbedtls_to_psa_error( - mbedtls_ecp_check_privkey( &ecp->grp, &ecp->d ) ); - if( status != PSA_SUCCESS ) - goto exit; } *p_ecp = ecp; @@ -892,8 +891,7 @@ static psa_status_t psa_import_ecp_key( psa_key_slot_t *slot, exit: /* Always free the PK object (will also free contained ECP context) */ mbedtls_ecp_keypair_free( ecp ); - if( ecp != NULL ) - mbedtls_free( ecp ); + mbedtls_free( ecp ); /* Free the allocated buffer only on error. */ if( status != PSA_SUCCESS ) @@ -2003,12 +2001,12 @@ static psa_status_t psa_validate_optional_attributes( { mbedtls_rsa_context *rsa = NULL; mbedtls_mpi actual, required; + int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; psa_status_t status = psa_load_rsa_representation( slot, &rsa ); if( status != PSA_SUCCESS ) return status; - int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; mbedtls_mpi_init( &actual ); mbedtls_mpi_init( &required ); ret = mbedtls_rsa_export( rsa, @@ -5592,8 +5590,7 @@ exit: mbedtls_platform_zeroize( shared_secret, shared_secret_size ); mbedtls_ecdh_free( &ecdh ); mbedtls_ecp_keypair_free( their_key ); - if( their_key != NULL) - mbedtls_free( their_key ); + mbedtls_free( their_key ); return( status ); } From 7f39187d6b6fcca0775e303d3509fff36a451c7e Mon Sep 17 00:00:00 2001 From: Steven Cooreman Date: Thu, 30 Jul 2020 14:57:44 +0200 Subject: [PATCH 10/16] Convert load_xxx_representation to take buffers instead of a whole slot Avoids stack-allocating a key slot during ECDH, and mock-attaching a key to a key slot during key import. Signed-off-by: Steven Cooreman --- library/psa_crypto.c | 144 +++++++++++++++++++++++++++---------------- 1 file changed, 90 insertions(+), 54 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index b6a34dfd3..6e9d25961 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -518,15 +518,19 @@ static psa_status_t psa_check_rsa_key_byte_aligned( } #endif /* MBEDTLS_PK_PARSE_C */ -/** Load the contents of a key slot into an internal RSA representation +/** Load the contents of a key buffer into an internal RSA representation * - * \param[in] slot The slot from which to load the representation + * \param[in] buffer The buffer from which to load the representation. + * \param[in] size The size in bytes of \p buffer. + * \param[in] type The type of key contained in the \p buffer. * \param[out] p_rsa Returns a pointer to an RSA context on success. * The caller is responsible for freeing both the * contents of the context and the context itself * when done. */ -static psa_status_t psa_load_rsa_representation( const psa_key_slot_t *slot, +static psa_status_t psa_load_rsa_representation( const uint8_t *buffer, + size_t size, + psa_key_type_t type, mbedtls_rsa_context **p_rsa ) { #if defined(MBEDTLS_PK_PARSE_C) @@ -536,12 +540,12 @@ static psa_status_t psa_load_rsa_representation( const psa_key_slot_t *slot, mbedtls_pk_init( &ctx ); /* Parse the data. */ - if( PSA_KEY_TYPE_IS_KEY_PAIR( slot->attr.type ) ) + if( PSA_KEY_TYPE_IS_KEY_PAIR( type ) ) status = mbedtls_to_psa_error( - mbedtls_pk_parse_key( &ctx, slot->data.key.data, slot->data.key.bytes, NULL, 0 ) ); + mbedtls_pk_parse_key( &ctx, buffer, size, NULL, 0 ) ); else status = mbedtls_to_psa_error( - mbedtls_pk_parse_public_key( &ctx, slot->data.key.data, slot->data.key.bytes ) ); + mbedtls_pk_parse_public_key( &ctx, buffer, size ) ); if( status != PSA_SUCCESS ) goto exit; @@ -576,7 +580,9 @@ exit: mbedtls_pk_free( &ctx ); return( status ); #else - (void) slot; + (void) buffer; + (void) size; + (void) type; (void) rsa; return( PSA_ERROR_NOT_SUPPORTED ); #endif /* MBEDTLS_PK_PARSE_C */ @@ -656,14 +662,11 @@ static psa_status_t psa_import_rsa_key( psa_key_slot_t *slot, uint8_t* output = NULL; mbedtls_rsa_context *rsa = NULL; - /* Temporarily load input into slot. The cast here is safe since it'll - * only be used for load_rsa_representation, which doesn't modify the - * buffer. */ - slot->data.key.data = (uint8_t *)data; - slot->data.key.bytes = data_length; - /* Parse input */ - status = psa_load_rsa_representation( slot, &rsa ); + status = psa_load_rsa_representation( data, + data_length, + slot->attr.type, + &rsa ); if( status != PSA_SUCCESS ) goto exit; @@ -711,23 +714,27 @@ exit: #endif /* defined(MBEDTLS_RSA_C) */ #if defined(MBEDTLS_ECP_C) -/** Load the contents of a key slot into an internal ECP representation +/** Load the contents of a key buffer into an internal ECP representation * - * \param[in] slot The slot from which to load the representation + * \param[in] buffer The buffer from which to load the representation. + * \param[in] size The size in bytes of \p buffer. + * \param[in] type The type of key contained in the \p buffer. * \param[out] p_ecp Returns a pointer to an ECP context on success. * The caller is responsible for freeing both the * contents of the context and the context itself * when done. */ -static psa_status_t psa_load_ecp_representation( const psa_key_slot_t *slot, +static psa_status_t psa_load_ecp_representation( const uint8_t *buffer, + size_t size, + psa_key_type_t type, mbedtls_ecp_keypair **p_ecp ) { mbedtls_ecp_group_id grp_id = MBEDTLS_ECP_DP_NONE; - size_t data_length = slot->data.key.bytes; psa_status_t status; mbedtls_ecp_keypair *ecp = NULL; + size_t curve_size = size; - if( PSA_KEY_TYPE_IS_PUBLIC_KEY( slot->attr.type ) ) + if( PSA_KEY_TYPE_IS_PUBLIC_KEY( type ) ) { /* A public key is represented as: * - The byte 0x04; @@ -735,9 +742,9 @@ static psa_status_t psa_load_ecp_representation( const psa_key_slot_t *slot, * - `y_P` as a `ceiling(m/8)`-byte string, big-endian. * So its data length is 2m+1 where n is the key size in bits. */ - if( ( slot->data.key.bytes & 1 ) == 0 ) + if( ( size & 1 ) == 0 ) return( PSA_ERROR_INVALID_ARGUMENT ); - data_length = slot->data.key.bytes / 2; + curve_size = size / 2; } /* Allocate and initialize a key representation. */ @@ -747,8 +754,8 @@ static psa_status_t psa_load_ecp_representation( const psa_key_slot_t *slot, mbedtls_ecp_keypair_init( ecp ); /* Load the group. */ - grp_id = mbedtls_ecc_group_of_psa( PSA_KEY_TYPE_ECC_GET_FAMILY( slot->attr.type ), - data_length ); + grp_id = mbedtls_ecc_group_of_psa( PSA_KEY_TYPE_ECC_GET_FAMILY( type ), + curve_size ); if( grp_id == MBEDTLS_ECP_DP_NONE ) { status = PSA_ERROR_INVALID_ARGUMENT; @@ -761,13 +768,13 @@ static psa_status_t psa_load_ecp_representation( const psa_key_slot_t *slot, goto exit; /* Load the key material. */ - if( PSA_KEY_TYPE_IS_PUBLIC_KEY( slot->attr.type ) ) + if( PSA_KEY_TYPE_IS_PUBLIC_KEY( type ) ) { /* Load the public value. */ status = mbedtls_to_psa_error( mbedtls_ecp_point_read_binary( &ecp->grp, &ecp->Q, - slot->data.key.data, - slot->data.key.bytes ) ); + buffer, + size ) ); if( status != PSA_SUCCESS ) goto exit; @@ -783,8 +790,8 @@ static psa_status_t psa_load_ecp_representation( const psa_key_slot_t *slot, status = mbedtls_to_psa_error( mbedtls_ecp_read_key( ecp->grp.id, ecp, - slot->data.key.data, - slot->data.key.bytes ) ); + buffer, + size ) ); if( status != PSA_SUCCESS ) goto exit; @@ -855,14 +862,11 @@ static psa_status_t psa_import_ecp_key( psa_key_slot_t *slot, uint8_t* output = NULL; mbedtls_ecp_keypair *ecp = NULL; - /* Temporarily load input into slot. The cast here is safe since it'll - * only be used for load_ecp_representation, which doesn't modify the - * buffer. */ - slot->data.key.data = (uint8_t *)data; - slot->data.key.bytes = data_length; - /* Parse input */ - status = psa_load_ecp_representation( slot, &ecp ); + status = psa_load_ecp_representation( data, + data_length, + slot->attr.type, + &ecp ); if( status != PSA_SUCCESS ) goto exit; @@ -1440,7 +1444,10 @@ psa_status_t psa_get_key_attributes( psa_key_handle_t handle, { mbedtls_rsa_context *rsa = NULL; - status = psa_load_rsa_representation( slot, &rsa ); + status = psa_load_rsa_representation( slot->data.key.data, + slot->data.key.bytes, + slot->attr.type, + &rsa ); if( status != PSA_SUCCESS ) break; @@ -1552,7 +1559,11 @@ static psa_status_t psa_internal_export_key( const psa_key_slot_t *slot, { #if defined(MBEDTLS_RSA_C) mbedtls_rsa_context *rsa = NULL; - psa_status_t status = psa_load_rsa_representation( slot, &rsa ); + psa_status_t status = psa_load_rsa_representation( + slot->data.key.data, + slot->data.key.bytes, + slot->attr.type, + &rsa ); if( status != PSA_SUCCESS ) return status; @@ -1575,7 +1586,11 @@ static psa_status_t psa_internal_export_key( const psa_key_slot_t *slot, { #if defined(MBEDTLS_ECP_C) mbedtls_ecp_keypair *ecp = NULL; - psa_status_t status = psa_load_ecp_representation( slot, &ecp ); + psa_status_t status = psa_load_ecp_representation( + slot->data.key.data, + slot->data.key.bytes, + slot->attr.type, + &ecp ); if( status != PSA_SUCCESS ) return status; @@ -2003,7 +2018,11 @@ static psa_status_t psa_validate_optional_attributes( mbedtls_mpi actual, required; int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; - psa_status_t status = psa_load_rsa_representation( slot, &rsa ); + psa_status_t status = psa_load_rsa_representation( + slot->data.key.data, + slot->data.key.bytes, + slot->attr.type, + &rsa ); if( status != PSA_SUCCESS ) return status; @@ -3659,7 +3678,9 @@ psa_status_t psa_sign_hash( psa_key_handle_t handle, { mbedtls_rsa_context *rsa = NULL; - status = psa_load_rsa_representation( slot, + status = psa_load_rsa_representation( slot->data.key.data, + slot->data.key.bytes, + slot->attr.type, &rsa ); if( status != PSA_SUCCESS ) goto exit; @@ -3688,7 +3709,10 @@ psa_status_t psa_sign_hash( psa_key_handle_t handle, ) { mbedtls_ecp_keypair *ecp = NULL; - status = psa_load_ecp_representation( slot, &ecp ); + status = psa_load_ecp_representation( slot->data.key.data, + slot->data.key.bytes, + slot->attr.type, + &ecp ); if( status != PSA_SUCCESS ) goto exit; status = psa_ecdsa_sign( ecp, @@ -3763,7 +3787,10 @@ psa_status_t psa_verify_hash( psa_key_handle_t handle, { mbedtls_rsa_context *rsa = NULL; - status = psa_load_rsa_representation( slot, &rsa ); + status = psa_load_rsa_representation( slot->data.key.data, + slot->data.key.bytes, + slot->attr.type, + &rsa ); if( status != PSA_SUCCESS ) return status; @@ -3784,7 +3811,10 @@ psa_status_t psa_verify_hash( psa_key_handle_t handle, if( PSA_ALG_IS_ECDSA( alg ) ) { mbedtls_ecp_keypair *ecp = NULL; - status = psa_load_ecp_representation( slot, &ecp ); + status = psa_load_ecp_representation( slot->data.key.data, + slot->data.key.bytes, + slot->attr.type, + &ecp ); if( status != PSA_SUCCESS ) return status; status = psa_ecdsa_verify( ecp, @@ -3855,7 +3885,10 @@ psa_status_t psa_asymmetric_encrypt( psa_key_handle_t handle, mbedtls_rsa_context *rsa = NULL; int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; - status = psa_load_rsa_representation( slot, &rsa ); + status = psa_load_rsa_representation( slot->data.key.data, + slot->data.key.bytes, + slot->attr.type, + &rsa ); if( status != PSA_SUCCESS ) return status; @@ -3948,7 +3981,10 @@ psa_status_t psa_asymmetric_decrypt( psa_key_handle_t handle, mbedtls_rsa_context *rsa; int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; - status = psa_load_rsa_representation( slot, &rsa ); + status = psa_load_rsa_representation( slot->data.key.data, + slot->data.key.bytes, + slot->attr.type, + &rsa ); if( status != PSA_SUCCESS ) return status; @@ -5548,20 +5584,16 @@ static psa_status_t psa_key_agreement_ecdh( const uint8_t *peer_key, size_t *shared_secret_length ) { mbedtls_ecp_keypair *their_key; - psa_key_slot_t their_key_slot; mbedtls_ecdh_context ecdh; psa_status_t status; size_t bits = 0; psa_ecc_family_t curve = mbedtls_ecc_group_to_psa( our_key->grp.id, &bits ); mbedtls_ecdh_init( &ecdh ); - memset(&their_key_slot, 0, sizeof(their_key_slot)); - /* Creating ephemeral key slot for import purposes */ - their_key_slot.attr.type = PSA_KEY_TYPE_ECC_PUBLIC_KEY(curve); - their_key_slot.data.key.data = (uint8_t*) peer_key; - their_key_slot.data.key.bytes = peer_key_length; - - status = psa_load_ecp_representation( &their_key_slot, &their_key ); + status = psa_load_ecp_representation( peer_key, + peer_key_length, + PSA_KEY_TYPE_ECC_PUBLIC_KEY(curve), + &their_key ); if( status != PSA_SUCCESS ) goto exit; @@ -5613,7 +5645,11 @@ static psa_status_t psa_key_agreement_raw_internal( psa_algorithm_t alg, if( ! PSA_KEY_TYPE_IS_ECC_KEY_PAIR( private_key->attr.type ) ) return( PSA_ERROR_INVALID_ARGUMENT ); mbedtls_ecp_keypair *ecp = NULL; - psa_status_t status = psa_load_ecp_representation( private_key, &ecp ); + psa_status_t status = psa_load_ecp_representation( + private_key->data.key.data, + private_key->data.key.bytes, + private_key->attr.type, + &ecp ); if( status != PSA_SUCCESS ) return status; status = psa_key_agreement_ecdh( peer_key, peer_key_length, From 3fa684ed918e29bc4083553d3369218c87b9a636 Mon Sep 17 00:00:00 2001 From: Steven Cooreman Date: Thu, 30 Jul 2020 15:04:07 +0200 Subject: [PATCH 11/16] Allow importing Montgomery public keys in PSA Crypto PSA Crypto was checking the byte length of a to-be-imported public ECP key against the expected length for Weierstrass keys, forgetting that Curve25519/Curve448 exists. Signed-off-by: Steven Cooreman --- .../psa_curve25519_public_key_import.txt | 3 ++ library/psa_crypto.c | 35 +++++++++++++------ tests/suites/test_suite_psa_crypto.data | 4 +++ tests/suites/test_suite_psa_crypto.function | 25 ++++++++----- 4 files changed, 49 insertions(+), 18 deletions(-) create mode 100644 ChangeLog.d/psa_curve25519_public_key_import.txt diff --git a/ChangeLog.d/psa_curve25519_public_key_import.txt b/ChangeLog.d/psa_curve25519_public_key_import.txt new file mode 100644 index 000000000..2ea11e2c8 --- /dev/null +++ b/ChangeLog.d/psa_curve25519_public_key_import.txt @@ -0,0 +1,3 @@ +Bugfix + * PSA key import will now correctly import a Curve25519/Curve448 public key + instead of erroring out. Contributed by Steven Cooreman in #3492. diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 6e9d25961..056e67796 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -732,19 +732,34 @@ static psa_status_t psa_load_ecp_representation( const uint8_t *buffer, mbedtls_ecp_group_id grp_id = MBEDTLS_ECP_DP_NONE; psa_status_t status; mbedtls_ecp_keypair *ecp = NULL; - size_t curve_size = size; + size_t curve_size; if( PSA_KEY_TYPE_IS_PUBLIC_KEY( type ) ) { - /* A public key is represented as: - * - The byte 0x04; - * - `x_P` as a `ceiling(m/8)`-byte string, big-endian; - * - `y_P` as a `ceiling(m/8)`-byte string, big-endian. - * So its data length is 2m+1 where n is the key size in bits. - */ - if( ( size & 1 ) == 0 ) - return( PSA_ERROR_INVALID_ARGUMENT ); - curve_size = size / 2; + if( PSA_KEY_TYPE_ECC_GET_FAMILY( type ) == PSA_ECC_FAMILY_MONTGOMERY ) + { + /* A Montgomery public key is represented as its raw + * compressed public point. + */ + curve_size = size; + } + else + { + /* A Weierstrass public key is represented as: + * - The byte 0x04; + * - `x_P` as a `ceiling(m/8)`-byte string, big-endian; + * - `y_P` as a `ceiling(m/8)`-byte string, big-endian. + * So its data length is 2m+1 where n is the key size in bits. + */ + if( ( size & 1 ) == 0 ) + return( PSA_ERROR_INVALID_ARGUMENT ); + curve_size = size / 2; + } + } + else + { + /* Private keys are represented as the raw private value */ + curve_size = size; } /* Allocate and initialize a key representation. */ diff --git a/tests/suites/test_suite_psa_crypto.data b/tests/suites/test_suite_psa_crypto.data index 6a2859124..d1855fdb5 100644 --- a/tests/suites/test_suite_psa_crypto.data +++ b/tests/suites/test_suite_psa_crypto.data @@ -252,6 +252,10 @@ PSA import/export EC brainpoolP256r1 public key: good depends_on:MBEDTLS_PK_PARSE_C:MBEDTLS_PK_WRITE_C:MBEDTLS_ECP_C:MBEDTLS_ECP_DP_BP256R1_ENABLED import_export:"04768c8cae4abca6306db0ed81b0c4a6215c378066ec6d616c146e13f1c7df809b96ab6911c27d8a02339f0926840e55236d3d1efbe2669d090e4c4c660fada91d":PSA_KEY_TYPE_ECC_PUBLIC_KEY(PSA_ECC_FAMILY_BRAINPOOL_P_R1):PSA_KEY_USAGE_EXPORT:PSA_ALG_ECDSA_ANY:256:0:PSA_SUCCESS:1 +PSA import/export curve25519 public key: good +depends_on:MBEDTLS_PK_PARSE_C:MBEDTLS_PK_WRITE_C:MBEDTLS_ECP_C:MBEDTLS_ECP_DP_CURVE25519_ENABLED +import_export:"8520f0098930a754748b7ddcb43ef75a0dbf3a0d26381af4eba4a98eaa9b4e6a":PSA_KEY_TYPE_ECC_PUBLIC_KEY(PSA_ECC_FAMILY_MONTGOMERY):PSA_KEY_USAGE_EXPORT:PSA_ALG_ECDH:255:0:PSA_SUCCESS:1 + PSA import/export AES key: policy forbids export depends_on:MBEDTLS_AES_C:MBEDTLS_CIPHER_MODE_CTR import_export:"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa":PSA_KEY_TYPE_AES:PSA_KEY_USAGE_ENCRYPT | PSA_KEY_USAGE_DECRYPT:PSA_ALG_CTR:128:0:PSA_ERROR_NOT_PERMITTED:1 diff --git a/tests/suites/test_suite_psa_crypto.function b/tests/suites/test_suite_psa_crypto.function index 4576b8bb4..f4b9a8f67 100644 --- a/tests/suites/test_suite_psa_crypto.function +++ b/tests/suites/test_suite_psa_crypto.function @@ -961,14 +961,23 @@ static int exported_key_sanity_check( psa_key_type_t type, size_t bits, #if defined(MBEDTLS_ECP_C) if( PSA_KEY_TYPE_IS_ECC_PUBLIC_KEY( type ) ) { - /* The representation of an ECC public key is: - * - The byte 0x04; - * - `x_P` as a `ceiling(m/8)`-byte string, big-endian; - * - `y_P` as a `ceiling(m/8)`-byte string, big-endian; - * - where m is the bit size associated with the curve. - */ - TEST_EQUAL( p + 1 + 2 * PSA_BITS_TO_BYTES( bits ), end ); - TEST_EQUAL( p[0], 4 ); + if( PSA_KEY_TYPE_ECC_GET_FAMILY( type ) == PSA_ECC_FAMILY_MONTGOMERY ) + { + /* The representation of an ECC Montgomery public key is + * the raw compressed point */ + TEST_EQUAL( p + PSA_BITS_TO_BYTES( bits ), end ); + } + else + { + /* The representation of an ECC Weierstrass public key is: + * - The byte 0x04; + * - `x_P` as a `ceiling(m/8)`-byte string, big-endian; + * - `y_P` as a `ceiling(m/8)`-byte string, big-endian; + * - where m is the bit size associated with the curve. + */ + TEST_EQUAL( p + 1 + 2 * PSA_BITS_TO_BYTES( bits ), end ); + TEST_EQUAL( p[0], 4 ); + } } else #endif /* MBEDTLS_ECP_C */ From 4fed4553473e5f2ed154bcd90f5a937f6fa31922 Mon Sep 17 00:00:00 2001 From: Steven Cooreman Date: Mon, 3 Aug 2020 14:46:03 +0200 Subject: [PATCH 12/16] Apply review feedback * No need to check for NULL before free'ing * No need to reset variables that weren't touched * Set output buffer to zero if key output fails * Document internal functions and rearrange order of input arguments to better match other functions. * Clean up Montgomery fix to be less verbose code Signed-off-by: Steven Cooreman --- library/psa_crypto.c | 286 ++++++++++++++++++++++--------------------- 1 file changed, 145 insertions(+), 141 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 056e67796..dfd37ae4e 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -520,17 +520,17 @@ static psa_status_t psa_check_rsa_key_byte_aligned( /** Load the contents of a key buffer into an internal RSA representation * - * \param[in] buffer The buffer from which to load the representation. - * \param[in] size The size in bytes of \p buffer. - * \param[in] type The type of key contained in the \p buffer. - * \param[out] p_rsa Returns a pointer to an RSA context on success. - * The caller is responsible for freeing both the - * contents of the context and the context itself - * when done. + * \param[in] type The type of key contained in \p data. + * \param[in] data The buffer from which to load the representation. + * \param[in] data_length The size in bytes of \p data. + * \param[out] p_rsa Returns a pointer to an RSA context on success. + * The caller is responsible for freeing both the + * contents of the context and the context itself + * when done. */ -static psa_status_t psa_load_rsa_representation( const uint8_t *buffer, - size_t size, - psa_key_type_t type, +static psa_status_t psa_load_rsa_representation( psa_key_type_t type, + const uint8_t *data, + size_t data_length, mbedtls_rsa_context **p_rsa ) { #if defined(MBEDTLS_PK_PARSE_C) @@ -542,10 +542,10 @@ static psa_status_t psa_load_rsa_representation( const uint8_t *buffer, /* Parse the data. */ if( PSA_KEY_TYPE_IS_KEY_PAIR( type ) ) status = mbedtls_to_psa_error( - mbedtls_pk_parse_key( &ctx, buffer, size, NULL, 0 ) ); + mbedtls_pk_parse_key( &ctx, data, data_length, NULL, 0 ) ); else status = mbedtls_to_psa_error( - mbedtls_pk_parse_public_key( &ctx, buffer, size ) ); + mbedtls_pk_parse_public_key( &ctx, data, data_length ) ); if( status != PSA_SUCCESS ) goto exit; @@ -580,8 +580,8 @@ exit: mbedtls_pk_free( &ctx ); return( status ); #else - (void) buffer; - (void) size; + (void) data; + (void) data_length; (void) type; (void) rsa; return( PSA_ERROR_NOT_SUPPORTED ); @@ -620,7 +620,11 @@ static psa_status_t psa_export_rsa_key( psa_key_type_t type, ret = mbedtls_pk_write_pubkey( &pos, data, &pk ); if( ret < 0 ) + { + /* Clean up in case pk_write failed halfway through. */ + memset( data, 0, data_size ); return mbedtls_to_psa_error( ret ); + } /* The mbedtls_pk_xxx functions write to the end of the buffer. * Move the data to the beginning and erase remaining data @@ -663,9 +667,9 @@ static psa_status_t psa_import_rsa_key( psa_key_slot_t *slot, mbedtls_rsa_context *rsa = NULL; /* Parse input */ - status = psa_load_rsa_representation( data, + status = psa_load_rsa_representation( slot->attr.type, + data, data_length, - slot->attr.type, &rsa ); if( status != PSA_SUCCESS ) goto exit; @@ -700,8 +704,6 @@ exit: if( status != PSA_SUCCESS ) { mbedtls_free( output ); - slot->data.key.data = NULL; - slot->data.key.bytes = 0; return( status ); } @@ -716,50 +718,42 @@ exit: #if defined(MBEDTLS_ECP_C) /** Load the contents of a key buffer into an internal ECP representation * - * \param[in] buffer The buffer from which to load the representation. - * \param[in] size The size in bytes of \p buffer. - * \param[in] type The type of key contained in the \p buffer. - * \param[out] p_ecp Returns a pointer to an ECP context on success. - * The caller is responsible for freeing both the - * contents of the context and the context itself - * when done. + * \param[in] type The type of key contained in \p data. + * \param[in] data The buffer from which to load the representation. + * \param[in] data_length The size in bytes of \p data. + * \param[out] p_ecp Returns a pointer to an ECP context on success. + * The caller is responsible for freeing both the + * contents of the context and the context itself + * when done. */ -static psa_status_t psa_load_ecp_representation( const uint8_t *buffer, - size_t size, - psa_key_type_t type, +static psa_status_t psa_load_ecp_representation( psa_key_type_t type, + const uint8_t *data, + size_t data_length, mbedtls_ecp_keypair **p_ecp ) { mbedtls_ecp_group_id grp_id = MBEDTLS_ECP_DP_NONE; psa_status_t status; mbedtls_ecp_keypair *ecp = NULL; - size_t curve_size; + size_t curve_size = data_length; - if( PSA_KEY_TYPE_IS_PUBLIC_KEY( type ) ) + if( PSA_KEY_TYPE_IS_PUBLIC_KEY( type ) && + PSA_KEY_TYPE_ECC_GET_FAMILY( type ) != PSA_ECC_FAMILY_MONTGOMERY ) { - if( PSA_KEY_TYPE_ECC_GET_FAMILY( type ) == PSA_ECC_FAMILY_MONTGOMERY ) - { - /* A Montgomery public key is represented as its raw - * compressed public point. - */ - curve_size = size; - } - else - { - /* A Weierstrass public key is represented as: - * - The byte 0x04; - * - `x_P` as a `ceiling(m/8)`-byte string, big-endian; - * - `y_P` as a `ceiling(m/8)`-byte string, big-endian. - * So its data length is 2m+1 where n is the key size in bits. - */ - if( ( size & 1 ) == 0 ) - return( PSA_ERROR_INVALID_ARGUMENT ); - curve_size = size / 2; - } - } - else - { - /* Private keys are represented as the raw private value */ - curve_size = size; + /* A Weierstrass public key is represented as: + * - The byte 0x04; + * - `x_P` as a `ceiling(m/8)`-byte string, big-endian; + * - `y_P` as a `ceiling(m/8)`-byte string, big-endian. + * So its data length is 2m+1 where n is the key size in bits. + */ + if( ( data_length & 1 ) == 0 ) + return( PSA_ERROR_INVALID_ARGUMENT ); + curve_size = data_length / 2; + + /* Montgomery public keys are represented in compressed format, meaning + * their curve_size is equal to the amount of input. */ + + /* Private keys are represented in uncompressed private random integer + * format, meaning their curve_size is equal to the amount of input. */ } /* Allocate and initialize a key representation. */ @@ -788,8 +782,8 @@ static psa_status_t psa_load_ecp_representation( const uint8_t *buffer, /* Load the public value. */ status = mbedtls_to_psa_error( mbedtls_ecp_point_read_binary( &ecp->grp, &ecp->Q, - buffer, - size ) ); + data, + data_length ) ); if( status != PSA_SUCCESS ) goto exit; @@ -805,8 +799,8 @@ static psa_status_t psa_load_ecp_representation( const uint8_t *buffer, status = mbedtls_to_psa_error( mbedtls_ecp_read_key( ecp->grp.id, ecp, - buffer, - size ) ); + data, + data_length ) ); if( status != PSA_SUCCESS ) goto exit; @@ -823,6 +817,14 @@ exit: return status; } +/** Export an ECP key to export representation + * + * \param[in] type The type of key (public/private) to export + * \param[in] ecp The internal ECP representation from which to export + * \param[out] data The buffer to export to + * \param[in] data_size The length of the buffer to export to + * \param[out] data_length The amount of bytes written to \p data + */ static psa_status_t psa_export_ecp_key( psa_key_type_t type, mbedtls_ecp_keypair *ecp, uint8_t *data, @@ -844,12 +846,17 @@ static psa_status_t psa_export_ecp_key( psa_key_type_t type, return status; } - return( mbedtls_to_psa_error( + status = mbedtls_to_psa_error( mbedtls_ecp_point_write_binary( &ecp->grp, &ecp->Q, MBEDTLS_ECP_PF_UNCOMPRESSED, data_length, data, - data_size ) ) ); + data_size ) ); + + if( status != PSA_SUCCESS ) + memset( data, 0, data_size ); + + return status; } else { @@ -869,6 +876,12 @@ static psa_status_t psa_export_ecp_key( psa_key_type_t type, } } +/** Import an ECP key from import representation to a slot + * + * \param[in,out] slot The slot where to store the export representation to + * \param[in] data The buffer containing the import representation + * \param[in] data_length The amount of bytes in \p data + */ static psa_status_t psa_import_ecp_key( psa_key_slot_t *slot, const uint8_t *data, size_t data_length ) @@ -878,9 +891,9 @@ static psa_status_t psa_import_ecp_key( psa_key_slot_t *slot, mbedtls_ecp_keypair *ecp = NULL; /* Parse input */ - status = psa_load_ecp_representation( data, + status = psa_load_ecp_representation( slot->attr.type, + data, data_length, - slot->attr.type, &ecp ); if( status != PSA_SUCCESS ) goto exit; @@ -916,8 +929,6 @@ exit: if( status != PSA_SUCCESS ) { mbedtls_free( output ); - slot->data.key.data = NULL; - slot->data.key.bytes = 0; return( status ); } @@ -1193,10 +1204,6 @@ static psa_status_t psa_get_transparent_key( psa_key_handle_t handle, /** Wipe key data from a slot. Preserve metadata such as the policy. */ static psa_status_t psa_remove_key_data_from_memory( psa_key_slot_t *slot ) { - /* Check whether key is already clean */ - if( slot->data.key.data == NULL ) - return PSA_SUCCESS; - #if defined(MBEDTLS_PSA_CRYPTO_SE_C) if( psa_key_slot_is_external( slot ) ) { @@ -1459,9 +1466,9 @@ psa_status_t psa_get_key_attributes( psa_key_handle_t handle, { mbedtls_rsa_context *rsa = NULL; - status = psa_load_rsa_representation( slot->data.key.data, + status = psa_load_rsa_representation( slot->attr.type, + slot->data.key.data, slot->data.key.bytes, - slot->attr.type, &rsa ); if( status != PSA_SUCCESS ) break; @@ -1575,9 +1582,9 @@ static psa_status_t psa_internal_export_key( const psa_key_slot_t *slot, #if defined(MBEDTLS_RSA_C) mbedtls_rsa_context *rsa = NULL; psa_status_t status = psa_load_rsa_representation( + slot->attr.type, slot->data.key.data, slot->data.key.bytes, - slot->attr.type, &rsa ); if( status != PSA_SUCCESS ) return status; @@ -1602,9 +1609,9 @@ static psa_status_t psa_internal_export_key( const psa_key_slot_t *slot, #if defined(MBEDTLS_ECP_C) mbedtls_ecp_keypair *ecp = NULL; psa_status_t status = psa_load_ecp_representation( + slot->attr.type, slot->data.key.data, slot->data.key.bytes, - slot->attr.type, &ecp ); if( status != PSA_SUCCESS ) return status; @@ -2034,9 +2041,9 @@ static psa_status_t psa_validate_optional_attributes( int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; psa_status_t status = psa_load_rsa_representation( + slot->attr.type, slot->data.key.data, slot->data.key.bytes, - slot->attr.type, &rsa ); if( status != PSA_SUCCESS ) return status; @@ -3693,9 +3700,9 @@ psa_status_t psa_sign_hash( psa_key_handle_t handle, { mbedtls_rsa_context *rsa = NULL; - status = psa_load_rsa_representation( slot->data.key.data, + status = psa_load_rsa_representation( slot->attr.type, + slot->data.key.data, slot->data.key.bytes, - slot->attr.type, &rsa ); if( status != PSA_SUCCESS ) goto exit; @@ -3724,9 +3731,9 @@ psa_status_t psa_sign_hash( psa_key_handle_t handle, ) { mbedtls_ecp_keypair *ecp = NULL; - status = psa_load_ecp_representation( slot->data.key.data, + status = psa_load_ecp_representation( slot->attr.type, + slot->data.key.data, slot->data.key.bytes, - slot->attr.type, &ecp ); if( status != PSA_SUCCESS ) goto exit; @@ -3802,9 +3809,9 @@ psa_status_t psa_verify_hash( psa_key_handle_t handle, { mbedtls_rsa_context *rsa = NULL; - status = psa_load_rsa_representation( slot->data.key.data, + status = psa_load_rsa_representation( slot->attr.type, + slot->data.key.data, slot->data.key.bytes, - slot->attr.type, &rsa ); if( status != PSA_SUCCESS ) return status; @@ -3826,9 +3833,9 @@ psa_status_t psa_verify_hash( psa_key_handle_t handle, if( PSA_ALG_IS_ECDSA( alg ) ) { mbedtls_ecp_keypair *ecp = NULL; - status = psa_load_ecp_representation( slot->data.key.data, + status = psa_load_ecp_representation( slot->attr.type, + slot->data.key.data, slot->data.key.bytes, - slot->attr.type, &ecp ); if( status != PSA_SUCCESS ) return status; @@ -3898,31 +3905,29 @@ psa_status_t psa_asymmetric_encrypt( psa_key_handle_t handle, if( PSA_KEY_TYPE_IS_RSA( slot->attr.type ) ) { mbedtls_rsa_context *rsa = NULL; - int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; - - status = psa_load_rsa_representation( slot->data.key.data, + status = psa_load_rsa_representation( slot->attr.type, + slot->data.key.data, slot->data.key.bytes, - slot->attr.type, &rsa ); if( status != PSA_SUCCESS ) - return status; + goto rsa_exit; if( output_size < mbedtls_rsa_get_len( rsa ) ) { - mbedtls_rsa_free( rsa ); - mbedtls_free( rsa ); - return( PSA_ERROR_BUFFER_TOO_SMALL ); + status = PSA_ERROR_BUFFER_TOO_SMALL; + goto rsa_exit; } #if defined(MBEDTLS_PKCS1_V15) if( alg == PSA_ALG_RSA_PKCS1V15_CRYPT ) { - ret = mbedtls_rsa_pkcs1_encrypt( rsa, - mbedtls_ctr_drbg_random, - &global_data.ctr_drbg, - MBEDTLS_RSA_PUBLIC, - input_length, - input, - output ); + status = mbedtls_to_psa_error( + mbedtls_rsa_pkcs1_encrypt( rsa, + mbedtls_ctr_drbg_random, + &global_data.ctr_drbg, + MBEDTLS_RSA_PUBLIC, + input_length, + input, + output ) ); } else #endif /* MBEDTLS_PKCS1_V15 */ @@ -3930,28 +3935,29 @@ psa_status_t psa_asymmetric_encrypt( psa_key_handle_t handle, if( PSA_ALG_IS_RSA_OAEP( alg ) ) { psa_rsa_oaep_set_padding_mode( alg, rsa ); - ret = mbedtls_rsa_rsaes_oaep_encrypt( rsa, - mbedtls_ctr_drbg_random, - &global_data.ctr_drbg, - MBEDTLS_RSA_PUBLIC, - salt, salt_length, - input_length, - input, - output ); + status = mbedtls_to_psa_error( + mbedtls_rsa_rsaes_oaep_encrypt( rsa, + mbedtls_ctr_drbg_random, + &global_data.ctr_drbg, + MBEDTLS_RSA_PUBLIC, + salt, salt_length, + input_length, + input, + output ) ); } else #endif /* MBEDTLS_PKCS1_V21 */ { - mbedtls_rsa_free( rsa ); - mbedtls_free( rsa ); - return( PSA_ERROR_INVALID_ARGUMENT ); + status = PSA_ERROR_INVALID_ARGUMENT; + goto rsa_exit; } - if( ret == 0 ) +rsa_exit: + if( status == PSA_SUCCESS ) *output_length = mbedtls_rsa_get_len( rsa ); mbedtls_rsa_free( rsa ); mbedtls_free( rsa ); - return( mbedtls_to_psa_error( ret ) ); + return( status ); } else #endif /* defined(MBEDTLS_RSA_C) */ @@ -3993,34 +3999,32 @@ psa_status_t psa_asymmetric_decrypt( psa_key_handle_t handle, #if defined(MBEDTLS_RSA_C) if( slot->attr.type == PSA_KEY_TYPE_RSA_KEY_PAIR ) { - mbedtls_rsa_context *rsa; - int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; - - status = psa_load_rsa_representation( slot->data.key.data, + mbedtls_rsa_context *rsa = NULL; + status = psa_load_rsa_representation( slot->attr.type, + slot->data.key.data, slot->data.key.bytes, - slot->attr.type, &rsa ); if( status != PSA_SUCCESS ) return status; if( input_length != mbedtls_rsa_get_len( rsa ) ) { - mbedtls_rsa_free( rsa ); - mbedtls_free( rsa ); - return( PSA_ERROR_INVALID_ARGUMENT ); + status = PSA_ERROR_INVALID_ARGUMENT; + goto rsa_exit; } #if defined(MBEDTLS_PKCS1_V15) if( alg == PSA_ALG_RSA_PKCS1V15_CRYPT ) { - ret = mbedtls_rsa_pkcs1_decrypt( rsa, - mbedtls_ctr_drbg_random, - &global_data.ctr_drbg, - MBEDTLS_RSA_PRIVATE, - output_length, - input, - output, - output_size ); + status = mbedtls_to_psa_error( + mbedtls_rsa_pkcs1_decrypt( rsa, + mbedtls_ctr_drbg_random, + &global_data.ctr_drbg, + MBEDTLS_RSA_PRIVATE, + output_length, + input, + output, + output_size ) ); } else #endif /* MBEDTLS_PKCS1_V15 */ @@ -4028,27 +4032,27 @@ psa_status_t psa_asymmetric_decrypt( psa_key_handle_t handle, if( PSA_ALG_IS_RSA_OAEP( alg ) ) { psa_rsa_oaep_set_padding_mode( alg, rsa ); - ret = mbedtls_rsa_rsaes_oaep_decrypt( rsa, - mbedtls_ctr_drbg_random, - &global_data.ctr_drbg, - MBEDTLS_RSA_PRIVATE, - salt, salt_length, - output_length, - input, - output, - output_size ); + status = mbedtls_to_psa_error( + mbedtls_rsa_rsaes_oaep_decrypt( rsa, + mbedtls_ctr_drbg_random, + &global_data.ctr_drbg, + MBEDTLS_RSA_PRIVATE, + salt, salt_length, + output_length, + input, + output, + output_size ) ); } else #endif /* MBEDTLS_PKCS1_V21 */ { - mbedtls_rsa_free( rsa ); - mbedtls_free( rsa ); - return( PSA_ERROR_INVALID_ARGUMENT ); + status = PSA_ERROR_INVALID_ARGUMENT; } +rsa_exit: mbedtls_rsa_free( rsa ); mbedtls_free( rsa ); - return( mbedtls_to_psa_error( ret ) ); + return( status ); } else #endif /* defined(MBEDTLS_RSA_C) */ @@ -5605,9 +5609,9 @@ static psa_status_t psa_key_agreement_ecdh( const uint8_t *peer_key, psa_ecc_family_t curve = mbedtls_ecc_group_to_psa( our_key->grp.id, &bits ); mbedtls_ecdh_init( &ecdh ); - status = psa_load_ecp_representation( peer_key, + status = psa_load_ecp_representation( PSA_KEY_TYPE_ECC_PUBLIC_KEY(curve), + peer_key, peer_key_length, - PSA_KEY_TYPE_ECC_PUBLIC_KEY(curve), &their_key ); if( status != PSA_SUCCESS ) goto exit; @@ -5661,9 +5665,9 @@ static psa_status_t psa_key_agreement_raw_internal( psa_algorithm_t alg, return( PSA_ERROR_INVALID_ARGUMENT ); mbedtls_ecp_keypair *ecp = NULL; psa_status_t status = psa_load_ecp_representation( + private_key->attr.type, private_key->data.key.data, private_key->data.key.bytes, - private_key->attr.type, &ecp ); if( status != PSA_SUCCESS ) return status; From 291498600b0a768c26033c6416cd251a12ad8c43 Mon Sep 17 00:00:00 2001 From: Steven Cooreman Date: Wed, 5 Aug 2020 15:43:42 +0200 Subject: [PATCH 13/16] Style fixes * return is treated as a function call * space between opening and closing parentheses * remove whiteline between assignment and checking of same variable Signed-off-by: Steven Cooreman --- library/psa_crypto.c | 70 ++++++++++++++++++++------------------------ 1 file changed, 32 insertions(+), 38 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index dfd37ae4e..12f05d135 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -567,7 +567,6 @@ static psa_status_t psa_load_rsa_representation( psa_key_type_t type, goto exit; } status = psa_check_rsa_key_byte_aligned( mbedtls_pk_rsa( ctx ) ); - if( status != PSA_SUCCESS ) goto exit; @@ -623,7 +622,7 @@ static psa_status_t psa_export_rsa_key( psa_key_type_t type, { /* Clean up in case pk_write failed halfway through. */ memset( data, 0, data_size ); - return mbedtls_to_psa_error( ret ); + return( mbedtls_to_psa_error( ret ) ); } /* The mbedtls_pk_xxx functions write to the end of the buffer. @@ -682,7 +681,6 @@ static psa_status_t psa_import_rsa_key( psa_key_slot_t *slot, * the smallest representation that's allowed as input, so a straight-up * allocation of the same size as the input buffer will be large enough. */ output = mbedtls_calloc( 1, data_length ); - if( output == NULL ) { status = PSA_ERROR_INSUFFICIENT_MEMORY; @@ -694,7 +692,6 @@ static psa_status_t psa_import_rsa_key( psa_key_slot_t *slot, output, data_length, &data_length); - exit: /* Always free the RSA object */ mbedtls_rsa_free( rsa ); @@ -757,9 +754,9 @@ static psa_status_t psa_load_ecp_representation( psa_key_type_t type, } /* Allocate and initialize a key representation. */ - ecp = mbedtls_calloc(1, sizeof(mbedtls_ecp_keypair)); + ecp = mbedtls_calloc( 1, sizeof( mbedtls_ecp_keypair ) ); if( ecp == NULL ) - return PSA_ERROR_INSUFFICIENT_MEMORY; + return( PSA_ERROR_INSUFFICIENT_MEMORY ); mbedtls_ecp_keypair_init( ecp ); /* Load the group. */ @@ -801,7 +798,6 @@ static psa_status_t psa_load_ecp_representation( psa_key_type_t type, ecp, data, data_length ) ); - if( status != PSA_SUCCESS ) goto exit; } @@ -814,7 +810,7 @@ exit: mbedtls_free( ecp ); } - return status; + return( status ); } /** Export an ECP key to export representation @@ -843,7 +839,7 @@ static psa_status_t psa_export_ecp_key( psa_key_type_t type, mbedtls_ecp_mul( &ecp->grp, &ecp->Q, &ecp->d, &ecp->grp.G, mbedtls_ctr_drbg_random, &global_data.ctr_drbg ) ); if( status != PSA_SUCCESS ) - return status; + return( status ); } status = mbedtls_to_psa_error( @@ -852,24 +848,23 @@ static psa_status_t psa_export_ecp_key( psa_key_type_t type, data_length, data, data_size ) ); - if( status != PSA_SUCCESS ) memset( data, 0, data_size ); - return status; + return( status ); } else { - if( data_size < PSA_BITS_TO_BYTES(ecp->grp.nbits) ) + if( data_size < PSA_BITS_TO_BYTES( ecp->grp.nbits ) ) return( PSA_ERROR_BUFFER_TOO_SMALL ); status = mbedtls_to_psa_error( mbedtls_ecp_write_key( ecp, data, - PSA_BITS_TO_BYTES(ecp->grp.nbits) ) ); + PSA_BITS_TO_BYTES( ecp->grp.nbits ) ) ); if( status == PSA_SUCCESS ) { - *data_length = PSA_BITS_TO_BYTES(ecp->grp.nbits); + *data_length = PSA_BITS_TO_BYTES( ecp->grp.nbits ); } return( status ); @@ -907,7 +902,6 @@ static psa_status_t psa_import_ecp_key( psa_key_slot_t *slot, * for other input formats then the export format, so this is a 1-1 * copy operation. */ output = mbedtls_calloc( 1, data_length ); - if( output == NULL ) { status = PSA_ERROR_INSUFFICIENT_MEMORY; @@ -919,7 +913,6 @@ static psa_status_t psa_import_ecp_key( psa_key_slot_t *slot, output, data_length, &data_length); - exit: /* Always free the PK object (will also free contained ECP context) */ mbedtls_ecp_keypair_free( ecp ); @@ -967,14 +960,14 @@ static psa_status_t psa_allocate_buffer_to_slot( psa_key_slot_t *slot, size_t buffer_length ) { if( slot->data.key.data != NULL ) - return PSA_ERROR_ALREADY_EXISTS; + return( PSA_ERROR_ALREADY_EXISTS ); slot->data.key.data = mbedtls_calloc( 1, buffer_length ); if( slot->data.key.data == NULL ) - return PSA_ERROR_INSUFFICIENT_MEMORY; + return( PSA_ERROR_INSUFFICIENT_MEMORY ); slot->data.key.bytes = buffer_length; - return PSA_SUCCESS; + return( PSA_SUCCESS ); } /** Import key data into a slot. `slot->attr.type` must have been set @@ -1005,15 +998,15 @@ psa_status_t psa_import_key_into_slot( psa_key_slot_t *slot, status = validate_unstructured_key_bit_size( slot->attr.type, bit_size ); if( status != PSA_SUCCESS ) - return status; + return( status ); /* Allocate memory for the key */ status = psa_allocate_buffer_to_slot( slot, data_length ); if( status != PSA_SUCCESS ) - return status; + return( status ); /* copy key into allocated buffer */ - memcpy(slot->data.key.data, data, data_length); + memcpy( slot->data.key.data, data, data_length ); /* Write the actual key size to the slot. * psa_start_key_creation() wrote the size declared by the @@ -1587,7 +1580,7 @@ static psa_status_t psa_internal_export_key( const psa_key_slot_t *slot, slot->data.key.bytes, &rsa ); if( status != PSA_SUCCESS ) - return status; + return( status ); status = psa_export_rsa_key( PSA_KEY_TYPE_RSA_PUBLIC_KEY, rsa, @@ -1614,7 +1607,7 @@ static psa_status_t psa_internal_export_key( const psa_key_slot_t *slot, slot->data.key.bytes, &ecp ); if( status != PSA_SUCCESS ) - return status; + return( status ); status = psa_export_ecp_key( PSA_KEY_TYPE_ECC_PUBLIC_KEY( PSA_KEY_TYPE_ECC_GET_FAMILY( @@ -2046,7 +2039,7 @@ static psa_status_t psa_validate_optional_attributes( slot->data.key.bytes, &rsa ); if( status != PSA_SUCCESS ) - return status; + return( status ); mbedtls_mpi_init( &actual ); mbedtls_mpi_init( &required ); @@ -3036,7 +3029,7 @@ static psa_status_t psa_hmac_setup_internal( psa_hmac_internal_data *hmac, status = psa_hash_update( &hmac->hash_ctx, ipad, block_size ); cleanup: - mbedtls_platform_zeroize( ipad, sizeof(ipad) ); + mbedtls_platform_zeroize( ipad, sizeof( ipad ) ); return( status ); } @@ -3814,7 +3807,7 @@ psa_status_t psa_verify_hash( psa_key_handle_t handle, slot->data.key.bytes, &rsa ); if( status != PSA_SUCCESS ) - return status; + return( status ); status = psa_rsa_verify( rsa, alg, @@ -3838,13 +3831,13 @@ psa_status_t psa_verify_hash( psa_key_handle_t handle, slot->data.key.bytes, &ecp ); if( status != PSA_SUCCESS ) - return status; + return( status ); status = psa_ecdsa_verify( ecp, hash, hash_length, signature, signature_length ); mbedtls_ecp_keypair_free( ecp ); mbedtls_free( ecp ); - return status; + return( status ); } else #endif /* defined(MBEDTLS_ECDSA_C) */ @@ -4005,7 +3998,7 @@ psa_status_t psa_asymmetric_decrypt( psa_key_handle_t handle, slot->data.key.bytes, &rsa ); if( status != PSA_SUCCESS ) - return status; + return( status ); if( input_length != mbedtls_rsa_get_len( rsa ) ) { @@ -4815,7 +4808,7 @@ psa_status_t psa_key_derivation_get_capacity(const psa_key_derivation_operation_ if( operation->alg == 0 ) { /* This is a blank key derivation operation. */ - return PSA_ERROR_BAD_STATE; + return( PSA_ERROR_BAD_STATE ); } *capacity = operation->capacity; @@ -5062,7 +5055,7 @@ psa_status_t psa_key_derivation_output_bytes( if( operation->alg == 0 ) { /* This is a blank operation. */ - return PSA_ERROR_BAD_STATE; + return( PSA_ERROR_BAD_STATE ); } if( output_length > operation->capacity ) @@ -5670,14 +5663,14 @@ static psa_status_t psa_key_agreement_raw_internal( psa_algorithm_t alg, private_key->data.key.bytes, &ecp ); if( status != PSA_SUCCESS ) - return status; + return( status ); status = psa_key_agreement_ecdh( peer_key, peer_key_length, ecp, shared_secret, shared_secret_size, shared_secret_length ); mbedtls_ecp_keypair_free( ecp ); mbedtls_free( ecp ); - return status; + return( status ); #endif /* MBEDTLS_ECDH_C */ default: (void) private_key; @@ -5884,7 +5877,7 @@ static psa_status_t psa_generate_key_internal( /* Allocate memory for the key */ status = psa_allocate_buffer_to_slot( slot, PSA_BITS_TO_BYTES( bits ) ); if( status != PSA_SUCCESS ) - return status; + return( status ); status = psa_generate_random( slot->data.key.data, slot->data.key.bytes ); @@ -5934,7 +5927,7 @@ static psa_status_t psa_generate_key_internal( if( status != PSA_SUCCESS ) { mbedtls_rsa_free( &rsa ); - return status; + return( status ); } status = psa_export_rsa_key( type, @@ -5983,7 +5976,7 @@ static psa_status_t psa_generate_key_internal( if( status != PSA_SUCCESS ) { mbedtls_ecp_keypair_free( &ecp ); - return status; + return( status ); } status = mbedtls_to_psa_error( @@ -5996,8 +5989,9 @@ static psa_status_t psa_generate_key_internal( } else #endif /* MBEDTLS_ECP_C */ - + { return( PSA_ERROR_NOT_SUPPORTED ); + } return( PSA_SUCCESS ); } From fd4d69a72e3e051eee1d3b5a7ecb7f2d806e9683 Mon Sep 17 00:00:00 2001 From: Steven Cooreman Date: Wed, 5 Aug 2020 15:46:33 +0200 Subject: [PATCH 14/16] Simplified key slot deletion And zeroize key buffer before freeing to avoid keys hanging around on the heap. Signed-off-by: Steven Cooreman --- library/psa_crypto.c | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 12f05d135..43f6205d4 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -1204,24 +1204,15 @@ static psa_status_t psa_remove_key_data_from_memory( psa_key_slot_t *slot ) } else #endif /* MBEDTLS_PSA_CRYPTO_SE_C */ - if( slot->attr.type == PSA_KEY_TYPE_NONE ) - { - /* No key material to clean. */ - } - else if( key_type_is_raw_bytes( slot->attr.type ) || - PSA_KEY_TYPE_IS_RSA( slot->attr.type ) || - PSA_KEY_TYPE_IS_ECC( slot->attr.type ) ) { + /* Data pointer will always be either a valid pointer or NULL in an + * initialized slot, so we can just free it. */ + if( slot->data.key.data != NULL ) + mbedtls_platform_zeroize( slot->data.key.data, slot->data.key.bytes); mbedtls_free( slot->data.key.data ); slot->data.key.data = NULL; slot->data.key.bytes = 0; } - else - { - /* Shouldn't happen: the key type is not any type that we - * put in. */ - return( PSA_ERROR_CORRUPTION_DETECTED ); - } return( PSA_SUCCESS ); } From b7f6deaae7ab66bf39e2ec20d4747e0d4894d3ba Mon Sep 17 00:00:00 2001 From: Steven Cooreman Date: Wed, 5 Aug 2020 16:07:20 +0200 Subject: [PATCH 15/16] Add buffer zeroization when ecp_write_key fails Signed-off-by: Steven Cooreman --- library/psa_crypto.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 43f6205d4..d931a5063 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -863,9 +863,9 @@ static psa_status_t psa_export_ecp_key( psa_key_type_t type, data, PSA_BITS_TO_BYTES( ecp->grp.nbits ) ) ); if( status == PSA_SUCCESS ) - { *data_length = PSA_BITS_TO_BYTES( ecp->grp.nbits ); - } + else + memset( data, 0, data_size ); return( status ); } @@ -5974,8 +5974,10 @@ static psa_status_t psa_generate_key_internal( mbedtls_ecp_write_key( &ecp, slot->data.key.data, bytes ) ); mbedtls_ecp_keypair_free( &ecp ); - if( status != PSA_SUCCESS ) + if( status != PSA_SUCCESS ) { + memset( slot->data.key.data, 0, bytes ); psa_remove_key_data_from_memory( slot ); + } return( status ); } else From d4867877f17f4d3657406ded5e14b91fb1df2e69 Mon Sep 17 00:00:00 2001 From: Steven Cooreman Date: Wed, 5 Aug 2020 16:31:39 +0200 Subject: [PATCH 16/16] Initialize key pointer in ecdh to NULL Since it is being dereferenced by free on exit it should be inited to NULL. Also added a small test that would trigger the issue. Signed-off-by: Steven Cooreman --- library/psa_crypto.c | 2 +- tests/suites/test_suite_psa_crypto.data | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index d931a5063..bc1619cc1 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -5586,7 +5586,7 @@ static psa_status_t psa_key_agreement_ecdh( const uint8_t *peer_key, size_t shared_secret_size, size_t *shared_secret_length ) { - mbedtls_ecp_keypair *their_key; + mbedtls_ecp_keypair *their_key = NULL; mbedtls_ecdh_context ecdh; psa_status_t status; size_t bits = 0; diff --git a/tests/suites/test_suite_psa_crypto.data b/tests/suites/test_suite_psa_crypto.data index d1855fdb5..d982f81f6 100644 --- a/tests/suites/test_suite_psa_crypto.data +++ b/tests/suites/test_suite_psa_crypto.data @@ -2413,6 +2413,10 @@ PSA key agreement setup: ECDH + HKDF-SHA-256: good depends_on:MBEDTLS_PK_PARSE_C:MBEDTLS_ECP_C:MBEDTLS_ECP_DP_SECP256R1_ENABLED:MBEDTLS_ECDH_C:MBEDTLS_SHA256_C key_agreement_setup:PSA_ALG_KEY_AGREEMENT(PSA_ALG_ECDH, PSA_ALG_HKDF(PSA_ALG_SHA_256)):PSA_KEY_TYPE_ECC_KEY_PAIR(PSA_ECC_FAMILY_SECP_R1):"c88f01f510d9ac3f70a292daa2316de544e9aab8afe84049c62a9c57862d1433":"04d12dfb5289c8d4f81208b70270398c342296970a0bccb74c736fc7554494bf6356fbf3ca366cc23e8157854c13c58d6aac23f046ada30f8353e74f33039872ab":PSA_SUCCESS +PSA key agreement setup: ECDH + HKDF-SHA-256: public key not on curve +depends_on:MBEDTLS_PK_PARSE_C:MBEDTLS_ECP_C:MBEDTLS_ECP_DP_SECP256R1_ENABLED:MBEDTLS_ECDH_C:MBEDTLS_SHA256_C +key_agreement_setup:PSA_ALG_KEY_AGREEMENT(PSA_ALG_ECDH, PSA_ALG_HKDF(PSA_ALG_SHA_256)):PSA_KEY_TYPE_ECC_KEY_PAIR(PSA_ECC_FAMILY_SECP_R1):"c88f01f510d9ac3f70a292daa2316de544e9aab8afe84049c62a9c57862d1433":"04d12dfb5289c8d4f81208b70270398c342296970a0bccb74c736fc7554494bf6356fbf3ca366cc23e8157854c13c58d6aac23f046ada30f8353e74f33039872ff":PSA_ERROR_INVALID_ARGUMENT + PSA key agreement setup: ECDH + HKDF-SHA-256: public key on different curve depends_on:MBEDTLS_PK_PARSE_C:MBEDTLS_ECP_C:MBEDTLS_ECP_DP_SECP256R1_ENABLED:MBEDTLS_ECP_DP_SECP384R1_ENABLED:MBEDTLS_ECDH_C:MBEDTLS_SHA256_C key_agreement_setup:PSA_ALG_KEY_AGREEMENT(PSA_ALG_ECDH, PSA_ALG_HKDF(PSA_ALG_SHA_256)):PSA_KEY_TYPE_ECC_KEY_PAIR(PSA_ECC_FAMILY_SECP_R1):"c88f01f510d9ac3f70a292daa2316de544e9aab8afe84049c62a9c57862d1433":"04e558dbef53eecde3d3fccfc1aea08a89a987475d12fd950d83cfa41732bc509d0d1ac43a0336def96fda41d0774a3571dcfbec7aacf3196472169e838430367f66eebe3c6e70c416dd5f0c68759dd1fff83fa40142209dff5eaad96db9e6386c":PSA_ERROR_INVALID_ARGUMENT