Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion base_layer/p2p/src/services/liveness/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -486,7 +486,7 @@ mod test {
dht_header: DhtMessageHeader {
version: DhtProtocolVersion::latest(),
destination: Default::default(),
origin_mac: Vec::new(),
message_signature: Vec::new(),
ephemeral_public_key: None,
message_type: DhtMessageType::None,
flags: Default::default(),
Expand Down
2 changes: 1 addition & 1 deletion base_layer/p2p/src/test_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ pub fn make_dht_header(trace: MessageTag) -> DhtMessageHeader {
DhtMessageHeader {
version: DhtProtocolVersion::latest(),
destination: NodeDestination::Unknown,
origin_mac: Vec::new(),
message_signature: Vec::new(),
ephemeral_public_key: None,
message_type: DhtMessageType::None,
flags: DhtMessageFlags::NONE,
Expand Down
2 changes: 1 addition & 1 deletion base_layer/wallet/tests/support/comms_and_services.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ pub fn create_dummy_message<T>(inner: T, public_key: &CommsPublicKey) -> DomainM
dht_header: DhtMessageHeader {
version: DhtProtocolVersion::latest(),
ephemeral_public_key: None,
origin_mac: Vec::new(),
message_signature: Vec::new(),
message_type: Default::default(),
flags: Default::default(),
destination: Default::default(),
Expand Down
39 changes: 26 additions & 13 deletions comms/dht/src/crypt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,10 @@ use chacha20::{
Key,
Nonce,
};
use digest::{Digest, FixedOutput};
use rand::{rngs::OsRng, RngCore};
use tari_comms::types::{Challenge, CommsPublicKey};
use tari_crypto::{
hashing::{DomainSeparatedHasher, GenericHashDomain},
keys::{DiffieHellmanSharedSecret, PublicKey},
tari_utilities::{epoch_time::EpochTime, ByteArray},
};
Expand All @@ -43,6 +43,8 @@ use crate::{
version::DhtProtocolVersion,
};

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Please use comms_dht_hash_domain()


#[derive(Debug, Clone, Zeroize)]
#[zeroize(drop)]
pub struct CipherKey(chacha20::Key);
Expand Down Expand Up @@ -89,8 +91,8 @@ pub fn encrypt(cipher_key: &CipherKey, plain_text: &[u8]) -> Vec<u8> {
}

/// Generates a 32-byte hashed challenge that commits to the message header and body
pub fn create_message_challenge(header: &DhtMessageHeader, body: &[u8]) -> [u8; 32] {
create_message_challenge_parts(
pub fn create_message_domain_separated_hash(header: &DhtMessageHeader, body: &[u8]) -> [u8; 32] {
create_message_domain_separated_hash_parts(
header.version,
&header.destination,
header.message_type,
Expand All @@ -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(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

protocol_version: DhtProtocolVersion,
destination: &NodeDestination,
message_type: DhtMessageType,
Expand All @@ -111,14 +113,10 @@ pub fn create_message_challenge_parts(
ephemeral_public_key: Option<&CommsPublicKey>,
body: &[u8],
) -> [u8; 32] {
let mut mac_challenge = Challenge::new();
mac_challenge.update(&protocol_version.as_bytes());
mac_challenge.update(destination.to_inner_bytes());
mac_challenge.update(&(message_type as i32).to_le_bytes());
mac_challenge.update(&flags.bits().to_le_bytes());
// get byte representation of `expires` input
let expires = expires.map(|t| t.as_u64().to_le_bytes()).unwrap_or_default();
mac_challenge.update(&expires);

// get byte representation of `ephemeral_public_key`
let e_pk = ephemeral_public_key
.map(|e_pk| {
let mut buf = [0u8; 32];
Expand All @@ -127,10 +125,25 @@ pub fn create_message_challenge_parts(
buf
})
.unwrap_or_default();
mac_challenge.update(&e_pk);

mac_challenge.update(&body);
mac_challenge.finalize_fixed().into()
// 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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

.chain(&protocol_version.as_bytes())
.chain(destination.to_inner_bytes())
.chain(&(message_type as i32).to_le_bytes())
.chain(&flags.bits().to_le_bytes())
.chain(&expires)
.chain(&e_pk)
.chain(&body)
.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
Comment on lines +140 to +146
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

#[cfg(test)]
Expand Down
6 changes: 3 additions & 3 deletions comms/dht/src/dedup/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,11 @@ use crate::{
const LOG_TARGET: &str = "comms::dht::dedup";

pub fn hash_inbound_message(msg: &DhtInboundMessage) -> [u8; 32] {
create_message_hash(&msg.dht_header.origin_mac, &msg.body)
create_message_hash(&msg.dht_header.message_signature, &msg.body)
}

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()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.")

}

/// # DHT Deduplication middleware
Expand Down
6 changes: 3 additions & 3 deletions comms/dht/src/dht.rs
Original file line number Diff line number Diff line change
Expand Up @@ -606,8 +606,8 @@ mod test {
true,
);

let origin_mac = dht_envelope.header.as_ref().unwrap().origin_mac.clone();
assert!(!origin_mac.is_empty());
let message_signature = dht_envelope.header.as_ref().unwrap().message_signature.clone();
assert!(!message_signature.is_empty());
let inbound_message = make_comms_inbound_message(&node_identity, dht_envelope.to_encoded_bytes().into());

service.call(inbound_message).await.unwrap();
Expand All @@ -616,7 +616,7 @@ mod test {
let (params, _) = oms_mock_state.pop_call().await.unwrap();

// Check that OMS got a request to forward with the original Dht Header
assert_eq!(params.dht_header.unwrap().origin_mac, origin_mac);
assert_eq!(params.dht_header.unwrap().message_signature, message_signature);

// Check the next service was not called
assert_eq!(spy.call_count(), 0);
Expand Down
14 changes: 7 additions & 7 deletions comms/dht/src/envelope.rs
Original file line number Diff line number Diff line change
Expand Up @@ -140,9 +140,9 @@ impl DhtMessageType {
pub struct DhtMessageHeader {
pub version: DhtProtocolVersion,
pub destination: NodeDestination,
/// Encoded OriginMac. Depending on message flags, this may be encrypted. This can refer to the same peer that sent
/// the message or another peer if the message is being propagated.
pub origin_mac: Vec<u8>,
/// Encoded MessageSignature. Depending on message flags, this may be encrypted. This can refer to the same peer
/// that sent the message or another peer if the message is being propagated.
pub message_signature: Vec<u8>,
pub ephemeral_public_key: Option<CommsPublicKey>,
pub message_type: DhtMessageType,
pub flags: DhtMessageFlags,
Expand All @@ -153,7 +153,7 @@ pub struct DhtMessageHeader {
impl DhtMessageHeader {
pub fn is_valid(&self) -> bool {
if self.flags.is_encrypted() {
!self.origin_mac.is_empty() && self.ephemeral_public_key.is_some()
!self.message_signature.is_empty() && self.ephemeral_public_key.is_some()
} else {
true
}
Expand All @@ -165,7 +165,7 @@ impl PartialEq for DhtMessageHeader {
fn eq(&self, other: &Self) -> bool {
self.version == other.version &&
self.destination == other.destination &&
self.origin_mac == other.origin_mac &&
self.message_signature == other.message_signature &&
self.ephemeral_public_key == other.ephemeral_public_key &&
self.message_type == other.message_type &&
self.flags == other.flags &&
Expand Down Expand Up @@ -207,7 +207,7 @@ impl TryFrom<DhtHeader> for DhtMessageHeader {
Ok(Self {
version,
destination,
origin_mac: header.origin_mac,
message_signature: header.message_signature,
ephemeral_public_key,
message_type: DhtMessageType::from_i32(header.message_type).ok_or(DhtMessageError::InvalidMessageType)?,
flags: DhtMessageFlags::from_bits(header.flags).ok_or(DhtMessageError::InvalidMessageFlags)?,
Expand Down Expand Up @@ -238,7 +238,7 @@ impl From<DhtMessageHeader> for DhtHeader {
.as_ref()
.map(ByteArray::to_vec)
.unwrap_or_else(Vec::new),
origin_mac: header.origin_mac,
message_signature: header.message_signature,
destination: Some(header.destination.into()),
message_type: header.message_type as i32,
flags: header.flags.bits(),
Expand Down
76 changes: 41 additions & 35 deletions comms/dht/src/inbound/decryption.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ use crate::{
crypt::CipherKey,
envelope::DhtMessageHeader,
inbound::message::{DecryptedDhtMessage, DhtInboundMessage},
origin_mac::{OriginMac, OriginMacError, ProtoOriginMac},
message_signature::{MessageSignature, MessageSignatureError, ProtoMessageSignature},
DhtConfig,
};

Expand All @@ -48,15 +48,15 @@ const LOG_TARGET: &str = "comms::middleware::decryption";
#[derive(Error, Debug)]
enum DecryptionError {
#[error("Failed to validate origin MAC signature")]
OriginMacInvalidSignature,
MessageSignatureInvalidSignature,
#[error("Origin MAC not provided for encrypted message")]
OriginMacNotProvided,
MessageSignatureNotProvided,
#[error("Failed to decrypt origin MAC")]
OriginMacDecryptedFailed,
MessageSignatureDecryptedFailed,
#[error("Failed to decode clear-text origin MAC")]
OriginMacClearTextDecodeFailed,
MessageSignatureClearTextDecodeFailed,
#[error("Origin MAC error: {0}")]
OriginMacError(#[from] OriginMacError),
MessageSignatureError(#[from] MessageSignatureError),
#[error("Ephemeral public key not provided for encrypted message")]
EphemeralKeyNotProvided,
#[error("Message rejected because this node could not decrypt a message that was addressed to it")]
Expand Down Expand Up @@ -169,11 +169,11 @@ where S: Service<DecryptedDhtMessage, Response = (), Error = PipelineError>
next_service.oneshot(msg).await
},

Err(err @ OriginMacNotProvided) |
Err(err @ MessageSignatureNotProvided) |
Err(err @ EphemeralKeyNotProvided) |
Err(err @ EncryptedMessageNoDestination) |
Err(err @ OriginMacInvalidSignature) |
Err(err @ OriginMacError(_)) => {
Err(err @ MessageSignatureInvalidSignature) |
Err(err @ MessageSignatureError(_)) => {
// This message should not have been propagated, or has been manipulated in some way. Ban the source of
// this message.
connectivity
Expand Down Expand Up @@ -244,13 +244,14 @@ where S: Service<DecryptedDhtMessage, Response = (), Error = PipelineError>
let shared_secret = crypt::generate_ecdh_secret(node_identity.secret_key(), e_pk);

// Decrypt and verify the origin
let authenticated_origin = match Self::attempt_decrypt_origin_mac(&shared_secret, dht_header) {
Ok(origin_mac) => {
let authenticated_origin = match Self::attempt_decrypt_message_signature(&shared_secret, dht_header) {
Ok(message_signature) => {
// 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);
Self::authenticate_origin_mac(&origin_mac, &msg_challenge)?;
origin_mac.into_signer_public_key()
let binding_message_representation =
crypt::create_message_domain_separated_hash(&message.dht_header, &message.body);
Self::authenticate_message_signature(&message_signature, &binding_message_representation)?;
message_signature.into_signer_public_key()
},
Err(err) => {
trace!(
Expand All @@ -269,7 +270,7 @@ where S: Service<DecryptedDhtMessage, Response = (), Error = PipelineError>
message.tag,
message.dht_header.message_tag
);
return Err(DecryptionError::OriginMacDecryptedFailed);
return Err(DecryptionError::MessageSignatureDecryptedFailed);
}
return Ok(DecryptedDhtMessage::failed(message));
},
Expand Down Expand Up @@ -317,29 +318,32 @@ where S: Service<DecryptedDhtMessage, Response = (), Error = PipelineError>
}
}

fn attempt_decrypt_origin_mac(
fn attempt_decrypt_message_signature(
shared_secret: &CipherKey,
dht_header: &DhtMessageHeader,
) -> Result<OriginMac, DecryptionError> {
let encrypted_origin_mac = Some(&dht_header.origin_mac)
) -> Result<MessageSignature, DecryptionError> {
let encrypted_message_signature = Some(&dht_header.message_signature)
.filter(|b| !b.is_empty())
// This should not have been sent/propagated
.ok_or( DecryptionError::OriginMacNotProvided)?;
.ok_or( DecryptionError::MessageSignatureNotProvided)?;

let decrypted_bytes = crypt::decrypt(shared_secret, encrypted_origin_mac)
.map_err(|_| DecryptionError::OriginMacDecryptedFailed)?;
let origin_mac = ProtoOriginMac::decode(decrypted_bytes.as_slice())
.map_err(|_| DecryptionError::OriginMacDecryptedFailed)?;
let decrypted_bytes = crypt::decrypt(shared_secret, encrypted_message_signature)
.map_err(|_| DecryptionError::MessageSignatureDecryptedFailed)?;
let message_signature = ProtoMessageSignature::decode(decrypted_bytes.as_slice())
.map_err(|_| DecryptionError::MessageSignatureDecryptedFailed)?;

let origin_mac = origin_mac.try_into()?;
Ok(origin_mac)
let message_signature = message_signature.try_into()?;
Ok(message_signature)
}

fn authenticate_origin_mac(origin_mac: &OriginMac, message: &[u8]) -> Result<(), DecryptionError> {
if origin_mac.verify(message) {
fn authenticate_message_signature(
message_signature: &MessageSignature,
message: &[u8],
) -> Result<(), DecryptionError> {
if message_signature.verify(message) {
Ok(())
} else {
Err(DecryptionError::OriginMacInvalidSignature)
Err(DecryptionError::MessageSignatureInvalidSignature)
}
}

Expand Down Expand Up @@ -378,17 +382,19 @@ where S: Service<DecryptedDhtMessage, Response = (), Error = PipelineError>
}

async fn success_not_encrypted(message: DhtInboundMessage) -> Result<DecryptedDhtMessage, DecryptionError> {
let authenticated_pk = if message.dht_header.origin_mac.is_empty() {
let authenticated_pk = if message.dht_header.message_signature.is_empty() {
None
} else {
let origin_mac: OriginMac = ProtoOriginMac::decode(message.dht_header.origin_mac.as_slice())
.map_err(|_| DecryptionError::OriginMacClearTextDecodeFailed)?
.try_into()?;
let message_signature: MessageSignature =
ProtoMessageSignature::decode(message.dht_header.message_signature.as_slice())
.map_err(|_| DecryptionError::MessageSignatureClearTextDecodeFailed)?
.try_into()?;

let mac_challenge = crypt::create_message_challenge(&message.dht_header, &message.body);
let binding_message_representation =
crypt::create_message_domain_separated_hash(&message.dht_header, &message.body);

Self::authenticate_origin_mac(&origin_mac, &mac_challenge)?;
Some(origin_mac.into_signer_public_key())
Self::authenticate_message_signature(&message_signature, &binding_message_representation)?;
Some(message_signature.into_signer_public_key())
};

match EnvelopeBody::decode(message.body.as_slice()) {
Expand Down
4 changes: 2 additions & 2 deletions comms/dht/src/inbound/message.rs
Original file line number Diff line number Diff line change
Expand Up @@ -172,8 +172,8 @@ impl DecryptedDhtMessage {
self.dht_header.flags.contains(DhtMessageFlags::ENCRYPTED)
}

pub fn has_origin_mac(&self) -> bool {
!self.dht_header.origin_mac.is_empty()
pub fn has_message_signature(&self) -> bool {
!self.dht_header.message_signature.is_empty()
}

pub fn body_len(&self) -> usize {
Expand Down
2 changes: 1 addition & 1 deletion comms/dht/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ use tari_common::hashing_domain::HashingDomain;

mod filter;
mod logging_middleware;
mod origin_mac;
mod message_signature;
mod peer_validator;
mod proto;
mod rpc;
Expand Down
Loading