diff --git a/core/.changelog.d/4285.feat b/core/.changelog.d/4285.feat new file mode 100644 index 0000000000..46ad33772a --- /dev/null +++ b/core/.changelog.d/4285.feat @@ -0,0 +1 @@ +Forbidden multisig to singlesig change outputs. diff --git a/core/src/apps/bitcoin/sign_tx/matchcheck.py b/core/src/apps/bitcoin/sign_tx/matchcheck.py index a9db1aa11e..3cedb75034 100644 --- a/core/src/apps/bitcoin/sign_tx/matchcheck.py +++ b/core/src/apps/bitcoin/sign_tx/matchcheck.py @@ -107,6 +107,11 @@ class MultisigFingerprintChecker(MatchChecker): return multisig.multisig_fingerprint(txio.multisig) +class MultisigChecker(MatchChecker): + def attribute_from_tx(self, txio: TxInput | TxOutput) -> Any: + return txio.multisig is not None + + class ScriptTypeChecker(MatchChecker): def attribute_from_tx(self, txio: TxInput | TxOutput) -> Any: from trezor.enums import InputScriptType diff --git a/core/src/apps/bitcoin/sign_tx/tx_info.py b/core/src/apps/bitcoin/sign_tx/tx_info.py index deb10dfd04..200a984b04 100644 --- a/core/src/apps/bitcoin/sign_tx/tx_info.py +++ b/core/src/apps/bitcoin/sign_tx/tx_info.py @@ -53,11 +53,15 @@ class TxInfoBase: from trezor.utils import HashWriter from .matchcheck import ( + MultisigChecker, MultisigFingerprintChecker, ScriptTypeChecker, WalletPathChecker, ) + # Whether all inputs are multisig or all inputs are singlesig, used to validate change-output. + self.multisig = MultisigChecker() + # Checksum of multisig inputs, used to validate change-output. self.multisig_fingerprint = MultisigFingerprintChecker() @@ -90,6 +94,7 @@ class TxInfoBase: self.min_sequence = min(self.min_sequence, txi.sequence) if not common.input_is_external(txi): + self.multisig.add_input(txi) self.wallet_path.add_input(txi) self.script_type.add_input(txi) self.multisig_fingerprint.add_input(txi) @@ -107,17 +112,12 @@ class TxInfoBase: if txo.script_type not in common.CHANGE_OUTPUT_SCRIPT_TYPES: 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 txo.multisig and not self.multisig_fingerprint.output_matches(txo): return False return ( - self.wallet_path.output_matches(txo) + self.multisig.output_matches(txo) + and self.wallet_path.output_matches(txo) and self.script_type.output_matches(txo) and len(txo.address_n) >= common.BIP32_WALLET_DEPTH and txo.address_n[-2] <= _BIP32_CHANGE_CHAIN diff --git a/legacy/firmware/.changelog.d/4285.feat b/legacy/firmware/.changelog.d/4285.feat new file mode 100644 index 0000000000..46ad33772a --- /dev/null +++ b/legacy/firmware/.changelog.d/4285.feat @@ -0,0 +1 @@ +Forbidden multisig to singlesig change outputs. diff --git a/legacy/firmware/signing.c b/legacy/firmware/signing.c index 4d7d700e15..3140013cd4 100644 --- a/legacy/firmware/signing.c +++ b/legacy/firmware/signing.c @@ -121,6 +121,9 @@ typedef struct { uint32_t segwit_count; uint32_t next_legacy_input; uint32_t min_sequence; + bool multisig; + bool multisig_set; + bool multisig_mismatch; bool multisig_fp_set; bool multisig_fp_mismatch; uint8_t multisig_fp[32]; @@ -953,6 +956,22 @@ void phase2_request_orig_input(void) { } } +static void extract_multisig(TxInfo *tx_info, const TxInputType *txinput) { + if (tx_info->multisig_mismatch) { + return; + } + + if (!tx_info->multisig_set) { + tx_info->multisig = txinput->has_multisig; + tx_info->multisig_set = true; + return; + } + + if (txinput->has_multisig != tx_info->multisig) { + tx_info->multisig_mismatch = true; + } +} + static bool extract_input_multisig_fp(TxInfo *tx_info, const TxInputType *txinput) { if (txinput->has_multisig && !tx_info->multisig_fp_mismatch) { @@ -978,6 +997,12 @@ static bool extract_input_multisig_fp(TxInfo *tx_info, return true; } +bool check_change_multisig(const TxInfo *tx_info, + const TxOutputType *txoutput) { + return tx_info->multisig_set && !tx_info->multisig_mismatch && + txoutput->has_multisig == tx_info->multisig; +} + bool check_change_multisig_fp(const TxInfo *tx_info, const TxOutputType *txoutput) { uint8_t h[32] = {0}; @@ -1243,6 +1268,9 @@ static bool tx_info_init(TxInfo *tx_info, uint32_t inputs_count, tx_info->segwit_count = 0; tx_info->next_legacy_input = 0xffffffff; tx_info->min_sequence = SEQUENCE_FINAL; + tx_info->multisig_set = false; + tx_info->multisig = false; + tx_info->multisig_mismatch = false; tx_info->multisig_fp_set = false; tx_info->multisig_fp_mismatch = false; tx_info->in_address_n_count = 0; @@ -1704,6 +1732,10 @@ static bool signing_validate_bin_output(TxOutputBinType *tx_bin_output) { static bool tx_info_add_input(TxInfo *tx_info, const TxInputType *txinput) { if (txinput->script_type != InputScriptType_EXTERNAL) { + // Remember whether the input is singlesig or multisig. Change-outputs must + // use the same type as all inputs. + extract_multisig(tx_info, txinput); + // Compute multisig fingerprint for change-output detection. In order for an // output to be considered a change-output, it must have the same // fingerprint as all inputs. @@ -2069,14 +2101,10 @@ 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 (!check_change_multisig(tx_info, txoutput)) { + return false; + } + if (txoutput->has_multisig && !check_change_multisig_fp(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 e94eb60fd3..f9d826739d 100644 --- a/tests/device_tests/bitcoin/test_multisig_change.py +++ b/tests/device_tests/bitcoin/test_multisig_change.py @@ -144,7 +144,7 @@ def _responses( INP1: messages.TxInputType, INP2: messages.TxInputType, change: int = 0, - foreign: bool = False, + foreign: int = 0, ): resp = [ request_input(0), @@ -152,21 +152,21 @@ def _responses( request_output(0), ] + if foreign == 1: + resp.append(messages.ButtonRequest(code=B.UnknownDerivationPath)) if change != 1: resp.append(messages.ButtonRequest(code=B.ConfirmOutput)) if is_core(client): resp.append(messages.ButtonRequest(code=B.ConfirmOutput)) - elif foreign: - resp.append(messages.ButtonRequest(code=B.UnknownDerivationPath)) resp.append(request_output(1)) + if foreign == 2: + resp.append(messages.ButtonRequest(code=B.UnknownDerivationPath)) if change != 2: resp.append(messages.ButtonRequest(code=B.ConfirmOutput)) if is_core(client): resp.append(messages.ButtonRequest(code=B.ConfirmOutput)) - elif foreign: - resp.append(messages.ButtonRequest(code=B.UnknownDerivationPath)) resp += [ messages.ButtonRequest(code=B.SignTx), @@ -241,9 +241,7 @@ def test_external_internal(client: Client): ) with client: - client.set_expected_responses( - _responses(client, INP1, INP2, change=2, foreign=True) - ) + client.set_expected_responses(_responses(client, INP1, INP2, foreign=2)) if is_core(client): IF = InputFlowConfirmAllWarnings(client) client.set_input_flow(IF.get()) @@ -277,9 +275,7 @@ def test_internal_external(client: Client): ) with client: - client.set_expected_responses( - _responses(client, INP1, INP2, change=1, foreign=True) - ) + client.set_expected_responses(_responses(client, INP1, INP2, foreign=1)) if is_core(client): IF = InputFlowConfirmAllWarnings(client) client.set_input_flow(IF.get()) @@ -408,7 +404,7 @@ def test_multisig_change_match_second(client: Client): # inputs match, change mismatches (second tries to be change but isn't) -def test_multisig_mismatch_change(client: Client): +def test_multisig_mismatch_multisig_change(client: Client): multisig_out2 = messages.MultisigRedeemScriptType( nodes=[NODE_EXT1, NODE_INT, NODE_EXT3], address_n=[1, 0], @@ -446,6 +442,39 @@ def test_multisig_mismatch_change(client: Client): ) +# inputs match, change mismatches (second tries to be change but isn't) +def test_multisig_mismatch_singlesig_change(client: Client): + out1 = messages.TxOutputType( + address="3B23k4kFBRtu49zvpG3Z9xuFzfpHvxBcwt", + amount=40_000_000, + script_type=messages.OutputScriptType.PAYTOADDRESS, + ) + + out2 = messages.TxOutputType( + address_n=[H_(45), 0, 1, 0], + amount=44_000_000, + ) + + with client: + client.set_expected_responses(_responses(client, INP1, INP2, foreign=2)) + if is_core(client): + IF = InputFlowConfirmAllWarnings(client) + client.set_input_flow(IF.get()) + _, serialized_tx = btc.sign_tx( + client, + "Bitcoin", + [INP1, INP2], + [out1, out2], + prev_txes=TX_API, + ) + + # Transaction does not exist on the blockchain, not using assert_tx_matches() + assert ( + serialized_tx.hex() + == "0100000002e53cf4e3fcd37f8c439286ce636476e1faeebf86bbb2f228a6b78d1b47c8c61601000000b500483045022100e736206001f5b5083ebc82dbcab6aa328a1f4d085cfbba177c0e9a03f704841702200986f7ca17facdc375a49598303cf743d54dc818ce011549968db232fb42c61d014c69522103dc07026aacb5918dac4e09f9da8290d0ae22161699636c22cace78082116a7792103e70db185fad69c2971f0107a42930e5d82a9ed3a11b922a96fdfc4124b63e54c2103f3fe007a1e34ac76c1a2528e9149f90f9f93739929797afab6a8e18d682fa71053aeffffffff185315ae8050e18efa70d6ca96378a1194f57e2b102511f68b3a1414ee340cd800000000b500483045022100db44361db468ce45195db9e54484384f04575bf674605ef350482ed185c260bd02202737d7eda8ee4b166f36c8ea1bffa3fabee944e3645209fe1b796b778969173f014c6952210297ad8a5df42f9e362ef37d9a4ddced89d8f7a143690649aa0d0ff049c7daca842103ed1fd93989595d7ad4b488efd05a22c0239482c9a20923f2f214a38e54f6c41a2103f91460d79e4e463d7d90cb75254bcd62b515a99a950574c721efdc5f711dff3553aeffffffff02005a62020000000017a91466528dd543f94d162c8111d2ec248d25ba9b90948700639f02000000001976a9149b139230e4fe91c05a37ec334dc8378f3dbe377088ac00000000" + ) + + # inputs mismatch, change matches with first input def test_multisig_mismatch_inputs(client: Client): multisig_out1 = messages.MultisigRedeemScriptType(