BOLT 4: Merge final_expiry_too_soon into incorrect_or_unknown_payment

[joostjager]

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]

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]

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]

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]

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]

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]

Updated PR to remove incorrect_or_unknown_payment_details parameters

[t-bast]

ACK f517e88

[joostjager]

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]

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

[t-bast]

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]

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]

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

[Roasbeef]

LGTM 🐲

[joostjager]

Rebased after merge of explicit data types.

[t-bast]

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.

[joostjager]

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]

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.

[joostjager]

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]

LGTM

[TheBlueMatt]

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.

[TheBlueMatt]

Also, as pointed out at 11f6017 this adds a lot of complexity to error handling.

[t-bast]

I'm a little confused by this - how can someone identify a payment differently based on the CLTV?

Have you re-read this comment: #608 (comment)?
It seems plausible to me, the nodes that are not the destination can only return incorrect_or_unknown_payment (because they really aren't the recipient) whereas the real destination would return final_expiry_too_soon if the attacker set the cltv in the right range, thus revealing it is the destination.

Or is there something that you think doesn't work in this attack scenario?

[t-bast]

However, this type of probing wouldn't work anymore now that we have a payment_secret, the probing nodes would just be rejected because they don't know the correct value there, but this was before we introduced payment_secrets IIRC

[TheBlueMatt]

Have you re-read this comment: #608 (comment)?

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.

[t-bast]

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 expiry_delta for final payments, doesn't it?
If for example Alice, Bob and Carol use respectively 6, 10 and 15 blocks and the attacker sends an HTLC with final expiry currentBlockHeight + 12, only Carol will answer final_expiry_too_soon, for Alice and Bob the CLTV is ok.

I agree, it's a bit far-fetched and is in practice hard to do for the attacker, and on top of that payment_secret completely solves this issue, but that was the reasoning at the time when we made that change. Does that make sense?

[TheBlueMatt]

If for example Alice, Bob and Carol use respectively 6, 10 and 15 blocks and the attacker sends an HTLC with final expiry currentBlockHeight + 12, only Carol will answer final_expiry_too_soon, for Alice and Bob the CLTV is ok.

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?

[t-bast]

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

[cfromknecht]

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.

@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 final_expiry_too_soon rather than incorrect_payment_details. With this design I don't think it's possible to reorder the checks?

If i understand your suggestion, that would imply implementations necessarily have to use a static final CLTV delta?

[TheBlueMatt]

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.

[joostjager]

With payment_secret being required now, I think that there is indeed no point anymore for the sender to interpret the incorrect_or_unknown_payment failure message parameters.

In fact, lnd isn't looking at these parameters. Probably a left-over todo that became redundant with the introduction of payment_secret.

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'?