From 04dfd704325a6dbc2a13eb7f418eaca9ae9ca549 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 7 Aug 2025 22:27:26 +0200 Subject: [PATCH] psa_cipher_decrypt: treat status and output length as sensitive In `psa_cipher_decrypt()` and in the corresponding function in our built-in implementation `mbedtls_psa_cipher_decrypt()`, 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_decrypt()` should be constant-time if the underlying legacy function (including the cipher implementation) is. Signed-off-by: Gilles Peskine --- library/psa_crypto.c | 12 ++++++++---- library/psa_crypto_cipher.c | 18 +++++++++++------- 2 files changed, 19 insertions(+), 11 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 51ec72fa19..5f2cad42b7 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -4857,13 +4857,17 @@ psa_status_t psa_cipher_decrypt(mbedtls_svc_key_id_t key, exit: unlock_status = psa_unregister_read_under_mutex(slot); - if (status == PSA_SUCCESS) { + if (unlock_status != PSA_SUCCESS) { status = unlock_status; } - if (status != PSA_SUCCESS) { - *output_length = 0; - } + /* 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_INPUT_FREE(input_external, input); LOCAL_OUTPUT_FREE(output_external, output); diff --git a/library/psa_crypto_cipher.c b/library/psa_crypto_cipher.c index 022bdfa49f..83069c7852 100644 --- a/library/psa_crypto_cipher.c +++ b/library/psa_crypto_cipher.c @@ -724,17 +724,21 @@ psa_status_t mbedtls_psa_cipher_decrypt( &operation, mbedtls_buffer_offset(output, accumulated_length), output_size - accumulated_length, &olength); - if (status != PSA_SUCCESS) { - goto exit; - } *output_length = accumulated_length + olength; exit: - if (status == PSA_SUCCESS) { - status = mbedtls_psa_cipher_abort(&operation); - } else { - mbedtls_psa_cipher_abort(&operation); + /* C99 doesn't allow a declaration to follow a label */; + psa_status_t abort_status = mbedtls_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; } return status;