From 95d278ef7b8165cff73fdb351f6e003469779ddb Mon Sep 17 00:00:00 2001 From: Laura Makdah Date: Wed, 6 Apr 2022 16:07:51 -0700 Subject: [PATCH] Added the following security checks: - for batches, reconstruct the batch and ensure its digest matches the one provided - ensure the stream stops after n items, where n is the number of sequences in the requested range --- sui_core/src/safe_client.rs | 85 +++++++++++++++++++++---------------- sui_types/src/batch.rs | 42 +++++++++++++++--- 2 files changed, 84 insertions(+), 43 deletions(-) diff --git a/sui_core/src/safe_client.rs b/sui_core/src/safe_client.rs index 7b774f99dd869..8d9e8c1f11eeb 100644 --- a/sui_core/src/safe_client.rs +++ b/sui_core/src/safe_client.rs @@ -9,7 +9,7 @@ use std::io; use sui_types::crypto::PublicKeyBytes; use sui_types::{base_types::*, committee::*, fp_ensure}; -use sui_types::batch::{SignedBatch, TxSequenceNumber, UpdateItem}; +use sui_types::batch::{AuthorityBatch, SignedBatch, UpdateItem}; use sui_types::{ error::{SuiError, SuiResult}, messages::*, @@ -175,6 +175,7 @@ impl SafeClient { request: BatchInfoRequest, signed_batch: &SignedBatch, ) -> SuiResult { + // check the signature of the batch signed_batch .signature .check(signed_batch, signed_batch.authority)?; @@ -188,18 +189,23 @@ impl SafeClient { authority: self.address } ); - // todo: ensure signature valid over the set of transactions in the batch - // todo: ensure signature valid over the hash of the previous batch - Ok(()) - } - fn check_update_item_transaction_response( - &self, - _request: BatchInfoRequest, - _seq: &TxSequenceNumber, - _digest: &TransactionDigest, - ) -> SuiResult { - todo!(); + // reconstruct the batch and make sure the constructed digest matches the provided one + let provided_digest = signed_batch.batch.transactions_digest; + + let reconstructed_batch = AuthorityBatch::make_next_with_previous_digest( + Some(provided_digest), + &signed_batch.batch.transaction_batch.0, + ); + let computed_digest = reconstructed_batch.digest(); + + fp_ensure!( + provided_digest == computed_digest, + SuiError::ByzantineAuthoritySuspicion { + authority: self.address + } + ); + Ok(()) } /// This function is used by the higher level authority logic to report an @@ -304,34 +310,39 @@ where .handle_batch_stream(request.clone()) .await?; + let seq_requested = request.end - request.start; + let mut seq_to_be_returned = seq_requested as usize; + // check for overflow + if seq_requested > usize::MAX as u64 { + seq_to_be_returned = usize::MAX; + } + let client = self.clone(); - let stream = Box::pin(batch_info_items.then(move |batch_info_item| { - let req_clone = request.clone(); - let client = client.clone(); - async move { - match &batch_info_item { - Ok(BatchInfoResponseItem(UpdateItem::Batch(signed_batch))) => { - if let Err(err) = - client.check_update_item_batch_response(req_clone, signed_batch) - { - client.report_client_error(err.clone()); - return Err(err); + let stream = Box::pin( + batch_info_items + .then(move |batch_info_item| { + let req_clone = request.clone(); + let client = client.clone(); + async move { + match &batch_info_item { + Ok(BatchInfoResponseItem(UpdateItem::Batch(signed_batch))) => { + if let Err(err) = + client.check_update_item_batch_response(req_clone, signed_batch) + { + client.report_client_error(err.clone()); + return Err(err); + } + batch_info_item + } + Ok(BatchInfoResponseItem(UpdateItem::Transaction((_seq, _digest)))) => { + batch_info_item + } + Err(e) => Err(e.clone()), } - batch_info_item } - Ok(BatchInfoResponseItem(UpdateItem::Transaction((seq, digest)))) => { - if let Err(err) = - client.check_update_item_transaction_response(req_clone, seq, digest) - { - client.report_client_error(err.clone()); - return Err(err); - } - batch_info_item - } - Err(e) => Err(e.clone()), - } - } - })); + }) + .take(seq_to_be_returned), + ); Ok(Box::pin(stream)) } } diff --git a/sui_types/src/batch.rs b/sui_types/src/batch.rs index 93eb24dd5b1a7..30387b8b3dbc9 100644 --- a/sui_types/src/batch.rs +++ b/sui_types/src/batch.rs @@ -17,7 +17,7 @@ pub enum UpdateItem { pub type BatchDigest = [u8; 32]; #[derive(Eq, PartialEq, Ord, PartialOrd, Clone, Hash, Default, Debug, Serialize, Deserialize)] -pub struct TransactionBatch(Vec<(TxSequenceNumber, TransactionDigest)>); +pub struct TransactionBatch(pub Vec<(TxSequenceNumber, TransactionDigest)>); impl BcsSignable for TransactionBatch {} #[derive(Eq, PartialEq, Ord, PartialOrd, Clone, Default, Debug, Serialize, Deserialize)] @@ -35,8 +35,11 @@ pub struct AuthorityBatch { /// The digest of the previous block, if there is one pub previous_digest: Option, - // The digest of all transactions digests in this batch + /// The digest of all transactions digests in this batch pub transactions_digest: [u8; 32], + + /// The set of transactions in this batch + pub transaction_batch: TransactionBatch, } impl BcsSignable for AuthorityBatch {} @@ -49,8 +52,8 @@ impl AuthorityBatch { /// The first batch for any authority indexes at zero /// and has zero length. pub fn initial() -> AuthorityBatch { - let to_hash = TransactionBatch(Vec::new()); - let transactions_digest = sha3_hash(&to_hash); + let transaction_batch = TransactionBatch(Vec::new()); + let transactions_digest = sha3_hash(&transaction_batch); AuthorityBatch { next_sequence_number: 0, @@ -58,6 +61,7 @@ impl AuthorityBatch { size: 0, previous_digest: None, transactions_digest, + transaction_batch, } } @@ -73,8 +77,8 @@ impl AuthorityBatch { let initial_sequence_number = transaction_vec[0].0 as u64; let next_sequence_number = (transaction_vec[transaction_vec.len() - 1].0 + 1) as u64; - let to_hash = TransactionBatch(transaction_vec); - let transactions_digest = sha3_hash(&to_hash); + let transaction_batch = TransactionBatch(transaction_vec); + let transactions_digest = sha3_hash(&transaction_batch); AuthorityBatch { next_sequence_number, @@ -82,6 +86,32 @@ impl AuthorityBatch { size: transactions.len() as u64, previous_digest: Some(previous_batch.digest()), transactions_digest, + transaction_batch, + } + } + + /// Make a batch, containing some transactions, and following the previous + /// batch, using the digest of the previous batch. + pub fn make_next_with_previous_digest( + previous_batch_digest: Option, + transactions: &[(TxSequenceNumber, TransactionDigest)], + ) -> AuthorityBatch { + let transaction_vec = transactions.to_vec(); + debug_assert!(!transaction_vec.is_empty()); + + let initial_sequence_number = transaction_vec[0].0 as u64; + let next_sequence_number = (transaction_vec[transaction_vec.len() - 1].0 + 1) as u64; + + let transaction_batch = TransactionBatch(transaction_vec); + let transactions_digest = sha3_hash(&transaction_batch); + + AuthorityBatch { + next_sequence_number, + initial_sequence_number, + size: transactions.len() as u64, + previous_digest: previous_batch_digest, + transactions_digest, + transaction_batch, } } }