Rebased: Tighten security around potential P2P issues

.changelog/unreleased/improvements/2131-validate-tx-bytes-len.md

@@ -0,0 +1,2 @@
+- Tighten security around potential P2P issues
+  ([\#2131](https://github.com/anoma/namada/pull/2131))

apps/src/lib/config/genesis.rs

@@ -153,7 +153,7 @@ pub struct ImplicitAccount {
     BorshDeserialize,
 )]
 pub struct Parameters {
-    // Max payload size, in bytes, for a tx batch proposal.
+    /// Max payload size, in bytes, for a tx batch proposal.
     pub max_proposal_bytes: ProposalBytes,
     /// Max block gas
     pub max_block_gas: u64,

apps/src/lib/config/genesis/chain.rs

@@ -262,6 +262,7 @@ impl Finalized {
             fee_unshielding_descriptions_limit,
             max_block_gas,
             minimum_gas_price,
+            max_tx_bytes,
             ..
         } = self.parameters.parameters.clone();
 
@@ -291,6 +292,7 @@ impl Finalized {
         let pos_inflation_amount = 0;
 
         namada::ledger::parameters::Parameters {
+            max_tx_bytes,
             epoch_duration,
             max_expected_time_per_block,
             vp_whitelist,

apps/src/lib/config/genesis/templates.rs

@@ -242,6 +242,9 @@ pub struct Parameters<T: TemplateValidation> {
     Eq,
 )]
 pub struct ChainParams<T: TemplateValidation> {
+    /// Max payload size, in bytes, for a tx decided through
+    /// the consensus protocol.
+    pub max_tx_bytes: u32,
     /// Name of the native token - this must one of the tokens from
     /// `tokens.toml` file
     pub native_token: Alias,
@@ -296,6 +299,7 @@ impl ChainParams<Unvalidated> {
         tokens: &Tokens,
     ) -> eyre::Result<ChainParams<Validated>> {
         let ChainParams {
+            max_tx_bytes,
             native_token,
             min_num_of_blocks,
             max_expected_time_per_block,
@@ -342,6 +346,7 @@ impl ChainParams<Unvalidated> {
         }
 
         Ok(ChainParams {
+            max_tx_bytes,
             native_token,
             min_num_of_blocks,
             max_expected_time_per_block,

apps/src/lib/node/ledger/shell/mod.rs

@@ -48,6 +48,7 @@ use namada::ledger::storage::{
     DBIter, Sha256Hasher, Storage, StorageHasher, TempWlStorage, WlStorage, DB,
     EPOCH_SWITCH_BLOCKS_DELAY,
 };
+use namada::ledger::storage_api::tx::validate_tx_bytes;
 use namada::ledger::storage_api::{self, StorageRead};
 use namada::ledger::{parameters, pos, protocol};
 use namada::proof_of_stake::{self, process_slashes, read_pos_params, slash};
@@ -154,6 +155,7 @@ pub enum ErrorCodes {
     TxGasLimit = 11,
     FeeError = 12,
     InvalidVoteExtension = 13,
+    TooLarge = 14,
 }
 
 impl ErrorCodes {
@@ -167,7 +169,8 @@ impl ErrorCodes {
             Ok | WasmRuntimeError => true,
             InvalidTx | InvalidSig | InvalidOrder | ExtraTxs
             | Undecryptable | AllocationError | ReplayTx | InvalidChainId
-            | ExpiredTx | TxGasLimit | FeeError | InvalidVoteExtension => false,
+            | ExpiredTx | TxGasLimit | FeeError | InvalidVoteExtension
+            | TooLarge => false,
         }
     }
 }
@@ -1072,6 +1075,18 @@ where
         const VALID_MSG: &str = "Mempool validation passed";
         const INVALID_MSG: &str = "Mempool validation failed";
 
+        // check tx bytes
+        //
+        // NB: always keep this as the first tx check,
+        // as it is a pretty cheap one
+        if !validate_tx_bytes(&self.wl_storage, tx_bytes.len())
+            .expect("Failed to get max tx bytes param from storage")
+        {
+            response.code = ErrorCodes::TooLarge.into();
+            response.log = format!("{INVALID_MSG}: Tx too large");
+            return response;
+        }
+
         // Tx format check
         let tx = match Tx::try_from(tx_bytes).map_err(Error::TxDecoding) {
             Ok(t) => t,
@@ -2089,6 +2104,7 @@ mod test_utils {
             .new_epoch(BlockHeight(1));
         // initialize parameter storage
         let params = Parameters {
+            max_tx_bytes: 1024 * 1024,
             epoch_duration: EpochDuration {
                 min_num_of_blocks: 1,
                 min_duration: DurationSecs(3600),
@@ -2941,4 +2957,57 @@ mod shell_tests {
         );
         assert_eq!(result.code, ErrorCodes::FeeError.into());
     }
+
+    /// Test max tx bytes parameter in CheckTx
+    #[test]
+    fn test_max_tx_bytes_check_tx() {
+        let (shell, _recv, _, _) = test_utils::setup();
+
+        let max_tx_bytes: u32 = {
+            let key = parameters::storage::get_max_tx_bytes_key();
+            shell
+                .wl_storage
+                .read(&key)
+                .expect("Failed to read from storage")
+                .expect("Max tx bytes should have been written to storage")
+        };
+
+        let new_tx = |size: u32| {
+            let keypair = super::test_utils::gen_keypair();
+            let mut wrapper =
+                Tx::from_type(TxType::Wrapper(Box::new(WrapperTx::new(
+                    Fee {
+                        amount_per_gas_unit: 100.into(),
+                        token: shell.wl_storage.storage.native_token.clone(),
+                    },
+                    keypair.ref_to(),
+                    Epoch(0),
+                    GAS_LIMIT_MULTIPLIER.into(),
+                    None,
+                ))));
+            wrapper.header.chain_id = shell.chain_id.clone();
+            wrapper.set_code(Code::new("wasm_code".as_bytes().to_owned()));
+            wrapper.set_data(Data::new(vec![0; size as usize]));
+            wrapper.add_section(Section::Signature(Signature::new(
+                wrapper.sechashes(),
+                [(0, keypair)].into_iter().collect(),
+                None,
+            )));
+            wrapper
+        };
+
+        // test a small tx
+        let result = shell.mempool_validate(
+            new_tx(50).to_bytes().as_ref(),
+            MempoolTxType::NewTransaction,
+        );
+        assert!(result.code != ErrorCodes::TooLarge.into());
+
+        // max tx bytes + 1, on the other hand, is not
+        let result = shell.mempool_validate(
+            new_tx(max_tx_bytes + 1).to_bytes().as_ref(),
+            MempoolTxType::NewTransaction,
+        );
+        assert_eq!(result.code, ErrorCodes::TooLarge.into());
+    }
 }

apps/src/lib/node/ledger/shell/process_proposal.rs

@@ -7,6 +7,7 @@ use namada::core::ledger::storage::WlStorage;
 use namada::ledger::pos::PosQueries;
 use namada::ledger::protocol::get_fee_unshielding_transaction;
 use namada::ledger::storage::TempWlStorage;
+use namada::ledger::storage_api::tx::validate_tx_bytes;
 use namada::proof_of_stake::find_validator_by_raw_hash;
 use namada::types::internal::TxInQueue;
 use namada::types::transaction::protocol::{
@@ -266,6 +267,19 @@ where
     where
         CA: 'static + WasmCacheAccess + Sync,
     {
+        // check tx bytes
+        //
+        // NB: always keep this as the first tx check,
+        // as it is a pretty cheap one
+        if !validate_tx_bytes(&self.wl_storage, tx_bytes.len())
+            .expect("Failed to get max tx bytes param from storage")
+        {
+            return TxResult {
+                code: ErrorCodes::TooLarge.into(),
+                info: "Tx too large".into(),
+            };
+        }
+
         // try to allocate space for this tx
         if let Err(e) = metadata.txs_bin.try_dump(tx_bytes) {
             return TxResult {
@@ -2039,4 +2053,69 @@ mod test_process_proposal {
             );
         }
     }
+
+    /// Test max tx bytes parameter in ProcessProposal
+    #[test]
+    fn test_max_tx_bytes_process_proposal() {
+        use namada::ledger::parameters::storage::get_max_tx_bytes_key;
+        let (shell, _recv, _, _) = test_utils::setup_at_height(3u64);
+
+        let max_tx_bytes: u32 = {
+            let key = get_max_tx_bytes_key();
+            shell
+                .wl_storage
+                .read(&key)
+                .expect("Failed to read from storage")
+                .expect("Max tx bytes should have been written to storage")
+        };
+
+        let new_tx = |size: u32| {
+            let keypair = super::test_utils::gen_keypair();
+            let mut wrapper =
+                Tx::from_type(TxType::Wrapper(Box::new(WrapperTx::new(
+                    Fee {
+                        amount_per_gas_unit: 100.into(),
+                        token: shell.wl_storage.storage.native_token.clone(),
+                    },
+                    keypair.ref_to(),
+                    Epoch(0),
+                    GAS_LIMIT_MULTIPLIER.into(),
+                    None,
+                ))));
+            wrapper.header.chain_id = shell.chain_id.clone();
+            wrapper.set_code(Code::new("wasm_code".as_bytes().to_owned()));
+            wrapper.set_data(Data::new(vec![0; size as usize]));
+            wrapper.add_section(Section::Signature(Signature::new(
+                wrapper.sechashes(),
+                [(0, keypair)].into_iter().collect(),
+                None,
+            )));
+            wrapper
+        };
+
+        let request = ProcessProposal {
+            txs: vec![new_tx(max_tx_bytes + 1).to_bytes()],
+        };
+        match shell.process_proposal(request) {
+            Ok(_) => panic!("Test failed"),
+            Err(TestError::RejectProposal(response)) => {
+                assert_eq!(
+                    response[0].result.code,
+                    u32::from(ErrorCodes::TooLarge)
+                );
+            }
+        }
+
+        let request = ProcessProposal {
+            txs: vec![new_tx(0).to_bytes()],
+        };
+        match shell.process_proposal(request) {
+            Ok(_) => panic!("Test failed"),
+            Err(TestError::RejectProposal(response)) => {
+                assert!(
+                    response[0].result.code != u32::from(ErrorCodes::TooLarge)
+                );
+            }
+        }
+    }
 }

apps/src/lib/node/ledger/storage/mod.rs

@@ -146,6 +146,7 @@ mod tests {
         let mut wl_storage = WlStorage::new(WriteLog::default(), storage);
         // initialize parameter storage
         let params = Parameters {
+            max_tx_bytes: 1024 * 1024,
             epoch_duration: EpochDuration {
                 min_num_of_blocks: 1,
                 min_duration: DurationSecs(3600),

apps/src/lib/node/ledger/tendermint_node.rs

@@ -347,22 +347,45 @@ pub fn id_from_pk(pk: &common::PublicKey) -> TendermintNodeId {
 
 async fn update_tendermint_config(
     home_dir: impl AsRef<Path>,
-    config: TendermintConfig,
+    mut config: TendermintConfig,
 ) -> Result<()> {
     let home_dir = home_dir.as_ref();
     let path = home_dir.join("config").join("config.toml");
-    let mut config = config.clone();
 
     config.moniker =
         Moniker::from_str(&format!("{}-{}", config.moniker, namada_version()))
             .expect("Invalid moniker");
 
     config.consensus.create_empty_blocks = true;
 
-    // We set this to true as we don't want any invalid tx be re-applied. This
-    // also implies that it's not possible for an invalid tx to become valid
-    // again in the future.
-    config.mempool.keep_invalid_txs_in_cache = false;
+    // mempool config
+    // https://forum.cosmos.network/t/our-understanding-of-the-cosmos-hub-mempool-issues/12040
+    {
+        // We set this to true as we don't want any invalid tx be re-applied.
+        // This also implies that it's not possible for an invalid tx to
+        // become valid again in the future.
+        config.mempool.keep_invalid_txs_in_cache = false;
+
+        // Drop txs from the mempool that are larger than 1 MiB
+        //
+        // The application (Namada) can assign arbitrary max tx sizes,
+        // which are subject to consensus. Either way, nodes are able to
+        // configure their local mempool however they please.
+        //
+        // 1 MiB is a reasonable value that allows governance proposal txs
+        // containing wasm code to be proposed by a leading validator
+        // during some round's start
+        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;
+
+        // Hold up to 4k txs in the mempool
+        config.mempool.size = 4000;
+    }
 
     // Bumped from the default `1_000_000`, because some WASMs can be
     // quite large
@@ -408,11 +431,11 @@ async fn write_tm_genesis(
         .try_into()
         .expect("Couldn't convert DateTimeUtc to Tendermint Time");
     let size = block::Size {
-        // maximum size of a serialized Tendermint block
-        // cannot go over 100 MiB
-        max_bytes: (100 << 20) - 1, /* unsure if we are dealing with an open
-                                     * range, so it's better to subtract one,
-                                     * here */
+        // 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,
         // gas is metered app-side, so we disable it
         // at the Tendermint level
         max_gas: -1,

core/src/ledger/parameters/mod.rs

@@ -35,6 +35,8 @@ pub const ADDRESS: Address = Address::Internal(InternalAddress::Parameters);
     BorshSchema,
 )]
 pub struct Parameters {
+    /// Max payload size, in bytes, for a mempool tx.
+    pub max_tx_bytes: u32,
     /// Epoch duration (read only)
     pub epoch_duration: EpochDuration,
     /// Maximum expected time per block (read only)
@@ -117,6 +119,7 @@ impl Parameters {
         S: StorageRead + StorageWrite,
     {
         let Self {
+            max_tx_bytes,
             epoch_duration,
             max_expected_time_per_block,
             max_proposal_bytes,
@@ -135,6 +138,10 @@ impl Parameters {
             fee_unshielding_descriptions_limit,
         } = self;
 
+        // write max tx bytes parameter
+        let max_tx_bytes_key = storage::get_max_tx_bytes_key();
+        storage.write(&max_tx_bytes_key, max_tx_bytes)?;
+
         // write max proposal bytes parameter
         let max_proposal_bytes_key = storage::get_max_proposal_bytes_key();
         storage.write(&max_proposal_bytes_key, max_proposal_bytes)?;
@@ -542,7 +549,15 @@ where
         .ok_or(ReadError::ParametersMissing)
         .into_storage_result()?;
 
+    // read max tx bytes
+    let max_tx_bytes_key = storage::get_max_tx_bytes_key();
+    let value = storage.read(&max_tx_bytes_key)?;
+    let max_tx_bytes = value
+        .ok_or(ReadError::ParametersMissing)
+        .into_storage_result()?;
+
     Ok(Parameters {
+        max_tx_bytes,
         epoch_duration,
         max_expected_time_per_block,
         max_proposal_bytes,