From 3f96b07c8eb8a2d2260be2151d2754dcf969ddf8 Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Thu, 6 Jun 2019 16:51:33 +0200 Subject: [PATCH 1/9] Don't clear secrets in pippenger implementation This code is not supposed to handle secret data. --- src/ecmult_impl.h | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/src/ecmult_impl.h b/src/ecmult_impl.h index f03fa9469d..419a03d849 100644 --- a/src/ecmult_impl.h +++ b/src/ecmult_impl.h @@ -1005,7 +1005,6 @@ static int secp256k1_ecmult_pippenger_batch(const secp256k1_callback* error_call struct secp256k1_pippenger_state *state_space; size_t idx = 0; size_t point_idx = 0; - int i, j; int bucket_window; (void)ctx; @@ -1055,18 +1054,6 @@ static int secp256k1_ecmult_pippenger_batch(const secp256k1_callback* error_call } secp256k1_ecmult_pippenger_wnaf(buckets, bucket_window, state_space, r, scalars, points, idx); - - /* Clear data */ - for(i = 0; (size_t)i < idx; i++) { - secp256k1_scalar_clear(&scalars[i]); - state_space->ps[i].skew_na = 0; - for(j = 0; j < WNAF_SIZE(bucket_window+1); j++) { - state_space->wnaf_na[i * WNAF_SIZE(bucket_window+1) + j] = 0; - } - } - for(i = 0; i < 1< Date: Thu, 6 Jun 2019 21:22:28 +0200 Subject: [PATCH 2/9] Add secp256k1_memclear() for clearing secret data We rely on memset() and an __asm__ memory barrier where it's available or on SecureZeroMemory() on Windows. The fallback implementation uses a volatile function pointer to memset which the compiler is not clever enough to optimize. --- src/util.h | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/src/util.h b/src/util.h index 9a86e78757..00f1db1e74 100644 --- a/src/util.h +++ b/src/util.h @@ -11,11 +11,17 @@ #include "libsecp256k1-config.h" #endif +#include #include #include #include #include +#if defined(_MSC_VER) +// For SecureZeroMemory +#include +#endif + typedef struct { void (*fn)(const char *text, void* data); const void* data; @@ -175,4 +181,29 @@ static SECP256K1_INLINE void memczero(void *s, size_t len, int flag) { } } +/* Cleanses memory to prevent leaking sensitive info. Won't be optimized out. */ +static SECP256K1_INLINE void memclear(void *ptr, size_t len) { +#if defined(_MSC_VER) + /* SecureZeroMemory is guaranteed not to be optimized out by MSVC. */ + SecureZeroMemory(ptr, n); +#elif defined(__GNUC__) + /* We use a memory barrier that scares the compiler away from optimizing out the memset. + * + * Quoting Adam Langley in commit ad1907fe73334d6c696c8539646c21b11178f20f + * in BoringSSL (ISC License): + * As best as we can tell, this is sufficient to break any optimisations that + * might try to eliminate "superfluous" memsets. + * This method used in memzero_explicit() the Linux kernel, too. Its advantage is that it is + * pretty efficient, because the compiler can still implement the memset() efficently, + * just not remove it entirely. See "Dead Store Elimination (Still) Considered Harmful" by + * Yang et al. (USENIX Security 2017) for more background. + */ + memset(ptr, 0, len); + __asm__ __volatile__("" : : "r"(ptr) : "memory"); +#else + void *(*volatile const volatile_memset)(void *, int, size_t) = memset; + volatile_memset(ptr, 0, len); +#endif +} + #endif /* SECP256K1_UTIL_H */ From b17a7df8145a6a86d49c354c7e7b59a432ea5346 Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Wed, 12 Jun 2019 15:49:28 +0200 Subject: [PATCH 3/9] Make _set_fe_int( . , 0 ) set magnitude to 0 --- src/field.h | 7 ++++--- src/field_10x26_impl.h | 2 +- src/field_5x52_impl.h | 2 +- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/field.h b/src/field.h index 8283e4b182..7627dbac77 100644 --- a/src/field.h +++ b/src/field.h @@ -14,8 +14,8 @@ * - Each field element can be normalized or not. * - Each field element has a magnitude, which represents how far away * its representation is away from normalization. Normalized elements - * always have a magnitude of 1, but a magnitude of 1 doesn't imply - * normality. + * always have a magnitude of 0 or 1, but a magnitude of 1 doesn't + * imply normality. */ #if defined HAVE_CONFIG_H @@ -51,7 +51,8 @@ static int secp256k1_fe_normalizes_to_zero(secp256k1_fe *r); * implementation may optionally normalize the input, but this should not be relied upon. */ static int secp256k1_fe_normalizes_to_zero_var(secp256k1_fe *r); -/** Set a field element equal to a small integer. Resulting field element is normalized. */ +/** Set a field element equal to a small integer. Resulting field element is normalized; it has + * magnitude 0 if a == 0, and magnitude 1 otherwise. */ static void secp256k1_fe_set_int(secp256k1_fe *r, int a); /** Sets a field element equal to zero, initializing all fields. */ diff --git a/src/field_10x26_impl.h b/src/field_10x26_impl.h index 39304245da..8b0396864d 100644 --- a/src/field_10x26_impl.h +++ b/src/field_10x26_impl.h @@ -266,7 +266,7 @@ SECP256K1_INLINE static void secp256k1_fe_set_int(secp256k1_fe *r, int a) { r->n[0] = a; r->n[1] = r->n[2] = r->n[3] = r->n[4] = r->n[5] = r->n[6] = r->n[7] = r->n[8] = r->n[9] = 0; #ifdef VERIFY - r->magnitude = 1; + r->magnitude = (a != 0); r->normalized = 1; secp256k1_fe_verify(r); #endif diff --git a/src/field_5x52_impl.h b/src/field_5x52_impl.h index 71aa550e41..2d91007a28 100644 --- a/src/field_5x52_impl.h +++ b/src/field_5x52_impl.h @@ -229,7 +229,7 @@ SECP256K1_INLINE static void secp256k1_fe_set_int(secp256k1_fe *r, int a) { r->n[0] = a; r->n[1] = r->n[2] = r->n[3] = r->n[4] = 0; #ifdef VERIFY - r->magnitude = 1; + r->magnitude = (a != 0); r->normalized = 1; secp256k1_fe_verify(r); #endif From b33a8e49e89f5a1ea02a9b9b9b208cb4f3d59e44 Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Wed, 12 Jun 2019 16:05:06 +0200 Subject: [PATCH 4/9] Separate secp256k1_fe_set_int( . , 0 ) from secp256k1_fe_clear() There are two uses of the secp256k1_fe_clear() function that are now separated into these two functions in order to reflect the intent: 1) initializing the memory prior to being used -> converted to fe_set_int( . , 0 ) 2) zeroing the memory after being used such that no sensitive data remains. -> remains as fe_clear() In the latter case, 'magnitude' and 'normalized' need to be overwritten when VERIFY is enabled. Co-Authored-By: isle2983 --- src/ecmult_const_impl.h | 4 ++-- src/field.h | 2 +- src/field_10x26_impl.h | 2 +- src/field_5x52_impl.h | 2 +- src/group_impl.h | 10 +++++----- src/tests.c | 2 +- 6 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/ecmult_const_impl.h b/src/ecmult_const_impl.h index 011ccf0d42..4f203e8d9e 100644 --- a/src/ecmult_const_impl.h +++ b/src/ecmult_const_impl.h @@ -23,8 +23,8 @@ VERIFY_CHECK(((n) & 1) == 1); \ VERIFY_CHECK((n) >= -((1 << ((w)-1)) - 1)); \ VERIFY_CHECK((n) <= ((1 << ((w)-1)) - 1)); \ - VERIFY_SETUP(secp256k1_fe_clear(&(r)->x)); \ - VERIFY_SETUP(secp256k1_fe_clear(&(r)->y)); \ + VERIFY_SETUP(secp256k1_fe_set_int(&(r)->x, 0)); \ + VERIFY_SETUP(secp256k1_fe_set_int(&(r)->y, 0)); \ for (m = 0; 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. */ \ diff --git a/src/field.h b/src/field.h index 7627dbac77..4d3189b10a 100644 --- a/src/field.h +++ b/src/field.h @@ -55,7 +55,7 @@ static int secp256k1_fe_normalizes_to_zero_var(secp256k1_fe *r); * magnitude 0 if a == 0, and magnitude 1 otherwise. */ static void secp256k1_fe_set_int(secp256k1_fe *r, int a); -/** Sets a field element equal to zero, initializing all fields. */ +/** Clear a field element to prevent leaking sensitive information. */ static void secp256k1_fe_clear(secp256k1_fe *a); /** Verify whether a field element is zero. Requires the input to be normalized. */ diff --git a/src/field_10x26_impl.h b/src/field_10x26_impl.h index 8b0396864d..ac9abe79ee 100644 --- a/src/field_10x26_impl.h +++ b/src/field_10x26_impl.h @@ -293,7 +293,7 @@ SECP256K1_INLINE static void secp256k1_fe_clear(secp256k1_fe *a) { int i; #ifdef VERIFY a->magnitude = 0; - a->normalized = 1; + a->normalized = 0; #endif for (i=0; i<10; i++) { a->n[i] = 0; diff --git a/src/field_5x52_impl.h b/src/field_5x52_impl.h index 2d91007a28..e837984b3f 100644 --- a/src/field_5x52_impl.h +++ b/src/field_5x52_impl.h @@ -256,7 +256,7 @@ SECP256K1_INLINE static void secp256k1_fe_clear(secp256k1_fe *a) { int i; #ifdef VERIFY a->magnitude = 0; - a->normalized = 1; + a->normalized = 0; #endif for (i=0; i<5; i++) { a->n[i] = 0; diff --git a/src/group_impl.h b/src/group_impl.h index 43b039becf..169f8e15de 100644 --- a/src/group_impl.h +++ b/src/group_impl.h @@ -194,15 +194,15 @@ static void secp256k1_ge_globalz_set_table_gej(size_t len, secp256k1_ge *r, secp static void secp256k1_gej_set_infinity(secp256k1_gej *r) { r->infinity = 1; - secp256k1_fe_clear(&r->x); - secp256k1_fe_clear(&r->y); - secp256k1_fe_clear(&r->z); + secp256k1_fe_set_int(&r->x, 0); + secp256k1_fe_set_int(&r->y, 0); + secp256k1_fe_set_int(&r->z, 0); } static void secp256k1_ge_set_infinity(secp256k1_ge *r) { r->infinity = 1; - secp256k1_fe_clear(&r->x); - secp256k1_fe_clear(&r->y); + secp256k1_fe_set_int(&r->x, 0); + secp256k1_fe_set_int(&r->y, 0); } static void secp256k1_gej_clear(secp256k1_gej *r) { diff --git a/src/tests.c b/src/tests.c index 2f2cb71539..30516477d1 100644 --- a/src/tests.c +++ b/src/tests.c @@ -79,7 +79,7 @@ void random_field_element_magnitude(secp256k1_fe *fe) { if (n == 0) { return; } - secp256k1_fe_clear(&zero); + secp256k1_fe_set_int(&zero, 0); secp256k1_fe_negate(&zero, &zero, 0); secp256k1_fe_mul_int(&zero, n - 1); secp256k1_fe_add(fe, &zero); From 9006e91309cc8cbd33fe4888cee6fe104f1cd4f7 Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Sat, 8 Jun 2019 11:56:15 +0200 Subject: [PATCH 5/9] Separate between clearing memory and setting to zero in tests Co-Authored-By: isle2983 Co-Authored-By: Pieter Wuille --- src/tests.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/tests.c b/src/tests.c index 30516477d1..3f67a13cfd 100644 --- a/src/tests.c +++ b/src/tests.c @@ -2063,7 +2063,7 @@ void test_ge(void) { secp256k1_fe zfi2, zfi3; secp256k1_gej_set_infinity(&gej[0]); - secp256k1_ge_clear(&ge[0]); + secp256k1_ge_set_gej(&ge[0], &gej[0]); secp256k1_ge_set_gej_var(&ge[0], &gej[0]); for (i = 0; i < runs; i++) { int j; @@ -2860,12 +2860,12 @@ void test_ecmult_multi(secp256k1_scratch *scratch, secp256k1_ecmult_multi_func e random_group_element_test(&pt[ncount]); } - secp256k1_scalar_clear(&sc[0]); + secp256k1_scalar_set_int(&sc[0], 0); CHECK(ecmult_multi(&ctx->error_callback, &ctx->ecmult_ctx, scratch, &r, &szero, ecmult_multi_callback, &data, 20)); - secp256k1_scalar_clear(&sc[1]); - secp256k1_scalar_clear(&sc[2]); - secp256k1_scalar_clear(&sc[3]); - secp256k1_scalar_clear(&sc[4]); + secp256k1_scalar_set_int(&sc[1], 0); + secp256k1_scalar_set_int(&sc[2], 0); + secp256k1_scalar_set_int(&sc[3], 0); + secp256k1_scalar_set_int(&sc[4], 0); CHECK(ecmult_multi(&ctx->error_callback, &ctx->ecmult_ctx, scratch, &r, &szero, ecmult_multi_callback, &data, 6)); CHECK(ecmult_multi(&ctx->error_callback, &ctx->ecmult_ctx, scratch, &r, &szero, ecmult_multi_callback, &data, 5)); CHECK(secp256k1_gej_is_infinity(&r)); From 443675e52f0495569309e774629aaf7fd3c83bfd Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Sat, 8 Jun 2019 13:54:14 +0200 Subject: [PATCH 6/9] Use secp256k1_memclear() to clear stack memory instead of memset() All of the invocations of secp256k1mem_clear() operate on stack memory and happen after the function is done with the memory object. This commit replaces existing memset() invocations and also adds secp256k1_memclear() to code locations where clearing was missing; there is no guarantee that this commit covers all code locations where clearing is necessary. Co-Authored-By: isle2983 --- src/ecmult_gen_impl.h | 7 ++++--- src/hash_impl.h | 12 +++++++----- src/modules/ecdh/main_impl.h | 5 +++-- src/modules/recovery/main_impl.h | 8 ++++---- src/num_gmp_impl.h | 12 ++++++------ src/secp256k1.c | 4 ++-- 6 files changed, 26 insertions(+), 22 deletions(-) diff --git a/src/ecmult_gen_impl.h b/src/ecmult_gen_impl.h index 30ac16518b..d48b7fa27e 100644 --- a/src/ecmult_gen_impl.h +++ b/src/ecmult_gen_impl.h @@ -151,7 +151,7 @@ static void secp256k1_ecmult_gen(const secp256k1_ecmult_gen_context *ctx, secp25 secp256k1_ge_from_storage(&add, &adds); secp256k1_gej_add_ge(r, r, &add); } - bits = 0; + memclear(&bits, sizeof(bits)); secp256k1_ge_clear(&add); secp256k1_scalar_clear(&gnb); } @@ -182,7 +182,7 @@ static void secp256k1_ecmult_gen_blind(secp256k1_ecmult_gen_context *ctx, const memcpy(keydata + 32, seed32, 32); } secp256k1_rfc6979_hmac_sha256_initialize(&rng, keydata, seed32 ? 64 : 32); - memset(keydata, 0, sizeof(keydata)); + memclear(keydata, sizeof(keydata)); /* Accept unobservably small non-uniformity. */ secp256k1_rfc6979_hmac_sha256_generate(&rng, nonce32, 32); overflow = !secp256k1_fe_set_b32(&s, nonce32); @@ -196,11 +196,12 @@ static void secp256k1_ecmult_gen_blind(secp256k1_ecmult_gen_context *ctx, const /* A blinding value of 0 works, but would undermine the projection hardening. */ secp256k1_scalar_cmov(&b, &secp256k1_scalar_one, secp256k1_scalar_is_zero(&b)); secp256k1_rfc6979_hmac_sha256_finalize(&rng); - memset(nonce32, 0, 32); secp256k1_ecmult_gen(ctx, &gb, &b); secp256k1_scalar_negate(&b, &b); ctx->blind = b; ctx->initial = gb; + + memclear(nonce32, sizeof(nonce32)); secp256k1_scalar_clear(&b); secp256k1_gej_clear(&gb); } diff --git a/src/hash_impl.h b/src/hash_impl.h index 782f97216c..b235a58982 100644 --- a/src/hash_impl.h +++ b/src/hash_impl.h @@ -161,6 +161,10 @@ static void secp256k1_sha256_finalize(secp256k1_sha256 *hash, unsigned char *out hash->s[i] = 0; } memcpy(out32, (const unsigned char*)out, 32); + + memclear(sizedesc, sizeof(sizedesc)); + memclear(out, sizeof(out)); + memclear(hash, sizeof(secp256k1_sha256)); } static void secp256k1_hmac_sha256_initialize(secp256k1_hmac_sha256 *hash, const unsigned char *key, size_t keylen) { @@ -188,7 +192,7 @@ static void secp256k1_hmac_sha256_initialize(secp256k1_hmac_sha256 *hash, const rkey[n] ^= 0x5c ^ 0x36; } secp256k1_sha256_write(&hash->inner, rkey, sizeof(rkey)); - memset(rkey, 0, sizeof(rkey)); + memclear(rkey, sizeof(rkey)); } static void secp256k1_hmac_sha256_write(secp256k1_hmac_sha256 *hash, const unsigned char *data, size_t size) { @@ -199,7 +203,7 @@ static void secp256k1_hmac_sha256_finalize(secp256k1_hmac_sha256 *hash, unsigned unsigned char temp[32]; secp256k1_sha256_finalize(&hash->inner, temp); secp256k1_sha256_write(&hash->outer, temp, 32); - memset(temp, 0, 32); + memclear(temp, sizeof(temp)); secp256k1_sha256_finalize(&hash->outer, out32); } @@ -266,9 +270,7 @@ static void secp256k1_rfc6979_hmac_sha256_generate(secp256k1_rfc6979_hmac_sha256 } static void secp256k1_rfc6979_hmac_sha256_finalize(secp256k1_rfc6979_hmac_sha256 *rng) { - memset(rng->k, 0, 32); - memset(rng->v, 0, 32); - rng->retry = 0; + memclear(rng, sizeof(secp256k1_rfc6979_hmac_sha256)); } #undef BE32 diff --git a/src/modules/ecdh/main_impl.h b/src/modules/ecdh/main_impl.h index 07a25b80d4..6ce5157059 100644 --- a/src/modules/ecdh/main_impl.h +++ b/src/modules/ecdh/main_impl.h @@ -61,9 +61,10 @@ int secp256k1_ecdh(const secp256k1_context* ctx, unsigned char *output, const se ret = hashfp(output, x, y, data); - memset(x, 0, 32); - memset(y, 0, 32); + memclear(x, sizeof(x)); + memclear(y, sizeof(y)); secp256k1_scalar_clear(&s); + secp256k1_ge_clear(&pt); return !!ret & !overflow; } diff --git a/src/modules/recovery/main_impl.h b/src/modules/recovery/main_impl.h index ed356e53a5..e885be1d2a 100644 --- a/src/modules/recovery/main_impl.h +++ b/src/modules/recovery/main_impl.h @@ -121,8 +121,7 @@ static int secp256k1_ecdsa_sig_recover(const secp256k1_ecmult_context *ctx, cons } int secp256k1_ecdsa_sign_recoverable(const secp256k1_context* ctx, secp256k1_ecdsa_recoverable_signature *signature, const unsigned char *msg32, const unsigned char *seckey, secp256k1_nonce_function noncefp, const void* noncedata) { - secp256k1_scalar r, s; - secp256k1_scalar sec, non, msg; + secp256k1_scalar r, s, sec; int recid; int ret = 0; int overflow = 0; @@ -138,6 +137,7 @@ int secp256k1_ecdsa_sign_recoverable(const secp256k1_context* ctx, secp256k1_ecd secp256k1_scalar_set_b32(&sec, seckey, &overflow); /* Fail if the secret key is invalid. */ if (!overflow && !secp256k1_scalar_is_zero(&sec)) { + secp256k1_scalar non, msg; unsigned char nonce32[32]; unsigned int count = 0; secp256k1_scalar_set_b32(&msg, msg32, NULL); @@ -154,11 +154,11 @@ int secp256k1_ecdsa_sign_recoverable(const secp256k1_context* ctx, secp256k1_ecd } count++; } - memset(nonce32, 0, 32); + memclear(nonce32, sizeof(nonce32)); secp256k1_scalar_clear(&msg); secp256k1_scalar_clear(&non); - secp256k1_scalar_clear(&sec); } + secp256k1_scalar_clear(&sec); if (ret) { secp256k1_ecdsa_recoverable_signature_save(signature, &r, &s, recid); } else { diff --git a/src/num_gmp_impl.h b/src/num_gmp_impl.h index 0ae2a8ba0e..85eecb3074 100644 --- a/src/num_gmp_impl.h +++ b/src/num_gmp_impl.h @@ -39,7 +39,7 @@ static void secp256k1_num_get_bin(unsigned char *r, unsigned int rlen, const sec if (len > shift) { memcpy(r + rlen - len + shift, tmp + shift, len - shift); } - memset(tmp, 0, sizeof(tmp)); + memclear(tmp, sizeof(tmp)); } static void secp256k1_num_set_bin(secp256k1_num *r, const unsigned char *a, unsigned int alen) { @@ -85,7 +85,7 @@ static void secp256k1_num_mod(secp256k1_num *r, const secp256k1_num *m) { if (r->limbs >= m->limbs) { mp_limb_t t[2*NUM_LIMBS]; mpn_tdiv_qr(t, r->data, 0, r->data, r->limbs, m->data, m->limbs); - memset(t, 0, sizeof(t)); + memclear(t, sizeof(t)); r->limbs = m->limbs; while (r->limbs > 1 && r->data[r->limbs-1]==0) { r->limbs--; @@ -139,9 +139,9 @@ static void secp256k1_num_mod_inverse(secp256k1_num *r, const secp256k1_num *a, } else { r->limbs = sn; } - memset(g, 0, sizeof(g)); - memset(u, 0, sizeof(u)); - memset(v, 0, sizeof(v)); + memclear(g, sizeof(g)); + memclear(u, sizeof(u)); + memclear(v, sizeof(v)); } static int secp256k1_num_jacobi(const secp256k1_num *a, const secp256k1_num *b) { @@ -256,7 +256,7 @@ static void secp256k1_num_mul(secp256k1_num *r, const secp256k1_num *a, const se VERIFY_CHECK(r->limbs <= 2*NUM_LIMBS); mpn_copyi(r->data, tmp, r->limbs); r->neg = a->neg ^ b->neg; - memset(tmp, 0, sizeof(tmp)); + memclear(tmp, sizeof(tmp)); } static void secp256k1_num_shift(secp256k1_num *r, int bits) { diff --git a/src/secp256k1.c b/src/secp256k1.c index c10ac4cda5..9d8d235065 100644 --- a/src/secp256k1.c +++ b/src/secp256k1.c @@ -456,7 +456,7 @@ static int nonce_function_rfc6979(unsigned char *nonce32, const unsigned char *m buffer_append(keydata, &offset, algo16, 16); } secp256k1_rfc6979_hmac_sha256_initialize(&rng, keydata, offset); - memset(keydata, 0, sizeof(keydata)); + memclear(keydata, sizeof(keydata)); for (i = 0; i <= counter; i++) { secp256k1_rfc6979_hmac_sha256_generate(&rng, nonce32, 32); } @@ -508,7 +508,7 @@ int secp256k1_ecdsa_sign(const secp256k1_context* ctx, secp256k1_ecdsa_signature } count++; } - memset(nonce32, 0, 32); + memclear(nonce32, sizeof(nonce32)); secp256k1_scalar_clear(&msg); secp256k1_scalar_clear(&non); secp256k1_scalar_clear(&sec); From dda8737db2c34d53b7eee1a036923ab1cb192e36 Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Sat, 8 Jun 2019 11:29:49 +0200 Subject: [PATCH 7/9] Implement various _clear() functions with secp256k1_memclear() --- src/field_10x26_impl.h | 11 ----------- src/field_5x52_impl.h | 11 ----------- src/field_impl.h | 4 ++++ src/group_impl.h | 9 ++------- src/scalar_4x64_impl.h | 7 ------- src/scalar_8x32_impl.h | 11 ----------- src/scalar_impl.h | 4 ++++ src/scalar_low_impl.h | 1 - 8 files changed, 10 insertions(+), 48 deletions(-) diff --git a/src/field_10x26_impl.h b/src/field_10x26_impl.h index ac9abe79ee..947aa7fe09 100644 --- a/src/field_10x26_impl.h +++ b/src/field_10x26_impl.h @@ -289,17 +289,6 @@ SECP256K1_INLINE static int secp256k1_fe_is_odd(const secp256k1_fe *a) { return a->n[0] & 1; } -SECP256K1_INLINE static void secp256k1_fe_clear(secp256k1_fe *a) { - int i; -#ifdef VERIFY - a->magnitude = 0; - a->normalized = 0; -#endif - for (i=0; i<10; i++) { - a->n[i] = 0; - } -} - static int secp256k1_fe_cmp_var(const secp256k1_fe *a, const secp256k1_fe *b) { int i; #ifdef VERIFY diff --git a/src/field_5x52_impl.h b/src/field_5x52_impl.h index e837984b3f..f118ab92d4 100644 --- a/src/field_5x52_impl.h +++ b/src/field_5x52_impl.h @@ -252,17 +252,6 @@ SECP256K1_INLINE static int secp256k1_fe_is_odd(const secp256k1_fe *a) { return a->n[0] & 1; } -SECP256K1_INLINE static void secp256k1_fe_clear(secp256k1_fe *a) { - int i; -#ifdef VERIFY - a->magnitude = 0; - a->normalized = 0; -#endif - for (i=0; i<5; i++) { - a->n[i] = 0; - } -} - static int secp256k1_fe_cmp_var(const secp256k1_fe *a, const secp256k1_fe *b) { int i; #ifdef VERIFY diff --git a/src/field_impl.h b/src/field_impl.h index 485921a60e..688fea6768 100644 --- a/src/field_impl.h +++ b/src/field_impl.h @@ -36,6 +36,10 @@ SECP256K1_INLINE static int secp256k1_fe_equal_var(const secp256k1_fe *a, const return secp256k1_fe_normalizes_to_zero_var(&na); } +SECP256K1_INLINE static void secp256k1_fe_clear(secp256k1_fe *a) { + memclear(a, sizeof(secp256k1_fe)); +} + static int secp256k1_fe_sqrt(secp256k1_fe *r, const secp256k1_fe *a) { /** Given that p is congruent to 3 mod 4, we can compute the square root of * a mod p as the (p+1)/4'th power of a. diff --git a/src/group_impl.h b/src/group_impl.h index 169f8e15de..d5782967d7 100644 --- a/src/group_impl.h +++ b/src/group_impl.h @@ -206,16 +206,11 @@ static void secp256k1_ge_set_infinity(secp256k1_ge *r) { } static void secp256k1_gej_clear(secp256k1_gej *r) { - r->infinity = 0; - secp256k1_fe_clear(&r->x); - secp256k1_fe_clear(&r->y); - secp256k1_fe_clear(&r->z); + memclear(r, sizeof(secp256k1_gej)); } static void secp256k1_ge_clear(secp256k1_ge *r) { - r->infinity = 0; - secp256k1_fe_clear(&r->x); - secp256k1_fe_clear(&r->y); + memclear(r, sizeof(secp256k1_ge)); } static int secp256k1_ge_set_xquad(secp256k1_ge *r, const secp256k1_fe *x) { diff --git a/src/scalar_4x64_impl.h b/src/scalar_4x64_impl.h index 2d81006c00..b4abe44e6f 100644 --- a/src/scalar_4x64_impl.h +++ b/src/scalar_4x64_impl.h @@ -24,13 +24,6 @@ #define SECP256K1_N_H_2 ((uint64_t)0xFFFFFFFFFFFFFFFFULL) #define SECP256K1_N_H_3 ((uint64_t)0x7FFFFFFFFFFFFFFFULL) -SECP256K1_INLINE static void secp256k1_scalar_clear(secp256k1_scalar *r) { - r->d[0] = 0; - r->d[1] = 0; - r->d[2] = 0; - r->d[3] = 0; -} - SECP256K1_INLINE static void secp256k1_scalar_set_int(secp256k1_scalar *r, unsigned int v) { r->d[0] = v; r->d[1] = 0; diff --git a/src/scalar_8x32_impl.h b/src/scalar_8x32_impl.h index f5042891f3..b4bbea4310 100644 --- a/src/scalar_8x32_impl.h +++ b/src/scalar_8x32_impl.h @@ -34,17 +34,6 @@ #define SECP256K1_N_H_6 ((uint32_t)0xFFFFFFFFUL) #define SECP256K1_N_H_7 ((uint32_t)0x7FFFFFFFUL) -SECP256K1_INLINE static void secp256k1_scalar_clear(secp256k1_scalar *r) { - r->d[0] = 0; - r->d[1] = 0; - r->d[2] = 0; - r->d[3] = 0; - r->d[4] = 0; - r->d[5] = 0; - r->d[6] = 0; - r->d[7] = 0; -} - SECP256K1_INLINE static void secp256k1_scalar_set_int(secp256k1_scalar *r, unsigned int v) { r->d[0] = v; r->d[1] = 0; diff --git a/src/scalar_impl.h b/src/scalar_impl.h index c9b38f3c7c..2be9ffcc65 100644 --- a/src/scalar_impl.h +++ b/src/scalar_impl.h @@ -27,6 +27,10 @@ static const secp256k1_scalar secp256k1_scalar_one = SECP256K1_SCALAR_CONST(0, 0, 0, 0, 0, 0, 0, 1); static const secp256k1_scalar secp256k1_scalar_zero = SECP256K1_SCALAR_CONST(0, 0, 0, 0, 0, 0, 0, 0); +SECP256K1_INLINE static void secp256k1_scalar_clear(secp256k1_scalar *r) { + memclear(r, sizeof(secp256k1_scalar)); +} + #ifndef USE_NUM_NONE static void secp256k1_scalar_get_num(secp256k1_num *r, const secp256k1_scalar *a) { unsigned char c[32]; diff --git a/src/scalar_low_impl.h b/src/scalar_low_impl.h index ad81f378bf..739f6a4aba 100644 --- a/src/scalar_low_impl.h +++ b/src/scalar_low_impl.h @@ -15,7 +15,6 @@ SECP256K1_INLINE static int secp256k1_scalar_is_even(const secp256k1_scalar *a) return !(*a & 1); } -SECP256K1_INLINE static void secp256k1_scalar_clear(secp256k1_scalar *r) { *r = 0; } SECP256K1_INLINE static void secp256k1_scalar_set_int(secp256k1_scalar *r, unsigned int v) { *r = v; } SECP256K1_INLINE static unsigned int secp256k1_scalar_get_bits(const secp256k1_scalar *a, unsigned int offset, unsigned int count) { From 244c749a0af7751fe37a78da8461b0e61b2414c0 Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Sat, 8 Jun 2019 13:21:52 +0200 Subject: [PATCH 8/9] Don't rely on memset to set signed integers to 0 --- src/ecmult_impl.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/ecmult_impl.h b/src/ecmult_impl.h index 419a03d849..2de6772bc2 100644 --- a/src/ecmult_impl.h +++ b/src/ecmult_impl.h @@ -406,7 +406,9 @@ static int secp256k1_ecmult_wnaf(int *wnaf, int len, const secp256k1_scalar *a, VERIFY_CHECK(a != NULL); VERIFY_CHECK(2 <= w && w <= 31); - memset(wnaf, 0, len * sizeof(wnaf[0])); + for (bit = 0; bit < len; bit++) { + wnaf[bit] = 0; + } s = *a; if (secp256k1_scalar_get_bits(&s, 255, 1)) { @@ -414,6 +416,7 @@ static int secp256k1_ecmult_wnaf(int *wnaf, int len, const secp256k1_scalar *a, sign = -1; } + bit = 0; while (bit < len) { int now; int word; From 6a34c60455d23fe494e4c61baf59ab0e7d755aee Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Mon, 27 Apr 2020 18:55:36 +0200 Subject: [PATCH 9/9] Introduce separate _clean functions for hash module This gives the caller more control about whether the state should be cleaned (= should be considered secret), which will be useful for example for Schnorr signature verification in the future. Moreover, it gives the caller the possibility to clean a hash struct without finalizing it. --- src/ecmult_gen_impl.h | 1 + src/hash.h | 3 +++ src/hash_impl.h | 14 ++++++++++++-- src/modules/ecdh/main_impl.h | 1 + src/secp256k1.c | 4 +++- 5 files changed, 20 insertions(+), 3 deletions(-) diff --git a/src/ecmult_gen_impl.h b/src/ecmult_gen_impl.h index d48b7fa27e..d3eba05f2c 100644 --- a/src/ecmult_gen_impl.h +++ b/src/ecmult_gen_impl.h @@ -204,6 +204,7 @@ static void secp256k1_ecmult_gen_blind(secp256k1_ecmult_gen_context *ctx, const memclear(nonce32, sizeof(nonce32)); secp256k1_scalar_clear(&b); secp256k1_gej_clear(&gb); + secp256k1_rfc6979_hmac_sha256_clear(&rng); } #endif /* SECP256K1_ECMULT_GEN_IMPL_H */ diff --git a/src/hash.h b/src/hash.h index de26e4b89f..9725757d73 100644 --- a/src/hash.h +++ b/src/hash.h @@ -19,6 +19,7 @@ typedef struct { static void secp256k1_sha256_initialize(secp256k1_sha256 *hash); static void secp256k1_sha256_write(secp256k1_sha256 *hash, const unsigned char *data, size_t size); static void secp256k1_sha256_finalize(secp256k1_sha256 *hash, unsigned char *out32); +static void secp256k1_sha256_clear(secp256k1_sha256 *hash); typedef struct { secp256k1_sha256 inner, outer; @@ -27,6 +28,7 @@ typedef struct { static void secp256k1_hmac_sha256_initialize(secp256k1_hmac_sha256 *hash, const unsigned char *key, size_t size); static void secp256k1_hmac_sha256_write(secp256k1_hmac_sha256 *hash, const unsigned char *data, size_t size); static void secp256k1_hmac_sha256_finalize(secp256k1_hmac_sha256 *hash, unsigned char *out32); +static void secp256k1_hmac_sha256_clear(secp256k1_hmac_sha256 *hash); typedef struct { unsigned char v[32]; @@ -37,5 +39,6 @@ typedef struct { static void secp256k1_rfc6979_hmac_sha256_initialize(secp256k1_rfc6979_hmac_sha256 *rng, const unsigned char *key, size_t keylen); static void secp256k1_rfc6979_hmac_sha256_generate(secp256k1_rfc6979_hmac_sha256 *rng, unsigned char *out, size_t outlen); static void secp256k1_rfc6979_hmac_sha256_finalize(secp256k1_rfc6979_hmac_sha256 *rng); +static void secp256k1_rfc6979_hmac_sha256_clear(secp256k1_rfc6979_hmac_sha256 *rng); #endif /* SECP256K1_HASH_H */ diff --git a/src/hash_impl.h b/src/hash_impl.h index b235a58982..0f77dc4953 100644 --- a/src/hash_impl.h +++ b/src/hash_impl.h @@ -164,7 +164,10 @@ static void secp256k1_sha256_finalize(secp256k1_sha256 *hash, unsigned char *out memclear(sizedesc, sizeof(sizedesc)); memclear(out, sizeof(out)); - memclear(hash, sizeof(secp256k1_sha256)); +} + +static void secp256k1_sha256_clear(secp256k1_sha256 *hash) { + memclear(hash, sizeof(*hash)); } static void secp256k1_hmac_sha256_initialize(secp256k1_hmac_sha256 *hash, const unsigned char *key, size_t keylen) { @@ -207,6 +210,9 @@ static void secp256k1_hmac_sha256_finalize(secp256k1_hmac_sha256 *hash, unsigned secp256k1_sha256_finalize(&hash->outer, out32); } +static void secp256k1_hmac_sha256_clear(secp256k1_hmac_sha256 *hash) { + memclear(hash, sizeof(*hash)); +} static void secp256k1_rfc6979_hmac_sha256_initialize(secp256k1_rfc6979_hmac_sha256 *rng, const unsigned char *key, size_t keylen) { secp256k1_hmac_sha256 hmac; @@ -270,7 +276,11 @@ static void secp256k1_rfc6979_hmac_sha256_generate(secp256k1_rfc6979_hmac_sha256 } static void secp256k1_rfc6979_hmac_sha256_finalize(secp256k1_rfc6979_hmac_sha256 *rng) { - memclear(rng, sizeof(secp256k1_rfc6979_hmac_sha256)); + (void) rng; +} + +static void secp256k1_rfc6979_hmac_sha256_clear(secp256k1_rfc6979_hmac_sha256 *rng) { + memclear(rng, sizeof(*rng)); } #undef BE32 diff --git a/src/modules/ecdh/main_impl.h b/src/modules/ecdh/main_impl.h index 6ce5157059..ad148d7c1c 100644 --- a/src/modules/ecdh/main_impl.h +++ b/src/modules/ecdh/main_impl.h @@ -19,6 +19,7 @@ static int ecdh_hash_function_sha256(unsigned char *output, const unsigned char secp256k1_sha256_write(&sha, &version, 1); secp256k1_sha256_write(&sha, x32, 32); secp256k1_sha256_finalize(&sha, output); + secp256k1_sha256_clear(&sha); return 1; } diff --git a/src/secp256k1.c b/src/secp256k1.c index 9d8d235065..8f450553a6 100644 --- a/src/secp256k1.c +++ b/src/secp256k1.c @@ -456,11 +456,13 @@ static int nonce_function_rfc6979(unsigned char *nonce32, const unsigned char *m buffer_append(keydata, &offset, algo16, 16); } secp256k1_rfc6979_hmac_sha256_initialize(&rng, keydata, offset); - memclear(keydata, sizeof(keydata)); for (i = 0; i <= counter; i++) { secp256k1_rfc6979_hmac_sha256_generate(&rng, nonce32, 32); } secp256k1_rfc6979_hmac_sha256_finalize(&rng); + + memclear(keydata, sizeof(keydata)); + secp256k1_rfc6979_hmac_sha256_clear(&rng); return 1; }