From 0a211dfe91588d2986a8735e1969dd9202a8b025 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sat, 17 Dec 2016 15:25:55 -0500 Subject: [PATCH] Remove BN_FLG_CONSTTIME. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BN_FLG_CONSTTIME is a ridiculous API and easy to mess up (CVE-2016-2178). Instead, code that needs a particular algorithm which preserves secrecy of some arguemnt should call into that algorithm directly. This is never set outside the library and is finally unused within the library! Credit for all this goes almost entirely to Brian Smith. I just took care of the last bits. Note there was one BN_FLG_CONSTTIME check that was still reachable, the BN_mod_inverse in RSA key generation. However, it used the same code in both cases for even moduli and φ(n) is even if n is not a power of two. Traditionally, RSA keys are not powers of two, even though it would make the modular reductions a lot easier. When reviewing, check that I didn't remove a BN_FLG_CONSTTIME that led to a BN_mod_exp(_mont) or BN_mod_inverse call (with the exception of the RSA one mentioned above). They should all go to functions for the algorithms themselves like BN_mod_exp_mont_consttime. This CL shows the checks are a no-op for all our tests: https://boringssl-review.googlesource.com/c/12927/ BUG=125 Change-Id: I19cbb375cc75aac202bd76b51ca098841d84f337 Reviewed-on: https://boringssl-review.googlesource.com/12926 Reviewed-by: Adam Langley --- crypto/bn/bn.c | 14 ------------ crypto/bn/bn_test.cc | 9 -------- crypto/bn/exponentiation.c | 16 -------------- crypto/bn/gcd.c | 12 +++------- crypto/bn/montgomery.c | 3 --- crypto/dh/dh.c | 10 +++------ crypto/dsa/dsa.c | 9 +------- crypto/rsa/rsa_impl.c | 11 +++------- include/openssl/bn.h | 45 +++++++++++++------------------------- util/doc.go | 2 +- 10 files changed, 26 insertions(+), 105 deletions(-) diff --git a/crypto/bn/bn.c b/crypto/bn/bn.c index 31bb937f..e3c55f28 100644 --- a/crypto/bn/bn.c +++ b/crypto/bn/bn.c @@ -172,12 +172,6 @@ const BIGNUM *BN_value_one(void) { return &kOne; } -void BN_with_flags(BIGNUM *out, const BIGNUM *in, int flags) { - OPENSSL_memcpy(out, in, sizeof(BIGNUM)); - out->flags &= ~BN_FLG_MALLOCED; - out->flags |= BN_FLG_STATIC_DATA | flags; -} - /* BN_num_bits_word returns the minimum number of bits needed to represent the * value in |l|. */ unsigned BN_num_bits_word(BN_ULONG l) { @@ -369,11 +363,3 @@ void bn_correct_top(BIGNUM *bn) { bn->neg = 0; } } - -int BN_get_flags(const BIGNUM *bn, int flags) { - return bn->flags & flags; -} - -void BN_set_flags(BIGNUM *bn, int flags) { - bn->flags |= flags; -} diff --git a/crypto/bn/bn_test.cc b/crypto/bn/bn_test.cc index 4f544a7f..8f93ad0d 100644 --- a/crypto/bn/bn_test.cc +++ b/crypto/bn/bn_test.cc @@ -632,15 +632,6 @@ static bool TestModInv(FileTest *t, BN_CTX *ctx) { return false; } - BN_set_flags(a.get(), BN_FLG_CONSTTIME); - - if (!ret || - !BN_mod_inverse(ret.get(), a.get(), m.get(), ctx) || - !ExpectBIGNUMsEqual(t, "inv(A) (mod M) (constant-time)", mod_inv.get(), - ret.get())) { - return false; - } - return true; } diff --git a/crypto/bn/exponentiation.c b/crypto/bn/exponentiation.c index 3161a2a8..933a731c 100644 --- a/crypto/bn/exponentiation.c +++ b/crypto/bn/exponentiation.c @@ -140,12 +140,6 @@ int BN_exp(BIGNUM *r, const BIGNUM *a, const BIGNUM *p, BN_CTX *ctx) { int i, bits, ret = 0; BIGNUM *v, *rr; - if ((p->flags & BN_FLG_CONSTTIME) != 0) { - /* BN_FLG_CONSTTIME only supported by BN_mod_exp_mont() */ - OPENSSL_PUT_ERROR(BN, ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED); - return 0; - } - BN_CTX_start(ctx); if (r == a || r == p) { rr = BN_CTX_get(ctx); @@ -437,12 +431,6 @@ static int mod_exp_recp(BIGNUM *r, const BIGNUM *a, const BIGNUM *p, BIGNUM *val[TABLE_SIZE]; BN_RECP_CTX recp; - if (BN_get_flags(p, BN_FLG_CONSTTIME) != 0) { - /* BN_FLG_CONSTTIME only supported by BN_mod_exp_mont() */ - OPENSSL_PUT_ERROR(BN, ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED); - return 0; - } - bits = BN_num_bits(p); if (bits == 0) { @@ -593,10 +581,6 @@ int BN_mod_exp_mont(BIGNUM *rr, const BIGNUM *a, const BIGNUM *p, BIGNUM *val[TABLE_SIZE]; BN_MONT_CTX *new_mont = NULL; - if (BN_get_flags(p, BN_FLG_CONSTTIME) != 0) { - return BN_mod_exp_mont_consttime(rr, a, p, m, ctx, mont); - } - if (!BN_is_odd(m)) { OPENSSL_PUT_ERROR(BN, BN_R_CALLED_WITH_EVEN_MODULUS); return 0; diff --git a/crypto/bn/gcd.c b/crypto/bn/gcd.c index 9e62da00..7c20b8e2 100644 --- a/crypto/bn/gcd.c +++ b/crypto/bn/gcd.c @@ -399,10 +399,6 @@ err: BIGNUM *BN_mod_inverse(BIGNUM *out, const BIGNUM *a, const BIGNUM *n, BN_CTX *ctx) { - int no_inverse; - - BIGNUM *a_reduced = NULL; - BIGNUM *new_out = NULL; if (out == NULL) { new_out = BN_new(); @@ -414,10 +410,7 @@ BIGNUM *BN_mod_inverse(BIGNUM *out, const BIGNUM *a, const BIGNUM *n, } int ok = 0; - - int no_branch = - (a->flags & BN_FLG_CONSTTIME) != 0 || (n->flags & BN_FLG_CONSTTIME) != 0; - + BIGNUM *a_reduced = NULL; if (a->neg || BN_ucmp(a, n) >= 0) { a_reduced = BN_dup(a); if (a_reduced == NULL) { @@ -429,7 +422,8 @@ BIGNUM *BN_mod_inverse(BIGNUM *out, const BIGNUM *a, const BIGNUM *n, a = a_reduced; } - if (no_branch || !BN_is_odd(n)) { + int no_inverse; + if (!BN_is_odd(n)) { if (!bn_mod_inverse_general(out, &no_inverse, a, n, ctx)) { goto err; } diff --git a/crypto/bn/montgomery.c b/crypto/bn/montgomery.c index 70f0585c..aa5bc424 100644 --- a/crypto/bn/montgomery.c +++ b/crypto/bn/montgomery.c @@ -187,9 +187,6 @@ int BN_MONT_CTX_set(BN_MONT_CTX *mont, const BIGNUM *mod, BN_CTX *ctx) { OPENSSL_PUT_ERROR(BN, ERR_R_INTERNAL_ERROR); return 0; } - if (BN_get_flags(mod, BN_FLG_CONSTTIME)) { - BN_set_flags(&mont->N, BN_FLG_CONSTTIME); - } /* Find n0 such that n0 * N == -1 (mod r). * diff --git a/crypto/dh/dh.c b/crypto/dh/dh.c index 69a7ec81..33c36f31 100644 --- a/crypto/dh/dh.c +++ b/crypto/dh/dh.c @@ -258,7 +258,6 @@ int DH_generate_key(DH *dh) { int generate_new_key = 0; BN_CTX *ctx = NULL; BIGNUM *pub_key = NULL, *priv_key = NULL; - BIGNUM local_priv; if (BN_num_bits(dh->p) > OPENSSL_DH_MAX_MODULUS_BITS) { OPENSSL_PUT_ERROR(DH, DH_R_MODULUS_TOO_LARGE); @@ -317,8 +316,7 @@ int DH_generate_key(DH *dh) { } } - BN_with_flags(&local_priv, priv_key, BN_FLG_CONSTTIME); - if (!BN_mod_exp_mont_consttime(pub_key, dh->g, &local_priv, dh->p, ctx, + if (!BN_mod_exp_mont_consttime(pub_key, dh->g, priv_key, dh->p, ctx, dh->method_mont_p)) { goto err; } @@ -347,7 +345,6 @@ int DH_compute_key(unsigned char *out, const BIGNUM *peers_key, DH *dh) { BIGNUM *shared_key; int ret = -1; int check_result; - BIGNUM local_priv; if (BN_num_bits(dh->p) > OPENSSL_DH_MAX_MODULUS_BITS) { OPENSSL_PUT_ERROR(DH, DH_R_MODULUS_TOO_LARGE); @@ -379,9 +376,8 @@ int DH_compute_key(unsigned char *out, const BIGNUM *peers_key, DH *dh) { goto err; } - BN_with_flags(&local_priv, dh->priv_key, BN_FLG_CONSTTIME); - if (!BN_mod_exp_mont_consttime(shared_key, peers_key, &local_priv, dh->p, ctx, - dh->method_mont_p)) { + if (!BN_mod_exp_mont_consttime(shared_key, peers_key, dh->priv_key, dh->p, + ctx, dh->method_mont_p)) { OPENSSL_PUT_ERROR(DH, ERR_R_BN_LIB); goto err; } diff --git a/crypto/dsa/dsa.c b/crypto/dsa/dsa.c index 15583be0..e2b6695e 100644 --- a/crypto/dsa/dsa.c +++ b/crypto/dsa/dsa.c @@ -434,7 +434,6 @@ int DSA_generate_key(DSA *dsa) { int ok = 0; BN_CTX *ctx = NULL; BIGNUM *pub_key = NULL, *priv_key = NULL; - BIGNUM prk; ctx = BN_CTX_new(); if (ctx == NULL) { @@ -461,12 +460,9 @@ int DSA_generate_key(DSA *dsa) { } } - BN_init(&prk); - BN_with_flags(&prk, priv_key, BN_FLG_CONSTTIME); - if (!BN_MONT_CTX_set_locked(&dsa->method_mont_p, &dsa->method_mont_lock, dsa->p, ctx) || - !BN_mod_exp_mont_consttime(pub_key, dsa->g, &prk, dsa->p, ctx, + !BN_mod_exp_mont_consttime(pub_key, dsa->g, priv_key, dsa->p, ctx, dsa->method_mont_p)) { goto err; } @@ -844,8 +840,6 @@ int DSA_sign_setup(const DSA *dsa, BN_CTX *ctx_in, BIGNUM **out_kinv, goto err; } - BN_set_flags(&k, BN_FLG_CONSTTIME); - if (!BN_MONT_CTX_set_locked((BN_MONT_CTX **)&dsa->method_mont_p, (CRYPTO_MUTEX *)&dsa->method_mont_lock, dsa->p, ctx) || @@ -873,7 +867,6 @@ int DSA_sign_setup(const DSA *dsa, BN_CTX *ctx_in, BIGNUM **out_kinv, goto err; } - BN_set_flags(&kq, BN_FLG_CONSTTIME); if (!BN_mod_exp_mont_consttime(r, dsa->g, &kq, dsa->p, ctx, dsa->method_mont_p)) { goto err; diff --git a/crypto/rsa/rsa_impl.c b/crypto/rsa/rsa_impl.c index 3834be5a..8e0aa9c6 100644 --- a/crypto/rsa/rsa_impl.c +++ b/crypto/rsa/rsa_impl.c @@ -769,8 +769,6 @@ 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_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; @@ -999,9 +997,7 @@ int rsa_default_multi_prime_keygen(RSA *rsa, int bits, int num_primes, goto err; } } - pr0 = &local_r0; - BN_with_flags(pr0, r0, BN_FLG_CONSTTIME); - if (!BN_mod_inverse(rsa->d, rsa->e, pr0, ctx)) { + if (!BN_mod_inverse(rsa->d, rsa->e, r0, ctx)) { goto err; /* d */ } @@ -1019,10 +1015,9 @@ int rsa_default_multi_prime_keygen(RSA *rsa, int bits, int num_primes, * from constant-time, |bn_mod_inverse_secret_prime| uses the same modular * exponentation logic as in RSA private key operations and, if the RSAZ-1024 * code is enabled, will be optimized for common RSA prime sizes. */ - p = &local_p; - BN_with_flags(p, rsa->p, BN_FLG_CONSTTIME); if (!BN_MONT_CTX_set_locked(&rsa->mont_p, &rsa->lock, rsa->p, ctx) || - !bn_mod_inverse_secret_prime(rsa->iqmp, rsa->q, p, ctx, rsa->mont_p)) { + !bn_mod_inverse_secret_prime(rsa->iqmp, rsa->q, rsa->p, ctx, + rsa->mont_p)) { goto err; } diff --git a/include/openssl/bn.h b/include/openssl/bn.h index 18beba4e..b34ebe36 100644 --- a/include/openssl/bn.h +++ b/include/openssl/bn.h @@ -194,13 +194,6 @@ OPENSSL_EXPORT void BN_clear(BIGNUM *bn); /* BN_value_one returns a static BIGNUM with value 1. */ OPENSSL_EXPORT const BIGNUM *BN_value_one(void); -/* BN_with_flags initialises a stack allocated |BIGNUM| with pointers to the - * contents of |in| but with |flags| ORed into the flags field. - * - * Note: the two BIGNUMs share state and so |out| should /not/ be passed to - * |BN_free|. */ -OPENSSL_EXPORT void BN_with_flags(BIGNUM *out, const BIGNUM *in, int flags); - /* Basic functions. */ @@ -233,12 +226,6 @@ OPENSSL_EXPORT void BN_set_negative(BIGNUM *bn, int sign); /* BN_is_negative returns one if |bn| is negative and zero otherwise. */ OPENSSL_EXPORT int BN_is_negative(const BIGNUM *bn); -/* BN_get_flags returns |bn->flags| & |flags|. */ -OPENSSL_EXPORT int BN_get_flags(const BIGNUM *bn, int flags); - -/* BN_set_flags sets |flags| on |bn|. */ -OPENSSL_EXPORT void BN_set_flags(BIGNUM *bn, int flags); - /* Conversion functions. */ @@ -762,11 +749,10 @@ OPENSSL_EXPORT int BN_gcd(BIGNUM *r, const BIGNUM *a, const BIGNUM *b, /* BN_mod_inverse sets |out| equal to |a|^-1, mod |n|. If |out| is NULL, a * fresh BIGNUM is allocated. It returns the result or NULL on error. * - * If either of |a| or |n| have |BN_FLG_CONSTTIME| set then the operation is - * performed using an algorithm that avoids some branches but which isn't - * constant-time. This function shouldn't be used for secret values, even - * with |BN_FLG_CONSTTIME|; use |BN_mod_inverse_blinded| instead. Or, if - * |n| is guaranteed to be prime, use + * If |n| is even then the operation is performed using an algorithm that avoids + * some branches but which isn't constant-time. This function shouldn't be used + * for secret values; use |BN_mod_inverse_blinded| instead. Or, if |n| is + * guaranteed to be prime, use * |BN_mod_exp_mont_consttime(out, a, m_minus_2, m, ctx, m_mont)|, taking * advantage of Fermat's Little Theorem. */ OPENSSL_EXPORT BIGNUM *BN_mod_inverse(BIGNUM *out, const BIGNUM *a, @@ -775,11 +761,9 @@ OPENSSL_EXPORT BIGNUM *BN_mod_inverse(BIGNUM *out, const BIGNUM *a, /* BN_mod_inverse_blinded sets |out| equal to |a|^-1, mod |n|, where |n| is the * Montgomery modulus for |mont|. |a| must be non-negative and must be less * than |n|. |n| must be greater than 1. |a| is blinded (masked by a random - * value) to protect it against side-channel attacks. |BN_mod_inverse_blinded| - * may or may not ignore the |BN_FLG_CONSTTIME| flag on any/all of its inputs. - * It returns one on success or zero on failure. On failure, if the failure was - * caused by |a| having no inverse mod |n| then |*out_no_inverse| will be set - * to one; otherwise it will be set to zero. */ + * value) to protect it against side-channel attacks. On failure, if the failure + * was caused by |a| having no inverse mod |n| then |*out_no_inverse| will be + * set to one; otherwise it will be set to zero. */ int BN_mod_inverse_blinded(BIGNUM *out, int *out_no_inverse, const BIGNUM *a, const BN_MONT_CTX *mont, BN_CTX *ctx); @@ -860,9 +844,9 @@ OPENSSL_EXPORT int BN_exp(BIGNUM *r, const BIGNUM *a, const BIGNUM *p, BN_CTX *ctx); /* BN_mod_exp sets |r| equal to |a|^{|p|} mod |m|. It does so with the best - * algorithm for the values provided and can run in constant time if - * |BN_FLG_CONSTTIME| is set for |p|. It returns one on success or zero - * otherwise. */ + * algorithm for the values provided. It returns one on success or zero + * otherwise. The |BN_mod_exp_mont_consttime| variant must be used if the + * exponent is secret. */ OPENSSL_EXPORT int BN_mod_exp(BIGNUM *r, const BIGNUM *a, const BIGNUM *p, const BIGNUM *m, BN_CTX *ctx); @@ -930,10 +914,11 @@ 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| and |BN_mod_inverse| will call - * |BN_mod_inverse_no_branch|. */ -#define BN_FLG_CONSTTIME 0x04 +/* |BN_FLG_CONSTTIME| has been removed and intentionally omitted so code relying + * on it will not compile. Consumers outside BoringSSL should use the + * higher-level cryptographic algorithms exposed by other modules. Consumers + * within the library should call the appropriate timing-sensitive algorithm + * directly. */ #if defined(__cplusplus) diff --git a/util/doc.go b/util/doc.go index 681b8349..987794c9 100644 --- a/util/doc.go +++ b/util/doc.go @@ -139,7 +139,7 @@ func extractComment(lines []string, lineNo int) (comment []string, rest []string } func extractDecl(lines []string, lineNo int) (decl string, rest []string, restLineNo int, err error) { - if len(lines) == 0 { + if len(lines) == 0 || len(lines[0]) == 0 { return "", lines, lineNo, nil }