diff --git a/discovery/gossiper_test.go b/discovery/gossiper_test.go index db7e04842..800bef69c 100644 --- a/discovery/gossiper_test.go +++ b/discovery/gossiper_test.go @@ -859,9 +859,30 @@ func TestProcessAnnouncement(t *testing.T) { t.Fatalf("edge wasn't added to router: %v", err) } + // We'll craft an invalid channel update, setting no message flags. + ua, err := createUpdateAnnouncement(0, 0, remoteKeyPriv1, timestamp) + require.NoError(t, err, "can't create update announcement") + ua.MessageFlags = 0 + + // We send an invalid channel update and expect it to fail. + select { + case err = <-ctx.gossiper.ProcessRemoteAnnouncement(ua, nodePeer): + case <-time.After(2 * time.Second): + t.Fatal("remote announcement not processed") + } + require.ErrorContains(t, err, "max htlc flag not set for channel "+ + "update") + + // We should not broadcast the channel update. + select { + case <-ctx.broadcastedMessage: + t.Fatal("gossiper should not have broadcast channel update") + case <-time.After(2 * trickleDelay): + } + // We'll then craft the channel policy of the remote party and also send // it to the gossiper. - ua, err := createUpdateAnnouncement(0, 0, remoteKeyPriv1, timestamp) + ua, err = createUpdateAnnouncement(0, 0, remoteKeyPriv1, timestamp) require.NoError(t, err, "can't create update announcement") select { @@ -2861,24 +2882,26 @@ func TestNodeAnnouncementNoChannels(t *testing.T) { } // TestOptionalFieldsChannelUpdateValidation tests that we're able to properly -// validate the msg flags and optional max HTLC field of a ChannelUpdate. +// validate the msg flags and max HTLC field of a ChannelUpdate. func TestOptionalFieldsChannelUpdateValidation(t *testing.T) { t.Parallel() ctx, err := createTestCtx(t, 0) require.NoError(t, err, "can't create context") + processRemoteAnnouncement := ctx.gossiper.ProcessRemoteAnnouncement + chanUpdateHeight := uint32(0) timestamp := uint32(123456) nodePeer := &mockPeer{remoteKeyPriv1.PubKey(), nil, nil} - // In this scenario, we'll test whether the message flags field in a channel - // update is properly handled. + // In this scenario, we'll test whether the message flags field in a + // channel update is properly handled. chanAnn, err := createRemoteChannelAnnouncement(chanUpdateHeight) require.NoError(t, err, "can't create channel announcement") select { - case err = <-ctx.gossiper.ProcessRemoteAnnouncement(chanAnn, nodePeer): + case err = <-processRemoteAnnouncement(chanAnn, nodePeer): case <-time.After(2 * time.Second): t.Fatal("did not process remote announcement") } @@ -2886,7 +2909,9 @@ func TestOptionalFieldsChannelUpdateValidation(t *testing.T) { // The first update should fail from an invalid max HTLC field, which is // less than the min HTLC. - chanUpdAnn, err := createUpdateAnnouncement(0, 0, remoteKeyPriv1, timestamp) + chanUpdAnn, err := createUpdateAnnouncement( + 0, 0, remoteKeyPriv1, timestamp, + ) require.NoError(t, err, "unable to create channel update") chanUpdAnn.HtlcMinimumMsat = 5000 @@ -2896,7 +2921,7 @@ func TestOptionalFieldsChannelUpdateValidation(t *testing.T) { } select { - case err = <-ctx.gossiper.ProcessRemoteAnnouncement(chanUpdAnn, nodePeer): + case err = <-processRemoteAnnouncement(chanUpdAnn, nodePeer): case <-time.After(2 * time.Second): t.Fatal("did not process remote announcement") } @@ -2913,7 +2938,7 @@ func TestOptionalFieldsChannelUpdateValidation(t *testing.T) { } select { - case err = <-ctx.gossiper.ProcessRemoteAnnouncement(chanUpdAnn, nodePeer): + case err = <-processRemoteAnnouncement(chanUpdAnn, nodePeer): case <-time.After(2 * time.Second): t.Fatal("did not process remote announcement") } @@ -2921,19 +2946,36 @@ func TestOptionalFieldsChannelUpdateValidation(t *testing.T) { t.Fatalf("expected chan update to error, instead got %v", err) } - // The final update should succeed, since setting the flag 0 means the - // nonsense max_htlc field will just be ignored. + // The third update should not succeed, a channel update with no message + // flag set is invalid. chanUpdAnn.MessageFlags = 0 if err := signUpdate(remoteKeyPriv1, chanUpdAnn); err != nil { t.Fatalf("unable to sign channel update: %v", err) } select { - case err = <-ctx.gossiper.ProcessRemoteAnnouncement(chanUpdAnn, nodePeer): + case err = <-processRemoteAnnouncement(chanUpdAnn, nodePeer): case <-time.After(2 * time.Second): t.Fatal("did not process remote announcement") } - require.NoError(t, err, "unable to process announcement") + require.ErrorContains(t, err, "max htlc flag not set") + + // The final update should succeed. + chanUpdAnn, err = createUpdateAnnouncement( + 0, 0, remoteKeyPriv1, timestamp, + ) + require.NoError(t, err, "unable to create channel update") + + if err := signUpdate(remoteKeyPriv1, chanUpdAnn); err != nil { + t.Fatalf("unable to sign channel update: %v", err) + } + + select { + case err = <-processRemoteAnnouncement(chanUpdAnn, nodePeer): + case <-time.After(2 * time.Second): + t.Fatal("did not process remote announcement") + } + require.NoError(t, err, "expected update to be processed") } // TestSendChannelUpdateReliably ensures that the latest channel update for a diff --git a/docs/release-notes/release-notes-0.16.0.md b/docs/release-notes/release-notes-0.16.0.md index 80ad5fa71..eab9512a5 100644 --- a/docs/release-notes/release-notes-0.16.0.md +++ b/docs/release-notes/release-notes-0.16.0.md @@ -17,6 +17,9 @@ current gossip sync query status. bytes](https://github.com/lightningnetwork/lnd/pull/6913). This moves LND closer to being spec compliant. +* Channel updates without the maxHTLC message flag set are recognized as invalid + and are [not relayed](https://github.com/lightningnetwork/lnd/pull/7415). + ## RPC * The `RegisterConfirmationsNtfn` call of the `chainnotifier` RPC sub-server diff --git a/netann/channel_update_test.go b/netann/channel_update_test.go index 4d4a6d72b..e49e5c65e 100644 --- a/netann/channel_update_test.go +++ b/netann/channel_update_test.go @@ -182,8 +182,8 @@ func TestUpdateDisableFlag(t *testing.T) { // Finally, validate the signature using the router's // verification logic. - err = routing.ValidateChannelUpdateAnn( - pubKey, 0, newUpdate, + err = routing.VerifyChannelUpdateSignature( + newUpdate, pubKey, ) if err != nil { t.Fatalf("channel update failed to "+ diff --git a/routing/ann_validation.go b/routing/ann_validation.go index 9231313ce..35f0afd4d 100644 --- a/routing/ann_validation.go +++ b/routing/ann_validation.go @@ -165,22 +165,25 @@ func VerifyChannelUpdateSignature(msg *lnwire.ChannelUpdate, func ValidateChannelUpdateFields(capacity btcutil.Amount, msg *lnwire.ChannelUpdate) error { - if msg.MessageFlags.HasMaxHtlc() { - maxHtlc := msg.HtlcMaximumMsat - if maxHtlc == 0 || maxHtlc < msg.HtlcMinimumMsat { - return errors.Errorf("invalid max htlc for channel "+ - "update %v", spew.Sdump(msg)) - } + // The maxHTLC flag is mandatory. + if !msg.MessageFlags.HasMaxHtlc() { + return errors.Errorf("max htlc flag not set for channel "+ + "update %v", spew.Sdump(msg)) + } - // For light clients, the capacity will not be set so we'll skip - // checking whether the MaxHTLC value respects the channel's - // capacity. - capacityMsat := lnwire.NewMSatFromSatoshis(capacity) - if capacityMsat != 0 && maxHtlc > capacityMsat { - return errors.Errorf("max_htlc(%v) for channel "+ - "update greater than capacity(%v)", maxHtlc, - capacityMsat) - } + maxHtlc := msg.HtlcMaximumMsat + if maxHtlc == 0 || maxHtlc < msg.HtlcMinimumMsat { + return errors.Errorf("invalid max htlc for channel "+ + "update %v", spew.Sdump(msg)) + } + + // For light clients, the capacity will not be set so we'll skip + // checking whether the MaxHTLC value respects the channel's + // capacity. + capacityMsat := lnwire.NewMSatFromSatoshis(capacity) + if capacityMsat != 0 && maxHtlc > capacityMsat { + return errors.Errorf("max_htlc (%v) for channel update "+ + "greater than capacity (%v)", maxHtlc, capacityMsat) } return nil