From 81899aba115f815410bcf943122abbfd21a7890d 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 ca10c1466e..e3706b896a 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -7376,9 +7376,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) { @@ -7386,6 +7386,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; @@ -7412,6 +7414,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, @@ -7432,6 +7436,8 @@ exit: unlock_status = psa_unregister_read(slot); + LOCAL_INPUT_FREE(peer_key_external, peer_key); + LOCAL_OUTPUT_FREE(output_external, output); return (status == PSA_SUCCESS) ? unlock_status : status; } From 9739ac047abd97b4a18b7cebc232b15c406cfed4 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 | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index e3706b896a..e64d172593 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -7341,12 +7341,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; @@ -7356,9 +7357,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 { @@ -7370,7 +7375,7 @@ psa_status_t psa_key_derivation_key_agreement(psa_key_derivation_operation_t *op } unlock_status = psa_unregister_read(slot); - + LOCAL_INPUT_FREE(peer_key_external, peer_key); return (status == PSA_SUCCESS) ? unlock_status : status; } From fe2bda32572ed90813f6c2c1181593ac82a684e5 Mon Sep 17 00:00:00 2001 From: Thomas Daubney Date: Thu, 15 Feb 2024 13:35:06 +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 c65603b3b7..b56bd34ce5 100755 --- a/tests/scripts/generate_psa_wrappers.py +++ b/tests/scripts/generate_psa_wrappers.py @@ -163,6 +163,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 728afed3de..35500b597e 100644 --- a/tests/src/psa_test_wrappers.c +++ b/tests/src/psa_test_wrappers.c @@ -684,7 +684,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; } @@ -923,7 +929,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 50f58fc3e476d50233d1288548920938558da965 Mon Sep 17 00:00:00 2001 From: Thomas Daubney Date: Thu, 15 Feb 2024 14:24:03 +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 e64d172593..b78e8696bb 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -7363,7 +7363,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 d997e7ad9a1462d6241d3e817b2c467e2c554a0c Mon Sep 17 00:00:00 2001 From: Thomas Daubney Date: Tue, 20 Feb 2024 11:24:07 +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 b78e8696bb..df43593934 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -7429,7 +7429,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 89d8c2a1b4c98058d7f12f8c219ef03bc6d2881a Mon Sep 17 00:00:00 2001 From: Thomas Daubney Date: Wed, 21 Feb 2024 11:16:16 +0000 Subject: [PATCH 6/9] Rework check for failed output allocation 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 df43593934..67ac79e90b 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -7395,6 +7395,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; @@ -7422,23 +7423,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_unregister_read(slot); From 0576a6a174cab8b4d090e400bec2fbc875f47d9c Mon Sep 17 00:00:00 2001 From: Thomas Daubney Date: Wed, 21 Feb 2024 15:15:00 +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 67ac79e90b..0e000b3d25 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -7429,20 +7429,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 5390acada9a017e0df672ddccfc8c33cf7c84821 Mon Sep 17 00:00:00 2001 From: Thomas Daubney Date: Thu, 22 Feb 2024 11:06:04 +0000 Subject: [PATCH 8/9] Decouple if statements 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 0e000b3d25..ca8cf2de55 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -7429,17 +7429,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 a4866945b83833c6f4624ce7da463148f2527622 Mon Sep 17 00:00:00 2001 From: Thomas Daubney Date: Wed, 6 Mar 2024 16:32:25 +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 20942bf81f..9878237211 100644 --- a/tests/suites/test_suite_psa_crypto_op_fail.function +++ b/tests/suites/test_suite_psa_crypto_op_fail.function @@ -359,9 +359,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_RAW_KEY_AGREEMENT_OUTPUT_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();