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 <Gilles.Peskine@arm.com>
diff --git a/library/psa_crypto_cipher.c b/library/psa_crypto_cipher.c
index 6d0378b..022bdfa 100644
--- a/library/psa_crypto_cipher.c
+++ b/library/psa_crypto_cipher.c
@@ -552,9 +552,21 @@
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 @@
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;
}