fuzz: add fuzz target for P2PGossipSync gossip message handling#4532
fuzz: add fuzz target for P2PGossipSync gossip message handling#4532NishantBansal2003 wants to merge 1 commit intolightningdevkit:mainfrom
Conversation
|
I've assigned @joostjager as a reviewer! |
Signed-off-by: Nishant Bansal <nishant.bansal.282003@gmail.com>
NishantBansal2003
force-pushed
the
fuzz-gossip
branch
from
cb9e2a8 to
d29150d
Compare
|
|
||
| let graph = network_graph.read_only(); | ||
| let node_id = NodeId::from_pubkey(&peer.node_id); | ||
| let node = graph.node(&node_id).unwrap(); | ||
| let info = node.announcement_info.as_ref().unwrap(); | ||
| assert_eq!(info.last_update(), timestamp); |
There was a problem hiding this comment.
The Ok branch assertions look up peer.node_id (the original, pre-mutation value), but if the node_id was mutated at line 184-186, the accepted announcement would be stored under ann.contents.node_id (the mutated value). In secp256k1_fuzz mode, signature verification is simplified and could theoretically allow a mutated node_id to pass, causing graph.node(&NodeId::from_pubkey(&peer.node_id)).unwrap() to panic on a node that was never updated.
To be robust regardless of fuzz-mode crypto behavior, this should use the actual message values:
| let graph = network_graph.read_only(); | |
| let node_id = NodeId::from_pubkey(&peer.node_id); | |
| let node = graph.node(&node_id).unwrap(); | |
| let info = node.announcement_info.as_ref().unwrap(); | |
| assert_eq!(info.last_update(), timestamp); | |
| let graph = network_graph.read_only(); | |
| let node = graph.node(&ann.contents.node_id).unwrap(); | |
| let info = node.announcement_info.as_ref().unwrap(); | |
| assert_eq!(info.last_update(), timestamp); |
| let graph = network_graph.read_only(); | ||
| let chan = graph.channel(scid).unwrap(); | ||
| assert_eq!(chan.node_one, NodeId::from_pubkey(&peer1.node_id)); | ||
| assert_eq!(chan.node_two, NodeId::from_pubkey(&peer2.node_id)); | ||
|
|
||
| let node1_id = NodeId::from_pubkey(&peer1.node_id); | ||
| let node2_id = NodeId::from_pubkey(&peer2.node_id); | ||
| assert!(graph.node(&node1_id).is_some()); | ||
| assert!(graph.node(&node2_id).is_some()); |
There was a problem hiding this comment.
Same issue as the node announcement case: these assertions use the original peer1.node_id/peer2.node_id, but the node_id_1/node_id_2 fields may have been mutated at lines 238-246. If a mutation passes signature verification in secp256k1_fuzz mode, chan.node_one/chan.node_two would hold the mutated values, causing the assert_eq! to fail spuriously.
Use the actual announcement contents instead:
| let graph = network_graph.read_only(); | |
| let chan = graph.channel(scid).unwrap(); | |
| assert_eq!(chan.node_one, NodeId::from_pubkey(&peer1.node_id)); | |
| assert_eq!(chan.node_two, NodeId::from_pubkey(&peer2.node_id)); | |
| let node1_id = NodeId::from_pubkey(&peer1.node_id); | |
| let node2_id = NodeId::from_pubkey(&peer2.node_id); | |
| assert!(graph.node(&node1_id).is_some()); | |
| assert!(graph.node(&node2_id).is_some()); | |
| let graph = network_graph.read_only(); | |
| let chan = graph.channel(scid).unwrap(); | |
| assert_eq!(chan.node_one, ann.contents.node_id_1); | |
| assert_eq!(chan.node_two, ann.contents.node_id_2); | |
| assert!(graph.node(&ann.contents.node_id_1).is_some()); | |
| assert!(graph.node(&ann.contents.node_id_2).is_some()); |
Review SummaryOne new issue found (plus two from prior review that are still unresolved): New inline comment
Prior comments (still unresolved)
No other bugs, security issues, or logic errors found. The fuzz target structure is sound, the UTXO mock is correct, event assertions for |
| @@ -0,0 +1,462 @@ | |||
| //! Test that no series of gossip messages received from peers can result in a crash. We do this | |||
There was a problem hiding this comment.
Nit: This file is missing the standard licensing header that other fuzz targets (e.g., full_stack.rs, chanmon_consistency.rs, feature_flags.rs) include at the top:
// This file is Copyright its original authors, visible in version control
// history.
//
// This file is licensed under the Apache License, Version 2.0 <LICENSE-APACHE
// or http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your option.
// You may not use this file except in accordance with one or both of these
// licenses.
The doc comment (//! Test that ...) should come after this header.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4532 +/- ##
=======================================
Coverage 87.10% 87.11%
=======================================
Files 163 163
Lines 108740 108740
Branches 108740 108740
=======================================
+ Hits 94723 94730 +7
+ Misses 11531 11526 -5
+ Partials 2486 2484 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
Cool, thanks! Have you run any comparisons on the coverage of this vs the router fuzzer? I'm wondering if it makes sense to add a new fuzzer here or focus more CPU on the router fuzzer given it tests very similar codepaths.
Coverage wise, the additional benefit is that P2PGossipSync also gets covered, so that's a plus. My main goal was to cover all announcement messages in the fuzz tests (which the One option is to extend the OR, we can keep a separate fuzz target ( WDYT? I'm fine with either approach since both will ultimately achieve full coverage of gossip message processing. |
4 participants
Added gossip state machine fuzz tests for gossip discovery state handling. In these fuzz tests, we read bytes from the fuzz input to determine actions such as connecting peers, feeding channel announcements, node announcements, channel updates, or query messages. Both valid and malformed messages are generated to exercise error paths.
The reason for adding these fuzz tests is that
P2PGossipSyncis the ultimate sink for gossip messages received from peers, so there must not be any crashes or unintended behavior in LDK when handling messages from potentially malicious peers.I only included the states that LDK currently handles. For example, I did not add
reply_channel_rangeorquery_scidsmessages since LDK does not process them at the moment. Similarly, sinceP2PGossipSyncdoes not handlegossip_timestamp_filter, it is not included in these fuzz tests. Additionally, there is limited benefit in fuzzinggossip_timestamp_filterin LDK, as it is typically processed only once per peer and involves minimal logical processing.