From 4304276539eb0457d38d36c2b131ccdec2d99f3a Mon Sep 17 00:00:00 2001 From: Thomas Daubney Date: Thu, 15 Feb 2024 12:57:26 +0000 Subject: [PATCH 1/9] Add buffer protection to psa_raw_key_agreement Signed-off-by: Thomas Daubney --- library/psa_crypto.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index d30462b6e..16121926b 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -5278,9 +5278,9 @@ psa_status_t psa_key_derivation_key_agreement(psa_key_derivation_operation_t *op psa_status_t psa_raw_key_agreement(psa_algorithm_t alg, mbedtls_svc_key_id_t private_key, - const uint8_t *peer_key, + const uint8_t *peer_key_external, size_t peer_key_length, - uint8_t *output, + uint8_t *output_external, size_t output_size, size_t *output_length) { @@ -5288,6 +5288,8 @@ psa_status_t psa_raw_key_agreement(psa_algorithm_t alg, psa_status_t unlock_status = PSA_ERROR_CORRUPTION_DETECTED; psa_key_slot_t *slot = NULL; size_t expected_length; + LOCAL_INPUT_DECLARE(peer_key_external, peer_key); + LOCAL_OUTPUT_DECLARE(output_external, output); if (!PSA_ALG_IS_KEY_AGREEMENT(alg)) { status = PSA_ERROR_INVALID_ARGUMENT; @@ -5314,6 +5316,8 @@ psa_status_t psa_raw_key_agreement(psa_algorithm_t alg, goto exit; } + LOCAL_INPUT_ALLOC(peer_key_external, peer_key_length, peer_key); + LOCAL_OUTPUT_ALLOC(output_external, output_size, output); status = psa_key_agreement_raw_internal(alg, slot, peer_key, peer_key_length, output, output_size, @@ -5334,6 +5338,8 @@ exit: unlock_status = psa_unlock_key_slot(slot); + LOCAL_INPUT_FREE(peer_key_external, peer_key); + LOCAL_OUTPUT_FREE(output_external, output); return (status == PSA_SUCCESS) ? unlock_status : status; } From 9da359fc659936edd0d18be93cce00b89f169245 Mon Sep 17 00:00:00 2001 From: Thomas Daubney Date: Thu, 15 Feb 2024 13:15:47 +0000 Subject: [PATCH 2/9] Add buffer protection to psa_key_derivation_key_agreement Signed-off-by: Thomas Daubney --- library/psa_crypto.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 16121926b..d1973a59a 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -5243,12 +5243,13 @@ exit: psa_status_t psa_key_derivation_key_agreement(psa_key_derivation_operation_t *operation, psa_key_derivation_step_t step, mbedtls_svc_key_id_t private_key, - const uint8_t *peer_key, + const uint8_t *peer_key_external, size_t peer_key_length) { psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED; psa_status_t unlock_status = PSA_ERROR_CORRUPTION_DETECTED; psa_key_slot_t *slot; + LOCAL_INPUT_DECLARE(peer_key_external, peer_key); if (!PSA_ALG_IS_KEY_AGREEMENT(operation->alg)) { return PSA_ERROR_INVALID_ARGUMENT; @@ -5258,9 +5259,13 @@ psa_status_t psa_key_derivation_key_agreement(psa_key_derivation_operation_t *op if (status != PSA_SUCCESS) { return status; } + + LOCAL_INPUT_ALLOC(peer_key_external, peer_key_length, peer_key) status = psa_key_agreement_internal(operation, step, slot, peer_key, peer_key_length); + +exit: if (status != PSA_SUCCESS) { psa_key_derivation_abort(operation); } else { @@ -5273,6 +5278,7 @@ psa_status_t psa_key_derivation_key_agreement(psa_key_derivation_operation_t *op unlock_status = psa_unlock_key_slot(slot); + LOCAL_INPUT_FREE(peer_key_external, peer_key); return (status == PSA_SUCCESS) ? unlock_status : status; } From db5d607cb170560a98d3fca09f5a81e15dbbf35b Mon Sep 17 00:00:00 2001 From: Thomas Daubney Date: Thu, 15 Feb 2024 14:18:02 +0000 Subject: [PATCH 3/9] Generate test wrappers Signed-off-by: Thomas Daubney --- tests/scripts/generate_psa_wrappers.py | 3 +++ tests/src/psa_test_wrappers.c | 14 ++++++++++++++ 2 files changed, 17 insertions(+) diff --git a/tests/scripts/generate_psa_wrappers.py b/tests/scripts/generate_psa_wrappers.py index 0b7cf4429..92c931092 100755 --- a/tests/scripts/generate_psa_wrappers.py +++ b/tests/scripts/generate_psa_wrappers.py @@ -171,6 +171,9 @@ class PSAWrapperGenerator(c_wrapper_generator.Base): 'psa_hash_compute', 'psa_hash_compare'): return True + if function_name in ('psa_key_derivation_key_agreement', + 'psa_raw_key_agreement'): + return True return False def _write_function_call(self, out: typing_util.Writable, diff --git a/tests/src/psa_test_wrappers.c b/tests/src/psa_test_wrappers.c index c85b65d06..efb9f89cc 100644 --- a/tests/src/psa_test_wrappers.c +++ b/tests/src/psa_test_wrappers.c @@ -500,7 +500,13 @@ psa_status_t mbedtls_test_wrap_psa_key_derivation_key_agreement( const uint8_t *arg3_peer_key, size_t arg4_peer_key_length) { +#if defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) + MBEDTLS_TEST_MEMORY_POISON(arg3_peer_key, arg4_peer_key_length); +#endif /* defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) */ psa_status_t status = (psa_key_derivation_key_agreement)(arg0_operation, arg1_step, arg2_private_key, arg3_peer_key, arg4_peer_key_length); +#if defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) + MBEDTLS_TEST_MEMORY_UNPOISON(arg3_peer_key, arg4_peer_key_length); +#endif /* defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) */ return status; } @@ -652,7 +658,15 @@ psa_status_t mbedtls_test_wrap_psa_raw_key_agreement( size_t arg5_output_size, size_t *arg6_output_length) { +#if defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) + MBEDTLS_TEST_MEMORY_POISON(arg2_peer_key, arg3_peer_key_length); + MBEDTLS_TEST_MEMORY_POISON(arg4_output, arg5_output_size); +#endif /* defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) */ psa_status_t status = (psa_raw_key_agreement)(arg0_alg, arg1_private_key, arg2_peer_key, arg3_peer_key_length, arg4_output, arg5_output_size, arg6_output_length); +#if defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) + MBEDTLS_TEST_MEMORY_UNPOISON(arg2_peer_key, arg3_peer_key_length); + MBEDTLS_TEST_MEMORY_UNPOISON(arg4_output, arg5_output_size); +#endif /* defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) */ return status; } From 3c0c6b1c4beaa75256eb7823d853ae30019035de Mon Sep 17 00:00:00 2001 From: Thomas Daubney Date: Thu, 15 Feb 2024 14:25:08 +0000 Subject: [PATCH 4/9] Conditionally include exit label Signed-off-by: Thomas Daubney --- library/psa_crypto.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index d1973a59a..319ad8312 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -5265,7 +5265,9 @@ psa_status_t psa_key_derivation_key_agreement(psa_key_derivation_operation_t *op slot, peer_key, peer_key_length); +#if defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) exit: +#endif if (status != PSA_SUCCESS) { psa_key_derivation_abort(operation); } else { From 26d1c43821c0a4d5cc3edbae15ae5f9848f06409 Mon Sep 17 00:00:00 2001 From: Thomas Daubney Date: Tue, 20 Feb 2024 11:26:55 +0000 Subject: [PATCH 5/9] Check output allocated before randomising Signed-off-by: Thomas Daubney --- library/psa_crypto.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 319ad8312..994f9a950 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -5332,7 +5332,7 @@ psa_status_t psa_raw_key_agreement(psa_algorithm_t alg, output_length); exit: - if (status != PSA_SUCCESS) { + if (status != PSA_SUCCESS && output != NULL) { /* If an error happens and is not handled properly, the output * may be used as a key to protect sensitive data. Arrange for such * a key to be random, which is likely to result in decryption or From 0736df33ac8c893c3e1f10d1c231754b803c3b95 Mon Sep 17 00:00:00 2001 From: Thomas Daubney Date: Wed, 21 Feb 2024 12:28:20 +0000 Subject: [PATCH 6/9] Check for output allocation before randomising Signed-off-by: Thomas Daubney --- library/psa_crypto.c | 29 ++++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 994f9a950..964ae51bf 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -5298,6 +5298,7 @@ psa_status_t psa_raw_key_agreement(psa_algorithm_t alg, size_t expected_length; LOCAL_INPUT_DECLARE(peer_key_external, peer_key); LOCAL_OUTPUT_DECLARE(output_external, output); + LOCAL_OUTPUT_ALLOC(output_external, output_size, output); if (!PSA_ALG_IS_KEY_AGREEMENT(alg)) { status = PSA_ERROR_INVALID_ARGUMENT; @@ -5325,23 +5326,29 @@ psa_status_t psa_raw_key_agreement(psa_algorithm_t alg, } LOCAL_INPUT_ALLOC(peer_key_external, peer_key_length, peer_key); - LOCAL_OUTPUT_ALLOC(output_external, output_size, output); status = psa_key_agreement_raw_internal(alg, slot, peer_key, peer_key_length, output, output_size, output_length); exit: - if (status != PSA_SUCCESS && output != NULL) { - /* If an error happens and is not handled properly, the output - * may be used as a key to protect sensitive data. Arrange for such - * a key to be random, which is likely to result in decryption or - * verification errors. This is better than filling the buffer with - * some constant data such as zeros, which would result in the data - * being protected with a reproducible, easily knowable key. - */ - psa_generate_random(output, output_size); - *output_length = output_size; + /* Check for successful allocation of output. */ + if (output != NULL && status != PSA_ERROR_INSUFFICIENT_MEMORY) { + /* output allocated. */ + if (status != PSA_SUCCESS) { + /* If an error happens and is not handled properly, the output + * may be used as a key to protect sensitive data. Arrange for such + * a key to be random, which is likely to result in decryption or + * verification errors. This is better than filling the buffer with + * some constant data such as zeros, which would result in the data + * being protected with a reproducible, easily knowable key. + */ + psa_generate_random(output, output_size); + *output_length = output_size; + } + } else { + /* output allocation failed. */ + *output_length = 0; } unlock_status = psa_unlock_key_slot(slot); From 2ea8d8fa3c1f95fa16cc4affa4cf858a4f75f632 Mon Sep 17 00:00:00 2001 From: Thomas Daubney Date: Wed, 21 Feb 2024 15:16:01 +0000 Subject: [PATCH 7/9] Revise how output allocation is checked Signed-off-by: Thomas Daubney --- library/psa_crypto.c | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 964ae51bf..dac487efd 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -5332,20 +5332,16 @@ psa_status_t psa_raw_key_agreement(psa_algorithm_t alg, output_length); exit: - /* Check for successful allocation of output. */ - if (output != NULL && status != PSA_ERROR_INSUFFICIENT_MEMORY) { - /* output allocated. */ - if (status != PSA_SUCCESS) { - /* If an error happens and is not handled properly, the output - * may be used as a key to protect sensitive data. Arrange for such - * a key to be random, which is likely to result in decryption or - * verification errors. This is better than filling the buffer with - * some constant data such as zeros, which would result in the data - * being protected with a reproducible, easily knowable key. - */ - psa_generate_random(output, output_size); - *output_length = output_size; - } + if (output != NULL && status != PSA_SUCCESS) { + /* If an error happens and is not handled properly, the output + * may be used as a key to protect sensitive data. Arrange for such + * a key to be random, which is likely to result in decryption or + * verification errors. This is better than filling the buffer with + * some constant data such as zeros, which would result in the data + * being protected with a reproducible, easily knowable key. + */ + psa_generate_random(output, output_size); + *output_length = output_size; } else { /* output allocation failed. */ *output_length = 0; From 09cf4f2e78e1dc93d8bb30e77f6f543dcec2caba Mon Sep 17 00:00:00 2001 From: Thomas Daubney Date: Thu, 22 Feb 2024 11:08:22 +0000 Subject: [PATCH 8/9] Decouple if statement in psa_raw_key_agreement exit. Signed-off-by: Thomas Daubney --- library/psa_crypto.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index dac487efd..e0b91c7f5 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -5332,17 +5332,21 @@ psa_status_t psa_raw_key_agreement(psa_algorithm_t alg, output_length); exit: + /* Check for successful allocation of output, + * with an unsuccessful status. */ if (output != NULL && status != PSA_SUCCESS) { /* If an error happens and is not handled properly, the output - * may be used as a key to protect sensitive data. Arrange for such - * a key to be random, which is likely to result in decryption or - * verification errors. This is better than filling the buffer with - * some constant data such as zeros, which would result in the data - * being protected with a reproducible, easily knowable key. - */ + * may be used as a key to protect sensitive data. Arrange for such + * a key to be random, which is likely to result in decryption or + * verification errors. This is better than filling the buffer with + * some constant data such as zeros, which would result in the data + * being protected with a reproducible, easily knowable key. + */ psa_generate_random(output, output_size); *output_length = output_size; - } else { + } + + if (output == NULL) { /* output allocation failed. */ *output_length = 0; } From 9d0fe6e8df83dc4b50ad3412a4aa6b6814dfb465 Mon Sep 17 00:00:00 2001 From: Thomas Daubney Date: Wed, 6 Mar 2024 17:34:35 +0000 Subject: [PATCH 9/9] Fix issue with large allocation in tests In test_suite_psa_crypto_op_fail.generated.function the function key_agreement_fail was setting the public_key_length variable to SIZE_MAX which meant that a huge allocation was being attempted. Signed-off-by: Thomas Daubney --- tests/suites/test_suite_psa_crypto_op_fail.function | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/suites/test_suite_psa_crypto_op_fail.function b/tests/suites/test_suite_psa_crypto_op_fail.function index 0d5d53890..4e709a0a8 100644 --- a/tests/suites/test_suite_psa_crypto_op_fail.function +++ b/tests/suites/test_suite_psa_crypto_op_fail.function @@ -332,9 +332,9 @@ void key_agreement_fail(int key_type_arg, data_t *key_data, psa_key_attributes_t attributes = PSA_KEY_ATTRIBUTES_INIT; mbedtls_svc_key_id_t key_id = MBEDTLS_SVC_KEY_ID_INIT; uint8_t public_key[PSA_EXPORT_PUBLIC_KEY_MAX_SIZE] = { 0 }; - size_t public_key_length = SIZE_MAX; + size_t public_key_length = 0; uint8_t output[PSA_SIGNATURE_MAX_SIZE] = { 0 }; - size_t length = SIZE_MAX; + size_t length = 0; psa_key_derivation_operation_t operation = PSA_KEY_DERIVATION_OPERATION_INIT; PSA_INIT();