fix: address points in issue #4138 and companions #4336

jorgeantonio21 · 2022-07-22T13:30:28Z

Description
--- Fix issues #4138, #4333 and #4170.

Motivation and Context
--- Add domain separation and cipher authentication for out bound messaging

How Has This Been Tested?
--- Unit tests.

AaronFeickert · 2022-07-22T15:12:30Z

comms/dht/src/crypt.rs

+const DOMAIN_SEPARATION_CHALLENGE_LABEL: &str = "com.tari.comms.dht.crypt.challenge";
+const DOMAIN_SEPARATION_KEY_MESSAGE_LABEL: &str = "com.tari.comms.dht.crypt.challenge";


Domain separators must be unique.

Definitely, changed !

AaronFeickert · 2022-07-22T15:13:21Z

comms/dht/src/crypt.rs


 /// Generates a Diffie-Hellman secret `kx.G` as a `chacha20::Key` given secret scalar `k` and public key `P = x.G`.
-pub fn generate_ecdh_secret<PK>(secret_key: &PK::K, public_key: &PK) -> CipherKey
+pub fn generate_ecdh_secret<PK>(secret_key: &PK::K, public_key: &PK) -> [u8; 32]


This encoded output size is specific to the elliptic curve group used, and does not hold generically.

AaronFeickert · 2022-07-22T15:14:00Z

comms/dht/src/crypt.rs

 where PK: PublicKey + DiffieHellmanSharedSecret<PK = PK> {
    // TODO: PK will still leave the secret in released memory. Implementing Zerioze on RistrettoPublicKey is not
    //       currently possible because (Compressed)RistrettoPoint does not implement it.
    let k = PK::shared_secret(secret_key, public_key);
-    CipherKey(*Key::from_slice(k.as_bytes()))
+    let mut output = [0u8; 32];


This encoded size is specific to the elliptic curve group used, and is not generic.

It might be useful to define a generic DiffieHellmanSharedSecret type that holds the result of an ECDH exchange.

@jorgeantonio21 Would like to have the well-defined DH return type, perhaps for a follow-up PR?

AaronFeickert · 2022-07-22T15:15:21Z

comms/dht/src/crypt.rs

+            .finalize()
+            .into_vec();
+
+    // Domain separation uses Challenge = Blake256, thus its output has 32-byte length


It is possible to check and enforce this at compile time, in case later changes subtly break this?

The use of a specific ECDH shared secret type would mean this function enforces this input type.

AaronFeickert · 2022-07-22T15:15:35Z

comms/dht/src/crypt.rs

+            .finalize()
+            .into_vec();
+
+    // Domain separation uses Challenge = Blake256, thus its output has 32-byte length


Is it possible to check and enforce this at compile time, in case later changes subtly break this?

The use of a specific ECDH shared secret type would mean this function enforces this input type.

It is possible to check and enforce this at compile time, in case later changes subtly break this?

I believe so. Noting that this will not really break sublty and will panic in Key::from_slice if not 32-bytes. There are many tests that would signal this breakage, so we can decide if the following is worthwhile.

digest::Output<D> returned from finalize implements Into<[u8;32]> for 32-byte outputs - this could be used to generate a compile error if the DomainSeparatedHash<D> type exposed the inner output type e.g into_inner(self) -> Output<D>

let domain_separated_hash = DomainSeparatedHasher::<Blake256, GenericHashDomain>::new(DOMAIN_SEPARATION_KEY_SIGNATURE_LABEL) .chain(data) .finalize(); // Will not compile if Into<[u8;32]> is not implemented. let output_array : [u8;32] = domain_separated_hash.into_inner().into();

This requires adding the into_inner function or reimplementing (something like) From<[u8;D::OutputSize::to_usize()]> on DomainSeparatedHash.

@sdbondi How generic can that be made? It may be the case that other domain-separated hash function applications require output of a different length.

@AaronFeickert That is handled by the generic_array crate. Here, we are enforcing in this function (or else compile fails) that the hash output is correctly convertible to a 32-byte array representation.

Adding trait bounds are not necessary here as we use concrete types, but the trait bounds would be where digest::Output<D>: Into<[u8;32]>

This became possible with this change (tari-project/tari-crypto#119) since we can now use digest::Output<D>

let domain_separated_hash = DomainSeparatedHasher::<Blake256, GenericHashDomain>::new(DOMAIN_SEPARATION_KEY_SIGNATURE_LABEL) .chain(data) .finalize(); // Will not compile if Into<[u8;32]> is not implemented. let output_array : [u8;32] = domain_separated_hash.into();

So as the tari_crypto domain hasher code currently stands, you'd need to disambiguate between the functions on the hasher type and the impl Digest functions with the same name.
e.g.

let hasher = comms_dht_hash_domain_key_signature().chain(data); let domain_separated_hash =<DomainSeparatedHasher<CommsChallenge, DHTCommsHashDomain> as Digest>::finalize(hasher); let hash: [u8; 32] = domain_separated_hash.into();

Since we know that we're using Blake256 and breakage will be caught in tests - I think we can leave this as is

AaronFeickert · 2022-07-22T15:17:50Z

comms/dht/src/crypt.rs

+/// Produces authenticated encryption of the signature using the ChaCha20-Poly1305 stream cipher,
+/// refer to https://docs.rs/chacha20poly1305/latest/chacha20poly1305/# for more details.
+/// Attention: as pointed in https://github.com/tari-project/tari/issues/4138, it is possible
+/// to use a fixed length Nonce, with homogeneous zero data, as this does not incur any security


Not a fixed-length nonce, but a fixed nonce.

Definitely ! Thank you !

…signatures

AaronFeickert · 2022-07-22T18:33:09Z

comms/dht/src/crypt.rs

+            .map_err(|_| DhtOutboundError::CipherError(String::from("Authenticated encryption failed")))
+            .unwrap();
+
+        assert_eq!(encrypted.len(), n + 16);


You could remove the magic number by getting the tag size programmatically.

sdbondi

Ack

Tested with memorynet

jorgeantonio21 added 5 commits July 22, 2022 14:29

address points in issue tari-project#4138 and companions

2f10996

Merge branch 'development' into ja-derived-keys

a103aba

correct bugs in chacha20poly1305 utilization

33da112

address domain separation regarding challenge generation for mac origin

0b17607

run cargo fmt

da661f8

AaronFeickert mentioned this pull request Jul 22, 2022

fix: hash echd secret #4334

Closed

AaronFeickert requested changes Jul 22, 2022

View reviewed changes

AaronFeickert mentioned this pull request Jul 22, 2022

Examine peer banning logic #4339

Closed

add tests for failure modes of new authentication encryption for key …

aee19eb

…signatures

jorgeantonio21 changed the title ~~Draft: address points in issue #4138 and companions~~ fix: address points in issue #4138 and companions Jul 22, 2022

jorgeantonio21 added 2 commits July 22, 2022 19:30

renaming

b6e29d5

run cargo fmt

95a9948

AaronFeickert reviewed Jul 22, 2022

View reviewed changes

jorgeantonio21 added 15 commits July 22, 2022 19:39

clippy: too many lines

ed61369

rename origin_mac to message_signature

20b9323

run cargo fmt

58c5e5a

add output type for ecdh exchange

cc743e3

add new use of hashing API

5600252

merge message-challenge-with-api branch

c2bb431

add changes

2fa7515

add generic constant length array size for generate_ecdh_secret method

27f1ab6

add minor changes

ea94992

Merge branch 'development' into ja-derived-keys

c3a1d0a

update tari-crypto tag version on cargo.toml

52d8e8d

merge development

0801a20

refactor to use of hash_domain! macro

065620a

remove generic from ecdh excahnge method

c06b98c

Merge branch 'development' into ja-derived-keys

c13ace2

jorgeantonio21 marked this pull request as ready for review July 28, 2022 11:02

sdbondi approved these changes Jul 28, 2022

View reviewed changes

stringhandler added the mq-approved label Jul 28, 2022

Merge branch 'development' into ja-derived-keys

b89f8af

aviator-app bot merged commit 2ca0672 into tari-project:development Jul 28, 2022

jorgeantonio21 mentioned this pull request Jul 28, 2022

fix: address domain separation regarding challenge generation for mac origin (see issue #4333) #4338

Closed

sdbondi mentioned this pull request Jul 29, 2022

Message signature ("origin MAC") challenge should use hashing API #4332

Closed

SWvheerden mentioned this pull request Nov 24, 2022

Message decryption relies (in part) on deserialization for validity #4138

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: address points in issue #4138 and companions #4336

fix: address points in issue #4138 and companions #4336

jorgeantonio21 commented Jul 22, 2022 •

edited

Loading

AaronFeickert Jul 22, 2022

jorgeantonio21 Jul 24, 2022

AaronFeickert Jul 22, 2022

AaronFeickert Jul 22, 2022

AaronFeickert Jul 22, 2022

sdbondi Jul 28, 2022

AaronFeickert Jul 22, 2022

AaronFeickert Jul 22, 2022

AaronFeickert Jul 22, 2022 •

edited

Loading

AaronFeickert Jul 22, 2022

sdbondi Jul 26, 2022

AaronFeickert Jul 26, 2022

sdbondi Jul 28, 2022 •

edited

Loading

sdbondi Jul 28, 2022

AaronFeickert Jul 22, 2022

jorgeantonio21 Jul 24, 2022

AaronFeickert Jul 22, 2022

sdbondi left a comment

		const DOMAIN_SEPARATION_CHALLENGE_LABEL: &str = "com.tari.comms.dht.crypt.challenge";
		const DOMAIN_SEPARATION_KEY_MESSAGE_LABEL: &str = "com.tari.comms.dht.crypt.challenge";

fix: address points in issue #4138 and companions #4336

fix: address points in issue #4138 and companions #4336

Conversation

jorgeantonio21 commented Jul 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AaronFeickert Jul 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdbondi Jul 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdbondi left a comment

Choose a reason for hiding this comment

jorgeantonio21 commented Jul 22, 2022 •

edited

Loading

AaronFeickert Jul 22, 2022 •

edited

Loading

sdbondi Jul 28, 2022 •

edited

Loading