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

jorgeantonio21 · 2022-07-22T14:54:10Z

Description

Add use of hashing API to create challenge for mac origin.

Motivation and Context

Tackle issue.

How Has This Been Tested?

Existing unit tests.

AaronFeickert · 2022-07-22T15:05:59Z

comms/dht/src/inbound/decryption.rs

@@ -248,7 +248,7 @@ where S: Service<DecryptedDhtMessage, Response = (), Error = PipelineError>
            Ok(origin_mac) => {
                // If this fails, discard the message because we decrypted and deserialized the message with our shared
                // ECDH secret but the message could not be authenticated
-                let msg_challenge = crypt::create_message_challenge(&message.dht_header, &message.body);
+                let msg_challenge = crypt::create_message_domain_separated_hash(&message.dht_header, &message.body);


Recommend renaming to be consistent with the rest of the PR.

Something like binding_message_representation might be nicely expressive if used consistently.

I renamed these variables, as you suggested, as well as the terminology OriginMac/origin_mac

hansieodendaal

Looking good, please see suggestions below.

hansieodendaal · 2022-07-25T08:34:33Z

comms/dht/src/crypt.rs

@@ -43,6 +43,8 @@ use crate::{
    version::DhtProtocolVersion,
 };

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


Suggested change

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

Please use comms_dht_hash_domain()

hansieodendaal · 2022-07-25T08:35:43Z

comms/dht/src/crypt.rs

+    // we digest the given data into a domain independent hash function to produce a signature
+    // use of the hashing API for domain separation and deal with variable length input
+    let domain_separated_hash =
+        DomainSeparatedHasher::<Challenge, GenericHashDomain>::new(DOMAIN_SEPARATION_CHALLENGE_LABEL)


Suggested change

DomainSeparatedHasher::<Challenge, GenericHashDomain>::new(DOMAIN_SEPARATION_CHALLENGE_LABEL)

comms_dht_hash_domain().mac_hasher::<Challenge>()

Note that mac_hasher can also be used, which implements the LengthExtensionAttackResistant trait.

hansieodendaal · 2022-07-25T08:38:32Z

comms/dht/src/crypt.rs

+            .finalize()
+            .into_vec();
+
+    let mut output = [0u8; 32];
+    // the use of Challenge bind to Blake256, should always produce a 32-byte length output data
+    output.copy_from_slice(domain_separated_hash.as_slice());
+    output


Suggested change

.finalize()

.into_vec();

let mut output = [0u8; 32];

// the use of Challenge bind to Blake256, should always produce a 32-byte length output data

output.copy_from_slice(domain_separated_hash.as_slice());

output

.finalize();

domain_separated_hash.hash_to_bytes()

.expect("Fixed output of '0 < size <= 32' bytes with 32 byte hasher cannot fail.")

This way the runtime panic is not hidden in copy_from_slice. As an alternative, the function could return a result and the runtime error could be handled.

hansieodendaal · 2022-07-25T08:56:20Z

comms/dht/src/crypt.rs

@@ -102,7 +104,7 @@ pub fn create_message_challenge(header: &DhtMessageHeader, body: &[u8]) -> [u8;
 }

 /// Generates a 32-byte hashed challenge that commits to all message parts
-pub fn create_message_challenge_parts(
+pub fn create_message_domain_separated_hash_parts(


Suggested change

pub fn create_message_domain_separated_hash_parts(

pub fn create_message_hash(

The previous name conveys the intent well enough, but the addition of _parts is confusing, so create_message_challenge could also work.

hansieodendaal · 2022-07-25T09:15:18Z

comms/dht/src/dedup/mod.rs

-pub fn create_message_hash(origin_mac: &[u8], body: &[u8]) -> [u8; 32] {
-    Challenge::new().chain(origin_mac).chain(&body).finalize().into()
+pub fn create_message_hash(message_signature: &[u8], body: &[u8]) -> [u8; 32] {
+    Challenge::new().chain(message_signature).chain(&body).finalize().into()


Suggested change

Challenge::new().chain(message_signature).chain(&body).finalize().into()

comms_dht_hash_domain()

.mac_hasher::<Challenge>()

.chain(message_signature)

.chain(&body)

.finalize()

.hash_to_bytes()

.expect("Fixed output of '0 < size <= 32' bytes with 32 byte hasher cannot fail.")

hansieodendaal · 2022-07-25T09:52:25Z

comms/dht/src/message_signature.rs

    signer_public_key: CommsPublicKey,
    signature: Signature,
 }

-fn construct_origin_mac_hash(
+fn construct_message_signature_hash(


This function should also implement comms_dht_hash_domain() as

pub fn create_message_hash(message_signature: &[u8], body: &[u8]) -> [u8; 32] { comms_dht_hash_domain() .hasher::<Challenge>() .chain(message_signature) .chain(&body) .finalize() .hash_to_bytes() .expect("Fixed output of '0 < size <= 32' bytes with as 32 byte hasher cannot fail.") }

jorgeantonio21 · 2022-07-28T12:42:04Z

This PR has been addressed by this.

jorgeantonio21 added 2 commits July 22, 2022 15:47

address domain separation regarding challenge generation for mac origin

0b17607

run cargo fmt

da661f8

AaronFeickert reviewed Jul 22, 2022

View reviewed changes

jorgeantonio21 added 4 commits July 22, 2022 19:30

renaming

b6e29d5

run cargo fmt

95a9948

rename origin_mac to message_signature

20b9323

run cargo fmt

58c5e5a

hansieodendaal requested changes Jul 25, 2022

View reviewed changes

jorgeantonio21 closed this Jul 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jorgeantonio21 commented Jul 22, 2022 •

edited by sdbondi

Loading

AaronFeickert Jul 22, 2022

AaronFeickert Jul 22, 2022

jorgeantonio21 Jul 24, 2022

hansieodendaal left a comment

hansieodendaal Jul 25, 2022

hansieodendaal Jul 25, 2022

hansieodendaal Jul 25, 2022

hansieodendaal Jul 25, 2022

hansieodendaal Jul 25, 2022

hansieodendaal Jul 25, 2022

jorgeantonio21 commented Jul 28, 2022 •

edited

Loading

	DomainSeparatedHasher::<Challenge, GenericHashDomain>::new(DOMAIN_SEPARATION_CHALLENGE_LABEL)
	comms_dht_hash_domain().mac_hasher::<Challenge>()

	pub fn create_message_domain_separated_hash_parts(
	pub fn create_message_hash(

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

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

Conversation

jorgeantonio21 commented Jul 22, 2022 • edited by sdbondi Loading

Description

Motivation and Context

How Has This Been Tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hansieodendaal left a 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

jorgeantonio21 commented Jul 28, 2022 • edited Loading

jorgeantonio21 commented Jul 22, 2022 •

edited by sdbondi

Loading

jorgeantonio21 commented Jul 28, 2022 •

edited

Loading