Fix uninit values passed into cmov

src/ecmult_const_impl.h

@@ -14,7 +14,7 @@
 
 /* This is like `ECMULT_TABLE_GET_GE` but is constant time */
 #define ECMULT_CONST_TABLE_GET_GE(r,pre,n,w) do { \
- int m; \
+ int m = 0; \
  /* Extract the sign-bit for a constant time absolute-value. */ \
  int mask = (n) >> (sizeof(n) * CHAR_BIT - 1); \
  int abs_n = ((n) + mask) ^ mask; \
@@ -25,7 +25,11 @@
  VERIFY_CHECK((n) <= ((1 << ((w)-1)) - 1)); \
  VERIFY_SETUP(secp256k1_fe_clear(&(r)->x)); \
  VERIFY_SETUP(secp256k1_fe_clear(&(r)->y)); \
- for (m = 0; m < ECMULT_TABLE_SIZE(w); m++) { \
+ /* Unconditionally set r->x = (pre)[m].x. r->y = (pre)[m].y. because it's either the correct one \
+ * or will get replaced in the later iterations, this is needed to make sure `r` is initialized. */ \
+ (r)->x = (pre)[m].x; \
+ (r)->y = (pre)[m].y; \
+ for (m = 1; m < ECMULT_TABLE_SIZE(w); m++) { \
  /* This loop is used to avoid secret data in array indices. See
  * the comment in ecmult_gen_impl.h for rationale. */ \
  secp256k1_fe_cmov(&(r)->x, &(pre)[m].x, m == idx_n); \

src/field.h

@@ -125,10 +125,10 @@ static void secp256k1_fe_to_storage(secp256k1_fe_storage *r, const secp256k1_fe
 /** Convert a field element back from the storage type. */
 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. */
+/** If flag is true, set *r equal to *a; otherwise leave it. Constant-time.  Both *r and *a must be initialized.*/
 static void secp256k1_fe_storage_cmov(secp256k1_fe_storage *r, const secp256k1_fe_storage *a, int flag);
 
-/** If flag is true, set *r equal to *a; otherwise leave it. Constant-time. */
+/** If flag is true, set *r equal to *a; otherwise leave it. Constant-time.  Both *r and *a must be initialized.*/
 static void secp256k1_fe_cmov(secp256k1_fe *r, const secp256k1_fe *a, int flag);
 
 #endif /* SECP256K1_FIELD_H */

src/field_10x26_impl.h

@@ -1097,6 +1097,7 @@ static void secp256k1_fe_sqr(secp256k1_fe *r, const secp256k1_fe *a) {
 
 static SECP256K1_INLINE void secp256k1_fe_cmov(secp256k1_fe *r, const secp256k1_fe *a, int flag) {
  uint32_t mask0, mask1;
+ VG_CHECK_VERIFY(r->n, sizeof(r->n));
  mask0 = flag + ~((uint32_t)0);
  mask1 = ~mask0;
  r->n[0] = (r->n[0] & mask0) | (a->n[0] & mask1);
@@ -1119,6 +1120,7 @@ static SECP256K1_INLINE void secp256k1_fe_cmov(secp256k1_fe *r, const secp256k1_
 
 static SECP256K1_INLINE void secp256k1_fe_storage_cmov(secp256k1_fe_storage *r, const secp256k1_fe_storage *a, int flag) {
  uint32_t mask0, mask1;
+ VG_CHECK_VERIFY(r->n, sizeof(r->n));
  mask0 = flag + ~((uint32_t)0);
  mask1 = ~mask0;
  r->n[0] = (r->n[0] & mask0) | (a->n[0] & mask1);

src/field_5x52_impl.h

@@ -449,6 +449,7 @@ static void secp256k1_fe_sqr(secp256k1_fe *r, const secp256k1_fe *a) {
 
 static SECP256K1_INLINE void secp256k1_fe_cmov(secp256k1_fe *r, const secp256k1_fe *a, int flag) {
  uint64_t mask0, mask1;
+ VG_CHECK_VERIFY(r->n, sizeof(r->n));
  mask0 = flag + ~((uint64_t)0);
  mask1 = ~mask0;
  r->n[0] = (r->n[0] & mask0) | (a->n[0] & mask1);
@@ -466,6 +467,7 @@ static SECP256K1_INLINE void secp256k1_fe_cmov(secp256k1_fe *r, const secp256k1_
 
 static SECP256K1_INLINE void secp256k1_fe_storage_cmov(secp256k1_fe_storage *r, const secp256k1_fe_storage *a, int flag) {
  uint64_t mask0, mask1;
+ VG_CHECK_VERIFY(r->n, sizeof(r->n));
  mask0 = flag + ~((uint64_t)0);
  mask1 = ~mask0;
  r->n[0] = (r->n[0] & mask0) | (a->n[0] & mask1);

src/group.h

@@ -132,7 +132,7 @@ 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. */
+/** If flag is true, set *r equal to *a; otherwise leave it. Constant-time.  Both *r and *a must be initialized.*/
 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. */

src/scalar.h

@@ -111,7 +111,7 @@ static void secp256k1_scalar_split_lambda(secp256k1_scalar *r1, secp256k1_scalar
 /** 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. */
+/** If flag is true, set *r equal to *a; otherwise leave it. Constant-time.  Both *r and *a must be initialized.*/
 static void secp256k1_scalar_cmov(secp256k1_scalar *r, const secp256k1_scalar *a, int flag);
 
 #endif /* SECP256K1_SCALAR_H */

src/scalar_4x64_impl.h

@@ -948,6 +948,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;
+ VG_CHECK_VERIFY(r->d, sizeof(r->d));
  mask0 = flag + ~((uint64_t)0);
  mask1 = ~mask0;
  r->d[0] = (r->d[0] & mask0) | (a->d[0] & mask1);

src/scalar_8x32_impl.h

@@ -720,6 +720,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;
+ VG_CHECK_VERIFY(r->d, sizeof(r->d));
  mask0 = flag + ~((uint32_t)0);
  mask1 = ~mask0;
  r->d[0] = (r->d[0] & mask0) | (a->d[0] & mask1);

src/scalar_low_impl.h

@@ -116,6 +116,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;
+ VG_CHECK_VERIFY(r, sizeof(*r));
  mask0 = flag + ~((uint32_t)0);
  mask1 = ~mask0;
  *r = (*r & mask0) | (*a & mask1);

src/secp256k1.c

@@ -468,7 +468,8 @@ const secp256k1_nonce_function secp256k1_nonce_function_rfc6979 = nonce_function
 const secp256k1_nonce_function secp256k1_nonce_function_default = nonce_function_rfc6979;
 
 int secp256k1_ecdsa_sign(const secp256k1_context* ctx, secp256k1_ecdsa_signature *signature, const unsigned char *msg32, const unsigned char *seckey, secp256k1_nonce_function noncefp, const void* noncedata) {
- secp256k1_scalar r, s;
+ /* Default initialization here is important so we won't pass uninit values to the cmov in the end */
+ secp256k1_scalar r = secp256k1_scalar_zero, s = secp256k1_scalar_zero;
  secp256k1_scalar sec, non, msg;
  int ret = 0;
  int is_sec_valid;

src/tests.c

@@ -32,17 +32,6 @@ void ECDSA_SIG_get0(const ECDSA_SIG *sig, const BIGNUM **pr, const BIGNUM **ps)
 #include "contrib/lax_der_parsing.c"
 #include "contrib/lax_der_privatekey_parsing.c"
 
-#if !defined(VG_CHECK)
-# if defined(VALGRIND)
-# include <valgrind/memcheck.h>
-# define VG_UNDEF(x,y) VALGRIND_MAKE_MEM_UNDEFINED((x),(y))
-# define VG_CHECK(x,y) VALGRIND_CHECK_MEM_IS_DEFINED((x),(y))
-# else
-# define VG_UNDEF(x,y)
-# define VG_CHECK(x,y)
-# endif
-#endif
-
 static int count = 64;
 static secp256k1_context *ctx = NULL;
 

src/util.h

@@ -69,6 +69,25 @@ static SECP256K1_INLINE void secp256k1_callback_call(const secp256k1_callback *
 #define VERIFY_SETUP(stmt)
 #endif
 
+/* Define `VG_UNDEF` and `VG_CHECK` when VALGRIND is defined */
+#if !defined(VG_CHECK)
+# if defined(VALGRIND)
+# include <valgrind/memcheck.h>
+# define VG_UNDEF(x,y) VALGRIND_MAKE_MEM_UNDEFINED((x),(y))
+# define VG_CHECK(x,y) VALGRIND_CHECK_MEM_IS_DEFINED((x),(y))
+# else
+# define VG_UNDEF(x,y)
+# define VG_CHECK(x,y)
+# endif
+#endif
+
+/* Like `VG_CHECK` but on VERIFY only */
+#if defined(VERIFY)
+#define VG_CHECK_VERIFY(x,y) VG_CHECK((x), (y))
+#else
+#define VG_CHECK_VERIFY(x,y)
+#endif
+
 static SECP256K1_INLINE void *checked_malloc(const secp256k1_callback* cb, size_t size) {
  void *ret = malloc(size);
  if (ret == NULL) {