From 7c3b5c86a5fdfedd1a4f048167b85fbb16cfa29b Mon Sep 17 00:00:00 2001 From: gabrielkerekes Date: Wed, 18 Aug 2021 13:08:18 +0200 Subject: [PATCH] fix(cardano): forbid mixing paths from multiple accounts in a single transaction --- .../fixtures/cardano/sign_tx.failed.json | 291 +++++++++++++++++- common/tests/fixtures/cardano/sign_tx.json | 32 +- core/src/all_modules.py | 2 + core/src/apps/cardano/certificates.py | 10 +- core/src/apps/cardano/get_address.py | 7 +- core/src/apps/cardano/helpers/__init__.py | 2 + .../cardano/helpers/account_path_check.py | 99 ++++++ core/src/apps/cardano/helpers/paths.py | 8 +- core/src/apps/cardano/sign_tx.py | 110 ++++--- 9 files changed, 486 insertions(+), 75 deletions(-) create mode 100644 core/src/apps/cardano/helpers/account_path_check.py diff --git a/common/tests/fixtures/cardano/sign_tx.failed.json b/common/tests/fixtures/cardano/sign_tx.failed.json index c2b0798531..1980d0a629 100644 --- a/common/tests/fixtures/cardano/sign_tx.failed.json +++ b/common/tests/fixtures/cardano/sign_tx.failed.json @@ -444,7 +444,7 @@ "signing_mode": "ORDINARY_TRANSACTION" }, "result": { - "error_message": "Outputs can not contain both address and address_parameters fields!" + "error_message": "Invalid output" } }, { @@ -745,7 +745,7 @@ "auxiliary_data": null, "inputs": [ { - "path": "m/1852'/1815'/0'/0/0", + "path": "m/1852'/1815'/190'/0/0", "prev_hash": "3b40265111d8bb3c3c608d95b3a0bf83461ace32d79336579a1939b3aad1c0b7", "prev_index": 0 } @@ -820,7 +820,7 @@ "auxiliary_data": null, "inputs": [ { - "path": "m/1852'/1815'/0'/0/0", + "path": "m/1852'/1815'/190'/0/0", "prev_hash": "3b40265111d8bb3c3c608d95b3a0bf83461ace32d79336579a1939b3aad1c0b7", "prev_index": 0 } @@ -929,6 +929,291 @@ "result": { "error_message": "Invalid token bundle in output" } + }, + { + "description": "Input and change output account mismatch", + "parameters": { + "protocol_magic": 764824073, + "network_id": 1, + "fee": 42, + "ttl": 10, + "certificates": [], + "withdrawals": [], + "auxiliary_data": null, + "inputs": [ + { + "path": "m/1852'/1815'/0'/0/0", + "prev_hash": "3b40265111d8bb3c3c608d95b3a0bf83461ace32d79336579a1939b3aad1c0b7", + "prev_index": 0 + } + ], + "outputs": [ + { + "address": "addr1q84sh2j72ux0l03fxndjnhctdg7hcppsaejafsa84vh7lwgmcs5wgus8qt4atk45lvt4xfxpjtwfhdmvchdf2m3u3hlsd5tq5r", + "amount": "1" + }, + { + "addressType": 0, + "path": "m/1852'/1815'/1'/0/0", + "stakingPath": "m/1852'/1815'/0'/2/0", + "amount": "7120787" + } + ], + "signing_mode": "ORDINARY_TRANSACTION" + }, + "result": { + "error_message": "Invalid witness request" + } + }, + { + "description": "Input and stake deregistration certificate account mismatch", + "parameters": { + "protocol_magic": 764824073, + "network_id": 1, + "fee": 42, + "ttl": 10, + "certificates": [ + { + "type": 1, + "path": "m/1852'/1815'/1'/2/0" + } + ], + "withdrawals": [], + "auxiliary_data": null, + "inputs": [ + { + "path": "m/1852'/1815'/0'/0/0", + "prev_hash": "3b40265111d8bb3c3c608d95b3a0bf83461ace32d79336579a1939b3aad1c0b7", + "prev_index": 0 + } + ], + "outputs": [ + { + "address": "addr1q84sh2j72ux0l03fxndjnhctdg7hcppsaejafsa84vh7lwgmcs5wgus8qt4atk45lvt4xfxpjtwfhdmvchdf2m3u3hlsd5tq5r", + "amount": "1" + } + ], + "signing_mode": "ORDINARY_TRANSACTION" + }, + "result": { + "error_message": "Invalid witness request" + } + }, + { + "description": "Input and withdrawal account mismatch", + "parameters": { + "protocol_magic": 764824073, + "network_id": 1, + "fee": 42, + "ttl": 10, + "certificates": [], + "withdrawals": [ + { + "path": "m/1852'/1815'/1'/2/0", + "amount": "1000" + } + ], + "auxiliary_data": null, + "inputs": [ + { + "path": "m/1852'/1815'/0'/0/0", + "prev_hash": "3b40265111d8bb3c3c608d95b3a0bf83461ace32d79336579a1939b3aad1c0b7", + "prev_index": 0 + } + ], + "outputs": [ + { + "address": "addr1q84sh2j72ux0l03fxndjnhctdg7hcppsaejafsa84vh7lwgmcs5wgus8qt4atk45lvt4xfxpjtwfhdmvchdf2m3u3hlsd5tq5r", + "amount": "1" + } + ], + "signing_mode": "ORDINARY_TRANSACTION" + }, + "result": { + "error_message": "Invalid witness request" + } + }, + { + "description": "Change output and stake deregistration account mismatch", + "parameters": { + "protocol_magic": 764824073, + "network_id": 1, + "fee": 42, + "ttl": 10, + "certificates": [ + { + "type": 1, + "path": "m/1852'/1815'/1'/2/0" + } + ], + "withdrawals": [], + "auxiliary_data": null, + "inputs": [ + { + "path": "m/1852'/1815'/0'/0/0", + "prev_hash": "3b40265111d8bb3c3c608d95b3a0bf83461ace32d79336579a1939b3aad1c0b7", + "prev_index": 0 + } + ], + "outputs": [ + { + "address": "addr1q84sh2j72ux0l03fxndjnhctdg7hcppsaejafsa84vh7lwgmcs5wgus8qt4atk45lvt4xfxpjtwfhdmvchdf2m3u3hlsd5tq5r", + "amount": "1" + }, + { + "addressType": 0, + "path": "m/1852'/1815'/0'/0/0", + "stakingPath": "m/1852'/1815'/0'/2/0", + "amount": "7120787" + } + ], + "signing_mode": "ORDINARY_TRANSACTION" + }, + "result": { + "error_message": "Invalid certificate" + } + }, + { + "description": "Change output and withdrawal account mismatch", + "parameters": { + "protocol_magic": 764824073, + "network_id": 1, + "fee": 42, + "ttl": 10, + "certificates": [], + "withdrawals": [ + { + "path": "m/1852'/1815'/1'/2/0", + "amount": "1000" + } + ], + "auxiliary_data": null, + "inputs": [ + { + "path": "m/1852'/1815'/0'/0/0", + "prev_hash": "3b40265111d8bb3c3c608d95b3a0bf83461ace32d79336579a1939b3aad1c0b7", + "prev_index": 0 + } + ], + "outputs": [ + { + "address": "addr1q84sh2j72ux0l03fxndjnhctdg7hcppsaejafsa84vh7lwgmcs5wgus8qt4atk45lvt4xfxpjtwfhdmvchdf2m3u3hlsd5tq5r", + "amount": "1" + }, + { + "addressType": 0, + "path": "m/1852'/1815'/0'/0/0", + "stakingPath": "m/1852'/1815'/0'/2/0", + "amount": "7120787" + } + ], + "signing_mode": "ORDINARY_TRANSACTION" + }, + "result": { + "error_message": "Invalid withdrawal" + } + }, + { + "description": "Stake deregistration certificate and withdrawal account mismatch", + "parameters": { + "protocol_magic": 764824073, + "network_id": 1, + "fee": 42, + "ttl": 10, + "certificates": [ + { + "type": 1, + "path": "m/1852'/1815'/0'/2/0" + } + ], + "withdrawals": [ + { + "path": "m/1852'/1815'/1'/2/0", + "amount": "1000" + } + ], + "auxiliary_data": null, + "inputs": [ + { + "path": "m/1852'/1815'/0'/0/0", + "prev_hash": "3b40265111d8bb3c3c608d95b3a0bf83461ace32d79336579a1939b3aad1c0b7", + "prev_index": 0 + } + ], + "outputs": [ + { + "address": "addr1q84sh2j72ux0l03fxndjnhctdg7hcppsaejafsa84vh7lwgmcs5wgus8qt4atk45lvt4xfxpjtwfhdmvchdf2m3u3hlsd5tq5r", + "amount": "1" + } + ], + "signing_mode": "ORDINARY_TRANSACTION" + }, + "result": { + "error_message": "Invalid withdrawal" + } + }, + { + "description": "Byron to Shelley transfer input account mismatch", + "parameters": { + "protocol_magic": 764824073, + "network_id": 1, + "fee": 42, + "certificates": [], + "withdrawals": [], + "auxiliary_data": null, + "inputs": [ + { + "path": "m/44'/1815'/0'/0/0", + "prev_hash": "3b40265111d8bb3c3c608d95b3a0bf83461ace32d79336579a1939b3aad1c0b7", + "prev_index": 0 + }, + { + "path": "m/1852'/1815'/1'/0/0", + "prev_hash": "3b40265111d8bb3c3c608d95b3a0bf83461ace32d79336579a1939b3aad1c0b7", + "prev_index": 1 + } + ], + "outputs": [ + { + "address": "addr1z90z7zqwhya6mpk5q929ur897g3pp9kkgalpreny8y304r2dcrtx0sf3dluyu4erzr3xtmdnzvcyfzekkuteu2xagx0qeva0pr", + "amount": "1" + } + ], + "signing_mode": "ORDINARY_TRANSACTION" + }, + "result": { + "error_message": "Invalid witness request" + } + }, + { + "description": "Byron to Shelley transfer output account mismatch", + "parameters": { + "protocol_magic": 764824073, + "network_id": 1, + "fee": 42, + "certificates": [], + "withdrawals": [], + "auxiliary_data": null, + "inputs": [ + { + "path": "m/44'/1815'/0'/0/0", + "prev_hash": "3b40265111d8bb3c3c608d95b3a0bf83461ace32d79336579a1939b3aad1c0b7", + "prev_index": 0 + } + ], + "outputs": [ + { + "addressType": 0, + "path": "m/1852'/1815'/1'/0/0", + "stakingPath": "m/1852'/1815'/1'/2/0", + "amount": "7120787" + } + ], + "signing_mode": "ORDINARY_TRANSACTION" + }, + "result": { + "error_message": "Invalid witness request" + } } ] } diff --git a/common/tests/fixtures/cardano/sign_tx.json b/common/tests/fixtures/cardano/sign_tx.json index 0d6f010e75..e4f3e0ed6c 100644 --- a/common/tests/fixtures/cardano/sign_tx.json +++ b/common/tests/fixtures/cardano/sign_tx.json @@ -252,7 +252,7 @@ "auxiliary_data": null, "inputs": [ { - "path": "m/1852'/1815'/0'/0/0", + "path": "m/1852'/1815'/190'/0/0", "prev_hash": "3b40265111d8bb3c3c608d95b3a0bf83461ace32d79336579a1939b3aad1c0b7", "prev_index": 0 } @@ -276,8 +276,8 @@ "witnesses": [ { "type": 1, - "pub_key": "5d010cf16fdeff40955633d6c565f3844a288a24967cf6b76acbeb271b4f13c1", - "signature": "4d8f0fd9798b62937b1739dcb893dfc6a0abd9eb2ad98244ced00d78949a2fd2751fac4bb2ebe5577688e5d9f47da79a52b5c571c3082996514ab5995dc2580d", + "pub_key": "eae29ffe0e7b8c844036c0a3bf4c7da1106c8fd08ca908d52bb76f7c7baa5719", + "signature": "8049c6d62abf26a303e184be326ff941e8c123c1cba2ce83daffb12c7c0cf31379e18f7afdd877d70a0fc2a4c8b144a22a3382489f7eff9c94ec9328177d0c0e", "chain_code": null } ] @@ -708,7 +708,7 @@ "auxiliary_data": null, "inputs": [ { - "path": "m/1852'/1815'/0'/0/0", + "path": "m/1852'/1815'/190'/0/0", "prev_hash": "3b40265111d8bb3c3c608d95b3a0bf83461ace32d79336579a1939b3aad1c0b7", "prev_index": 0 } @@ -726,8 +726,8 @@ "witnesses": [ { "type": 1, - "pub_key": "5d010cf16fdeff40955633d6c565f3844a288a24967cf6b76acbeb271b4f13c1", - "signature": "19b71c5b97a188cf9ff479b6ef7a45a0df94a917845d15c03a5fa508042724dcee1e8f944b91141ed91e6faa228806b3644e2560cb231470d09788ac5065ff00", + "pub_key": "eae29ffe0e7b8c844036c0a3bf4c7da1106c8fd08ca908d52bb76f7c7baa5719", + "signature": "26d1381ae36a70fb08edf04445fe90f37c6ed76283f637d2239caf83e6632c2722c784e7bf41770804f0e483cdd3c68b025d8fa81d53f4fb06b16ec261271808", "chain_code": null }, { @@ -1285,10 +1285,6 @@ { "path": "m/1852'/1815'/0'/2/0", "amount": "1000" - }, - { - "path": "m/1852'/1815'/1'/2/0", - "amount": "1000" } ], "auxiliary_data": { @@ -1306,36 +1302,30 @@ "signing_mode": "ORDINARY_TRANSACTION" }, "result": { - "tx_hash": "75a98c63e05095201f8309bb49d181e1ad67b380f9ab23f91fab32758eb65ae5", + "tx_hash": "ee0dfef8b97857ebe7aa8935af50e9f8f608ff4054c0c034600750d722d90631", "witnesses": [ { "type": 1, "pub_key": "5d010cf16fdeff40955633d6c565f3844a288a24967cf6b76acbeb271b4f13c1", - "signature": "86cb9f0df29e2e0685f913adadd941b5a568a122a36133a34e74749d65dbaf9346884adb3b1e86fee6f16ceccc7395d71bc9950350ed8760eed452eaef961e0b", + "signature": "7d17407e4e8f8b89f8794c022408a84e6f7ef163957d9d7e8ebee4cf9b5c87750c7c559f3a2663441535eec88ebce8540e7d7ea30897de984b1053b818374007", "chain_code": null }, { "type": 1, "pub_key": "36a8ef21d5b98fdf23a27325cf643deaac35e912c835e35037f23d1061ae5b16", - "signature": "36cc6f5099b52ee8799cd7fb3e960f64c415dcdf494e4a1271869ed980617dcd1b419a4278a700e4c40bd4bf189102cff4959e089bb1ba8c62a233901f963608", + "signature": "df62ec013a32d137c86931cec726d104cbc3193776026ec36d10450d9cbd289abc4c2d44311878b3aba035a8aec2c076522183027f9da046b586b5de5c460504", "chain_code": null }, { "type": 1, "pub_key": "e90d7b0a6cf831b0042d37961dd528842860e77914e715bcece676c75353b812", - "signature": "170d7e783b16ab20b517e0282e7937ca7c7eebd2546bbd219584c1ede9a104e98c2da9906d6b4238aeeeeeeff970f03da04b4a5e3342b4169db4cea2f2134008", + "signature": "e249396d227f1d0540e58b64610bdb990eb1f1db9b3bae4a3d4a8088679af4a3bab464a5c912f7041a5fabc37e3009b3e1f4d76e2406429a0ebed85b880ecd0c", "chain_code": null }, { "type": 1, "pub_key": "bc65be1b0b9d7531778a1317c2aa6de936963c3f9ac7d5ee9e9eda25e0c97c5e", - "signature": "0e486eae9df832c3f3e635e0322c1b1bbfa1834e31fb43efce0bd6e66c949587a5a9edad9ff5abb0213d964640b98457a3afc82dfa4acc2e9108e66930435607", - "chain_code": null - }, - { - "type": 1, - "pub_key": "79f81c0b814ee61b4463792a7a5cf45940f15813d8ebe1ecc6261ecd8afc5799", - "signature": "16c37673a359a9a96213291281dcb029cc5b504f5a285711dd5f44ea7bff0d9729a00e2ef1d7761f5e9575ceda16ff02ccb223a573e7faf67e2b63de6f3bee0c", + "signature": "0dfd139ce3e255664a77de7d199ce5e4f1a1238ec17a6acec4aaae79be2ccd9b1d21127164c059c8aea2c4b91292aaf352c824550db7594b59e4eca6455d3f03", "chain_code": null } ], diff --git a/core/src/all_modules.py b/core/src/all_modules.py index 9fcdeff06e..c59b9b18c5 100644 --- a/core/src/all_modules.py +++ b/core/src/all_modules.py @@ -446,6 +446,8 @@ if utils.BITCOIN_ONLY: import apps.cardano.get_public_key apps.cardano.helpers import apps.cardano.helpers + apps.cardano.helpers.account_path_check + import apps.cardano.helpers.account_path_check apps.cardano.helpers.bech32 import apps.cardano.helpers.bech32 apps.cardano.helpers.hash_builder_collection diff --git a/core/src/apps/cardano/certificates.py b/core/src/apps/cardano/certificates.py index 9cb0334d89..e2fe8b5427 100644 --- a/core/src/apps/cardano/certificates.py +++ b/core/src/apps/cardano/certificates.py @@ -26,6 +26,7 @@ if False: from apps.common.cbor import CborSequence from . import seed + from .helpers.account_path_check import AccountPathChecker POOL_HASH_SIZE = 28 VRF_KEY_HASH_SIZE = 32 @@ -42,6 +43,7 @@ def validate_certificate( signing_mode: CardanoTxSigningMode, protocol_magic: int, network_id: int, + account_path_checker: AccountPathChecker, ) -> None: if ( signing_mode == CardanoTxSigningMode.ORDINARY_TRANSACTION @@ -73,6 +75,8 @@ def validate_certificate( certificate.pool_parameters, protocol_magic, network_id ) + account_path_checker.add_certificate(certificate) + def cborize_certificate( keychain: seed.Keychain, certificate: CardanoTxCertificate @@ -147,7 +151,9 @@ def _validate_pool_parameters( _validate_pool_metadata(pool_parameters.metadata) -def validate_pool_owner(owner: CardanoPoolOwner) -> None: +def validate_pool_owner( + owner: CardanoPoolOwner, account_path_checker: AccountPathChecker +) -> None: assert_certificate_cond( owner.staking_key_hash is not None or owner.staking_key_path is not None ) @@ -158,6 +164,8 @@ def validate_pool_owner(owner: CardanoPoolOwner) -> None: SCHEMA_STAKING_ANY_ACCOUNT.match(owner.staking_key_path) ) + account_path_checker.add_pool_owner(owner) + def validate_pool_relay(pool_relay: CardanoPoolRelayParameters) -> None: if pool_relay.type == CardanoPoolRelayType.SINGLE_HOST_IP: diff --git a/core/src/apps/cardano/get_address.py b/core/src/apps/cardano/get_address.py index 2c1bb30fc8..8f29e00933 100644 --- a/core/src/apps/cardano/get_address.py +++ b/core/src/apps/cardano/get_address.py @@ -7,7 +7,7 @@ from apps.common import paths from . import seed from .address import derive_human_readable_address, validate_address_parameters from .helpers import protocol_magics, staking_use_cases -from .helpers.paths import SCHEMA_ADDRESS +from .helpers.paths import SCHEMA_PAYMENT, SCHEMA_STAKING from .helpers.utils import to_account_path from .layout import ( ADDRESS_TYPE_NAMES, @@ -33,8 +33,9 @@ async def get_address( ctx, keychain, address_parameters.address_n, - # path must match the ADDRESS schema - SCHEMA_ADDRESS.match(address_parameters.address_n), + # path must match the PAYMENT or STAKING schema + SCHEMA_PAYMENT.match(address_parameters.address_n) + or SCHEMA_STAKING.match(address_parameters.address_n), ) validate_network_info(msg.network_id, msg.protocol_magic) diff --git a/core/src/apps/cardano/helpers/__init__.py b/core/src/apps/cardano/helpers/__init__.py index 16a05e5e4b..3d16e01a93 100644 --- a/core/src/apps/cardano/helpers/__init__.py +++ b/core/src/apps/cardano/helpers/__init__.py @@ -3,6 +3,7 @@ from trezor import wire INVALID_ADDRESS = wire.ProcessError("Invalid address") INVALID_ADDRESS_PARAMETERS = wire.ProcessError("Invalid address parameters") NETWORK_MISMATCH = wire.ProcessError("Output address network mismatch") +INVALID_OUTPUT = wire.ProcessError("Invalid output") INVALID_CERTIFICATE = wire.ProcessError("Invalid certificate") INVALID_WITHDRAWAL = wire.ProcessError("Invalid withdrawal") INVALID_TOKEN_BUNDLE_OUTPUT = wire.ProcessError("Invalid token bundle in output") @@ -13,6 +14,7 @@ INVALID_STAKE_POOL_REGISTRATION_TX_STRUCTURE = wire.ProcessError( INVALID_STAKEPOOL_REGISTRATION_TX_WITNESSES = wire.ProcessError( "Stakepool registration transaction can only contain staking witnesses" ) +INVALID_WITNESS_REQUEST = wire.ProcessError("Invalid witness request") LOVELACE_MAX_SUPPLY = 45_000_000_000 * 1_000_000 ADDRESS_KEY_HASH_SIZE = 28 diff --git a/core/src/apps/cardano/helpers/account_path_check.py b/core/src/apps/cardano/helpers/account_path_check.py new file mode 100644 index 0000000000..eb75e631b3 --- /dev/null +++ b/core/src/apps/cardano/helpers/account_path_check.py @@ -0,0 +1,99 @@ +from ...common.paths import HARDENED +from ..seed import is_byron_path, is_shelley_path +from . import ( + INVALID_CERTIFICATE, + INVALID_OUTPUT, + INVALID_WITHDRAWAL, + INVALID_WITNESS_REQUEST, +) +from .paths import ACCOUNT_PATH_INDEX, ACCOUNT_PATH_LENGTH +from .utils import to_account_path + +if False: + from trezor import wire + from trezor.messages import ( + CardanoPoolOwner, + CardanoTxCertificate, + CardanoTxOutput, + CardanoTxWitnessRequest, + CardanoTxWithdrawal, + ) + + +class AccountPathChecker: + """ + Used to verify that all paths in a transaction which are not being shown to the user belong + to the same account. Paths are matched against the path which is added first. If there's a mismatch, + an error is raised. + """ + + UNDEFINED = object() + + def __init__(self) -> None: + self.account_path: object | list[int] = self.UNDEFINED + + def _add(self, path: list[int], error: wire.ProcessError) -> None: + account_path = to_account_path(path) + if self.account_path is self.UNDEFINED: + self.account_path = account_path + elif ( + self.account_path != account_path + and not self._is_byron_and_shelley_equivalent(account_path) + ): + raise error + + def _is_byron_and_shelley_equivalent(self, account_path: list[int]) -> bool: + """ + For historical purposes Byron path (44'/1815'/0') is considered equivalent to the Shelley + path with the same account (1852'/1815'/0'). This combination of accounts is allowed + in order to make Byron to Shelley migrations possible with the Shelley path staying hidden + from the user. This way the user can be sure that the funds are being moved between the user's + accounts without being bothered by more screens. + """ + assert isinstance(self.account_path, list) + is_control_path_byron_or_shelley = is_byron_path( + self.account_path + ) or is_shelley_path(self.account_path) + + is_new_path_byron_or_shelley = is_byron_path(account_path) or is_shelley_path( + account_path + ) + + return ( + is_control_path_byron_or_shelley + and is_new_path_byron_or_shelley + and len(self.account_path) == ACCOUNT_PATH_LENGTH + and len(account_path) == ACCOUNT_PATH_LENGTH + and self.account_path[ACCOUNT_PATH_INDEX] == 0 | HARDENED + and account_path[ACCOUNT_PATH_INDEX] == 0 | HARDENED + ) + + def add_output(self, output: CardanoTxOutput) -> None: + if not output.address_parameters: + return + + if not output.address_parameters.address_n: + return + + self._add(output.address_parameters.address_n, INVALID_OUTPUT) + + def add_certificate(self, certificate: CardanoTxCertificate) -> None: + if not certificate.path: + return + + self._add(certificate.path, INVALID_CERTIFICATE) + + def add_pool_owner(self, pool_owner: CardanoPoolOwner) -> None: + if not pool_owner.staking_key_path: + return + + self._add(pool_owner.staking_key_path, INVALID_CERTIFICATE) + + def add_withdrawal(self, withdrawal: CardanoTxWithdrawal) -> None: + if not withdrawal.path: + return + + self._add(withdrawal.path, INVALID_WITHDRAWAL) + + def add_witness_request(self, witness_request: CardanoTxWitnessRequest) -> None: + self._add(witness_request.path, INVALID_WITNESS_REQUEST) diff --git a/core/src/apps/cardano/helpers/paths.py b/core/src/apps/cardano/helpers/paths.py index a7141a017f..98b4f2b68d 100644 --- a/core/src/apps/cardano/helpers/paths.py +++ b/core/src/apps/cardano/helpers/paths.py @@ -9,18 +9,14 @@ SHELLEY_ROOT = [1852 | HARDENED, SLIP44_ID | HARDENED] # fmt: off SCHEMA_PUBKEY = PathSchema.parse("m/[44,1852]'/coin_type'/account'/*", SLIP44_ID) -SCHEMA_ADDRESS = PathSchema.parse("m/[44,1852]'/coin_type'/account'/[0,1,2]/address_index", SLIP44_ID) +SCHEMA_PAYMENT = PathSchema.parse("m/[44,1852]'/coin_type'/account'/[0,1]/address_index", SLIP44_ID) # staking is only allowed on Shelley paths with suffix /2/0 SCHEMA_STAKING = PathSchema.parse("m/1852'/coin_type'/account'/2/0", SLIP44_ID) SCHEMA_STAKING_ANY_ACCOUNT = PathSchema.parse("m/1852'/coin_type'/[0-%s]'/2/0" % (HARDENED - 1), SLIP44_ID) # fmt: on -# the maximum allowed change address. this should be large enough for normal -# use and still allow to quickly brute-force the correct bip32 path -MAX_SAFE_CHANGE_ADDRESS_INDEX = const(1_000_000) -MAX_SAFE_ACCOUNT_INDEX = const(100) | HARDENED +ACCOUNT_PATH_LENGTH = const(3) ACCOUNT_PATH_INDEX = const(2) -BIP_PATH_LENGTH = const(5) CHANGE_OUTPUT_PATH_NAME = "Change output path" CHANGE_OUTPUT_STAKING_PATH_NAME = "Change output staking path" diff --git a/core/src/apps/cardano/sign_tx.py b/core/src/apps/cardano/sign_tx.py index 0bb042c755..40fa16a6b1 100644 --- a/core/src/apps/cardano/sign_tx.py +++ b/core/src/apps/cardano/sign_tx.py @@ -56,6 +56,7 @@ from .certificates import ( validate_pool_relay, ) from .helpers import ( + INVALID_OUTPUT, INVALID_STAKE_POOL_REGISTRATION_TX_STRUCTURE, INVALID_STAKEPOOL_REGISTRATION_TX_WITNESSES, INVALID_TOKEN_BUNDLE_OUTPUT, @@ -65,17 +66,14 @@ from .helpers import ( protocol_magics, staking_use_cases, ) +from .helpers.account_path_check import AccountPathChecker from .helpers.hash_builder_collection import HashBuilderDict, HashBuilderList from .helpers.paths import ( - ACCOUNT_PATH_INDEX, - BIP_PATH_LENGTH, CERTIFICATE_PATH_NAME, CHANGE_OUTPUT_PATH_NAME, CHANGE_OUTPUT_STAKING_PATH_NAME, - MAX_SAFE_ACCOUNT_INDEX, - MAX_SAFE_CHANGE_ADDRESS_INDEX, POOL_OWNER_STAKING_PATH_NAME, - SCHEMA_ADDRESS, + SCHEMA_PAYMENT, SCHEMA_STAKING, SCHEMA_STAKING_ANY_ACCOUNT, WITNESS_PATH_NAME, @@ -139,18 +137,25 @@ async def sign_tx( ) ) + account_path_checker = AccountPathChecker() + hash_fn = hashlib.blake2b(outlen=32) tx_dict: HashBuilderDict[int, Any] = HashBuilderDict(tx_body_map_item_count) tx_dict.start(hash_fn) with tx_dict: - await _process_transaction(ctx, msg, keychain, tx_dict) + await _process_transaction(ctx, msg, keychain, tx_dict, account_path_checker) await _confirm_transaction(ctx, msg, is_network_id_verifiable) try: tx_hash = hash_fn.digest() response_after_witness_requests = await _process_witness_requests( - ctx, keychain, tx_hash, msg.witness_requests_count, msg.signing_mode + ctx, + keychain, + tx_hash, + msg.witness_requests_count, + msg.signing_mode, + account_path_checker, ) await ctx.call(response_after_witness_requests, CardanoTxHostAck) @@ -186,6 +191,7 @@ async def _process_transaction( msg: CardanoSignTxInit, keychain: seed.Keychain, tx_dict: HashBuilderDict, + account_path_checker: AccountPathChecker, ) -> None: inputs_list: HashBuilderList[tuple[bytes, int]] = HashBuilderList(msg.inputs_count) with tx_dict.add(TX_BODY_KEY_INPUTS, inputs_list): @@ -201,6 +207,7 @@ async def _process_transaction( msg.signing_mode, msg.protocol_magic, msg.network_id, + account_path_checker, ) tx_dict.add(TX_BODY_KEY_FEE, msg.fee) @@ -219,6 +226,7 @@ async def _process_transaction( msg.signing_mode, msg.protocol_magic, msg.network_id, + account_path_checker, ) if msg.withdrawals_count > 0: @@ -233,6 +241,7 @@ async def _process_transaction( msg.withdrawals_count, msg.protocol_magic, msg.network_id, + account_path_checker, ) if msg.has_auxiliary_data: @@ -287,12 +296,13 @@ async def _process_outputs( signing_mode: CardanoTxSigningMode, protocol_magic: int, network_id: int, + account_path_checker: AccountPathChecker, ) -> None: """Read, validate, confirm and serialize the outputs, return the total non-change output amount.""" total_amount = 0 for _ in range(outputs_count): output: CardanoTxOutput = await ctx.call(CardanoTxItemAck(), CardanoTxOutput) - _validate_output(output, protocol_magic, network_id) + _validate_output(output, protocol_magic, network_id, account_path_checker) if signing_mode == CardanoTxSigningMode.ORDINARY_TRANSACTION: await _show_output( ctx, @@ -393,6 +403,7 @@ async def _process_certificates( signing_mode: CardanoTxSigningMode, protocol_magic: int, network_id: int, + account_path_checker: AccountPathChecker, ) -> None: """Read, validate, confirm and serialize the certificates.""" if certificates_count == 0: @@ -402,7 +413,9 @@ async def _process_certificates( certificate: CardanoTxCertificate = await ctx.call( CardanoTxItemAck(), CardanoTxCertificate ) - validate_certificate(certificate, signing_mode, protocol_magic, network_id) + validate_certificate( + certificate, signing_mode, protocol_magic, network_id, account_path_checker + ) await _show_certificate(ctx, certificate, signing_mode) if certificate.type == CardanoCertificateType.STAKE_POOL_REGISTRATION: @@ -428,6 +441,7 @@ async def _process_certificates( pool_owners_list, pool_parameters.owners_count, network_id, + account_path_checker, ) relays_list: HashBuilderList[cbor.CborSequence] = HashBuilderList( @@ -449,11 +463,12 @@ async def _process_pool_owners( pool_owners_list: HashBuilderList[bytes], owners_count: int, network_id: int, + account_path_checker: AccountPathChecker, ) -> None: owners_as_path_count = 0 for _ in range(owners_count): owner: CardanoPoolOwner = await ctx.call(CardanoTxItemAck(), CardanoPoolOwner) - validate_pool_owner(owner) + validate_pool_owner(owner, account_path_checker) await _show_pool_owner(ctx, keychain, owner, network_id) pool_owners_list.append(cborize_pool_owner(keychain, owner)) @@ -484,6 +499,7 @@ async def _process_withdrawals( withdrawals_count: int, protocol_magic: int, network_id: int, + account_path_checker: AccountPathChecker, ) -> None: """Read, validate, confirm and serialize the withdrawals.""" if withdrawals_count == 0: @@ -496,7 +512,7 @@ async def _process_withdrawals( withdrawal: CardanoTxWithdrawal = await ctx.call( CardanoTxItemAck(), CardanoTxWithdrawal ) - _validate_withdrawal(withdrawal, seen_withdrawals) + _validate_withdrawal(withdrawal, seen_withdrawals, account_path_checker) await confirm_withdrawal(ctx, withdrawal) reward_address = derive_address_bytes( keychain, @@ -551,11 +567,12 @@ async def _process_witness_requests( tx_hash: bytes, witness_requests_count: int, signing_mode: CardanoTxSigningMode, + account_path_checker: AccountPathChecker, ) -> CardanoTxResponseType: response: CardanoTxResponseType = CardanoTxItemAck() for _ in range(witness_requests_count): witness_request = await ctx.call(response, CardanoTxWitnessRequest) - _validate_witness(signing_mode, witness_request) + _validate_witness_request(witness_request, signing_mode, account_path_checker) await _show_witness(ctx, witness_request.path) response = ( @@ -616,21 +633,22 @@ def _validate_stake_pool_registration_tx_structure(msg: CardanoSignTxInit) -> No def _validate_output( - output: CardanoTxOutput, protocol_magic: int, network_id: int + output: CardanoTxOutput, + protocol_magic: int, + network_id: int, + account_path_checker: AccountPathChecker, ) -> None: if output.address_parameters and output.address is not None: - raise wire.ProcessError( - "Outputs can not contain both address and address_parameters fields!" - ) + raise INVALID_OUTPUT if output.address_parameters: validate_address_parameters(output.address_parameters) elif output.address is not None: validate_output_address(output.address, protocol_magic, network_id) else: - raise wire.ProcessError( - "Each output must have an address field or address_parameters!" - ) + raise INVALID_OUTPUT + + account_path_checker.add_output(output) async def _show_output( @@ -644,7 +662,7 @@ async def _show_output( if output.address_parameters: await _fail_or_warn_if_invalid_path( ctx, - SCHEMA_ADDRESS, + SCHEMA_PAYMENT, output.address_parameters.address_n, CHANGE_OUTPUT_PATH_NAME, ) @@ -703,7 +721,9 @@ async def _show_certificate( def _validate_withdrawal( - withdrawal: CardanoTxWithdrawal, seen_withdrawals: set[tuple[int, ...]] + withdrawal: CardanoTxWithdrawal, + seen_withdrawals: set[tuple[int, ...]], + account_path_checker: AccountPathChecker, ) -> None: if not SCHEMA_STAKING_ANY_ACCOUNT.match(withdrawal.path): raise INVALID_WITHDRAWAL @@ -717,6 +737,8 @@ def _validate_withdrawal( else: seen_withdrawals.add(path_tuple) + account_path_checker.add_withdrawal(withdrawal) + def _get_output_address( keychain: seed.Keychain, @@ -768,14 +790,17 @@ async def _show_pool_owner( await confirm_stake_pool_owner(ctx, keychain, owner, network_id) -def _validate_witness( +def _validate_witness_request( + witness_request: CardanoTxWitnessRequest, signing_mode: CardanoTxSigningMode, - witness: CardanoTxWitnessRequest, + account_path_checker: AccountPathChecker, ) -> None: # witness path validation happens in _show_witness if signing_mode == CardanoTxSigningMode.POOL_REGISTRATION_AS_OWNER: - _ensure_no_payment_witness(witness) + _ensure_no_payment_witness(witness_request) + + account_path_checker.add_witness_request(witness_request) def _ensure_no_payment_witness(witness: CardanoTxWitnessRequest) -> None: @@ -799,12 +824,14 @@ async def _show_witness( ctx: wire.Context, witness_path: list[int], ) -> None: - await _fail_or_warn_if_invalid_path( - ctx, - SCHEMA_ADDRESS, - witness_path, - WITNESS_PATH_NAME, - ) + if not SCHEMA_PAYMENT.match(witness_path) and not SCHEMA_STAKING.match( + witness_path + ): + await _fail_or_warn_path( + ctx, + witness_path, + WITNESS_PATH_NAME, + ) async def _show_change_output_staking_warnings( @@ -856,19 +883,14 @@ async def _show_change_output_staking_warnings( def _should_hide_output(path: list[int]) -> bool: """Return whether the output address is from a safe path, so it could be hidden.""" - return ( - len(path) == BIP_PATH_LENGTH - and path[ACCOUNT_PATH_INDEX] <= MAX_SAFE_ACCOUNT_INDEX - and path[-2] < 2 - and path[-1] < MAX_SAFE_CHANGE_ADDRESS_INDEX - ) + return SCHEMA_PAYMENT.match(path) def _should_show_tokens( output: CardanoTxOutput, signing_mode: CardanoTxSigningMode ) -> bool: if signing_mode != CardanoTxSigningMode.ORDINARY_TRANSACTION: - return True + return False if output.address_parameters: return not _should_hide_output(output.address_parameters.address_n) @@ -893,7 +915,13 @@ async def _fail_or_warn_if_invalid_path( ctx: wire.Context, schema: PathSchema, path: list[int], path_name: str ) -> None: if not schema.match(path): - if safety_checks.is_strict(): - raise wire.DataError("Invalid %s" % path_name.lower()) - else: - await show_warning_path(ctx, path, path_name) + await _fail_or_warn_path(ctx, path, path_name) + + +async def _fail_or_warn_path( + ctx: wire.Context, path: list[int], path_name: str +) -> None: + if safety_checks.is_strict(): + raise wire.DataError("Invalid %s" % path_name.lower()) + else: + await show_warning_path(ctx, path, path_name)