Add missing return after error; fix wrong int-bool conversion
What the developer wrote
Authored by Salvatore Ingala
Add missing return after error; fix wrong int-bool conversion
What changed, and why it matters
This commit fixes two bugs in the Ledger Bitcoin app's MuSig (multi-signature) signing code. First, a function that should return true/false was returning -1 on error, which could be misinterpreted as 'true' (success). Second, after detecting a failed signature aggregation and sending an error code to the computer, the code kept running instead of stopping, which could lead to signing with invalid or partial data. Both are security-relevant control-flow bugs in a cryptocurrency signing path.
Review all bool-returning functions in the signing handlers for similar integer/bool mismatches, and audit all SEND_SW error paths to ensure they are followed by an immediate return. Consider adding static-analysis or linting rules to catch missing returns after error sends and bool/integer return mismatches.
Security signals we found
Incorrect error return value (integer -1 returned from bool function)
Missing early return after sending error status word
Fall-through execution after cryptographic aggregation failure
MuSig signing path affected
Potential use of invalid/partial public key or nonce data
Evidence from the diff
In src/handler/sign_psbt/musig_signing.c, compute_musig_per_input_info() returns bool but previously returned -1 on get_extended_pubkey_from_client() failure. Because -1 is non-zero, callers treating any non-zero value as true would see it as success. In sign_sighash_musig_and_yield(), after SEND_SW(dc, SW_INCORRECT_DATA) for a failed musig_pubkey_agg(), the function fell through and continued to recompute secnonce and yield a signature instead of returning. The patch changes the first return to false and adds a return false after the error send.
Changed components
src/handler/sign_psbt/musig_signing.ccompute_musig_per_input_info()sign_sighash_musig_and_yield()Ledger Bitcoin app MuSig signing flowInspect captured patch +2 / −1
diff --git a/src/handler/sign_psbt/musig_signing.c b/src/handler/sign_psbt/musig_signing.c
index 07d80ca..d9d5458 100644
--- a/src/handler/sign_psbt/musig_signing.c
+++ b/src/handler/sign_psbt/musig_signing.c
@@ -51,7 +51,7 @@ bool compute_musig_per_input_info(dispatcher_context_t *dc,
for (int i = 0; i < musig_info->n; i++) {
// we use ext_pubkey as a temporary variable; will overwrite later
if (0 > get_extended_pubkey_from_client(dc, &wdi, key_indexes[i], &ext_pubkey)) {
- return -1;
+ return false;
}
memcpy(out->keys[i], ext_pubkey.compressed_pubkey, sizeof(ext_pubkey.compressed_pubkey));
}
@@ -407,6 +407,7 @@ bool __attribute__((noinline)) sign_sighash_musig_and_yield(dispatcher_context_t
if (res < 0) {
PRINTF("Musig aggregation failed; disruptive signer has index %d\n", -res);
SEND_SW(dc, SW_INCORRECT_DATA);
+ return false;
}
// recompute secnonce from psbt_session randomness
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.