From df806b606a8a519d44baa9983bc5f4376623d120 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Mon, 15 Jan 2024 19:28:39 -0500 Subject: [PATCH] Clear some more false positives from constant-time validation Mostly bits of DSA and RSA keygen, flagged when we make the PRNG output secret by default. There's still a ton of RSA to resolve, mostly because our constant-time bignum strategy does not interact well with valgrind when handling RSA's secret-value / public-bit-length situation. Also RSA's ASN.1 serialization is unavoidably leaky. Bug: 676 Change-Id: I08d273959065c4db6fd44180a6ac56a82f862fe8 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65447 Auto-Submit: David Benjamin Reviewed-by: Bob Beck Commit-Queue: David Benjamin (cherry picked from commit fce5cf02378a839174935b83b58f54aba6c2bb3e) --- crypto/dsa/dsa.c | 31 ++++++++++++++++++++++--------- crypto/dsa/dsa_asn1.c | 6 ++++-- crypto/fipsmodule/bn/gcd_extra.c | 5 ++++- crypto/fipsmodule/bn/prime.c | 19 +++++++++++++------ crypto/fipsmodule/ec/ec_key.c | 5 ++++- crypto/fipsmodule/ec/scalar.c | 6 +++++- crypto/fipsmodule/rsa/rsa.c | 9 ++++++--- crypto/fipsmodule/rsa/rsa_impl.c | 21 +++++++++++++++------ 8 files changed, 73 insertions(+), 29 deletions(-) diff --git a/crypto/dsa/dsa.c b/crypto/dsa/dsa.c index 893a595f88..16db9b32a9 100644 --- a/crypto/dsa/dsa.c +++ b/crypto/dsa/dsa.c @@ -304,6 +304,8 @@ int dsa_internal_paramgen(DSA *dsa, size_t bits, const EVP_MD *evpmd, if (!RAND_bytes(seed, qsize)) { goto err; } + // DSA parameters are public. + CONSTTIME_DECLASSIFY(seed, qsize); } else { // If we come back through, use random seed next time. seed_in = NULL; @@ -544,6 +546,9 @@ int DSA_generate_key(DSA *dsa) { goto err; } + // The public key is computed from the private key, but is public. + bn_declassify(pub_key); + dsa->priv_key = priv_key; dsa->pub_key = pub_key; ok = 1; @@ -680,6 +685,10 @@ DSA_SIG *DSA_do_sign(const uint8_t *digest, size_t digest_len, const DSA *dsa) { goto err; } + // The signature is computed from the private key, but is public. + bn_declassify(r); + bn_declassify(s); + // Redo if r or s is zero as required by FIPS 186-3: this is // very unlikely. if (BN_is_zero(r) || BN_is_zero(s)) { @@ -931,15 +940,19 @@ static int dsa_sign_setup(const DSA *dsa, BN_CTX *ctx, BIGNUM **out_kinv, ctx) || // Compute r = (g^k mod p) mod q !BN_mod_exp_mont_consttime(r, dsa->g, &k, dsa->p, ctx, - dsa->method_mont_p) || - // Note |BN_mod| below is not constant-time and may leak information about - // |r|. |dsa->p| may be significantly larger than |dsa->q|, so this is not - // easily performed in constant-time with Montgomery reduction. - // - // However, |r| at this point is g^k (mod p). It is almost the value of - // |r| revealed in the signature anyway (g^k (mod p) (mod q)), going from - // it to |k| would require computing a discrete log. - !BN_mod(r, r, dsa->q, ctx) || + dsa->method_mont_p)) { + OPENSSL_PUT_ERROR(DSA, ERR_R_BN_LIB); + goto err; + } + // Note |BN_mod| below is not constant-time and may leak information about + // |r|. |dsa->p| may be significantly larger than |dsa->q|, so this is not + // easily performed in constant-time with Montgomery reduction. + // + // However, |r| at this point is g^k (mod p). It is almost the value of |r| + // revealed in the signature anyway (g^k (mod p) (mod q)), going from it to + // |k| would require computing a discrete log. + bn_declassify(r); + if (!BN_mod(r, r, dsa->q, ctx) || // Compute part of 's = inv(k) (m + xr) mod q' using Fermat's Little // Theorem. !bn_mod_inverse_prime(kinv, &k, dsa->q, ctx, dsa->method_mont_q)) { diff --git a/crypto/dsa/dsa_asn1.c b/crypto/dsa/dsa_asn1.c index 1985bb4f6f..0e9dd56c2d 100644 --- a/crypto/dsa/dsa_asn1.c +++ b/crypto/dsa/dsa_asn1.c @@ -63,6 +63,7 @@ #include "internal.h" #include "../bytestring/internal.h" +#include "../crypto/internal.h" #define OPENSSL_DSA_MAX_MODULUS_BITS 10000 @@ -119,8 +120,9 @@ int dsa_check_key(const DSA *dsa) { if (dsa->priv_key != NULL) { // The private key is a non-zero element of the scalar field, determined by // |q|. - if (BN_is_negative(dsa->priv_key) || BN_is_zero(dsa->priv_key) || - BN_cmp(dsa->priv_key, dsa->q) >= 0) { + if (BN_is_negative(dsa->priv_key) || + constant_time_declassify_int(BN_is_zero(dsa->priv_key)) || + constant_time_declassify_int(BN_cmp(dsa->priv_key, dsa->q) >= 0)) { OPENSSL_PUT_ERROR(DSA, DSA_R_INVALID_PARAMETERS); return 0; } diff --git a/crypto/fipsmodule/bn/gcd_extra.c b/crypto/fipsmodule/bn/gcd_extra.c index b4fe00e3e4..76f337cbca 100644 --- a/crypto/fipsmodule/bn/gcd_extra.c +++ b/crypto/fipsmodule/bn/gcd_extra.c @@ -314,7 +314,10 @@ int bn_mod_inverse_consttime(BIGNUM *r, int *out_no_inverse, const BIGNUM *a, } assert(BN_is_zero(v)); - if (!BN_is_one(u)) { + // While the inputs and output are secret, this function considers whether the + // input was invertible to be public. It is used as part of RSA key + // generation, where inputs are chosen to already be invertible. + if (constant_time_declassify_int(!BN_is_one(u))) { *out_no_inverse = 1; OPENSSL_PUT_ERROR(BN, BN_R_NO_INVERSE); goto err; diff --git a/crypto/fipsmodule/bn/prime.c b/crypto/fipsmodule/bn/prime.c index 99839e4414..b31456470b 100644 --- a/crypto/fipsmodule/bn/prime.c +++ b/crypto/fipsmodule/bn/prime.c @@ -502,7 +502,10 @@ int BN_generate_prime_ex(BIGNUM *ret, int bits, int safe, const BIGNUM *add, static int bn_trial_division(uint16_t *out, const BIGNUM *bn) { const size_t num_primes = num_trial_division_primes(bn); for (size_t i = 1; i < num_primes; i++) { - if (bn_mod_u16_consttime(bn, kPrimes[i]) == 0) { + // During RSA key generation, |bn| may be secret, but only if |bn| was + // prime, so it is safe to leak failed trial divisions. + if (constant_time_declassify_int(bn_mod_u16_consttime(bn, kPrimes[i]) == + 0)) { *out = kPrimes[i]; return 1; } @@ -588,7 +591,8 @@ int bn_miller_rabin_iteration(const BN_MILLER_RABIN *miller_rabin, // To avoid leaking |a|, we run the loop to |w_bits| and mask off all // iterations once |j| = |a|. for (int j = 1; j < miller_rabin->w_bits; j++) { - if (constant_time_eq_int(j, miller_rabin->a) & ~is_possibly_prime) { + if (constant_time_declassify_w(constant_time_eq_int(j, miller_rabin->a) & + ~is_possibly_prime)) { // If the loop is done and we haven't seen z = 1 or z = w-1 yet, the // value is composite and we can break in variable time. break; @@ -608,12 +612,14 @@ int bn_miller_rabin_iteration(const BN_MILLER_RABIN *miller_rabin, // Step 4.5.3. If z = 1 and the loop is not done, the previous value of z // was not -1. There are no non-trivial square roots of 1 modulo a prime, so // w is composite and we may exit in variable time. - if (BN_equal_consttime(z, miller_rabin->one_mont) & ~is_possibly_prime) { + if (constant_time_declassify_w( + BN_equal_consttime(z, miller_rabin->one_mont) & + ~is_possibly_prime)) { break; } } - *out_is_possibly_prime = is_possibly_prime & 1; + *out_is_possibly_prime = constant_time_declassify_w(is_possibly_prime) & 1; ret = 1; err: @@ -751,8 +757,9 @@ int BN_primality_test(int *out_is_probably_prime, const BIGNUM *w, int checks, crypto_word_t uniform_iterations = 0; // Using |constant_time_lt_w| seems to prevent the compiler from optimizing // this into two jumps. - for (int i = 1; (i <= BN_PRIME_CHECKS_BLINDED) | - constant_time_lt_w(uniform_iterations, checks); + for (int i = 1; constant_time_declassify_w( + (i <= BN_PRIME_CHECKS_BLINDED) | + constant_time_lt_w(uniform_iterations, checks)); i++) { // Step 4.1-4.2 int is_uniform; diff --git a/crypto/fipsmodule/ec/ec_key.c b/crypto/fipsmodule/ec/ec_key.c index c98620f62f..ccc3221029 100644 --- a/crypto/fipsmodule/ec/ec_key.c +++ b/crypto/fipsmodule/ec/ec_key.c @@ -238,7 +238,10 @@ int EC_KEY_set_private_key(EC_KEY *key, const BIGNUM *priv_key) { return 0; } if (!ec_bignum_to_scalar(key->group, &scalar->scalar, priv_key) || - ec_scalar_is_zero(key->group, &scalar->scalar)) { + // Zero is not a valid private key, so it is safe to leak the result of + // this comparison. + constant_time_declassify_int( + ec_scalar_is_zero(key->group, &scalar->scalar))) { OPENSSL_PUT_ERROR(EC, EC_R_INVALID_PRIVATE_KEY); ec_wrapped_scalar_free(scalar); return 0; diff --git a/crypto/fipsmodule/ec/scalar.c b/crypto/fipsmodule/ec/scalar.c index a86ee0fbbc..b05d50845d 100644 --- a/crypto/fipsmodule/ec/scalar.c +++ b/crypto/fipsmodule/ec/scalar.c @@ -23,8 +23,12 @@ int ec_bignum_to_scalar(const EC_GROUP *group, EC_SCALAR *out, const BIGNUM *in) { + // Scalars, which are often secret, must be reduced modulo the order. Those + // that are not will be discarded, so leaking the result of the comparison is + // safe. if (!bn_copy_words(out->words, group->order.N.width, in) || - !bn_less_than_words(out->words, group->order.N.d, group->order.N.width)) { + !constant_time_declassify_int(bn_less_than_words( + out->words, group->order.N.d, group->order.N.width))) { OPENSSL_PUT_ERROR(EC, EC_R_INVALID_SCALAR); return 0; } diff --git a/crypto/fipsmodule/rsa/rsa.c b/crypto/fipsmodule/rsa/rsa.c index 9372c5da5d..02ad388440 100644 --- a/crypto/fipsmodule/rsa/rsa.c +++ b/crypto/fipsmodule/rsa/rsa.c @@ -1319,8 +1319,10 @@ int RSA_check_key(const RSA *key) { // n was bound by |is_public_component_of_rsa_key_good|. This also implicitly // checks p and q are odd, which is a necessary condition for Montgomery // reduction. - if (BN_is_negative(key->p) || BN_cmp(key->p, key->n) >= 0 || - BN_is_negative(key->q) || BN_cmp(key->q, key->n) >= 0) { + if (BN_is_negative(key->p) || + constant_time_declassify_int(BN_cmp(key->p, key->n) >= 0) || + BN_is_negative(key->q) || + constant_time_declassify_int(BN_cmp(key->q, key->n) >= 0)) { OPENSSL_PUT_ERROR(RSA, RSA_R_BAD_RSA_PARAMETERS); goto out; } @@ -1351,7 +1353,8 @@ int RSA_check_key(const RSA *key) { goto out; } - if (!BN_is_one(&tmp) || !BN_is_one(&de)) { + if (constant_time_declassify_int(!BN_is_one(&tmp)) || + constant_time_declassify_int(!BN_is_one(&de))) { OPENSSL_PUT_ERROR(RSA, RSA_R_D_E_NOT_CONGRUENT_TO_1); goto out; } diff --git a/crypto/fipsmodule/rsa/rsa_impl.c b/crypto/fipsmodule/rsa/rsa_impl.c index 8db1acac93..4839bad482 100644 --- a/crypto/fipsmodule/rsa/rsa_impl.c +++ b/crypto/fipsmodule/rsa/rsa_impl.c @@ -945,20 +945,25 @@ static int generate_prime(BIGNUM *out, int bits, const BIGNUM *e, // retrying. That is, we reject a negligible fraction of primes that are // within the FIPS bound, but we will never accept a prime outside the // bound, ensuring the resulting RSA key is the right size. - if (BN_cmp(out, sqrt2) <= 0) { + // + // Values over the threshold are discarded, so it is safe to leak this + // comparison. + if (constant_time_declassify_int(BN_cmp(out, sqrt2) <= 0)) { continue; } // RSA key generation's bottleneck is discarding composites. If it fails // trial division, do not bother computing a GCD or performing Miller-Rabin. if (!bn_odd_number_is_obviously_composite(out)) { - // Check gcd(out-1, e) is one (steps 4.5 and 5.6). + // Check gcd(out-1, e) is one (steps 4.5 and 5.6). Leaking the final + // result of this comparison is safe because, if not relatively prime, the + // value will be discarded. int relatively_prime; - if (!BN_sub(tmp, out, BN_value_one()) || + if (!bn_usub_consttime(tmp, out, BN_value_one()) || !bn_is_relatively_prime(&relatively_prime, tmp, e, ctx)) { goto err; } - if (relatively_prime) { + if (constant_time_declassify_int(relatively_prime)) { // Test |out| for primality (steps 4.5.1 and 5.6.1). int is_probable_prime; if (!BN_primality_test(&is_probable_prime, out, @@ -1116,8 +1121,9 @@ static int rsa_generate_key_impl(RSA *rsa, int bits, const BIGNUM *e_value, } // Retry if |rsa->d| <= 2^|prime_bits|. See appendix B.3.1's guidance on - // values for d. - } while (BN_cmp(rsa->d, pow2_prime_bits) <= 0); + // values for d. When we retry, p and q are discarded, so it is safe to leak + // this comparison. + } while (constant_time_declassify_int(BN_cmp(rsa->d, pow2_prime_bits) <= 0)); assert(BN_num_bits(pm1) == (unsigned)prime_bits); assert(BN_num_bits(qm1) == (unsigned)prime_bits); @@ -1131,6 +1137,9 @@ static int rsa_generate_key_impl(RSA *rsa, int bits, const BIGNUM *e_value, } bn_set_minimal_width(rsa->n); + // |rsa->n| is computed from the private key, but is public. + bn_declassify(rsa->n); + // Sanity-check that |rsa->n| has the specified size. This is implied by // |generate_prime|'s bounds. if (BN_num_bits(rsa->n) != (unsigned)bits) {