From f0e85db718f5165b06585a49b14a66f8ad643aea Mon Sep 17 00:00:00 2001 From: Seun Lanlege Date: Tue, 28 Jan 2025 07:11:29 +0000 Subject: [PATCH] fix critical vulnerability in ismp-grandpa --- Cargo.lock | 6 +++--- Cargo.toml | 6 +++--- .../consensus/grandpa/primitives/Cargo.toml | 2 +- .../grandpa/primitives/src/justification.rs | 15 ++----------- modules/consensus/grandpa/prover/src/lib.rs | 2 +- modules/consensus/grandpa/verifier/Cargo.toml | 4 ++-- .../consensus/grandpa/verifier/src/tests.rs | 4 ++-- modules/ismp/clients/grandpa/Cargo.toml | 14 ++++++++++--- parachain/runtimes/nexus/src/lib.rs | 21 +++++++++---------- 9 files changed, 35 insertions(+), 39 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index bd6eec63e..e8500add5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6616,7 +6616,7 @@ dependencies = [ [[package]] name = "grandpa-verifier" -version = "0.1.1" +version = "0.1.2" dependencies = [ "anyhow", "derive_more 0.99.18", @@ -6649,7 +6649,7 @@ dependencies = [ [[package]] name = "grandpa-verifier-primitives" -version = "0.1.1" +version = "0.1.2" dependencies = [ "anyhow", "finality-grandpa", @@ -7732,7 +7732,7 @@ version = "0.1.1" [[package]] name = "ismp-grandpa" -version = "15.0.0" +version = "15.0.1" dependencies = [ "anyhow", "ckb-merkle-mountain-range", diff --git a/Cargo.toml b/Cargo.toml index 40ef624c6..b241645c1 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -283,13 +283,13 @@ geth-primitives = { path = "./modules/consensus/geth-primitives", default-featur sync-committee-primitives = { path = "./modules/consensus/sync-committee/primitives", default-features = false } sync-committee-prover = { path = "./modules/consensus/sync-committee/prover" } sync-committee-verifier = { path = "./modules/consensus/sync-committee/verifier", default-features = false } -grandpa-verifier-primitives = { version = "0.1.1", path = "./modules/consensus/grandpa/primitives", default-features = false } -grandpa-verifier = { version = "0.1.1", path = "./modules/consensus/grandpa/verifier", default-features = false } +grandpa-verifier-primitives = { version = "0.1.2", path = "./modules/consensus/grandpa/primitives", default-features = false } +grandpa-verifier = { version = "0.1.2", path = "./modules/consensus/grandpa/verifier", default-features = false } grandpa-prover = { path = "./modules/consensus/grandpa/prover" } # consensus clients ismp-bsc = { path = "./modules/ismp/clients/bsc", default-features = false } -ismp-grandpa = { version = "15.0.0", path = "./modules/ismp/clients/grandpa", default-features = false } +ismp-grandpa = { version = "15.0.1", path = "./modules/ismp/clients/grandpa", default-features = false } ismp-parachain = { version = "15.0.1", path = "./modules/ismp/clients/parachain/client", default-features = false } ismp-parachain-inherent = { version = "15.0.0", path = "./modules/ismp/clients/parachain/inherent" } ismp-parachain-runtime-api = { version = "15.0.0", path = "./modules/ismp/clients/parachain/runtime-api", default-features = false } diff --git a/modules/consensus/grandpa/primitives/Cargo.toml b/modules/consensus/grandpa/primitives/Cargo.toml index e0ab267f9..e17f6a9b0 100644 --- a/modules/consensus/grandpa/primitives/Cargo.toml +++ b/modules/consensus/grandpa/primitives/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "grandpa-verifier-primitives" -version = "0.1.1" +version = "0.1.2" edition = "2021" authors = ["Polytope Labs "] license = "Apache-2.0" diff --git a/modules/consensus/grandpa/primitives/src/justification.rs b/modules/consensus/grandpa/primitives/src/justification.rs index e5a3dc22e..9feb4fa14 100644 --- a/modules/consensus/grandpa/primitives/src/justification.rs +++ b/modules/consensus/grandpa/primitives/src/justification.rs @@ -26,7 +26,7 @@ use sp_consensus_grandpa::{ ScheduledChange, SetId, GRANDPA_ENGINE_ID, }; use sp_core::ed25519; -use sp_runtime::{generic::OpaqueDigestItemId, traits::Header as HeaderT}; +use sp_runtime::{generic::OpaqueDigestItemId, traits::Header as HeaderT, RuntimeAppPublic}; use sp_std::prelude::*; /// A GRANDPA justification for block finality, it includes a commit message and @@ -243,20 +243,9 @@ where H: Encode, N: Encode, { - log::trace!(target: "pallet_grandpa", "Justification Message {:?}", (round, set_id)); let buf = (message, round, set_id).encode(); - let signature_bytes: &[u8] = signature.as_ref(); - let sp_finality_signature: ed25519::Signature = signature_bytes - .try_into() - .map_err(|err| anyhow!("Failed to convert ed25519 signature: {err:#?}"))?; - - let id_bytes: &[u8] = id.as_ref(); - let pub_key: ed25519::Public = id_bytes - .try_into() - .map_err(|err| anyhow!("Failed to convert public key: {err:#?}"))?; - - if !sp_io::crypto::ed25519_verify(&sp_finality_signature, &buf, &pub_key) { + if !id.verify(&buf, signature) { Err(anyhow!("invalid signature for precommit in grandpa justification"))? } diff --git a/modules/consensus/grandpa/prover/src/lib.rs b/modules/consensus/grandpa/prover/src/lib.rs index 058ba30b7..7c27b555d 100644 --- a/modules/consensus/grandpa/prover/src/lib.rs +++ b/modules/consensus/grandpa/prover/src/lib.rs @@ -158,7 +158,7 @@ where Ok(ConsensusState { current_authorities, - current_set_id: current_set_id + 1, + current_set_id, latest_height, latest_hash: hash.into(), slot_duration, diff --git a/modules/consensus/grandpa/verifier/Cargo.toml b/modules/consensus/grandpa/verifier/Cargo.toml index 0bdae52c5..08372535b 100644 --- a/modules/consensus/grandpa/verifier/Cargo.toml +++ b/modules/consensus/grandpa/verifier/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "grandpa-verifier" -version = "0.1.1" +version = "0.1.2" edition = "2021" authors = ["Polytope Labs "] license = "Apache-2.0" @@ -14,7 +14,7 @@ keywords = ["substrate", "polkadot-sdk", "ISMP", "interoperability", "GRANDPA"] targets = ["x86_64-unknown-linux-gnu"] [dependencies] -codec = { workspace = true, features = ["derive"]} +codec = { workspace = true, features = ["derive"] } anyhow = { workspace = true, default-features = false } finality-grandpa = { version = "0.16.0", features = ["derive-codec"], default-features = false } serde = { workspace = true, features = ["derive"] } diff --git a/modules/consensus/grandpa/verifier/src/tests.rs b/modules/consensus/grandpa/verifier/src/tests.rs index dcebb4745..ab274dd3d 100644 --- a/modules/consensus/grandpa/verifier/src/tests.rs +++ b/modules/consensus/grandpa/verifier/src/tests.rs @@ -44,7 +44,7 @@ async fn follow_grandpa_justifications() { let relay_ws_url = std::env::var("RELAY_HOST") .unwrap_or_else(|_| "wss://hyperbridge-paseo-relay.blockops.network:443".to_string()); - let para_ids = vec![2000]; + let para_ids = vec![1000]; println!("Connecting to relay chain {relay_ws_url}"); let prover = GrandpaProver::::new(ProverOptions { @@ -85,7 +85,7 @@ async fn follow_grandpa_justifications() { // slot duration in milliseconds for parachains let slot_duration = 6000; - let hash = prover.client.rpc().block_hash(Some(10u64.into())).await.unwrap().unwrap(); + let hash = prover.client.rpc().finalized_head().await.unwrap(); let mut consensus_state = prover.initialize_consensus_state(slot_duration, hash).await.unwrap(); println!("Grandpa proofs are now available"); while let Some(Ok(_)) = subscription.next().await { diff --git a/modules/ismp/clients/grandpa/Cargo.toml b/modules/ismp/clients/grandpa/Cargo.toml index f070670d9..f65c0bf0c 100644 --- a/modules/ismp/clients/grandpa/Cargo.toml +++ b/modules/ismp/clients/grandpa/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "ismp-grandpa" -version = "15.0.0" +version = "15.0.1" edition = "2021" authors = ["Polytope Labs "] license = "Apache-2.0" @@ -15,9 +15,7 @@ readme = "./README.md" anyhow = { workspace = true } codec = { workspace = true, features = ["derive"] } primitive-types = { workspace = true } -scale-info = { version = "2.1.1", default-features = false, features = ["derive"] } merkle-mountain-range = { workspace = true } -finality-grandpa = { version = "0.16.0", features = ["derive-codec"], default-features = false } frame-benchmarking = { workspace = true, optional = true } sp-std = { workspace = true } @@ -39,6 +37,16 @@ sp-core = { workspace = true } # cumulus substrate-state-machine = { workspace = true } +[dependencies.scale-info] +version = "2.1.1" +default-features = false +features = ["derive"] + +[dependencies.finality-grandpa] +version = "0.16.0" +features = ["derive-codec"] +default-features = false + [features] default = ["std"] std = [ diff --git a/parachain/runtimes/nexus/src/lib.rs b/parachain/runtimes/nexus/src/lib.rs index dff6bd088..6f9ee10f3 100644 --- a/parachain/runtimes/nexus/src/lib.rs +++ b/parachain/runtimes/nexus/src/lib.rs @@ -335,8 +335,8 @@ impl Contains for IsTreasurySpend { fn contains(c: &RuntimeCall) -> bool { matches!( c, - RuntimeCall::Treasury(pallet_treasury::Call::spend { .. }) - | RuntimeCall::Treasury(pallet_treasury::Call::spend_local { .. }) + RuntimeCall::Treasury(pallet_treasury::Call::spend { .. }) | + RuntimeCall::Treasury(pallet_treasury::Call::spend_local { .. }) ) } } @@ -764,20 +764,19 @@ impl InstanceFilter for ProxyType { fn filter(&self, c: &RuntimeCall) -> bool { match self { ProxyType::Any => true, - ProxyType::NonTransfer => { - !matches!(c, RuntimeCall::Balances { .. } | RuntimeCall::Assets { .. }) - }, + ProxyType::NonTransfer => + !matches!(c, RuntimeCall::Balances { .. } | RuntimeCall::Assets { .. }), ProxyType::CancelProxy => matches!( c, - RuntimeCall::Proxy(pallet_proxy::Call::reject_announcement { .. }) - | RuntimeCall::Utility { .. } - | RuntimeCall::Multisig { .. } + RuntimeCall::Proxy(pallet_proxy::Call::reject_announcement { .. }) | + RuntimeCall::Utility { .. } | + RuntimeCall::Multisig { .. } ), ProxyType::Collator => matches!( c, - RuntimeCall::CollatorSelection { .. } - | RuntimeCall::Utility { .. } - | RuntimeCall::Multisig { .. } + RuntimeCall::CollatorSelection { .. } | + RuntimeCall::Utility { .. } | + RuntimeCall::Multisig { .. } ), } }