What the developer wrote
Authored by ww3512687
fix multisig
What changed, and why it matters
This commit fixes a Bitcoin multisig signing flow in the Keystone 3 hardware wallet firmware. Previously, when a transaction did not need a signature, the code exported the PSBT but did not properly handle the result object; when it did need a signature, it signed but skipped updating the UI status and freeing the result. The patch unifies both branches so the result is always processed and cleaned up. It also wraps some legacy Keystone transaction code in a WEB3_VERSION compile guard, likely preventing it from running on Bitcoin-only firmware builds where it could behave incorrectly.
Review the ownership semantics of UREncodeResult inside MultisigSignResult to confirm the unified free path is safe. Verify that WEB3_VERSION is correctly defined for the intended product variants and that excluding these legacy paths does not break other supported workflows. Consider adding regression tests for both export-only and signing multisig flows.
Security signals we found
Memory-management asymmetry in multisig signing path (possible leak or use-after-free depending on btc_sign_multisig_psbt ownership)
Missing UI status update in the signing branch of BtcSignPsbtMultisig
Legacy UTXO code paths executed unconditionally despite BTC_ONLY build, now gated by WEB3_VERSION
Potential NULL/undefined behavior from xPub/hdPath on BTC_ONLY builds before the guard
Evidence from the diff
In BtcSignPsbtMultisig(), the original code had an asymmetric if/else: the export branch created a MultisigSignResult, set UI status, and freed it; the sign branch returned the raw UREncodeResult without freeing its containing MultisigSignResult or updating status. The refactor assigns result in a ternary and then processes/free it uniformly. Additionally, three legacy-Keystone-transaction code paths in GetBtcSignDataDynamic(), GuiGetParsedQrData(), and GuiGetPsbtCheckResult() are now guarded by #ifdef WEB3_VERSION. On BTC_ONLY builds these blocks are compiled out, which avoids calling GuiGetUtxoPubKeyAndHdPath(), utxo_sign_keystone/parse_keystone/check_keystone, and returning uninitialized/unfreed xPub/hdPath values.
Changed components
src/ui/gui_chain/gui_btc.cBitcoin multisig PSBT signing flowLegacy Keystone UTXO transaction parsing/signing/checking helpersInspect captured patch +10 / −4
diff --git a/src/ui/gui_chain/gui_btc.c b/src/ui/gui_chain/gui_btc.c
index bfa6a31..98ae253 100644
--- a/src/ui/gui_chain/gui_btc.c
+++ b/src/ui/gui_chain/gui_btc.c
@@ -172,13 +172,13 @@ static UREncodeResult *BtcSignPsbtMultisig(void *data, uint8_t *seed, int len, u
{
#ifdef BTC_ONLY
UREncodeResult *encodeResult = NULL;
- if (!GuiGetCurrentTransactionNeedSign()) {
- MultisigSignResult *result = btc_export_multisig_psbt(data);
+ MultisigSignResult *result = !GuiGetCurrentTransactionNeedSign()
+ ? btc_export_multisig_psbt(data)
+ : btc_sign_multisig_psbt(data, seed, len, mfp, mfpLen);
+ if (result) {
encodeResult = result->ur_result;
GuiMultisigTransactionSignatureSetSignStatus(result->sign_status, result->is_completed, result->psbt_hex, result->psbt_len);
free_MultisigSignResult(result);
- } else {
- encodeResult = btc_sign_multisig_psbt(data, seed, len, mfp, mfpLen);
}
return encodeResult;
#else
@@ -250,10 +250,12 @@ static UREncodeResult *GetBtcSignDataDynamic(bool unLimit)
} else if (SupportSignLegacyKeystoneTransactions(urType)) {
char *hdPath = NULL;
char *xPub = NULL;
+#ifdef WEB3_VERSION
if (0 != GuiGetUtxoPubKeyAndHdPath(viewType, &xPub, &hdPath)) {
return NULL;
}
encodeResult = utxo_sign_keystone(data, urType, mfp, sizeof(mfp), xPub, SOFTWARE_VERSION, seed, len);
+#endif
} else if (urType == BtcSignRequest) {
encodeResult = btc_sign_msg(data, seed, len, mfp, sizeof(mfp));
} else if (urType == SeedSignerMessage) {
@@ -420,12 +422,14 @@ void *GuiGetParsedQrData(void)
} else if (SupportSignLegacyKeystoneTransactions(urType)) {
char *hdPath = NULL;
char *xPub = NULL;
+#ifdef WEB3_VERSION
if (0 != GuiGetUtxoPubKeyAndHdPath(viewType, &xPub, &hdPath)) {
return NULL;
}
g_parseResult = utxo_parse_keystone(crypto, urType, mfp, sizeof(mfp), xPub);
SRAM_FREE(public_keys);
CHECK_CHAIN_RETURN(g_parseResult);
+#endif
return g_parseResult;
} else if (SupportSignPsbtExtend(urType)) {
g_parseResult = utxo_parse_extend_psbt(crypto, public_keys, mfp, sizeof(mfp));
@@ -558,10 +562,12 @@ PtrT_TransactionCheckResult GuiGetPsbtCheckResult(void)
} else if (SupportSignLegacyKeystoneTransactions(urType)) {
char *hdPath = NULL;
char *xPub = NULL;
+#ifdef WEB3_VERSION
if (0 != GuiGetUtxoPubKeyAndHdPath(viewType, &xPub, &hdPath)) {
return NULL;
}
result = utxo_check_keystone(crypto, urType, mfp, sizeof(mfp), xPub);
+#endif
} else if (urType == BtcSignRequest) {
result = btc_check_msg(crypto, mfp, sizeof(mfp));
} else if (urType == SeedSignerMessage) {
Why this scored 42/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.