highperfocused/lnd
1
0
Fork 0
mirror of https://github.com/lightningnetwork/lnd.git synced 2025-03-25 17:22:59 +01:00
Code Issues Packages Projects Releases Wiki Activity

Merge pull request #7768 from ProofOfKeags/spurious-htlc-rejections

Browse Source 
invoices+htlcswitch: fix spurious htlc rejections from overly strict onion packet integrity checks
This commit is contained in:
Olaoluwa Osuntokun 2023-07-11 10:51:22 -07:00 committed by GitHub
commit 33b470b4a6
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 265 additions and 79 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
docs/release-notes/release-notes-0.17.0.md
View File

@ -162,6 +162,8 @@ unlock or create.
* [Updated bbolt to v1.3.7](https://github.com/lightningnetwork/lnd/pull/7796)
in order to address mmap issues affecting certain older iPhone devices.
* [Stop rejecting payments that overpay or over-timelock the final hop](https://github.com/lightningnetwork/lnd/pull/7768)
### Tooling and documentation
* Add support for [custom `RPCHOST` and
@ -187,6 +189,7 @@ unlock or create.
* Hampus Sjöberg
* hieblmi
* Jordi Montes
* Keagan McClelland
* Lele Calo
* Matt Morehouse
* Maxwell Sayles

21
htlcswitch/link.go
View File

@ -1929,9 +1929,9 @@ func (l *channelLink) handleUpstreamMsg(msg lnwire.Message) {
// Instead of truncating the slice to conserve memory
// allocations, we simply set the uncommitted preimage slice to
// nil so that a new one will be initialized if any more
// witnesses are discovered. We do this maximum size of the
// slice can occupy 15KB, and want to ensure we release that
// memory back to the runtime.
// witnesses are discovered. We do this because the maximum size
// that the slice can occupy is 15KB, and we want to ensure we
// release that memory back to the runtime.
l.uncommittedPreimages = nil
// We just received a new updates to our local commitment
@ -3237,11 +3237,10 @@ func (l *channelLink) processExitHop(pd *lnwallet.PaymentDescriptor,
// As we're the exit hop, we'll double check the hop-payload included in
// the HTLC to ensure that it was crafted correctly by the sender and
// matches the HTLC we were extended.
if pd.Amount != fwdInfo.AmountToForward {
l.log.Errorf("onion payload of incoming htlc(%x) has incorrect "+
"value: expected %v, got %v", pd.RHash,
// is compatible with the HTLC we were extended.
if pd.Amount < fwdInfo.AmountToForward {
l.log.Errorf("onion payload of incoming htlc(%x) has "+
"incompatible value: expected <=%v, got %v", pd.RHash,
pd.Amount, fwdInfo.AmountToForward)
failure := NewLinkError(
@ -3254,9 +3253,9 @@ func (l *channelLink) processExitHop(pd *lnwallet.PaymentDescriptor,
// We'll also ensure that our time-lock value has been computed
// correctly.
if pd.Timeout != fwdInfo.OutgoingCTLV {
l.log.Errorf("onion payload of incoming htlc(%x) has incorrect "+
"time-lock: expected %v, got %v",
if pd.Timeout < fwdInfo.OutgoingCTLV {
l.log.Errorf("onion payload of incoming htlc(%x) has "+
"incompatible time-lock: expected <=%v, got %v",
pd.RHash[:], pd.Timeout, fwdInfo.OutgoingCTLV)
failure := NewLinkError(

235
htlcswitch/link_test.go
View File

@ -12,6 +12,7 @@ import (
"runtime"
"sync"
"testing"
"testing/quick"
"time"
"github.com/btcsuite/btcd/btcec/v2"
@ -19,7 +20,6 @@ import (
"github.com/btcsuite/btcd/chaincfg/chainhash"
"github.com/btcsuite/btcd/wire"
"github.com/davecgh/go-spew/spew"
"github.com/go-errors/errors"
sphinx "github.com/lightningnetwork/lightning-onion"
"github.com/lightningnetwork/lnd/build"
"github.com/lightningnetwork/lnd/channeldb"
@ -125,7 +125,7 @@ func createInterceptorFunc(prefix, receiver string, messages []expectedMessage,
if messageChanID == chanID {
if len(expectToReceive) == 0 {
return false, errors.Errorf("%v received "+
return false, fmt.Errorf("%v received "+
"unexpected message out of range: %v",
receiver, m.MsgType())
}
@ -134,9 +134,13 @@ func createInterceptorFunc(prefix, receiver string, messages []expectedMessage,
expectToReceive = expectToReceive[1:]
if expectedMessage.message.MsgType() != m.MsgType() {
return false, errors.Errorf("%v received wrong message: \n"+
"real: %v\nexpected: %v", receiver, m.MsgType(),
expectedMessage.message.MsgType())
return false, fmt.Errorf(
"%v received wrong message: \n"+
"real: %v\nexpected: %v",
receiver,
m.MsgType(),
expectedMessage.message.MsgType(),
)
}
if debug {
@ -721,11 +725,10 @@ func TestChannelLinkCancelFullCommitment(t *testing.T) {
}
}
// TestExitNodeTimelockPayloadMismatch tests that when an exit node receives an
// incoming HTLC, if the time lock encoded in the payload of the forwarded HTLC
// doesn't match the expected payment value, then the HTLC will be rejected
// with the appropriate error.
func TestExitNodeTimelockPayloadMismatch(t *testing.T) {
// TestExitNodeHLTCTimelockExceedsPayload tests that when an exit node receives
// an incoming HTLC, if the timelock of the incoming HTLC is greater than or
// equal to the timelock encoded in the payload, then the HTLC will be accepted.
func TestExitNodeHTLCTimelockExceedsPayload(t *testing.T) {
t.Parallel()
channels, _, err := createClusterChannels(
@ -733,35 +736,75 @@ func TestExitNodeTimelockPayloadMismatch(t *testing.T) {
)
require.NoError(t, err, "unable to create channel")
n := newThreeHopNetwork(t, channels.aliceToBob, channels.bobToAlice,
channels.bobToCarol, channels.carolToBob, testStartingHeight)
if err := n.start(); err != nil {
t.Fatal(err)
}
n := newThreeHopNetwork(
t, channels.aliceToBob, channels.bobToAlice,
channels.bobToCarol, channels.carolToBob, testStartingHeight,
)
require.NoError(t, n.start())
t.Cleanup(n.stop)
const amount = btcutil.SatoshiPerBitcoin
htlcAmt, htlcExpiry, hops := generateHops(amount,
testStartingHeight, n.firstBobChannelLink)
htlcAmt, htlcExpiry, hops := generateHops(
amount, testStartingHeight, n.firstBobChannelLink,
)
// In order to exercise this case, we'll now _manually_ modify the
// per-hop payload for outgoing time lock to be the incorrect value.
// per-hop payload for outgoing time lock to be a compatible value that
// differs from the specified expiry.
// The proper value of the outgoing CLTV should be the policy set by
// the receiving node, instead we set it to be a random value.
hops[0].FwdInfo.OutgoingCTLV = 500
// the receiving node, instead we set it to be a value less than the
// incoming HTLC timelock.
hops[0].FwdInfo.OutgoingCTLV = htlcExpiry - 1
firstHop := n.firstBobChannelLink.ShortChanID()
_, err = makePayment(
n.aliceServer, n.bobServer, firstHop, hops, amount, htlcAmt,
htlcExpiry,
).Wait(30 * time.Second)
if err == nil {
t.Fatalf("payment should have failed but didn't")
}
require.NoError(t, err, "payment should have succeeded but didn't")
}
rtErr, ok := err.(ClearTextError)
if !ok {
t.Fatalf("expected a ClearTextError, instead got: %T", err)
}
// TestExitNodeTimelockPayloadExceedsHTLC tests that when an exit node receives
// an incoming HTLC, if the timelock encoded in the payload of the forwarded
// HTLC exceeds the timelock on the incoming HTLC, then the HTLC will be
// rejected with the appropriate error.
func TestExitNodeTimelockPayloadExceedsHTLC(t *testing.T) {
t.Parallel()
channels, _, err := createClusterChannels(
t, btcutil.SatoshiPerBitcoin*5, btcutil.SatoshiPerBitcoin*5,
)
require.NoError(t, err, "unable to create channel")
n := newThreeHopNetwork(
t, channels.aliceToBob, channels.bobToAlice,
channels.bobToCarol, channels.carolToBob, testStartingHeight,
)
require.NoError(t, n.start())
t.Cleanup(n.stop)
const amount = btcutil.SatoshiPerBitcoin
htlcAmt, htlcExpiry, hops := generateHops(
amount, testStartingHeight, n.firstBobChannelLink,
)
// In order to exercise this case, we'll now _manually_ modify the
// per-hop payload for outgoing time lock to be the incorrect value.
// The proper value of the outgoing CLTV should be the policy set by
// the receiving node, instead we set it to be a value greater than the
// incoming HTLC timelock.
hops[0].FwdInfo.OutgoingCTLV = htlcExpiry + 1
firstHop := n.firstBobChannelLink.ShortChanID()
_, err = makePayment(
n.aliceServer, n.bobServer, firstHop, hops, amount, htlcAmt,
htlcExpiry,
).Wait(30 * time.Second)
require.NotNil(t, err, "payment should have failed but didn't")
rtErr := &ForwardingError{}
require.ErrorAs(
t, err, &rtErr, "expected a ClearTextError, instead got: %T",
err,
)
switch rtErr.WireMessage().(type) {
case *lnwire.FailFinalIncorrectCltvExpiry:
@ -771,43 +814,97 @@ func TestExitNodeTimelockPayloadMismatch(t *testing.T) {
}
}
// TestExitNodeAmountPayloadMismatch tests that when an exit node receives an
// incoming HTLC, if the amount encoded in the onion payload of the forwarded
// HTLC doesn't match the expected payment value, then the HTLC will be
// rejected.
func TestExitNodeAmountPayloadMismatch(t *testing.T) {
// TestExitNodeHTLCUnderpaysPayloadAmount tests that when an exit node receives
// an incoming HTLC, if the amount offered in the HTLC is less than the amount
// encoded in the onion payload then the HTLC will be rejected with the
// appropriate error.
func TestExitNodeHTLCUnderpaysPayloadAmount(t *testing.T) {
t.Parallel()
channels, _, err := createClusterChannels(
t, btcutil.SatoshiPerBitcoin*5, btcutil.SatoshiPerBitcoin*5,
t, btcutil.SatoshiPerBitcoin*5,
btcutil.SatoshiPerBitcoin*5,
)
require.NoError(t, err, "unable to create channel")
n := newThreeHopNetwork(t, channels.aliceToBob, channels.bobToAlice,
channels.bobToCarol, channels.carolToBob, testStartingHeight)
if err := n.start(); err != nil {
t.Fatal(err)
}
n := newThreeHopNetwork(
t, channels.aliceToBob, channels.bobToAlice,
channels.bobToCarol, channels.carolToBob,
testStartingHeight,
)
require.NoError(t, n.start())
t.Cleanup(n.stop)
const amount = btcutil.SatoshiPerBitcoin
htlcAmt, htlcExpiry, hops := generateHops(amount, testStartingHeight,
n.firstBobChannelLink)
f := func(underpaymentRand uint64) bool {
const amount = btcutil.SatoshiPerBitcoin / 100
underpayment := lnwire.MilliSatoshi(
underpaymentRand%(amount-1) + 1,
)
// In order to exercise this case, we'll now _manually_ modify the
// per-hop payload for amount to be the incorrect value. The proper
// value of the amount to forward should be the amount that the
// receiving node expects to receive.
hops[0].FwdInfo.AmountToForward = 1
firstHop := n.firstBobChannelLink.ShortChanID()
_, err = makePayment(
n.aliceServer, n.bobServer, firstHop, hops, amount, htlcAmt,
htlcExpiry,
).Wait(30 * time.Second)
if err == nil {
t.Fatalf("payment should have failed but didn't")
htlcAmt, htlcExpiry, hops := generateHops(
amount, testStartingHeight, n.firstBobChannelLink,
)
// In order to exercise this case, we'll now _manually_ modify
// the per-hop payload for amount to be the incorrect value.
// The acceptable values of the amount to forward should be less
// than the incoming HTLC value.
hops[0].FwdInfo.AmountToForward = amount + underpayment
firstHop := n.firstBobChannelLink.ShortChanID()
_, err = makePayment(
n.aliceServer, n.bobServer, firstHop, hops, amount,
htlcAmt, htlcExpiry,
).Wait(30 * time.Second)
assertFailureCode(t, err, lnwire.CodeFinalIncorrectHtlcAmount)
return err != nil
}
assertFailureCode(t, err, lnwire.CodeFinalIncorrectHtlcAmount)
err = quick.Check(f, nil)
require.NoError(t, err, "payment should have failed but didn't")
}
// TestExitNodeHTLCExceedsAmountPayload tests that when an exit node receives an
// incoming HTLC, if the amount encoded in the onion payload of the forwarded
// HTLC is lower than the incoming HTLC value, then the HTLC will be accepted.
func TestExitNodeHTLCExceedsAmountPayload(t *testing.T) {
t.Parallel()
channels, _, err := createClusterChannels(
t, btcutil.SatoshiPerBitcoin*5,
btcutil.SatoshiPerBitcoin*5,
)
require.NoError(t, err, "unable to create channel")
n := newThreeHopNetwork(t, channels.aliceToBob,
channels.bobToAlice, channels.bobToCarol,
channels.carolToBob, testStartingHeight)
require.NoError(t, n.start())
t.Cleanup(n.stop)
f := func(overpaymentRand uint64) bool {
const amount = btcutil.SatoshiPerBitcoin / 100
overpayment := lnwire.MilliSatoshi(
overpaymentRand%(amount-1) + 1,
)
htlcAmt, htlcExpiry, hops := generateHops(amount,
testStartingHeight, n.firstBobChannelLink)
// In order to exercise this case, we'll now _manually_ modify
// the per-hop payload for amount to be the incorrect value.
// The acceptable values of the amount to forward should be
// lower than the incoming HTLC value.
hops[0].FwdInfo.AmountToForward = amount - overpayment
firstHop := n.firstBobChannelLink.ShortChanID()
_, err = makePayment(
n.aliceServer, n.bobServer, firstHop, hops, amount,
htlcAmt, htlcExpiry,
).Wait(30 * time.Second)
return err == nil
}
err = quick.Check(f, nil)
require.NoError(t, err, "payment should have succeeded but didn't")
}
// TestLinkForwardTimelockPolicyMismatch tests that if a node is an
@ -3512,25 +3609,37 @@ func TestChannelRetransmission(t *testing.T) {
// bandwidth of htlc links hasn't been changed.
invoice, err = receiver.registry.LookupInvoice(rhash)
if err != nil {
err = errors.Errorf("unable to get invoice: %v", err)
err = fmt.Errorf(
"unable to get invoice: %w", err,
)
continue
}
if invoice.State != invpkg.ContractSettled {
err = errors.Errorf("alice invoice haven't been settled")
err = fmt.Errorf(
"alice invoice haven't been settled",
)
continue
}
aliceExpectedBandwidth := aliceBandwidthBefore - htlcAmt
if aliceExpectedBandwidth != n.aliceChannelLink.Bandwidth() {
err = errors.Errorf("expected alice to have %v, instead has %v",
aliceExpectedBandwidth, n.aliceChannelLink.Bandwidth())
err = fmt.Errorf(
"expected alice to have %v,"+
" instead has %v",
aliceExpectedBandwidth,
n.aliceChannelLink.Bandwidth(),
)
continue
}
bobExpectedBandwidth := bobBandwidthBefore + htlcAmt
if bobExpectedBandwidth != n.firstBobChannelLink.Bandwidth() {
err = errors.Errorf("expected bob to have %v, instead has %v",
bobExpectedBandwidth, n.firstBobChannelLink.Bandwidth())
err = fmt.Errorf(
"expected bob to have %v,"+
" instead has %v",
bobExpectedBandwidth,
n.firstBobChannelLink.Bandwidth(),
)
continue
}
@ -5517,8 +5626,10 @@ func TestExpectedFee(t *testing.T) {
}
fee := ExpectedFee(f, test.htlcAmt)
if fee != test.expected {
t.Errorf("expected fee to be (%v), instead got (%v)", test.expected,
fee)
t.Errorf(
"expected fee to be (%v), instead got (%v)",
test.expected, fee,
)
}
}
}

78
invoices/invoiceregistry_test.go
View File

@ -4,6 +4,7 @@ import (
"crypto/rand"
"math"
"testing"
"testing/quick"
"time"
"github.com/lightningnetwork/lnd/amp"
@ -925,6 +926,83 @@ func TestMppPayment(t *testing.T) {
}
}
// TestMppPaymentWithOverpayment tests settling of an invoice with multiple
// partial payments. It covers the case where the mpp overpays what is in the
// invoice.
func TestMppPaymentWithOverpayment(t *testing.T) {
t.Parallel()
f := func(overpaymentRand uint64) bool {
ctx := newTestContext(t, nil)
// Add the invoice.
_, err := ctx.registry.AddInvoice(
testInvoice, testInvoicePaymentHash,
)
if err != nil {
t.Fatal(err)
}
mppPayload := &mockPayload{
mpp: record.NewMPP(testInvoiceAmt, [32]byte{}),
}
// We constrain overpayment amount to be [1,1000].
overpayment := lnwire.MilliSatoshi((overpaymentRand % 999) + 1)
// Send htlc 1.
hodlChan1 := make(chan interface{}, 1)
resolution, err := ctx.registry.NotifyExitHopHtlc(
testInvoicePaymentHash, testInvoice.Terms.Value/2,
testHtlcExpiry, testCurrentHeight, getCircuitKey(11),
hodlChan1, mppPayload,
)
if err != nil {
t.Fatal(err)
}
if resolution != nil {
t.Fatal("expected no direct resolution")
}
// Send htlc 2.
hodlChan2 := make(chan interface{}, 1)
resolution, err = ctx.registry.NotifyExitHopHtlc(
testInvoicePaymentHash,
testInvoice.Terms.Value/2+overpayment, testHtlcExpiry,
testCurrentHeight, getCircuitKey(12), hodlChan2,
mppPayload,
)
if err != nil {
t.Fatal(err)
}
settleResolution, ok :=
resolution.(*invpkg.HtlcSettleResolution)
if !ok {
t.Fatalf("expected settle resolution, got: %T",
resolution)
}
if settleResolution.Outcome != invpkg.ResultSettled {
t.Fatalf("expected result settled, got: %v",
settleResolution.Outcome)
}
// Check that settled amount is equal to the sum of values of
// the htlcs 1 and 2.
inv, err := ctx.registry.LookupInvoice(testInvoicePaymentHash)
if err != nil {
t.Fatal(err)
}
if inv.State != invpkg.ContractSettled {
t.Fatal("expected invoice to be settled")
}
return inv.AmtPaid == testInvoice.Terms.Value+overpayment
}
if err := quick.Check(f, nil); err != nil {
t.Fatalf("amount incorrect: %v", err)
}
}
// Tests that invoices are canceled after expiration.
func TestInvoiceExpiryWithRegistry(t *testing.T) {
t.Parallel()

7
invoices/update.go
View File

@ -214,11 +214,6 @@ func updateMpp(ctx *invoiceUpdateCtx, inv *Invoice) (*InvoiceUpdateDesc,
// Add amount of new htlc.
newSetTotal += ctx.amtPaid
// Make sure the communicated set total isn't overpaid.
if newSetTotal > ctx.mpp.TotalMsat() {
return nil, ctx.failRes(ResultHtlcSetOverpayment), nil
}
// The invoice is still open. Check the expiry.
if ctx.expiry < uint32(ctx.currentHeight+ctx.finalCltvRejectDelta) {
return nil, ctx.failRes(ResultExpiryTooSoon), nil
@ -243,7 +238,7 @@ func updateMpp(ctx *invoiceUpdateCtx, inv *Invoice) (*InvoiceUpdateDesc,
}
// If the invoice cannot be settled yet, only record the htlc.
setComplete := newSetTotal == ctx.mpp.TotalMsat()
setComplete := newSetTotal >= ctx.mpp.TotalMsat()
if !setComplete {
return &update, ctx.acceptRes(resultPartialAccepted), nil
}