From 5fc20fc56a9469ab63749c936d70c9f9ce961c82 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Wed, 21 Aug 2024 13:15:13 +0100 Subject: [PATCH 01/12] Add tests for optionally safe code paths in bignum Not adding _unsafe version to the tests targeting behaviour related to RR as it is independent from the secret involved in the safe/unsafe distinction. Signed-off-by: Janos Follath --- tests/suites/test_suite_bignum.function | 51 +++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/tests/suites/test_suite_bignum.function b/tests/suites/test_suite_bignum.function index 3ac4e10ea6..5e18441d7e 100644 --- a/tests/suites/test_suite_bignum.function +++ b/tests/suites/test_suite_bignum.function @@ -989,7 +989,13 @@ void mpi_exp_mod_min_RR(char *input_A, char *input_E, * against a smaller RR. */ TEST_LE_U(RR.n, N.n - 1); +#if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) + mbedtls_mpi_optionally_safe_codepath_reset(); +#endif res = mbedtls_mpi_exp_mod(&Z, &A, &E, &N, &RR); +#if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) + TEST_EQUAL(mbedtls_mpi_optionally_safe_codepath, MBEDTLS_MPI_IS_SECRET); +#endif /* We know that exp_mod internally needs RR to be as large as N. * Validate that it is the case now, otherwise there was probably * a buffer overread. */ @@ -1022,7 +1028,26 @@ void mpi_exp_mod(char *input_A, char *input_E, TEST_ASSERT(mbedtls_test_read_mpi(&N, input_N) == 0); TEST_ASSERT(mbedtls_test_read_mpi(&X, input_X) == 0); +#if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) + mbedtls_mpi_optionally_safe_codepath_reset(); +#endif res = mbedtls_mpi_exp_mod(&Z, &A, &E, &N, NULL); +#if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) + TEST_EQUAL(mbedtls_mpi_optionally_safe_codepath, MBEDTLS_MPI_IS_SECRET); +#endif + TEST_ASSERT(res == exp_result); + if (res == 0) { + TEST_ASSERT(sign_is_valid(&Z)); + TEST_ASSERT(mbedtls_mpi_cmp_mpi(&Z, &X) == 0); + } + +#if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) + mbedtls_mpi_optionally_safe_codepath_reset(); +#endif + res = mbedtls_mpi_exp_mod_unsafe(&Z, &A, &E, &N, NULL); +#if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) + TEST_EQUAL(mbedtls_mpi_optionally_safe_codepath, MBEDTLS_MPI_IS_PUBLIC); +#endif TEST_ASSERT(res == exp_result); if (res == 0) { TEST_ASSERT(sign_is_valid(&Z)); @@ -1030,7 +1055,13 @@ void mpi_exp_mod(char *input_A, char *input_E, } /* Now test again with the speed-up parameter supplied as an output. */ +#if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) + mbedtls_mpi_optionally_safe_codepath_reset(); +#endif res = mbedtls_mpi_exp_mod(&Z, &A, &E, &N, &RR); +#if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) + TEST_EQUAL(mbedtls_mpi_optionally_safe_codepath, MBEDTLS_MPI_IS_SECRET); +#endif TEST_ASSERT(res == exp_result); if (res == 0) { TEST_ASSERT(sign_is_valid(&Z)); @@ -1038,7 +1069,13 @@ void mpi_exp_mod(char *input_A, char *input_E, } /* Now test again with the speed-up parameter supplied in calculated form. */ +#if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) + mbedtls_mpi_optionally_safe_codepath_reset(); +#endif res = mbedtls_mpi_exp_mod(&Z, &A, &E, &N, &RR); +#if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) + TEST_EQUAL(mbedtls_mpi_optionally_safe_codepath, MBEDTLS_MPI_IS_SECRET); +#endif TEST_ASSERT(res == exp_result); if (res == 0) { TEST_ASSERT(sign_is_valid(&Z)); @@ -1078,7 +1115,21 @@ void mpi_exp_mod_size(int A_bytes, int E_bytes, int N_bytes, TEST_ASSERT(mbedtls_test_read_mpi(&RR, input_RR) == 0); } +#if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) + mbedtls_mpi_optionally_safe_codepath_reset(); +#endif TEST_ASSERT(mbedtls_mpi_exp_mod(&Z, &A, &E, &N, &RR) == exp_result); +#if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) + TEST_EQUAL(mbedtls_mpi_optionally_safe_codepath, MBEDTLS_MPI_IS_SECRET); +#endif + +#if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) + mbedtls_mpi_optionally_safe_codepath_reset(); +#endif + TEST_ASSERT(mbedtls_mpi_exp_mod_unsafe(&Z, &A, &E, &N, &RR) == exp_result); +#if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) + TEST_EQUAL(mbedtls_mpi_optionally_safe_codepath, MBEDTLS_MPI_IS_SECRET); +#endif exit: mbedtls_mpi_free(&A); mbedtls_mpi_free(&E); mbedtls_mpi_free(&N); From 55be79b500ed73facee057c2873ccf23eeba3310 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Wed, 21 Aug 2024 13:24:01 +0100 Subject: [PATCH 02/12] Add tests for optionally safe code paths in RSA Only add the test hooks where it is meaningful. That is, not adding where the operation is essentially the same or the target is not the function that is being tested. Signed-off-by: Janos Follath --- tests/suites/test_suite_rsa.function | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/suites/test_suite_rsa.function b/tests/suites/test_suite_rsa.function index e82452927e..e0206eca29 100644 --- a/tests/suites/test_suite_rsa.function +++ b/tests/suites/test_suite_rsa.function @@ -1,5 +1,6 @@ /* BEGIN_HEADER */ #include "mbedtls/rsa.h" +#include "bignum_core.h" #include "rsa_alt_helpers.h" #include "rsa_internal.h" /* END_HEADER */ @@ -489,7 +490,13 @@ void mbedtls_rsa_public(data_t *message_str, int mod, TEST_EQUAL(mbedtls_rsa_get_bitlen(&ctx), (size_t) mod); TEST_ASSERT(mbedtls_rsa_check_pubkey(&ctx) == 0); +#if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) + mbedtls_mpi_optionally_safe_codepath_reset(); +#endif TEST_ASSERT(mbedtls_rsa_public(&ctx, message_str->x, output) == result); +#if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) + TEST_EQUAL(mbedtls_mpi_optionally_safe_codepath, MBEDTLS_MPI_IS_PUBLIC); +#endif if (result == 0) { TEST_ASSERT(mbedtls_test_hexcmp(output, result_str->x, @@ -554,9 +561,15 @@ void mbedtls_rsa_private(data_t *message_str, int mod, /* repeat three times to test updating of blinding values */ for (i = 0; i < 3; i++) { memset(output, 0x00, sizeof(output)); +#if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) + mbedtls_mpi_optionally_safe_codepath_reset(); +#endif TEST_ASSERT(mbedtls_rsa_private(&ctx, mbedtls_test_rnd_pseudo_rand, &rnd_info, message_str->x, output) == result); +#if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) + TEST_EQUAL(mbedtls_mpi_optionally_safe_codepath, MBEDTLS_MPI_IS_SECRET); +#endif if (result == 0) { TEST_ASSERT(mbedtls_test_hexcmp(output, result_str->x, From 42f72b3ea5b7210b2dbde99a3b3f7f7a8155a1dd Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Thu, 22 Aug 2024 08:25:33 +0100 Subject: [PATCH 03/12] Introduce MBEDTLS_MPI_IS_TEST A + B + 1 is not a good way to get a number that's neither A nor B. This can be a problem for example if values later are changed to A = 0 and B = -1. Signed-off-by: Janos Follath --- library/bignum_core.c | 3 +-- library/bignum_core.h | 7 +++++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/library/bignum_core.c b/library/bignum_core.c index 4231554b84..76b1da72ee 100644 --- a/library/bignum_core.c +++ b/library/bignum_core.c @@ -747,8 +747,7 @@ static void exp_mod_precompute_window(const mbedtls_mpi_uint *A, } #if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) -// Set to a default that is neither MBEDTLS_MPI_IS_PUBLIC nor MBEDTLS_MPI_IS_SECRET -int mbedtls_mpi_optionally_safe_codepath = MBEDTLS_MPI_IS_PUBLIC + MBEDTLS_MPI_IS_SECRET + 1; +int mbedtls_mpi_optionally_safe_codepath = MBEDTLS_MPI_IS_TEST; #endif /* diff --git a/library/bignum_core.h b/library/bignum_core.h index cf6485a148..e64128f160 100644 --- a/library/bignum_core.h +++ b/library/bignum_core.h @@ -110,6 +110,10 @@ * other than the two below is passed, default to the safe path.) */ #define MBEDTLS_MPI_IS_PUBLIC 0x2a2a2a2a #define MBEDTLS_MPI_IS_SECRET 0 +#if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) +// Default value for testing that is neither MBEDTLS_MPI_IS_PUBLIC nor MBEDTLS_MPI_IS_SECRET +#define MBEDTLS_MPI_IS_TEST 1 +#endif /** Count leading zero bits in a given integer. * @@ -825,8 +829,7 @@ extern int mbedtls_mpi_optionally_safe_codepath; static inline void mbedtls_mpi_optionally_safe_codepath_reset(void) { - // Set to a default that is neither MBEDTLS_MPI_IS_PUBLIC nor MBEDTLS_MPI_IS_SECRET - mbedtls_mpi_optionally_safe_codepath = MBEDTLS_MPI_IS_PUBLIC + MBEDTLS_MPI_IS_SECRET + 1; + mbedtls_mpi_optionally_safe_codepath = MBEDTLS_MPI_IS_TEST; } #endif From e86607c498a4b9bca6359290d32a7a14ec5cdb1f Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Thu, 22 Aug 2024 17:07:58 +0100 Subject: [PATCH 04/12] Initial local variables to secure default Unfortunately compilers aren't good at analyzing whether variables are analyzed on all code paths, and it is better to initialize to the safe-path values. Signed-off-by: Janos Follath --- library/bignum_core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/library/bignum_core.c b/library/bignum_core.c index 76b1da72ee..ca2af9c463 100644 --- a/library/bignum_core.c +++ b/library/bignum_core.c @@ -856,8 +856,8 @@ static void mbedtls_mpi_core_exp_mod_optionally_safe(mbedtls_mpi_uint *X, /* We'll process the bits of E from most significant * (limb_index=E_limbs-1, E_bit_index=biL-1) to least significant * (limb_index=0, E_bit_index=0). */ - size_t E_limb_index; - size_t E_bit_index; + size_t E_limb_index = E_limbs; + size_t E_bit_index = 0; exp_mod_calc_first_bit_optionally_safe(E, E_limbs, E_public, &E_limb_index, &E_bit_index); From 2f8ad595db07954f384bbe1a78e649073c02b117 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Thu, 22 Aug 2024 17:13:25 +0100 Subject: [PATCH 05/12] Explain the choice of the value of MBEDTLS_MPI_IS_PUBLIC Signed-off-by: Janos Follath --- library/bignum_core.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/library/bignum_core.h b/library/bignum_core.h index e64128f160..ba2978eb33 100644 --- a/library/bignum_core.h +++ b/library/bignum_core.h @@ -107,7 +107,10 @@ * // safe path * } * not the other way round, in order to prevent misuse. (This is, if a value - * other than the two below is passed, default to the safe path.) */ + * other than the two below is passed, default to the safe path.) + * + * The value of MBEDTLS_MPI_IS_PUBLIC is chosen in a way that is unlikely to happen by accident, but + * which can be used as an immediate value in a Thumb2 comparison (for code size). */ #define MBEDTLS_MPI_IS_PUBLIC 0x2a2a2a2a #define MBEDTLS_MPI_IS_SECRET 0 #if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) From 96cfd7a77aeda10750da88682b6d8cd9c6dbe058 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Thu, 22 Aug 2024 18:30:06 +0100 Subject: [PATCH 06/12] Move bignum code path testing out of the library Without this, it's not at all obvious that turning on MBEDTLS_TEST_HOOKS doesn't change the functional behavior of the code. Signed-off-by: Janos Follath --- library/bignum_core.c | 21 ++++----- library/bignum_core.h | 12 ----- library/bignum_core_invasive.h | 23 ++++++++++ tests/include/test/bignum_codepath_check.h | 48 ++++++++++++++++++++ tests/src/bignum_codepath_check.c | 39 ++++++++++++++++ tests/src/helpers.c | 12 +++++ tests/suites/test_suite_bignum.function | 29 ++++++------ tests/suites/test_suite_bignum_core.function | 17 +++---- tests/suites/test_suite_rsa.function | 9 ++-- 9 files changed, 161 insertions(+), 49 deletions(-) create mode 100644 library/bignum_core_invasive.h create mode 100644 tests/include/test/bignum_codepath_check.h create mode 100644 tests/src/bignum_codepath_check.c diff --git a/library/bignum_core.c b/library/bignum_core.c index ca2af9c463..c8b1474928 100644 --- a/library/bignum_core.c +++ b/library/bignum_core.c @@ -747,7 +747,8 @@ static void exp_mod_precompute_window(const mbedtls_mpi_uint *A, } #if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) -int mbedtls_mpi_optionally_safe_codepath = MBEDTLS_MPI_IS_TEST; +void (*mbedtls_safe_codepath_hook)(void) = NULL; +void (*mbedtls_unsafe_codepath_hook)(void) = NULL; #endif /* @@ -780,7 +781,8 @@ static inline void exp_mod_calc_first_bit_optionally_safe(const mbedtls_mpi_uint *E_bit_index = E_bits % biL; #if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) - mbedtls_mpi_optionally_safe_codepath = MBEDTLS_MPI_IS_PUBLIC; + if(mbedtls_unsafe_codepath_hook != NULL) + mbedtls_unsafe_codepath_hook(); #endif } else { /* @@ -790,10 +792,8 @@ static inline void exp_mod_calc_first_bit_optionally_safe(const mbedtls_mpi_uint *E_limb_index = E_limbs; *E_bit_index = 0; #if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) - // Only mark the codepath safe if there wasn't an unsafe codepath before - if (mbedtls_mpi_optionally_safe_codepath != MBEDTLS_MPI_IS_PUBLIC) { - mbedtls_mpi_optionally_safe_codepath = MBEDTLS_MPI_IS_SECRET; - } + if(mbedtls_safe_codepath_hook != NULL) + mbedtls_safe_codepath_hook(); #endif } } @@ -812,7 +812,8 @@ static inline void exp_mod_table_lookup_optionally_safe(mbedtls_mpi_uint *Wselec if (window_public == MBEDTLS_MPI_IS_PUBLIC) { memcpy(Wselect, Wtable + window * AN_limbs, AN_limbs * ciL); #if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) - mbedtls_mpi_optionally_safe_codepath = MBEDTLS_MPI_IS_PUBLIC; + if(mbedtls_unsafe_codepath_hook != NULL) + mbedtls_unsafe_codepath_hook(); #endif } else { /* Select Wtable[window] without leaking window through @@ -820,10 +821,8 @@ static inline void exp_mod_table_lookup_optionally_safe(mbedtls_mpi_uint *Wselec mbedtls_mpi_core_ct_uint_table_lookup(Wselect, Wtable, AN_limbs, welem, window); #if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) - // Only mark the codepath safe if there wasn't an unsafe codepath before - if (mbedtls_mpi_optionally_safe_codepath != MBEDTLS_MPI_IS_PUBLIC) { - mbedtls_mpi_optionally_safe_codepath = MBEDTLS_MPI_IS_SECRET; - } + if(mbedtls_safe_codepath_hook != NULL) + mbedtls_safe_codepath_hook(); #endif } } diff --git a/library/bignum_core.h b/library/bignum_core.h index ba2978eb33..5a77ef82e2 100644 --- a/library/bignum_core.h +++ b/library/bignum_core.h @@ -824,16 +824,4 @@ void mbedtls_mpi_core_from_mont_rep(mbedtls_mpi_uint *X, mbedtls_mpi_uint mm, mbedtls_mpi_uint *T); -/* - * Can't define thread local variables with our abstraction layer: do nothing if threading is on. - */ -#if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) -extern int mbedtls_mpi_optionally_safe_codepath; - -static inline void mbedtls_mpi_optionally_safe_codepath_reset(void) -{ - mbedtls_mpi_optionally_safe_codepath = MBEDTLS_MPI_IS_TEST; -} -#endif - #endif /* MBEDTLS_BIGNUM_CORE_H */ diff --git a/library/bignum_core_invasive.h b/library/bignum_core_invasive.h new file mode 100644 index 0000000000..167099dc91 --- /dev/null +++ b/library/bignum_core_invasive.h @@ -0,0 +1,23 @@ +/** + * \file bignum_core_invasive.h + * + * \brief Function declarations for invasive functions of bignum core. + */ +/** + * Copyright The Mbed TLS Contributors + * SPDX-License-Identifier: Apache-2.0 OR GPL-2.0-or-later + */ + +#ifndef MBEDTLS_BIGNUM_CORE_INVASIVE_H +#define MBEDTLS_BIGNUM_CORE_INVASIVE_H + +#include "bignum_core.h" + +#if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) + +extern void (*mbedtls_safe_codepath_hook)(void); +extern void (*mbedtls_unsafe_codepath_hook)(void); + +#endif /* MBEDTLS_TEST_HOOKS && !MBEDTLS_THREADING_C */ + +#endif /* MBEDTLS_BIGNUM_CORE_INVASIVE_H */ diff --git a/tests/include/test/bignum_codepath_check.h b/tests/include/test/bignum_codepath_check.h new file mode 100644 index 0000000000..6ab68bb5a5 --- /dev/null +++ b/tests/include/test/bignum_codepath_check.h @@ -0,0 +1,48 @@ +/** Support for path tracking in optionally safe bignum functions + * + * The functions are called when an optionally safe path is taken and logs it with a single + * variable. This variable is at any time in one of three states: + * - MBEDTLS_MPI_IS_TEST: No optionally safe path has been taken since the last reset + * - MBEDTLS_MPI_IS_SECRET: Only safe paths were teken since the last reset + * - MBEDTLS_MPI_IS_PUBLIC: At least one unsafe path has been taken since the last reset + * + * Using a simple global variable to track execution path. Making it work with multithreading + * doesn't worth the effort as multithreaded tests add little to no value here. + */ +/* + * Copyright The Mbed TLS Contributors + * SPDX-License-Identifier: Apache-2.0 OR GPL-2.0-or-later + */ + +#ifndef BIGNUM_CODEPATH_CHECK_H +#define BIGNUM_CODEPATH_CHECK_H + +#include "bignum_core.h" + +#if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) + +extern int mbedtls_codepath_check; + +/** + * \brief Setup the codepath test hooks used by optionally safe bignum functions to signal + * the path taken. + */ +void mbedtls_codepath_test_hooks_setup(void); + +/** + * \brief Teardown the codepath test hooks used by optionally safe bignum functions to + * signal the path taken. + */ +void mbedtls_codepath_test_hooks_teardown(void); + +/** + * \brief Reset the state of the codepath to the initial state. + */ +static inline void mbedtls_codepath_reset(void) +{ + mbedtls_codepath_check = MBEDTLS_MPI_IS_TEST; +} + +#endif /* MBEDTLS_TEST_HOOKS && !MBEDTLS_THREADING_C */ + +#endif /* BIGNUM_CODEPATH_CHECK_H */ diff --git a/tests/src/bignum_codepath_check.c b/tests/src/bignum_codepath_check.c new file mode 100644 index 0000000000..b6b85d9aba --- /dev/null +++ b/tests/src/bignum_codepath_check.c @@ -0,0 +1,39 @@ +/** Support for path tracking in optionally safe bignum functions + */ +/* + * Copyright The Mbed TLS Contributors + * SPDX-License-Identifier: Apache-2.0 OR GPL-2.0-or-later + */ + +#include "test/bignum_codepath_check.h" +#include "bignum_core_invasive.h" + +#if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) +int mbedtls_codepath_check = MBEDTLS_MPI_IS_TEST; + +void mbedtls_codepath_take_safe(void) +{ + if(mbedtls_codepath_check == MBEDTLS_MPI_IS_TEST) { + mbedtls_codepath_check = MBEDTLS_MPI_IS_SECRET; + } +} + +void mbedtls_codepath_take_unsafe(void) +{ + mbedtls_codepath_check = MBEDTLS_MPI_IS_PUBLIC; +} + +void mbedtls_codepath_test_hooks_setup(void) +{ + mbedtls_safe_codepath_hook = mbedtls_codepath_take_safe; + mbedtls_unsafe_codepath_hook = mbedtls_codepath_take_unsafe; +} + +void mbedtls_codepath_test_hooks_teardown(void) +{ + mbedtls_safe_codepath_hook = NULL; + mbedtls_unsafe_codepath_hook = NULL; +} + +#endif /* MBEDTLS_TEST_HOOKS && !MBEDTLS_THREADING_C */ + diff --git a/tests/src/helpers.c b/tests/src/helpers.c index 065d17d3e0..db50296e01 100644 --- a/tests/src/helpers.c +++ b/tests/src/helpers.c @@ -16,6 +16,9 @@ #if defined(MBEDTLS_TEST_HOOKS) && defined(MBEDTLS_PSA_CRYPTO_C) #include #endif +#if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) +#include +#endif #if defined(MBEDTLS_THREADING_C) #include "mbedtls/threading.h" #endif @@ -342,6 +345,11 @@ int mbedtls_test_platform_setup(void) mbedtls_mutex_init(&mbedtls_test_info_mutex); #endif /* MBEDTLS_THREADING_C */ + +#if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) + mbedtls_codepath_test_hooks_setup(); +#endif /* MBEDTLS_TEST_HOOKS && !MBEDTLS_THREADING_C */ + return ret; } @@ -359,6 +367,10 @@ void mbedtls_test_platform_teardown(void) #if defined(MBEDTLS_PLATFORM_C) mbedtls_platform_teardown(&platform_ctx); #endif /* MBEDTLS_PLATFORM_C */ + +#if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) + mbedtls_codepath_test_hooks_teardown(); +#endif /* MBEDTLS_TEST_HOOKS && !MBEDTLS_THREADING_C */ } int mbedtls_test_ascii2uc(const char c, unsigned char *uc) diff --git a/tests/suites/test_suite_bignum.function b/tests/suites/test_suite_bignum.function index 5e18441d7e..c71f3c8bb1 100644 --- a/tests/suites/test_suite_bignum.function +++ b/tests/suites/test_suite_bignum.function @@ -5,6 +5,7 @@ #include "bignum_core.h" #include "bignum_internal.h" #include "test/constant_flow.h" +#include "test/bignum_codepath_check.h" #if MBEDTLS_MPI_MAX_BITS > 792 #define MPI_MAX_BITS_LARGER_THAN_792 @@ -990,11 +991,11 @@ void mpi_exp_mod_min_RR(char *input_A, char *input_E, TEST_LE_U(RR.n, N.n - 1); #if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) - mbedtls_mpi_optionally_safe_codepath_reset(); + mbedtls_codepath_reset(); #endif res = mbedtls_mpi_exp_mod(&Z, &A, &E, &N, &RR); #if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) - TEST_EQUAL(mbedtls_mpi_optionally_safe_codepath, MBEDTLS_MPI_IS_SECRET); + TEST_EQUAL(mbedtls_codepath_check, MBEDTLS_MPI_IS_SECRET); #endif /* We know that exp_mod internally needs RR to be as large as N. * Validate that it is the case now, otherwise there was probably @@ -1029,11 +1030,11 @@ void mpi_exp_mod(char *input_A, char *input_E, TEST_ASSERT(mbedtls_test_read_mpi(&X, input_X) == 0); #if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) - mbedtls_mpi_optionally_safe_codepath_reset(); + mbedtls_codepath_reset(); #endif res = mbedtls_mpi_exp_mod(&Z, &A, &E, &N, NULL); #if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) - TEST_EQUAL(mbedtls_mpi_optionally_safe_codepath, MBEDTLS_MPI_IS_SECRET); + TEST_EQUAL(mbedtls_codepath_check, MBEDTLS_MPI_IS_SECRET); #endif TEST_ASSERT(res == exp_result); if (res == 0) { @@ -1042,11 +1043,11 @@ void mpi_exp_mod(char *input_A, char *input_E, } #if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) - mbedtls_mpi_optionally_safe_codepath_reset(); + mbedtls_codepath_reset(); #endif res = mbedtls_mpi_exp_mod_unsafe(&Z, &A, &E, &N, NULL); #if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) - TEST_EQUAL(mbedtls_mpi_optionally_safe_codepath, MBEDTLS_MPI_IS_PUBLIC); + TEST_EQUAL(mbedtls_codepath_check, MBEDTLS_MPI_IS_PUBLIC); #endif TEST_ASSERT(res == exp_result); if (res == 0) { @@ -1056,11 +1057,11 @@ void mpi_exp_mod(char *input_A, char *input_E, /* Now test again with the speed-up parameter supplied as an output. */ #if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) - mbedtls_mpi_optionally_safe_codepath_reset(); + mbedtls_codepath_reset(); #endif res = mbedtls_mpi_exp_mod(&Z, &A, &E, &N, &RR); #if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) - TEST_EQUAL(mbedtls_mpi_optionally_safe_codepath, MBEDTLS_MPI_IS_SECRET); + TEST_EQUAL(mbedtls_codepath_check, MBEDTLS_MPI_IS_SECRET); #endif TEST_ASSERT(res == exp_result); if (res == 0) { @@ -1070,11 +1071,11 @@ void mpi_exp_mod(char *input_A, char *input_E, /* Now test again with the speed-up parameter supplied in calculated form. */ #if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) - mbedtls_mpi_optionally_safe_codepath_reset(); + mbedtls_codepath_reset(); #endif res = mbedtls_mpi_exp_mod(&Z, &A, &E, &N, &RR); #if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) - TEST_EQUAL(mbedtls_mpi_optionally_safe_codepath, MBEDTLS_MPI_IS_SECRET); + TEST_EQUAL(mbedtls_codepath_check, MBEDTLS_MPI_IS_SECRET); #endif TEST_ASSERT(res == exp_result); if (res == 0) { @@ -1116,19 +1117,19 @@ void mpi_exp_mod_size(int A_bytes, int E_bytes, int N_bytes, } #if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) - mbedtls_mpi_optionally_safe_codepath_reset(); + mbedtls_codepath_reset(); #endif TEST_ASSERT(mbedtls_mpi_exp_mod(&Z, &A, &E, &N, &RR) == exp_result); #if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) - TEST_EQUAL(mbedtls_mpi_optionally_safe_codepath, MBEDTLS_MPI_IS_SECRET); + TEST_EQUAL(mbedtls_codepath_check, MBEDTLS_MPI_IS_SECRET); #endif #if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) - mbedtls_mpi_optionally_safe_codepath_reset(); + mbedtls_codepath_reset(); #endif TEST_ASSERT(mbedtls_mpi_exp_mod_unsafe(&Z, &A, &E, &N, &RR) == exp_result); #if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) - TEST_EQUAL(mbedtls_mpi_optionally_safe_codepath, MBEDTLS_MPI_IS_SECRET); + TEST_EQUAL(mbedtls_codepath_check, MBEDTLS_MPI_IS_SECRET); #endif exit: diff --git a/tests/suites/test_suite_bignum_core.function b/tests/suites/test_suite_bignum_core.function index 08dac2e279..c2b44bccdd 100644 --- a/tests/suites/test_suite_bignum_core.function +++ b/tests/suites/test_suite_bignum_core.function @@ -4,6 +4,7 @@ #include "bignum_core.h" #include "constant_time_internal.h" #include "test/constant_flow.h" +#include "test/bignum_codepath_check.h" /** Verifies mbedtls_mpi_core_add(). * @@ -1233,22 +1234,22 @@ void mpi_core_exp_mod(char *input_N, char *input_A, /* Test the safe variant */ #if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) - mbedtls_mpi_optionally_safe_codepath_reset(); + mbedtls_codepath_reset(); #endif mbedtls_mpi_core_exp_mod(Y, A, N, N_limbs, E, E_limbs, R2, T); #if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) - TEST_EQUAL(mbedtls_mpi_optionally_safe_codepath, MBEDTLS_MPI_IS_SECRET); + TEST_EQUAL(mbedtls_codepath_check, MBEDTLS_MPI_IS_SECRET); #endif TEST_EQUAL(0, memcmp(X, Y, N_limbs * sizeof(mbedtls_mpi_uint))); /* Test the unsafe variant */ #if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) - mbedtls_mpi_optionally_safe_codepath_reset(); + mbedtls_codepath_reset(); #endif mbedtls_mpi_core_exp_mod_unsafe(Y, A, N, N_limbs, E, E_limbs, R2, T); #if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) - TEST_EQUAL(mbedtls_mpi_optionally_safe_codepath, MBEDTLS_MPI_IS_PUBLIC); + TEST_EQUAL(mbedtls_codepath_check, MBEDTLS_MPI_IS_PUBLIC); #endif TEST_EQUAL(0, memcmp(X, Y, N_limbs * sizeof(mbedtls_mpi_uint))); @@ -1258,21 +1259,21 @@ void mpi_core_exp_mod(char *input_N, char *input_A, memcpy(A_copy, A, sizeof(*A_copy) * A_limbs); #if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) - mbedtls_mpi_optionally_safe_codepath_reset(); + mbedtls_codepath_reset(); #endif mbedtls_mpi_core_exp_mod(A, A, N, N_limbs, E, E_limbs, R2, T); #if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) - TEST_EQUAL(mbedtls_mpi_optionally_safe_codepath, MBEDTLS_MPI_IS_SECRET); + TEST_EQUAL(mbedtls_codepath_check, MBEDTLS_MPI_IS_SECRET); #endif TEST_EQUAL(0, memcmp(X, A, N_limbs * sizeof(mbedtls_mpi_uint))); memcpy(A, A_copy, sizeof(*A) * A_limbs); #if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) - mbedtls_mpi_optionally_safe_codepath_reset(); + mbedtls_codepath_reset(); #endif mbedtls_mpi_core_exp_mod_unsafe(A, A, N, N_limbs, E, E_limbs, R2, T); #if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) - TEST_EQUAL(mbedtls_mpi_optionally_safe_codepath, MBEDTLS_MPI_IS_PUBLIC); + TEST_EQUAL(mbedtls_codepath_check, MBEDTLS_MPI_IS_PUBLIC); #endif TEST_EQUAL(0, memcmp(X, A, N_limbs * sizeof(mbedtls_mpi_uint))); diff --git a/tests/suites/test_suite_rsa.function b/tests/suites/test_suite_rsa.function index e0206eca29..75f3f428c0 100644 --- a/tests/suites/test_suite_rsa.function +++ b/tests/suites/test_suite_rsa.function @@ -3,6 +3,7 @@ #include "bignum_core.h" #include "rsa_alt_helpers.h" #include "rsa_internal.h" +#include "test/bignum_codepath_check.h" /* END_HEADER */ /* BEGIN_DEPENDENCIES @@ -491,11 +492,11 @@ void mbedtls_rsa_public(data_t *message_str, int mod, TEST_ASSERT(mbedtls_rsa_check_pubkey(&ctx) == 0); #if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) - mbedtls_mpi_optionally_safe_codepath_reset(); + mbedtls_codepath_reset(); #endif TEST_ASSERT(mbedtls_rsa_public(&ctx, message_str->x, output) == result); #if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) - TEST_EQUAL(mbedtls_mpi_optionally_safe_codepath, MBEDTLS_MPI_IS_PUBLIC); + TEST_EQUAL(mbedtls_codepath_check, MBEDTLS_MPI_IS_PUBLIC); #endif if (result == 0) { @@ -562,13 +563,13 @@ void mbedtls_rsa_private(data_t *message_str, int mod, for (i = 0; i < 3; i++) { memset(output, 0x00, sizeof(output)); #if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) - mbedtls_mpi_optionally_safe_codepath_reset(); + mbedtls_codepath_reset(); #endif TEST_ASSERT(mbedtls_rsa_private(&ctx, mbedtls_test_rnd_pseudo_rand, &rnd_info, message_str->x, output) == result); #if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) - TEST_EQUAL(mbedtls_mpi_optionally_safe_codepath, MBEDTLS_MPI_IS_SECRET); + TEST_EQUAL(mbedtls_codepath_check, MBEDTLS_MPI_IS_SECRET); #endif if (result == 0) { From e9cc10d2affa668a6847d4cffc3a53160f5239d9 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Thu, 22 Aug 2024 18:55:40 +0100 Subject: [PATCH 07/12] Fix incorrect test result Signed-off-by: Janos Follath --- tests/suites/test_suite_bignum.function | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/suites/test_suite_bignum.function b/tests/suites/test_suite_bignum.function index c71f3c8bb1..1102e18403 100644 --- a/tests/suites/test_suite_bignum.function +++ b/tests/suites/test_suite_bignum.function @@ -1129,7 +1129,7 @@ void mpi_exp_mod_size(int A_bytes, int E_bytes, int N_bytes, #endif TEST_ASSERT(mbedtls_mpi_exp_mod_unsafe(&Z, &A, &E, &N, &RR) == exp_result); #if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) - TEST_EQUAL(mbedtls_codepath_check, MBEDTLS_MPI_IS_SECRET); + TEST_EQUAL(mbedtls_codepath_check, MBEDTLS_MPI_IS_PUBLIC); #endif exit: From 0a75adcf4e943941d88deaff98951170348cc723 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Thu, 22 Aug 2024 20:00:23 +0100 Subject: [PATCH 08/12] Prepare codepath tests for early termination Signed-off-by: Janos Follath --- tests/include/test/bignum_codepath_check.h | 43 ++++++++++++++++++++++ tests/suites/test_suite_bignum.function | 14 +++---- tests/suites/test_suite_rsa.function | 4 +- 3 files changed, 52 insertions(+), 9 deletions(-) diff --git a/tests/include/test/bignum_codepath_check.h b/tests/include/test/bignum_codepath_check.h index 6ab68bb5a5..34dfc5651c 100644 --- a/tests/include/test/bignum_codepath_check.h +++ b/tests/include/test/bignum_codepath_check.h @@ -43,6 +43,49 @@ static inline void mbedtls_codepath_reset(void) mbedtls_codepath_check = MBEDTLS_MPI_IS_TEST; } +/** Check the codepath taken and fail if it doesn't match. + * + * When a function returns with an error, it can do so before reaching any interesting codepath. The + * same can happen if a parameter to the function is zero. In these cases we need to allow + * uninitialised value for the codepath tracking variable. + * + * This macro expands to an instruction, not an expression. + * It may jump to the \c exit label. + * + * \param path The expected codepath. + * This expression may be evaluated multiple times. + * \param ret The expected return value. + * \param E The MPI parameter that can cause shortcuts. + */ +#define ASSERT_BIGNUM_CODEPATH(path, ret, E) \ + do { \ + if((ret)!=0 || (E).n == 0) \ + TEST_ASSERT(mbedtls_codepath_check == (path) || \ + mbedtls_codepath_check == MBEDTLS_MPI_IS_TEST); \ + else \ + TEST_EQUAL(mbedtls_codepath_check, (path)); \ + } while (0) + +/** Check the codepath taken and fail if it doesn't match. + * + * When a function returns with an error, it can do so before reaching any interesting codepath. In + * this case we need to allow uninitialised value for the codepath tracking variable. + * + * This macro expands to an instruction, not an expression. + * It may jump to the \c exit label. + * + * \param path The expected codepath. + * This expression may be evaluated multiple times. + * \param ret The expected return value. + */ +#define ASSERT_RSA_CODEPATH(path, ret) \ + do { \ + if((ret)!=0) \ + TEST_ASSERT(mbedtls_codepath_check == (path) || \ + mbedtls_codepath_check == MBEDTLS_MPI_IS_TEST); \ + else \ + TEST_EQUAL(mbedtls_codepath_check, (path)); \ + } while (0) #endif /* MBEDTLS_TEST_HOOKS && !MBEDTLS_THREADING_C */ #endif /* BIGNUM_CODEPATH_CHECK_H */ diff --git a/tests/suites/test_suite_bignum.function b/tests/suites/test_suite_bignum.function index 1102e18403..3d2b8a1240 100644 --- a/tests/suites/test_suite_bignum.function +++ b/tests/suites/test_suite_bignum.function @@ -995,7 +995,7 @@ void mpi_exp_mod_min_RR(char *input_A, char *input_E, #endif res = mbedtls_mpi_exp_mod(&Z, &A, &E, &N, &RR); #if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) - TEST_EQUAL(mbedtls_codepath_check, MBEDTLS_MPI_IS_SECRET); + ASSERT_BIGNUM_CODEPATH(MBEDTLS_MPI_IS_SECRET, res, E); #endif /* We know that exp_mod internally needs RR to be as large as N. * Validate that it is the case now, otherwise there was probably @@ -1034,7 +1034,7 @@ void mpi_exp_mod(char *input_A, char *input_E, #endif res = mbedtls_mpi_exp_mod(&Z, &A, &E, &N, NULL); #if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) - TEST_EQUAL(mbedtls_codepath_check, MBEDTLS_MPI_IS_SECRET); + ASSERT_BIGNUM_CODEPATH(MBEDTLS_MPI_IS_SECRET, res, E); #endif TEST_ASSERT(res == exp_result); if (res == 0) { @@ -1047,7 +1047,7 @@ void mpi_exp_mod(char *input_A, char *input_E, #endif res = mbedtls_mpi_exp_mod_unsafe(&Z, &A, &E, &N, NULL); #if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) - TEST_EQUAL(mbedtls_codepath_check, MBEDTLS_MPI_IS_PUBLIC); + ASSERT_BIGNUM_CODEPATH(MBEDTLS_MPI_IS_PUBLIC, res, E); #endif TEST_ASSERT(res == exp_result); if (res == 0) { @@ -1061,7 +1061,7 @@ void mpi_exp_mod(char *input_A, char *input_E, #endif res = mbedtls_mpi_exp_mod(&Z, &A, &E, &N, &RR); #if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) - TEST_EQUAL(mbedtls_codepath_check, MBEDTLS_MPI_IS_SECRET); + ASSERT_BIGNUM_CODEPATH(MBEDTLS_MPI_IS_SECRET, res, E); #endif TEST_ASSERT(res == exp_result); if (res == 0) { @@ -1075,7 +1075,7 @@ void mpi_exp_mod(char *input_A, char *input_E, #endif res = mbedtls_mpi_exp_mod(&Z, &A, &E, &N, &RR); #if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) - TEST_EQUAL(mbedtls_codepath_check, MBEDTLS_MPI_IS_SECRET); + ASSERT_BIGNUM_CODEPATH(MBEDTLS_MPI_IS_SECRET, res, E); #endif TEST_ASSERT(res == exp_result); if (res == 0) { @@ -1121,7 +1121,7 @@ void mpi_exp_mod_size(int A_bytes, int E_bytes, int N_bytes, #endif TEST_ASSERT(mbedtls_mpi_exp_mod(&Z, &A, &E, &N, &RR) == exp_result); #if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) - TEST_EQUAL(mbedtls_codepath_check, MBEDTLS_MPI_IS_SECRET); + ASSERT_BIGNUM_CODEPATH(MBEDTLS_MPI_IS_SECRET, exp_result, E); #endif #if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) @@ -1129,7 +1129,7 @@ void mpi_exp_mod_size(int A_bytes, int E_bytes, int N_bytes, #endif TEST_ASSERT(mbedtls_mpi_exp_mod_unsafe(&Z, &A, &E, &N, &RR) == exp_result); #if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) - TEST_EQUAL(mbedtls_codepath_check, MBEDTLS_MPI_IS_PUBLIC); + ASSERT_BIGNUM_CODEPATH(MBEDTLS_MPI_IS_PUBLIC, exp_result, E); #endif exit: diff --git a/tests/suites/test_suite_rsa.function b/tests/suites/test_suite_rsa.function index 75f3f428c0..98ea9efb1c 100644 --- a/tests/suites/test_suite_rsa.function +++ b/tests/suites/test_suite_rsa.function @@ -496,7 +496,7 @@ void mbedtls_rsa_public(data_t *message_str, int mod, #endif TEST_ASSERT(mbedtls_rsa_public(&ctx, message_str->x, output) == result); #if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) - TEST_EQUAL(mbedtls_codepath_check, MBEDTLS_MPI_IS_PUBLIC); + ASSERT_RSA_CODEPATH(MBEDTLS_MPI_IS_PUBLIC, result); #endif if (result == 0) { @@ -569,7 +569,7 @@ void mbedtls_rsa_private(data_t *message_str, int mod, &rnd_info, message_str->x, output) == result); #if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) - TEST_EQUAL(mbedtls_codepath_check, MBEDTLS_MPI_IS_SECRET); + ASSERT_RSA_CODEPATH(MBEDTLS_MPI_IS_SECRET, result); #endif if (result == 0) { From 126cfedba49b51fc96763f67bb7ceee727d6ff51 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 2 Sep 2024 10:42:46 +0200 Subject: [PATCH 09/12] Fix code style MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- library/bignum_core.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/library/bignum_core.c b/library/bignum_core.c index c8b1474928..88582c2d38 100644 --- a/library/bignum_core.c +++ b/library/bignum_core.c @@ -781,8 +781,9 @@ static inline void exp_mod_calc_first_bit_optionally_safe(const mbedtls_mpi_uint *E_bit_index = E_bits % biL; #if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) - if(mbedtls_unsafe_codepath_hook != NULL) + if (mbedtls_unsafe_codepath_hook != NULL) { mbedtls_unsafe_codepath_hook(); + } #endif } else { /* @@ -792,8 +793,9 @@ static inline void exp_mod_calc_first_bit_optionally_safe(const mbedtls_mpi_uint *E_limb_index = E_limbs; *E_bit_index = 0; #if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) - if(mbedtls_safe_codepath_hook != NULL) + if (mbedtls_safe_codepath_hook != NULL) { mbedtls_safe_codepath_hook(); + } #endif } } @@ -812,8 +814,9 @@ static inline void exp_mod_table_lookup_optionally_safe(mbedtls_mpi_uint *Wselec if (window_public == MBEDTLS_MPI_IS_PUBLIC) { memcpy(Wselect, Wtable + window * AN_limbs, AN_limbs * ciL); #if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) - if(mbedtls_unsafe_codepath_hook != NULL) + if (mbedtls_unsafe_codepath_hook != NULL) { mbedtls_unsafe_codepath_hook(); + } #endif } else { /* Select Wtable[window] without leaking window through @@ -821,8 +824,9 @@ static inline void exp_mod_table_lookup_optionally_safe(mbedtls_mpi_uint *Wselec mbedtls_mpi_core_ct_uint_table_lookup(Wselect, Wtable, AN_limbs, welem, window); #if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) - if(mbedtls_safe_codepath_hook != NULL) + if (mbedtls_safe_codepath_hook != NULL) { mbedtls_safe_codepath_hook(); + } #endif } } From 4bc15d89cbaf7f4a7d70f89bdaf86047b896a5e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 2 Sep 2024 11:12:09 +0200 Subject: [PATCH 10/12] Fix guards on #include MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The rest of the file uses mbedtls_mpi_uint_t unconditionally, so its definition should also be #include'd unconditionally. Signed-off-by: Manuel Pégourié-Gonnard --- library/bignum_core.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/library/bignum_core.h b/library/bignum_core.h index 5a77ef82e2..fc761347ef 100644 --- a/library/bignum_core.h +++ b/library/bignum_core.h @@ -70,9 +70,7 @@ #include "common.h" -#if defined(MBEDTLS_BIGNUM_C) #include "mbedtls/bignum.h" -#endif #include "constant_time_internal.h" From 9ec6d45e99f259716bd957a09ec3c86c9479ccfe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 2 Sep 2024 12:41:05 +0200 Subject: [PATCH 11/12] Fix code style (for real this time, hopefully) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit For some reason I didn't think about other files in the previous commit. Signed-off-by: Manuel Pégourié-Gonnard --- tests/include/test/bignum_codepath_check.h | 10 ++++++---- tests/src/bignum_codepath_check.c | 3 +-- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/tests/include/test/bignum_codepath_check.h b/tests/include/test/bignum_codepath_check.h index 34dfc5651c..2f954e94fa 100644 --- a/tests/include/test/bignum_codepath_check.h +++ b/tests/include/test/bignum_codepath_check.h @@ -59,11 +59,12 @@ static inline void mbedtls_codepath_reset(void) */ #define ASSERT_BIGNUM_CODEPATH(path, ret, E) \ do { \ - if((ret)!=0 || (E).n == 0) \ + if ((ret) != 0 || (E).n == 0) { \ TEST_ASSERT(mbedtls_codepath_check == (path) || \ mbedtls_codepath_check == MBEDTLS_MPI_IS_TEST); \ - else \ + } else { \ TEST_EQUAL(mbedtls_codepath_check, (path)); \ + } \ } while (0) /** Check the codepath taken and fail if it doesn't match. @@ -80,11 +81,12 @@ static inline void mbedtls_codepath_reset(void) */ #define ASSERT_RSA_CODEPATH(path, ret) \ do { \ - if((ret)!=0) \ + if ((ret) != 0) { \ TEST_ASSERT(mbedtls_codepath_check == (path) || \ mbedtls_codepath_check == MBEDTLS_MPI_IS_TEST); \ - else \ + } else { \ TEST_EQUAL(mbedtls_codepath_check, (path)); \ + } \ } while (0) #endif /* MBEDTLS_TEST_HOOKS && !MBEDTLS_THREADING_C */ diff --git a/tests/src/bignum_codepath_check.c b/tests/src/bignum_codepath_check.c index b6b85d9aba..b752d13f82 100644 --- a/tests/src/bignum_codepath_check.c +++ b/tests/src/bignum_codepath_check.c @@ -13,7 +13,7 @@ int mbedtls_codepath_check = MBEDTLS_MPI_IS_TEST; void mbedtls_codepath_take_safe(void) { - if(mbedtls_codepath_check == MBEDTLS_MPI_IS_TEST) { + if (mbedtls_codepath_check == MBEDTLS_MPI_IS_TEST) { mbedtls_codepath_check = MBEDTLS_MPI_IS_SECRET; } } @@ -36,4 +36,3 @@ void mbedtls_codepath_test_hooks_teardown(void) } #endif /* MBEDTLS_TEST_HOOKS && !MBEDTLS_THREADING_C */ - From 15fa9ceedd9ca7866a7cecf8835c7624ff537483 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 3 Sep 2024 10:10:18 +0200 Subject: [PATCH 12/12] Misc improvements to comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- library/bignum_core.h | 2 +- tests/include/test/bignum_codepath_check.h | 9 +++++---- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/library/bignum_core.h b/library/bignum_core.h index fc761347ef..264ee63550 100644 --- a/library/bignum_core.h +++ b/library/bignum_core.h @@ -104,7 +104,7 @@ * } else { * // safe path * } - * not the other way round, in order to prevent misuse. (This is, if a value + * not the other way round, in order to prevent misuse. (That is, if a value * other than the two below is passed, default to the safe path.) * * The value of MBEDTLS_MPI_IS_PUBLIC is chosen in a way that is unlikely to happen by accident, but diff --git a/tests/include/test/bignum_codepath_check.h b/tests/include/test/bignum_codepath_check.h index 2f954e94fa..3d72be1b2e 100644 --- a/tests/include/test/bignum_codepath_check.h +++ b/tests/include/test/bignum_codepath_check.h @@ -6,8 +6,8 @@ * - MBEDTLS_MPI_IS_SECRET: Only safe paths were teken since the last reset * - MBEDTLS_MPI_IS_PUBLIC: At least one unsafe path has been taken since the last reset * - * Using a simple global variable to track execution path. Making it work with multithreading - * doesn't worth the effort as multithreaded tests add little to no value here. + * Use a simple global variable to track execution path. Making it work with multithreading + * isn't worth the effort as multithreaded tests add little to no value here. */ /* * Copyright The Mbed TLS Contributors @@ -47,7 +47,7 @@ static inline void mbedtls_codepath_reset(void) * * When a function returns with an error, it can do so before reaching any interesting codepath. The * same can happen if a parameter to the function is zero. In these cases we need to allow - * uninitialised value for the codepath tracking variable. + * the codepath tracking variable to still have its initial "not set" value. * * This macro expands to an instruction, not an expression. * It may jump to the \c exit label. @@ -70,7 +70,8 @@ static inline void mbedtls_codepath_reset(void) /** Check the codepath taken and fail if it doesn't match. * * When a function returns with an error, it can do so before reaching any interesting codepath. In - * this case we need to allow uninitialised value for the codepath tracking variable. + * this case we need to allow the codepath tracking variable to still have its + * initial "not set" value. * * This macro expands to an instruction, not an expression. * It may jump to the \c exit label.