musig: Clear secrets before ARG_CHECK failures in partial_sign #1960

pull fametrano wants to merge 1 commits into bitcoin-core:master from fametrano:musig-partial-sign-clear changing 2 files +13 −2

fametrano commented at 11:21 PM on October 10, 2026: contributor

secp256k1_musig_partial_sign loads the two secret nonce scalars and then zeroes the secnonce, so the stack copies are the only ones left. The ARG_CHECKs that follow, four on NULL arguments and one on the secnonce matching the keypair, return without calling secp256k1_musig_partial_sign_clear, which every other exit after the load calls.

This clears the secrets before each of these checks fails, as #1849 did for secp256k1_musig_nonce_gen_counter. The checks keep their order, and the secnonce is still zeroed when the call fails; a new test checks that. It matters only when the illegal callback returns and the API is misused, the key mismatch being the realistic case.
Yudis-bit commented at 9:10 AM on October 11, 2026: contributor

Tested ACK edeb2376e19013e752629505ba695a57a7490732 on Windows x86_64.
real-or-random added the label side-channel on Oct 12, 2026
real-or-random added the label tweak/refactor on Oct 12, 2026
real-or-random commented at 6:49 AM on October 12, 2026: contributor

Concept ACK on clearing the secret, but I think this could be simplified by loading the secnonce only after the ARG_CHECKS
fametrano force-pushed on Oct 13, 2026
fametrano commented at 8:29 AM on October 13, 2026: contributor

Thanks @real-or-random, done in 986d6ad (force-pushed): the NULL checks now come before the secnonce is loaded, so a call with a NULL argument leaves the secnonce as is and produces no signature. The key-match check needs both the secnonce and the keypair loaded, so it still clears the secrets before it fails. The header says the function "overwrites the given secnonce with zeros"; should it note the NULL case? @Yudis-bit, the head changed: could you re-test 986d6ad and renew your ACK if it still holds?
Yudis-bit commented at 10:44 AM on October 13, 2026: contributor

Tested ACK 986d6ad on Windows x86_64.

I don't think the header needs to mention the NULL case either, passing NULL is already invalid API usage under ARG_NONNULL.

in src/modules/musig/session_impl.h:639 in 986d6ad150

 639 |          secp256k1_musig_partial_sign_clear(&sk, k);
 640 |          return 0;
 641 |      }
 642 | +    /* ARG_CHECK returns without clearing the secrets, so clear them first. */
 643 | +    if (!(secp256k1_fe_equal(&pk.x, &keypair_pk.x)
 644 | +          && secp256k1_fe_equal(&pk.y, &keypair_pk.y))) {

theStack commented at 7:07 AM on November 3, 2026:

to avoid doing these comparisons twice, we could store the result in a variable (e.g. secnonce_matches_keypair) and then branch and ARG_CHECK on that

theStack commented at 7:07 AM on November 3, 2026: contributor

Concept ACK
real-or-random commented at 9:43 AM on November 3, 2026: contributor

the NULL checks now come before the secnonce is loaded, so a call with a NULL argument leaves the secnonce as is and produces no signature.

Now that I see your comment, I'm pretty sure that my suggestion was simply misguided. Clearing the secnonce was deliberate, even if we fail, so we should keep this. Sorry.

musig: Clear secrets before ARG_CHECK failures in partial_sign

secp256k1_musig_partial_sign loads the secnonce scalars into stack
variables and zeroes the secnonce, so those copies are the only ones
left. The ARG_CHECKs after that point return without calling
secp256k1_musig_partial_sign_clear, which leaves the secrets on the
stack. Clear them before these checks fail.

Also test that the secnonce is zeroed when signing fails.

379a2f079e

fametrano force-pushed on Nov 3, 2026
fametrano commented at 7:21 AM on November 4, 2026: contributor

Restored in 379a2f0: the secnonce is again zeroed before the NULL checks, as on master, and the secrets are cleared before any of the later ARG_CHECKs fails. A new test checks that the secnonce is zeroed when the call fails on a NULL argument; it fails on the previous head. @theStack, the key match is now computed once into secnonce_matches_keypair, which the if and the ARG_CHECK both use. I also rebased on master.

Contributors

Labels

side-channel tweak/refactor

Linked (view graph)

#1849 musig: always clear out secret key in `secp256k1_musig_nonce_gen_counter`