Remove BN_FLG_CONSTTIME.

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 <[email protected]>

Author: davidben

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

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

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;

crypto/bn/gcd.c

@@ -399,10 +399,6 @@ int BN_mod_inverse_odd(BIGNUM *out, int *out_no_inverse, const BIGNUM *a,
 
 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;
     }

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).
    *

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

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;

crypto/rsa/rsa_impl.c

@@ -769,8 +769,6 @@ static int mod_exp(BIGNUM *r0, const BIGNUM *I, RSA *rsa, BN_CTX *ctx) {
 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;
   }
 

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)

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
 	}