From d1e398c3744141ca8780a79e8cd23f97c0117f44 Mon Sep 17 00:00:00 2001 From: Ryan Everett Date: Fri, 19 Jan 2024 14:46:00 +0000 Subject: [PATCH 1/9] Protect psa_key_derivation_input_bytes Signed-off-by: Ryan Everett --- library/psa_crypto.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 57844c5b76..d93b65b4eb 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -7093,12 +7093,20 @@ static psa_status_t psa_key_derivation_input_integer_internal( psa_status_t psa_key_derivation_input_bytes( psa_key_derivation_operation_t *operation, psa_key_derivation_step_t step, - const uint8_t *data, + const uint8_t *data_external, size_t data_length) { - return psa_key_derivation_input_internal(operation, step, - PSA_KEY_TYPE_NONE, - data, data_length); + psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED; + LOCAL_INPUT_DECLARE(data_external, data); + + LOCAL_INPUT_ALLOC(data_external, data_length, data); + + status = psa_key_derivation_input_internal(operation, step, + PSA_KEY_TYPE_NONE, + data, data_length); +exit: + LOCAL_INPUT_FREE(data_external, data); + return status; } psa_status_t psa_key_derivation_input_integer( From f943e22bb9b8291fc8f49b4027b428ad45d6e789 Mon Sep 17 00:00:00 2001 From: Ryan Everett Date: Fri, 19 Jan 2024 14:46:39 +0000 Subject: [PATCH 2/9] Protect key_derivation_output_bytes If the alloc fails I belive it is okay to preserve the algorithm. The alloc cannot fail with BAD_STATE, and this setting is only used to differentiate between a exhausted and blank. Signed-off-by: Ryan Everett --- library/psa_crypto.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index d93b65b4eb..85728c3e19 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -5801,10 +5801,12 @@ static psa_status_t psa_key_derivation_pbkdf2_read( psa_status_t psa_key_derivation_output_bytes( psa_key_derivation_operation_t *operation, - uint8_t *output, + uint8_t *output_external, size_t output_length) { psa_status_t status; + LOCAL_OUTPUT_DECLARE(output_external, output); + psa_algorithm_t kdf_alg = psa_key_derivation_get_kdf_alg(operation); if (operation->alg == 0) { @@ -5828,6 +5830,8 @@ psa_status_t psa_key_derivation_output_bytes( * output_length > 0. */ return PSA_ERROR_INSUFFICIENT_DATA; } + + LOCAL_OUTPUT_ALLOC(output_external, output_length, output); operation->capacity -= output_length; #if defined(BUILTIN_ALG_ANY_HKDF) @@ -5861,10 +5865,15 @@ psa_status_t psa_key_derivation_output_bytes( { (void) kdf_alg; - return PSA_ERROR_BAD_STATE; + status = PSA_ERROR_BAD_STATE; + LOCAL_OUTPUT_FREE(output_external, output); + + return status; } exit: + LOCAL_OUTPUT_FREE(output_external, output); + if (status != PSA_SUCCESS) { /* Preserve the algorithm upon errors, but clear all sensitive state. * This allows us to differentiate between exhausted operations and From da9227de7c3d822d13c4d821e07755c9b3db42ef Mon Sep 17 00:00:00 2001 From: Ryan Everett Date: Thu, 25 Jan 2024 11:37:22 +0000 Subject: [PATCH 3/9] Fix psa_key_derivation_output_bytes Signed-off-by: Ryan Everett --- library/psa_crypto.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 85728c3e19..a09877e974 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -5814,13 +5814,6 @@ psa_status_t psa_key_derivation_output_bytes( return PSA_ERROR_BAD_STATE; } - if (output_length > operation->capacity) { - operation->capacity = 0; - /* Go through the error path to wipe all confidential data now - * that the operation object is useless. */ - status = PSA_ERROR_INSUFFICIENT_DATA; - goto exit; - } if (output_length == 0 && operation->capacity == 0) { /* Edge case: this is a finished operation, and 0 bytes * were requested. The right error in this case could @@ -5832,6 +5825,14 @@ psa_status_t psa_key_derivation_output_bytes( } LOCAL_OUTPUT_ALLOC(output_external, output_length, output); + if (output_length > operation->capacity) { + operation->capacity = 0; + /* Go through the error path to wipe all confidential data now + * that the operation object is useless. */ + status = PSA_ERROR_INSUFFICIENT_DATA; + goto exit; + } + operation->capacity -= output_length; #if defined(BUILTIN_ALG_ANY_HKDF) @@ -5872,8 +5873,6 @@ psa_status_t psa_key_derivation_output_bytes( } exit: - LOCAL_OUTPUT_FREE(output_external, output); - if (status != PSA_SUCCESS) { /* Preserve the algorithm upon errors, but clear all sensitive state. * This allows us to differentiate between exhausted operations and @@ -5884,6 +5883,8 @@ exit: operation->alg = alg; memset(output, '!', output_length); } + + LOCAL_OUTPUT_FREE(output_external, output); return status; } From 198a4d98d54f49d5aaf5646441e58d338231bee0 Mon Sep 17 00:00:00 2001 From: Ryan Everett Date: Thu, 25 Jan 2024 11:44:56 +0000 Subject: [PATCH 4/9] Generate test wrappers for key derivation Signed-off-by: Ryan Everett --- tests/scripts/generate_psa_wrappers.py | 2 +- tests/src/psa_test_wrappers.c | 12 ++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/tests/scripts/generate_psa_wrappers.py b/tests/scripts/generate_psa_wrappers.py index 3cdafed167..75a20fa30c 100755 --- a/tests/scripts/generate_psa_wrappers.py +++ b/tests/scripts/generate_psa_wrappers.py @@ -143,7 +143,7 @@ class PSAWrapperGenerator(c_wrapper_generator.Base): """Whether the specified buffer argument to a PSA function should be copied. """ # Proof-of-concept: just instrument one function for now - if function_name == 'psa_cipher_encrypt': + if function_name == 'psa_cipher_encrypt' or function_name == 'psa_key_derivation_output_bytes' or function_name == 'psa_key_derivation_input_bytes': return True if function_name in ('psa_import_key', 'psa_export_key', diff --git a/tests/src/psa_test_wrappers.c b/tests/src/psa_test_wrappers.c index bb1409e10b..31aa92b2c0 100644 --- a/tests/src/psa_test_wrappers.c +++ b/tests/src/psa_test_wrappers.c @@ -612,7 +612,13 @@ psa_status_t mbedtls_test_wrap_psa_key_derivation_input_bytes( const uint8_t *arg2_data, size_t arg3_data_length) { +#if defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) + MBEDTLS_TEST_MEMORY_POISON(arg2_data, arg3_data_length); +#endif /* defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) */ psa_status_t status = (psa_key_derivation_input_bytes)(arg0_operation, arg1_step, arg2_data, arg3_data_length); +#if defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) + MBEDTLS_TEST_MEMORY_UNPOISON(arg2_data, arg3_data_length); +#endif /* defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) */ return status; } @@ -654,7 +660,13 @@ psa_status_t mbedtls_test_wrap_psa_key_derivation_output_bytes( uint8_t *arg1_output, size_t arg2_output_length) { +#if defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) + MBEDTLS_TEST_MEMORY_POISON(arg1_output, arg2_output_length); +#endif /* defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) */ psa_status_t status = (psa_key_derivation_output_bytes)(arg0_operation, arg1_output, arg2_output_length); +#if defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) + MBEDTLS_TEST_MEMORY_UNPOISON(arg1_output, arg2_output_length); +#endif /* defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) */ return status; } From 0f54727bf48a5441388465e427d6150c01bd9d61 Mon Sep 17 00:00:00 2001 From: Ryan Everett Date: Thu, 25 Jan 2024 11:55:23 +0000 Subject: [PATCH 5/9] Restructure wrapper script Signed-off-by: Ryan Everett --- tests/scripts/generate_psa_wrappers.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/scripts/generate_psa_wrappers.py b/tests/scripts/generate_psa_wrappers.py index 75a20fa30c..816dd5f53b 100755 --- a/tests/scripts/generate_psa_wrappers.py +++ b/tests/scripts/generate_psa_wrappers.py @@ -143,7 +143,9 @@ class PSAWrapperGenerator(c_wrapper_generator.Base): """Whether the specified buffer argument to a PSA function should be copied. """ # Proof-of-concept: just instrument one function for now - if function_name == 'psa_cipher_encrypt' or function_name == 'psa_key_derivation_output_bytes' or function_name == 'psa_key_derivation_input_bytes': + if function_name == 'psa_cipher_encrypt': + return True + if function_name == 'psa_key_derivation_output_bytes' or function_name == 'psa_key_derivation_input_bytes': return True if function_name in ('psa_import_key', 'psa_export_key', From b41c3c958280ac790aba465ec4aa15adb7745faa Mon Sep 17 00:00:00 2001 From: Ryan Everett Date: Thu, 25 Jan 2024 11:56:35 +0000 Subject: [PATCH 6/9] Guard the exit to stop unused label warning Signed-off-by: Ryan Everett --- library/psa_crypto.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index a09877e974..e8ec3f2058 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -7114,7 +7114,9 @@ psa_status_t psa_key_derivation_input_bytes( status = psa_key_derivation_input_internal(operation, step, PSA_KEY_TYPE_NONE, data, data_length); +#if defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) exit: +#endif LOCAL_INPUT_FREE(data_external, data); return status; } From 5d2e82f0cecf85b347a7d170e55597c58226118c Mon Sep 17 00:00:00 2001 From: Ryan Everett Date: Wed, 7 Feb 2024 17:24:59 +0000 Subject: [PATCH 7/9] Guard memcpy so that it won't fail on null input pointer Signed-off-by: Ryan Everett --- library/psa_crypto.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index e8ec3f2058..5ad9f23df8 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -6864,12 +6864,12 @@ static psa_status_t psa_pbkdf2_hmac_set_password(psa_algorithm_t hash_alg, { psa_status_t status = PSA_SUCCESS; if (input_len > PSA_HASH_BLOCK_LENGTH(hash_alg)) { - status = psa_hash_compute(hash_alg, input, input_len, output, - PSA_HMAC_MAX_HASH_BLOCK_SIZE, output_len); - } else { + return psa_hash_compute(hash_alg, input, input_len, output, + PSA_HMAC_MAX_HASH_BLOCK_SIZE, output_len); + } else if (input_len > 0) { memcpy(output, input, input_len); - *output_len = PSA_HASH_BLOCK_LENGTH(hash_alg); } + *output_len = PSA_HASH_BLOCK_LENGTH(hash_alg); return status; } #endif /* MBEDTLS_PSA_BUILTIN_ALG_PBKDF2_HMAC */ From eb8c665a5337443dd516e0b8ab67e93c8ea3919b Mon Sep 17 00:00:00 2001 From: Ryan Everett Date: Wed, 7 Feb 2024 17:25:39 +0000 Subject: [PATCH 8/9] Reformat wrapper generation code Signed-off-by: Ryan Everett --- tests/scripts/generate_psa_wrappers.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/scripts/generate_psa_wrappers.py b/tests/scripts/generate_psa_wrappers.py index 816dd5f53b..9ca6b297cb 100755 --- a/tests/scripts/generate_psa_wrappers.py +++ b/tests/scripts/generate_psa_wrappers.py @@ -145,7 +145,8 @@ class PSAWrapperGenerator(c_wrapper_generator.Base): # Proof-of-concept: just instrument one function for now if function_name == 'psa_cipher_encrypt': return True - if function_name == 'psa_key_derivation_output_bytes' or function_name == 'psa_key_derivation_input_bytes': + 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', From ee5920a7d5c3b26dfcf492254fe1323d3da2badf Mon Sep 17 00:00:00 2001 From: Ryan Everett Date: Fri, 9 Feb 2024 15:09:28 +0000 Subject: [PATCH 9/9] Fix error path in `psa_key_derivation_output_bytes` Co-authored-by: David Horstmann Signed-off-by: Ryan Everett --- library/psa_crypto.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 5ad9f23df8..00eef4c97e 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -5881,7 +5881,9 @@ exit: psa_algorithm_t alg = operation->alg; psa_key_derivation_abort(operation); operation->alg = alg; - memset(output, '!', output_length); + if (output != NULL) { + memset(output, '!', output_length); + } } LOCAL_OUTPUT_FREE(output_external, output);