Skip to content

Commit

Permalink
refactor(consensus): Refactor malicious_code in ecdsa component
Browse files Browse the repository at this point in the history
  • Loading branch information
ninegua committed Jan 15, 2024
1 parent 7f6e7df commit d5fd7bf
Show file tree
Hide file tree
Showing 4 changed files with 150 additions and 99 deletions.
21 changes: 17 additions & 4 deletions rs/consensus/src/ecdsa.rs
Expand Up @@ -205,6 +205,8 @@ use std::sync::Arc;
use std::time::{Duration, Instant};

pub(crate) mod complaints;
#[cfg(any(feature = "malicious_code", test))]
pub(crate) mod malicious_pre_signer;
pub(crate) mod payload_builder;
pub(crate) mod payload_verifier;
pub(crate) mod pre_signer;
Expand All @@ -228,7 +230,7 @@ pub const INACTIVE_TRANSCRIPT_PURGE_SECS: Duration = Duration::from_secs(60);
/// `EcdsaImpl` is the consensus component responsible for processing threshold
/// ECDSA payloads.
pub struct EcdsaImpl {
pre_signer: Box<dyn EcdsaPreSigner>,
pre_signer: Box<EcdsaPreSignerImpl>,
signer: Box<dyn EcdsaSigner>,
complaint_handler: Box<dyn EcdsaComplaintHandler>,
consensus_block_cache: Arc<dyn ConsensusBlockCache>,
Expand All @@ -237,6 +239,8 @@ pub struct EcdsaImpl {
last_transcript_purge_ts: RefCell<Instant>,
metrics: EcdsaClientMetrics,
logger: ReplicaLogger,
#[cfg_attr(not(feature = "malicious_code"), allow(dead_code))]
malicious_flags: MaliciousFlags,
}

impl EcdsaImpl {
Expand All @@ -256,7 +260,6 @@ impl EcdsaImpl {
crypto.clone(),
metrics_registry.clone(),
logger.clone(),
malicious_flags,
));
let signer = Box::new(EcdsaSignerImpl::new(
node_id,
Expand All @@ -283,6 +286,7 @@ impl EcdsaImpl {
last_transcript_purge_ts: RefCell::new(Instant::now()),
metrics: EcdsaClientMetrics::new(metrics_registry),
logger,
malicious_flags,
}
}

Expand Down Expand Up @@ -351,14 +355,23 @@ impl<T: EcdsaPool> ChangeSetProducer<T> for EcdsaImpl {
fn on_state_change(&self, ecdsa_pool: &T) -> EcdsaChangeSet {
let metrics = self.metrics.clone();
let pre_signer = || {
timed_call(
let changeset = timed_call(
"pre_signer",
|| {
self.pre_signer
.on_state_change(ecdsa_pool, self.complaint_handler.as_transcript_loader())
},
&metrics.on_state_change_duration,
)
);
#[cfg(any(feature = "malicious_code", test))]
if self.malicious_flags.is_ecdsa_malicious() {
return super::ecdsa::malicious_pre_signer::maliciously_alter_changeset(
changeset,
&self.pre_signer,
&self.malicious_flags,
);
}
changeset
};
let signer = || {
timed_call(
Expand Down
127 changes: 127 additions & 0 deletions rs/consensus/src/ecdsa/malicious_pre_signer.rs
@@ -0,0 +1,127 @@
//! The malicious pre signature process manager

use crate::consensus::metrics::EcdsaPreSignerMetrics;
use crate::ecdsa::{
pre_signer::EcdsaPreSignerImpl, utils::transcript_op_summary, EcdsaBlockReaderImpl,
};
use ic_interfaces::{
crypto::BasicSigner,
ecdsa::{EcdsaChangeAction, EcdsaChangeSet},
};
use ic_logger::{warn, ReplicaLogger};
use ic_registry_client_helpers::node::RegistryVersion;
use ic_types::{
consensus::ecdsa::{EcdsaBlockReader, EcdsaMessage},
crypto::canister_threshold_sig::idkg::{IDkgDealing, IDkgTranscriptParams, SignedIDkgDealing},
crypto::{BasicSigOf, CryptoResult},
malicious_flags::MaliciousFlags,
NodeId,
};
use std::collections::BTreeSet;

// A dealing is corrupted by changing some internal value.
// Since a dealing is signed, the signature must be re-computed so that the corrupted dealing
// is not trivially discarded and a proper complaint can be generated.
// To sign a dealing we only need something that implements the trait BasicSigner<IDkgDealing>,
// which `ConsensusCrypto` does.
// However, for Rust something of type dyn ConsensusCrypto (self.crypto is of type
// Arc<dyn ConsensusCrypto>, but the Arc<> is not relevant here) cannot be coerced into
// something of type dyn BasicSigner<IDkgDealing>. This is true for any sub trait implemented
// by ConsensusCrypto and is not specific to Crypto traits.
// Doing so would require `dyn upcasting coercion`, see
// https://github.com/rust-lang/rust/issues/65991 and
// https://articles.bchlr.de/traits-dynamic-dispatch-upcasting.
// As workaround a trivial implementation of BasicSigner<IDkgDealing> is provided by delegating to
// self.crypto.
impl BasicSigner<IDkgDealing> for EcdsaPreSignerImpl {
fn sign_basic(
&self,
message: &IDkgDealing,
signer: NodeId,
registry_version: RegistryVersion,
) -> CryptoResult<BasicSigOf<IDkgDealing>> {
self.crypto.sign_basic(message, signer, registry_version)
}
}

/// Modify the given changeset with malicious behavior.
pub fn maliciously_alter_changeset(
changeset: EcdsaChangeSet,
pre_signer: &EcdsaPreSignerImpl,
malicious_flags: &MaliciousFlags,
) -> EcdsaChangeSet {
let block_reader =
EcdsaBlockReaderImpl::new(pre_signer.consensus_block_cache.finalized_chain());

changeset
.into_iter()
.flat_map(|action| match action {
EcdsaChangeAction::AddToValidated(EcdsaMessage::EcdsaSignedDealing(dealing))
if malicious_flags.maliciously_corrupt_ecdsa_dealings =>
{
let transcript_id = dealing.idkg_dealing().transcript_id;
block_reader
.requested_transcripts()
.find(|params_ref| params_ref.transcript_id == transcript_id)
.and_then(|params_ref| {
pre_signer.resolve_ref(params_ref, &block_reader, "malicious_send_dealing")
})
.map(|params| {
let dealing = maliciously_corrupt_ecdsa_dealings(
pre_signer,
pre_signer.node_id,
dealing,
&params,
&pre_signer.log,
&pre_signer.metrics,
);
EcdsaChangeAction::AddToValidated(EcdsaMessage::EcdsaSignedDealing(dealing))
})
}
_ => Some(action),
})
.collect::<Vec<_>>()
}

/// Helper to corrupt the signed crypto dealing for malicious testing
fn maliciously_corrupt_ecdsa_dealings(
pre_signer: &EcdsaPreSignerImpl,
node_id: NodeId,
idkg_dealing: SignedIDkgDealing,
transcript_params: &IDkgTranscriptParams,
log: &ReplicaLogger,
metrics: &EcdsaPreSignerMetrics,
) -> SignedIDkgDealing {
let mut rng = rand::thread_rng();
let mut exclude_set = BTreeSet::new();
exclude_set.insert(node_id);
match ic_crypto_test_utils_canister_threshold_sigs::corrupt_signed_idkg_dealing(
idkg_dealing,
transcript_params,
pre_signer,
node_id,
&exclude_set,
&mut rng,
) {
Ok(dealing) => {
warn!(
every_n_seconds => 2,
log,
"Corrupted dealing: transcript_id = {:?}", transcript_params.transcript_id()
);
metrics.pre_sign_metrics_inc("dealing_corrupted");
dealing
}
Err(err) => {
warn!(
log,
"Failed to corrupt dealing: transcript_id = {:?}, type = {:?}, error = {:?}",
transcript_params.transcript_id(),
transcript_op_summary(transcript_params.operation_type()),
err
);
metrics.pre_sign_errors_inc("corrupt_dealing");
panic!("Failed to corrupt dealing")
}
}
}
99 changes: 6 additions & 93 deletions rs/consensus/src/ecdsa/pre_signer.rs
Expand Up @@ -21,21 +21,13 @@ use ic_types::crypto::canister_threshold_sig::idkg::{
IDkgTranscriptId, IDkgTranscriptOperation, IDkgTranscriptParams, SignedIDkgDealing,
};
use ic_types::crypto::CryptoHashOf;
use ic_types::malicious_flags::MaliciousFlags;
use ic_types::signature::BasicSignatureBatch;
use ic_types::{Height, NodeId};
use std::cell::RefCell;
use std::collections::{btree_map::Entry, BTreeMap, BTreeSet};
use std::fmt::{self, Debug, Formatter};
use std::sync::Arc;

#[cfg(feature = "malicious_code")]
use {
ic_interfaces::crypto::BasicSigner, ic_registry_client_helpers::node::RegistryVersion,
ic_types::crypto::canister_threshold_sig::idkg::IDkgDealing, ic_types::crypto::BasicSigOf,
ic_types::crypto::CryptoResult,
};

pub(crate) trait EcdsaPreSigner: Send {
/// The on_state_change() called from the main ECDSA path.
fn on_state_change(
Expand All @@ -46,14 +38,12 @@ pub(crate) trait EcdsaPreSigner: Send {
}

pub(crate) struct EcdsaPreSignerImpl {
node_id: NodeId,
consensus_block_cache: Arc<dyn ConsensusBlockCache>,
crypto: Arc<dyn ConsensusCrypto>,
pub(crate) node_id: NodeId,
pub(crate) consensus_block_cache: Arc<dyn ConsensusBlockCache>,
pub(crate) crypto: Arc<dyn ConsensusCrypto>,
schedule: RoundRobin,
metrics: EcdsaPreSignerMetrics,
log: ReplicaLogger,
#[cfg_attr(not(feature = "malicious_code"), allow(dead_code))]
malicious_flags: MaliciousFlags,
pub(crate) metrics: EcdsaPreSignerMetrics,
pub(crate) log: ReplicaLogger,
}

impl EcdsaPreSignerImpl {
Expand All @@ -63,7 +53,6 @@ impl EcdsaPreSignerImpl {
crypto: Arc<dyn ConsensusCrypto>,
metrics_registry: MetricsRegistry,
log: ReplicaLogger,
malicious_flags: MaliciousFlags,
) -> Self {
Self {
node_id,
Expand All @@ -72,7 +61,6 @@ impl EcdsaPreSignerImpl {
schedule: RoundRobin::default(),
metrics: EcdsaPreSignerMetrics::new(metrics_registry),
log,
malicious_flags,
}
}

Expand Down Expand Up @@ -624,10 +612,6 @@ impl EcdsaPreSignerImpl {
match IDkgProtocol::create_dealing(&*self.crypto, transcript_params) {
Ok(idkg_dealing) => {
self.metrics.pre_sign_metrics_inc("dealing_created");

#[cfg(feature = "malicious_code")]
let idkg_dealing = self.crypto_corrupt_dealing(idkg_dealing, transcript_params);

self.metrics.pre_sign_metrics_inc("dealing_sent");
vec![EcdsaChangeAction::AddToValidated(
EcdsaMessage::EcdsaSignedDealing(idkg_dealing),
Expand Down Expand Up @@ -698,51 +682,6 @@ impl EcdsaPreSignerImpl {
}
}

/// Helper to corrupt the signed crypto dealing for malicious testing
#[cfg(feature = "malicious_code")]
fn crypto_corrupt_dealing(
&self,
idkg_dealing: SignedIDkgDealing,
transcript_params: &IDkgTranscriptParams,
) -> SignedIDkgDealing {
if !self.malicious_flags.maliciously_corrupt_ecdsa_dealings {
return idkg_dealing;
}

let mut rng = rand::thread_rng();
let mut exclude_set = BTreeSet::new();
exclude_set.insert(self.node_id);
match ic_crypto_test_utils_canister_threshold_sigs::corrupt_signed_idkg_dealing(
idkg_dealing,
transcript_params,
self,
self.node_id,
&exclude_set,
&mut rng,
) {
Ok(dealing) => {
warn!(
every_n_seconds => 2,
self.log,
"Corrupted dealing: transcript_id = {:?}", transcript_params.transcript_id()
);
self.metrics.pre_sign_metrics_inc("dealing_corrupted");
dealing
}
Err(err) => {
warn!(
self.log,
"Failed to corrupt dealing: transcript_id = {:?}, type = {:?}, error = {:?}",
transcript_params.transcript_id(),
transcript_op_summary(transcript_params.operation_type()),
err
);
self.metrics.pre_sign_errors_inc("corrupt_dealing");
panic!("Failed to corrupt dealing")
}
}
}

/// Helper to do private verification of a dealing and, if successful, issue a support share for it.
/// Assumes we are a receiver for the dealing.
fn crypto_create_dealing_support(
Expand Down Expand Up @@ -936,7 +875,7 @@ impl EcdsaPreSignerImpl {
}

/// Resolves the IDkgTranscriptParamsRef -> IDkgTranscriptParams.
fn resolve_ref(
pub(crate) fn resolve_ref(
&self,
transcript_params_ref: &IDkgTranscriptParamsRef,
block_reader: &dyn EcdsaBlockReader,
Expand Down Expand Up @@ -1033,32 +972,6 @@ impl EcdsaPreSigner for EcdsaPreSignerImpl {
}
}

// A dealing is corrupted by changing some internal value.
// Since a dealing is signed, the signature must be re-computed so that the corrupted dealing
// is not trivially discarded and a proper complaint can be generated.
// To sign a dealing we only need something that implements the trait BasicSigner<IDkgDealing>,
// which `ConsensusCrypto` does.
// However, for Rust something of type dyn ConsensusCrypto (self.crypto is of type
// Arc<dyn ConsensusCrypto>, but the Arc<> is not relevant here) cannot be coerced into
// something of type dyn BasicSigner<IDkgDealing>. This is true for any sub trait implemented
// by ConsensusCrypto and is not specific to Crypto traits.
// Doing so would require `dyn upcasting coercion`, see
// https://github.com/rust-lang/rust/issues/65991 and
// https://articles.bchlr.de/traits-dynamic-dispatch-upcasting.
// As workaround a trivial implementation of BasicSigner<IDkgDealing> is provided by delegating to
// self.crypto.
#[cfg(feature = "malicious_code")]
impl BasicSigner<IDkgDealing> for EcdsaPreSignerImpl {
fn sign_basic(
&self,
message: &IDkgDealing,
signer: NodeId,
registry_version: RegistryVersion,
) -> CryptoResult<BasicSigOf<IDkgDealing>> {
self.crypto.sign_basic(message, signer, registry_version)
}
}

pub(crate) trait EcdsaTranscriptBuilder {
/// Returns the specified transcript if it can be successfully
/// built from the current entries in the ECDSA pool
Expand Down
2 changes: 0 additions & 2 deletions rs/consensus/src/ecdsa/test_utils.rs
Expand Up @@ -42,7 +42,6 @@ use ic_types::crypto::canister_threshold_sig::{
ThresholdEcdsaSigShare,
};
use ic_types::crypto::AlgorithmId;
use ic_types::malicious_behaviour::MaliciousBehaviour;
use ic_types::messages::CallbackId;
use ic_types::{signature::*, Time};
use ic_types::{Height, NodeId, PrincipalId, Randomness, RegistryVersion, SubnetId};
Expand Down Expand Up @@ -520,7 +519,6 @@ pub(crate) fn create_pre_signer_dependencies_with_crypto(
consensus_crypto.unwrap_or(crypto),
metrics_registry.clone(),
logger.clone(),
MaliciousBehaviour::new(false).malicious_flags,
);
let ecdsa_pool = EcdsaPoolImpl::new(pool_config, logger, metrics_registry);

Expand Down

0 comments on commit d5fd7bf

Please sign in to comment.