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

1. 
   furszy commented at 6:11 PM on June 6, 2026: 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);
   

2. 
   theStack commented at 5:54 PM on June 8, 2026: 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): 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.

3. real-or-random added the label side-channel on Jun 9, 2026
4. real-or-random added the label tweak/refactor on Jun 9, 2026
5. real-or-random added this to the milestone 0.7.2 on Jul 28, 2026
6. in src/modules/ellswift/main_impl.h:452 in 9db953fe86

   ---

   ---

   

   real-or-random commented at 8:46 AM on July 28, 2026:

   nit:

       /* Set up hasher state. The used RNG is H(seckey23 || "\x00"*32 [|| auxrnd32] || cnt++),
   

   could be fixed on the side here

   ---

   

   furszy commented at 2:45 PM on July 30, 2026:

   Sure. Done as suggested.

7. in src/modules/ellswift/main_impl.h:457 in 9db953fe86

    453 | @@ -454,7 +454,7 @@ int secp256k1_ellswift_create(const secp256k1_context *ctx, unsigned char *ell64
    454 |      secp256k1_ellswift_sha256_init_create(&hash);
    455 |      secp256k1_sha256_write(secp256k1_get_hash_context(ctx), &hash, seckey32, 32);
    456 |      secp256k1_sha256_write(secp256k1_get_hash_context(ctx), &hash, zero32, sizeof(zero32));
    457 | -    secp256k1_declassify(ctx, &hash, sizeof(hash)); /* private key is hashed now */
    458 | +    secp256k1_declassify(ctx, &hash.s, sizeof(hash.s)); /* The private key has been hashed, but sk remains in the buffer */
   

   ---

   ---

   

   real-or-random commented at 8:48 AM on July 28, 2026:

   I think the comment is still confusing to me. Isn't sk the same as the "private key" (and actually called seckey32)?

   ---

   

   furszy commented at 2:46 PM on July 30, 2026:

   yeah, pushed an improvement.

8. 
   real-or-random commented at 8:48 AM on July 28, 2026: contributor

   Concept ACK

9. ellswift: don't declassify or leave sk in sha256 buffer

   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.

   0ae17e304e
10. furszy force-pushed on Jul 30, 2026
11. real-or-random approved
12. 
    real-or-random commented at 2:52 PM on July 30, 2026: contributor

    utACK 0ae17e304e17a29c277b563139c4810ee7a96319

13. theStack approved
14. 
    theStack commented at 3:18 PM on July 30, 2026: contributor

    ACK 0ae17e304e17a29c277b563139c4810ee7a96319

    Checked that

    diff --git a/src/modules/ellswift/main_impl.h b/src/modules/ellswift/main_impl.h
    index 817e7788..44bc6868 100644
    --- a/src/modules/ellswift/main_impl.h
    +++ b/src/modules/ellswift/main_impl.h
    @@ -462,10 +462,16 @@ int secp256k1_ellswift_create(const secp256k1_context *ctx, unsigned char *ell64
         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: succeed because sk sits in the buffer */
    +    if (!auxrnd32) VERIFY_CHECK(memcmp(hash.buf, seckey32, 32) == 0);
    +
         secp256k1_memczero(ell64, 64, !ret);
         secp256k1_scalar_clear(&seckey_scalar);
         secp256k1_sha256_clear(&hash);
     
    +    /* DEMO: succeed because sk doesn't sit in buffer anymore */
    +    if (!auxrnd32) VERIFY_CHECK(memcmp(hash.buf, seckey32, 32) != 0);
    +
         return ret;
     }
    

    succeeds in the tests and also ran the ctime-tests locally.

15. real-or-random merged this on Jul 30, 2026
16. real-or-random closed this on Jul 30, 2026

    ---

17. 
    real-or-random commented at 3:44 PM on July 30, 2026: contributor

    Oh I should have waited for CI before merging this. Well, let's wait for it on master...

18. furszy deleted the branch on Jul 30, 2026
19. 
    furszy commented at 4:31 PM on July 30, 2026: member

    Oh I should have waited for CI before merging this. Well, let's wait for it on master...

    Don't worry much. Only comments changed on the last push and CI was green before.

Contributors

furszytheStackreal-or-random

Milestone
0.8.0