From 6850c4792dd6a7b5ff9280e05f918f1ec7606a20 Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Mon, 21 Oct 2024 02:32:40 +0300 Subject: [PATCH 1/3] Detect invalid proposer signature on RPC block processing --- .../beacon_chain/src/block_verification.rs | 58 ++++++++++++++----- .../gossip_methods.rs | 3 +- beacon_node/network/src/sync/tests/lookups.rs | 6 +- 3 files changed, 48 insertions(+), 19 deletions(-) diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index 3ae19430aad..8b0dcb96bad 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -207,24 +207,18 @@ pub enum BlockError { /// /// The block is invalid and the peer is faulty. IncorrectBlockProposer { block: u64, local_shuffling: u64 }, - /// The proposal signature in invalid. - /// - /// ## Peer scoring - /// - /// The block is invalid and the peer is faulty. - ProposalSignatureInvalid, /// The `block.proposal_index` is not known. /// /// ## Peer scoring /// /// The block is invalid and the peer is faulty. UnknownValidator(u64), - /// A signature in the block is invalid (exactly which is unknown). + /// A signature in the block is invalid /// /// ## Peer scoring /// /// The block is invalid and the peer is faulty. - InvalidSignature, + InvalidSignature(InvalidSignature), /// The provided block is not from a later slot than its parent. /// /// ## Peer scoring @@ -328,6 +322,17 @@ pub enum BlockError { InternalError(String), } +/// Which specific signature(s) are invalid in a SignedBeaconBlock +#[derive(Debug)] +pub enum InvalidSignature { + // The outter signature in a SignedBeaconBlock + ProposerSignature, + // One or more signatures in BeaconBlockBody + BlockBodySignatures, + // One or more signatures in SignedBeaconBlock + Unknown, +} + impl From for BlockError { fn from(e: AvailabilityCheckError) -> Self { Self::AvailabilityCheck(e) @@ -522,7 +527,9 @@ pub enum BlockSlashInfo { impl BlockSlashInfo { pub fn from_early_error_block(header: SignedBeaconBlockHeader, e: BlockError) -> Self { match e { - BlockError::ProposalSignatureInvalid => BlockSlashInfo::SignatureInvalid(e), + BlockError::InvalidSignature(InvalidSignature::ProposerSignature) => { + BlockSlashInfo::SignatureInvalid(e) + } // `InvalidSignature` could indicate any signature in the block, so we want // to recheck the proposer signature alone. _ => BlockSlashInfo::SignatureNotChecked(header, e), @@ -651,7 +658,7 @@ pub fn signature_verify_chain_segment( } if signature_verifier.verify().is_err() { - return Err(BlockError::InvalidSignature); + return Err(BlockError::InvalidSignature(InvalidSignature::Unknown)); } drop(pubkey_cache); @@ -963,7 +970,9 @@ impl GossipVerifiedBlock { }; if !signature_is_valid { - return Err(BlockError::ProposalSignatureInvalid); + return Err(BlockError::InvalidSignature( + InvalidSignature::ProposerSignature, + )); } chain @@ -1097,7 +1106,26 @@ impl SignatureVerifiedBlock { parent: Some(parent), }) } else { - Err(BlockError::InvalidSignature) + // Re-verify the proposer signature in isolation to attribute fault + let pubkey = pubkey_cache + .get(block.message().proposer_index() as usize) + .ok_or_else(|| BlockError::UnknownValidator(block.message().proposer_index()))?; + if block.as_block().verify_signature( + Some(block_root), + pubkey, + &state.fork(), + chain.genesis_validators_root, + &chain.spec, + ) { + // Proposer signature is valid, the invalid signature must be in the body + Err(BlockError::InvalidSignature( + InvalidSignature::BlockBodySignatures, + )) + } else { + Err(BlockError::InvalidSignature( + InvalidSignature::ProposerSignature, + )) + } } } @@ -1152,7 +1180,9 @@ impl SignatureVerifiedBlock { consensus_context, }) } else { - Err(BlockError::InvalidSignature) + Err(BlockError::InvalidSignature( + InvalidSignature::BlockBodySignatures, + )) } } @@ -1979,7 +2009,7 @@ impl BlockBlobError for BlockError { } fn proposer_signature_invalid() -> Self { - BlockError::ProposalSignatureInvalid + BlockError::InvalidSignature(InvalidSignature::ProposerSignature) } } diff --git a/beacon_node/network/src/network_beacon_processor/gossip_methods.rs b/beacon_node/network/src/network_beacon_processor/gossip_methods.rs index e92f4504762..19d5432a714 100644 --- a/beacon_node/network/src/network_beacon_processor/gossip_methods.rs +++ b/beacon_node/network/src/network_beacon_processor/gossip_methods.rs @@ -1273,13 +1273,12 @@ impl NetworkBeaconProcessor { Err(e @ BlockError::StateRootMismatch { .. }) | Err(e @ BlockError::IncorrectBlockProposer { .. }) | Err(e @ BlockError::BlockSlotLimitReached) - | Err(e @ BlockError::ProposalSignatureInvalid) | Err(e @ BlockError::NonLinearSlots) | Err(e @ BlockError::UnknownValidator(_)) | Err(e @ BlockError::PerBlockProcessingError(_)) | Err(e @ BlockError::NonLinearParentRoots) | Err(e @ BlockError::BlockIsNotLaterThanParent { .. }) - | Err(e @ BlockError::InvalidSignature) + | Err(e @ BlockError::InvalidSignature(_)) | Err(e @ BlockError::WeakSubjectivityConflict) | Err(e @ BlockError::InconsistentFork(_)) | Err(e @ BlockError::ExecutionPayloadError(_)) diff --git a/beacon_node/network/src/sync/tests/lookups.rs b/beacon_node/network/src/sync/tests/lookups.rs index 9f2c9ef66f0..dd19481f677 100644 --- a/beacon_node/network/src/sync/tests/lookups.rs +++ b/beacon_node/network/src/sync/tests/lookups.rs @@ -1661,7 +1661,7 @@ fn test_parent_lookup_too_many_processing_attempts_must_blacklist() { rig.assert_not_failed_chain(block_root); // send the right parent but fail processing rig.parent_lookup_block_response(id, peer_id, Some(parent.clone().into())); - rig.parent_block_processed(block_root, BlockError::InvalidSignature.into()); + rig.parent_block_processed(block_root, BlockError::BlockSlotLimitReached.into()); rig.parent_lookup_block_response(id, peer_id, None); rig.expect_penalty(peer_id, "lookup_block_processing_failure"); } @@ -2555,7 +2555,7 @@ mod deneb_only { fn invalid_parent_processed(mut self) -> Self { self.rig.parent_block_processed( self.block_root, - BlockProcessingResult::Err(BlockError::ProposalSignatureInvalid), + BlockProcessingResult::Err(BlockError::BlockSlotLimitReached), ); assert_eq!(self.rig.active_parent_lookups_count(), 1); self @@ -2564,7 +2564,7 @@ mod deneb_only { fn invalid_block_processed(mut self) -> Self { self.rig.single_block_component_processed( self.block_req_id.expect("block request id").lookup_id, - BlockProcessingResult::Err(BlockError::ProposalSignatureInvalid), + BlockProcessingResult::Err(BlockError::BlockSlotLimitReached), ); self.rig.assert_single_lookups_count(1); self From 049f7541bc3f80cdcae23d077f197e945fe73e99 Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Thu, 21 Nov 2024 17:25:57 +0700 Subject: [PATCH 2/3] Update beacon tests --- beacon_node/beacon_chain/src/lib.rs | 2 +- .../beacon_chain/tests/block_verification.rs | 59 +++++++++++-------- 2 files changed, 36 insertions(+), 25 deletions(-) diff --git a/beacon_node/beacon_chain/src/lib.rs b/beacon_node/beacon_chain/src/lib.rs index 2953516fb1a..35527d76113 100644 --- a/beacon_node/beacon_chain/src/lib.rs +++ b/beacon_node/beacon_chain/src/lib.rs @@ -78,7 +78,7 @@ pub use beacon_fork_choice_store::{BeaconForkChoiceStore, Error as ForkChoiceSto pub use block_verification::{ build_blob_data_column_sidecars, get_block_root, BlockError, ExecutionPayloadError, ExecutionPendingBlock, GossipVerifiedBlock, IntoExecutionPendingBlock, IntoGossipVerifiedBlock, - PayloadVerificationOutcome, PayloadVerificationStatus, + InvalidSignature, PayloadVerificationOutcome, PayloadVerificationStatus, }; pub use block_verification_types::AvailabilityPendingExecutedBlock; pub use block_verification_types::ExecutedBlock; diff --git a/beacon_node/beacon_chain/tests/block_verification.rs b/beacon_node/beacon_chain/tests/block_verification.rs index f094a173eec..d04e7026d9b 100644 --- a/beacon_node/beacon_chain/tests/block_verification.rs +++ b/beacon_node/beacon_chain/tests/block_verification.rs @@ -9,7 +9,7 @@ use beacon_chain::{ }; use beacon_chain::{ BeaconSnapshot, BlockError, ChainConfig, ChainSegmentResult, IntoExecutionPendingBlock, - NotifyExecutionLayer, + InvalidSignature, NotifyExecutionLayer, }; use logging::test_logger; use slasher::{Config as SlasherConfig, Slasher}; @@ -436,7 +436,7 @@ async fn assert_invalid_signature( .process_chain_segment(blocks, NotifyExecutionLayer::Yes) .await .into_block_error(), - Err(BlockError::InvalidSignature) + Err(BlockError::InvalidSignature(InvalidSignature::Unknown)) ), "should not import chain segment with an invalid {} signature", item @@ -478,7 +478,12 @@ async fn assert_invalid_signature( ) .await; assert!( - matches!(process_res, Err(BlockError::InvalidSignature)), + matches!( + process_res, + Err(BlockError::InvalidSignature( + InvalidSignature::BlockBodySignatures + )) + ), "should not import individual block with an invalid {} signature, got: {:?}", item, process_res @@ -534,21 +539,25 @@ async fn invalid_signature_gossip_block() { .into_block_error() .expect("should import all blocks prior to the one being tested"); let signed_block = SignedBeaconBlock::from_block(block, junk_signature()); + let process_res = harness + .chain + .process_block( + signed_block.canonical_root(), + Arc::new(signed_block), + NotifyExecutionLayer::Yes, + BlockImportSource::Lookup, + || Ok(()), + ) + .await; assert!( matches!( - harness - .chain - .process_block( - signed_block.canonical_root(), - Arc::new(signed_block), - NotifyExecutionLayer::Yes, - BlockImportSource::Lookup, - || Ok(()), - ) - .await, - Err(BlockError::InvalidSignature) + process_res, + Err(BlockError::InvalidSignature( + InvalidSignature::ProposerSignature + )) ), - "should not import individual block with an invalid gossip signature", + "should not import individual block with an invalid gossip signature, got: {:?}", + process_res ); } } @@ -576,16 +585,18 @@ async fn invalid_signature_block_proposal() { }) .collect::>(); // Ensure the block will be rejected if imported in a chain segment. + let process_res = harness + .chain + .process_chain_segment(blocks, NotifyExecutionLayer::Yes) + .await + .into_block_error(); assert!( matches!( - harness - .chain - .process_chain_segment(blocks, NotifyExecutionLayer::Yes) - .await - .into_block_error(), - Err(BlockError::InvalidSignature) + process_res, + Err(BlockError::InvalidSignature(InvalidSignature::Unknown)) ), - "should not import chain segment with an invalid block signature", + "should not import chain segment with an invalid block signature, got: {:?}", + process_res ); } } @@ -879,7 +890,7 @@ async fn invalid_signature_deposit() { .process_chain_segment(blocks, NotifyExecutionLayer::Yes) .await .into_block_error(), - Err(BlockError::InvalidSignature) + Err(BlockError::InvalidSignature(InvalidSignature::Unknown)) ), "should not throw an invalid signature error for a bad deposit signature" ); @@ -1075,7 +1086,7 @@ async fn block_gossip_verification() { ))) .await ), - BlockError::ProposalSignatureInvalid + BlockError::InvalidSignature(InvalidSignature::ProposerSignature) ), "should not import a block with an invalid proposal signature" ); From be8b3e1be52f7e3f736801f503552e5b22c991a5 Mon Sep 17 00:00:00 2001 From: Lion - dapplion <35266934+dapplion@users.noreply.github.com> Date: Tue, 28 Jan 2025 01:05:58 -0300 Subject: [PATCH 3/3] Update beacon_node/beacon_chain/src/block_verification.rs Co-authored-by: Pawan Dhananjay --- beacon_node/beacon_chain/src/block_verification.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index 5b90140f823..12652763763 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -326,7 +326,7 @@ pub enum BlockError { /// Which specific signature(s) are invalid in a SignedBeaconBlock #[derive(Debug)] pub enum InvalidSignature { - // The outter signature in a SignedBeaconBlock + // The outer signature in a SignedBeaconBlock ProposerSignature, // One or more signatures in BeaconBlockBody BlockBodySignatures,