fix!: improve wallet key derivation by use of proper domain separation (see issue #4170)

base_layer/key_manager/Cargo.toml

@@ -11,6 +11,7 @@ edition = "2021"
 crate-type = ["lib", "cdylib"]
 
 [dependencies]
+tari_common = { version = "^0.34", path = "../../common" }
 tari_common_types = { version = "^0.34", path = "../../base_layer/common_types" }
 tari_crypto = { git = "https://github.com/tari-project/tari-crypto.git", tag = "v0.15.0" }
 tari_utilities = { git = "https://github.com/tari-project/tari_utilities.git", tag = "v0.4.4" }
@@ -19,7 +20,7 @@ arrayvec = "0.7.1"
 argon2 = { version = "0.2", features = ["std"] }
 blake2 = "0.9.1"
 chacha20 = "0.7.1"
-chrono = { version = "0.4.19", default-features = false, features = ["serde"] }
+chrono = { version = "0.4.19", default-features = true, features = ["serde"] }
 clear_on_drop = "=0.2.4"
 console_error_panic_hook = "0.1.7"
 crc32fast = "1.2.1"

base_layer/key_manager/src/key_manager.rs

@@ -26,11 +26,13 @@ use derivative::Derivative;
 use digest::Digest;
 use serde::{Deserialize, Serialize};
 use tari_crypto::{
+    hash::blake2::Blake256,
+    hashing::LengthExtensionAttackResistant,
     keys::SecretKey,
-    tari_utilities::{byte_array::ByteArrayError, hex::Hex},
+    tari_utilities::byte_array::ByteArrayError,
 };
 
-use crate::cipher_seed::CipherSeed;
+use crate::{base_layer_key_manager, cipher_seed::CipherSeed};
 
 #[derive(Clone, Derivative, Serialize, Deserialize)]
 #[derivative(Debug)]
@@ -45,7 +47,7 @@ where K: SecretKey
 
 #[derive(Clone, Derivative, PartialEq, Serialize, Deserialize)]
 #[derivative(Debug)]
-pub struct KeyManager<K: SecretKey, D: Digest> {
+pub struct KeyManager<K: SecretKey, D: Digest + LengthExtensionAttackResistant> {
     #[derivative(Debug = "ignore")]
     seed: CipherSeed,
     #[derivative(Debug = "ignore")]
@@ -58,7 +60,7 @@ pub struct KeyManager<K: SecretKey, D: Digest> {
 impl<K, D> KeyManager<K, D>
 where
     K: SecretKey,
-    D: Digest,
+    D: Digest + LengthExtensionAttackResistant,
 {
     /// Creates a new KeyManager with a new randomly selected entropy
     pub fn new() -> KeyManager<K, D> {
@@ -82,15 +84,21 @@ where
         }
     }
 
-    /// Derive a new private key from master key: derived_key=SHA256(master_key||branch_seed||index)
+    /// Derive a new private key from master key: derived_key=H(master_key||branch_seed||index), for some
+    /// hash function H which is Length attack resistant, such as Blake2b.
     pub fn derive_key(&self, key_index: u64) -> Result<DerivedKey<K>, ByteArrayError> {
-        let concatenated = format!(
-            "{}{}{}",
-            self.seed.entropy().to_vec().to_hex(),
-            self.branch_seed,
-            key_index
-        );
-        match K::from_bytes(D::digest(&concatenated.into_bytes()).as_slice()) {
+        // apply domain separation to generate derive key. Under the hood, the hashing api prepends the length of each
+        // piece of data for concatenation, reducing the risk of collisions due to redundance of variable length
+        // input
+        let derive_key = base_layer_key_manager()
+            .hasher::<Blake256>()
+            .chain(self.seed.entropy())
+            .chain(self.branch_seed.as_str().as_bytes())
+            .chain(key_index.to_le_bytes())
+            .finalize()
+            .into_vec();
+
+        match K::from_bytes(derive_key.as_slice()) {
             Ok(k) => Ok(DerivedKey { k, key_index }),
             Err(e) => Err(e),
         }
@@ -118,7 +126,7 @@ where
 impl<K, D> Default for KeyManager<K, D>
 where
     K: SecretKey,
-    D: Digest,
+    D: Digest + LengthExtensionAttackResistant,
 {
     fn default() -> Self {
         Self::new()
@@ -127,21 +135,20 @@ where
 
 #[cfg(test)]
 mod test {
-    use sha2::Sha256;
-    use tari_crypto::ristretto::RistrettoSecretKey;
+    use tari_crypto::{hash::blake2::Blake256, ristretto::RistrettoSecretKey};
 
     use crate::key_manager::*;
 
     #[test]
     fn test_new_keymanager() {
-        let km1 = KeyManager::<RistrettoSecretKey, Sha256>::new();
-        let km2 = KeyManager::<RistrettoSecretKey, Sha256>::new();
+        let km1 = KeyManager::<RistrettoSecretKey, Blake256>::new();
+        let km2 = KeyManager::<RistrettoSecretKey, Blake256>::new();
         assert_ne!(km1.seed, km2.seed);
     }
 
     #[test]
     fn test_derive_and_next_key() {
-        let mut km = KeyManager::<RistrettoSecretKey, Sha256>::new();
+        let mut km = KeyManager::<RistrettoSecretKey, Blake256>::new();
         let next_key1_result = km.next_key();
         let next_key2_result = km.next_key();
         let desired_key_index1 = 1;
@@ -161,7 +168,7 @@ mod test {
 
     #[test]
     fn test_derive_and_next_key_with_branch_seed() {
-        let mut km = KeyManager::<RistrettoSecretKey, Sha256>::from(CipherSeed::new(), "Test".to_string(), 0);
+        let mut km = KeyManager::<RistrettoSecretKey, Blake256>::from(CipherSeed::new(), "Test".to_string(), 0);
         let next_key1_result = km.next_key();
         let next_key2_result = km.next_key();
         let desired_key_index1 = 1;
@@ -182,8 +189,8 @@ mod test {
     #[test]
     fn test_use_of_branch_seed() {
         let x = CipherSeed::new();
-        let mut km1 = KeyManager::<RistrettoSecretKey, Sha256>::from(x.clone(), "some".to_string(), 0);
-        let mut km2 = KeyManager::<RistrettoSecretKey, Sha256>::from(x, "other".to_string(), 0);
+        let mut km1 = KeyManager::<RistrettoSecretKey, Blake256>::from(x.clone(), "some".to_string(), 0);
+        let mut km2 = KeyManager::<RistrettoSecretKey, Blake256>::from(x, "other".to_string(), 0);
         let next_key1 = km1.next_key().unwrap();
         let next_key2 = km2.next_key().unwrap();
         assert_ne!(next_key1.k, next_key2.k);

base_layer/key_manager/src/lib.rs

@@ -11,3 +11,25 @@ pub mod mnemonic_wordlists;
 #[allow(clippy::unused_unit)]
 #[cfg(feature = "wasm")]
 pub mod wasm;
+
+use tari_common::hashing_domain::*;
+
+/// The key manager domain separated hashing domain
+/// Usage:
+///   let hash = comms_dht_hash_domain().digest::<Blake256>(b"my secret");
+///   etc.
+pub fn base_layer_key_manager_mac_generation() -> HashingDomain {
+    HashingDomain::new("base_layer.key_manager.cipher_seed.mac_generation")
+}
+
+pub fn base_layer_key_manager_argon2_encoding() -> HashingDomain {
+    HashingDomain::new("base_layer.key_manager.cipher_seed.argon2_encoding")
+}
+
+pub fn base_layer_key_manager_chacha20_encoding() -> HashingDomain {
+    HashingDomain::new("base_layer.key_manager.cipher_seed.chacha20_encoding")
+}
+
+pub fn base_layer_key_manager() -> HashingDomain {
+    HashingDomain::new("base_layer.key_manager.key_manager")
+}

base_layer/key_manager/src/wasm.rs

@@ -193,7 +193,7 @@ mod test {
         let next_key = response.key_manager.next_key().unwrap();
         assert_eq!(
             next_key.k.to_hex(),
-            "5c06999ed20e18bbb76245826141f8ae8700a648d87ec4da5a2a7507ce4b5f0e".to_string()
+            "07d6c715bca8d83c0eb6d29d01a4b3e123aefabfb2c7ac875b22c22e22df660a".to_string()
         )
     }