Use secp256k1_mem_clear() to clear stack memory instead of memset()

real-or-random · isle2983 · real-or-random · commit 6a62ca65cc42 · 2019-06-12T16:13:22.000+02:00
All of the invocations of secp256k1_mem_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_mem_clear() 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 &lt;isle2983@yahoo.com&gt;
diff --git 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;
+    secp256k1_mem_clear(&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));
+    secp256k1_mem_clear(keydata, sizeof(keydata));
     /* Retry for out of range results to achieve uniformity. */
     do {
         secp256k1_rfc6979_hmac_sha256_generate(&rng, nonce32, 32);
@@ -199,11 +199,12 @@ static void secp256k1_ecmult_gen_blind(secp256k1_ecmult_gen_context *ctx, const
         retry = retry || secp256k1_scalar_is_zero(&b);
     } while (retry); /* This branch true is cryptographically unreachable. Requires sha256_hmac output > order. */
     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;
+
+    secp256k1_mem_clear(nonce32, sizeof(nonce32));
     secp256k1_scalar_clear(&b);
     secp256k1_gej_clear(&gb);
 }
diff --git 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);
+
+    secp256k1_mem_clear(sizedesc, sizeof(sizedesc));
+    secp256k1_mem_clear(out, sizeof(out));
+    secp256k1_mem_clear(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));
+    secp256k1_mem_clear(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);
+    secp256k1_mem_clear(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;
+    secp256k1_mem_clear(rng, sizeof(secp256k1_rfc6979_hmac_sha256));
 }
 
 #undef BE32
diff --git a/src/modules/ecdh/main_impl.h b/src/modules/ecdh/main_impl.h
@@ -58,9 +58,13 @@ int secp256k1_ecdh(const secp256k1_context* ctx, unsigned char *output, const se
         secp256k1_fe_get_b32(y, &pt.y);
 
         ret = hashfp(output, x, y, data);
+        secp256k1_mem_clear(x, sizeof(x));
+        secp256k1_mem_clear(y, sizeof(y));
     }
 
     secp256k1_scalar_clear(&s);
+    secp256k1_ge_clear(&pt);
+
     return ret;
 }
 
diff --git 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);
+        secp256k1_mem_clear(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
@@ -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));
+    secp256k1_mem_clear(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));
+        secp256k1_mem_clear(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));
+    secp256k1_mem_clear(g, sizeof(g));
+    secp256k1_mem_clear(u, sizeof(u));
+    secp256k1_mem_clear(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));
+    secp256k1_mem_clear(tmp, sizeof(tmp));
 }
 
 static void secp256k1_num_shift(secp256k1_num *r, int bits) {
diff --git a/src/secp256k1.c b/src/secp256k1.c
@@ -435,7 +435,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_mem_clear(keydata, sizeof(keydata));
    for (i = 0; i <= counter; i++) {
        secp256k1_rfc6979_hmac_sha256_generate(&rng, nonce32, 32);
    }
@@ -447,8 +447,7 @@ 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;
-    secp256k1_scalar sec, non, msg;
+    secp256k1_scalar r, s, sec;
     int ret = 0;
     int overflow = 0;
     VERIFY_CHECK(ctx != NULL);
@@ -463,6 +462,7 @@ int secp256k1_ecdsa_sign(const secp256k1_context* ctx, secp256k1_ecdsa_signature
     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);
@@ -479,11 +479,11 @@ int secp256k1_ecdsa_sign(const secp256k1_context* ctx, secp256k1_ecdsa_signature
             }
             count++;
         }
-        memset(nonce32, 0, 32);
+        secp256k1_mem_clear(nonce32, sizeof(nonce32));
         secp256k1_scalar_clear(&msg);
         secp256k1_scalar_clear(&non);
-        secp256k1_scalar_clear(&sec);
     }
+    secp256k1_scalar_clear(&sec);
     if (ret) {
         secp256k1_ecdsa_signature_save(signature, &r, &s);
     } else {

Original file line number	Diff line number	Diff line change
`@@ -161,6 +161,10 @@ static void secp256k1_sha256_finalize(secp256k1_sha256 hash, unsigned char out`
`161`	`161`	`hash->s[i] = 0;`
`162`	`162`	`}`
`163`	`163`	`memcpy(out32, (const unsigned char*)out, 32);`
	`164`	`+`
	`165`	`+ secp256k1_mem_clear(sizedesc, sizeof(sizedesc));`
	`166`	`+ secp256k1_mem_clear(out, sizeof(out));`
	`167`	`+ secp256k1_mem_clear(hash, sizeof(secp256k1_sha256));`
`164`	`168`	`}`
`165`	`169`
`166`	`170`	`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`
`188`	`192`	`rkey[n] ^= 0x5c ^ 0x36;`
`189`	`193`	`}`
`190`	`194`	`secp256k1_sha256_write(&hash->inner, rkey, sizeof(rkey));`
`191`		`- memset(rkey, 0, sizeof(rkey));`
	`195`	`+ secp256k1_mem_clear(rkey, sizeof(rkey));`
`192`	`196`	`}`
`193`	`197`
`194`	`198`	`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`
`199`	`203`	`unsigned char temp[32];`
`200`	`204`	`secp256k1_sha256_finalize(&hash->inner, temp);`
`201`	`205`	`secp256k1_sha256_write(&hash->outer, temp, 32);`
`202`		`- memset(temp, 0, 32);`
	`206`	`+ secp256k1_mem_clear(temp, sizeof(temp));`
`203`	`207`	`secp256k1_sha256_finalize(&hash->outer, out32);`
`204`	`208`	`}`
`205`	`209`
`@@ -266,9 +270,7 @@ static void secp256k1_rfc6979_hmac_sha256_generate(secp256k1_rfc6979_hmac_sha256`
`266`	`270`	`}`
`267`	`271`
`268`	`272`	`static void secp256k1_rfc6979_hmac_sha256_finalize(secp256k1_rfc6979_hmac_sha256 *rng) {`
`269`		`- memset(rng->k, 0, 32);`
`270`		`- memset(rng->v, 0, 32);`
`271`		`- rng->retry = 0;`
	`273`	`+ secp256k1_mem_clear(rng, sizeof(secp256k1_rfc6979_hmac_sha256));`
`272`	`274`	`}`
`273`	`275`
`274`	`276`	`#undef BE32`
Original file line number	Diff line number	Diff line change
`@@ -58,9 +58,13 @@ int secp256k1_ecdh(const secp256k1_context* ctx, unsigned char *output, const se`
`58`	`58`	`secp256k1_fe_get_b32(y, &pt.y);`
`59`	`59`
`60`	`60`	`ret = hashfp(output, x, y, data);`
	`61`	`+ secp256k1_mem_clear(x, sizeof(x));`
	`62`	`+ secp256k1_mem_clear(y, sizeof(y));`
`61`	`63`	`}`
`62`	`64`
`63`	`65`	`secp256k1_scalar_clear(&s);`
	`66`	`+ secp256k1_ge_clear(&pt);`
	`67`	`+`
`64`	`68`	`return ret;`
`65`	`69`	`}`
`66`	`70`