From d37e0c4639c66f7bb4b6e2c039c96304b7536185 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Thu, 16 Jan 2025 16:24:35 +0000 Subject: [PATCH 01/10] Add constant-flow testing for PKCS7 padding Signed-off-by: David Horstmann --- library/cipher.c | 6 +++-- library/cipher_invasive.h | 27 +++++++++++++++++++++ tests/suites/test_suite_cipher.function | 22 +++++++++++++++++ tests/suites/test_suite_cipher.padding.data | 12 +++++++++ 4 files changed, 65 insertions(+), 2 deletions(-) create mode 100644 library/cipher_invasive.h diff --git a/library/cipher.c b/library/cipher.c index 7f4c121492..5e14e1eab6 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,9 @@ 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) +MBEDTLS_STATIC_TESTABLE int get_pkcs_padding(unsigned char *input, + size_t input_len, + size_t *data_len) { size_t i, pad_idx; unsigned char padding_len; diff --git a/library/cipher_invasive.h b/library/cipher_invasive.h new file mode 100644 index 0000000000..2dbd1d2c7b --- /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 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..2444ef9d8f 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,21 @@ 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 = get_pkcs_padding(decrypted_block->x, decrypted_block->len, + &calculated_len); + TEST_CF_PUBLIC(decrypted_block->x, decrypted_block->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..bd04a50882 100644 --- a/tests/suites/test_suite_cipher.padding.data +++ b/tests/suites/test_suite_cipher.padding.data @@ -217,3 +217,15 @@ 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, 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 From 652ea21737fcc6b2fdcc0be24e2462c17f16d67c Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Mon, 20 Jan 2025 17:44:00 +0000 Subject: [PATCH 02/10] Fix timing side-channel in PKCS7 padding Previously, we were checking if the last padding byte was in the range 1-16 and returning early if not. This was to prevent an integer overflow in the output length. Instead, do the checks in constant-time and conditionally set the output length based on whether the padding is correct or not, preventing both the side-channel and the integer overflow. Signed-off-by: David Horstmann --- library/cipher.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/library/cipher.c b/library/cipher.c index 5e14e1eab6..9be53b7427 100644 --- a/library/cipher.c +++ b/library/cipher.c @@ -851,10 +851,6 @@ MBEDTLS_STATIC_TESTABLE int get_pkcs_padding(unsigned char *input, } 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)); @@ -868,6 +864,9 @@ MBEDTLS_STATIC_TESTABLE int get_pkcs_padding(unsigned char *input, 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 */ From 16674559929f6b37aee738953151b2b67eeeade9 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 22 Jan 2025 11:18:14 +0000 Subject: [PATCH 03/10] Add ChangeLog entry for PKCS#7 side channel fix Signed-off-by: David Horstmann --- ChangeLog.d/pkcs7-padding-side-channel-fix.txt | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 ChangeLog.d/pkcs7-padding-side-channel-fix.txt 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..f34c095056 --- /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 last byte of each block of the plaintext. From 32d8c9052815e51202759cbc11cfea7499db2470 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 22 Jan 2025 11:26:00 +0000 Subject: [PATCH 04/10] Disable check-names for static padding function Check names is intended for public APIs. It doesn't matter if we call a static function a non-namespaced name, so add an exception in the invasive testing header file. Signed-off-by: David Horstmann --- library/cipher_invasive.h | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/library/cipher_invasive.h b/library/cipher_invasive.h index 2dbd1d2c7b..f1584007b8 100644 --- a/library/cipher_invasive.h +++ b/library/cipher_invasive.h @@ -18,9 +18,10 @@ #if defined(MBEDTLS_TEST_HOOKS) && defined(MBEDTLS_CIPHER_C) -MBEDTLS_STATIC_TESTABLE int get_pkcs_padding(unsigned char *input, - size_t input_len, - size_t *data_len); +MBEDTLS_STATIC_TESTABLE +int get_pkcs_padding(unsigned char *input, //no-check-names + size_t input_len, + size_t *data_len); #endif From ab7bb5734d26e29faee843a1fc7cabdfd8805f4a Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 5 Mar 2025 18:05:04 +0000 Subject: [PATCH 05/10] Appease check-names with prefix Adding an mbedtls_prefix is preferred to having '//no-check-names' littered about. Signed-off-by: David Horstmann --- library/cipher.c | 6 +++--- library/cipher_invasive.h | 7 +++---- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/library/cipher.c b/library/cipher.c index 9be53b7427..ebb03f464e 100644 --- a/library/cipher.c +++ b/library/cipher.c @@ -839,9 +839,9 @@ static void add_pkcs_padding(unsigned char *output, size_t output_len, } } -MBEDTLS_STATIC_TESTABLE int get_pkcs_padding(unsigned char *input, - size_t input_len, - size_t *data_len) +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; diff --git a/library/cipher_invasive.h b/library/cipher_invasive.h index f1584007b8..702f8f73e9 100644 --- a/library/cipher_invasive.h +++ b/library/cipher_invasive.h @@ -18,10 +18,9 @@ #if defined(MBEDTLS_TEST_HOOKS) && defined(MBEDTLS_CIPHER_C) -MBEDTLS_STATIC_TESTABLE -int get_pkcs_padding(unsigned char *input, //no-check-names - size_t input_len, - size_t *data_len); +MBEDTLS_STATIC_TESTABLE int mbedtls_get_pkcs_padding(unsigned char *input, + size_t input_len, + size_t *data_len); #endif From 5a5440e63af436b106a01aa4ba314b0cb8fd3129 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Fri, 28 Mar 2025 10:57:45 +0000 Subject: [PATCH 06/10] Update to the new name in usages as well Somehow the uses of the function were missed when renaming. Signed-off-by: David Horstmann --- library/cipher.c | 2 +- tests/suites/test_suite_cipher.function | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/library/cipher.c b/library/cipher.c index ebb03f464e..d0ba01ea01 100644 --- a/library/cipher.c +++ b/library/cipher.c @@ -1145,7 +1145,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/tests/suites/test_suite_cipher.function b/tests/suites/test_suite_cipher.function index 2444ef9d8f..3eb46257df 100644 --- a/tests/suites/test_suite_cipher.function +++ b/tests/suites/test_suite_cipher.function @@ -1272,8 +1272,8 @@ void get_pkcs_padding(data_t *decrypted_block, int exp_ret, int exp_len) size_t calculated_len; TEST_CF_SECRET(decrypted_block->x, decrypted_block->len); - ret = get_pkcs_padding(decrypted_block->x, decrypted_block->len, - &calculated_len); + ret = mbedtls_get_pkcs_padding(decrypted_block->x, decrypted_block->len, + &calculated_len); TEST_CF_PUBLIC(decrypted_block->x, decrypted_block->len); TEST_EQUAL(ret, exp_ret); From d75c4c23818c0221cb8adba705b3dcff9a65c324 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Fri, 28 Mar 2025 17:16:27 +0000 Subject: [PATCH 07/10] Remove unnecessary TEST_CF_PUBLIC macro call Signed-off-by: David Horstmann --- tests/suites/test_suite_cipher.function | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/suites/test_suite_cipher.function b/tests/suites/test_suite_cipher.function index 3eb46257df..2856ae5699 100644 --- a/tests/suites/test_suite_cipher.function +++ b/tests/suites/test_suite_cipher.function @@ -1274,7 +1274,6 @@ void get_pkcs_padding(data_t *decrypted_block, int exp_ret, int exp_len) TEST_CF_SECRET(decrypted_block->x, decrypted_block->len); ret = mbedtls_get_pkcs_padding(decrypted_block->x, decrypted_block->len, &calculated_len); - TEST_CF_PUBLIC(decrypted_block->x, decrypted_block->len); TEST_EQUAL(ret, exp_ret); if (exp_ret == 0) { From b2b1c3bb8193dd8965b14b3d8bf889e0c271fde5 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Fri, 28 Mar 2025 17:24:45 +0000 Subject: [PATCH 08/10] Add testcase for maximum padding length Signed-off-by: David Horstmann --- tests/suites/test_suite_cipher.padding.data | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/suites/test_suite_cipher.padding.data b/tests/suites/test_suite_cipher.padding.data index bd04a50882..85b14c1f2f 100644 --- a/tests/suites/test_suite_cipher.padding.data +++ b/tests/suites/test_suite_cipher.padding.data @@ -224,6 +224,9 @@ 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 From bbf1a015036a4fbcf713dc06fc6dd24726db59ec Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Fri, 28 Mar 2025 17:31:15 +0000 Subject: [PATCH 09/10] Modify ChangeLog entry to full plaintext recovery Signed-off-by: David Horstmann --- ChangeLog.d/pkcs7-padding-side-channel-fix.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ChangeLog.d/pkcs7-padding-side-channel-fix.txt b/ChangeLog.d/pkcs7-padding-side-channel-fix.txt index f34c095056..b813b84ce8 100644 --- a/ChangeLog.d/pkcs7-padding-side-channel-fix.txt +++ b/ChangeLog.d/pkcs7-padding-side-channel-fix.txt @@ -1,4 +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 last byte of each block of the plaintext. + ciphertexts to recover the plaintext through a timing oracle attack. From 850e5b317d17b3fe965ad172b5799e60661a144c Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Fri, 28 Mar 2025 17:33:45 +0000 Subject: [PATCH 10/10] Document assumption of mbedtls_get_pkcs_padding Point out that the input length must be the same as the cipher's block size. Signed-off-by: David Horstmann --- library/cipher.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/library/cipher.c b/library/cipher.c index d0ba01ea01..2ae01dd84d 100644 --- a/library/cipher.c +++ b/library/cipher.c @@ -839,6 +839,11 @@ static void add_pkcs_padding(unsigned char *output, size_t output_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)