Skip to content
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

Merged

Conversation

@joostjager
Copy link
Contributor

commented May 13, 2019

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.

@joostjager joostjager changed the title BOLT 4: Merge final_expiry_too_soon into incorrect_or_unknown_payment… BOLT 4: Merge final_expiry_too_soon into incorrect_or_unknown_payment May 13, 2019

@joostjager

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

Thinking about this more, I am not sure what the use is of the htlc_msat and the newly proposed cltv_expiry in the incorrect_or_unknown_payment_details message.

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 final_incorrect_cltv_expiry or final_incorrect_htlc_amount is returned.

Doesn't this mean the the parameters of incorrect_or_unknown_payment_details never provide useful information to the sender and that they could just as well be removed?

@t-bast

This comment has been minimized.

Copy link
Collaborator

commented Jun 11, 2019

We provide these values to supply more information to the original sender of the payment. These values are not meant to help a 'prober'.

I don't understand that comment, could you elaborate? These two values (htlc_msat and cltv_expiry) correspond to the values the erroring node received in update_add_htlc, but they can't be read by a prober since they are encrypted for the sender on the way back.

For clarity, let's look at the following scenario:

A -> B -> C -> D

where D sends an incorrect_or_unknown_payment_details error.

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 htlc_msat and cltv_expiry cannot be trusted since either C or D could have controlled them. I think that these values are included only to help troubleshoot a potential bug in a node's implementation. Since they can't be read by intermediate nodes, it doesn't really hurt to include them in the response, does it?

@joostjager

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

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 incorrect_or_unknown_payment_details is for the sender to find out what the real problem is. Whether it is unknown payment hash, incorrect amount or final cltv expiry too soon. But as we just report back the values of the htlc, the sender won't learn anything new. It isn't even desirable for the sender to receive the expected values (rather than the htlc values), as this is what enables probing.

It seems that my conclusion is the same as yours: the details in incorrect_or_unknown_payment_details are just for debugging. I am not sure if this is something we should want, as it may allow for node fingerprinting (for example if there is an implementation that always returns 0 for one or the other parameter) and uses up space. Space is not a concern in the current fixed length failure message, but it may be if that would change in the future.

@t-bast

This comment has been minimized.

Copy link
Collaborator

commented Jun 11, 2019

In this case the values can be read by the prober, because it initiated a new (probe) payment.

But those values don't help the prober at all, do they?

But as we just report back the values of the htlc, the sender won't learn anything new.

If the sender trusts the erroring node, the sender does learn something new. In the A -> B -> C -> D example the sender learns what htlc values C sent to D (if A trusts D to report the actual values sent by C). This could in theory help the sender figure out a misconfiguration issue on C's side.

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?

@joostjager

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

But those values don't help the prober at all, do they?

No, they don't help the prober. It is just what they specified themselves.

If the sender trusts the erroring node, the sender does learn something new. In the A -> B -> C -> D example the sender learns what htlc values C sent to D

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: final_incorrect_htlc_amount. So to detect that case, we don't need the incorrect_or_unknown_payment_details (parameter).

@joostjager joostjager force-pushed the joostjager:remove-final-expiry-too-soon branch 2 times, most recently from bc72592 to f517e88 Jun 12, 2019

@joostjager

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

Updated PR to remove incorrect_or_unknown_payment_details parameters

@t-bast

This comment has been minimized.

Copy link
Collaborator

commented Jun 12, 2019

ACK f517e88

@joostjager

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

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 current_height parameter to the incorrect_or_unknown_payment_details failure message. This allows the sender to find out whether they are constructing the route correctly. If route_final_expiry - current_height >= invoice_cltv_delta, the wrong invoice_cltv_delta is used. Otherwise a node along the path delayed.

@joostjager joostjager force-pushed the joostjager:remove-final-expiry-too-soon branch 2 times, most recently from fe607f2 to 0473851 Jun 12, 2019

@joostjager

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

@t-bast added height parameter in fixup commit to address problem of determining whether a failure is terminal or not.

@t-bast

This comment has been minimized.

Copy link
Collaborator

commented Jun 12, 2019

ACK 0473851

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...

@joostjager

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

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

@joostjager joostjager force-pushed the joostjager:remove-final-expiry-too-soon branch from 0473851 to 5da97a9 Jun 25, 2019

@joostjager

This comment has been minimized.

Copy link
Contributor Author

commented Jun 25, 2019

@rustyrussell we came to a tentative ack in yesterday's irc meeting on this pr. What is your opinion on these changes?

@Roasbeef
Copy link
Member

left a comment

LGTM 🐲

@joostjager joostjager force-pushed the joostjager:remove-final-expiry-too-soon branch 2 times, most recently from 23be50c to 15d81e3 Aug 5, 2019

@joostjager

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

Rebased after merge of explicit data types.

@t-bast

This comment has been minimized.

Copy link
Collaborator

commented Aug 5, 2019

I think this is a useful change because providing the block_height could allow detecting nodes that delay HTLCs (especially if we find a way to improve error attribution as you proposed on the ML).

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.

BOLT 4: Merge final_expiry_too_soon into incorrect_or_unknown_payment…
…_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.

@joostjager joostjager force-pushed the joostjager:remove-final-expiry-too-soon branch from 15d81e3 to 745d485 Aug 5, 2019

@joostjager

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

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 unknown_pay_hash or a final_expiry_too_soon is returned, B can find out the final destination for the original route. There is some guesswork involved with respect to the probe amount and expiry (see pr description), but as far as I can see it is a probe vector nonetheless.

@t-bast

This comment has been minimized.

Copy link
Collaborator

commented Aug 5, 2019

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.

@t-bast
t-bast approved these changes Aug 5, 2019
@joostjager

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

It does seem a bit far-fetched honestly

It is similar to #516, just a different parameter

I think this is a useful change because providing the block_height could allow detecting nodes that delay HTLCs (especially if we find a way to improve error attribution as you proposed on the ML).

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.

@cfromknecht
Copy link
Collaborator

left a comment

LGTM

@t-bast t-bast moved this from Scheduled to Accepted in Specification Meeting Agenda Aug 19, 2019

@Roasbeef Roasbeef merged commit 6729755 into lightningnetwork:master Aug 19, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@TheBlueMatt TheBlueMatt referenced this pull request Aug 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants
You can’t perform that action at this time.