Multisig fix in legacy

legacy/firmware/.changelog.d/4396.added

@@ -0,0 +1 @@
+Added support for lexicographic sorting of pubkeys in multisig.

legacy/firmware/.changelog.d/4396.changed

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

legacy/firmware/.changelog.d/4396.changed.2

@@ -0,0 +1 @@
+Forbid per-node paths in multisig change outputs and multisig receive addresses.

legacy/firmware/.changelog.d/4396.changed.3

@@ -0,0 +1 @@
+Remove deprecated Unchained Capital's multisig path.

legacy/firmware/crypto.c

@@ -18,6 +18,7 @@
  */
 
 #include "crypto.h"
+#include <stdlib.h>
 #include <string.h>
 #include "address.h"
 #include "aes/aes.h"
@@ -368,9 +369,56 @@ 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) {
+  const uint32_t n = cryptoMultisigPubkeyCount(multisig);
+  if (n < 1 || n > 15) {
+    return 0;
+  }
+
+  for (uint32_t i = 0; i < n; i++) {
+    const HDNode *pubnode = cryptoMultisigPubkey(coin, multisig, i);
+    if (!pubnode) {
+      return 0;
+    }
+    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;
+}
+
 int cryptoMultisigPubkeyIndex(const CoinInfo *coin,
                               const MultisigRedeemScriptType *multisig,
                               const uint8_t *pubkey) {
+  uint32_t n = cryptoMultisigPubkeyCount(multisig);
+
+  uint8_t pubkeys[33 * n];
+  if (!cryptoMultisigPubkeys(coin, multisig, pubkeys)) {
+    return -1;
+  }
+
+  for (size_t i = 0; i < n; i++) {
+    if (memcmp(pubkeys + i * 33, pubkey, 33) == 0) {
+      return i;
+    }
+  }
+  return -1;
+}
+
+int cryptoMultisigXpubIndex(const CoinInfo *coin,
+                            const MultisigRedeemScriptType *multisig,
+                            const uint8_t *pubkey) {
   for (size_t i = 0; i < cryptoMultisigPubkeyCount(multisig); i++) {
     const HDNode *pubnode = cryptoMultisigPubkey(coin, multisig, i);
     if (pubnode && memcmp(pubnode->public_key, pubkey, 33) == 0) {
@@ -380,9 +428,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;
@@ -403,21 +457,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));
@@ -527,17 +580,7 @@ bool coin_path_check(const CoinInfo *coin, InputScriptType script_type,
       valid = valid && (address_n[2] <= PATH_MAX_CHANGE);
       valid = valid && (address_n[3] <= PATH_MAX_ADDRESS_INDEX);
     } else if (address_n_count == 5) {
-      if (address_n[1] & PATH_HARDENED) {
-        // Unchained Capital compatibility pattern. Will be removed in the
-        // future.
-        // m / 45' / coin_type' / account' / [0-1000000] / address_index
-        valid = valid && check_cointype(coin, address_n[1], full_check);
-        valid = valid && (address_n[2] & PATH_HARDENED);
-        valid =
-            valid && ((address_n[2] & PATH_UNHARDEN_MASK) <= PATH_MAX_ACCOUNT);
-        valid = valid && (address_n[3] <= 1000000);
-        valid = valid && (address_n[4] <= PATH_MAX_ADDRESS_INDEX);
-      } else {
+      if ((address_n[1] & PATH_HARDENED) == 0) {
         // Casa proposed "universal multisig" pattern with unhardened parts.
         // m/45'/coin_type/account/change/address_index
         valid = valid &&
@@ -909,3 +952,27 @@ bool cryptoCosiVerify(const ed25519_signature signature, const uint8_t *message,
   res = ed25519_sign_open(message, message_len, pk_combined, signature);
   return res == 0;
 }
+
+bool multisig_uses_single_path(const MultisigRedeemScriptType *multisig) {
+  if (multisig->pubkeys_count == 0) {
+    // Pubkeys are specified by multisig.nodes and multisig.address_n, in this
+    // case all the pubkeys use the same path
+    return true;
+  } else {
+    // Pubkeys are specified by multisig.pubkeys, in this case we check that all
+    // the pubkeys use the same path
+    for (int i = 0; i < multisig->pubkeys_count; i++) {
+      if (multisig->pubkeys[i].address_n_count !=
+          multisig->pubkeys[0].address_n_count) {
+        return false;
+      }
+      for (int j = 0; j < multisig->pubkeys[i].address_n_count; j++) {
+        if (multisig->pubkeys[i].address_n[j] !=
+            multisig->pubkeys[0].address_n[j]) {
+          return false;
+        }
+      }
+    }
+    return true;
+  }
+}

legacy/firmware/crypto.h

@@ -88,6 +88,14 @@ int cryptoMultisigPubkeyIndex(const CoinInfo *coin,
                               const MultisigRedeemScriptType *multisig,
                               const uint8_t *pubkey);
 
+int cryptoMultisigXpubIndex(const CoinInfo *coin,
+                            const MultisigRedeemScriptType *multisig,
+                            const uint8_t *pubkey);
+
+uint32_t cryptoMultisigPubkeys(const CoinInfo *coin,
+                               const MultisigRedeemScriptType *multisig,
+                               uint8_t *pubkeys);
+
 int cryptoMultisigFingerprint(const MultisigRedeemScriptType *multisig,
                               uint8_t *hash);
 
@@ -115,5 +123,6 @@ void slip21_from_seed(const uint8_t *seed, int seed_len, Slip21Node *out);
 void slip21_derive_path(Slip21Node *inout, const uint8_t *label,
                         size_t label_len);
 const uint8_t *slip21_key(const Slip21Node *node);
+bool multisig_uses_single_path(const MultisigRedeemScriptType *multisig);
 
 #endif

legacy/firmware/fsm.c

@@ -442,6 +442,17 @@ bool fsm_layoutPathWarning(void) {
   return true;
 }
 
+bool fsm_layoutDifferentPathsWarning(void) {
+  layoutDialogSwipe(&bmp_icon_warning, _("Abort"), _("Continue"), NULL,
+                    _("Ussing different paths"), _("for different XPUBs."),
+                    NULL, _("Continue at your"), _("own risk!"), NULL);
+  if (!protectButton(ButtonRequestType_ButtonRequest_Warning, false)) {
+    fsm_sendFailure(FailureType_Failure_ActionCancelled, NULL);
+    return false;
+  }
+  return true;
+}
+
 #include "fsm_msg_coin.h"
 #include "fsm_msg_common.h"
 #include "fsm_msg_crypto.h"

legacy/firmware/fsm.h

@@ -152,6 +152,7 @@ bool fsm_layoutSignMessage(const uint8_t *msg, uint32_t len);
 bool fsm_layoutVerifyMessage(const uint8_t *msg, uint32_t len);
 
 bool fsm_layoutPathWarning(void);
+bool fsm_layoutDifferentPathsWarning(void);
 bool fsm_checkCoinPath(const CoinInfo *coin, InputScriptType script_type,
                        uint32_t address_n_count, const uint32_t *address_n,
                        bool has_multisig, MessageType message_type,

legacy/firmware/fsm_msg_coin.h

@@ -296,18 +296,44 @@ 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 (!multisig_uses_single_path(&(msg->multisig))) {
+        // An address that uses different derivation paths for different xpubs
+        // could be difficult to discover if the user did not note all the
+        // paths. The reason is that each path ends with an address index, which
+        // can have 1,000,000 possible values. If the address is a t-out-of-n
+        // multisig, the total number of possible paths is 1,000,000^n. This can
+        // be exploited by an attacker who has compromised the user's computer.
+        // The attacker could randomize the address indices and then demand a
+        // ransom from the user to reveal the paths. To prevent this, we require
+        // that all xpubs use the same derivation path.
+        if (config_getSafetyCheckLevel() == SafetyCheckLevel_Strict) {
+          fsm_sendFailure(
+              FailureType_Failure_DataError,
+              _("Using different paths for different xpubs is not allowed"
+
+                ));
+          layoutHome();
+          return;
+        }
+        fsm_layoutDifferentPathsWarning();
+      }
+      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));
       desc[10] = '0' + (m % 10);
       desc[15] = (n < 10) ? ' ' : ('0' + (n / 10));
       desc[16] = '0' + (n % 10);
       multisig_index =
-          cryptoMultisigPubkeyIndex(coin, &(msg->multisig), node->public_key);
+          cryptoMultisigXpubIndex(coin, &(msg->multisig), node->public_key);
     } else {
       strlcpy(desc, _("Address:"), sizeof(desc));
     }

legacy/firmware/signing.c

@@ -124,6 +124,8 @@ typedef struct {
   bool multisig_fp_set;
   bool multisig_fp_mismatch;
   uint8_t multisig_fp[32];
+  bool is_multisig;
+  MatchState is_multisig_state;
   uint32_t in_address_n[8];
   size_t in_address_n_count;
   InputScriptType in_script_type;
@@ -978,6 +980,22 @@ static bool extract_input_multisig_fp(TxInfo *tx_info,
   return true;
 }
 
+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->is_multisig_state = MatchState_MATCH;
+      tx_info->is_multisig = txinput->has_multisig;
+      return;
+    case MatchState_MATCH:
+      if (txinput->has_multisig != tx_info->is_multisig) {
+        tx_info->is_multisig_state = MatchState_MISMATCH;
+      }
+      return;
+  }
+}
+
 bool check_change_multisig_fp(const TxInfo *tx_info,
                               const TxOutputType *txoutput) {
   uint8_t h[32] = {0};
@@ -986,6 +1004,12 @@ bool check_change_multisig_fp(const TxInfo *tx_info,
          memcmp(tx_info->multisig_fp, h, 32) == 0;
 }
 
+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) {
   // 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 +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->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;
@@ -1711,6 +1736,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_is_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,15 +2098,25 @@ 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)) {
+  if (txoutput->has_multisig) {
+    if (!check_change_multisig_fp(tx_info, txoutput)) {
+      return false;
+    }
+    if (!multisig_uses_single_path(&(txoutput->multisig))) {
+      // An address that uses different derivation paths for different xpubs
+      // could be difficult to discover if the user did not note all the paths.
+      // The reason is that each path ends with an address index, which can have
+      // 1,000,000 possible values. If the address is a t-out-of-n multisig, the
+      // total number of possible paths is 1,000,000^n. This can be exploited by
+      // an attacker who has compromised the user's computer. The attacker could
+      // randomize the address indices and then demand a ransom from the user to
+      // reveal the paths. To prevent this, we require that all xpubs use the
+      // same derivation path.
+      return false;
+    }
+  }
+
+  if (!check_change_is_multisig(tx_info, txoutput)) {
     return false;
   }