From d21381eefdad91695d01cf0058e544cab116e595 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Vejpustek?= Date: Fri, 22 Nov 2024 17:54:51 +0100 Subject: [PATCH 1/6] refactor(legacy): introduce cryptoMultisigPubkeys --- legacy/firmware/crypto.c | 19 +++++++++++++++++++ legacy/firmware/crypto.h | 5 +++++ legacy/firmware/transaction.c | 20 ++++++++++++++------ 3 files changed, 38 insertions(+), 6 deletions(-) diff --git a/legacy/firmware/crypto.c b/legacy/firmware/crypto.c index 08f457b1822..4c9f027b24e 100644 --- a/legacy/firmware/crypto.c +++ b/legacy/firmware/crypto.c @@ -368,6 +368,25 @@ uint32_t cryptoMultisigPubkeyCount(const MultisigRedeemScriptType *multisig) { : multisig->pubkeys_count; } +uint32_t cryptoMultisigPubkeys(const CoinInfo *coin, + const MultisigRedeemScriptType *multisig, + uint8_t *pubkeys) { + const uint32_t n = cryptoMultisigPubkeyCount(multisig); + if (n < 1 || n > 15) { + return 0; + } + + for (uint32_t i = 0; i < n; i++) { + const HDNode *pubnode = cryptoMultisigPubkey(coin, multisig, i); + if (!pubnode) { + return 0; + } + memcpy(pubkeys + i * 33, pubnode->public_key, 33); + } + + return n; +} + int cryptoMultisigPubkeyIndex(const CoinInfo *coin, const MultisigRedeemScriptType *multisig, const uint8_t *pubkey) { diff --git a/legacy/firmware/crypto.h b/legacy/firmware/crypto.h index 8df938fc268..98c81fbd8ef 100644 --- a/legacy/firmware/crypto.h +++ b/legacy/firmware/crypto.h @@ -88,6 +88,10 @@ int cryptoMultisigPubkeyIndex(const CoinInfo *coin, const MultisigRedeemScriptType *multisig, const uint8_t *pubkey); +uint32_t cryptoMultisigPubkeys(const CoinInfo *coin, + const MultisigRedeemScriptType *multisig, + uint8_t *pubkeys); + int cryptoMultisigFingerprint(const MultisigRedeemScriptType *multisig, uint8_t *hash); @@ -115,5 +119,6 @@ void slip21_from_seed(const uint8_t *seed, int seed_len, Slip21Node *out); void slip21_derive_path(Slip21Node *inout, const uint8_t *label, size_t label_len); const uint8_t *slip21_key(const Slip21Node *node); +bool multisig_uses_single_path(const MultisigRedeemScriptType *multisig); #endif diff --git a/legacy/firmware/transaction.c b/legacy/firmware/transaction.c index 8659dbf4b93..77ae1f77267 100644 --- a/legacy/firmware/transaction.c +++ b/legacy/firmware/transaction.c @@ -373,6 +373,12 @@ uint32_t compile_script_multisig(const CoinInfo *coin, const uint32_t n = cryptoMultisigPubkeyCount(multisig); if (m < 1 || m > 15) return 0; if (n < 1 || n > 15) return 0; + + uint8_t pubkeys[33 * n]; + if (!cryptoMultisigPubkeys(coin, multisig, pubkeys)) { + return 0; + } + uint32_t r = 0; if (out) { out[r] = 0x50 + m; @@ -380,9 +386,7 @@ uint32_t compile_script_multisig(const CoinInfo *coin, for (uint32_t i = 0; i < n; i++) { out[r] = 33; r++; // OP_PUSH 33 - const HDNode *pubnode = cryptoMultisigPubkey(coin, multisig, i); - if (!pubnode) return 0; - memcpy(out + r, pubnode->public_key, 33); + memcpy(out + r, pubkeys + 33 * i, 33); r += 33; } out[r] = 0x50 + n; @@ -409,6 +413,12 @@ uint32_t compile_script_multisig_hash(const CoinInfo *coin, if (m < 1 || m > 15) return 0; if (n < 1 || n > 15) return 0; + // allocate on stack instead of heap + uint8_t pubkeys[33 * n]; + if (!cryptoMultisigPubkeys(coin, multisig, pubkeys)) { + return 0; + } + Hasher hasher = {0}; hasher_Init(&hasher, coin->curve->hasher_script); @@ -418,9 +428,7 @@ uint32_t compile_script_multisig_hash(const CoinInfo *coin, for (uint32_t i = 0; i < n; i++) { d[0] = 33; hasher_Update(&hasher, d, 1); // OP_PUSH 33 - const HDNode *pubnode = cryptoMultisigPubkey(coin, multisig, i); - if (!pubnode) return 0; - hasher_Update(&hasher, pubnode->public_key, 33); + hasher_Update(&hasher, pubkeys + 33 * i, 33); } d[0] = 0x50 + n; d[1] = 0xAE; From 8fbd872bd1c416fcc9dfb29ef0e65e4cef456b08 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Vejpustek?= Date: Fri, 22 Nov 2024 18:44:19 +0100 Subject: [PATCH 2/6] feat(legacy): forbid multisig to singlesig change outputs --- legacy/firmware/.changelog.d/4396.changed | 1 + legacy/firmware/signing.c | 41 +++++++++++++++---- .../bitcoin/test_multisig_change.py | 4 +- 3 files changed, 36 insertions(+), 10 deletions(-) create mode 100644 legacy/firmware/.changelog.d/4396.changed diff --git a/legacy/firmware/.changelog.d/4396.changed b/legacy/firmware/.changelog.d/4396.changed new file mode 100644 index 00000000000..ba68fd7fa63 --- /dev/null +++ b/legacy/firmware/.changelog.d/4396.changed @@ -0,0 +1 @@ +Forbid multisig to singlesig change outputs. diff --git a/legacy/firmware/signing.c b/legacy/firmware/signing.c index 4d7d700e155..81864cbf48e 100644 --- a/legacy/firmware/signing.c +++ b/legacy/firmware/signing.c @@ -124,6 +124,8 @@ typedef struct { bool multisig_fp_set; bool multisig_fp_mismatch; uint8_t multisig_fp[32]; + bool multisig; + MatchState multisig_state; uint32_t in_address_n[8]; size_t in_address_n_count; InputScriptType in_script_type; @@ -978,6 +980,22 @@ static bool extract_input_multisig_fp(TxInfo *tx_info, return true; } +static void extract_multisig(TxInfo *tx_info, const TxInputType *txinput) { + switch (tx_info->multisig_state) { + case MatchState_MISMATCH: + return; + case MatchState_UNDEFINED: + tx_info->multisig_state = MatchState_MATCH; + tx_info->multisig = txinput->has_multisig; + return; + case MatchState_MATCH: + if (txinput->has_multisig != tx_info->multisig) { + tx_info->multisig_state = MatchState_MISMATCH; + } + return; + } +} + bool check_change_multisig_fp(const TxInfo *tx_info, const TxOutputType *txoutput) { uint8_t h[32] = {0}; @@ -986,6 +1004,12 @@ bool check_change_multisig_fp(const TxInfo *tx_info, memcmp(tx_info->multisig_fp, h, 32) == 0; } +bool check_change_multisig(const TxInfo *tx_info, + const TxOutputType *txoutput) { + return tx_info->multisig_state == MatchState_MATCH && + tx_info->multisig == txoutput->has_multisig; +} + static InputScriptType simple_script_type(InputScriptType script_type) { // SPENDMULTISIG is used only for non-SegWit and is effectively the same as // SPENDADDRESS. For SegWit inputs and outputs multisig is indicated by the @@ -1245,6 +1269,7 @@ static bool tx_info_init(TxInfo *tx_info, uint32_t inputs_count, tx_info->min_sequence = SEQUENCE_FINAL; tx_info->multisig_fp_set = false; tx_info->multisig_fp_mismatch = false; + tx_info->multisig_state = MatchState_UNDEFINED; tx_info->in_address_n_count = 0; tx_info->in_script_type = 0; tx_info->in_script_type_state = MatchState_UNDEFINED; @@ -1711,6 +1736,10 @@ static bool tx_info_add_input(TxInfo *tx_info, const TxInputType *txinput) { return false; } + // Remember whether all inputs are singlesig or all inputs are multisig. + // Change-outputs must be of the same type. + extract_multisig(tx_info, txinput); + // Remember the input's script type. Change-outputs must use the same script // type as all inputs. extract_input_script_type(tx_info, txinput); @@ -2069,18 +2098,14 @@ static bool is_change_output(const TxInfo *tx_info, return false; } - /* - * Check the multisig fingerprint only for multisig outputs. This means that - * a transfer from a multisig account to a singlesig account is treated as a - * change-output as long as all other change-output conditions are satisfied. - * This goes a bit against the concept of a multisig account, but the other - * cosigners will notice that they are relinquishing control of the funds, so - * there is no security risk. - */ if (txoutput->has_multisig && !check_change_multisig_fp(tx_info, txoutput)) { return false; } + if (!check_change_multisig(tx_info, txoutput)) { + return false; + } + if (!check_change_script_type(tx_info, txoutput)) { return false; } diff --git a/tests/device_tests/bitcoin/test_multisig_change.py b/tests/device_tests/bitcoin/test_multisig_change.py index 9703a9b6727..2becd97f67b 100644 --- a/tests/device_tests/bitcoin/test_multisig_change.py +++ b/tests/device_tests/bitcoin/test_multisig_change.py @@ -226,7 +226,7 @@ def test_external_internal(client: Client): client, INP1, INP2, - change_indices=[] if is_core(client) else [2], + change_indices=[], foreign_indices=[2], ) ) @@ -262,7 +262,7 @@ def test_internal_external(client: Client): client, INP1, INP2, - change_indices=[] if is_core(client) else [1], + change_indices=[], foreign_indices=[1], ) ) From 93f1ee7262624355c980500a4c01def3ad25dc0f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Vejpustek?= Date: Fri, 22 Nov 2024 18:08:29 +0100 Subject: [PATCH 3/6] feat(legacy): support sortedmulti --- legacy/firmware/.changelog.d/4396.added | 1 + legacy/firmware/crypto.c | 40 ++-- legacy/firmware/fsm_msg_coin.h | 9 +- legacy/firmware/signing.c | 30 +-- legacy/firmware/transaction.c | 16 -- tests/device_tests/bitcoin/test_getaddress.py | 1 - tests/device_tests/bitcoin/test_multisig.py | 1 - .../bitcoin/test_multisig_change.py | 181 +++++++++++++++++- 8 files changed, 227 insertions(+), 52 deletions(-) create mode 100644 legacy/firmware/.changelog.d/4396.added diff --git a/legacy/firmware/.changelog.d/4396.added b/legacy/firmware/.changelog.d/4396.added new file mode 100644 index 00000000000..72b264b78ec --- /dev/null +++ b/legacy/firmware/.changelog.d/4396.added @@ -0,0 +1 @@ +Added support for lexicographic sorting of pubkeys in multisig. diff --git a/legacy/firmware/crypto.c b/legacy/firmware/crypto.c index 4c9f027b24e..81ec506360e 100644 --- a/legacy/firmware/crypto.c +++ b/legacy/firmware/crypto.c @@ -18,6 +18,7 @@ */ #include "crypto.h" +#include #include #include "address.h" #include "aes/aes.h" @@ -368,6 +369,11 @@ uint32_t cryptoMultisigPubkeyCount(const MultisigRedeemScriptType *multisig) { : multisig->pubkeys_count; } +static int comparePubkeysLexicographically(const void *first, + const void *second) { + return memcmp(first, second, 33); +} + uint32_t cryptoMultisigPubkeys(const CoinInfo *coin, const MultisigRedeemScriptType *multisig, uint8_t *pubkeys) { @@ -384,6 +390,11 @@ uint32_t cryptoMultisigPubkeys(const CoinInfo *coin, memcpy(pubkeys + i * 33, pubnode->public_key, 33); } + if (multisig->has_pubkeys_order && + multisig->pubkeys_order == MultisigPubkeysOrder_LEXICOGRAPHIC) { + qsort(pubkeys, n, 33, comparePubkeysLexicographically); + } + return n; } @@ -399,9 +410,15 @@ int cryptoMultisigPubkeyIndex(const CoinInfo *coin, return -1; } +static int comparePubnodesLexicographically(const void *first, + const void *second) { + return memcmp(*(const HDNodeType **)first, *(const HDNodeType **)second, + sizeof(HDNodeType)); +} + int cryptoMultisigFingerprint(const MultisigRedeemScriptType *multisig, uint8_t *hash) { - static const HDNodeType *pubnodes[15], *swap; + static const HDNodeType *pubnodes[15]; const uint32_t n = cryptoMultisigPubkeyCount(multisig); if (n < 1 || n > 15) { return 0; @@ -422,21 +439,20 @@ int cryptoMultisigFingerprint(const MultisigRedeemScriptType *multisig, if (pubnodes[i]->public_key.size != 33) return 0; if (pubnodes[i]->chain_code.size != 32) return 0; } - // minsort according to pubkey - for (uint32_t i = 0; i < n - 1; i++) { - for (uint32_t j = n - 1; j > i; j--) { - if (memcmp(pubnodes[i]->public_key.bytes, pubnodes[j]->public_key.bytes, - 33) > 0) { - swap = pubnodes[i]; - pubnodes[i] = pubnodes[j]; - pubnodes[j] = swap; - } - } + + if (multisig->has_pubkeys_order && + multisig->pubkeys_order == MultisigPubkeysOrder_LEXICOGRAPHIC) { + // If the order of pubkeys is lexicographic, we don't want the fingerprint + // to depend on the order of the pubnodes, so we sort the pubnodes before + // hashing. + qsort(pubnodes, n, sizeof(HDNodeType *), comparePubnodesLexicographically); } - // hash sorted nodes + SHA256_CTX ctx = {0}; sha256_Init(&ctx); sha256_Update(&ctx, (const uint8_t *)&(multisig->m), sizeof(uint32_t)); + sha256_Update(&ctx, (const uint8_t *)&(multisig->pubkeys_order), + sizeof(uint32_t)); for (uint32_t i = 0; i < n; i++) { sha256_Update(&ctx, (const uint8_t *)&(pubnodes[i]->depth), sizeof(uint32_t)); diff --git a/legacy/firmware/fsm_msg_coin.h b/legacy/firmware/fsm_msg_coin.h index b767859b765..4efe0087a31 100644 --- a/legacy/firmware/fsm_msg_coin.h +++ b/legacy/firmware/fsm_msg_coin.h @@ -296,10 +296,15 @@ void fsm_msgGetAddress(const GetAddress *msg) { } if (msg->has_show_display && msg->show_display) { - char desc[20] = {0}; + char desc[29] = {0}; int multisig_index = 0; if (msg->has_multisig) { - strlcpy(desc, "Multisig __ of __:", sizeof(desc)); + if (msg->multisig.has_pubkeys_order && + msg->multisig.pubkeys_order == MultisigPubkeysOrder_LEXICOGRAPHIC) { + strlcpy(desc, "Multisig __ of __ (sorted):", sizeof(desc)); + } else { + strlcpy(desc, "Multisig __ of __:", sizeof(desc)); + } const uint32_t m = msg->multisig.m; const uint32_t n = cryptoMultisigPubkeyCount(&(msg->multisig)); desc[9] = (m < 10) ? ' ' : ('0' + (m / 10)); diff --git a/legacy/firmware/signing.c b/legacy/firmware/signing.c index 81864cbf48e..af53d426629 100644 --- a/legacy/firmware/signing.c +++ b/legacy/firmware/signing.c @@ -124,8 +124,8 @@ typedef struct { bool multisig_fp_set; bool multisig_fp_mismatch; uint8_t multisig_fp[32]; - bool multisig; - MatchState multisig_state; + bool is_multisig; + MatchState is_multisig_state; uint32_t in_address_n[8]; size_t in_address_n_count; InputScriptType in_script_type; @@ -980,17 +980,17 @@ static bool extract_input_multisig_fp(TxInfo *tx_info, return true; } -static void extract_multisig(TxInfo *tx_info, const TxInputType *txinput) { - switch (tx_info->multisig_state) { +static void extract_is_multisig(TxInfo *tx_info, const TxInputType *txinput) { + switch (tx_info->is_multisig_state) { case MatchState_MISMATCH: return; case MatchState_UNDEFINED: - tx_info->multisig_state = MatchState_MATCH; - tx_info->multisig = txinput->has_multisig; + tx_info->is_multisig_state = MatchState_MATCH; + tx_info->is_multisig = txinput->has_multisig; return; case MatchState_MATCH: - if (txinput->has_multisig != tx_info->multisig) { - tx_info->multisig_state = MatchState_MISMATCH; + if (txinput->has_multisig != tx_info->is_multisig) { + tx_info->is_multisig_state = MatchState_MISMATCH; } return; } @@ -1004,10 +1004,10 @@ bool check_change_multisig_fp(const TxInfo *tx_info, memcmp(tx_info->multisig_fp, h, 32) == 0; } -bool check_change_multisig(const TxInfo *tx_info, - const TxOutputType *txoutput) { - return tx_info->multisig_state == MatchState_MATCH && - tx_info->multisig == txoutput->has_multisig; +bool check_change_is_multisig(const TxInfo *tx_info, + const TxOutputType *txoutput) { + return tx_info->is_multisig_state == MatchState_MATCH && + tx_info->is_multisig == txoutput->has_multisig; } static InputScriptType simple_script_type(InputScriptType script_type) { @@ -1269,7 +1269,7 @@ static bool tx_info_init(TxInfo *tx_info, uint32_t inputs_count, tx_info->min_sequence = SEQUENCE_FINAL; tx_info->multisig_fp_set = false; tx_info->multisig_fp_mismatch = false; - tx_info->multisig_state = MatchState_UNDEFINED; + tx_info->is_multisig_state = MatchState_UNDEFINED; tx_info->in_address_n_count = 0; tx_info->in_script_type = 0; tx_info->in_script_type_state = MatchState_UNDEFINED; @@ -1738,7 +1738,7 @@ static bool tx_info_add_input(TxInfo *tx_info, const TxInputType *txinput) { // Remember whether all inputs are singlesig or all inputs are multisig. // Change-outputs must be of the same type. - extract_multisig(tx_info, txinput); + extract_is_multisig(tx_info, txinput); // Remember the input's script type. Change-outputs must use the same script // type as all inputs. @@ -2102,7 +2102,7 @@ static bool is_change_output(const TxInfo *tx_info, return false; } - if (!check_change_multisig(tx_info, txoutput)) { + if (!check_change_is_multisig(tx_info, txoutput)) { return false; } diff --git a/legacy/firmware/transaction.c b/legacy/firmware/transaction.c index 77ae1f77267..19dc54cf052 100644 --- a/legacy/firmware/transaction.c +++ b/legacy/firmware/transaction.c @@ -364,11 +364,6 @@ uint32_t compile_script_sig(uint32_t address_type, const uint8_t *pubkeyhash, uint32_t compile_script_multisig(const CoinInfo *coin, const MultisigRedeemScriptType *multisig, uint8_t *out) { - if (multisig->pubkeys_order != MultisigPubkeysOrder_PRESERVED) { - fsm_sendFailure(FailureType_Failure_DataError, - _("Sortedmulti is not supported")); - return 0; - } const uint32_t m = multisig->m; const uint32_t n = cryptoMultisigPubkeyCount(multisig); if (m < 1 || m > 15) return 0; @@ -402,12 +397,6 @@ uint32_t compile_script_multisig(const CoinInfo *coin, uint32_t compile_script_multisig_hash(const CoinInfo *coin, const MultisigRedeemScriptType *multisig, uint8_t *hash) { - if (multisig->pubkeys_order != MultisigPubkeysOrder_PRESERVED) { - fsm_sendFailure(FailureType_Failure_DataError, - _("Sortedmulti is not supported")); - return 0; - } - const uint32_t m = multisig->m; const uint32_t n = cryptoMultisigPubkeyCount(multisig); if (m < 1 || m > 15) return 0; @@ -457,11 +446,6 @@ uint32_t serialize_script_sig(const uint8_t *signature, uint32_t signature_len, uint32_t serialize_script_multisig(const CoinInfo *coin, const MultisigRedeemScriptType *multisig, uint8_t sighash, uint8_t *out) { - if (multisig->pubkeys_order != MultisigPubkeysOrder_PRESERVED) { - fsm_sendFailure(FailureType_Failure_DataError, - _("Sortedmulti is not supported")); - return 0; - } uint32_t r = 0; #if !BITCOIN_ONLY if (!coin->decred) { diff --git a/tests/device_tests/bitcoin/test_getaddress.py b/tests/device_tests/bitcoin/test_getaddress.py index 73d984a4cec..aec4b871c7f 100644 --- a/tests/device_tests/bitcoin/test_getaddress.py +++ b/tests/device_tests/bitcoin/test_getaddress.py @@ -197,7 +197,6 @@ def test_altcoin_address_mac(client: Client): @pytest.mark.multisig -@pytest.mark.models(skip="legacy", reason="Sortedmulti is not supported") def test_multisig_pubkeys_order(client: Client): xpub_internal = btc.get_public_node(client, parse_path("m/45h/0")).xpub xpub_external = btc.get_public_node(client, parse_path("m/44h/1")).xpub diff --git a/tests/device_tests/bitcoin/test_multisig.py b/tests/device_tests/bitcoin/test_multisig.py index 7a3da905d68..c6bef617f01 100644 --- a/tests/device_tests/bitcoin/test_multisig.py +++ b/tests/device_tests/bitcoin/test_multisig.py @@ -162,7 +162,6 @@ def test_2_of_3(client: Client, chunkify: bool): @pytest.mark.multisig -@pytest.mark.models(skip="legacy", reason="Sortedmulti is not supported") def test_pubkeys_order(client: Client): node_internal = btc.get_public_node( client, parse_path("m/45h/0"), coin_name="Bitcoin" diff --git a/tests/device_tests/bitcoin/test_multisig_change.py b/tests/device_tests/bitcoin/test_multisig_change.py index 2becd97f67b..cbdad201846 100644 --- a/tests/device_tests/bitcoin/test_multisig_change.py +++ b/tests/device_tests/bitcoin/test_multisig_change.py @@ -83,6 +83,30 @@ m=2, ) +multisig_in4 = messages.MultisigRedeemScriptType( + nodes=[NODE_EXT1, NODE_EXT2, NODE_INT], + address_n=[0, 0], + signatures=[b"", b"", b""], + m=2, + pubkeys_order=messages.MultisigPubkeysOrder.LEXICOGRAPHIC, +) + +multisig_in5 = messages.MultisigRedeemScriptType( + nodes=[NODE_EXT2, NODE_EXT1, NODE_INT], + address_n=[0, 1], + signatures=[b"", b"", b""], + m=2, + pubkeys_order=messages.MultisigPubkeysOrder.LEXICOGRAPHIC, +) + +multisig_in6 = messages.MultisigRedeemScriptType( + nodes=[NODE_EXT1, NODE_EXT3, NODE_INT], + address_n=[0, 1], + signatures=[b"", b"", b""], + m=2, + pubkeys_order=messages.MultisigPubkeysOrder.LEXICOGRAPHIC, +) + prev_hash_1, prev_tx_1 = forge_prevtx( [("3Ltgk5WPUMLcT2QvwRXKj9CWsYuAKqeHJ8", 50_000_000)] ) @@ -119,7 +143,51 @@ multisig=multisig_in3, ) -TX_API = {prev_hash_1: prev_tx_1, prev_hash_2: prev_tx_2, prev_hash_3: prev_tx_3} +prev_hash_4, prev_tx_4 = forge_prevtx( + [("3HwrvQEfYw4wUvGHpGmixWB15HPgqrvTh1", 50_000_000)] +) +INP4 = messages.TxInputType( + address_n=[H_(45), 0, 0, 0], + amount=50_000_000, + prev_hash=prev_hash_4, + prev_index=0, + script_type=messages.InputScriptType.SPENDMULTISIG, + multisig=multisig_in4, +) + +prev_hash_5, prev_tx_5 = forge_prevtx( + [("3Md42fbNjSH3qwnj5jDvT6CSzJKVXHiXSc", 34_500_000)] +) +INP5 = messages.TxInputType( + address_n=[H_(45), 0, 0, 1], + amount=34_500_000, + prev_hash=prev_hash_5, + prev_index=0, + script_type=messages.InputScriptType.SPENDMULTISIG, + multisig=multisig_in5, +) + +prev_hash_6, prev_tx_6 = forge_prevtx( + [("35PBSvszuvxhEDypGYcUhEQDigvKY8C5Rc", 55_500_000)] +) +INP6 = messages.TxInputType( + address_n=[H_(45), 0, 0, 1], + amount=55_500_000, + prev_hash=prev_hash_6, + prev_index=0, + script_type=messages.InputScriptType.SPENDMULTISIG, + multisig=multisig_in6, +) + + +TX_API = { + prev_hash_1: prev_tx_1, + prev_hash_2: prev_tx_2, + prev_hash_3: prev_tx_3, + prev_hash_4: prev_tx_4, + prev_hash_5: prev_tx_5, + prev_hash_6: prev_tx_6, +} def _responses( @@ -373,10 +441,46 @@ def test_multisig_change_match_second(client: Client): ) -# inputs match, change mismatches (second tries to be change but isn't) +# inputs match, change matches (first is change) +def test_sorted_multisig_change_match_first(client: Client): + multisig_out1 = messages.MultisigRedeemScriptType( + nodes=[NODE_EXT1, NODE_INT, NODE_EXT2], + address_n=[1, 0], + signatures=[b"", b"", b""], + m=2, + pubkeys_order=messages.MultisigPubkeysOrder.LEXICOGRAPHIC, + ) + + out1 = messages.TxOutputType( + address_n=[H_(45), 0, 1, 0], + multisig=multisig_out1, + amount=40_000_000, + script_type=messages.OutputScriptType.PAYTOMULTISIG, + ) + + out2 = messages.TxOutputType( + address="3PkXLsY7AUZCrCKGvX8FfP2EawowUBMbcg", + amount=44_000_000, + script_type=messages.OutputScriptType.PAYTOADDRESS, + ) + + with client: + client.set_expected_responses( + _responses(client, INP4, INP5, change_indices=[1]) + ) + btc.sign_tx( + client, + "Bitcoin", + [INP4, INP5], + [out1, out2], + prev_txes=TX_API, + ) + + +# inputs match, change mismatches (second tries to be change but isn't because the pubkeys are in different order) def test_multisig_mismatch_multisig_change(client: Client): multisig_out2 = messages.MultisigRedeemScriptType( - nodes=[NODE_EXT1, NODE_INT, NODE_EXT3], + nodes=[NODE_EXT1, NODE_INT, NODE_EXT2], address_n=[1, 0], signatures=[b"", b"", b""], m=2, @@ -406,6 +510,39 @@ def test_multisig_mismatch_multisig_change(client: Client): ) +# inputs match, change mismatches (second tries to be change but isn't because the pubkeys are different) +def test_sorted_multisig_mismatch_multisig_change(client: Client): + multisig_out2 = messages.MultisigRedeemScriptType( + nodes=[NODE_EXT1, NODE_INT, NODE_EXT3], + address_n=[1, 0], + signatures=[b"", b"", b""], + m=2, + ) + + out1 = messages.TxOutputType( + address="3B23k4kFBRtu49zvpG3Z9xuFzfpHvxBcwt", + amount=40_000_000, + script_type=messages.OutputScriptType.PAYTOADDRESS, + ) + + out2 = messages.TxOutputType( + address_n=[H_(45), 0, 1, 0], + multisig=multisig_out2, + amount=44_000_000, + script_type=messages.OutputScriptType.PAYTOMULTISIG, + ) + + with client: + client.set_expected_responses(_responses(client, INP4, INP5)) + btc.sign_tx( + client, + "Bitcoin", + [INP4, INP5], + [out1, out2], + prev_txes=TX_API, + ) + + # 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): @@ -443,10 +580,10 @@ def test_multisig_mismatch_multisig_change_different_paths(client: Client): ) -# inputs mismatch, change matches with first input +# inputs mismatch because the pubkeys are different, change matches with first input def test_multisig_mismatch_inputs(client: Client): multisig_out1 = messages.MultisigRedeemScriptType( - nodes=[NODE_EXT2, NODE_EXT1, NODE_INT], + nodes=[NODE_EXT1, NODE_EXT2, NODE_INT], address_n=[1, 0], signatures=[b"", b"", b""], m=2, @@ -474,3 +611,37 @@ def test_multisig_mismatch_inputs(client: Client): [out1, out2], prev_txes=TX_API, ) + + +# inputs mismatch because the pubkeys are different, change matches with first input +def test_sorted_multisig_mismatch_inputs(client: Client): + multisig_out1 = messages.MultisigRedeemScriptType( + nodes=[NODE_EXT1, NODE_EXT2, NODE_INT], + address_n=[1, 0], + signatures=[b"", b"", b""], + m=2, + pubkeys_order=messages.MultisigPubkeysOrder.LEXICOGRAPHIC, + ) + + out1 = messages.TxOutputType( + address_n=[H_(45), 0, 1, 0], + multisig=multisig_out1, + amount=40_000_000, + script_type=messages.OutputScriptType.PAYTOMULTISIG, + ) + + out2 = messages.TxOutputType( + address="3PkXLsY7AUZCrCKGvX8FfP2EawowUBMbcg", + amount=65_000_000, + script_type=messages.OutputScriptType.PAYTOADDRESS, + ) + + with client: + client.set_expected_responses(_responses(client, INP4, INP6)) + btc.sign_tx( + client, + "Bitcoin", + [INP4, INP6], + [out1, out2], + prev_txes=TX_API, + ) From fbea03b16aaa688b7dff6269399ea14c36bd4e01 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Vejpustek?= Date: Fri, 22 Nov 2024 19:08:54 +0100 Subject: [PATCH 4/6] fix(legacy): disallow using per-node paths --- legacy/firmware/.changelog.d/4396.changed.2 | 1 + legacy/firmware/crypto.c | 24 +++++++++++++++++++ legacy/firmware/fsm.c | 11 +++++++++ legacy/firmware/fsm.h | 1 + legacy/firmware/fsm_msg_coin.h | 21 ++++++++++++++++ legacy/firmware/signing.c | 18 ++++++++++++-- tests/device_tests/bitcoin/test_getaddress.py | 1 - .../bitcoin/test_multisig_change.py | 3 +-- 8 files changed, 75 insertions(+), 5 deletions(-) create mode 100644 legacy/firmware/.changelog.d/4396.changed.2 diff --git a/legacy/firmware/.changelog.d/4396.changed.2 b/legacy/firmware/.changelog.d/4396.changed.2 new file mode 100644 index 00000000000..87cd7c8ae52 --- /dev/null +++ b/legacy/firmware/.changelog.d/4396.changed.2 @@ -0,0 +1 @@ +Forbid per-node paths in multisig change outputs and multisig receive addresses. diff --git a/legacy/firmware/crypto.c b/legacy/firmware/crypto.c index 81ec506360e..5e36bb275b4 100644 --- a/legacy/firmware/crypto.c +++ b/legacy/firmware/crypto.c @@ -944,3 +944,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 07c4c24b1cd..6bf1d0cb85a 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 8b31c999148..37d2183c020 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 4efe0087a31..74fbb362e33 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 af53d426629..917c1abfd60 100644 --- a/legacy/firmware/signing.c +++ b/legacy/firmware/signing.c @@ -2098,8 +2098,22 @@ 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)) { + return false; + } + if (!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_is_multisig(tx_info, txoutput)) { diff --git a/tests/device_tests/bitcoin/test_getaddress.py b/tests/device_tests/bitcoin/test_getaddress.py index aec4b871c7f..f3bc18b81aa 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 cbdad201846..7beaa31badc 100644 --- a/tests/device_tests/bitcoin/test_multisig_change.py +++ b/tests/device_tests/bitcoin/test_multisig_change.py @@ -543,8 +543,7 @@ def test_sorted_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") +# inputs match, change mismatches (second tries to be change but isn't because is uses per-node paths) def test_multisig_mismatch_multisig_change_different_paths(client: Client): multisig_out2 = messages.MultisigRedeemScriptType( pubkeys=[ From c9adb55c07908128ff388fa3ecff485d46b789c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Vejpustek?= Date: Fri, 22 Nov 2024 19:14:33 +0100 Subject: [PATCH 5/6] feat(core): remove deprecated path --- legacy/firmware/.changelog.d/4396.changed.3 | 1 + legacy/firmware/crypto.c | 12 +----------- 2 files changed, 2 insertions(+), 11 deletions(-) create mode 100644 legacy/firmware/.changelog.d/4396.changed.3 diff --git a/legacy/firmware/.changelog.d/4396.changed.3 b/legacy/firmware/.changelog.d/4396.changed.3 new file mode 100644 index 00000000000..07d5ba35c8d --- /dev/null +++ b/legacy/firmware/.changelog.d/4396.changed.3 @@ -0,0 +1 @@ +Remove deprecated Unchained Capital's multisig path. diff --git a/legacy/firmware/crypto.c b/legacy/firmware/crypto.c index 5e36bb275b4..a5ba9114922 100644 --- a/legacy/firmware/crypto.c +++ b/legacy/firmware/crypto.c @@ -562,17 +562,7 @@ bool coin_path_check(const CoinInfo *coin, InputScriptType script_type, valid = valid && (address_n[2] <= PATH_MAX_CHANGE); valid = valid && (address_n[3] <= PATH_MAX_ADDRESS_INDEX); } else if (address_n_count == 5) { - if (address_n[1] & PATH_HARDENED) { - // Unchained Capital compatibility pattern. Will be removed in the - // future. - // m / 45' / coin_type' / account' / [0-1000000] / address_index - valid = valid && check_cointype(coin, address_n[1], full_check); - valid = valid && (address_n[2] & PATH_HARDENED); - valid = - valid && ((address_n[2] & PATH_UNHARDEN_MASK) <= PATH_MAX_ACCOUNT); - valid = valid && (address_n[3] <= 1000000); - valid = valid && (address_n[4] <= PATH_MAX_ADDRESS_INDEX); - } else { + if ((address_n[1] & PATH_HARDENED) == 0) { // Casa proposed "universal multisig" pattern with unhardened parts. // m/45'/coin_type/account/change/address_index valid = valid && From cf7312ccac817eeb38874caec9e4f5a17217c00c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Vejpustek?= Date: Thu, 12 Dec 2024 14:40:30 +0100 Subject: [PATCH 6/6] fixup! feat(legacy): support sortedmulti --- legacy/firmware/crypto.c | 18 +++++++++++++++ legacy/firmware/crypto.h | 4 ++++ legacy/firmware/fsm_msg_coin.h | 2 +- tests/device_tests/bitcoin/test_multisig.py | 25 ++++++++++++++++----- 4 files changed, 42 insertions(+), 7 deletions(-) diff --git a/legacy/firmware/crypto.c b/legacy/firmware/crypto.c index a5ba9114922..c84e5193405 100644 --- a/legacy/firmware/crypto.c +++ b/legacy/firmware/crypto.c @@ -401,6 +401,24 @@ uint32_t cryptoMultisigPubkeys(const CoinInfo *coin, int cryptoMultisigPubkeyIndex(const CoinInfo *coin, const MultisigRedeemScriptType *multisig, const uint8_t *pubkey) { + uint32_t n = cryptoMultisigPubkeyCount(multisig); + + uint8_t pubkeys[33 * n]; + if (!cryptoMultisigPubkeys(coin, multisig, pubkeys)) { + return -1; + } + + for (size_t i = 0; i < n; i++) { + if (memcmp(pubkeys + i * 33, pubkey, 33) == 0) { + return i; + } + } + return -1; +} + +int cryptoMultisigXpubIndex(const CoinInfo *coin, + const MultisigRedeemScriptType *multisig, + const uint8_t *pubkey) { for (size_t i = 0; i < cryptoMultisigPubkeyCount(multisig); i++) { const HDNode *pubnode = cryptoMultisigPubkey(coin, multisig, i); if (pubnode && memcmp(pubnode->public_key, pubkey, 33) == 0) { diff --git a/legacy/firmware/crypto.h b/legacy/firmware/crypto.h index 98c81fbd8ef..02c8566eaae 100644 --- a/legacy/firmware/crypto.h +++ b/legacy/firmware/crypto.h @@ -88,6 +88,10 @@ int cryptoMultisigPubkeyIndex(const CoinInfo *coin, const MultisigRedeemScriptType *multisig, const uint8_t *pubkey); +int cryptoMultisigXpubIndex(const CoinInfo *coin, + const MultisigRedeemScriptType *multisig, + const uint8_t *pubkey); + uint32_t cryptoMultisigPubkeys(const CoinInfo *coin, const MultisigRedeemScriptType *multisig, uint8_t *pubkeys); diff --git a/legacy/firmware/fsm_msg_coin.h b/legacy/firmware/fsm_msg_coin.h index 74fbb362e33..0745e91182a 100644 --- a/legacy/firmware/fsm_msg_coin.h +++ b/legacy/firmware/fsm_msg_coin.h @@ -333,7 +333,7 @@ void fsm_msgGetAddress(const GetAddress *msg) { desc[15] = (n < 10) ? ' ' : ('0' + (n / 10)); desc[16] = '0' + (n % 10); multisig_index = - cryptoMultisigPubkeyIndex(coin, &(msg->multisig), node->public_key); + cryptoMultisigXpubIndex(coin, &(msg->multisig), node->public_key); } else { strlcpy(desc, _("Address:"), sizeof(desc)); } diff --git a/tests/device_tests/bitcoin/test_multisig.py b/tests/device_tests/bitcoin/test_multisig.py index c6bef617f01..2a01db8108f 100644 --- a/tests/device_tests/bitcoin/test_multisig.py +++ b/tests/device_tests/bitcoin/test_multisig.py @@ -170,10 +170,13 @@ def test_pubkeys_order(client: Client): client, parse_path("m/45h/1"), coin_name="Bitcoin" ).node + # A dummy signature is used to ensure that the signatures are serialized in the correct order + dummy_signature = bytes(71) + multisig_unsorted_1 = messages.MultisigRedeemScriptType( nodes=[node_internal, node_external], address_n=[0, 0], - signatures=[b"", b"", b""], + signatures=[b"", dummy_signature], m=1, pubkeys_order=messages.MultisigPubkeysOrder.PRESERVED, ) @@ -181,7 +184,7 @@ def test_pubkeys_order(client: Client): multisig_unsorted_2 = messages.MultisigRedeemScriptType( nodes=[node_external, node_internal], address_n=[0, 0], - signatures=[b"", b"", b""], + signatures=[dummy_signature, b""], m=1, pubkeys_order=messages.MultisigPubkeysOrder.PRESERVED, ) @@ -189,7 +192,7 @@ def test_pubkeys_order(client: Client): multisig_sorted_1 = messages.MultisigRedeemScriptType( nodes=[node_internal, node_external], address_n=[0, 0], - signatures=[b"", b"", b""], + signatures=[b"", dummy_signature], m=1, pubkeys_order=messages.MultisigPubkeysOrder.LEXICOGRAPHIC, ) @@ -197,7 +200,7 @@ def test_pubkeys_order(client: Client): multisig_sorted_2 = messages.MultisigRedeemScriptType( nodes=[node_external, node_internal], address_n=[0, 0], - signatures=[b"", b"", b""], + signatures=[b"", dummy_signature], m=1, pubkeys_order=messages.MultisigPubkeysOrder.LEXICOGRAPHIC, ) @@ -209,6 +212,16 @@ def test_pubkeys_order(client: Client): client, "Bitcoin", parse_path("m/45h/0/0/0"), multisig=multisig_unsorted_2 ) + pubkey_internal = btc.get_public_node( + client, parse_path("m/45h/0/0/0"), coin_name="Bitcoin" + ).node.public_key + pubkey_external = btc.get_public_node( + client, parse_path("m/45h/1/0/0"), coin_name="Bitcoin" + ).node.public_key + + # This assertion implies that script pubkey of multisig_sorted_1, multisig_sorted_2 and multisig_unsorted_1 are the same + assert pubkey_internal < pubkey_external + prev_hash, prev_tx = forge_prevtx( [(address_unsorted_1, 1_000_000), (address_unsorted_2, 1_000_000)] ) @@ -277,9 +290,9 @@ def test_pubkeys_order(client: Client): multisig=multisig_sorted_2, ) - tx_unsorted_1 = "0100000001637ffac0d4fbd8a6c02b114e36b079615ec3e4bdf09b769c7bf8b5fd6f8e7817000000009200483045022100f062d71445509d84a5769f219f4f0158dc7ff4351d671e6fa6bfdb171435064802206e1104f1d14f010bcc166cca6a9bd24b4a497475f8e23167858b4a3e92ac6a67014751210262e9ac5bea4c84c7dea650424ed768cf123af9e447eef3c63d37c41d1f825e49210369b79f2094a6eb89e7aff0e012a5699f7272968a341e48e99e64a54312f2932b52aeffffffff01301b0f000000000017a91440bfd8ae9d806d5bd7cf475ce6a80535836285148700000000" + tx_unsorted_1 = "0100000001637ffac0d4fbd8a6c02b114e36b079615ec3e4bdf09b769c7bf8b5fd6f8e781700000000db00483045022100f062d71445509d84a5769f219f4f0158dc7ff4351d671e6fa6bfdb171435064802206e1104f1d14f010bcc166cca6a9bd24b4a497475f8e23167858b4a3e92ac6a6701480000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000014751210262e9ac5bea4c84c7dea650424ed768cf123af9e447eef3c63d37c41d1f825e49210369b79f2094a6eb89e7aff0e012a5699f7272968a341e48e99e64a54312f2932b52aeffffffff01301b0f000000000017a91440bfd8ae9d806d5bd7cf475ce6a80535836285148700000000" - tx_unsorted_2 = "0100000001637ffac0d4fbd8a6c02b114e36b079615ec3e4bdf09b769c7bf8b5fd6f8e781701000000910047304402204914036468434698e2d87985007a66691f170195e4a16507bbb86b4c00da5fde02200a788312d447b3796ee5288ce9e9c0247896debfa473339302bc928da6dd78cb014751210369b79f2094a6eb89e7aff0e012a5699f7272968a341e48e99e64a54312f2932b210262e9ac5bea4c84c7dea650424ed768cf123af9e447eef3c63d37c41d1f825e4952aeffffffff01301b0f000000000017a914320ad0ff0f1b605ab1fa8e29b70d22827cf45a9f8700000000" + tx_unsorted_2 = "0100000001637ffac0d4fbd8a6c02b114e36b079615ec3e4bdf09b769c7bf8b5fd6f8e781701000000da004800000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000147304402204914036468434698e2d87985007a66691f170195e4a16507bbb86b4c00da5fde02200a788312d447b3796ee5288ce9e9c0247896debfa473339302bc928da6dd78cb014751210369b79f2094a6eb89e7aff0e012a5699f7272968a341e48e99e64a54312f2932b210262e9ac5bea4c84c7dea650424ed768cf123af9e447eef3c63d37c41d1f825e4952aeffffffff01301b0f000000000017a914320ad0ff0f1b605ab1fa8e29b70d22827cf45a9f8700000000" _, tx = btc.sign_tx( client,