From d50fe878015f823010b0f844ee5a5478e3ed1847 Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Fri, 13 Oct 2023 04:40:59 -0400 Subject: [PATCH] Correct the prior documented TOCTOU Now, if a malicious validator set publishes a malicious `Batch` at the last moment, it'll cause all future `Batch`s signed by the next validator set to require a bool being set (yet they never will set it). This will prevent the handover. The only overhead is having two distinct `batch_message` calls on-chain. --- coordinator/src/main.rs | 9 ------- processor/src/substrate_signer.rs | 24 ++++++++++++++++++- processor/src/tests/substrate_signer.rs | 3 ++- .../client/tests/common/in_instructions.rs | 3 ++- substrate/in-instructions/pallet/src/lib.rs | 15 ++++++++---- .../in-instructions/primitives/src/lib.rs | 4 ++-- tests/coordinator/src/tests/batch.rs | 2 +- tests/processor/src/tests/batch.rs | 6 +++-- 8 files changed, 45 insertions(+), 21 deletions(-) diff --git a/coordinator/src/main.rs b/coordinator/src/main.rs index 2375bad44..c1625ce5b 100644 --- a/coordinator/src/main.rs +++ b/coordinator/src/main.rs @@ -924,15 +924,6 @@ async fn handle_processor_messages( MainDb::::set_did_handover(&mut txn, spec.set()); } - // TODO: There is a race condition here. We may verify all `Batch`s from the prior - // set, start signing the handover Batch `n`, start signing `n+1`, have `n+1` - // signed before `n` (or at the same time), yet then the prior set forges a - // malicious Batch `n`. - // - // The malicious Batch `n` would be publishable to Serai, as Serai can't - // distinguish what's intended to be a handover `Batch`, yet then anyone could - // publish the new set's `n+1`, causing their acceptance of the handover. - Some(Transaction::Batch(block.0, id.id)) } else { Some(Transaction::BatchPreprocess(SignData { diff --git a/processor/src/substrate_signer.rs b/processor/src/substrate_signer.rs index e7c0ded61..843fce438 100644 --- a/processor/src/substrate_signer.rs +++ b/processor/src/substrate_signer.rs @@ -52,6 +52,18 @@ impl SubstrateSignerDb { D::key(b"SUBSTRATE_SIGNER", dst, key) } + fn first_batch_key(key: [u8; 32]) -> Vec { + Self::sign_key(b"first_batch", key) + } + fn save_first_batch(txn: &mut D::Transaction<'_>, key: [u8; 32], id: u32) { + txn.put(Self::first_batch_key(key), id.to_le_bytes()); + } + fn first_batch(getter: &G, key: [u8; 32]) -> Option { + getter + .get(Self::first_batch_key(key)) + .map(|bytes| u32::from_le_bytes(bytes.try_into().unwrap())) + } + fn completed_key(id: [u8; 32]) -> Vec { Self::sign_key(b"completed", id) } @@ -229,6 +241,11 @@ impl SubstrateSigner { return; } + let group_key = self.keys.group_key().to_bytes(); + if SubstrateSignerDb::::first_batch(txn, group_key).is_none() { + SubstrateSignerDb::::save_first_batch(txn, group_key, batch.id); + } + self.signable.insert(id, batch); self.attempt(txn, id, 0).await; } @@ -270,8 +287,13 @@ impl SubstrateSigner { Err(e) => todo!("malicious signer: {:?}", e), }; + let batch = &self.signable[&id.id]; + let is_first_batch = + SubstrateSignerDb::::first_batch(txn, self.keys.group_key().to_bytes()).unwrap() == + batch.id; + let (machine, share) = - match machine.sign(preprocesses, &batch_message(&self.signable[&id.id])) { + match machine.sign(preprocesses, &batch_message(is_first_batch, batch)) { Ok(res) => res, Err(e) => todo!("malicious signer: {:?}", e), }; diff --git a/processor/src/tests/substrate_signer.rs b/processor/src/tests/substrate_signer.rs index a457b56fd..8cc047ff0 100644 --- a/processor/src/tests/substrate_signer.rs +++ b/processor/src/tests/substrate_signer.rs @@ -146,8 +146,9 @@ async fn test_substrate_signer() { signers.get_mut(i).unwrap().events.pop_front().unwrap() { assert_eq!(signed_batch.batch, batch); + // SubstrateSigner will believe this is the first batch for this set, hence `true` assert!(Public::from_raw(keys[&participant_one].group_key().to_bytes()) - .verify(&batch_message(&batch), &signed_batch.signature)); + .verify(&batch_message(true, &batch), &signed_batch.signature)); } else { panic!("didn't get signed batch back"); } diff --git a/substrate/client/tests/common/in_instructions.rs b/substrate/client/tests/common/in_instructions.rs index 45f903e83..1a67e518c 100644 --- a/substrate/client/tests/common/in_instructions.rs +++ b/substrate/client/tests/common/in_instructions.rs @@ -39,7 +39,8 @@ pub async fn provide_batch(batch: Batch) -> [u8; 32] { let block = publish_tx(&Serai::execute_batch(SignedBatch { batch: batch.clone(), - signature: pair.sign(&batch_message(&batch)), + // TODO: This `batch.id == 0` line only works when session == 0 + signature: pair.sign(&batch_message(batch.id == 0, &batch)), })) .await; diff --git a/substrate/in-instructions/pallet/src/lib.rs b/substrate/in-instructions/pallet/src/lib.rs index 4bb943acb..cf43570a4 100644 --- a/substrate/in-instructions/pallet/src/lib.rs +++ b/substrate/in-instructions/pallet/src/lib.rs @@ -155,14 +155,21 @@ pub mod pallet { // verify the signature let network = batch.batch.network; let (current_session, prior, current) = keys_for_network::(network)?; - let batch_message = batch_message(&batch.batch); // Check the prior key first since only a single `Batch` (the last one) will be when prior is // Some yet prior wasn't the signing key - let valid_by_prior = - if let Some(key) = prior { key.verify(&batch_message, &batch.signature) } else { false }; + let valid_by_prior = if let Some(key) = prior { + key.verify(&batch_message(false, &batch.batch), &batch.signature) + } else { + false + }; let valid = valid_by_prior || (if let Some(key) = current { - key.verify(&batch_message, &batch.signature) + key.verify( + // This `== 0` is valid as either it'll be the first Batch for the first set, or if + // they never had a Batch, the first Batch for the next set + &batch_message((batch.batch.id == 0) || prior.is_some(), &batch.batch), + &batch.signature, + ) } else { false }); diff --git a/substrate/in-instructions/primitives/src/lib.rs b/substrate/in-instructions/primitives/src/lib.rs index 74349de9b..860fd5eae 100644 --- a/substrate/in-instructions/primitives/src/lib.rs +++ b/substrate/in-instructions/primitives/src/lib.rs @@ -84,6 +84,6 @@ impl Zeroize for SignedBatch { // TODO: Make this an associated method? /// The message for the batch signature. -pub fn batch_message(batch: &Batch) -> Vec { - [b"InInstructions-batch".as_ref(), &batch.encode()].concat() +pub fn batch_message(is_first_batch_of_set: bool, batch: &Batch) -> Vec { + [b"InInstructions-batch".as_ref(), &(is_first_batch_of_set, batch).encode()].concat() } diff --git a/tests/coordinator/src/tests/batch.rs b/tests/coordinator/src/tests/batch.rs index caa567333..ef17f430c 100644 --- a/tests/coordinator/src/tests/batch.rs +++ b/tests/coordinator/src/tests/batch.rs @@ -165,7 +165,7 @@ pub async fn batch( let signature = Signature( schnorrkel::keys::Keypair::from_bytes(&schnorrkel_key_pair) .unwrap() - .sign_simple(b"substrate", &batch_message(&batch)) + .sign_simple(b"substrate", &batch_message(batch.id == 0, &batch)) .to_bytes(), ); diff --git a/tests/processor/src/tests/batch.rs b/tests/processor/src/tests/batch.rs index b78836649..8932a53ec 100644 --- a/tests/processor/src/tests/batch.rs +++ b/tests/processor/src/tests/batch.rs @@ -137,8 +137,10 @@ pub(crate) async fn sign_batch( messages::substrate::ProcessorMessage::SignedBatch { batch: this_batch }, ) => { if batch.is_none() { - assert!(PublicKey::from_raw(key) - .verify(&batch_message(&this_batch.batch), &this_batch.signature)); + assert!(PublicKey::from_raw(key).verify( + &batch_message(this_batch.batch.id == 0, &this_batch.batch), + &this_batch.signature + )); batch = Some(this_batch.clone()); }