Start making asserts constant-time too

We've historically settled on treating asserts as not in scope for our
constant-time goals. Production binaries are expected to be optimized
builds, with debug assertions turned off. (We have a handful of
assertions in perf-sensitive code that you definitely do not want to run
with.) Secret data has invariants too, so it is useful to be able to
write debug assertions on them.

However, combined with our default CMake build being a debug build, this
seems to cause some confusion with researchers sometimes. Also, if we
ever get language-level constant-time support, we would need to resolve
this mismatch anyway. (I assume any language support would put enough
into the type system to force us to declassify any intentional branches
on secret-by-data-flow bools, notably those we assert on.) So I'm
inclined to just make our asserts constant-time.

There are two issues around asserts, at least with our valgrind-based
validation:

The first is that a couple of asserts over secret data compute their
condition leakily. We can just fix these. The only such ones I found
were in bn_reduce_once and bn_gcd_consttime.

The second is that almost every assert over secret data will be flagged
as an invalid branch by valgrind. However, presuming the condition
itself was computed in constant time, this branch is actually safe. If
we were willing to abort the process when false, the assert is clearly
publicly true. We just need to declassify the boolean to assert on it.

assert(constant_time_declassify_int(expr)) is really long, so I made an
internal wrapper macro declassify_assert(expr). Not sure if that's the
best name. constant_time_declassify_assert(expr) is kinda long.
constant_time_assert(expr) fits with the rest of that namespace, but
reads as if we're somehow running an assert without branching, when the
whole point is that we *are* branching and need to explicitly say it's
okay to.

Fixed: 339
Change-Id: Ie33b99bf9a269b11d2c48d246cc4934be7e239ff
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65467
Reviewed-by: Bob Beck <[email protected]>
Commit-Queue: David Benjamin <[email protected]>
(cherry picked from commit 9b8b483276da2b3d36ea21e97743e310314a8de0)

CMakeLists.txt

@@ -773,8 +773,6 @@ endif()
 
 if(CONSTANT_TIME_VALIDATION)
   add_definitions(-DBORINGSSL_CONSTANT_TIME_VALIDATION)
-  # Asserts will often test secret data.
-  add_definitions(-DNDEBUG)
 endif()
 
 # CMake's iOS support uses Apple's multiple-architecture toolchain. It takes an

crypto/cipher_extra/tls_cbc.c

@@ -123,8 +123,8 @@ void EVP_tls_cbc_copy_mac(uint8_t *out, size_t md_size, const uint8_t *in,
   size_t mac_end = in_len;
   size_t mac_start = mac_end - md_size;
 
-  assert(orig_len >= in_len);
-  assert(in_len >= md_size);
+  declassify_assert(orig_len >= in_len);
+  declassify_assert(in_len >= md_size);
   assert(md_size <= EVP_MAX_MD_SIZE);
   assert(md_size > 0);
 

crypto/fipsmodule/bn/bytes.c

@@ -246,7 +246,7 @@ void bn_assert_fits_in_bytes(const BIGNUM *bn, size_t num) {
 void bn_words_to_big_endian(uint8_t *out, size_t out_len, const BN_ULONG *in,
                             size_t in_len) {
   // The caller should have selected an output length without truncation.
-  assert(fits_in_bytes(in, in_len, out_len));
+  declassify_assert(fits_in_bytes(in, in_len, out_len));
   size_t num_bytes = in_len * sizeof(BN_ULONG);
   if (out_len < num_bytes) {
     num_bytes = out_len;

crypto/fipsmodule/bn/div.c

@@ -425,7 +425,7 @@ BN_ULONG bn_reduce_once(BN_ULONG *r, const BN_ULONG *a, BN_ULONG carry,
   //
   // Although |carry| may be one if it was one on input and |bn_sub_words|
   // returns zero, this would give |r| > |m|, violating our input assumptions.
-  assert(carry == 0 || carry == (BN_ULONG)-1);
+  declassify_assert(carry + 1 <= 1);
   bn_select_words(r, carry, a /* r < 0 */, r /* r >= 0 */, num);
   return carry;
 }
@@ -434,7 +434,7 @@ BN_ULONG bn_reduce_once_in_place(BN_ULONG *r, BN_ULONG carry, const BN_ULONG *m,
                                  BN_ULONG *tmp, size_t num) {
   // See |bn_reduce_once| for why this logic works.
   carry -= bn_sub_words(tmp, r, m, num);
-  assert(carry == 0 || carry == (BN_ULONG)-1);
+  declassify_assert(carry + 1 <= 1);
   bn_select_words(r, carry, r /* tmp < 0 */, tmp /* tmp >= 0 */, num);
   return carry;
 }
@@ -504,7 +504,7 @@ int bn_div_consttime(BIGNUM *quotient, BIGNUM *remainder,
   // |divisor_min_bits| bits, the top |divisor_min_bits - 1| can be incorporated
   // without reductions. This significantly speeds up |RSA_check_key|. For
   // simplicity, we round down to a whole number of words.
-  assert(divisor_min_bits <= BN_num_bits(divisor));
+  declassify_assert(divisor_min_bits <= BN_num_bits(divisor));
   int initial_words = 0;
   if (divisor_min_bits > 0) {
     initial_words = (divisor_min_bits - 1) / BN_BITS2;

crypto/fipsmodule/bn/div_extra.c

@@ -39,7 +39,7 @@ static uint16_t mod_u16(uint32_t n, uint16_t d, uint32_t p, uint32_t m) {
 
   // Multiply and subtract to get the remainder.
   n -= d * t;
-  assert(n < d);
+  declassify_assert(n < d);
   return n;
 }
 

crypto/fipsmodule/bn/exponentiation.c

@@ -1058,7 +1058,7 @@ int BN_mod_exp_mont_consttime(BIGNUM *rr, const BIGNUM *a, const BIGNUM *p,
 
   // Prepare a^1 in the Montgomery domain.
   assert(!a->neg);
-  assert(BN_ucmp(a, m) < 0);
+  declassify_assert(BN_ucmp(a, m) < 0);
   if (!BN_to_montgomery(&am, a, mont, ctx) ||
       !bn_resize_words(&am, top)) {
     goto err;

crypto/fipsmodule/bn/gcd_extra.c

@@ -93,7 +93,7 @@ static int bn_gcd_consttime(BIGNUM *r, unsigned *out_shift, const BIGNUM *x,
     // At least one of |u| and |v| is now even.
     BN_ULONG u_is_odd = word_is_odd_mask(u->d[0]);
     BN_ULONG v_is_odd = word_is_odd_mask(v->d[0]);
-    assert(!(u_is_odd & v_is_odd));
+    declassify_assert(!(u_is_odd & v_is_odd));
 
     // If both are even, the final GCD gains a factor of two.
     shift += 1 & (~u_is_odd & ~v_is_odd);
@@ -106,7 +106,7 @@ static int bn_gcd_consttime(BIGNUM *r, unsigned *out_shift, const BIGNUM *x,
   // One of |u| or |v| is zero at this point. The algorithm usually makes |u|
   // zero, unless |y| was already zero on input. Fix this by combining the
   // values.
-  assert(BN_is_zero(u) || BN_is_zero(v));
+  declassify_assert(BN_is_zero(u) | BN_is_zero(v));
   for (size_t i = 0; i < width; i++) {
     v->d[i] |= u->d[i];
   }
@@ -289,7 +289,7 @@ int bn_mod_inverse_consttime(BIGNUM *r, int *out_no_inverse, const BIGNUM *a,
     // and |v| is now even.
     BN_ULONG u_is_even = ~word_is_odd_mask(u->d[0]);
     BN_ULONG v_is_even = ~word_is_odd_mask(v->d[0]);
-    assert(u_is_even != v_is_even);
+    declassify_assert(u_is_even != v_is_even);
 
     // Halve the even one and adjust the corresponding coefficient.
     maybe_rshift1_words(u->d, u_is_even, tmp->d, n_width);
@@ -313,7 +313,7 @@ int bn_mod_inverse_consttime(BIGNUM *r, int *out_no_inverse, const BIGNUM *a,
     maybe_rshift1_words_carry(D->d, D_carry, v_is_even, tmp->d, a_width);
   }
 
-  assert(BN_is_zero(v));
+  declassify_assert(BN_is_zero(v));
   // 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.

crypto/fipsmodule/bn/montgomery_inv.c

@@ -154,7 +154,7 @@ static uint64_t bn_neg_inv_mod_r_u64(uint64_t n) {
 
   // The invariant now shows that u*r - v*n == 1 since r == 2 * alpha.
 #if BN_BITS2 == 64 && defined(BN_ULLONG)
-  assert(1 == ((BN_ULLONG)u * 2 * alpha) - ((BN_ULLONG)v * beta));
+  declassify_assert(1 == ((BN_ULLONG)u * 2 * alpha) - ((BN_ULLONG)v * beta));
 #endif
 
   return v;

crypto/fipsmodule/bn/mul.c

@@ -293,7 +293,7 @@ static void bn_mul_recursive(BN_ULONG *r, const BN_ULONG *a, const BN_ULONG *b,
   }
 
   // The product should fit without carries.
-  assert(c == 0);
+  declassify_assert(c == 0);
 }
 
 // bn_mul_part_recursive sets |r| to |a| * |b|, using |t| as scratch space. |r|
@@ -407,7 +407,7 @@ static void bn_mul_part_recursive(BN_ULONG *r, const BN_ULONG *a,
   }
 
   // The product should fit without carries.
-  assert(c == 0);
+  declassify_assert(c == 0);
 }
 
 // bn_mul_impl implements |BN_mul| and |bn_mul_consttime|. Note this function

crypto/fipsmodule/bn/prime.c

@@ -788,7 +788,7 @@ int BN_primality_test(int *out_is_probably_prime, const BIGNUM *w, int checks,
     }
   }
 
-  assert(uniform_iterations >= (crypto_word_t)checks);
+  declassify_assert(uniform_iterations >= (crypto_word_t)checks);
   *out_is_probably_prime = 1;
   ret = 1;
 

crypto/fipsmodule/bn/random.c

@@ -351,7 +351,8 @@ int bn_rand_secret_range(BIGNUM *r, int *out_is_uniform, BN_ULONG min_inclusive,
   // If the value is not in range, force it to be in range.
   r->d[0] |= constant_time_select_w(in_range, 0, min_inclusive);
   r->d[words - 1] &= constant_time_select_w(in_range, BN_MASK2, mask >> 1);
-  assert(bn_in_range_words(r->d, min_inclusive, max_exclusive->d, words));
+  declassify_assert(
+      bn_in_range_words(r->d, min_inclusive, max_exclusive->d, words));
 
   r->neg = 0;
   r->width = (int)words;

crypto/fipsmodule/curve25519/curve25519_nohw.c

@@ -77,7 +77,7 @@ typedef uint64_t fe_limb_t;
 #define assert_fe(f)                                                    \
   do {                                                                  \
     for (unsigned _assert_fe_i = 0; _assert_fe_i < 5; _assert_fe_i++) { \
-      assert(f[_assert_fe_i] <= UINT64_C(0x8cccccccccccc));             \
+      declassify_assert(f[_assert_fe_i] <= UINT64_C(0x8cccccccccccc));  \
     }                                                                   \
   } while (0)
 
@@ -94,7 +94,7 @@ typedef uint64_t fe_limb_t;
 #define assert_fe_loose(f)                                              \
   do {                                                                  \
     for (unsigned _assert_fe_i = 0; _assert_fe_i < 5; _assert_fe_i++) { \
-      assert(f[_assert_fe_i] <= UINT64_C(0x1a666666666664));            \
+      declassify_assert(f[_assert_fe_i] <= UINT64_C(0x1a666666666664)); \
     }                                                                   \
   } while (0)
 
@@ -116,8 +116,8 @@ typedef uint32_t fe_limb_t;
 #define assert_fe(f)                                                     \
   do {                                                                   \
     for (unsigned _assert_fe_i = 0; _assert_fe_i < 10; _assert_fe_i++) { \
-      assert(f[_assert_fe_i] <=                                          \
-             ((_assert_fe_i & 1) ? 0x2333333u : 0x4666666u));            \
+      declassify_assert(f[_assert_fe_i] <=                               \
+                        ((_assert_fe_i & 1) ? 0x2333333u : 0x4666666u)); \
     }                                                                    \
   } while (0)
 
@@ -134,8 +134,8 @@ typedef uint32_t fe_limb_t;
 #define assert_fe_loose(f)                                               \
   do {                                                                   \
     for (unsigned _assert_fe_i = 0; _assert_fe_i < 10; _assert_fe_i++) { \
-      assert(f[_assert_fe_i] <=                                          \
-             ((_assert_fe_i & 1) ? 0x6999999u : 0xd333332u));            \
+      declassify_assert(f[_assert_fe_i] <=                               \
+                        ((_assert_fe_i & 1) ? 0x6999999u : 0xd333332u)); \
     }                                                                    \
   } while (0)
 
@@ -146,7 +146,7 @@ OPENSSL_STATIC_ASSERT(sizeof(fe) == sizeof(fe_limb_t) * FE_NUM_LIMBS,
 
 static void fe_frombytes_strict(fe *h, const uint8_t s[32]) {
   // |fiat_25519_from_bytes| requires the top-most bit be clear.
-  assert((s[31] & 0x80) == 0);
+  declassify_assert((s[31] & 0x80) == 0);
   fiat_25519_from_bytes(h->v, s);
   assert_fe(h->v);
 }

crypto/fipsmodule/rsa/rsa_impl.c

@@ -738,7 +738,7 @@ static int mod_exp(BIGNUM *r0, const BIGNUM *I, RSA *rsa, BN_CTX *ctx) {
 
   // This is a pre-condition for |mod_montgomery|. It was already checked by the
   // caller.
-  assert(BN_ucmp(I, n) < 0);
+  declassify_assert(BN_ucmp(I, n) < 0);
 
   if (!mod_montgomery(r1, I, q, rsa->mont_q, p, ctx) ||
       !mod_montgomery(r2, I, p, rsa->mont_p, q, ctx) ||
@@ -773,7 +773,7 @@ static int mod_exp(BIGNUM *r0, const BIGNUM *I, RSA *rsa, BN_CTX *ctx) {
   // bound the width slightly higher, so fix it. This trips constant-time checks
   // because a naive data flow analysis does not realize the excess words are
   // publicly zero.
-  assert(BN_cmp(r0, n) < 0);
+  declassify_assert(BN_cmp(r0, n) < 0);
   bn_assert_fits_in_bytes(r0, BN_num_bytes(n));
   if (!bn_resize_words(r0, n->width)) {
     goto err;

crypto/internal.h

@@ -552,6 +552,12 @@ static inline int constant_time_declassify_int(int v) {
   return value_barrier_u32(v);
 }
 
+// declassify_assert behaves like |assert| but declassifies the result of
+// evaluating |expr|. This allows the assertion to branch on the (presumably
+// public) result, but still ensures that values leading up to the computation
+// were secret.
+#define declassify_assert(expr) assert(constant_time_declassify_int(expr))
+
 
 // Thread-safe initialisation.
 
@@ -1165,21 +1171,21 @@ static inline uint64_t CRYPTO_rotr_u64(uint64_t value, int shift) {
 
 static inline uint32_t CRYPTO_addc_u32(uint32_t x, uint32_t y, uint32_t carry,
                                        uint32_t *out_carry) {
-  assert(carry <= 1);
+  declassify_assert(carry <= 1);
   return CRYPTO_GENERIC_ADDC(x, y, carry, out_carry);
 }
 
 static inline uint64_t CRYPTO_addc_u64(uint64_t x, uint64_t y, uint64_t carry,
                                        uint64_t *out_carry) {
-  assert(carry <= 1);
+  declassify_assert(carry <= 1);
   return CRYPTO_GENERIC_ADDC(x, y, carry, out_carry);
 }
 
 #else
 
 static inline uint32_t CRYPTO_addc_u32(uint32_t x, uint32_t y, uint32_t carry,
                                        uint32_t *out_carry) {
-  assert(carry <= 1);
+  declassify_assert(carry <= 1);
   uint64_t ret = carry;
   ret += (uint64_t)x + y;
   *out_carry = (uint32_t)(ret >> 32);
@@ -1188,7 +1194,7 @@ static inline uint32_t CRYPTO_addc_u32(uint32_t x, uint32_t y, uint32_t carry,
 
 static inline uint64_t CRYPTO_addc_u64(uint64_t x, uint64_t y, uint64_t carry,
                                        uint64_t *out_carry) {
-  assert(carry <= 1);
+  declassify_assert(carry <= 1);
 #if defined(BORINGSSL_HAS_UINT128)
   uint128_t ret = carry;
   ret += (uint128_t)x + y;
@@ -1217,29 +1223,29 @@ static inline uint64_t CRYPTO_addc_u64(uint64_t x, uint64_t y, uint64_t carry,
 
 static inline uint32_t CRYPTO_subc_u32(uint32_t x, uint32_t y, uint32_t borrow,
                                        uint32_t *out_borrow) {
-  assert(borrow <= 1);
+  declassify_assert(borrow <= 1);
   return CRYPTO_GENERIC_SUBC(x, y, borrow, out_borrow);
 }
 
 static inline uint64_t CRYPTO_subc_u64(uint64_t x, uint64_t y, uint64_t borrow,
                                        uint64_t *out_borrow) {
-  assert(borrow <= 1);
+  declassify_assert(borrow <= 1);
   return CRYPTO_GENERIC_SUBC(x, y, borrow, out_borrow);
 }
 
 #else
 
 static inline uint32_t CRYPTO_subc_u32(uint32_t x, uint32_t y, uint32_t borrow,
                                        uint32_t *out_borrow) {
-  assert(borrow <= 1);
+  declassify_assert(borrow <= 1);
   uint32_t ret = x - y - borrow;
   *out_borrow = (x < y) | ((x == y) & borrow);
   return ret;
 }
 
 static inline uint64_t CRYPTO_subc_u64(uint64_t x, uint64_t y, uint64_t borrow,
                                        uint64_t *out_borrow) {
-  assert(borrow <= 1);
+  declassify_assert(borrow <= 1);
   uint64_t ret = x - y - borrow;
   *out_borrow = (x < y) | ((x == y) & borrow);
   return ret;