fix: use dht inbound error for decryption (see issue #4596)

comms/dht/src/crypt.rs

@@ -47,7 +47,7 @@ use crate::{
     comms_dht_hash_domain_key_message,
     comms_dht_hash_domain_key_signature,
     envelope::{DhtMessageFlags, DhtMessageHeader, DhtMessageType, NodeDestination},
-    outbound::DhtOutboundError,
+    error::DhtEncryptError,
     version::DhtProtocolVersion,
 };
 
@@ -69,10 +69,10 @@ pub fn generate_ecdh_secret(secret_key: &CommsSecretKey, public_key: &CommsPubli
     output
 }
 
-fn pad_message_to_base_length_multiple(message: &[u8]) -> Result<Vec<u8>, DhtOutboundError> {
+fn pad_message_to_base_length_multiple(message: &[u8]) -> Result<Vec<u8>, DhtEncryptError> {
     // We require a 32-bit length representation, and also don't want to overflow after including this encoding
     if message.len() > ((u32::max_value() - (size_of::<u32>() as u32)) as usize) {
-        return Err(DhtOutboundError::PaddingError("Message is too long".to_string()));
+        return Err(DhtEncryptError::PaddingError("Message is too long".to_string()));
     }
     let message_length = message.len();
     let encoded_length = (message_length as u32).to_le_bytes();
@@ -93,20 +93,20 @@ fn pad_message_to_base_length_multiple(message: &[u8]) -> Result<Vec<u8>, DhtOut
     Ok(padded_message)
 }
 
-fn get_original_message_from_padded_text(padded_message: &[u8]) -> Result<Vec<u8>, DhtOutboundError> {
+fn get_original_message_from_padded_text(padded_message: &[u8]) -> Result<Vec<u8>, DhtEncryptError> {
     // NOTE: This function can return errors relating to message length
     // It is important not to leak error types to an adversary, or to have timing differences
 
     // The padded message must be long enough to extract the encoded message length
     if padded_message.len() < size_of::<u32>() {
-        return Err(DhtOutboundError::PaddingError(
+        return Err(DhtEncryptError::PaddingError(
             "Padded message is not long enough for length extraction".to_string(),
         ));
     }
 
     // The padded message must be a multiple of the base length
     if (padded_message.len() % MESSAGE_BASE_LENGTH) != 0 {
-        return Err(DhtOutboundError::PaddingError(
+        return Err(DhtEncryptError::PaddingError(
             "Padded message must be a multiple of the base length".to_string(),
         ));
     }
@@ -119,9 +119,9 @@ fn get_original_message_from_padded_text(padded_message: &[u8]) -> Result<Vec<u8
     // The padded message is too short for the decoded length
     let end = message_length
         .checked_add(size_of::<u32>())
-        .ok_or_else(|| DhtOutboundError::PaddingError("Claimed unpadded message length is too large".to_string()))?;
+        .ok_or_else(|| DhtEncryptError::PaddingError("Claimed unpadded message length is too large".to_string()))?;
     if end > padded_message.len() {
-        return Err(DhtOutboundError::CipherError(
+        return Err(DhtEncryptError::CipherError(
             "Claimed unpadded message length is too large".to_string(),
         ));
     }
@@ -150,11 +150,9 @@ pub fn generate_key_signature_for_authenticated_encryption(data: &[u8]) -> Authe
 }
 
 /// Decrypts cipher text using ChaCha20 stream cipher given the cipher key and cipher text with integral nonce.
-pub fn decrypt(cipher_key: &CipherKey, cipher_text: &[u8]) -> Result<Vec<u8>, DhtOutboundError> {
+pub fn decrypt(cipher_key: &CipherKey, cipher_text: &[u8]) -> Result<Vec<u8>, DhtEncryptError> {
     if cipher_text.len() < size_of::<Nonce>() {
-        return Err(DhtOutboundError::CipherError(
-            "Cipher text is not long enough to include nonce".to_string(),
-        ));
+        return Err(DhtEncryptError::InvalidDecryptionNonceNotIncluded);
     }
 
     let (nonce, cipher_text) = cipher_text.split_at(size_of::<Nonce>());
@@ -172,21 +170,21 @@ pub fn decrypt(cipher_key: &CipherKey, cipher_text: &[u8]) -> Result<Vec<u8>, Dh
 pub fn decrypt_with_chacha20_poly1305(
     cipher_key: &AuthenticatedCipherKey,
     cipher_signature: &[u8],
-) -> Result<Vec<u8>, DhtOutboundError> {
+) -> Result<Vec<u8>, DhtEncryptError> {
     let nonce = [0u8; size_of::<chacha20poly1305::Nonce>()];
 
     let nonce_ga = chacha20poly1305::Nonce::from_slice(&nonce);
 
     let cipher = ChaCha20Poly1305::new(&cipher_key.0);
     let decrypted_signature = cipher
         .decrypt(nonce_ga, cipher_signature)
-        .map_err(|_| DhtOutboundError::CipherError(String::from("Authenticated decryption failed")))?;
+        .map_err(|_| DhtEncryptError::InvalidAuthenticatedDecryption)?;
 
     Ok(decrypted_signature)
 }
 
 /// Encrypt the plain text using the ChaCha20 stream cipher
-pub fn encrypt(cipher_key: &CipherKey, plain_text: &[u8]) -> Result<Vec<u8>, DhtOutboundError> {
+pub fn encrypt(cipher_key: &CipherKey, plain_text: &[u8]) -> Result<Vec<u8>, DhtEncryptError> {
     // pad plain_text to avoid message length leaks
     let plain_text = pad_message_to_base_length_multiple(plain_text)?;
 
@@ -212,7 +210,7 @@ pub fn encrypt(cipher_key: &CipherKey, plain_text: &[u8]) -> Result<Vec<u8>, Dht
 pub fn encrypt_with_chacha20_poly1305(
     cipher_key: &AuthenticatedCipherKey,
     signature: &[u8],
-) -> Result<Vec<u8>, DhtOutboundError> {
+) -> Result<Vec<u8>, DhtEncryptError> {
     let nonce = [0u8; size_of::<chacha20poly1305::Nonce>()];
 
     let nonce_ga = chacha20poly1305::Nonce::from_slice(&nonce);
@@ -221,7 +219,7 @@ pub fn encrypt_with_chacha20_poly1305(
     // length of encrypted equals signature.len() + 16 (the latter being the tag size for ChaCha20-poly1305)
     let encrypted = cipher
         .encrypt(nonce_ga, signature)
-        .map_err(|_| DhtOutboundError::CipherError(String::from("Authenticated encryption failed")))?;
+        .map_err(|_| DhtEncryptError::CipherError(String::from("Authenticated encryption failed")))?;
 
     Ok(encrypted)
 }
@@ -326,7 +324,7 @@ mod test {
 
         let encrypted = cipher
             .encrypt(nonce_ga, signature)
-            .map_err(|_| DhtOutboundError::CipherError(String::from("Authenticated encryption failed")))
+            .map_err(|_| DhtEncryptError::CipherError(String::from("Authenticated encryption failed")))
             .unwrap();
 
         assert_eq!(encrypted.len(), n + 16);
@@ -353,7 +351,7 @@ mod test {
         assert!(decrypt_with_chacha20_poly1305(&key, encrypted.as_slice())
             .unwrap_err()
             .to_string()
-            .contains("Authenticated decryption failed"));
+            .contains("Invalid authenticated decryption"));
     }
 
     #[test]
@@ -373,7 +371,7 @@ mod test {
         assert!(decrypt_with_chacha20_poly1305(&key, encrypted.as_slice())
             .unwrap_err()
             .to_string()
-            .contains("Authenticated decryption failed"));
+            .contains("Invalid authenticated decryption"));
     }
 
     #[test]
@@ -395,7 +393,7 @@ mod test {
         assert!(decrypt_with_chacha20_poly1305(&other_key, encrypted.as_slice())
             .unwrap_err()
             .to_string()
-            .contains("Authenticated decryption failed"));
+            .contains("Invalid authenticated decryption"));
     }
 
     #[test]

comms/dht/src/error.rs

@@ -0,0 +1,37 @@
+// Copyright 2019, The Tari Project
+//
+// Redistribution and use in source and binary forms, with or without modification, are permitted provided that the
+// following conditions are met:
+//
+// 1. Redistributions of source code must retain the above copyright notice, this list of conditions and the following
+// disclaimer.
+//
+// 2. Redistributions in binary form must reproduce the above copyright notice, this list of conditions and the
+// following disclaimer in the documentation and/or other materials provided with the distribution.
+//
+// 3. Neither the name of the copyright holder nor the names of its contributors may be used to endorse or promote
+// products derived from this software without specific prior written permission.
+//
+// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES,
+// INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+// DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
+// SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY,
+// 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 thiserror::Error;
+
+#[derive(Debug, Error)]
+pub enum DhtEncryptError {
+    #[error("Message body invalid")]
+    InvalidMessageBody,
+    #[error("Invalid decryption, nonce not included")]
+    InvalidDecryptionNonceNotIncluded,
+    #[error("Invalid authenticated decryption")]
+    InvalidAuthenticatedDecryption,
+    #[error("Cipher error: `{0}`")]
+    CipherError(String),
+    #[error("Padding error: `{0}`")]
+    PaddingError(String),
+}

comms/dht/src/inbound/decryption.rs

@@ -37,6 +37,7 @@ use tower::{layer::Layer, Service, ServiceExt};
 use crate::{
     crypt,
     envelope::DhtMessageHeader,
+    error::DhtEncryptError,
     inbound::message::{DecryptedDhtMessage, DhtInboundMessage, ValidatedDhtInboundMessage},
     message_signature::{MessageSignature, MessageSignatureError, ProtoMessageSignature},
     DhtConfig,
@@ -68,10 +69,10 @@ enum DecryptionError {
     MessageRejectDecryptionFailed,
     #[error("Failed to decode envelope body")]
     EnvelopeBodyDecodeFailed,
-    #[error("Failed to decrypt message body")]
-    MessageBodyDecryptionFailed,
     #[error("Encrypted message without a destination is invalid")]
     EncryptedMessageNoDestination,
+    #[error("Decryption failed: {0}")]
+    DecryptionFailedMalformedCipher(#[from] DhtEncryptError),
 }
 
 /// This layer is responsible for attempting to decrypt inbound messages.
@@ -406,7 +407,7 @@ where S: Service<DecryptedDhtMessage, Response = (), Error = PipelineError>
     ) -> Result<EnvelopeBody, DecryptionError> {
         let key_message = crypt::generate_key_message(shared_secret);
         let decrypted =
-            crypt::decrypt(&key_message, message_body).map_err(|_| DecryptionError::MessageBodyDecryptionFailed)?;
+            crypt::decrypt(&key_message, message_body).map_err(DecryptionError::DecryptionFailedMalformedCipher)?;
         // Deserialization into an EnvelopeBody is done here to determine if the
         // decryption produced valid bytes or not.
         EnvelopeBody::decode(decrypted.as_slice())
@@ -432,7 +433,7 @@ where S: Service<DecryptedDhtMessage, Response = (), Error = PipelineError>
                 }
                 Ok(body)
             })
-            .map_err(|_| DecryptionError::MessageBodyDecryptionFailed)
+            .map_err(|_| DecryptionError::EnvelopeBodyDecodeFailed)
     }
 
     async fn success_not_encrypted(

comms/dht/src/inbound/error.rs

@@ -23,7 +23,12 @@
 use tari_comms::{message::MessageError, peer_manager::PeerManagerError};
 use thiserror::Error;
 
-use crate::{discovery::DhtDiscoveryError, outbound::DhtOutboundError, peer_validator::PeerValidatorError};
+use crate::{
+    discovery::DhtDiscoveryError,
+    error::DhtEncryptError,
+    outbound::DhtOutboundError,
+    peer_validator::PeerValidatorError,
+};
 
 #[derive(Debug, Error)]
 pub enum DhtInboundError {
@@ -33,6 +38,8 @@ pub enum DhtInboundError {
     PeerManagerError(#[from] PeerManagerError),
     #[error("DhtOutboundError: {0}")]
     DhtOutboundError(#[from] DhtOutboundError),
+    #[error("DhtEncryptError: {0}")]
+    DhtEncryptError(#[from] DhtEncryptError),
     #[error("Message body invalid")]
     InvalidMessageBody,
     #[error("All given addresses were invalid")]

comms/dht/src/inbound/mod.rs

@@ -38,6 +38,7 @@ mod metrics;
 pub use metrics::MetricsLayer;
 
 mod error;
+pub use error::DhtInboundError;
 
 mod message;
 

comms/dht/src/lib.rs

@@ -91,6 +91,9 @@ pub use dht::{Dht, DhtInitializationError};
 mod discovery;
 pub use discovery::{DhtDiscoveryError, DhtDiscoveryRequester};
 
+mod error;
+pub use error::DhtEncryptError;
+
 mod network_discovery;
 pub use network_discovery::NetworkDiscoveryConfig;
 

comms/dht/src/outbound/error.rs

@@ -26,10 +26,15 @@ use tari_utilities::message_format::MessageFormatError;
 use thiserror::Error;
 use tokio::sync::mpsc::error::SendError;
 
-use crate::outbound::{message::SendFailure, DhtOutboundRequest};
+use crate::{
+    error::DhtEncryptError,
+    outbound::{message::SendFailure, DhtOutboundRequest},
+};
 
 #[derive(Debug, Error)]
 pub enum DhtOutboundError {
+    #[error("DhtEncryptError: {0}")]
+    DhtEncryptError(#[from] DhtEncryptError),
     #[error("`Failed to send: {0}")]
     SendError(#[from] SendError<DhtOutboundRequest>),
     #[error("MessageSerializationError: {0}")]

comms/dht/src/store_forward/error.rs

@@ -33,6 +33,8 @@ use thiserror::Error;
 use crate::{
     actor::DhtActorError,
     envelope::DhtMessageError,
+    error::DhtEncryptError,
+    inbound::DhtInboundError,
     message_signature::MessageSignatureError,
     outbound::DhtOutboundError,
     storage::StorageError,
@@ -49,8 +51,12 @@ pub enum StoreAndForwardError {
     PeerManagerError(#[from] PeerManagerError),
     #[error("DhtOutboundError: {0}")]
     DhtOutboundError(#[from] DhtOutboundError),
+    #[error("DhtEncryptError: {0}")]
+    DhtEncryptError(#[from] DhtEncryptError),
     #[error("Received stored message has an invalid destination")]
     InvalidDestination,
+    #[error("DhtInboundError: {0}")]
+    DhtInboundError(#[from] DhtInboundError),
     #[error("Received stored message has an invalid origin signature: {0}")]
     InvalidMessageSignature(#[from] MessageSignatureError),
     #[error("Invalid envelope body")]