From e7d384488e85650cd2a29493bc55fa5e68bf59c3 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 07d8477eb0..8f58455209 100644 --- a/src/ecmult_impl.h +++ b/src/ecmult_impl.h @@ -660,7 +660,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; secp256k1_gej_set_infinity(r); @@ -708,18 +707,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 | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/src/util.h b/src/util.h index 49af867a46..c2add8acf9 100644 --- a/src/util.h +++ b/src/util.h @@ -8,11 +8,17 @@ #define SECP256K1_UTIL_H #include "../include/secp256k1.h" +#include "checkmem.h" +#include #include #include #include #include +#if defined(_MSC_VER) +/* For SecureZeroMemory */ +#include +#endif #define STR_(x) #x #define STR(x) STR_(x) @@ -221,6 +227,34 @@ static SECP256K1_INLINE void secp256k1_memczero(void *s, size_t len, int flag) { } } +/* Cleanses memory to prevent leaking sensitive info. Won't be optimized out. */ +static SECP256K1_INLINE void secp256k1_memclear(void *ptr, size_t len) { +#if defined(_MSC_VER) + /* SecureZeroMemory is guaranteed not to be optimized out by MSVC. */ + SecureZeroMemory(ptr, len); +#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 is 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 +#ifdef VERIFY + SECP256K1_CHECKMEM_UNDEFINE(ptr, len); +#endif +} + /** Semantics like memcmp. Variable-time. * * We use this to avoid possible compiler bugs with memcmp, e.g. From d79a6ccd43ae1681280e5e27d9cd181285bf8540 Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Wed, 12 Jun 2019 16:05:06 +0200 Subject: [PATCH 3/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/field.h | 6 +----- src/field_impl.h | 2 +- src/group_impl.h | 10 +++++----- src/testutil.h | 2 +- 4 files changed, 8 insertions(+), 12 deletions(-) diff --git a/src/field.h b/src/field.h index 8c65a3aff6..9599030991 100644 --- a/src/field.h +++ b/src/field.h @@ -144,11 +144,7 @@ static int secp256k1_fe_normalizes_to_zero_var(const secp256k1_fe *r); */ static void secp256k1_fe_set_int(secp256k1_fe *r, int a); -/** Set a field element to 0. - * - * On input, a does not need to be initialized. - * On output, a represents 0, is normalized and has magnitude 0. - */ +/** Clear a field element to prevent leaking sensitive information. */ static void secp256k1_fe_clear(secp256k1_fe *a); /** Determine whether a represents field element 0. diff --git a/src/field_impl.h b/src/field_impl.h index 989e9cdb2f..8cf6f22ce4 100644 --- a/src/field_impl.h +++ b/src/field_impl.h @@ -235,7 +235,7 @@ SECP256K1_INLINE static void secp256k1_fe_add_int(secp256k1_fe *r, int a) { static void secp256k1_fe_impl_clear(secp256k1_fe *a); SECP256K1_INLINE static void secp256k1_fe_clear(secp256k1_fe *a) { a->magnitude = 0; - a->normalized = 1; + a->normalized = 0; secp256k1_fe_impl_clear(a); SECP256K1_FE_VERIFY(a); diff --git a/src/group_impl.h b/src/group_impl.h index 2e096f4147..10aff43373 100644 --- a/src/group_impl.h +++ b/src/group_impl.h @@ -283,17 +283,17 @@ static void secp256k1_ge_table_set_globalz(size_t len, secp256k1_ge *a, const se 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); SECP256K1_GEJ_VERIFY(r); } 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); SECP256K1_GE_VERIFY(r); } diff --git a/src/testutil.h b/src/testutil.h index fc56854dd3..64b3bb41c0 100644 --- a/src/testutil.h +++ b/src/testutil.h @@ -34,7 +34,7 @@ static void testutil_random_fe_magnitude(secp256k1_fe *fe, int m) { if (n == 0) { return; } - secp256k1_fe_clear(&zero); + secp256k1_fe_set_int(&zero, 0); secp256k1_fe_negate(&zero, &zero, 0); secp256k1_fe_mul_int_unchecked(&zero, n - 1); secp256k1_fe_add(fe, &zero); From e3497bbf001f8e06dcc21d27fed173be77ef7136 Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Sat, 8 Jun 2019 11:56:15 +0200 Subject: [PATCH 4/9] Separate between clearing memory and setting to zero in tests Co-Authored-By: isle2983 Co-Authored-By: Pieter Wuille --- src/bench_ecmult.c | 2 +- src/tests.c | 13 ++++++------- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/src/bench_ecmult.c b/src/bench_ecmult.c index 7dc52ad87b..3974af75f4 100644 --- a/src/bench_ecmult.c +++ b/src/bench_ecmult.c @@ -71,7 +71,7 @@ static void bench_ecmult_teardown_helper(bench_data* data, size_t* seckey_offset secp256k1_scalar sum_scalars; secp256k1_gej_set_infinity(&sum_output); - secp256k1_scalar_clear(&sum_scalars); + secp256k1_scalar_set_int(&sum_scalars, 0); for (i = 0; i < iters; ++i) { secp256k1_gej_add_var(&sum_output, &sum_output, &data->output[i], NULL); if (scalar_gen_offset != NULL) { diff --git a/src/tests.c b/src/tests.c index dba4097b1a..270b1c83ec 100644 --- a/src/tests.c +++ b/src/tests.c @@ -3671,8 +3671,7 @@ static void test_ge(void) { secp256k1_fe zfi2, zfi3; secp256k1_gej_set_infinity(&gej[0]); - secp256k1_ge_clear(&ge[0]); - secp256k1_ge_set_gej_var(&ge[0], &gej[0]); + secp256k1_ge_set_infinity(&ge[0]); for (i = 0; i < runs; i++) { int j, k; secp256k1_ge g; @@ -4797,12 +4796,12 @@ static void test_ecmult_multi(secp256k1_scratch *scratch, secp256k1_ecmult_multi testutil_random_ge_test(&pt[ncount]); } - secp256k1_scalar_clear(&sc[0]); + secp256k1_scalar_set_int(&sc[0], 0); CHECK(ecmult_multi(&CTX->error_callback, scratch, &r, &secp256k1_scalar_zero, 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, scratch, &r, &secp256k1_scalar_zero, ecmult_multi_callback, &data, 6)); CHECK(ecmult_multi(&CTX->error_callback, scratch, &r, &secp256k1_scalar_zero, ecmult_multi_callback, &data, 5)); CHECK(secp256k1_gej_is_infinity(&r)); From 9bb368d146683aa9b44645f1b75a0ed230df97e9 Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Sat, 8 Jun 2019 13:54:14 +0200 Subject: [PATCH 5/9] Use secp256k1_memclear() to clear stack memory instead of memset() All of the invocations of secp256k1_memclear() 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 | 8 ++++---- src/hash_impl.h | 11 ++++++----- src/modules/ecdh/main_impl.h | 5 +++-- src/modules/ellswift/main_impl.h | 2 +- src/modules/musig/session_impl.h | 10 +++++----- src/modules/schnorrsig/main_impl.h | 2 +- src/secp256k1.c | 4 ++-- src/util.h | 2 +- 8 files changed, 23 insertions(+), 21 deletions(-) diff --git a/src/ecmult_gen_impl.h b/src/ecmult_gen_impl.h index 2fbf623ca3..6903cd1bb8 100644 --- a/src/ecmult_gen_impl.h +++ b/src/ecmult_gen_impl.h @@ -277,8 +277,8 @@ static void secp256k1_ecmult_gen(const secp256k1_ecmult_gen_context *ctx, secp25 /* Cleanup. */ secp256k1_fe_clear(&neg); secp256k1_ge_clear(&add); - memset(&adds, 0, sizeof(adds)); - memset(&recoded, 0, sizeof(recoded)); + secp256k1_memclear(&adds, sizeof(adds)); + secp256k1_memclear(&recoded, sizeof(recoded)); } /* Setup blinding values for secp256k1_ecmult_gen. */ @@ -310,7 +310,7 @@ static void secp256k1_ecmult_gen_blind(secp256k1_ecmult_gen_context *ctx, const VERIFY_CHECK(seed32 != NULL); memcpy(keydata + 32, seed32, 32); secp256k1_rfc6979_hmac_sha256_initialize(&rng, keydata, 64); - memset(keydata, 0, sizeof(keydata)); + secp256k1_memclear(keydata, sizeof(keydata)); /* Compute projective blinding factor (cannot be 0). */ secp256k1_rfc6979_hmac_sha256_generate(&rng, nonce32, 32); @@ -325,13 +325,13 @@ static void secp256k1_ecmult_gen_blind(secp256k1_ecmult_gen_context *ctx, const * which secp256k1_gej_add_ge cannot handle. */ 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); secp256k1_scalar_add(&ctx->scalar_offset, &b, &diff); secp256k1_ge_set_gej(&ctx->ge_offset, &gb); /* Clean up. */ + secp256k1_memclear(nonce32, sizeof(nonce32)); secp256k1_scalar_clear(&b); secp256k1_gej_clear(&gb); secp256k1_fe_clear(&f); diff --git a/src/hash_impl.h b/src/hash_impl.h index 89f75ace74..49393c6750 100644 --- a/src/hash_impl.h +++ b/src/hash_impl.h @@ -156,6 +156,9 @@ static void secp256k1_sha256_finalize(secp256k1_sha256 *hash, unsigned char *out secp256k1_write_be32(&out32[4*i], hash->s[i]); hash->s[i] = 0; } + + secp256k1_memclear(sizedesc, sizeof(sizedesc)); + secp256k1_memclear(hash, sizeof(secp256k1_sha256)); } /* Initializes a sha256 struct and writes the 64 byte string @@ -196,7 +199,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)); + secp256k1_memclear(rkey, sizeof(rkey)); } static void secp256k1_hmac_sha256_write(secp256k1_hmac_sha256 *hash, const unsigned char *data, size_t size) { @@ -207,7 +210,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); + secp256k1_memclear(temp, sizeof(temp)); secp256k1_sha256_finalize(&hash->outer, out32); } @@ -274,9 +277,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; + secp256k1_memclear(rng, sizeof(secp256k1_rfc6979_hmac_sha256)); } #undef Round diff --git a/src/modules/ecdh/main_impl.h b/src/modules/ecdh/main_impl.h index 82b082a9f0..9286f70f35 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); + secp256k1_memclear(x, sizeof(x)); + secp256k1_memclear(y, sizeof(y)); secp256k1_scalar_clear(&s); + secp256k1_ge_clear(&pt); return !!ret & !overflow; } diff --git a/src/modules/ellswift/main_impl.h b/src/modules/ellswift/main_impl.h index b54ec08a22..78e850048a 100644 --- a/src/modules/ellswift/main_impl.h +++ b/src/modules/ellswift/main_impl.h @@ -580,7 +580,7 @@ int secp256k1_ellswift_xdh(const secp256k1_context *ctx, unsigned char *output, /* Invoke hasher */ ret = hashfp(output, sx, ell_a64, ell_b64, data); - memset(sx, 0, 32); + secp256k1_memclear(sx, sizeof(sx)); secp256k1_fe_clear(&px); secp256k1_scalar_clear(&s); diff --git a/src/modules/musig/session_impl.h b/src/modules/musig/session_impl.h index 2715b09d57..e22d1a5451 100644 --- a/src/modules/musig/session_impl.h +++ b/src/modules/musig/session_impl.h @@ -385,11 +385,11 @@ static void secp256k1_nonce_function_musig(secp256k1_scalar *k, const unsigned c secp256k1_scalar_set_b32(&k[i], buf, NULL); /* Attempt to erase secret data */ - memset(buf, 0, sizeof(buf)); - memset(&sha_tmp, 0, sizeof(sha_tmp)); + secp256k1_memclear(buf, sizeof(buf)); + secp256k1_memclear(&sha_tmp, sizeof(sha_tmp)); } - memset(rand, 0, sizeof(rand)); - memset(&sha, 0, sizeof(sha)); + secp256k1_memclear(rand, sizeof(rand)); + secp256k1_memclear(&sha, sizeof(sha)); } int secp256k1_musig_nonce_gen_internal(const secp256k1_context* ctx, secp256k1_musig_secnonce *secnonce, secp256k1_musig_pubnonce *pubnonce, const unsigned char *input_nonce, const unsigned char *seckey, const secp256k1_pubkey *pubkey, const unsigned char *msg32, const secp256k1_musig_keyagg_cache *keyagg_cache, const unsigned char *extra_input32) { @@ -509,7 +509,7 @@ int secp256k1_musig_nonce_gen_counter(const secp256k1_context* ctx, secp256k1_mu if (!secp256k1_musig_nonce_gen_internal(ctx, secnonce, pubnonce, buf, seckey, &pubkey, msg32, keyagg_cache, extra_input32)) { return 0; } - memset(seckey, 0, sizeof(seckey)); + secp256k1_memclear(seckey, sizeof(seckey)); return 1; } diff --git a/src/modules/schnorrsig/main_impl.h b/src/modules/schnorrsig/main_impl.h index 57f7eadd3c..783714fec7 100644 --- a/src/modules/schnorrsig/main_impl.h +++ b/src/modules/schnorrsig/main_impl.h @@ -187,7 +187,7 @@ static int secp256k1_schnorrsig_sign_internal(const secp256k1_context* ctx, unsi secp256k1_memczero(sig64, 64, !ret); secp256k1_scalar_clear(&k); secp256k1_scalar_clear(&sk); - memset(seckey, 0, sizeof(seckey)); + secp256k1_memclear(seckey, sizeof(seckey)); return ret; } diff --git a/src/secp256k1.c b/src/secp256k1.c index 93b7b459a3..f4e83dc2cc 100644 --- a/src/secp256k1.c +++ b/src/secp256k1.c @@ -494,7 +494,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)); + secp256k1_memclear(keydata, sizeof(keydata)); for (i = 0; i <= counter; i++) { secp256k1_rfc6979_hmac_sha256_generate(&rng, nonce32, 32); } @@ -548,7 +548,7 @@ static int secp256k1_ecdsa_sign_inner(const secp256k1_context* ctx, secp256k1_sc * seckey. As a result is_sec_valid is included in ret only after ret was * used as a branching variable. */ ret &= is_sec_valid; - memset(nonce32, 0, 32); + secp256k1_memclear(nonce32, sizeof(nonce32)); secp256k1_scalar_clear(&msg); secp256k1_scalar_clear(&non); secp256k1_scalar_clear(&sec); diff --git a/src/util.h b/src/util.h index c2add8acf9..2605141d05 100644 --- a/src/util.h +++ b/src/util.h @@ -285,7 +285,7 @@ static SECP256K1_INLINE int secp256k1_is_zero_array(const unsigned char *s, size } ret = (acc == 0); /* acc may contain secret values. Try to explicitly clear it. */ - acc = 0; + secp256k1_memclear(&acc, sizeof(acc)); return ret; } From 97c57f42ba8aca51479d23874b7608b9500a5bc9 Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Sat, 8 Jun 2019 11:29:49 +0200 Subject: [PATCH 6/9] Implement various _clear() functions with secp256k1_memclear() --- src/field.h | 1 - src/field_10x26_impl.h | 7 ------- src/field_5x52_impl.h | 7 ------- src/field_impl.h | 13 ++++--------- src/group_impl.h | 13 ++----------- src/scalar_4x64_impl.h | 7 ------- src/scalar_8x32_impl.h | 11 ----------- src/scalar_impl.h | 4 ++++ src/scalar_low_impl.h | 2 -- 9 files changed, 10 insertions(+), 55 deletions(-) diff --git a/src/field.h b/src/field.h index 9599030991..1f6ba7460f 100644 --- a/src/field.h +++ b/src/field.h @@ -81,7 +81,6 @@ static const secp256k1_fe secp256k1_const_beta = SECP256K1_FE_CONST( # define secp256k1_fe_normalizes_to_zero secp256k1_fe_impl_normalizes_to_zero # define secp256k1_fe_normalizes_to_zero_var secp256k1_fe_impl_normalizes_to_zero_var # define secp256k1_fe_set_int secp256k1_fe_impl_set_int -# define secp256k1_fe_clear secp256k1_fe_impl_clear # define secp256k1_fe_is_zero secp256k1_fe_impl_is_zero # define secp256k1_fe_is_odd secp256k1_fe_impl_is_odd # define secp256k1_fe_cmp_var secp256k1_fe_impl_cmp_var diff --git a/src/field_10x26_impl.h b/src/field_10x26_impl.h index 666068c712..ea14c27318 100644 --- a/src/field_10x26_impl.h +++ b/src/field_10x26_impl.h @@ -270,13 +270,6 @@ SECP256K1_INLINE static int secp256k1_fe_impl_is_odd(const secp256k1_fe *a) { return a->n[0] & 1; } -SECP256K1_INLINE static void secp256k1_fe_impl_clear(secp256k1_fe *a) { - int i; - for (i=0; i<10; i++) { - a->n[i] = 0; - } -} - static int secp256k1_fe_impl_cmp_var(const secp256k1_fe *a, const secp256k1_fe *b) { int i; for (i = 9; i >= 0; i--) { diff --git a/src/field_5x52_impl.h b/src/field_5x52_impl.h index 76031f755e..46dca6b981 100644 --- a/src/field_5x52_impl.h +++ b/src/field_5x52_impl.h @@ -212,13 +212,6 @@ SECP256K1_INLINE static int secp256k1_fe_impl_is_odd(const secp256k1_fe *a) { return a->n[0] & 1; } -SECP256K1_INLINE static void secp256k1_fe_impl_clear(secp256k1_fe *a) { - int i; - for (i=0; i<5; i++) { - a->n[i] = 0; - } -} - static int secp256k1_fe_impl_cmp_var(const secp256k1_fe *a, const secp256k1_fe *b) { int i; for (i = 4; i >= 0; i--) { diff --git a/src/field_impl.h b/src/field_impl.h index 8cf6f22ce4..896507a3a4 100644 --- a/src/field_impl.h +++ b/src/field_impl.h @@ -18,6 +18,10 @@ #error "Please select wide multiplication implementation" #endif +SECP256K1_INLINE static void secp256k1_fe_clear(secp256k1_fe *a) { + secp256k1_memclear(a, sizeof(secp256k1_fe)); +} + SECP256K1_INLINE static int secp256k1_fe_equal(const secp256k1_fe *a, const secp256k1_fe *b) { secp256k1_fe na; SECP256K1_FE_VERIFY(a); @@ -232,15 +236,6 @@ SECP256K1_INLINE static void secp256k1_fe_add_int(secp256k1_fe *r, int a) { SECP256K1_FE_VERIFY(r); } -static void secp256k1_fe_impl_clear(secp256k1_fe *a); -SECP256K1_INLINE static void secp256k1_fe_clear(secp256k1_fe *a) { - a->magnitude = 0; - a->normalized = 0; - secp256k1_fe_impl_clear(a); - - SECP256K1_FE_VERIFY(a); -} - static int secp256k1_fe_impl_is_zero(const secp256k1_fe *a); SECP256K1_INLINE static int secp256k1_fe_is_zero(const secp256k1_fe *a) { SECP256K1_FE_VERIFY(a); diff --git a/src/group_impl.h b/src/group_impl.h index 10aff43373..c668fb2401 100644 --- a/src/group_impl.h +++ b/src/group_impl.h @@ -299,20 +299,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); - - SECP256K1_GEJ_VERIFY(r); + secp256k1_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); - - SECP256K1_GE_VERIFY(r); + secp256k1_memclear(r, sizeof(secp256k1_ge)); } static int secp256k1_ge_set_xo_var(secp256k1_ge *r, const secp256k1_fe *x, int odd) { diff --git a/src/scalar_4x64_impl.h b/src/scalar_4x64_impl.h index 4aaec85b99..807b9b70ab 100644 --- a/src/scalar_4x64_impl.h +++ b/src/scalar_4x64_impl.h @@ -29,13 +29,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 c7d87b17d8..2610496052 100644 --- a/src/scalar_8x32_impl.h +++ b/src/scalar_8x32_impl.h @@ -38,17 +38,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 972d8041b0..dbb5b0a01c 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) { + secp256k1_memclear(r, sizeof(secp256k1_scalar)); +} + static int secp256k1_scalar_set_b32_seckey(secp256k1_scalar *r, const unsigned char *bin) { int overflow; secp256k1_scalar_set_b32(r, bin, &overflow); diff --git a/src/scalar_low_impl.h b/src/scalar_low_impl.h index 45d2f3e460..84e1a380a3 100644 --- a/src/scalar_low_impl.h +++ b/src/scalar_low_impl.h @@ -19,8 +19,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 % EXHAUSTIVE_TEST_ORDER; From 99cc9fd6d01db1165d08b88d45c0de85a59b70cf Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Sat, 8 Jun 2019 13:21:52 +0200 Subject: [PATCH 7/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 8f58455209..0b53b3fcb9 100644 --- a/src/ecmult_impl.h +++ b/src/ecmult_impl.h @@ -171,7 +171,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_limb32(&s, 255, 1)) { @@ -179,6 +181,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 349e6ab916b3dd106fea9b56dacec065df5fa457 Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Mon, 27 Apr 2020 18:55:36 +0200 Subject: [PATCH 8/9] Introduce separate _clear functions for hash module This gives the caller more control about whether the state should be cleaned (= should be considered secret). 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 | 16 ++++++++++++---- src/modules/ecdh/main_impl.h | 1 + src/modules/ellswift/main_impl.h | 2 ++ src/modules/musig/session_impl.h | 4 ++-- src/modules/schnorrsig/main_impl.h | 1 + src/secp256k1.c | 5 ++++- 8 files changed, 26 insertions(+), 7 deletions(-) diff --git a/src/ecmult_gen_impl.h b/src/ecmult_gen_impl.h index 6903cd1bb8..070a121308 100644 --- a/src/ecmult_gen_impl.h +++ b/src/ecmult_gen_impl.h @@ -335,6 +335,7 @@ static void secp256k1_ecmult_gen_blind(secp256k1_ecmult_gen_context *ctx, const secp256k1_scalar_clear(&b); secp256k1_gej_clear(&gb); secp256k1_fe_clear(&f); + secp256k1_rfc6979_hmac_sha256_clear(&rng); } #endif /* SECP256K1_ECMULT_GEN_IMPL_H */ diff --git a/src/hash.h b/src/hash.h index 4e0384cfbf..6d903ca7e0 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 49393c6750..956e0ea48b 100644 --- a/src/hash_impl.h +++ b/src/hash_impl.h @@ -156,9 +156,6 @@ static void secp256k1_sha256_finalize(secp256k1_sha256 *hash, unsigned char *out secp256k1_write_be32(&out32[4*i], hash->s[i]); hash->s[i] = 0; } - - secp256k1_memclear(sizedesc, sizeof(sizedesc)); - secp256k1_memclear(hash, sizeof(secp256k1_sha256)); } /* Initializes a sha256 struct and writes the 64 byte string @@ -174,6 +171,10 @@ static void secp256k1_sha256_initialize_tagged(secp256k1_sha256 *hash, const uns secp256k1_sha256_write(hash, buf, 32); } +static void secp256k1_sha256_clear(secp256k1_sha256 *hash) { + secp256k1_memclear(hash, sizeof(*hash)); +} + static void secp256k1_hmac_sha256_initialize(secp256k1_hmac_sha256 *hash, const unsigned char *key, size_t keylen) { size_t n; unsigned char rkey[64]; @@ -214,6 +215,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) { + secp256k1_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; @@ -277,7 +281,11 @@ static void secp256k1_rfc6979_hmac_sha256_generate(secp256k1_rfc6979_hmac_sha256 } static void secp256k1_rfc6979_hmac_sha256_finalize(secp256k1_rfc6979_hmac_sha256 *rng) { - secp256k1_memclear(rng, sizeof(secp256k1_rfc6979_hmac_sha256)); + (void) rng; +} + +static void secp256k1_rfc6979_hmac_sha256_clear(secp256k1_rfc6979_hmac_sha256 *rng) { + secp256k1_memclear(rng, sizeof(*rng)); } #undef Round diff --git a/src/modules/ecdh/main_impl.h b/src/modules/ecdh/main_impl.h index 9286f70f35..a3dc18332b 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/modules/ellswift/main_impl.h b/src/modules/ellswift/main_impl.h index 78e850048a..745a969139 100644 --- a/src/modules/ellswift/main_impl.h +++ b/src/modules/ellswift/main_impl.h @@ -510,6 +510,7 @@ static int ellswift_xdh_hash_function_prefix(unsigned char *output, const unsign secp256k1_sha256_write(&sha, ell_b64, 64); secp256k1_sha256_write(&sha, x32, 32); secp256k1_sha256_finalize(&sha, output); + secp256k1_sha256_clear(&sha); return 1; } @@ -539,6 +540,7 @@ static int ellswift_xdh_hash_function_bip324(unsigned char* output, const unsign secp256k1_sha256_write(&sha, ell_b64, 64); secp256k1_sha256_write(&sha, x32, 32); secp256k1_sha256_finalize(&sha, output); + secp256k1_sha256_clear(&sha); return 1; } diff --git a/src/modules/musig/session_impl.h b/src/modules/musig/session_impl.h index e22d1a5451..d646ec11e0 100644 --- a/src/modules/musig/session_impl.h +++ b/src/modules/musig/session_impl.h @@ -386,10 +386,10 @@ static void secp256k1_nonce_function_musig(secp256k1_scalar *k, const unsigned c /* Attempt to erase secret data */ secp256k1_memclear(buf, sizeof(buf)); - secp256k1_memclear(&sha_tmp, sizeof(sha_tmp)); + secp256k1_sha256_clear(&sha_tmp); } secp256k1_memclear(rand, sizeof(rand)); - secp256k1_memclear(&sha, sizeof(sha)); + secp256k1_sha256_clear(&sha); } int secp256k1_musig_nonce_gen_internal(const secp256k1_context* ctx, secp256k1_musig_secnonce *secnonce, secp256k1_musig_pubnonce *pubnonce, const unsigned char *input_nonce, const unsigned char *seckey, const secp256k1_pubkey *pubkey, const unsigned char *msg32, const secp256k1_musig_keyagg_cache *keyagg_cache, const unsigned char *extra_input32) { diff --git a/src/modules/schnorrsig/main_impl.h b/src/modules/schnorrsig/main_impl.h index 783714fec7..261f4e4e27 100644 --- a/src/modules/schnorrsig/main_impl.h +++ b/src/modules/schnorrsig/main_impl.h @@ -93,6 +93,7 @@ static int nonce_function_bip340(unsigned char *nonce32, const unsigned char *ms secp256k1_sha256_write(&sha, xonly_pk32, 32); secp256k1_sha256_write(&sha, msg, msglen); secp256k1_sha256_finalize(&sha, nonce32); + secp256k1_sha256_clear(&sha); return 1; } diff --git a/src/secp256k1.c b/src/secp256k1.c index f4e83dc2cc..00c7285a0e 100644 --- a/src/secp256k1.c +++ b/src/secp256k1.c @@ -494,11 +494,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); - secp256k1_memclear(keydata, sizeof(keydata)); for (i = 0; i <= counter; i++) { secp256k1_rfc6979_hmac_sha256_generate(&rng, nonce32, 32); } secp256k1_rfc6979_hmac_sha256_finalize(&rng); + + secp256k1_memclear(keydata, sizeof(keydata)); + secp256k1_rfc6979_hmac_sha256_clear(&rng); return 1; } @@ -799,6 +801,7 @@ int secp256k1_tagged_sha256(const secp256k1_context* ctx, unsigned char *hash32, secp256k1_sha256_initialize_tagged(&sha, tag, taglen); secp256k1_sha256_write(&sha, msg, msglen); secp256k1_sha256_finalize(&sha, hash32); + secp256k1_sha256_clear(&sha); return 1; } From 765ef53335a3e0fafdafe1e757f6fe0789f2797f Mon Sep 17 00:00:00 2001 From: Sebastian Falbesoner Date: Fri, 11 Oct 2024 18:00:59 +0200 Subject: [PATCH 9/9] Clear _gej instances after point multiplication to avoid potential leaks Quoting sipa (see https://github.com/bitcoin-core/secp256k1/pull/1479#discussion_r1790079414): "When performing an EC multiplication A = aG for secret a, the resulting _affine_ coordinates of A are presumed to not leak information about a (ECDLP), but the same is not necessarily true for the Jacobian coordinates that come out of our multiplication algorithm." For the ECDH point multiplication result, the result in Jacobi coordinates should be cleared not only to avoid leaking the scalar, but even more so as it's a representation of the resulting shared secret. --- src/modules/ecdh/main_impl.h | 1 + src/modules/musig/session_impl.h | 1 + src/modules/schnorrsig/main_impl.h | 1 + src/secp256k1.c | 1 + 4 files changed, 4 insertions(+) diff --git a/src/modules/ecdh/main_impl.h b/src/modules/ecdh/main_impl.h index a3dc18332b..842b5359e3 100644 --- a/src/modules/ecdh/main_impl.h +++ b/src/modules/ecdh/main_impl.h @@ -66,6 +66,7 @@ int secp256k1_ecdh(const secp256k1_context* ctx, unsigned char *output, const se secp256k1_memclear(y, sizeof(y)); secp256k1_scalar_clear(&s); secp256k1_ge_clear(&pt); + secp256k1_gej_clear(&res); return !!ret & !overflow; } diff --git a/src/modules/musig/session_impl.h b/src/modules/musig/session_impl.h index d646ec11e0..2733e47d6c 100644 --- a/src/modules/musig/session_impl.h +++ b/src/modules/musig/session_impl.h @@ -450,6 +450,7 @@ int secp256k1_musig_nonce_gen_internal(const secp256k1_context* ctx, secp256k1_m secp256k1_ge_set_gej(&nonce_pts[i], &nonce_ptj); secp256k1_declassify(ctx, &nonce_pts[i], sizeof(nonce_pts)); secp256k1_scalar_clear(&k[i]); + secp256k1_gej_clear(&nonce_ptj); } /* None of the nonce_pts will be infinity because k != 0 with overwhelming * probability */ diff --git a/src/modules/schnorrsig/main_impl.h b/src/modules/schnorrsig/main_impl.h index 261f4e4e27..82bba2f597 100644 --- a/src/modules/schnorrsig/main_impl.h +++ b/src/modules/schnorrsig/main_impl.h @@ -189,6 +189,7 @@ static int secp256k1_schnorrsig_sign_internal(const secp256k1_context* ctx, unsi secp256k1_scalar_clear(&k); secp256k1_scalar_clear(&sk); secp256k1_memclear(seckey, sizeof(seckey)); + secp256k1_gej_clear(&rj); return ret; } diff --git a/src/secp256k1.c b/src/secp256k1.c index 00c7285a0e..a248519dfd 100644 --- a/src/secp256k1.c +++ b/src/secp256k1.c @@ -597,6 +597,7 @@ static int secp256k1_ec_pubkey_create_helper(const secp256k1_ecmult_gen_context secp256k1_ecmult_gen(ecmult_gen_ctx, &pj, seckey_scalar); secp256k1_ge_set_gej(p, &pj); + secp256k1_gej_clear(&pj); return ret; }