feat(core.legacy): forbid multisig to singlesig change outputs

core/.changelog.d/4285.feat

@@ -0,0 +1 @@
+Forbidden multisig to singlesig change outputs.

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

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

legacy/firmware/.changelog.d/4285.feat

@@ -0,0 +1 @@
+Forbidden multisig to singlesig change outputs.

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;
   }

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(