discovery: enforce strict validation of peer gossip messages

[NishantBansal2003]

While fuzzing the gossip state machine, I found that LND does not validate the chain hash in ReplyChannelRange, ReplyShortChanIDsEnd and GossipTimestampRange messages. It would be safer to enforce strict validation on these messages to prevent malformed responses from being processed. I verified that CLN performs chain hash validation for these message types.

Additionally, we can add validation to ensure that if a timestamp is provided, it is provided for each ShortChanID in ReplyChannelRange. Although this is already verified during encode/decode, enforcing the check here adds defensive safety. Without it, a panic could occur if validation is ever missing in the Decode function.

[gemini-code-assist]

Summary of Changes

Hello @NishantBansal2003, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly enhances the robustness of the gossip state machine by introducing strict validation for incoming peer gossip messages. Specifically, it ensures that ReplyChannelRange, ReplyShortChanIDsEnd, and GossipTimestampRange messages contain the correct chain hash, preventing the processing of messages intended for different networks. Additionally, ReplyChannelRange messages now undergo a defensive check to confirm that the number of provided timestamps aligns with the number of short channel IDs, mitigating potential panics. These changes improve the network's resilience against malformed or malicious gossip data.

Highlights

• Strict Chain Hash Validation: Implemented strict chain hash validation for ReplyChannelRange, ReplyShortChanIDsEnd, and GossipTimestampRange messages to prevent processing malformed responses from peers and ensure messages pertain to the correct blockchain network.
• Defensive Timestamp Validation: Added validation to ReplyChannelRange messages to ensure that if timestamps are provided, their count matches the number of Short Channel IDs, enhancing defensive safety against potential panics.
• Code Refactoring and Test Coverage: Refactored the processing of ReplyShortChanIDsEnd messages into a dedicated function and introduced comprehensive unit tests to cover the new validation logic for malformed gossip messages.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Changelog

• discovery/syncer.go
  • Updated a log message from 'query' to 'reply' for channel range processing.
  • Introduced a new function processScidEndReply to handle ReplyShortChanIDsEnd messages, including chain hash validation.
  • Modified the main channelGraphSyncer loop to call processScidEndReply for ReplyShortChanIDsEnd messages.
  • Added chain hash validation to processChanRangeReply.
  • Implemented a check in processChanRangeReply to ensure the count of timestamps matches the count of short channel IDs.
  • Added chain hash validation to ApplyGossipFilter for GossipTimestampRange messages.
• discovery/syncer_test.go
  • Initialized chainHash in newTestSyncer configuration.
  • Added ChainHash field to GossipTimestampRange messages in TestGossipSyncerApplyNoHistoricalGossipFilter, TestGossipSyncerApplyGossipFilter, and TestGossipSyncerSyncTransitions tests.
  • Added ChainHash field to ChannelAnnouncement1 and ChannelUpdate1 messages in TestGossipSyncerReplyShortChanIDs.
  • Added ChainHash field to QueryShortChanIDs messages in TestGossipSyncerReplyShortChanIDs.
  • Added ChainHash field to QueryChannelRange messages in TestGossipSyncerReplyChanRangeQuery, TestGossipSyncerReplyChanRangeQueryBlockRange, TestGossipSyncerReplyChanRangeQueryNoNewChans, and TestGossipSyncerHistoricalSync tests.
  • Added ChainHash field to ReplyChannelRange messages in testGossipSyncerProcessChanRangeReply.
  • Introduced TestGossipSyncerProcessMalformedChanRangeReply to verify rejection of ReplyChannelRange messages with incorrect chain hash or mismatched timestamp/SCID counts.
  • Introduced TestGossipSyncerProcessMalformedShortChanIDsEndReply to verify rejection of ReplyShortChanIDsEnd messages with incorrect chain hash.
  • Introduced TestGossipSyncerProcessMalformedGossipTimestampRange to verify rejection of GossipTimestampRange messages with incorrect chain hash.

Activity

• No human activity has been recorded on this pull request yet.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[lightninglabs-deploy]

🟠 PR Severity: HIGH

discovery/syncer.go | 1 file | 61 lines changed

🟠 High (1 file)

• discovery/syncer.go - Gossip protocol message validation

Analysis

This PR adds strict validation for chain hash in gossip messages (ReplyChannelRange, ReplyShortChanIDsEnd, GossipTimestampRange) and validates timestamp consistency in ReplyChannelRange. The changes affect the gossip state machine's message processing logic.

Why HIGH severity:

• The discovery/* package handles the gossip protocol, a core networking component
• Changes modify message validation logic in the syncer, which processes peer gossip messages
• While the changes add defensive checks (improving security), they touch critical path code that processes untrusted peer input
• Improper validation could lead to accepting malformed messages or rejecting valid ones, affecting network participation

Key review areas:

• Verify chain hash validation matches spec requirements and doesn't break interoperability with other implementations
• Ensure the timestamp validation logic correctly handles edge cases
• Confirm error handling doesn't break existing gossip sync flows

---

_{To override, add a severity-override-{critical,high,medium,low} label.}

[gemini-code-assist]

Code Review

This is a solid pull request that hardens the gossip message processing by adding strict validation for the chain hash in ReplyChannelRange, ReplyShortChanIDsEnd, and GossipTimestampRange messages. The defensive check for timestamp consistency in ReplyChannelRange is also a good addition for robustness.

The changes are well-implemented and follow existing patterns in the codebase. The refactoring to introduce processScidEndReply improves the structure of the channelGraphSyncer. The new tests for malformed messages are comprehensive and directly verify the new validation logic.

Overall, this is a high-quality contribution that improves the security and stability of the gossip syncer. I have no further recommendations.

[Roasbeef]

FWIW we verify this already during decode:

lnd/lnwire/reply_channel_range.go

Lines 112 to 116 in f9e3825

	// Check that a timestamp was provided for each SCID.
	if len(c.Timestamps) != len(c.ShortChanIDs) {
	return fmt.Errorf("number of timestamps does not " +
	"match number of SCIDs")
	}

[NishantBansal2003]

True, I already mentioned this in the comments and PR description. The reason I am still adding this check is:

• For additional safety and to make the requirement more explicit during final processing
• When doing gossip state machine fuzzing, I bypass the normal message encode/decode path and send messages directly, which skips this validation and can lead to false positive panic reports