From 9c37a9e118c004f7c38c7389eb00219a314504e4 Mon Sep 17 00:00:00 2001 From: Alex Pyattaev Date: Mon, 3 Feb 2025 15:36:18 +0000 Subject: [PATCH 1/5] add validation of allowed CrdsValues in gossip Protocol enum variants can now be pruned better in Sanitize impl --- gossip/src/protocol.rs | 43 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 39 insertions(+), 4 deletions(-) diff --git a/gossip/src/protocol.rs b/gossip/src/protocol.rs index 4c9beb3b48f44e..eaa68c5f0bb014 100644 --- a/gossip/src/protocol.rs +++ b/gossip/src/protocol.rs @@ -1,6 +1,7 @@ +//! Definitions for the base of all Gossip protocol messages use { crate::{ - crds_data::MAX_WALLCLOCK, + crds_data::{CrdsData, MAX_WALLCLOCK}, crds_gossip_pull::CrdsFilter, crds_value::CrdsValue, ping_pong::{self, Pong}, @@ -42,10 +43,10 @@ const GOSSIP_PING_TOKEN_SIZE: usize = 32; pub(crate) const PULL_RESPONSE_MIN_SERIALIZED_SIZE: usize = 161; // TODO These messages should go through the gpu pipeline for spam filtering +/// Gossip protocol messages base enum #[derive(Serialize, Deserialize, Debug)] #[allow(clippy::large_enum_variant)] pub(crate) enum Protocol { - /// Gossip protocol messages PullRequest(CrdsFilter, CrdsValue), PullResponse(Pubkey, Vec), PushMessage(Pubkey, Vec), @@ -152,10 +153,44 @@ impl Sanitize for Protocol { match self { Protocol::PullRequest(filter, val) => { filter.sanitize()?; + // PullRequest is only allowed to have ContactInfo in its CrdsData + match val.data() { + CrdsData::LegacyContactInfo(_) => {} + CrdsData::ContactInfo(_) => {} + _ => { + return Err(SanitizeError::InvalidValue); + } + } val.sanitize() } - Protocol::PullResponse(_, val) => val.sanitize(), - Protocol::PushMessage(_, val) => val.sanitize(), + Protocol::PullResponse(_, val) => { + // PullResponse is allowed to carry anything in its CrdsData, except for deprecated fields + for v in val { + match v.data() { + CrdsData::LegacyVersion(_) => { + return Err(SanitizeError::InvalidValue); + } + _ => { + v.sanitize()?; + } + } + } + Ok(()) + } + Protocol::PushMessage(_, val) => { + //Push is allowed to carry anything in its CrdsData, except for deprecated fields + for v in val { + match v.data() { + CrdsData::LegacyVersion(_) => { + return Err(SanitizeError::InvalidValue); + } + _ => { + v.sanitize()?; + } + } + } + Ok(()) + } Protocol::PruneMessage(from, val) => { if *from != val.pubkey { Err(SanitizeError::InvalidValue) From bd967a0ae6dfdc65d176bb54b2eadb5679074260 Mon Sep 17 00:00:00 2001 From: Alex Pyattaev Date: Tue, 4 Feb 2025 07:45:57 +0200 Subject: [PATCH 2/5] Update gossip/src/protocol.rs Style change for PullRequest sanitize. Co-authored-by: Greg Cusack --- gossip/src/protocol.rs | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/gossip/src/protocol.rs b/gossip/src/protocol.rs index eaa68c5f0bb014..78551a687e0236 100644 --- a/gossip/src/protocol.rs +++ b/gossip/src/protocol.rs @@ -154,14 +154,10 @@ impl Sanitize for Protocol { Protocol::PullRequest(filter, val) => { filter.sanitize()?; // PullRequest is only allowed to have ContactInfo in its CrdsData - match val.data() { - CrdsData::LegacyContactInfo(_) => {} - CrdsData::ContactInfo(_) => {} - _ => { - return Err(SanitizeError::InvalidValue); - } + if let CrdsData::LegacyContactInfo(_) | CrdsData::ContactInfo(_) = val.data() { + return val.sanitize(); } - val.sanitize() + return Err(SanitizeError::InvalidValue); } Protocol::PullResponse(_, val) => { // PullResponse is allowed to carry anything in its CrdsData, except for deprecated fields From 97b5712b1a0f7b59ed190a7af922b7709c28aa39 Mon Sep 17 00:00:00 2001 From: Alex Pyattaev Date: Tue, 4 Feb 2025 09:45:02 +0000 Subject: [PATCH 3/5] ban LegacyVersion CRDS --- gossip/src/crds_data.rs | 2 +- gossip/src/protocol.rs | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/gossip/src/crds_data.rs b/gossip/src/crds_data.rs index 67cd680647f1ce..b2961e0c3ae369 100644 --- a/gossip/src/crds_data.rs +++ b/gossip/src/crds_data.rs @@ -89,7 +89,7 @@ impl Sanitize for CrdsData { } val.sanitize() } - CrdsData::LegacyVersion(version) => version.sanitize(), + CrdsData::LegacyVersion(_) => Err(SanitizeError::InvalidValue), CrdsData::Version(version) => version.sanitize(), CrdsData::NodeInstance(node) => node.sanitize(), CrdsData::DuplicateShred(ix, shred) => { diff --git a/gossip/src/protocol.rs b/gossip/src/protocol.rs index 78551a687e0236..8429b3420b75fa 100644 --- a/gossip/src/protocol.rs +++ b/gossip/src/protocol.rs @@ -154,10 +154,10 @@ impl Sanitize for Protocol { Protocol::PullRequest(filter, val) => { filter.sanitize()?; // PullRequest is only allowed to have ContactInfo in its CrdsData - if let CrdsData::LegacyContactInfo(_) | CrdsData::ContactInfo(_) = val.data() { - return val.sanitize(); + match val.data() { + CrdsData::LegacyContactInfo(_) | CrdsData::ContactInfo(_) => val.sanitize(), + _ => Err(SanitizeError::InvalidValue), } - return Err(SanitizeError::InvalidValue); } Protocol::PullResponse(_, val) => { // PullResponse is allowed to carry anything in its CrdsData, except for deprecated fields From 814abfa6027926021eed8189bd6981d096bed592 Mon Sep 17 00:00:00 2001 From: Alex Pyattaev Date: Tue, 4 Feb 2025 22:23:34 +0000 Subject: [PATCH 4/5] can not ban LegacyVersion yet --- gossip/src/crds_data.rs | 2 +- gossip/src/protocol.rs | 15 +++------------ 2 files changed, 4 insertions(+), 13 deletions(-) diff --git a/gossip/src/crds_data.rs b/gossip/src/crds_data.rs index b2961e0c3ae369..67cd680647f1ce 100644 --- a/gossip/src/crds_data.rs +++ b/gossip/src/crds_data.rs @@ -89,7 +89,7 @@ impl Sanitize for CrdsData { } val.sanitize() } - CrdsData::LegacyVersion(_) => Err(SanitizeError::InvalidValue), + CrdsData::LegacyVersion(version) => version.sanitize(), CrdsData::Version(version) => version.sanitize(), CrdsData::NodeInstance(node) => node.sanitize(), CrdsData::DuplicateShred(ix, shred) => { diff --git a/gossip/src/protocol.rs b/gossip/src/protocol.rs index 8429b3420b75fa..070a89e59fd2b1 100644 --- a/gossip/src/protocol.rs +++ b/gossip/src/protocol.rs @@ -160,18 +160,9 @@ impl Sanitize for Protocol { } } Protocol::PullResponse(_, val) => { - // PullResponse is allowed to carry anything in its CrdsData, except for deprecated fields - for v in val { - match v.data() { - CrdsData::LegacyVersion(_) => { - return Err(SanitizeError::InvalidValue); - } - _ => { - v.sanitize()?; - } - } - } - Ok(()) + // PullResponse is allowed to carry anything in its CrdsData, including deprecated Crds + // such that a deprecated Crds does not get pulled and then rejected. + val.sanitize() } Protocol::PushMessage(_, val) => { //Push is allowed to carry anything in its CrdsData, except for deprecated fields From da0b1e5d6da8d984ef8b3bdde7dc6cb7837d869e Mon Sep 17 00:00:00 2001 From: Alex Pyattaev Date: Tue, 4 Feb 2025 22:41:05 +0000 Subject: [PATCH 5/5] also allow ingestion of Push with LegacyVersion to prevent amplifications --- gossip/src/protocol.rs | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/gossip/src/protocol.rs b/gossip/src/protocol.rs index 070a89e59fd2b1..11499be91e8612 100644 --- a/gossip/src/protocol.rs +++ b/gossip/src/protocol.rs @@ -165,18 +165,10 @@ impl Sanitize for Protocol { val.sanitize() } Protocol::PushMessage(_, val) => { - //Push is allowed to carry anything in its CrdsData, except for deprecated fields - for v in val { - match v.data() { - CrdsData::LegacyVersion(_) => { - return Err(SanitizeError::InvalidValue); - } - _ => { - v.sanitize()?; - } - } - } - Ok(()) + // PushMessage is allowed to carry anything in its CrdsData, including deprecated Crds + // such that a deprecated Crds gets ingested instead of the node having to pull it from + // other nodes that have inserted it into their Crds table + val.sanitize() } Protocol::PruneMessage(from, val) => { if *from != val.pubkey {