lnpeer: harden _check_unfulfilled_htlc - #10899
Conversation
eab727a to
8940aef
Compare
ecdsa
changed the title
d4efb6b to
7ccce15
Compare
7ccce15 to
d0fd39b
Compare
c93a565 to
f999bd5
Compare
| # Payment key creation: | ||
| # * for regular forwarded htlcs -> "scid.hex() + ':%d' % htlc_id" [htlc key] | ||
| # * for trampoline forwarding -> "payment hash + payment secret from outer onion" | ||
| # * for trampoline forwarding -> "fwd:" + (payment hash + payment secret from outer onion).hex() | ||
| # * for final non-trampoline htlcs (we are receiver) -> "payment hash + payment secret from onion" | ||
| # * for final trampoline htlcs (we are receiver) -> 2. step grouping: | ||
| # 1. grouping of htlcs by "payments hash + outer onion payment secret", a 'multi-trampoline mpp part'. |
There was a problem hiding this comment.
Ideally we should introduce full domain-separation, so each category would have its own namespace.
I understand it is simpler not to touch the non-forwarding categories as that would definitely require a wallet DB upgrade -- whereas for forwarding, we can skip DB upgrades atm.
At the very least, please choose a prefix for "trampoline forwarding" that makes it clear it is for trampoline forwarding. E.g. fwd_t.
We could also add a prefix to regular forwarding now, e.g. fwd_r.
There was a problem hiding this comment.
note: I now realize that this namespace separation does not prevent the attack we discussed. it is certainly cleaner, but I have yet to find a scenario where it would actually prevent something.
There was a problem hiding this comment.
right, the bug has nothing to do with payment_keys colliding. It is a timing race. see #10899 (comment)
f999bd5 to
396799a
Compare
Apply the comparison of BOLT4 L547-L553 to forwarding requests too, not only if we are the final recipient: the difference between the two onions is the fee/cltv budget we are given (see _maybe_forward_trampoline). Also tolerate an inner payload with missing fields. Collapse the mpp and non-mpp cases while here: they are the same comparison, as a sender not using mpp must set total_msat equal to amt_to_forward (BOLT4 L406).
We must fail a HTLC to forward if its payment_hash matches one of our invoices. Instead of allowing the HTLC to enter a MPP set, we fail it before, in _check_unfulfilled_htlc. Also rewrite test_refuse_to_forward_htlc_that_corresponds_to_payreq_we_created: The new test uses send_trampoline_htlc_to_forward and only two peers, so it belongs in the TestPeerDirect class. The new test is logically equivalent and 4~5 times faster.
This prevents collisions with payment_keys to an invoice of ours. Note that this is not defense-in-depth: this commit does NOT prevent a node from releasing the preimage, if it receives a tiny trampoline HTLC to forward for one of its own invoices. Indeed, the namespaced bucket would still enter the SETTLING state. What prevents the attack is the previous commit, and only that.
396799a to
4af12fd
Compare
SomberNight
left a comment
SomberNight
left a comment
There was a problem hiding this comment.
I merged the first commit into master in ddd22c8.
| async def test_refuse_to_forward_htlc_that_corresponds_to_payreq_we_created(self): | ||
| """Alice holds an invoice created by Bob, hence she knows RHASH and Bob's payment_secret. | ||
| She sends Bob a dust htlc whose outer onion is addressed to Bob (using the invoice's | ||
| payment_secret, and claiming a tiny total_msat), but whose inner trampoline onion asks Bob | ||
| to *forward* the payment. | ||
| Bob must fail such htlcs, and he must not release the preimage. | ||
| """ |
There was a problem hiding this comment.
I am confused about this test.
The commit message says:
Also rewrite test_refuse_to_forward_htlc_that_corresponds_to_payreq_we_created:
The new test uses send_trampoline_htlc_to_forward and
only two peers, so it belongs in the TestPeerDirect class.
The new test is logically equivalent and 4~5 times faster.
Without the new check in lnpeer.py, the new test fails but the old test passes.
With that, how could the two tests be logically equivalent?
There was a problem hiding this comment.
First I thought that the difference between the two tests is that the old one has the attacker send an htlc with matching paymenthash but different payment_secret, whereas in the new test both paymenthash and payment_secret match.
but now I think the whole payment_secret stuff and the combined payment_key colliding is a complete red herring.
The meaningful difference between the old test and the new test is the timing!
Try with this diff for the old test, now it will fail as the preimage gets released:
diff --git a/tests/test_lnpeer.py b/tests/test_lnpeer.py
index db86707637..e78afc4429 100644
--- a/tests/test_lnpeer.py
+++ b/tests/test_lnpeer.py
@@ -2123,6 +2123,14 @@ class TestPeerForwarding(TestPeer):
# now graph is linear: A <-> B <-> D
graph = self.prepare_chans_and_peers_in_graph(graph_def)
peers = graph.peers.values()
+
+ w2 = graph.workers['bob']
+ orig_maybe_forward_htlc_set = w2.maybe_forward_htlc_set
+ async def maybe_forward_htlc_set(*args, **kwargs):
+ await asyncio.sleep(0.15)
+ return await orig_maybe_forward_htlc_set(*args, **kwargs)
+ w2.maybe_forward_htlc_set = maybe_forward_htlc_set
+
async def pay():
lnaddr1, pay_req1 = self.prepare_invoice(
graph.workers['bob'],
2 participants
see commits