musig: clear `pubnonce` output for invalid `seckey` #1850

l0rinc commented at 10:41 AM on April 29, 2026: none

This is a follow-up to PR #1849.

Problem

secp256k1_musig_nonce_gen returns failure if its arguments are invalid, for example when a non-NULL seckey fails secret-key validation.

On that path, secp256k1_musig_nonce_gen_internal invalidates the secret nonce on failure, but still unconditionally saves the derived public nonce output before returning failure.

Fix

Clear pubnonce before returning the invalid-seckey failure, so a failed nonce-generation call does not leave a newly written pubnonce behind.

The implementation still saves the public nonce before clearing it on failure (instead of branching around the save), to preserve constant-time control flow with respect to the secret-key validation result.

The second commit only extends existing MuSig API failure-path coverage for paths where secp256k1_musig_nonce_gen_internal already clears pubnonce at function entry before any of its own validation steps.

in src/modules/musig/tests_impl.h:289 in fde940f38a outdated

 285 | @@ -286,6 +286,7 @@ static void musig_api_tests(void) {
 286 |      /* invalid seckey */
 287 |      CHECK(secp256k1_musig_nonce_gen(CTX, &secnonce[0], &pubnonce[0], session_secrand[0], max64, &pk[0], msg, &keyagg_cache, max64) == 0);
 288 |      CHECK(memcmp_and_randomize(secnonce[0].data, zeros132, sizeof(secnonce[0].data)) == 0);
 289 | +    CHECK(memcmp_and_randomize(pubnonce[0].data, zeros132, sizeof(pubnonce[0].data)) == 0);

theStack commented at 3:25 PM on April 29, 2026:

in fde940f38a0ddaa3c4b556eeff42bcd3ed89c319: nit: I suspect this change was meant to be part of the next commit?

l0rinc commented at 4:14 PM on April 29, 2026:

No, this is the one that reproduces the fix (it fails without the fix), the next commit adds these checks that were already correct. Would it help if I moved that commit first?

theStack commented at 4:44 PM on April 29, 2026:

Ah, I see now. Swapping commits would indeed help to clarify that I think, but happy to (re-)ACK in either order.

l0rinc commented at 5:40 PM on April 29, 2026:

Swapped the commits, thanks!

theStack commented at 3:30 PM on April 29, 2026: contributor

Concept ACK

AFAICT this pattern matches what we do in similar API functions (e.g. _schnorrsig_sign32, _keypair_create, _ellswift_create).

theStack commented at 4:44 PM on April 29, 2026: contributor

ACK 7ed2abc96871d78baaeafe2b664381f01068f49

musig: test cleared nonce outputs on failures

`secp256k1_musig_nonce_gen_internal` clears `pubnonce` at the top of the function, before the later validation steps covered here.

Extend the existing MuSig API failure-path checks for `nonce_gen` and `nonce_gen_counter` to assert that these paths leave the public nonce output cleared.

bd3859827e

musig: clear pubnonce output for invalid seckey

`secp256k1_musig_nonce_gen` returns failure for invalid arguments, including a non-NULL `seckey` that fails validation.

`secp256k1_musig_nonce_gen_internal` already invalidates `secnonce` on that failure, but it still saves derived nonce points into `pubnonce` before returning.

Clear `pubnonce` with a conditional clear after the save, so the cleanup stays branchless with respect to the secret-key validation result and reverting this cleanup leaves stale derived nonce data in the MuSig API test.

d679d2cf91

l0rinc force-pushed on Apr 29, 2026

theStack approved

theStack commented at 6:48 PM on April 29, 2026: contributor

re-ACK d679d2cf91b9129d8f8394d58f1a54f6a82ede54

real-or-random commented at 1:45 PM on April 30, 2026: contributor

On that path, secp256k1_musig_nonce_gen_internal invalidates the secret nonce on failure, but still unconditionally saves the derived public nonce output before returning failure.

Why is that a problem?

real-or-random added the label tweak/refactor on Apr 30, 2026

theStack commented at 4:01 PM on April 30, 2026: contributor

On that path, secp256k1_musig_nonce_gen_internal invalidates the secret nonce on failure, but still unconditionally saves the derived public nonce output before returning failure.

Why is that a problem?

From a reviewer's perspective, I see this PR as a small consistency improvement, fully enforcing the (unwritten?) rule "if any of the input parameters are invalid or the function fails for any other reason, all of the out parameters are zeroed/invalidated". Probably not an actual problem in practice though, considering that users should always check for return values before evaluating out-parameters in the first place.

real-or-random commented at 6:16 AM on May 1, 2026: contributor

Sorry for the very short comment earlier.

(unwritten?) rule "if any of the input parameters are invalid or the function fails for any other reason, all of the out parameters are zeroed/invalidated".

I'm not sure if we there is such a rule. I can't find any discussion on this, what I remember is that we want to make sure that output parameters are written to at all, simply to avoid uninitialized memory (because that can trigger UB).

Probably not an actual problem in practice though, considering that users should always check for return values before evaluating out-parameters in the first place.

In this case, it's not a problem at all because the pubnonce is simply a random group element for which nobody knows the discrete logarithm (not even the code itself because it was deleted). Also, the pubnonce is (computationally) independent of whether the secret key was invalid or not.

Of course, you can ask if there should be a general rule of the kind you mentioned above. Maybe it's good practice, but the drawbacks can be an increase in code complexity and (potentially) an impact on performance. Another spot where we don't follow this rule is secp256k1_ecdh.

If we can get consensus on this rule, then yes, let's do this, but then let's also write the rule down.

In some of these functions (including the one touched here), I think we're overdoing it with insisting on constant-time behavior. If the secret key is invalid (zero or overflow), it's totally fine to return early. We want to caller to branch on the return value, so not branching inside our code is a bit pointless. Any attacker who is able to measure time will be able to figure that out eventually because the caller will branch anyway. In this case, you can even observe on the network whether a pubnonce will be sent or not.

I'd even go as far as saying that secp256k1_scalar_set_b32_seckey should internally declassify, or at least there should be a variant that does. What we have now encourages a constant-time coding style around that function. Then execution continues with an invalid intermediate values, and that can be problematic if we pass that intermediate value to other functions which don't expect it. That makes it hard to reason about the code. (Concrete examples: #1316 and https://github.com/BlockstreamResearch/secp256k1-zkp/pull/282).

So if we want to enforce this rule here, then I'd prefer an early return instead of a late memczero.

l0rinc commented at 7:51 PM on May 3, 2026: none

Thanks for the detailed explanation, that makes sense. Closing this since the current framing overstates the issue, and the broader output-clearing rule is not established.

l0rinc closed this on May 3, 2026