diff --git a/discovery/gossiper.go b/discovery/gossiper.go index 96a8c047ae3..15e5817ded4 100644 --- a/discovery/gossiper.go +++ b/discovery/gossiper.go @@ -2502,6 +2502,22 @@ func (d *AuthenticatedGossiper) handleNodeAnnouncement(ctx context.Context, "node=%x, source=%x", nMsg.peer, timestamp, nodeAnn.NodeID, nMsg.source.SerializeCompressed()) + // Although not explicitly required by BOLT 7 for node announcements + // (unlike channel updates), we still enforce non-zero timestamps as a + // sanity check. A timestamp of zero is likely indicative of a bug or + // uninitialized message. + if nodeAnn.Timestamp == 0 { + err := fmt.Errorf("rejecting node announcement with zero "+ + "timestamp for node %x", nodeAnn.NodeID) + + log.Warnf("Rejecting node announcement from peer=%v: %v", + nMsg.peer, err) + + nMsg.err <- err + + return nil, false + } + // We'll quickly ask the router if it already has a newer update for // this node so we can skip validating signatures if not required. if d.cfg.Graph.IsStaleNode(ctx, nodeAnn.NodeID, timestamp) { @@ -3056,6 +3072,27 @@ func (d *AuthenticatedGossiper) handleChanUpdate(ctx context.Context, // quickly reject it. timestamp := time.Unix(int64(upd.Timestamp), 0) + // Per BOLT 7, the timestamp MUST be greater than 0. + if upd.Timestamp == 0 { + err := fmt.Errorf("rejecting channel update with zero "+ + "timestamp for short_chan_id(%v)", shortChanID) + + // Only increase ban score for remote peers. + if nMsg.isRemote { + log.Warnf("Increasing ban score for peer=%v: %v", + nMsg.peer, err) + + dcErr := d.handleBadPeer(nMsg.peer) + if dcErr != nil { + err = dcErr + } + } + + nMsg.err <- err + + return nil, false + } + // Fetch the SCID we should be using to lock the channelMtx and make // graph queries with. graphScid, err := d.cfg.FindBaseByAlias(upd.ShortChannelID) diff --git a/discovery/gossiper_test.go b/discovery/gossiper_test.go index 01faf5561c4..b9f97e52bee 100644 --- a/discovery/gossiper_test.go +++ b/discovery/gossiper_test.go @@ -2930,6 +2930,78 @@ func TestExtraDataNodeAnnouncementValidation(t *testing.T) { require.NoError(t, err, "unable to process announcement") } +// TestZeroTimestampNodeAnnouncementRejection tests that a NodeAnnouncement with +// a zero timestamp is rejected per BOLT 7. +func TestZeroTimestampNodeAnnouncementRejection(t *testing.T) { + t.Parallel() + ctx := t.Context() + + tCtx, err := createTestCtx(t, 0, false) + require.NoError(t, err, "can't create context") + + remotePeer := &mockPeer{ + remoteKeyPriv1.PubKey(), nil, nil, atomic.Bool{}, + } + + // Create a node announcement with a zero timestamp. + nodeAnn, err := createNodeAnnouncement(remoteKeyPriv1, 0) + require.NoError(t, err, "can't create node announcement") + + // Processing the announcement should fail with a zero timestamp error. + select { + case err = <-tCtx.gossiper.ProcessRemoteAnnouncement( + ctx, nodeAnn, remotePeer, + ): + case <-time.After(2 * time.Second): + t.Fatal("did not process remote announcement") + } + require.Error(t, err) + require.Contains(t, err.Error(), "zero timestamp") +} + +// TestZeroTimestampChannelUpdateRejection tests that a ChannelUpdate with a +// zero timestamp is rejected per BOLT 7. +func TestZeroTimestampChannelUpdateRejection(t *testing.T) { + t.Parallel() + ctx := t.Context() + + tCtx, err := createTestCtx(t, 0, false) + require.NoError(t, err, "can't create context") + + remotePeer := &mockPeer{ + remoteKeyPriv1.PubKey(), nil, nil, atomic.Bool{}, + } + + // First, we need to process a channel announcement so that the channel + // update has a valid channel to refer to. + chanAnn, err := tCtx.createRemoteChannelAnnouncement(0) + require.NoError(t, err, "unable to create chan ann") + + select { + case err = <-tCtx.gossiper.ProcessRemoteAnnouncement( + ctx, chanAnn, remotePeer, + ): + case <-time.After(2 * time.Second): + t.Fatal("did not process remote announcement") + } + require.NoError(t, err, "unable to process chan ann") + + // Now create a channel update with a zero timestamp. + chanUpdAnn, err := createUpdateAnnouncement(0, 0, remoteKeyPriv1, 0) + require.NoError(t, err, "unable to create chan update") + + // Processing the update should fail with a zero timestamp error. + select { + case err = <-tCtx.gossiper.ProcessRemoteAnnouncement( + ctx, chanUpdAnn, remotePeer, + ): + case <-time.After(2 * time.Second): + t.Fatal("did not process remote announcement") + } + require.Error(t, err) + require.Contains(t, err.Error(), "zero timestamp") +} + // assertBroadcast checks that num messages are being broadcasted from the // gossiper. The broadcasted messages are returned. func assertBroadcast(t *testing.T, ctx *testCtx, num int) []lnwire.Message { diff --git a/docs/release-notes/release-notes-0.20.1.md b/docs/release-notes/release-notes-0.20.1.md index 571cfc32671..14b9a76a8d4 100644 --- a/docs/release-notes/release-notes-0.20.1.md +++ b/docs/release-notes/release-notes-0.20.1.md @@ -103,6 +103,12 @@ # Technical and Architectural Updates ## BOLT Spec Updates +* [Enforce non-zero timestamps](https://github.com/lightningnetwork/lnd/pull/10469) + for `channel_update` (as required by BOLT 7) and `node_announcement` messages. + Gossip messages with zero timestamps are now rejected. For `channel_update` + messages, remote peers sending such invalid messages will have their ban score + incremented. + ## Testing ## Database