From b36966be3a968aced14ff185dc8235175011732a Mon Sep 17 00:00:00 2001 From: Sats And Sports Date: Wed, 15 Apr 2026 11:03:07 +0000 Subject: [PATCH] Tighten TreeAnnounce validation to match spanning tree specification Adds TreeAnnounce::validate_semantics() called from handle_tree_announce before any tree-state mutation. Enforces that the ancestry accompanying a parent declaration conforms to the spanning tree rules: - first ancestry entry matches the signed sender - is_root declarations carry a single-entry ancestry - non-root declarations include the signed parent as the second entry - the advertised root is the minimum node_addr in the ancestry Non-conforming announcements are rejected with a warn log and no state change. Adds unit tests for each rejected shape plus an integration test covering the full receive path in a two-node tree. Co-authored-by: Johnathan Corgan --- CHANGELOG.md | 4 + src/node/tests/spanning_tree.rs | 73 ++++++++++++ src/node/tree.rs | 9 ++ src/protocol/tree.rs | 192 +++++++++++++++++++++++++++++++- src/tree/mod.rs | 28 +++++ 5 files changed, 305 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4873bd5..93bf41b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,6 +44,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 bind with `EAFNOSUPPORT` ([#61](https://github.com/jmcorgan/fips/issues/61), reported by [@SwapMarket](https://github.com/SwapMarket)) +- Tighten TreeAnnounce ancestry validation to match the spanning + tree specification. The receive path now verifies that the + ancestry is structurally consistent with the signed parent + declaration before mutating tree state. ## [0.2.0] - 2026-03-22 diff --git a/src/node/tests/spanning_tree.rs b/src/node/tests/spanning_tree.rs index bcc85e8..ee7c796 100644 --- a/src/node/tests/spanning_tree.rs +++ b/src/node/tests/spanning_tree.rs @@ -5,6 +5,8 @@ //! reused by bloom filter tests. use super::*; +use crate::protocol::TreeAnnounce; +use crate::tree::{CoordEntry, ParentDeclaration, TreeCoordinate}; /// A test node bundling a Node with its transport and packet channel. pub(super) struct TestNode { @@ -717,3 +719,74 @@ async fn test_spanning_tree_disconnected() { verify_tree_convergence_components(&nodes, &[vec![0, 1, 2], vec![3, 4, 5]]); cleanup_nodes(&mut nodes).await; } + +/// Tests that a node ignores a signed TreeAnnounce whose advertised root is not the smallest node_addr in the ancestry. +#[tokio::test] +async fn test_rejects_tree_announce_with_inconsistent_root() { + // Start from a healthy 2-node tree so node B already has a normal, trusted + // view of node A's coordinates. + let mut nodes = run_tree_test(2, &[(0, 1)], false).await; + + let a_addr = *nodes[0].node.node_addr(); + let current_root = *nodes[1].node.tree_state().root(); + let current_depth = nodes[1].node.tree_state().my_coords().depth(); + let peer_coords_before = nodes[1] + .node + .get_peer(&a_addr) + .unwrap() + .coords() + .unwrap() + .clone(); + let accepted_before = nodes[1].node.stats().tree.accepted; + + // Use two fixed synthetic ancestors so the forged path is explicit: + // - fake_parent = 00000000000000000000000000000000 + // - fake_root = 00000000000000000000000000000001 + // + // The forged ancestry is therefore: + // [A, 000...000, 000...001] + // + // That makes 000...001 the advertised root because it is the final entry, + // even though 000...000 is smaller and appears earlier in the path. + let fake_parent = NodeAddr::from_bytes([0u8; 16]); + let mut fake_root_bytes = [0u8; 16]; + fake_root_bytes[15] = 1; + let fake_root = NodeAddr::from_bytes(fake_root_bytes); + + // Sign a fresh declaration from A. The 99/12345 values are just a newer + // sequence/timestamp so the announce would be acceptable on freshness + // grounds if its ancestry semantics were valid. + let mut declaration = ParentDeclaration::new(a_addr, fake_parent, 99, 12345); + declaration.sign(nodes[0].node.identity()).unwrap(); + + let announce = TreeAnnounce::new( + declaration, + TreeCoordinate::new(vec![ + CoordEntry::new(a_addr, 99, 12345), + CoordEntry::new(fake_parent, 98, 12344), + CoordEntry::new(fake_root, 97, 12343), + ]) + .unwrap(), + ); + let encoded = announce.encode().unwrap(); + + nodes[1] + .node + .handle_tree_announce(&a_addr, &encoded[1..]) + .await; + + // B should reject the malformed ancestry before mutating either its local + // tree state or its cached view of peer A. + assert_eq!(*nodes[1].node.tree_state().root(), current_root); + assert_eq!( + nodes[1].node.tree_state().my_coords().depth(), + current_depth + ); + assert_eq!(nodes[1].node.stats().tree.accepted, accepted_before); + assert_eq!( + nodes[1].node.get_peer(&a_addr).unwrap().coords().unwrap(), + &peer_coords_before + ); + + cleanup_nodes(&mut nodes).await; +} diff --git a/src/node/tree.rs b/src/node/tree.rs index 9fd9906..88a8261 100644 --- a/src/node/tree.rs +++ b/src/node/tree.rs @@ -172,6 +172,15 @@ impl Node { return; } + if let Err(e) = announce.validate_semantics() { + warn!( + from = %self.peer_display_name(from), + error = %e, + "Rejected TreeAnnounce with invalid ancestry" + ); + return; + } + let now_ms = std::time::SystemTime::now() .duration_since(std::time::UNIX_EPOCH) .map(|d| d.as_millis() as u64) diff --git a/src/protocol/tree.rs b/src/protocol/tree.rs index ca65f0c..1f0d54e 100644 --- a/src/protocol/tree.rs +++ b/src/protocol/tree.rs @@ -3,7 +3,7 @@ use super::error::ProtocolError; use super::link::LinkMessageType; use crate::NodeAddr; -use crate::tree::{CoordEntry, ParentDeclaration, TreeCoordinate}; +use crate::tree::{CoordEntry, ParentDeclaration, TreeCoordinate, TreeError}; use secp256k1::schnorr::Signature; /// Spanning tree announcement carrying parent declaration and ancestry. @@ -35,6 +35,58 @@ impl TreeAnnounce { } } + /// Validate that the ancestry is structurally consistent with the signed + /// declaration. + /// + /// Expected properties: + /// - the first ancestry entry is the declaring node's `node_addr` + /// - a root declaration has exactly one ancestry entry + /// - a non-root declaration has at least two ancestry entries + /// - for a non-root declaration, the second ancestry entry matches `parent_id` + /// - the final ancestry entry is the advertised root + /// - the advertised root is the smallest `node_addr` in the ancestry + pub fn validate_semantics(&self) -> Result<(), TreeError> { + let entries = self.ancestry.entries(); + let declared_node = *self.declaration.node_addr(); + let declared_parent = *self.declaration.parent_id(); + + if entries[0].node_addr != declared_node { + return Err(TreeError::AncestryNodeMismatch { + declared: declared_node, + ancestry: entries[0].node_addr, + }); + } + + if self.declaration.is_root() { + if entries.len() != 1 { + return Err(TreeError::RootDeclarationMismatch); + } + } else { + let ancestry_parent = entries.get(1).ok_or(TreeError::AncestryTooShort)?.node_addr; + if ancestry_parent != declared_parent { + return Err(TreeError::AncestryParentMismatch { + declared: declared_parent, + ancestry: ancestry_parent, + }); + } + } + + let advertised_root = *self.ancestry.root_id(); + let minimum = entries + .iter() + .map(|entry| entry.node_addr) + .min() + .expect("TreeCoordinate is never empty"); + if advertised_root != minimum { + return Err(TreeError::AncestryRootNotMinimum { + advertised: advertised_root, + minimum, + }); + } + + Ok(()) + } + /// Encode as link-layer plaintext (includes msg_type byte). /// /// The declaration must be signed. The encoded format is: @@ -381,4 +433,142 @@ mod tests { let result = announce.encode(); assert!(matches!(result, Err(ProtocolError::InvalidSignature))); } + + /// Tests that a well-formed non-root ancestry is accepted. + #[test] + fn test_tree_announce_validate_semantics_accepts_valid_non_root() { + use crate::identity::Identity; + + let identity = Identity::generate(); + let node_addr = *identity.node_addr(); + let parent = make_node_addr(2); + let root = make_node_addr(1); + + let mut decl = ParentDeclaration::new(node_addr, parent, 5, 1000); + decl.sign(&identity).unwrap(); + + let ancestry = TreeCoordinate::new(vec![ + CoordEntry::new(node_addr, 5, 1000), + CoordEntry::new(parent, 4, 900), + CoordEntry::new(root, 3, 800), + ]) + .unwrap(); + + let announce = TreeAnnounce::new(decl, ancestry); + assert!(announce.validate_semantics().is_ok()); + } + + /// Tests that an ancestry is rejected if the final node_addr is not the smallest entry in the path. + #[test] + fn test_tree_announce_validate_semantics_rejects_non_minimal_root() { + use crate::identity::Identity; + + let identity = Identity::generate(); + let node_addr = *identity.node_addr(); + let smaller = make_node_addr(0); + let advertised_root = make_node_addr(1); + + let mut decl = ParentDeclaration::new(node_addr, smaller, 5, 1000); + decl.sign(&identity).unwrap(); + + let ancestry = TreeCoordinate::new(vec![ + CoordEntry::new(node_addr, 5, 1000), + CoordEntry::new(smaller, 4, 900), + CoordEntry::new(advertised_root, 3, 800), + ]) + .unwrap(); + + let announce = TreeAnnounce::new(decl, ancestry); + assert!(matches!( + announce.validate_semantics(), + Err(TreeError::AncestryRootNotMinimum { + advertised, + minimum, + }) if advertised == advertised_root && minimum == smaller + )); + } + + /// Tests that an ancestry is rejected if the first ancestry hop does not match the signed parent_id. + #[test] + fn test_tree_announce_validate_semantics_rejects_parent_mismatch() { + use crate::identity::Identity; + + let identity = Identity::generate(); + let node_addr = *identity.node_addr(); + let declared_parent = make_node_addr(2); + let ancestry_parent = make_node_addr(3); + + let mut decl = ParentDeclaration::new(node_addr, declared_parent, 5, 1000); + decl.sign(&identity).unwrap(); + + let ancestry = TreeCoordinate::new(vec![ + CoordEntry::new(node_addr, 5, 1000), + CoordEntry::new(ancestry_parent, 4, 900), + CoordEntry::new(make_node_addr(1), 3, 800), + ]) + .unwrap(); + + let announce = TreeAnnounce::new(decl, ancestry); + assert!(matches!( + announce.validate_semantics(), + Err(TreeError::AncestryParentMismatch { + declared, + ancestry, + }) if declared == declared_parent && ancestry == ancestry_parent + )); + } + + /// Tests that an ancestry is rejected if the first path entry does not match the signed sender node_addr. + #[test] + fn test_tree_announce_validate_semantics_rejects_sender_mismatch() { + use crate::identity::Identity; + + let identity = Identity::generate(); + let node_addr = *identity.node_addr(); + let ancestry_sender = make_node_addr(9); + let parent = make_node_addr(2); + + let mut decl = ParentDeclaration::new(node_addr, parent, 5, 1000); + decl.sign(&identity).unwrap(); + + let ancestry = TreeCoordinate::new(vec![ + CoordEntry::new(ancestry_sender, 5, 1000), + CoordEntry::new(parent, 4, 900), + CoordEntry::new(make_node_addr(1), 3, 800), + ]) + .unwrap(); + + let announce = TreeAnnounce::new(decl, ancestry); + assert!(matches!( + announce.validate_semantics(), + Err(TreeError::AncestryNodeMismatch { + declared, + ancestry, + }) if declared == node_addr && ancestry == ancestry_sender + )); + } + + /// Tests that a self-root declaration is rejected if its ancestry contains extra ancestors. + #[test] + fn test_tree_announce_validate_semantics_rejects_root_with_ancestors() { + use crate::identity::Identity; + + let identity = Identity::generate(); + let node_addr = *identity.node_addr(); + + let mut decl = ParentDeclaration::self_root(node_addr, 5, 1000); + decl.sign(&identity).unwrap(); + + let ancestry = TreeCoordinate::new(vec![ + CoordEntry::new(node_addr, 5, 1000), + CoordEntry::new(make_node_addr(0), 4, 900), + ]) + .unwrap(); + + let announce = TreeAnnounce::new(decl, ancestry); + assert!(matches!( + announce.validate_semantics(), + Err(TreeError::RootDeclarationMismatch) + )); + } } diff --git a/src/tree/mod.rs b/src/tree/mod.rs index 47354d7..21a1d72 100644 --- a/src/tree/mod.rs +++ b/src/tree/mod.rs @@ -25,6 +25,34 @@ pub enum TreeError { #[error("invalid ancestry: does not reach claimed root")] AncestryNotToRoot, + #[error("invalid ancestry: root declaration must contain only the sender")] + RootDeclarationMismatch, + + #[error("invalid ancestry: non-root declaration must include a parent hop")] + AncestryTooShort, + + #[error("invalid ancestry: sender {declared} does not match first path entry {ancestry}")] + AncestryNodeMismatch { + declared: NodeAddr, + ancestry: NodeAddr, + }, + + #[error( + "invalid ancestry: signed parent {declared} does not match first ancestry hop {ancestry}" + )] + AncestryParentMismatch { + declared: NodeAddr, + ancestry: NodeAddr, + }, + + #[error( + "invalid ancestry: advertised root {advertised} is not the minimum path entry {minimum}" + )] + AncestryRootNotMinimum { + advertised: NodeAddr, + minimum: NodeAddr, + }, + #[error("signature verification failed for node {0:?}")] InvalidSignature(NodeAddr),