Skip to content

Commit

Permalink
fix(comms)!: use domain hasher for noise DH key derivation (#4432)
Browse files Browse the repository at this point in the history
Description
---
Hashes the noise DH shared secret  

Motivation and Context
---
All DH shared secrets should be hashed to remove any possible slight bias in the resulting key.

Ref #4393 

How Has This Been Tested?
---
Unit test updated. Existing tests pass
  • Loading branch information
sdbondi authored Aug 10, 2022
1 parent fe69e03 commit c93182c
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 5 deletions.
18 changes: 13 additions & 5 deletions comms/core/src/noise/crypto_resolver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,18 +20,21 @@
// 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::Digest;
use rand::rngs::OsRng;
use snow::{
params::{CipherChoice, DHChoice, HashChoice},
resolvers::{CryptoResolver, DefaultResolver},
types::{Cipher, Dh, Hash, Random},
};
use tari_crypto::{
hash::blake2::Blake256,
hashing::DomainSeparatedHasher,
keys::{DiffieHellmanSharedSecret, PublicKey, SecretKey},
tari_utilities::ByteArray,
};

use crate::types::{CommsPublicKey, CommsSecretKey};
use crate::types::{CommsCoreHashDomain, CommsPublicKey, CommsSecretKey};

macro_rules! copy_slice {
($inslice:expr, $outslice:expr) => {
Expand Down Expand Up @@ -63,6 +66,11 @@ impl CryptoResolver for TariCryptoResolver {
}
}

fn noise_kdf(shared_key: &CommsPublicKey) -> [u8; 32] {
let hasher = DomainSeparatedHasher::<Blake256, CommsCoreHashDomain>::new_with_label("noise.dh");
Digest::finalize(hasher.chain(shared_key.as_bytes())).into()
}

#[derive(Default)]
struct CommsDiffieHellman {
secret_key: CommsSecretKey,
Expand Down Expand Up @@ -107,8 +115,8 @@ impl Dh for CommsDiffieHellman {
fn dh(&self, public_key: &[u8], out: &mut [u8]) -> Result<(), ()> {
let pk = CommsPublicKey::from_bytes(&public_key[..self.pub_len()]).map_err(|_| ())?;
let shared = CommsPublicKey::shared_secret(&self.secret_key, &pk);
let shared_bytes = shared.as_bytes();
copy_slice!(shared_bytes, out);
let hash = noise_kdf(&shared);
copy_slice!(hash, out);
Ok(())
}
}
Expand Down Expand Up @@ -149,11 +157,11 @@ mod test {

let (secret_key2, public_key2) = CommsPublicKey::random_keypair(&mut OsRng);
let expected_shared = CommsPublicKey::shared_secret(&secret_key2, &public_key);
let expected_shared = noise_kdf(&expected_shared);

let mut out = [0; 32];
dh.dh(public_key2.as_bytes(), &mut out).unwrap();
let shared = CommsPublicKey::from_bytes(&out).unwrap();

assert_eq!(shared, expected_shared);
assert_eq!(out, expected_shared);
}
}
3 changes: 3 additions & 0 deletions comms/core/src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
use tari_crypto::{
hash::blake2::Blake256,
hash_domain,
keys::PublicKey,
ristretto::RistrettoPublicKey,
signatures::SchnorrSignature,
Expand Down Expand Up @@ -55,3 +56,5 @@ pub type CommsDataStore = LMDBStore;
pub type CommsDatabase = LMDBWrapper<PeerId, Peer>;
#[cfg(test)]
pub type CommsDatabase = HashmapDatabase<PeerId, Peer>;

hash_domain!(CommsCoreHashDomain, "com.tari.comms.core", 0);

0 comments on commit c93182c

Please sign in to comment.