feat(legacy): forbid multisig to singlesig change outputs

legacy/firmware/signing.c

@@ -124,6 +124,9 @@ typedef struct {
   bool multisig_fp_set;
   bool multisig_fp_mismatch;
   uint8_t multisig_fp[32];
+  bool multisig;
+  bool multisig_set;
+  bool multisig_mismatch;
   uint32_t in_address_n[8];
   size_t in_address_n_count;
   InputScriptType in_script_type;
@@ -978,6 +981,22 @@ static bool extract_input_multisig_fp(TxInfo *tx_info,
   return true;
 }
 
+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;
+  }
+}
+
 bool check_change_multisig_fp(const TxInfo *tx_info,
                               const TxOutputType *txoutput) {
   uint8_t h[32] = {0};
@@ -986,6 +1005,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_set && !tx_info->multisig_mismatch &&
+         txoutput->has_multisig == tx_info->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 +1270,8 @@ 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_set = false;
+  tx_info->multisig_mismatch = false;
   tx_info->in_address_n_count = 0;
   tx_info->in_script_type = 0;
   tx_info->in_script_type_state = MatchState_UNDEFINED;
@@ -1711,6 +1738,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 +2100,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;
   }

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],
             )
         )