diff --git a/crypto/bn/bn_test.cc b/crypto/bn/bn_test.cc index 044af5fe..75ef17aa 100644 --- a/crypto/bn/bn_test.cc +++ b/crypto/bn/bn_test.cc @@ -1173,42 +1173,35 @@ static bool TestNegativeZero(BN_CTX *ctx) { return false; } - for (int consttime = 0; consttime < 2; consttime++) { - bssl::UniquePtr numerator(BN_new()), denominator(BN_new()); - if (!numerator || !denominator) { - return false; - } + bssl::UniquePtr numerator(BN_new()), denominator(BN_new()); + if (!numerator || !denominator) { + return false; + } - if (consttime) { - BN_set_flags(numerator.get(), BN_FLG_CONSTTIME); - BN_set_flags(denominator.get(), BN_FLG_CONSTTIME); - } + // Test that BN_div never gives negative zero in the quotient. + if (!BN_set_word(numerator.get(), 1) || + !BN_set_word(denominator.get(), 2)) { + return false; + } + BN_set_negative(numerator.get(), 1); + if (!BN_div(a.get(), b.get(), numerator.get(), denominator.get(), ctx)) { + return false; + } + if (!BN_is_zero(a.get()) || BN_is_negative(a.get())) { + fprintf(stderr, "Incorrect quotient.\n"); + return false; + } - // Test that BN_div never gives negative zero in the quotient. - if (!BN_set_word(numerator.get(), 1) || - !BN_set_word(denominator.get(), 2)) { - return false; - } - BN_set_negative(numerator.get(), 1); - if (!BN_div(a.get(), b.get(), numerator.get(), denominator.get(), ctx)) { - return false; - } - if (!BN_is_zero(a.get()) || BN_is_negative(a.get())) { - fprintf(stderr, "Incorrect quotient (consttime = %d).\n", consttime); - return false; - } - - // Test that BN_div never gives negative zero in the remainder. - if (!BN_set_word(denominator.get(), 1)) { - return false; - } - if (!BN_div(a.get(), b.get(), numerator.get(), denominator.get(), ctx)) { - return false; - } - if (!BN_is_zero(b.get()) || BN_is_negative(b.get())) { - fprintf(stderr, "Incorrect remainder (consttime = %d).\n", consttime); - return false; - } + // Test that BN_div never gives negative zero in the remainder. + if (!BN_set_word(denominator.get(), 1)) { + return false; + } + if (!BN_div(a.get(), b.get(), numerator.get(), denominator.get(), ctx)) { + return false; + } + if (!BN_is_zero(b.get()) || BN_is_negative(b.get())) { + fprintf(stderr, "Incorrect remainder.\n"); + return false; } // Test that BN_set_negative will not produce a negative zero. diff --git a/crypto/bn/div.c b/crypto/bn/div.c index ab492812..6e3df7d3 100644 --- a/crypto/bn/div.c +++ b/crypto/bn/div.c @@ -182,7 +182,12 @@ static inline void bn_div_rem_words(BN_ULONG *quotient_out, BN_ULONG *rem_out, * Thus: * dv->neg == num->neg ^ divisor->neg (unless the result is zero) * rm->neg == num->neg (unless the remainder is zero) - * If 'dv' or 'rm' is NULL, the respective value is not returned. */ + * If 'dv' or 'rm' is NULL, the respective value is not returned. + * + * This was specifically designed to contain fewer branches that may leak + * sensitive information; see "New Branch Prediction Vulnerabilities in OpenSSL + * and Necessary Software Countermeasures" by Onur Acıçmez, Shay Gueron, and + * Jean-Pierre Seifert. */ int BN_div(BIGNUM *dv, BIGNUM *rm, const BIGNUM *num, const BIGNUM *divisor, BN_CTX *ctx) { int norm_shift, i, loop; @@ -190,7 +195,6 @@ int BN_div(BIGNUM *dv, BIGNUM *rm, const BIGNUM *num, const BIGNUM *divisor, BN_ULONG *resp, *wnump; BN_ULONG d0, d1; int num_n, div_n; - int no_branch = 0; /* Invalid zero-padding would have particularly bad consequences * so don't just rely on bn_check_top() here */ @@ -200,28 +204,11 @@ int BN_div(BIGNUM *dv, BIGNUM *rm, const BIGNUM *num, const BIGNUM *divisor, return 0; } - if ((num->flags & BN_FLG_CONSTTIME) != 0 || - (divisor->flags & BN_FLG_CONSTTIME) != 0) { - no_branch = 1; - } - if (BN_is_zero(divisor)) { OPENSSL_PUT_ERROR(BN, BN_R_DIV_BY_ZERO); return 0; } - if (!no_branch && BN_ucmp(num, divisor) < 0) { - if (rm != NULL) { - if (BN_copy(rm, num) == NULL) { - return 0; - } - } - if (dv != NULL) { - BN_zero(dv); - } - return 1; - } - BN_CTX_start(ctx); tmp = BN_CTX_get(ctx); snum = BN_CTX_get(ctx); @@ -247,26 +234,23 @@ int BN_div(BIGNUM *dv, BIGNUM *rm, const BIGNUM *num, const BIGNUM *divisor, } snum->neg = 0; - if (no_branch) { - /* Since we don't know whether snum is larger than sdiv, - * we pad snum with enough zeroes without changing its - * value. - */ - if (snum->top <= sdiv->top + 1) { - if (bn_wexpand(snum, sdiv->top + 2) == NULL) { - goto err; - } - for (i = snum->top; i < sdiv->top + 2; i++) { - snum->d[i] = 0; - } - snum->top = sdiv->top + 2; - } else { - if (bn_wexpand(snum, snum->top + 1) == NULL) { - goto err; - } - snum->d[snum->top] = 0; - snum->top++; + /* Since we don't want to have special-case logic for the case where snum is + * larger than sdiv, we pad snum with enough zeroes without changing its + * value. */ + if (snum->top <= sdiv->top + 1) { + if (bn_wexpand(snum, sdiv->top + 2) == NULL) { + goto err; } + for (i = snum->top; i < sdiv->top + 2; i++) { + snum->d[i] = 0; + } + snum->top = sdiv->top + 2; + } else { + if (bn_wexpand(snum, snum->top + 1) == NULL) { + goto err; + } + snum->d[snum->top] = 0; + snum->top++; } div_n = sdiv->top; @@ -294,7 +278,7 @@ int BN_div(BIGNUM *dv, BIGNUM *rm, const BIGNUM *num, const BIGNUM *divisor, if (!bn_wexpand(res, (loop + 1))) { goto err; } - res->top = loop - no_branch; + res->top = loop - 1; resp = &(res->d[loop - 1]); /* space for temp */ @@ -302,15 +286,6 @@ int BN_div(BIGNUM *dv, BIGNUM *rm, const BIGNUM *num, const BIGNUM *divisor, goto err; } - if (!no_branch) { - if (BN_ucmp(&wnum, sdiv) >= 0) { - bn_sub_words(wnum.d, wnum.d, sdiv->d, div_n); - *resp = 1; - } else { - res->top--; - } - } - /* if res->top == 0 then clear the neg value otherwise decrease * the resp pointer */ if (res->top == 0) { @@ -401,9 +376,7 @@ int BN_div(BIGNUM *dv, BIGNUM *rm, const BIGNUM *num, const BIGNUM *divisor, rm->neg = neg; } } - if (no_branch) { - bn_correct_top(res); - } + bn_correct_top(res); BN_CTX_end(ctx); return 1; diff --git a/crypto/bn/gcd.c b/crypto/bn/gcd.c index a1ed5d91..72809634 100644 --- a/crypto/bn/gcd.c +++ b/crypto/bn/gcd.c @@ -423,9 +423,6 @@ BIGNUM *BN_mod_inverse(BIGNUM *out, const BIGNUM *a, const BIGNUM *n, if (a_reduced == NULL) { goto err; } - if (no_branch) { - BN_set_flags(a_reduced, BN_FLG_CONSTTIME); - } if (!BN_nnmod(a_reduced, a_reduced, n, ctx)) { goto err; } @@ -481,15 +478,13 @@ err: /* bn_mod_inverse_general is the general inversion algorithm that works for * both even and odd |n|. It was specifically designed to contain fewer - * branches that may leak sensitive information. See "New Branch Prediction + * branches that may leak sensitive information; see "New Branch Prediction * Vulnerabilities in OpenSSL and Necessary Software Countermeasures" by * Onur Acıçmez, Shay Gueron, and Jean-Pierre Seifert. */ static int bn_mod_inverse_general(BIGNUM *out, int *out_no_inverse, const BIGNUM *a, const BIGNUM *n, BN_CTX *ctx) { BIGNUM *A, *B, *X, *Y, *M, *D, *T; - BIGNUM local_A; - BIGNUM *pA; int ret = 0; int sign; @@ -532,14 +527,8 @@ static int bn_mod_inverse_general(BIGNUM *out, int *out_no_inverse, * sign*Y*a == A (mod |n|) */ - /* Turn BN_FLG_CONSTTIME flag on, so that when BN_div is invoked, - * BN_div_no_branch will be called eventually. - */ - pA = &local_A; - BN_with_flags(pA, A, BN_FLG_CONSTTIME); - /* (D, M) := (A/B, A%B) ... */ - if (!BN_div(D, M, pA, B, ctx)) { + if (!BN_div(D, M, A, B, ctx)) { goto err; } diff --git a/crypto/rsa/rsa_impl.c b/crypto/rsa/rsa_impl.c index 1ee0d82e..70c3f7a5 100644 --- a/crypto/rsa/rsa_impl.c +++ b/crypto/rsa/rsa_impl.c @@ -590,17 +590,9 @@ int rsa_default_private_transform(RSA *rsa, uint8_t *out, const uint8_t *in, if (!mod_exp(result, f, rsa, ctx)) { goto err; } - } else { - BIGNUM local_d; - BIGNUM *d = NULL; - - BN_init(&local_d); - d = &local_d; - BN_with_flags(d, rsa->d, BN_FLG_CONSTTIME); - - if (!BN_mod_exp_mont_consttime(result, f, d, rsa->n, ctx, rsa->mont_n)) { - goto err; - } + } else if (!BN_mod_exp_mont_consttime(result, f, rsa->d, rsa->n, ctx, + rsa->mont_n)) { + goto err; } /* Verify the result to protect against fault attacks as described in the @@ -658,8 +650,6 @@ static int mod_exp(BIGNUM *r0, const BIGNUM *I, RSA *rsa, BN_CTX *ctx) { assert(rsa->iqmp != NULL); BIGNUM *r1, *m1, *vrfy; - BIGNUM local_dmp1, local_dmq1, local_c, local_r1; - BIGNUM *dmp1, *dmq1, *c, *pr1; int ret = 0; size_t i, num_additional_primes = 0; @@ -677,23 +667,9 @@ static int mod_exp(BIGNUM *r0, const BIGNUM *I, RSA *rsa, BN_CTX *ctx) { goto err; } - { - BIGNUM local_p, local_q; - BIGNUM *p = NULL, *q = NULL; - - /* Make sure BN_mod in Montgomery initialization uses BN_FLG_CONSTTIME. */ - BN_init(&local_p); - p = &local_p; - BN_with_flags(p, rsa->p, BN_FLG_CONSTTIME); - - BN_init(&local_q); - q = &local_q; - BN_with_flags(q, rsa->q, BN_FLG_CONSTTIME); - - if (!BN_MONT_CTX_set_locked(&rsa->mont_p, &rsa->lock, p, ctx) || - !BN_MONT_CTX_set_locked(&rsa->mont_q, &rsa->lock, q, ctx)) { - goto err; - } + if (!BN_MONT_CTX_set_locked(&rsa->mont_p, &rsa->lock, rsa->p, ctx) || + !BN_MONT_CTX_set_locked(&rsa->mont_q, &rsa->lock, rsa->q, ctx)) { + goto err; } if (!BN_MONT_CTX_set_locked(&rsa->mont_n, &rsa->lock, rsa->n, ctx)) { @@ -701,30 +677,22 @@ static int mod_exp(BIGNUM *r0, const BIGNUM *I, RSA *rsa, BN_CTX *ctx) { } /* compute I mod q */ - c = &local_c; - BN_with_flags(c, I, BN_FLG_CONSTTIME); - if (!BN_mod(r1, c, rsa->q, ctx)) { + if (!BN_mod(r1, I, rsa->q, ctx)) { goto err; } /* compute r1^dmq1 mod q */ - dmq1 = &local_dmq1; - BN_with_flags(dmq1, rsa->dmq1, BN_FLG_CONSTTIME); - if (!BN_mod_exp_mont_consttime(m1, r1, dmq1, rsa->q, ctx, rsa->mont_q)) { + if (!BN_mod_exp_mont_consttime(m1, r1, rsa->dmq1, rsa->q, ctx, rsa->mont_q)) { goto err; } /* compute I mod p */ - c = &local_c; - BN_with_flags(c, I, BN_FLG_CONSTTIME); - if (!BN_mod(r1, c, rsa->p, ctx)) { + if (!BN_mod(r1, I, rsa->p, ctx)) { goto err; } /* compute r1^dmp1 mod p */ - dmp1 = &local_dmp1; - BN_with_flags(dmp1, rsa->dmp1, BN_FLG_CONSTTIME); - if (!BN_mod_exp_mont_consttime(r0, r1, dmp1, rsa->p, ctx, rsa->mont_p)) { + if (!BN_mod_exp_mont_consttime(r0, r1, rsa->dmp1, rsa->p, ctx, rsa->mont_p)) { goto err; } @@ -743,11 +711,7 @@ static int mod_exp(BIGNUM *r0, const BIGNUM *I, RSA *rsa, BN_CTX *ctx) { goto err; } - /* Turn BN_FLG_CONSTTIME flag on before division operation */ - pr1 = &local_r1; - BN_with_flags(pr1, r1, BN_FLG_CONSTTIME); - - if (!BN_mod(r0, pr1, rsa->p, ctx)) { + if (!BN_mod(r0, r1, rsa->p, ctx)) { goto err; } @@ -771,30 +735,23 @@ static int mod_exp(BIGNUM *r0, const BIGNUM *I, RSA *rsa, BN_CTX *ctx) { for (i = 0; i < num_additional_primes; i++) { /* multi-prime RSA. */ - BIGNUM local_exp, local_prime; - BIGNUM *exp = &local_exp, *prime = &local_prime; RSA_additional_prime *ap = sk_RSA_additional_prime_value(rsa->additional_primes, i); - BN_with_flags(exp, ap->exp, BN_FLG_CONSTTIME); - BN_with_flags(prime, ap->prime, BN_FLG_CONSTTIME); - /* c will already point to a BIGNUM with the correct flags. */ - if (!BN_mod(r1, c, prime, ctx)) { + if (!BN_mod(r1, I, ap->prime, ctx)) { goto err; } - if (!BN_MONT_CTX_set_locked(&ap->mont, &rsa->lock, prime, ctx) || - !BN_mod_exp_mont_consttime(m1, r1, exp, prime, ctx, ap->mont)) { + if (!BN_MONT_CTX_set_locked(&ap->mont, &rsa->lock, ap->prime, ctx) || + !BN_mod_exp_mont_consttime(m1, r1, ap->exp, ap->prime, ctx, ap->mont)) { goto err; } - BN_set_flags(m1, BN_FLG_CONSTTIME); - if (!BN_sub(m1, m1, r0) || !BN_mul(m1, m1, ap->coeff, ctx) || - !BN_mod(m1, m1, prime, ctx) || - (BN_is_negative(m1) && !BN_add(m1, m1, prime)) || + !BN_mod(m1, m1, ap->prime, ctx) || + (BN_is_negative(m1) && !BN_add(m1, m1, ap->prime)) || !BN_mul(m1, m1, ap->r, ctx) || !BN_add(r0, r0, m1)) { goto err; @@ -811,8 +768,8 @@ err: int rsa_default_multi_prime_keygen(RSA *rsa, int bits, int num_primes, BIGNUM *e_value, BN_GENCB *cb) { BIGNUM *r0 = NULL, *r1 = NULL, *r2 = NULL, *r3 = NULL, *tmp; - BIGNUM local_r0, local_d, local_p; - BIGNUM *pr0, *d, *p; + BIGNUM local_r0, local_p; + BIGNUM *pr0, *p; int prime_bits, ok = -1, n = 0, i, j; BN_CTX *ctx = NULL; STACK_OF(RSA_additional_prime) *additional_primes = NULL; @@ -1047,24 +1004,19 @@ int rsa_default_multi_prime_keygen(RSA *rsa, int bits, int num_primes, goto err; /* d */ } - /* set up d for correct BN_FLG_CONSTTIME flag */ - d = &local_d; - BN_with_flags(d, rsa->d, BN_FLG_CONSTTIME); - /* calculate d mod (p-1) */ - if (!BN_mod(rsa->dmp1, d, r1, ctx)) { + if (!BN_mod(rsa->dmp1, rsa->d, r1, ctx)) { goto err; } /* calculate d mod (q-1) */ - if (!BN_mod(rsa->dmq1, d, r2, ctx)) { + if (!BN_mod(rsa->dmq1, rsa->d, r2, ctx)) { goto err; } /* calculate inverse of q mod p */ p = &local_p; BN_with_flags(p, rsa->p, BN_FLG_CONSTTIME); - if (!BN_mod_inverse(rsa->iqmp, rsa->q, p, ctx)) { goto err; } diff --git a/include/openssl/bn.h b/include/openssl/bn.h index 8deb278f..c98e0bd6 100644 --- a/include/openssl/bn.h +++ b/include/openssl/bn.h @@ -913,9 +913,9 @@ OPENSSL_EXPORT unsigned BN_num_bits_word(BN_ULONG l); #define BN_FLG_MALLOCED 0x01 #define BN_FLG_STATIC_DATA 0x02 -/* avoid leaking exponent information through timing, BN_mod_exp_mont() will - * call BN_mod_exp_mont_consttime, BN_div() will call BN_div_no_branch, - * BN_mod_inverse() will call BN_mod_inverse_no_branch. */ +/* Avoid leaking exponent information through timing. |BN_mod_exp_mont| will + * call |BN_mod_exp_mont_consttime| and |BN_mod_inverse| will call + * |BN_mod_inverse_no_branch|. */ #define BN_FLG_CONSTTIME 0x04