-
Notifications
You must be signed in to change notification settings - Fork 493
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
BOLT 4: Merge final_expiry_too_soon into incorrect_or_unknown_payment #608
BOLT 4: Merge final_expiry_too_soon into incorrect_or_unknown_payment #608
Conversation
Thinking about this more, I am not sure what the use is of the We provide these values to supply more information to the original sender of the payment. These values are not meant to help a 'prober'. The original sender of the payment already included the amount and expiry that the receiver should expect in the onion payload for the final hop. If those don't match the htlc that is received, either Doesn't this mean the the parameters of |
I don't understand that comment, could you elaborate? These two values ( For clarity, let's look at the following scenario:
where D sends an That means that either C or D is malicious. C might be sending an invalid HTLC to D to probe its position in the payment path. But D could be malicious as well and send back an error even if the HTLC was valid (D would be sacrificing fees when doing that). A can't distinguish between these two cases. So I agree that |
A prober in this context is an intermediate node that initiates a new payment with the same hash to several nodes in the network. The goal is to find out which one is the recipient of the original payment. In this case the values can be read by the prober, because it initiated a new (probe) payment. I think the original idea of including details in It seems that my conclusion is the same as yours: the details in |
But those values don't help the prober at all, do they?
If the sender trusts the erroring node, the sender does learn something new. In the But as we both point out, A has no way to really trust these values so maybe they're not useful at all...if someone with more historic background on those wants to chime in, maybe there's something we're missing? |
No, they don't help the prober. It is just what they specified themselves.
In the case that C sends a different value to D than what was specified in the onion payload for D, D will return a different failure: |
bc72592
to
f517e88
Compare
Updated PR to remove |
ACK |
One problem that this change introduces is that the sender can no longer distinguish between building a route with an incorrect final ctlv expiry (because it uses the wrong payment details) from an intermediate node along the path delaying the payment so that the (correctly calculated) cltv delta is no longer big enough when the htlc gets to the receiver. Knowing which of the two situations we are in determines whether we should try to pay via another route (that doesn't contain the delaying node) or that we can give up because we don't have the correct payment details. A possible solution could be to add a |
fe607f2
to
0473851
Compare
@t-bast added height parameter in fixup commit to address problem of determining whether a failure is terminal or not. |
ACK Note that in the case where an intermediate node delayed the payment, we have no way of knowing which one did and we might need to "soft-blacklist" all nodes in the route including the recipient... |
Yes, that is unfortunate indeed. I posted to the ML today with some of the ideas that have been developed. https://lists.linuxfoundation.org/pipermail/lightning-dev/2019-June/002015.html |
0473851
to
5da97a9
Compare
@rustyrussell we came to a tentative ack in yesterday's irc meeting on this pr. What is your opinion on these changes? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🐲
23be50c
to
15d81e3
Compare
Rebased after merge of explicit data types. |
I think this is a useful change because providing the But I think this doesn't do much against probing attacks. Correct me if you had something else in mind, but probing is only useful if you are the next to last node because you de-anonymize the recipient. But even with your change, it's really easy to probe for that information: instead of forwarding the HTLC the normal way, you forward it by creating a one-hop payment to the next node (with the values you were supposed to forward). If the next node fulfills the HTLC, you know they are the recipient. I think we can't fix probing attacks by the next-to-last node until we have scriptless scripts / anonymous multi-hop locks. |
…_details In commit 914ebab the incorrect_payment_amount error was merged into incorrect_or_unknown_payment_details to prevent a probing attack that allowed intermediate nodes to learn the final destination of a payment. A similar attack is possible using the htlc expiry value. By trying payments with the correct amount but low expiry values to candidate destinations, an incorrect_or_unknown_payment_details error can be elicited. This would signal that the probe payment was sent to the final destination. For the intermediate node to determine the correct amount, an estimate must be calculated. A logical choice would be the outgoing amount of the intermediate node plus some allowance for routing fees that would otherwise be paid to subsequent nodes along the path. Picking a low enough - but not too low - expiry value is more tricky. A reasonable guess could be made using external knowledge of the final destination's implementation defaults or the type of invoice that is paid to. Especially in the case of an hodl invoice that typically has a large expiry delta, it is easier to make a correct guess. This form of attack is arguably harder to realize than the amount probe that was previously possible. The attacker may accidentally pay the invoice if the expiry value guess satisfies the invoice final cltv requirement. In that case, the attacker still has the incoming htlc to pull which limits the loss.
15d81e3
to
745d485
Compare
The intent of the change is to fix probing attacks by other nodes than the next-to-last node. So suppose the route is A->B->C->D and B wants to figure out where A's money is going. B can then, instead of forwarding to C, craft new routes from B to a set of potential destinations and intentionally give them a too low final cltv expiry. By observing whether a |
Got it. It does seem a bit far-fetched honestly (Bob would need to know he's the first hop and would need to already have guesses for the destination), but it doesn't hurt to fix it. |
It is similar to #516, just a different parameter
Yes indeed, it helps decide whether to give up on the payment (because we are using the wrong details) or keep trying with different routes. Error attribution on this part is poor, but even by penalizing all nodes, a working route may be found. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I'm a little confused by this - how can someone identify a payment differently based on the CLTV? Is this something that was specific to LND checking if an HTLC is known before checking the CLTV is sane? Couldn't it instead have been solved by reordering the checks. |
Also, as pointed out at 11f6017 this adds a lot of complexity to error handling. |
Have you re-read this comment: #608 (comment)? Or is there something that you think doesn't work in this attack scenario? |
However, this type of probing wouldn't work anymore now that we have a |
Right, that comment specifically requires that the recipient responds with a hash error before checking the CLTV value. This can be much more trivially fixed by just checking the CLTV value before checking if the payment hash matches a known payment hash. |
That would work only if all nodes use the same I agree, it's a bit far-fetched and is in practice hard to do for the attacker, and on top of that |
But that doesn't tell you anything except that Carol has a different final_cltv_expiry_delta than Alice and Bob, it doesn't reveal whether the specific payment had anything to do with Alice, Carol, or Bob. That said, I think maybe the reason lnd had this issue must be because they have different final_cltv_expiry_deltas per payment. ie if you give out different cltv_expiry values in different invoices, you'd need to look up the invoice before you can accept or reject something based on its cltv_delta. That said, I'm not sure if they need to, or if that was just a side-effect of the way their payment flow was built - maybe @cfromknecht can enlighten us a bit on his ACK? |
That's right, it doesn't tell you for sure that Carol is the final recipient, only that Alice and Bob are not the final recipient, good point. |
@TheBlueMatt This is indeed the case, for lnd each invoice has an optionally-configurable CLTV delta that first must be looked up in order to know if the HTLC's timelock satisfies the invoice's CLTV. Before this change, a prober could send payments to nodes with the correct payment hash but invalid timelock and locate the destination as the node that returns If i understand your suggestion, that would imply implementations necessarily have to use a static final CLTV delta? |
Correct me if I'm wrong, but lnd now requires payment_secrets in received payments. Given that, can we revert this now? Indeed, it seems like you could not otherwise protect privacy, but now you can simply check that the payment_secret matches before checking the CLTV. |
With In fact, As these parameters were optional to begin with, it seems safe to not send them. But can we remove them from the spec? Suppose we'd want to extend this message with another parameter in the future, we can't re-use the same space. There might be a node implementation out there that still writes the amount and height in there, which will then be misinterpreted by newer nodes. Perhaps the bytes have to be marked as 'reserved'? |
This replaces `final_expiry_too_soon` with `incorrect_or_unknown_payment` as was done in lightning/bolts#608. Note that the rationale for this (that it may expose whether you are the final recipient for the payment or not) does not currently apply to us - we don't apply different final CLTV values to different payments. However, we might in the future, and this will make us slightly more consistent with other nodes.
This replaces `final_expiry_too_soon` with `incorrect_or_unknown_payment` as was done in lightning/bolts#608. Note that the rationale for this (that it may expose whether you are the final recipient for the payment or not) does not currently apply to us - we don't apply different final CLTV values to different payments. However, we might in the future, and this will make us slightly more consistent with other nodes.
This replaces `final_expiry_too_soon` with `incorrect_or_unknown_payment` as was done in lightning/bolts#608. Note that the rationale for this (that it may expose whether you are the final recipient for the payment or not) does not currently apply to us - we don't apply different final CLTV values to different payments. However, we might in the future, and this will make us slightly more consistent with other nodes.
This replaces `final_expiry_too_soon` with `incorrect_or_unknown_payment` as was done in lightning/bolts#608. Note that the rationale for this (that it may expose whether you are the final recipient for the payment or not) does not currently apply to us - we don't apply different final CLTV values to different payments. However, we might in the future, and this will make us slightly more consistent with other nodes.
This replaces `final_expiry_too_soon` with `incorrect_or_unknown_payment` as was done in lightning/bolts#608. Note that the rationale for this (that it may expose whether you are the final recipient for the payment or not) does not currently apply to us - we don't apply different final CLTV values to different payments. However, we might in the future, and this will make us slightly more consistent with other nodes.
In commit 914ebab the incorrect_payment_amount error was merged into incorrect_or_unknown_payment_details to prevent a probing attack that allowed intermediate nodes to learn the final destination of a payment.
A similar attack is possible using the htlc expiry value. By trying payments with the correct amount but low expiry values to candidate destinations, an incorrect_or_unknown_payment_details error can be
elicited. This would signal that the probe payment was sent to the final destination.
For the intermediate node to determine the correct amount, an estimate must be calculated. A logical choice would be the outgoing amount of the intermediate node plus some allowance for routing fees that would otherwise be paid to subsequent nodes along the path.
Picking a low enough - but not too low - expiry value is more tricky. A reasonable guess could be made using external knowledge of the final destination's implementation defaults or the type of invoice that
is paid to. Especially in the case of an hodl invoice that typically has a large expiry delta, it is easier to make a correct guess.
This form of attack is arguably harder to realize than the amount probe that was previously possible. The attacker may accidentally pay the invoice if the expiry value guess satisfies the invoice final cltv requirement. In that case, the attacker still has the incoming htlc to pull which limits the loss.