From 6c9bb0f71ea70ca271d454de36840aa95652f0d4 Mon Sep 17 00:00:00 2001 From: Ronald Cron Date: Thu, 15 Jul 2021 09:38:11 +0200 Subject: [PATCH 01/15] test: psa cipher: Add unexpected IV setting/generation negative tests Signed-off-by: Ronald Cron --- tests/suites/test_suite_psa_crypto.function | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/tests/suites/test_suite_psa_crypto.function b/tests/suites/test_suite_psa_crypto.function index 9136cb91da..a545211429 100644 --- a/tests/suites/test_suite_psa_crypto.function +++ b/tests/suites/test_suite_psa_crypto.function @@ -2526,6 +2526,9 @@ void cipher_encrypt_alg_without_iv( int alg_arg, mbedtls_svc_key_id_t key = MBEDTLS_SVC_KEY_ID_INIT; psa_key_type_t key_type = key_type_arg; psa_algorithm_t alg = alg_arg; + psa_cipher_operation_t operation = PSA_CIPHER_OPERATION_INIT; + uint8_t iv[1] = { 0x5a }; + size_t iv_length; unsigned char *output = NULL; size_t output_buffer_size = 0; size_t output_length = 0; @@ -2543,6 +2546,14 @@ void cipher_encrypt_alg_without_iv( int alg_arg, PSA_ASSERT( psa_import_key( &attributes, key_data->x, key_data->len, &key ) ); + PSA_ASSERT( psa_cipher_encrypt_setup( &operation, key, alg ) ); + TEST_EQUAL( psa_cipher_set_iv( &operation, iv, sizeof( iv ) ), + PSA_ERROR_BAD_STATE ); + PSA_ASSERT( psa_cipher_encrypt_setup( &operation, key, alg ) ); + TEST_EQUAL( psa_cipher_generate_iv( &operation, iv, sizeof( iv ), + &iv_length ), + PSA_ERROR_BAD_STATE ); + PSA_ASSERT( psa_cipher_encrypt( key, alg, input->x, input->len, output, output_buffer_size, &output_length ) ); TEST_ASSERT( output_length <= From 2d75cd72da963075abe8c0ef3d4a895362ec2b9b Mon Sep 17 00:00:00 2001 From: Ronald Cron Date: Fri, 9 Jul 2021 14:43:26 +0200 Subject: [PATCH 02/15] test: psa driver wrapper: Add non regression test for psa_cipher_generate_iv() Add non regression test for invalid usage of the output buffer in psa_cipher_generate_iv(). The output buffer should not be used to pass the IV to the driver as a local attacker could be able to control the used IV. Signed-off-by: Ronald Cron --- .../test_suite_psa_crypto_driver_wrappers.function | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/suites/test_suite_psa_crypto_driver_wrappers.function b/tests/suites/test_suite_psa_crypto_driver_wrappers.function index 6d78ad51a0..39de0ca219 100644 --- a/tests/suites/test_suite_psa_crypto_driver_wrappers.function +++ b/tests/suites/test_suite_psa_crypto_driver_wrappers.function @@ -923,10 +923,23 @@ void cipher_entry_points( int alg_arg, int key_type_arg, mbedtls_test_driver_cipher_hooks.hits = 0; mbedtls_test_driver_cipher_hooks.forced_status = PSA_ERROR_GENERIC_ERROR; + /* Set the output buffer in a given state. */ + for( size_t i = 0; i < 16; i++ ) + output[i] = 0xa5; + status = psa_cipher_generate_iv( &operation, output, 16, &function_output_length ); /* When generating the IV fails, it should call abort too */ TEST_EQUAL( mbedtls_test_driver_cipher_hooks.hits, 2 ); TEST_EQUAL( status, mbedtls_test_driver_cipher_hooks.forced_status ); + /* + * Check that the output buffer is still in the same state. + * This will fail if the output buffer is used by the core to pass the IV + * it generated to the driver (and is not restored). + */ + for( size_t i = 0; i < 16; i++ ) + { + TEST_EQUAL( output[i], 0xa5 ); + } /* Failure should prevent further operations from executing on the driver */ mbedtls_test_driver_cipher_hooks.hits = 0; status = psa_cipher_update( &operation, From 2fb9052838d2395f25baf3fd4d8a4e7e9f4ec32c Mon Sep 17 00:00:00 2001 From: Ronald Cron Date: Mon, 5 Jul 2021 12:31:44 +0200 Subject: [PATCH 03/15] psa: cipher: Fix invalid output buffer usage in psa_cipher_generate_iv() Don't use the output buffer in psa_cipher_generate_iv() to pass the generated IV to the driver as local attacker could potentially control it. Signed-off-by: Ronald Cron --- library/psa_crypto.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 6e73d12c68..e422133633 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -3314,8 +3314,8 @@ psa_status_t psa_cipher_generate_iv( psa_cipher_operation_t *operation, size_t *iv_length ) { psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED; - - *iv_length = 0; + uint8_t local_iv[PSA_CIPHER_IV_MAX_SIZE]; + size_t default_iv_length; if( operation->id == 0 ) { @@ -3329,28 +3329,38 @@ psa_status_t psa_cipher_generate_iv( psa_cipher_operation_t *operation, goto exit; } - if( iv_size < operation->default_iv_length ) + default_iv_length = operation->default_iv_length; + if( iv_size < default_iv_length ) { status = PSA_ERROR_BUFFER_TOO_SMALL; goto exit; } - status = psa_generate_random( iv, operation->default_iv_length ); + if( default_iv_length > PSA_CIPHER_IV_MAX_SIZE ) + { + status = PSA_ERROR_GENERIC_ERROR; + goto exit; + } + + status = psa_generate_random( local_iv, default_iv_length ); if( status != PSA_SUCCESS ) goto exit; status = psa_driver_wrapper_cipher_set_iv( operation, - iv, - operation->default_iv_length ); + local_iv, default_iv_length ); exit: if( status == PSA_SUCCESS ) { + memcpy( iv, local_iv, default_iv_length ); + *iv_length = default_iv_length; operation->iv_set = 1; - *iv_length = operation->default_iv_length; } else + { + *iv_length = 0; psa_cipher_abort( operation ); + } return( status ); } From 2391952a4c1ce7f973decdb2593fad9dc4748c03 Mon Sep 17 00:00:00 2001 From: Ronald Cron Date: Thu, 8 Jul 2021 19:00:07 +0200 Subject: [PATCH 04/15] psa: cipher: Align APIs execution flow Align the execution of cipher one-shot APIs with that of cipher multi-part APIs: always exit through the exit-labelled section. Signed-off-by: Ronald Cron --- library/psa_crypto.c | 36 ++++++++++++++++++++++++------------ 1 file changed, 24 insertions(+), 12 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index e422133633..6163de1258 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -3501,20 +3501,21 @@ psa_status_t psa_cipher_encrypt( mbedtls_svc_key_id_t key, { psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED; psa_status_t unlock_status = PSA_ERROR_CORRUPTION_DETECTED; - psa_key_slot_t *slot; + psa_key_slot_t *slot = NULL; psa_key_type_t key_type; size_t iv_length; - *output_length = 0; - if( ! PSA_ALG_IS_CIPHER( alg ) ) - return( PSA_ERROR_INVALID_ARGUMENT ); + { + status = PSA_ERROR_INVALID_ARGUMENT; + goto exit; + } status = psa_get_and_lock_key_slot_with_policy( key, &slot, PSA_KEY_USAGE_ENCRYPT, alg ); if( status != PSA_SUCCESS ) - return( status ); + goto exit; psa_key_attributes_t attributes = { .core = slot->attr @@ -3543,8 +3544,13 @@ psa_status_t psa_cipher_encrypt( mbedtls_svc_key_id_t key, exit: unlock_status = psa_unlock_key_slot( slot ); + if( status == PSA_SUCCESS ) + status = unlock_status; - return( ( status == PSA_SUCCESS ) ? unlock_status : status ); + if( status != PSA_SUCCESS ) + *output_length = 0; + + return( status ); } psa_status_t psa_cipher_decrypt( mbedtls_svc_key_id_t key, @@ -3557,18 +3563,19 @@ psa_status_t psa_cipher_decrypt( mbedtls_svc_key_id_t key, { psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED; psa_status_t unlock_status = PSA_ERROR_CORRUPTION_DETECTED; - psa_key_slot_t *slot; - - *output_length = 0; + psa_key_slot_t *slot = NULL; if( ! PSA_ALG_IS_CIPHER( alg ) ) - return( PSA_ERROR_INVALID_ARGUMENT ); + { + status = PSA_ERROR_INVALID_ARGUMENT; + goto exit; + } status = psa_get_and_lock_key_slot_with_policy( key, &slot, PSA_KEY_USAGE_DECRYPT, alg ); if( status != PSA_SUCCESS ) - return( status ); + goto exit; psa_key_attributes_t attributes = { .core = slot->attr @@ -3587,8 +3594,13 @@ psa_status_t psa_cipher_decrypt( mbedtls_svc_key_id_t key, exit: unlock_status = psa_unlock_key_slot( slot ); + if( status == PSA_SUCCESS ) + status = unlock_status; - return( ( status == PSA_SUCCESS ) ? unlock_status : status ); + if( status != PSA_SUCCESS ) + *output_length = 0; + + return( status ); } From e9a45fcecbfc3e19818b18cf23bedf28b1f1f2e0 Mon Sep 17 00:00:00 2001 From: Ronald Cron Date: Fri, 9 Jul 2021 17:56:40 +0200 Subject: [PATCH 05/15] test: psa driver: Remove unnecessary IV generation Signed-off-by: Ronald Cron --- tests/src/drivers/test_driver_cipher.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/src/drivers/test_driver_cipher.c b/tests/src/drivers/test_driver_cipher.c index 89a7b59944..755f6a0a78 100644 --- a/tests/src/drivers/test_driver_cipher.c +++ b/tests/src/drivers/test_driver_cipher.c @@ -64,8 +64,6 @@ psa_status_t mbedtls_test_transparent_cipher_encrypt( if( mbedtls_test_driver_cipher_hooks.forced_status != PSA_SUCCESS ) return( mbedtls_test_driver_cipher_hooks.forced_status ); - psa_generate_random( output, PSA_CIPHER_IV_LENGTH( attributes->core.type, alg ) ); - return( mbedtls_transparent_test_driver_cipher_encrypt( attributes, key_buffer, key_buffer_size, alg, input, input_length, From 9b67428e22909754412543474a2f39694de9fddd Mon Sep 17 00:00:00 2001 From: Ronald Cron Date: Fri, 9 Jul 2021 09:19:35 +0200 Subject: [PATCH 06/15] psa: cipher: Add IV parameters to cipher_encrypt entry point Signed-off-by: Ronald Cron --- library/psa_crypto.c | 8 ++++--- library/psa_crypto_cipher.c | 32 +++++++++++++------------- library/psa_crypto_cipher.h | 20 ++++++++-------- library/psa_crypto_driver_wrappers.c | 8 +++++++ library/psa_crypto_driver_wrappers.h | 2 ++ tests/include/test/drivers/cipher.h | 2 ++ tests/src/drivers/test_driver_cipher.c | 7 +++++- 7 files changed, 49 insertions(+), 30 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 6163de1258..3a286aa037 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -3539,15 +3539,17 @@ psa_status_t psa_cipher_encrypt( mbedtls_svc_key_id_t key, status = psa_driver_wrapper_cipher_encrypt( &attributes, slot->key.data, slot->key.bytes, - alg, input, input_length, - output, output_size, output_length ); + alg, output, iv_length, input, input_length, + output + iv_length, output_size - iv_length, output_length ); exit: unlock_status = psa_unlock_key_slot( slot ); if( status == PSA_SUCCESS ) status = unlock_status; - if( status != PSA_SUCCESS ) + if( status == PSA_SUCCESS ) + *output_length += iv_length; + else *output_length = 0; return( status ); diff --git a/library/psa_crypto_cipher.c b/library/psa_crypto_cipher.c index d8c722bb1b..eda6e0060c 100644 --- a/library/psa_crypto_cipher.c +++ b/library/psa_crypto_cipher.c @@ -451,6 +451,8 @@ static psa_status_t cipher_encrypt( const psa_key_attributes_t *attributes, const uint8_t *key_buffer, size_t key_buffer_size, psa_algorithm_t alg, + const uint8_t *iv, + size_t iv_length, const uint8_t *input, size_t input_length, uint8_t *output, @@ -459,38 +461,32 @@ static psa_status_t cipher_encrypt( const psa_key_attributes_t *attributes, { psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED; mbedtls_psa_cipher_operation_t operation = MBEDTLS_PSA_CIPHER_OPERATION_INIT; - size_t olength, accumulated_length; + size_t update_output_length, finish_output_length; status = cipher_encrypt_setup( &operation, attributes, key_buffer, key_buffer_size, alg ); if( status != PSA_SUCCESS ) goto exit; - accumulated_length = 0; - if( operation.iv_length > 0 ) + if( iv_length > 0 ) { - status = cipher_set_iv( &operation, output, operation.iv_length ); + status = cipher_set_iv( &operation, iv, iv_length ); if( status != PSA_SUCCESS ) goto exit; - - accumulated_length = operation.iv_length; } status = cipher_update( &operation, input, input_length, - output + operation.iv_length, - output_size - operation.iv_length, - &olength ); + output, output_size, &update_output_length ); if( status != PSA_SUCCESS ) goto exit; - accumulated_length += olength; - - status = cipher_finish( &operation, output + accumulated_length, - output_size - accumulated_length, &olength ); + status = cipher_finish( &operation, output + update_output_length, + output_size - update_output_length, + &finish_output_length ); if( status != PSA_SUCCESS ) goto exit; - *output_length = accumulated_length + olength; + *output_length = update_output_length + finish_output_length; exit: if( status == PSA_SUCCESS ) @@ -606,6 +602,8 @@ psa_status_t mbedtls_psa_cipher_encrypt( const psa_key_attributes_t *attributes, const uint8_t *key_buffer, size_t key_buffer_size, psa_algorithm_t alg, + const uint8_t *iv, + size_t iv_length, const uint8_t *input, size_t input_length, uint8_t *output, @@ -613,7 +611,7 @@ psa_status_t mbedtls_psa_cipher_encrypt( const psa_key_attributes_t *attributes, size_t *output_length ) { return( cipher_encrypt( attributes, key_buffer, key_buffer_size, - alg, input, input_length, + alg, iv, iv_length, input, input_length, output, output_size, output_length ) ); } @@ -692,6 +690,8 @@ psa_status_t mbedtls_transparent_test_driver_cipher_encrypt( const uint8_t *key_buffer, size_t key_buffer_size, psa_algorithm_t alg, + const uint8_t *iv, + size_t iv_length, const uint8_t *input, size_t input_length, uint8_t *output, @@ -699,7 +699,7 @@ psa_status_t mbedtls_transparent_test_driver_cipher_encrypt( size_t *output_length ) { return( cipher_encrypt( attributes, key_buffer, key_buffer_size, - alg, input, input_length, + alg, iv, iv_length, input, input_length, output, output_size, output_length ) ); } diff --git a/library/psa_crypto_cipher.h b/library/psa_crypto_cipher.h index 5971e8d3f0..76ff22acf7 100644 --- a/library/psa_crypto_cipher.h +++ b/library/psa_crypto_cipher.h @@ -213,16 +213,12 @@ psa_status_t mbedtls_psa_cipher_abort( mbedtls_psa_cipher_operation_t *operation * \param[in] alg The cipher algorithm to compute * (\c PSA_ALG_XXX value such that * #PSA_ALG_IS_CIPHER(\p alg) is true). - * \param[in] input Buffer containing the message to encrypt. - * \param[in] input_length Size of the \p input buffer in bytes. + * \param[in] iv Buffer containing the IV for encryption. The + * IV has been generated by the core. + * \param[in] iv_length Size of the \p iv in bytes. + * \param[in] input Buffer containing the message to encrypt. + * \param[in] input_length Size of the \p input buffer in bytes. * \param[in,out] output Buffer where the output is to be written. - * The core has generated and written the IV - * at the beginning of this buffer before - * this function is called. The size of the IV - * is PSA_CIPHER_IV_LENGTH( key_type, alg ) where - * \c key_type is the type of the key identified - * by \p key and \p alg is the cipher algorithm - * to compute. * \param[in] output_size Size of the \p output buffer in bytes. * \param[out] output_length On success, the number of bytes that make up * the returned output. Initialized to zero @@ -235,7 +231,7 @@ psa_status_t mbedtls_psa_cipher_abort( mbedtls_psa_cipher_operation_t *operation * \retval #PSA_ERROR_BUFFER_TOO_SMALL * The size of the \p output buffer is too small. * \retval #PSA_ERROR_INVALID_ARGUMENT - * The size of \p iv is not acceptable for the chosen algorithm, + * The size \p iv_length is not acceptable for the chosen algorithm, * or the chosen algorithm does not use an IV. * The total input size passed to this operation is not valid for * this particular algorithm. For example, the algorithm is a based @@ -249,6 +245,8 @@ psa_status_t mbedtls_psa_cipher_encrypt( const psa_key_attributes_t *attributes, const uint8_t *key_buffer, size_t key_buffer_size, psa_algorithm_t alg, + const uint8_t *iv, + size_t iv_length, const uint8_t *input, size_t input_length, uint8_t *output, @@ -342,6 +340,8 @@ psa_status_t mbedtls_transparent_test_driver_cipher_encrypt( const uint8_t *key_buffer, size_t key_buffer_size, psa_algorithm_t alg, + const uint8_t *iv, + size_t iv_length, const uint8_t *input, size_t input_length, uint8_t *output, diff --git a/library/psa_crypto_driver_wrappers.c b/library/psa_crypto_driver_wrappers.c index 38d0e300e2..59daee9654 100644 --- a/library/psa_crypto_driver_wrappers.c +++ b/library/psa_crypto_driver_wrappers.c @@ -741,6 +741,8 @@ psa_status_t psa_driver_wrapper_cipher_encrypt( const uint8_t *key_buffer, size_t key_buffer_size, psa_algorithm_t alg, + const uint8_t *iv, + size_t iv_length, const uint8_t *input, size_t input_length, uint8_t *output, @@ -762,6 +764,8 @@ psa_status_t psa_driver_wrapper_cipher_encrypt( key_buffer, key_buffer_size, alg, + iv, + iv_length, input, input_length, output, @@ -778,6 +782,8 @@ psa_status_t psa_driver_wrapper_cipher_encrypt( key_buffer, key_buffer_size, alg, + iv, + iv_length, input, input_length, output, @@ -795,6 +801,8 @@ psa_status_t psa_driver_wrapper_cipher_encrypt( key_buffer, key_buffer_size, alg, + iv, + iv_length, input, input_length, output, diff --git a/library/psa_crypto_driver_wrappers.h b/library/psa_crypto_driver_wrappers.h index 38a6ee82a7..9eb08bc9f8 100644 --- a/library/psa_crypto_driver_wrappers.h +++ b/library/psa_crypto_driver_wrappers.h @@ -102,6 +102,8 @@ psa_status_t psa_driver_wrapper_cipher_encrypt( const uint8_t *key_buffer, size_t key_buffer_size, psa_algorithm_t alg, + const uint8_t *iv, + size_t iv_length, const uint8_t *input, size_t input_length, uint8_t *output, diff --git a/tests/include/test/drivers/cipher.h b/tests/include/test/drivers/cipher.h index 142f3b7655..33a5e66579 100644 --- a/tests/include/test/drivers/cipher.h +++ b/tests/include/test/drivers/cipher.h @@ -53,6 +53,7 @@ psa_status_t mbedtls_test_transparent_cipher_encrypt( const psa_key_attributes_t *attributes, const uint8_t *key, size_t key_length, psa_algorithm_t alg, + const uint8_t *iv, size_t iv_length, const uint8_t *input, size_t input_length, uint8_t *output, size_t output_size, size_t *output_length); @@ -98,6 +99,7 @@ psa_status_t mbedtls_test_opaque_cipher_encrypt( const psa_key_attributes_t *attributes, const uint8_t *key, size_t key_length, psa_algorithm_t alg, + const uint8_t *iv, size_t iv_length, const uint8_t *input, size_t input_length, uint8_t *output, size_t output_size, size_t *output_length); diff --git a/tests/src/drivers/test_driver_cipher.c b/tests/src/drivers/test_driver_cipher.c index 755f6a0a78..33bfddd0a0 100644 --- a/tests/src/drivers/test_driver_cipher.c +++ b/tests/src/drivers/test_driver_cipher.c @@ -40,6 +40,8 @@ psa_status_t mbedtls_test_transparent_cipher_encrypt( const uint8_t *key_buffer, size_t key_buffer_size, psa_algorithm_t alg, + const uint8_t *iv, + size_t iv_length, const uint8_t *input, size_t input_length, uint8_t *output, @@ -66,7 +68,7 @@ psa_status_t mbedtls_test_transparent_cipher_encrypt( return( mbedtls_transparent_test_driver_cipher_encrypt( attributes, key_buffer, key_buffer_size, - alg, input, input_length, + alg, iv, iv_length, input, input_length, output, output_size, output_length ) ); } @@ -240,6 +242,7 @@ psa_status_t mbedtls_test_opaque_cipher_encrypt( const psa_key_attributes_t *attributes, const uint8_t *key, size_t key_length, psa_algorithm_t alg, + const uint8_t *iv, size_t iv_length, const uint8_t *input, size_t input_length, uint8_t *output, size_t output_size, size_t *output_length) { @@ -247,6 +250,8 @@ psa_status_t mbedtls_test_opaque_cipher_encrypt( (void) key; (void) key_length; (void) alg; + (void) iv; + (void) iv_length; (void) input; (void) input_length; (void) output; From c60772c5d9d05dc08416a0e43e2285e37843bea2 Mon Sep 17 00:00:00 2001 From: Ronald Cron Date: Fri, 9 Jul 2021 16:55:02 +0200 Subject: [PATCH 07/15] test: psa driver wrapper: Add cipher_encrypt negative testing Signed-off-by: Ronald Cron --- ..._suite_psa_crypto_driver_wrappers.function | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tests/suites/test_suite_psa_crypto_driver_wrappers.function b/tests/suites/test_suite_psa_crypto_driver_wrappers.function index 39de0ca219..49764634c9 100644 --- a/tests/suites/test_suite_psa_crypto_driver_wrappers.function +++ b/tests/suites/test_suite_psa_crypto_driver_wrappers.function @@ -872,6 +872,26 @@ void cipher_entry_points( int alg_arg, int key_type_arg, PSA_ASSERT( psa_import_key( &attributes, key_data->x, key_data->len, &key ) ); + /* + * Test encrypt failure + * First test that if we don't force a driver error, encryption is + * successfull, then force driver error. + */ + status = psa_cipher_encrypt( + key, alg, input->x, input->len, + output, output_buffer_size, &function_output_length ); + TEST_EQUAL( mbedtls_test_driver_cipher_hooks.hits, 1 ); + TEST_EQUAL( status, PSA_SUCCESS ); + mbedtls_test_driver_cipher_hooks.hits = 0; + + mbedtls_test_driver_cipher_hooks.forced_status = PSA_ERROR_GENERIC_ERROR; + status = psa_cipher_encrypt( + key, alg, input->x, input->len, + output, output_buffer_size, &function_output_length ); + TEST_EQUAL( mbedtls_test_driver_cipher_hooks.hits, 1 ); + TEST_EQUAL( status, PSA_ERROR_GENERIC_ERROR ); + mbedtls_test_driver_cipher_hooks.hits = 0; + /* Test setup call, encrypt */ mbedtls_test_driver_cipher_hooks.forced_status = PSA_ERROR_GENERIC_ERROR; status = psa_cipher_encrypt_setup( &operation, key, alg ); From 13ed5c1c05b3b6e184e380bd9098c3364b3a1636 Mon Sep 17 00:00:00 2001 From: Ronald Cron Date: Fri, 9 Jul 2021 18:20:46 +0200 Subject: [PATCH 08/15] test: psa driver wrapper: Add non regression test for psa_cipher_encrypt() Add non regression test for invalid usage of the output buffer in psa_cipher_encrypt(). The output buffer should not be used to pass the IV to the driver as a local attacker could be able to control the used IV. Signed-off-by: Ronald Cron --- .../test_suite_psa_crypto_driver_wrappers.function | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/suites/test_suite_psa_crypto_driver_wrappers.function b/tests/suites/test_suite_psa_crypto_driver_wrappers.function index 49764634c9..74cc032420 100644 --- a/tests/suites/test_suite_psa_crypto_driver_wrappers.function +++ b/tests/suites/test_suite_psa_crypto_driver_wrappers.function @@ -885,11 +885,24 @@ void cipher_entry_points( int alg_arg, int key_type_arg, mbedtls_test_driver_cipher_hooks.hits = 0; mbedtls_test_driver_cipher_hooks.forced_status = PSA_ERROR_GENERIC_ERROR; + /* Set the output buffer in a given state. */ + for( size_t i = 0; i < output_buffer_size; i++ ) + output[i] = 0xa5; + status = psa_cipher_encrypt( key, alg, input->x, input->len, output, output_buffer_size, &function_output_length ); TEST_EQUAL( mbedtls_test_driver_cipher_hooks.hits, 1 ); TEST_EQUAL( status, PSA_ERROR_GENERIC_ERROR ); + /* + * Check that the output buffer is still in the same state. + * This will fail if the output buffer is used by the core to pass the IV + * it generated to the driver (and is not restored). + */ + for( size_t i = 0; i < output_buffer_size; i++ ) + { + TEST_EQUAL( output[i], 0xa5 ); + } mbedtls_test_driver_cipher_hooks.hits = 0; /* Test setup call, encrypt */ From c6e6f50d4783a12957f904a5d127897b05bcf706 Mon Sep 17 00:00:00 2001 From: Ronald Cron Date: Fri, 9 Jul 2021 18:44:58 +0200 Subject: [PATCH 09/15] psa: cipher: Fix invalid output buffer usage in psa_cipher_encrypt() Don't use the output buffer in psa_cipher_encrypt() to pass the generated IV to the driver as local attacker could potentially control it. Signed-off-by: Ronald Cron --- 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 3a286aa037..a8df97fabd 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -3502,8 +3502,8 @@ psa_status_t psa_cipher_encrypt( mbedtls_svc_key_id_t key, psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED; psa_status_t unlock_status = PSA_ERROR_CORRUPTION_DETECTED; psa_key_slot_t *slot = NULL; - psa_key_type_t key_type; - size_t iv_length; + uint8_t local_iv[PSA_CIPHER_IV_MAX_SIZE]; + size_t default_iv_length = 0; if( ! PSA_ALG_IS_CIPHER( alg ) ) { @@ -3521,26 +3521,31 @@ psa_status_t psa_cipher_encrypt( mbedtls_svc_key_id_t key, .core = slot->attr }; - key_type = slot->attr.type; - iv_length = PSA_CIPHER_IV_LENGTH( key_type, alg ); - - if( iv_length > 0 ) + default_iv_length = PSA_CIPHER_IV_LENGTH( slot->attr.type, alg ); + if( default_iv_length > PSA_CIPHER_IV_MAX_SIZE ) { - if( output_size < iv_length ) + status = PSA_ERROR_GENERIC_ERROR; + goto exit; + } + + if( default_iv_length > 0 ) + { + if( output_size < default_iv_length ) { status = PSA_ERROR_BUFFER_TOO_SMALL; goto exit; } - status = psa_generate_random( output, iv_length ); + status = psa_generate_random( local_iv, default_iv_length ); if( status != PSA_SUCCESS ) goto exit; } status = psa_driver_wrapper_cipher_encrypt( &attributes, slot->key.data, slot->key.bytes, - alg, output, iv_length, input, input_length, - output + iv_length, output_size - iv_length, output_length ); + alg, local_iv, default_iv_length, input, input_length, + output + default_iv_length, output_size - default_iv_length, + output_length ); exit: unlock_status = psa_unlock_key_slot( slot ); @@ -3548,7 +3553,11 @@ exit: status = unlock_status; if( status == PSA_SUCCESS ) - *output_length += iv_length; + { + if( default_iv_length > 0 ) + memcpy( output, local_iv, default_iv_length ); + *output_length += default_iv_length; + } else *output_length = 0; From cae590905363747d26fb5617b71bd567541a2f39 Mon Sep 17 00:00:00 2001 From: Ronald Cron Date: Fri, 26 Nov 2021 18:53:58 +0100 Subject: [PATCH 10/15] psa: aead: Fix invalid output buffer usage in generate_nonce() Don't use the output buffer in psa_aead_generate_nonce() to pass the generated nonce to the driver as a local attacker could potentially control it. Signed-off-by: Ronald Cron --- library/psa_crypto.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 0a04ba1061..daa34ffa34 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -3868,6 +3868,7 @@ psa_status_t psa_aead_generate_nonce( psa_aead_operation_t *operation, size_t *nonce_length ) { psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED; + uint8_t local_nonce[PSA_AEAD_NONCE_MAX_SIZE]; size_t required_nonce_size; *nonce_length = 0; @@ -3892,15 +3893,24 @@ psa_status_t psa_aead_generate_nonce( psa_aead_operation_t *operation, goto exit; } - status = psa_generate_random( nonce, required_nonce_size ); + if( required_nonce_size > sizeof( local_nonce ) ) + { + status = PSA_ERROR_GENERIC_ERROR; + goto exit; + } + + status = psa_generate_random( local_nonce, required_nonce_size ); if( status != PSA_SUCCESS ) goto exit; - status = psa_aead_set_nonce( operation, nonce, required_nonce_size ); + status = psa_aead_set_nonce( operation, local_nonce, required_nonce_size ); exit: if( status == PSA_SUCCESS ) + { + memcpy( nonce, local_nonce, required_nonce_size ); *nonce_length = required_nonce_size; + } else psa_aead_abort( operation ); From 6fd156aa6bd97ac4daf555243f9334398327058b Mon Sep 17 00:00:00 2001 From: Ronald Cron Date: Thu, 2 Dec 2021 11:26:07 +0100 Subject: [PATCH 11/15] Add change log Signed-off-by: Ronald Cron --- ChangeLog.d/fix-cipher-iv.txt | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 ChangeLog.d/fix-cipher-iv.txt diff --git a/ChangeLog.d/fix-cipher-iv.txt b/ChangeLog.d/fix-cipher-iv.txt new file mode 100644 index 0000000000..e7af6414ac --- /dev/null +++ b/ChangeLog.d/fix-cipher-iv.txt @@ -0,0 +1,5 @@ +Security + * In psa_cipher_generate_iv() and psa_cipher_encrypt(), do not read back + from the output buffer. This fixes a potential policy bypass or decryption + oracle vulnerability if the output buffer is in memory that is shared with + an untrusted application. From a393619dc2f69e33a69097444c0f5c4e78243a9c Mon Sep 17 00:00:00 2001 From: Ronald Cron Date: Mon, 6 Dec 2021 08:38:57 +0100 Subject: [PATCH 12/15] Change test on local nonce buffer size to an assertion Signed-off-by: Ronald Cron --- library/psa_crypto.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index daa34ffa34..d3a2865ab7 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -3893,11 +3893,9 @@ psa_status_t psa_aead_generate_nonce( psa_aead_operation_t *operation, goto exit; } - if( required_nonce_size > sizeof( local_nonce ) ) - { - status = PSA_ERROR_GENERIC_ERROR; - goto exit; - } +#if defined(assert) + assert( required_nonce_size <= sizeof( local_nonce ) ); +#endif status = psa_generate_random( local_nonce, required_nonce_size ); if( status != PSA_SUCCESS ) From 01186270137225b812b18afb53acd66bc88002eb Mon Sep 17 00:00:00 2001 From: Ronald Cron Date: Thu, 2 Dec 2021 11:26:07 +0100 Subject: [PATCH 13/15] Add change log Signed-off-by: Ronald Cron --- ChangeLog.d/fix-aead-nonce.txt | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 ChangeLog.d/fix-aead-nonce.txt diff --git a/ChangeLog.d/fix-aead-nonce.txt b/ChangeLog.d/fix-aead-nonce.txt new file mode 100644 index 0000000000..767cc1d4a2 --- /dev/null +++ b/ChangeLog.d/fix-aead-nonce.txt @@ -0,0 +1,5 @@ +Security + * In psa_aead_generate_nonce(), do not read back from the output buffer. + This fixes a potential policy bypass or decryption oracle vulnerability + if the output buffer is in memory that is shared with an untrusted + application. From 0b4d12313a6814ce45ffc42972539676effabb55 Mon Sep 17 00:00:00 2001 From: Ronald Cron Date: Tue, 7 Dec 2021 10:45:00 +0100 Subject: [PATCH 14/15] Remove assertion on local nonce buffer size Signed-off-by: Ronald Cron --- library/psa_crypto.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index d3a2865ab7..c2b4e48d34 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -3893,10 +3893,6 @@ psa_status_t psa_aead_generate_nonce( psa_aead_operation_t *operation, goto exit; } -#if defined(assert) - assert( required_nonce_size <= sizeof( local_nonce ) ); -#endif - status = psa_generate_random( local_nonce, required_nonce_size ); if( status != PSA_SUCCESS ) goto exit; From e217edf49c7557f6f0a76b93c44f3d170ac5d603 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 8 Dec 2021 13:28:12 +0000 Subject: [PATCH 15/15] Add changelog entry for session copy bugfix Signed-off-by: David Horstmann --- ChangeLog.d/fix-session-copy-bug.txt | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 ChangeLog.d/fix-session-copy-bug.txt diff --git a/ChangeLog.d/fix-session-copy-bug.txt b/ChangeLog.d/fix-session-copy-bug.txt new file mode 100644 index 0000000000..46e3d8ef61 --- /dev/null +++ b/ChangeLog.d/fix-session-copy-bug.txt @@ -0,0 +1,6 @@ +Bugfix + * Fix a double-free that happened after mbedtls_ssl_set_session() or + mbedtls_ssl_get_session() failed with MBEDTLS_ERR_SSL_ALLOC_FAILED + (out of memory). After that, calling mbedtls_ssl_session_free() + and mbedtls_ssl_free() would cause an internal session buffer to + be free()'d twice.