Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[3.6] Reduce performance regression in RSA public operations #9281

Merged
merged 21 commits into from
Aug 22, 2024
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 34 additions & 1 deletion include/mbedtls/bignum.h
Original file line number Diff line number Diff line change
Expand Up @@ -880,7 +880,40 @@ 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
*
* \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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry but I realized I made a mistake in a previous approval:

Since it's not in the public API, we can look for a better way after 3.6.1.

But as of 878af12 we are adding mbedtls_mpi_exp_mod_unsafe to the public API of the 3.6 LTS branch. I'm ok with the prototype but I don't like the name. Also I don't like to rush when extending the API of an LTS branch.

Can we make this an internal function instead? It's only meant to be used in rsa.c after all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you don't like the name we need to have a larger discussion because we already have a few functions called _unsafe (as Janos pointed out already), so if we make a change it should apply to all functions.

But I fully agree about making the new function internal, especially considering it's an LTS and we don't have time to have the naming discussion.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we already have a few functions called _unsafe

Not in the public API. I don't care nearly as much about internal functions.

$ git show -s --oneline
28cdd11908 (HEAD, upstream-restricted/test-mbedtls-3.6.1rc, upstream-public/mbedtls-3.6) Merge pull request #9479 from gilles-peskine-arm/psa-keystore-static-release-update-3.6
$ git grep _unsafe include

We do have one internal function called xxx_unsafe (mbedtls_mpi_core_get_mont_r2_unsafe) and while I'm ok (but not enthusiastic) about the name, I'm bothered that its documentation doesn't explain in what way it's unsafe.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, the other function is not public, so that's an additional reason to keep the new one private.

(I also agree that naming is less important for internal functions, but we can still have a discussion about it some time. I can see arguments for calling them _leaky rather than _unsafe as it's more informative. Also, for the small static functions that are currently called optionally_safe, I think slow_or_leaky would make anyone think before using them. But let's have this discussion another time.)

const mbedtls_mpi *E, const mbedtls_mpi *N,
mbedtls_mpi *prec_RR);

/**
* \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.
Expand Down
30 changes: 26 additions & 4 deletions library/bignum.c
Original file line number Diff line number Diff line change
Expand Up @@ -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(mbedtls_mpi *X, const mbedtls_mpi *A,
const mbedtls_mpi *E, const mbedtls_mpi *N,
mbedtls_mpi *prec_RR)
/*
* 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, int E_public,
const mbedtls_mpi *N, mbedtls_mpi *prec_RR)
{
int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED;

Expand Down Expand Up @@ -1695,7 +1699,11 @@ 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);
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);
}

Expand All @@ -1720,6 +1728,20 @@ int mbedtls_mpi_exp_mod(mbedtls_mpi *X, const mbedtls_mpi *A,
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, 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, MBEDTLS_MPI_IS_PUBLIC, N, prec_RR);
}

/*
* Greatest common divisor: G = gcd(A, B) (HAC 14.54)
*/
Expand Down
163 changes: 144 additions & 19 deletions library/bignum_core.c
Original file line number Diff line number Diff line change
Expand Up @@ -746,7 +746,93 @@ static void exp_mod_precompute_window(const mbedtls_mpi_uint *A,
}
}

#if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C)
// Set to a default that is neither MBEDTLS_MPI_IS_PUBLIC nor MBEDTLS_MPI_IS_SECRET
int mbedtls_mpi_optionally_safe_codepath = MBEDTLS_MPI_IS_PUBLIC + MBEDTLS_MPI_IS_SECRET + 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this. A + B + 1 is not a good way to get a number that's neither A nor B - what if A = 0 and B = -1? I know that's not true here, but this expression has a dependency on the values of A and B (and we don't have a static assert that this value = neither of them).

How would you feel about a third value specified in the #defines, of something like MBEDTLS_IS_NETHER_FOR_TESTING (name is horrible, but you get the idea)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm unresolving this as we've reverted past the commit that addressed this

Copy link
Contributor

@yanesca yanesca Aug 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've already added this commit to the followup PR locally, I just haven't pushed it, because don't want to trigger the CI.

#endif

/*
* 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) {
/*
* Skip leading zero bits.
*/
size_t E_bits = mbedtls_mpi_core_bitlen(E, E_limbs);
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
} 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;
#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;
}
#endif
}
}

/*
* 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);
#if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C)
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) && !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;
}
#endif
}
}

/* 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.
*
Expand All @@ -758,16 +844,25 @@ 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)
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,
int E_public,
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;
size_t E_bit_index;
Comment on lines +860 to +861
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocker: I don't like relying on auxiliary functions to initialize variables. Unfortunately compilers aren't good at analyzing whether variables are analyzed on all code paths, and we don't run Coverity assiduously enough to notice all problems. I would strongly prefer to initialize to the safe-path values here, and have exp_mod_calc_first_bit_optionally_safe modify the values on the unsafe path.

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
Expand All @@ -786,7 +881,7 @@ void mbedtls_mpi_core_exp_mod(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);
Expand All @@ -798,11 +893,6 @@ void mbedtls_mpi_core_exp_mod(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;
/* At any given time, window contains window_bits bits from E.
* window_bits can go up to wsize. */
size_t window_bits = 0;
Expand All @@ -828,10 +918,9 @@ 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);

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);
Expand All @@ -841,6 +930,42 @@ 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,
MBEDTLS_MPI_IS_SECRET,
RR,
T);
}

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,
MBEDTLS_MPI_IS_PUBLIC,
RR,
T);
}

mbedtls_mpi_uint mbedtls_mpi_core_sub_int(mbedtls_mpi_uint *X,
const mbedtls_mpi_uint *A,
mbedtls_mpi_uint c, /* doubles as carry */
Expand Down
70 changes: 70 additions & 0 deletions library/bignum_core.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 0x2a2a2a2a
#define MBEDTLS_MPI_IS_SECRET 0

/** Count leading zero bits in a given integer.
*
* \warning The result is undefined if \p a == 0
Expand Down Expand Up @@ -604,6 +625,42 @@ 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.
*
* \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.
*
* \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.
*/
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:
* X = A^E mod N, where \p A is already in Montgomery form.
Expand Down Expand Up @@ -760,4 +817,17 @@ void mbedtls_mpi_core_from_mont_rep(mbedtls_mpi_uint *X,
mbedtls_mpi_uint mm,
mbedtls_mpi_uint *T);

/*
* Can't define thread local variables with our abstraction layer: do nothing if threading is on.
*/
#if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C)
extern int mbedtls_mpi_optionally_safe_codepath;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mbedtls_mpi_optionally_safe_codepath gives me hives. It's not at all obvious that turning on MBEDTLS_TEST_HOOKS doesn't change the functional behavior of the code: I had to carefully check the places where mbedtls_mpi_optionally_safe_codepath is used and verify that when the library reads it, it only influences how the library might modify it and nothing else.

mbedtls_mpi_optionally_safe_codepath should be documented at the place where it's declared. But the documentation belongs in the test code... The solution to this conundrum is that the variable should be defined in test code. The library code should call a tracing function through a function pointer (doing nothing if the function pointer is null).

This would normally be a blocker. I'm prepared to waive it under time pressure for the 3.6.1 release if we schedule a high-priority follow-up with the assurance that it will not get forgotten on the big tech debt heap.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could declare that the 3.6.1 EPIC can't be closed until that follow-up is done, so that we have to do the follow-up before the end of the quarter.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reopened the followup to address this: #9493


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;
}
#endif

#endif /* MBEDTLS_BIGNUM_CORE_H */
2 changes: 1 addition & 1 deletion library/rsa.c
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
Loading