diff --git a/core/.changelog.d/4351.changed.1 b/core/.changelog.d/4351.changed.1 new file mode 100644 index 0000000000..ba68fd7fa6 --- /dev/null +++ b/core/.changelog.d/4351.changed.1 @@ -0,0 +1 @@ +Forbid multisig to singlesig change outputs. diff --git a/core/src/apps/bitcoin/sign_tx/change_detector.py b/core/src/apps/bitcoin/sign_tx/change_detector.py index 39630aa6ed..b668206437 100644 --- a/core/src/apps/bitcoin/sign_tx/change_detector.py +++ b/core/src/apps/bitcoin/sign_tx/change_detector.py @@ -46,17 +46,9 @@ class ChangeDetector: 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_fingerprint.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/core/src/apps/bitcoin/sign_tx/matchcheck.py b/core/src/apps/bitcoin/sign_tx/matchcheck.py index a9db1aa11e..1acb812cef 100644 --- a/core/src/apps/bitcoin/sign_tx/matchcheck.py +++ b/core/src/apps/bitcoin/sign_tx/matchcheck.py @@ -103,7 +103,11 @@ class MultisigFingerprintChecker(MatchChecker): from .. import multisig if not txio.multisig: - return None + # The fingerprint of a singlesig input or output is defined as an empty byte string. + # This has two consequences: First, a singlesig output matches if and only if all + # the added inputs are singlesig. Second, a multisig output does not match if any of + # the added inputs is singlesig. + return bytes() return multisig.multisig_fingerprint(txio.multisig) diff --git a/core/tests/test_apps.bitcoin.change_detector.py b/core/tests/test_apps.bitcoin.change_detector.py index 52183eab08..d262d146b5 100644 --- a/core/tests/test_apps.bitcoin.change_detector.py +++ b/core/tests/test_apps.bitcoin.change_detector.py @@ -120,17 +120,17 @@ class TestChangeDetector(unittest.TestCase): # External output assert self.d.output_is_change(get_external_singlesig_output()) == False - def test_multisig_different_xpubs_order(self): - # Different order of xpubs - self.d.add_input(get_multisig_input([H_(45), 0, 0, 0], [xpub1, xpub2])) - self.d.add_input(get_multisig_input([H_(45), 0, 0, 0], [xpub2, xpub1])) + def test_singlesig_different_account_indices(self): + # Different account indices + self.d.add_input(get_singlesig_input([H_(45), 0, 0, 0])) + self.d.add_input(get_singlesig_input([H_(45), 1, 0, 0])) - # Same ouputs as inputs - assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1, xpub2])) == True - assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub2, xpub1])) == True + # Same outputs as inputs + assert self.d.output_is_change(get_internal_singlesig_output([H_(45), 0, 0, 0])) == False + assert self.d.output_is_change(get_internal_singlesig_output([H_(45), 1, 0, 0])) == False - # Singlesig instead of multisig - assert self.d.output_is_change(get_internal_singlesig_output([H_(45), 0, 0, 0])) == True + # Multisig instead of singlesig + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1])) == False # External output assert self.d.output_is_change(get_external_singlesig_output()) == False @@ -149,7 +149,7 @@ class TestChangeDetector(unittest.TestCase): assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 1, 1], [xpub1, xpub2])) == True # Singlesig instead of multisig - assert self.d.output_is_change(get_internal_singlesig_output([H_(45), 0, 0, 0])) == True + assert self.d.output_is_change(get_internal_singlesig_output([H_(45), 0, 0, 0])) == False # Different account index assert self.d.output_is_change(get_internal_singlesig_output([H_(45), 1, 0, 0])) == False @@ -163,17 +163,17 @@ class TestChangeDetector(unittest.TestCase): # External output assert self.d.output_is_change(get_external_singlesig_output()) == False - def test_singlesig_different_account_indices(self): - # Different account indices - self.d.add_input(get_singlesig_input([H_(45), 0, 0, 0])) - self.d.add_input(get_singlesig_input([H_(45), 1, 0, 0])) + def test_multisig_different_xpubs_order(self): + # Different order of xpubs + self.d.add_input(get_multisig_input([H_(45), 0, 0, 0], [xpub1, xpub2])) + self.d.add_input(get_multisig_input([H_(45), 0, 0, 0], [xpub2, xpub1])) - # Same outputs as inputs + # Same ouputs as inputs + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1, xpub2])) == True + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub2, xpub1])) == True + + # Singlesig instead of multisig assert self.d.output_is_change(get_internal_singlesig_output([H_(45), 0, 0, 0])) == False - assert self.d.output_is_change(get_internal_singlesig_output([H_(45), 1, 0, 0])) == False - - # Multisig instead of singlesig - assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1])) == False # External output assert self.d.output_is_change(get_external_singlesig_output()) == False @@ -188,7 +188,7 @@ class TestChangeDetector(unittest.TestCase): assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1, xpub3])) == False # Singlesig instead of multisig - assert self.d.output_is_change(get_internal_singlesig_output([H_(45), 0, 0, 0])) == True + assert self.d.output_is_change(get_internal_singlesig_output([H_(45), 0, 0, 0])) == False # External output assert self.d.output_is_change(get_external_singlesig_output()) == False diff --git a/tests/device_tests/bitcoin/test_multisig_change.py b/tests/device_tests/bitcoin/test_multisig_change.py index 694b35bde1..55caaf9342 100644 --- a/tests/device_tests/bitcoin/test_multisig_change.py +++ b/tests/device_tests/bitcoin/test_multisig_change.py @@ -222,7 +222,13 @@ def test_external_internal(client: Client): with client: client.set_expected_responses( - _responses(client, INP1, INP2, change_indices=[2], foreign_indices=[2]) + _responses( + client, + INP1, + INP2, + change_indices=[] if is_core(client) else [2], + foreign_indices=[2], + ) ) if is_core(client): IF = InputFlowConfirmAllWarnings(client) @@ -252,7 +258,13 @@ def test_internal_external(client: Client): with client: client.set_expected_responses( - _responses(client, INP1, INP2, change_indices=[1], foreign_indices=[1]) + _responses( + client, + INP1, + INP2, + change_indices=[] if is_core(client) else [1], + foreign_indices=[1], + ) ) if is_core(client): IF = InputFlowConfirmAllWarnings(client)