diff --git a/core/CHANGELOG.md b/core/CHANGELOG.md index d008518ca..3b9da201b 100644 --- a/core/CHANGELOG.md +++ b/core/CHANGELOG.md @@ -29,6 +29,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). ### Security - Show non-empty passphrase on device when it was entered on host. +- Show warning if nLockTime is set but ineffective due to all nSequence values being 0xffffffff. ## 2.3.2 [5th August 2020] diff --git a/core/src/apps/bitcoin/sign_tx/__init__.py b/core/src/apps/bitcoin/sign_tx/__init__.py index 28cacc3ac..4bd4b0b79 100644 --- a/core/src/apps/bitcoin/sign_tx/__init__.py +++ b/core/src/apps/bitcoin/sign_tx/__init__.py @@ -72,7 +72,9 @@ async def sign_tx( ) progress.report_init() elif isinstance(req, helpers.UiConfirmNonDefaultLocktime): - res = await layout.confirm_nondefault_locktime(ctx, req.lock_time) + res = await layout.confirm_nondefault_locktime( + ctx, req.lock_time, req.lock_time_disabled + ) progress.report_init() elif isinstance(req, helpers.UiConfirmForeignAddress): res = await paths.show_path_warning(ctx, req.address_n) diff --git a/core/src/apps/bitcoin/sign_tx/approvers.py b/core/src/apps/bitcoin/sign_tx/approvers.py index eacc2fe05..eeb25bba8 100644 --- a/core/src/apps/bitcoin/sign_tx/approvers.py +++ b/core/src/apps/bitcoin/sign_tx/approvers.py @@ -15,6 +15,9 @@ from . import helpers, tx_weight if False: from ..authorization import CoinJoinAuthorization +# Setting nSequence to this value for every input in a transaction disables nLockTime. +_SEQUENCE_FINAL = const(0xFFFFFFFF) + # An Approver object computes the transaction totals and either prompts the user # to confirm transaction parameters (output addresses, amounts and fees) or uses @@ -25,6 +28,7 @@ class Approver: self.tx = tx self.coin = coin self.weight = tx_weight.TxWeightCalculator(tx.inputs_count, tx.outputs_count) + self.min_sequence = _SEQUENCE_FINAL # the minimum nSequence of all inputs # amounts self.total_in = 0 # sum of input amounts @@ -35,11 +39,13 @@ class Approver: async def add_internal_input(self, txi: TxInputType, amount: int) -> None: self.weight.add_input(txi) self.total_in += amount + self.min_sequence = min(self.min_sequence, txi.sequence) def add_external_input(self, txi: TxInputType) -> None: self.weight.add_input(txi) self.total_in += txi.amount self.external_in += txi.amount + self.min_sequence = min(self.min_sequence, txi.sequence) def add_change_output(self, txo: TxOutputType, script_pubkey: bytes) -> None: self.weight.add_output(script_pubkey) @@ -100,7 +106,10 @@ class BasicApprover(Approver): if self.change_count > self.MAX_SILENT_CHANGE_COUNT: await helpers.confirm_change_count_over_threshold(self.change_count) if self.tx.lock_time > 0: - await helpers.confirm_nondefault_locktime(self.tx.lock_time) + lock_time_disabled = self.min_sequence == _SEQUENCE_FINAL + await helpers.confirm_nondefault_locktime( + self.tx.lock_time, lock_time_disabled + ) if not self.external_in: await helpers.confirm_total(total, fee, self.coin) else: diff --git a/core/src/apps/bitcoin/sign_tx/helpers.py b/core/src/apps/bitcoin/sign_tx/helpers.py index 2d8e593d0..124b620fb 100644 --- a/core/src/apps/bitcoin/sign_tx/helpers.py +++ b/core/src/apps/bitcoin/sign_tx/helpers.py @@ -76,8 +76,9 @@ class UiConfirmForeignAddress: class UiConfirmNonDefaultLocktime: - def __init__(self, lock_time: int): + def __init__(self, lock_time: int, lock_time_disabled): self.lock_time = lock_time + self.lock_time_disabled = lock_time_disabled __eq__ = utils.obj_eq @@ -106,8 +107,8 @@ def confirm_foreign_address(address_n: list) -> Awaitable[Any]: # type: ignore return (yield UiConfirmForeignAddress(address_n)) -def confirm_nondefault_locktime(lock_time: int) -> Awaitable[Any]: # type: ignore - return (yield UiConfirmNonDefaultLocktime(lock_time)) +def confirm_nondefault_locktime(lock_time: int, lock_time_disabled: bool) -> Awaitable[Any]: # type: ignore + return (yield UiConfirmNonDefaultLocktime(lock_time, lock_time_disabled)) def request_tx_meta(tx_req: TxRequest, coin: CoinInfo, tx_hash: bytes = None) -> Awaitable[Any]: # type: ignore diff --git a/core/src/apps/bitcoin/sign_tx/layout.py b/core/src/apps/bitcoin/sign_tx/layout.py index dc00a6627..5b9530ce1 100644 --- a/core/src/apps/bitcoin/sign_tx/layout.py +++ b/core/src/apps/bitcoin/sign_tx/layout.py @@ -101,13 +101,21 @@ async def confirm_change_count_over_threshold( await require_confirm(ctx, text, ButtonRequestType.SignTx) -async def confirm_nondefault_locktime(ctx: wire.Context, lock_time: int) -> None: - text = Text("Confirm locktime", ui.ICON_SEND, ui.GREEN) - text.normal("Locktime for this transaction is set to") - if lock_time < _LOCKTIME_TIMESTAMP_MIN_VALUE: - text.normal("blockheight:") +async def confirm_nondefault_locktime( + ctx: wire.Context, lock_time: int, lock_time_disabled: bool +) -> None: + if lock_time_disabled: + text = Text("Warning", ui.ICON_SEND, ui.GREEN) + text.normal("Locktime is set but will", "have no effect.") + text.br_half() else: - text.normal("timestamp:") - text.bold(str(lock_time)) + text = Text("Confirm locktime", ui.ICON_SEND, ui.GREEN) + text.normal("Locktime for this", "transaction is set to") + if lock_time < _LOCKTIME_TIMESTAMP_MIN_VALUE: + text.normal("blockheight:") + else: + text.normal("timestamp:") + text.bold(str(lock_time)) + text.normal("Continue?") await require_confirm(ctx, text, ButtonRequestType.SignTx) diff --git a/core/tests/test_apps.bitcoin.approver.py b/core/tests/test_apps.bitcoin.approver.py index 7fec4a4bd..30cf76b9a 100644 --- a/core/tests/test_apps.bitcoin.approver.py +++ b/core/tests/test_apps.bitcoin.approver.py @@ -35,6 +35,7 @@ class TestApprover(unittest.TestCase): TxInputType( amount=denomination + 1000000 * (i + 1), script_type=InputScriptType.EXTERNAL, + sequence=0xffffffff, ) for i in range(99) ] @@ -45,6 +46,7 @@ class TestApprover(unittest.TestCase): address_n=[H_(84), H_(0), H_(0), 0, 1], amount=denomination + 1000000, script_type=InputScriptType.SPENDWITNESS, + sequence=0xffffffff, ) ) diff --git a/core/tests/test_apps.bitcoin.segwit.signtx.native_p2wpkh_grs.py b/core/tests/test_apps.bitcoin.segwit.signtx.native_p2wpkh_grs.py index c0d05629a..80715244b 100644 --- a/core/tests/test_apps.bitcoin.segwit.signtx.native_p2wpkh_grs.py +++ b/core/tests/test_apps.bitcoin.segwit.signtx.native_p2wpkh_grs.py @@ -103,7 +103,7 @@ class TestSignSegwitTxNativeP2WPKH_GRS(unittest.TestCase): helpers.UiConfirmOutput(out2, coin), True, - helpers.UiConfirmNonDefaultLocktime(tx.lock_time), + helpers.UiConfirmNonDefaultLocktime(tx.lock_time, lock_time_disabled=False), True, helpers.UiConfirmTotal(12300000, 11000, coin), @@ -225,7 +225,7 @@ class TestSignSegwitTxNativeP2WPKH_GRS(unittest.TestCase): TxRequest(request_type=TXOUTPUT, details=TxRequestDetailsType(request_index=1, tx_hash=None), serialized=EMPTY_SERIALIZED), TxAck(tx=TransactionType(outputs=[out2])), - helpers.UiConfirmNonDefaultLocktime(tx.lock_time), + helpers.UiConfirmNonDefaultLocktime(tx.lock_time, lock_time_disabled=False), True, helpers.UiConfirmTotal(5000000 + 11000, 11000, coin), diff --git a/core/tests/test_apps.bitcoin.segwit.signtx.p2wpkh_in_p2sh_grs.py b/core/tests/test_apps.bitcoin.segwit.signtx.p2wpkh_in_p2sh_grs.py index d24482968..91a8f4947 100644 --- a/core/tests/test_apps.bitcoin.segwit.signtx.p2wpkh_in_p2sh_grs.py +++ b/core/tests/test_apps.bitcoin.segwit.signtx.p2wpkh_in_p2sh_grs.py @@ -103,7 +103,7 @@ class TestSignSegwitTxP2WPKHInP2SH_GRS(unittest.TestCase): helpers.UiConfirmOutput(out2, coin), True, - helpers.UiConfirmNonDefaultLocktime(tx.lock_time), + helpers.UiConfirmNonDefaultLocktime(tx.lock_time, lock_time_disabled=False), True, helpers.UiConfirmTotal(123445789 + 11000, 11000, coin), @@ -225,7 +225,7 @@ class TestSignSegwitTxP2WPKHInP2SH_GRS(unittest.TestCase): TxRequest(request_type=TXOUTPUT, details=TxRequestDetailsType(request_index=1, tx_hash=None), serialized=EMPTY_SERIALIZED), TxAck(tx=TransactionType(outputs=[out2])), - helpers.UiConfirmNonDefaultLocktime(tx.lock_time), + helpers.UiConfirmNonDefaultLocktime(tx.lock_time, lock_time_disabled=False), True, helpers.UiConfirmTotal(12300000 + 11000, 11000, coin), diff --git a/tests/ui_tests/fixtures.json b/tests/ui_tests/fixtures.json index 5c6c0241e..8f56c7c0f 100644 --- a/tests/ui_tests/fixtures.json +++ b/tests/ui_tests/fixtures.json @@ -332,14 +332,14 @@ "test_msg_signtx_external.py::test_p2wsh_external_presigned": "8374d50b803db0160de39ce7e5a4170112f4c99d703490920a2de735bd261bda", "test_msg_signtx_grs.py-test_legacy": "3a80ea724a93ed225d64f8def739d63b11f8c096455f971feabec8be6f7597fb", "test_msg_signtx_grs.py-test_legacy_change": "8dfc140534bdaa08f6916831dc0d510f57b07617f30df748e4e0456d4dd93ece", -"test_msg_signtx_grs.py-test_send_segwit_native": "a1098f71dd6c197cb8a4a45806808fbacc130044585cb003e61110d8bda80114", -"test_msg_signtx_grs.py-test_send_segwit_native_change": "2b9119aea79b50a26bd86cc49fd5421aedee42245b83919bf142d2ca2e50dd16", -"test_msg_signtx_grs.py-test_send_segwit_p2sh": "e596ff7f2e10633d8de6bd5b98928020dfacf578ce71d6d6e0ca27011ed8bd53", -"test_msg_signtx_grs.py-test_send_segwit_p2sh_change": "65a33cc9ecf52bdab93d1066bfaff7f30c9908fc5ada8a672c39df57eb6129c2", "test_msg_signtx_invalid_path.py-test_invalid_path_fail": "b0f22cba2dbab2cd21c15c002b66ed89b6c728b10daa8d0c0e78abd4164a3912", "test_msg_signtx_invalid_path.py-test_invalid_path_pass_forkid": "667dcb09b569e5b4e091e6b1ac7e8e057c0c730c931b22f8c0ee64050f3f467b", -"test_msg_signtx_komodo.py-test_one_one_fee_sapling": "aa32424c04f65c0e2de57f6eb26830eb037c7b4daaf300ae61d30968affd9193", -"test_msg_signtx_komodo.py-test_one_one_rewards_claim": "e53f221fda81469027e39e21877a81a8fafbffbece0a45aeda12aae8873b0464", +"test_msg_signtx_grs.py-test_send_segwit_native": "82dfa15178d33e757da58943aff36dcc0eebb984e34832b71f6ca09b2a525cbc", +"test_msg_signtx_grs.py-test_send_segwit_native_change": "d8ae74de3aada1d136c4119f2306a63bd109901ce15d00ae916ba5b4457e798e", +"test_msg_signtx_grs.py-test_send_segwit_p2sh": "9ab885dd3b390813f8a47e1d1587abe07ab713e9f8696dc667b3a2925f23c103", +"test_msg_signtx_grs.py-test_send_segwit_p2sh_change": "6c352ab975a75a150f7c3415a967fb8635395ff8db0de89ecb9c2011cb519509", +"test_msg_signtx_komodo.py-test_one_one_fee_sapling": "14bad8852ee51f6fec12677cced9ffafa0cbae91b4ba94e988a800544072ed21", +"test_msg_signtx_komodo.py-test_one_one_rewards_claim": "751e83d63bf01c6c57047b5e004629d613df75342371cd43a7b4b80a07f4b88d", "test_msg_signtx_peercoin.py::test_timestamp_included": "825b9bdf5238c5c6415a254a6bae4b2bd9df8fc5cb31f66f0c20145cb4e60bbb", "test_msg_signtx_segwit.py-test_attack_change_input_address": "5ae71202c062ef7942626a80a4ceeb8d8c4ea5065a97f0de6a97505e9cb82c2c", "test_msg_signtx_segwit.py-test_attack_mixed_inputs": "ed4cf8ff26ca1abb0adf20e1020dad7861b5e63ead2a1a9c0c5e9080d37f6f1c",