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>

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) {
+    /* C99 doesn't allow a declaration to follow a label */;
-        status = psa_cipher_abort(operation);
+    psa_status_t abort_status = psa_cipher_abort(operation);
-    } else {
+    /* Normally abort shouldn't fail unless the operation is in a bad
-        *output_length = 0;
+     * state, in which case we'd expect finish to fail with the same error.
-        (void) psa_cipher_abort(operation);
+     * 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;

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;
 }