From 59200a22aa4b7c32559affb41ab89f3c0c573e93 Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Tue, 21 Feb 2023 15:07:40 +0000 Subject: [PATCH 1/4] Improve psa_wipe_output_buffer Change name and document to ensure suitability only for "tags" is explicit. Add support for output size of zero in PSA_SUCCESS case. Signed-off-by: Paul Elliott --- library/psa_crypto.c | 49 +++++++++++++++++++++++++------------------- 1 file changed, 28 insertions(+), 21 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 3ec9273de9..fa6991e9fd 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -2684,34 +2684,41 @@ static psa_status_t psa_sign_verify_check_alg(int input_is_message, } /** - * \brief Fill the unused part of the output buffer (the - * whole buffer on error, the trailing part on - * success) with something that isn't a valid - * signature (barring an attack on the signature - * and deliberately-crafted input), in case the - * caller doesn't check the return status properly. + * \brief For output buffers which contain "tags" + * (outputs that may be checked for validity like + * Hashes, MACs and signatures), fill the unused + * part of the output buffer (the whole buffer on + * error, the trailing part on success) with + * something that isn't a valid tag (barring an + * attack on the tag and deliberately-crafted + * input), in case the caller doesn't check the + * return status properly. * * \param output_buffer pointer to buffer to wipe. May not be NULL * unless \p output_buffer_size is zero. * \param status status of function called to generate * output_buffer originally * \param output_buffer_size Size of output buffer. If zero, \p output_buffer - * could be NULL + * could be NULL. * \param output_buffer_length Length of data written to output_buffer, must be * less than \p output_buffer_size */ -static void psa_wipe_output_buffer(uint8_t *output_buffer, psa_status_t status, - size_t output_buffer_size, size_t output_buffer_length) +static void psa_wipe_tag_output_buffer(uint8_t *output_buffer, psa_status_t status, + size_t output_buffer_size, size_t output_buffer_length) { - if (status == PSA_SUCCESS) { - memset(output_buffer + output_buffer_length, '!', - output_buffer_size - output_buffer_length); - } else if (output_buffer_size > 0) { - memset(output_buffer, '!', output_buffer_size); + size_t offset = 0; + + if (output_buffer_size == 0) { + /* If output_buffer_size is 0 then we have nothing to do. We must not + call memset because output_buffer may be NULL in this case */ + return; } - /* If output_buffer_size is 0 then we have nothing to do. We must - * not call memset because output_buffer may be NULL in this - * case.*/ + + if (status == PSA_SUCCESS) { + offset = output_buffer_length; + } + + memset(output_buffer + offset, '!', output_buffer_size - offset); } static psa_status_t psa_sign_internal(mbedtls_svc_key_id_t key, @@ -2776,8 +2783,8 @@ static psa_status_t psa_sign_internal(mbedtls_svc_key_id_t key, exit: - psa_wipe_output_buffer(signature, status, signature_size, - *signature_length); + psa_wipe_tag_output_buffer(signature, status, signature_size, + *signature_length); unlock_status = psa_unlock_key_slot(slot); @@ -3293,8 +3300,8 @@ psa_status_t psa_sign_hash_complete( exit: - psa_wipe_output_buffer(signature, status, signature_size, - *signature_length); + psa_wipe_tag_output_buffer(signature, status, signature_size, + *signature_length); if (status != PSA_OPERATION_INCOMPLETE) { if (status != PSA_SUCCESS) { From 7bc24cc512518a2d21a652a19caee835c4228820 Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Fri, 24 Feb 2023 18:04:16 +0000 Subject: [PATCH 2/4] Fix typos in documentation. Signed-off-by: Paul Elliott --- library/psa_crypto.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index fa6991e9fd..9dddb8f88f 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -2686,7 +2686,7 @@ static psa_status_t psa_sign_verify_check_alg(int input_is_message, /** * \brief For output buffers which contain "tags" * (outputs that may be checked for validity like - * Hashes, MACs and signatures), fill the unused + * hashes, MACs and signatures), fill the unused * part of the output buffer (the whole buffer on * error, the trailing part on success) with * something that isn't a valid tag (barring an @@ -2694,9 +2694,9 @@ static psa_status_t psa_sign_verify_check_alg(int input_is_message, * input), in case the caller doesn't check the * return status properly. * - * \param output_buffer pointer to buffer to wipe. May not be NULL + * \param output_buffer Pointer to buffer to wipe. May not be NULL * unless \p output_buffer_size is zero. - * \param status status of function called to generate + * \param status Status of function called to generate * output_buffer originally * \param output_buffer_size Size of output buffer. If zero, \p output_buffer * could be NULL. From dc42ca8a7eb8774bb58b0669766b732556eec36c Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Fri, 24 Feb 2023 18:11:59 +0000 Subject: [PATCH 3/4] Use psa_wipe_tag_buffer() for MAC and aead code. Signed-off-by: Paul Elliott --- library/psa_crypto.c | 95 ++++++++++++++++++++------------------------ 1 file changed, 43 insertions(+), 52 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 9dddb8f88f..9cc2aa924f 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -322,6 +322,43 @@ psa_status_t mbedtls_to_psa_error(int ret) } } +/** + * \brief For output buffers which contain "tags" + * (outputs that may be checked for validity like + * hashes, MACs and signatures), fill the unused + * part of the output buffer (the whole buffer on + * error, the trailing part on success) with + * something that isn't a valid tag (barring an + * attack on the tag and deliberately-crafted + * input), in case the caller doesn't check the + * return status properly. + * + * \param output_buffer Pointer to buffer to wipe. May not be NULL + * unless \p output_buffer_size is zero. + * \param status Status of function called to generate + * output_buffer originally + * \param output_buffer_size Size of output buffer. If zero, \p output_buffer + * could be NULL. + * \param output_buffer_length Length of data written to output_buffer, must be + * less than \p output_buffer_size + */ +static void psa_wipe_tag_output_buffer(uint8_t *output_buffer, psa_status_t status, + size_t output_buffer_size, size_t output_buffer_length) { + size_t offset = 0; + + if (output_buffer_size == 0) { + /* If output_buffer_size is 0 then we have nothing to do. We must not + call memset because output_buffer may be NULL in this case */ + return; + } + + if (status == PSA_SUCCESS) { + offset = output_buffer_length; + } + + memset(output_buffer + offset, '!', output_buffer_size - offset); +} + @@ -2504,10 +2541,7 @@ exit: operation->mac_size = 0; } - if (mac_size > operation->mac_size) { - memset(&mac[operation->mac_size], '!', - mac_size - operation->mac_size); - } + psa_wipe_tag_output_buffer(mac, status, mac_size, *mac_length); abort_status = psa_mac_abort(operation); @@ -2601,9 +2635,8 @@ exit: *mac_length = mac_size; operation_mac_size = 0; } - if (mac_size > operation_mac_size) { - memset(&mac[operation_mac_size], '!', mac_size - operation_mac_size); - } + + psa_wipe_tag_output_buffer(mac, status, mac_size, *mac_length); unlock_status = psa_unlock_key_slot(slot); @@ -2683,44 +2716,6 @@ static psa_status_t psa_sign_verify_check_alg(int input_is_message, return PSA_SUCCESS; } -/** - * \brief For output buffers which contain "tags" - * (outputs that may be checked for validity like - * hashes, MACs and signatures), fill the unused - * part of the output buffer (the whole buffer on - * error, the trailing part on success) with - * something that isn't a valid tag (barring an - * attack on the tag and deliberately-crafted - * input), in case the caller doesn't check the - * return status properly. - * - * \param output_buffer Pointer to buffer to wipe. May not be NULL - * unless \p output_buffer_size is zero. - * \param status Status of function called to generate - * output_buffer originally - * \param output_buffer_size Size of output buffer. If zero, \p output_buffer - * could be NULL. - * \param output_buffer_length Length of data written to output_buffer, must be - * less than \p output_buffer_size - */ -static void psa_wipe_tag_output_buffer(uint8_t *output_buffer, psa_status_t status, - size_t output_buffer_size, size_t output_buffer_length) -{ - size_t offset = 0; - - if (output_buffer_size == 0) { - /* If output_buffer_size is 0 then we have nothing to do. We must not - call memset because output_buffer may be NULL in this case */ - return; - } - - if (status == PSA_SUCCESS) { - offset = output_buffer_length; - } - - memset(output_buffer + offset, '!', output_buffer_size - offset); -} - static psa_status_t psa_sign_internal(mbedtls_svc_key_id_t key, int input_is_message, psa_algorithm_t alg, @@ -4917,18 +4912,14 @@ psa_status_t psa_aead_finish(psa_aead_operation_t *operation, tag, tag_size, tag_length); exit: + + /* In case the operation fails and the user fails to check for failure or * the zero tag size, make sure the tag is set to something implausible. * Even if the operation succeeds, make sure we clear the rest of the * buffer to prevent potential leakage of anything previously placed in * the same buffer.*/ - if (tag != NULL) { - if (status != PSA_SUCCESS) { - memset(tag, '!', tag_size); - } else if (*tag_length < tag_size) { - memset(tag + *tag_length, '!', (tag_size - *tag_length)); - } - } + psa_wipe_tag_output_buffer(tag, status, tag_size, *tag_length); psa_aead_abort(operation); From 7118d17df148cbf2d67ea8f90ccef4feaa6e5e45 Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Sun, 26 Feb 2023 16:57:05 +0000 Subject: [PATCH 4/4] Pacify code style checker Signed-off-by: Paul Elliott --- library/psa_crypto.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 9cc2aa924f..f5bd65b036 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -343,7 +343,8 @@ psa_status_t mbedtls_to_psa_error(int ret) * less than \p output_buffer_size */ static void psa_wipe_tag_output_buffer(uint8_t *output_buffer, psa_status_t status, - size_t output_buffer_size, size_t output_buffer_length) { + size_t output_buffer_size, size_t output_buffer_length) +{ size_t offset = 0; if (output_buffer_size == 0) {