What the developer wrote
Authored by soralit
refactor: multisig
What changed, and why it matters
This commit fixes a logic bug in how Bitcoin multisig wallet policies are validated and cleans up several related code paths. The most important change corrects a flawed condition that could have allowed invalid or nonsensical multisig policies (for example, a threshold of zero or a threshold larger than the total number of signers) to be accepted. It also improves memory safety by adding proper cleanup of secret seed data and freeing a previously unfreed response object, and it removes weak-symbol function stubs that could have led to unexpected behavior when Bitcoin-only features are disabled.
Treat this as a security-relevant fix and include it in the next firmware release. Review whether the old policy-validation bug was reachable through user-facing multisig creation flows, and consider whether any invalid wallets created with the buggy logic need migration or re-validation. Audit other uses of CHECK_ERRCODE_RETURN for macro signature compatibility.
Security signals we found
Corrected boolean logic flaw in multisig policy validation that could allow invalid threshold/total combinations
Fixed swapped argument order in policy validation call
Added missing free method for Response<MultiSigWallet> to prevent memory leaks
Replaced weak-symbol function stubs with explicit BTC_ONLY-gated static implementations to avoid unintended fallback behavior
Added secure clearing of seed material and secret cache on all exit paths in multisig wallet import flow
Evidence from the diff
The patch refactors multisig handling across Rust and C layers. Key technical changes: (1) In rust/apps/bitcoin/src/multi_sig/wallet.rs, is_valid_multi_sig_policy is corrected from (2..=15).contains(&total) && threshold <= total || threshold >= 1 to (2..=15).contains(&total) && threshold <= total && threshold >= 1. The old expression used || with threshold >= 1, meaning any threshold >=1 would pass regardless of total, and threshold <= total was not enforced when threshold >=1. The call site is also fixed from is_valid_multi_sig_policy(threshold, total) to is_valid_multi_sig_policy(total, threshold), matching the function’s parameter order. (2) In rust/rust_c/src/bitcoin/multi_sig/structs.rs, a make_free_method! is added for Response<MultiSigWallet> so the C side can properly free the wrapped response. (3) In src/ui/gui_chain/gui_btc.c, weak-symbol stubs for GuiGetSignPsbtBytesCodeData, GuiGetParsedPsbtStrData, and GuiGetPsbtStrCheckResult are replaced with static implementations gated by #ifdef BTC_ONLY, and CHECK_ERRCODE_RETURN macro calls are updated to a new signature. (4) In src/ui/gui_widgets/btc_only/multi_sig/gui_import_multisig_wallet_info_widgets.c, seed length calculation is unified via GetCurrentAccountSeedLen(), the response is freed with the new free_Response_MultiSigWallet, and memset_s/ClearSecretCache() are called on all error and success paths to clear sensitive seed material from stack memory.
Changed components
Bitcoin multisig wallet creation/validation (Rust)Rust-C FFI for multisig wallet structsBitcoin-only GUI transaction signing flow (C)Bitcoin-only multisig wallet import UI widget (C)Inspect captured patch +34 / −24
diff --git a/rust/apps/bitcoin/src/multi_sig/wallet.rs b/rust/apps/bitcoin/src/multi_sig/wallet.rs
index 4db6c61..bf39fe3 100644
--- a/rust/apps/bitcoin/src/multi_sig/wallet.rs
+++ b/rust/apps/bitcoin/src/multi_sig/wallet.rs
@@ -151,6 +151,7 @@ pub fn parse_bsms_wallet_config(bytes: Bytes) -> Result<BsmsWallet, BitcoinError
Ok(wallet)
}
+// Create a multisig wallet from Keystone is not supported
pub fn create_wallet(
creator: &str,
name: &str,
@@ -161,7 +162,7 @@ pub fn create_wallet(
network: Network,
xfp: &str,
) -> Result<MultiSigWalletConfig, BitcoinError> {
- if !is_valid_multi_sig_policy(threshold, total) {
+ if !is_valid_multi_sig_policy(total, threshold) {
return Err(BitcoinError::MultiSigWalletCrateError(
"not a valid policy".to_string(),
));
@@ -407,7 +408,7 @@ fn process_xpub_and_xfp(
}
fn is_valid_multi_sig_policy(total: u32, threshold: u32) -> bool {
- (2..=15).contains(&total) && threshold <= total || threshold >= 1
+ (2..=15).contains(&total) && threshold <= total && threshold >= 1
}
fn is_valid_xfp(xfp: &str) -> bool {
diff --git a/rust/rust_c/src/bitcoin/multi_sig/structs.rs b/rust/rust_c/src/bitcoin/multi_sig/structs.rs
index dce3c45..6f346f4 100644
--- a/rust/rust_c/src/bitcoin/multi_sig/structs.rs
+++ b/rust/rust_c/src/bitcoin/multi_sig/structs.rs
@@ -4,6 +4,7 @@ use alloc::string::ToString;
use crate::common::ffi::VecFFI;
use crate::common::free::Free;
+use crate::common::structs::Response;
use crate::common::types::{Ptr, PtrBytes, PtrString, PtrT};
use crate::common::ur::UREncodeResult;
use crate::common::utils::{convert_c_char, recover_c_char};
@@ -314,6 +315,7 @@ impl Free for MultiSigWallet {
impl_c_ptr!(MultiSigWallet);
make_free_method!(MultiSigWallet);
+make_free_method!(Response<MultiSigWallet>);
#[repr(C)]
pub struct MultisigSignResult {
diff --git a/src/ui/gui_chain/gui_btc.c b/src/ui/gui_chain/gui_btc.c
index f695ca4..40370f2 100644
--- a/src/ui/gui_chain/gui_btc.c
+++ b/src/ui/gui_chain/gui_btc.c
@@ -12,6 +12,7 @@
#include "gui_chain_components.h"
#include "gui_home_widgets.h"
#include "gui_transaction_detail_widgets.h"
+#include "err_code.h"
#ifdef BTC_ONLY
#include "gui_multisig_transaction_signature_widgets.h"
#endif
@@ -105,10 +106,6 @@ static int32_t GuiGetUtxoPubKeyAndHdPath(ViewType viewType, char **xPub, char **
}
#endif
-__attribute__((weak)) UREncodeResult *GuiGetSignPsbtBytesCodeData(void)
-{
- return NULL;
-}
#ifdef BTC_ONLY
static UREncodeResult *GuiGetSignPsbtBytesCodeData(void)
{
@@ -128,7 +125,7 @@ static UREncodeResult *GuiGetSignPsbtBytesCodeData(void)
uint8_t seed[64];
int len = GetCurrentAccountSeedLen();
int ret = GetAccountSeed(GetCurrentAccountIndex(), seed, SecretCacheGetPassword());
- CHECK_ERRCODE_RETURN("GetAccountSeed", ret);
+ CHECK_ERRCODE_RETURN(ret);
MultisigSignResult *result = btc_sign_multisig_psbt_bytes(g_psbtBytes, g_psbtBytesLen, seed, len, mfp, sizeof(mfp));
encodeResult = result->ur_result;
GuiMultisigTransactionSignatureSetSignStatus(result->sign_status, result->is_completed, result->psbt_hex, result->psbt_len);
@@ -140,6 +137,11 @@ static UREncodeResult *GuiGetSignPsbtBytesCodeData(void)
SetLockScreen(enable);
return encodeResult;
}
+#else
+static UREncodeResult *GuiGetSignPsbtBytesCodeData(void)
+{
+ return NULL;
+}
#endif
UREncodeResult *GuiGetBtcSignQrCodeData(void)
@@ -231,7 +233,7 @@ static UREncodeResult *GetBtcSignDataDynamic(bool unLimit)
uint8_t seed[64];
int len = GetCurrentAccountSeedLen();
int ret = GetAccountSeed(GetCurrentAccountIndex(), seed, SecretCacheGetPassword());
- CHECK_ERRCODE_RETURN("GetAccountSeed", ret);
+ CHECK_ERRCODE_RETURN(ret);
if (urType == CryptoPSBT) {
if (GuiGetCurrentTransactionType() == TRANSACTION_TYPE_BTC_MULTISIG) {
@@ -261,12 +263,6 @@ static UREncodeResult *GetBtcSignDataDynamic(bool unLimit)
SetLockScreen(enable);
return encodeResult;
}
-
-__attribute__((weak)) void *GuiGetParsedPsbtStrData(void)
-{
- return NULL;
-}
-
#ifdef BTC_ONLY
static void *GuiGetParsedPsbtStrData(void)
{
@@ -326,6 +322,12 @@ static void *GuiGetParsedPsbtStrData(void)
SRAM_FREE(wallet_config);
return g_parseResult;
}
+#else
+static void *GuiGetParsedPsbtStrData(void)
+{
+ return NULL;
+}
+
#endif
static void PreparePublicKeys(PtrT_CSliceFFI_ExtendedPublicKey public_keys) {
@@ -471,13 +473,8 @@ void *GuiGetParsedQrData(void)
return NULL;
}
-__attribute__((weak)) PtrT_TransactionCheckResult GuiGetPsbtStrCheckResult(void)
-{
- return NULL;
-}
-
#ifdef BTC_ONLY
-PtrT_TransactionCheckResult GuiGetPsbtStrCheckResult(void)
+static PtrT_TransactionCheckResult GuiGetPsbtStrCheckResult(void)
{
PtrT_TransactionCheckResult result = NULL;
PtrT_CSliceFFI_ExtendedPublicKey public_keys = SRAM_MALLOC(sizeof(CSliceFFI_ExtendedPublicKey));
@@ -515,6 +512,11 @@ PtrT_TransactionCheckResult GuiGetPsbtStrCheckResult(void)
SRAM_FREE(wallet_config);
return result;
}
+#else
+static PtrT_TransactionCheckResult GuiGetPsbtStrCheckResult(void)
+{
+ return NULL;
+}
#endif
static PtrT_TransactionCheckResult CheckPsbt(void *crypto, uint8_t *mfp, PtrT_CSliceFFI_ExtendedPublicKey public_keys) {
diff --git a/src/ui/gui_widgets/btc_only/multi_sig/gui_import_multisig_wallet_info_widgets.c b/src/ui/gui_widgets/btc_only/multi_sig/gui_import_multisig_wallet_info_widgets.c
index 5de1dca..ce7d49e 100644
--- a/src/ui/gui_widgets/btc_only/multi_sig/gui_import_multisig_wallet_info_widgets.c
+++ b/src/ui/gui_widgets/btc_only/multi_sig/gui_import_multisig_wallet_info_widgets.c
@@ -144,25 +144,30 @@ void GuiImportMultisigWalletInfoWidgetsRestart()
void GuiImportMultisigWalletInfoVerifyPasswordSuccess(void)
{
uint8_t seed[64] = {0};
- int len = (GetMnemonicType() == MNEMONIC_TYPE_BIP39) ? sizeof(seed) : GetCurrentAccountEntropyLen();
+ int len = GetCurrentAccountSeedLen();
GetAccountSeed(GetCurrentAccountIndex(), seed, SecretCacheGetPassword());
uint8_t mfp[4] = {0};
GetMasterFingerPrint(mfp);
Response_MultiSigWallet *response = parse_and_verify_multisig_config(seed, len, g_wallet->config_text, mfp, 4);
if (response->error_code != 0) {
- printf("errorMessage: %s\r\n", response->error_message);
g_noticeWindow = GuiCreateErrorCodeWindow(ERR_MULTISIG_WALLET_CONFIG_INVALID, &g_noticeWindow, GuiCloseWarnningDialog);
- free_MultiSigWallet(response->data);
+ free_Response_MultiSigWallet(response);
+ memset_s(seed, sizeof(seed), 0, sizeof(seed));
+ ClearSecretCache();
return;
}
+ free_Response_MultiSigWallet(response);
MultiSigWalletItem_t *wallet = AddMultisigWalletToCurrentAccount(g_wallet, SecretCacheGetPassword());
if (wallet == NULL) {
- printf("multi sigwallet not found\n");
+ memset_s(seed, sizeof(seed), 0, sizeof(seed));
+ ClearSecretCache();
return;
}
GuiDeleteKeyboardWidget(g_keyboardWidget);
char *verifyCode = SRAM_MALLOC(MAX_VERIFY_CODE_LEN);
strcpy_s(verifyCode, MAX_VERIFY_CODE_LEN, wallet->verifyCode);
+ memset_s(seed, sizeof(seed), 0, sizeof(seed));
+ ClearSecretCache();
GuiCloseCurrentWorkingView();
GuiFrameOpenViewWithParam(&g_multisigWalletExportView, verifyCode, strnlen_s(verifyCode, MAX_VERIFY_CODE_LEN));
}
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.