From c910b5dd211d076a3f2971974ba04da00172c0fa Mon Sep 17 00:00:00 2001 From: brentstone Date: Thu, 7 Mar 2024 20:26:21 +0100 Subject: [PATCH 1/7] check metadata length in VP --- wasm/wasm_source/src/vp_user.rs | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/wasm/wasm_source/src/vp_user.rs b/wasm/wasm_source/src/vp_user.rs index 655d73ccd6..000a8d0961 100644 --- a/wasm/wasm_source/src/vp_user.rs +++ b/wasm/wasm_source/src/vp_user.rs @@ -188,9 +188,14 @@ fn validate_pos_changes( // Metadata changes must be signed by the validator whose // metadata is manipulated let is_valid_metadata_change = || { - let metadata = is_validator_metadata_key(key); - match metadata { - Some(address) => address == owner && **valid_sig, + let validator = is_validator_metadata_key(key); + let metadata = ctx + .post() + .read::(key)? + .expect("Metadata should exist in post state"); + let valid_len = metadata.len() <= 500; // FIXME: hard-code for now + match validator { + Some(address) => valid_len && address == owner && **valid_sig, None => false, } }; From 233f283139384ad1af04c61f1a95c33d1be18305 Mon Sep 17 00:00:00 2001 From: brentstone Date: Thu, 7 Mar 2024 20:26:37 +0100 Subject: [PATCH 2/7] check metadata length in client --- crates/sdk/src/error.rs | 3 +++ crates/sdk/src/tx.rs | 56 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+) diff --git a/crates/sdk/src/error.rs b/crates/sdk/src/error.rs index 23cea67723..0aec671536 100644 --- a/crates/sdk/src/error.rs +++ b/crates/sdk/src/error.rs @@ -309,6 +309,9 @@ pub enum TxSubmitError { /// An empty string was provided as a new email #[error("An empty string cannot be provided as a new email")] InvalidEmail, + /// The metadata string is too long + #[error("The provided metadata string is too long")] + MetadataTooLong, /// The consensus key is not Ed25519 #[error("The consensus key must be an ed25519 key")] ConsensusKeyNotEd25519, diff --git a/crates/sdk/src/tx.rs b/crates/sdk/src/tx.rs index 15b69575d0..d5a02971e4 100644 --- a/crates/sdk/src/tx.rs +++ b/crates/sdk/src/tx.rs @@ -733,6 +733,62 @@ pub async fn build_validator_metadata_change( ); return Err(Error::from(TxSubmitError::InvalidEmail)); } + // Check that the email is within 500 characters + if email.len() > 500 { + edisplay_line!( + context.io(), + "Email provided is too long, must be within 500 characters" + ); + if !tx_args.force { + return Err(Error::from(TxSubmitError::MetadataTooLong)); + } + } + } + + // Check that any new metadata provided is within 500 characters + if let Some(description) = description.as_ref() { + if description.len() > 500 { + edisplay_line!( + context.io(), + "Description provided is too long, must be within 500 characters" + ); + if !tx_args.force { + return Err(Error::from(TxSubmitError::MetadataTooLong)); + } + } + } + if let Some(website) = website.as_ref() { + if website.len() > 500 { + edisplay_line!( + context.io(), + "Website provided is too long, must be within 500 characters" + ); + if !tx_args.force { + return Err(Error::from(TxSubmitError::MetadataTooLong)); + } + } + } + if let Some(discord_handle) = discord_handle.as_ref() { + if discord_handle.len() > 500 { + edisplay_line!( + context.io(), + "Discord handle provided is too long, must be within 500 characters" + ); + if !tx_args.force { + return Err(Error::from(TxSubmitError::MetadataTooLong)); + } + } + } + if let Some(avatar) = avatar.as_ref() { + if avatar.len() > 500 { + edisplay_line!( + context.io(), + "Avatar provided is too long, must be within 500 characters" + ); + if !tx_args.force { + return Err(Error::from(TxSubmitError::MetadataTooLong)); + } + } } // If there's a new commission rate, it must be valid From 97c7f9ece004e16d54036b510d82cd3d59fe2efe Mon Sep 17 00:00:00 2001 From: brentstone Date: Fri, 8 Mar 2024 09:51:34 +0100 Subject: [PATCH 3/7] refactor --- crates/proof_of_stake/src/parameters.rs | 3 +++ crates/sdk/src/tx.rs | 34 +++++++++++++++---------- wasm/wasm_source/src/vp_user.rs | 10 +++++--- 3 files changed, 30 insertions(+), 17 deletions(-) diff --git a/crates/proof_of_stake/src/parameters.rs b/crates/proof_of_stake/src/parameters.rs index 30d60b2b3e..f165c81fb1 100644 --- a/crates/proof_of_stake/src/parameters.rs +++ b/crates/proof_of_stake/src/parameters.rs @@ -132,6 +132,9 @@ pub enum ValidationError { UnbondingLenTooShort(u64, u64), } +/// The maximum string length of any validator metadata +pub const MAX_VALIDATOR_METADATA_LEN: u64 = 500; + /// The number of fundamental units per whole token of the native staking token pub const TOKENS_PER_NAM: u64 = 1_000_000; diff --git a/crates/sdk/src/tx.rs b/crates/sdk/src/tx.rs index d5a02971e4..aab6b5889c 100644 --- a/crates/sdk/src/tx.rs +++ b/crates/sdk/src/tx.rs @@ -44,7 +44,9 @@ use namada_governance::storage::proposal::{ }; use namada_governance::storage::vote::ProposalVote; use namada_ibc::storage::channel_key; -use namada_proof_of_stake::parameters::PosParams; +use namada_proof_of_stake::parameters::{ + PosParams, MAX_VALIDATOR_METADATA_LEN, +}; use namada_proof_of_stake::types::{CommissionPair, ValidatorState}; use namada_token::storage_key::balance_key; use namada_token::DenominatedAmount; @@ -733,11 +735,12 @@ pub async fn build_validator_metadata_change( ); return Err(Error::from(TxSubmitError::InvalidEmail)); } - // Check that the email is within 500 characters - if email.len() > 500 { + // Check that the email is within MAX_VALIDATOR_METADATA_LEN characters + if email.len() as u64 > MAX_VALIDATOR_METADATA_LEN { edisplay_line!( context.io(), - "Email provided is too long, must be within 500 characters" + "Email provided is too long, must be within \ + {MAX_VALIDATOR_METADATA_LEN} characters" ); if !tx_args.force { return Err(Error::from(TxSubmitError::MetadataTooLong)); @@ -745,12 +748,14 @@ pub async fn build_validator_metadata_change( } } - // Check that any new metadata provided is within 500 characters + // Check that any new metadata provided is within MAX_VALIDATOR_METADATA_LEN + // characters if let Some(description) = description.as_ref() { - if description.len() > 500 { + if description.len() as u64 > MAX_VALIDATOR_METADATA_LEN { edisplay_line!( context.io(), - "Description provided is too long, must be within 500 characters" + "Description provided is too long, must be within \ + {MAX_VALIDATOR_METADATA_LEN} characters" ); if !tx_args.force { return Err(Error::from(TxSubmitError::MetadataTooLong)); @@ -758,10 +763,11 @@ pub async fn build_validator_metadata_change( } } if let Some(website) = website.as_ref() { - if website.len() > 500 { + if website.len() as u64 > MAX_VALIDATOR_METADATA_LEN { edisplay_line!( context.io(), - "Website provided is too long, must be within 500 characters" + "Website provided is too long, must be within \ + {MAX_VALIDATOR_METADATA_LEN} characters" ); if !tx_args.force { return Err(Error::from(TxSubmitError::MetadataTooLong)); @@ -769,10 +775,11 @@ pub async fn build_validator_metadata_change( } } if let Some(discord_handle) = discord_handle.as_ref() { - if discord_handle.len() > 500 { + if discord_handle.len() as u64 > MAX_VALIDATOR_METADATA_LEN { edisplay_line!( context.io(), - "Discord handle provided is too long, must be within 500 characters" + "Discord handle provided is too long, must be within \ + {MAX_VALIDATOR_METADATA_LEN} characters" ); if !tx_args.force { return Err(Error::from(TxSubmitError::MetadataTooLong)); @@ -780,10 +787,11 @@ pub async fn build_validator_metadata_change( } } if let Some(avatar) = avatar.as_ref() { - if avatar.len() > 500 { + if avatar.len() as u64 > MAX_VALIDATOR_METADATA_LEN { edisplay_line!( context.io(), - "Avatar provided is too long, must be within 500 characters" + "Avatar provided is too long, must be within \ + {MAX_VALIDATOR_METADATA_LEN} characters" ); if !tx_args.force { return Err(Error::from(TxSubmitError::MetadataTooLong)); diff --git a/wasm/wasm_source/src/vp_user.rs b/wasm/wasm_source/src/vp_user.rs index 000a8d0961..87c461fc82 100644 --- a/wasm/wasm_source/src/vp_user.rs +++ b/wasm/wasm_source/src/vp_user.rs @@ -15,6 +15,7 @@ use core::ops::Deref; use namada_vp_prelude::*; use once_cell::unsync::Lazy; +use proof_of_stake::parameters::MAX_VALIDATOR_METADATA_LEN; use proof_of_stake::storage::{read_pos_params, validator_state_handle}; use proof_of_stake::storage_key::{ is_below_capacity_validator_set_key, is_bond_epoched_meta_key, is_bond_key, @@ -193,11 +194,12 @@ fn validate_pos_changes( .post() .read::(key)? .expect("Metadata should exist in post state"); - let valid_len = metadata.len() <= 500; // FIXME: hard-code for now - match validator { + let valid_len = (metadata.len() as u64) <= MAX_VALIDATOR_METADATA_LEN; // FIXME: hard-code for now + let is_valid = match validator { Some(address) => valid_len && address == owner && **valid_sig, None => false, - } + }; + VpResult::Ok(is_valid) }; // Changes in validator state @@ -329,7 +331,7 @@ fn validate_pos_changes( || is_valid_reward_claim() || is_valid_redelegation() || is_valid_commission_rate_change() - || is_valid_metadata_change() + || is_valid_metadata_change()? || is_valid_become_validator() || **valid_sig) } From 8ff8c8f84a0479de3a1ee76d45c3afc70567706f Mon Sep 17 00:00:00 2001 From: brentstone Date: Fri, 8 Mar 2024 09:53:14 +0100 Subject: [PATCH 4/7] changelog: add #2845 --- .changelog/unreleased/improvements/2845-limit-metadata-size.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 .changelog/unreleased/improvements/2845-limit-metadata-size.md diff --git a/.changelog/unreleased/improvements/2845-limit-metadata-size.md b/.changelog/unreleased/improvements/2845-limit-metadata-size.md new file mode 100644 index 0000000000..ff436b85ab --- /dev/null +++ b/.changelog/unreleased/improvements/2845-limit-metadata-size.md @@ -0,0 +1,2 @@ +- Limit the character length of the validator metadata strings. + ([\#2845](https://github.com/anoma/namada/pull/2845)) \ No newline at end of file From 70e6b879fee8e65161f173402b977428aa363952 Mon Sep 17 00:00:00 2001 From: brentstone Date: Fri, 8 Mar 2024 15:09:20 +0100 Subject: [PATCH 5/7] genesis metadata validation --- .../src/lib/config/genesis/transactions.rs | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/crates/apps/src/lib/config/genesis/transactions.rs b/crates/apps/src/lib/config/genesis/transactions.rs index a1b4213b38..446d172f1a 100644 --- a/crates/apps/src/lib/config/genesis/transactions.rs +++ b/crates/apps/src/lib/config/genesis/transactions.rs @@ -23,6 +23,7 @@ use namada::core::token; use namada::core::token::{DenominatedAmount, NATIVE_MAX_DECIMAL_PLACES}; use namada::ledger::pos::common::PublicKey; use namada::ledger::pos::types::ValidatorMetaData; +use namada::proof_of_stake::parameters::MAX_VALIDATOR_METADATA_LEN; use namada::tx::data::{pos, Fee, TxType}; use namada::tx::{ verify_standalone_sig, Code, Commitment, Data, Section, SignatureIndex, Tx, @@ -1356,6 +1357,54 @@ pub fn validate_validator_account( ); } + // Check that the validator metadata is not too large + let metadata = &signed_tx.data.metadata; + if metadata.email.len() as u64 > MAX_VALIDATOR_METADATA_LEN { + panic!( + "The email metadata of the validator with address {} is too long, \ + must be within {MAX_VALIDATOR_METADATA_LEN} characters", + signed_tx.data.address + ); + } + if let Some(description) = metadata.description.as_ref() { + if description.len() as u64 > MAX_VALIDATOR_METADATA_LEN { + panic!( + "The description metadata of the validator with address {} is \ + too long, must be within {MAX_VALIDATOR_METADATA_LEN} \ + characters", + signed_tx.data.address + ); + } + } + if let Some(website) = metadata.website.as_ref() { + if website.len() as u64 > MAX_VALIDATOR_METADATA_LEN { + panic!( + "The website metadata of the validator with address {} is too \ + long, must be within {MAX_VALIDATOR_METADATA_LEN} characters", + signed_tx.data.address + ); + } + } + if let Some(discord_handle) = metadata.discord_handle.as_ref() { + if discord_handle.len() as u64 > MAX_VALIDATOR_METADATA_LEN { + panic!( + "The discord handle metadata of the validator with address {} \ + is too long, must be within {MAX_VALIDATOR_METADATA_LEN} \ + characters", + signed_tx.data.address + ); + } + } + if let Some(avatar) = metadata.avatar.as_ref() { + if avatar.len() as u64 > MAX_VALIDATOR_METADATA_LEN { + panic!( + "The avatar metadata of the validator with address {} is too \ + long, must be within {MAX_VALIDATOR_METADATA_LEN} characters", + signed_tx.data.address + ); + } + } + // Check signature let mut is_valid = { let maybe_threshold = { From 39a62fc59f2a17e3dd34a5a48be75f32d1574a7d Mon Sep 17 00:00:00 2001 From: brentstone Date: Fri, 8 Mar 2024 17:20:28 +0100 Subject: [PATCH 6/7] fix bug for deleting metadata --- wasm/wasm_source/src/vp_user.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/wasm/wasm_source/src/vp_user.rs b/wasm/wasm_source/src/vp_user.rs index 87c461fc82..fcc69aa654 100644 --- a/wasm/wasm_source/src/vp_user.rs +++ b/wasm/wasm_source/src/vp_user.rs @@ -190,11 +190,12 @@ fn validate_pos_changes( // metadata is manipulated let is_valid_metadata_change = || { let validator = is_validator_metadata_key(key); - let metadata = ctx - .post() - .read::(key)? - .expect("Metadata should exist in post state"); - let valid_len = (metadata.len() as u64) <= MAX_VALIDATOR_METADATA_LEN; // FIXME: hard-code for now + let metadata = ctx.post().read::(key)?; + let valid_len = if let Some(metadata) = metadata { + (metadata.len() as u64) <= MAX_VALIDATOR_METADATA_LEN + } else { + true + }; let is_valid = match validator { Some(address) => valid_len && address == owner && **valid_sig, None => false, From fa33c45535ebd7e52dd5b4126e90f6d0a978354c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Zemanovi=C4=8D?= Date: Mon, 1 Apr 2024 17:23:32 +0100 Subject: [PATCH 7/7] wasm/vp_user: only read metadata if it's changed --- wasm/wasm_source/src/vp_user.rs | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/wasm/wasm_source/src/vp_user.rs b/wasm/wasm_source/src/vp_user.rs index fcc69aa654..c5c6135dd7 100644 --- a/wasm/wasm_source/src/vp_user.rs +++ b/wasm/wasm_source/src/vp_user.rs @@ -189,15 +189,16 @@ fn validate_pos_changes( // Metadata changes must be signed by the validator whose // metadata is manipulated let is_valid_metadata_change = || { - let validator = is_validator_metadata_key(key); - let metadata = ctx.post().read::(key)?; - let valid_len = if let Some(metadata) = metadata { - (metadata.len() as u64) <= MAX_VALIDATOR_METADATA_LEN - } else { - true - }; - let is_valid = match validator { - Some(address) => valid_len && address == owner && **valid_sig, + let is_valid = match is_validator_metadata_key(key) { + Some(address) => { + let metadata = ctx.post().read::(key)?; + let valid_len = if let Some(metadata) = metadata { + (metadata.len() as u64) <= MAX_VALIDATOR_METADATA_LEN + } else { + true + }; + valid_len && address == owner && **valid_sig + } None => false, }; VpResult::Ok(is_valid)