ellswift: don't declassify or leave sk in sha256 buffer #1863

secp256k1_ellswift_create assumes sha256 clears the data in its buffer after hashing it, which is not the case. So we shouldn't declassify the whole struct, only the hash result. We should also clear it at the end, so the sk doesn't linger on the stack when no aux rnd is given.

On master, can add the following diff and run the tests to see sk sitting in the buffer.

diff --git a/src/modules/ellswift/main_impl.h b/src/modules/ellswift/main_impl.h
--- a/src/modules/ellswift/main_impl.h	
+++ b/src/modules/ellswift/main_impl.h	
@@ -461,6 +461,9 @@
     secp256k1_ellswift_elligatorswift_var(ctx, ell64, &t, &p, &hash); /* puts u in ell64[0..32] */
     secp256k1_fe_get_b32(ell64 + 32, &t); /* puts t in ell64[32..64] */
 
+    /* DEMO: fail because sk sits in the buffer */
+    if (!auxrnd32) VERIFY_CHECK(memcmp(hash.buf, seckey32, 32) != 0);
     secp256k1_memczero(ell64, 64, !ret);

Concept ACK

Interesting find. I was very confused at first why there wouldn't be further writes on this passed hashing object within secp256k1_ellswift_elligatorswift_var, but it's just copied over to a new instance for each secp256k1_ellswift_prng call (which makse sense for reusing the state with different counters): https://github.com/bitcoin-core/secp256k1/blob/13db747f2b558659b26f8ca3f5ea6e6d34e2054c/src/modules/ellswift/main_impl.h#L310-L311

Fwiw Bitcoin Core's BIP324 implementation is not affected by this, as we pass in the randomness (auxrnd32 parameter), which IIUC overwrites the secret key in the _sha256 object buffer.