diff --git a/legacy/firmware/.changelog.d/4396.added b/legacy/firmware/.changelog.d/4396.added new file mode 100644 index 0000000000..72b264b78e --- /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 4c9f027b24..81ec506360 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 b767859b76..4efe0087a3 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 81864cbf48..af53d42662 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 77ae1f7726..19dc54cf05 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 73d984a4ce..aec4b871c7 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 7a3da905d6..c6bef617f0 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 2becd97f67..cbdad20184 100644 --- a/tests/device_tests/bitcoin/test_multisig_change.py +++ b/tests/device_tests/bitcoin/test_multisig_change.py @@ -83,6 +83,30 @@ multisig_in3 = messages.MultisigRedeemScriptType( 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 @@ INP3 = messages.TxInputType( 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, + )