From 2b2e2d01458f75cae8c5a8c7fd3064ae0295e3d0 Mon Sep 17 00:00:00 2001 From: refi93 Date: Sat, 15 Dec 2018 01:59:42 +0100 Subject: [PATCH 1/2] cardano better address export,sign tx UX,format fix, prot. magic instead of network --- src/apps/cardano/get_address.py | 15 ++-- src/apps/cardano/layout/__init__.py | 71 ++++++++++++++++- src/apps/cardano/sign_tx.py | 114 +++++++++++---------------- src/trezor/messages/CardanoSignTx.py | 6 +- vendor/trezor-common | 2 +- 5 files changed, 129 insertions(+), 79 deletions(-) diff --git a/src/apps/cardano/get_address.py b/src/apps/cardano/get_address.py index 9b03ebca4..541538654 100644 --- a/src/apps/cardano/get_address.py +++ b/src/apps/cardano/get_address.py @@ -1,10 +1,10 @@ -from trezor import log, ui, wire +from trezor import log, wire from trezor.messages.CardanoAddress import CardanoAddress from apps.cardano import seed from apps.cardano.address import derive_address_and_node, validate_full_path -from apps.cardano.layout import confirm_with_pagination from apps.common import paths +from apps.common.layout import address_n_to_str, show_address, show_qr async def get_address(ctx, msg): @@ -18,11 +18,12 @@ async def get_address(ctx, msg): if __debug__: log.exception(__name__, e) raise wire.ProcessError("Deriving address failed") - if msg.show_display: - if not await confirm_with_pagination( - ctx, address, "Export address", icon=ui.ICON_SEND, icon_color=ui.GREEN - ): - raise wire.ActionCancelled("Exporting cancelled") + desc = address_n_to_str(msg.address_n) + while True: + if await show_address(ctx, address, desc=desc): + break + if await show_qr(ctx, address, desc=desc): + break return CardanoAddress(address=address) diff --git a/src/apps/cardano/layout/__init__.py b/src/apps/cardano/layout/__init__.py index aff13d2b1..531b99de8 100644 --- a/src/apps/cardano/layout/__init__.py +++ b/src/apps/cardano/layout/__init__.py @@ -3,10 +3,77 @@ from micropython import const from trezor import ui from trezor.messages import ButtonRequestType, MessageType from trezor.messages.ButtonRequest import ButtonRequest -from trezor.ui.confirm import CONFIRMED, ConfirmDialog +from trezor.ui.confirm import CONFIRMED, ConfirmDialog, HoldToConfirmDialog from trezor.ui.scroll import Scrollpage, animate_swipe, paginate from trezor.ui.text import Text -from trezor.utils import chunks +from trezor.utils import chunks, format_amount + + +def format_coin_amount(amount): + return "%s %s" % (format_amount(amount, 6), "ADA") + + +async def confirm_sending(ctx, amount, to): + to_lines = list(chunks(to, 17)) + + t1 = Text("Confirm transaction", ui.ICON_SEND, icon_color=ui.GREEN) + t1.normal("Confirm sending:") + t1.bold(format_coin_amount(amount)) + t1.normal("to:") + t1.bold(to_lines[0]) + pages = [t1] + + LINES_PER_PAGE = 4 + if len(to_lines) > 1: + to_pages = list(chunks(to_lines[1:], LINES_PER_PAGE)) + for page in to_pages: + t = Text("Confirm transaction", ui.ICON_SEND, icon_color=ui.GREEN) + for line in page: + t.bold(line) + pages.append(t) + + await ctx.call(ButtonRequest(code=ButtonRequestType.Other), MessageType.ButtonAck) + + paginator = paginate(create_renderer(ConfirmDialog), len(pages), const(0), pages) + return await ctx.wait(paginator) == CONFIRMED + + +async def confirm_transaction(ctx, amount, fee, network_name): + t1 = Text("Confirm transaction", ui.ICON_SEND, icon_color=ui.GREEN) + t1.normal("Total amount:") + t1.bold(format_coin_amount(amount)) + t1.normal("including fee:") + t1.bold(format_coin_amount(fee)) + + t2 = Text("Confirm transaction", ui.ICON_SEND, icon_color=ui.GREEN) + t2.normal("Network:") + t2.bold(network_name) + + pages = [t1, t2] + + await ctx.call(ButtonRequest(code=ButtonRequestType.Other), MessageType.ButtonAck) + + paginator = paginate( + create_renderer(HoldToConfirmDialog), len(pages), const(0), pages + ) + return await ctx.wait(paginator) == CONFIRMED + + +def create_renderer(confirmation_wrapper): + @ui.layout + async def page_renderer(page: int, page_count: int, pages: list): + # for some reason page index can be equal to page count + if page >= page_count: + page = page_count - 1 + + content = Scrollpage(pages[page], page, page_count) + if page + 1 >= page_count: + return await confirmation_wrapper(content) + else: + content.render() + await animate_swipe() + + return page_renderer async def confirm_with_pagination( diff --git a/src/apps/cardano/sign_tx.py b/src/apps/cardano/sign_tx.py index ae453af1e..fb85a628a 100644 --- a/src/apps/cardano/sign_tx.py +++ b/src/apps/cardano/sign_tx.py @@ -1,74 +1,59 @@ -from trezor import log, ui, wire +from micropython import const + +from trezor import log, wire from trezor.crypto import base58, hashlib from trezor.crypto.curve import ed25519 from trezor.messages.CardanoSignedTx import CardanoSignedTx from trezor.messages.CardanoTxRequest import CardanoTxRequest from trezor.messages.MessageType import CardanoTxAck -from trezor.ui.text import BR from apps.cardano import cbor, seed from apps.cardano.address import derive_address_and_node, validate_full_path -from apps.cardano.layout import confirm_with_pagination, progress -from apps.common.layout import address_n_to_str, split_address +from apps.cardano.layout import confirm_sending, confirm_transaction, progress from apps.common.paths import validate_path from apps.common.seed import remove_ed25519_prefix from apps.homescreen.homescreen import display_homescreen +# 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_CHANGE_ADDRESS_INDEX = const(1000000) +ACCOUNT_PREFIX_DEPTH = const(2) + +KNOWN_PROTOCOL_MAGICS = {764824073: "Mainnet", 1097911063: "Testnet"} + + +# we consider addresses from the external chain as possible change addresses as well +def is_change(output, inputs): + for input in inputs: + inp = input.address_n + if ( + not output[:ACCOUNT_PREFIX_DEPTH] == inp[:ACCOUNT_PREFIX_DEPTH] + or not output[-2] < 2 + or not output[-1] < MAX_CHANGE_ADDRESS_INDEX + ): + return False + return True + async def show_tx( ctx, outputs: list, outcoins: list, - change_derivation_paths: list, - change_coins: list, - fee: float, - tx_size: float, + fee: int, network_name: str, + raw_inputs: list, + raw_outputs: list, ) -> bool: - lines = ("%s ADA" % _micro_ada_to_ada(fee), BR, "Tx size:", "%s bytes" % tx_size) - if not await confirm_with_pagination( - ctx, lines, "Confirm fee", ui.ICON_SEND, ui.GREEN - ): - return False - - if not await confirm_with_pagination( - ctx, "%s network" % network_name, "Confirm network", ui.ICON_SEND, ui.GREEN - ): - return False - for index, output in enumerate(outputs): - if not await confirm_with_pagination( - ctx, output, "Confirm output", ui.ICON_SEND, ui.GREEN - ): - return False + if is_change(raw_outputs[index].address_n, raw_inputs): + continue - if not await confirm_with_pagination( - ctx, - "%s ADA" % _micro_ada_to_ada(outcoins[index]), - "Confirm amount", - ui.ICON_SEND, - ui.GREEN, - ): + if not await confirm_sending(ctx, outcoins[index], output): return False - for index, change in enumerate(change_derivation_paths): - if not await confirm_with_pagination( - ctx, - list(split_address(address_n_to_str(change))), - "Confirm change", - ui.ICON_SEND, - ui.GREEN, - ): - return False - - if not await confirm_with_pagination( - ctx, - "%s ADA" % _micro_ada_to_ada(change_coins[index]), - "Confirm amount", - ui.ICON_SEND, - ui.GREEN, - ): - return False + total_amount = sum(outcoins) + if not await confirm_transaction(ctx, total_amount, fee, network_name): + return False return True @@ -100,7 +85,7 @@ async def sign_tx(ctx, msg): # sign the transaction bundle and prepare the result transaction = Transaction( - msg.inputs, msg.outputs, transactions, keychain, msg.network + msg.inputs, msg.outputs, transactions, keychain, msg.protocol_magic ) tx_body, tx_hash = transaction.serialise_tx() tx = CardanoSignedTx(tx_body=tx_body, tx_hash=tx_hash) @@ -115,24 +100,24 @@ async def sign_tx(ctx, msg): ctx, transaction.output_addresses, transaction.outgoing_coins, - transaction.change_derivation_paths, - transaction.change_coins, transaction.fee, - len(tx_body), transaction.network_name, + transaction.inputs, + transaction.outputs, ): raise wire.ActionCancelled("Signing cancelled") return tx -def _micro_ada_to_ada(amount: float) -> float: - return amount / 1000000 - - class Transaction: def __init__( - self, inputs: list, outputs: list, transactions: list, keychain, network: int + self, + inputs: list, + outputs: list, + transactions: list, + keychain, + protocol_magic: int, ): self.inputs = inputs self.outputs = outputs @@ -140,14 +125,9 @@ class Transaction: self.keychain = keychain # attributes have to be always empty in current Cardano self.attributes = {} - if network == 1: - self.network_name = "Testnet" - self.network_magic = b"\x01\x1a\x41\x70\xcb\x17\x58\x20" - elif network == 2: - self.network_name = "Mainnet" - self.network_magic = b"\x01\x1a\x2d\x96\x4a\x09\x58\x20" - else: - raise wire.ProcessError("Unknown network index %d" % network) + + self.network_name = KNOWN_PROTOCOL_MAGICS.get(protocol_magic, "Unknown") + self.protocol_magic = protocol_magic def _process_inputs(self): input_coins = [] @@ -217,7 +197,9 @@ class Transaction: def _build_witnesses(self, tx_aux_hash: str): witnesses = [] for index, node in enumerate(self.nodes): - message = self.network_magic + tx_aux_hash + message = ( + b"\x01" + cbor.encode(self.protocol_magic) + b"\x58\x20" + tx_aux_hash + ) signature = ed25519.sign_ext( node.private_key(), node.private_key_ext(), message ) diff --git a/src/trezor/messages/CardanoSignTx.py b/src/trezor/messages/CardanoSignTx.py index 38848a542..84f330321 100644 --- a/src/trezor/messages/CardanoSignTx.py +++ b/src/trezor/messages/CardanoSignTx.py @@ -20,12 +20,12 @@ class CardanoSignTx(p.MessageType): inputs: List[CardanoTxInputType] = None, outputs: List[CardanoTxOutputType] = None, transactions_count: int = None, - network: int = None, + protocol_magic: int = None, ) -> None: self.inputs = inputs if inputs is not None else [] self.outputs = outputs if outputs is not None else [] self.transactions_count = transactions_count - self.network = network + self.protocol_magic = protocol_magic @classmethod def get_fields(cls): @@ -33,5 +33,5 @@ class CardanoSignTx(p.MessageType): 1: ('inputs', CardanoTxInputType, p.FLAG_REPEATED), 2: ('outputs', CardanoTxOutputType, p.FLAG_REPEATED), 3: ('transactions_count', p.UVarintType, 0), - 4: ('network', p.UVarintType, 0), + 5: ('protocol_magic', p.UVarintType, 0), } diff --git a/vendor/trezor-common b/vendor/trezor-common index 877778fc9..b947aa3d4 160000 --- a/vendor/trezor-common +++ b/vendor/trezor-common @@ -1 +1 @@ -Subproject commit 877778fc93db87544df64e4fc1fa91deee1c8334 +Subproject commit b947aa3d45af58605f8f090078a2de3d5878571d From 516dc4608de0c737ebc6d3c1282c326294aed0ae Mon Sep 17 00:00:00 2001 From: refi93 Date: Mon, 21 Jan 2019 10:15:53 +0100 Subject: [PATCH 2/2] add cardano output address validation --- src/apps/cardano/address.py | 40 +++++++++++++++++++++++++++++++------ src/apps/cardano/sign_tx.py | 8 +++++++- 2 files changed, 41 insertions(+), 7 deletions(-) diff --git a/src/apps/cardano/address.py b/src/apps/cardano/address.py index 078d30903..d870aa73b 100644 --- a/src/apps/cardano/address.py +++ b/src/apps/cardano/address.py @@ -1,3 +1,4 @@ +from trezor import log from trezor.crypto import base58, crc, hashlib from apps.cardano import cbor @@ -5,6 +6,14 @@ from apps.common import HARDENED from apps.common.seed import remove_ed25519_prefix +def _encode_address_raw(address_data_encoded): + return base58.encode( + cbor.encode( + [cbor.Tagged(24, address_data_encoded), crc.crc32(address_data_encoded)] + ) + ) + + def derive_address_and_node(keychain, path: list): node = keychain.derive(path) @@ -16,12 +25,31 @@ def derive_address_and_node(keychain, path: list): address_data = [address_root, address_attributes, address_type] address_data_encoded = cbor.encode(address_data) - address = base58.encode( - cbor.encode( - [cbor.Tagged(24, address_data_encoded), crc.crc32(address_data_encoded)] - ) - ) - return (address, node) + return (_encode_address_raw(address_data_encoded), node) + + +def is_safe_output_address(address) -> bool: + """ + Determines whether it is safe to include the address as-is as + a tx output, preventing unintended side effects (e.g. CBOR injection) + """ + try: + address_hex = base58.decode(address) + address_unpacked = cbor.decode(address_hex) + except ValueError as e: + if __debug__: + log.exception(__name__, e) + return False + + if not isinstance(address_unpacked, list) or len(address_unpacked) != 2: + return False + + address_data_encoded = address_unpacked[0] + + if not isinstance(address_data_encoded, bytes): + return False + + return _encode_address_raw(address_data_encoded) == address def validate_full_path(path: list) -> bool: diff --git a/src/apps/cardano/sign_tx.py b/src/apps/cardano/sign_tx.py index fb85a628a..12cfa5b39 100644 --- a/src/apps/cardano/sign_tx.py +++ b/src/apps/cardano/sign_tx.py @@ -8,7 +8,11 @@ from trezor.messages.CardanoTxRequest import CardanoTxRequest from trezor.messages.MessageType import CardanoTxAck from apps.cardano import cbor, seed -from apps.cardano.address import derive_address_and_node, validate_full_path +from apps.cardano.address import ( + derive_address_and_node, + is_safe_output_address, + validate_full_path, +) from apps.cardano.layout import confirm_sending, confirm_transaction, progress from apps.common.paths import validate_path from apps.common.seed import remove_ed25519_prefix @@ -184,6 +188,8 @@ class Transaction: raise wire.ProcessError( "Each output must have address or address_n field!" ) + if not is_safe_output_address(output.address): + raise wire.ProcessError("Invalid output address!") outgoing_coins.append(output.amount) output_addresses.append(output.address)