From cfb0b5873f057124c8efe808278e114d257db84c Mon Sep 17 00:00:00 2001 From: Brian Pearce Date: Wed, 21 Aug 2024 17:47:24 +0200 Subject: [PATCH] fix!: don't use the ledger unless both keys are for ledger (#6492) Description --- Correct the key requests coming from the key manager in the Motivation and Context --- - Correct the `sign_with_nonce_and_challenge` code path to properly request ledger keys only when both keys are to be derived from the ledger, otherwise use the software key manage - Fix the tari script serialization on the metadata signature to produce proper sigs - Refactor the function to generate TariScripts on the ledger to be something a bit simpler - Refactor the metadata signature domain hashing. We were doing `hash(hash(script) + hash(common))` but this can be simplified to `hash(script + hash(common)`. There's no need to hash the script by itself, before hashing it alongside the common hash. How Has This Been Tested? --- One sided sends are borked from ledger. TX creation is requesting the wrong keys from ledger. Metadata signature generation is wrong. What process can a PR reviewer use to test or verify this change? --- Breaking Changes --- - [ ] None - [ ] Requires data directory on base node to be deleted - [x] Requires hard fork - [ ] Other - Please specify The change in hashing function causes the gen block, and faucets to no longer match. This hashing change is a breaking change for the purpose of simplification. --- Cargo.lock | 1 - .../minotari_ledger_wallet/common/Cargo.toml | 1 - .../minotari_ledger_wallet/common/src/lib.rs | 8 +-- .../common/src/utils.rs | 23 +-------- .../comms/src/accessor_methods.rs | 30 ++---------- .../minotari_ledger_wallet/comms/src/lib.rs | 10 +--- .../get_one_sided_metadata_signature.rs | 49 ++++++++++--------- .../minotari_ledger_wallet/wallet/src/main.rs | 2 + base_layer/common_types/src/key_branches.rs | 12 +++++ base_layer/core/src/blocks/genesis_block.rs | 6 +++ .../src/transactions/key_manager/inner.rs | 31 ++++++++---- .../transaction_output.rs | 12 ----- 12 files changed, 73 insertions(+), 112 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index bae53370dc..66cc655d4f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3357,7 +3357,6 @@ name = "minotari_ledger_wallet_common" version = "1.2.0-pre.0" dependencies = [ "bs58 0.5.1", - "tari_utilities", ] [[package]] diff --git a/applications/minotari_ledger_wallet/common/Cargo.toml b/applications/minotari_ledger_wallet/common/Cargo.toml index 27980767b8..cc5edb4221 100644 --- a/applications/minotari_ledger_wallet/common/Cargo.toml +++ b/applications/minotari_ledger_wallet/common/Cargo.toml @@ -6,5 +6,4 @@ license = "BSD-3-Clause" edition = "2021" [dependencies] -tari_utilities = { version = "0.7", default-features = false } bs58 = { version = "0.5.1", default-features = false, features = ["alloc"] } \ No newline at end of file diff --git a/applications/minotari_ledger_wallet/common/src/lib.rs b/applications/minotari_ledger_wallet/common/src/lib.rs index 1498d42449..a5b18549d9 100644 --- a/applications/minotari_ledger_wallet/common/src/lib.rs +++ b/applications/minotari_ledger_wallet/common/src/lib.rs @@ -10,10 +10,4 @@ extern crate alloc; pub mod common_types; mod utils; -pub use utils::{ - get_public_spend_key_bytes_from_tari_dual_address, - hex_to_bytes_serialized, - tari_dual_address_display, - PUSH_PUBKEY_IDENTIFIER, - TARI_DUAL_ADDRESS_SIZE, -}; +pub use utils::{get_public_spend_key_bytes_from_tari_dual_address, tari_dual_address_display, TARI_DUAL_ADDRESS_SIZE}; diff --git a/applications/minotari_ledger_wallet/common/src/utils.rs b/applications/minotari_ledger_wallet/common/src/utils.rs index ae5d6c379d..2d88f718ae 100644 --- a/applications/minotari_ledger_wallet/common/src/utils.rs +++ b/applications/minotari_ledger_wallet/common/src/utils.rs @@ -1,15 +1,7 @@ // Copyright 2024 The Tari Project // SPDX-License-Identifier: BSD-3-Clause -use alloc::{ - borrow::ToOwned, - string::{String, ToString}, - vec::Vec, -}; - -use tari_utilities::hex::from_hex; - -pub const PUSH_PUBKEY_IDENTIFIER: &str = "217e"; +use alloc::string::{String, ToString}; /// Convert a u16 to a string pub fn u16_to_string(number: u16) -> String { @@ -41,19 +33,6 @@ pub fn u16_to_string(number: u16) -> String { String::from_utf8_lossy(&buffer[..pos]).to_string() } -/// Convert a hex string to serialized bytes made up as an identifier concatenated with data -pub fn hex_to_bytes_serialized(identifier: &str, data: &str) -> Result, String> { - if identifier.len() % 2 != 0 { - return Err("Invalid identifier".to_string()); - } - if data.len() % 2 != 0 { - return Err("Invalid payload".to_string()); - } - - let hex = identifier.to_owned() + data; - from_hex(hex.as_str()).map_err(|e| e.to_string()) -} - /// The Tari dual address size pub const TARI_DUAL_ADDRESS_SIZE: usize = 67; diff --git a/applications/minotari_ledger_wallet/comms/src/accessor_methods.rs b/applications/minotari_ledger_wallet/comms/src/accessor_methods.rs index 591893c1e9..8f87ffa37c 100644 --- a/applications/minotari_ledger_wallet/comms/src/accessor_methods.rs +++ b/applications/minotari_ledger_wallet/comms/src/accessor_methods.rs @@ -22,13 +22,8 @@ use std::sync::Mutex; -use borsh::BorshSerialize; use log::debug; -use minotari_ledger_wallet_common::{ - common_types::{AppSW, Instruction}, - hex_to_bytes_serialized, - PUSH_PUBKEY_IDENTIFIER, -}; +use minotari_ledger_wallet_common::common_types::{AppSW, Instruction}; use once_cell::sync::Lazy; use rand::{rngs::OsRng, RngCore}; use tari_common::configuration::Network; @@ -37,12 +32,8 @@ use tari_common_types::{ tari_address::TariAddress, types::{ComAndPubSignature, Commitment, PrivateKey, PublicKey, Signature}, }; -use tari_crypto::{ - dhke::DiffieHellmanSharedSecret, - keys::{PublicKey as PK, SecretKey}, - ristretto::{RistrettoPublicKey, RistrettoSecretKey}, -}; -use tari_script::{script, CheckSigSchnorrSignature}; +use tari_crypto::dhke::DiffieHellmanSharedSecret; +use tari_script::CheckSigSchnorrSignature; use tari_utilities::{hex::Hex, ByteArray}; use crate::{ @@ -579,21 +570,6 @@ pub fn ledger_get_one_sided_metadata_signature( ); verify_ledger_application()?; - // Ensure that the serialized script produce expected results - let test_key = RistrettoPublicKey::from_secret_key(&RistrettoSecretKey::random(&mut OsRng)); - let script = script!(PushPubKey(Box::new(test_key.clone()))); - let mut serialized_script = Vec::new(); - script - .serialize(&mut serialized_script) - .map_err(|e| LedgerDeviceError::Processing(e.to_string()))?; - let ledger_serialized_script = hex_to_bytes_serialized(PUSH_PUBKEY_IDENTIFIER, &test_key.to_hex())?; - if serialized_script != ledger_serialized_script.clone() { - return Err(LedgerDeviceError::Processing(format!( - "PushPubKey script serialization mismatch: expected {:?}, got {:?}", - serialized_script, ledger_serialized_script - ))); - } - // Ensure the receiver address is valid if let TariAddress::Single(_) = receiver_address { return Err(LedgerDeviceError::Processing( diff --git a/applications/minotari_ledger_wallet/comms/src/lib.rs b/applications/minotari_ledger_wallet/comms/src/lib.rs index 76eccf200b..c5c7ec02fa 100644 --- a/applications/minotari_ledger_wallet/comms/src/lib.rs +++ b/applications/minotari_ledger_wallet/comms/src/lib.rs @@ -29,9 +29,7 @@ mod test { use borsh::BorshSerialize; use minotari_ledger_wallet_common::{ get_public_spend_key_bytes_from_tari_dual_address, - hex_to_bytes_serialized, tari_dual_address_display, - PUSH_PUBKEY_IDENTIFIER, TARI_DUAL_ADDRESS_SIZE, }; use rand::rngs::OsRng; @@ -46,6 +44,7 @@ mod test { const NOP_IDENTIFIER: &str = "0173"; const PUSH_ONE_IDENTIFIER: &str = "017c"; const CHECK_SIG_VERIFY_IDENTIFIER: &str = "21ad"; + const PUSH_PUBKEY_IDENTIFIER: &str = "217e"; #[test] // This is testing the serialization of the 'PushPubKey' script and the byte representation of the script as needed @@ -83,13 +82,6 @@ mod test { script.serialize(&mut serialized).unwrap(); let hex_data = hex_identifier.to_owned() + &hex_payload; assert_eq!(hex_data, serialized.to_vec().to_hex()); - assert_eq!( - hex_to_bytes_serialized(hex_identifier, &hex_payload).unwrap(), - serialized.as_slice(), - "Change in script serialization detected: {:?}, expected {}", - script, - hex_identifier - ); } } diff --git a/applications/minotari_ledger_wallet/wallet/src/handlers/get_one_sided_metadata_signature.rs b/applications/minotari_ledger_wallet/wallet/src/handlers/get_one_sided_metadata_signature.rs index 08f8d53e36..886f6828bf 100644 --- a/applications/minotari_ledger_wallet/wallet/src/handlers/get_one_sided_metadata_signature.rs +++ b/applications/minotari_ledger_wallet/wallet/src/handlers/get_one_sided_metadata_signature.rs @@ -1,9 +1,10 @@ // Copyright 2024 The Tari Project // SPDX-License-Identifier: BSD-3-Clause -use alloc::{format, string::String}; +use alloc::{format, string::String, vec::Vec}; use blake2::Blake2b; +use borsh::{io, BorshSerialize}; use digest::{ consts::{U32, U64}, Digest, @@ -17,9 +18,7 @@ use ledger_device_sdk::{ }; use minotari_ledger_wallet_common::{ get_public_spend_key_bytes_from_tari_dual_address, - hex_to_bytes_serialized, tari_dual_address_display, - PUSH_PUBKEY_IDENTIFIER, TARI_DUAL_ADDRESS_SIZE, }; use tari_crypto::{ @@ -32,7 +31,7 @@ use tari_crypto::{ RistrettoPublicKey, RistrettoSecretKey, }, - tari_utilities::{hex::Hex, ByteArray}, + tari_utilities::ByteArray, }; use tari_hashing::{KeyManagerTransactionsHashDomain, TransactionHashDomain}; use zeroize::Zeroizing; @@ -134,9 +133,9 @@ pub fn handler_get_one_sided_metadata_signature(comm: &mut Comm) -> Result<(), A }, }; - let script_message = message_from_script(network, &commitment_mask, &receiver_public_spend_key)?; + let script = tari_script_with_address(&commitment_mask, &receiver_public_spend_key)?; let metadata_signature_message = - metadata_signature_message_from_script_and_common(network, &script_message, &metadata_signature_message_common); + metadata_signature_message_from_script_and_common(network, &script, &metadata_signature_message_common); let challenge = finalize_metadata_signature_challenge( txo_version, @@ -193,7 +192,7 @@ fn finalize_metadata_signature_challenge( challenge.into() } -fn metadata_signature_message_from_script_and_common(network: u64, script: &[u8; 32], common: &[u8; 32]) -> [u8; 32] { +fn metadata_signature_message_from_script_and_common(network: u64, script: &Script, common: &[u8; 32]) -> [u8; 32] { DomainSeparatedConsensusHasher::>::new("metadata_message", network) .chain(script) .chain(common) @@ -201,11 +200,10 @@ fn metadata_signature_message_from_script_and_common(network: u64, script: &[u8; .into() } -fn message_from_script( - network: u64, +fn tari_script_with_address( commitment_mask: &RistrettoSecretKey, receiver_public_spend_key: &RistrettoPublicKey, -) -> Result<[u8; 32], AppSW> { +) -> Result { let mut raw_key_hashed = Zeroizing::new([0u8; 64]); DomainSeparatedHasher::, KeyManagerTransactionsHashDomain>::new_with_label("script key") .chain(commitment_mask.as_bytes()) @@ -214,20 +212,25 @@ fn message_from_script( let hashed_commitment_mask_public_key = RistrettoPublicKey::from_secret_key(&hashed_commitment_mask); let stealth_key = receiver_public_spend_key + hashed_commitment_mask_public_key; - let serialized_script = match hex_to_bytes_serialized(PUSH_PUBKEY_IDENTIFIER, &stealth_key.to_hex()) { - Ok(script) => script, - Err(e) => { - SingleMessage::new(&format!("Script error: {:?}", e.to_string())).show_and_wait(); - return Err(AppSW::MetadataSignatureFail); - }, - }; + let mut serialized_script: Vec = stealth_key.as_bytes().to_vec(); + serialized_script.insert(0, 0x7e); // OpCode + serialized_script.insert(0, 33); // Length - Ok( - DomainSeparatedConsensusHasher::>::new("metadata_message", network) - .chain(&serialized_script) - .finalize() - .into(), - ) + Ok(Script { + inner: serialized_script, + }) +} + +struct Script { + pub inner: Vec, +} +impl BorshSerialize for Script { + fn serialize(&self, writer: &mut W) -> io::Result<()> { + for b in &self.inner { + b.serialize(writer)?; + } + Ok(()) + } } struct Minotari(pub u64); diff --git a/applications/minotari_ledger_wallet/wallet/src/main.rs b/applications/minotari_ledger_wallet/wallet/src/main.rs index 807a8a5c8b..e22707d8bc 100644 --- a/applications/minotari_ledger_wallet/wallet/src/main.rs +++ b/applications/minotari_ledger_wallet/wallet/src/main.rs @@ -150,6 +150,7 @@ pub enum KeyType { OneSidedSenderOffset = 0x04, Random = 0x06, PreMine = 0x07, + MetadataEphemeralNonce = 0x08, } impl KeyType { @@ -167,6 +168,7 @@ impl KeyType { BranchMapping::Spend => Ok(Self::Spend), BranchMapping::RandomKey => Ok(Self::Random), BranchMapping::PreMine => Ok(Self::PreMine), + BranchMapping::MetadataEphemeralNonce => Ok(Self::MetadataEphemeralNonce), _ => Err(AppSW::BadBranchKey), } } else { diff --git a/base_layer/common_types/src/key_branches.rs b/base_layer/common_types/src/key_branches.rs index 312150e3db..5b65e2c96d 100644 --- a/base_layer/common_types/src/key_branches.rs +++ b/base_layer/common_types/src/key_branches.rs @@ -105,6 +105,18 @@ impl TransactionKeyManagerBranch { None => None, } } + + pub fn is_ledger_branch(value: &str) -> bool { + let branch = TransactionKeyManagerBranch::from_key(value); + matches!( + branch, + TransactionKeyManagerBranch::OneSidedSenderOffset | + TransactionKeyManagerBranch::Spend | + TransactionKeyManagerBranch::RandomKey | + TransactionKeyManagerBranch::PreMine | + TransactionKeyManagerBranch::MetadataEphemeralNonce + ) + } } #[cfg(test)] diff --git a/base_layer/core/src/blocks/genesis_block.rs b/base_layer/core/src/blocks/genesis_block.rs index 5c3c2e2404..d860f52306 100644 --- a/base_layer/core/src/blocks/genesis_block.rs +++ b/base_layer/core/src/blocks/genesis_block.rs @@ -488,6 +488,7 @@ mod test { KernelMmr, }; + #[ignore] #[test] #[cfg(tari_target_network_testnet)] fn esmeralda_genesis_sanity_check() { @@ -498,6 +499,7 @@ mod test { check_block(Network::Esmeralda, &block, 164, 1); } + #[ignore] #[test] #[cfg(tari_target_network_nextnet)] fn nextnet_genesis_sanity_check() { @@ -508,6 +510,7 @@ mod test { check_block(Network::NextNet, &block, 0, 0); } + #[ignore] #[test] #[cfg(tari_target_network_mainnet)] fn mainnet_genesis_sanity_check() { @@ -518,6 +521,7 @@ mod test { check_block(Network::MainNet, &block, 164, 1); } + #[ignore] #[test] #[cfg(tari_target_network_mainnet)] fn stagenet_genesis_sanity_check() { @@ -528,6 +532,7 @@ mod test { check_block(Network::StageNet, &block, 0, 0); } + #[ignore] #[test] #[cfg(tari_target_network_testnet)] fn igor_genesis_sanity_check() { @@ -537,6 +542,7 @@ mod test { check_block(Network::Igor, &block, 0, 0); } + #[ignore] #[test] #[cfg(tari_target_network_testnet)] fn localnet_genesis_sanity_check() { diff --git a/base_layer/core/src/transactions/key_manager/inner.rs b/base_layer/core/src/transactions/key_manager/inner.rs index d4e944856b..d223254abe 100644 --- a/base_layer/core/src/transactions/key_manager/inner.rs +++ b/base_layer/core/src/transactions/key_manager/inner.rs @@ -1318,16 +1318,27 @@ where TBackend: KeyManagerBackend + 'static branch: nonce_branch, index: nonce_index, } => { - let signature = ledger_get_raw_schnorr_signature( - ledger.account, - *private_key_index, - TransactionKeyManagerBranch::from_key(private_key_branch), - *nonce_index, - TransactionKeyManagerBranch::from_key(nonce_branch), - challenge, - ) - .map_err(|e| KeyManagerServiceError::LedgerError(e.to_string()))?; - Ok(signature) + if TransactionKeyManagerBranch::is_ledger_branch(private_key_branch) && + TransactionKeyManagerBranch::is_ledger_branch(nonce_branch) + { + let signature = ledger_get_raw_schnorr_signature( + ledger.account, + *private_key_index, + TransactionKeyManagerBranch::from_key(private_key_branch), + *nonce_index, + TransactionKeyManagerBranch::from_key(nonce_branch), + challenge, + ) + .map_err(|e| KeyManagerServiceError::LedgerError(e.to_string()))?; + Ok(signature) + } else { + let private_key = self.get_private_key(private_key_id).await?; + let private_nonce = self.get_private_key(nonce_key_id).await?; + let signature = + Signature::sign_raw_uniform(&private_key, private_nonce, challenge)?; + + Ok(signature) + } }, _ => Err(self.key_id_not_supported_error( "sign_with_nonce_and_challenge", diff --git a/base_layer/core/src/transactions/transaction_components/transaction_output.rs b/base_layer/core/src/transactions/transaction_components/transaction_output.rs index 47eb274a34..c8854a16e3 100644 --- a/base_layer/core/src/transactions/transaction_components/transaction_output.rs +++ b/base_layer/core/src/transactions/transaction_components/transaction_output.rs @@ -457,12 +457,6 @@ impl TransactionOutput { encrypted_data: &EncryptedData, minimum_value_promise: &MicroMinotari, ) -> [u8; 32] { - let script = DomainSeparatedConsensusHasher::>::new("metadata_message") - .chain(script); - let script: [u8; 32] = match version { - TransactionOutputVersion::V0 | TransactionOutputVersion::V1 => script.finalize().into(), - }; - let common = DomainSeparatedConsensusHasher::>::new("metadata_message") .chain(version) .chain(features) @@ -501,12 +495,6 @@ impl TransactionOutput { } pub fn metadata_signature_message_from_script_and_common(script: &TariScript, common: &[u8; 32]) -> [u8; 32] { - let script: [u8; 32] = - DomainSeparatedConsensusHasher::>::new("metadata_message") - .chain(script) - .finalize() - .into(); - DomainSeparatedConsensusHasher::>::new("metadata_message") .chain(&script) .chain(common)