diff --git a/.changelog/unreleased/bug-fixes/3960-memo-validation-in-vp.md b/.changelog/unreleased/bug-fixes/3960-memo-validation-in-vp.md new file mode 100644 index 00000000000..3c47481c45e --- /dev/null +++ b/.changelog/unreleased/bug-fixes/3960-memo-validation-in-vp.md @@ -0,0 +1,2 @@ +- Added validation of the transaction's memo in the validity predicates to catch + any possible tamperings. ([\#3960](https://github.com/anoma/namada/pull/3960)) \ No newline at end of file diff --git a/crates/core/src/hash.rs b/crates/core/src/hash.rs index bfddb087bde..b5c42777ee7 100644 --- a/crates/core/src/hash.rs +++ b/crates/core/src/hash.rs @@ -175,7 +175,7 @@ impl Hash { } /// Return zeros - pub fn zero() -> Self { + pub const fn zero() -> Self { Self([0u8; HASH_LENGTH]) } diff --git a/crates/tests/src/integration/masp.rs b/crates/tests/src/integration/masp.rs index f88f08ecdd7..132fb572f4b 100644 --- a/crates/tests/src/integration/masp.rs +++ b/crates/tests/src/integration/masp.rs @@ -5166,13 +5166,7 @@ fn identical_output_descriptions() -> Result<()> { let tx: namada_sdk::tx::Tx = serde_json::from_slice(&tx_bytes).unwrap(); // Inject some randomness in the cloned tx to change the hash let mut tx_clone = tx.clone(); - let mut cmt = tx_clone.header.batch.first().unwrap().to_owned(); - let random_hash: Vec<_> = (0..namada_sdk::hash::HASH_LENGTH) - .map(|_| rand::random::()) - .collect(); - cmt.memo_hash = namada_sdk::hash::Hash(random_hash.try_into().unwrap()); - tx_clone.header.batch.clear(); - tx_clone.header.batch.insert(cmt); + tx_clone.add_memo(&[1, 2, 3]); let signing_data = SigningTxData { owner: None, diff --git a/crates/vp_prelude/src/lib.rs b/crates/vp_prelude/src/lib.rs index cc698fdbcd8..7f6057c06c9 100644 --- a/crates/vp_prelude/src/lib.rs +++ b/crates/vp_prelude/src/lib.rs @@ -52,7 +52,7 @@ use namada_vm_env::vp::*; use namada_vm_env::{read_from_buffer, read_key_val_bytes_from_buffer}; pub use namada_vp_env::{collection_validation, VpEnv}; pub use sha2::{Digest, Sha256, Sha384, Sha512}; -use tx::BatchedTxRef; +use tx::{BatchedTxRef, TxCommitments}; pub use { namada_account as account, namada_parameters as parameters, namada_proof_of_stake as proof_of_stake, namada_token as token, @@ -131,9 +131,22 @@ impl VerifySigGadget { &mut self, ctx: &Ctx, tx_data: &Tx, + cmt: &TxCommitments, owner: &Address, ) -> VpResult { if !self.has_validated_sig { + // First check that the memo section of this inner tx has not been + // tampered with + if cmt.memo_hash != namada_core::hash::Hash::zero() { + tx_data.get_section(&cmt.memo_hash).ok_or_else(|| { + VpError::Erased(format!( + "Memo section with hash {} is missing", + cmt.memo_hash + )) + })?; + } + + // Then check the signature verify_signatures(ctx, tx_data, owner)?; self.has_validated_sig = true; } @@ -149,10 +162,11 @@ impl VerifySigGadget { predicate: F, ctx: &Ctx, tx_data: &Tx, + cmt: &TxCommitments, owner: &Address, ) -> VpResult { if predicate() { - self.verify_signatures(ctx, tx_data, owner)?; + self.verify_signatures(ctx, tx_data, cmt, owner)?; } Ok(()) } diff --git a/wasm/vp_implicit/src/lib.rs b/wasm/vp_implicit/src/lib.rs index cffc14fe06c..e15f9cb02fa 100644 --- a/wasm/vp_implicit/src/lib.rs +++ b/wasm/vp_implicit/src/lib.rs @@ -69,6 +69,7 @@ fn validate_tx( || source == addr, ctx, &tx, + cmt, &addr, )?, PosAction::Bond(Bond { @@ -85,6 +86,7 @@ fn validate_tx( || source == addr, ctx, &tx, + cmt, &addr, )? } @@ -100,10 +102,17 @@ fn validate_tx( || source == addr, ctx, &tx, + cmt, &addr, )?, Action::Masp(MaspAction::MaspAuthorizer(source)) => gadget - .verify_signatures_when(|| source == addr, ctx, &tx, &addr)?, + .verify_signatures_when( + || source == addr, + ctx, + &tx, + cmt, + &addr, + )?, Action::Masp(MaspAction::MaspSectionRef(_)) => (), Action::IbcShielding => (), } @@ -164,6 +173,7 @@ fn validate_tx( || change.is_negative(), ctx, &tx, + cmt, &addr, )?; let sign = if change.non_negative() { "" } else { "-" }; @@ -193,12 +203,13 @@ fn validate_tx( || minter_addr == &addr, ctx, &tx, + cmt, &addr, ), KeyType::Masp | KeyType::Ibc => Ok(()), KeyType::Unknown => { // Unknown changes require a valid signature - gadget.verify_signatures(ctx, &tx, &addr) + gadget.verify_signatures(ctx, &tx, cmt, &addr) } }; validate_change().inspect_err(|reason| { @@ -1039,4 +1050,176 @@ mod tests { .contains("InvalidSectionSignature") ); } + + // Test malleability attacks on memo sections + #[test] + fn test_tampered_memo_section() { + // Initialize a tx environment + let mut tx_env = TestTxEnv::default(); + + let secret_key = key::testing::keypair_1(); + let public_key = secret_key.ref_to(); + let vp_owner: Address = (&public_key).into(); + let target = address::testing::established_address_2(); + let token = address::testing::nam(); + let amount = token::Amount::from_uint(10_098_123, 0).unwrap(); + + tx_env.init_parameters(None, None, None); + + // Spawn the accounts to be able to modify their storage + tx_env.spawn_accounts([&vp_owner, &target, &token]); + tx_env.init_account_storage(&vp_owner, vec![public_key.clone()], 1); + + // Credit the tokens to the VP owner before running the transaction to + // be able to transfer from it + tx_env.credit_tokens(&vp_owner, &token, amount); + // write the denomination of NAM into storage + token::write_denom( + &mut tx_env.state, + &token, + token::NATIVE_MAX_DECIMAL_PLACES.into(), + ) + .unwrap(); + + let amount = token::DenominatedAmount::new( + amount, + token::NATIVE_MAX_DECIMAL_PLACES.into(), + ); + // Initialize VP environment from a transaction + vp_host_env::init_from_tx(vp_owner.clone(), tx_env, |address| { + // Apply transfer in a transaction + tx_host_env::token::transfer( + tx::ctx(), + address, + &target, + &token, + amount.amount(), + ) + .unwrap(); + }); + let mut vp_env = vp_host_env::take(); + let pks_map = AccountPublicKeysMap::from_iter(vec![public_key]); + + let mut tx = vp_env.batched_tx.tx.clone(); + tx.set_data(Data::new(vec![])); + tx.set_code(Code::new(vec![], None)); + let (_, memo_hash) = tx.add_memo(&[1, 2, 3]); + tx.push_default_inner_tx(); + tx.set_data(Data::new(vec![])); + tx.set_code(Code::new(vec![], None)); + tx.add_memo(&[1, 2, 3]); + tx.add_section(Section::Authorization(Authorization::new( + vec![tx.raw_header_hash()], + pks_map.index_secret_keys(vec![secret_key]), + None, + ))); + + // Tamper with the first memo section + let memo_section = tx + .sections + .iter_mut() + .find(|sec| sec.get_hash() == memo_hash) + .unwrap(); + let mut bytes = memo_section.extra_data().unwrap(); + *bytes.last_mut().unwrap() = 4; + *memo_section = Section::ExtraData(Code::new(bytes, None)); + + let signed_tx = tx.clone().batch_first_tx(); + vp_env.batched_tx = signed_tx.clone(); + let keys_changed: BTreeSet = + vp_env.all_touched_storage_keys(); + let verifiers: BTreeSet
= BTreeSet::default(); + vp_host_env::set(vp_env); + + // 1. Tampared memo section of the current inner tx causes tx rejection + // when validating the signature + let err = validate_tx( + &CTX, + signed_tx.clone(), + vp_owner.clone(), + keys_changed, + verifiers, + ) + .unwrap_err(); + if let VpError::Erased(msg) = err { + assert_eq!( + msg, + format!("Memo section with hash {} is missing", memo_hash) + ); + } else { + panic!("Got wrong error message"); + } + + // 2. If signature is not checked, a tampered memo section should not + // cause rejection + assert!( + validate_tx( + &CTX, + signed_tx, + vp_owner.clone(), + Default::default(), + Default::default(), + ) + .is_ok() + ); + + // 3. If the memo section of another inner tx has been tampered with the + // current inner tx does not get rejected + let mut tx_env = TestTxEnv::default(); + let vp_owner = address::testing::established_address_1(); + let keypair = key::testing::keypair_1(); + let public_key = keypair.ref_to(); + let target = address::testing::established_address_2(); + let token = address::testing::nam(); + let amount = token::Amount::from_uint(10_098_123, 0).unwrap(); + + // Spawn the accounts to be able to modify their storage + tx_env.spawn_accounts([&vp_owner, &target, &token]); + tx_env.init_account_storage(&vp_owner, vec![public_key.clone()], 1); + + // Credit the tokens to the VP owner before running the transaction to + // be able to transfer from it + tx_env.credit_tokens(&vp_owner, &token, amount); + // write the denomination of NAM into storage + token::write_denom( + &mut tx_env.state, + &token, + token::NATIVE_MAX_DECIMAL_PLACES.into(), + ) + .unwrap(); + + let amount = token::DenominatedAmount::new( + amount, + token::NATIVE_MAX_DECIMAL_PLACES.into(), + ); + + // Initialize VP environment from a transaction that requires a + // signature check + vp_host_env::init_from_tx(vp_owner.clone(), tx_env, |address| { + // Apply transfer in a transaction + tx_host_env::token::transfer( + tx::ctx(), + address, + &target, + &token, + amount.amount(), + ) + .unwrap(); + }); + let mut vp_env = vp_host_env::take(); + let second_inner_tx = tx.header.batch.get_index(1).unwrap().clone(); + let signed_tx = tx.batch_tx(second_inner_tx); + vp_env.batched_tx = signed_tx.clone(); + vp_host_env::set(vp_env); + assert!( + validate_tx( + &CTX, + signed_tx, + vp_owner, + Default::default(), + Default::default(), + ) + .is_ok() + ); + } } diff --git a/wasm/vp_user/src/lib.rs b/wasm/vp_user/src/lib.rs index 78544afca61..12d0d603bb7 100644 --- a/wasm/vp_user/src/lib.rs +++ b/wasm/vp_user/src/lib.rs @@ -68,6 +68,7 @@ fn validate_tx( || source == addr, ctx, &tx, + cmt, &addr, )?, PosAction::Bond(Bond { @@ -84,6 +85,7 @@ fn validate_tx( || source == addr, ctx, &tx, + cmt, &addr, )? } @@ -99,10 +101,17 @@ fn validate_tx( || source == addr, ctx, &tx, + cmt, &addr, )?, Action::Masp(MaspAction::MaspAuthorizer(source)) => gadget - .verify_signatures_when(|| source == addr, ctx, &tx, &addr)?, + .verify_signatures_when( + || source == addr, + ctx, + &tx, + cmt, + &addr, + )?, Action::Masp(MaspAction::MaspSectionRef(_)) => (), Action::IbcShielding => (), } @@ -124,6 +133,7 @@ fn validate_tx( || change.is_negative(), ctx, &tx, + cmt, &addr, )?; let sign = if change.non_negative() { "" } else { "-" }; @@ -153,6 +163,7 @@ fn validate_tx( || minter_addr == &addr, ctx, &tx, + cmt, &addr, ), KeyType::Vp(owner) => { @@ -162,13 +173,14 @@ fn validate_tx( || owner == &addr && vp_overwritten, ctx, &tx, + cmt, &addr, ) } KeyType::Masp | KeyType::Ibc => Ok(()), KeyType::Unknown => { // Unknown changes require a valid signature - gadget.verify_signatures(ctx, &tx, &addr) + gadget.verify_signatures(ctx, &tx, cmt, &addr) } }; validate_change().inspect_err(|reason| { @@ -1533,4 +1545,176 @@ mod tests { .is_ok() ); } + + // Test malleability attacks on memo sections + #[test] + fn test_tampered_memo_section() { + // Initialize a tx environment + let mut tx_env = TestTxEnv::default(); + + let vp_owner = address::testing::established_address_1(); + let keypair = key::testing::keypair_1(); + let public_key = keypair.ref_to(); + let target = address::testing::established_address_2(); + let token = address::testing::nam(); + let amount = token::Amount::from_uint(10_098_123, 0).unwrap(); + + // Spawn the accounts to be able to modify their storage + tx_env.spawn_accounts([&vp_owner, &target, &token]); + tx_env.init_account_storage(&vp_owner, vec![public_key.clone()], 1); + + // Credit the tokens to the VP owner before running the transaction to + // be able to transfer from it + tx_env.credit_tokens(&vp_owner, &token, amount); + // write the denomination of NAM into storage + token::write_denom( + &mut tx_env.state, + &token, + token::NATIVE_MAX_DECIMAL_PLACES.into(), + ) + .unwrap(); + + let amount = token::DenominatedAmount::new( + amount, + token::NATIVE_MAX_DECIMAL_PLACES.into(), + ); + + // Initialize VP environment from a transaction that requires a + // signature check + vp_host_env::init_from_tx(vp_owner.clone(), tx_env, |address| { + // Apply transfer in a transaction + tx_host_env::token::transfer( + tx::ctx(), + address, + &target, + &token, + amount.amount(), + ) + .unwrap(); + }); + let mut vp_env = vp_host_env::take(); + let pks_map = AccountPublicKeysMap::from_iter(vec![public_key]); + + let mut tx = vp_env.batched_tx.tx.clone(); + tx.set_data(Data::new(vec![])); + tx.set_code(Code::new(vec![], None)); + let (_, memo_hash) = tx.add_memo(&[1, 2, 3]); + tx.push_default_inner_tx(); + tx.set_data(Data::new(vec![])); + tx.set_code(Code::new(vec![], None)); + tx.add_memo(&[1, 2, 3]); + tx.add_section(Section::Authorization(Authorization::new( + vec![tx.raw_header_hash()], + pks_map.index_secret_keys(vec![keypair]), + None, + ))); + + // Tamper with the first memo section + let memo_section = tx + .sections + .iter_mut() + .find(|sec| sec.get_hash() == memo_hash) + .unwrap(); + let mut bytes = memo_section.extra_data().unwrap(); + *bytes.last_mut().unwrap() = 4; + *memo_section = Section::ExtraData(Code::new(bytes, None)); + + let signed_tx = tx.clone().batch_first_tx(); + vp_env.batched_tx = signed_tx.clone(); + let keys_changed: BTreeSet = + vp_env.all_touched_storage_keys(); + let verifiers: BTreeSet
= BTreeSet::default(); + vp_host_env::set(vp_env); + + // 1. Tampared memo section of the current inner tx causes tx rejection + // when validating the signature + let err = validate_tx( + &CTX, + signed_tx.clone(), + vp_owner.clone(), + keys_changed, + verifiers, + ) + .unwrap_err(); + if let VpError::Erased(msg) = err { + assert_eq!( + msg, + format!("Memo section with hash {} is missing", memo_hash) + ); + } else { + panic!("Got wrong error message"); + } + + // 2. If signature is not checked, a tampered memo section should not + // cause rejection + assert!( + validate_tx( + &CTX, + signed_tx, + vp_owner.clone(), + Default::default(), + Default::default(), + ) + .is_ok() + ); + + // 3. If the memo section of another inner tx has been tampered with the + // current inner tx does not get rejected + let mut tx_env = TestTxEnv::default(); + let vp_owner = address::testing::established_address_1(); + let keypair = key::testing::keypair_1(); + let public_key = keypair.ref_to(); + let target = address::testing::established_address_2(); + let token = address::testing::nam(); + let amount = token::Amount::from_uint(10_098_123, 0).unwrap(); + + // Spawn the accounts to be able to modify their storage + tx_env.spawn_accounts([&vp_owner, &target, &token]); + tx_env.init_account_storage(&vp_owner, vec![public_key.clone()], 1); + + // Credit the tokens to the VP owner before running the transaction to + // be able to transfer from it + tx_env.credit_tokens(&vp_owner, &token, amount); + // write the denomination of NAM into storage + token::write_denom( + &mut tx_env.state, + &token, + token::NATIVE_MAX_DECIMAL_PLACES.into(), + ) + .unwrap(); + + let amount = token::DenominatedAmount::new( + amount, + token::NATIVE_MAX_DECIMAL_PLACES.into(), + ); + + // Initialize VP environment from a transaction that requires a + // signature check + vp_host_env::init_from_tx(vp_owner.clone(), tx_env, |address| { + // Apply transfer in a transaction + tx_host_env::token::transfer( + tx::ctx(), + address, + &target, + &token, + amount.amount(), + ) + .unwrap(); + }); + let mut vp_env = vp_host_env::take(); + let second_inner_tx = tx.header.batch.get_index(1).unwrap().clone(); + let signed_tx = tx.batch_tx(second_inner_tx); + vp_env.batched_tx = signed_tx.clone(); + vp_host_env::set(vp_env); + assert!( + validate_tx( + &CTX, + signed_tx, + vp_owner, + Default::default(), + Default::default(), + ) + .is_ok() + ); + } }