From 88efd7471007fd250328ae25f0abcfaeac71bd1a Mon Sep 17 00:00:00 2001 From: Andrew Kozlik Date: Wed, 26 Jan 2022 19:58:17 +0100 Subject: [PATCH] feat(legacy): Make Bitcoin path checks same as in core. --- .../firmware/.changelog.d/noissue.security.1 | 1 + legacy/firmware/crypto.c | 320 ++++++++---------- legacy/firmware/crypto.h | 8 +- legacy/firmware/fsm.h | 4 + legacy/firmware/fsm_msg_coin.h | 51 +-- legacy/firmware/signing.c | 11 +- .../bitcoin/test_signtx_invalid_path.py | 1 - 7 files changed, 186 insertions(+), 210 deletions(-) create mode 100644 legacy/firmware/.changelog.d/noissue.security.1 diff --git a/legacy/firmware/.changelog.d/noissue.security.1 b/legacy/firmware/.changelog.d/noissue.security.1 new file mode 100644 index 000000000..20b941c5e --- /dev/null +++ b/legacy/firmware/.changelog.d/noissue.security.1 @@ -0,0 +1 @@ +Make Bitcoin path checks as strict as in Trezor T. diff --git a/legacy/firmware/crypto.c b/legacy/firmware/crypto.c index ddcfc8444..1a1296d12 100644 --- a/legacy/firmware/crypto.c +++ b/legacy/firmware/crypto.c @@ -487,231 +487,204 @@ static bool check_cointype(const CoinInfo *coin, uint32_t slip44, bool full) { bool coin_path_check(const CoinInfo *coin, InputScriptType script_type, uint32_t address_n_count, const uint32_t *address_n, - bool has_multisig, CoinPathCheckLevel level) { - // For level BASIC this function checks that a coin without strong replay - // protection doesn't access paths that are known to be used by another coin. - // Used by SignTx to ensure that a user cannot be coerced into signing a - // testnet transaction or a Litecoin transaction which in fact spends Bitcoin. - // For level KNOWN this function checks that the path is a recognized path for - // the given coin. Used by GetAddress to prevent ransom attacks where a user - // could be coerced to use an address with an unenumerable path. - // For level SCRIPT_TYPE this function makes the same checks as in level - // KNOWN, but includes script type checks. + bool has_multisig, bool full_check) { + // This function checks that the path is a recognized path for the given coin. + // Used by GetAddress to prevent ransom attacks where a user could be coerced + // to use an address with an unenumerable path and used by SignTx to ensure + // that a user cannot be coerced into signing a testnet transaction or a + // Litecoin transaction which in fact spends Bitcoin. If full_check is true, + // then this function also checks that the path fully matches the script type + // and coin type. This is used to determine whether a warning should be shown. - const bool check_known = (level >= CoinPathCheckLevel_KNOWN); - const bool check_script_type = (level >= CoinPathCheckLevel_SCRIPT_TYPE); + if (address_n_count == 0) { + return false; + } bool valid = true; // m/44' : BIP44 Legacy // m / purpose' / coin_type' / account' / change / address_index - if (address_n_count > 0 && address_n[0] == (PATH_HARDENED + 44)) { - if (check_known) { - valid = valid && (address_n_count == 5); - } else { - valid = valid && (address_n_count >= 2); - } - valid = valid && check_cointype(coin, address_n[1], check_known); - if (check_script_type) { + if (address_n[0] == PATH_HARDENED + 44) { + valid = valid && (address_n_count == 5); + 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] <= PATH_MAX_CHANGE); + valid = valid && (address_n[4] <= PATH_MAX_ADDRESS_INDEX); + if (full_check) { valid = valid && (script_type == InputScriptType_SPENDADDRESS); valid = valid && (!has_multisig); } - if (check_known) { - valid = valid && (address_n[2] & PATH_HARDENED); - valid = valid && ((address_n[2] & 0x7fffffff) <= PATH_MAX_ACCOUNT); - valid = valid && (address_n[3] <= PATH_MAX_CHANGE); - valid = valid && (address_n[4] <= PATH_MAX_ADDRESS_INDEX); - } return valid; } - if (address_n_count > 0 && address_n[0] == (PATH_HARDENED + 45)) { - if (check_script_type) { - valid = valid && has_multisig; - } - + if (address_n[0] == PATH_HARDENED + 45) { if (address_n_count == 4) { // m/45' - BIP45 Copay Abandoned Multisig P2SH // m / purpose' / cosigner_index / change / address_index // Patterns without a coin_type field must be treated as Bitcoin paths. - valid = valid && check_cointype(coin, SLIP44_BITCOIN, check_known); - if (check_script_type) { - valid = valid && (script_type == InputScriptType_SPENDMULTISIG); - } - if (check_known) { - valid = valid && (address_n[1] <= 100); - valid = valid && (address_n[2] <= PATH_MAX_CHANGE); - valid = valid && (address_n[3] <= PATH_MAX_ADDRESS_INDEX); - } + valid = valid && check_cointype(coin, SLIP44_BITCOIN, false); + valid = valid && (address_n[1] <= 100); + valid = valid && (address_n[2] <= PATH_MAX_CHANGE); + valid = valid && (address_n[3] <= PATH_MAX_ADDRESS_INDEX); } else if (address_n_count == 5) { // 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], check_known); - if (check_script_type) { - valid = valid && (script_type == InputScriptType_SPENDADDRESS || - script_type == InputScriptType_SPENDMULTISIG); - } - if (check_known) { - valid = valid && (address_n[2] & PATH_HARDENED); - valid = valid && ((address_n[2] & 0x7fffffff) <= PATH_MAX_ACCOUNT); - valid = valid && (address_n[3] <= 1000000); - valid = valid && (address_n[4] <= PATH_MAX_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_count == 6) { // Unchained Capital compatibility pattern. Will be removed in the // future. // m/45'/coin_type'/account'/[0-1000000]/change/address_index // m/45'/coin_type/account/[0-1000000]/change/address_index valid = valid && - check_cointype(coin, PATH_HARDENED | address_n[1], check_known); - if (check_script_type) { - valid = valid && (script_type == InputScriptType_SPENDADDRESS || - script_type == InputScriptType_SPENDMULTISIG); - } - if (check_known) { - valid = valid && ((address_n[1] & PATH_HARDENED) == - (address_n[2] & PATH_HARDENED)); - valid = valid && ((address_n[2] & 0x7fffffff) <= PATH_MAX_ACCOUNT); - valid = valid && (address_n[3] <= 1000000); - valid = valid && (address_n[4] <= PATH_MAX_CHANGE); - valid = valid && (address_n[5] <= PATH_MAX_ADDRESS_INDEX); - } + check_cointype(coin, PATH_HARDENED | address_n[1], full_check); + valid = valid && ((address_n[1] & PATH_HARDENED) == + (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_CHANGE); + valid = valid && (address_n[5] <= PATH_MAX_ADDRESS_INDEX); } else { - if (check_known) { - return false; - } + return false; + } + + if (full_check) { + valid = valid && (script_type == InputScriptType_SPENDADDRESS || + script_type == InputScriptType_SPENDMULTISIG); + valid = valid && has_multisig; } return valid; } - // m/48' - BIP48 Copay Multisig P2SH - // m / purpose' / coin_type' / account' / change / address_index - // Electrum: - // m / purpose' / coin_type' / account' / type' / change / address_index - if (address_n_count > 0 && address_n[0] == (PATH_HARDENED + 48)) { - if (check_known) { - valid = valid && (address_n_count == 5 || address_n_count == 6); - } else { - valid = valid && (address_n_count >= 2); - } - valid = valid && check_cointype(coin, address_n[1], check_known); - if (check_script_type) { - valid = valid && has_multisig; - // we do not support Multisig with Taproot yet - valid = valid && (script_type == InputScriptType_SPENDMULTISIG || - script_type == InputScriptType_SPENDP2SHWITNESS || - script_type == InputScriptType_SPENDWITNESS); - } - if (check_known) { - valid = valid && (address_n[2] & PATH_HARDENED); - valid = valid && ((address_n[2] & 0x7fffffff) <= PATH_MAX_ACCOUNT); - if (address_n_count == 5) { - valid = valid && (address_n[3] <= PATH_MAX_CHANGE); - valid = valid && (address_n[4] <= PATH_MAX_ADDRESS_INDEX); - } else if (address_n_count == 6) { - valid = valid && (address_n[3] & PATH_HARDENED); - valid = valid && ((address_n[3] & 0x7fffffff) <= 3); - valid = valid && (address_n[4] <= PATH_MAX_CHANGE); - valid = valid && (address_n[5] <= PATH_MAX_ADDRESS_INDEX); - } else { - return false; + if (address_n[0] == PATH_HARDENED + 48) { + valid = valid && (address_n_count == 5 || address_n_count == 6); + 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); + if (address_n_count == 5) { + // [OBSOLETE] m/48' Copay Multisig P2SH + // m / purpose' / coin_type' / account' / change / address_index + // NOTE: this pattern is not recognized by trezor-core + valid = valid && (address_n[3] <= PATH_MAX_CHANGE); + valid = valid && (address_n[4] <= PATH_MAX_ADDRESS_INDEX); + if (full_check) { + valid = valid && has_multisig; + valid = valid && (script_type == InputScriptType_SPENDMULTISIG); } + } else if (address_n_count == 6) { + // BIP-48: + // m / purpose' / coin_type' / account' / type' / change / address_index + valid = valid && (address_n[3] & PATH_HARDENED); + uint32_t type = address_n[3] & PATH_UNHARDEN_MASK; + valid = valid && (type <= 2); + valid = valid && (type == 0 || coin->has_segwit); + valid = valid && (address_n[4] <= PATH_MAX_CHANGE); + valid = valid && (address_n[5] <= PATH_MAX_ADDRESS_INDEX); + if (full_check) { + valid = valid && has_multisig; + switch (type) { + case 0: + valid = valid && (script_type == InputScriptType_SPENDMULTISIG || + script_type == InputScriptType_SPENDADDRESS); + break; + case 1: + valid = valid && (script_type == InputScriptType_SPENDP2SHWITNESS); + break; + case 2: + valid = valid && (script_type == InputScriptType_SPENDWITNESS); + break; + default: + return false; + } + } + } else { + return false; } return valid; } // m/49' : BIP49 SegWit // m / purpose' / coin_type' / account' / change / address_index - if (address_n_count > 0 && address_n[0] == (PATH_HARDENED + 49)) { + if (address_n[0] == PATH_HARDENED + 49) { valid = valid && coin->has_segwit; - if (check_known) { - valid = valid && (address_n_count == 5); - } else { - valid = valid && (address_n_count >= 2); - } - valid = valid && check_cointype(coin, address_n[1], check_known); - if (check_script_type) { + valid = valid && (address_n_count == 5); + 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] <= PATH_MAX_CHANGE); + valid = valid && (address_n[4] <= PATH_MAX_ADDRESS_INDEX); + if (full_check) { valid = valid && (script_type == InputScriptType_SPENDP2SHWITNESS); } - if (check_known) { - valid = valid && (address_n[2] & PATH_HARDENED); - valid = valid && ((address_n[2] & 0x7fffffff) <= PATH_MAX_ACCOUNT); - valid = valid && (address_n[3] <= PATH_MAX_CHANGE); - valid = valid && (address_n[4] <= PATH_MAX_ADDRESS_INDEX); - } return valid; } // m/84' : BIP84 Native SegWit // m / purpose' / coin_type' / account' / change / address_index - if (address_n_count > 0 && address_n[0] == (PATH_HARDENED + 84)) { + if (address_n[0] == PATH_HARDENED + 84) { valid = valid && coin->has_segwit; valid = valid && (coin->bech32_prefix != NULL); - if (check_known) { - valid = valid && (address_n_count == 5); - } else { - valid = valid && (address_n_count >= 2); - } - valid = valid && check_cointype(coin, address_n[1], check_known); - if (check_script_type) { + valid = valid && (address_n_count == 5); + 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] <= PATH_MAX_CHANGE); + valid = valid && (address_n[4] <= PATH_MAX_ADDRESS_INDEX); + if (full_check) { valid = valid && (script_type == InputScriptType_SPENDWITNESS); } - if (check_known) { - valid = valid && (address_n[2] & PATH_HARDENED); - valid = valid && ((address_n[2] & 0x7fffffff) <= PATH_MAX_ACCOUNT); - valid = valid && (address_n[3] <= PATH_MAX_CHANGE); - valid = valid && (address_n[4] <= PATH_MAX_ADDRESS_INDEX); - } return valid; } // m/86' : BIP86 Taproot // m / purpose' / coin_type' / account' / change / address_index - if (address_n_count > 0 && address_n[0] == (PATH_HARDENED + 86)) { + if (address_n[0] == PATH_HARDENED + 86) { valid = valid && coin->has_taproot; valid = valid && (coin->bech32_prefix != NULL); - if (check_known) { - valid = valid && (address_n_count == 5); - } else { - valid = valid && (address_n_count >= 2); - } - valid = valid && check_cointype(coin, address_n[1], check_known); - if (check_script_type) { + valid = valid && (address_n_count == 5); + 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] <= PATH_MAX_CHANGE); + valid = valid && (address_n[4] <= PATH_MAX_ADDRESS_INDEX); + if (full_check) { // we do not support Multisig with Taproot yet valid = valid && !has_multisig; valid = valid && (script_type == InputScriptType_SPENDTAPROOT); } - if (check_known) { - valid = valid && (address_n[2] & PATH_HARDENED); - valid = valid && ((address_n[2] & 0x7fffffff) <= PATH_MAX_ACCOUNT); - valid = valid && (address_n[3] <= PATH_MAX_CHANGE); - valid = valid && (address_n[4] <= PATH_MAX_ADDRESS_INDEX); - } return valid; } // Green Address compatibility pattern. Will be removed in the future. // m / [1,4] / address_index - if (address_n_count > 0 && (address_n[0] == 1 || address_n[0] == 4)) { + if (address_n[0] == 1 || address_n[0] == 4) { valid = valid && (coin->coin_type == SLIP44_BITCOIN); - if (check_known) { - valid = valid && (address_n_count == 2); - valid = valid && (address_n[1] <= PATH_MAX_ADDRESS_INDEX); + valid = valid && (address_n_count == 2); + valid = valid && (address_n[1] <= PATH_MAX_ADDRESS_INDEX); + if (full_check) { + valid = valid && (script_type != InputScriptType_SPENDTAPROOT); } return valid; } // Green Address compatibility pattern. Will be removed in the future. // m / 3' / [1-100]' / [1,4] / address_index - if (address_n_count > 0 && address_n[0] == (PATH_HARDENED + 3)) { + if (address_n[0] == PATH_HARDENED + 3) { valid = valid && (coin->coin_type == SLIP44_BITCOIN); - if (check_known) { - valid = valid && (address_n_count == 4); - valid = valid && (address_n[1] & PATH_HARDENED); - valid = valid && ((address_n[1] & 0x7fffffff) <= 100); - valid = valid && (address_n[2] == 1 || address_n[2] == 4); - valid = valid && (address_n[3] <= PATH_MAX_ADDRESS_INDEX); + valid = valid && (address_n_count == 4); + valid = valid && (address_n[1] & PATH_HARDENED); + valid = valid && ((address_n[1] & PATH_UNHARDEN_MASK) <= 100); + valid = valid && (address_n[2] == 1 || address_n[2] == 4); + valid = valid && (address_n[3] <= PATH_MAX_ADDRESS_INDEX); + if (full_check) { + valid = valid && (script_type != InputScriptType_SPENDTAPROOT); } return valid; } @@ -719,41 +692,36 @@ bool coin_path_check(const CoinInfo *coin, InputScriptType script_type, // Green Address compatibility patterns. Will be removed in the future. // m / 1195487518 // m / 1195487518 / 6 / address_index - if (address_n_count > 0 && address_n[0] == 1195487518) { + if (address_n[0] == 1195487518) { valid = valid && (coin->coin_type == SLIP44_BITCOIN); - if (check_known) { - if (address_n_count == 3) { - valid = valid && (address_n[1] == 6); - valid = valid && (address_n[2] <= PATH_MAX_ADDRESS_INDEX); - } else if (address_n_count != 1) { - return false; - } + if (address_n_count == 3) { + valid = valid && (address_n[1] == 6); + valid = valid && (address_n[2] <= PATH_MAX_ADDRESS_INDEX); + } else if (address_n_count != 1) { + return false; + } + if (full_check) { + return false; } return valid; } // Casa compatibility pattern. Will be removed in the future. // m / 49 / coin_type / account / change / address_index - if (address_n_count > 0 && address_n[0] == 49) { - if (check_known) { - valid = valid && (address_n_count == 5); - } else { - valid = valid && (address_n_count >= 2); - } - valid = valid && - check_cointype(coin, PATH_HARDENED | address_n[1], check_known); - if (check_script_type) { + if (address_n[0] == 49) { + valid = valid && (address_n_count == 5); + valid = + valid && check_cointype(coin, PATH_HARDENED | address_n[1], full_check); + valid = valid && ((address_n[1] & PATH_HARDENED) == 0); + valid = valid && (address_n[2] <= PATH_MAX_ACCOUNT); + valid = valid && (address_n[3] <= PATH_MAX_CHANGE); + valid = valid && (address_n[4] <= PATH_MAX_ADDRESS_INDEX); + if (full_check) { valid = valid && (script_type == InputScriptType_SPENDP2SHWITNESS); } - if (check_known) { - valid = valid && ((address_n[1] & PATH_HARDENED) == 0); - valid = valid && (address_n[2] <= PATH_MAX_ACCOUNT); - valid = valid && (address_n[3] <= PATH_MAX_CHANGE); - valid = valid && (address_n[4] <= PATH_MAX_ADDRESS_INDEX); - } return valid; } - // we allow unknown paths only when a full check is not required - return level == CoinPathCheckLevel_BASIC; + // unknown path + return false; } diff --git a/legacy/firmware/crypto.h b/legacy/firmware/crypto.h index b8675ccb6..04e98d374 100644 --- a/legacy/firmware/crypto.h +++ b/legacy/firmware/crypto.h @@ -32,12 +32,6 @@ #include "messages-bitcoin.pb.h" #include "messages-crypto.pb.h" -typedef enum _CoinPathCheckLevel { - CoinPathCheckLevel_BASIC = 0, - CoinPathCheckLevel_KNOWN = 1, - CoinPathCheckLevel_SCRIPT_TYPE = 2, -} CoinPathCheckLevel; - #define PATH_HARDENED 0x80000000 #define PATH_UNHARDEN_MASK 0x7fffffff #define PATH_MAX_ACCOUNT 100 @@ -85,6 +79,6 @@ int cryptoIdentityFingerprint(const IdentityType *identity, uint8_t *hash); bool coin_path_check(const CoinInfo *coin, InputScriptType script_type, uint32_t address_n_count, const uint32_t *address_n, - bool has_multisig, CoinPathCheckLevel level); + bool has_multisig, bool full_check); #endif diff --git a/legacy/firmware/fsm.h b/legacy/firmware/fsm.h index c22cca9bf..d005e1cfc 100644 --- a/legacy/firmware/fsm.h +++ b/legacy/firmware/fsm.h @@ -20,6 +20,7 @@ #ifndef __FSM_H__ #define __FSM_H__ +#include "coins.h" #include "messages-bitcoin.pb.h" #include "messages-crypto.pb.h" #include "messages-debug.pb.h" @@ -144,5 +145,8 @@ 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_checkCoinPath(const CoinInfo *coin, InputScriptType script_type, + uint32_t address_n_count, const uint32_t *address_n, + bool has_multisig, bool show_warning); #endif diff --git a/legacy/firmware/fsm_msg_coin.h b/legacy/firmware/fsm_msg_coin.h index 71f4afcc3..36c5901a7 100644 --- a/legacy/firmware/fsm_msg_coin.h +++ b/legacy/firmware/fsm_msg_coin.h @@ -151,24 +151,25 @@ void fsm_msgTxAck(TxAck *msg) { signing_txack(&(msg->tx)); } -static bool fsm_checkCoinPath(const CoinInfo *coin, InputScriptType script_type, - uint32_t address_n_count, - const uint32_t *address_n, bool has_multisig) { - if (!coin_path_check(coin, script_type, address_n_count, address_n, - has_multisig, CoinPathCheckLevel_SCRIPT_TYPE)) { - if (config_getSafetyCheckLevel() == SafetyCheckLevel_Strict && - !coin_path_check(coin, script_type, address_n_count, address_n, - has_multisig, CoinPathCheckLevel_KNOWN)) { - fsm_sendFailure(FailureType_Failure_DataError, _("Forbidden key path")); - layoutHome(); - return false; - } - - if (!fsm_layoutPathWarning()) { - layoutHome(); - return false; - } +bool fsm_checkCoinPath(const CoinInfo *coin, InputScriptType script_type, + uint32_t address_n_count, const uint32_t *address_n, + bool has_multisig, bool show_warning) { + if (coin_path_check(coin, script_type, address_n_count, address_n, + has_multisig, true)) { + return true; } + + if (config_getSafetyCheckLevel() == SafetyCheckLevel_Strict && + !coin_path_check(coin, script_type, address_n_count, address_n, + has_multisig, false)) { + fsm_sendFailure(FailureType_Failure_DataError, _("Forbidden key path")); + return false; + } + + if (show_warning) { + return fsm_layoutPathWarning(); + } + return true; } @@ -181,6 +182,14 @@ void fsm_msgGetAddress(const GetAddress *msg) { const CoinInfo *coin = fsm_getCoin(msg->has_coin_name, msg->coin_name); if (!coin) return; + + if (!fsm_checkCoinPath(coin, msg->script_type, msg->address_n_count, + msg->address_n, msg->has_multisig, + msg->show_display)) { + layoutHome(); + return; + } + HDNode *node = fsm_getDerivedNode(coin->curve_name, msg->address_n, msg->address_n_count, NULL); if (!node) return; @@ -220,11 +229,6 @@ void fsm_msgGetAddress(const GetAddress *msg) { strlcpy(desc, _("Address:"), sizeof(desc)); } - if (!fsm_checkCoinPath(coin, msg->script_type, msg->address_n_count, - msg->address_n, msg->has_multisig)) { - return; - } - uint32_t multisig_xpub_magic = coin->xpub_magic; if (msg->has_multisig && coin->has_segwit) { if (!msg->has_ignore_xpub_magic || !msg->ignore_xpub_magic) { @@ -269,7 +273,8 @@ void fsm_msgSignMessage(const SignMessage *msg) { if (!coin) return; if (!fsm_checkCoinPath(coin, msg->script_type, msg->address_n_count, - msg->address_n, false)) { + msg->address_n, false, true)) { + layoutHome(); return; } diff --git a/legacy/firmware/signing.c b/legacy/firmware/signing.c index d26ba0ff7..1b00442b7 100644 --- a/legacy/firmware/signing.c +++ b/legacy/firmware/signing.c @@ -747,13 +747,12 @@ static bool fill_input_script_sig(TxInputType *tinput) { static bool derive_node(TxInputType *tinput) { if (!coin_path_check(coin, tinput->script_type, tinput->address_n_count, - tinput->address_n, tinput->has_multisig, - CoinPathCheckLevel_BASIC)) { + tinput->address_n, tinput->has_multisig, false)) { if (is_replacement) { fsm_sendFailure( FailureType_Failure_ProcessError, _("Non-standard paths not allowed in replacement transactions.")); - layoutHome(); + signing_abort(); return false; } @@ -1373,6 +1372,12 @@ static bool signing_add_input(TxInputType *txinput) { // hash all input data to check it later (relevant for fee computation) tx_input_check_hash(&hasher_check, txinput); + if (!fsm_checkCoinPath(coin, txinput->script_type, txinput->address_n_count, + txinput->address_n, txinput->has_multisig, true)) { + signing_abort(); + return false; + } + if (!fill_input_script_pubkey(coin, &root, txinput)) { fsm_sendFailure(FailureType_Failure_ProcessError, _("Failed to derive scriptPubKey")); diff --git a/tests/device_tests/bitcoin/test_signtx_invalid_path.py b/tests/device_tests/bitcoin/test_signtx_invalid_path.py index cb8020133..69bf4efe4 100644 --- a/tests/device_tests/bitcoin/test_signtx_invalid_path.py +++ b/tests/device_tests/bitcoin/test_signtx_invalid_path.py @@ -174,7 +174,6 @@ def test_attack_path_segwit(client: Client): ) -@pytest.mark.skip_t1(reason="T1 only prevents using paths known to be altcoins") def test_invalid_path_fail_asap(client: Client): inp1 = messages.TxInputType( address_n=parse_path("m/0"),