From 18f4a471737b3ff6973ea431f3e0b38135c97153 Mon Sep 17 00:00:00 2001 From: Andrew Kozlik Date: Thu, 24 Jun 2021 14:30:18 +0200 Subject: [PATCH 01/15] fix(legacy): Disable spending testnet coins from Bitcoin paths. --- .../firmware/.changelog.d/noissue.security.1 | 1 + legacy/firmware/crypto.c | 18 ++++++++++++++---- 2 files changed, 15 insertions(+), 4 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..471cda364 --- /dev/null +++ b/legacy/firmware/.changelog.d/noissue.security.1 @@ -0,0 +1 @@ +Disable all testnet coins from accessing Bitcoin paths. diff --git a/legacy/firmware/crypto.c b/legacy/firmware/crypto.c index a8cff67e9..1ed5f21c3 100644 --- a/legacy/firmware/crypto.c +++ b/legacy/firmware/crypto.c @@ -33,6 +33,12 @@ #include "segwit_addr.h" #include "sha2.h" +// SLIP-44 hardened coin type for Bitcoin +#define SLIP44_BITCOIN 0x80000000 + +// SLIP-44 hardened coin type for all Testnet coins +#define SLIP44_TESTNET 0x80000001 + uint32_t ser_length(uint32_t len, uint8_t *out) { if (len < 253) { out[0] = len & 0xFF; @@ -512,10 +518,14 @@ static bool check_cointype(const CoinInfo *coin, uint32_t slip44, bool full) { (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) { + // 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. However, we + // cannot allow spending any testnet coins from Bitcoin paths, because + // otherwise an attacker could trick the user into spending BCH on a Bitcoin + // path by signing a seemingly harmless BCH Testnet transaction. + if (slip44 == SLIP44_BITCOIN && coin->has_fork_id && + coin->coin_type != SLIP44_TESTNET) { return true; } } From a6390e4b824f9405ecd5513155f0a094885c210a Mon Sep 17 00:00:00 2001 From: Andrew Kozlik Date: Tue, 22 Jun 2021 16:54:34 +0200 Subject: [PATCH 02/15] fix(legacy): Allow known non-standard paths in GetAddress and SignTx. --- legacy/firmware/.changelog.d/1660.fixed | 1 + .../firmware/.changelog.d/noissue.security.4 | 1 + legacy/firmware/crypto.c | 108 ++++++++++++++++-- tests/device_tests/test_nonstandard_paths.py | 9 -- 4 files changed, 103 insertions(+), 16 deletions(-) create mode 100644 legacy/firmware/.changelog.d/1660.fixed create mode 100644 legacy/firmware/.changelog.d/noissue.security.4 diff --git a/legacy/firmware/.changelog.d/1660.fixed b/legacy/firmware/.changelog.d/1660.fixed new file mode 100644 index 000000000..badda7e78 --- /dev/null +++ b/legacy/firmware/.changelog.d/1660.fixed @@ -0,0 +1 @@ +Allow non-standard paths used by Unchained Capital, Green Address and Casa. diff --git a/legacy/firmware/.changelog.d/noissue.security.4 b/legacy/firmware/.changelog.d/noissue.security.4 new file mode 100644 index 000000000..87d744983 --- /dev/null +++ b/legacy/firmware/.changelog.d/noissue.security.4 @@ -0,0 +1 @@ +Restrict BIP-45 paths to Bitcoin and coins with strong replay protection. diff --git a/legacy/firmware/crypto.c b/legacy/firmware/crypto.c index 1ed5f21c3..da07aa44c 100644 --- a/legacy/firmware/crypto.c +++ b/legacy/firmware/crypto.c @@ -555,15 +555,49 @@ bool coin_known_path_check(const CoinInfo *coin, InputScriptType script_type, return valid; } - // m/45' - BIP45 Copay Abandoned Multisig P2SH - // m / purpose' / cosigner_index / change / address_index if (address_n_count > 0 && address_n[0] == (0x80000000 + 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 &= check_cointype(coin, SLIP44_BITCOIN, full); + } 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 &= check_cointype(coin, address_n[1], full); + } 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 &= check_cointype(coin, 0x80000000 | address_n[1], full); + } + if (full) { - valid &= (script_type == InputScriptType_SPENDMULTISIG); - valid &= (address_n_count == 4); - valid &= (address_n[1] & 0x80000000) == 0; - valid &= (address_n[2] & 0x80000000) == 0; - valid &= (address_n[3] & 0x80000000) == 0; + if (address_n_count == 4) { + valid &= (script_type == InputScriptType_SPENDMULTISIG); + valid &= (address_n[1] & 0x80000000) == 0; + valid &= (address_n[2] & 0x80000000) == 0; + valid &= (address_n[3] & 0x80000000) == 0; + } else if (address_n_count == 5) { + valid &= (script_type == InputScriptType_SPENDADDRESS || + script_type == InputScriptType_SPENDMULTISIG); + valid &= (address_n[2] & 0x80000000) == 0x80000000; + valid &= (address_n[2] & 0x7fffffff) <= 100; + valid &= address_n[3] <= 1000000; + valid &= address_n[4] <= 1000000; + } else if (address_n_count == 6) { + valid &= (script_type == InputScriptType_SPENDADDRESS || + script_type == InputScriptType_SPENDMULTISIG); + valid &= (address_n[1] & 0x80000000) == (address_n[2] & 0x80000000); + valid &= (address_n[2] & 0x7fffffff) <= 100; + valid &= address_n[3] <= 1000000; + valid &= address_n[4] <= 1; + valid &= address_n[5] <= 1000000; + } else { + return false; + } } return valid; } @@ -628,6 +662,66 @@ bool coin_known_path_check(const CoinInfo *coin, InputScriptType script_type, 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)) { + valid &= (coin->coin_type == SLIP44_BITCOIN); + if (full) { + valid &= (address_n_count == 2); + valid &= (address_n[1] <= 1000000); + } + 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] == (0x80000000 + 3)) { + valid &= (coin->coin_type == SLIP44_BITCOIN); + if (full) { + valid &= (address_n_count == 4); + valid &= (address_n[1] & 0x80000000) == 0x80000000; + valid &= (address_n[1] & 0x7fffffff) <= 100; + valid &= address_n[2] == 1 || address_n[2] == 4; + valid &= address_n[3] <= 1000000; + } + return valid; + } + + // 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) { + valid &= (coin->coin_type == SLIP44_BITCOIN); + if (full) { + if (address_n_count == 3) { + valid &= (address_n[1] == 6); + valid &= (address_n[2] <= 1000000); + } else if (address_n_count != 1) { + 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 (full) { + valid &= (address_n_count == 5); + } else { + valid &= (address_n_count >= 2); + } + valid &= check_cointype(coin, 0x80000000 | address_n[1], full); + if (full) { + valid &= (script_type == InputScriptType_SPENDP2SHWITNESS); + valid &= (address_n[1] & 0x80000000) == 0; + valid &= address_n[2] <= 100; + valid &= address_n[3] <= 1; + valid &= address_n[4] <= 1000000; + } + return valid; + } + // we don't check unknown paths return true; } diff --git a/tests/device_tests/test_nonstandard_paths.py b/tests/device_tests/test_nonstandard_paths.py index 7a8d84967..8082779b4 100644 --- a/tests/device_tests/test_nonstandard_paths.py +++ b/tests/device_tests/test_nonstandard_paths.py @@ -109,12 +109,6 @@ def test_getpublicnode(client, path, script_types): assert res.xpub -# See https://github.com/trezor/trezor-firmware/issues/1660 -# T1 fails on: -# test_getaddress[m/45'/0'/63'/1000000/0/255-script_types5] -# test_getaddress[m/45'/0'/63'/1000000/255-script_types7] -# test_getaddress[m/45'/0/63/1000000/0/255-script_types6] -@pytest.mark.skip_t1 @pytest.mark.parametrize("path, script_types", VECTORS) def test_getaddress(client, path, script_types): for script_type in script_types: @@ -170,9 +164,6 @@ def test_signtx(client, path, script_types): assert serialized_tx.hex() -# See https://github.com/trezor/trezor-firmware/issues/1660 -# T1 fails on the Unchained paths -@pytest.mark.skip_t1 @pytest.mark.multisig @pytest.mark.parametrize("paths, address_index", VECTORS_MULTISIG) def test_getaddress_multisig(client, paths, address_index): From 3884232f741fbab96e873e0c5a65afcea5311f94 Mon Sep 17 00:00:00 2001 From: Andrew Kozlik Date: Tue, 22 Jun 2021 17:29:40 +0200 Subject: [PATCH 03/15] feat(legacy): Restrict path ranges for account, change and address index. --- .../firmware/.changelog.d/noissue.security.3 | 1 + legacy/firmware/crypto.c | 66 ++++++++++++------- 2 files changed, 43 insertions(+), 24 deletions(-) create mode 100644 legacy/firmware/.changelog.d/noissue.security.3 diff --git a/legacy/firmware/.changelog.d/noissue.security.3 b/legacy/firmware/.changelog.d/noissue.security.3 new file mode 100644 index 000000000..7f1adb70b --- /dev/null +++ b/legacy/firmware/.changelog.d/noissue.security.3 @@ -0,0 +1 @@ +Restrict the BIP-32 path ranges of account, change and address_index fields. diff --git a/legacy/firmware/crypto.c b/legacy/firmware/crypto.c index da07aa44c..d0d6f1e41 100644 --- a/legacy/firmware/crypto.c +++ b/legacy/firmware/crypto.c @@ -33,6 +33,10 @@ #include "segwit_addr.h" #include "sha2.h" +#define PATH_MAX_ACCOUNT 100 +#define PATH_MAX_CHANGE 1 +#define PATH_MAX_ADDRESS_INDEX 1000000 + // SLIP-44 hardened coin type for Bitcoin #define SLIP44_BITCOIN 0x80000000 @@ -549,8 +553,9 @@ bool coin_known_path_check(const CoinInfo *coin, InputScriptType script_type, if (full) { valid &= (script_type == InputScriptType_SPENDADDRESS); valid &= (address_n[2] & 0x80000000) == 0x80000000; - valid &= (address_n[3] & 0x80000000) == 0; - valid &= (address_n[4] & 0x80000000) == 0; + valid &= (address_n[2] & 0x7fffffff) <= PATH_MAX_ACCOUNT; + valid &= (address_n[3] <= PATH_MAX_CHANGE); + valid &= (address_n[4] <= PATH_MAX_ADDRESS_INDEX); } return valid; } @@ -577,24 +582,24 @@ bool coin_known_path_check(const CoinInfo *coin, InputScriptType script_type, if (full) { if (address_n_count == 4) { valid &= (script_type == InputScriptType_SPENDMULTISIG); - valid &= (address_n[1] & 0x80000000) == 0; - valid &= (address_n[2] & 0x80000000) == 0; - valid &= (address_n[3] & 0x80000000) == 0; + valid &= (address_n[1] <= 100); + valid &= (address_n[2] <= PATH_MAX_CHANGE); + valid &= (address_n[3] <= PATH_MAX_ADDRESS_INDEX); } else if (address_n_count == 5) { valid &= (script_type == InputScriptType_SPENDADDRESS || script_type == InputScriptType_SPENDMULTISIG); valid &= (address_n[2] & 0x80000000) == 0x80000000; - valid &= (address_n[2] & 0x7fffffff) <= 100; - valid &= address_n[3] <= 1000000; - valid &= address_n[4] <= 1000000; + valid &= (address_n[2] & 0x7fffffff) <= PATH_MAX_ACCOUNT; + valid &= (address_n[3] <= 1000000); + valid &= (address_n[4] <= PATH_MAX_ADDRESS_INDEX); } else if (address_n_count == 6) { valid &= (script_type == InputScriptType_SPENDADDRESS || script_type == InputScriptType_SPENDMULTISIG); valid &= (address_n[1] & 0x80000000) == (address_n[2] & 0x80000000); - valid &= (address_n[2] & 0x7fffffff) <= 100; - valid &= address_n[3] <= 1000000; - valid &= address_n[4] <= 1; - valid &= address_n[5] <= 1000000; + valid &= (address_n[2] & 0x7fffffff) <= PATH_MAX_ACCOUNT; + valid &= (address_n[3] <= 1000000); + valid &= (address_n[4] <= PATH_MAX_CHANGE); + valid &= (address_n[5] <= PATH_MAX_ADDRESS_INDEX); } else { return false; } @@ -618,7 +623,18 @@ bool coin_known_path_check(const CoinInfo *coin, InputScriptType script_type, (script_type == InputScriptType_SPENDP2SHWITNESS) || (script_type == InputScriptType_SPENDWITNESS); valid &= (address_n[2] & 0x80000000) == 0x80000000; - valid &= (address_n[4] & 0x80000000) == 0; + valid &= (address_n[2] & 0x7fffffff) <= PATH_MAX_ACCOUNT; + if (address_n_count == 5) { + valid &= (address_n[3] <= PATH_MAX_CHANGE); + valid &= (address_n[4] <= PATH_MAX_ADDRESS_INDEX); + } else if (address_n_count == 6) { + valid &= (address_n[3] & 0x80000000) == 0x80000000; + valid &= (address_n[3] & 0x7fffffff) <= 3; + valid &= (address_n[4] <= PATH_MAX_CHANGE); + valid &= (address_n[5] <= PATH_MAX_ADDRESS_INDEX); + } else { + return false; + } } return valid; } @@ -636,8 +652,9 @@ bool coin_known_path_check(const CoinInfo *coin, InputScriptType script_type, if (full) { valid &= (script_type == InputScriptType_SPENDP2SHWITNESS); valid &= (address_n[2] & 0x80000000) == 0x80000000; - valid &= (address_n[3] & 0x80000000) == 0; - valid &= (address_n[4] & 0x80000000) == 0; + valid &= (address_n[2] & 0x7fffffff) <= PATH_MAX_ACCOUNT; + valid &= (address_n[3] <= PATH_MAX_CHANGE); + valid &= (address_n[4] <= PATH_MAX_ADDRESS_INDEX); } return valid; } @@ -656,8 +673,9 @@ bool coin_known_path_check(const CoinInfo *coin, InputScriptType script_type, if (full) { valid &= (script_type == InputScriptType_SPENDWITNESS); valid &= (address_n[2] & 0x80000000) == 0x80000000; - valid &= (address_n[3] & 0x80000000) == 0; - valid &= (address_n[4] & 0x80000000) == 0; + valid &= (address_n[2] & 0x7fffffff) <= PATH_MAX_ACCOUNT; + valid &= (address_n[3] <= PATH_MAX_CHANGE); + valid &= (address_n[4] <= PATH_MAX_ADDRESS_INDEX); } return valid; } @@ -668,7 +686,7 @@ bool coin_known_path_check(const CoinInfo *coin, InputScriptType script_type, valid &= (coin->coin_type == SLIP44_BITCOIN); if (full) { valid &= (address_n_count == 2); - valid &= (address_n[1] <= 1000000); + valid &= (address_n[1] <= PATH_MAX_ADDRESS_INDEX); } return valid; } @@ -681,8 +699,8 @@ bool coin_known_path_check(const CoinInfo *coin, InputScriptType script_type, valid &= (address_n_count == 4); valid &= (address_n[1] & 0x80000000) == 0x80000000; valid &= (address_n[1] & 0x7fffffff) <= 100; - valid &= address_n[2] == 1 || address_n[2] == 4; - valid &= address_n[3] <= 1000000; + valid &= (address_n[2] == 1 || address_n[2] == 4); + valid &= (address_n[3] <= PATH_MAX_ADDRESS_INDEX); } return valid; } @@ -695,7 +713,7 @@ bool coin_known_path_check(const CoinInfo *coin, InputScriptType script_type, if (full) { if (address_n_count == 3) { valid &= (address_n[1] == 6); - valid &= (address_n[2] <= 1000000); + valid &= (address_n[2] <= PATH_MAX_ADDRESS_INDEX); } else if (address_n_count != 1) { return false; } @@ -715,9 +733,9 @@ bool coin_known_path_check(const CoinInfo *coin, InputScriptType script_type, if (full) { valid &= (script_type == InputScriptType_SPENDP2SHWITNESS); valid &= (address_n[1] & 0x80000000) == 0; - valid &= address_n[2] <= 100; - valid &= address_n[3] <= 1; - valid &= address_n[4] <= 1000000; + valid &= (address_n[2] <= PATH_MAX_ACCOUNT); + valid &= (address_n[3] <= PATH_MAX_CHANGE); + valid &= (address_n[4] <= PATH_MAX_ADDRESS_INDEX); } return valid; } From 3e9f8a32acbf39f9cb5559607cc84774219f7c65 Mon Sep 17 00:00:00 2001 From: Andrew Kozlik Date: Wed, 23 Jun 2021 15:41:22 +0200 Subject: [PATCH 04/15] fix(core): Fix insufficient BIP-32 path checks. - Disable testnet coins from accessing Bitcoin paths. - Restrict BIP-45 paths to Bitcoin and coins with strong replay protection. --- core/.changelog.d/noissue.security | 1 + core/.changelog.d/noissue.security.1 | 1 + core/src/apps/bitcoin/keychain.py | 54 ++++++++++++++++++------ core/tests/test_apps.bitcoin.keychain.py | 10 ++--- 4 files changed, 48 insertions(+), 18 deletions(-) create mode 100644 core/.changelog.d/noissue.security create mode 100644 core/.changelog.d/noissue.security.1 diff --git a/core/.changelog.d/noissue.security b/core/.changelog.d/noissue.security new file mode 100644 index 000000000..471cda364 --- /dev/null +++ b/core/.changelog.d/noissue.security @@ -0,0 +1 @@ +Disable all testnet coins from accessing Bitcoin paths. diff --git a/core/.changelog.d/noissue.security.1 b/core/.changelog.d/noissue.security.1 new file mode 100644 index 000000000..87d744983 --- /dev/null +++ b/core/.changelog.d/noissue.security.1 @@ -0,0 +1 @@ +Restrict BIP-45 paths to Bitcoin and coins with strong replay protection. diff --git a/core/src/apps/bitcoin/keychain.py b/core/src/apps/bitcoin/keychain.py index 75ca1071c..13948e651 100644 --- a/core/src/apps/bitcoin/keychain.py +++ b/core/src/apps/bitcoin/keychain.py @@ -1,4 +1,5 @@ import gc +from micropython import const from trezor import wire from trezor.enums import InputScriptType @@ -61,6 +62,12 @@ PATTERN_UNCHAINED_UNHARDENED = ( ) PATTERN_UNCHAINED_DEPRECATED = "m/45'/coin_type'/account'/[0-1000000]/address_index" +# SLIP-44 coin type for Bitcoin +SLIP44_BITCOIN = const(0) + +# SLIP-44 coin type for all Testnet coins +SLIP44_TESTNET = const(1) + def validate_path_against_script_type( coin: coininfo.CoinInfo, @@ -82,7 +89,7 @@ def validate_path_against_script_type( if script_type == InputScriptType.SPENDADDRESS and not multisig: patterns.append(PATTERN_BIP44) - if coin.coin_name in BITCOIN_NAMES: + if coin.slip44 == SLIP44_BITCOIN: patterns.append(PATTERN_GREENADDRESS_A) patterns.append(PATTERN_GREENADDRESS_B) @@ -90,11 +97,15 @@ def validate_path_against_script_type( script_type in (InputScriptType.SPENDADDRESS, InputScriptType.SPENDMULTISIG) and multisig ): - patterns.append(PATTERN_BIP45) patterns.append(PATTERN_PURPOSE48_RAW) - if coin.coin_name in BITCOIN_NAMES: + if coin.slip44 == SLIP44_BITCOIN or ( + coin.fork_id is not None and coin.slip44 != SLIP44_TESTNET + ): + patterns.append(PATTERN_BIP45) + if coin.slip44 == SLIP44_BITCOIN: patterns.append(PATTERN_GREENADDRESS_A) patterns.append(PATTERN_GREENADDRESS_B) + if coin.coin_name in BITCOIN_NAMES: patterns.append(PATTERN_UNCHAINED_HARDENED) patterns.append(PATTERN_UNCHAINED_UNHARDENED) patterns.append(PATTERN_UNCHAINED_DEPRECATED) @@ -103,16 +114,17 @@ def validate_path_against_script_type( patterns.append(PATTERN_BIP49) if multisig: patterns.append(PATTERN_PURPOSE48_P2SHSEGWIT) - if coin.coin_name in BITCOIN_NAMES: + if coin.slip44 == SLIP44_BITCOIN: patterns.append(PATTERN_GREENADDRESS_A) patterns.append(PATTERN_GREENADDRESS_B) + if coin.coin_name in BITCOIN_NAMES: patterns.append(PATTERN_CASA) elif coin.segwit and script_type == InputScriptType.SPENDWITNESS: patterns.append(PATTERN_BIP84) if multisig: patterns.append(PATTERN_PURPOSE48_SEGWIT) - if coin.coin_name in BITCOIN_NAMES: + if coin.slip44 == SLIP44_BITCOIN: patterns.append(PATTERN_GREENADDRESS_A) patterns.append(PATTERN_GREENADDRESS_B) @@ -125,18 +137,29 @@ def get_schemas_for_coin(coin: coininfo.CoinInfo) -> Iterable[PathSchema]: # basic patterns patterns = [ PATTERN_BIP44, - PATTERN_BIP45, PATTERN_PURPOSE48_RAW, ] - # compatibility patterns - if coin.coin_name in BITCOIN_NAMES: + # patterns without coin_type field must be treated as if coin_type == 0 + if coin.slip44 == SLIP44_BITCOIN or ( + coin.fork_id is not None and coin.slip44 != SLIP44_TESTNET + ): + patterns.append(PATTERN_BIP45) + + if coin.slip44 == SLIP44_BITCOIN: patterns.extend( ( PATTERN_GREENADDRESS_A, PATTERN_GREENADDRESS_B, PATTERN_GREENADDRESS_SIGN_A, PATTERN_GREENADDRESS_SIGN_B, + ) + ) + + # compatibility patterns + if coin.coin_name in BITCOIN_NAMES: + patterns.extend( + ( PATTERN_CASA, PATTERN_UNCHAINED_HARDENED, PATTERN_UNCHAINED_UNHARDENED, @@ -157,11 +180,16 @@ def get_schemas_for_coin(coin: coininfo.CoinInfo) -> Iterable[PathSchema]: schemas = [PathSchema.parse(pattern, coin.slip44) for pattern in patterns] - # 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: - schemas.extend(PathSchema.parse(pattern, 0) for pattern in patterns) + # 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. However, we + # cannot allow spending any testnet coins from Bitcoin paths, because + # otherwise an attacker could trick the user into spending BCH on a Bitcoin + # path by signing a seemingly harmless BCH Testnet transaction. + if coin.fork_id is not None and coin.slip44 != SLIP44_TESTNET: + schemas.extend( + PathSchema.parse(pattern, SLIP44_BITCOIN) for pattern in patterns + ) gc.collect() return [schema.copy() for schema in schemas] diff --git a/core/tests/test_apps.bitcoin.keychain.py b/core/tests/test_apps.bitcoin.keychain.py index 611c73fe3..695a272e9 100644 --- a/core/tests/test_apps.bitcoin.keychain.py +++ b/core/tests/test_apps.bitcoin.keychain.py @@ -53,15 +53,11 @@ class TestBitcoinKeychain(unittest.TestCase): valid_addresses = ( [H_(44), H_(1), H_(0), 0, 0], - [H_(45), 99, 1, 1000], [H_(48), H_(1), H_(0), H_(2), 1, 1000], [H_(49), H_(1), H_(0), 0, 10], [H_(84), H_(1), H_(0), 0, 10], # Casa: [49, 1, 0, 0, 10], - # Green: - [1, 1000], - [H_(3), H_(10), 4, 1000], ) invalid_addresses = ( [H_(43), H_(1), H_(0), 0, 0], @@ -69,6 +65,10 @@ class TestBitcoinKeychain(unittest.TestCase): [44, 1, 0, 0, 0], [H_(44), H_(1), H_(0)], [H_(44), H_(1), H_(0), 0, 0, 0], + [H_(45), 99, 1, 1000], + # Green: + [1, 1000], + [H_(3), H_(10), 4, 1000], ) for addr in valid_addresses: @@ -141,7 +141,6 @@ class TestAltcoinKeychains(unittest.TestCase): self.assertTrue(coin.segwit) valid_addresses = ( [H_(44), H_(2), H_(0), 0, 0], - [H_(45), 99, 1, 1000], [H_(48), H_(2), H_(0), H_(2), 1, 1000], [H_(49), H_(2), H_(0), 0, 10], [H_(84), H_(2), H_(0), 0, 10], @@ -150,6 +149,7 @@ class TestAltcoinKeychains(unittest.TestCase): [H_(43), H_(2), H_(0), 0, 0], # Bitcoin paths: [H_(44), H_(0), H_(0), 0, 0], + [H_(45), 99, 1, 1000], [H_(49), H_(0), H_(0), 0, 0], [H_(84), H_(0), H_(0), 0, 0], From 9b0e980c4461e0b0f60e35ecf24ce6e0f60bc98e Mon Sep 17 00:00:00 2001 From: Andrew Kozlik Date: Wed, 23 Jun 2021 17:28:50 +0200 Subject: [PATCH 05/15] fix(legacy): Don't allow unknown paths in GetAddress. --- legacy/firmware/.changelog.d/noissue.security | 1 + legacy/firmware/crypto.c | 12 ++++++++++-- tests/device_tests/test_msg_getaddress_show.py | 12 ++++++++++++ 3 files changed, 23 insertions(+), 2 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..9e933c364 --- /dev/null +++ b/legacy/firmware/.changelog.d/noissue.security @@ -0,0 +1 @@ +Don't show addresses that have an unrecognized path. diff --git a/legacy/firmware/crypto.c b/legacy/firmware/crypto.c index d0d6f1e41..dfb1a3533 100644 --- a/legacy/firmware/crypto.c +++ b/legacy/firmware/crypto.c @@ -540,6 +540,14 @@ static bool check_cointype(const CoinInfo *coin, uint32_t slip44, bool full) { bool coin_known_path_check(const CoinInfo *coin, InputScriptType script_type, uint32_t address_n_count, const uint32_t *address_n, bool full) { + // If full == true, 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. + // If full == false, 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. + bool valid = true; // m/44' : BIP44 Legacy // m / purpose' / coin_type' / account' / change / address_index @@ -740,6 +748,6 @@ bool coin_known_path_check(const CoinInfo *coin, InputScriptType script_type, return valid; } - // we don't check unknown paths - return true; + // we allow unknown paths when a full check is not required + return !full; } diff --git a/tests/device_tests/test_msg_getaddress_show.py b/tests/device_tests/test_msg_getaddress_show.py index a378622fb..7d263bfe5 100644 --- a/tests/device_tests/test_msg_getaddress_show.py +++ b/tests/device_tests/test_msg_getaddress_show.py @@ -17,6 +17,7 @@ import pytest from trezorlib import btc, messages, tools +from trezorlib.exceptions import TrezorFailure VECTORS = ( # path, script_type, address ( @@ -51,6 +52,17 @@ def test_show(client, path, script_type, address): ) +def test_show_unrecognized_path(client): + with pytest.raises(TrezorFailure): + btc.get_address( + client, + "Bitcoin", + tools.parse_path("m/24684621h/516582h/5156h/21/856"), + script_type=messages.InputScriptType.SPENDWITNESS, + show_display=True, + ) + + @pytest.mark.multisig def test_show_multisig_3(client): node = btc.get_public_node( From 28421594c99f859239d6e3c2a42b8ef2991f8b38 Mon Sep 17 00:00:00 2001 From: Andrew Kozlik Date: Wed, 23 Jun 2021 17:38:35 +0200 Subject: [PATCH 06/15] fix(legacy): Use short-circuit evaluation in coin_known_path_check(). --- legacy/firmware/crypto.c | 163 ++++++++++++++++++++------------------- 1 file changed, 82 insertions(+), 81 deletions(-) diff --git a/legacy/firmware/crypto.c b/legacy/firmware/crypto.c index dfb1a3533..dc079795d 100644 --- a/legacy/firmware/crypto.c +++ b/legacy/firmware/crypto.c @@ -553,17 +553,17 @@ bool coin_known_path_check(const CoinInfo *coin, InputScriptType script_type, // m / purpose' / coin_type' / account' / change / address_index if (address_n_count > 0 && address_n[0] == (0x80000000 + 44)) { if (full) { - valid &= (address_n_count == 5); + valid = valid && (address_n_count == 5); } else { - valid &= (address_n_count >= 2); + valid = valid && (address_n_count >= 2); } - valid &= check_cointype(coin, address_n[1], full); + valid = valid && check_cointype(coin, address_n[1], full); if (full) { - valid &= (script_type == InputScriptType_SPENDADDRESS); - valid &= (address_n[2] & 0x80000000) == 0x80000000; - valid &= (address_n[2] & 0x7fffffff) <= PATH_MAX_ACCOUNT; - valid &= (address_n[3] <= PATH_MAX_CHANGE); - valid &= (address_n[4] <= PATH_MAX_ADDRESS_INDEX); + valid = valid && (script_type == InputScriptType_SPENDADDRESS); + valid = valid && ((address_n[2] & 0x80000000) == 0x80000000); + 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; } @@ -573,41 +573,42 @@ bool coin_known_path_check(const CoinInfo *coin, InputScriptType script_type, // 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 &= check_cointype(coin, SLIP44_BITCOIN, full); + valid = valid && check_cointype(coin, SLIP44_BITCOIN, full); } 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 &= check_cointype(coin, address_n[1], full); + valid = valid && check_cointype(coin, address_n[1], full); } 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 &= check_cointype(coin, 0x80000000 | address_n[1], full); + valid = valid && check_cointype(coin, 0x80000000 | address_n[1], full); } if (full) { if (address_n_count == 4) { - valid &= (script_type == InputScriptType_SPENDMULTISIG); - valid &= (address_n[1] <= 100); - valid &= (address_n[2] <= PATH_MAX_CHANGE); - valid &= (address_n[3] <= PATH_MAX_ADDRESS_INDEX); + valid = valid && (script_type == InputScriptType_SPENDMULTISIG); + 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) { - valid &= (script_type == InputScriptType_SPENDADDRESS || - script_type == InputScriptType_SPENDMULTISIG); - valid &= (address_n[2] & 0x80000000) == 0x80000000; - valid &= (address_n[2] & 0x7fffffff) <= PATH_MAX_ACCOUNT; - valid &= (address_n[3] <= 1000000); - valid &= (address_n[4] <= PATH_MAX_ADDRESS_INDEX); + valid = valid && (script_type == InputScriptType_SPENDADDRESS || + script_type == InputScriptType_SPENDMULTISIG); + valid = valid && ((address_n[2] & 0x80000000) == 0x80000000); + valid = valid && ((address_n[2] & 0x7fffffff) <= PATH_MAX_ACCOUNT); + valid = valid && (address_n[3] <= 1000000); + valid = valid && (address_n[4] <= PATH_MAX_ADDRESS_INDEX); } else if (address_n_count == 6) { - valid &= (script_type == InputScriptType_SPENDADDRESS || - script_type == InputScriptType_SPENDMULTISIG); - valid &= (address_n[1] & 0x80000000) == (address_n[2] & 0x80000000); - valid &= (address_n[2] & 0x7fffffff) <= PATH_MAX_ACCOUNT; - valid &= (address_n[3] <= 1000000); - valid &= (address_n[4] <= PATH_MAX_CHANGE); - valid &= (address_n[5] <= PATH_MAX_ADDRESS_INDEX); + valid = valid && (script_type == InputScriptType_SPENDADDRESS || + script_type == InputScriptType_SPENDMULTISIG); + valid = valid && + ((address_n[1] & 0x80000000) == (address_n[2] & 0x80000000)); + 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); } else { return false; } @@ -621,25 +622,25 @@ bool coin_known_path_check(const CoinInfo *coin, InputScriptType script_type, // m / purpose' / coin_type' / account' / type' / change / address_index if (address_n_count > 0 && address_n[0] == (0x80000000 + 48)) { if (full) { - valid &= (address_n_count == 5) || (address_n_count == 6); + valid = valid && (address_n_count == 5 || address_n_count == 6); } else { - valid &= (address_n_count >= 2); + valid = valid && (address_n_count >= 2); } - valid &= check_cointype(coin, address_n[1], full); + valid = valid && check_cointype(coin, address_n[1], full); if (full) { - valid &= (script_type == InputScriptType_SPENDMULTISIG) || - (script_type == InputScriptType_SPENDP2SHWITNESS) || - (script_type == InputScriptType_SPENDWITNESS); - valid &= (address_n[2] & 0x80000000) == 0x80000000; - valid &= (address_n[2] & 0x7fffffff) <= PATH_MAX_ACCOUNT; + valid = valid && (script_type == InputScriptType_SPENDMULTISIG || + script_type == InputScriptType_SPENDP2SHWITNESS || + script_type == InputScriptType_SPENDWITNESS); + valid = valid && ((address_n[2] & 0x80000000) == 0x80000000); + valid = valid && ((address_n[2] & 0x7fffffff) <= PATH_MAX_ACCOUNT); if (address_n_count == 5) { - valid &= (address_n[3] <= PATH_MAX_CHANGE); - valid &= (address_n[4] <= PATH_MAX_ADDRESS_INDEX); + valid = valid && (address_n[3] <= PATH_MAX_CHANGE); + valid = valid && (address_n[4] <= PATH_MAX_ADDRESS_INDEX); } else if (address_n_count == 6) { - valid &= (address_n[3] & 0x80000000) == 0x80000000; - valid &= (address_n[3] & 0x7fffffff) <= 3; - valid &= (address_n[4] <= PATH_MAX_CHANGE); - valid &= (address_n[5] <= PATH_MAX_ADDRESS_INDEX); + valid = valid && ((address_n[3] & 0x80000000) == 0x80000000); + 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; } @@ -650,19 +651,19 @@ bool coin_known_path_check(const CoinInfo *coin, InputScriptType script_type, // m/49' : BIP49 SegWit // m / purpose' / coin_type' / account' / change / address_index if (address_n_count > 0 && address_n[0] == (0x80000000 + 49)) { - valid &= coin->has_segwit; + valid = valid && coin->has_segwit; if (full) { - valid &= (address_n_count == 5); + valid = valid && (address_n_count == 5); } else { - valid &= (address_n_count >= 2); + valid = valid && (address_n_count >= 2); } - valid &= check_cointype(coin, address_n[1], full); + valid = valid && check_cointype(coin, address_n[1], full); if (full) { - valid &= (script_type == InputScriptType_SPENDP2SHWITNESS); - valid &= (address_n[2] & 0x80000000) == 0x80000000; - valid &= (address_n[2] & 0x7fffffff) <= PATH_MAX_ACCOUNT; - valid &= (address_n[3] <= PATH_MAX_CHANGE); - valid &= (address_n[4] <= PATH_MAX_ADDRESS_INDEX); + valid = valid && (script_type == InputScriptType_SPENDP2SHWITNESS); + valid = valid && ((address_n[2] & 0x80000000) == 0x80000000); + 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; } @@ -670,20 +671,20 @@ bool coin_known_path_check(const CoinInfo *coin, InputScriptType script_type, // m/84' : BIP84 Native SegWit // m / purpose' / coin_type' / account' / change / address_index if (address_n_count > 0 && address_n[0] == (0x80000000 + 84)) { - valid &= coin->has_segwit; - valid &= coin->bech32_prefix != NULL; + valid = valid && coin->has_segwit; + valid = valid && (coin->bech32_prefix != NULL); if (full) { - valid &= (address_n_count == 5); + valid = valid && (address_n_count == 5); } else { - valid &= (address_n_count >= 2); + valid = valid && (address_n_count >= 2); } - valid &= check_cointype(coin, address_n[1], full); + valid = valid && check_cointype(coin, address_n[1], full); if (full) { - valid &= (script_type == InputScriptType_SPENDWITNESS); - valid &= (address_n[2] & 0x80000000) == 0x80000000; - valid &= (address_n[2] & 0x7fffffff) <= PATH_MAX_ACCOUNT; - valid &= (address_n[3] <= PATH_MAX_CHANGE); - valid &= (address_n[4] <= PATH_MAX_ADDRESS_INDEX); + valid = valid && (script_type == InputScriptType_SPENDWITNESS); + valid = valid && ((address_n[2] & 0x80000000) == 0x80000000); + 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; } @@ -691,10 +692,10 @@ bool coin_known_path_check(const CoinInfo *coin, InputScriptType script_type, // 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)) { - valid &= (coin->coin_type == SLIP44_BITCOIN); + valid = valid && (coin->coin_type == SLIP44_BITCOIN); if (full) { - valid &= (address_n_count == 2); - valid &= (address_n[1] <= PATH_MAX_ADDRESS_INDEX); + valid = valid && (address_n_count == 2); + valid = valid && (address_n[1] <= PATH_MAX_ADDRESS_INDEX); } return valid; } @@ -702,13 +703,13 @@ bool coin_known_path_check(const CoinInfo *coin, InputScriptType script_type, // 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] == (0x80000000 + 3)) { - valid &= (coin->coin_type == SLIP44_BITCOIN); + valid = valid && (coin->coin_type == SLIP44_BITCOIN); if (full) { - valid &= (address_n_count == 4); - valid &= (address_n[1] & 0x80000000) == 0x80000000; - valid &= (address_n[1] & 0x7fffffff) <= 100; - valid &= (address_n[2] == 1 || address_n[2] == 4); - valid &= (address_n[3] <= PATH_MAX_ADDRESS_INDEX); + valid = valid && (address_n_count == 4); + valid = valid && ((address_n[1] & 0x80000000) == 0x80000000); + 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); } return valid; } @@ -717,11 +718,11 @@ bool coin_known_path_check(const CoinInfo *coin, InputScriptType script_type, // m / 1195487518 // m / 1195487518 / 6 / address_index if (address_n_count > 0 && address_n[0] == 1195487518) { - valid &= (coin->coin_type == SLIP44_BITCOIN); + valid = valid && (coin->coin_type == SLIP44_BITCOIN); if (full) { if (address_n_count == 3) { - valid &= (address_n[1] == 6); - valid &= (address_n[2] <= PATH_MAX_ADDRESS_INDEX); + valid = valid && (address_n[1] == 6); + valid = valid && (address_n[2] <= PATH_MAX_ADDRESS_INDEX); } else if (address_n_count != 1) { return false; } @@ -733,17 +734,17 @@ bool coin_known_path_check(const CoinInfo *coin, InputScriptType script_type, // m / 49 / coin_type / account / change / address_index if (address_n_count > 0 && address_n[0] == 49) { if (full) { - valid &= (address_n_count == 5); + valid = valid && (address_n_count == 5); } else { - valid &= (address_n_count >= 2); + valid = valid && (address_n_count >= 2); } - valid &= check_cointype(coin, 0x80000000 | address_n[1], full); + valid = valid && check_cointype(coin, 0x80000000 | address_n[1], full); if (full) { - valid &= (script_type == InputScriptType_SPENDP2SHWITNESS); - valid &= (address_n[1] & 0x80000000) == 0; - valid &= (address_n[2] <= PATH_MAX_ACCOUNT); - valid &= (address_n[3] <= PATH_MAX_CHANGE); - valid &= (address_n[4] <= PATH_MAX_ADDRESS_INDEX); + valid = valid && (script_type == InputScriptType_SPENDP2SHWITNESS); + valid = valid && ((address_n[1] & 0x80000000) == 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; } From c06761882827abb2802b27d75a10504fe970da74 Mon Sep 17 00:00:00 2001 From: Andrew Kozlik Date: Thu, 24 Jun 2021 11:11:01 +0200 Subject: [PATCH 07/15] fix(common): Fix incorrect SLIP-44 coin type for Bgold and SmartCash Testnets. --- common/defs/bitcoin/bgold_testnet.json | 2 +- common/defs/bitcoin/smartcash_testnet.json | 2 +- core/.changelog.d/noissue.security.2 | 1 + core/src/apps/common/coininfo.py | 4 ++-- legacy/firmware/.changelog.d/noissue.security.2 | 1 + 5 files changed, 6 insertions(+), 4 deletions(-) create mode 100644 core/.changelog.d/noissue.security.2 create mode 100644 legacy/firmware/.changelog.d/noissue.security.2 diff --git a/common/defs/bitcoin/bgold_testnet.json b/common/defs/bitcoin/bgold_testnet.json index 4c4dd8c2e..a4908afff 100644 --- a/common/defs/bitcoin/bgold_testnet.json +++ b/common/defs/bitcoin/bgold_testnet.json @@ -21,7 +21,7 @@ "xpub_magic_multisig_segwit_native": 70617039, "bech32_prefix": "tbtg", "cashaddr_prefix": null, - "slip44": 156, + "slip44": 1, "segwit": true, "decred": false, "fork_id": 79, diff --git a/common/defs/bitcoin/smartcash_testnet.json b/common/defs/bitcoin/smartcash_testnet.json index 16cdee8f6..94caa5e1d 100644 --- a/common/defs/bitcoin/smartcash_testnet.json +++ b/common/defs/bitcoin/smartcash_testnet.json @@ -21,7 +21,7 @@ "xpub_magic_multisig_segwit_native": null, "bech32_prefix": null, "cashaddr_prefix": null, - "slip44": 224, + "slip44": 1, "segwit": false, "decred": false, "fork_id": null, diff --git a/core/.changelog.d/noissue.security.2 b/core/.changelog.d/noissue.security.2 new file mode 100644 index 000000000..6c34e3650 --- /dev/null +++ b/core/.changelog.d/noissue.security.2 @@ -0,0 +1 @@ +Ensure that all testnet coins use SLIP-44 coin type 1. diff --git a/core/src/apps/common/coininfo.py b/core/src/apps/common/coininfo.py index 0d532028e..620adc8aa 100644 --- a/core/src/apps/common/coininfo.py +++ b/core/src/apps/common/coininfo.py @@ -390,7 +390,7 @@ def by_name(name: str) -> CoinInfo: xpub_magic_multisig_segwit_native=0x043587cf, bech32_prefix="tbtg", cashaddr_prefix=None, - slip44=156, + slip44=1, segwit=True, fork_id=79, force_bip143=True, @@ -1482,7 +1482,7 @@ def by_name(name: str) -> CoinInfo: xpub_magic_multisig_segwit_native=None, bech32_prefix=None, cashaddr_prefix=None, - slip44=224, + slip44=1, segwit=False, fork_id=None, force_bip143=False, diff --git a/legacy/firmware/.changelog.d/noissue.security.2 b/legacy/firmware/.changelog.d/noissue.security.2 new file mode 100644 index 000000000..6c34e3650 --- /dev/null +++ b/legacy/firmware/.changelog.d/noissue.security.2 @@ -0,0 +1 @@ +Ensure that all testnet coins use SLIP-44 coin type 1. From e3faece8114b25893f5672ec12ab53d2b1d5ece0 Mon Sep 17 00:00:00 2001 From: Andrew Kozlik Date: Thu, 24 Jun 2021 15:31:49 +0200 Subject: [PATCH 08/15] fix(tests): Fix device tests after having disabled testnet for BIP-45 paths. --- tests/device_tests/test_multisig_change.py | 47 ++++++++++++---------- tests/ui_tests/fixtures.json | 17 ++++---- 2 files changed, 34 insertions(+), 30 deletions(-) diff --git a/tests/device_tests/test_multisig_change.py b/tests/device_tests/test_multisig_change.py index 9bd6a53bc..2bd5a99c2 100644 --- a/tests/device_tests/test_multisig_change.py +++ b/tests/device_tests/test_multisig_change.py @@ -26,6 +26,9 @@ from .signtx import request_finished, request_input, request_meta, request_outpu B = proto.ButtonRequestType TX_API = TxCache("Testnet") +# NOTE: This test case was migrated from Testnet to Bitcoin, because we +# disabled testnet for BIP-45 paths. So we are still using the Testnet TxCache +# here, but everything else has been changed to Bitcoin mainnet. TXHASH_16c6c8 = bytes.fromhex( "16c6c8471b8db7a628f2b2bb86bfeefae1766463ce8692438c7fd3fce3f43ce5" @@ -40,25 +43,25 @@ TXHASH_b0946d = bytes.fromhex( class TestMultisigChange: node_ext1 = bip32.deserialize( - "tpubDADHV9u9Y6gkggintTdMjJE3be58zKNLhpxBQyuEM6Pwx3sN9JVLmMCMN4DNVwL9AKec27z5TaWcWuHzMXiGAtcra5DjwWbvppGX4gaEGVN" + "xpub69qexv5TppjJQtXwSGeGXNtgGWyUzvsHACMt4Rr61Be4CmCf55eFcuXX828aySNuNR7hQYUCvUgZpioNxfs2HTAZWUUSFywhErg7JfTPv3Y" ) # m/1 => 02c0d0c5fee952620757c6128dbf327c996cd72ed3358d15d6518a1186099bc15e # m/2 => 0375b9dfaad928ce1a7eed88df7c084e67d99e9ab74332419458a9a45779706801 node_ext2 = bip32.deserialize( - "tpubDADHV9u9Y6gkhWXBmDJ6TUhZajLWjvKukRe2w9FfhdbQpUux8Z8jnPHNAZqFRgHPg9sR7YR93xThM32M7NfRu8S5WyDtext7S62sqxeJNkd" + "xpub69qexv5TppjJRiLLK2K1FZNCFcErkXprCo3jabCXMiqX5CFF4LHedwcXvXkTuBL9tFLWVxuGWrdeerXjiWpC1gynTNUaySDsr8SU5xMpj5R" ) # m/1 => 0388460dc439f4c8f5bcfc268c36e11b4375cad5c3535c336cfdf8c32c3afad5c1 # m/2 => 03a04f945d5a3685729dde697d574076de4bdf38e904f813b22a851548e1110fc0 node_ext3 = bip32.deserialize( - "tpubDADHV9u9Y6gkmM5ohWRGTswrc6fr7soH7e2D2ic5a86PDUaHc5Ln9EbER69cEr5bDZPa7EXguJ1MhWVzPZpZWVdG5fvoF3hfirXvRbpCCBg" + "xpub69qexv5TppjJVYtxFKSBFxcVGyaC8VJDa1RugAYwEDLVUBuaXrVgznvQB44piM8MRerfVf1pNCBK1L1NzhyKd4Ay25BVZX3S8twWfZDxmz7" ) # m/1 => 02e0c21e2a7cf00b94c5421725acff97f9826598b91f2340c5ddda730caca7d648 # m/2 => 03928301ffb8c0d7a364b794914c716ba3107cc78a6fe581028b0d8638b22e8573 node_int = bip32.deserialize( - "tpubDADHV9u9Y6gke2Vw3rWE8KRXmeK8PTtsF5B3Cqjo6h3SoiyRtzxjnDVG1knxrqB8BpP1dMAd6MR3Ps5UXibiFDtQuWVPXLkJ3HvttZYbH12" + "xpub69qexv5TppjJNEK5bfX8vQ6ASXDUQ5PohSajrHgeknHZ4SJipn7edmpRmiiBLLDtPur71mekZFazhgas8rkUMnS7quk5qp64TLLV8ShrxZJ" ) # m/1 => 03f91460d79e4e463d7d90cb75254bcd62b515a99a950574c721efdc5f711dff35 # m/2 => 038caebd6f753bbbd2bb1f3346a43cd32140648583673a31d62f2dfb56ad0ab9e3 @@ -174,13 +177,13 @@ class TestMultisigChange: @pytest.mark.setup_client(mnemonic=MNEMONIC12) def test_external_external(self, client): out1 = proto.TxOutputType( - address="muevUcG1Bb8eM2nGUGhqmeujHRX7YXjSEu", + address="1F8yBZB2NZhPZvJekhjTwjhQRRvQeTjjXr", amount=40000000, script_type=proto.OutputScriptType.PAYTOADDRESS, ) out2 = proto.TxOutputType( - address="mwdrpMVSJxxsM8f8xbnCHn9ERaRT1NG1UX", + address="1H7uXJQTVwXca2BXF2opTrvuZapk8Cm8zY", amount=44000000, script_type=proto.OutputScriptType.PAYTOADDRESS, ) @@ -189,7 +192,7 @@ class TestMultisigChange: client.set_expected_responses(self._responses(self.inp1, self.inp2)) _, serialized_tx = btc.sign_tx( client, - "Testnet", + "Bitcoin", [self.inp1, self.inp2], [out1, out2], prev_txes=TX_API, @@ -205,7 +208,7 @@ class TestMultisigChange: @pytest.mark.setup_client(mnemonic=MNEMONIC12) def test_external_internal(self, client): out1 = proto.TxOutputType( - address="muevUcG1Bb8eM2nGUGhqmeujHRX7YXjSEu", + address="1F8yBZB2NZhPZvJekhjTwjhQRRvQeTjjXr", amount=40000000, script_type=proto.OutputScriptType.PAYTOADDRESS, ) @@ -222,7 +225,7 @@ class TestMultisigChange: ) _, serialized_tx = btc.sign_tx( client, - "Testnet", + "Bitcoin", [self.inp1, self.inp2], [out1, out2], prev_txes=TX_API, @@ -244,7 +247,7 @@ class TestMultisigChange: ) out2 = proto.TxOutputType( - address="mwdrpMVSJxxsM8f8xbnCHn9ERaRT1NG1UX", + address="1H7uXJQTVwXca2BXF2opTrvuZapk8Cm8zY", amount=44000000, script_type=proto.OutputScriptType.PAYTOADDRESS, ) @@ -255,7 +258,7 @@ class TestMultisigChange: ) _, serialized_tx = btc.sign_tx( client, - "Testnet", + "Bitcoin", [self.inp1, self.inp2], [out1, out2], prev_txes=TX_API, @@ -271,13 +274,13 @@ class TestMultisigChange: @pytest.mark.setup_client(mnemonic=MNEMONIC12) def test_multisig_external_external(self, client): out1 = proto.TxOutputType( - address="2N2aFoogGntQFFwdUVPfRmutXD22ThcNTsR", + address="3B23k4kFBRtu49zvpG3Z9xuFzfpHvxBcwt", amount=40000000, script_type=proto.OutputScriptType.PAYTOADDRESS, ) out2 = proto.TxOutputType( - address="2NFJjQcU8mw4Z3ywpbek8HL1VoJ27GDrkHw", + address="3PkXLsY7AUZCrCKGvX8FfP2EawowUBMbcg", amount=44000000, script_type=proto.OutputScriptType.PAYTOADDRESS, ) @@ -286,7 +289,7 @@ class TestMultisigChange: client.set_expected_responses(self._responses(self.inp1, self.inp2)) _, serialized_tx = btc.sign_tx( client, - "Testnet", + "Bitcoin", [self.inp1, self.inp2], [out1, out2], prev_txes=TX_API, @@ -316,7 +319,7 @@ class TestMultisigChange: ) out2 = proto.TxOutputType( - address="2NFJjQcU8mw4Z3ywpbek8HL1VoJ27GDrkHw", + address="3PkXLsY7AUZCrCKGvX8FfP2EawowUBMbcg", amount=44000000, script_type=proto.OutputScriptType.PAYTOADDRESS, ) @@ -327,7 +330,7 @@ class TestMultisigChange: ) _, serialized_tx = btc.sign_tx( client, - "Testnet", + "Bitcoin", [self.inp1, self.inp2], [out1, out2], prev_txes=TX_API, @@ -350,7 +353,7 @@ class TestMultisigChange: ) out1 = proto.TxOutputType( - address="2N2aFoogGntQFFwdUVPfRmutXD22ThcNTsR", + address="3B23k4kFBRtu49zvpG3Z9xuFzfpHvxBcwt", amount=40000000, script_type=proto.OutputScriptType.PAYTOADDRESS, ) @@ -368,7 +371,7 @@ class TestMultisigChange: ) _, serialized_tx = btc.sign_tx( client, - "Testnet", + "Bitcoin", [self.inp1, self.inp2], [out1, out2], prev_txes=TX_API, @@ -391,7 +394,7 @@ class TestMultisigChange: ) out1 = proto.TxOutputType( - address="2N2aFoogGntQFFwdUVPfRmutXD22ThcNTsR", + address="3B23k4kFBRtu49zvpG3Z9xuFzfpHvxBcwt", amount=40000000, script_type=proto.OutputScriptType.PAYTOADDRESS, ) @@ -407,7 +410,7 @@ class TestMultisigChange: client.set_expected_responses(self._responses(self.inp1, self.inp2)) _, serialized_tx = btc.sign_tx( client, - "Testnet", + "Bitcoin", [self.inp1, self.inp2], [out1, out2], prev_txes=TX_API, @@ -437,7 +440,7 @@ class TestMultisigChange: ) out2 = proto.TxOutputType( - address="2NFJjQcU8mw4Z3ywpbek8HL1VoJ27GDrkHw", + address="3PkXLsY7AUZCrCKGvX8FfP2EawowUBMbcg", amount=65000000, script_type=proto.OutputScriptType.PAYTOADDRESS, ) @@ -446,7 +449,7 @@ class TestMultisigChange: client.set_expected_responses(self._responses(self.inp1, self.inp3)) _, serialized_tx = btc.sign_tx( client, - "Testnet", + "Bitcoin", [self.inp1, self.inp3], [out1, out2], prev_txes=TX_API, diff --git a/tests/ui_tests/fixtures.json b/tests/ui_tests/fixtures.json index 1c6f771da..172bc8285 100644 --- a/tests/ui_tests/fixtures.json +++ b/tests/ui_tests/fixtures.json @@ -271,6 +271,7 @@ "test_msg_getaddress_show.py::test_show_multisig_xpubs[InputScriptType.SPENDP2SHWITNESS-1-3P-82103d63": "afcf421cbb68a5808e8184674dceb09152a08cdc1b04f9f59807be2221e1842a", "test_msg_getaddress_show.py::test_show_multisig_xpubs[InputScriptType.SPENDWITNESS-2-bc1qqn-5a90fe2a": "5e0dc7790e1dea0839c8d5f44d1a9fb3463364866987cecda6eff01a2dd4fd1b", "test_msg_getaddress_show.py::test_show_multisig_xpubs[InputScriptType.SPENDWITNESS-2-bc1qqn-b8a7ece8": "c34ab5baa05e59ca95c5ece02b816b0cc82a67f79462612da0d014d0724dd2fc", +"test_msg_getaddress_show.py::test_show_unrecognized_path": "5a80508a71a9ef64f94762b07636f90e464832f0f4a3102af8fa1a8c69e94586", "test_msg_getecdhsessionkey.py-test_ecdh": "75fe462e6afa73742949ede4f3529d2e0ec08f8f1b67c04a57189c8657fcbdcd", "test_msg_getentropy.py::test_entropy[128]": "a722fa2048fa3102889ec05558d25f837a364ef2a118e85975683e10a56f1356", "test_msg_getentropy.py::test_entropy[129]": "a722fa2048fa3102889ec05558d25f837a364ef2a118e85975683e10a56f1356", @@ -640,14 +641,14 @@ "test_multisig.py-test_2_of_3": "2be92556edf4ff8eed340d535f379ee6915eae34fef25d669ce865848e7b4705", "test_multisig.py-test_attack_change_input": "89859fea184df09ce96df5281d405c0e85c87ba7efbafa4b3fdf7d9402c0fc44", "test_multisig.py-test_missing_pubkey": "a69ccaba89fdb284243c476a5fae3551e8aacf52a59e73f41a1e9f0a38ab93f0", -"test_multisig_change.py-test_external_external": "0f9bc8153070c4bad85bf7ae9d1d30843752538b4d1f8964bc748015a07f00bc", -"test_multisig_change.py-test_external_internal": "f60554e040d8965b8f3efdf9eea1f940b64b5844f12d1ee802abbec8d15ea9bf", -"test_multisig_change.py-test_internal_external": "f6b695c7bd338a6d465d81c7ace92f8021b01932a42e6b8e52ec4261f10bcf33", -"test_multisig_change.py-test_multisig_change_match_first": "05b894da12a881b5000eea56eef50b3c3be619fc43474ebcbed8e3129d64929b", -"test_multisig_change.py-test_multisig_change_match_second": "a095e5f0e53c76850f6fee94623cf724e2aa2a61442aa771a4e7ee81370111f5", -"test_multisig_change.py-test_multisig_external_external": "ddecdadd659b0d1360a6a255c6f9dbf2c5b813039877b76d4062ddab765e1912", -"test_multisig_change.py-test_multisig_mismatch_change": "7cb243b20be31a587dced4aaaf782a2d8487595369dde66aacb1b9a76e89c4fe", -"test_multisig_change.py-test_multisig_mismatch_inputs": "64741bd84c5394e719125c1fbe8c34ef866ac63ca24ee1299e4268c59a199466", +"test_multisig_change.py-test_external_external": "c0f0e105f7361b79718c0f5e208a4cafaa18ab1c18289da4e8bba0be4938aa00", +"test_multisig_change.py-test_external_internal": "b8a981257afddcd57dfa6510632c1ecc38935b7d466dadc3ef2c05a02a3c059a", +"test_multisig_change.py-test_internal_external": "05bcf4ed8f908c2228d82075d1b40df43ced865e9e09df400a3a955f636ac2d1", +"test_multisig_change.py-test_multisig_change_match_first": "8216c87b1956e6852421bcea3b7fff6457e0e7726d6eec5312d1126a23635cbb", +"test_multisig_change.py-test_multisig_change_match_second": "a4974ba1b357d8ecd146e8a6112c13829e2812b5425cc7b8f2d417a05f99de51", +"test_multisig_change.py-test_multisig_external_external": "a7a18c6ff1a3afaf501abb7459c7ded8dc591dd406034e13f5ca83ffe9d4bab7", +"test_multisig_change.py-test_multisig_mismatch_change": "f25b0a640bf10c502f3c6484cebe4df4ad05a8e679891d40f863b9d40fc6a8b5", +"test_multisig_change.py-test_multisig_mismatch_inputs": "dc8960fe817c15b5262aaa2f6a83b37db7e27e196e8abdf7f76d47e4f68a6fec", "test_nonstandard_paths.py::test_getaddress[m-1195487518-6-255-script_types3]": "4e1ff4e743e91769318fa032d10343c08ee016820df26c1b2a7c6e41b342b24c", "test_nonstandard_paths.py::test_getaddress[m-1195487518-script_types2]": "fc89fc6ed34c87e94e45a4a0aeab1904009da2f52c5a54ca427209ae7ec0c968", "test_nonstandard_paths.py::test_getaddress[m-3'-100'-4-255-script_types1]": "d16eecce052da865fe42416a7ab742f567cf7b508d1bb137de46116a513db52c", From 3f647f1b7ba46a224757e94109a837a6d2389a28 Mon Sep 17 00:00:00 2001 From: Andrew Kozlik Date: Thu, 24 Jun 2021 15:45:16 +0200 Subject: [PATCH 09/15] chore(common): Ensure that testnet coins use slip44 coin type 1. --- common/tools/coin_info.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/common/tools/coin_info.py b/common/tools/coin_info.py index 67d7ba8d9..66943068a 100755 --- a/common/tools/coin_info.py +++ b/common/tools/coin_info.py @@ -184,6 +184,9 @@ def validate_btc(coin): if not coin["max_address_length"] >= coin["min_address_length"]: errors.append("max address length must not be smaller than min address length") + if "testnet" in coin["coin_name"].lower() and coin["slip44"] != 1: + errors.append("testnet coins must use slip44 coin type 1") + if coin["segwit"]: if coin["bech32_prefix"] is None: errors.append("bech32_prefix must be defined for segwit-enabled coin") From b8cb53109852e508a340392fe9d3dadaf73e64a4 Mon Sep 17 00:00:00 2001 From: Andrew Kozlik Date: Fri, 25 Jun 2021 15:23:15 +0200 Subject: [PATCH 10/15] feat(legacy): Check presence of multisig parameters in coin_known_path_check() for consistency with core checks. --- legacy/firmware/crypto.c | 5 ++++- legacy/firmware/crypto.h | 2 +- legacy/firmware/fsm_msg_coin.h | 6 ++++-- legacy/firmware/signing.c | 2 +- 4 files changed, 10 insertions(+), 5 deletions(-) diff --git a/legacy/firmware/crypto.c b/legacy/firmware/crypto.c index dc079795d..d3b138135 100644 --- a/legacy/firmware/crypto.c +++ b/legacy/firmware/crypto.c @@ -539,7 +539,7 @@ static bool check_cointype(const CoinInfo *coin, uint32_t slip44, bool full) { bool coin_known_path_check(const CoinInfo *coin, InputScriptType script_type, uint32_t address_n_count, const uint32_t *address_n, - bool full) { + bool has_multisig, bool full) { // If full == true, 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. @@ -560,6 +560,7 @@ bool coin_known_path_check(const CoinInfo *coin, InputScriptType script_type, valid = valid && check_cointype(coin, address_n[1], full); if (full) { valid = valid && (script_type == InputScriptType_SPENDADDRESS); + valid = valid && (!has_multisig); valid = valid && ((address_n[2] & 0x80000000) == 0x80000000); valid = valid && ((address_n[2] & 0x7fffffff) <= PATH_MAX_ACCOUNT); valid = valid && (address_n[3] <= PATH_MAX_CHANGE); @@ -588,6 +589,7 @@ bool coin_known_path_check(const CoinInfo *coin, InputScriptType script_type, } if (full) { + valid = valid && has_multisig; if (address_n_count == 4) { valid = valid && (script_type == InputScriptType_SPENDMULTISIG); valid = valid && (address_n[1] <= 100); @@ -628,6 +630,7 @@ bool coin_known_path_check(const CoinInfo *coin, InputScriptType script_type, } valid = valid && check_cointype(coin, address_n[1], full); if (full) { + valid = valid && has_multisig; valid = valid && (script_type == InputScriptType_SPENDMULTISIG || script_type == InputScriptType_SPENDP2SHWITNESS || script_type == InputScriptType_SPENDWITNESS); diff --git a/legacy/firmware/crypto.h b/legacy/firmware/crypto.h index c889d8962..c47c707f5 100644 --- a/legacy/firmware/crypto.h +++ b/legacy/firmware/crypto.h @@ -84,6 +84,6 @@ int cryptoIdentityFingerprint(const IdentityType *identity, uint8_t *hash); bool coin_known_path_check(const CoinInfo *coin, InputScriptType script_type, uint32_t address_n_count, const uint32_t *address_n, - bool full); + bool has_multisig, bool full); #endif diff --git a/legacy/firmware/fsm_msg_coin.h b/legacy/firmware/fsm_msg_coin.h index 96687b8fa..6e0143142 100644 --- a/legacy/firmware/fsm_msg_coin.h +++ b/legacy/firmware/fsm_msg_coin.h @@ -184,8 +184,10 @@ void fsm_msgGetAddress(const GetAddress *msg) { } if (!coin_known_path_check(coin, msg->script_type, msg->address_n_count, - msg->address_n, true)) { - if (config_getSafetyCheckLevel() == SafetyCheckLevel_Strict) { + msg->address_n, msg->has_multisig, true)) { + if (config_getSafetyCheckLevel() == SafetyCheckLevel_Strict && + !coin_known_path_check(coin, msg->script_type, msg->address_n_count, + msg->address_n, msg->has_multisig, false)) { fsm_sendFailure(FailureType_Failure_DataError, _("Forbidden key path")); layoutHome(); return; diff --git a/legacy/firmware/signing.c b/legacy/firmware/signing.c index 008e8d4c9..5cc29758c 100644 --- a/legacy/firmware/signing.c +++ b/legacy/firmware/signing.c @@ -686,7 +686,7 @@ bool compile_input_script_sig(TxInputType *tinput) { } } if (!coin_known_path_check(coin, tinput->script_type, tinput->address_n_count, - tinput->address_n, false)) { + tinput->address_n, tinput->has_multisig, false)) { if (config_getSafetyCheckLevel() == SafetyCheckLevel_Strict) { return false; } From 5fb0f9a6c58b77ff9ed5c2c665658ad8fc3327ac Mon Sep 17 00:00:00 2001 From: Andrew Kozlik Date: Tue, 29 Jun 2021 13:38:59 +0200 Subject: [PATCH 11/15] feat(legacy): Distinguish between known path checks and script type checks in coin_path_check(). --- legacy/firmware/crypto.c | 124 ++++++++++++++++++++------------- legacy/firmware/crypto.h | 12 +++- legacy/firmware/fsm_msg_coin.h | 10 +-- legacy/firmware/signing.c | 5 +- 4 files changed, 94 insertions(+), 57 deletions(-) diff --git a/legacy/firmware/crypto.c b/legacy/firmware/crypto.c index d3b138135..7b3d49835 100644 --- a/legacy/firmware/crypto.c +++ b/legacy/firmware/crypto.c @@ -537,30 +537,37 @@ static bool check_cointype(const CoinInfo *coin, uint32_t slip44, bool 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 has_multisig, bool full) { - // If full == true, 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. - // If full == false, this function checks that a coin without strong replay +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. + + const bool check_known = (level >= CoinPathCheckLevel_KNOWN); + const bool check_script_type = (level >= CoinPathCheckLevel_SCRIPT_TYPE); bool valid = true; // m/44' : BIP44 Legacy // m / purpose' / coin_type' / account' / change / address_index if (address_n_count > 0 && address_n[0] == (0x80000000 + 44)) { - if (full) { + if (check_known) { valid = valid && (address_n_count == 5); } else { valid = valid && (address_n_count >= 2); } - valid = valid && check_cointype(coin, address_n[1], full); - if (full) { + valid = valid && check_cointype(coin, address_n[1], check_known); + if (check_script_type) { valid = valid && (script_type == InputScriptType_SPENDADDRESS); valid = valid && (!has_multisig); + } + if (check_known) { valid = valid && ((address_n[2] & 0x80000000) == 0x80000000); valid = valid && ((address_n[2] & 0x7fffffff) <= PATH_MAX_ACCOUNT); valid = valid && (address_n[3] <= PATH_MAX_CHANGE); @@ -570,51 +577,63 @@ bool coin_known_path_check(const CoinInfo *coin, InputScriptType script_type, } if (address_n_count > 0 && address_n[0] == (0x80000000 + 45)) { + if (check_script_type) { + valid = valid && has_multisig; + } + 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, full); + 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); + } } 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], full); + 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] & 0x80000000) == 0x80000000); + valid = valid && ((address_n[2] & 0x7fffffff) <= 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, 0x80000000 | address_n[1], full); - } - - if (full) { - valid = valid && has_multisig; - if (address_n_count == 4) { - valid = valid && (script_type == InputScriptType_SPENDMULTISIG); - 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) { - valid = valid && (script_type == InputScriptType_SPENDADDRESS || - script_type == InputScriptType_SPENDMULTISIG); - valid = valid && ((address_n[2] & 0x80000000) == 0x80000000); - valid = valid && ((address_n[2] & 0x7fffffff) <= PATH_MAX_ACCOUNT); - valid = valid && (address_n[3] <= 1000000); - valid = valid && (address_n[4] <= PATH_MAX_ADDRESS_INDEX); - } else if (address_n_count == 6) { + valid = + valid && check_cointype(coin, 0x80000000 | 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] & 0x80000000) == (address_n[2] & 0x80000000)); 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); - } else { + } + } else { + if (check_known) { return false; } } + return valid; } @@ -623,17 +642,19 @@ bool coin_known_path_check(const CoinInfo *coin, InputScriptType script_type, // Electrum: // m / purpose' / coin_type' / account' / type' / change / address_index if (address_n_count > 0 && address_n[0] == (0x80000000 + 48)) { - if (full) { + 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], full); - if (full) { + valid = valid && check_cointype(coin, address_n[1], check_known); + if (check_script_type) { valid = valid && has_multisig; valid = valid && (script_type == InputScriptType_SPENDMULTISIG || script_type == InputScriptType_SPENDP2SHWITNESS || script_type == InputScriptType_SPENDWITNESS); + } + if (check_known) { valid = valid && ((address_n[2] & 0x80000000) == 0x80000000); valid = valid && ((address_n[2] & 0x7fffffff) <= PATH_MAX_ACCOUNT); if (address_n_count == 5) { @@ -655,14 +676,16 @@ bool coin_known_path_check(const CoinInfo *coin, InputScriptType script_type, // m / purpose' / coin_type' / account' / change / address_index if (address_n_count > 0 && address_n[0] == (0x80000000 + 49)) { valid = valid && coin->has_segwit; - if (full) { + if (check_known) { valid = valid && (address_n_count == 5); } else { valid = valid && (address_n_count >= 2); } - valid = valid && check_cointype(coin, address_n[1], full); - if (full) { + valid = valid && check_cointype(coin, address_n[1], check_known); + if (check_script_type) { valid = valid && (script_type == InputScriptType_SPENDP2SHWITNESS); + } + if (check_known) { valid = valid && ((address_n[2] & 0x80000000) == 0x80000000); valid = valid && ((address_n[2] & 0x7fffffff) <= PATH_MAX_ACCOUNT); valid = valid && (address_n[3] <= PATH_MAX_CHANGE); @@ -676,14 +699,16 @@ bool coin_known_path_check(const CoinInfo *coin, InputScriptType script_type, if (address_n_count > 0 && address_n[0] == (0x80000000 + 84)) { valid = valid && coin->has_segwit; valid = valid && (coin->bech32_prefix != NULL); - if (full) { + if (check_known) { valid = valid && (address_n_count == 5); } else { valid = valid && (address_n_count >= 2); } - valid = valid && check_cointype(coin, address_n[1], full); - if (full) { + valid = valid && check_cointype(coin, address_n[1], check_known); + if (check_script_type) { valid = valid && (script_type == InputScriptType_SPENDWITNESS); + } + if (check_known) { valid = valid && ((address_n[2] & 0x80000000) == 0x80000000); valid = valid && ((address_n[2] & 0x7fffffff) <= PATH_MAX_ACCOUNT); valid = valid && (address_n[3] <= PATH_MAX_CHANGE); @@ -696,7 +721,7 @@ bool coin_known_path_check(const CoinInfo *coin, InputScriptType script_type, // m / [1,4] / address_index if (address_n_count > 0 && (address_n[0] == 1 || address_n[0] == 4)) { valid = valid && (coin->coin_type == SLIP44_BITCOIN); - if (full) { + if (check_known) { valid = valid && (address_n_count == 2); valid = valid && (address_n[1] <= PATH_MAX_ADDRESS_INDEX); } @@ -707,7 +732,7 @@ bool coin_known_path_check(const CoinInfo *coin, InputScriptType script_type, // m / 3' / [1-100]' / [1,4] / address_index if (address_n_count > 0 && address_n[0] == (0x80000000 + 3)) { valid = valid && (coin->coin_type == SLIP44_BITCOIN); - if (full) { + if (check_known) { valid = valid && (address_n_count == 4); valid = valid && ((address_n[1] & 0x80000000) == 0x80000000); valid = valid && ((address_n[1] & 0x7fffffff) <= 100); @@ -722,7 +747,7 @@ bool coin_known_path_check(const CoinInfo *coin, InputScriptType script_type, // m / 1195487518 / 6 / address_index if (address_n_count > 0 && address_n[0] == 1195487518) { valid = valid && (coin->coin_type == SLIP44_BITCOIN); - if (full) { + if (check_known) { if (address_n_count == 3) { valid = valid && (address_n[1] == 6); valid = valid && (address_n[2] <= PATH_MAX_ADDRESS_INDEX); @@ -736,14 +761,17 @@ bool coin_known_path_check(const CoinInfo *coin, InputScriptType script_type, // 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 (full) { + if (check_known) { valid = valid && (address_n_count == 5); } else { valid = valid && (address_n_count >= 2); } - valid = valid && check_cointype(coin, 0x80000000 | address_n[1], full); - if (full) { + valid = + valid && check_cointype(coin, 0x80000000 | address_n[1], check_known); + if (check_script_type) { valid = valid && (script_type == InputScriptType_SPENDP2SHWITNESS); + } + if (check_known) { valid = valid && ((address_n[1] & 0x80000000) == 0); valid = valid && (address_n[2] <= PATH_MAX_ACCOUNT); valid = valid && (address_n[3] <= PATH_MAX_CHANGE); @@ -752,6 +780,6 @@ bool coin_known_path_check(const CoinInfo *coin, InputScriptType script_type, return valid; } - // we allow unknown paths when a full check is not required - return !full; + // we allow unknown paths only when a full check is not required + return level == CoinPathCheckLevel_BASIC; } diff --git a/legacy/firmware/crypto.h b/legacy/firmware/crypto.h index c47c707f5..006db4bf2 100644 --- a/legacy/firmware/crypto.h +++ b/legacy/firmware/crypto.h @@ -32,6 +32,12 @@ #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 ser_length_size(len) ((len) < 253 ? 1 : (len) < 0x10000 ? 3 : 5) uint32_t ser_length(uint32_t len, uint8_t *out); @@ -82,8 +88,8 @@ int cryptoMultisigFingerprint(const MultisigRedeemScriptType *multisig, int cryptoIdentityFingerprint(const IdentityType *identity, uint8_t *hash); -bool coin_known_path_check(const CoinInfo *coin, InputScriptType script_type, - uint32_t address_n_count, const uint32_t *address_n, - bool has_multisig, 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); #endif diff --git a/legacy/firmware/fsm_msg_coin.h b/legacy/firmware/fsm_msg_coin.h index 6e0143142..e62bbd7eb 100644 --- a/legacy/firmware/fsm_msg_coin.h +++ b/legacy/firmware/fsm_msg_coin.h @@ -183,11 +183,13 @@ void fsm_msgGetAddress(const GetAddress *msg) { strlcpy(desc, _("Address:"), sizeof(desc)); } - if (!coin_known_path_check(coin, msg->script_type, msg->address_n_count, - msg->address_n, msg->has_multisig, true)) { + if (!coin_path_check(coin, msg->script_type, msg->address_n_count, + msg->address_n, msg->has_multisig, + CoinPathCheckLevel_SCRIPT_TYPE)) { if (config_getSafetyCheckLevel() == SafetyCheckLevel_Strict && - !coin_known_path_check(coin, msg->script_type, msg->address_n_count, - msg->address_n, msg->has_multisig, false)) { + !coin_path_check(coin, msg->script_type, msg->address_n_count, + msg->address_n, msg->has_multisig, + CoinPathCheckLevel_KNOWN)) { fsm_sendFailure(FailureType_Failure_DataError, _("Forbidden key path")); layoutHome(); return; diff --git a/legacy/firmware/signing.c b/legacy/firmware/signing.c index 5cc29758c..0e611825e 100644 --- a/legacy/firmware/signing.c +++ b/legacy/firmware/signing.c @@ -685,8 +685,9 @@ bool compile_input_script_sig(TxInputType *tinput) { return false; } } - if (!coin_known_path_check(coin, tinput->script_type, tinput->address_n_count, - tinput->address_n, tinput->has_multisig, false)) { + if (!coin_path_check(coin, tinput->script_type, tinput->address_n_count, + tinput->address_n, tinput->has_multisig, + CoinPathCheckLevel_BASIC)) { if (config_getSafetyCheckLevel() == SafetyCheckLevel_Strict) { return false; } From 01c1ae426fe911619f0312fa417ecdd7e102848c Mon Sep 17 00:00:00 2001 From: Martin Milata Date: Wed, 30 Jun 2021 18:32:58 +0200 Subject: [PATCH 12/15] docs: add security fixes to changelogs --- core/.changelog.d/noissue.security | 1 - core/.changelog.d/noissue.security.1 | 1 - core/.changelog.d/noissue.security.2 | 1 - core/CHANGELOG.md | 5 +++++ legacy/firmware/.changelog.d/1660.fixed | 1 - legacy/firmware/.changelog.d/noissue.security | 1 - legacy/firmware/.changelog.d/noissue.security.1 | 1 - legacy/firmware/.changelog.d/noissue.security.2 | 1 - legacy/firmware/.changelog.d/noissue.security.3 | 1 - legacy/firmware/.changelog.d/noissue.security.4 | 1 - legacy/firmware/CHANGELOG.md | 11 +++++++++++ 11 files changed, 16 insertions(+), 9 deletions(-) delete mode 100644 core/.changelog.d/noissue.security delete mode 100644 core/.changelog.d/noissue.security.1 delete mode 100644 core/.changelog.d/noissue.security.2 delete mode 100644 legacy/firmware/.changelog.d/1660.fixed delete mode 100644 legacy/firmware/.changelog.d/noissue.security delete mode 100644 legacy/firmware/.changelog.d/noissue.security.1 delete mode 100644 legacy/firmware/.changelog.d/noissue.security.2 delete mode 100644 legacy/firmware/.changelog.d/noissue.security.3 delete mode 100644 legacy/firmware/.changelog.d/noissue.security.4 diff --git a/core/.changelog.d/noissue.security b/core/.changelog.d/noissue.security deleted file mode 100644 index 471cda364..000000000 --- a/core/.changelog.d/noissue.security +++ /dev/null @@ -1 +0,0 @@ -Disable all testnet coins from accessing Bitcoin paths. diff --git a/core/.changelog.d/noissue.security.1 b/core/.changelog.d/noissue.security.1 deleted file mode 100644 index 87d744983..000000000 --- a/core/.changelog.d/noissue.security.1 +++ /dev/null @@ -1 +0,0 @@ -Restrict BIP-45 paths to Bitcoin and coins with strong replay protection. diff --git a/core/.changelog.d/noissue.security.2 b/core/.changelog.d/noissue.security.2 deleted file mode 100644 index 6c34e3650..000000000 --- a/core/.changelog.d/noissue.security.2 +++ /dev/null @@ -1 +0,0 @@ -Ensure that all testnet coins use SLIP-44 coin type 1. diff --git a/core/CHANGELOG.md b/core/CHANGELOG.md index 7b5816a3c..9a47d382a 100644 --- a/core/CHANGELOG.md +++ b/core/CHANGELOG.md @@ -26,6 +26,11 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). - Fix red screen on shutdown. [#1658] - Empty passphrase is properly cached in Cardano functions [#1659] +### Security +- Ensure that all testnet coins use SLIP-44 coin type 1. +- Disable all testnet coins from accessing Bitcoin paths. +- Restrict BIP-45 paths to Bitcoin and coins with strong replay protection. + ## 2.4.0 [9th June 2021] diff --git a/legacy/firmware/.changelog.d/1660.fixed b/legacy/firmware/.changelog.d/1660.fixed deleted file mode 100644 index badda7e78..000000000 --- a/legacy/firmware/.changelog.d/1660.fixed +++ /dev/null @@ -1 +0,0 @@ -Allow non-standard paths used by Unchained Capital, Green Address and Casa. diff --git a/legacy/firmware/.changelog.d/noissue.security b/legacy/firmware/.changelog.d/noissue.security deleted file mode 100644 index 9e933c364..000000000 --- a/legacy/firmware/.changelog.d/noissue.security +++ /dev/null @@ -1 +0,0 @@ -Don't show addresses that have an unrecognized path. diff --git a/legacy/firmware/.changelog.d/noissue.security.1 b/legacy/firmware/.changelog.d/noissue.security.1 deleted file mode 100644 index 471cda364..000000000 --- a/legacy/firmware/.changelog.d/noissue.security.1 +++ /dev/null @@ -1 +0,0 @@ -Disable all testnet coins from accessing Bitcoin paths. diff --git a/legacy/firmware/.changelog.d/noissue.security.2 b/legacy/firmware/.changelog.d/noissue.security.2 deleted file mode 100644 index 6c34e3650..000000000 --- a/legacy/firmware/.changelog.d/noissue.security.2 +++ /dev/null @@ -1 +0,0 @@ -Ensure that all testnet coins use SLIP-44 coin type 1. diff --git a/legacy/firmware/.changelog.d/noissue.security.3 b/legacy/firmware/.changelog.d/noissue.security.3 deleted file mode 100644 index 7f1adb70b..000000000 --- a/legacy/firmware/.changelog.d/noissue.security.3 +++ /dev/null @@ -1 +0,0 @@ -Restrict the BIP-32 path ranges of account, change and address_index fields. diff --git a/legacy/firmware/.changelog.d/noissue.security.4 b/legacy/firmware/.changelog.d/noissue.security.4 deleted file mode 100644 index 87d744983..000000000 --- a/legacy/firmware/.changelog.d/noissue.security.4 +++ /dev/null @@ -1 +0,0 @@ -Restrict BIP-45 paths to Bitcoin and coins with strong replay protection. diff --git a/legacy/firmware/CHANGELOG.md b/legacy/firmware/CHANGELOG.md index d27d79375..cf8eb65d1 100644 --- a/legacy/firmware/CHANGELOG.md +++ b/legacy/firmware/CHANGELOG.md @@ -10,6 +10,16 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). - Removed support for Firo [#1647] - Removed support for Hatch [#1650] +### Fixed +- Allow non-standard paths used by Unchained Capital, Green Address and Casa. [#1660] + +### Security +- Ensure that all testnet coins use SLIP-44 coin type 1. +- Restrict BIP-45 paths to Bitcoin and coins with strong replay protection. +- Don't show addresses that have an unrecognized path. +- Disable all testnet coins from accessing Bitcoin paths. +- Restrict the BIP-32 path ranges of `account`, `change` and `address_index` fields. + ## 1.10.1 [9th June 2021] @@ -400,3 +410,4 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). [#1627]: https://github.com/trezor/trezor-firmware/issues/1627 [#1647]: https://github.com/trezor/trezor-firmware/issues/1647 [#1650]: https://github.com/trezor/trezor-firmware/issues/1650 +[#1660]: https://github.com/trezor/trezor-firmware/issues/1660 From 8e6a647e8905d65082dfb849e35f9b2c70f5e45e Mon Sep 17 00:00:00 2001 From: matejcik Date: Fri, 9 Jul 2021 14:35:01 +0200 Subject: [PATCH 13/15] fix(core): do not send ButtonRequest on every paging event partial revert of 54db2291f2335585fc14c0c5317353527fb2f452 from #1671 (cherry picked from commit 1e1963f1ee3a0e435a3e8f9568ff4a780b6ca53c) --- common/protob/messages-common.proto | 5 ----- core/CHANGELOG.md | 2 +- core/src/trezor/messages.py | 2 -- core/src/trezor/ui/components/tt/scroll.py | 5 +---- python/src/trezorlib/messages.py | 3 --- 5 files changed, 2 insertions(+), 15 deletions(-) diff --git a/common/protob/messages-common.proto b/common/protob/messages-common.proto index 8e3ed56a1..dbdbc0171 100644 --- a/common/protob/messages-common.proto +++ b/common/protob/messages-common.proto @@ -47,11 +47,6 @@ message Failure { message ButtonRequest { optional ButtonRequestType code = 1; // enum identifier of the screen optional uint32 pages = 2; // if the screen is paginated, number of pages - optional uint32 page_number = 3; // if the screen is paginated, current page (1-based) - /* Rationale: both fields are optional, and neither field can have 0 as a valid - value. So both testing `if pages` and `if page_number` do the right thing. - Also the following is always true: `page_is_last = (page_number == pages)` */ - /** * Type of button request */ diff --git a/core/CHANGELOG.md b/core/CHANGELOG.md index 9a47d382a..7ccdaece0 100644 --- a/core/CHANGELOG.md +++ b/core/CHANGELOG.md @@ -7,7 +7,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). ## 2.4.1 [14th July 2021] ### Added -- ButtonRequest is sent also after every screen of a multi-page view. [#1671] +- ButtonRequest for multi-page views contains number of pages. [#1671] ### Changed - Converted altcoin apps to common layout code. [#1538] diff --git a/core/src/trezor/messages.py b/core/src/trezor/messages.py index d4834a4c9..6b03796a6 100644 --- a/core/src/trezor/messages.py +++ b/core/src/trezor/messages.py @@ -283,14 +283,12 @@ if TYPE_CHECKING: class ButtonRequest(protobuf.MessageType): code: ButtonRequestType | None pages: int | None - page_number: int | None def __init__( self, *, code: ButtonRequestType | None = None, pages: int | None = None, - page_number: int | None = None, ) -> None: pass diff --git a/core/src/trezor/ui/components/tt/scroll.py b/core/src/trezor/ui/components/tt/scroll.py index 2d0c12dbe..a262a8820 100644 --- a/core/src/trezor/ui/components/tt/scroll.py +++ b/core/src/trezor/ui/components/tt/scroll.py @@ -111,12 +111,9 @@ class Paginated(ui.Layout): code: ButtonRequestType = ButtonRequestType.Other, ) -> Any: workflow.close_others() + await ctx.call(ButtonRequest(code=code, pages=len(self.pages)), ButtonAck) result = WAS_PAGED while result is WAS_PAGED: - br = ButtonRequest( - code=code, pages=len(self.pages), page_number=self.page + 1 - ) - await ctx.call(br, ButtonAck) result = await self return result diff --git a/python/src/trezorlib/messages.py b/python/src/trezorlib/messages.py index 89196d157..106a1f35a 100644 --- a/python/src/trezorlib/messages.py +++ b/python/src/trezorlib/messages.py @@ -701,7 +701,6 @@ class ButtonRequest(protobuf.MessageType): FIELDS = { 1: protobuf.Field("code", ButtonRequestType, repeated=False, required=False), 2: protobuf.Field("pages", "uint32", repeated=False, required=False), - 3: protobuf.Field("page_number", "uint32", repeated=False, required=False), } def __init__( @@ -709,11 +708,9 @@ class ButtonRequest(protobuf.MessageType): *, code: Optional[ButtonRequestType] = None, pages: Optional[int] = None, - page_number: Optional[int] = None, ) -> None: self.code = code self.pages = pages - self.page_number = page_number class ButtonAck(protobuf.MessageType): From 46e05307645e4a446e7c64708f38042aea6e845d Mon Sep 17 00:00:00 2001 From: matejcik Date: Fri, 9 Jul 2021 14:48:40 +0200 Subject: [PATCH 14/15] fix(tests): auto-swipe by using only ButtonRequest.pages (cherry picked from commit f8bb90366addcce3218d65e723b4185aee3b0df8) --- core/src/apps/debug/__init__.py | 2 +- python/src/trezorlib/debuglink.py | 13 ++++------ tests/common.py | 26 ++++++------------- tests/device_tests/cardano/test_sign_tx.py | 9 ++----- tests/device_tests/test_msg_backup_device.py | 15 +++-------- .../device_tests/test_msg_getaddress_show.py | 6 ++--- .../test_msg_resetdevice_bip39_t2.py | 23 ++++------------ .../test_msg_resetdevice_slip39_advanced.py | 11 ++------ .../test_msg_resetdevice_slip39_basic.py | 7 +---- tests/device_tests/test_msg_signmessage.py | 15 ++++------- .../test_msg_signtx_replacement.py | 3 ++- tests/device_tests/test_reset_backup.py | 21 +++------------ .../device_tests/test_reset_recovery_bip39.py | 13 ++-------- .../test_reset_recovery_slip39_advanced.py | 7 +---- .../test_reset_recovery_slip39_basic.py | 13 ++-------- 15 files changed, 46 insertions(+), 138 deletions(-) diff --git a/core/src/apps/debug/__init__.py b/core/src/apps/debug/__init__.py index 3c3d67c0d..8d3609684 100644 --- a/core/src/apps/debug/__init__.py +++ b/core/src/apps/debug/__init__.py @@ -56,7 +56,7 @@ if __debug__: def notify_layout_change(layout: Layout) -> None: storage.current_content[:] = layout.read_content() - if storage.watch_layout_changes: + if storage.watch_layout_changes or layout_change_chan.takers: layout_change_chan.publish(storage.current_content) async def dispatch_debuglink_decision(msg: DebugLinkDecision) -> None: diff --git a/python/src/trezorlib/debuglink.py b/python/src/trezorlib/debuglink.py index dd7f5ff18..052fa3f2c 100644 --- a/python/src/trezorlib/debuglink.py +++ b/python/src/trezorlib/debuglink.py @@ -158,8 +158,8 @@ class DebugLink: def press_no(self): self.input(button=False) - def swipe_up(self): - self.input(swipe=messages.DebugSwipeDirection.UP) + def swipe_up(self, wait=False): + self.input(swipe=messages.DebugSwipeDirection.UP, wait=wait) def swipe_down(self): self.input(swipe=messages.DebugSwipeDirection.DOWN) @@ -234,13 +234,10 @@ class DebugUI: if self.input_flow is None: if br.code == messages.ButtonRequestType.PinEntry: self.debuglink.input(self.get_pin()) - elif ( - br.pages is not None - and br.page_number is not None - and br.pages > br.page_number - ): - self.debuglink.swipe_up() else: + if br.pages is not None: + for _ in range(br.pages - 1): + self.debuglink.swipe_up(wait=True) self.debuglink.press_yes() elif self.input_flow is self.INPUT_FLOW_DONE: raise AssertionError("input flow ended prematurely") diff --git a/tests/common.py b/tests/common.py index 368fc8afc..faece150e 100644 --- a/tests/common.py +++ b/tests/common.py @@ -20,7 +20,7 @@ from pathlib import Path import pytest from trezorlib import btc, tools -from trezorlib.messages import ButtonRequest, ButtonRequestType as B +from trezorlib.messages import ButtonRequestType as B # fmt: off # 1 2 3 4 5 6 7 8 9 10 11 12 @@ -188,15 +188,14 @@ def read_and_confirm_mnemonic(debug, choose_wrong=False): mnemonic = yield from read_and_confirm_mnemonic(client.debug) """ mnemonic = [] - while True: - br = yield + br = yield + for _ in range(br.pages - 1): mnemonic.extend(debug.read_reset_word().split()) - if br.page_number < br.pages: - debug.swipe_up() - else: - # last page is confirmation - debug.press_yes() - break + debug.swipe_up(wait=True) + + # last page is confirmation + mnemonic.extend(debug.read_reset_word().split()) + debug.press_yes() # check share for _ in range(3): @@ -210,15 +209,6 @@ def read_and_confirm_mnemonic(debug, choose_wrong=False): return " ".join(mnemonic) -def paging_responses(pages, code=None): - """Generate a sequence of ButtonRequests for paging through a specified number - of screens. - """ - return [ - ButtonRequest(code=code, page_number=i + 1, pages=pages) for i in range(pages) - ] - - def get_test_address(client): """Fetch a testnet address on a fixed path. Useful to make a pin/passphrase protected call, or to identify the root secret (seed+passphrase)""" diff --git a/tests/device_tests/cardano/test_sign_tx.py b/tests/device_tests/cardano/test_sign_tx.py index ba78d90d3..6d7fbd48b 100644 --- a/tests/device_tests/cardano/test_sign_tx.py +++ b/tests/device_tests/cardano/test_sign_tx.py @@ -102,13 +102,8 @@ def test_cardano_sign_tx_with_multiple_chunks(client, parameters, result): expected_responses = [ messages.PassphraseRequest(), - # XXX as many ButtonRequests as paginations. We are relying on the fact that - # there is only one fixture whose pagination is known. - # If that changes, we'll need to figure out something else. - messages.ButtonRequest(page_number=1), - messages.ButtonRequest(page_number=2), - messages.ButtonRequest(page_number=1), - messages.ButtonRequest(page_number=2), + messages.ButtonRequest(), + messages.ButtonRequest(), ] expected_responses += [ messages.CardanoSignedTxChunk(signed_tx_chunk=bytes.fromhex(signed_tx_chunk)) diff --git a/tests/device_tests/test_msg_backup_device.py b/tests/device_tests/test_msg_backup_device.py index 4aeb4273f..b44f18608 100644 --- a/tests/device_tests/test_msg_backup_device.py +++ b/tests/device_tests/test_msg_backup_device.py @@ -27,7 +27,6 @@ from ..common import ( MNEMONIC_SLIP39_ADVANCED_20, MNEMONIC_SLIP39_BASIC_20_3of6, click_through, - paging_responses, read_and_confirm_mnemonic, ) @@ -37,8 +36,6 @@ from ..common import ( def test_backup_bip39(client): assert client.features.needs_backup is True mnemonic = None - words = 12 - mnemonic_pages = ((words + 3) // 4) + 1 def input_flow(): nonlocal mnemonic @@ -56,9 +53,7 @@ def test_backup_bip39(client): client.set_expected_responses( [ messages.ButtonRequest(code=B.ResetDevice), - ] - + paging_responses(mnemonic_pages, code=B.ResetDevice) - + [ + messages.ButtonRequest(code=B.ResetDevice), messages.ButtonRequest(code=B.Success), messages.ButtonRequest(code=B.Success), messages.Success, @@ -81,8 +76,6 @@ def test_backup_bip39(client): def test_backup_slip39_basic(client): assert client.features.needs_backup is True mnemonics = [] - words = 20 - mnemonic_pages = ((words + 3) // 4) + 1 def input_flow(): # 1. Checklist @@ -110,7 +103,7 @@ def test_backup_slip39_basic(client): client.set_expected_responses( [messages.ButtonRequest(code=B.ResetDevice)] * 6 # intro screens + [ - *paging_responses(mnemonic_pages, code=B.ResetDevice), + messages.ButtonRequest(code=B.ResetDevice), messages.ButtonRequest(code=B.Success), ] * 5 # individual shares @@ -139,8 +132,6 @@ def test_backup_slip39_basic(client): def test_backup_slip39_advanced(client): assert client.features.needs_backup is True mnemonics = [] - words = 20 - mnemonic_pages = ((words + 3) // 4) + 1 def input_flow(): # 1. Checklist @@ -177,7 +168,7 @@ def test_backup_slip39_advanced(client): ] * 5 # group thresholds + [ - *paging_responses(mnemonic_pages, code=B.ResetDevice), + messages.ButtonRequest(code=B.ResetDevice), messages.ButtonRequest(code=B.Success), ] * 25 # individual shares diff --git a/tests/device_tests/test_msg_getaddress_show.py b/tests/device_tests/test_msg_getaddress_show.py index 7d263bfe5..447130189 100644 --- a/tests/device_tests/test_msg_getaddress_show.py +++ b/tests/device_tests/test_msg_getaddress_show.py @@ -202,7 +202,7 @@ def test_show_multisig_xpubs( lines1 = client.debug.wait_layout().lines assert lines1[0] == "XPUB #1 " + ("(yours)" if i == 0 else "(cosigner)") client.debug.swipe_up() - yield + lines2 = client.debug.wait_layout().lines assert lines2[0] == "XPUB #1 " + ("(yours)" if i == 0 else "(cosigner)") assert "".join(lines1[1:] + lines2[1:]) == xpubs[0] @@ -212,7 +212,7 @@ def test_show_multisig_xpubs( lines1 = client.debug.wait_layout().lines assert lines1[0] == "XPUB #2 " + ("(yours)" if i == 1 else "(cosigner)") client.debug.swipe_up() - yield + lines2 = client.debug.wait_layout().lines assert lines2[0] == "XPUB #2 " + ("(yours)" if i == 1 else "(cosigner)") assert "".join(lines1[1:] + lines2[1:]) == xpubs[1] @@ -222,7 +222,7 @@ def test_show_multisig_xpubs( lines1 = client.debug.wait_layout().lines assert lines1[0] == "XPUB #3 " + ("(yours)" if i == 2 else "(cosigner)") client.debug.swipe_up() - yield + lines2 = client.debug.wait_layout().lines assert lines2[0] == "XPUB #3 " + ("(yours)" if i == 2 else "(cosigner)") assert "".join(lines1[1:] + lines2[1:]) == xpubs[2] diff --git a/tests/device_tests/test_msg_resetdevice_bip39_t2.py b/tests/device_tests/test_msg_resetdevice_bip39_t2.py index 1f033140e..727926bad 100644 --- a/tests/device_tests/test_msg_resetdevice_bip39_t2.py +++ b/tests/device_tests/test_msg_resetdevice_bip39_t2.py @@ -27,7 +27,6 @@ from ..common import ( MNEMONIC12, click_through, generate_entropy, - paging_responses, read_and_confirm_mnemonic, ) @@ -35,8 +34,6 @@ EXTERNAL_ENTROPY = b"zlutoucky kun upel divoke ody" * 2 def reset_device(client, strength): - words = strength // 32 * 3 - mnemonic_pages = ((words + 3) // 4) + 1 mnemonic = None def input_flow(): @@ -67,9 +64,7 @@ def reset_device(client, strength): proto.EntropyRequest(), proto.ButtonRequest(code=B.ResetDevice), proto.ButtonRequest(code=B.ResetDevice), - ] - + paging_responses(mnemonic_pages, code=B.ResetDevice) - + [ + proto.ButtonRequest(code=B.ResetDevice), proto.ButtonRequest(code=B.Success), proto.ButtonRequest(code=B.Success), proto.Success, @@ -124,8 +119,6 @@ class TestMsgResetDeviceT2: def test_reset_device_pin(self, client): mnemonic = None strength = 256 # 24 words - words = strength // 32 * 3 - mnemonic_pages = (words // 4) + 1 def input_flow(): nonlocal mnemonic @@ -182,9 +175,7 @@ class TestMsgResetDeviceT2: proto.EntropyRequest(), proto.ButtonRequest(code=B.ResetDevice), proto.ButtonRequest(code=B.ResetDevice), - ] - + paging_responses(mnemonic_pages, code=B.ResetDevice) - + [ + proto.ButtonRequest(code=B.ResetDevice), proto.ButtonRequest(code=B.Success), proto.ButtonRequest(code=B.Success), proto.Success, @@ -223,8 +214,6 @@ class TestMsgResetDeviceT2: def test_reset_failed_check(self, client): mnemonic = None strength = 256 # 24 words - words = strength // 32 * 3 - mnemonic_pages = (words // 4) + 1 def input_flow(): nonlocal mnemonic @@ -264,11 +253,9 @@ class TestMsgResetDeviceT2: proto.EntropyRequest(), proto.ButtonRequest(code=B.ResetDevice), proto.ButtonRequest(code=B.ResetDevice), - ] - + paging_responses(mnemonic_pages, code=B.ResetDevice) - + [proto.ButtonRequest(code=B.ResetDevice)] - + paging_responses(mnemonic_pages, code=B.ResetDevice) - + [ + proto.ButtonRequest(code=B.ResetDevice), + proto.ButtonRequest(code=B.ResetDevice), + proto.ButtonRequest(code=B.ResetDevice), proto.ButtonRequest(code=B.Success), proto.ButtonRequest(code=B.Success), proto.Success, diff --git a/tests/device_tests/test_msg_resetdevice_slip39_advanced.py b/tests/device_tests/test_msg_resetdevice_slip39_advanced.py index 240159038..7d4836e65 100644 --- a/tests/device_tests/test_msg_resetdevice_slip39_advanced.py +++ b/tests/device_tests/test_msg_resetdevice_slip39_advanced.py @@ -23,12 +23,7 @@ from trezorlib import device, messages as proto from trezorlib.exceptions import TrezorFailure from trezorlib.messages import BackupType, ButtonRequestType as B -from ..common import ( - click_through, - generate_entropy, - paging_responses, - read_and_confirm_mnemonic, -) +from ..common import click_through, generate_entropy, read_and_confirm_mnemonic EXTERNAL_ENTROPY = b"zlutoucky kun upel divoke ody" * 2 @@ -39,8 +34,6 @@ class TestMsgResetDeviceT2: @pytest.mark.setup_client(uninitialized=True) def test_reset_device_slip39_advanced(self, client): strength = 128 - word_count = 20 - mnemonic_page_count = (word_count // 4) + 1 member_threshold = 3 all_mnemonics = [] @@ -101,7 +94,7 @@ class TestMsgResetDeviceT2: ] + [ # individual mnemonic - *paging_responses(mnemonic_page_count, code=B.ResetDevice), + proto.ButtonRequest(code=B.ResetDevice), proto.ButtonRequest(code=B.Success), ] * (5 * 5) # groups * shares diff --git a/tests/device_tests/test_msg_resetdevice_slip39_basic.py b/tests/device_tests/test_msg_resetdevice_slip39_basic.py index 00d9760e1..501ec57ee 100644 --- a/tests/device_tests/test_msg_resetdevice_slip39_basic.py +++ b/tests/device_tests/test_msg_resetdevice_slip39_basic.py @@ -28,16 +28,11 @@ from ..common import ( EXTERNAL_ENTROPY, click_through, generate_entropy, - paging_responses, read_and_confirm_mnemonic, ) def reset_device(client, strength): - # per SLIP-39: strength in bits, rounded up to nearest multiple of 10, plus 70 bits - # of metadata, split into 10-bit words - word_count = ((strength + 9) // 10) + 7 - mnemonic_pages = ((word_count + 3) // 4) + 1 member_threshold = 3 all_mnemonics = [] @@ -84,7 +79,7 @@ def reset_device(client, strength): ] + [ # individual mnemonic - *paging_responses(mnemonic_pages, code=B.ResetDevice), + proto.ButtonRequest(code=B.ResetDevice), proto.ButtonRequest(code=B.Success), ] * 5 # number of shares diff --git a/tests/device_tests/test_msg_signmessage.py b/tests/device_tests/test_msg_signmessage.py index 5483cd77c..87d818950 100644 --- a/tests/device_tests/test_msg_signmessage.py +++ b/tests/device_tests/test_msg_signmessage.py @@ -242,12 +242,8 @@ def test_signmessage_pagination(client, message): # start assuming there was a word break; this avoids prepending space at start word_break = True - page = 0 - while True: - br = yield - assert br.page_number == page + 1 - page = br.page_number - + br = yield + for i in range(br.pages): layout = client.debug.wait_layout() for line in layout.lines[1:]: if line == "-": @@ -261,11 +257,10 @@ def test_signmessage_pagination(client, message): # attach with space message_read += " " + line - if page < br.pages: + if i < br.pages - 1: client.debug.swipe_up() - else: - client.debug.press_yes() - break + + client.debug.press_yes() with client: client.set_input_flow(input_flow) diff --git a/tests/device_tests/test_msg_signtx_replacement.py b/tests/device_tests/test_msg_signtx_replacement.py index d3a59de4f..29e9bcd6b 100644 --- a/tests/device_tests/test_msg_signtx_replacement.py +++ b/tests/device_tests/test_msg_signtx_replacement.py @@ -505,6 +505,7 @@ def test_p2wpkh_in_p2sh_fee_bump_from_external(client): orig_index=0, ) + t1 = client.features.model == "1" with client: client.set_expected_responses( [ @@ -517,7 +518,7 @@ def test_p2wpkh_in_p2sh_fee_bump_from_external(client): request_output(0), request_orig_output(0, TXHASH_334cd7), messages.ButtonRequest(code=B.ConfirmOutput), - messages.ButtonRequest(code=B.ConfirmOutput), + (t1, messages.ButtonRequest(code=B.ConfirmOutput)), request_orig_output(1, TXHASH_334cd7), messages.ButtonRequest(code=B.SignTx), request_input(0), diff --git a/tests/device_tests/test_reset_backup.py b/tests/device_tests/test_reset_backup.py index 4e6f08224..6b6b343f7 100644 --- a/tests/device_tests/test_reset_backup.py +++ b/tests/device_tests/test_reset_backup.py @@ -23,18 +23,11 @@ from shamir_mnemonic import shamir from trezorlib import device, messages from trezorlib.messages import BackupType, ButtonRequestType as B -from ..common import ( - EXTERNAL_ENTROPY, - click_through, - paging_responses, - read_and_confirm_mnemonic, -) +from ..common import EXTERNAL_ENTROPY, click_through, read_and_confirm_mnemonic def backup_flow_bip39(client): mnemonic = None - words = 12 - mnemonic_pages = ((words + 3) // 4) + 1 def input_flow(): nonlocal mnemonic @@ -59,9 +52,7 @@ def backup_flow_bip39(client): client.set_expected_responses( [ messages.ButtonRequest(code=B.ResetDevice), - ] - + paging_responses(mnemonic_pages, code=B.ResetDevice) - + [ + messages.ButtonRequest(code=B.ResetDevice), messages.ButtonRequest(code=B.Success), messages.ButtonRequest(code=B.Success), messages.Success, @@ -76,8 +67,6 @@ def backup_flow_bip39(client): def backup_flow_slip39_basic(client): mnemonics = [] - words = 20 - mnemonic_pages = ((words + 3) // 4) + 1 def input_flow(): # 1. Checklist @@ -105,7 +94,7 @@ def backup_flow_slip39_basic(client): client.set_expected_responses( [messages.ButtonRequest(code=B.ResetDevice)] * 6 # intro screens + [ - *paging_responses(mnemonic_pages, code=B.ResetDevice), + messages.ButtonRequest(code=B.ResetDevice), messages.ButtonRequest(code=B.Success), ] * 5 # individual shares @@ -124,8 +113,6 @@ def backup_flow_slip39_basic(client): def backup_flow_slip39_advanced(client): mnemonics = [] - words = 20 - mnemonic_pages = ((words + 3) // 4) + 1 def input_flow(): # 1. Confirm Reset @@ -166,7 +153,7 @@ def backup_flow_slip39_advanced(client): ] * 5 # group thresholds + [ - *paging_responses(mnemonic_pages, code=B.ResetDevice), + messages.ButtonRequest(code=B.ResetDevice), messages.ButtonRequest(code=B.Success), ] * 25 # individual shares diff --git a/tests/device_tests/test_reset_recovery_bip39.py b/tests/device_tests/test_reset_recovery_bip39.py index 2eda9ccb0..7be9dd7e0 100644 --- a/tests/device_tests/test_reset_recovery_bip39.py +++ b/tests/device_tests/test_reset_recovery_bip39.py @@ -23,12 +23,7 @@ from trezorlib import btc, device, messages from trezorlib.messages import BackupType, ButtonRequestType as B from trezorlib.tools import parse_path -from ..common import ( - EXTERNAL_ENTROPY, - click_through, - paging_responses, - read_and_confirm_mnemonic, -) +from ..common import EXTERNAL_ENTROPY, click_through, read_and_confirm_mnemonic @pytest.mark.skip_t1 @@ -44,8 +39,6 @@ def test_reset_recovery(client): def reset(client, strength=128, skip_backup=False): - words = strength // 32 * 3 - mnemonic_pages = ((words + 3) // 4) + 1 mnemonic = None def input_flow(): @@ -77,9 +70,7 @@ def reset(client, strength=128, skip_backup=False): messages.EntropyRequest(), messages.ButtonRequest(code=B.ResetDevice), messages.ButtonRequest(code=B.ResetDevice), - ] - + paging_responses(mnemonic_pages, code=B.ResetDevice) - + [ + messages.ButtonRequest(code=B.ResetDevice), messages.ButtonRequest(code=B.Success), messages.ButtonRequest(code=B.Success), messages.Success, diff --git a/tests/device_tests/test_reset_recovery_slip39_advanced.py b/tests/device_tests/test_reset_recovery_slip39_advanced.py index 02133de6e..b47c04e99 100644 --- a/tests/device_tests/test_reset_recovery_slip39_advanced.py +++ b/tests/device_tests/test_reset_recovery_slip39_advanced.py @@ -25,7 +25,6 @@ from trezorlib.tools import parse_path from ..common import ( EXTERNAL_ENTROPY, click_through, - paging_responses, read_and_confirm_mnemonic, recovery_enter_shares, ) @@ -60,10 +59,6 @@ def test_reset_recovery(client): def reset(client, strength=128): all_mnemonics = [] - # per SLIP-39: strength in bits, rounded up to nearest multiple of 10, plus 70 bits - # of metadata, split into 10-bit words - word_count = ((strength + 9) // 10) + 7 - mnemonic_pages = ((word_count + 3) // 4) + 1 def input_flow(): # 1. Confirm Reset @@ -122,7 +117,7 @@ def reset(client, strength=128): ] + [ # individual mnemonic - *paging_responses(mnemonic_pages, code=B.ResetDevice), + messages.ButtonRequest(code=B.ResetDevice), messages.ButtonRequest(code=B.Success), ] * (5 * 5) # groups * shares diff --git a/tests/device_tests/test_reset_recovery_slip39_basic.py b/tests/device_tests/test_reset_recovery_slip39_basic.py index 37b69b614..e6045ccfc 100644 --- a/tests/device_tests/test_reset_recovery_slip39_basic.py +++ b/tests/device_tests/test_reset_recovery_slip39_basic.py @@ -23,12 +23,7 @@ from trezorlib import btc, device, messages from trezorlib.messages import BackupType, ButtonRequestType as B from trezorlib.tools import parse_path -from ..common import ( - click_through, - paging_responses, - read_and_confirm_mnemonic, - recovery_enter_shares, -) +from ..common import click_through, read_and_confirm_mnemonic, recovery_enter_shares EXTERNAL_ENTROPY = b"zlutoucky kun upel divoke ody" * 2 MOCK_OS_URANDOM = mock.Mock(return_value=EXTERNAL_ENTROPY) @@ -51,10 +46,6 @@ def test_reset_recovery(client): def reset(client, strength=128): all_mnemonics = [] - # per SLIP-39: strength in bits, rounded up to nearest multiple of 10, plus 70 bits - # of metadata, split into 10-bit words - word_count = ((strength + 9) // 10) + 7 - mnemonic_pages = ((word_count + 3) // 4) + 1 def input_flow(): # 1. Confirm Reset @@ -98,7 +89,7 @@ def reset(client, strength=128): ] + [ # individual mnemonic - *paging_responses(mnemonic_pages, code=B.ResetDevice), + messages.ButtonRequest(code=B.ResetDevice), messages.ButtonRequest(code=B.Success), ] * 5 # number of shares From 24bb4016388fca4b998285b95dcd408f4ed0bff6 Mon Sep 17 00:00:00 2001 From: Andrew Kozlik Date: Tue, 13 Jul 2021 11:21:27 +0200 Subject: [PATCH 15/15] fix(legacy,core): Fix operation source account encoding in Stellar. --- core/CHANGELOG.md | 1 + core/src/apps/stellar/operations/serialize.py | 5 +++-- legacy/firmware/CHANGELOG.md | 1 + legacy/firmware/stellar.c | 2 +- tests/device_tests/test_msg_stellar_sign_transaction.py | 4 ++-- tests/ui_tests/fixtures.json | 2 +- 6 files changed, 9 insertions(+), 6 deletions(-) diff --git a/core/CHANGELOG.md b/core/CHANGELOG.md index 7ccdaece0..a44ccb8f7 100644 --- a/core/CHANGELOG.md +++ b/core/CHANGELOG.md @@ -30,6 +30,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). - Ensure that all testnet coins use SLIP-44 coin type 1. - Disable all testnet coins from accessing Bitcoin paths. - Restrict BIP-45 paths to Bitcoin and coins with strong replay protection. +- Fix operation source account encoding in Stellar. ## 2.4.0 [9th June 2021] diff --git a/core/src/apps/stellar/operations/serialize.py b/core/src/apps/stellar/operations/serialize.py index fbd7f74e1..f8ccdff2f 100644 --- a/core/src/apps/stellar/operations/serialize.py +++ b/core/src/apps/stellar/operations/serialize.py @@ -138,8 +138,9 @@ def _write_set_options_int(w, value: int): def write_account(w, source_account: str): if source_account is None: writers.write_bool(w, False) - return - writers.write_pubkey(w, source_account) + else: + writers.write_bool(w, True) + writers.write_pubkey(w, source_account) def _write_asset_code(w, asset_type: int, asset_code: str): diff --git a/legacy/firmware/CHANGELOG.md b/legacy/firmware/CHANGELOG.md index cf8eb65d1..871036576 100644 --- a/legacy/firmware/CHANGELOG.md +++ b/legacy/firmware/CHANGELOG.md @@ -19,6 +19,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). - Don't show addresses that have an unrecognized path. - Disable all testnet coins from accessing Bitcoin paths. - Restrict the BIP-32 path ranges of `account`, `change` and `address_index` fields. +- Fix operation source account encoding in Stellar. ## 1.10.1 [9th June 2021] diff --git a/legacy/firmware/stellar.c b/legacy/firmware/stellar.c index ccc7bd569..8b79adb30 100644 --- a/legacy/firmware/stellar.c +++ b/legacy/firmware/stellar.c @@ -165,8 +165,8 @@ bool stellar_signingInit(const StellarSignTx *msg) { bool stellar_confirmSourceAccount(bool has_source_account, const char *str_account) { + stellar_hashupdate_bool(has_source_account); if (!has_source_account) { - stellar_hashupdate_bool(false); return true; } diff --git a/tests/device_tests/test_msg_stellar_sign_transaction.py b/tests/device_tests/test_msg_stellar_sign_transaction.py index dd9a55e4b..70070ecd5 100644 --- a/tests/device_tests/test_msg_stellar_sign_transaction.py +++ b/tests/device_tests/test_msg_stellar_sign_transaction.py @@ -234,7 +234,7 @@ def test_sign_tx_allow_trust_op(client): def test_sign_tx_change_trust_op(client): op = messages.StellarChangeTrustOp() - op.limit = 500111000 + op.limit = 5000000000 op.source_account = "GBOVKZBEM2YYLOCDCUXJ4IMRKHN4LCJAE7WEAEA2KF562XFAGDBOB64V" op.asset = messages.StellarAssetType( @@ -248,7 +248,7 @@ def test_sign_tx_change_trust_op(client): assert ( b64encode(response.signature) - == b"OZdDO/qW8o/xbV6nZaDM/D7z9/fqbrk+P4lSzzCqeD3C8nGOg+Jl33JqHek0zNNOW9Pn+tPpfdoQnuZWJzocCw==" + == b"B7UyKRCzVf6esTkzDJgac0vJ1YfI4Z7Ecq65/3TY0+D/VB3myZVg06LMjgIf10q8kF+GvJwN6XGKRd6q1wxHAw==" ) diff --git a/tests/ui_tests/fixtures.json b/tests/ui_tests/fixtures.json index 172bc8285..626a07208 100644 --- a/tests/ui_tests/fixtures.json +++ b/tests/ui_tests/fixtures.json @@ -593,7 +593,7 @@ "test_msg_stellar_sign_transaction.py::test_sign_tx_account_merge_op": "cba8c17ea77956cbe0cb77fdbaeac7408cdc934d60f1a3d0c2f1d4375ce49ebd", "test_msg_stellar_sign_transaction.py::test_sign_tx_allow_trust_op": "e1585be0bbf49a9dc0902fca6e8df6fa5851cbbe27b22a88710944209ac29315", "test_msg_stellar_sign_transaction.py::test_sign_tx_bump_sequence_op": "eaf6d92c7b897338959b20d4aa1281110d0a5179b4bd53ceab88ee50b2456b80", -"test_msg_stellar_sign_transaction.py::test_sign_tx_change_trust_op": "8d6cc0009b370753bdf0b674650543ae24e68cb420cb451e23631002e497790e", +"test_msg_stellar_sign_transaction.py::test_sign_tx_change_trust_op": "c3e3e56d684deb29a1558bbb35c13aad80e26dd58230aeed9ee3ddd127efcb52", "test_msg_stellar_sign_transaction.py::test_sign_tx_create_account_op": "a1dcb3b4630fc45a771b4a6e65eab879adfe59c38624881e140cf08c1f83a9df", "test_msg_stellar_sign_transaction.py::test_sign_tx_manage_offer_op": "32b68728958da31382907f8c0a76547011d264241ec4974fa85c487ffdb7a1b0", "test_msg_stellar_sign_transaction.py::test_sign_tx_passive_offer_op": "ab6808486a4e98bbf990b55d60489a4f3464572428e9374a8f55d5352581a678",