diff --git a/legacy/firmware/crypto.c b/legacy/firmware/crypto.c index 4e8403e1d4..1c86bc6bc4 100644 --- a/legacy/firmware/crypto.c +++ b/legacy/firmware/crypto.c @@ -934,3 +934,27 @@ bool cryptoCosiVerify(const ed25519_signature signature, const uint8_t *message, res = ed25519_sign_open(message, message_len, pk_combined, signature); return res == 0; } + +bool multisig_uses_single_path(const MultisigRedeemScriptType *multisig) { + if (multisig->pubkeys_count == 0) { + // Pubkeys are specified by multisig.nodes and multisig.address_n, in this + // case all the pubkeys use the same path + return true; + } else { + // Pubkeys are specified by multisig.pubkeys, in this case we check that all + // the pubkeys use the same path + for (int i = 0; i < multisig->pubkeys_count; i++) { + if (multisig->pubkeys[i].address_n_count != + multisig->pubkeys[0].address_n_count) { + return false; + } + for (int j = 0; j < multisig->pubkeys[i].address_n_count; j++) { + if (multisig->pubkeys[i].address_n[j] != + multisig->pubkeys[0].address_n[j]) { + return false; + } + } + } + return true; + } +} diff --git a/legacy/firmware/fsm.c b/legacy/firmware/fsm.c index 07c4c24b1c..6bf1d0cb85 100644 --- a/legacy/firmware/fsm.c +++ b/legacy/firmware/fsm.c @@ -442,6 +442,17 @@ bool fsm_layoutPathWarning(void) { return true; } +bool fsm_layoutDifferentPathsWarning(void) { + layoutDialogSwipe(&bmp_icon_warning, _("Abort"), _("Continue"), NULL, + _("Ussing different paths"), _("for different XPUBs."), + NULL, _("Continue at your"), _("own risk!"), NULL); + if (!protectButton(ButtonRequestType_ButtonRequest_Warning, false)) { + fsm_sendFailure(FailureType_Failure_ActionCancelled, NULL); + return false; + } + return true; +} + #include "fsm_msg_coin.h" #include "fsm_msg_common.h" #include "fsm_msg_crypto.h" diff --git a/legacy/firmware/fsm.h b/legacy/firmware/fsm.h index 8b31c99914..37d2183c02 100644 --- a/legacy/firmware/fsm.h +++ b/legacy/firmware/fsm.h @@ -152,6 +152,7 @@ bool fsm_layoutSignMessage(const uint8_t *msg, uint32_t len); bool fsm_layoutVerifyMessage(const uint8_t *msg, uint32_t len); bool fsm_layoutPathWarning(void); +bool fsm_layoutDifferentPathsWarning(void); bool fsm_checkCoinPath(const CoinInfo *coin, InputScriptType script_type, uint32_t address_n_count, const uint32_t *address_n, bool has_multisig, MessageType message_type, diff --git a/legacy/firmware/fsm_msg_coin.h b/legacy/firmware/fsm_msg_coin.h index 4efe0087a3..74fbb362e3 100644 --- a/legacy/firmware/fsm_msg_coin.h +++ b/legacy/firmware/fsm_msg_coin.h @@ -299,6 +299,27 @@ void fsm_msgGetAddress(const GetAddress *msg) { char desc[29] = {0}; int multisig_index = 0; if (msg->has_multisig) { + if (!multisig_uses_single_path(&(msg->multisig))) { + // An address that uses different derivation paths for different xpubs + // could be difficult to discover if the user did not note all the + // paths. The reason is that each path ends with an address index, which + // can have 1,000,000 possible values. If the address is a t-out-of-n + // multisig, the total number of possible paths is 1,000,000^n. This can + // be exploited by an attacker who has compromised the user's computer. + // The attacker could randomize the address indices and then demand a + // ransom from the user to reveal the paths. To prevent this, we require + // that all xpubs use the same derivation path. + if (config_getSafetyCheckLevel() == SafetyCheckLevel_Strict) { + fsm_sendFailure( + FailureType_Failure_DataError, + _("Using different paths for different xpubs is not allowed" + + )); + layoutHome(); + return; + } + fsm_layoutDifferentPathsWarning(); + } if (msg->multisig.has_pubkeys_order && msg->multisig.pubkeys_order == MultisigPubkeysOrder_LEXICOGRAPHIC) { strlcpy(desc, "Multisig __ of __ (sorted):", sizeof(desc)); diff --git a/legacy/firmware/signing.c b/legacy/firmware/signing.c index f4ac2d3c47..9428870df4 100644 --- a/legacy/firmware/signing.c +++ b/legacy/firmware/signing.c @@ -2100,8 +2100,20 @@ static bool is_change_output(const TxInfo *tx_info, return false; } - if (txoutput->has_multisig && !check_change_multisig_fp(tx_info, txoutput)) { - return false; + if (txoutput->has_multisig) { + if (!(check_change_multisig_fp(tx_info, txoutput) && + multisig_uses_single_path(&(txoutput->multisig)))) { + // An address that uses different derivation paths for different xpubs + // could be difficult to discover if the user did not note all the paths. + // The reason is that each path ends with an address index, which can have + // 1,000,000 possible values. If the address is a t-out-of-n multisig, the + // total number of possible paths is 1,000,000^n. This can be exploited by + // an attacker who has compromised the user's computer. The attacker could + // randomize the address indices and then demand a ransom from the user to + // reveal the paths. To prevent this, we require that all xpubs use the + // same derivation path. + return false; + } } if (!check_change_multisig(tx_info, txoutput)) { diff --git a/tests/device_tests/bitcoin/test_getaddress.py b/tests/device_tests/bitcoin/test_getaddress.py index aec4b871c7..f3bc18b81a 100644 --- a/tests/device_tests/bitcoin/test_getaddress.py +++ b/tests/device_tests/bitcoin/test_getaddress.py @@ -437,7 +437,6 @@ def test_crw(client: Client): @pytest.mark.multisig -@pytest.mark.models(skip="legacy", reason="Not fixed") def test_multisig_different_paths(client: Client): nodes = [ btc.get_public_node(client, parse_path(f"m/45h/{i}"), coin_name="Bitcoin").node diff --git a/tests/device_tests/bitcoin/test_multisig_change.py b/tests/device_tests/bitcoin/test_multisig_change.py index 2becd97f67..859e859ae9 100644 --- a/tests/device_tests/bitcoin/test_multisig_change.py +++ b/tests/device_tests/bitcoin/test_multisig_change.py @@ -407,7 +407,6 @@ def test_multisig_mismatch_multisig_change(client: Client): # inputs match, change mismatches (second tries to be change but isn't) -@pytest.mark.models(skip="legacy", reason="Not fixed") def test_multisig_mismatch_multisig_change_different_paths(client: Client): multisig_out2 = messages.MultisigRedeemScriptType( pubkeys=[