Merge bitcoin-core#1579: Clear sensitive memory without getting optimized out (revival of bitcoin-core#636)

765ef53 Clear _gej instances after point multiplication to avoid potential leaks (Sebastian Falbesoner)
349e6ab Introduce separate _clear functions for hash module (Tim Ruffing)
99cc9fd Don't rely on memset to set signed integers to 0 (Tim Ruffing)
97c57f4 Implement various _clear() functions with secp256k1_memclear() (Tim Ruffing)
9bb368d Use secp256k1_memclear() to clear stack memory instead of memset() (Tim Ruffing)
e3497bb Separate between clearing memory and setting to zero in tests (Tim Ruffing)
d79a6cc Separate secp256k1_fe_set_int( . , 0 ) from secp256k1_fe_clear() (Tim Ruffing)
1c08126 Add secp256k1_memclear() for clearing secret data (Tim Ruffing)
e7d3844 Don't clear secrets in pippenger implementation (Tim Ruffing)

Pull request description:

  This PR picks up bitcoin-core#636 (which in turn picked up bitcoin-core#448, so this is take number three) and is essentially a rebase on master.

  Some changes to the original PR:
  * the clearing function now has the `secp256k1_` prefix again, since the related helper `_memczero` got it as well (see PR bitcoin-core#835 / commit e89278f)
  * the original commit b17a7df ("Make _set_fe_int( . , 0 ) set magnitude to 0") is not needed anymore, since it was already applied in PR bitcoin-core#943 (commit d49011f)
  * clearing of stack memory with `secp256k1_memclear` is now also done on modules that have been newly introduced since then, i.e. schnorr and ellswift (of course, there is still no guarantee that all places where clearing is necessary are covered)

  So far I haven't looked at any disassembly and possible performance implications yet (there were some concerns expressed in bitcoin-core#636 (comment)), happy to go deeper there if this gets Concept ACKed.

  The proposed method of using a memory barrier to prevent optimizating away the memset is still used in BoringSSL (where it was originally picked up from) and in the Linux Kernel, see e.g. https://github.com/google/boringssl/blob/5af122c3dfc163b5d1859f1f450756e8e320a142/crypto/mem.c#L335 and https://github.com/torvalds/linux/blob/d4560686726f7a357922f300fc81f5964be8df04/include/linux/string.h#L348 / https://github.com/torvalds/linux/blob/d4560686726f7a357922f300fc81f5964be8df04/include/linux/compiler.h#L102

  Fixes bitcoin-core#185.

ACKs for top commit:
  sipa:
    reACK 765ef53
  real-or-random:
    ACK 765ef53

Tree-SHA512: 5a034d5ad14178c06928022459f3d4f0877d06f576b24ab07b86b3608b0b3e9273217b8309a1db606f024f3032731f13013114b1e0828964b578814d1efb2959

Author: real-or-random

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) {

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,16 +325,17 @@ 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);
+    secp256k1_rfc6979_hmac_sha256_clear(&rng);
 }
 
 #endif /* SECP256K1_ECMULT_GEN_IMPL_H */

src/ecmult_impl.h

@@ -171,14 +171,17 @@ 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)) {
         secp256k1_scalar_negate(&s, &s);
         sign = -1;
     }
 
+    bit = 0;
     while (bit < len) {
         int now;
         int word;
@@ -660,7 +663,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 +710,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<<bucket_window; i++) {
-        secp256k1_gej_clear(&buckets[i]);
-    }
     secp256k1_scratch_apply_checkpoint(error_callback, scratch, scratch_checkpoint);
     return 1;
 }

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
@@ -144,11 +143,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.

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--) {

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--) {

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 = 1;
-    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);

src/group_impl.h

@@ -283,36 +283,27 @@ 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);
 }
 
 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) {

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 */

src/hash_impl.h

@@ -171,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];
@@ -196,7 +200,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,10 +211,13 @@ 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);
 }
 
+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;
@@ -274,9 +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) {
-    memset(rng->k, 0, 32);
-    memset(rng->v, 0, 32);
-    rng->retry = 0;
+    (void) rng;
+}
+
+static void secp256k1_rfc6979_hmac_sha256_clear(secp256k1_rfc6979_hmac_sha256 *rng) {
+    secp256k1_memclear(rng, sizeof(*rng));
 }
 
 #undef Round

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;
 }
@@ -61,9 +62,11 @@ 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);
+    secp256k1_gej_clear(&res);
 
     return !!ret & !overflow;
 }

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;
 }
@@ -580,7 +582,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);
 

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_sha256_clear(&sha_tmp);
     }
-    memset(rand, 0, sizeof(rand));
-    memset(&sha, 0, sizeof(sha));
+    secp256k1_memclear(rand, sizeof(rand));
+    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) {
@@ -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[i]));
         secp256k1_scalar_clear(&k[i]);
+        secp256k1_gej_clear(&nonce_ptj);
     }
     /* None of the nonce_pts will be infinity because k != 0 with overwhelming
      * probability */
@@ -509,7 +510,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;
 }
 

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;
 }
 
@@ -187,7 +188,8 @@ 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));
+    secp256k1_gej_clear(&rj);
 
     return ret;
 }

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;