From 261f4b713b43c60f488bfd38a844f953a6c51995 Mon Sep 17 00:00:00 2001 From: Alexandru Gheorghe <49718502+alexggh@users.noreply.github.com> Date: Wed, 31 Jan 2024 13:41:04 +0200 Subject: [PATCH] Fail early at obviously invalid approvals (#3152) Signed-off-by: Alexandru Gheorghe --- .../network/approval-distribution/src/lib.rs | 74 ++++++++++++++++--- .../approval-distribution/src/tests.rs | 27 +++++++ 2 files changed, 92 insertions(+), 9 deletions(-) diff --git a/polkadot/node/network/approval-distribution/src/lib.rs b/polkadot/node/network/approval-distribution/src/lib.rs index c5a8b0a6e9..f4e4027016 100644 --- a/polkadot/node/network/approval-distribution/src/lib.rs +++ b/polkadot/node/network/approval-distribution/src/lib.rs @@ -36,7 +36,9 @@ use polkadot_node_network_protocol::{ UnifiedReputationChange as Rep, Versioned, View, }; use polkadot_node_primitives::approval::{ - v1::{AssignmentCertKind, BlockApprovalMeta, IndirectAssignmentCert}, + v1::{ + AssignmentCertKind, BlockApprovalMeta, IndirectAssignmentCert, IndirectSignedApprovalVote, + }, v2::{ AsBitIndex, AssignmentCertKindV2, CandidateBitfield, IndirectAssignmentCertV2, IndirectSignedApprovalVoteV2, @@ -1063,17 +1065,17 @@ impl State { .await; }, Versioned::V3(protocol_v3::ApprovalDistributionMessage::Approvals(approvals)) => { - self.process_incoming_approvals(ctx, metrics, peer_id, approvals).await; + let sanitized_approvals = + self.sanitize_v2_approvals(peer_id, ctx.sender(), approvals).await; + self.process_incoming_approvals(ctx, metrics, peer_id, sanitized_approvals) + .await; }, Versioned::V1(protocol_v1::ApprovalDistributionMessage::Approvals(approvals)) | Versioned::V2(protocol_v2::ApprovalDistributionMessage::Approvals(approvals)) => { - self.process_incoming_approvals( - ctx, - metrics, - peer_id, - approvals.into_iter().map(|approval| approval.into()).collect::>(), - ) - .await; + let sanitized_approvals = + self.sanitize_v1_approvals(peer_id, ctx.sender(), approvals).await; + self.process_incoming_approvals(ctx, metrics, peer_id, sanitized_approvals) + .await; }, } } @@ -2196,6 +2198,60 @@ impl State { sanitized_assignments } + + // Filter out obviously invalid candidate indicies. + async fn sanitize_v1_approvals( + &mut self, + peer_id: PeerId, + sender: &mut impl overseer::ApprovalDistributionSenderTrait, + approval: Vec, + ) -> Vec { + let mut sanitized_approvals = Vec::new(); + for approval in approval.into_iter() { + if approval.candidate_index as usize > MAX_BITFIELD_SIZE { + // Punish the peer for the invalid message. + modify_reputation(&mut self.reputation, sender, peer_id, COST_OVERSIZED_BITFIELD) + .await; + gum::debug!( + target: LOG_TARGET, + block_hash = ?approval.block_hash, + candidate_index = ?approval.candidate_index, + "Bad approval v1, invalid candidate index" + ); + } else { + sanitized_approvals.push(approval.into()) + } + } + + sanitized_approvals + } + + // Filter out obviously invalid candidate indicies. + async fn sanitize_v2_approvals( + &mut self, + peer_id: PeerId, + sender: &mut impl overseer::ApprovalDistributionSenderTrait, + approval: Vec, + ) -> Vec { + let mut sanitized_approvals = Vec::new(); + for approval in approval.into_iter() { + if approval.candidate_indices.len() as usize > MAX_BITFIELD_SIZE { + // Punish the peer for the invalid message. + modify_reputation(&mut self.reputation, sender, peer_id, COST_OVERSIZED_BITFIELD) + .await; + gum::debug!( + target: LOG_TARGET, + block_hash = ?approval.block_hash, + candidate_indices_len = ?approval.candidate_indices.len(), + "Bad approval v2, invalid candidate indices size" + ); + } else { + sanitized_approvals.push(approval) + } + } + + sanitized_approvals + } } // This adjusts the required routing of messages in blocks that pass the block filter diff --git a/polkadot/node/network/approval-distribution/src/tests.rs b/polkadot/node/network/approval-distribution/src/tests.rs index 11d8293165..6c88dd53ad 100644 --- a/polkadot/node/network/approval-distribution/src/tests.rs +++ b/polkadot/node/network/approval-distribution/src/tests.rs @@ -3621,6 +3621,33 @@ fn import_versioned_approval() { assert_eq!(approvals.len(), 1); } ); + + // send an obviously invalid approval + let approval = IndirectSignedApprovalVote { + block_hash: hash, + // Invalid candidate index, should not pass sanitization. + candidate_index: 16777284, + validator: validator_index, + signature: dummy_signature(), + }; + let msg = protocol_v2::ApprovalDistributionMessage::Approvals(vec![approval.clone()]); + send_message_from_peer_v2(overseer, &peer_a, msg).await; + + expect_reputation_change(overseer, &peer_a, COST_OVERSIZED_BITFIELD).await; + + // send an obviously invalid approval + let approval = IndirectSignedApprovalVoteV2 { + block_hash: hash, + // Invalid candidates len, should not pass sanitization. + candidate_indices: 16777284.into(), + validator: validator_index, + signature: dummy_signature(), + }; + let msg = protocol_v3::ApprovalDistributionMessage::Approvals(vec![approval.clone()]); + send_message_from_peer_v3(overseer, &peer_a, msg).await; + + expect_reputation_change(overseer, &peer_a, COST_OVERSIZED_BITFIELD).await; + virtual_overseer }); }