From 6cb9f35d8c84d6b3d92e2071a31fb6b183da949f Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Sun, 27 Jul 2025 21:22:39 +0200 Subject: [PATCH] Switch legacy cipher to constant-time invalid padding reporting In internal `get_padding` functions, report whether the padding was invalid through a separate output parameter, rather than the return code. Take advantage of this to have `mbedtls_cipher_finish_padded()` be the easy path that just passes the `invalid_padding` through. Make `mbedtls_cipher_finish()` a wrapper around `mbedtls_cipher_finish_padded()` that converts the invalid-padding output into an error code. Signed-off-by: Gilles Peskine --- include/mbedtls/cipher.h | 11 ++++- library/cipher.c | 62 ++++++++++++++----------- library/cipher_invasive.h | 3 +- tests/suites/test_suite_cipher.function | 43 +++++++++++++---- 4 files changed, 78 insertions(+), 41 deletions(-) diff --git a/include/mbedtls/cipher.h b/include/mbedtls/cipher.h index 6ef703ddab..3778f44551 100644 --- a/include/mbedtls/cipher.h +++ b/include/mbedtls/cipher.h @@ -329,8 +329,15 @@ typedef struct mbedtls_cipher_context_t { /** Padding functions to use, if relevant for * the specific cipher mode. */ - void(*MBEDTLS_PRIVATE(add_padding))(unsigned char *output, size_t olen, size_t data_len); - int(*MBEDTLS_PRIVATE(get_padding))(unsigned char *input, size_t ilen, size_t *data_len); + void(*MBEDTLS_PRIVATE(add_padding))(unsigned char *output, size_t olen, + size_t data_len); + /* Report invalid-padding condition through the output parameter + * invalid_padding. To minimize changes in Mbed TLS 3.6, where this + * declaration is in a public header, use the public type size_t + * rather than the internal type mbedtls_ct_condition_t. */ + int(*MBEDTLS_PRIVATE(get_padding))(unsigned char *input, size_t ilen, + size_t *data_len, + size_t *invalid_padding); #endif /** Buffer for input that has not been processed yet. */ diff --git a/library/cipher.c b/library/cipher.c index f3e2f91f82..f9d46213db 100644 --- a/library/cipher.c +++ b/library/cipher.c @@ -846,7 +846,8 @@ static void add_pkcs_padding(unsigned char *output, size_t output_len, */ MBEDTLS_STATIC_TESTABLE int mbedtls_get_pkcs_padding(unsigned char *input, size_t input_len, - size_t *data_len) + size_t *data_len, + size_t *invalid_padding) { size_t i, pad_idx; unsigned char padding_len; @@ -872,7 +873,8 @@ MBEDTLS_STATIC_TESTABLE int mbedtls_get_pkcs_padding(unsigned char *input, /* If the padding is invalid, set the output length to 0 */ *data_len = mbedtls_ct_if(bad, 0, input_len - padding_len); - return mbedtls_ct_error_if_else_0(bad, MBEDTLS_ERR_CIPHER_INVALID_PADDING); + *invalid_padding = mbedtls_ct_size_if_else_0(bad, SIZE_MAX); + return 0; } #endif /* MBEDTLS_CIPHER_PADDING_PKCS7 */ @@ -893,7 +895,7 @@ static void add_one_and_zeros_padding(unsigned char *output, } static int get_one_and_zeros_padding(unsigned char *input, size_t input_len, - size_t *data_len) + size_t *data_len, size_t *invalid_padding) { if (NULL == input || NULL == data_len) { return MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA; @@ -916,7 +918,8 @@ static int get_one_and_zeros_padding(unsigned char *input, size_t input_len, in_padding = mbedtls_ct_bool_and(in_padding, mbedtls_ct_bool_not(is_nonzero)); } - return mbedtls_ct_error_if_else_0(bad, MBEDTLS_ERR_CIPHER_INVALID_PADDING); + *invalid_padding = mbedtls_ct_size_if_else_0(bad, SIZE_MAX); + return 0; } #endif /* MBEDTLS_CIPHER_PADDING_ONE_AND_ZEROS */ @@ -937,7 +940,7 @@ static void add_zeros_and_len_padding(unsigned char *output, } static int get_zeros_and_len_padding(unsigned char *input, size_t input_len, - size_t *data_len) + size_t *data_len, size_t *invalid_padding) { size_t i, pad_idx; unsigned char padding_len; @@ -963,7 +966,8 @@ static int get_zeros_and_len_padding(unsigned char *input, size_t input_len, bad = mbedtls_ct_bool_or(bad, nonzero_pad_byte); } - return mbedtls_ct_error_if_else_0(bad, MBEDTLS_ERR_CIPHER_INVALID_PADDING); + *invalid_padding = mbedtls_ct_size_if_else_0(bad, SIZE_MAX); + return 0; } #endif /* MBEDTLS_CIPHER_PADDING_ZEROS_AND_LEN */ @@ -978,7 +982,7 @@ static void add_zeros_padding(unsigned char *output, } static int get_zeros_padding(unsigned char *input, size_t input_len, - size_t *data_len) + size_t *data_len, size_t *invalid_padding) { size_t i; mbedtls_ct_condition_t done = MBEDTLS_CT_FALSE, prev_done; @@ -994,6 +998,7 @@ static int get_zeros_padding(unsigned char *input, size_t input_len, *data_len = mbedtls_ct_size_if(mbedtls_ct_bool_ne(done, prev_done), i, *data_len); } + *invalid_padding = 0; return 0; } #endif /* MBEDTLS_CIPHER_PADDING_ZEROS */ @@ -1005,20 +1010,21 @@ static int get_zeros_padding(unsigned char *input, size_t input_len, * but a trivial get_padding function */ static int get_no_padding(unsigned char *input, size_t input_len, - size_t *data_len) + size_t *data_len, size_t *invalid_padding) { if (NULL == input || NULL == data_len) { return MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA; } *data_len = input_len; - + *invalid_padding = 0; return 0; } #endif /* MBEDTLS_CIPHER_MODE_WITH_PADDING */ -int mbedtls_cipher_finish(mbedtls_cipher_context_t *ctx, - unsigned char *output, size_t *olen) +int mbedtls_cipher_finish_padded(mbedtls_cipher_context_t *ctx, + unsigned char *output, size_t *olen, + size_t *invalid_padding) { if (ctx->cipher_info == NULL) { return MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA; @@ -1034,6 +1040,7 @@ int mbedtls_cipher_finish(mbedtls_cipher_context_t *ctx, #endif /* MBEDTLS_USE_PSA_CRYPTO && !MBEDTLS_DEPRECATED_REMOVED */ *olen = 0; + *invalid_padding = 0; #if defined(MBEDTLS_CIPHER_MODE_WITH_PADDING) /* CBC mode requires padding so we make sure a call to @@ -1110,7 +1117,7 @@ int mbedtls_cipher_finish(mbedtls_cipher_context_t *ctx, /* Set output size for decryption */ if (MBEDTLS_DECRYPT == ctx->operation) { return ctx->get_padding(output, mbedtls_cipher_get_block_size(ctx), - olen); + olen, invalid_padding); } /* Set output size for encryption */ @@ -1124,20 +1131,16 @@ int mbedtls_cipher_finish(mbedtls_cipher_context_t *ctx, return MBEDTLS_ERR_CIPHER_FEATURE_UNAVAILABLE; } -int mbedtls_cipher_finish_padded(mbedtls_cipher_context_t *ctx, - unsigned char *output, size_t *olen, - size_t *invalid_padding) +int mbedtls_cipher_finish(mbedtls_cipher_context_t *ctx, + unsigned char *output, size_t *olen) { - *invalid_padding = 0; - int ret = mbedtls_cipher_finish(ctx, output, olen); -#if defined(MBEDTLS_CIPHER_PADDING_PKCS7) || \ - defined(MBEDTLS_CIPHER_PADDING_ONE_AND_ZEROS) || \ - defined(MBEDTLS_CIPHER_PADDING_ZEROS_AND_LEN) - if (ret == MBEDTLS_ERR_CIPHER_INVALID_PADDING) { - ret = 0; - *invalid_padding = SIZE_MAX; + size_t invalid_padding = 0; + int ret = mbedtls_cipher_finish_padded(ctx, output, olen, + &invalid_padding); + if (ret == 0) { + ret = mbedtls_ct_error_if_else_0(invalid_padding, + MBEDTLS_ERR_CIPHER_INVALID_PADDING); } -#endif return ret; } @@ -1410,14 +1413,17 @@ int mbedtls_cipher_crypt(mbedtls_cipher_context_t *ctx, return ret; } - if ((ret = mbedtls_cipher_finish(ctx, output + *olen, - &finish_olen)) != 0) { + size_t invalid_padding = 0; + if ((ret = mbedtls_cipher_finish_padded(ctx, output + *olen, + &finish_olen, + &invalid_padding)) != 0) { return ret; } - *olen += finish_olen; - return 0; + ret = mbedtls_ct_error_if_else_0(invalid_padding, + MBEDTLS_ERR_CIPHER_INVALID_PADDING); + return ret; } #if defined(MBEDTLS_CIPHER_MODE_AEAD) diff --git a/library/cipher_invasive.h b/library/cipher_invasive.h index 702f8f73e9..e82a0a7f99 100644 --- a/library/cipher_invasive.h +++ b/library/cipher_invasive.h @@ -20,7 +20,8 @@ MBEDTLS_STATIC_TESTABLE int mbedtls_get_pkcs_padding(unsigned char *input, size_t input_len, - size_t *data_len); + size_t *data_len, + size_t *invalid_padding); #endif diff --git a/tests/suites/test_suite_cipher.function b/tests/suites/test_suite_cipher.function index fbc48b7974..9e3e026834 100644 --- a/tests/suites/test_suite_cipher.function +++ b/tests/suites/test_suite_cipher.function @@ -1475,8 +1475,8 @@ exit: /* END_CASE */ /* BEGIN_CASE depends_on:MBEDTLS_CIPHER_MODE_CBC */ -void check_padding(int pad_mode, data_t *input, int ret, int dlen_check - ) +void check_padding(int pad_mode, data_t *input, + int expected_ret, int dlen_check) { mbedtls_cipher_info_t cipher_info; mbedtls_cipher_context_t ctx; @@ -1489,10 +1489,20 @@ void check_padding(int pad_mode, data_t *input, int ret, int dlen_check TEST_ASSERT(0 == mbedtls_cipher_set_padding_mode(&ctx, pad_mode)); - - TEST_ASSERT(ret == ctx.get_padding(input->x, input->len, &dlen)); - if (0 == ret) { - TEST_ASSERT(dlen == (size_t) dlen_check); + size_t invalid_padding = 42; + int ret = ctx.get_padding(input->x, input->len, &dlen, &invalid_padding); + switch (expected_ret) { + case 0: + TEST_EQUAL(ret, 0); + TEST_EQUAL(invalid_padding, 0); + TEST_EQUAL(dlen, dlen_check); + break; + case MBEDTLS_ERR_CIPHER_INVALID_PADDING: + TEST_EQUAL(ret, 0); + TEST_EQUAL(invalid_padding, SIZE_MAX); + break; + default: + TEST_EQUAL(ret, expected_ret); } } /* END_CASE */ @@ -1585,14 +1595,27 @@ void get_pkcs_padding(data_t *decrypted_block, int exp_ret, int exp_len) { int ret; size_t calculated_len; + size_t invalid_padding; TEST_CF_SECRET(decrypted_block->x, decrypted_block->len); ret = mbedtls_get_pkcs_padding(decrypted_block->x, decrypted_block->len, - &calculated_len); + &calculated_len, &invalid_padding); - TEST_EQUAL(ret, exp_ret); - if (exp_ret == 0) { - TEST_EQUAL(calculated_len, exp_len); + switch (exp_ret) { + case 0: + TEST_EQUAL(ret, 0); + TEST_EQUAL(invalid_padding, 0); + TEST_EQUAL(calculated_len, exp_len); + break; + + case MBEDTLS_ERR_CIPHER_INVALID_PADDING: + TEST_EQUAL(ret, 0); + TEST_EQUAL(invalid_padding, ~(size_t) 0); + break; + + default: + TEST_EQUAL(ret, exp_ret); + break; } } /* END_CASE */