Skip to content

Commit

Permalink
Merge dashpay#754: Fix uninit values passed into cmov
Browse files Browse the repository at this point in the history
f79a7ad Add valgrind uninit check to cmovs output (Elichai Turkel)
a39c2b0 Fixed UB(arithmetics on uninit values) in cmovs (Elichai Turkel)

Pull request description:

  This should fix dashpay#753.
  Used @peterdettman's solution here for the `ECMULT_CONST_TABLE_GET_GE` bitcoin-core/secp256k1#753 (comment)
  and in ecdsa_sign I initialize `s` and `r` to a zero scalar.

  The second commit adds a valgrind check to the cmovs that could've caught this (in ecdsa_sign, not in ecmult_const because there's a scalar clear there under `VERIFY_SETUP`)

ACKs for top commit:
  sipa:
    utACK f79a7ad
  jonasnick:
    ACK f79a7ad
  real-or-random:
    ACK f79a7ad

Tree-SHA512: 6fd7b7c84f392bda733a973f4dcfc12bf1478aac2591e2c87b69e637847d3b063c4243cc8feccaffc3a5824c18183a5e66bd4251c2322abaf63bb6439b38defe
  • Loading branch information
real-or-random committed Jun 2, 2020
2 parents 05d315a + f79a7ad commit 5e1c885
Show file tree
Hide file tree
Showing 12 changed files with 38 additions and 18 deletions.
8 changes: 6 additions & 2 deletions src/ecmult_const_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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; \
Expand All @@ -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); \
Expand Down
4 changes: 2 additions & 2 deletions src/field.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
2 changes: 2 additions & 0 deletions src/field_10x26_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand Down
2 changes: 2 additions & 0 deletions src/field_5x52_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion src/group.h
Original file line number Diff line number Diff line change
Expand Up @@ -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. */
Expand Down
2 changes: 1 addition & 1 deletion src/scalar.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
1 change: 1 addition & 0 deletions src/scalar_4x64_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
1 change: 1 addition & 0 deletions src/scalar_8x32_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
1 change: 1 addition & 0 deletions src/scalar_low_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
3 changes: 2 additions & 1 deletion src/secp256k1.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
11 changes: 0 additions & 11 deletions src/tests.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
19 changes: 19 additions & 0 deletions src/util.h
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down

0 comments on commit 5e1c885

Please sign in to comment.