From a5b860a5f85dc24004755632bcb0c997aafc312d Mon Sep 17 00:00:00 2001 From: Steven Cooreman Date: Fri, 19 Mar 2021 19:04:39 +0100 Subject: [PATCH] Migrate MAC finish calls into the software driver Step 3/x in moving the driver. Separate commits should make for easier review. Additional changes on top of code movement: * Copied the implementation of safer_memcmp from psa_crypto into psa_cipher_mac since the mac_verify driver implementation depends on it, and it isn't available through external linkage Signed-off-by: Steven Cooreman --- library/psa_crypto.c | 117 +++++++-------------------------------- library/psa_crypto_mac.c | 102 ++++++++++++++++++++++++++++++---- 2 files changed, 110 insertions(+), 109 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 7d7d05317..3ca8c9d8f 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -2374,59 +2374,16 @@ psa_status_t psa_mac_update( psa_mac_operation_t *operation, return( status ); } -static psa_status_t psa_mac_finish_internal( mbedtls_psa_mac_operation_t *operation, - uint8_t *mac, - size_t mac_size ) -{ - if( ! operation->key_set ) - return( PSA_ERROR_BAD_STATE ); - if( operation->iv_required && ! operation->iv_set ) - return( PSA_ERROR_BAD_STATE ); - - if( mac_size < operation->mac_size ) - return( PSA_ERROR_BUFFER_TOO_SMALL ); - -#if defined(MBEDTLS_PSA_BUILTIN_ALG_CMAC) - if( operation->alg == PSA_ALG_CMAC ) - { - uint8_t tmp[PSA_BLOCK_CIPHER_BLOCK_MAX_SIZE]; - int ret = mbedtls_cipher_cmac_finish( &operation->ctx.cmac, tmp ); - if( ret == 0 ) - memcpy( mac, tmp, operation->mac_size ); - mbedtls_platform_zeroize( tmp, sizeof( tmp ) ); - return( mbedtls_to_psa_error( ret ) ); - } - else -#endif /* MBEDTLS_PSA_BUILTIN_ALG_CMAC */ -#if defined(MBEDTLS_PSA_BUILTIN_ALG_HMAC) - if( PSA_ALG_IS_HMAC( operation->alg ) ) - { - return( psa_hmac_finish_internal( &operation->ctx.hmac, - mac, operation->mac_size ) ); - } - else -#endif /* MBEDTLS_PSA_BUILTIN_ALG_HMAC */ - { - /* This shouldn't happen if `operation` was initialized by - * a setup function. */ - return( PSA_ERROR_BAD_STATE ); - } -} - -psa_status_t psa_mac_sign_finish( psa_mac_operation_t *psa_operation, +psa_status_t psa_mac_sign_finish( psa_mac_operation_t *operation, uint8_t *mac, size_t mac_size, size_t *mac_length ) { - /* Temporary recast to avoid changing a lot of lines */ - mbedtls_psa_mac_operation_t* operation = &psa_operation->ctx.mbedtls_ctx; + psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED; + psa_status_t abort_status = PSA_ERROR_CORRUPTION_DETECTED; - psa_status_t status; - - if( operation->alg == 0 ) - { + if( operation->id == 0 ) return( PSA_ERROR_BAD_STATE ); - } /* Fill the output buffer with something that isn't a valid mac * (barring an attack on the mac and deliberately-crafted input), @@ -2437,68 +2394,32 @@ psa_status_t psa_mac_sign_finish( psa_mac_operation_t *psa_operation, if( mac_size != 0 ) memset( mac, '!', mac_size ); - if( ! operation->is_sign ) - { - return( PSA_ERROR_BAD_STATE ); - } + status = psa_driver_wrapper_mac_sign_finish( operation, + mac, mac_size, mac_length ); - status = psa_mac_finish_internal( operation, mac, mac_size ); + abort_status = psa_mac_abort( operation ); - if( status == PSA_SUCCESS ) - { - status = psa_mac_abort( psa_operation ); - if( status == PSA_SUCCESS ) - *mac_length = operation->mac_size; - else - memset( mac, '!', mac_size ); - } - else - psa_mac_abort( psa_operation ); - return( status ); + if( status != PSA_SUCCESS && mac_size > 0 ) + memset( mac, '!', mac_size ); + + return( status == PSA_SUCCESS ? abort_status : status ); } -psa_status_t psa_mac_verify_finish( psa_mac_operation_t *psa_operation, +psa_status_t psa_mac_verify_finish( psa_mac_operation_t *operation, const uint8_t *mac, size_t mac_length ) { - /* Temporary recast to avoid changing a lot of lines */ - mbedtls_psa_mac_operation_t* operation = &psa_operation->ctx.mbedtls_ctx; + psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED; + psa_status_t abort_status = PSA_ERROR_CORRUPTION_DETECTED; - uint8_t actual_mac[PSA_MAC_MAX_SIZE]; - psa_status_t status; - - if( operation->alg == 0 ) - { + if( operation->id == 0 ) return( PSA_ERROR_BAD_STATE ); - } - if( operation->is_sign ) - { - return( PSA_ERROR_BAD_STATE ); - } - if( operation->mac_size != mac_length ) - { - status = PSA_ERROR_INVALID_SIGNATURE; - goto cleanup; - } + status = psa_driver_wrapper_mac_verify_finish( operation, + mac, mac_length ); + abort_status = psa_mac_abort( operation ); - status = psa_mac_finish_internal( operation, - actual_mac, sizeof( actual_mac ) ); - if( status != PSA_SUCCESS ) - goto cleanup; - - if( safer_memcmp( mac, actual_mac, mac_length ) != 0 ) - status = PSA_ERROR_INVALID_SIGNATURE; - -cleanup: - if( status == PSA_SUCCESS ) - status = psa_mac_abort( psa_operation ); - else - psa_mac_abort( psa_operation ); - - mbedtls_platform_zeroize( actual_mac, sizeof( actual_mac ) ); - - return( status ); + return( status == PSA_SUCCESS ? abort_status : status ); } diff --git a/library/psa_crypto_mac.c b/library/psa_crypto_mac.c index 252afca1f..72386d873 100644 --- a/library/psa_crypto_mac.c +++ b/library/psa_crypto_mac.c @@ -457,18 +457,77 @@ static psa_status_t mac_update( } } +static psa_status_t mac_finish_internal( mbedtls_psa_mac_operation_t *operation, + uint8_t *mac, + size_t mac_size ) +{ + if( ! operation->key_set ) + return( PSA_ERROR_BAD_STATE ); + if( operation->iv_required && ! operation->iv_set ) + return( PSA_ERROR_BAD_STATE ); + + if( mac_size < operation->mac_size ) + return( PSA_ERROR_BUFFER_TOO_SMALL ); + +#if defined(BUILTIN_ALG_CMAC) + if( operation->alg == PSA_ALG_CMAC ) + { + uint8_t tmp[PSA_BLOCK_CIPHER_BLOCK_MAX_SIZE]; + int ret = mbedtls_cipher_cmac_finish( &operation->ctx.cmac, tmp ); + if( ret == 0 ) + memcpy( mac, tmp, operation->mac_size ); + mbedtls_platform_zeroize( tmp, sizeof( tmp ) ); + return( mbedtls_to_psa_error( ret ) ); + } + else +#endif /* BUILTIN_ALG_CMAC */ +#if defined(BUILTIN_ALG_HMAC) + if( PSA_ALG_IS_HMAC( operation->alg ) ) + { + return( psa_hmac_finish_internal( &operation->ctx.hmac, + mac, operation->mac_size ) ); + } + else +#endif /* BUILTIN_ALG_HMAC */ + { + /* This shouldn't happen if `operation` was initialized by + * a setup function. */ + return( PSA_ERROR_BAD_STATE ); + } +} + static psa_status_t mac_sign_finish( mbedtls_psa_mac_operation_t *operation, uint8_t *mac, size_t mac_size, size_t *mac_length ) { - /* To be fleshed out in a subsequent commit */ - (void) operation; - (void) mac; - (void) mac_size; - (void) mac_length; - return( PSA_ERROR_NOT_SUPPORTED ); + psa_status_t status; + + if( operation->alg == 0 ) + return( PSA_ERROR_BAD_STATE ); + + if( ! operation->is_sign ) + return( PSA_ERROR_BAD_STATE ); + + status = mac_finish_internal( operation, mac, mac_size ); + + if( status == PSA_SUCCESS ) + *mac_length = operation->mac_size; + + return( status ); +} + +/* constant-time buffer comparison */ +static inline int safer_memcmp( const uint8_t *a, const uint8_t *b, size_t n ) +{ + size_t i; + unsigned char diff = 0; + + for( i = 0; i < n; i++ ) + diff |= a[i] ^ b[i]; + + return( diff ); } static psa_status_t mac_verify_finish( @@ -476,11 +535,32 @@ static psa_status_t mac_verify_finish( const uint8_t *mac, size_t mac_length ) { - /* To be fleshed out in a subsequent commit */ - (void) operation; - (void) mac; - (void) mac_length; - return( PSA_ERROR_NOT_SUPPORTED ); + uint8_t actual_mac[PSA_MAC_MAX_SIZE]; + psa_status_t status; + + if( operation->alg == 0 ) + return( PSA_ERROR_BAD_STATE ); + + if( operation->is_sign ) + return( PSA_ERROR_BAD_STATE ); + + if( operation->mac_size != mac_length ) + { + status = PSA_ERROR_INVALID_SIGNATURE; + goto cleanup; + } + + status = mac_finish_internal( operation, actual_mac, sizeof( actual_mac ) ); + if( status != PSA_SUCCESS ) + goto cleanup; + + if( safer_memcmp( mac, actual_mac, mac_length ) != 0 ) + status = PSA_ERROR_INVALID_SIGNATURE; + +cleanup: + mbedtls_platform_zeroize( actual_mac, sizeof( actual_mac ) ); + + return( status ); } #endif /* MBEDTLS_PSA_BUILTIN_MAC || PSA_CRYPTO_DRIVER_TEST */