From 2271c7148b8c2dcc28833df0fb797461cab96c16 Mon Sep 17 00:00:00 2001 From: Alexandru Gheorghe Date: Wed, 2 Aug 2023 13:08:12 +0300 Subject: [PATCH] Enforce target candidate is part of the signature Signed-off-by: Alexandru Gheorghe --- node/core/approval-voting/src/lib.rs | 8 ++-- node/core/approval-voting/src/tests.rs | 18 ++++----- node/core/dispute-coordinator/src/import.rs | 2 +- node/primitives/src/disputes/mod.rs | 20 ++++++++-- primitives/src/v5/mod.rs | 43 +++++++++++++-------- runtime/parachains/src/disputes.rs | 17 +++++--- 6 files changed, 68 insertions(+), 40 deletions(-) diff --git a/node/core/approval-voting/src/lib.rs b/node/core/approval-voting/src/lib.rs index b212874d356c..3c6bb5f2a142 100644 --- a/node/core/approval-voting/src/lib.rs +++ b/node/core/approval-voting/src/lib.rs @@ -2268,9 +2268,9 @@ where let candidate_hashes: Vec = approved_candidates_info.iter().map(|candidate| candidate.1).collect(); // Signature check: - match DisputeStatement::Valid(ValidDisputeStatementKind::ApprovalCheckingV2( - candidate_hashes.clone(), - )) + match DisputeStatement::Valid( + ValidDisputeStatementKind::ApprovalCheckingMultipleCandidates(candidate_hashes.clone()), + ) .check_signature( &pubkey, *candidate_hashes.first().expect("Checked above this is not empty; qed"), @@ -3243,7 +3243,7 @@ fn sign_approval( ) -> Option { let key = keystore.key_pair::(public).ok().flatten()?; - let payload = ApprovalVoteMultipleCandidates(candidate_hashes).signing_payload(session_index); + let payload = ApprovalVoteMultipleCandidates(&candidate_hashes).signing_payload(session_index); Some(key.sign(&payload[..])) } diff --git a/node/core/approval-voting/src/tests.rs b/node/core/approval-voting/src/tests.rs index 3ad5457aace7..ed9cd9791e25 100644 --- a/node/core/approval-voting/src/tests.rs +++ b/node/core/approval-voting/src/tests.rs @@ -1975,9 +1975,9 @@ fn test_signing_a_single_candidate_is_backwards_compatible() { session_index, ); - assert!(DisputeStatement::Valid(ValidDisputeStatementKind::ApprovalCheckingV2(vec![ - candidate_hash - ])) + assert!(DisputeStatement::Valid( + ValidDisputeStatementKind::ApprovalCheckingMultipleCandidates(vec![candidate_hash]) + ) .check_signature(&Sr25519Keyring::Alice.public().into(), candidate_hash, session_index, &sig_c,) .is_ok()); @@ -1990,9 +1990,9 @@ fn test_signing_a_single_candidate_is_backwards_compatible() { ) .is_ok()); - assert!(DisputeStatement::Valid(ValidDisputeStatementKind::ApprovalCheckingV2(vec![ - candidate_hash - ])) + assert!(DisputeStatement::Valid( + ValidDisputeStatementKind::ApprovalCheckingMultipleCandidates(vec![candidate_hash]) + ) .check_signature(&Sr25519Keyring::Alice.public().into(), candidate_hash, session_index, &sig_a,) .is_ok()); @@ -2002,9 +2002,9 @@ fn test_signing_a_single_candidate_is_backwards_compatible() { session_index, ); - assert!(DisputeStatement::Valid(ValidDisputeStatementKind::ApprovalCheckingV2( - candidate_hashes.clone() - )) + assert!(DisputeStatement::Valid( + ValidDisputeStatementKind::ApprovalCheckingMultipleCandidates(candidate_hashes.clone()) + ) .check_signature( &Sr25519Keyring::Alice.public().into(), *candidate_hashes.first().expect("test"), diff --git a/node/core/dispute-coordinator/src/import.rs b/node/core/dispute-coordinator/src/import.rs index dbe4f69ec66d..a760fe6326f3 100644 --- a/node/core/dispute-coordinator/src/import.rs +++ b/node/core/dispute-coordinator/src/import.rs @@ -528,7 +528,7 @@ impl ImportResult { { let pub_key = &env.session_info().validators.get(index).expect("indices are validated by approval-voting subsystem; qed"); let session_index = env.session_index(); - candidate_hashes.contains(&votes.candidate_receipt.hash()) && DisputeStatement::Valid(ValidDisputeStatementKind::ApprovalCheckingV2(candidate_hashes.clone())) + candidate_hashes.contains(&votes.candidate_receipt.hash()) && DisputeStatement::Valid(ValidDisputeStatementKind::ApprovalCheckingMultipleCandidates(candidate_hashes.clone())) .check_signature(pub_key, *candidate_hashes.first().expect("Valid votes have at least one candidate; qed"), session_index, &sig) .is_ok() }, diff --git a/node/primitives/src/disputes/mod.rs b/node/primitives/src/disputes/mod.rs index 0871f4d766f4..040a92575e4e 100644 --- a/node/primitives/src/disputes/mod.rs +++ b/node/primitives/src/disputes/mod.rs @@ -46,6 +46,15 @@ pub struct SignedDisputeStatement { session_index: SessionIndex, } +/// Errors encountered while signing a dispute statement +#[derive(Debug)] +pub enum SignedDisputeStatementError { + /// Encountered a keystore error while signing + KeyStoreError(KeystoreError), + /// Could not generate signing payload + PayloadError, +} + /// Tracked votes on candidates, for the purposes of dispute resolution. #[derive(Debug, Clone)] pub struct CandidateVotes { @@ -108,7 +117,7 @@ impl ValidCandidateVotes { ValidDisputeStatementKind::BackingSeconded(_) => false, ValidDisputeStatementKind::Explicit | ValidDisputeStatementKind::ApprovalChecking | - ValidDisputeStatementKind::ApprovalCheckingV2(_) => { + ValidDisputeStatementKind::ApprovalCheckingMultipleCandidates(_) => { occupied.insert((kind.clone(), sig)); kind != occupied.get().0 }, @@ -214,16 +223,19 @@ impl SignedDisputeStatement { candidate_hash: CandidateHash, session_index: SessionIndex, validator_public: ValidatorId, - ) -> Result, KeystoreError> { + ) -> Result, SignedDisputeStatementError> { let dispute_statement = if valid { DisputeStatement::Valid(ValidDisputeStatementKind::Explicit) } else { DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit) }; - let data = dispute_statement.payload_data(candidate_hash, session_index); + let data = dispute_statement + .payload_data(candidate_hash, session_index) + .map_err(|_| SignedDisputeStatementError::PayloadError)?; let signature = keystore - .sr25519_sign(ValidatorId::ID, validator_public.as_ref(), &data)? + .sr25519_sign(ValidatorId::ID, validator_public.as_ref(), &data) + .map_err(SignedDisputeStatementError::KeyStoreError)? .map(|sig| Self { dispute_statement, candidate_hash, diff --git a/primitives/src/v5/mod.rs b/primitives/src/v5/mod.rs index d3280f8be525..af169a599505 100644 --- a/primitives/src/v5/mod.rs +++ b/primitives/src/v5/mod.rs @@ -1069,9 +1069,9 @@ impl ApprovalVote { /// A vote of approvalf for multiple candidates. #[derive(Clone, RuntimeDebug)] -pub struct ApprovalVoteMultipleCandidates(pub Vec); +pub struct ApprovalVoteMultipleCandidates<'a>(pub &'a Vec); -impl ApprovalVoteMultipleCandidates { +impl<'a> ApprovalVoteMultipleCandidates<'a> { /// Yields the signing payload for this approval vote. pub fn signing_payload(&self, session_index: SessionIndex) -> Vec { const MAGIC: [u8; 4] = *b"APPR"; @@ -1260,28 +1260,39 @@ pub enum DisputeStatement { impl DisputeStatement { /// Get the payload data for this type of dispute statement. - pub fn payload_data(&self, candidate_hash: CandidateHash, session: SessionIndex) -> Vec { + pub fn payload_data( + &self, + candidate_hash: CandidateHash, + session: SessionIndex, + ) -> Result, ()> { match self { DisputeStatement::Valid(ValidDisputeStatementKind::Explicit) => - ExplicitDisputeStatement { valid: true, candidate_hash, session }.signing_payload(), + Ok(ExplicitDisputeStatement { valid: true, candidate_hash, session } + .signing_payload()), DisputeStatement::Valid(ValidDisputeStatementKind::BackingSeconded( inclusion_parent, - )) => CompactStatement::Seconded(candidate_hash).signing_payload(&SigningContext { + )) => Ok(CompactStatement::Seconded(candidate_hash).signing_payload(&SigningContext { session_index: session, parent_hash: *inclusion_parent, - }), + })), DisputeStatement::Valid(ValidDisputeStatementKind::BackingValid(inclusion_parent)) => - CompactStatement::Valid(candidate_hash).signing_payload(&SigningContext { + Ok(CompactStatement::Valid(candidate_hash).signing_payload(&SigningContext { session_index: session, parent_hash: *inclusion_parent, - }), + })), DisputeStatement::Valid(ValidDisputeStatementKind::ApprovalChecking) => - ApprovalVote(candidate_hash).signing_payload(session), - DisputeStatement::Valid(ValidDisputeStatementKind::ApprovalCheckingV2( - candidate_hashes, - )) => ApprovalVoteMultipleCandidates(candidate_hashes.clone()).signing_payload(session), + Ok(ApprovalVote(candidate_hash).signing_payload(session)), + DisputeStatement::Valid( + ValidDisputeStatementKind::ApprovalCheckingMultipleCandidates(candidate_hashes), + ) => + if candidate_hashes.contains(&candidate_hash) { + Ok(ApprovalVoteMultipleCandidates(candidate_hashes).signing_payload(session)) + } else { + Err(()) + }, DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit) => - ExplicitDisputeStatement { valid: false, candidate_hash, session }.signing_payload(), + Ok(ExplicitDisputeStatement { valid: false, candidate_hash, session } + .signing_payload()), } } @@ -1293,7 +1304,7 @@ impl DisputeStatement { session: SessionIndex, validator_signature: &ValidatorSignature, ) -> Result<(), ()> { - let payload = self.payload_data(candidate_hash, session); + let payload = self.payload_data(candidate_hash, session)?; if validator_signature.verify(&payload[..], &validator_public) { Ok(()) @@ -1325,7 +1336,7 @@ impl DisputeStatement { Self::Valid(ValidDisputeStatementKind::BackingValid(_)) => true, Self::Valid(ValidDisputeStatementKind::Explicit) | Self::Valid(ValidDisputeStatementKind::ApprovalChecking) | - Self::Valid(ValidDisputeStatementKind::ApprovalCheckingV2(_)) | + Self::Valid(ValidDisputeStatementKind::ApprovalCheckingMultipleCandidates(_)) | Self::Invalid(_) => false, } } @@ -1350,7 +1361,7 @@ pub enum ValidDisputeStatementKind { /// TODO: Fixme this probably means we can't create this version /// untill all nodes have been updated to support it. #[codec(index = 4)] - ApprovalCheckingV2(Vec), + ApprovalCheckingMultipleCandidates(Vec), } /// Different kinds of statements of invalidity on a candidate. diff --git a/runtime/parachains/src/disputes.rs b/runtime/parachains/src/disputes.rs index 5e253005ad17..fc5e4039561c 100644 --- a/runtime/parachains/src/disputes.rs +++ b/runtime/parachains/src/disputes.rs @@ -1330,24 +1330,29 @@ fn check_signature( statement: &DisputeStatement, validator_signature: &ValidatorSignature, ) -> Result<(), ()> { - let payload = match *statement { + let payload = match statement { DisputeStatement::Valid(ValidDisputeStatementKind::Explicit) => ExplicitDisputeStatement { valid: true, candidate_hash, session }.signing_payload(), DisputeStatement::Valid(ValidDisputeStatementKind::BackingSeconded(inclusion_parent)) => CompactStatement::Seconded(candidate_hash).signing_payload(&SigningContext { session_index: session, - parent_hash: inclusion_parent, + parent_hash: *inclusion_parent, }), DisputeStatement::Valid(ValidDisputeStatementKind::BackingValid(inclusion_parent)) => CompactStatement::Valid(candidate_hash).signing_payload(&SigningContext { session_index: session, - parent_hash: inclusion_parent, + parent_hash: *inclusion_parent, }), DisputeStatement::Valid(ValidDisputeStatementKind::ApprovalChecking) => ApprovalVote(candidate_hash).signing_payload(session), - DisputeStatement::Valid(ValidDisputeStatementKind::ApprovalCheckingV2(_)) => - // TODO: Fixme - ApprovalVoteMultipleCandidates(vec![candidate_hash]).signing_payload(session), + DisputeStatement::Valid(ValidDisputeStatementKind::ApprovalCheckingMultipleCandidates( + candidates, + )) => + if candidates.contains(&candidate_hash) { + ApprovalVoteMultipleCandidates(candidates).signing_payload(session) + } else { + return Err(()) + }, DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit) => ExplicitDisputeStatement { valid: false, candidate_hash, session }.signing_payload(), };