Skip to content

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

Open
furszy wants to merge 1 commit into
bitcoin-core:masterfrom
furszy:2026_ellswift_clear_sha_buf
Open

ellswift: don't declassify or leave sk in sha256 buffer#1863
furszy wants to merge 1 commit into
bitcoin-core:masterfrom
furszy:2026_ellswift_clear_sha_buf

Conversation

@furszy

@furszy furszy commented Jun 6, 2026

Copy link
Copy Markdown
Member

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

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.
@theStack

theStack commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

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

static void secp256k1_ellswift_prng(const secp256k1_hash_ctx *hash_ctx, unsigned char* out32, const secp256k1_sha256 *hasher, uint32_t cnt) {
secp256k1_sha256 hash = *hasher;

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants