diff --git a/ChangeLog.d/pkcs7-padding-side-channel-fix.txt b/ChangeLog.d/pkcs7-padding-side-channel-fix.txt new file mode 100644 index 0000000000..b813b84ce8 --- /dev/null +++ b/ChangeLog.d/pkcs7-padding-side-channel-fix.txt @@ -0,0 +1,4 @@ +Security + * Fix a timing side channel in the implementation of PKCS#7 padding + which would allow an attacker who can request decryption of arbitrary + ciphertexts to recover the plaintext through a timing oracle attack. diff --git a/library/cipher.c b/library/cipher.c index 7f4c121492..2ae01dd84d 100644 --- a/library/cipher.c +++ b/library/cipher.c @@ -14,6 +14,7 @@ #if defined(MBEDTLS_CIPHER_C) #include "mbedtls/cipher.h" +#include "cipher_invasive.h" #include "cipher_wrap.h" #include "mbedtls/platform_util.h" #include "mbedtls/error.h" @@ -838,8 +839,14 @@ static void add_pkcs_padding(unsigned char *output, size_t output_len, } } -static int get_pkcs_padding(unsigned char *input, size_t input_len, - size_t *data_len) +/* + * Get the length of the PKCS7 padding. + * + * Note: input_len must be the block size of the cipher. + */ +MBEDTLS_STATIC_TESTABLE int mbedtls_get_pkcs_padding(unsigned char *input, + size_t input_len, + size_t *data_len) { size_t i, pad_idx; unsigned char padding_len; @@ -849,10 +856,6 @@ static int get_pkcs_padding(unsigned char *input, size_t input_len, } padding_len = input[input_len - 1]; - if (padding_len == 0 || padding_len > input_len) { - return MBEDTLS_ERR_CIPHER_INVALID_PADDING; - } - *data_len = input_len - padding_len; mbedtls_ct_condition_t bad = mbedtls_ct_uint_gt(padding_len, input_len); bad = mbedtls_ct_bool_or(bad, mbedtls_ct_uint_eq(padding_len, 0)); @@ -866,6 +869,9 @@ static int get_pkcs_padding(unsigned char *input, size_t input_len, bad = mbedtls_ct_bool_or(bad, mbedtls_ct_bool_and(in_padding, different)); } + /* 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); } #endif /* MBEDTLS_CIPHER_PADDING_PKCS7 */ @@ -1144,7 +1150,7 @@ int mbedtls_cipher_set_padding_mode(mbedtls_cipher_context_t *ctx, #if defined(MBEDTLS_CIPHER_PADDING_PKCS7) case MBEDTLS_PADDING_PKCS7: ctx->add_padding = add_pkcs_padding; - ctx->get_padding = get_pkcs_padding; + ctx->get_padding = mbedtls_get_pkcs_padding; break; #endif #if defined(MBEDTLS_CIPHER_PADDING_ONE_AND_ZEROS) diff --git a/library/cipher_invasive.h b/library/cipher_invasive.h new file mode 100644 index 0000000000..702f8f73e9 --- /dev/null +++ b/library/cipher_invasive.h @@ -0,0 +1,27 @@ +/** + * \file cipher_invasive.h + * + * \brief Cipher module: interfaces for invasive testing only. + * + * The interfaces in this file are intended for testing purposes only. + * They SHOULD NOT be made available in library integrations except when + * building the library for testing. + */ +/* + * Copyright The Mbed TLS Contributors + * SPDX-License-Identifier: Apache-2.0 OR GPL-2.0-or-later + */ +#ifndef MBEDTLS_CIPHER_INVASIVE_H +#define MBEDTLS_CIPHER_INVASIVE_H + +#include "common.h" + +#if defined(MBEDTLS_TEST_HOOKS) && defined(MBEDTLS_CIPHER_C) + +MBEDTLS_STATIC_TESTABLE int mbedtls_get_pkcs_padding(unsigned char *input, + size_t input_len, + size_t *data_len); + +#endif + +#endif /* MBEDTLS_CIPHER_INVASIVE_H */ diff --git a/tests/suites/test_suite_cipher.function b/tests/suites/test_suite_cipher.function index 040c35ca58..2856ae5699 100644 --- a/tests/suites/test_suite_cipher.function +++ b/tests/suites/test_suite_cipher.function @@ -6,6 +6,10 @@ #include "mbedtls/gcm.h" #endif +#include "cipher_invasive.h" + +#include "test/constant_flow.h" + #if defined(MBEDTLS_CIPHER_HAVE_SOME_AEAD_VIA_LEGACY_OR_USE_PSA) || defined(MBEDTLS_NIST_KW_C) #define MBEDTLS_CIPHER_AUTH_CRYPT #endif @@ -1260,3 +1264,20 @@ exit: mbedtls_free(key); } /* END_CASE */ + +/* BEGIN_CASE depends_on:MBEDTLS_TEST_HOOKS */ +void get_pkcs_padding(data_t *decrypted_block, int exp_ret, int exp_len) +{ + int ret; + size_t calculated_len; + + TEST_CF_SECRET(decrypted_block->x, decrypted_block->len); + ret = mbedtls_get_pkcs_padding(decrypted_block->x, decrypted_block->len, + &calculated_len); + + TEST_EQUAL(ret, exp_ret); + if (exp_ret == 0) { + TEST_EQUAL(calculated_len, exp_len); + } +} +/* END_CASE */ diff --git a/tests/suites/test_suite_cipher.padding.data b/tests/suites/test_suite_cipher.padding.data index 0370fb3d28..85b14c1f2f 100644 --- a/tests/suites/test_suite_cipher.padding.data +++ b/tests/suites/test_suite_cipher.padding.data @@ -217,3 +217,18 @@ check_padding:MBEDTLS_PADDING_NONE:"DABBAD0001":0:5 Check no padding #3 (correct by definition) check_padding:MBEDTLS_PADDING_NONE:"":0:0 + +Constant-time PKCS7 padding, valid #1 +get_pkcs_padding:"00112233445566778899AABBCCDDEE01":0:15 + +Constant-time PKCS7 padding, valid #2 +get_pkcs_padding:"00112233445566778899AA0505050505":0:11 + +Constant-time PKCS7 padding, valid #3 +get_pkcs_padding:"10101010101010101010101010101010":0:0 + +Constant-time PKCS7 padding, invalid zero +get_pkcs_padding:"00112233445566778899AABBCCDDEE00":MBEDTLS_ERR_CIPHER_INVALID_PADDING:0 + +Constant-time PKCS7 padding, invalid > 16 +get_pkcs_padding:"00112233445566778899AABBCCDDEE11":MBEDTLS_ERR_CIPHER_INVALID_PADDING:0