From a39c2b09de304b8f24716b59219ae37c2538c242 Mon Sep 17 00:00:00 2001 From: Elichai Turkel Date: Wed, 20 May 2020 15:09:13 +0300 Subject: [PATCH] Fixed UB(arithmetics on uninit values) in cmovs --- src/ecmult_const_impl.h | 8 ++++++-- src/field.h | 4 ++-- src/group.h | 2 +- src/scalar.h | 2 +- src/secp256k1.c | 3 ++- 5 files changed, 12 insertions(+), 7 deletions(-) diff --git a/src/ecmult_const_impl.h b/src/ecmult_const_impl.h index 011ccf0d4251e..6d6d354aa4e1a 100644 --- a/src/ecmult_const_impl.h +++ b/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); \ diff --git a/src/field.h b/src/field.h index 8283e4b182e70..7993a1f11e32d 100644 --- a/src/field.h +++ b/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 */ diff --git a/src/group.h b/src/group.h index ded4e1dabd007..863644f0f0bbf 100644 --- a/src/group.h +++ b/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. */ diff --git a/src/scalar.h b/src/scalar.h index 6dc7574ca82bc..2a747035230ba 100644 --- a/src/scalar.h +++ b/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 */ diff --git a/src/secp256k1.c b/src/secp256k1.c index de66be5785ab5..2b2238b8f8f6b 100644 --- a/src/secp256k1.c +++ b/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;