From 192ff1e817d926d7db5f16d014df4e83268269ce Mon Sep 17 00:00:00 2001 From: Tiago Carvalho Date: Fri, 10 May 2024 14:11:34 +0100 Subject: [PATCH 1/3] Validate borsh deserialization of ProposalBytes --- crates/core/src/chain.rs | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/crates/core/src/chain.rs b/crates/core/src/chain.rs index df8113ba46..3b3c1c5eed 100644 --- a/crates/core/src/chain.rs +++ b/crates/core/src/chain.rs @@ -2,6 +2,7 @@ // TODO move BlockHash and BlockHeight here from the storage types use std::fmt; +use std::io::{self, Read}; use std::num::NonZeroU64; use std::str::FromStr; @@ -32,13 +33,28 @@ pub const CHAIN_ID_PREFIX_SEP: char = '.'; Hash, Debug, BorshSerialize, - BorshDeserialize, BorshDeserializer, )] +#[repr(transparent)] pub struct ProposalBytes { inner: NonZeroU64, } +impl BorshDeserialize for ProposalBytes { + fn deserialize_reader(reader: &mut R) -> std::io::Result { + let value: u64 = BorshDeserialize::deserialize_reader(reader)?; + Self::new(value).ok_or_else(|| { + io::Error::new( + io::ErrorKind::InvalidInput, + format!( + "ProposalBytes value must be in the range 1 - {}", + Self::RAW_MAX.get() + ), + ) + }) + } +} + impl Serialize for ProposalBytes { fn serialize(&self, s: S) -> Result where From 3f581ab53211976a537bc0da972f8505d1e1f022 Mon Sep 17 00:00:00 2001 From: Tiago Carvalho Date: Fri, 10 May 2024 16:07:37 +0100 Subject: [PATCH 2/3] Avoid using hardcoded values when configuring CometBFT --- crates/apps/src/lib/node/ledger/tendermint_node.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/crates/apps/src/lib/node/ledger/tendermint_node.rs b/crates/apps/src/lib/node/ledger/tendermint_node.rs index 7c70de5958..c617d6efe3 100644 --- a/crates/apps/src/lib/node/ledger/tendermint_node.rs +++ b/crates/apps/src/lib/node/ledger/tendermint_node.rs @@ -4,7 +4,7 @@ use std::process::Stdio; use std::str::FromStr; use borsh_ext::BorshSerializeExt; -use namada::core::chain::ChainId; +use namada::core::chain::{ChainId, ProposalBytes}; use namada::core::key::*; use namada::core::storage::BlockHeight; use namada::core::time::DateTimeUtc; @@ -418,10 +418,7 @@ async fn update_tendermint_config( config.mempool.max_tx_bytes = 1024 * 1024; // Hold 50x the max amount of txs in a block - // - // 6 MiB is the default Namada max proposal size governance - // parameter -> 50 * 6 MiB - config.mempool.max_txs_bytes = 50 * 6 * 1024 * 1024; + config.mempool.max_txs_bytes = 50 * ProposalBytes::MAX.get(); // Hold up to 4k txs in the mempool config.mempool.size = 4000; @@ -475,12 +472,13 @@ async fn write_tm_genesis( .try_into() .expect("Failed to convert initial genesis height"); } + const EVIDENCE_AND_PROTOBUF_OVERHEAD: u64 = 10 * 1024 * 1024; let size = block::Size { // maximum size of a serialized Tendermint block. // on Namada, we have a hard-cap of 16 MiB (6 MiB max // txs in a block + 10 MiB reserved for evidence data, // block headers and protobuf serialization overhead) - max_bytes: 16 * 1024 * 1024, + max_bytes: EVIDENCE_AND_PROTOBUF_OVERHEAD + ProposalBytes::MAX.get(), // gas is metered app-side, so we disable it // at the Tendermint level max_gas: -1, From d8f959cc97e48930fb685343b6fad8b0b3b2f9b5 Mon Sep 17 00:00:00 2001 From: Tiago Carvalho Date: Fri, 10 May 2024 16:09:07 +0100 Subject: [PATCH 3/3] Changelog for #3220 --- .../unreleased/bug-fixes/3220-max-proposal-bytes-validation.md | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/unreleased/bug-fixes/3220-max-proposal-bytes-validation.md diff --git a/.changelog/unreleased/bug-fixes/3220-max-proposal-bytes-validation.md b/.changelog/unreleased/bug-fixes/3220-max-proposal-bytes-validation.md new file mode 100644 index 0000000000..92ef4838f8 --- /dev/null +++ b/.changelog/unreleased/bug-fixes/3220-max-proposal-bytes-validation.md @@ -0,0 +1,3 @@ +- Harden the implementation of `BorshDeserialize` on `ProposalBytes`. + Moreover, avoid using magic numbers when configuring CometBFT. + ([\#3220](https://github.com/anoma/namada/pull/3220)) \ No newline at end of file