From 8db8d1a83e1e64ef74f3aa360b48d1d793042146 Mon Sep 17 00:00:00 2001 From: Thomas Daubney Date: Thu, 18 Jan 2024 13:18:58 +0000 Subject: [PATCH 1/7] Implement safe buffer copying in MAC API Use buffer local copy macros to implement safe copy mechanism in MAC API. Signed-off-by: Thomas Daubney --- library/psa_crypto.c | 57 ++++++++++++++++++++++++++++++++++---------- 1 file changed, 45 insertions(+), 12 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 57844c5b76..bd67887e32 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -2652,35 +2652,45 @@ psa_status_t psa_mac_verify_setup(psa_mac_operation_t *operation, } psa_status_t psa_mac_update(psa_mac_operation_t *operation, - const uint8_t *input, + const uint8_t *input_external, size_t input_length) { + psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED; + LOCAL_INPUT_DECLARE(input_external, input); + if (operation->id == 0) { - return PSA_ERROR_BAD_STATE; + status = PSA_ERROR_BAD_STATE; + return status; } /* Don't require hash implementations to behave correctly on a * zero-length input, which may have an invalid pointer. */ if (input_length == 0) { - return PSA_SUCCESS; + status = PSA_SUCCESS; + return status; } - psa_status_t status = psa_driver_wrapper_mac_update(operation, - input, input_length); + LOCAL_INPUT_ALLOC(input_external, input_length, input); + status = psa_driver_wrapper_mac_update(operation, input, input_length); + if (status != PSA_SUCCESS) { psa_mac_abort(operation); } +exit: + LOCAL_INPUT_FREE(input_external, input); + return status; } psa_status_t psa_mac_sign_finish(psa_mac_operation_t *operation, - uint8_t *mac, + uint8_t *mac_external, size_t mac_size, size_t *mac_length) { psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED; psa_status_t abort_status = PSA_ERROR_CORRUPTION_DETECTED; + LOCAL_OUTPUT_DECLARE(mac_external, mac); if (operation->id == 0) { status = PSA_ERROR_BAD_STATE; @@ -2704,6 +2714,7 @@ psa_status_t psa_mac_sign_finish(psa_mac_operation_t *operation, goto exit; } + LOCAL_OUTPUT_ALLOC(mac_external, mac_size, mac); status = psa_driver_wrapper_mac_sign_finish(operation, mac, operation->mac_size, mac_length); @@ -2723,16 +2734,18 @@ exit: psa_wipe_tag_output_buffer(mac, status, mac_size, *mac_length); abort_status = psa_mac_abort(operation); + LOCAL_OUTPUT_FREE(mac_external, mac); return status == PSA_SUCCESS ? abort_status : status; } psa_status_t psa_mac_verify_finish(psa_mac_operation_t *operation, - const uint8_t *mac, + const uint8_t *mac_external, size_t mac_length) { psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED; psa_status_t abort_status = PSA_ERROR_CORRUPTION_DETECTED; + LOCAL_INPUT_DECLARE(mac_external, mac); if (operation->id == 0) { status = PSA_ERROR_BAD_STATE; @@ -2749,11 +2762,13 @@ psa_status_t psa_mac_verify_finish(psa_mac_operation_t *operation, goto exit; } + LOCAL_INPUT_ALLOC(mac_external, mac_length, mac); status = psa_driver_wrapper_mac_verify_finish(operation, mac, mac_length); exit: abort_status = psa_mac_abort(operation); + LOCAL_INPUT_FREE(mac_external, mac); return status == PSA_SUCCESS ? abort_status : status; } @@ -2825,28 +2840,42 @@ exit: psa_status_t psa_mac_compute(mbedtls_svc_key_id_t key, psa_algorithm_t alg, - const uint8_t *input, + const uint8_t *input_external, size_t input_length, - uint8_t *mac, + uint8_t *mac_external, size_t mac_size, size_t *mac_length) { - return psa_mac_compute_internal(key, alg, + psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED; + LOCAL_INPUT_DECLARE(input_external, input); + LOCAL_OUTPUT_DECLARE(mac_external, mac); + + LOCAL_INPUT_ALLOC(input_external, input_length, input); + LOCAL_OUTPUT_ALLOC(mac_external, mac_size, mac); + status = psa_mac_compute_internal(key, alg, input, input_length, mac, mac_size, mac_length, 1); +exit: + LOCAL_INPUT_FREE(input_external, input); + LOCAL_OUTPUT_FREE(mac_external, mac); + + return status; } psa_status_t psa_mac_verify(mbedtls_svc_key_id_t key, psa_algorithm_t alg, - const uint8_t *input, + const uint8_t *input_external, size_t input_length, - const uint8_t *mac, + const uint8_t *mac_external, size_t mac_length) { psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED; uint8_t actual_mac[PSA_MAC_MAX_SIZE]; size_t actual_mac_length; + LOCAL_INPUT_DECLARE(input_external, input); + LOCAL_INPUT_DECLARE(mac_external, mac); + LOCAL_INPUT_ALLOC(input_external, input_length, input); status = psa_mac_compute_internal(key, alg, input, input_length, actual_mac, sizeof(actual_mac), @@ -2859,6 +2888,8 @@ psa_status_t psa_mac_verify(mbedtls_svc_key_id_t key, status = PSA_ERROR_INVALID_SIGNATURE; goto exit; } + + LOCAL_INPUT_ALLOC(mac_external, mac_length, mac); if (mbedtls_ct_memcmp(mac, actual_mac, actual_mac_length) != 0) { status = PSA_ERROR_INVALID_SIGNATURE; goto exit; @@ -2866,6 +2897,8 @@ psa_status_t psa_mac_verify(mbedtls_svc_key_id_t key, exit: mbedtls_platform_zeroize(actual_mac, sizeof(actual_mac)); + LOCAL_INPUT_FREE(input_external, input); + LOCAL_INPUT_FREE(mac_external, mac); return status; } From a1cf1010cc7e213abb84ea320b4cec62812df434 Mon Sep 17 00:00:00 2001 From: Thomas Daubney Date: Tue, 30 Jan 2024 11:18:54 +0000 Subject: [PATCH 2/7] Generate test wrappers for mac functions Signed-off-by: Thomas Daubney --- tests/scripts/generate_psa_wrappers.py | 6 +++++ tests/src/psa_test_wrappers.c | 34 ++++++++++++++++++++++++++ 2 files changed, 40 insertions(+) diff --git a/tests/scripts/generate_psa_wrappers.py b/tests/scripts/generate_psa_wrappers.py index 3cdafed167..1f3b127e94 100755 --- a/tests/scripts/generate_psa_wrappers.py +++ b/tests/scripts/generate_psa_wrappers.py @@ -154,6 +154,12 @@ class PSAWrapperGenerator(c_wrapper_generator.Base): 'psa_sign_hash', 'psa_verify_hash'): return True + if function_name in ('psa_mac_update', + 'psa_mac_sign_finish', + 'psa_mac_verify_finish', + 'psa_mac_compute', + 'psa_mac_verify'): + 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 bb1409e10b..333a85c5ce 100644 --- a/tests/src/psa_test_wrappers.c +++ b/tests/src/psa_test_wrappers.c @@ -704,7 +704,15 @@ psa_status_t mbedtls_test_wrap_psa_mac_compute( size_t arg5_mac_size, size_t *arg6_mac_length) { +#if defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) + MBEDTLS_TEST_MEMORY_POISON(arg2_input, arg3_input_length); + MBEDTLS_TEST_MEMORY_POISON(arg4_mac, arg5_mac_size); +#endif /* defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) */ psa_status_t status = (psa_mac_compute)(arg0_key, arg1_alg, arg2_input, arg3_input_length, arg4_mac, arg5_mac_size, arg6_mac_length); +#if defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) + MBEDTLS_TEST_MEMORY_UNPOISON(arg2_input, arg3_input_length); + MBEDTLS_TEST_MEMORY_UNPOISON(arg4_mac, arg5_mac_size); +#endif /* defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) */ return status; } @@ -715,7 +723,13 @@ psa_status_t mbedtls_test_wrap_psa_mac_sign_finish( size_t arg2_mac_size, size_t *arg3_mac_length) { +#if defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) + MBEDTLS_TEST_MEMORY_POISON(arg1_mac, arg2_mac_size); +#endif /* defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) */ psa_status_t status = (psa_mac_sign_finish)(arg0_operation, arg1_mac, arg2_mac_size, arg3_mac_length); +#if defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) + MBEDTLS_TEST_MEMORY_UNPOISON(arg1_mac, arg2_mac_size); +#endif /* defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) */ return status; } @@ -735,7 +749,13 @@ psa_status_t mbedtls_test_wrap_psa_mac_update( const uint8_t *arg1_input, size_t arg2_input_length) { +#if defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) + MBEDTLS_TEST_MEMORY_POISON(arg1_input, arg2_input_length); +#endif /* defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) */ psa_status_t status = (psa_mac_update)(arg0_operation, arg1_input, arg2_input_length); +#if defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) + MBEDTLS_TEST_MEMORY_UNPOISON(arg1_input, arg2_input_length); +#endif /* defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) */ return status; } @@ -748,7 +768,15 @@ psa_status_t mbedtls_test_wrap_psa_mac_verify( const uint8_t *arg4_mac, size_t arg5_mac_length) { +#if defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) + MBEDTLS_TEST_MEMORY_POISON(arg2_input, arg3_input_length); + MBEDTLS_TEST_MEMORY_POISON(arg4_mac, arg5_mac_length); +#endif /* defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) */ psa_status_t status = (psa_mac_verify)(arg0_key, arg1_alg, arg2_input, arg3_input_length, arg4_mac, arg5_mac_length); +#if defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) + MBEDTLS_TEST_MEMORY_UNPOISON(arg2_input, arg3_input_length); + MBEDTLS_TEST_MEMORY_UNPOISON(arg4_mac, arg5_mac_length); +#endif /* defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) */ return status; } @@ -758,7 +786,13 @@ psa_status_t mbedtls_test_wrap_psa_mac_verify_finish( const uint8_t *arg1_mac, size_t arg2_mac_length) { +#if defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) + MBEDTLS_TEST_MEMORY_POISON(arg1_mac, arg2_mac_length); +#endif /* defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) */ psa_status_t status = (psa_mac_verify_finish)(arg0_operation, arg1_mac, arg2_mac_length); +#if defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) + MBEDTLS_TEST_MEMORY_UNPOISON(arg1_mac, arg2_mac_length); +#endif /* defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) */ return status; } From c6705c6cb2369c69edef2c905edfcb5b6e411058 Mon Sep 17 00:00:00 2001 From: Thomas Daubney Date: Tue, 30 Jan 2024 11:21:47 +0000 Subject: [PATCH 3/7] Conditionally include exit label ... on MAC functions where the label was only added due to the modifications required by this PR. Signed-off-by: Thomas Daubney --- library/psa_crypto.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index bd67887e32..9db697787c 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -2677,7 +2677,9 @@ psa_status_t psa_mac_update(psa_mac_operation_t *operation, psa_mac_abort(operation); } +#if defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) exit: +#endif LOCAL_INPUT_FREE(input_external, input); return status; @@ -2855,7 +2857,10 @@ psa_status_t psa_mac_compute(mbedtls_svc_key_id_t key, status = psa_mac_compute_internal(key, alg, input, input_length, mac, mac_size, mac_length, 1); + +#if defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) exit: +#endif LOCAL_INPUT_FREE(input_external, input); LOCAL_OUTPUT_FREE(mac_external, mac); From 7480a74cba8528faee4b4e31e68106746b7c2b03 Mon Sep 17 00:00:00 2001 From: Thomas Daubney Date: Tue, 30 Jan 2024 11:29:47 +0000 Subject: [PATCH 4/7] Fix code style Signed-off-by: Thomas Daubney --- library/psa_crypto.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 9db697787c..356137dc09 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -2855,8 +2855,8 @@ psa_status_t psa_mac_compute(mbedtls_svc_key_id_t key, LOCAL_INPUT_ALLOC(input_external, input_length, input); LOCAL_OUTPUT_ALLOC(mac_external, mac_size, mac); status = psa_mac_compute_internal(key, alg, - input, input_length, - mac, mac_size, mac_length, 1); + input, input_length, + mac, mac_size, mac_length, 1); #if defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) exit: From 1ffc5cb4a5cf709f612bb7c858cfd2e183142d27 Mon Sep 17 00:00:00 2001 From: Thomas Daubney Date: Wed, 31 Jan 2024 18:09:36 +0000 Subject: [PATCH 5/7] Modify allocation and buffer wiping in sign_finish Allocate immediately after declaration and only wipe tag buffer if allocation didn't fail. Signed-off-by: Thomas Daubney --- library/psa_crypto.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 356137dc09..a9456fd0ea 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -2693,6 +2693,7 @@ psa_status_t psa_mac_sign_finish(psa_mac_operation_t *operation, psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED; psa_status_t abort_status = PSA_ERROR_CORRUPTION_DETECTED; LOCAL_OUTPUT_DECLARE(mac_external, mac); + LOCAL_OUTPUT_ALLOC(mac_external, mac_size, mac); if (operation->id == 0) { status = PSA_ERROR_BAD_STATE; @@ -2716,7 +2717,7 @@ psa_status_t psa_mac_sign_finish(psa_mac_operation_t *operation, goto exit; } - LOCAL_OUTPUT_ALLOC(mac_external, mac_size, mac); + status = psa_driver_wrapper_mac_sign_finish(operation, mac, operation->mac_size, mac_length); @@ -2733,7 +2734,9 @@ exit: operation->mac_size = 0; } - psa_wipe_tag_output_buffer(mac, status, mac_size, *mac_length); + if (status != PSA_ERROR_INSUFFICIENT_MEMORY) { + psa_wipe_tag_output_buffer(mac, status, mac_size, *mac_length); + } abort_status = psa_mac_abort(operation); LOCAL_OUTPUT_FREE(mac_external, mac); From 03f1ea3624831f94d7ef9d389a9991632077aee1 Mon Sep 17 00:00:00 2001 From: Thomas Daubney Date: Thu, 1 Feb 2024 16:16:27 +0000 Subject: [PATCH 6/7] Change condition on wiping tag buffer 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 a9456fd0ea..5621cec980 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -2734,7 +2734,7 @@ exit: operation->mac_size = 0; } - if (status != PSA_ERROR_INSUFFICIENT_MEMORY) { + if (mac != NULL) { psa_wipe_tag_output_buffer(mac, status, mac_size, *mac_length); } From 4a46d73bb0ef5bfb16c19973d8fec894aa42a9ff Mon Sep 17 00:00:00 2001 From: Thomas Daubney Date: Mon, 26 Feb 2024 13:49:26 +0000 Subject: [PATCH 7/7] Suppress pylint Signed-off-by: Thomas Daubney --- tests/scripts/generate_psa_wrappers.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/scripts/generate_psa_wrappers.py b/tests/scripts/generate_psa_wrappers.py index ef28cbf18b..0918dccb24 100755 --- a/tests/scripts/generate_psa_wrappers.py +++ b/tests/scripts/generate_psa_wrappers.py @@ -142,6 +142,7 @@ class PSAWrapperGenerator(c_wrapper_generator.Base): _buffer_name: Optional[str]) -> bool: """Whether the specified buffer argument to a PSA function should be copied. """ + #pylint: disable=too-many-return-statements if function_name.startswith('psa_aead'): return True if function_name == 'psa_cipher_encrypt':