From f069b14527d269562976413b3c56d0f3725fa707 Mon Sep 17 00:00:00 2001 From: Aaron Feickert <66188213+AaronFeickert@users.noreply.github.com> Date: Mon, 5 Dec 2022 09:58:44 +0200 Subject: [PATCH] fix: improve key handling (#4994) Description --- Updates the use of `CoreTransactionAEADKey` and `CommsNoiseKey` to be populated in place. Switches `CommsNoiseKey` to use a `SafeArray` under the hood. Extends the work of [PR 4967](https://github.com/tari-project/tari/pull/4967). Motivation and Context --- Earlier work updates the use of keys for value encryption and Noise. Value encryption keys use a `SafeArray` type in a `Hidden` wrapper, and Noise keys use a `Hidden` array. However, in both cases, a copy of the hash output used to populate the keys is left in memory. This work mitigates the problem. Because the hashing API now supports in-place output finalization via `finalize_into` and `finalize_into_reset`, we can populate keys directly by mutable reference. It also renames `CoreTransactionAEADKey` to `EncryptedValueKey` for clarity, and switches `CommsNoiseKey` to be a `SafeArray` under the hood. There is [discussion](https://github.com/tari-project/tari/pull/4967#discussion_r1033628944) on whether `CommsNoiseKey` needs to be a `SafeArray`; while the reasoning makes sense, I still think it's good practice to take advantage of the benefits of `SafeArray` for array-like key types unless there's a compelling performance reason otherwise. Discussion on this is welcome! How Has This Been Tested? --- Existing tests pass. --- .../transaction_components/encrypted_value.rs | 19 ++++++++----------- .../transaction_components/mod.rs | 2 +- comms/core/src/noise/crypto_resolver.rs | 18 ++++++++++-------- comms/core/src/noise/mod.rs | 4 ++-- 4 files changed, 21 insertions(+), 22 deletions(-) diff --git a/base_layer/core/src/transactions/transaction_components/encrypted_value.rs b/base_layer/core/src/transactions/transaction_components/encrypted_value.rs index b32f78af6f..defd032c18 100644 --- a/base_layer/core/src/transactions/transaction_components/encrypted_value.rs +++ b/base_layer/core/src/transactions/transaction_components/encrypted_value.rs @@ -30,7 +30,7 @@ use chacha20poly1305::{ KeyInit, Nonce, }; -use digest::generic_array::GenericArray; +use digest::{generic_array::GenericArray, FixedOutput}; use serde::{Deserialize, Serialize}; use tari_common_types::types::{Commitment, PrivateKey}; use tari_crypto::{hash::blake2::Blake256, hashing::DomainSeparatedHasher}; @@ -38,7 +38,7 @@ use tari_utilities::{safe_array::SafeArray, ByteArray, ByteArrayError}; use thiserror::Error; use zeroize::Zeroize; -use super::{CoreTransactionAEADKey, AEAD_KEY_LEN}; +use super::EncryptedValueKey; use crate::transactions::{tari_amount::MicroTari, TransactionKdfDomain}; const SIZE: usize = 24; @@ -117,16 +117,13 @@ impl EncryptedValue { } } -// Generate a ChaCha20-Poly1305 key from an ECDH shared secret and commitment using Blake2b -fn kdf_aead(shared_secret: &PrivateKey, commitment: &Commitment) -> CoreTransactionAEADKey { - let output = DomainSeparatedHasher::::new_with_label("encrypted_value") - .chain(shared_secret.as_bytes()) +// Generate a ChaCha20-Poly1305 key from a private key and commitment using Blake2b +fn kdf_aead(private_key: &PrivateKey, commitment: &Commitment) -> EncryptedValueKey { + let mut aead_key = EncryptedValueKey::from(SafeArray::default()); + DomainSeparatedHasher::::new_with_label("encrypted_value") + .chain(private_key.as_bytes()) .chain(commitment.as_bytes()) - .finalize(); - - let default_array = SafeArray::::default(); - let mut aead_key = CoreTransactionAEADKey::from(default_array); - aead_key.reveal_mut().copy_from_slice(&output.as_ref()[..AEAD_KEY_LEN]); + .finalize_into(GenericArray::from_mut_slice(aead_key.reveal_mut())); aead_key } diff --git a/base_layer/core/src/transactions/transaction_components/mod.rs b/base_layer/core/src/transactions/transaction_components/mod.rs index d317a80a0d..92ef03697c 100644 --- a/base_layer/core/src/transactions/transaction_components/mod.rs +++ b/base_layer/core/src/transactions/transaction_components/mod.rs @@ -79,7 +79,7 @@ pub const MAX_TRANSACTION_RECIPIENTS: usize = 15; pub(crate) const AEAD_KEY_LEN: usize = std::mem::size_of::(); // Type for hiding aead key encryption -hidden_type!(CoreTransactionAEADKey, SafeArray); +hidden_type!(EncryptedValueKey, SafeArray); //---------------------------------------- Crate functions ----------------------------------------------------// diff --git a/comms/core/src/noise/crypto_resolver.rs b/comms/core/src/noise/crypto_resolver.rs index fdc9277568..d8df16b8a1 100644 --- a/comms/core/src/noise/crypto_resolver.rs +++ b/comms/core/src/noise/crypto_resolver.rs @@ -20,6 +20,7 @@ // WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE // USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. +use digest::{generic_array::GenericArray, FixedOutput}; use rand::rngs::OsRng; use snow::{ params::{CipherChoice, DHChoice, HashChoice}, @@ -32,8 +33,9 @@ use tari_crypto::{ keys::{PublicKey, SecretKey}, tari_utilities::ByteArray, }; +use tari_utilities::safe_array::SafeArray; -use super::{CommsNoiseKey, NOISE_KEY_LEN}; +use super::CommsNoiseKey; use crate::types::{CommsCoreHashDomain, CommsDHKE, CommsPublicKey, CommsSecretKey}; macro_rules! copy_slice { @@ -67,12 +69,12 @@ impl CryptoResolver for TariCryptoResolver { } fn noise_kdf(shared_key: &CommsDHKE) -> CommsNoiseKey { - let hasher = DomainSeparatedHasher::::new_with_label("noise.dh"); - let mut comms_noise_kdf = CommsNoiseKey::from([0u8; NOISE_KEY_LEN]); - comms_noise_kdf - .reveal_mut() - .copy_from_slice(hasher.chain(shared_key.as_bytes()).finalize().as_ref()); - comms_noise_kdf + let mut comms_noise_key = CommsNoiseKey::from(SafeArray::default()); + DomainSeparatedHasher::::new_with_label("noise.dh") + .chain(shared_key.as_bytes()) + .finalize_into(GenericArray::from_mut_slice(comms_noise_key.reveal_mut())); + + comms_noise_key } #[derive(Default)] @@ -129,7 +131,7 @@ impl Dh for CommsDiffieHellman { mod test { use snow::Keypair; - use super::*; + use super::{super::NOISE_KEY_LEN, *}; use crate::noise::config::NOISE_IX_PARAMETER; fn build_keypair() -> Keypair { diff --git a/comms/core/src/noise/mod.rs b/comms/core/src/noise/mod.rs index 2b5c01f93d..822db101cd 100644 --- a/comms/core/src/noise/mod.rs +++ b/comms/core/src/noise/mod.rs @@ -33,9 +33,9 @@ pub use error::NoiseError; mod socket; pub use socket::NoiseSocket; -use tari_utilities::{hidden_type, Hidden}; +use tari_utilities::{hidden_type, safe_array::SafeArray, Hidden}; use zeroize::Zeroize; pub(crate) const NOISE_KEY_LEN: usize = 32; -hidden_type!(CommsNoiseKey, [u8; NOISE_KEY_LEN]); +hidden_type!(CommsNoiseKey, SafeArray);