From 3b380daedbce9fae3e7ed7e84f18e97876e7e6f3 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 7 Aug 2025 21:59:07 +0200 Subject: [PATCH] psa_cipher_finish: treat status and output length as sensitive In `psa_cipher_finish()` and in the corresponding function in our built-in implementation `mbedtls_psa_cipher_finish()`, treat `status` and `*output_length` as sensitive variables whose value must not leak through a timing side channel. This is important when doing decryption with unpadding, where leaking the validity or amount of padding can enable a padding oracle attack. With this change, `psa_cipher_finish()` should be constant-time if the underlying legacy function (including the cipher implementation) is. Signed-off-by: Gilles Peskine --- library/psa_crypto.c | 26 +++++++++++++++++++++----- library/psa_crypto_cipher.c | 36 ++++++++++++++++++++++++++---------- 2 files changed, 47 insertions(+), 15 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 9c28609d7e..51ec72fa19 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -73,6 +73,8 @@ #include "mbedtls/psa_util.h" #include "mbedtls/threading.h" +#include "constant_time_internal.h" + #if defined(MBEDTLS_PSA_BUILTIN_ALG_HKDF) || \ defined(MBEDTLS_PSA_BUILTIN_ALG_HKDF_EXTRACT) || \ defined(MBEDTLS_PSA_BUILTIN_ALG_HKDF_EXPAND) @@ -4692,13 +4694,27 @@ psa_status_t psa_cipher_finish(psa_cipher_operation_t *operation, output_length); exit: - if (status == PSA_SUCCESS) { - status = psa_cipher_abort(operation); - } else { - *output_length = 0; - (void) psa_cipher_abort(operation); + /* C99 doesn't allow a declaration to follow a label */; + psa_status_t abort_status = psa_cipher_abort(operation); + /* Normally abort shouldn't fail unless the operation is in a bad + * state, in which case we'd expect finish to fail with the same error. + * So it doesn't matter much which call's error code we pick when both + * fail. However, in unauthenticated decryption specifically, the + * distinction between PSA_SUCCESS and PSA_ERROR_INVALID_PADDING is + * security-sensitive (risk of a padding oracle attack), so here we + * must not have a code path that depends on the value of status. */ + if (abort_status != PSA_SUCCESS) { + status = abort_status; } + /* Set *output_length to 0 if status != PSA_SUCCESS, without + * leaking the value of status through a timing side channel + * (status == PSA_ERROR_INVALID_PADDING is sensitive when doing + * unpadded decryption, due to the risk of padding oracle attack). */ + mbedtls_ct_condition_t success = + mbedtls_ct_bool_not(mbedtls_ct_bool(status)); + *output_length = mbedtls_ct_size_if_else_0(success, *output_length); + LOCAL_OUTPUT_FREE(output_external, output); return status; diff --git a/library/psa_crypto_cipher.c b/library/psa_crypto_cipher.c index 6d0378bd7e..022bdfa49f 100644 --- a/library/psa_crypto_cipher.c +++ b/library/psa_crypto_cipher.c @@ -552,9 +552,21 @@ psa_status_t mbedtls_psa_cipher_finish( uint8_t *output, size_t output_size, size_t *output_length) { psa_status_t status = PSA_ERROR_GENERIC_ERROR; - uint8_t temp_output_buffer[MBEDTLS_MAX_BLOCK_LENGTH]; size_t invalid_padding = 0; + uint8_t temp_output_buffer[MBEDTLS_MAX_BLOCK_LENGTH] = { 0 }; + if (output_size > sizeof(temp_output_buffer)) { + output_size = sizeof(temp_output_buffer); + } + /* We will copy output_size bytes from temp_output_buffer to the + * output buffer. We can't use *output_length to determine how + * much to copy because we must not leak that value through timing + * when doing decryption with unpadding. But the underlying function + * is not guaranteed to write beyond *output_length. To ensure we don't + * leak the former content of the stack to the caller, wipe that + * former content. */ + memset(temp_output_buffer, 0, output_size); + if (operation->ctx.cipher.unprocessed_len != 0) { if (operation->alg == PSA_ALG_ECB_NO_PADDING || operation->alg == PSA_ALG_CBC_NO_PADDING) { @@ -572,22 +584,26 @@ psa_status_t mbedtls_psa_cipher_finish( goto exit; } - if (*output_length == 0) { + if (output_size == 0) { ; /* Nothing to copy. Note that output may be NULL in this case. */ - } else if (output_size >= *output_length) { - memcpy(output, temp_output_buffer, *output_length); } else { - status = PSA_ERROR_BUFFER_TOO_SMALL; + /* Do not use the value of *output_length to determine how much + * to copy. When decrypting a padded cipher, the output length is + * sensitive, and leaking it could allow a padding oracle attack. */ + memcpy(output, temp_output_buffer, output_size); } + status = mbedtls_ct_error_if_else_0(invalid_padding, + PSA_ERROR_INVALID_PADDING); + mbedtls_ct_condition_t buffer_too_small = + mbedtls_ct_uint_lt(output_size, *output_length); + status = mbedtls_ct_error_if(buffer_too_small, + PSA_ERROR_BUFFER_TOO_SMALL, + status); + exit: mbedtls_platform_zeroize(temp_output_buffer, sizeof(temp_output_buffer)); - - if (status == PSA_SUCCESS) { - status = mbedtls_ct_size_if_else_0(invalid_padding, - PSA_ERROR_INVALID_PADDING); - } return status; }