Merge pull request #6913 from bottlepay/relax-failure-len-check

htlcswitch: relax failure message length check

channeldb/mp_payment.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"errors"
 	"io"
+	"math"
 	"time"
 
 	"github.com/btcsuite/btcd/btcec/v2"
@@ -298,7 +299,7 @@ func deserializeHTLCFailInfo(r io.Reader) (*HTLCFailInfo, error) {
 
 	// Read failure.
 	failureBytes, err := wire.ReadVarBytes(
-		r, 0, lnwire.FailureMessageLength, "failure",
+		r, 0, math.MaxUint16, "failure",
 	)
 	if err != nil {
 		return nil, err

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

@@ -5,6 +5,10 @@
 * Warning messages from peers are now recognized and
   [logged](https://github.com/lightningnetwork/lnd/pull/6546) by lnd.
 
+* Decrypt onion failure messages with a [length greater than 256
+  bytes](https://github.com/lightningnetwork/lnd/pull/6913). This moves LND
+  closer to being spec compliant.
+
 ## RPC
 
 * The `RegisterConfirmationsNtfn` call of the `chainnotifier` RPC sub-server 

go.mod

@@ -31,7 +31,7 @@ require (
 	github.com/kkdai/bstream v1.0.0
 	github.com/lightninglabs/neutrino v0.14.2
 	github.com/lightninglabs/protobuf-hex-display v1.4.3-hex-display
-	github.com/lightningnetwork/lightning-onion v1.0.2-0.20220211021909-bb84a1ccb0c5
+	github.com/lightningnetwork/lightning-onion v1.2.1-0.20221202012345-ca23184850a1
 	github.com/lightningnetwork/lnd/cert v1.1.1
 	github.com/lightningnetwork/lnd/clock v1.1.0
 	github.com/lightningnetwork/lnd/healthcheck v1.2.2

go.sum

@@ -440,8 +440,8 @@ github.com/lightninglabs/neutrino v0.14.2 h1:yrnZUCYMZ5ECtXhgDrzqPq2oX8awoAN2D/c
 github.com/lightninglabs/neutrino v0.14.2/go.mod h1:OICUeTCn+4Tu27YRJIpWvvqySxx4oH4vgdP33Sw9RDc=
 github.com/lightninglabs/protobuf-hex-display v1.4.3-hex-display h1:RZJ8H4ueU/aQ9pFtx5wqsuD3B/DezrewJeVwDKKYY8E=
 github.com/lightninglabs/protobuf-hex-display v1.4.3-hex-display/go.mod h1:2oKOBU042GKFHrdbgGiKax4xVrFiZu51lhacUZQ9MnE=
-github.com/lightningnetwork/lightning-onion v1.0.2-0.20220211021909-bb84a1ccb0c5 h1:TkKwqFcQTGYoI+VEqyxA8rxpCin8qDaYX0AfVRinT3k=
-github.com/lightningnetwork/lightning-onion v1.0.2-0.20220211021909-bb84a1ccb0c5/go.mod h1:7dDx73ApjEZA0kcknI799m2O5kkpfg4/gr7N092ojNo=
+github.com/lightningnetwork/lightning-onion v1.2.1-0.20221202012345-ca23184850a1 h1:Wm0g70gkcAu2pGpNZwfWPSVOY21j8IyYsNewwK4OkT4=
+github.com/lightningnetwork/lightning-onion v1.2.1-0.20221202012345-ca23184850a1/go.mod h1:7dDx73ApjEZA0kcknI799m2O5kkpfg4/gr7N092ojNo=
 github.com/lightningnetwork/lnd/cert v1.1.1 h1:Nsav0RlIDRbOnzz2Yu69SQlK939IKya3Q2S0mDviIN8=
 github.com/lightningnetwork/lnd/cert v1.1.1/go.mod h1:1P46svkkd73oSoeI4zjkVKgZNwGq8bkGuPR8z+5vQUs=
 github.com/lightningnetwork/lnd/clock v1.0.1/go.mod h1:KnQudQ6w0IAMZi1SgvecLZQZ43ra2vpDNj7H/aasemg=

htlcswitch/failure_test.go

@@ -0,0 +1,86 @@
+package htlcswitch
+
+import (
+	"encoding/hex"
+	"encoding/json"
+	"os"
+	"testing"
+
+	"github.com/btcsuite/btcd/btcec/v2"
+	sphinx "github.com/lightningnetwork/lightning-onion"
+	"github.com/lightningnetwork/lnd/lnwire"
+	"github.com/lightningnetwork/lnd/tlv"
+	"github.com/stretchr/testify/require"
+)
+
+// TestLongFailureMessage tests that longer failure messages can be interpreted
+// correctly.
+func TestLongFailureMessage(t *testing.T) {
+	t.Parallel()
+
+	var testData struct {
+		SessionKey string
+		Path       []string
+		Reason     string
+	}
+
+	// Use long 1024-byte test vector from BOLT 04.
+	testDataBytes, err := os.ReadFile("testdata/long_failure_msg.json")
+	require.NoError(t, err)
+	require.NoError(t, json.Unmarshal(testDataBytes, &testData))
+
+	sessionKeyBytes, _ := hex.DecodeString(testData.SessionKey)
+
+	reason, _ := hex.DecodeString(testData.Reason)
+
+	sphinxPath := make([]*btcec.PublicKey, len(testData.Path))
+	for i, sKey := range testData.Path {
+		bKey, err := hex.DecodeString(sKey)
+		require.NoError(t, err)
+
+		key, err := btcec.ParsePubKey(bKey)
+		require.NoError(t, err)
+
+		sphinxPath[i] = key
+	}
+
+	sessionKey, _ := btcec.PrivKeyFromBytes(sessionKeyBytes)
+
+	circuit := &sphinx.Circuit{
+		SessionKey:  sessionKey,
+		PaymentPath: sphinxPath,
+	}
+
+	errorDecryptor := &SphinxErrorDecrypter{
+		OnionErrorDecrypter: sphinx.NewOnionErrorDecrypter(circuit),
+	}
+
+	// Assert that the failure message can still be extracted.
+	failure, err := errorDecryptor.DecryptError(reason)
+	require.NoError(t, err)
+
+	incorrectDetails, ok := failure.msg.(*lnwire.FailIncorrectDetails)
+	require.True(t, ok)
+
+	var value varBytesRecordProducer
+
+	extraData := incorrectDetails.ExtraOpaqueData()
+	typeMap, err := extraData.ExtractRecords(&value)
+	require.NoError(t, err)
+	require.Len(t, typeMap, 1)
+
+	expectedValue := make([]byte, 300)
+	for i := range expectedValue {
+		expectedValue[i] = 128
+	}
+
+	require.Equal(t, expectedValue, value.data)
+}
+
+type varBytesRecordProducer struct {
+	data []byte
+}
+
+func (v *varBytesRecordProducer) Record() tlv.Record {
+	return tlv.MakePrimitiveRecord(34001, &v.data)
+}

htlcswitch/link.go

@@ -2,6 +2,7 @@ package htlcswitch
 
 import (
 	"bytes"
+	crand "crypto/rand"
 	"crypto/sha256"
 	"fmt"
 	prand "math/rand"
@@ -1850,6 +1851,35 @@ func (l *channelLink) handleUpstreamMsg(msg lnwire.Message) {
 		}
 
 	case *lnwire.UpdateFailHTLC:
+		// Verify that the failure reason is at least 256 bytes plus
+		// overhead.
+		const minimumFailReasonLength = lnwire.FailureMessageLength +
+			2 + 2 + 32
+
+		if len(msg.Reason) < minimumFailReasonLength {
+			// We've received a reason with a non-compliant length.
+			// Older nodes happily relay back these failures that
+			// may originate from a node further downstream.
+			// Therefore we can't just fail the channel.
+			//
+			// We want to be compliant ourselves, so we also can't
+			// pass back the reason unmodified. And we must make
+			// sure that we don't hit the magic length check of 260
+			// bytes in processRemoteSettleFails either.
+			//
+			// 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)
+			_, err := crand.Read(msg.Reason[:])
+			if err != nil {
+				l.log.Errorf("Random generation error: %v", err)
+
+				return
+			}
+		}
+
+		// Add fail to the update log.
 		idx := msg.ID
 		err := l.channel.ReceiveFailHTLC(idx, msg.Reason[:])
 		if err != nil {

htlcswitch/link_test.go

@@ -2255,11 +2255,14 @@ func TestChannelLinkBandwidthConsistency(t *testing.T) {
 	// With that processed, we'll now generate an HTLC fail (sent by the
 	// remote peer) to cancel the HTLC we just added. This should return us
 	// back to the bandwidth of the link right before the HTLC was sent.
-	err = bobChannel.FailHTLC(bobIndex, []byte("nop"), nil, nil, nil)
+	reason := make([]byte, 292)
+	copy(reason, []byte("nop"))
+
+	err = bobChannel.FailHTLC(bobIndex, reason, nil, nil, nil)
 	require.NoError(t, err, "unable to fail htlc")
 	failMsg := &lnwire.UpdateFailHTLC{
 		ID:     1,
-		Reason: lnwire.OpaqueReason([]byte("nop")),
+		Reason: lnwire.OpaqueReason(reason),
 	}
 
 	aliceLink.HandleChannelUpdate(failMsg)
@@ -2431,7 +2434,8 @@ func TestChannelLinkBandwidthConsistency(t *testing.T) {
 		outgoingChanID: addPkt.outgoingChanID,
 		outgoingHTLCID: addPkt.outgoingHTLCID,
 		htlc: &lnwire.UpdateFailHTLC{
-			ID: 1,
+			ID:     1,
+			Reason: reason,
 		},
 		obfuscator: NewMockObfuscator(),
 	}
@@ -6606,3 +6610,104 @@ func assertFailureCode(t *testing.T, err error, code lnwire.FailCode) {
 			code, rtErr.WireMessage().Code())
 	}
 }
+
+// TestChannelLinkShortFailureRelay tests that failure reasons that are too
+// short are replaced by a spec-compliant length failure message and relayed
+// back.
+func TestChannelLinkShortFailureRelay(t *testing.T) {
+	t.Parallel()
+
+	defer timeout()()
+
+	const chanAmt = btcutil.SatoshiPerBitcoin * 5
+
+	aliceLink, bobChannel, batchTicker, start, _, err :=
+		newSingleLinkTestHarness(t, chanAmt, 0)
+	require.NoError(t, err, "unable to create link")
+
+	require.NoError(t, start())
+
+	coreLink, ok := aliceLink.(*channelLink)
+	require.True(t, ok)
+
+	mockPeer, ok := coreLink.cfg.Peer.(*mockPeer)
+	require.True(t, ok)
+
+	aliceMsgs := mockPeer.sentMsgs
+	switchChan := make(chan *htlcPacket)
+
+	coreLink.cfg.ForwardPackets = func(linkQuit chan struct{}, _ bool,
+		packets ...*htlcPacket) error {
+
+		for _, p := range packets {
+			switchChan <- p
+		}
+
+		return nil
+	}
+
+	ctx := linkTestContext{
+		t:          t,
+		aliceLink:  aliceLink,
+		aliceMsgs:  aliceMsgs,
+		bobChannel: bobChannel,
+	}
+
+	// Send and lock in htlc from Alice to Bob.
+	const htlcID = 0
+
+	htlc, _ := generateHtlcAndInvoice(t, htlcID)
+	ctx.sendHtlcAliceToBob(htlcID, htlc)
+	ctx.receiveHtlcAliceToBob()
+
+	batchTicker <- time.Now()
+
+	ctx.receiveCommitSigAliceToBob(1)
+	ctx.sendRevAndAckBobToAlice()
+
+	ctx.sendCommitSigBobToAlice(1)
+	ctx.receiveRevAndAckAliceToBob()
+
+	// Return a short htlc failure from Bob to Alice and lock in.
+	shortReason := make([]byte, 260)
+
+	err = bobChannel.FailHTLC(0, shortReason, nil, nil, nil)
+	require.NoError(t, err)
+
+	aliceLink.HandleChannelUpdate(&lnwire.UpdateFailHTLC{
+		ID:     htlcID,
+		Reason: shortReason,
+	})
+
+	ctx.sendCommitSigBobToAlice(0)
+	ctx.receiveRevAndAckAliceToBob()
+	ctx.receiveCommitSigAliceToBob(0)
+	ctx.sendRevAndAckBobToAlice()
+
+	// Assert that switch gets the fail message.
+	msg := <-switchChan
+
+	htlcFailMsg, ok := msg.htlc.(*lnwire.UpdateFailHTLC)
+	require.True(t, ok)
+
+	// Assert that it is not a converted error.
+	require.False(t, msg.convertedError)
+
+	// Assert that the length is corrected to the spec-compliant length of
+	// 256 bytes plus overhead.
+	require.Len(t, htlcFailMsg.Reason, 292)
+
+	// Stop the link
+	aliceLink.Stop()
+
+	// Check that no unexpected messages were sent.
+	select {
+	case msg := <-aliceMsgs:
+		require.Fail(t, "did not expect message %T", msg)
+
+	case msg := <-switchChan:
+		require.Fail(t, "did not expect switch message %T", msg)
+
+	default:
+	}
+}

htlcswitch/mock.go

@@ -419,12 +419,16 @@ func (o *mockObfuscator) Reextract(
 	return nil
 }
 
+var fakeHmac = []byte("hmachmachmachmachmachmachmachmac")
+
 func (o *mockObfuscator) EncryptFirstHop(failure lnwire.FailureMessage) (
 	lnwire.OpaqueReason, error) {
 
 	o.failure = failure
 
 	var b bytes.Buffer
+	b.Write(fakeHmac)
+
 	if err := lnwire.EncodeFailure(&b, failure, 0); err != nil {
 		return nil, err
 	}
@@ -436,7 +440,12 @@ func (o *mockObfuscator) IntermediateEncrypt(reason lnwire.OpaqueReason) lnwire.
 }
 
 func (o *mockObfuscator) EncryptMalformedError(reason lnwire.OpaqueReason) lnwire.OpaqueReason {
-	return reason
+	var b bytes.Buffer
+	b.Write(fakeHmac)
+
+	b.Write(reason)
+
+	return b.Bytes()
 }
 
 // mockDeobfuscator mock implementation of the failure deobfuscator which
@@ -447,7 +456,13 @@ func newMockDeobfuscator() ErrorDecrypter {
 	return &mockDeobfuscator{}
 }
 
-func (o *mockDeobfuscator) DecryptError(reason lnwire.OpaqueReason) (*ForwardingError, error) {
+func (o *mockDeobfuscator) DecryptError(reason lnwire.OpaqueReason) (
+	*ForwardingError, error) {
+
+	if !bytes.Equal(reason[:32], fakeHmac) {
+		return nil, errors.New("fake decryption error")
+	}
+	reason = reason[32:]
 
 	r := bytes.NewReader(reason)
 	failure, err := lnwire.DecodeFailure(r, 0)