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

htlcswitch: Disallow circular routes on same channel #3915

Merged
merged 3 commits into from Feb 3, 2020

Conversation

carlaKC
Copy link
Collaborator

@carlaKC carlaKC commented Jan 15, 2020

This PR adds some protection against circular liquidity attacks, as described in #3771.

Two types of protection against circular routes are identified:

  1. Same channel circles: disallow htlcs with the same incoming and outgoing channel ID

Update: after discussion, decided to only implement (1), which simplifies the PR.
2. Pair channel circles: disallow forwarding of htlcs over a pair of channels when a htlc has already forwarded a htlc with that hash in the opposite direction

Context for not implementing (2):

  1. Effect on base MPP: base MPP uses duplicate htlcs, which are indistinguishable from circular routes (in almost all cases, except for payments which reverse over the same channel pair)
  2. Pair channel effectiveness: pair channel protection is probably quite easy to get around, so not worth implementing

@carlaKC carlaKC force-pushed the 3771-loopattackprotection branch from 0d8f836 to c2875ca Compare Jan 17, 2020
@carlaKC carlaKC removed request for Roasbeef and cfromknecht Jan 17, 2020
@carlaKC carlaKC force-pushed the 3771-loopattackprotection branch from c2875ca to 6573b3a Compare Jan 17, 2020
@carlaKC carlaKC changed the title [WIP, conceptual review only] htlcswitch: Disallow Circular Routes htlcswitch: Disallow circular routes on same chanenl Jan 17, 2020
@carlaKC carlaKC requested review from joostjager and Roasbeef Jan 17, 2020
@carlaKC
Copy link
Collaborator Author

carlaKC commented Jan 17, 2020

Quite simple change if we only disallow circles in the same channel.
Started working on an itest for this as well, but unsure if it's worth adding (in terms of adding more for travis to do).

@carlaKC carlaKC changed the title htlcswitch: Disallow circular routes on same chanenl htlcswitch: Disallow circular routes on same channel Jan 17, 2020
config.go Outdated
@@ -341,6 +341,8 @@ type config struct {
Watchtower *lncfg.Watchtower `group:"watchtower" namespace:"watchtower"`

LegacyProtocol *lncfg.LegacyProtocol `group:"legacyprotocol" namespace:"legacyprotocol"`

AllowCircularRoute bool `long:"allow-circular" description:"If true, our node will allow htlc forwards that arrive and depart on the same channel."`
Copy link
Collaborator

@joostjager joostjager Jan 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Match var name and flag name? Is there a use case for allowing this btw?

Copy link
Collaborator Author

@carlaKC carlaKC Jan 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that keysend will replace the need for this eventually, but I'm worried about this change breaking no-invoice apps which tip by dropping off fees (could be the case that they drop off over the same channel for single channel nodes) in the meantime.

htlcswitch/link.go Outdated Show resolved Hide resolved
@Roasbeef Roasbeef added this to the 0.10.0 milestone Jan 17, 2020
@Roasbeef Roasbeef added this to In Progress in v0.10.0-beta via automation Jan 17, 2020
@carlaKC carlaKC force-pushed the 3771-loopattackprotection branch 2 times, most recently from 12c7a19 to 29796d2 Compare Jan 21, 2020
@carlaKC carlaKC requested review from halseth and removed request for Roasbeef Jan 21, 2020
@carlaKC
Copy link
Collaborator Author

carlaKC commented Jan 21, 2020

Moved check into the switch and added an itest.

The downside of moving this check into the switch is that it exposes the switch to link level information in the form of channelupdate/temporary failure. Instead of adding a ChannelUpdate function to ChannelLink interface, I could add TemporaryFailure which returns the correct error?

@carlaKC carlaKC requested a review from joostjager Jan 21, 2020
htlcswitch/switch.go Outdated Show resolved Hide resolved
}

return NewDetailedLinkError(
lnwire.NewTemporaryChannelFailure(update),
Copy link
Collaborator

@joostjager joostjager Jan 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to return an update. There isn't a problem with the policy.

Copy link
Collaborator Author

@carlaKC carlaKC Jan 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nil update is fine because the spec has a length field, right? It doesn't specifically say whether we have to include an update.

Also double checking, TemporaryChannelUpdate is the correct error to send?
Seems like the most fitting one in the bolts: "The channel from the processing node was unable to handle this HTLC, but may be able to handle it, or others, later."

Copy link
Collaborator

@joostjager joostjager Jan 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think that is ok. It doesn't matter much which error we return, because the sender must know what they are doing.

lntest/itest/lnd_test.go Outdated Show resolved Hide resolved
// whether the htlc will produce a circular forward through the
// same channel, and fail it if the switch does not allow
// forwards of this nature.
if linkErr := s.CheckCircularForward(
Copy link
Collaborator

@joostjager joostjager Jan 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering how this check works together with the previous non-strict forwarding decision. Could it be that the same in and out chan was requested, but that the out channel for some reason (concurrent payment perhaps?) doesn't have enough balance and another channel to the same node is selected?

Copy link
Collaborator Author

@carlaKC carlaKC Jan 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that it's fair to treat these payments as 'suspicious' because they are taking an inefficient route. While there may be some custom use cases out there, node operators who are expecting to use them should configure their node to allow circular routes.

If somebody tries to route a payment over the same channel (and we do not allow that), we could just catch that early and not apply non-strict forwarding? Which would just mean moving the check up in handlePacketForward and failing early.

Copy link
Collaborator

@joostjager joostjager Jan 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes moving it up sounds good. Or maybe basing the check on a pubkey comparison, maybe that is even better.

Copy link
Collaborator Author

@carlaKC carlaKC Jan 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disallowing based on pubkey would mean that people can't do 2 hop circular rebalances, which I think would cause some issues for existing nodes if their lnd peers suddenly stopped letting them rebalance.

Copy link
Collaborator

@joostjager joostjager Jan 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two hop circular rebalances are problematic anyway, because of non-strict forwarding. You may just as well get back your money through the same channel.

Comparing on the exact chan ids is also a risk for peers that have multiple channels between them. The ping pong route can still be executed. I think it is more important to prevent that than to allow the two hop rebalance which isn't reliable anyway.

Copy link
Collaborator Author

@carlaKC carlaKC Jan 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is unreliable but I think a lot of people rely on it at the moment. Would say that we'd probably disable by default if we disallow to the same peer, so that we don't break a lot of people's setups.

I think that single channel is still a valuable defense to have. Without it, an attacker can lock up funds in every channel on your node by zig zagging back and fourth through each channel. The takes capacity/(max htlc size*19) payments to lock up your balance (assuming one hop where they lock up their own balance to route the first hop to your node. I think that to one peer would need wider discussion, @alexbosworth has an operational requirement for that ability, for example.

Copy link
Collaborator

@joostjager joostjager Jan 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, single channel is better than nothing. I have no data myself on how many people rely on two hop rebalancing. Detecting peer-level ping-pong can be implemented later if there are doubts.

One way to lock up the channel is to use up all its capacity, but another way is to use up all commitment tx slots. There is a max number of htlcs that the tx can hold. That way it costs only negligible money to do the attack.

@carlaKC carlaKC force-pushed the 3771-loopattackprotection branch from 29796d2 to 4e7c36b Compare Jan 23, 2020
@Roasbeef Roasbeef requested review from bhandras and removed request for halseth Jan 24, 2020
Copy link
Collaborator

@bhandras bhandras left a comment

just a few nits...

htlcswitch/switch.go Outdated Show resolved Hide resolved
htlcswitch/switch.go Outdated Show resolved Hide resolved
htlcswitch/switch.go Outdated Show resolved Hide resolved
htlcswitch/switch.go Outdated Show resolved Hide resolved
htlcswitch/switch_test.go Outdated Show resolved Hide resolved
htlcswitch/switch_test.go Outdated Show resolved Hide resolved
htlcswitch/switch_test.go Outdated Show resolved Hide resolved
htlcswitch/switch_test.go Outdated Show resolved Hide resolved
v0.10.0-beta automation moved this from In Progress to Review In Progress Jan 24, 2020
@carlaKC carlaKC force-pushed the 3771-loopattackprotection branch 2 times, most recently from a356bc0 to 1ba6de5 Compare Jan 28, 2020
@carlaKC carlaKC force-pushed the 3771-loopattackprotection branch from 1ba6de5 to 3cee449 Compare Jan 28, 2020
@carlaKC carlaKC requested a review from bhandras Jan 28, 2020
Copy link
Collaborator

@bhandras bhandras left a comment

LGTM

@carlaKC carlaKC requested a review from joostjager Jan 29, 2020
Copy link
Collaborator

@joostjager joostjager left a comment

One question remaining about the duplicate error, see comment.

htlcswitch/link_test.go Show resolved Hide resolved
htlcswitch/switch.go Outdated Show resolved Hide resolved
addErr := fmt.Errorf(circularRouteErrTemplate,
htlc.PaymentHash[:], packet.outgoingChanID)

return s.failAddPacket(
Copy link
Collaborator

@joostjager joostjager Jan 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering, what is the purpose of returning addErr here (via failAddPacket) and not just linkErr? FailureDetail already describes what is the problem.

Copy link
Collaborator Author

@carlaKC carlaKC Jan 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The addrErr provides the specific channel ID that the circle was attempted on. I've actually just removed this in #3844 though, I think we can get by without it. If we find ourselves wanting specific link errors, we can just make new FailureDetail types that have additional metadata.

Copy link
Collaborator Author

@carlaKC carlaKC Jan 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going to kill addrErr here and just pass the linkErr in twice, since the change to remove addErr from failAddPacket is already in 3844.

carlaKC added 3 commits Jan 30, 2020
Add a CheckCircularForward function which detects packets which are
forwards over the same incoming and outgoing link, and errors if the
node is configured to disallow forwards of this nature. This check is
added to increase the cost of a liquidity lockup attack, because it
increases the length of the route required to lock up an individual
node's bandwidth. Since nodes are currently limited to 20 hops,
increasing the length of the route needed to lock up capital increases
the number of malicious payments an attacker will have to route, which
increases the capital requirement of the attack overall.
Add a bool to the switch's config which can be used to disable same
channel circular route checks.
@carlaKC carlaKC force-pushed the 3771-loopattackprotection branch from 3cee449 to afc7cc7 Compare Jan 30, 2020
@carlaKC carlaKC requested a review from joostjager Jan 30, 2020
v0.10.0-beta automation moved this from Review In Progress to Merge Queue Jan 30, 2020
Copy link
Collaborator

@joostjager joostjager left a comment

One step to harden the LN against routing attacks completed ♻️

Copy link
Contributor

@cfromknecht cfromknecht left a comment

@carlaKC well done, very well tested too :) LGTM 🎉

// switch is configured to allow circular routes, log that we are
// allowing the route then return nil.
if allowCircular {
log.Debugf("allowing circular route over link: %v "+
Copy link
Contributor

@cfromknecht cfromknecht Feb 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: capitalize log message

@Roasbeef Roasbeef merged commit e25cca1 into lightningnetwork:master Feb 3, 2020
2 checks passed
v0.10.0-beta automation moved this from Merge Queue to Done Feb 3, 2020
@carlaKC carlaKC deleted the 3771-loopattackprotection branch Oct 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
v0.10.0-beta
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants