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 <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
(cherry picked from commit fce5cf02378a839174935b83b58f54aba6c2bb3e)

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)) {

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

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;

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;

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;

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

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

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) {