From 7a2c95e9897f57abfbe5410c9e62982d52bff291 Mon Sep 17 00:00:00 2001 From: Stan Bondi Date: Wed, 24 Aug 2022 16:36:26 +0400 Subject: [PATCH] fix(comms/dht): fixes invalid peer ban on invalid encrypted msg signature (#4519) Description --- - fixes invalid ban of source peer on the encrypted message signature, which can only be validated by the sender (#4339 ) - add two additional unit tests for this case Motivation and Context --- Ref #4339 Previously, a sender may send a message with an invalid encrypted signature that cannot be validated and rejected by intermediate nodes. On receipt by the sender, an invalid signature is encountered and previously this would result in sending peer being banned. This PR changes this to only discard the message. The sender is only banned if unencrypted header data is malformed/invalid. How Has This Been Tested? --- New unit tests + existing tests + manually (no breaking changes) --- comms/dht/src/inbound/decryption.rs | 319 +++++++++++++----- comms/dht/src/inbound/message.rs | 32 ++ comms/dht/src/message_signature.rs | 24 +- .../dht/src/store_forward/saf_handler/task.rs | 2 +- comms/dht/src/store_forward/store.rs | 7 +- 5 files changed, 289 insertions(+), 95 deletions(-) diff --git a/comms/dht/src/inbound/decryption.rs b/comms/dht/src/inbound/decryption.rs index 4d961775cb..03b805c361 100644 --- a/comms/dht/src/inbound/decryption.rs +++ b/comms/dht/src/inbound/decryption.rs @@ -37,7 +37,7 @@ use tower::{layer::Layer, Service, ServiceExt}; use crate::{ crypt, envelope::DhtMessageHeader, - inbound::message::{DecryptedDhtMessage, DhtInboundMessage}, + inbound::message::{DecryptedDhtMessage, DhtInboundMessage, ValidatedDhtInboundMessage}, message_signature::{MessageSignature, MessageSignatureError, ProtoMessageSignature}, DhtConfig, }; @@ -46,20 +46,24 @@ const LOG_TARGET: &str = "comms::middleware::decryption"; #[derive(Error, Debug)] enum DecryptionError { - #[error("Failed to validate origin MAC signature")] - MessageSignatureInvalidSignature, - #[error("Origin MAC not provided for encrypted message")] - MessageSignatureNotProvided, + #[error("Failed to validate ENCRYPTED message signature")] + MessageSignatureInvalidEncryptedSignature, + #[error("Failed to validate CLEARTEXT message signature")] + MessageSignatureInvalidClearTextSignature, + #[error("Message signature not provided for encrypted message")] + MessageSignatureNotProvidedForEncryptedMessage, #[error("Failed to decrypt message signature")] MessageSignatureDecryptedFailed, #[error("Failed to deserialize message signature")] MessageSignatureDeserializedFailed, - #[error("Failed to decode clear-text origin MAC")] + #[error("Failed to decode clear-text message signature")] MessageSignatureClearTextDecodeFailed, - #[error("Origin MAC error: {0}")] - MessageSignatureError(#[from] MessageSignatureError), + #[error("Message signature error for cleartext message: {0}")] + MessageSignatureErrorClearText(MessageSignatureError), + #[error("Message signature error for encrypted message: {0}")] + MessageSignatureErrorEncrypted(MessageSignatureError), #[error("Ephemeral public key not provided for encrypted message")] - EphemeralKeyNotProvided, + EphemeralKeyNotProvidedForEncryptedMessage, #[error("Message rejected because this node could not decrypt a message that was addressed to it")] MessageRejectDecryptionFailed, #[error("Failed to decode envelope body")] @@ -169,12 +173,27 @@ where S: Service trace!(target: LOG_TARGET, "Passing onto next service (Trace: {})", msg.tag); next_service.oneshot(msg).await }, + // The peer received an invalid message signature however we cannot ban the source peer because they have no + // way to validate this + Err(err @ MessageSignatureInvalidEncryptedSignature) | Err(err @ MessageSignatureErrorEncrypted(_)) => { + warn!( + target: LOG_TARGET, + "SECURITY: {} ({}, peer={}, trace={}). Message discarded", err, tag, source.node_id, trace_id + ); + Err(err.into()) + }, - Err(err @ MessageSignatureNotProvided) | - Err(err @ EphemeralKeyNotProvided) | + // These are verifiable error cases that can be checked by every node + Err(err @ MessageSignatureNotProvidedForEncryptedMessage) | + Err(err @ EphemeralKeyNotProvidedForEncryptedMessage) | + Err(err @ MessageSignatureClearTextDecodeFailed) | + Err(err @ MessageSignatureInvalidClearTextSignature) | Err(err @ EncryptedMessageNoDestination) | - Err(err @ MessageSignatureInvalidSignature) | - Err(err @ MessageSignatureError(_)) => { + Err(err @ MessageSignatureErrorClearText(_)) => { + warn!( + target: LOG_TARGET, + "SECURITY: {} ({}, peer={}, trace={}). Message discarded", err, tag, source.node_id, trace_id + ); // This message should not have been propagated, or has been manipulated in some way. Ban the source of // this message. connectivity @@ -201,25 +220,30 @@ where S: Service node_identity: Arc, message: DhtInboundMessage, ) -> Result { - let dht_header = &message.dht_header; + let validated_msg = Self::initial_validation(message)?; - if !dht_header.flags.is_encrypted() { - return Self::success_not_encrypted(message).await; + if !validated_msg.header().flags.is_encrypted() { + return Self::success_not_encrypted(validated_msg).await; } + trace!( target: LOG_TARGET, "Decrypting message {} (Trace: {})", - message.tag, - message.dht_header.message_tag + validated_msg.message().tag, + validated_msg.message().dht_header.message_tag ); - // Check if there is no destination specified and discard - if dht_header.destination.is_unknown() { - return Err(DecryptionError::EncryptedMessageNoDestination); - } + let dht_header = validated_msg.header(); + + let e_pk = dht_header + .ephemeral_public_key + .as_ref() + // No ephemeral key with ENCRYPTED flag set + .ok_or( DecryptionError::EphemeralKeyNotProvidedForEncryptedMessage)?; - if !message.dht_header.destination.is_unknown() && - message + if !validated_msg.message().dht_header.destination.is_unknown() && + validated_msg + .message() .dht_header .destination .public_key() @@ -229,20 +253,15 @@ where S: Service debug!( target: LOG_TARGET, "Encrypted message (source={}, {}) not destined for this peer. Passing to next service (Trace: {})", - message.source_peer.node_id, - message.dht_header.message_tag, - message.tag + validated_msg.message().source_peer.node_id, + validated_msg.message().dht_header.message_tag, + validated_msg.message().tag ); - return Ok(DecryptedDhtMessage::failed(message)); + return Ok(DecryptedDhtMessage::failed(validated_msg.into_message())); } - let e_pk = dht_header - .ephemeral_public_key - .as_ref() - // No ephemeral key with ENCRYPTED flag set - .ok_or( DecryptionError::EphemeralKeyNotProvided)?; - let shared_secret = crypt::generate_ecdh_secret(node_identity.secret_key(), e_pk); + let message = validated_msg.message(); // Decrypt and verify the origin let authenticated_origin = match Self::attempt_decrypt_message_signature(&shared_secret, dht_header) { @@ -251,7 +270,10 @@ where S: Service // ECDH secret but the message could not be authenticated let binding_message_representation = crypt::create_message_domain_separated_hash(&message.dht_header, &message.body); - Self::authenticate_message_signature(&message_signature, &binding_message_representation)?; + + if !message_signature.verify(&binding_message_representation) { + return Err(DecryptionError::MessageSignatureInvalidEncryptedSignature); + } message_signature.into_signer_public_key() }, Err(err) => { @@ -273,7 +295,7 @@ where S: Service ); return Err(DecryptionError::MessageSignatureDecryptedFailed); } - return Ok(DecryptedDhtMessage::failed(message)); + return Ok(DecryptedDhtMessage::failed(validated_msg.into_message())); }, }; @@ -293,7 +315,7 @@ where S: Service Ok(DecryptedDhtMessage::succeeded( message_body, Some(authenticated_origin), - message, + validated_msg.into_message(), )) }, Err(err) => { @@ -314,11 +336,48 @@ where S: Service return Err(DecryptionError::MessageRejectDecryptionFailed); } - Ok(DecryptedDhtMessage::failed(message)) + Ok(DecryptedDhtMessage::failed(validated_msg.into_message())) }, } } + /// Performs message validation that should be performed by all nodes. If an error is encountered, the message is + /// invalid and should never have been sent. + fn initial_validation(message: DhtInboundMessage) -> Result { + if message.dht_header.flags.is_encrypted() { + // Check if there is no destination specified and discard + if message.dht_header.destination.is_unknown() { + return Err(DecryptionError::EncryptedMessageNoDestination); + } + + // No e_pk is invalid for encrypted messages + if message.dht_header.ephemeral_public_key.is_none() { + return Err(DecryptionError::EphemeralKeyNotProvidedForEncryptedMessage); + } + + Ok(ValidatedDhtInboundMessage::new(message, None)) + } else if message.dht_header.message_signature.is_empty() { + Ok(ValidatedDhtInboundMessage::new(message, None)) + } else { + let message_signature: MessageSignature = + ProtoMessageSignature::decode(message.dht_header.message_signature.as_slice()) + .map_err(|_| DecryptionError::MessageSignatureClearTextDecodeFailed)? + .try_into() + .map_err(DecryptionError::MessageSignatureErrorClearText)?; + + let binding_message_representation = + crypt::create_message_domain_separated_hash(&message.dht_header, &message.body); + + if !message_signature.verify(&binding_message_representation) { + return Err(DecryptionError::MessageSignatureInvalidClearTextSignature); + } + Ok(ValidatedDhtInboundMessage::new( + message, + Some(message_signature.into_signer_public_key()), + )) + } + } + fn attempt_decrypt_message_signature( shared_secret: &[u8], dht_header: &DhtMessageHeader, @@ -326,7 +385,7 @@ where S: Service let encrypted_message_signature = Some(&dht_header.message_signature) .filter(|b| !b.is_empty()) // This should not have been sent/propagated - .ok_or( DecryptionError::MessageSignatureNotProvided)?; + .ok_or( DecryptionError::MessageSignatureNotProvidedForEncryptedMessage)?; // obtain key signature for authenticated decrypt signature let key_signature = crypt::generate_key_signature_for_authenticated_encryption(shared_secret); @@ -335,21 +394,12 @@ where S: Service let message_signature = ProtoMessageSignature::decode(decrypted_bytes.as_slice()) .map_err(|_| DecryptionError::MessageSignatureDeserializedFailed)?; - let message_signature = message_signature.try_into()?; + let message_signature = message_signature + .try_into() + .map_err(DecryptionError::MessageSignatureErrorEncrypted)?; Ok(message_signature) } - fn authenticate_message_signature( - message_signature: &MessageSignature, - message: &[u8], - ) -> Result<(), DecryptionError> { - if message_signature.verify(message) { - Ok(()) - } else { - Err(DecryptionError::MessageSignatureInvalidSignature) - } - } - fn attempt_decrypt_message_body( shared_secret: &[u8], message_body: &[u8], @@ -385,31 +435,24 @@ where S: Service .map_err(|_| DecryptionError::MessageBodyDecryptionFailed) } - async fn success_not_encrypted(message: DhtInboundMessage) -> Result { - let authenticated_pk = if message.dht_header.message_signature.is_empty() { - None - } else { - let message_signature: MessageSignature = - ProtoMessageSignature::decode(message.dht_header.message_signature.as_slice()) - .map_err(|_| DecryptionError::MessageSignatureClearTextDecodeFailed)? - .try_into()?; - - let binding_message_representation = - crypt::create_message_domain_separated_hash(&message.dht_header, &message.body); - - Self::authenticate_message_signature(&message_signature, &binding_message_representation)?; - Some(message_signature.into_signer_public_key()) - }; - - match EnvelopeBody::decode(message.body.as_slice()) { + async fn success_not_encrypted( + validated: ValidatedDhtInboundMessage, + ) -> Result { + let authenticated_pk = validated.authenticated_origin().cloned(); + let msg = validated.message(); + match EnvelopeBody::decode(msg.body.as_slice()) { Ok(deserialized) => { trace!( target: LOG_TARGET, "Message {} is not encrypted. Passing onto next service (Trace: {})", - message.tag, - message.dht_header.message_tag + msg.tag, + msg.dht_header.message_tag ); - Ok(DecryptedDhtMessage::succeeded(deserialized, authenticated_pk, message)) + Ok(DecryptedDhtMessage::succeeded( + deserialized, + authenticated_pk, + validated.into_message(), + )) }, Err(err) => { // Message was not encrypted but failed to deserialize - immediately discard @@ -417,9 +460,9 @@ where S: Service debug!( target: LOG_TARGET, "Unable to deserialize message {}: {}. Message will be discarded. (Trace: {})", - message.tag, + msg.tag, err, - message.dht_header.message_tag + msg.dht_header.message_tag ); Err(DecryptionError::EnvelopeBodyDecodeFailed) }, @@ -433,18 +476,25 @@ mod test { use futures::{executor::block_on, future}; use tari_comms::{ - message::MessageExt, + message::{MessageExt, MessageTag}, runtime, test_utils::mocks::create_connectivity_mock, wrap_in_envelope_body, }; use tari_test_utils::{counter_context, unpack_enum}; + use tokio::time::sleep; use tower::service_fn; use super::*; use crate::{ - envelope::DhtMessageFlags, - test_utils::{make_dht_inbound_message, make_node_identity}, + envelope::{DhtEnvelope, DhtMessageFlags}, + test_utils::{ + make_dht_header, + make_dht_inbound_message, + make_keypair, + make_node_identity, + make_valid_message_signature, + }, }; #[test] @@ -525,7 +575,6 @@ mod test { #[runtime::test] async fn decrypt_inbound_fail_destination() { - let _ = env_logger::try_init(); let (connectivity, mock) = create_connectivity_mock(); mock.spawn(); let result = Arc::new(Mutex::new(None)); @@ -551,7 +600,6 @@ mod test { #[runtime::test] async fn decrypt_inbound_fail_no_destination() { - let _ = env_logger::try_init(); let (connectivity, mock) = create_connectivity_mock(); mock.spawn(); let result = Arc::new(Mutex::new(None)); @@ -580,4 +628,119 @@ mod test { unpack_enum!(DecryptionError::EncryptedMessageNoDestination = err); assert!(result.lock().unwrap().is_none()); } + + #[runtime::test] + async fn decrypt_inbound_fail_invalid_signature_encrypted() { + let (connectivity, mock) = create_connectivity_mock(); + let mock_state = mock.spawn(); + let result = Arc::new(Mutex::new(None)); + let service = service_fn({ + let result = result.clone(); + move |msg: DecryptedDhtMessage| { + *result.lock().unwrap() = Some(msg); + future::ready(Result::<(), PipelineError>::Ok(())) + } + }); + let node_identity = make_node_identity(); + let mut service = DecryptionService::new(Default::default(), node_identity.clone(), connectivity, service); + + let plain_text_msg = b"Secret message".to_vec(); + let (e_secret_key, e_public_key) = make_keypair(); + let shared_secret = crypt::generate_ecdh_secret(&e_secret_key, node_identity.public_key()); + let key_message = crypt::generate_key_message(&shared_secret); + let msg_tag = MessageTag::new(); + + let message = crypt::encrypt(&key_message, &plain_text_msg); + let header = make_dht_header( + &node_identity, + &e_public_key, + &e_secret_key, + &message, + DhtMessageFlags::ENCRYPTED, + true, + msg_tag, + true, + ) + .unwrap(); + let envelope = DhtEnvelope::new(header.into(), &message.into()); + let msg_tag = MessageTag::new(); + let mut inbound_msg = DhtInboundMessage::new( + msg_tag, + envelope.header.unwrap().try_into().unwrap(), + Arc::new(node_identity.to_peer()), + envelope.body, + ); + + // Sign invalid data. Other peers cannot validate this while propagating, but this should not cause them to be + // banned. + let signature = make_valid_message_signature(&node_identity, b"sign invalid data"); + let key_signature = crypt::generate_key_signature_for_authenticated_encryption(&shared_secret); + + inbound_msg.dht_header.message_signature = + crypt::encrypt_with_chacha20_poly1305(&key_signature, &signature).unwrap(); + + let err = service.call(inbound_msg).await.unwrap_err(); + let err = err.downcast::().unwrap(); + unpack_enum!(DecryptionError::MessageSignatureInvalidEncryptedSignature = err); + assert!(result.lock().unwrap().is_none()); + + // Proving a negative i.e. ban is not called, we have no choice but to sleep to wait for any potential calls to + // be registered. This should ensure that if this bug re-occurs that this test is flaky. + sleep(Duration::from_secs(1)).await; + assert_eq!(mock_state.count_calls_containing("BanPeer").await, 0); + } + + #[runtime::test] + async fn decrypt_inbound_fail_invalid_signature_cleartext() { + let (connectivity, mock) = create_connectivity_mock(); + let mock_state = mock.spawn(); + let result = Arc::new(Mutex::new(None)); + let service = service_fn({ + let result = result.clone(); + move |msg: DecryptedDhtMessage| { + *result.lock().unwrap() = Some(msg); + future::ready(Result::<(), PipelineError>::Ok(())) + } + }); + let node_identity = make_node_identity(); + let mut service = DecryptionService::new(Default::default(), node_identity.clone(), connectivity, service); + + let plain_text_msg = b"Public message".to_vec(); + let (e_secret_key, e_public_key) = make_keypair(); + let shared_secret = crypt::generate_ecdh_secret(&e_secret_key, node_identity.public_key()); + let key_message = crypt::generate_key_message(&shared_secret); + let msg_tag = MessageTag::new(); + + let message = crypt::encrypt(&key_message, &plain_text_msg); + let header = make_dht_header( + &node_identity, + &e_public_key, + &e_secret_key, + &message, + DhtMessageFlags::NONE, + true, + msg_tag, + true, + ) + .unwrap(); + let envelope = DhtEnvelope::new(header.into(), &message.into()); + let msg_tag = MessageTag::new(); + let mut inbound_msg = DhtInboundMessage::new( + msg_tag, + envelope.header.unwrap().try_into().unwrap(), + Arc::new(node_identity.to_peer()), + envelope.body, + ); + + inbound_msg.dht_header.ephemeral_public_key = Some(e_public_key.clone()); + inbound_msg.dht_header.message_signature = make_valid_message_signature(&node_identity, b"sign invalid data"); + + let err = service.call(inbound_msg).await.unwrap_err(); + let err = err.downcast::().unwrap(); + unpack_enum!(DecryptionError::MessageSignatureInvalidClearTextSignature = err); + assert!(result.lock().unwrap().is_none()); + + mock_state.await_call_count(1).await; + assert_eq!(mock_state.count_calls_containing("BanPeer").await, 1); + } } diff --git a/comms/dht/src/inbound/message.rs b/comms/dht/src/inbound/message.rs index bca18831b0..5d89f98e98 100644 --- a/comms/dht/src/inbound/message.rs +++ b/comms/dht/src/inbound/message.rs @@ -38,6 +38,37 @@ use crate::{ envelope::{DhtMessageFlags, DhtMessageHeader}, }; +#[derive(Debug, Clone)] +pub struct ValidatedDhtInboundMessage { + message: DhtInboundMessage, + authenticated_origin: Option, +} + +impl ValidatedDhtInboundMessage { + pub fn new(message: DhtInboundMessage, authenticated_origin: Option) -> Self { + Self { + message, + authenticated_origin, + } + } + + pub fn into_message(self) -> DhtInboundMessage { + self.message + } + + pub fn message(&self) -> &DhtInboundMessage { + &self.message + } + + pub fn header(&self) -> &DhtMessageHeader { + &self.message.dht_header + } + + pub fn authenticated_origin(&self) -> Option<&CommsPublicKey> { + self.authenticated_origin.as_ref() + } +} + #[derive(Debug, Clone)] pub struct DhtInboundMessage { pub tag: MessageTag, @@ -48,6 +79,7 @@ pub struct DhtInboundMessage { pub dedup_hit_count: u32, pub body: Vec, } + impl DhtInboundMessage { pub fn new(tag: MessageTag, dht_header: DhtMessageHeader, source_peer: Arc, body: Vec) -> Self { Self { diff --git a/comms/dht/src/message_signature.rs b/comms/dht/src/message_signature.rs index fa9aff68cb..494c9a5a90 100644 --- a/comms/dht/src/message_signature.rs +++ b/comms/dht/src/message_signature.rs @@ -68,7 +68,7 @@ impl MessageSignature { } } - /// Returns true if the provided message valid for this origin MAC, otherwise false. + /// Returns true if the provided message valid for this message signature, otherwise false. pub fn verify(&self, message: &[u8]) -> bool { let challenge = construct_message_signature_hash(&self.signer_public_key, self.signature.get_public_nonce(), message); @@ -95,13 +95,13 @@ impl TryFrom for MessageSignature { fn try_from(message_signature: ProtoMessageSignature) -> Result { let signer_public_key = CommsPublicKey::from_bytes(&message_signature.signer_public_key) - .map_err(|_| MessageSignatureError::InvalidSignerPublicKey)?; + .map_err(|_| MessageSignatureError::InvalidSignerPublicKeyBytes)?; let public_nonce = CommsPublicKey::from_bytes(&message_signature.public_nonce) - .map_err(|_| MessageSignatureError::InvalidPublicNonce)?; + .map_err(|_| MessageSignatureError::InvalidPublicNonceBytes)?; let signature = CommsSecretKey::from_bytes(&message_signature.signature) - .map_err(|_| MessageSignatureError::InvalidSignature)?; + .map_err(|_| MessageSignatureError::InvalidSignatureBytes)?; Ok(Self { signer_public_key, @@ -123,15 +123,13 @@ pub struct ProtoMessageSignature { #[derive(Debug, thiserror::Error)] pub enum MessageSignatureError { - #[error("Failed to decrypt origin MAC")] - DecryptedFailed, - #[error("Failed to validate origin MAC signature")] - InvalidSignature, - #[error("Origin MAC contained an invalid public nonce")] - InvalidPublicNonce, - #[error("Origin MAC contained an invalid signer public key")] - InvalidSignerPublicKey, - #[error("Origin MAC failed to verify")] + #[error("Failed to validate message signature")] + InvalidSignatureBytes, + #[error("Message signature contained an invalid public nonce")] + InvalidPublicNonceBytes, + #[error("Message signature contained an invalid signer public key")] + InvalidSignerPublicKeyBytes, + #[error("Message signature failed to verify")] VerificationFailed, } diff --git a/comms/dht/src/store_forward/saf_handler/task.rs b/comms/dht/src/store_forward/saf_handler/task.rs index 8702b60812..8b7281f65a 100644 --- a/comms/dht/src/store_forward/saf_handler/task.rs +++ b/comms/dht/src/store_forward/saf_handler/task.rs @@ -563,7 +563,7 @@ where S: Service trace!( target: LOG_TARGET, - "Attempting to decrypt origin mac ({} byte(s))", + "Attempting to decrypt message signature ({} byte(s))", header.message_signature.len() ); let shared_secret = crypt::generate_ecdh_secret(node_identity.secret_key(), ephemeral_public_key); diff --git a/comms/dht/src/store_forward/store.rs b/comms/dht/src/store_forward/store.rs index b4bb88b1d6..03b54c976e 100644 --- a/comms/dht/src/store_forward/store.rs +++ b/comms/dht/src/store_forward/store.rs @@ -272,7 +272,7 @@ where S: Service + Se Some(_) => { // If the message doesnt have an origin we wont store it if !message.has_message_signature() { - log_not_eligible("it is a cleartext message and does not have an origin MAC"); + log_not_eligible("it is a cleartext message and does not have an message signature"); return Ok(None); } @@ -303,8 +303,9 @@ where S: Service + Se // TODO: #banheuristic - the source peer should not have propagated this message debug!( target: LOG_TARGET, - "Store task received an encrypted message with no origin MAC. This message {} is invalid and \ - should not be stored or propagated. Dropping message. Sent by node '{}' (Trace: {})", + "Store task received an encrypted message with no message signature. This message {} is \ + invalid and should not be stored or propagated. Dropping message. Sent by node '{}' (Trace: \ + {})", message.tag, message.source_peer.node_id.short_str(), message.dht_header.message_tag