From 06fa67c8d3b2e68df45b34b315dd26193f45645a Mon Sep 17 00:00:00 2001 From: Adam Langley Date: Tue, 8 Sep 2015 16:31:33 -0700 Subject: [PATCH] Stop using |ERR_peek_last_error| in RSA blinding. History has shown there are bugs in not setting the error code appropriately, which makes any decision making based on |ERR_peek_last_error|, etc. suspect. Also, this call was interfering with the link-time optimizer's ability to discard the implementations of many functions in crypto/err during dead code elimination. Change-Id: Iba9e553bf0a72a1370ceb17ff275f5a20fca31ec Reviewed-on: https://boringssl-review.googlesource.com/5748 Reviewed-by: Adam Langley --- crypto/bn/gcd.c | 28 +++++++++++++++++++++------- crypto/rsa/blinding.c | 7 ++++--- include/openssl/bn.h | 8 ++++++++ 3 files changed, 33 insertions(+), 10 deletions(-) diff --git a/crypto/bn/gcd.c b/crypto/bn/gcd.c index c33a3cd5..e1061498 100644 --- a/crypto/bn/gcd.c +++ b/crypto/bn/gcd.c @@ -223,20 +223,23 @@ err: } /* solves ax == 1 (mod n) */ -static BIGNUM *BN_mod_inverse_no_branch(BIGNUM *out, const BIGNUM *a, - const BIGNUM *n, BN_CTX *ctx); +static BIGNUM *BN_mod_inverse_no_branch(BIGNUM *out, int *out_no_inverse, + const BIGNUM *a, const BIGNUM *n, + BN_CTX *ctx); -BIGNUM *BN_mod_inverse(BIGNUM *out, const BIGNUM *a, const BIGNUM *n, - BN_CTX *ctx) { +BIGNUM *BN_mod_inverse_ex(BIGNUM *out, int *out_no_inverse, const BIGNUM *a, + const BIGNUM *n, BN_CTX *ctx) { BIGNUM *A, *B, *X, *Y, *M, *D, *T, *R = NULL; BIGNUM *ret = NULL; int sign; if ((a->flags & BN_FLG_CONSTTIME) != 0 || (n->flags & BN_FLG_CONSTTIME) != 0) { - return BN_mod_inverse_no_branch(out, a, n, ctx); + return BN_mod_inverse_no_branch(out, out_no_inverse, a, n, ctx); } + *out_no_inverse = 0; + BN_CTX_start(ctx); A = BN_CTX_get(ctx); B = BN_CTX_get(ctx); @@ -522,6 +525,7 @@ BIGNUM *BN_mod_inverse(BIGNUM *out, const BIGNUM *a, const BIGNUM *n, } } } else { + *out_no_inverse = 1; OPENSSL_PUT_ERROR(BN, BN_R_NO_INVERSE); goto err; } @@ -535,16 +539,25 @@ err: return ret; } +BIGNUM *BN_mod_inverse(BIGNUM *out, const BIGNUM *a, const BIGNUM *n, + BN_CTX *ctx) { + int no_inverse; + return BN_mod_inverse_ex(out, &no_inverse, a, n, ctx); +} + /* BN_mod_inverse_no_branch is a special version of BN_mod_inverse. * It does not contain branches that may leak sensitive information. */ -static BIGNUM *BN_mod_inverse_no_branch(BIGNUM *out, const BIGNUM *a, - const BIGNUM *n, BN_CTX *ctx) { +static BIGNUM *BN_mod_inverse_no_branch(BIGNUM *out, int *out_no_inverse, + const BIGNUM *a, const BIGNUM *n, + BN_CTX *ctx) { BIGNUM *A, *B, *X, *Y, *M, *D, *T, *R = NULL; BIGNUM local_A, local_B; BIGNUM *pA, *pB; BIGNUM *ret = NULL; int sign; + *out_no_inverse = 0; + BN_CTX_start(ctx); A = BN_CTX_get(ctx); B = BN_CTX_get(ctx); @@ -682,6 +695,7 @@ static BIGNUM *BN_mod_inverse_no_branch(BIGNUM *out, const BIGNUM *a, } } } else { + *out_no_inverse = 1; OPENSSL_PUT_ERROR(BN, BN_R_NO_INVERSE); goto err; } diff --git a/crypto/rsa/blinding.c b/crypto/rsa/blinding.c index 75c34225..c93cee1b 100644 --- a/crypto/rsa/blinding.c +++ b/crypto/rsa/blinding.c @@ -325,10 +325,11 @@ BN_BLINDING *BN_BLINDING_create_param( if (!BN_rand_range(ret->A, ret->mod)) { goto err; } - if (BN_mod_inverse(ret->Ai, ret->A, ret->mod, ctx) == NULL) { + + int no_inverse; + if (BN_mod_inverse_ex(ret->Ai, &no_inverse, ret->A, ret->mod, ctx) == NULL) { /* this should almost never happen for good RSA keys */ - uint32_t error = ERR_peek_last_error(); - if (ERR_GET_REASON(error) == BN_R_NO_INVERSE) { + if (no_inverse) { if (retry_counter-- == 0) { OPENSSL_PUT_ERROR(RSA, RSA_R_TOO_MANY_ITERATIONS); goto err; diff --git a/include/openssl/bn.h b/include/openssl/bn.h index 7edb7784..0a1f5e5f 100644 --- a/include/openssl/bn.h +++ b/include/openssl/bn.h @@ -706,6 +706,14 @@ OPENSSL_EXPORT int BN_gcd(BIGNUM *r, const BIGNUM *a, const BIGNUM *b, OPENSSL_EXPORT BIGNUM *BN_mod_inverse(BIGNUM *out, const BIGNUM *a, const BIGNUM *n, BN_CTX *ctx); +/* BN_mod_inverse_ex acts like |BN_mod_inverse| except that, when it returns + * zero, it will set |*out_no_inverse| to one if the failure was caused because + * |a| has no inverse mod |n|. Otherwise it will set |*out_no_inverse| to + * zero. */ +OPENSSL_EXPORT BIGNUM *BN_mod_inverse_ex(BIGNUM *out, int *out_no_inverse, + const BIGNUM *a, const BIGNUM *n, + BN_CTX *ctx); + /* BN_kronecker returns the Kronecker symbol of |a| and |b| (which is -1, 0 or * 1), or -2 on error. */ OPENSSL_EXPORT int BN_kronecker(const BIGNUM *a, const BIGNUM *b, BN_CTX *ctx);