kris/boringssl
1
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

Remove the non-no_branch branch of BN_div.

Browse Source 
Simplify the code, and in particular make |BN_div|, |BN_mod|, and
|BN_nnmod| insensitive to |BN_FLG_CONSTTIME|. This improves the
effectiveness of testing by reducing the number of branches that are
likely to go untested or less tested.

There is no performance-sensitive code that uses BN_div but doesn't
already use BN_FLG_CONSTTIME except RSA signature verification and
EC_GROUP creation. RSA signature verification, ECDH, and ECDSA
performance aren't significantly different with this change.

Change-Id: Ie34c4ce925b939150529400cc60e1f414c7676cd
Reviewed-on: https://boringssl-review.googlesource.com/9105
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This commit is contained in:
Brian Smith 2016-08-02 18:21:18 -10:00 committed by David Benjamin
parent 4cb8494d25
commit 9f05de4ebb
5 changed files with 76 additions and 169 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

61
crypto/bn/bn_test.cc
View File

@ -1173,42 +1173,35 @@ static bool TestNegativeZero(BN_CTX *ctx) {
return false;
}
for (int consttime = 0; consttime < 2; consttime++) {
bssl::UniquePtr<BIGNUM> numerator(BN_new()), denominator(BN_new());
if (!numerator || !denominator) {
return false;
}
bssl::UniquePtr<BIGNUM> 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.

75
crypto/bn/div.c
View File

@ -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;

15
crypto/bn/gcd.c
View File

@ -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;
}

88
crypto/rsa/rsa_impl.c
View File

@ -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;
}

6
include/openssl/bn.h
View File

@ -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