Fix missing return value checks for point_add calls
What the developer wrote
Authored by Salvatore Ingala
Fix missing return value checks for point_add calls
Also made the cast from size_t to int explicit, and fixed PRINTF
message that had an off-by-one.
What changed, and why it matters
This commit fixes three places in the Ledger Bitcoin app's MuSig2 code where the result of an elliptic-curve point-addition operation was not checked for failure. If point_add fails (for example, because it produced the special 'point at infinity' or another error condition), the code previously continued using an invalid public key or nonce. The patch now aborts the signing operation and returns an error. It also corrects a debug-print index that was off by one and makes a type conversion explicit.
Treat as a security-hardening fix and include it in the next release. Review whether any other cryptographic calls in the MuSig2 implementation have unchecked return values. No independent CVE or advisory is supplied, so further vendor assessment is warranted to determine exploitability.
Security signals we found
Unchecked cryptographic operation return value (point_add)
Potential use of invalid/infinite elliptic-curve point in MuSig2 aggregation
MuSig2 nonce aggregation failure path now returns correct cosigner index
Explicit integer cast from size_t to int in return value
Evidence from the diff
The patch adds return-value checks for point_add in musig_key_agg, musig_nonce_agg, and apply_tweak within src/musig/musig.c. point_add appears to return cx_err_t (with CX_OK indicating success). Previously, failures were silently ignored, potentially leaving ctx->Q or R_j in an invalid/infinite state. The patch also changes the PRINTF in sign_sighash_musig_and_yield from -res to -res - 1 so the reported ‘disruptive signer index’ matches the actual cosigner index returned by musig_nonce_agg. Finally, it makes the size_t-to-int cast in the return expressions explicit.
Changed components
src/musig/musig.csrc/handler/sign_psbt/musig_signing.cInspect captured patch +14 / −5
### 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
@@ -204,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)) {
@@ -303,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)) {
@@ -354,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.