From 4a523a608e67eeffebed1f30a3a9f77d3f977850 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Mon, 11 Mar 2024 13:32:16 +0000 Subject: [PATCH 1/5] Add buffer copying to psa_sign_hash_start/complete Add buffer protection to: * psa_sign_hash_start(), which takes an input buffer for the hash. * psa_sign_hash_complete(), which takes an output buffer for the calculated signature. Signed-off-by: David Horstmann --- library/psa_crypto.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 65381755d..06e720d5a 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -3556,13 +3556,15 @@ static psa_status_t psa_sign_hash_abort_internal( psa_status_t psa_sign_hash_start( psa_sign_hash_interruptible_operation_t *operation, mbedtls_svc_key_id_t key, psa_algorithm_t alg, - const uint8_t *hash, size_t hash_length) + const uint8_t *hash_external, size_t hash_length) { psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED; psa_status_t unlock_status = PSA_ERROR_CORRUPTION_DETECTED; psa_key_slot_t *slot; psa_key_attributes_t attributes; + LOCAL_INPUT_DECLARE(hash_external, hash); + /* Check that start has not been previously called, or operation has not * previously errored. */ if (operation->id != 0 || operation->error_occurred) { @@ -3588,6 +3590,8 @@ psa_status_t psa_sign_hash_start( goto exit; } + LOCAL_INPUT_ALLOC(hash_external, hash_length, hash); + attributes = (psa_key_attributes_t) { .core = slot->attr }; @@ -3612,17 +3616,21 @@ exit: operation->error_occurred = 1; } + LOCAL_INPUT_FREE(hash_external, hash); + return (status == PSA_SUCCESS) ? unlock_status : status; } psa_status_t psa_sign_hash_complete( psa_sign_hash_interruptible_operation_t *operation, - uint8_t *signature, size_t signature_size, + uint8_t *signature_external, size_t signature_size, size_t *signature_length) { psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED; + LOCAL_OUTPUT_DECLARE(signature_external, signature); + *signature_length = 0; /* Check that start has been called first, and that operation has not @@ -3639,6 +3647,8 @@ psa_status_t psa_sign_hash_complete( goto exit; } + LOCAL_OUTPUT_ALLOC(signature_external, signature_size, signature); + status = psa_driver_wrapper_sign_hash_complete(operation, signature, signature_size, signature_length); @@ -3659,6 +3669,8 @@ exit: psa_sign_hash_abort_internal(operation); } + LOCAL_OUTPUT_FREE(signature_external, signature); + return status; } From 0fea6a52b41aa8a88c0f1b6e5aa8371196009d63 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Mon, 11 Mar 2024 13:41:05 +0000 Subject: [PATCH 2/5] Add buffer copying to psa_verify_hash_start() Protect input buffers to psa_verify_hash_start(), namely the hash and signature parameters. Signed-off-by: David Horstmann --- library/psa_crypto.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 06e720d5a..9574cd71d 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -3717,13 +3717,16 @@ static psa_status_t psa_verify_hash_abort_internal( psa_status_t psa_verify_hash_start( psa_verify_hash_interruptible_operation_t *operation, mbedtls_svc_key_id_t key, psa_algorithm_t alg, - const uint8_t *hash, size_t hash_length, - const uint8_t *signature, size_t signature_length) + const uint8_t *hash_external, size_t hash_length, + const uint8_t *signature_external, size_t signature_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(hash_external, hash); + LOCAL_INPUT_DECLARE(signature_external, signature); + /* Check that start has not been previously called, or operation has not * previously errored. */ if (operation->id != 0 || operation->error_occurred) { @@ -3745,6 +3748,9 @@ psa_status_t psa_verify_hash_start( return status; } + LOCAL_INPUT_ALLOC(hash_external, hash_length, hash); + LOCAL_INPUT_ALLOC(signature_external, signature_length, signature); + psa_key_attributes_t attributes = { .core = slot->attr }; @@ -3757,6 +3763,9 @@ psa_status_t psa_verify_hash_start( slot->key.bytes, alg, hash, hash_length, signature, signature_length); +#if defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) +exit: +#endif if (status != PSA_SUCCESS) { operation->error_occurred = 1; @@ -3769,6 +3778,9 @@ psa_status_t psa_verify_hash_start( operation->error_occurred = 1; } + LOCAL_INPUT_FREE(hash_external, hash); + LOCAL_INPUT_FREE(signature_external, signature); + return (status == PSA_SUCCESS) ? unlock_status : status; } From 5d64c6acca134defdaca582c186b6887ed83f9d0 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Mon, 11 Mar 2024 13:58:07 +0000 Subject: [PATCH 3/5] Generate memory poisoning in wrappers Generate memory poisoning code in test wrappers for: * psa_sign_hash_start() * psa_sign_hash_complete() * psa_verify_hash_start() Signed-off-by: David Horstmann --- tests/scripts/generate_psa_wrappers.py | 4 ++++ tests/src/psa_test_wrappers.c | 20 ++++++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/tests/scripts/generate_psa_wrappers.py b/tests/scripts/generate_psa_wrappers.py index a2d878726..4e8ee6c80 100755 --- a/tests/scripts/generate_psa_wrappers.py +++ b/tests/scripts/generate_psa_wrappers.py @@ -143,6 +143,10 @@ class PSAWrapperGenerator(c_wrapper_generator.Base): """Whether the specified buffer argument to a PSA function should be copied. """ #pylint: disable=too-many-return-statements + if function_name in ('psa_sign_hash_start', + 'psa_sign_hash_complete', + 'psa_verify_hash_start'): + return True if function_name.startswith('psa_pake'): return True if function_name.startswith('psa_aead'): diff --git a/tests/src/psa_test_wrappers.c b/tests/src/psa_test_wrappers.c index 71ea09c44..f303af833 100644 --- a/tests/src/psa_test_wrappers.c +++ b/tests/src/psa_test_wrappers.c @@ -1162,7 +1162,13 @@ psa_status_t mbedtls_test_wrap_psa_sign_hash_complete( size_t arg2_signature_size, size_t *arg3_signature_length) { +#if defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) + MBEDTLS_TEST_MEMORY_POISON(arg1_signature, arg2_signature_size); +#endif /* defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) */ psa_status_t status = (psa_sign_hash_complete)(arg0_operation, arg1_signature, arg2_signature_size, arg3_signature_length); +#if defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) + MBEDTLS_TEST_MEMORY_UNPOISON(arg1_signature, arg2_signature_size); +#endif /* defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) */ return status; } @@ -1174,7 +1180,13 @@ psa_status_t mbedtls_test_wrap_psa_sign_hash_start( const uint8_t *arg3_hash, size_t arg4_hash_length) { +#if defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) + MBEDTLS_TEST_MEMORY_POISON(arg3_hash, arg4_hash_length); +#endif /* defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) */ psa_status_t status = (psa_sign_hash_start)(arg0_operation, arg1_key, arg2_alg, arg3_hash, arg4_hash_length); +#if defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) + MBEDTLS_TEST_MEMORY_UNPOISON(arg3_hash, arg4_hash_length); +#endif /* defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) */ return status; } @@ -1247,7 +1259,15 @@ psa_status_t mbedtls_test_wrap_psa_verify_hash_start( const uint8_t *arg5_signature, size_t arg6_signature_length) { +#if defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) + MBEDTLS_TEST_MEMORY_POISON(arg3_hash, arg4_hash_length); + MBEDTLS_TEST_MEMORY_POISON(arg5_signature, arg6_signature_length); +#endif /* defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) */ psa_status_t status = (psa_verify_hash_start)(arg0_operation, arg1_key, arg2_alg, arg3_hash, arg4_hash_length, arg5_signature, arg6_signature_length); +#if defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) + MBEDTLS_TEST_MEMORY_UNPOISON(arg3_hash, arg4_hash_length); + MBEDTLS_TEST_MEMORY_UNPOISON(arg5_signature, arg6_signature_length); +#endif /* defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) */ return status; } From 5ba3f5f7a5aaa518bd1b17b577a4e7d0010e4c9a Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Mon, 11 Mar 2024 15:57:43 +0000 Subject: [PATCH 4/5] Flip logic of generate_psa_wrappers.py Change from a long list of PSA functions to a list of excluded false-positives. Signed-off-by: David Horstmann --- tests/scripts/generate_psa_wrappers.py | 54 ++++---------------------- 1 file changed, 8 insertions(+), 46 deletions(-) diff --git a/tests/scripts/generate_psa_wrappers.py b/tests/scripts/generate_psa_wrappers.py index 4e8ee6c80..1a05c7513 100755 --- a/tests/scripts/generate_psa_wrappers.py +++ b/tests/scripts/generate_psa_wrappers.py @@ -142,52 +142,14 @@ 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 in ('psa_sign_hash_start', - 'psa_sign_hash_complete', - 'psa_verify_hash_start'): - return True - if function_name.startswith('psa_pake'): - return True - if function_name.startswith('psa_aead'): - return True - if function_name in {'psa_cipher_encrypt', 'psa_cipher_decrypt', - 'psa_cipher_update', 'psa_cipher_finish', - 'psa_cipher_generate_iv', 'psa_cipher_set_iv'}: - return True - if function_name in ('psa_key_derivation_output_bytes', - 'psa_key_derivation_input_bytes'): - return True - if function_name in ('psa_import_key', - 'psa_export_key', - 'psa_export_public_key'): - return True - if function_name in ('psa_sign_message', - 'psa_verify_message', - 'psa_sign_hash', - 'psa_verify_hash'): - return True - if function_name in ('psa_hash_update', - 'psa_hash_finish', - 'psa_hash_verify', - 'psa_hash_compute', - 'psa_hash_compare'): - return True - if function_name in ('psa_key_derivation_key_agreement', - 'psa_raw_key_agreement'): - return True - if function_name == 'psa_generate_random': - 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 - if function_name in ('psa_asymmetric_encrypt', - 'psa_asymmetric_decrypt'): - return True - return False + # False-positives that do not need buffer copying + if function_name in ('mbedtls_psa_inject_entropy', + 'psa_crypto_driver_pake_get_password', + 'psa_crypto_driver_pake_get_user', + 'psa_crypto_driver_pake_get_peer'): + return False + + return True def _write_function_call(self, out: typing_util.Writable, function: c_wrapper_generator.FunctionInfo, From c5064c83a1d7a65c387b1a09e806e3694df680d3 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Mon, 11 Mar 2024 17:02:03 +0000 Subject: [PATCH 5/5] Do not attempt to wipe output buffer if it is NULL If the output buffer is NULL, it either: * Does not need wiping because it is zero-length. * Has failed allocation of a copy. * Has not yet been written to as a copy hasn't been allocated. In any of these circumstances, we should not try to write the buffer, so perform a NULL check before wiping it. Signed-off-by: David Horstmann --- library/psa_crypto.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 9574cd71d..7473aef4d 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -3658,8 +3658,10 @@ psa_status_t psa_sign_hash_complete( exit: - psa_wipe_tag_output_buffer(signature, status, signature_size, - *signature_length); + if (signature != NULL) { + psa_wipe_tag_output_buffer(signature, status, signature_size, + *signature_length); + } if (status != PSA_OPERATION_INCOMPLETE) { if (status != PSA_SUCCESS) {