Merge pull request #542 from LedgerHQ/musig-fixes
What the developer wrote
Authored by Salvatore Ingala
Merge pull request #542 from LedgerHQ/musig-fixes
Add some missing error propagation in `musig.c`
What changed, and why it matters
This commit fixes missing error handling in the Ledger Bitcoin app's MuSig2 multi-signature code. Several cryptographic functions could fail silently or return incorrect results because their error codes were ignored. The patch now checks those return values and aborts the signing process when something goes wrong. It also corrects a debug-print index so a disruptive co-signer is reported with the right number.
Treat this as a security-hardening fix and include it in the next release. Review whether any earlier code path could have produced an invalid MuSig signature or leaked information through side channels before the patch. Add regression tests that supply out-of-curve coordinates and point-addition failure conditions.
Security signals we found
Missing error propagation in cryptographic point operations
Unchecked return value of crypto_tr_lift_x could lead to use of an unspecified point
Unchecked point_add return value in key aggregation, nonce aggregation, and tweaking
Potential infinite or invalid curve points propagated through MuSig2 signing flow
Debug-only error path in nonce aggregation now returns failure instead of continuing
Evidence from the diff
The patch adds error propagation for crypto_tr_lift_x and point_add calls in musig.c. Previously, cpoint() did not check whether crypto_tr_lift_x succeeded before treating the output as a valid point, and musig_key_agg, musig_nonce_agg, and apply_tweak ignored the return value of point_add. The fix returns -1 on failure, preventing subsequent operations on invalid/infinite points. musig_signing.c adjusts the printed disruptive-signer index to match the new negative return convention.
Changed components
src/musig/musig.csrc/handler/sign_psbt/musig_signing.csrc/crypto.hInspect captured patch +21 / −6
### src/crypto.h
@@ -381,6 +381,9 @@ void crypto_tr_tagged_hash_init(cx_sha256_t *hash_context, const uint8_t *tag, u
* Pointer to a 32-byte array.
* @param[out] out
* Pointer to an array that will received the output as an uncompressed 65-bytes pubkey.
+ *
+ * @return 0 on success, -1 if x is not the x-coordinate of a point on the curve. On failure, the
+ * content of `out` is unspecified and must not be used.
*/
int crypto_tr_lift_x(const uint8_t x[static 32], uint8_t out[static 65]);
### src/handler/sign_psbt/musig_signing.c
@@ -424,7 +424,7 @@ bool __attribute__((noinline)) sign_sighash_musig_and_yield(dispatcher_context_t
musig_pubnonce_t aggnonce;
int res = musig_nonce_agg(nonces, musig_info->n, &aggnonce);
if (res < 0) {
- PRINTF("Musig aggregation failed; disruptive signer has index %d\n", -res);
+ PRINTF("Musig aggregation failed; disruptive signer has index %d\n", -res - 1);
SEND_SW(dc, SW_INCORRECT_DATA);
return false;
}
### src/musig/musig.c
@@ -90,7 +90,10 @@ static bool has_even_y(const point_t *P) {
}
static int cpoint(const uint8_t x[33], point_t *out) {
- crypto_tr_lift_x(&x[1], out->raw);
+ if (0 > crypto_tr_lift_x(&x[1], out->raw)) {
+ PRINTF("Invalid compressed point: not on curve\n");
+ return -1;
+ }
if (is_point_infinite(out)) {
PRINTF("Invalid compressed point\n");
return -1;
@@ -201,7 +204,10 @@ int musig_key_agg(const plain_pk_t pubkeys[], size_t n_keys, musig_keyagg_contex
return -1;
}
- point_add(&ctx->Q, &P, &ctx->Q);
+ if (CX_OK != point_add(&ctx->Q, &P, &ctx->Q)) {
+ PRINTF("Point addition failed in musig_key_agg\n");
+ return -1;
+ }
}
if (is_point_infinite(&ctx->Q)) {
@@ -300,9 +306,13 @@ int musig_nonce_agg(const musig_pubnonce_t pubnonces[], size_t n_keys, musig_pub
point_t R_ij;
if (0 > cpoint(&pubnonces[i].raw[(j - 1) * sizeof(plain_pk_t)], &R_ij)) {
PRINTF("Musig2 nonce aggregation: invalid contribution from cosigner %d\n", i);
- return -i - 1;
+ return -(int) i - 1;
+ }
+ if (CX_OK != point_add(&R_j, &R_ij, &R_j)) {
+ // this should never happen
+ PRINTF("Point addition failed for cosigner %d in musig_nonce_agg\n", i);
+ return -(int) i - 1;
}
- point_add(&R_j, &R_ij, &R_j);
}
if (is_point_infinite(&R_j)) {
@@ -351,7 +361,9 @@ static int apply_tweak(musig_keyagg_context_t *ctx, const uint8_t tweak[static 3
}
// compute the resulting tweaked point g * Q + tweak * G
- point_add(&ctx->Q, &T, &ctx->Q);
+ if (CX_OK != point_add(&ctx->Q, &T, &ctx->Q)) {
+ return -1;
+ }
if (is_point_infinite(&ctx->Q)) {
PRINTF("The result of tweaking cannot be infinity\n");
return -1;Why this scored 59/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.