diff --git a/electrum/lnpeer.py b/electrum/lnpeer.py index 96855e45d..62833ac00 100644 --- a/electrum/lnpeer.py +++ b/electrum/lnpeer.py @@ -1625,6 +1625,9 @@ class Peer(Logger): except Exception: raise OnionRoutingFailure(code=OnionFailureCode.INVALID_ONION_PAYLOAD, data=b'\x00\x00\x00') if htlc.cltv_expiry - next_cltv_expiry < next_chan.forwarding_cltv_expiry_delta: + log_fail_reason( + f"INCORRECT_CLTV_EXPIRY. " + f"{htlc.cltv_expiry=} - {next_cltv_expiry=} < {next_chan.forwarding_cltv_expiry_delta=}") data = htlc.cltv_expiry.to_bytes(4, byteorder="big") + outgoing_chan_upd_message raise OnionRoutingFailure(code=OnionFailureCode.INCORRECT_CLTV_EXPIRY, data=data) if htlc.cltv_expiry - lnutil.MIN_FINAL_CLTV_EXPIRY_ACCEPTED <= local_height \ @@ -1639,6 +1642,9 @@ class Peer(Logger): if htlc.amount_msat - next_amount_msat_htlc < forwarding_fees: data = next_amount_msat_htlc.to_bytes(8, byteorder="big") + outgoing_chan_upd_message raise OnionRoutingFailure(code=OnionFailureCode.FEE_INSUFFICIENT, data=data) + if self._maybe_refuse_to_forward_htlc_that_corresponds_to_payreq_we_created(htlc.payment_hash): + log_fail_reason(f"RHASH corresponds to payreq we created") + raise OnionRoutingFailure(code=OnionFailureCode.TEMPORARY_NODE_FAILURE, data=b'') self.logger.info( f"maybe_forward_htlc. will forward HTLC: inc_chan={incoming_chan.short_channel_id}. inc_htlc={str(htlc)}. " f"next_chan={next_chan.get_id_for_log()}.") @@ -1707,6 +1713,12 @@ class Peer(Logger): self.logger.exception('') raise OnionRoutingFailure(code=OnionFailureCode.INVALID_ONION_PAYLOAD, data=b'\x00\x00\x00') + if self._maybe_refuse_to_forward_htlc_that_corresponds_to_payreq_we_created(payment_hash): + self.logger.debug( + f"maybe_forward_trampoline. will FAIL HTLC(s). " + f"RHASH corresponds to payreq we created. {payment_hash.hex()=}") + raise OnionRoutingFailure(code=OnionFailureCode.TEMPORARY_NODE_FAILURE, data=b'') + # these are the fee/cltv paid by the sender # pay_to_node will raise if they are not sufficient trampoline_cltv_delta = cltv_expiry - cltv_from_onion @@ -1733,6 +1745,23 @@ class Peer(Logger): # FIXME: adapt the error code raise OnionRoutingFailure(code=OnionFailureCode.UNKNOWN_NEXT_PEER, data=b'') + def _maybe_refuse_to_forward_htlc_that_corresponds_to_payreq_we_created(self, payment_hash: bytes) -> bool: + """Returns True if the HTLC should be failed. + We must not forward HTLCs with a matching payment_hash to a payment request we created. + Example attack: + - Bob creates payment request with HASH1, for 1 BTC; and gives the payreq to Alice + - Alice sends htlc A->B->C, for 1 sat, with HASH1 + - Bob must not release the preimage of HASH1 + """ + payment_info = self.lnworker.get_payment_info(payment_hash) + is_our_payreq = payment_info and payment_info.direction == RECEIVED + # note: If we don't have the preimage for a payment request, then it must be a hold invoice. + # Hold invoices are created by other parties (e.g. a counterparty initiating a submarine swap), + # and it is the other party choosing the payment_hash. If we failed HTLCs with payment_hashes colliding + # with hold invoices, then a party that can make us save a hold invoice for an arbitrary hash could + # also make us fail arbitrary HTLCs. + return bool(is_our_payreq and self.lnworker.get_preimage(payment_hash)) + def maybe_fulfill_htlc( self, *, chan: Channel, diff --git a/electrum/tests/test_lnpeer.py b/electrum/tests/test_lnpeer.py index 4a6cd785a..ad072fe44 100644 --- a/electrum/tests/test_lnpeer.py +++ b/electrum/tests/test_lnpeer.py @@ -538,12 +538,17 @@ class TestPeer(ElectrumTestCase): *, amount_msat=100_000_000, include_routing_hints=False, + payment_preimage: bytes = None, + payment_hash: bytes = None, ) -> Tuple[LnAddr, str]: amount_btc = amount_msat/Decimal(COIN*1000) - payment_preimage = os.urandom(32) - RHASH = sha256(payment_preimage) - info = PaymentInfo(RHASH, amount_msat, RECEIVED, PR_UNPAID) - w2.save_preimage(RHASH, payment_preimage) + if payment_preimage is None and not payment_hash: + payment_preimage = os.urandom(32) + if payment_hash is None: + payment_hash = sha256(payment_preimage) + info = PaymentInfo(payment_hash, amount_msat, RECEIVED, PR_UNPAID) + if payment_preimage: + w2.save_preimage(payment_hash, payment_preimage) w2.save_payment_info(info) if include_routing_hints: routing_hints, trampoline_hints = w2.calc_routing_hints_for_invoice(amount_msat) @@ -552,11 +557,11 @@ class TestPeer(ElectrumTestCase): trampoline_hints = [] invoice_features = w2.features.for_invoice() if invoice_features.supports(LnFeatures.PAYMENT_SECRET_OPT): - payment_secret = w2.get_payment_secret(RHASH) + payment_secret = w2.get_payment_secret(payment_hash) else: payment_secret = None lnaddr1 = LnAddr( - paymenthash=RHASH, + paymenthash=payment_hash, amount=amount_btc, tags=[('c', lnutil.MIN_FINAL_CLTV_EXPIRY_FOR_INVOICE), ('d', 'coffee'), @@ -1046,6 +1051,57 @@ class TestPeer(ElectrumTestCase): with self.assertRaises(PaymentDone): await f() + async def test_refuse_to_forward_htlc_that_corresponds_to_payreq_we_created(self): + # This test checks that the following attack does not work: + # - Bob creates payment request with HASH1, for 1 BTC; and gives the payreq to Alice + # - Alice sends htlc A->B->D, for 100k sat, with HASH1 + # - Bob must not release the preimage of HASH1 + graph_def = self.GRAPH_DEFINITIONS['square_graph'] + graph_def.pop('carol') + graph_def['alice']['channels'].pop('carol') + # now graph is linear: A <-> B <-> D + graph = self.prepare_chans_and_peers_in_graph(graph_def) + peers = graph.peers.values() + async def pay(): + lnaddr1, pay_req1 = self.prepare_invoice( + graph.workers['bob'], + amount_msat=100_000_000_000, + ) + lnaddr2, pay_req2 = self.prepare_invoice( + graph.workers['dave'], + amount_msat=100_000_000, + payment_hash=lnaddr1.paymenthash, # Dave is cooperating with Alice, and he reuses Bob's hash + include_routing_hints=True, + ) + with self.subTest(msg="try to make Bob forward in legacy (non-trampoline) mode"): + result, log = await graph.workers['alice'].pay_invoice(pay_req2, attempts=1) + self.assertFalse(result) + self.assertEqual(OnionFailureCode.TEMPORARY_NODE_FAILURE, log[0].failure_msg.code) + self.assertEqual(None, graph.workers['alice'].get_preimage(lnaddr1.paymenthash)) + with self.subTest(msg="try to make Bob forward in trampoline mode"): + # declare Bob as trampoline forwarding node + electrum.trampoline._TRAMPOLINE_NODES_UNITTESTS = { + graph.workers['bob'].name: LNPeerAddr(host="127.0.0.1", port=9735, pubkey=graph.workers['bob'].node_keypair.pubkey), + } + await self._activate_trampoline(graph.workers['alice']) + result, log = await graph.workers['alice'].pay_invoice(pay_req2, attempts=5) + self.assertFalse(result) + self.assertEqual(OnionFailureCode.TEMPORARY_NODE_FAILURE, log[0].failure_msg.code) + self.assertEqual(None, graph.workers['alice'].get_preimage(lnaddr1.paymenthash)) + raise SuccessfulTest() + + async def f(): + async with OldTaskGroup() as group: + for peer in peers: + await group.spawn(peer._message_loop()) + await group.spawn(peer.htlc_switch()) + for peer in peers: + await peer.initialized + await group.spawn(pay()) + + with self.assertRaises(SuccessfulTest): + await f() + @needs_test_with_all_chacha20_implementations async def test_payment_with_temp_channel_failure_and_liquidity_hints(self): # prepare channels such that a temporary channel failure happens at c->d