htlcswitch: relax failure message length check #6913

joostjager · 2022-09-13T14:16:25Z

The spec does not dictate but only recommends a length of 256 bytes. Future tlv extensions may push the failure message length over this limit.

With this change, receivers can ignore the lengthier extensions without handling it as an unreadable failure. Before long tlv extensions can be used, a sufficiently large number of senders needs to be upgraded. Therefore the sooner this PR is released, the better.

Depends lightningnetwork/lightning-onion#59

joostjager · 2022-09-14T12:51:58Z

Is there any chance that this can be part of v0.15.2? Strictly speaking lnd is in a vulnerable position. If other implementations would decide to deviate from the recommended failure message size, senders using lnd will see their mission control messed up because of all the penalties applied for unreadable failures.

joostjager · 2022-09-27T08:30:14Z

Need to decide on how to signal sender capabilities as per lightning/bolts#1021 (comment)

joostjager · 2022-09-27T20:42:54Z

This PR could be merged regardless of the signaling mechanism though. On its own, I don't think it does any harm and just makes lnd spec compliant.

The same fix in Eclair: ACINQ/eclair#2438

halseth · 2022-10-11T09:41:28Z

lnwire/onion_error.go

@@ -1235,10 +1235,6 @@ func DecodeFailure(r io.Reader, pver uint32) (FailureMessage, error) {
 	if err := ReadElement(r, &failureLength); err != nil {
 		return nil, fmt.Errorf("unable to read error len: %v", err)
 	}
-	if failureLength > FailureMessageLength {


is this constant in use anywhere else? If so, we should check whether their use must be changed. If not, maybe it can be deleted.

Oof, there is a lot. Need to take a look at that.

All fixed except for the length check here. See #6913 (comment)

I think perhaps a less disruptive way to become closer to spec compliant is instead of removing this check just require the length to be at least FailureMessageLength.

We are already enforcing exactly FailureMessageLength in lightning_onion and it seems like we cannot easily just remove the check altogether without opening up a can of works wrt those converted errors.

If we instead start allowing larger errors than before, we can support the larger errors as we want (and that the spec says we should support), and still we can distinguish the malformed errors by their length, as no valid error message could be FailureMessageLength +4 including the HMAC.

Thoughts?

Definitely a nice intermediate step. One thing to consider is that if we increase the recommended failure length to 1024 bytes eventually, we need to do something extra to also extend converted malformed failures to that length.

Perhaps when we detect a malformed failure, clip off the padding again, and re-add a longer padding block? Or store the unencrypted failures without padding at all, so that they can be padded right before returning them to the switch.

If we stick to lengths >= 256, I don't think we need to make any further changes to this PR. Every failure reason != 260 is interpreted as a regular failure already and this would also work for longer reasons.

joostjager · 2022-10-12T13:17:31Z

Short cut coming back at us: #3027 (comment)

Lightning Labs (@Crypt-iQ perhaps?) - how do we get rid of this?

joostjager · 2022-10-13T09:20:08Z

I've created two draft PRs that show different ways of getting variable length failure messages though the channel state machine:

htlcswitch+lnwallet: ReceiveMalformedFailHTLC #7030: Implement receiving of a malformed failure in the channel state machine
htlcswitch: allow variable length failure messages in channel state machine #7031: Encode variable length failures so that they can be distinguished from fixed length and malformed failures.

The easy alternative is to just allow longer failure message and no shorter ones: #6913 (comment). PR is implementing this currently.

halseth

LGTM 👍

docs/release-notes/release-notes-0.16.0.md

go.mod

lnwire/onion_error.go

htlcswitch/link.go

Crypt-iQ

lgtm once last test-related comment addressed

htlcswitch/link_test.go

lightninglabs-deploy · 2022-11-16T18:20:22Z

@joostjager, remember to re-request review from reviewers when ready

halseth · 2022-11-23T11:14:33Z

htlcswitch/link.go

+			// Because the reason is unreadable for the payer
+			// anyway, we just replace it by a compliant-length
+			// series of random bytes.
+			msg.Reason = make([]byte, minimumFailReasonLength)


For debugging/visibility purposes, could it be an idea to pass back a static "I'm sorry the original error is unreadable"+pad error?

Suppose we'd actually handle this outcome differently (so going beyond debugging/visibility), this might motivate an attacker to corrupt failure messages in this exact way.

Also if the origin node receives this message, they won't know which node set it because this is part of the intermediate transform logic. They do know that it must have been an lnd node 😄

I like the suggestion, but not sure if it is actually good or beneficial. It should never happen with unmodified nodes, because all is set to 256 bytes currently.

That's true, not very likely to happen.

halseth

LGTM

carlaKC

Just one minor comment then lgtm!

lnwire/onion_error.go

Roasbeef · 2022-12-02T01:24:36Z

@joostjager the dependent PR has been merged, so we can bump the dep here to unblock the CI.

This mock is used in the switch test TestUpdateFailMalformedHTLCErrorConversion. But because the mock isn't very realistic, it doesn't detect problems in the handling of malformed failures in the link.

Adds extra checks to make sure the failure message is well-formed.

joostjager · 2022-12-02T08:05:50Z

Removed go.mod replace and bumped lightning-onion dep.

This commit modifies the link behavior so that every failure reason that we pass back is length-compliant (>=256 bytes).

htlcswitch/failure_test.go

This fixes an incompatibility where lnd enforces a strict 256 byte failure message, where as the spec sets this only as the recommended length.

joostjager mentioned this pull request Sep 13, 2022

TLV failure message and length relaxation lightning/bolts#1021

Merged

joostjager force-pushed the relax-failure-len-check branch from e925b9d to 05d0267 Compare September 14, 2022 08:43

joostjager mentioned this pull request Sep 14, 2022

lnwire: increase onion failure message length to 1024 #6916

Draft

joostjager force-pushed the relax-failure-len-check branch 4 times, most recently from 52e2860 to 47a5852 Compare September 14, 2022 12:44

joostjager requested a review from Roasbeef September 14, 2022 13:36

joostjager mentioned this pull request Sep 21, 2022

htlcswitch: add inbound routing fees receive support #6703

Merged

saubyk added this to the v0.16.0 milestone Sep 26, 2022

saubyk added the htlcswitch label Sep 26, 2022

joostjager removed the request for review from Roasbeef September 27, 2022 08:28

halseth reviewed Oct 11, 2022

View reviewed changes

joostjager force-pushed the relax-failure-len-check branch from 47a5852 to b0a2792 Compare October 12, 2022 09:32

This was referenced Oct 13, 2022

htlcswitch+lnwallet: ReceiveMalformedFailHTLC #7030

Closed

htlcswitch: allow variable length failure messages in channel state machine #7031

Closed

joostjager requested a review from halseth October 13, 2022 14:57

joostjager force-pushed the relax-failure-len-check branch from b0a2792 to 0cfd9d1 Compare October 14, 2022 10:41

joostjager mentioned this pull request Oct 14, 2022

crypto: relax failure message length check lightningnetwork/lightning-onion#59

Merged

joostjager force-pushed the relax-failure-len-check branch 2 times, most recently from 15c4888 to a834040 Compare October 14, 2022 11:53

halseth approved these changes Oct 19, 2022

View reviewed changes

docs/release-notes/release-notes-0.16.0.md Outdated Show resolved Hide resolved

go.mod Outdated Show resolved Hide resolved

joostjager force-pushed the relax-failure-len-check branch from a834040 to 7691697 Compare October 19, 2022 12:13

joostjager requested a review from guggero October 19, 2022 13:27

guggero removed their request for review October 20, 2022 15:47

joostjager force-pushed the relax-failure-len-check branch 3 times, most recently from 3fbb6f3 to 1828262 Compare November 3, 2022 13:19

Crypt-iQ reviewed Nov 8, 2022

View reviewed changes

lnwire/onion_error.go Outdated Show resolved Hide resolved

htlcswitch/link.go Show resolved Hide resolved

joostjager force-pushed the relax-failure-len-check branch 2 times, most recently from 0db93ae to 09751ea Compare November 9, 2022 15:37

Crypt-iQ approved these changes Nov 9, 2022

View reviewed changes

htlcswitch/link_test.go Show resolved Hide resolved

joostjager force-pushed the relax-failure-len-check branch from 09751ea to 92068db Compare November 9, 2022 17:18

saubyk requested a review from carlaKC November 22, 2022 18:12

halseth suggested changes Nov 23, 2022

View reviewed changes

halseth approved these changes Nov 25, 2022

View reviewed changes

carlaKC reviewed Nov 29, 2022

View reviewed changes

lnwire/onion_error.go Show resolved Hide resolved

joostjager requested a review from carlaKC December 1, 2022 07:52

carlaKC approved these changes Dec 1, 2022

View reviewed changes

joostjager added 2 commits December 2, 2022 09:04

htlcswitch/test: more realistic mock encryption

e9440a2

This mock is used in the switch test TestUpdateFailMalformedHTLCErrorConversion. But because the mock isn't very realistic, it doesn't detect problems in the handling of malformed failures in the link.

lnwire: verify failure message length

9730bc1

Adds extra checks to make sure the failure message is well-formed.

joostjager force-pushed the relax-failure-len-check branch from 92068db to 46bcc61 Compare December 2, 2022 08:05

joostjager added 2 commits December 2, 2022 09:28

link: ensure minimum failure reason length

5f4465b

This commit modifies the link behavior so that every failure reason that we pass back is length-compliant (>=256 bytes).

lnwire: add extra opaque data to FailIncorrectDetails

5ff5838

joostjager force-pushed the relax-failure-len-check branch from 46bcc61 to fc73ee2 Compare December 2, 2022 08:30

guggero approved these changes Dec 2, 2022

View reviewed changes

htlcswitch/failure_test.go Outdated Show resolved Hide resolved

htlcswitch/failure_test.go Outdated Show resolved Hide resolved

guggero reviewed Dec 2, 2022

View reviewed changes

htlcswitch/failure_test.go Outdated Show resolved Hide resolved

htlcswitch/failure_test.go Outdated Show resolved Hide resolved

htlcswitch/failure_test.go Outdated Show resolved Hide resolved

lnwire: allow longer failure messages

4c8ea29

This fixes an incompatibility where lnd enforces a strict 256 byte failure message, where as the spec sets this only as the recommended length.

joostjager force-pushed the relax-failure-len-check branch from fc73ee2 to 4c8ea29 Compare December 2, 2022 13:28

guggero merged commit 91c0a19 into lightningnetwork:master Dec 2, 2022

saubyk mentioned this pull request Dec 14, 2022

[bug]: incorrect handling of malformed htlc failures on startup #7037

Open

joostjager mentioned this pull request Oct 18, 2023

htlcswitch: return inbound channel update #6967

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

htlcswitch: relax failure message length check #6913

htlcswitch: relax failure message length check #6913

joostjager commented Sep 13, 2022

joostjager commented Sep 14, 2022 •

edited

Loading

joostjager commented Sep 27, 2022

joostjager commented Sep 27, 2022 •

edited

Loading

halseth Oct 11, 2022

joostjager Oct 11, 2022

joostjager Oct 13, 2022

halseth Oct 14, 2022

joostjager Oct 14, 2022

joostjager Oct 14, 2022 •

edited

Loading

joostjager commented Oct 12, 2022 •

edited

Loading

joostjager commented Oct 13, 2022 •

edited

Loading

halseth left a comment

Crypt-iQ left a comment

lightninglabs-deploy commented Nov 16, 2022

halseth Nov 23, 2022

joostjager Nov 23, 2022

halseth Nov 25, 2022

halseth left a comment

carlaKC left a comment

Roasbeef commented Dec 2, 2022

joostjager commented Dec 2, 2022

htlcswitch: relax failure message length check #6913

htlcswitch: relax failure message length check #6913

Conversation

joostjager commented Sep 13, 2022

joostjager commented Sep 14, 2022 • edited Loading

joostjager commented Sep 27, 2022

joostjager commented Sep 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joostjager Oct 14, 2022 • edited Loading

Choose a reason for hiding this comment

joostjager commented Oct 12, 2022 • edited Loading

joostjager commented Oct 13, 2022 • edited Loading

halseth left a comment

Choose a reason for hiding this comment

Crypt-iQ left a comment

Choose a reason for hiding this comment

lightninglabs-deploy commented Nov 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

halseth left a comment

Choose a reason for hiding this comment

carlaKC left a comment

Choose a reason for hiding this comment

Roasbeef commented Dec 2, 2022

joostjager commented Dec 2, 2022

joostjager commented Sep 14, 2022 •

edited

Loading

joostjager commented Sep 27, 2022 •

edited

Loading

joostjager Oct 14, 2022 •

edited

Loading

joostjager commented Oct 12, 2022 •

edited

Loading

joostjager commented Oct 13, 2022 •

edited

Loading