From 0af5cd18c226e5659d6467615dca7cbd879a9f00 Mon Sep 17 00:00:00 2001 From: Andrew Kozlik Date: Tue, 25 Jan 2022 22:54:31 +0100 Subject: [PATCH] feat(legacy): Strict path validation for Ethereum. --- legacy/firmware/.changelog.d/noissue.security | 1 + legacy/firmware/ethereum.c | 49 +++++++++++++++ legacy/firmware/ethereum.h | 4 ++ legacy/firmware/ethereum_networks.h.mako | 35 +++++++++-- legacy/firmware/fsm_msg_ethereum.h | 59 ++++++++++++++++++- 5 files changed, 143 insertions(+), 5 deletions(-) create mode 100644 legacy/firmware/.changelog.d/noissue.security diff --git a/legacy/firmware/.changelog.d/noissue.security b/legacy/firmware/.changelog.d/noissue.security new file mode 100644 index 000000000..71a0f5aa8 --- /dev/null +++ b/legacy/firmware/.changelog.d/noissue.security @@ -0,0 +1 @@ +Strict path validations for altcoins. diff --git a/legacy/firmware/ethereum.c b/legacy/firmware/ethereum.c index 66289f393..0804410d8 100644 --- a/legacy/firmware/ethereum.c +++ b/legacy/firmware/ethereum.c @@ -1040,3 +1040,52 @@ bool ethereum_parse(const char *address, uint8_t pubkeyhash[20]) { } return true; } + +bool ethereum_path_check(uint32_t address_n_count, const uint32_t *address_n, + bool pubkey_export, uint64_t chain) { + bool valid = (address_n_count >= 3); + valid = valid && (address_n[0] == (PATH_HARDENED | 44)); + valid = valid && (address_n[1] & PATH_HARDENED); + valid = valid && (address_n[2] & PATH_HARDENED); + valid = valid && ((address_n[2] & PATH_UNHARDEN_MASK) <= PATH_MAX_ACCOUNT); + + uint32_t path_slip44 = address_n[1] & PATH_UNHARDEN_MASK; + if (chain == CHAIN_ID_UNKNOWN) { + valid = valid && (is_ethereum_slip44(path_slip44)); + } else { + uint32_t chain_slip44 = ethereum_slip44_by_chain_id(chain); + if (chain_slip44 == SLIP44_UNKNOWN) { + // Allow Ethereum or testnet paths for unknown networks. + valid = valid && (path_slip44 == 60 || path_slip44 == 1); + } else if (chain_slip44 != 60 && chain_slip44 != 1) { + // Allow cross-signing with Ethereum unless it's testnet. + valid = valid && (path_slip44 == chain_slip44 || path_slip44 == 60); + } else { + valid = valid && (path_slip44 == chain_slip44); + } + } + + if (pubkey_export) { + // m/44'/coin_type'/account'/* + return valid; + } + + if (address_n_count == 3) { + // SEP-0005 for non-UTXO-based currencies, defined by Stellar: + // https://github.com/stellar/stellar-protocol/blob/master/ecosystem/sep-0005.md + // m/44'/coin_type'/account' + return valid; + } + + // We believe Ethereum should use the SEP-0005 scheme for everything, because + // it is account-based, rather than UTXO-based. Unfortunately, a lot of + // Ethereum tools (MEW, Metamask) do not use such scheme and set account = 0 + // and then iterate the address index. For compatibility, we allow this scheme + // as well. + // m/44'/coin_type'/account'/change/address_index + valid = valid && (address_n_count == 5); + valid = valid && (address_n[3] <= PATH_MAX_CHANGE); + valid = valid && (address_n[4] <= PATH_MAX_ADDRESS_INDEX); + + return valid; +} diff --git a/legacy/firmware/ethereum.h b/legacy/firmware/ethereum.h index feb287925..bae4c56ba 100644 --- a/legacy/firmware/ethereum.h +++ b/legacy/firmware/ethereum.h @@ -25,6 +25,8 @@ #include "bip32.h" #include "messages-ethereum.pb.h" +#define CHAIN_ID_UNKNOWN UINT64_MAX + void ethereum_signing_init(const EthereumSignTx *msg, const HDNode *node); void ethereum_signing_init_eip1559(const EthereumSignTxEIP1559 *msg, const HDNode *node); @@ -39,4 +41,6 @@ void ethereum_typed_hash_sign(const EthereumSignTypedHash *msg, EthereumTypedDataSignature *resp); bool ethereum_parse(const char *address, uint8_t pubkeyhash[20]); +bool ethereum_path_check(uint32_t address_n_count, const uint32_t *address_n, + bool pubkey_export, uint64_t chain); #endif diff --git a/legacy/firmware/ethereum_networks.h.mako b/legacy/firmware/ethereum_networks.h.mako index 6416aba39..5049f883c 100644 --- a/legacy/firmware/ethereum_networks.h.mako +++ b/legacy/firmware/ethereum_networks.h.mako @@ -3,14 +3,19 @@ BKSL = "\\" networks = list(supported_on("trezor1", eth)) max_chain_id_length = 0 +max_slip44_length = 0 max_suffix_length = 0 for n in networks: max_chain_id_length = max(len(str(n.chain_id)), max_chain_id_length) + max_slip44_length = max(len(str(n.slip44)), max_slip44_length) max_suffix_length = max(len(n.shortcut), max_suffix_length) def align_chain_id(n): return "{:>{w}}".format(n.chain_id, w=max_chain_id_length) +def align_slip44(n): + return "{:>{w}}".format(n.slip44, w=max_slip44_length) + def align_suffix(n): cstr = c_str(" " + n.shortcut) + ";" # we add two quotes, a space and a semicolon. hence +4 chars @@ -23,12 +28,34 @@ def align_suffix(n): #ifndef __ETHEREUM_NETWORKS_H__ #define __ETHEREUM_NETWORKS_H__ +#define SLIP44_UNKNOWN UINT32_MAX + #define ASSIGN_ETHEREUM_SUFFIX(suffix, chain_id) ${BKSL} - switch (chain_id) { ${BKSL} + switch (chain_id) { ${BKSL} % for n in networks: - case ${align_chain_id(n)}: suffix = ${align_suffix(n)} break; /* ${n.name} */ ${BKSL} + case ${align_chain_id(n)}: suffix = ${align_suffix(n)} break; /* ${n.name} */ ${BKSL} % endfor - default: suffix = " UNKN"; break; /* unknown chain */ ${BKSL} - } + default: suffix = " UNKN"; break; /* unknown chain */ ${BKSL} + } + +static bool is_ethereum_slip44(uint32_t slip44) { + switch (slip44) { +% for slip44 in sorted(set(n.slip44 for n in networks)): + case ${slip44}: +% endfor + return true; + default: + return false; + } +} + +static int32_t ethereum_slip44_by_chain_id(uint64_t chain_id) { + switch (chain_id) { +% for n in networks: + case ${align_chain_id(n)}: return ${align_slip44(n)}; /* ${n.name} */ +% endfor + default: return SLIP44_UNKNOWN; /* unknown chain */ + } +} #endif diff --git a/legacy/firmware/fsm_msg_ethereum.h b/legacy/firmware/fsm_msg_ethereum.h index 3ca3b1070..383433534 100644 --- a/legacy/firmware/fsm_msg_ethereum.h +++ b/legacy/firmware/fsm_msg_ethereum.h @@ -17,6 +17,22 @@ * along with this library. If not, see . */ +static bool fsm_ethereumCheckPath(uint32_t address_n_count, + const uint32_t *address_n, bool pubkey_export, + uint64_t chain_id) { + if (ethereum_path_check(address_n_count, address_n, pubkey_export, + chain_id)) { + return true; + } + + if (config_getSafetyCheckLevel() == SafetyCheckLevel_Strict) { + fsm_sendFailure(FailureType_Failure_DataError, _("Forbidden key path")); + return false; + } + + return fsm_layoutPathWarning(); +} + void fsm_msgEthereumGetPublicKey(const EthereumGetPublicKey *msg) { RESP_INIT(EthereumPublicKey); @@ -28,6 +44,12 @@ void fsm_msgEthereumGetPublicKey(const EthereumGetPublicKey *msg) { const CoinInfo *coin = fsm_getCoin(true, "Bitcoin"); if (!coin) return; + if (!fsm_ethereumCheckPath(msg->address_n_count, msg->address_n, true, + CHAIN_ID_UNKNOWN)) { + layoutHome(); + return; + } + const char *curve = coin->curve_name; uint32_t fingerprint; HDNode *node = fsm_getDerivedNode(curve, msg->address_n, msg->address_n_count, @@ -71,6 +93,12 @@ void fsm_msgEthereumSignTx(const EthereumSignTx *msg) { CHECK_PIN + if (!fsm_ethereumCheckPath(msg->address_n_count, msg->address_n, false, + msg->chain_id)) { + layoutHome(); + return; + } + const HDNode *node = fsm_getDerivedNode(SECP256K1_NAME, msg->address_n, msg->address_n_count, NULL); if (!node) return; @@ -83,6 +111,12 @@ void fsm_msgEthereumSignTxEIP1559(const EthereumSignTxEIP1559 *msg) { CHECK_PIN + if (!fsm_ethereumCheckPath(msg->address_n_count, msg->address_n, false, + msg->chain_id)) { + layoutHome(); + return; + } + const HDNode *node = fsm_getDerivedNode(SECP256K1_NAME, msg->address_n, msg->address_n_count, NULL); if (!node) return; @@ -101,13 +135,22 @@ void fsm_msgEthereumGetAddress(const EthereumGetAddress *msg) { CHECK_PIN + if (!fsm_ethereumCheckPath(msg->address_n_count, msg->address_n, false, + CHAIN_ID_UNKNOWN)) { + layoutHome(); + return; + } + const HDNode *node = fsm_getDerivedNode(SECP256K1_NAME, msg->address_n, msg->address_n_count, NULL); if (!node) return; uint8_t pubkeyhash[20]; - if (!hdnode_get_ethereum_pubkeyhash(node, pubkeyhash)) return; + if (!hdnode_get_ethereum_pubkeyhash(node, pubkeyhash)) { + layoutHome(); + return; + } uint32_t slip44 = (msg->address_n_count > 1) ? (msg->address_n[1] & PATH_UNHARDEN_MASK) : 0; @@ -150,12 +193,19 @@ void fsm_msgEthereumSignMessage(const EthereumSignMessage *msg) { CHECK_PIN + if (!fsm_ethereumCheckPath(msg->address_n_count, msg->address_n, false, + CHAIN_ID_UNKNOWN)) { + layoutHome(); + return; + } + const HDNode *node = fsm_getDerivedNode(SECP256K1_NAME, msg->address_n, msg->address_n_count, NULL); if (!node) return; uint8_t pubkeyhash[20] = {0}; if (!hdnode_get_ethereum_pubkeyhash(node, pubkeyhash)) { + layoutHome(); return; } @@ -230,6 +280,12 @@ void fsm_msgEthereumSignTypedHash(const EthereumSignTypedHash *msg) { return; } + if (!fsm_ethereumCheckPath(msg->address_n_count, msg->address_n, false, + CHAIN_ID_UNKNOWN)) { + layoutHome(); + return; + } + layoutDialogSwipe(&bmp_icon_warning, _("Abort"), _("Continue"), NULL, _("Unable to show"), _("EIP-712 data."), NULL, _("Sign at your own risk."), NULL, NULL); @@ -245,6 +301,7 @@ void fsm_msgEthereumSignTypedHash(const EthereumSignTypedHash *msg) { uint8_t pubkeyhash[20] = {0}; if (!hdnode_get_ethereum_pubkeyhash(node, pubkeyhash)) { + layoutHome(); return; }