From bc3216baa1177f40c111d2bbe8e1b9d47bc0f370 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Fri, 12 Mar 2021 10:23:17 -0800 Subject: [PATCH] Add VERIFY_ONLY_CHECK that only evaluates in VERIFY mode --- src/ecmult_const_impl.h | 2 +- src/group_impl.h | 2 +- src/modules/extrakeys/main_impl.h | 2 +- src/scalar_4x64_impl.h | 4 +--- src/scalar_8x32_impl.h | 4 +--- src/scalar_impl.h | 6 +++--- src/scalar_low_impl.h | 4 +--- src/secp256k1.c | 2 +- src/util.h | 9 ++++++++- 9 files changed, 18 insertions(+), 17 deletions(-) diff --git a/src/ecmult_const_impl.h b/src/ecmult_const_impl.h index 0e1fb965cb..6fde627bf8 100644 --- a/src/ecmult_const_impl.h +++ b/src/ecmult_const_impl.h @@ -129,7 +129,7 @@ static int secp256k1_wnaf_const(int *wnaf, const secp256k1_scalar *scalar, int w } while (word * w < size); wnaf[word] = u * global_sign; - VERIFY_CHECK(secp256k1_scalar_is_zero(&s)); + VERIFY_ONLY_CHECK(secp256k1_scalar_is_zero(&s)); VERIFY_CHECK(word == WNAF_SIZE_BITS(size, w)); return skew; } diff --git a/src/group_impl.h b/src/group_impl.h index b7094c5377..52b0b99f03 100644 --- a/src/group_impl.h +++ b/src/group_impl.h @@ -616,7 +616,7 @@ static void secp256k1_gej_add_ge(secp256k1_gej *r, const secp256k1_gej *a, const static void secp256k1_gej_rescale(secp256k1_gej *r, const secp256k1_fe *s) { /* Operations: 4 mul, 1 sqr */ secp256k1_fe zz; - VERIFY_CHECK(!secp256k1_fe_is_zero(s)); + VERIFY_ONLY_CHECK(!secp256k1_fe_is_zero(s)); secp256k1_fe_sqr(&zz, s); secp256k1_fe_mul(&r->x, &r->x, &zz); /* r->x *= s^2 */ secp256k1_fe_mul(&r->y, &r->y, &zz); diff --git a/src/modules/extrakeys/main_impl.h b/src/modules/extrakeys/main_impl.h index 7390b22718..ecdd09678a 100644 --- a/src/modules/extrakeys/main_impl.h +++ b/src/modules/extrakeys/main_impl.h @@ -60,7 +60,7 @@ int secp256k1_xonly_pubkey_serialize(const secp256k1_context* ctx, unsigned char * Requires that the coordinates of r are normalized. */ static int secp256k1_extrakeys_ge_even_y(secp256k1_ge *r) { int y_parity = 0; - VERIFY_CHECK(!secp256k1_ge_is_infinity(r)); + VERIFY_ONLY_CHECK(!secp256k1_ge_is_infinity(r)); if (secp256k1_fe_is_odd(&r->y)) { secp256k1_fe_negate(&r->y, &r->y, 1); diff --git a/src/scalar_4x64_impl.h b/src/scalar_4x64_impl.h index 3eaa0418c6..0cad37b8e4 100644 --- a/src/scalar_4x64_impl.h +++ b/src/scalar_4x64_impl.h @@ -108,10 +108,8 @@ static void secp256k1_scalar_cadd_bit(secp256k1_scalar *r, unsigned int bit, int r->d[2] = t & 0xFFFFFFFFFFFFFFFFULL; t >>= 64; t += (uint128_t)r->d[3] + (((uint64_t)((bit >> 6) == 3)) << (bit & 0x3F)); r->d[3] = t & 0xFFFFFFFFFFFFFFFFULL; -#ifdef VERIFY VERIFY_CHECK((t >> 64) == 0); - VERIFY_CHECK(secp256k1_scalar_check_overflow(r) == 0); -#endif + VERIFY_ONLY_CHECK(secp256k1_scalar_check_overflow(r) == 0); } static void secp256k1_scalar_set_b32(secp256k1_scalar *r, const unsigned char *b32, int *overflow) { diff --git a/src/scalar_8x32_impl.h b/src/scalar_8x32_impl.h index bf98e01d76..f6a100b0fb 100644 --- a/src/scalar_8x32_impl.h +++ b/src/scalar_8x32_impl.h @@ -156,10 +156,8 @@ static void secp256k1_scalar_cadd_bit(secp256k1_scalar *r, unsigned int bit, int r->d[6] = t & 0xFFFFFFFFULL; t >>= 32; t += (uint64_t)r->d[7] + (((uint32_t)((bit >> 5) == 7)) << (bit & 0x1F)); r->d[7] = t & 0xFFFFFFFFULL; -#ifdef VERIFY VERIFY_CHECK((t >> 32) == 0); - VERIFY_CHECK(secp256k1_scalar_check_overflow(r) == 0); -#endif + VERIFY_ONLY_CHECK(secp256k1_scalar_check_overflow(r) == 0); } static void secp256k1_scalar_set_b32(secp256k1_scalar *r, const unsigned char *b32, int *overflow) { diff --git a/src/scalar_impl.h b/src/scalar_impl.h index 61c1fbd58e..69003cb203 100644 --- a/src/scalar_impl.h +++ b/src/scalar_impl.h @@ -498,17 +498,17 @@ static void secp256k1_scalar_split_lambda_verify(const secp256k1_scalar *r1, con secp256k1_scalar_mul(&s, &secp256k1_const_lambda, r2); secp256k1_scalar_add(&s, &s, r1); - VERIFY_CHECK(secp256k1_scalar_eq(&s, k)); + VERIFY_ONLY_CHECK(secp256k1_scalar_eq(&s, k)); secp256k1_scalar_negate(&s, r1); secp256k1_scalar_get_b32(buf1, r1); secp256k1_scalar_get_b32(buf2, &s); - VERIFY_CHECK(secp256k1_memcmp_var(buf1, k1_bound, 32) < 0 || secp256k1_memcmp_var(buf2, k1_bound, 32) < 0); + VERIFY_ONLY_CHECK(secp256k1_memcmp_var(buf1, k1_bound, 32) < 0 || secp256k1_memcmp_var(buf2, k1_bound, 32) < 0); secp256k1_scalar_negate(&s, r2); secp256k1_scalar_get_b32(buf1, r2); secp256k1_scalar_get_b32(buf2, &s); - VERIFY_CHECK(secp256k1_memcmp_var(buf1, k2_bound, 32) < 0 || secp256k1_memcmp_var(buf2, k2_bound, 32) < 0); + VERIFY_ONLY_CHECK(secp256k1_memcmp_var(buf1, k2_bound, 32) < 0 || secp256k1_memcmp_var(buf2, k2_bound, 32) < 0); } #endif /* VERIFY */ #endif /* !defined(EXHAUSTIVE_TEST_ORDER) */ diff --git a/src/scalar_low_impl.h b/src/scalar_low_impl.h index 98ffd1536e..eb982735ce 100644 --- a/src/scalar_low_impl.h +++ b/src/scalar_low_impl.h @@ -39,12 +39,10 @@ static int secp256k1_scalar_add(secp256k1_scalar *r, const secp256k1_scalar *a, static void secp256k1_scalar_cadd_bit(secp256k1_scalar *r, unsigned int bit, int flag) { if (flag && bit < 32) *r += ((uint32_t)1 << bit); -#ifdef VERIFY VERIFY_CHECK(bit < 32); /* Verify that adding (1 << bit) will not overflow any in-range scalar *r by overflowing the underlying uint32_t. */ VERIFY_CHECK(((uint32_t)1 << bit) - 1 <= UINT32_MAX - EXHAUSTIVE_TEST_ORDER); - VERIFY_CHECK(secp256k1_scalar_check_overflow(r) == 0); -#endif + VERIFY_ONLY_CHECK(secp256k1_scalar_check_overflow(r) == 0); } static void secp256k1_scalar_set_b32(secp256k1_scalar *r, const unsigned char *b32, int *overflow) { diff --git a/src/secp256k1.c b/src/secp256k1.c index 4f56c27c8a..4410072035 100644 --- a/src/secp256k1.c +++ b/src/secp256k1.c @@ -268,7 +268,7 @@ static void secp256k1_pubkey_save(secp256k1_pubkey* pubkey, secp256k1_ge* ge) { secp256k1_ge_to_storage(&s, ge); memcpy(&pubkey->data[0], &s, sizeof(s)); } else { - VERIFY_CHECK(!secp256k1_ge_is_infinity(ge)); + VERIFY_ONLY_CHECK(!secp256k1_ge_is_infinity(ge)); secp256k1_fe_normalize_var(&ge->x); secp256k1_fe_normalize_var(&ge->y); secp256k1_fe_get_b32(pubkey->data, &ge->x); diff --git a/src/util.h b/src/util.h index b7e457c48b..2fc7dd5183 100644 --- a/src/util.h +++ b/src/util.h @@ -57,15 +57,22 @@ static SECP256K1_INLINE void secp256k1_callback_call(const secp256k1_callback * } while(0) #endif -/* Like assert(), but when VERIFY is defined, and side-effect safe. */ +/* VERIFY_CHECK is like assert(), but only enabled when VERIFY is defined, and side-effect safe + (even in non-VERIFY mode, the argument is evaluated - and hopefully optimized out). + VERIFY_ONLY_CHECK is similar but its argument is only evaluated in VERIFY mode; it is + intended for nontrivial consistency checks that may be too hard to optimize out, or + risk introducing variable-time behavior. */ #if defined(COVERAGE) #define VERIFY_CHECK(check) +#define VERIFY_ONLY_CHECK(check) #define VERIFY_SETUP(stmt) #elif defined(VERIFY) #define VERIFY_CHECK CHECK +#define VERIFY_ONLY_CHECK CHECK #define VERIFY_SETUP(stmt) do { stmt; } while(0) #else #define VERIFY_CHECK(cond) do { (void)(cond); } while(0) +#define VERIFY_ONLY_CHECK(cond) do { if (0) { (void)(cond); } } while(0) #define VERIFY_SETUP(stmt) #endif