From a20bd2a685f55d5e0a4a4b5add4e8f7f350f675f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Zemanovi=C4=8D?= Date: Sun, 12 Nov 2023 17:09:05 +0100 Subject: [PATCH 1/5] client: sign tx_init_validator with all keys used --- apps/src/lib/client/tx.rs | 16 +++++++++++--- sdk/src/signing.rs | 45 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 3 deletions(-) diff --git a/apps/src/lib/client/tx.rs b/apps/src/lib/client/tx.rs index 306c08c412a..5a2483c5d5c 100644 --- a/apps/src/lib/client/tx.rs +++ b/apps/src/lib/client/tx.rs @@ -27,7 +27,9 @@ use rand::rngs::OsRng; use super::rpc; use crate::cli::{args, safe_exit}; use crate::client::rpc::query_wasm_code_hash; -use crate::client::tx::signing::{default_sign, SigningTxData}; +use crate::client::tx::signing::{ + default_sign, init_validator_signing_data, SigningTxData, +}; use crate::client::tx::tx::ProcessTxResponse; use crate::config::TendermintMode; use crate::facade::tendermint_rpc::endpoint::broadcast::tx_sync::Response; @@ -477,7 +479,7 @@ pub async fn submit_init_validator<'a>( scheme, ) .unwrap(); - let protocol_key = validator_keys.get_protocol_keypair().ref_to(); + let protocol_key = validator_keys.get_protocol_keypair().to_public(); let validator_vp_code_hash = query_wasm_code_hash(namada, validator_vp_code_path.to_str().unwrap()) @@ -546,9 +548,17 @@ pub async fn submit_init_validator<'a>( validator_vp_code_hash: extra_section_hash, }; + // Put together all the PKs that we have to sign with to verify ownership + let mut all_pks = data.account_keys.clone(); + all_pks.push(consensus_key.to_public()); + all_pks.push(eth_cold_pk); + all_pks.push(eth_hot_pk); + all_pks.push(data.protocol_key.clone()); + tx.add_code_from_hash(tx_code_hash).add_data(data); - let signing_data = aux_signing_data(namada, &tx_args, None, None).await?; + let signing_data = + init_validator_signing_data(namada, &tx_args, all_pks).await?; tx::prepare_tx( namada, diff --git a/sdk/src/signing.rs b/sdk/src/signing.rs index 39120f82616..eee5a31d605 100644 --- a/sdk/src/signing.rs +++ b/sdk/src/signing.rs @@ -384,6 +384,51 @@ pub async fn aux_signing_data<'a>( }) } +pub async fn init_validator_signing_data<'a>( + context: &impl Namada<'a>, + args: &args::Tx, + validator_keys: Vec, +) -> Result { + let mut public_keys = if args.wrapper_fee_payer.is_none() { + tx_signers(context, args, None).await? + } else { + vec![] + }; + public_keys.extend(validator_keys.clone()); + + let account_public_keys_map = + Some(AccountPublicKeysMap::from_iter(validator_keys)); + + let fee_payer = if args.disposable_signing_key { + context + .wallet_mut() + .await + .gen_disposable_signing_key(&mut OsRng) + .to_public() + } else { + match &args.wrapper_fee_payer { + Some(keypair) => keypair.to_public(), + None => public_keys.get(0).ok_or(TxError::InvalidFeePayer)?.clone(), + } + }; + + if fee_payer == masp_tx_key().to_public() { + other_err( + "The gas payer cannot be the MASP, please provide a different gas \ + payer." + .to_string(), + )?; + } + + Ok(SigningTxData { + owner: None, + public_keys, + threshold: 0, + account_public_keys_map, + fee_payer, + }) +} + /// Informations about the post-tx balance of the tx's source. Used to correctly /// handle fee validation in the wrapper tx pub struct TxSourcePostBalance { From 2a979817a1cc9d350bdf93f4d38c5c14decb0754 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Zemanovi=C4=8D?= Date: Sun, 12 Nov 2023 17:10:47 +0100 Subject: [PATCH 2/5] ledger: add a tx host fn for sig verification --- shared/src/vm/host_env.rs | 79 ++++++++++++++++++++++++++++++++++ shared/src/vm/wasm/host_env.rs | 3 +- tests/src/vm_host_env/tx.rs | 9 ++++ tx_prelude/src/lib.rs | 37 +++++++++++++++- vm_env/src/lib.rs | 12 ++++++ 5 files changed, 138 insertions(+), 2 deletions(-) diff --git a/shared/src/vm/host_env.rs b/shared/src/vm/host_env.rs index 2ea0037f9a1..9f9ed07a184 100644 --- a/shared/src/vm/host_env.rs +++ b/shared/src/vm/host_env.rs @@ -2076,6 +2076,85 @@ where sentinel.set_invalid_commitment(); } +/// Verify a transaction signature +#[allow(clippy::too_many_arguments)] +pub fn tx_verify_tx_section_signature( + env: &TxVmEnv, + hash_list_ptr: u64, + hash_list_len: u64, + public_keys_map_ptr: u64, + public_keys_map_len: u64, + threshold: u8, + max_signatures_ptr: u64, + max_signatures_len: u64, +) -> TxResult +where + MEM: VmMemory, + DB: storage::DB + for<'iter> storage::DBIter<'iter>, + H: StorageHasher, + CA: WasmCacheAccess, +{ + let (hash_list, gas) = env + .memory + .read_bytes(hash_list_ptr, hash_list_len as _) + .map_err(|e| TxRuntimeError::MemoryError(Box::new(e)))?; + + let sentinel = unsafe { env.ctx.sentinel.get() }; + tx_charge_gas(env, gas)?; + let hashes = <[Hash; 1]>::try_from_slice(&hash_list) + .map_err(TxRuntimeError::EncodingError)?; + + let (public_keys_map, gas) = env + .memory + .read_bytes(public_keys_map_ptr, public_keys_map_len as _) + .map_err(|e| TxRuntimeError::MemoryError(Box::new(e)))?; + tx_charge_gas(env, gas)?; + let public_keys_map = + namada_core::types::account::AccountPublicKeysMap::try_from_slice( + &public_keys_map, + ) + .map_err(TxRuntimeError::EncodingError)?; + + tx_charge_gas(env, gas)?; + + let (max_signatures, gas) = env + .memory + .read_bytes(max_signatures_ptr, max_signatures_len as _) + .map_err(|e| TxRuntimeError::MemoryError(Box::new(e)))?; + tx_charge_gas(env, gas)?; + let max_signatures = Option::::try_from_slice(&max_signatures) + .map_err(TxRuntimeError::EncodingError)?; + + let tx = unsafe { env.ctx.tx.get() }; + + tx_charge_gas( + env, + gas::VERIFY_TX_SIG_GAS * public_keys_map.idx_to_pk.len() as u64, + )?; + match tx.verify_signatures( + &hashes, + public_keys_map, + &None, + threshold, + max_signatures, + // This uses VpGasMeter, so we're charging the gas here manually + // instead + &mut None, + ) { + Ok(_) => Ok(HostEnvResult::Success.to_i64()), + Err(err) => match err { + namada_core::proto::Error::OutOfGas(inner) => { + sentinel.set_out_of_gas(); + Err(TxRuntimeError::OutOfGas(inner)) + } + namada_core::proto::Error::InvalidSectionSignature(_) => { + Ok(HostEnvResult::Fail.to_i64()) + } + _ => Ok(HostEnvResult::Fail.to_i64()), + }, + } +} + /// Evaluate a validity predicate with the given input data. pub fn vp_eval( env: &VpVmEnv<'static, MEM, DB, H, EVAL, CA>, diff --git a/shared/src/vm/wasm/host_env.rs b/shared/src/vm/wasm/host_env.rs index a7dc51cc6ff..ff961c5a8d9 100644 --- a/shared/src/vm/wasm/host_env.rs +++ b/shared/src/vm/wasm/host_env.rs @@ -86,7 +86,8 @@ where "namada_tx_get_native_token" => Function::new_native_with_env(wasm_store, env.clone(), host_env::tx_get_native_token), "namada_tx_log_string" => Function::new_native_with_env(wasm_store, env.clone(), host_env::tx_log_string), "namada_tx_ibc_execute" => Function::new_native_with_env(wasm_store, env.clone(), host_env::tx_ibc_execute), - "namada_tx_set_commitment_sentinel" => Function::new_native_with_env(wasm_store, env.clone(), host_env::tx_set_commitment_sentinel) + "namada_tx_set_commitment_sentinel" => Function::new_native_with_env(wasm_store, env.clone(), host_env::tx_set_commitment_sentinel), + "namada_tx_verify_tx_section_signature" => Function::new_native_with_env(wasm_store, env.clone(), host_env::tx_verify_tx_section_signature), }, } } diff --git a/tests/src/vm_host_env/tx.rs b/tests/src/vm_host_env/tx.rs index 96c1b67cda1..605a54acae1 100644 --- a/tests/src/vm_host_env/tx.rs +++ b/tests/src/vm_host_env/tx.rs @@ -507,4 +507,13 @@ mod native_tx_host_env { native_host_fn!(tx_log_string(str_ptr: u64, str_len: u64)); native_host_fn!(tx_charge_gas(used_gas: u64)); native_host_fn!("non-result", tx_set_commitment_sentinel()); + native_host_fn!(tx_verify_tx_section_signature( + hash_list_ptr: u64, + hash_list_len: u64, + public_keys_map_ptr: u64, + public_keys_map_len: u64, + threshold: u8, + max_signatures_ptr: u64, + max_signatures_len: u64, + ) -> i64); } diff --git a/tx_prelude/src/lib.rs b/tx_prelude/src/lib.rs index 0bfefdec5ed..71b56068ee1 100644 --- a/tx_prelude/src/lib.rs +++ b/tx_prelude/src/lib.rs @@ -19,7 +19,6 @@ use std::marker::PhantomData; pub use borsh::{BorshDeserialize, BorshSerialize}; pub use borsh_ext; use borsh_ext::BorshSerializeExt; -pub use namada_core::ledger::eth_bridge; pub use namada_core::ledger::governance::storage as gov_storage; pub use namada_core::ledger::parameters::storage as parameters_storage; pub use namada_core::ledger::storage::types::encode; @@ -28,11 +27,14 @@ pub use namada_core::ledger::storage_api::{ ResultExt, StorageRead, StorageWrite, }; pub use namada_core::ledger::tx_env::TxEnv; +pub use namada_core::ledger::{eth_bridge, parameters}; pub use namada_core::proto::{Section, Tx}; +use namada_core::types::account::AccountPublicKeysMap; pub use namada_core::types::address::Address; use namada_core::types::chain::CHAIN_ID_LENGTH; pub use namada_core::types::ethereum_events::EthAddress; use namada_core::types::internal::HostEnvResult; +use namada_core::types::key::common; use namada_core::types::storage::TxIndex; pub use namada_core::types::storage::{ self, BlockHash, BlockHeight, Epoch, Header, BLOCK_HASH_LENGTH, @@ -361,3 +363,36 @@ impl TxEnv for Ctx { pub fn tx_ibc_execute() { unsafe { namada_tx_ibc_execute() } } + +/// Verify section signatures against the given list of keys +pub fn verify_signatures_of_pks( + ctx: &Ctx, + tx: &Tx, + pks: Vec, +) -> EnvResult { + let max_signatures_per_transaction = + parameters::max_signatures_per_transaction(ctx)?; + + // Require signatures from all the given keys + let threshold = u8::try_from(pks.len()).into_storage_result()?; + let public_keys_index_map = AccountPublicKeysMap::from_iter(pks); + + // Serialize parameters + let max_signatures = max_signatures_per_transaction.serialize_to_vec(); + let public_keys_map = public_keys_index_map.serialize_to_vec(); + let targets = [tx.raw_header_hash()].serialize_to_vec(); + + let valid = unsafe { + namada_tx_verify_tx_section_signature( + targets.as_ptr() as _, + targets.len() as _, + public_keys_map.as_ptr() as _, + public_keys_map.len() as _, + threshold, + max_signatures.as_ptr() as _, + max_signatures.len() as _, + ) + }; + + Ok(HostEnvResult::is_success(valid)) +} diff --git a/vm_env/src/lib.rs b/vm_env/src/lib.rs index c3497c5c272..565f5d96b4b 100644 --- a/vm_env/src/lib.rs +++ b/vm_env/src/lib.rs @@ -117,6 +117,18 @@ pub mod tx { /// Set the sentinel for a wrong tx section commitment pub fn namada_tx_set_commitment_sentinel(); + + // Verify the signatures of a tx + pub fn namada_tx_verify_tx_section_signature( + hash_list_ptr: u64, + hash_list_len: u64, + public_keys_map_ptr: u64, + public_keys_map_len: u64, + threshold: u8, + max_signatures_ptr: u64, + max_signatures_len: u64, + ) -> i64; + } } From 03524aa9d03b8467f41551b8af8ab4a1b506221a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Zemanovi=C4=8D?= Date: Sun, 12 Nov 2023 17:16:04 +0100 Subject: [PATCH 3/5] wasm/tx_init_validator: check ownership of used keys with sigs --- wasm/wasm_source/src/tx_init_validator.rs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/wasm/wasm_source/src/tx_init_validator.rs b/wasm/wasm_source/src/tx_init_validator.rs index 554b4a8ec89..0666ad7965b 100644 --- a/wasm/wasm_source/src/tx_init_validator.rs +++ b/wasm/wasm_source/src/tx_init_validator.rs @@ -32,6 +32,22 @@ fn apply_tx(ctx: &mut Ctx, tx_data: Tx) -> TxResult { .code .hash(); + // Check that the tx has been signed with all the keys to be used for the + // validator account + let mut all_pks = init_validator.account_keys.clone(); + all_pks.push(init_validator.consensus_key.clone()); + all_pks.push(key::common::PublicKey::Secp256k1( + init_validator.eth_cold_key.clone(), + )); + all_pks.push(key::common::PublicKey::Secp256k1( + init_validator.eth_hot_key.clone(), + )); + all_pks.push(init_validator.protocol_key.clone()); + if !matches!(verify_signatures_of_pks(ctx, &signed, all_pks), Ok(true)) { + debug_log!("Keys ownership signature verification failed"); + panic!() + } + // Register the validator in PoS match ctx.init_validator(init_validator, validator_vp_code_hash) { Ok(validator_address) => { From 0a64056517a53bbd5b1a340de08acef335db766c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Zemanovi=C4=8D?= Date: Sun, 12 Nov 2023 17:26:02 +0100 Subject: [PATCH 4/5] client: store validator protocol key in the regular wallet --- apps/src/lib/client/tx.rs | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/apps/src/lib/client/tx.rs b/apps/src/lib/client/tx.rs index 5a2483c5d5c..ce99a3c65f0 100644 --- a/apps/src/lib/client/tx.rs +++ b/apps/src/lib/client/tx.rs @@ -365,6 +365,7 @@ pub async fn submit_init_validator<'a>( let validator_key_alias = format!("{}-key", alias); let consensus_key_alias = format!("{}-consensus-key", alias); + let protocol_key_alias = format!("{}-protocol-key", alias); let threshold = match threshold { Some(threshold) => threshold, @@ -479,7 +480,25 @@ pub async fn submit_init_validator<'a>( scheme, ) .unwrap(); - let protocol_key = validator_keys.get_protocol_keypair().to_public(); + let protocol_sk = validator_keys.get_protocol_keypair(); + let protocol_key = protocol_sk.to_public(); + + // Store the protocol key in the wallet so that we can sign the tx with it + // to verify ownership + display_line!(namada.io(), "Storing protocol key in the wallet..."); + let password = read_and_confirm_encryption_password(unsafe_dont_encrypt); + namada + .wallet_mut() + .await + .insert_keypair( + protocol_key_alias, + tx_args.wallet_alias_force, + protocol_sk.clone(), + password, + None, + None, + ) + .map_err(|err| error::Error::Other(err.to_string()))?; let validator_vp_code_hash = query_wasm_code_hash(namada, validator_vp_code_path.to_str().unwrap()) From a9771c28e5c3770c1b09eff9f8308641b7447c47 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Zemanovi=C4=8D?= Date: Sun, 12 Nov 2023 17:38:16 +0100 Subject: [PATCH 5/5] changelog: add #2163 --- .../unreleased/improvements/2163-init-validator-own-keys.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 .changelog/unreleased/improvements/2163-init-validator-own-keys.md diff --git a/.changelog/unreleased/improvements/2163-init-validator-own-keys.md b/.changelog/unreleased/improvements/2163-init-validator-own-keys.md new file mode 100644 index 00000000000..e5660d18615 --- /dev/null +++ b/.changelog/unreleased/improvements/2163-init-validator-own-keys.md @@ -0,0 +1,2 @@ +- Require to verify ownership of all validator keys when initialized on-chain. + ([\#2163](https://github.com/anoma/namada/pull/2163)) \ No newline at end of file