From b7b46b6980f4c9538aa1fe46c04e5712630c1893 Mon Sep 17 00:00:00 2001 From: Tomas Susanka Date: Thu, 9 Nov 2017 12:45:24 +0100 Subject: [PATCH] wallet/signing: amount is checked during witness signature with test --- src/apps/wallet/sign_tx/signing.py | 12 ++- tests/test_apps.wallet.segwit.signtx.py | 109 ++++++++++++++++++++++++ 2 files changed, 118 insertions(+), 3 deletions(-) diff --git a/src/apps/wallet/sign_tx/signing.py b/src/apps/wallet/sign_tx/signing.py index 9d252fb6e..d262576b2 100644 --- a/src/apps/wallet/sign_tx/signing.py +++ b/src/apps/wallet/sign_tx/signing.py @@ -94,7 +94,7 @@ async def check_tx_fee(tx: SignTx, root): raise SigningError(FailureType.ActionCancelled, 'Total cancelled') - return h_first, tx_req, txo_bin, bip143, segwit + return h_first, tx_req, txo_bin, bip143, segwit, total_in async def sign_tx(tx: SignTx, root): @@ -103,7 +103,7 @@ async def sign_tx(tx: SignTx, root): # Phase 1 - h_first, tx_req, txo_bin, bip143, segwit = await check_tx_fee(tx, root) + h_first, tx_req, txo_bin, bip143, segwit, authorized_in = await check_tx_fee(tx, root) # Phase 2 # - sign inputs @@ -215,7 +215,13 @@ async def sign_tx(tx: SignTx, root): if segwit[i]: # STAGE_REQUEST_SEGWIT_WITNESS txi = await request_tx_input(tx_req, i) - # todo check amount? + + # Check amount + if txi.amount > authorized_in: + raise SigningError(FailureType.ProcessError, + 'Transaction has changed during signing') + authorized_in -= txi.amount + key_sign = node_derive(root, txi.address_n) key_sign_pub = key_sign.public_key() bip143_hash = bip143.preimage_hash(tx, txi, ecdsa_hash_pubkey(key_sign_pub)) diff --git a/tests/test_apps.wallet.segwit.signtx.py b/tests/test_apps.wallet.segwit.signtx.py index c0c88494f..d83bee796 100644 --- a/tests/test_apps.wallet.segwit.signtx.py +++ b/tests/test_apps.wallet.segwit.signtx.py @@ -17,6 +17,7 @@ from trezor.messages import OutputScriptType from apps.common import coins from apps.wallet.sign_tx import signing + class TestSignSegwitTx(unittest.TestCase): # pylint: disable=C0301 @@ -210,6 +211,114 @@ class TestSignSegwitTx(unittest.TestCase): with self.assertRaises(StopIteration): signer.send(None) + # see https://github.com/trezor/trezor-mcu/commit/6b615ce40567cc0da0b3b38ff668916aaae9dd4b#commitcomment-25505919 + # for the rational behind this attack + def test_send_p2wpkh_in_p2sh_attack_amount(self): + + coin = coins.by_name('Testnet') + + seed = bip39.seed(' '.join(['all'] * 12), '') + root = bip32.from_seed(seed, 'secp256k1') + + inp1 = TxInputType( + # 49'/1'/0'/1/0" - 2N1LGaGg836mqSQqiuUBLfcyGBhyZbremDX + address_n=[49 | 0x80000000, 1 | 0x80000000, 0 | 0x80000000, 1, 0], + amount=10, + prev_hash=unhexlify('20912f98ea3ed849042efed0fdac8cb4fc301961c5988cba56902d8ffb61c337'), + prev_index=0, + script_type=InputScriptType.SPENDP2SHWITNESS, + sequence=0xffffffff, + ) + inpattack = TxInputType( + # 49'/1'/0'/1/0" - 2N1LGaGg836mqSQqiuUBLfcyGBhyZbremDX + address_n=[49 | 0x80000000, 1 | 0x80000000, 0 | 0x80000000, 1, 0], + amount=9, # modified! + prev_hash=unhexlify('20912f98ea3ed849042efed0fdac8cb4fc301961c5988cba56902d8ffb61c337'), + prev_index=0, + script_type=InputScriptType.SPENDP2SHWITNESS, + sequence=0xffffffff, + ) + out1 = TxOutputType( + address='mhRx1CeVfaayqRwq5zgRQmD7W5aWBfD5mC', + amount=8, + script_type=OutputScriptType.PAYTOADDRESS, + address_n=None, + ) + out2 = TxOutputType( + address_n = [49 | 0x80000000, 1 | 0x80000000, 0 | 0x80000000, 1, 0], + script_type = OutputScriptType.PAYTOP2SHWITNESS, + amount = 1, + ) + tx = SignTx(coin_name='Testnet', version=None, lock_time=None, inputs_count=1, outputs_count=2) + + messages = [ + None, + + # check fee + TxRequest(request_type=TXINPUT, details=TxRequestDetailsType(request_index=0, tx_hash=None)), + TxAck(tx=TransactionType(inputs=[inpattack])), + + TxRequest(request_type=TXOUTPUT, details=TxRequestDetailsType(request_index=0, tx_hash=None), + serialized=None), + TxAck(tx=TransactionType(outputs=[out1])), + + signing.UiConfirmOutput(out1, coin), + True, + + TxRequest(request_type=TXOUTPUT, details=TxRequestDetailsType(request_index=1, tx_hash=None), + serialized=None), + TxAck(tx=TransactionType(outputs=[out2])), + + signing.UiConfirmTotal(8, 0, coin), + True, + + TxRequest(request_type=TXINPUT, details=TxRequestDetailsType(request_index=0, tx_hash=None), + serialized=None), + TxAck(tx=TransactionType(inputs=[inp1])), + + TxRequest(request_type=TXOUTPUT, details=TxRequestDetailsType(request_index=0, tx_hash=None), + serialized=TxRequestSerializedType( + # returned serialized inpattack + serialized_tx=unhexlify( + '0100000000010137c361fb8f2d9056ba8c98c5611930fcb48cacfdd0fe2e0449d83eea982f91200000000017160014d16b8c0680c61fc6ed2e407455715055e41052f5ffffffff'), + )), + TxAck(tx=TransactionType(outputs=[out1])), + + TxRequest(request_type=TXOUTPUT, details=TxRequestDetailsType(request_index=1, tx_hash=None), + serialized=TxRequestSerializedType( + # returned serialized out1 + serialized_tx=unhexlify( + '0208000000000000001976a91414fdede0ddc3be652a0ce1afbc1b509a55b6b94888ac'), + signature_index=None, + signature=None, + )), + TxAck(tx=TransactionType(outputs=[out2])), + + # segwit + TxRequest(request_type=TXINPUT, details=TxRequestDetailsType(request_index=0, tx_hash=None), + serialized=TxRequestSerializedType( + # returned serialized out2 + serialized_tx=unhexlify( + '010000000000000017a91458b53ea7f832e8f096e896b8713a8c6df0e892ca87'), + signature_index=None, + signature=None, + )), + TxAck(tx=TransactionType(inputs=[inp1])), + + TxRequest(request_type=TXFINISHED, details=None) + ] + + signer = signing.sign_tx(tx, root) + i = 0 + messages_count = int(len(messages) / 2) + for request, response in chunks(messages, 2): + if i == messages_count - 1: # last message should throw SigningError + self.assertRaises(signing.SigningError, signer.send, request) + else: + self.assertEqualEx(signer.send(request), response) + i += 1 + with self.assertRaises(StopIteration): + signer.send(None) def assertEqualEx(self, a, b): # hack to avoid adding __eq__ to signing.Ui* classes