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 1 files +10 −5

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. Five ARG_CHECKs follow: four on NULL arguments and one, after the secret key is loaded, on the secnonce's public key matching the keypair. ARG_CHECK returns directly, so these exits skip secp256k1_musig_partial_sign_clear, which every other exit after the load calls.

This moves the four NULL checks before the secnonce is loaded, so a call with a NULL argument now leaves the secnonce untouched. The key-match check needs both the secnonce and the keypair loaded, so it stays in place and the nonces and the secret key are cleared before it fails. The checks and their messages are unchanged.

It matters only when the illegal callback returns and the API is misused, the key mismatch being the realistic case. It does for these exits what #1849 did for secp256k1_musig_nonce_gen_counter.

Assisted-by: my usual tools (LLM, Internet, computer). Mistakes are mine.
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

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.

Move the NULL checks before the secnonce is loaded. The check that the
secnonce matches the keypair needs both loaded, so clear the secrets
before it fails. Its condition is repeated rather than stored so that
the ARG_CHECK message keeps the expression.

986d6ad150

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.

Contributors

fametrano

Yudis-bit

real-or-random

Labels

side-channel tweak/refactor

Linked (view graph)

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