From 13d4cf0a37237ec3deff3d2b10981a748f9e890f Mon Sep 17 00:00:00 2001 From: LucienSong Date: Sun, 24 May 2026 06:18:49 +0800 Subject: [PATCH 1/2] security: fix audit findings C-1, C-3, H-1, H-3, H-4 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit C-1: charge batch precompile gas before verification loop; cap count at 256 C-3: verify vote signatures in handle_wpoa_vote (mirror view_change path) H-1: gate L2 recursive-proof stub behind stub-l2-verifier feature H-3: unify batch precompile algorithm dispatch with single verify H-4: document hickory-proto advisory acceptance with full mitigation rationale C-1 (crates/evm/src/precompiles.rs): Add MAX_BATCH_SIGNATURES=256 constant; parse count from header before any work; reject oversized batches upfront; charge gas via checked_mul BEFORE entering the verification loop. C-3 (crates/node/src/node/p2p_handlers.rs): In handle_wpoa_vote, after the FF.6 finality check, look up the voter's public key from known_authorities, infer sig algorithm via infer_signature_type_from_address, verify the signature against block_hash.as_bytes() using MultiVerifier, and drop+penalise on failure — exactly mirroring handle_wpoa_view_change. Update existing wpoa_handle_vote_reaches_quorum test to use real key pairs and valid sigs. H-1 (crates/node/src/node/system_rewards.rs, crates/node/Cargo.toml, crates/node/src/node/mod.rs): Replace soft-pass on RecursiveProof decode error and NotImplemented with hard NodeError::Startup unless compiled with the new stub-l2-verifier feature (default=off). Add startup warn! banner in Node::new when feature is enabled. H-3 (crates/evm/src/precompiles.rs): Replace verify_legacy_mldsa65_item (Dilithium3-only) in batch loop with verify_mldsa65 (ML-DSA-65 first, then Dilithium3 fallback) to match the single-verify dispatch path. H-4 (deny.toml): Expand the hickory-proto advisory acceptance comment with full mitigation rationale (DNS-only bootstrap usage, failure mode analysis). cargo update -p hickory-proto produced no change — no compatible patched release available for libp2p 0.56. Security audit ref: 2026-05-24 consolidated report Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- Cargo.lock | 20 +-- crates/evm/src/precompiles.rs | 74 ++++++++++- crates/node/Cargo.toml | 7 + crates/node/src/node/mod.rs | 169 +++++++++++++++++++++---- crates/node/src/node/p2p_handlers.rs | 74 +++++++++++ crates/node/src/node/system_rewards.rs | 62 ++++++--- deny.toml | 14 +- 7 files changed, 359 insertions(+), 61 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ddebdb7d..9e3b4d94 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -256,7 +256,7 @@ version = "1.1.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "40c48f72fd53cd289104fc64099abca73db4166ad86ea0b4341abe65af83dadc" dependencies = [ - "windows-sys 0.61.2", + "windows-sys 0.60.2", ] [[package]] @@ -267,7 +267,7 @@ checksum = "291e6a250ff86cd4a820112fb8898808a366d8f9f58ce16d1f538353ad55747d" dependencies = [ "anstyle", "once_cell_polyfill", - "windows-sys 0.61.2", + "windows-sys 0.60.2", ] [[package]] @@ -1818,7 +1818,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "39cab71617ae0d63f51a36d69f866391735b51691dbda63cf6f96d042b63efeb" dependencies = [ "libc", - "windows-sys 0.61.2", + "windows-sys 0.59.0", ] [[package]] @@ -2904,7 +2904,7 @@ checksum = "3640c1c38b8e4e43584d8df18be5fc6b0aa314ce6ebf51b53313d4306cca8e46" dependencies = [ "hermit-abi", "libc", - "windows-sys 0.61.2", + "windows-sys 0.59.0", ] [[package]] @@ -4058,7 +4058,7 @@ version = "0.50.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7957b9740744892f114936ab4a57b3f487491bbeafaf8083688b16841a4240e5" dependencies = [ - "windows-sys 0.61.2", + "windows-sys 0.59.0", ] [[package]] @@ -5428,7 +5428,7 @@ dependencies = [ "errno", "libc", "linux-raw-sys", - "windows-sys 0.61.2", + "windows-sys 0.59.0", ] [[package]] @@ -5496,7 +5496,7 @@ dependencies = [ "security-framework", "security-framework-sys", "webpki-root-certs 0.26.11", - "windows-sys 0.52.0", + "windows-sys 0.59.0", ] [[package]] @@ -6286,7 +6286,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3a766e1110788c36f4fa1c2b71b387a7815aa65f88ce0229841826633d93723e" dependencies = [ "libc", - "windows-sys 0.61.2", + "windows-sys 0.60.2", ] [[package]] @@ -6439,7 +6439,7 @@ dependencies = [ "getrandom 0.4.2", "once_cell", "rustix", - "windows-sys 0.61.2", + "windows-sys 0.59.0", ] [[package]] @@ -7264,7 +7264,7 @@ version = "0.1.11" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c2a7b1c03c876122aa43f3020e6c3c3ee5c05081c9a00739faf7503aeba10d22" dependencies = [ - "windows-sys 0.61.2", + "windows-sys 0.59.0", ] [[package]] diff --git a/crates/evm/src/precompiles.rs b/crates/evm/src/precompiles.rs index 8b4c56a2..c245a82a 100644 --- a/crates/evm/src/precompiles.rs +++ b/crates/evm/src/precompiles.rs @@ -41,6 +41,8 @@ const PQ_PRECOMPILE_ADDRS: [Address; 6] = [ pub const PQ_MLDSA65_VERIFY_GAS: u64 = 46_000; pub const PQ_SLHDSA_VERIFY_GAS: u64 = 2_300_000; pub const PQ_MLDSA65_BATCH_VERIFY_GAS_PER_SIG: u64 = 12_000; +/// C-1: Hard cap on batch size to prevent unbounded CPU work regardless of gas. +pub const MAX_BATCH_SIGNATURES: u32 = 256; pub const BLAKE3_BASE_GAS: u64 = 30; pub const BLAKE3_WORD_GAS: u64 = 6; pub const PQ_ADDR_DERIVE_GAS: u64 = 200; @@ -171,11 +173,30 @@ fn run_slhdsa_sha2_256f_verify(gas_limit: u64, input: &[u8]) -> InterpreterResul fn run_mldsa65_batch_verify(gas_limit: u64, input: &[u8]) -> InterpreterResult { let mut result = base_result(gas_limit); - let (count, valid) = verify_mldsa65_batch(input); - let gas = PQ_MLDSA65_BATCH_VERIFY_GAS_PER_SIG.saturating_mul(count as u64); - if !charge_gas(&mut result, gas) { + + // C-1: Parse count from the header BEFORE any verification work. + let Some(count_bytes) = input.get(..4) else { + result.result = InstructionResult::PrecompileError; + return result; + }; + let count = u32::from_be_bytes(count_bytes.try_into().expect("slice length checked")); + + // C-1: Reject oversized batches up-front to bound CPU work. + if count > MAX_BATCH_SIGNATURES { + result.result = InstructionResult::PrecompileError; return result; } + + // C-1: Charge the full gas cost BEFORE entering the verification loop so + // that a caller with gas_limit = 0 receives OOG without doing any work. + let total_cost = PQ_MLDSA65_BATCH_VERIFY_GAS_PER_SIG + .checked_mul(count as u64) + .unwrap_or(u64::MAX); + if !charge_gas(&mut result, total_cost) { + return result; + } + + let (_, valid) = verify_mldsa65_batch(input); result.output = bool_output(valid); result } @@ -269,6 +290,11 @@ fn verify_slhdsa_sha2_256f(input: &[u8]) -> bool { .unwrap_or(false) } +/// Legacy Dilithium3-only item verifier, kept for wire-format reference. +/// No longer called from the batch path after H-3 unification; batch now +/// delegates to `verify_mldsa65` (ML-DSA-65 primary + Dilithium3 fallback). +/// TODO: remove once Dilithium3 legacy wires are fully retired (audit L-2). +#[allow(dead_code)] fn verify_legacy_mldsa65_item(input: &[u8]) -> bool { if input.len() < 8 { return false; @@ -317,10 +343,15 @@ fn verify_mldsa65_batch(input: &[u8]) -> (usize, bool) { if sig_start > input.len() { return (count, false); } - // Pass the item starting from cursor (i.e., includes 4-byte pk_len prefix) + // H-3: Use the same ML-DSA-65-first dispatch as the single-verify path + // (verify_mldsa65) so batch and single verification are consistent. + // Legacy Dilithium3-only dispatch (verify_legacy_mldsa65_item) is kept + // below for reference but no longer used here. + // TODO: once Dilithium3 legacy wires are retired, remove the fallback + // entirely (audit finding L-2). let item_start = cursor - 4; // include pk_len prefix let item = &input[item_start..]; - valid &= verify_legacy_mldsa65_item(item); + valid &= verify_mldsa65(item); // Advance cursor by pk_len + 4 (msg_len) + msg_len + sig_len let sig_len = DILITHIUM3_SIGNATURE_BYTES; let item_end = cursor + pk_len + 4 + msg_len + sig_len; @@ -443,4 +474,37 @@ mod tests { assert!(!sp.is_precompile(&address)); } } + + /// C-1: gas_limit=0 with count=100 must return OOG without panicking or + /// performing any verification work (no reachable panic path inside the + /// verification loop should be triggered). + #[test] + fn batch_verify_oog_before_verification_loop() { + // Build a minimal input with count=100 and no actual signature data. + // The function must return PrecompileOOG before entering the loop. + let mut input = Vec::new(); + input.extend_from_slice(&100u32.to_be_bytes()); // count = 100 + // No signature items — if the loop ran it would return false due to + // missing data, but it must never reach there with gas_limit=0. + + let result = run_mldsa65_batch_verify(0, &input); + assert_eq!( + result.result, + InstructionResult::PrecompileOOG, + "C-1: expected OOG with gas_limit=0" + ); + } + + /// C-1: count > MAX_BATCH_SIGNATURES must be rejected immediately. + #[test] + fn batch_verify_rejects_oversized_count() { + let mut input = Vec::new(); + input.extend_from_slice(&(MAX_BATCH_SIGNATURES + 1).to_be_bytes()); + let result = run_mldsa65_batch_verify(u64::MAX, &input); + assert_eq!( + result.result, + InstructionResult::PrecompileError, + "C-1: expected PrecompileError for count > MAX_BATCH_SIGNATURES" + ); + } } diff --git a/crates/node/Cargo.toml b/crates/node/Cargo.toml index 7e11594c..9a4a5c68 100644 --- a/crates/node/Cargo.toml +++ b/crates/node/Cargo.toml @@ -31,3 +31,10 @@ hyper-util.workspace = true http-body-util.workspace = true [dev-dependencies] + +[features] +# H-1: stub-l2-verifier allows L2 recursive proof verification to soft-pass. +# This MUST NOT be enabled in production. It exists only for testnet / local +# development environments where the real recursive prover is not yet wired in. +# Default builds do NOT enable this feature. +stub-l2-verifier = [] diff --git a/crates/node/src/node/mod.rs b/crates/node/src/node/mod.rs index fdeb5a1c..7c01a73e 100644 --- a/crates/node/src/node/mod.rs +++ b/crates/node/src/node/mod.rs @@ -617,7 +617,7 @@ impl Node { metrics.block_height.set(current_head as i64); metrics.update_finality(current_head, finality_state.last_finalized_number()); - Self { + let node = Self { config, store, chain_store, @@ -665,7 +665,17 @@ impl Node { tx_rebroadcast_seen: parking_lot::Mutex::new(HashMap::new()), last_proposed_by: parking_lot::Mutex::new(HashMap::new()), stark_drain_frontier: Arc::new(std::sync::atomic::AtomicU64::new(0)), - } + }; + + // H-1: Emit a loud startup warning when stub-l2-verifier is compiled in. + // This must never appear in production logs. + #[cfg(feature = "stub-l2-verifier")] + tracing::warn!( + "⚠️ stub-l2-verifier feature enabled — L2 settlement proofs are NOT verified. \ + DO NOT USE IN PRODUCTION." + ); + + node } fn block_store(&self) -> BlockStoreBoundary<'_, S> { @@ -1784,7 +1794,10 @@ mod tests { /// L2 source-binding validation must accept a source that IS in /// `settled_stark_sources` (happy path for the new canonical check). + /// With stub-l2-verifier disabled (default), garbage proof_bytes yield an + /// error at Check 3; this test runs only with the stub enabled. #[test] + #[cfg(feature = "stub-l2-verifier")] fn l2_source_binding_accepts_settled_l1_source() { use shell_stark_prover::recursive_air::compute_aggregate_root; let (node, signer) = setup_node(); @@ -1832,6 +1845,58 @@ mod tests { .expect("settled L1 source should be accepted by L2 source-binding validation"); } + /// H-1: Without stub-l2-verifier, garbage proof_bytes must be a hard error. + #[test] + #[cfg(not(feature = "stub-l2-verifier"))] + fn l2_source_binding_rejects_invalid_proof_bytes_without_stub() { + use shell_stark_prover::recursive_air::compute_aggregate_root; + let (node, signer) = setup_node(); + store_genesis(&node); + let genesis_hash = node + .chain_store + .get_block_hash_by_number(0) + .unwrap() + .unwrap(); + let hashes = produce_witnessed_blocks(&node, &signer, 1); + + let l1_src = dummy_ordered_amendment(1, vec![genesis_hash, hashes[0]], 1); + let l1_src_json = serde_json::to_vec(&l1_src).unwrap(); + node.amendment_store + .put_amendment(&l1_src.block_hash, &l1_src_json) + .unwrap(); + node.settled_stark_sources + .lock() + .insert((1, l1_src.block_hash)); + + let root = u128::from_le_bytes(l1_src.proof.batch_root_bytes); + let agg_root = compute_aggregate_root(&[root]); + let l2 = ProofAmendment { + version: shell_stark_prover::amendment::PROOF_AMENDMENT_VERSION, + block_hash: l1_src.block_hash, + block_number: 1, + start_block: Some(1), + proof: shell_stark_prover::proof::SigBatchProof { + version: shell_stark_prover::proof::SIG_BATCH_PROOF_VERSION, + batch_root_bytes: agg_root.to_le_bytes(), + n_sigs: 1, + // H-1: These garbage bytes cannot be decoded as a RecursiveProof. + proof_bytes: vec![0x33; 128], + }, + prover: Address::from([0x44; 32]), + prover_signature: Bytes::from(vec![0x55; 8]), + layer: 2, + source_hashes: vec![l1_src.block_hash], + original_size: Some(10_000), + compressed_size: Some(128), + settlement_tx_hash: None, + }; + let err = node.validate_stark_proof_source_binding(&l2); + assert!( + err.is_err(), + "H-1: garbage proof_bytes must be a hard error without stub-l2-verifier, got Ok" + ); + } + #[test] fn stark_settlement_sequence_allows_l2_after_l1_in_same_block() { let (node, signer) = setup_node(); @@ -5705,10 +5770,14 @@ mod tests { #[test] fn wpoa_handle_vote_reaches_quorum() { + // C-3: All voters must have their pubkeys registered and use real + // signatures over block_hash.as_bytes() (the vote pre-image). let signer1 = DilithiumSigner::generate(); + let signer2 = DilithiumSigner::generate(); + let signer3 = DilithiumSigner::generate(); let addr1 = Address::from_public_key(signer1.public_key(), signer1.sig_type().as_u8()); - let addr2 = Address::from([0xaa; 32]); - let addr3 = Address::from([0xbb; 32]); + let addr2 = Address::from_public_key(signer2.public_key(), signer2.sig_type().as_u8()); + let addr3 = Address::from_public_key(signer3.public_key(), signer3.sig_type().as_u8()); let db = Arc::new(MemoryDb::new()); let chain_store = Arc::new(ChainStore::new(db.clone())); @@ -5726,6 +5795,10 @@ mod tests { let config = NodeConfig::dev(addr1); let node = Node::new(config, db, chain_store, world_state, tx_pool, consensus); + // C-3: Register all validator public keys so sig verification can proceed. + node.register_authority_pubkey(addr1, signer1.public_key().to_vec()); + node.register_authority_pubkey(addr2, signer2.public_key().to_vec()); + node.register_authority_pubkey(addr3, signer3.public_key().to_vec()); store_genesis_wpoa(&node); // Manually initialise the wPoA round (the event loop does this after @@ -5739,16 +5812,9 @@ mod tests { *node.wpoa_round.lock() = Some(round); } - // addr2 votes first — still below quorum - node.handle_wpoa_vote( - addr2, - block_hash, - block_number, - shell_crypto::PQSignature::new( - shell_crypto::SignatureType::Dilithium3, - vec![0u8; 32], - ), - ); + // addr2 votes first with a valid signature — still below quorum. + let sig2 = signer2.sign(block_hash.as_bytes()).unwrap(); + node.handle_wpoa_vote(addr2, block_hash, block_number, sig2); let phase1 = node .wpoa_round .lock() @@ -5760,16 +5826,9 @@ mod tests { "should still be Voting after 1 vote" ); - // addr3 votes — quorum (2 of 3) reached - node.handle_wpoa_vote( - addr3, - block_hash, - block_number, - shell_crypto::PQSignature::new( - shell_crypto::SignatureType::Dilithium3, - vec![0u8; 32], - ), - ); + // addr3 votes with a valid signature — quorum (2 of 3) reached. + let sig3 = signer3.sign(block_hash.as_bytes()).unwrap(); + node.handle_wpoa_vote(addr3, block_hash, block_number, sig3); let phase2 = node .wpoa_round .lock() @@ -5782,6 +5841,68 @@ mod tests { ); } + /// C-3: A vote with a garbage signature for a known validator must be + /// rejected — the round must NOT advance past Voting. + #[test] + fn wpoa_handle_vote_rejects_garbage_signature() { + let signer1 = DilithiumSigner::generate(); + let signer2 = DilithiumSigner::generate(); + let signer3 = DilithiumSigner::generate(); + let addr1 = Address::from_public_key(signer1.public_key(), signer1.sig_type().as_u8()); + let addr2 = Address::from_public_key(signer2.public_key(), signer2.sig_type().as_u8()); + let addr3 = Address::from_public_key(signer3.public_key(), signer3.sig_type().as_u8()); + + let db = Arc::new(MemoryDb::new()); + let chain_store = Arc::new(ChainStore::new(db.clone())); + let world_state = Arc::new(RwLock::new(WorldState::new(db.clone()))); + + let poa_cfg = PoaConfig::new(vec![addr1, addr2, addr3], 1); + let wpoa_cfg = WPoaConfig::from_poa(poa_cfg); + let engine = WPoaEngine::new(wpoa_cfg, Arc::new(MultiVerifier)); + let consensus: Arc> = Arc::new(RwLock::new(engine)); + + let tx_pool = Arc::new(TxPool::new(MempoolConfig { + chain_id: 1337, + ..MempoolConfig::default() + })); + + let config = NodeConfig::dev(addr1); + let node = Node::new(config, db, chain_store, world_state, tx_pool, consensus); + node.register_authority_pubkey(addr1, signer1.public_key().to_vec()); + node.register_authority_pubkey(addr2, signer2.public_key().to_vec()); + node.register_authority_pubkey(addr3, signer3.public_key().to_vec()); + store_genesis_wpoa(&node); + + let block_hash = hash(99); + let block_number = 1u64; + { + let weights = node.consensus.read().validator_weights(); + let mut round = WPoaRound::new(block_number, 0, weights); + let _ = round.on_block_proposed(block_hash, addr1); + *node.wpoa_round.lock() = Some(round); + } + + // Send votes with garbage signatures for addr2 and addr3. + // Neither should be accepted by the round (C-3 fix). + let garbage_sig = shell_crypto::PQSignature::new( + shell_crypto::SignatureType::Dilithium3, + vec![0xde, 0xad, 0xbe, 0xef], + ); + node.handle_wpoa_vote(addr2, block_hash, block_number, garbage_sig.clone()); + node.handle_wpoa_vote(addr3, block_hash, block_number, garbage_sig); + + let phase = node + .wpoa_round + .lock() + .as_ref() + .map(|r| r.phase_name().to_string()); + assert_eq!( + phase.as_deref(), + Some("Voting"), + "C-3: round must NOT advance when all votes have garbage signatures" + ); + } + // ── 6. Serde: NetworkMessage::WPoaVote roundtrip ────────────────────── #[test] diff --git a/crates/node/src/node/p2p_handlers.rs b/crates/node/src/node/p2p_handlers.rs index c8e21e00..70a1b58e 100644 --- a/crates/node/src/node/p2p_handlers.rs +++ b/crates/node/src/node/p2p_handlers.rs @@ -205,6 +205,80 @@ impl Node { } } + // C-3: Verify the vote's PQ signature before dispatching into consensus. + // The signing pre-image is the raw block hash bytes (mirrors event_loop.rs). + // Mirrors the same pattern used in handle_wpoa_view_change. + { + let known = self.known_authorities.read(); + let pubkey = match known.get(&voter) { + Some(pk) => pk.clone(), + None => { + tracing::warn!( + %voter, + "C-3: WPoA vote from unknown validator — rejecting" + ); + return; + } + }; + drop(known); // release read lock before potential peer_scorer lock + + let sig_type = match shell_crypto::infer_signature_type_from_address(&pubkey, &voter) { + Some(t) if shell_crypto::is_algorithm_allowed(t) => t, + Some(t) => { + tracing::warn!( + %voter, + algorithm = ?t, + "C-3: WPoA vote uses disallowed signature algorithm — rejecting" + ); + let peer_id = shell_consensus::ScoringPeerId::from(format!("{voter:?}")); + self.peer_scorer + .lock() + .record_event(&peer_id, shell_consensus::PeerEvent::InvalidProofPayload); + return; + } + None => { + tracing::warn!( + %voter, + "C-3: cannot infer signature algorithm for WPoA voter — rejecting" + ); + let peer_id = shell_consensus::ScoringPeerId::from(format!("{voter:?}")); + self.peer_scorer + .lock() + .record_event(&peer_id, shell_consensus::PeerEvent::InvalidProofPayload); + return; + } + }; + + let typed_sig = shell_crypto::PQSignature::new(sig_type, sig.data.clone()); + let verifier = MultiVerifier; + match verifier.verify(&pubkey, block_hash.as_bytes(), &typed_sig) { + Ok(true) => {} // valid — proceed to consensus + Ok(false) => { + tracing::warn!( + %voter, + "C-3: WPoA vote signature verification failed (possible forgery) — dropping" + ); + let peer_id = shell_consensus::ScoringPeerId::from(format!("{voter:?}")); + self.peer_scorer + .lock() + .record_event(&peer_id, shell_consensus::PeerEvent::InvalidProofPayload); + return; + } + Err(e) => { + tracing::warn!( + %voter, + error = %e, + "C-3: WPoA vote signature verification error — dropping" + ); + let peer_id = shell_consensus::ScoringPeerId::from(format!("{voter:?}")); + self.peer_scorer + .lock() + .record_event(&peer_id, shell_consensus::PeerEvent::InvalidProofPayload); + return; + } + } + } + let mut guard = self.wpoa_round.lock(); if let Some(ref mut round) = *guard { if round.block_number != block_number { diff --git a/crates/node/src/node/system_rewards.rs b/crates/node/src/node/system_rewards.rs index e57eac97..b9d4b066 100644 --- a/crates/node/src/node/system_rewards.rs +++ b/crates/node/src/node/system_rewards.rs @@ -365,6 +365,9 @@ impl Node { } // Check 3: recursive proof verification. + // H-1: Both soft-pass paths (decode error, NotImplemented) are now hard + // errors by default. They are only permitted when compiled with the + // `stub-l2-verifier` feature, which MUST NOT be enabled in production. let pub_inputs = shell_stark_prover::RecursivePublicInputs { l1_roots, aggregate_root: expected_agg_root, @@ -374,23 +377,31 @@ impl Node { end_block: amendment.block_number, }; let prover = shell_stark_prover::get_recursive_prover(); - // Check 3: recursive proof verification (best-effort; requires - // feature = "recursive"). Until the real prover is wired in, the - // scaffold returns NotImplemented — treated as a soft pass so that - // testnet L2 settlements can proceed. Source-binding checks (1 & 2) - // above are the canonical gate for now. if let Ok(rec_proof) = serde_json::from_slice::( &amendment.proof.proof_bytes, ) { match prover.verify_aggregation(&rec_proof, &pub_inputs) { Ok(()) => {} Err(shell_stark_prover::RecursiveProverError::NotImplemented) => { - // Recursive verifier is not yet active; soft-pass. - tracing::debug!( - block_hash = %amendment.block_hash, - "STARK L2 recursive proof verifier not yet active — \ - source-binding checks passed, soft-accepting" - ); + // H-1: recursive verifier not implemented. + #[cfg(feature = "stub-l2-verifier")] + { + tracing::debug!( + block_hash = %amendment.block_hash, + "STARK L2 recursive proof verifier not active (stub-l2-verifier) — \ + source-binding checks passed, soft-accepting" + ); + } + #[cfg(not(feature = "stub-l2-verifier"))] + { + self.metrics.stark_settlements_rejected.inc(); + return Err(NodeError::Startup(format!( + "STARK L2 recursive proof verifier is not implemented \ + (block #{}). Enable feature `stub-l2-verifier` only in \ + non-production environments to bypass this check.", + amendment.block_number + ))); + } } Err(e) => { self.metrics.stark_settlements_rejected.inc(); @@ -400,15 +411,26 @@ impl Node { } } } else { - // proof_bytes cannot be decoded as a RecursiveProof. - // The recursive verifier is scaffolded (returns NotImplemented), so - // source-binding checks 1 & 2 above are the canonical gate for now. - // Log a warning so this is visible when the real verifier is wired in. - tracing::warn!( - block_hash = %amendment.block_hash, - "STARK L2 proof_bytes are not a valid RecursiveProof — \ - soft-accepting because recursive verifier is not yet active" - ); + // H-1: proof_bytes cannot be decoded as a RecursiveProof — hard error + // unless the stub-l2-verifier feature is enabled. + #[cfg(not(feature = "stub-l2-verifier"))] + { + self.metrics.stark_settlements_rejected.inc(); + return Err(NodeError::Startup(format!( + "STARK L2 proof_bytes for block #{} cannot be decoded as a \ + RecursiveProof. Enable feature `stub-l2-verifier` only in \ + non-production environments to bypass this check.", + amendment.block_number + ))); + } + #[cfg(feature = "stub-l2-verifier")] + { + tracing::warn!( + block_hash = %amendment.block_hash, + "STARK L2 proof_bytes are not a valid RecursiveProof — \ + soft-accepting because stub-l2-verifier feature is enabled" + ); + } } Ok(()) diff --git a/deny.toml b/deny.toml index 2641ae52..211a3221 100644 --- a/deny.toml +++ b/deny.toml @@ -1,8 +1,18 @@ [advisories] # Deny crates with known security advisories. ignore = [ - # Pulled by libp2p 0.56 via hickory-proto 0.25.2. Upstream has not - # released a libp2p version that can consume hickory-proto >=0.26.1 yet. + # H-4 (audit finding 2026-05-24): hickory-proto 0.25.2 pulled transitively + # by libp2p 0.56. A patched version (>=0.26.1) exists on crates.io but + # libp2p has not yet published a release that accepts it, so we cannot bump + # without forking libp2p or waiting for an upstream libp2p release. + # + # Mitigation: hickory-proto/hickory-resolver are used by libp2p ONLY for + # DNS-based peer bootstrap (mDNS + DNS/SD). The failure mode is a + # bootstrap stall — DNS resolution returns an error and the node falls back + # to known static peers. There is NO data-integrity or remote-code-execution + # exposure for shell-chain: DNS results are not persisted, not used in + # consensus, and not accessible from the contract execution layer. + # This acceptance will be removed once libp2p ships a compatible release. "RUSTSEC-2026-0118", "RUSTSEC-2026-0119", ] From a273ca76295f9a7102aa6482f93daee07e40eed1 Mon Sep 17 00:00:00 2001 From: LucienSong Date: Sun, 24 May 2026 13:37:58 +0800 Subject: [PATCH 2/2] security: address PR #50 review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - CI clippy: use saturating_mul instead of checked_mul+unwrap_or - precompiles.rs: fix per-item slicing in verify_mldsa65_batch — was passing &input[item_start..] (open-ended), causing verify to see signature + trailing bytes from subsequent items. Now slices to exact item end so count>1 batches verify correctly. - p2p_handlers.rs: reject WPoA votes when sig.sig_type != infer_signature_type_from_address(voter); prevents algorithm-tag confusion in commit certificates. - precompiles.rs tests: add multi-item batch verify regression test (count=2 happy path + one-tampered-sig path). - p2p_handlers.rs tests: add wpoa_handle_vote_rejects_sig_type_mismatch asserting rejection when sig.sig_type does not match inferred type. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/evm/src/precompiles.rs | 98 ++++++++++++++++++++++++---- crates/node/src/node/mod.rs | 68 +++++++++++++++++++ crates/node/src/node/p2p_handlers.rs | 21 ++++++ 3 files changed, 175 insertions(+), 12 deletions(-) diff --git a/crates/evm/src/precompiles.rs b/crates/evm/src/precompiles.rs index c245a82a..8ea3e0f9 100644 --- a/crates/evm/src/precompiles.rs +++ b/crates/evm/src/precompiles.rs @@ -189,9 +189,7 @@ fn run_mldsa65_batch_verify(gas_limit: u64, input: &[u8]) -> InterpreterResult { // C-1: Charge the full gas cost BEFORE entering the verification loop so // that a caller with gas_limit = 0 receives OOG without doing any work. - let total_cost = PQ_MLDSA65_BATCH_VERIFY_GAS_PER_SIG - .checked_mul(count as u64) - .unwrap_or(u64::MAX); + let total_cost = PQ_MLDSA65_BATCH_VERIFY_GAS_PER_SIG.saturating_mul(count as u64); if !charge_gas(&mut result, total_cost) { return result; } @@ -343,21 +341,23 @@ fn verify_mldsa65_batch(input: &[u8]) -> (usize, bool) { if sig_start > input.len() { return (count, false); } + // Compute exact item boundaries before verifying so we pass only + // [item_start..item_end] to verify_mldsa65 — not an open-ended slice + // that would expose trailing bytes from subsequent items. + let sig_len = DILITHIUM3_SIGNATURE_BYTES; + let item_start = cursor - 4; // include pk_len prefix + let item_end = cursor + pk_len + 4 + msg_len + sig_len; + if item_end > input.len() { + return (count, false); + } // H-3: Use the same ML-DSA-65-first dispatch as the single-verify path // (verify_mldsa65) so batch and single verification are consistent. // Legacy Dilithium3-only dispatch (verify_legacy_mldsa65_item) is kept // below for reference but no longer used here. // TODO: once Dilithium3 legacy wires are retired, remove the fallback // entirely (audit finding L-2). - let item_start = cursor - 4; // include pk_len prefix - let item = &input[item_start..]; + let item = &input[item_start..item_end]; valid &= verify_mldsa65(item); - // Advance cursor by pk_len + 4 (msg_len) + msg_len + sig_len - let sig_len = DILITHIUM3_SIGNATURE_BYTES; - let item_end = cursor + pk_len + 4 + msg_len + sig_len; - if item_end > input.len() { - return (count, false); - } cursor = item_end; } @@ -373,7 +373,7 @@ fn bool_output(valid: bool) -> Bytes { #[cfg(test)] mod tests { use super::*; - use shell_crypto::{DilithiumSigner, MlDsaSigner, SignatureType, Signer, SphincsSigner}; + use shell_crypto::{DilithiumSigner, Signer, SphincsSigner}; #[test] fn pq_suite_addresses_match_spec() { @@ -507,4 +507,78 @@ mod tests { "C-1: expected PrecompileError for count > MAX_BATCH_SIGNATURES" ); } + + /// Helper: encode one batch item as [pk_len(4)][pubkey][msg_len(4)][msg][sig]. + fn encode_batch_item(pubkey: &[u8], message: &[u8], sig: &[u8]) -> Vec { + let mut item = Vec::new(); + item.extend_from_slice(&(pubkey.len() as u32).to_be_bytes()); + item.extend_from_slice(pubkey); + item.extend_from_slice(&(message.len() as u32).to_be_bytes()); + item.extend_from_slice(message); + item.extend_from_slice(sig); + item + } + + /// Regression test: count=2 happy path — both items must verify correctly. + /// Verifies the slicing fix: item_start..item_end rather than item_start.. + #[test] + fn batch_verify_multi_item_happy_path() { + let signer1 = DilithiumSigner::generate(); + let signer2 = DilithiumSigner::generate(); + + let msg1 = b"batch item one"; + let msg2 = b"batch item two"; + let sig1 = signer1.sign(msg1).unwrap(); + let sig2 = signer2.sign(msg2).unwrap(); + + let mut input = Vec::new(); + input.extend_from_slice(&2u32.to_be_bytes()); // count = 2 + input.extend(encode_batch_item(signer1.public_key(), msg1, &sig1.data)); + input.extend(encode_batch_item(signer2.public_key(), msg2, &sig2.data)); + + let gas = PQ_MLDSA65_BATCH_VERIFY_GAS_PER_SIG * 2 + 1_000; + let result = run_mldsa65_batch_verify(gas, &input); + let mut expected = [0u8; 32]; + expected[31] = 1; + assert_eq!( + result.output.as_ref(), + &expected, + "count=2 batch should verify successfully" + ); + } + + /// Regression test: count=2 with one tampered signature must return false. + #[test] + fn batch_verify_multi_item_tampered_sig_fails() { + let signer1 = DilithiumSigner::generate(); + let signer2 = DilithiumSigner::generate(); + + let msg1 = b"batch item one"; + let msg2 = b"batch item two"; + let sig1 = signer1.sign(msg1).unwrap(); + let sig2 = signer2.sign(msg2).unwrap(); + + // Tamper sig2: flip a byte in the middle. + let mut sig2_tampered = sig2.data.clone(); + let mid = sig2_tampered.len() / 2; + sig2_tampered[mid] ^= 0xFF; + + let mut input = Vec::new(); + input.extend_from_slice(&2u32.to_be_bytes()); // count = 2 + input.extend(encode_batch_item(signer1.public_key(), msg1, &sig1.data)); + input.extend(encode_batch_item( + signer2.public_key(), + msg2, + &sig2_tampered, + )); + + let gas = PQ_MLDSA65_BATCH_VERIFY_GAS_PER_SIG * 2 + 1_000; + let result = run_mldsa65_batch_verify(gas, &input); + let expected_false = [0u8; 32]; + assert_eq!( + result.output.as_ref(), + &expected_false, + "count=2 batch with tampered second sig must return false" + ); + } } diff --git a/crates/node/src/node/mod.rs b/crates/node/src/node/mod.rs index 7c01a73e..e94a78f3 100644 --- a/crates/node/src/node/mod.rs +++ b/crates/node/src/node/mod.rs @@ -5903,6 +5903,74 @@ mod tests { ); } + /// Security: A vote with a valid signature but a wrong `sig_type` tag + /// must be rejected — algorithm-tag confusion must not allow fake commit + /// certificates. + #[test] + fn wpoa_handle_vote_rejects_sig_type_mismatch() { + use shell_crypto::SignatureType; + + // Use DilithiumSigner so the voter's address encodes Dilithium3. + let signer = DilithiumSigner::generate(); + let addr = Address::from_public_key(signer.public_key(), signer.sig_type().as_u8()); + + // Need a second validator to bootstrap the round (proposer). + let proposer = DilithiumSigner::generate(); + let proposer_addr = + Address::from_public_key(proposer.public_key(), proposer.sig_type().as_u8()); + + let db = Arc::new(MemoryDb::new()); + let chain_store = Arc::new(ChainStore::new(db.clone())); + let world_state = Arc::new(RwLock::new(WorldState::new(db.clone()))); + + let poa_cfg = PoaConfig::new(vec![proposer_addr, addr], 1); + let wpoa_cfg = WPoaConfig::from_poa(poa_cfg); + let engine = WPoaEngine::new(wpoa_cfg, Arc::new(MultiVerifier)); + let consensus: Arc> = Arc::new(RwLock::new(engine)); + + let tx_pool = Arc::new(TxPool::new(MempoolConfig { + chain_id: 1337, + ..MempoolConfig::default() + })); + + let config = NodeConfig::dev(proposer_addr); + let node = Node::new(config, db, chain_store, world_state, tx_pool, consensus); + node.register_authority_pubkey(proposer_addr, proposer.public_key().to_vec()); + node.register_authority_pubkey(addr, signer.public_key().to_vec()); + store_genesis_wpoa(&node); + + let block_hash = hash(77); + let block_number = 1u64; + { + let weights = node.consensus.read().validator_weights(); + let mut round = WPoaRound::new(block_number, 0, weights); + let _ = round.on_block_proposed(block_hash, proposer_addr); + *node.wpoa_round.lock() = Some(round); + } + + // Build a valid Dilithium3 signature over block_hash, but lie about + // sig_type — claim it's MlDsa65 so the tag mismatches the inferred type. + let real_sig = signer.sign(block_hash.as_bytes()).unwrap(); + let mismatched_sig = shell_crypto::PQSignature::new( + SignatureType::MlDsa65, // wrong tag — signer used Dilithium3 + real_sig.data, + ); + + node.handle_wpoa_vote(addr, block_hash, block_number, mismatched_sig); + + // The vote must have been rejected: round stays in Voting (no votes accepted). + let phase = node + .wpoa_round + .lock() + .as_ref() + .map(|r| r.phase_name().to_string()); + assert_eq!( + phase.as_deref(), + Some("Voting"), + "algorithm-tag mismatch must cause vote rejection" + ); + } + // ── 6. Serde: NetworkMessage::WPoaVote roundtrip ────────────────────── #[test] diff --git a/crates/node/src/node/p2p_handlers.rs b/crates/node/src/node/p2p_handlers.rs index 70a1b58e..c64dc72b 100644 --- a/crates/node/src/node/p2p_handlers.rs +++ b/crates/node/src/node/p2p_handlers.rs @@ -249,6 +249,27 @@ impl Node { } }; + // Security: reject votes whose sender-controlled sig_type tag does + // not match the canonical algorithm inferred from the voter's address. + // Accepting a mismatched tag would allow an attacker to store a + // commit certificate whose algorithm label differs from the one used + // during verification (algorithm-tag confusion). + if sig.sig_type != sig_type { + tracing::warn!( + voter = %voter, + claimed = ?sig.sig_type, + expected = ?sig_type, + "vote sig_type mismatch: claimed={:?} expected={:?} — rejecting", + sig.sig_type, + sig_type, + ); + let peer_id = shell_consensus::ScoringPeerId::from(format!("{voter:?}")); + self.peer_scorer + .lock() + .record_event(&peer_id, shell_consensus::PeerEvent::InvalidProofPayload); + return; + } + let typed_sig = shell_crypto::PQSignature::new(sig_type, sig.data.clone()); let verifier = MultiVerifier; match verifier.verify(&pubkey, block_hash.as_bytes(), &typed_sig) {