Merge bitcoin-core/secp256k1#1783: Add VERIFY_CHECKs and documentation that flags must be 0 or 1

ae00c552df Add VERIFY_CHECKs that flags are 0 or 1 (John Moffett)

Pull request description:

  Flags for constant-time masking rely on the values being exactly `0` or `1` rather than `0` or true (any nonzero). One function, `secp256k1_fe_cmov` [documents](e7f7083b53/src/field.h (L315)) and [`VERIFY_CHECK`s](e7f7083b53/src/field_impl.h (L365)) this, but most don't.

  This updates the documentation and adds `VERIFY_CHECK`s enforcing `flag == 0 || flag == 1` for:

  `secp256k1_fe_storage_cmov`
  `secp256k1_gej_cmov`
  `secp256k1_ge_storage_cmov`
  `secp256k1_scalar_cadd_bit`
  `secp256k1_scalar_cond_negate`
  `secp256k1_scalar_cmov`
  `secp256k1_int_cmov`

ACKs for top commit:
  furszy:
    ACK ae00c55
  hebasto:
    re-ACK ae00c552df.

Tree-SHA512: c9d358929d39d93b0aea602d318429f7e82af96bf601f048a1cdeb0621b8adc6d1204648d352aa2060cb0f63db6dcf0da863854375ed313cea44dfad61c19a18
This commit is contained in:
merge-script
2025-12-15 20:43:58 +01:00
11 changed files with 30 additions and 8 deletions

View File

@@ -307,7 +307,8 @@ static void secp256k1_fe_to_storage(secp256k1_fe_storage *r, const secp256k1_fe
*/
static void secp256k1_fe_from_storage(secp256k1_fe *r, const secp256k1_fe_storage *a);
/** If flag is true, set *r equal to *a; otherwise leave it. Constant-time. Both *r and *a must be initialized.*/
/** If flag is 1, set *r equal to *a; if flag is 0, leave it. Constant-time.
* Both *r and *a must be initialized. Flag must be 0 or 1. */
static void secp256k1_fe_storage_cmov(secp256k1_fe_storage *r, const secp256k1_fe_storage *a, int flag);
/** Conditionally move a field element in constant time.

View File

@@ -1014,6 +1014,7 @@ SECP256K1_INLINE static void secp256k1_fe_impl_sqr(secp256k1_fe *r, const secp25
SECP256K1_INLINE static void secp256k1_fe_impl_cmov(secp256k1_fe *r, const secp256k1_fe *a, int flag) {
uint32_t mask0, mask1;
volatile int vflag = flag;
VERIFY_CHECK(flag == 0 || flag == 1);
SECP256K1_CHECKMEM_CHECK_VERIFY(r->n, sizeof(r->n));
mask0 = vflag + ~((uint32_t)0);
mask1 = ~mask0;
@@ -1097,6 +1098,7 @@ static SECP256K1_INLINE void secp256k1_fe_impl_half(secp256k1_fe *r) {
static SECP256K1_INLINE void secp256k1_fe_storage_cmov(secp256k1_fe_storage *r, const secp256k1_fe_storage *a, int flag) {
uint32_t mask0, mask1;
volatile int vflag = flag;
VERIFY_CHECK(flag == 0 || flag == 1);
SECP256K1_CHECKMEM_CHECK_VERIFY(r->n, sizeof(r->n));
mask0 = vflag + ~((uint32_t)0);
mask1 = ~mask0;

View File

@@ -349,6 +349,7 @@ SECP256K1_INLINE static void secp256k1_fe_impl_sqr(secp256k1_fe *r, const secp25
SECP256K1_INLINE static void secp256k1_fe_impl_cmov(secp256k1_fe *r, const secp256k1_fe *a, int flag) {
uint64_t mask0, mask1;
volatile int vflag = flag;
VERIFY_CHECK(flag == 0 || flag == 1);
SECP256K1_CHECKMEM_CHECK_VERIFY(r->n, sizeof(r->n));
mask0 = vflag + ~((uint64_t)0);
mask1 = ~mask0;
@@ -416,6 +417,7 @@ static SECP256K1_INLINE void secp256k1_fe_impl_half(secp256k1_fe *r) {
static SECP256K1_INLINE void secp256k1_fe_storage_cmov(secp256k1_fe_storage *r, const secp256k1_fe_storage *a, int flag) {
uint64_t mask0, mask1;
volatile int vflag = flag;
VERIFY_CHECK(flag == 0 || flag == 1);
SECP256K1_CHECKMEM_CHECK_VERIFY(r->n, sizeof(r->n));
mask0 = vflag + ~((uint64_t)0);
mask1 = ~mask0;

View File

@@ -169,10 +169,12 @@ static void secp256k1_ge_to_storage(secp256k1_ge_storage *r, const secp256k1_ge
/** Convert a group element back from the storage type. */
static void secp256k1_ge_from_storage(secp256k1_ge *r, const secp256k1_ge_storage *a);
/** If flag is true, set *r equal to *a; otherwise leave it. Constant-time. Both *r and *a must be initialized.*/
/** If flag is 1, set *r equal to *a; if flag is 0, leave it. Constant-time.
* Both *r and *a must be initialized. Flag must be 0 or 1. */
static void secp256k1_gej_cmov(secp256k1_gej *r, const secp256k1_gej *a, int flag);
/** If flag is true, set *r equal to *a; otherwise leave it. Constant-time. Both *r and *a must be initialized.*/
/** If flag is 1, set *r equal to *a; if flag is 0, leave it. Constant-time.
* Both *r and *a must be initialized. Flag must be 0 or 1. */
static void secp256k1_ge_storage_cmov(secp256k1_ge_storage *r, const secp256k1_ge_storage *a, int flag);
/** Rescale a jacobian point by b which must be non-zero. Constant-time. */

View File

@@ -898,6 +898,7 @@ static void secp256k1_ge_from_storage(secp256k1_ge *r, const secp256k1_ge_storag
static SECP256K1_INLINE void secp256k1_gej_cmov(secp256k1_gej *r, const secp256k1_gej *a, int flag) {
SECP256K1_GEJ_VERIFY(r);
SECP256K1_GEJ_VERIFY(a);
VERIFY_CHECK(flag == 0 || flag == 1);
secp256k1_fe_cmov(&r->x, &a->x, flag);
secp256k1_fe_cmov(&r->y, &a->y, flag);
@@ -908,6 +909,7 @@ static SECP256K1_INLINE void secp256k1_gej_cmov(secp256k1_gej *r, const secp256k
}
static SECP256K1_INLINE void secp256k1_ge_storage_cmov(secp256k1_ge_storage *r, const secp256k1_ge_storage *a, int flag) {
VERIFY_CHECK(flag == 0 || flag == 1);
secp256k1_fe_storage_cmov(&r->x, &a->x, flag);
secp256k1_fe_storage_cmov(&r->y, &a->y, flag);
}

View File

@@ -69,7 +69,8 @@ static int secp256k1_musig_secnonce_load(const secp256k1_context* ctx, secp256k1
return 1;
}
/* If flag is true, invalidate the secnonce; otherwise leave it. Constant-time. */
/* If flag is 1, invalidate the secnonce; if flag is 0, leave it.
* Constant-time. Flag must be 0 or 1. */
static void secp256k1_musig_secnonce_invalidate(const secp256k1_context* ctx, secp256k1_musig_secnonce *secnonce, int flag) {
secp256k1_memczero(secnonce->data, sizeof(secnonce->data), flag);
/* The flag argument is usually classified. So, the line above makes the

View File

@@ -48,7 +48,7 @@ static void secp256k1_scalar_get_b32(unsigned char *bin, const secp256k1_scalar*
/** Add two scalars together (modulo the group order). Returns whether it overflowed. */
static int secp256k1_scalar_add(secp256k1_scalar *r, const secp256k1_scalar *a, const secp256k1_scalar *b);
/** Conditionally add a power of two to a scalar. The result is not allowed to overflow. */
/** Conditionally add a power of two to a scalar. The result is not allowed to overflow. Flag must be 0 or 1. */
static void secp256k1_scalar_cadd_bit(secp256k1_scalar *r, unsigned int bit, int flag);
/** Multiply two scalars (modulo the group order). */
@@ -78,7 +78,7 @@ static int secp256k1_scalar_is_even(const secp256k1_scalar *a);
/** Check whether a scalar is higher than the group order divided by 2. */
static int secp256k1_scalar_is_high(const secp256k1_scalar *a);
/** Conditionally negate a number, in constant time.
/** Conditionally negate a number, in constant time. Flag must be 0 or 1.
* Returns -1 if the number was negated, 1 otherwise */
static int secp256k1_scalar_cond_negate(secp256k1_scalar *a, int flag);
@@ -95,7 +95,7 @@ static void secp256k1_scalar_split_lambda(secp256k1_scalar * SECP256K1_RESTRICT
/** Multiply a and b (without taking the modulus!), divide by 2**shift, and round to the nearest integer. Shift must be at least 256. */
static void secp256k1_scalar_mul_shift_var(secp256k1_scalar *r, const secp256k1_scalar *a, const secp256k1_scalar *b, unsigned int shift);
/** If flag is true, set *r equal to *a; otherwise leave it. Constant-time. Both *r and *a must be initialized.*/
/** If flag is 1, set *r equal to *a; if flag is 0, leave it. Constant-time. Both *r and *a must be initialized. Flag must be 0 or 1. */
static void secp256k1_scalar_cmov(secp256k1_scalar *r, const secp256k1_scalar *a, int flag);
/** Check invariants on a scalar (no-op unless VERIFY is enabled). */

View File

@@ -120,6 +120,7 @@ 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) {
secp256k1_uint128 t;
volatile int vflag = flag;
VERIFY_CHECK(flag == 0 || flag == 1);
SECP256K1_SCALAR_VERIFY(r);
VERIFY_CHECK(bit < 256);
@@ -259,6 +260,7 @@ static int secp256k1_scalar_cond_negate(secp256k1_scalar *r, int flag) {
uint64_t mask = -vflag;
uint64_t nonzero = (secp256k1_scalar_is_zero(r) != 0) - 1;
secp256k1_uint128 t;
VERIFY_CHECK(flag == 0 || flag == 1);
SECP256K1_SCALAR_VERIFY(r);
secp256k1_u128_from_u64(&t, r->d[0] ^ mask);
@@ -911,6 +913,7 @@ SECP256K1_INLINE static void secp256k1_scalar_mul_shift_var(secp256k1_scalar *r,
static SECP256K1_INLINE void secp256k1_scalar_cmov(secp256k1_scalar *r, const secp256k1_scalar *a, int flag) {
uint64_t mask0, mask1;
volatile int vflag = flag;
VERIFY_CHECK(flag == 0 || flag == 1);
SECP256K1_SCALAR_VERIFY(a);
SECP256K1_CHECKMEM_CHECK_VERIFY(r->d, sizeof(r->d));

View File

@@ -147,6 +147,7 @@ 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) {
uint64_t t;
volatile int vflag = flag;
VERIFY_CHECK(flag == 0 || flag == 1);
SECP256K1_SCALAR_VERIFY(r);
VERIFY_CHECK(bit < 256);
@@ -314,6 +315,7 @@ static int secp256k1_scalar_cond_negate(secp256k1_scalar *r, int flag) {
uint32_t mask = -vflag;
uint32_t nonzero = 0xFFFFFFFFUL * (secp256k1_scalar_is_zero(r) == 0);
uint64_t t = (uint64_t)(r->d[0] ^ mask) + ((SECP256K1_N_0 + 1) & mask);
VERIFY_CHECK(flag == 0 || flag == 1);
SECP256K1_SCALAR_VERIFY(r);
r->d[0] = t & nonzero; t >>= 32;
@@ -709,6 +711,7 @@ SECP256K1_INLINE static void secp256k1_scalar_mul_shift_var(secp256k1_scalar *r,
static SECP256K1_INLINE void secp256k1_scalar_cmov(secp256k1_scalar *r, const secp256k1_scalar *a, int flag) {
uint32_t mask0, mask1;
volatile int vflag = flag;
VERIFY_CHECK(flag == 0 || flag == 1);
SECP256K1_SCALAR_VERIFY(a);
SECP256K1_CHECKMEM_CHECK_VERIFY(r->d, sizeof(r->d));

View File

@@ -56,6 +56,7 @@ 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) {
SECP256K1_SCALAR_VERIFY(r);
VERIFY_CHECK(flag == 0 || flag == 1);
if (flag && bit < 32)
*r += ((uint32_t)1 << bit);
@@ -121,6 +122,7 @@ static int secp256k1_scalar_is_high(const secp256k1_scalar *a) {
static int secp256k1_scalar_cond_negate(secp256k1_scalar *r, int flag) {
SECP256K1_SCALAR_VERIFY(r);
VERIFY_CHECK(flag == 0 || flag == 1);
if (flag) secp256k1_scalar_negate(r, r);
@@ -157,6 +159,7 @@ SECP256K1_INLINE static int secp256k1_scalar_eq(const secp256k1_scalar *a, const
static SECP256K1_INLINE void secp256k1_scalar_cmov(secp256k1_scalar *r, const secp256k1_scalar *a, int flag) {
uint32_t mask0, mask1;
volatile int vflag = flag;
VERIFY_CHECK(flag == 0 || flag == 1);
SECP256K1_SCALAR_VERIFY(a);
SECP256K1_CHECKMEM_CHECK_VERIFY(r, sizeof(*r));

View File

@@ -212,6 +212,7 @@ static SECP256K1_INLINE void secp256k1_memczero(void *s, size_t len, int flag) {
take only be 0 or 1, which leads to variable time code. */
volatile int vflag = flag;
unsigned char mask = -(unsigned char) vflag;
VERIFY_CHECK(flag == 0 || flag == 1);
while (len) {
*p &= ~mask;
p++;
@@ -294,7 +295,8 @@ static SECP256K1_INLINE int secp256k1_is_zero_array(const unsigned char *s, size
return ret;
}
/** If flag is true, set *r equal to *a; otherwise leave it. Constant-time. Both *r and *a must be initialized and non-negative.*/
/** If flag is 1, set *r equal to *a; if flag is 0, leave it. Constant-time.
* Both *r and *a must be initialized and non-negative. Flag must be 0 or 1. */
static SECP256K1_INLINE void secp256k1_int_cmov(int *r, const int *a, int flag) {
unsigned int mask0, mask1, r_masked, a_masked;
/* Access flag with a volatile-qualified lvalue.
@@ -302,6 +304,7 @@ static SECP256K1_INLINE void secp256k1_int_cmov(int *r, const int *a, int flag)
take only be 0 or 1, which leads to variable time code. */
volatile int vflag = flag;
VERIFY_CHECK(flag == 0 || flag == 1);
/* Casting a negative int to unsigned and back to int is implementation defined behavior */
VERIFY_CHECK(*r >= 0 && *a >= 0);