From 75ed58723e93a45455ba2412d1d523931de0d978 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 18 Jun 2024 12:52:45 +0200 Subject: [PATCH 01/21] Add optionally unsafe variant of exp_mod for perf MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Attempt to partially solve the performance regression in 3.6.0 without adding too much code size. Signed-off-by: Manuel Pégourié-Gonnard --- include/mbedtls/bignum.h | 49 ++++++++++++++++++++++++++++++++++- library/bignum.c | 23 ++++++++++++++--- library/bignum_core.c | 55 +++++++++++++++++++++++++++++++--------- library/bignum_core.h | 38 +++++++++++++++++++++++++++ 4 files changed, 148 insertions(+), 17 deletions(-) diff --git a/include/mbedtls/bignum.h b/include/mbedtls/bignum.h index 71d7b97672..bb96f4fb89 100644 --- a/include/mbedtls/bignum.h +++ b/include/mbedtls/bignum.h @@ -44,6 +44,22 @@ goto cleanup; \ } while (0) +/* Constants to identify whether a value is public or secret. + * + * Parameters should be named X_public where X is the name of the + * corresponding input parameter. + * + * Implementation should always check using + * if (X_public == MBEDTLS_MPI_IS_PUBLIC) { + * // unsafe path + * } else { + * // 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.) */ +#define MBEDTLS_MPI_IS_PUBLIC 0x2a2a +#define MBEDTLS_MPI_IS_SECRET 0 + /* * Maximum size MPIs are allowed to grow to in number of limbs. */ @@ -880,7 +896,38 @@ int mbedtls_mpi_mod_int(mbedtls_mpi_uint *r, const mbedtls_mpi *A, mbedtls_mpi_sint b); /** - * \brief Perform a sliding-window exponentiation: X = A^E mod N + * \brief Perform a modular exponentiation: X = A^E mod N + * + * \param X The destination MPI. This must point to an initialized MPI. + * This must not alias E or N. + * \param A The base of the exponentiation. + * This must point to an initialized MPI. + * \param E The exponent MPI. This must point to an initialized MPI. + * \param N The base for the modular reduction. This must point to an + * initialized MPI. + * \param prec_RR A helper MPI depending solely on \p N which can be used to + * speed-up multiple modular exponentiations for the same value + * of \p N. This may be \c NULL. If it is not \c NULL, it must + * point to an initialized MPI. If it hasn't been used after + * the call to mbedtls_mpi_init(), this function will compute + * the helper value and store it in \p prec_RR for reuse on + * subsequent calls to this function. Otherwise, the function + * will assume that \p prec_RR holds the helper value set by a + * previous call to mbedtls_mpi_exp_mod(), and reuse it. + * + * \return \c 0 if successful. + * \return #MBEDTLS_ERR_MPI_ALLOC_FAILED if a memory allocation failed. + * \return #MBEDTLS_ERR_MPI_BAD_INPUT_DATA if \c N is negative or + * even, or if \c E is negative. + * \return Another negative error code on different kinds of failures. + * + */ +int mbedtls_mpi_exp_mod_optionally_safe(mbedtls_mpi *X, const mbedtls_mpi *A, + const mbedtls_mpi *E, const mbedtls_mpi *N, + mbedtls_mpi *prec_RR, int E_public); + +/** + * \brief Perform a modular exponentiation: X = A^E mod N * * \param X The destination MPI. This must point to an initialized MPI. * This must not alias E or N. diff --git a/library/bignum.c b/library/bignum.c index c45fd5bf24..4db2b10544 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1610,9 +1610,9 @@ int mbedtls_mpi_mod_int(mbedtls_mpi_uint *r, const mbedtls_mpi *A, mbedtls_mpi_s return 0; } -int mbedtls_mpi_exp_mod(mbedtls_mpi *X, const mbedtls_mpi *A, - const mbedtls_mpi *E, const mbedtls_mpi *N, - mbedtls_mpi *prec_RR) +int mbedtls_mpi_exp_mod_optionally_safe(mbedtls_mpi *X, const mbedtls_mpi *A, + const mbedtls_mpi *E, const mbedtls_mpi *N, + mbedtls_mpi *prec_RR, int E_public) { int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; @@ -1695,7 +1695,15 @@ int mbedtls_mpi_exp_mod(mbedtls_mpi *X, const mbedtls_mpi *A, { mbedtls_mpi_uint mm = mbedtls_mpi_core_montmul_init(N->p); mbedtls_mpi_core_to_mont_rep(X->p, X->p, N->p, N->n, mm, RR.p, T); - mbedtls_mpi_core_exp_mod(X->p, X->p, N->p, N->n, E->p, E->n, RR.p, T); + mbedtls_mpi_core_exp_mod_optionally_safe(X->p, + X->p, + N->p, + N->n, + E->p, + E->n, + RR.p, + T, + E_public); mbedtls_mpi_core_from_mont_rep(X->p, X->p, N->p, N->n, mm, T); } @@ -1720,6 +1728,13 @@ cleanup: return ret; } +int mbedtls_mpi_exp_mod(mbedtls_mpi *X, const mbedtls_mpi *A, + const mbedtls_mpi *E, const mbedtls_mpi *N, + mbedtls_mpi *prec_RR) +{ + return mbedtls_mpi_exp_mod_optionally_safe(X, A, E, N, prec_RR, MBEDTLS_MPI_IS_SECRET); +} + /* * Greatest common divisor: G = gcd(A, B) (HAC 14.54) */ diff --git a/library/bignum_core.c b/library/bignum_core.c index 1a3e0b9b6f..518b1bd3f3 100644 --- a/library/bignum_core.c +++ b/library/bignum_core.c @@ -758,14 +758,15 @@ static void exp_mod_precompute_window(const mbedtls_mpi_uint *A, * (The difference is that the body in our loop processes a single bit instead * of a full window.) */ -void mbedtls_mpi_core_exp_mod(mbedtls_mpi_uint *X, - const mbedtls_mpi_uint *A, - const mbedtls_mpi_uint *N, - size_t AN_limbs, - const mbedtls_mpi_uint *E, - size_t E_limbs, - const mbedtls_mpi_uint *RR, - mbedtls_mpi_uint *T) +void mbedtls_mpi_core_exp_mod_optionally_safe(mbedtls_mpi_uint *X, + const mbedtls_mpi_uint *A, + const mbedtls_mpi_uint *N, + size_t AN_limbs, + const mbedtls_mpi_uint *E, + size_t E_limbs, + const mbedtls_mpi_uint *RR, + mbedtls_mpi_uint *T, + int E_public) { const size_t wsize = exp_mod_get_window_size(E_limbs * biL); const size_t welem = ((size_t) 1) << wsize; @@ -803,6 +804,14 @@ void mbedtls_mpi_core_exp_mod(mbedtls_mpi_uint *X, * (limb_index=0, E_bit_index=0). */ size_t E_limb_index = E_limbs; size_t E_bit_index = 0; + if (E_public == MBEDTLS_MPI_IS_PUBLIC) { + size_t E_bits = mbedtls_mpi_core_bitlen(E, E_limbs); + if (E_bits != 0) { + E_limb_index = E_bits / biL; + E_bit_index = E_bits % biL; + } + } + /* At any given time, window contains window_bits bits from E. * window_bits can go up to wsize. */ size_t window_bits = 0; @@ -828,10 +837,14 @@ void mbedtls_mpi_core_exp_mod(mbedtls_mpi_uint *X, * when we've finished processing the exponent. */ if (window_bits == wsize || (E_bit_index == 0 && E_limb_index == 0)) { - /* Select Wtable[window] without leaking window through - * memory access patterns. */ - mbedtls_mpi_core_ct_uint_table_lookup(Wselect, Wtable, - AN_limbs, welem, window); + if (E_public == MBEDTLS_MPI_IS_PUBLIC) { + memcpy(Wselect, Wtable + window * AN_limbs, AN_limbs * ciL); + } else { + /* Select Wtable[window] without leaking window through + * memory access patterns. */ + mbedtls_mpi_core_ct_uint_table_lookup(Wselect, Wtable, + AN_limbs, welem, window); + } /* Multiply X by the selected element. */ mbedtls_mpi_core_montmul(X, X, Wselect, AN_limbs, N, AN_limbs, mm, temp); @@ -841,6 +854,24 @@ void mbedtls_mpi_core_exp_mod(mbedtls_mpi_uint *X, } while (!(E_bit_index == 0 && E_limb_index == 0)); } +void mbedtls_mpi_core_exp_mod(mbedtls_mpi_uint *X, + const mbedtls_mpi_uint *A, + const mbedtls_mpi_uint *N, size_t AN_limbs, + const mbedtls_mpi_uint *E, size_t E_limbs, + const mbedtls_mpi_uint *RR, + mbedtls_mpi_uint *T) +{ + mbedtls_mpi_core_exp_mod_optionally_safe(X, + A, + N, + AN_limbs, + E, + E_limbs, + RR, + T, + MBEDTLS_MPI_IS_SECRET); +} + mbedtls_mpi_uint mbedtls_mpi_core_sub_int(mbedtls_mpi_uint *X, const mbedtls_mpi_uint *A, mbedtls_mpi_uint c, /* doubles as carry */ diff --git a/library/bignum_core.h b/library/bignum_core.h index 92c8d47db5..c63cdee465 100644 --- a/library/bignum_core.h +++ b/library/bignum_core.h @@ -604,6 +604,44 @@ int mbedtls_mpi_core_random(mbedtls_mpi_uint *X, */ size_t mbedtls_mpi_core_exp_mod_working_limbs(size_t AN_limbs, size_t E_limbs); +/** + * \brief Perform a modular exponentiation with public or secret exponent: + * X = A^E mod N, where \p A is already in Montgomery form. + * + * \p X may be aliased to \p A, but not to \p RR or \p E, even if \p E_limbs == + * \p AN_limbs. + * + * \param[out] X The destination MPI, as a little endian array of length + * \p AN_limbs. + * \param[in] A The base MPI, as a little endian array of length \p AN_limbs. + * Must be in Montgomery form. + * \param[in] N The modulus, as a little endian array of length \p AN_limbs. + * \param AN_limbs The number of limbs in \p X, \p A, \p N, \p RR. + * \param[in] E The exponent, as a little endian array of length \p E_limbs. + * \param E_limbs The number of limbs in \p E. + * \param[in] RR The precomputed residue of 2^{2*biL} modulo N, as a little + * endian array of length \p AN_limbs. + * \param[in,out] T Temporary storage of at least the number of limbs returned + * by `mbedtls_mpi_core_exp_mod_working_limbs()`. + * Its initial content is unused and its final content is + * indeterminate. + * It must not alias or otherwise overlap any of the other + * parameters. + * It is up to the caller to zeroize \p T when it is no + * longer needed, and before freeing it if it was dynamically + * allocated. + * \param[in] E_public Set to MBEDTLS_MPI_IS_PUBLIC to gain some performance + * when the value of E is public. + * Set to MBEDTLS_MPI_IS_SECRET when the value of E is secret. + */ +void mbedtls_mpi_core_exp_mod_optionally_safe(mbedtls_mpi_uint *X, + const mbedtls_mpi_uint *A, + const mbedtls_mpi_uint *N, size_t AN_limbs, + const mbedtls_mpi_uint *E, size_t E_limbs, + const mbedtls_mpi_uint *RR, + mbedtls_mpi_uint *T, + int E_public); + /** * \brief Perform a modular exponentiation with secret exponent: * X = A^E mod N, where \p A is already in Montgomery form. From e084964068a2c79b1230670875b48a907e63c108 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 12 Aug 2024 17:26:24 +0100 Subject: [PATCH 02/21] Improve documentation of MBEDTLS_MPI_IS_PUBLIC Signed-off-by: Janos Follath --- include/mbedtls/bignum.h | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/include/mbedtls/bignum.h b/include/mbedtls/bignum.h index bb96f4fb89..cd7e2f6c66 100644 --- a/include/mbedtls/bignum.h +++ b/include/mbedtls/bignum.h @@ -44,7 +44,12 @@ goto cleanup; \ } while (0) -/* Constants to identify whether a value is public or secret. +/* Constants to identify whether a value is public or secret. If a parameter is marked as secret by + * this constant, the function must be constant time with respect to the parameter. + * + * This is only needed for functions with the _optionally_safe postfix. All other functions have + * fixed behavior that can't be changed at runtime and are constant time with respect to their + * parameters as prescribed by their documentation or by conventions in their module's documentation. * * Parameters should be named X_public where X is the name of the * corresponding input parameter. From 38ff70e169e5b00bbe63038456638f357f01db3e Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 12 Aug 2024 18:20:59 +0100 Subject: [PATCH 03/21] Make _optionally_safe functions internal The complexity of having functions whose security properties depend on a runtime argument can be dangerous. Limit misuse by making any such functions local. Signed-off-by: Janos Follath --- include/mbedtls/bignum.h | 8 +++++--- library/bignum.c | 31 +++++++++++++++++++------------ library/bignum_core.c | 39 ++++++++++++++++++++++++++++++--------- library/bignum_core.h | 18 ++++++++---------- 4 files changed, 62 insertions(+), 34 deletions(-) diff --git a/include/mbedtls/bignum.h b/include/mbedtls/bignum.h index cd7e2f6c66..26c61f5db2 100644 --- a/include/mbedtls/bignum.h +++ b/include/mbedtls/bignum.h @@ -903,6 +903,8 @@ int mbedtls_mpi_mod_int(mbedtls_mpi_uint *r, const mbedtls_mpi *A, /** * \brief Perform a modular exponentiation: X = A^E mod N * + * \warning This function is not constant time with respect to \p E (the exponent). + * * \param X The destination MPI. This must point to an initialized MPI. * This must not alias E or N. * \param A The base of the exponentiation. @@ -927,9 +929,9 @@ int mbedtls_mpi_mod_int(mbedtls_mpi_uint *r, const mbedtls_mpi *A, * \return Another negative error code on different kinds of failures. * */ -int mbedtls_mpi_exp_mod_optionally_safe(mbedtls_mpi *X, const mbedtls_mpi *A, - const mbedtls_mpi *E, const mbedtls_mpi *N, - mbedtls_mpi *prec_RR, int E_public); +int mbedtls_mpi_exp_mod_unsafe(mbedtls_mpi *X, const mbedtls_mpi *A, + const mbedtls_mpi *E, const mbedtls_mpi *N, + mbedtls_mpi *prec_RR); /** * \brief Perform a modular exponentiation: X = A^E mod N diff --git a/library/bignum.c b/library/bignum.c index 4db2b10544..6ac041eef9 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1610,9 +1610,13 @@ int mbedtls_mpi_mod_int(mbedtls_mpi_uint *r, const mbedtls_mpi *A, mbedtls_mpi_s return 0; } -int mbedtls_mpi_exp_mod_optionally_safe(mbedtls_mpi *X, const mbedtls_mpi *A, - const mbedtls_mpi *E, const mbedtls_mpi *N, - mbedtls_mpi *prec_RR, int E_public) +/* + * Warning! If the parameter E_public has MBEDTLS_MPI_IS_PUBLIC as its value, + * this function is not constant time with respect to the exponent (parameter E). + */ +static int mbedtls_mpi_exp_mod_optionally_safe(mbedtls_mpi *X, const mbedtls_mpi *A, + const mbedtls_mpi *E, const mbedtls_mpi *N, + mbedtls_mpi *prec_RR, int E_public) { int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; @@ -1695,15 +1699,11 @@ int mbedtls_mpi_exp_mod_optionally_safe(mbedtls_mpi *X, const mbedtls_mpi *A, { mbedtls_mpi_uint mm = mbedtls_mpi_core_montmul_init(N->p); mbedtls_mpi_core_to_mont_rep(X->p, X->p, N->p, N->n, mm, RR.p, T); - mbedtls_mpi_core_exp_mod_optionally_safe(X->p, - X->p, - N->p, - N->n, - E->p, - E->n, - RR.p, - T, - E_public); + if (E_public == MBEDTLS_MPI_IS_PUBLIC) { + mbedtls_mpi_core_exp_mod_unsafe(X->p, X->p, N->p, N->n, E->p, E->n, RR.p, T); + } else { + mbedtls_mpi_core_exp_mod(X->p, X->p, N->p, N->n, E->p, E->n, RR.p, T); + } mbedtls_mpi_core_from_mont_rep(X->p, X->p, N->p, N->n, mm, T); } @@ -1735,6 +1735,13 @@ int mbedtls_mpi_exp_mod(mbedtls_mpi *X, const mbedtls_mpi *A, return mbedtls_mpi_exp_mod_optionally_safe(X, A, E, N, prec_RR, MBEDTLS_MPI_IS_SECRET); } +int mbedtls_mpi_exp_mod_unsafe(mbedtls_mpi *X, const mbedtls_mpi *A, + const mbedtls_mpi *E, const mbedtls_mpi *N, + mbedtls_mpi *prec_RR) +{ + return mbedtls_mpi_exp_mod_optionally_safe(X, A, E, N, prec_RR, MBEDTLS_MPI_IS_PUBLIC); +} + /* * Greatest common divisor: G = gcd(A, B) (HAC 14.54) */ diff --git a/library/bignum_core.c b/library/bignum_core.c index 518b1bd3f3..ab6cf8fcdc 100644 --- a/library/bignum_core.c +++ b/library/bignum_core.c @@ -747,6 +747,9 @@ static void exp_mod_precompute_window(const mbedtls_mpi_uint *A, } /* Exponentiation: X := A^E mod N. + * + * Warning! If the parameter E_public has MBEDTLS_MPI_IS_PUBLIC as its value, + * this function is not constant time with respect to the exponent (parameter E). * * A must already be in Montgomery form. * @@ -758,15 +761,15 @@ static void exp_mod_precompute_window(const mbedtls_mpi_uint *A, * (The difference is that the body in our loop processes a single bit instead * of a full window.) */ -void mbedtls_mpi_core_exp_mod_optionally_safe(mbedtls_mpi_uint *X, - const mbedtls_mpi_uint *A, - const mbedtls_mpi_uint *N, - size_t AN_limbs, - const mbedtls_mpi_uint *E, - size_t E_limbs, - const mbedtls_mpi_uint *RR, - mbedtls_mpi_uint *T, - int E_public) +static void mbedtls_mpi_core_exp_mod_optionally_safe(mbedtls_mpi_uint *X, + const mbedtls_mpi_uint *A, + const mbedtls_mpi_uint *N, + size_t AN_limbs, + const mbedtls_mpi_uint *E, + size_t E_limbs, + const mbedtls_mpi_uint *RR, + mbedtls_mpi_uint *T, + int E_public) { const size_t wsize = exp_mod_get_window_size(E_limbs * biL); const size_t welem = ((size_t) 1) << wsize; @@ -872,6 +875,24 @@ void mbedtls_mpi_core_exp_mod(mbedtls_mpi_uint *X, MBEDTLS_MPI_IS_SECRET); } +void mbedtls_mpi_core_exp_mod_unsafe(mbedtls_mpi_uint *X, + const mbedtls_mpi_uint *A, + const mbedtls_mpi_uint *N, size_t AN_limbs, + const mbedtls_mpi_uint *E, size_t E_limbs, + const mbedtls_mpi_uint *RR, + mbedtls_mpi_uint *T) +{ + mbedtls_mpi_core_exp_mod_optionally_safe(X, + A, + N, + AN_limbs, + E, + E_limbs, + RR, + T, + MBEDTLS_MPI_IS_PUBLIC); +} + mbedtls_mpi_uint mbedtls_mpi_core_sub_int(mbedtls_mpi_uint *X, const mbedtls_mpi_uint *A, mbedtls_mpi_uint c, /* doubles as carry */ diff --git a/library/bignum_core.h b/library/bignum_core.h index c63cdee465..d208daf7ba 100644 --- a/library/bignum_core.h +++ b/library/bignum_core.h @@ -608,6 +608,8 @@ size_t mbedtls_mpi_core_exp_mod_working_limbs(size_t AN_limbs, size_t E_limbs); * \brief Perform a modular exponentiation with public or secret exponent: * X = A^E mod N, where \p A is already in Montgomery form. * + * \warning This function is not constant time with respect to \p E (the exponent). + * * \p X may be aliased to \p A, but not to \p RR or \p E, even if \p E_limbs == * \p AN_limbs. * @@ -630,17 +632,13 @@ size_t mbedtls_mpi_core_exp_mod_working_limbs(size_t AN_limbs, size_t E_limbs); * It is up to the caller to zeroize \p T when it is no * longer needed, and before freeing it if it was dynamically * allocated. - * \param[in] E_public Set to MBEDTLS_MPI_IS_PUBLIC to gain some performance - * when the value of E is public. - * Set to MBEDTLS_MPI_IS_SECRET when the value of E is secret. */ -void mbedtls_mpi_core_exp_mod_optionally_safe(mbedtls_mpi_uint *X, - const mbedtls_mpi_uint *A, - const mbedtls_mpi_uint *N, size_t AN_limbs, - const mbedtls_mpi_uint *E, size_t E_limbs, - const mbedtls_mpi_uint *RR, - mbedtls_mpi_uint *T, - int E_public); +void mbedtls_mpi_core_exp_mod_unsafe(mbedtls_mpi_uint *X, + const mbedtls_mpi_uint *A, + const mbedtls_mpi_uint *N, size_t AN_limbs, + const mbedtls_mpi_uint *E, size_t E_limbs, + const mbedtls_mpi_uint *RR, + mbedtls_mpi_uint *T); /** * \brief Perform a modular exponentiation with secret exponent: From bb3f295e4007e7953a8f314febd6cdbde9c885b7 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 12 Aug 2024 19:05:47 +0100 Subject: [PATCH 04/21] Move mixed security code to small local functions The complexity of having functions whose security properties depend on a runtime argument can be dangerous. Limit risk by isolating such code in small functions with limited scope. Signed-off-by: Janos Follath --- library/bignum_core.c | 70 +++++++++++++++++++++++++++++++++---------- 1 file changed, 55 insertions(+), 15 deletions(-) diff --git a/library/bignum_core.c b/library/bignum_core.c index ab6cf8fcdc..460a115d99 100644 --- a/library/bignum_core.c +++ b/library/bignum_core.c @@ -746,6 +746,56 @@ static void exp_mod_precompute_window(const mbedtls_mpi_uint *A, } } +/* + * This function calculates the indices of the exponent where the exponentiation algorithm should + * start processing. + * + * Warning! If the parameter E_public has MBEDTLS_MPI_IS_PUBLIC as its value, + * this function is not constant time with respect to the exponent (parameter E). + */ +static inline void exp_mod_calc_first_bit_optionally_safe(const mbedtls_mpi_uint *E, + size_t E_limbs, + int E_public, + size_t *E_limb_index, + size_t *E_bit_index) +{ + if (E_public == MBEDTLS_MPI_IS_PUBLIC) { + size_t E_bits = mbedtls_mpi_core_bitlen(E, E_limbs); + if (E_bits != 0) { + *E_limb_index = E_bits / biL; + *E_bit_index = E_bits % biL; + } + } else { + /* + * Here we need to be constant time with respect to E and can't do anything better than + * start at the first allocated bit. + */ + *E_limb_index = E_limbs; + *E_bit_index = 0; + } +} + +/* + * Warning! If the parameter window_public has MBEDTLS_MPI_IS_PUBLIC as its value, this function is + * not constant time with respect to the window parameter and consequently the exponent of the + * exponentiation (parameter E of mbedtls_mpi_core_exp_mod_optionally_safe). + */ +static inline void exp_mod_table_lookup_optionally_safe(mbedtls_mpi_uint *Wselect, + mbedtls_mpi_uint *Wtable, + size_t AN_limbs, size_t welem, + mbedtls_mpi_uint window, + int window_public) +{ + if (window_public == MBEDTLS_MPI_IS_PUBLIC) { + memcpy(Wselect, Wtable + window * AN_limbs, AN_limbs * ciL); + } else { + /* Select Wtable[window] without leaking window through + * memory access patterns. */ + mbedtls_mpi_core_ct_uint_table_lookup(Wselect, Wtable, + AN_limbs, welem, window); + } +} + /* Exponentiation: X := A^E mod N. * * Warning! If the parameter E_public has MBEDTLS_MPI_IS_PUBLIC as its value, @@ -807,13 +857,8 @@ static void mbedtls_mpi_core_exp_mod_optionally_safe(mbedtls_mpi_uint *X, * (limb_index=0, E_bit_index=0). */ size_t E_limb_index = E_limbs; size_t E_bit_index = 0; - if (E_public == MBEDTLS_MPI_IS_PUBLIC) { - size_t E_bits = mbedtls_mpi_core_bitlen(E, E_limbs); - if (E_bits != 0) { - E_limb_index = E_bits / biL; - E_bit_index = E_bits % biL; - } - } + exp_mod_calc_first_bit_optionally_safe(E, E_limbs, E_public, + &E_limb_index, &E_bit_index); /* At any given time, window contains window_bits bits from E. * window_bits can go up to wsize. */ @@ -840,14 +885,9 @@ static void mbedtls_mpi_core_exp_mod_optionally_safe(mbedtls_mpi_uint *X, * when we've finished processing the exponent. */ if (window_bits == wsize || (E_bit_index == 0 && E_limb_index == 0)) { - if (E_public == MBEDTLS_MPI_IS_PUBLIC) { - memcpy(Wselect, Wtable + window * AN_limbs, AN_limbs * ciL); - } else { - /* Select Wtable[window] without leaking window through - * memory access patterns. */ - mbedtls_mpi_core_ct_uint_table_lookup(Wselect, Wtable, - AN_limbs, welem, window); - } + + exp_mod_table_lookup_optionally_safe(Wselect, Wtable, AN_limbs, welem, + window, E_public); /* Multiply X by the selected element. */ mbedtls_mpi_core_montmul(X, X, Wselect, AN_limbs, N, AN_limbs, mm, temp); From 90b4271ff0e8bc541118b1dba49ad295c8239c19 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 12 Aug 2024 19:32:45 +0100 Subject: [PATCH 05/21] Move MBEDTLS_MPI_IS_* macros to bignum_core.h These macros are not part of any public or internal API, ideally they would be defined in the source files. The reason to put them in bignum_core.h to avoid duplication as macros for this purpose are needed in both bignum.c and bignum_core.c. Signed-off-by: Janos Follath --- include/mbedtls/bignum.h | 21 --------------------- library/bignum_core.h | 21 +++++++++++++++++++++ 2 files changed, 21 insertions(+), 21 deletions(-) diff --git a/include/mbedtls/bignum.h b/include/mbedtls/bignum.h index 26c61f5db2..a945be3614 100644 --- a/include/mbedtls/bignum.h +++ b/include/mbedtls/bignum.h @@ -44,27 +44,6 @@ goto cleanup; \ } while (0) -/* Constants to identify whether a value is public or secret. If a parameter is marked as secret by - * this constant, the function must be constant time with respect to the parameter. - * - * This is only needed for functions with the _optionally_safe postfix. All other functions have - * fixed behavior that can't be changed at runtime and are constant time with respect to their - * parameters as prescribed by their documentation or by conventions in their module's documentation. - * - * Parameters should be named X_public where X is the name of the - * corresponding input parameter. - * - * Implementation should always check using - * if (X_public == MBEDTLS_MPI_IS_PUBLIC) { - * // unsafe path - * } else { - * // 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.) */ -#define MBEDTLS_MPI_IS_PUBLIC 0x2a2a -#define MBEDTLS_MPI_IS_SECRET 0 - /* * Maximum size MPIs are allowed to grow to in number of limbs. */ diff --git a/library/bignum_core.h b/library/bignum_core.h index d208daf7ba..ee69aa7317 100644 --- a/library/bignum_core.h +++ b/library/bignum_core.h @@ -90,6 +90,27 @@ #define GET_BYTE(X, i) \ (((X)[(i) / ciL] >> (((i) % ciL) * 8)) & 0xff) +/* Constants to identify whether a value is public or secret. If a parameter is marked as secret by + * this constant, the function must be constant time with respect to the parameter. + * + * This is only needed for functions with the _optionally_safe postfix. All other functions have + * fixed behavior that can't be changed at runtime and are constant time with respect to their + * parameters as prescribed by their documentation or by conventions in their module's documentation. + * + * Parameters should be named X_public where X is the name of the + * corresponding input parameter. + * + * Implementation should always check using + * if (X_public == MBEDTLS_MPI_IS_PUBLIC) { + * // unsafe path + * } else { + * // 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.) */ +#define MBEDTLS_MPI_IS_PUBLIC 0x2a2a +#define MBEDTLS_MPI_IS_SECRET 0 + /** Count leading zero bits in a given integer. * * \warning The result is undefined if \p a == 0 From 0c292b26a55cfaebfe90ee3b4c850f257e1d4146 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 12 Aug 2024 19:55:02 +0100 Subject: [PATCH 06/21] Make MBEDTLS_MPI_IS_PUBLIC thumb friendly In Thumb instructions, constant can be: - any constant that can be produced by shifting an 8-bit value left by any number of bits within a 32-bit word - any constant of the form 0x00XY00XY - any constant of the form 0xXY00XY00 - any constant of the form 0xXYXYXYXY. Signed-off-by: Janos Follath --- library/bignum_core.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/bignum_core.h b/library/bignum_core.h index ee69aa7317..6c214a530b 100644 --- a/library/bignum_core.h +++ b/library/bignum_core.h @@ -108,7 +108,7 @@ * } * 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.) */ -#define MBEDTLS_MPI_IS_PUBLIC 0x2a2a +#define MBEDTLS_MPI_IS_PUBLIC 0x2a2a2a2a #define MBEDTLS_MPI_IS_SECRET 0 /** Count leading zero bits in a given integer. From a5fc8f342a980e2f75229fb42fb92a5d4b0b2c6a Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 12 Aug 2024 20:11:06 +0100 Subject: [PATCH 07/21] Move _public parameters next to their target It is easier to read if the parameter controlling constant timeness with respect to a parameter is next to that parameter. Signed-off-by: Janos Follath --- library/bignum.c | 8 ++++---- library/bignum_core.c | 12 ++++++------ 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index 6ac041eef9..b1f9c1bf0f 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1615,8 +1615,8 @@ int mbedtls_mpi_mod_int(mbedtls_mpi_uint *r, const mbedtls_mpi *A, mbedtls_mpi_s * this function is not constant time with respect to the exponent (parameter E). */ static int mbedtls_mpi_exp_mod_optionally_safe(mbedtls_mpi *X, const mbedtls_mpi *A, - const mbedtls_mpi *E, const mbedtls_mpi *N, - mbedtls_mpi *prec_RR, int E_public) + const mbedtls_mpi *E, int E_public, + const mbedtls_mpi *N, mbedtls_mpi *prec_RR) { int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; @@ -1732,14 +1732,14 @@ int mbedtls_mpi_exp_mod(mbedtls_mpi *X, const mbedtls_mpi *A, const mbedtls_mpi *E, const mbedtls_mpi *N, mbedtls_mpi *prec_RR) { - return mbedtls_mpi_exp_mod_optionally_safe(X, A, E, N, prec_RR, MBEDTLS_MPI_IS_SECRET); + return mbedtls_mpi_exp_mod_optionally_safe(X, A, E, MBEDTLS_MPI_IS_SECRET, N, prec_RR); } int mbedtls_mpi_exp_mod_unsafe(mbedtls_mpi *X, const mbedtls_mpi *A, const mbedtls_mpi *E, const mbedtls_mpi *N, mbedtls_mpi *prec_RR) { - return mbedtls_mpi_exp_mod_optionally_safe(X, A, E, N, prec_RR, MBEDTLS_MPI_IS_PUBLIC); + return mbedtls_mpi_exp_mod_optionally_safe(X, A, E, MBEDTLS_MPI_IS_PUBLIC, N, prec_RR); } /* diff --git a/library/bignum_core.c b/library/bignum_core.c index 460a115d99..33d66323f4 100644 --- a/library/bignum_core.c +++ b/library/bignum_core.c @@ -817,9 +817,9 @@ static void mbedtls_mpi_core_exp_mod_optionally_safe(mbedtls_mpi_uint *X, size_t AN_limbs, const mbedtls_mpi_uint *E, size_t E_limbs, + int E_public, const mbedtls_mpi_uint *RR, - mbedtls_mpi_uint *T, - int E_public) + mbedtls_mpi_uint *T) { const size_t wsize = exp_mod_get_window_size(E_limbs * biL); const size_t welem = ((size_t) 1) << wsize; @@ -910,9 +910,9 @@ void mbedtls_mpi_core_exp_mod(mbedtls_mpi_uint *X, AN_limbs, E, E_limbs, + MBEDTLS_MPI_IS_SECRET, RR, - T, - MBEDTLS_MPI_IS_SECRET); + T); } void mbedtls_mpi_core_exp_mod_unsafe(mbedtls_mpi_uint *X, @@ -928,9 +928,9 @@ void mbedtls_mpi_core_exp_mod_unsafe(mbedtls_mpi_uint *X, AN_limbs, E, E_limbs, + MBEDTLS_MPI_IS_PUBLIC, RR, - T, - MBEDTLS_MPI_IS_PUBLIC); + T); } mbedtls_mpi_uint mbedtls_mpi_core_sub_int(mbedtls_mpi_uint *X, From 020b9ab004e81bea811b9a437e976121778c222e Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Tue, 13 Aug 2024 07:53:20 +0100 Subject: [PATCH 08/21] Use actual exponent size for window calculation The allocated size can be significantly larger than the actual size. In the unsafe case we can use the actual size and gain some performance. Signed-off-by: Janos Follath --- library/bignum_core.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/library/bignum_core.c b/library/bignum_core.c index 33d66323f4..260a1f2661 100644 --- a/library/bignum_core.c +++ b/library/bignum_core.c @@ -821,7 +821,15 @@ static void mbedtls_mpi_core_exp_mod_optionally_safe(mbedtls_mpi_uint *X, const mbedtls_mpi_uint *RR, mbedtls_mpi_uint *T) { - const size_t wsize = exp_mod_get_window_size(E_limbs * biL); + /* 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 = 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); + + const size_t wsize = exp_mod_get_window_size(E_limb_index * biL); const size_t welem = ((size_t) 1) << wsize; /* This is how we will use the temporary storage T, which must have space @@ -852,14 +860,6 @@ static void mbedtls_mpi_core_exp_mod_optionally_safe(mbedtls_mpi_uint *X, /* X = 1 (in Montgomery presentation) initially */ memcpy(X, Wtable, AN_limbs * ciL); - /* 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 = 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); - /* At any given time, window contains window_bits bits from E. * window_bits can go up to wsize. */ size_t window_bits = 0; From e0842aa7512438b9e749f4137b15ee810052ecaf Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Tue, 13 Aug 2024 08:40:31 +0100 Subject: [PATCH 09/21] Add tests for optionally safe codepaths The new test hooks allow to check whether there was an unsafe call of an optionally safe function in the codepath. For the sake of simplicity the MBEDTLS_MPI_IS_* macros are reused for signalling safe/unsafe codepaths here too. Signed-off-by: Janos Follath --- library/bignum_core.c | 18 ++++++++++++++++++ library/bignum_core.h | 10 ++++++++++ tests/suites/test_suite_bignum_core.function | 12 ++++++++++++ 3 files changed, 40 insertions(+) diff --git a/library/bignum_core.c b/library/bignum_core.c index 260a1f2661..f46df0c8c7 100644 --- a/library/bignum_core.c +++ b/library/bignum_core.c @@ -765,6 +765,9 @@ static inline void exp_mod_calc_first_bit_optionally_safe(const mbedtls_mpi_uint *E_limb_index = E_bits / biL; *E_bit_index = E_bits % biL; } +#if defined(MBEDTLS_TEST_HOOKS) + mbedtls_mpi_optionally_safe_codepath = MBEDTLS_MPI_IS_PUBLIC; +#endif } else { /* * Here we need to be constant time with respect to E and can't do anything better than @@ -772,6 +775,12 @@ 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) + // 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; + } +#endif } } @@ -788,11 +797,20 @@ 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) + mbedtls_mpi_optionally_safe_codepath = MBEDTLS_MPI_IS_PUBLIC; +#endif } else { /* Select Wtable[window] without leaking window through * memory access patterns. */ mbedtls_mpi_core_ct_uint_table_lookup(Wselect, Wtable, AN_limbs, welem, window); +#if defined(MBEDTLS_TEST_HOOKS) + // 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; + } +#endif } } diff --git a/library/bignum_core.h b/library/bignum_core.h index 6c214a530b..50c53e6c39 100644 --- a/library/bignum_core.h +++ b/library/bignum_core.h @@ -817,4 +817,14 @@ void mbedtls_mpi_core_from_mont_rep(mbedtls_mpi_uint *X, mbedtls_mpi_uint mm, mbedtls_mpi_uint *T); +#if defined(MBEDTLS_TEST_HOOKS) +int mbedtls_mpi_optionally_safe_codepath; + +static inline void mbedtls_mpi_optionally_safe_codepath_reset() +{ + // 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; +} +#endif + #endif /* MBEDTLS_BIGNUM_CORE_H */ diff --git a/tests/suites/test_suite_bignum_core.function b/tests/suites/test_suite_bignum_core.function index db84d6238f..373bda4e8c 100644 --- a/tests/suites/test_suite_bignum_core.function +++ b/tests/suites/test_suite_bignum_core.function @@ -1229,13 +1229,25 @@ void mpi_core_exp_mod(char *input_N, char *input_A, TEST_CALLOC(T, working_limbs); +#if defined(MBEDTLS_TEST_HOOKS) + mbedtls_mpi_optionally_safe_codepath_reset(); +#endif mbedtls_mpi_core_exp_mod(Y, A, N, N_limbs, E, E_limbs, R2, T); +#if defined(MBEDTLS_TEST_HOOKS) + TEST_EQUAL(mbedtls_mpi_optionally_safe_codepath, MBEDTLS_MPI_IS_SECRET); +#endif TEST_EQUAL(0, memcmp(X, Y, N_limbs * sizeof(mbedtls_mpi_uint))); /* Check when output aliased to input */ +#if defined(MBEDTLS_TEST_HOOKS) + mbedtls_mpi_optionally_safe_codepath_reset(); +#endif mbedtls_mpi_core_exp_mod(A, A, N, N_limbs, E, E_limbs, R2, T); +#if defined(MBEDTLS_TEST_HOOKS) + TEST_EQUAL(mbedtls_mpi_optionally_safe_codepath, MBEDTLS_MPI_IS_SECRET); +#endif TEST_EQUAL(0, memcmp(X, A, N_limbs * sizeof(mbedtls_mpi_uint))); From 73426560984108ad1b72bcc9611c58ff0ca7166c Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Tue, 13 Aug 2024 11:39:03 +0100 Subject: [PATCH 10/21] Add tests for optionally unsafe code paths Signed-off-by: Janos Follath --- tests/suites/test_suite_bignum_core.function | 30 ++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) diff --git a/tests/suites/test_suite_bignum_core.function b/tests/suites/test_suite_bignum_core.function index 373bda4e8c..9c864ac870 100644 --- a/tests/suites/test_suite_bignum_core.function +++ b/tests/suites/test_suite_bignum_core.function @@ -1178,6 +1178,7 @@ void mpi_core_exp_mod(char *input_N, char *input_A, char *input_E, char *input_X) { mbedtls_mpi_uint *A = NULL; + mbedtls_mpi_uint *A_copy = NULL; mbedtls_mpi_uint *E = NULL; mbedtls_mpi_uint *N = NULL; mbedtls_mpi_uint *X = NULL; @@ -1229,6 +1230,8 @@ void mpi_core_exp_mod(char *input_N, char *input_A, TEST_CALLOC(T, working_limbs); + /* Test the safe variant */ + #if defined(MBEDTLS_TEST_HOOKS) mbedtls_mpi_optionally_safe_codepath_reset(); #endif @@ -1236,10 +1239,23 @@ void mpi_core_exp_mod(char *input_N, char *input_A, #if defined(MBEDTLS_TEST_HOOKS) TEST_EQUAL(mbedtls_mpi_optionally_safe_codepath, MBEDTLS_MPI_IS_SECRET); #endif - TEST_EQUAL(0, memcmp(X, Y, N_limbs * sizeof(mbedtls_mpi_uint))); - /* Check when output aliased to input */ + /* Test the unsafe variant */ + +#if defined(MBEDTLS_TEST_HOOKS) + mbedtls_mpi_optionally_safe_codepath_reset(); +#endif + mbedtls_mpi_core_exp_mod_unsafe(Y, A, N, N_limbs, E, E_limbs, R2, T); +#if defined(MBEDTLS_TEST_HOOKS) + TEST_EQUAL(mbedtls_mpi_optionally_safe_codepath, MBEDTLS_MPI_IS_PUBLIC); +#endif + TEST_EQUAL(0, memcmp(X, Y, N_limbs * sizeof(mbedtls_mpi_uint))); + + /* Check both with output aliased to input */ + + TEST_CALLOC(A_copy, A_limbs); + memcpy(A_copy, A, sizeof(A_copy) * A_limbs); #if defined(MBEDTLS_TEST_HOOKS) mbedtls_mpi_optionally_safe_codepath_reset(); @@ -1248,12 +1264,22 @@ void mpi_core_exp_mod(char *input_N, char *input_A, #if defined(MBEDTLS_TEST_HOOKS) TEST_EQUAL(mbedtls_mpi_optionally_safe_codepath, 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) + mbedtls_mpi_optionally_safe_codepath_reset(); +#endif + mbedtls_mpi_core_exp_mod_unsafe(A, A, N, N_limbs, E, E_limbs, R2, T); +#if defined(MBEDTLS_TEST_HOOKS) + TEST_EQUAL(mbedtls_mpi_optionally_safe_codepath, MBEDTLS_MPI_IS_PUBLIC); +#endif TEST_EQUAL(0, memcmp(X, A, N_limbs * sizeof(mbedtls_mpi_uint))); exit: mbedtls_free(T); mbedtls_free(A); + mbedtls_free(A_copy); mbedtls_free(E); mbedtls_free(N); mbedtls_free(X); From 2c62441f965b1c10edd4e3d62da218fcb971cfa9 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Thu, 15 Aug 2024 15:53:07 +0100 Subject: [PATCH 11/21] Fix mpi_core_exp_mod documentation Signed-off-by: Janos Follath --- library/bignum_core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/bignum_core.c b/library/bignum_core.c index f46df0c8c7..150bc766b2 100644 --- a/library/bignum_core.c +++ b/library/bignum_core.c @@ -866,7 +866,7 @@ static void mbedtls_mpi_core_exp_mod_optionally_safe(mbedtls_mpi_uint *X, const mbedtls_mpi_uint mm = mbedtls_mpi_core_montmul_init(N); - /* Set Wtable[i] = A^(2^i) (in Montgomery representation) */ + /* Set Wtable[i] = A^i (in Montgomery representation) */ exp_mod_precompute_window(A, N, AN_limbs, mm, RR, welem, Wtable, temp); From 9d72df8e6d7742741bc66f51e89d7ee65368ca33 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Thu, 15 Aug 2024 16:06:19 +0100 Subject: [PATCH 12/21] Optimise public RSA operations Signed-off-by: Janos Follath --- library/rsa.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/rsa.c b/library/rsa.c index 7eb4a259ea..d0a2695f17 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -1259,7 +1259,7 @@ int mbedtls_rsa_public(mbedtls_rsa_context *ctx, } olen = ctx->len; - MBEDTLS_MPI_CHK(mbedtls_mpi_exp_mod(&T, &T, &ctx->E, &ctx->N, &ctx->RN)); + MBEDTLS_MPI_CHK(mbedtls_mpi_exp_mod_unsafe(&T, &T, &ctx->E, &ctx->N, &ctx->RN)); MBEDTLS_MPI_CHK(mbedtls_mpi_write_binary(&T, output, olen)); cleanup: From a11269187ec25aded7de8c002e75c7fa231d7c09 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Tue, 20 Aug 2024 09:56:16 +0100 Subject: [PATCH 13/21] Fix optionally safe hooks declarations Signed-off-by: Janos Follath --- library/bignum_core.c | 5 +++++ library/bignum_core.h | 4 ++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/library/bignum_core.c b/library/bignum_core.c index 150bc766b2..463f145148 100644 --- a/library/bignum_core.c +++ b/library/bignum_core.c @@ -746,6 +746,11 @@ static void exp_mod_precompute_window(const mbedtls_mpi_uint *A, } } +#if defined(MBEDTLS_TEST_HOOKS) +// 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; +#endif + /* * This function calculates the indices of the exponent where the exponentiation algorithm should * start processing. diff --git a/library/bignum_core.h b/library/bignum_core.h index 50c53e6c39..ff6360b3e7 100644 --- a/library/bignum_core.h +++ b/library/bignum_core.h @@ -818,9 +818,9 @@ void mbedtls_mpi_core_from_mont_rep(mbedtls_mpi_uint *X, mbedtls_mpi_uint *T); #if defined(MBEDTLS_TEST_HOOKS) -int mbedtls_mpi_optionally_safe_codepath; +extern int mbedtls_mpi_optionally_safe_codepath; -static inline void mbedtls_mpi_optionally_safe_codepath_reset() +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; From 8786dd79f715f52444088858ad1e2eb79dd9f25c Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Tue, 20 Aug 2024 10:21:54 +0100 Subject: [PATCH 14/21] Disable optionally safe test hook in threading builds Signed-off-by: Janos Follath --- library/bignum_core.c | 10 +++++----- library/bignum_core.h | 5 ++++- tests/suites/test_suite_bignum_core.function | 16 ++++++++-------- 3 files changed, 17 insertions(+), 14 deletions(-) diff --git a/library/bignum_core.c b/library/bignum_core.c index 463f145148..2e2df37bb4 100644 --- a/library/bignum_core.c +++ b/library/bignum_core.c @@ -746,7 +746,7 @@ static void exp_mod_precompute_window(const mbedtls_mpi_uint *A, } } -#if defined(MBEDTLS_TEST_HOOKS) +#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; #endif @@ -770,7 +770,7 @@ static inline void exp_mod_calc_first_bit_optionally_safe(const mbedtls_mpi_uint *E_limb_index = E_bits / biL; *E_bit_index = E_bits % biL; } -#if defined(MBEDTLS_TEST_HOOKS) +#if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) mbedtls_mpi_optionally_safe_codepath = MBEDTLS_MPI_IS_PUBLIC; #endif } else { @@ -780,7 +780,7 @@ 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) +#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; @@ -802,7 +802,7 @@ 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) +#if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) mbedtls_mpi_optionally_safe_codepath = MBEDTLS_MPI_IS_PUBLIC; #endif } else { @@ -810,7 +810,7 @@ static inline void exp_mod_table_lookup_optionally_safe(mbedtls_mpi_uint *Wselec * memory access patterns. */ mbedtls_mpi_core_ct_uint_table_lookup(Wselect, Wtable, AN_limbs, welem, window); -#if defined(MBEDTLS_TEST_HOOKS) +#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; diff --git a/library/bignum_core.h b/library/bignum_core.h index ff6360b3e7..cf6485a148 100644 --- a/library/bignum_core.h +++ b/library/bignum_core.h @@ -817,7 +817,10 @@ void mbedtls_mpi_core_from_mont_rep(mbedtls_mpi_uint *X, mbedtls_mpi_uint mm, mbedtls_mpi_uint *T); -#if defined(MBEDTLS_TEST_HOOKS) +/* + * 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) diff --git a/tests/suites/test_suite_bignum_core.function b/tests/suites/test_suite_bignum_core.function index 9c864ac870..ce663299d6 100644 --- a/tests/suites/test_suite_bignum_core.function +++ b/tests/suites/test_suite_bignum_core.function @@ -1232,22 +1232,22 @@ void mpi_core_exp_mod(char *input_N, char *input_A, /* Test the safe variant */ -#if defined(MBEDTLS_TEST_HOOKS) +#if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) mbedtls_mpi_optionally_safe_codepath_reset(); #endif mbedtls_mpi_core_exp_mod(Y, A, N, N_limbs, E, E_limbs, R2, T); -#if defined(MBEDTLS_TEST_HOOKS) +#if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) TEST_EQUAL(mbedtls_mpi_optionally_safe_codepath, 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) +#if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) mbedtls_mpi_optionally_safe_codepath_reset(); #endif mbedtls_mpi_core_exp_mod_unsafe(Y, A, N, N_limbs, E, E_limbs, R2, T); -#if defined(MBEDTLS_TEST_HOOKS) +#if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) TEST_EQUAL(mbedtls_mpi_optionally_safe_codepath, MBEDTLS_MPI_IS_PUBLIC); #endif TEST_EQUAL(0, memcmp(X, Y, N_limbs * sizeof(mbedtls_mpi_uint))); @@ -1257,21 +1257,21 @@ void mpi_core_exp_mod(char *input_N, char *input_A, TEST_CALLOC(A_copy, A_limbs); memcpy(A_copy, A, sizeof(A_copy) * A_limbs); -#if defined(MBEDTLS_TEST_HOOKS) +#if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) mbedtls_mpi_optionally_safe_codepath_reset(); #endif mbedtls_mpi_core_exp_mod(A, A, N, N_limbs, E, E_limbs, R2, T); -#if defined(MBEDTLS_TEST_HOOKS) +#if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) TEST_EQUAL(mbedtls_mpi_optionally_safe_codepath, 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) +#if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) mbedtls_mpi_optionally_safe_codepath_reset(); #endif mbedtls_mpi_core_exp_mod_unsafe(A, A, N, N_limbs, E, E_limbs, R2, T); -#if defined(MBEDTLS_TEST_HOOKS) +#if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) TEST_EQUAL(mbedtls_mpi_optionally_safe_codepath, MBEDTLS_MPI_IS_PUBLIC); #endif TEST_EQUAL(0, memcmp(X, A, N_limbs * sizeof(mbedtls_mpi_uint))); From afb20796524f5bb746f4e0ee7e4e92a1e90cd380 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Tue, 20 Aug 2024 10:41:55 +0100 Subject: [PATCH 15/21] Clean up initialization in _core_exp_mod() Signed-off-by: Janos Follath --- library/bignum_core.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/library/bignum_core.c b/library/bignum_core.c index 2e2df37bb4..4231554b84 100644 --- a/library/bignum_core.c +++ b/library/bignum_core.c @@ -765,11 +765,21 @@ static inline void exp_mod_calc_first_bit_optionally_safe(const mbedtls_mpi_uint size_t *E_bit_index) { if (E_public == MBEDTLS_MPI_IS_PUBLIC) { + /* + * Skip leading zero bits. + */ size_t E_bits = mbedtls_mpi_core_bitlen(E, E_limbs); - if (E_bits != 0) { - *E_limb_index = E_bits / biL; - *E_bit_index = E_bits % biL; + if (E_bits == 0) { + /* + * If E is 0 mbedtls_mpi_core_bitlen() returns 0. Even if that is the case, we will want + * to represent it as a single 0 bit and as such the bitlength will be 1. + */ + E_bits = 1; } + + *E_limb_index = E_bits / biL; + *E_bit_index = E_bits % biL; + #if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) mbedtls_mpi_optionally_safe_codepath = MBEDTLS_MPI_IS_PUBLIC; #endif @@ -847,8 +857,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 = E_limbs; - size_t E_bit_index = 0; + size_t E_limb_index; + size_t E_bit_index; exp_mod_calc_first_bit_optionally_safe(E, E_limbs, E_public, &E_limb_index, &E_bit_index); From 878af1280729c60c4fb965005fae929963b0c20d Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Tue, 20 Aug 2024 12:33:42 +0100 Subject: [PATCH 16/21] Fix memory corruption in exp_mod tests Signed-off-by: Janos Follath --- tests/suites/test_suite_bignum_core.function | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/suites/test_suite_bignum_core.function b/tests/suites/test_suite_bignum_core.function index ce663299d6..08dac2e279 100644 --- a/tests/suites/test_suite_bignum_core.function +++ b/tests/suites/test_suite_bignum_core.function @@ -1255,7 +1255,7 @@ void mpi_core_exp_mod(char *input_N, char *input_A, /* Check both with output aliased to input */ TEST_CALLOC(A_copy, A_limbs); - memcpy(A_copy, A, sizeof(A_copy) * A_limbs); + memcpy(A_copy, A, sizeof(*A_copy) * A_limbs); #if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) mbedtls_mpi_optionally_safe_codepath_reset(); @@ -1266,7 +1266,7 @@ void mpi_core_exp_mod(char *input_N, char *input_A, #endif TEST_EQUAL(0, memcmp(X, A, N_limbs * sizeof(mbedtls_mpi_uint))); - memcpy(A, A_copy, sizeof(A) * A_limbs); + memcpy(A, A_copy, sizeof(*A) * A_limbs); #if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C) mbedtls_mpi_optionally_safe_codepath_reset(); #endif From 6c2086931d558e92beffff58181626eaf709337c Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Thu, 22 Aug 2024 12:34:10 +0100 Subject: [PATCH 17/21] Add changelog Signed-off-by: Janos Follath --- ChangeLog.d/fix-rsa-performance-regression.txt | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 ChangeLog.d/fix-rsa-performance-regression.txt diff --git a/ChangeLog.d/fix-rsa-performance-regression.txt b/ChangeLog.d/fix-rsa-performance-regression.txt new file mode 100644 index 0000000000..abcc7a5adc --- /dev/null +++ b/ChangeLog.d/fix-rsa-performance-regression.txt @@ -0,0 +1,2 @@ +Bugfix + * Fix unintended performance regression when using short RSA public keys. See #9232. From 82976f3548592e44a1328c696008bbf3af7063ad Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Thu, 22 Aug 2024 13:00:12 +0100 Subject: [PATCH 18/21] Make mbedtls_mpi_exp_mod_unsafe internal Signed-off-by: Janos Follath --- include/mbedtls/bignum.h | 33 --------------------------------- library/rsa.c | 10 ++++++++++ 2 files changed, 10 insertions(+), 33 deletions(-) diff --git a/include/mbedtls/bignum.h b/include/mbedtls/bignum.h index a945be3614..8367cd34e6 100644 --- a/include/mbedtls/bignum.h +++ b/include/mbedtls/bignum.h @@ -879,39 +879,6 @@ int mbedtls_mpi_mod_mpi(mbedtls_mpi *R, const mbedtls_mpi *A, int mbedtls_mpi_mod_int(mbedtls_mpi_uint *r, const mbedtls_mpi *A, mbedtls_mpi_sint b); -/** - * \brief Perform a modular exponentiation: X = A^E mod N - * - * \warning This function is not constant time with respect to \p E (the exponent). - * - * \param X The destination MPI. This must point to an initialized MPI. - * This must not alias E or N. - * \param A The base of the exponentiation. - * This must point to an initialized MPI. - * \param E The exponent MPI. This must point to an initialized MPI. - * \param N The base for the modular reduction. This must point to an - * initialized MPI. - * \param prec_RR A helper MPI depending solely on \p N which can be used to - * speed-up multiple modular exponentiations for the same value - * of \p N. This may be \c NULL. If it is not \c NULL, it must - * point to an initialized MPI. If it hasn't been used after - * the call to mbedtls_mpi_init(), this function will compute - * the helper value and store it in \p prec_RR for reuse on - * subsequent calls to this function. Otherwise, the function - * will assume that \p prec_RR holds the helper value set by a - * previous call to mbedtls_mpi_exp_mod(), and reuse it. - * - * \return \c 0 if successful. - * \return #MBEDTLS_ERR_MPI_ALLOC_FAILED if a memory allocation failed. - * \return #MBEDTLS_ERR_MPI_BAD_INPUT_DATA if \c N is negative or - * even, or if \c E is negative. - * \return Another negative error code on different kinds of failures. - * - */ -int mbedtls_mpi_exp_mod_unsafe(mbedtls_mpi *X, const mbedtls_mpi *A, - const mbedtls_mpi *E, const mbedtls_mpi *N, - mbedtls_mpi *prec_RR); - /** * \brief Perform a modular exponentiation: X = A^E mod N * diff --git a/library/rsa.c b/library/rsa.c index d0a2695f17..b6fea49d89 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -1228,6 +1228,16 @@ int mbedtls_rsa_check_pub_priv(const mbedtls_rsa_context *pub, return 0; } +/* + * This function is identical to mbedtls_mpi_exp_mod() the only difference is that this function is + * not constant time. + * + * WARNING! This function is not constant time. + */ +int mbedtls_mpi_exp_mod_unsafe(mbedtls_mpi *X, const mbedtls_mpi *A, + const mbedtls_mpi *E, const mbedtls_mpi *N, + mbedtls_mpi *prec_RR); + /* * Do an RSA public key operation */ From 5d16334e84c3206c380cfe9121847d23601f5e19 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Thu, 22 Aug 2024 14:49:58 +0100 Subject: [PATCH 19/21] Improve ChangeLog Co-authored-by: Gilles Peskine Signed-off-by: Janos Follath --- ChangeLog.d/fix-rsa-performance-regression.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ChangeLog.d/fix-rsa-performance-regression.txt b/ChangeLog.d/fix-rsa-performance-regression.txt index abcc7a5adc..1624ac2618 100644 --- a/ChangeLog.d/fix-rsa-performance-regression.txt +++ b/ChangeLog.d/fix-rsa-performance-regression.txt @@ -1,2 +1,2 @@ Bugfix - * Fix unintended performance regression when using short RSA public keys. See #9232. + * Fix unintended performance regression when using short RSA public keys. Fixes #9232. From 5f316972b2d0ee6ede71102a666df55b5f46e1e8 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Thu, 22 Aug 2024 14:53:13 +0100 Subject: [PATCH 20/21] Add header for mbedtls_mpi_exp_mod_unsafe() To silence no previous prototype warnings. And this is the proper way to do it anyway. Signed-off-by: Janos Follath --- library/bignum.c | 1 + library/bignum_internal.h | 50 +++++++++++++++++++++++++ library/rsa.c | 11 +----- tests/suites/test_suite_bignum.function | 1 + 4 files changed, 53 insertions(+), 10 deletions(-) create mode 100644 library/bignum_internal.h diff --git a/library/bignum.c b/library/bignum.c index b1f9c1bf0f..424490951d 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -27,6 +27,7 @@ #include "mbedtls/bignum.h" #include "bignum_core.h" +#include "bignum_internal.h" #include "bn_mul.h" #include "mbedtls/platform_util.h" #include "mbedtls/error.h" diff --git a/library/bignum_internal.h b/library/bignum_internal.h new file mode 100644 index 0000000000..aceaf55ea2 --- /dev/null +++ b/library/bignum_internal.h @@ -0,0 +1,50 @@ +/** + * \file bignum_internal.h + * + * \brief Internal-only bignum public-key cryptosystem API. + * + * This file declares bignum-related functions that are to be used + * only from within the Mbed TLS library itself. + * + */ +/* + * Copyright The Mbed TLS Contributors + * SPDX-License-Identifier: Apache-2.0 OR GPL-2.0-or-later + */ +#ifndef MBEDTLS_BIGNUM_INTERNAL_H +#define MBEDTLS_BIGNUM_INTERNAL_H + +/** + * \brief Perform a modular exponentiation: X = A^E mod N + * + * \warning This function is not constant time with respect to \p E (the exponent). + * + * \param X The destination MPI. This must point to an initialized MPI. + * This must not alias E or N. + * \param A The base of the exponentiation. + * This must point to an initialized MPI. + * \param E The exponent MPI. This must point to an initialized MPI. + * \param N The base for the modular reduction. This must point to an + * initialized MPI. + * \param prec_RR A helper MPI depending solely on \p N which can be used to + * speed-up multiple modular exponentiations for the same value + * of \p N. This may be \c NULL. If it is not \c NULL, it must + * point to an initialized MPI. If it hasn't been used after + * the call to mbedtls_mpi_init(), this function will compute + * the helper value and store it in \p prec_RR for reuse on + * subsequent calls to this function. Otherwise, the function + * will assume that \p prec_RR holds the helper value set by a + * previous call to mbedtls_mpi_exp_mod(), and reuse it. + * + * \return \c 0 if successful. + * \return #MBEDTLS_ERR_MPI_ALLOC_FAILED if a memory allocation failed. + * \return #MBEDTLS_ERR_MPI_BAD_INPUT_DATA if \c N is negative or + * even, or if \c E is negative. + * \return Another negative error code on different kinds of failures. + * + */ +int mbedtls_mpi_exp_mod_unsafe(mbedtls_mpi *X, const mbedtls_mpi *A, + const mbedtls_mpi *E, const mbedtls_mpi *N, + mbedtls_mpi *prec_RR); + +#endif /* bignum_internal.h */ diff --git a/library/rsa.c b/library/rsa.c index b6fea49d89..557faaf363 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -29,6 +29,7 @@ #include "mbedtls/rsa.h" #include "bignum_core.h" +#include "bignum_internal.h" #include "rsa_alt_helpers.h" #include "rsa_internal.h" #include "mbedtls/oid.h" @@ -1228,16 +1229,6 @@ int mbedtls_rsa_check_pub_priv(const mbedtls_rsa_context *pub, return 0; } -/* - * This function is identical to mbedtls_mpi_exp_mod() the only difference is that this function is - * not constant time. - * - * WARNING! This function is not constant time. - */ -int mbedtls_mpi_exp_mod_unsafe(mbedtls_mpi *X, const mbedtls_mpi *A, - const mbedtls_mpi *E, const mbedtls_mpi *N, - mbedtls_mpi *prec_RR); - /* * Do an RSA public key operation */ diff --git a/tests/suites/test_suite_bignum.function b/tests/suites/test_suite_bignum.function index f3a64e1837..afa97a4505 100644 --- a/tests/suites/test_suite_bignum.function +++ b/tests/suites/test_suite_bignum.function @@ -3,6 +3,7 @@ #include "mbedtls/entropy.h" #include "constant_time_internal.h" #include "bignum_core.h" +#include "bignum_internal.h" #include "test/constant_flow.h" #if MBEDTLS_MPI_MAX_BITS > 792 From 4c857c49b471d5e85a1148ab26eace3da4376eea Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Thu, 22 Aug 2024 15:45:18 +0100 Subject: [PATCH 21/21] Fix Changelog formatting Signed-off-by: Janos Follath --- ChangeLog.d/fix-rsa-performance-regression.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ChangeLog.d/fix-rsa-performance-regression.txt b/ChangeLog.d/fix-rsa-performance-regression.txt index 1624ac2618..603612a314 100644 --- a/ChangeLog.d/fix-rsa-performance-regression.txt +++ b/ChangeLog.d/fix-rsa-performance-regression.txt @@ -1,2 +1,3 @@ Bugfix - * Fix unintended performance regression when using short RSA public keys. Fixes #9232. + * Fix unintended performance regression when using short RSA public keys. + Fixes #9232.