diff --git a/htlcswitch/link.go b/htlcswitch/link.go index c3c824afc..23c37cebd 100644 --- a/htlcswitch/link.go +++ b/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 { diff --git a/htlcswitch/link_test.go b/htlcswitch/link_test.go index b16f3893c..cd3890d17 100644 --- a/htlcswitch/link_test.go +++ b/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: + } +}