From 1ba40585f9446372f69f19693c20f593283930af Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Tue, 13 Feb 2024 12:36:13 +0000 Subject: [PATCH 01/26] Use mpi_core_exp_mod in bignum. The two algorithms are not equivalent. The original bignum exponentiation was a sliding window algorithm. The one in mpi_core_exp_mod uses a fixed window approach. This change is intentional. We don't want to maintain two algorithms and decided to keep the fixed window algorithm. Signed-off-by: Janos Follath --- library/bignum.c | 279 ++++++++--------------------------------------- 1 file changed, 46 insertions(+), 233 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index d3d72ab747..3926da418a 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1683,13 +1683,7 @@ int mbedtls_mpi_exp_mod(mbedtls_mpi *X, const mbedtls_mpi *A, mbedtls_mpi *prec_RR) { int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; - size_t window_bitsize; - size_t i, j, nblimbs; - size_t bufsize, nbits; - size_t exponent_bits_in_window = 0; - mbedtls_mpi_uint ei, mm, state; - mbedtls_mpi RR, T, W[(size_t) 1 << MBEDTLS_MPI_WINDOW_SIZE], WW, Apos; - int neg; + mbedtls_mpi RR, T, E_core; if (mbedtls_mpi_cmp_int(N, 0) <= 0 || (N->p[0] & 1) == 0) { return MBEDTLS_ERR_MPI_BAD_INPUT_DATA; @@ -1704,89 +1698,15 @@ int mbedtls_mpi_exp_mod(mbedtls_mpi *X, const mbedtls_mpi *A, return MBEDTLS_ERR_MPI_BAD_INPUT_DATA; } - /* - * Init temps and window size - */ - mpi_montg_init(&mm, N); - mbedtls_mpi_init(&RR); mbedtls_mpi_init(&T); - mbedtls_mpi_init(&Apos); - mbedtls_mpi_init(&WW); - memset(W, 0, sizeof(W)); - - i = mbedtls_mpi_bitlen(E); - - window_bitsize = (i > 671) ? 6 : (i > 239) ? 5 : - (i > 79) ? 4 : (i > 23) ? 3 : 1; - -#if (MBEDTLS_MPI_WINDOW_SIZE < 6) - if (window_bitsize > MBEDTLS_MPI_WINDOW_SIZE) { - window_bitsize = MBEDTLS_MPI_WINDOW_SIZE; - } -#endif - - const size_t w_table_used_size = (size_t) 1 << window_bitsize; - - /* - * This function is not constant-trace: its memory accesses depend on the - * exponent value. To defend against timing attacks, callers (such as RSA - * and DHM) should use exponent blinding. However this is not enough if the - * adversary can find the exponent in a single trace, so this function - * takes extra precautions against adversaries who can observe memory - * access patterns. - * - * This function performs a series of multiplications by table elements and - * squarings, and we want the prevent the adversary from finding out which - * table element was used, and from distinguishing between multiplications - * and squarings. Firstly, when multiplying by an element of the window - * W[i], we do a constant-trace table lookup to obfuscate i. This leaves - * squarings as having a different memory access patterns from other - * multiplications. So secondly, we put the accumulator in the table as - * well, and also do a constant-trace table lookup to multiply by the - * accumulator which is W[x_index]. - * - * This way, all multiplications take the form of a lookup-and-multiply. - * The number of lookup-and-multiply operations inside each iteration of - * the main loop still depends on the bits of the exponent, but since the - * other operations in the loop don't have an easily recognizable memory - * trace, an adversary is unlikely to be able to observe the exact - * patterns. - * - * An adversary may still be able to recover the exponent if they can - * observe both memory accesses and branches. However, branch prediction - * exploitation typically requires many traces of execution over the same - * data, which is defeated by randomized blinding. - */ - const size_t x_index = 0; - mbedtls_mpi_init(&W[x_index]); - - j = N->n + 1; - /* All W[i] including the accumulator must have at least N->n limbs for - * the mpi_montmul() and mpi_montred() calls later. Here we ensure that - * W[1] and the accumulator W[x_index] are large enough. later we'll grow - * other W[i] to the same length. They must not be shrunk midway through - * this function! - */ - MBEDTLS_MPI_CHK(mbedtls_mpi_grow(&W[x_index], j)); - MBEDTLS_MPI_CHK(mbedtls_mpi_grow(&W[1], j)); - MBEDTLS_MPI_CHK(mbedtls_mpi_grow(&T, j * 2)); - - /* - * Compensate for negative A (and correct at the end) - */ - neg = (A->s == -1); - if (neg) { - MBEDTLS_MPI_CHK(mbedtls_mpi_copy(&Apos, A)); - Apos.s = 1; - A = &Apos; - } + mbedtls_mpi_init(&RR); + mbedtls_mpi_init(&T); + mbedtls_mpi_init(&E_core); /* * If 1st call, pre-compute R^2 mod N */ if (prec_RR == NULL || prec_RR->p == NULL) { - MBEDTLS_MPI_CHK(mbedtls_mpi_lset(&RR, 1)); - MBEDTLS_MPI_CHK(mbedtls_mpi_shift_l(&RR, N->n * 2 * biL)); - MBEDTLS_MPI_CHK(mbedtls_mpi_mod_mpi(&RR, &RR, N)); + MBEDTLS_MPI_CHK(mbedtls_mpi_core_get_mont_r2_unsafe(&RR, N)); if (prec_RR != NULL) { memcpy(prec_RR, &RR, sizeof(mbedtls_mpi)); @@ -1796,175 +1716,68 @@ int mbedtls_mpi_exp_mod(mbedtls_mpi *X, const mbedtls_mpi *A, } /* - * W[1] = A * R^2 * R^-1 mod N = A * R mod N + * Ensure that the exponent that we are passing to the core is not NULL. */ - if (mbedtls_mpi_cmp_mpi(A, N) >= 0) { - MBEDTLS_MPI_CHK(mbedtls_mpi_mod_mpi(&W[1], A, N)); - /* This should be a no-op because W[1] is already that large before - * mbedtls_mpi_mod_mpi(), but it's necessary to avoid an overflow - * in mpi_montmul() below, so let's make sure. */ - MBEDTLS_MPI_CHK(mbedtls_mpi_grow(&W[1], N->n + 1)); + if (E->n == 0) { + mbedtls_mpi_lset(&E_core, 0); } else { - MBEDTLS_MPI_CHK(mbedtls_mpi_copy(&W[1], A)); - } - - /* Note that this is safe because W[1] always has at least N->n limbs - * (it grew above and was preserved by mbedtls_mpi_copy()). */ - mpi_montmul(&W[1], &RR, N, mm, &T); - - /* - * W[x_index] = R^2 * R^-1 mod N = R mod N - */ - MBEDTLS_MPI_CHK(mbedtls_mpi_copy(&W[x_index], &RR)); - mpi_montred(&W[x_index], N, mm, &T); - - - if (window_bitsize > 1) { - /* - * W[i] = W[1] ^ i - * - * The first bit of the sliding window is always 1 and therefore we - * only need to store the second half of the table. - * - * (There are two special elements in the table: W[0] for the - * accumulator/result and W[1] for A in Montgomery form. Both of these - * are already set at this point.) - */ - j = w_table_used_size / 2; - - MBEDTLS_MPI_CHK(mbedtls_mpi_grow(&W[j], N->n + 1)); - MBEDTLS_MPI_CHK(mbedtls_mpi_copy(&W[j], &W[1])); - - for (i = 0; i < window_bitsize - 1; i++) { - mpi_montmul(&W[j], &W[j], N, mm, &T); - } - - /* - * W[i] = W[i - 1] * W[1] - */ - for (i = j + 1; i < w_table_used_size; i++) { - MBEDTLS_MPI_CHK(mbedtls_mpi_grow(&W[i], N->n + 1)); - MBEDTLS_MPI_CHK(mbedtls_mpi_copy(&W[i], &W[i - 1])); - - mpi_montmul(&W[i], &W[1], N, mm, &T); - } - } - - nblimbs = E->n; - bufsize = 0; - nbits = 0; - state = 0; - - while (1) { - if (bufsize == 0) { - if (nblimbs == 0) { - break; - } - - nblimbs--; - - bufsize = sizeof(mbedtls_mpi_uint) << 3; - } - - bufsize--; - - ei = (E->p[nblimbs] >> bufsize) & 1; - - /* - * skip leading 0s - */ - if (ei == 0 && state == 0) { - continue; - } - - if (ei == 0 && state == 1) { - /* - * out of window, square W[x_index] - */ - MBEDTLS_MPI_CHK(mpi_select(&WW, W, w_table_used_size, x_index)); - mpi_montmul(&W[x_index], &WW, N, mm, &T); - continue; - } - - /* - * add ei to current window - */ - state = 2; - - nbits++; - exponent_bits_in_window |= (ei << (window_bitsize - nbits)); - - if (nbits == window_bitsize) { - /* - * W[x_index] = W[x_index]^window_bitsize R^-1 mod N - */ - for (i = 0; i < window_bitsize; i++) { - MBEDTLS_MPI_CHK(mpi_select(&WW, W, w_table_used_size, - x_index)); - mpi_montmul(&W[x_index], &WW, N, mm, &T); - } - - /* - * W[x_index] = W[x_index] * W[exponent_bits_in_window] R^-1 mod N - */ - MBEDTLS_MPI_CHK(mpi_select(&WW, W, w_table_used_size, - exponent_bits_in_window)); - mpi_montmul(&W[x_index], &WW, N, mm, &T); - - state--; - nbits = 0; - exponent_bits_in_window = 0; - } + memcpy(&E_core, E, sizeof(mbedtls_mpi)); } /* - * process the remaining bits + * To preserve constness we need to make a copy of A. Using X for this to + * save memory. */ - for (i = 0; i < nbits; i++) { - MBEDTLS_MPI_CHK(mpi_select(&WW, W, w_table_used_size, x_index)); - mpi_montmul(&W[x_index], &WW, N, mm, &T); - - exponent_bits_in_window <<= 1; - - if ((exponent_bits_in_window & ((size_t) 1 << window_bitsize)) != 0) { - MBEDTLS_MPI_CHK(mpi_select(&WW, W, w_table_used_size, 1)); - mpi_montmul(&W[x_index], &WW, N, mm, &T); - } - } + MBEDTLS_MPI_CHK(mbedtls_mpi_copy(X, A)); /* - * W[x_index] = A^E * R * R^-1 mod N = A^E mod N + * Compensate for negative A (and correct at the end). */ - mpi_montred(&W[x_index], N, mm, &T); - - if (neg && E->n != 0 && (E->p[0] & 1) != 0) { - W[x_index].s = -1; - MBEDTLS_MPI_CHK(mbedtls_mpi_add_mpi(&W[x_index], N, &W[x_index])); - } + X->s = 1; /* - * Load the result in the output variable. + * Make sure that A has exactly as many limbs as N. */ - MBEDTLS_MPI_CHK(mbedtls_mpi_copy(X, &W[x_index])); + if (mbedtls_mpi_cmp_mpi(X, N) >= 0) { + MBEDTLS_MPI_CHK(mbedtls_mpi_mod_mpi(X, X, N)); + } + MBEDTLS_MPI_CHK(mbedtls_mpi_grow(X, N->n)); + + /* + * Allocate working memory for mbedtls_mpi_core_exp_mod() + */ + MBEDTLS_MPI_CHK(mbedtls_mpi_grow(&T, + mbedtls_mpi_core_exp_mod_working_limbs(N->n, E_core.n))); + + /* + * Convert to and from Montgomery around mbedtls_mpi_core_exp_mod(). + */ + 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.p); + mbedtls_mpi_core_exp_mod(X->p, X->p, N->p, N->n, E_core.p, E_core.n, RR.p, + T.p); + mbedtls_mpi_core_from_mont_rep(X->p, X->p, N->p, N->n, mm, T.p); + + /* + * Correct for negative A. + */ + if (A->s == -1 && (E_core.p[0] & 1) != 0) { + X->s = -1; + MBEDTLS_MPI_CHK(mbedtls_mpi_add_mpi(X, N, X)); + } cleanup: - /* The first bit of the sliding window is always 1 and therefore the first - * half of the table was unused. */ - for (i = w_table_used_size/2; i < w_table_used_size; i++) { - mbedtls_mpi_free(&W[i]); - } - - mbedtls_mpi_free(&W[x_index]); - mbedtls_mpi_free(&W[1]); mbedtls_mpi_free(&T); - mbedtls_mpi_free(&Apos); - mbedtls_mpi_free(&WW); if (prec_RR == NULL || prec_RR->p == NULL) { mbedtls_mpi_free(&RR); } + if (E->n == 0) { + mbedtls_mpi_free(&E_core); + } + return ret; } From 4b5edfa0bbdaa2524767e43c6a1976e6b54df0a6 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Tue, 13 Feb 2024 14:15:45 +0000 Subject: [PATCH 02/26] Bignum: remove unused functions Signed-off-by: Janos Follath --- library/bignum.c | 77 ------------------------------------------------ 1 file changed, 77 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index 3926da418a..9b8591bbe7 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1598,83 +1598,6 @@ int mbedtls_mpi_mod_int(mbedtls_mpi_uint *r, const mbedtls_mpi *A, mbedtls_mpi_s return 0; } -static void mpi_montg_init(mbedtls_mpi_uint *mm, const mbedtls_mpi *N) -{ - *mm = mbedtls_mpi_core_montmul_init(N->p); -} - -/** Montgomery multiplication: A = A * B * R^-1 mod N (HAC 14.36) - * - * \param[in,out] A One of the numbers to multiply. - * It must have at least as many limbs as N - * (A->n >= N->n), and any limbs beyond n are ignored. - * On successful completion, A contains the result of - * the multiplication A * B * R^-1 mod N where - * R = (2^ciL)^n. - * \param[in] B One of the numbers to multiply. - * It must be nonzero and must not have more limbs than N - * (B->n <= N->n). - * \param[in] N The modulus. \p N must be odd. - * \param mm The value calculated by `mpi_montg_init(&mm, N)`. - * This is -N^-1 mod 2^ciL. - * \param[in,out] T A bignum for temporary storage. - * It must be at least twice the limb size of N plus 1 - * (T->n >= 2 * N->n + 1). - * Its initial content is unused and - * its final content is indeterminate. - * It does not get reallocated. - */ -static void mpi_montmul(mbedtls_mpi *A, const mbedtls_mpi *B, - const mbedtls_mpi *N, mbedtls_mpi_uint mm, - mbedtls_mpi *T) -{ - mbedtls_mpi_core_montmul(A->p, A->p, B->p, B->n, N->p, N->n, mm, T->p); -} - -/* - * Montgomery reduction: A = A * R^-1 mod N - * - * See mpi_montmul() regarding constraints and guarantees on the parameters. - */ -static void mpi_montred(mbedtls_mpi *A, const mbedtls_mpi *N, - mbedtls_mpi_uint mm, mbedtls_mpi *T) -{ - mbedtls_mpi_uint z = 1; - mbedtls_mpi U; - U.n = 1; - U.s = 1; - U.p = &z; - - mpi_montmul(A, &U, N, mm, T); -} - -/** - * Select an MPI from a table without leaking the index. - * - * This is functionally equivalent to mbedtls_mpi_copy(R, T[idx]) except it - * reads the entire table in order to avoid leaking the value of idx to an - * attacker able to observe memory access patterns. - * - * \param[out] R Where to write the selected MPI. - * \param[in] T The table to read from. - * \param[in] T_size The number of elements in the table. - * \param[in] idx The index of the element to select; - * this must satisfy 0 <= idx < T_size. - * - * \return \c 0 on success, or a negative error code. - */ -static int mpi_select(mbedtls_mpi *R, const mbedtls_mpi *T, size_t T_size, size_t idx) -{ - int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; - - for (size_t i = 0; i < T_size; i++) { - MBEDTLS_MPI_CHK(mbedtls_mpi_safe_cond_assign(R, &T[i], - (unsigned char) mbedtls_ct_uint_eq(i, idx))); - } -cleanup: - return ret; -} - /* * Sliding-window exponentiation: X = A^E mod N (HAC 14.85) */ From 1609d57d532b9f4b51424e3882cfa8271418c074 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Wed, 14 Feb 2024 14:58:39 +0000 Subject: [PATCH 03/26] Increase default exponentiation window size The default window size as default is set to the value that believed to give the best performance. Since the algorithm changed, the fastest window size has changed as well. Signed-off-by: Janos Follath --- include/mbedtls/bignum.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/mbedtls/bignum.h b/include/mbedtls/bignum.h index 931e06d2c3..87520726cc 100644 --- a/include/mbedtls/bignum.h +++ b/include/mbedtls/bignum.h @@ -59,7 +59,7 @@ * * Reduction in size, reduces speed. */ -#define MBEDTLS_MPI_WINDOW_SIZE 2 /**< Maximum window size used. */ +#define MBEDTLS_MPI_WINDOW_SIZE 3 /**< Maximum window size used. */ #endif /* !MBEDTLS_MPI_WINDOW_SIZE */ #if !defined(MBEDTLS_MPI_MAX_SIZE) From f0543becf97d0dbab86745a574b2e38b401dcc5a Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Wed, 14 Feb 2024 15:12:16 +0000 Subject: [PATCH 04/26] Add Changelog Signed-off-by: Janos Follath --- ChangeLog.d/use_exp_mod_core.txt | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 ChangeLog.d/use_exp_mod_core.txt diff --git a/ChangeLog.d/use_exp_mod_core.txt b/ChangeLog.d/use_exp_mod_core.txt new file mode 100644 index 0000000000..e14533587a --- /dev/null +++ b/ChangeLog.d/use_exp_mod_core.txt @@ -0,0 +1,6 @@ +Changes + * Changed the default MBEDTLS_ECP_WINDOW_SIZE from 2 to 3. + The correlation between this define and RSA decryption performance has + changed lately due to security hardening. + To fix the performance degradation when using default values the + window was increased from 2 to 3. From c9faea0f70f5810dff223ecc341493ec40b37a08 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 19 Feb 2024 10:49:18 +0000 Subject: [PATCH 05/26] Bignum: Remove/update obsolete comments - We have moved to fixed window exponentiation and the algorithm used is properly documented and referenced in core already, no need for duplication. - A comment on mbedtls_mpi_copy states that mbedtls_mpi_exp_mod relies on it not to shrink X. This is not the case anymore, however we should probably still state that some functions might rely on this property as we don't know it for sure and it is safer to keep it that way. Signed-off-by: Janos Follath --- library/bignum.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index 9b8591bbe7..8a00ff5bf5 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -288,8 +288,7 @@ static int mbedtls_mpi_resize_clear(mbedtls_mpi *X, size_t limbs) * This function is not constant-time. Leading zeros in Y may be removed. * * Ensure that X does not shrink. This is not guaranteed by the public API, - * but some code in the bignum module relies on this property, for example - * in mbedtls_mpi_exp_mod(). + * but some code in the bignum module might still rely on this property. */ int mbedtls_mpi_copy(mbedtls_mpi *X, const mbedtls_mpi *Y) { @@ -1598,9 +1597,6 @@ int mbedtls_mpi_mod_int(mbedtls_mpi_uint *r, const mbedtls_mpi *A, mbedtls_mpi_s return 0; } -/* - * Sliding-window exponentiation: X = A^E mod N (HAC 14.85) - */ int mbedtls_mpi_exp_mod(mbedtls_mpi *X, const mbedtls_mpi *A, const mbedtls_mpi *E, const mbedtls_mpi *N, mbedtls_mpi *prec_RR) From 701ae1d3d9571d6a28dc65c2a566a7c7abdc6951 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 19 Feb 2024 10:56:54 +0000 Subject: [PATCH 06/26] Exp mod: move declarations before use Signed-off-by: Janos Follath --- library/bignum.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/library/bignum.c b/library/bignum.c index 8a00ff5bf5..674aab7d29 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1602,7 +1602,6 @@ int mbedtls_mpi_exp_mod(mbedtls_mpi *X, const mbedtls_mpi *A, mbedtls_mpi *prec_RR) { int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; - mbedtls_mpi RR, T, E_core; if (mbedtls_mpi_cmp_int(N, 0) <= 0 || (N->p[0] & 1) == 0) { return MBEDTLS_ERR_MPI_BAD_INPUT_DATA; @@ -1617,8 +1616,11 @@ int mbedtls_mpi_exp_mod(mbedtls_mpi *X, const mbedtls_mpi *A, return MBEDTLS_ERR_MPI_BAD_INPUT_DATA; } + mbedtls_mpi RR; mbedtls_mpi_init(&RR); + mbedtls_mpi T; mbedtls_mpi_init(&T); + mbedtls_mpi E_core; mbedtls_mpi_init(&E_core); /* From 576087d8366f05e5ceb91e7185049cc5355871fd Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 19 Feb 2024 11:05:01 +0000 Subject: [PATCH 07/26] Exp mod: use assignment instead memcpy memcpy() has the advantage of making the reader stop and arguably signal that the shallow copy here is intentional. But that hinges on having the right amount of & and the right size. An assignment is clearer and less risky. Signed-off-by: Janos Follath --- library/bignum.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index 674aab7d29..42b735f1a3 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1630,10 +1630,10 @@ int mbedtls_mpi_exp_mod(mbedtls_mpi *X, const mbedtls_mpi *A, MBEDTLS_MPI_CHK(mbedtls_mpi_core_get_mont_r2_unsafe(&RR, N)); if (prec_RR != NULL) { - memcpy(prec_RR, &RR, sizeof(mbedtls_mpi)); + *prec_RR = RR; } } else { - memcpy(&RR, prec_RR, sizeof(mbedtls_mpi)); + RR = *prec_RR; } /* @@ -1642,7 +1642,7 @@ int mbedtls_mpi_exp_mod(mbedtls_mpi *X, const mbedtls_mpi *A, if (E->n == 0) { mbedtls_mpi_lset(&E_core, 0); } else { - memcpy(&E_core, E, sizeof(mbedtls_mpi)); + E_core = *E; } /* From 583f047c9fb95448b10c7a4593bd2ac17e99bee5 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 19 Feb 2024 11:16:44 +0000 Subject: [PATCH 08/26] Exp mod: simplify 0 exponent handling Removing E_core and returning early achieves the same and is simpler (easier to read and maintain). Signed-off-by: Janos Follath --- library/bignum.c | 29 +++++++++++------------------ 1 file changed, 11 insertions(+), 18 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index 42b735f1a3..0f63c31df0 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1616,12 +1616,18 @@ int mbedtls_mpi_exp_mod(mbedtls_mpi *X, const mbedtls_mpi *A, return MBEDTLS_ERR_MPI_BAD_INPUT_DATA; } + /* + * Ensure that the exponent that we are passing to the core is not NULL. + */ + if (E->n == 0) { + ret = mbedtls_mpi_lset(X, 1); + return ret; + } + mbedtls_mpi RR; mbedtls_mpi_init(&RR); mbedtls_mpi T; mbedtls_mpi_init(&T); - mbedtls_mpi E_core; - mbedtls_mpi_init(&E_core); /* * If 1st call, pre-compute R^2 mod N @@ -1636,15 +1642,6 @@ int mbedtls_mpi_exp_mod(mbedtls_mpi *X, const mbedtls_mpi *A, RR = *prec_RR; } - /* - * Ensure that the exponent that we are passing to the core is not NULL. - */ - if (E->n == 0) { - mbedtls_mpi_lset(&E_core, 0); - } else { - E_core = *E; - } - /* * To preserve constness we need to make a copy of A. Using X for this to * save memory. @@ -1668,21 +1665,21 @@ int mbedtls_mpi_exp_mod(mbedtls_mpi *X, const mbedtls_mpi *A, * Allocate working memory for mbedtls_mpi_core_exp_mod() */ MBEDTLS_MPI_CHK(mbedtls_mpi_grow(&T, - mbedtls_mpi_core_exp_mod_working_limbs(N->n, E_core.n))); + mbedtls_mpi_core_exp_mod_working_limbs(N->n, E->n))); /* * Convert to and from Montgomery around mbedtls_mpi_core_exp_mod(). */ 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.p); - mbedtls_mpi_core_exp_mod(X->p, X->p, N->p, N->n, E_core.p, E_core.n, RR.p, + mbedtls_mpi_core_exp_mod(X->p, X->p, N->p, N->n, E->p, E->n, RR.p, T.p); mbedtls_mpi_core_from_mont_rep(X->p, X->p, N->p, N->n, mm, T.p); /* * Correct for negative A. */ - if (A->s == -1 && (E_core.p[0] & 1) != 0) { + if (A->s == -1 && (E->p[0] & 1) != 0) { X->s = -1; MBEDTLS_MPI_CHK(mbedtls_mpi_add_mpi(X, N, X)); } @@ -1695,10 +1692,6 @@ cleanup: mbedtls_mpi_free(&RR); } - if (E->n == 0) { - mbedtls_mpi_free(&E_core); - } - return ret; } From 467a5499a5466fc304c1ff3b5d0fff470f16fb4b Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 19 Feb 2024 11:27:38 +0000 Subject: [PATCH 09/26] Exp mod: clarify preprocessing Signed-off-by: Janos Follath --- library/bignum.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/library/bignum.c b/library/bignum.c index 0f63c31df0..4cebd95a95 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1654,7 +1654,14 @@ int mbedtls_mpi_exp_mod(mbedtls_mpi *X, const mbedtls_mpi *A, X->s = 1; /* - * Make sure that A has exactly as many limbs as N. + * Make sure that X is in a form that is safe for consumption by + * the core functions. + * + * - The core functions will not touch the limbs of X above N->n. The + * result will be correct if those limbs are 0, which the mod call + * ensures. + * - Also, X must have at least as many limbs as N for the calls to the + * core functions. */ if (mbedtls_mpi_cmp_mpi(X, N) >= 0) { MBEDTLS_MPI_CHK(mbedtls_mpi_mod_mpi(X, X, N)); From 518b5b60c658f91e0605af5c5dfef2836bbf9d0c Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 19 Feb 2024 11:29:34 +0000 Subject: [PATCH 10/26] Improve style Signed-off-by: Janos Follath --- library/bignum.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index 4cebd95a95..5ddcf729b5 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1671,8 +1671,8 @@ int mbedtls_mpi_exp_mod(mbedtls_mpi *X, const mbedtls_mpi *A, /* * Allocate working memory for mbedtls_mpi_core_exp_mod() */ - MBEDTLS_MPI_CHK(mbedtls_mpi_grow(&T, - mbedtls_mpi_core_exp_mod_working_limbs(N->n, E->n))); + size_t T_limbs = mbedtls_mpi_core_exp_mod_working_limbs(N->n, E->n); + MBEDTLS_MPI_CHK(mbedtls_mpi_grow(&T, T_limbs)); /* * Convert to and from Montgomery around mbedtls_mpi_core_exp_mod(). From 0512d178e01430ceeb97902a9f219e6575f672ee Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Tue, 20 Feb 2024 14:30:46 +0000 Subject: [PATCH 11/26] Exp mod: Make sure RR has enough limbs When generated by exp_mod, RR has enough limbs to be passed as a parameter to core functions. If it is received from the caller, it might be of any length. Signed-off-by: Janos Follath --- library/bignum.c | 1 + tests/suites/test_suite_bignum.function | 31 ++++++++++++++++++++++++ tests/suites/test_suite_bignum.misc.data | 6 +++++ 3 files changed, 38 insertions(+) diff --git a/library/bignum.c b/library/bignum.c index 5ddcf729b5..0b8fec36e5 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1639,6 +1639,7 @@ int mbedtls_mpi_exp_mod(mbedtls_mpi *X, const mbedtls_mpi *A, *prec_RR = RR; } } else { + MBEDTLS_MPI_CHK(mbedtls_mpi_grow(prec_RR, N->n)); RR = *prec_RR; } diff --git a/tests/suites/test_suite_bignum.function b/tests/suites/test_suite_bignum.function index 50be2d2f17..61df16ecab 100644 --- a/tests/suites/test_suite_bignum.function +++ b/tests/suites/test_suite_bignum.function @@ -965,6 +965,37 @@ exit: } /* END_CASE */ +/* BEGIN_CASE */ +void mpi_exp_mod_min_RR(char *input_A, char *input_E, + char *input_N, char *input_X, + int exp_result) +{ + mbedtls_mpi A, E, N, RR, Z, X; + int res; + mbedtls_mpi_init(&A); mbedtls_mpi_init(&E); mbedtls_mpi_init(&N); + mbedtls_mpi_init(&RR); mbedtls_mpi_init(&Z); mbedtls_mpi_init(&X); + + TEST_ASSERT(mbedtls_test_read_mpi(&A, input_A) == 0); + TEST_ASSERT(mbedtls_test_read_mpi(&E, input_E) == 0); + TEST_ASSERT(mbedtls_test_read_mpi(&N, input_N) == 0); + TEST_ASSERT(mbedtls_test_read_mpi(&X, input_X) == 0); + + TEST_ASSERT(mbedtls_mpi_core_get_mont_r2_unsafe(&RR, &N) == 0); + TEST_ASSERT(mbedtls_mpi_shrink(&RR, 0) == 0); + + res = mbedtls_mpi_exp_mod(&Z, &A, &E, &N, &RR); + TEST_ASSERT(res == exp_result); + if (res == 0) { + TEST_ASSERT(sign_is_valid(&Z)); + TEST_ASSERT(mbedtls_mpi_cmp_mpi(&Z, &X) == 0); + } + +exit: + mbedtls_mpi_free(&A); mbedtls_mpi_free(&E); mbedtls_mpi_free(&N); + mbedtls_mpi_free(&RR); mbedtls_mpi_free(&Z); mbedtls_mpi_free(&X); +} +/* END_CASE */ + /* BEGIN_CASE */ void mpi_exp_mod(char *input_A, char *input_E, char *input_N, char *input_X, diff --git a/tests/suites/test_suite_bignum.misc.data b/tests/suites/test_suite_bignum.misc.data index c53e42a8f3..8f5218c1fe 100644 --- a/tests/suites/test_suite_bignum.misc.data +++ b/tests/suites/test_suite_bignum.misc.data @@ -1391,6 +1391,12 @@ Test mbedtls_mpi_exp_mod (Negative base) [#2] depends_on:MPI_MAX_BITS_LARGER_THAN_792 mpi_exp_mod:"-9f13012cd92aa72fb86ac8879d2fde4f7fd661aaae43a00971f081cc60ca277059d5c37e89652e2af2585d281d66ef6a9d38a117e9608e9e7574cd142dc55278838a2161dd56db9470d4c1da2d5df15a908ee2eb886aaa890f23be16de59386663a12f1afbb325431a3e835e3fd89b98b96a6f77382f458ef9a37e1f84a03045c8676ab55291a94c2228ea15448ee96b626b998":"40a54d1b9e86789f06d9607fb158672d64867665c73ee9abb545fc7a785634b354c7bae5b962ce8040cf45f2c1f3d3659b2ee5ede17534c8fc2ec85c815e8df1fe7048d12c90ee31b88a68a081f17f0d8ce5f4030521e9400083bcea73a429031d4ca7949c2000d597088e0c39a6014d8bf962b73bb2e8083bd0390a4e00b9b3":"eeaf0ab9adb38dd69c33f80afa8fc5e86072618775ff3c0b9ea2314c9c256576d674df7496ea81d3383b4813d692c6e0e0d5d8e250b98be48e495c1d6089dad15dc7d7b46154d6b6ce8ef4ad69b15d4982559b297bcf1885c529f566660e57ec68edbc3c05726cc02fd4cbf4976eaa9afd5138fe8376435b9fc61d2fc0eb06e3":"21acc7199e1b90f9b4844ffe12c19f00ec548c5d32b21c647d48b6015d8eb9ec9db05b4f3d44db4227a2b5659c1a7cceb9d5fa8fa60376047953ce7397d90aaeb7465e14e820734f84aa52ad0fc66701bcbb991d57715806a11531268e1e83dd48288c72b424a6287e9ce4e5cc4db0dd67614aecc23b0124a5776d36e5c89483":0 +Test mbedtls_mpi_exp_mod (N.n=3, RR.n=1 on 32 bit) +mpi_exp_mod_min_RR:"10":"2":"10000000100000001":"100":0 + +Test mbedtls_mpi_exp_mod (N.n=3, RR.n=1 on 64 bit) +mpi_exp_mod_min_RR:"10":"2":"100000000000000010000000000000001":"100":0 + Base test GCD #1 mpi_gcd:"2b5":"261":"15" From 424c2655b98ab00fa35aa4a25fe553d24a17e638 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Wed, 21 Feb 2024 09:26:36 +0000 Subject: [PATCH 12/26] Exp mod: tidy up temporary storage allocation Signed-off-by: Janos Follath --- library/bignum.c | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index 0b8fec36e5..7681982298 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1624,10 +1624,17 @@ int mbedtls_mpi_exp_mod(mbedtls_mpi *X, const mbedtls_mpi *A, return ret; } + /* + * Allocate working memory for mbedtls_mpi_core_exp_mod() + */ + size_t T_limbs = mbedtls_mpi_core_exp_mod_working_limbs(N->n, E->n); + mbedtls_mpi_uint *T = (mbedtls_mpi_uint*) mbedtls_calloc(T_limbs, sizeof(mbedtls_mpi_uint)); + if (T == NULL) { + return MBEDTLS_ERR_MPI_ALLOC_FAILED; + } + mbedtls_mpi RR; mbedtls_mpi_init(&RR); - mbedtls_mpi T; - mbedtls_mpi_init(&T); /* * If 1st call, pre-compute R^2 mod N @@ -1669,20 +1676,14 @@ int mbedtls_mpi_exp_mod(mbedtls_mpi *X, const mbedtls_mpi *A, } MBEDTLS_MPI_CHK(mbedtls_mpi_grow(X, N->n)); - /* - * Allocate working memory for mbedtls_mpi_core_exp_mod() - */ - size_t T_limbs = mbedtls_mpi_core_exp_mod_working_limbs(N->n, E->n); - MBEDTLS_MPI_CHK(mbedtls_mpi_grow(&T, T_limbs)); - /* * Convert to and from Montgomery around mbedtls_mpi_core_exp_mod(). */ 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.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.p); - mbedtls_mpi_core_from_mont_rep(X->p, X->p, N->p, N->n, mm, T.p); + T); + mbedtls_mpi_core_from_mont_rep(X->p, X->p, N->p, N->n, mm, T); /* * Correct for negative A. @@ -1694,7 +1695,7 @@ int mbedtls_mpi_exp_mod(mbedtls_mpi *X, const mbedtls_mpi *A, cleanup: - mbedtls_mpi_free(&T); + mbedtls_mpi_zeroize_and_free(T, T_limbs); if (prec_RR == NULL || prec_RR->p == NULL) { mbedtls_mpi_free(&RR); From aec1a868fe783a85cd057b78163bd0b1e143f0c6 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Wed, 21 Feb 2024 11:24:20 +0000 Subject: [PATCH 13/26] Use mbedtls_ct_condition_t in mpi_core_check_zero Signed-off-by: Janos Follath --- library/bignum_core.c | 6 +++--- library/bignum_core.h | 8 ++++---- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/library/bignum_core.c b/library/bignum_core.c index dfed60d55b..f66739df92 100644 --- a/library/bignum_core.c +++ b/library/bignum_core.c @@ -856,8 +856,8 @@ mbedtls_mpi_uint mbedtls_mpi_core_sub_int(mbedtls_mpi_uint *X, return c; } -mbedtls_mpi_uint mbedtls_mpi_core_check_zero_ct(const mbedtls_mpi_uint *A, - size_t limbs) +mbedtls_ct_condition_t mbedtls_mpi_core_check_zero_ct(const mbedtls_mpi_uint *A, + size_t limbs) { mbedtls_mpi_uint bits = 0; @@ -865,7 +865,7 @@ mbedtls_mpi_uint mbedtls_mpi_core_check_zero_ct(const mbedtls_mpi_uint *A, bits |= A[i]; } - return bits; + return mbedtls_ct_bool(bits); } void mbedtls_mpi_core_to_mont_rep(mbedtls_mpi_uint *X, diff --git a/library/bignum_core.h b/library/bignum_core.h index b56be0a714..92c8d47db5 100644 --- a/library/bignum_core.h +++ b/library/bignum_core.h @@ -662,11 +662,11 @@ mbedtls_mpi_uint mbedtls_mpi_core_sub_int(mbedtls_mpi_uint *X, * \param[in] A The MPI to test. * \param limbs Number of limbs in \p A. * - * \return 0 if `A == 0` - * non-0 (may be any value) if `A != 0`. + * \return MBEDTLS_CT_FALSE if `A == 0` + * MBEDTLS_CT_TRUE if `A != 0`. */ -mbedtls_mpi_uint mbedtls_mpi_core_check_zero_ct(const mbedtls_mpi_uint *A, - size_t limbs); +mbedtls_ct_condition_t mbedtls_mpi_core_check_zero_ct(const mbedtls_mpi_uint *A, + size_t limbs); /** * \brief Returns the number of limbs of working memory required for From 86258f51b5bcbe4e2f8cbad6ef3c2e162f0d619b Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Wed, 21 Feb 2024 11:25:41 +0000 Subject: [PATCH 14/26] Exp mod: handle negative zero Signed-off-by: Janos Follath --- library/bignum.c | 4 +++- tests/suites/test_suite_bignum.misc.data | 3 +++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/library/bignum.c b/library/bignum.c index 7681982298..08b8b3429c 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1689,7 +1689,9 @@ int mbedtls_mpi_exp_mod(mbedtls_mpi *X, const mbedtls_mpi *A, * Correct for negative A. */ if (A->s == -1 && (E->p[0] & 1) != 0) { - X->s = -1; + mbedtls_ct_condition_t is_x_non_zero = mbedtls_mpi_core_check_zero_ct(X->p, X->n); + X->s = mbedtls_ct_uint_if(is_x_non_zero, -1, 1); + MBEDTLS_MPI_CHK(mbedtls_mpi_add_mpi(X, N, X)); } diff --git a/tests/suites/test_suite_bignum.misc.data b/tests/suites/test_suite_bignum.misc.data index 8f5218c1fe..a953153abe 100644 --- a/tests/suites/test_suite_bignum.misc.data +++ b/tests/suites/test_suite_bignum.misc.data @@ -1362,6 +1362,9 @@ mpi_exp_mod:"04":"00":"09":"1":0 Test mbedtls_mpi_exp_mod: 10 ^ 0 (1 limb) mod 9 mpi_exp_mod:"0a":"00":"09":"1":0 +Test mbedtls_mpi_exp_mod: -3 ^ 3 mod 27 +mpi_exp_mod:"-3":"3":"1b":"1b":0 + Test mbedtls_mpi_exp_mod: MAX_SIZE exponent mpi_exp_mod_size:2:MBEDTLS_MPI_MAX_SIZE:10:"":0 From 6bd5cae3e690054b3d5c8ae65841d01698ea9ecf Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Wed, 21 Feb 2024 11:27:31 +0000 Subject: [PATCH 15/26] Fix MBEDTLS_MPI_WINDOW_SIZE documentation Signed-off-by: Janos Follath --- include/mbedtls/bignum.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/mbedtls/bignum.h b/include/mbedtls/bignum.h index 87520726cc..71d7b97672 100644 --- a/include/mbedtls/bignum.h +++ b/include/mbedtls/bignum.h @@ -51,11 +51,11 @@ #if !defined(MBEDTLS_MPI_WINDOW_SIZE) /* - * Maximum window size used for modular exponentiation. Default: 2 + * Maximum window size used for modular exponentiation. Default: 3 * Minimum value: 1. Maximum value: 6. * * Result is an array of ( 2 ** MBEDTLS_MPI_WINDOW_SIZE ) MPIs used - * for the sliding window calculation. (So 64 by default) + * for the sliding window calculation. (So 8 by default) * * Reduction in size, reduces speed. */ From 0902572aa4f977a0ca067a3e2a149ada2b84520b Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Wed, 21 Feb 2024 11:50:25 +0000 Subject: [PATCH 16/26] Fix style Signed-off-by: Janos Follath --- library/bignum.c | 2 +- tests/suites/test_suite_bignum.function | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index 08b8b3429c..0e3eb4b8ac 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1628,7 +1628,7 @@ int mbedtls_mpi_exp_mod(mbedtls_mpi *X, const mbedtls_mpi *A, * Allocate working memory for mbedtls_mpi_core_exp_mod() */ size_t T_limbs = mbedtls_mpi_core_exp_mod_working_limbs(N->n, E->n); - mbedtls_mpi_uint *T = (mbedtls_mpi_uint*) mbedtls_calloc(T_limbs, sizeof(mbedtls_mpi_uint)); + mbedtls_mpi_uint *T = (mbedtls_mpi_uint *) mbedtls_calloc(T_limbs, sizeof(mbedtls_mpi_uint)); if (T == NULL) { return MBEDTLS_ERR_MPI_ALLOC_FAILED; } diff --git a/tests/suites/test_suite_bignum.function b/tests/suites/test_suite_bignum.function index 61df16ecab..47a6c981aa 100644 --- a/tests/suites/test_suite_bignum.function +++ b/tests/suites/test_suite_bignum.function @@ -967,8 +967,8 @@ exit: /* BEGIN_CASE */ void mpi_exp_mod_min_RR(char *input_A, char *input_E, - char *input_N, char *input_X, - int exp_result) + char *input_N, char *input_X, + int exp_result) { mbedtls_mpi A, E, N, RR, Z, X; int res; From fdab78685245661d442df72b439d39daa908ef13 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Thu, 22 Feb 2024 15:19:13 +0000 Subject: [PATCH 17/26] Use TEST_EQUAL instead of TEST_ASSERT in new code Signed-off-by: Janos Follath --- tests/suites/test_suite_bignum.function | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/suites/test_suite_bignum.function b/tests/suites/test_suite_bignum.function index 47a6c981aa..e4d4af8191 100644 --- a/tests/suites/test_suite_bignum.function +++ b/tests/suites/test_suite_bignum.function @@ -975,19 +975,19 @@ void mpi_exp_mod_min_RR(char *input_A, char *input_E, mbedtls_mpi_init(&A); mbedtls_mpi_init(&E); mbedtls_mpi_init(&N); mbedtls_mpi_init(&RR); mbedtls_mpi_init(&Z); mbedtls_mpi_init(&X); - TEST_ASSERT(mbedtls_test_read_mpi(&A, input_A) == 0); - TEST_ASSERT(mbedtls_test_read_mpi(&E, input_E) == 0); - TEST_ASSERT(mbedtls_test_read_mpi(&N, input_N) == 0); - TEST_ASSERT(mbedtls_test_read_mpi(&X, input_X) == 0); + TEST_EQUAL(mbedtls_test_read_mpi(&A, input_A), 0); + TEST_EQUAL(mbedtls_test_read_mpi(&E, input_E), 0); + TEST_EQUAL(mbedtls_test_read_mpi(&N, input_N), 0); + TEST_EQUAL(mbedtls_test_read_mpi(&X, input_X), 0); - TEST_ASSERT(mbedtls_mpi_core_get_mont_r2_unsafe(&RR, &N) == 0); - TEST_ASSERT(mbedtls_mpi_shrink(&RR, 0) == 0); + TEST_EQUAL(mbedtls_mpi_core_get_mont_r2_unsafe(&RR, &N), 0); + TEST_EQUAL(mbedtls_mpi_shrink(&RR, 0), 0); res = mbedtls_mpi_exp_mod(&Z, &A, &E, &N, &RR); - TEST_ASSERT(res == exp_result); + TEST_EQUAL(res, exp_result); if (res == 0) { - TEST_ASSERT(sign_is_valid(&Z)); - TEST_ASSERT(mbedtls_mpi_cmp_mpi(&Z, &X) == 0); + TEST_EQUAL(sign_is_valid(&Z), 1); + TEST_EQUAL(mbedtls_mpi_cmp_mpi(&Z, &X), 0); } exit: From 673461c389c8f54b4d127b1f7090b7e5da3f606e Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 21 Feb 2024 16:03:04 +0100 Subject: [PATCH 18/26] Improve validation in mpi_exp_mod_min_RR Check that the test case is hitting what it's supposed to hit, and that the library takes the expected defensive measure. Signed-off-by: Gilles Peskine --- tests/suites/test_suite_bignum.function | 8 ++++++++ tests/suites/test_suite_bignum.misc.data | 2 ++ 2 files changed, 10 insertions(+) diff --git a/tests/suites/test_suite_bignum.function b/tests/suites/test_suite_bignum.function index e4d4af8191..f3a64e1837 100644 --- a/tests/suites/test_suite_bignum.function +++ b/tests/suites/test_suite_bignum.function @@ -982,8 +982,16 @@ void mpi_exp_mod_min_RR(char *input_A, char *input_E, TEST_EQUAL(mbedtls_mpi_core_get_mont_r2_unsafe(&RR, &N), 0); TEST_EQUAL(mbedtls_mpi_shrink(&RR, 0), 0); + /* The objective of this test is to check that exp_mod defends + * against a smaller RR. */ + TEST_LE_U(RR.n, N.n - 1); res = mbedtls_mpi_exp_mod(&Z, &A, &E, &N, &RR); + /* 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. */ + TEST_EQUAL(RR.n, N.n); + TEST_EQUAL(res, exp_result); if (res == 0) { TEST_EQUAL(sign_is_valid(&Z), 1); diff --git a/tests/suites/test_suite_bignum.misc.data b/tests/suites/test_suite_bignum.misc.data index a953153abe..eb55dbe33b 100644 --- a/tests/suites/test_suite_bignum.misc.data +++ b/tests/suites/test_suite_bignum.misc.data @@ -1395,9 +1395,11 @@ depends_on:MPI_MAX_BITS_LARGER_THAN_792 mpi_exp_mod:"-9f13012cd92aa72fb86ac8879d2fde4f7fd661aaae43a00971f081cc60ca277059d5c37e89652e2af2585d281d66ef6a9d38a117e9608e9e7574cd142dc55278838a2161dd56db9470d4c1da2d5df15a908ee2eb886aaa890f23be16de59386663a12f1afbb325431a3e835e3fd89b98b96a6f77382f458ef9a37e1f84a03045c8676ab55291a94c2228ea15448ee96b626b998":"40a54d1b9e86789f06d9607fb158672d64867665c73ee9abb545fc7a785634b354c7bae5b962ce8040cf45f2c1f3d3659b2ee5ede17534c8fc2ec85c815e8df1fe7048d12c90ee31b88a68a081f17f0d8ce5f4030521e9400083bcea73a429031d4ca7949c2000d597088e0c39a6014d8bf962b73bb2e8083bd0390a4e00b9b3":"eeaf0ab9adb38dd69c33f80afa8fc5e86072618775ff3c0b9ea2314c9c256576d674df7496ea81d3383b4813d692c6e0e0d5d8e250b98be48e495c1d6089dad15dc7d7b46154d6b6ce8ef4ad69b15d4982559b297bcf1885c529f566660e57ec68edbc3c05726cc02fd4cbf4976eaa9afd5138fe8376435b9fc61d2fc0eb06e3":"21acc7199e1b90f9b4844ffe12c19f00ec548c5d32b21c647d48b6015d8eb9ec9db05b4f3d44db4227a2b5659c1a7cceb9d5fa8fa60376047953ce7397d90aaeb7465e14e820734f84aa52ad0fc66701bcbb991d57715806a11531268e1e83dd48288c72b424a6287e9ce4e5cc4db0dd67614aecc23b0124a5776d36e5c89483":0 Test mbedtls_mpi_exp_mod (N.n=3, RR.n=1 on 32 bit) +depends_on:MBEDTLS_HAVE_INT32 mpi_exp_mod_min_RR:"10":"2":"10000000100000001":"100":0 Test mbedtls_mpi_exp_mod (N.n=3, RR.n=1 on 64 bit) +depends_on:MBEDTLS_HAVE_INT64 mpi_exp_mod_min_RR:"10":"2":"100000000000000010000000000000001":"100":0 Base test GCD #1 From bd0a683e78941696fa3939c1d21aafca4aca5008 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Thu, 22 Feb 2024 15:48:01 +0000 Subject: [PATCH 19/26] Improve changelog Signed-off-by: Janos Follath --- ChangeLog.d/use_exp_mod_core.txt | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/ChangeLog.d/use_exp_mod_core.txt b/ChangeLog.d/use_exp_mod_core.txt index e14533587a..8f7193a310 100644 --- a/ChangeLog.d/use_exp_mod_core.txt +++ b/ChangeLog.d/use_exp_mod_core.txt @@ -1,6 +1,6 @@ Changes - * Changed the default MBEDTLS_ECP_WINDOW_SIZE from 2 to 3. - The correlation between this define and RSA decryption performance has - changed lately due to security hardening. - To fix the performance degradation when using default values the - window was increased from 2 to 3. + * mbedtls_mpi_exp_mod and code that uses it, notably RSA and DHM operations, + have changed their speed/memory compromise as part of a proactive security + improvement. The new default value of MBEDTLS_MPI_WINDOW_SIZE roughly + preserves the current speed, at the expense of increasing memory + consumption. From 30f49f19cca6f8e30c470c7f84dce98a8c6438d2 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Fri, 8 Mar 2024 16:29:54 +0000 Subject: [PATCH 20/26] Hinder unwanted optimisations We want this function to be constant time. Make it less likely that the compiler optimises it. 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 f66739df92..8374f3ae60 100644 --- a/library/bignum_core.c +++ b/library/bignum_core.c @@ -859,7 +859,7 @@ mbedtls_mpi_uint mbedtls_mpi_core_sub_int(mbedtls_mpi_uint *X, mbedtls_ct_condition_t mbedtls_mpi_core_check_zero_ct(const mbedtls_mpi_uint *A, size_t limbs) { - mbedtls_mpi_uint bits = 0; + volatile mbedtls_mpi_uint bits = 0; for (size_t i = 0; i < limbs; i++) { bits |= A[i]; From 4ec0fb592475add23df9284ee82b6805f82e24d9 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Fri, 8 Mar 2024 17:22:40 +0000 Subject: [PATCH 21/26] Avoid implementation defined behaviour The conversion back to signed short is an issue: the uint cast results in (-1 + UINT_MAX), which is OK. But then that can't be cast back to a signed value: "if the new type is signed and the value cannot be represented in it; either the result is implementation-defined or an implementation-defined signal is raised." Signed-off-by: Janos Follath --- library/bignum.c | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index 0e3eb4b8ac..78f6149a68 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -37,6 +37,19 @@ #include "mbedtls/platform.h" + + +/* + * Conditionally select an MPI sign in constant time. + * (MPI sign is the field s in mbedtls_mpi. It is unsigned short and only 1 and -1 are valid + * values.) + */ +static inline unsigned short mbedtls_ct_mpi_sign_if(mbedtls_ct_condition_t cond, + unsigned short sign1, unsigned short sign2) +{ + return (unsigned short) mbedtls_ct_uint_if(cond, sign1 + 1, sign2 + 1) - 1; +} + /* * Compare signed values in constant time */ @@ -112,7 +125,7 @@ int mbedtls_mpi_safe_cond_assign(mbedtls_mpi *X, { mbedtls_ct_condition_t do_assign = mbedtls_ct_bool(assign); - X->s = (int) mbedtls_ct_uint_if(do_assign, Y->s, X->s); + X->s = mbedtls_ct_mpi_sign_if(do_assign, Y->s, X->s); mbedtls_mpi_core_cond_assign(X->p, Y->p, Y->n, do_assign); @@ -149,8 +162,8 @@ int mbedtls_mpi_safe_cond_swap(mbedtls_mpi *X, MBEDTLS_MPI_CHK(mbedtls_mpi_grow(Y, X->n)); s = X->s; - X->s = (int) mbedtls_ct_uint_if(do_swap, Y->s, X->s); - Y->s = (int) mbedtls_ct_uint_if(do_swap, s, Y->s); + X->s = mbedtls_ct_mpi_sign_if(do_swap, Y->s, X->s); + Y->s = mbedtls_ct_mpi_sign_if(do_swap, s, Y->s); mbedtls_mpi_core_cond_swap(X->p, Y->p, X->n, do_swap); @@ -1690,7 +1703,7 @@ int mbedtls_mpi_exp_mod(mbedtls_mpi *X, const mbedtls_mpi *A, */ if (A->s == -1 && (E->p[0] & 1) != 0) { mbedtls_ct_condition_t is_x_non_zero = mbedtls_mpi_core_check_zero_ct(X->p, X->n); - X->s = mbedtls_ct_uint_if(is_x_non_zero, -1, 1); + X->s = mbedtls_ct_mpi_sign_if(is_x_non_zero, -1, 1); MBEDTLS_MPI_CHK(mbedtls_mpi_add_mpi(X, N, X)); } From 16ef486c2ce8e28a9f39ed3b713472b2d0bd3a39 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Fri, 8 Mar 2024 17:25:57 +0000 Subject: [PATCH 22/26] Improve style Signed-off-by: Janos Follath --- library/bignum.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index 78f6149a68..22e22aa076 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1694,8 +1694,7 @@ 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(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); /* From b888bc0be67bb605fe938ad787b3b621ab6129ba Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 11 Mar 2024 09:29:53 +0000 Subject: [PATCH 23/26] Fix typo Signed-off-by: Janos Follath --- library/bignum.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index 22e22aa076..eecdf1e99c 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -44,10 +44,10 @@ * (MPI sign is the field s in mbedtls_mpi. It is unsigned short and only 1 and -1 are valid * values.) */ -static inline unsigned short mbedtls_ct_mpi_sign_if(mbedtls_ct_condition_t cond, - unsigned short sign1, unsigned short sign2) +static inline signed short mbedtls_ct_mpi_sign_if(mbedtls_ct_condition_t cond, + signed short sign1, signed short sign2) { - return (unsigned short) mbedtls_ct_uint_if(cond, sign1 + 1, sign2 + 1) - 1; + return (signed short) mbedtls_ct_uint_if(cond, sign1 + 1, sign2 + 1) - 1; } /* From d6df0a5dacc0dd1cef11b5232eb546124bfe9f18 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 11 Mar 2024 09:40:03 +0000 Subject: [PATCH 24/26] Fix use of volatile We need the pointer, A, to be volatile, to ensure the reads happen. bits does not need to be volatile. Signed-off-by: Janos Follath --- library/bignum_core.c | 4 ++-- library/bignum_core.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/library/bignum_core.c b/library/bignum_core.c index 8374f3ae60..db6e231703 100644 --- a/library/bignum_core.c +++ b/library/bignum_core.c @@ -856,10 +856,10 @@ mbedtls_mpi_uint mbedtls_mpi_core_sub_int(mbedtls_mpi_uint *X, return c; } -mbedtls_ct_condition_t mbedtls_mpi_core_check_zero_ct(const mbedtls_mpi_uint *A, +mbedtls_ct_condition_t mbedtls_mpi_core_check_zero_ct(volatile const mbedtls_mpi_uint *A, size_t limbs) { - volatile mbedtls_mpi_uint bits = 0; + mbedtls_mpi_uint bits = 0; for (size_t i = 0; i < limbs; i++) { bits |= A[i]; diff --git a/library/bignum_core.h b/library/bignum_core.h index 92c8d47db5..00c557b816 100644 --- a/library/bignum_core.h +++ b/library/bignum_core.h @@ -665,7 +665,7 @@ mbedtls_mpi_uint mbedtls_mpi_core_sub_int(mbedtls_mpi_uint *X, * \return MBEDTLS_CT_FALSE if `A == 0` * MBEDTLS_CT_TRUE if `A != 0`. */ -mbedtls_ct_condition_t mbedtls_mpi_core_check_zero_ct(const mbedtls_mpi_uint *A, +mbedtls_ct_condition_t mbedtls_mpi_core_check_zero_ct(volatile const mbedtls_mpi_uint *A, size_t limbs); /** From adb9d2d8221755ea7d3a2fc75ad918dd23639161 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 11 Mar 2024 10:03:05 +0000 Subject: [PATCH 25/26] Remove volatile from declaration Use of volatile is more an internal implementation detail (ensuring const-time) than part of the contract (the caller doesn't care about volatile as such). Signed-off-by: Janos Follath --- library/bignum_core.c | 5 +++-- library/bignum_core.h | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/library/bignum_core.c b/library/bignum_core.c index db6e231703..54b519509b 100644 --- a/library/bignum_core.c +++ b/library/bignum_core.c @@ -856,13 +856,14 @@ mbedtls_mpi_uint mbedtls_mpi_core_sub_int(mbedtls_mpi_uint *X, return c; } -mbedtls_ct_condition_t mbedtls_mpi_core_check_zero_ct(volatile const mbedtls_mpi_uint *A, +mbedtls_ct_condition_t mbedtls_mpi_core_check_zero_ct(const mbedtls_mpi_uint *A, size_t limbs) { + volatile const mbedtls_mpi_uint* force_read_A = A; mbedtls_mpi_uint bits = 0; for (size_t i = 0; i < limbs; i++) { - bits |= A[i]; + bits |= force_read_A[i]; } return mbedtls_ct_bool(bits); diff --git a/library/bignum_core.h b/library/bignum_core.h index 00c557b816..92c8d47db5 100644 --- a/library/bignum_core.h +++ b/library/bignum_core.h @@ -665,7 +665,7 @@ mbedtls_mpi_uint mbedtls_mpi_core_sub_int(mbedtls_mpi_uint *X, * \return MBEDTLS_CT_FALSE if `A == 0` * MBEDTLS_CT_TRUE if `A != 0`. */ -mbedtls_ct_condition_t mbedtls_mpi_core_check_zero_ct(volatile const mbedtls_mpi_uint *A, +mbedtls_ct_condition_t mbedtls_mpi_core_check_zero_ct(const mbedtls_mpi_uint *A, size_t limbs); /** From 23dc8b5fd8ec57b293dc510e22d124a58fa11b1b Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 11 Mar 2024 10:39:57 +0000 Subject: [PATCH 26/26] Fix code style Co-authored-by: Dave Rodgman 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 54b519509b..1a3e0b9b6f 100644 --- a/library/bignum_core.c +++ b/library/bignum_core.c @@ -859,7 +859,7 @@ mbedtls_mpi_uint mbedtls_mpi_core_sub_int(mbedtls_mpi_uint *X, mbedtls_ct_condition_t mbedtls_mpi_core_check_zero_ct(const mbedtls_mpi_uint *A, size_t limbs) { - volatile const mbedtls_mpi_uint* force_read_A = A; + volatile const mbedtls_mpi_uint *force_read_A = A; mbedtls_mpi_uint bits = 0; for (size_t i = 0; i < limbs; i++) {