From 47848b04cfe517db2c63dfa26876d1a132d5e42a Mon Sep 17 00:00:00 2001 From: Pavol Rusnak Date: Sat, 15 Aug 2020 17:48:39 +0200 Subject: [PATCH 1/4] legacy: refactor check_cointype into a separate function --- legacy/firmware/crypto.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/legacy/firmware/crypto.c b/legacy/firmware/crypto.c index 5e642662282..bb28644b60f 100644 --- a/legacy/firmware/crypto.c +++ b/legacy/firmware/crypto.c @@ -508,6 +508,11 @@ int cryptoIdentityFingerprint(const IdentityType *identity, uint8_t *hash) { return 1; } +static bool check_cointype(const CoinInfo *coin, uint32_t slip44, bool full) { + (void)full; + return coin->coin_type == slip44; +} + bool coin_known_path_check(const CoinInfo *coin, InputScriptType script_type, uint32_t address_n_count, const uint32_t *address_n, bool full) { @@ -521,7 +526,7 @@ bool coin_known_path_check(const CoinInfo *coin, InputScriptType script_type, } else { valid &= (address_n_count >= 2); } - valid &= (address_n[1] == coin->coin_type); + valid &= check_cointype(coin, address_n[1], full); if (full) { valid &= (address_n[2] & 0x80000000) == 0x80000000; valid &= (address_n[3] & 0x80000000) == 0; @@ -556,7 +561,7 @@ bool coin_known_path_check(const CoinInfo *coin, InputScriptType script_type, } else { valid &= (address_n_count >= 2); } - valid &= (address_n[1] == coin->coin_type); + valid &= check_cointype(coin, address_n[1], full); if (full) { valid &= (address_n[2] & 0x80000000) == 0x80000000; valid &= (address_n[4] & 0x80000000) == 0; @@ -574,7 +579,7 @@ bool coin_known_path_check(const CoinInfo *coin, InputScriptType script_type, } else { valid &= (address_n_count >= 2); } - valid &= (address_n[1] == coin->coin_type); + valid &= check_cointype(coin, address_n[1], full); if (full) { valid &= (address_n[2] & 0x80000000) == 0x80000000; valid &= (address_n[3] & 0x80000000) == 0; @@ -594,7 +599,7 @@ bool coin_known_path_check(const CoinInfo *coin, InputScriptType script_type, } else { valid &= (address_n_count >= 2); } - valid &= (address_n[1] == coin->coin_type); + valid &= check_cointype(coin, address_n[1], full); if (full) { valid &= (address_n[2] & 0x80000000) == 0x80000000; valid &= (address_n[3] & 0x80000000) == 0; From 0630f1c4258210153315281c0f18a0f65a276fe3 Mon Sep 17 00:00:00 2001 From: Pavol Rusnak Date: Sat, 15 Aug 2020 17:59:51 +0200 Subject: [PATCH 2/4] legacy: allow spending coins from Bitcoin paths if the coin ... has implemented strong replay protection via SIGHASH_FORKID --- legacy/firmware/CHANGELOG.md | 2 ++ legacy/firmware/crypto.c | 11 +++++++++++ 2 files changed, 13 insertions(+) diff --git a/legacy/firmware/CHANGELOG.md b/legacy/firmware/CHANGELOG.md index 6fdb2aaf29d..8aabeb4a43d 100644 --- a/legacy/firmware/CHANGELOG.md +++ b/legacy/firmware/CHANGELOG.md @@ -11,6 +11,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). ### Changed - Print inverted question mark for non-printable characters. +- Allow spending coins from Bitcoin paths if the coin has implemented strong replay protection via `SIGHASH_FORKID`. [#1188] ### Deprecated @@ -331,3 +332,4 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). [#1030]: https://github.com/trezor/trezor-firmware/issues/1030 [#1098]: https://github.com/trezor/trezor-firmware/issues/1098 [#1165]: https://github.com/trezor/trezor-firmware/pull/1165 +[#1188]: https://github.com/trezor/trezor-firmware/issues/1188 diff --git a/legacy/firmware/crypto.c b/legacy/firmware/crypto.c index bb28644b60f..0848a55e6c6 100644 --- a/legacy/firmware/crypto.c +++ b/legacy/firmware/crypto.c @@ -509,7 +509,18 @@ int cryptoIdentityFingerprint(const IdentityType *identity, uint8_t *hash) { } static bool check_cointype(const CoinInfo *coin, uint32_t slip44, bool full) { +#if BITCOIN_ONLY (void)full; +#else + if (!full) { + // some wallets such as Electron-Cash (BCH) store coins on Bitcoin paths + // we can allow spending these coins from Bitcoin paths if the coin has + // implemented strong replay protection via SIGHASH_FORKID + if (slip44 == 0x80000000 && coin->has_fork_id) { + return true; + } + } +#endif return coin->coin_type == slip44; } From 4535b5f8da7eb0dbdb573adb4f8d017212cc5129 Mon Sep 17 00:00:00 2001 From: Pavol Rusnak Date: Sat, 15 Aug 2020 18:25:54 +0200 Subject: [PATCH 3/4] core: allow spending coins from Bitcoin paths if the coin ... has implemented strong replay protection via SIGHASH_FORKID --- core/CHANGELOG.md | 2 ++ core/src/apps/bitcoin/keychain.py | 10 ++++++++ core/tests/test_apps.wallet.keychain.py | 34 ++++++++++++++++++++++++- 3 files changed, 45 insertions(+), 1 deletion(-) diff --git a/core/CHANGELOG.md b/core/CHANGELOG.md index 26f7966c6a3..7276e63bc53 100644 --- a/core/CHANGELOG.md +++ b/core/CHANGELOG.md @@ -16,6 +16,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). - Print inverted question mark for non-printable characters. - Remove pre-fill bar from text rendering functions. [#1173] - Display coin name when signing or verifying messages. [#1159] +- Allow spending coins from Bitcoin paths if the coin has implemented strong replay protection via `SIGHASH_FORKID`. [#1188] ### Deprecated @@ -269,3 +270,4 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). [#1159]: https://github.com/trezor/trezor-firmware/issues/1159 [#1165]: https://github.com/trezor/trezor-firmware/pull/1165 [#1173]: https://github.com/trezor/trezor-firmware/pull/1173 +[#1188]: https://github.com/trezor/trezor-firmware/issues/1188 diff --git a/core/src/apps/bitcoin/keychain.py b/core/src/apps/bitcoin/keychain.py index 24501689c3c..6ad87b0af09 100644 --- a/core/src/apps/bitcoin/keychain.py +++ b/core/src/apps/bitcoin/keychain.py @@ -56,6 +56,16 @@ def get_namespaces_for_coin(coin: coininfo.CoinInfo): # m/0x4741b11e/6/pointer namespaces.append([0x4741B11E]) + # some wallets such as Electron-Cash (BCH) store coins on Bitcoin paths + # we can allow spending these coins from Bitcoin paths if the coin has + # implemented strong replay protection via SIGHASH_FORKID + if coin.fork_id is not None: + namespaces.append([44 | HARDENED, 0 | HARDENED]) + namespaces.append([48 | HARDENED, 0 | HARDENED]) + if coin.segwit: + namespaces.append([49 | HARDENED, 0 | HARDENED]) + namespaces.append([84 | HARDENED, 0 | HARDENED]) + return namespaces diff --git a/core/tests/test_apps.wallet.keychain.py b/core/tests/test_apps.wallet.keychain.py index e5f020cc07d..6808992b05d 100644 --- a/core/tests/test_apps.wallet.keychain.py +++ b/core/tests/test_apps.wallet.keychain.py @@ -87,14 +87,46 @@ def test_bcash(self): self.assertFalse(coin.segwit) valid_addresses = ( [44 | HARDENED, 145 | HARDENED], + [44 | HARDENED, 0 | HARDENED], [45 | HARDENED, 123456], [48 | HARDENED, 145 | HARDENED], + [48 | HARDENED, 0 | HARDENED], ) invalid_addresses = ( [43 | HARDENED, 145 | HARDENED], - [44 | HARDENED, 0 | HARDENED], + [43 | HARDENED, 0 | HARDENED], [49 | HARDENED, 145 | HARDENED], + [49 | HARDENED, 0 | HARDENED], [84 | HARDENED, 145 | HARDENED], + [84 | HARDENED, 0 | HARDENED], + ) + + for addr in valid_addresses: + keychain.derive(addr) + + for addr in invalid_addresses: + self.assertRaises(wire.DataError, keychain.derive, addr) + + def test_litecoin(self): + keychain, coin = await_result( + get_keychain_for_coin(wire.DUMMY_CONTEXT, "Litecoin") + ) + self.assertEqual(coin.coin_name, "Litecoin") + + self.assertTrue(coin.segwit) + valid_addresses = ( + [44 | HARDENED, 2 | HARDENED], + [45 | HARDENED, 123456], + [48 | HARDENED, 2 | HARDENED], + [49 | HARDENED, 2 | HARDENED], + [84 | HARDENED, 2 | HARDENED], + ) + invalid_addresses = ( + [43 | HARDENED, 2 | HARDENED], + [44 | HARDENED, 0 | HARDENED], + [48 | HARDENED, 0 | HARDENED], + [49 | HARDENED, 0 | HARDENED], + [84 | HARDENED, 0 | HARDENED], ) for addr in valid_addresses: From 806c7d6126c68eff417713f8c8c5077e14ad0048 Mon Sep 17 00:00:00 2001 From: Pavol Rusnak Date: Tue, 4 Aug 2020 20:02:57 +0200 Subject: [PATCH 4/4] tests: add tests for invalid paths --- .../test_msg_signtx_invalid_path.py | 94 +++++++++++++++++++ ...4594a0004cb3b640d6714fdb00b9128832dd5.json | 18 ++++ tests/ui_tests/fixtures.json | 2 + 3 files changed, 114 insertions(+) create mode 100644 tests/device_tests/test_msg_signtx_invalid_path.py create mode 100644 tests/txcache/bcash/8cc1f4adf7224ce855cf535a5104594a0004cb3b640d6714fdb00b9128832dd5.json diff --git a/tests/device_tests/test_msg_signtx_invalid_path.py b/tests/device_tests/test_msg_signtx_invalid_path.py new file mode 100644 index 00000000000..2e40abcd092 --- /dev/null +++ b/tests/device_tests/test_msg_signtx_invalid_path.py @@ -0,0 +1,94 @@ +# This file is part of the Trezor project. +# +# Copyright (C) 2012-2019 SatoshiLabs and contributors +# +# This library is free software: you can redistribute it and/or modify +# it under the terms of the GNU Lesser General Public License version 3 +# as published by the Free Software Foundation. +# +# This library is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU Lesser General Public License for more details. +# +# You should have received a copy of the License along with this library. +# If not, see . + +import pytest + +from trezorlib import btc, messages as proto +from trezorlib.exceptions import TrezorFailure +from trezorlib.tools import parse_path + +from ..tx_cache import TxCache + +TX_CACHE_MAINNET = TxCache("Bitcoin") +TX_CACHE_BCASH = TxCache("Bcash") + +TXHASH_8cc1f4 = bytes.fromhex( + "8cc1f4adf7224ce855cf535a5104594a0004cb3b640d6714fdb00b9128832dd5" +) +TXHASH_d5f65e = bytes.fromhex( + "d5f65ee80147b4bcc70b75e4bbf2d7382021b871bd8867ef8fa525ef50864882" +) + + +class TestMsgSigntxInvalidPath: + + # Adapted from TestMsgSigntx.test_one_one_fee, + # only changed the coin from Bitcoin to Litecoin. + # Litecoin does not have strong replay protection using SIGHASH_FORKID, + # spending from Bitcoin path should fail. + @pytest.mark.altcoin + def test_invalid_path_fail(self, client): + # tx: d5f65ee80147b4bcc70b75e4bbf2d7382021b871bd8867ef8fa525ef50864882 + # input 0: 0.0039 BTC + + inp1 = proto.TxInputType( + address_n=parse_path("44h/0h/0h/0/0"), + # amount=390000, + prev_hash=TXHASH_d5f65e, + prev_index=0, + ) + + # address is converted from 1MJ2tj2ThBE62zXbBYA5ZaN3fdve5CPAz1 by changing the version + out1 = proto.TxOutputType( + address="LfWz9wLHmqU9HoDkMg9NqbRosrHvEixeVZ", + amount=390000 - 10000, + script_type=proto.OutputScriptType.PAYTOADDRESS, + ) + + with pytest.raises(TrezorFailure) as exc: + btc.sign_tx(client, "Litecoin", [inp1], [out1], prev_txes=TX_CACHE_MAINNET) + + if client.features.model == "1": + assert exc.value.code == proto.FailureType.ProcessError + assert exc.value.message.endswith("Failed to compile input") + else: + assert exc.value.code == proto.FailureType.DataError + assert exc.value.message.endswith("Forbidden key path") + + # Adapted from TestMsgSigntx.test_one_one_fee, + # only changed the coin from Bitcoin to Bcash. + # Bcash does have strong replay protection using SIGHASH_FORKID, + # spending from Bitcoin path should work. + @pytest.mark.altcoin + def test_invalid_path_pass_forkid(self, client): + # tx: 8cc1f4adf7224ce855cf535a5104594a0004cb3b640d6714fdb00b9128832dd5 + # input 0: 0.0039 BTC + + inp1 = proto.TxInputType( + address_n=parse_path("44h/0h/0h/0/0"), + amount=390000, + prev_hash=TXHASH_8cc1f4, + prev_index=0, + ) + + # address is converted from 1MJ2tj2ThBE62zXbBYA5ZaN3fdve5CPAz1 to cashaddr format + out1 = proto.TxOutputType( + address="bitcoincash:qr0fk25d5zygyn50u5w7h6jkvctas52n0qxff9ja6r", + amount=390000 - 10000, + script_type=proto.OutputScriptType.PAYTOADDRESS, + ) + + btc.sign_tx(client, "Bcash", [inp1], [out1], prev_txes=TX_CACHE_BCASH) diff --git a/tests/txcache/bcash/8cc1f4adf7224ce855cf535a5104594a0004cb3b640d6714fdb00b9128832dd5.json b/tests/txcache/bcash/8cc1f4adf7224ce855cf535a5104594a0004cb3b640d6714fdb00b9128832dd5.json new file mode 100644 index 00000000000..8e3f199ee9a --- /dev/null +++ b/tests/txcache/bcash/8cc1f4adf7224ce855cf535a5104594a0004cb3b640d6714fdb00b9128832dd5.json @@ -0,0 +1,18 @@ +{ + "bin_outputs": [ + { + "amount": 390000, + "script_pubkey": "a9147f844bdb0b8fd54b64e3d16c85dc1170f1ff97c187" + } + ], + "inputs": [ + { + "prev_hash": "54aa5680dea781f45ebb536e53dffc526d68c0eb5c00547e323b2c32382dfba3", + "prev_index": 1, + "script_sig": "4830450221009e020b0390ccad533b73b552f8a99a9d827212c558e4f755503674d07c92ad4502202d606f7316990e0461c51d4add25054f19c697aa3e3c2ced4d568f0b2c57e62f0121023230848585885f63803a0a8aecdd6538792d5c539215c91698e315bf0253b43d", + "sequence": 4294967295 + } + ], + "lock_time": 0, + "version": 1 +} diff --git a/tests/ui_tests/fixtures.json b/tests/ui_tests/fixtures.json index e11616472ab..c75a464da07 100644 --- a/tests/ui_tests/fixtures.json +++ b/tests/ui_tests/fixtures.json @@ -335,6 +335,8 @@ "test_msg_signtx_grs.py-test_send_segwit_native_change": "2b9119aea79b50a26bd86cc49fd5421aedee42245b83919bf142d2ca2e50dd16", "test_msg_signtx_grs.py-test_send_segwit_p2sh": "e596ff7f2e10633d8de6bd5b98928020dfacf578ce71d6d6e0ca27011ed8bd53", "test_msg_signtx_grs.py-test_send_segwit_p2sh_change": "65a33cc9ecf52bdab93d1066bfaff7f30c9908fc5ada8a672c39df57eb6129c2", +"test_msg_signtx_invalid_path.py-test_invalid_path_fail": "b0f22cba2dbab2cd21c15c002b66ed89b6c728b10daa8d0c0e78abd4164a3912", +"test_msg_signtx_invalid_path.py-test_invalid_path_pass_forkid": "667dcb09b569e5b4e091e6b1ac7e8e057c0c730c931b22f8c0ee64050f3f467b", "test_msg_signtx_komodo.py-test_one_one_fee_sapling": "aa32424c04f65c0e2de57f6eb26830eb037c7b4daaf300ae61d30968affd9193", "test_msg_signtx_komodo.py-test_one_one_rewards_claim": "e53f221fda81469027e39e21877a81a8fafbffbece0a45aeda12aae8873b0464", "test_msg_signtx_peercoin.py::test_timestamp_included": "825b9bdf5238c5c6415a254a6bae4b2bd9df8fc5cb31f66f0c20145cb4e60bbb",