From 9bb368d146683aa9b44645f1b75a0ed230df97e9 Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Sat, 8 Jun 2019 13:54:14 +0200 Subject: [PATCH] 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 2fbf623c..6903cd1b 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 89f75ace..49393c67 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 82b082a9..9286f70f 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 b54ec08a..78e85004 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 2715b09d..e22d1a54 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 57f7eadd..783714fe 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 93b7b459..f4e83dc2 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 c2add8ac..2605141d 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; }