fix(dht): updates to message padding (#4594)

Description
---
[PR 4362](#4362) mitigates a metadata leak whereby encrypted messages are the same length as plaintext messages due to the use of a stream cipher. This work adds more complete length checks, such that padding can fail. It also more efficiently handles the edge case where no padding is needed.

Motivation and Context
---
To avoid directly leaking the length of plaintext messages after stream cipher encryption, [PR 4362](#4362) pads such messages to a multiple of a fixed base length after first prepending the original message length using a fixed encoding.

However, the following cases do not appear to be handled by the padding and unpadding code:
- The plaintext message length exceeds the fixed encoding length
- The ciphertext message is not long enough for extraction of the fixed encoding length
- The ciphertext message is not a multiple of the base length

Further, in the case where the message length (after length prepending) is exactly a multiple of the base length, an entire base length of padding is unnecessarily applied.

This work addresses these issues. The padding process now checks that the plaintext message does not exceed the limit enforced by the length encoding; as a result, it can now return an error that propagates to the encryption function caller. The padding algorithm has been simplified and now handles the multiple-of-the-base-length edge case by correctly applying no padding. The unpadding process now checks that it can safely extract the message length, and checks that the ciphertext message is a multiple of the base length.

How Has This Been Tested?
---
No test has been added for the case where the message length exceeds the limit allowed by the encoding, as this would imply very high memory usage (or swapping) exceeding 4 GB. Existing tests pass. A new test exercises the other failure modes.


* Updates to message padding

Adds better length checks. Simplifies the padding algorithm and handles an edge case hitting a base length multiple.

* Add test

* Propagate padding errors

* Rename parameter for clarity

* Better overflow and error handling

* Formatting

Co-authored-by: stringhandler <mikethetike@tari.com>

comms/dht/src/crypt.rs

@@ -56,7 +56,6 @@ use crate::{
 pub struct CipherKey(chacha20::Key);
 pub struct AuthenticatedCipherKey(chacha20poly1305::Key);
 
-const LITTLE_ENDIAN_U32_SIZE_REPRESENTATION: usize = 4;
 const MESSAGE_BASE_LENGTH: usize = 6000;
 
 /// Generates a Diffie-Hellman secret `kx.G` as a `chacha20::Key` given secret scalar `k` and public key `P = x.G`.
@@ -70,45 +69,68 @@ pub fn generate_ecdh_secret(secret_key: &CommsSecretKey, public_key: &CommsPubli
     output
 }
 
-fn pad_message_to_base_length_multiple(message: &[u8]) -> Vec<u8> {
-    let n = message.len();
-    // little endian representation of message length, to be appended to padded message,
-    // assuming our code runs on 64-bits system
-    let prepend_to_message = (n as u32).to_le_bytes();
-
-    let k = prepend_to_message.len();
-
-    let div_n_base_len = (n + k) / MESSAGE_BASE_LENGTH;
-    let output_size = (div_n_base_len + 1) * MESSAGE_BASE_LENGTH;
+fn pad_message_to_base_length_multiple(message: &[u8]) -> Result<Vec<u8>, DhtOutboundError> {
+    // 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()));
+    }
+    let message_length = message.len();
+    let encoded_length = (message_length as u32).to_le_bytes();
+
+    // Pad the message (if needed) to the next multiple of the base length
+    let padding_length = if ((message_length + size_of::<u32>()) % MESSAGE_BASE_LENGTH) == 0 {
+        0
+    } else {
+        MESSAGE_BASE_LENGTH - ((message_length + size_of::<u32>()) % MESSAGE_BASE_LENGTH)
+    };
+
+    // The padded message is the encoded length, message, and zero padding
+    let mut padded_message = Vec::with_capacity(size_of::<u32>() + message_length + padding_length);
+    padded_message.extend_from_slice(&encoded_length);
+    padded_message.extend_from_slice(message);
+    padded_message.extend(std::iter::repeat(0u8).take(padding_length));
+
+    Ok(padded_message)
+}
 
-    // join prepend_message_len | message | zero_padding
-    let mut output = Vec::with_capacity(output_size);
-    output.extend_from_slice(&prepend_to_message);
-    output.extend_from_slice(message);
-    output.extend(std::iter::repeat(0u8).take(output_size - n - k));
+fn get_original_message_from_padded_text(padded_message: &[u8]) -> Result<Vec<u8>, DhtOutboundError> {
+    // 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
 
-    output
-}
+    // The padded message must be long enough to extract the encoded message length
+    if padded_message.len() < size_of::<u32>() {
+        return Err(DhtOutboundError::PaddingError(
+            "Padded message is not long enough for length extraction".to_string(),
+        ));
+    }
 
-fn get_original_message_from_padded_text(message: &[u8]) -> Result<Vec<u8>, DhtOutboundError> {
-    let mut le_bytes = [0u8; 4];
-    le_bytes.copy_from_slice(&message[..LITTLE_ENDIAN_U32_SIZE_REPRESENTATION]);
+    // The padded message must be a multiple of the base length
+    if (padded_message.len() % MESSAGE_BASE_LENGTH) != 0 {
+        return Err(DhtOutboundError::PaddingError(
+            "Padded message must be a multiple of the base length".to_string(),
+        ));
+    }
 
-    // obtain length of original message, assuming our code runs on 64-bits system
-    let original_message_len = u32::from_le_bytes(le_bytes) as usize;
+    // Decode the message length
+    let mut encoded_length = [0u8; size_of::<u32>()];
+    encoded_length.copy_from_slice(&padded_message[0..size_of::<u32>()]);
+    let message_length = u32::from_le_bytes(encoded_length) as usize;
 
-    if original_message_len > message.len() {
+    // 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()))?;
+    if end > padded_message.len() {
         return Err(DhtOutboundError::CipherError(
-            "Original length message is invalid".to_string(),
+            "Claimed unpadded message length is too large".to_string(),
         ));
     }
 
-    // obtain original message
-    let start = LITTLE_ENDIAN_U32_SIZE_REPRESENTATION;
-    let end = LITTLE_ENDIAN_U32_SIZE_REPRESENTATION + original_message_len;
-    let original_message = &message[start..end];
+    // Remove the padding (we don't check for valid padding, as this is offloaded to authentication)
+    let start = size_of::<u32>();
+    let unpadded_message = &padded_message[start..end];
 
-    Ok(original_message.to_vec())
+    Ok(unpadded_message.to_vec())
 }
 
 pub fn generate_key_message(data: &[u8]) -> CipherKey {
@@ -164,9 +186,9 @@ pub fn decrypt_with_chacha20_poly1305(
 }
 
 /// Encrypt the plain text using the ChaCha20 stream cipher
-pub fn encrypt(cipher_key: &CipherKey, plain_text: &[u8]) -> Vec<u8> {
+pub fn encrypt(cipher_key: &CipherKey, plain_text: &[u8]) -> Result<Vec<u8>, DhtOutboundError> {
     // pad plain_text to avoid message length leaks
-    let plain_text = pad_message_to_base_length_multiple(plain_text);
+    let plain_text = pad_message_to_base_length_multiple(plain_text)?;
 
     let mut nonce = [0u8; size_of::<Nonce>()];
     OsRng.fill_bytes(&mut nonce);
@@ -179,7 +201,7 @@ pub fn encrypt(cipher_key: &CipherKey, plain_text: &[u8]) -> Vec<u8> {
 
     buf[nonce.len()..].copy_from_slice(plain_text.as_slice());
     cipher.apply_keystream(&mut buf[nonce.len()..]);
-    buf
+    Ok(buf)
 }
 
 /// Produces authenticated encryption of the signature using the ChaCha20-Poly1305 stream cipher,
@@ -266,7 +288,7 @@ mod test {
         let pk = CommsPublicKey::default();
         let key = CipherKey(*chacha20::Key::from_slice(pk.as_bytes()));
         let plain_text = "Last enemy position 0830h AJ 9863".as_bytes().to_vec();
-        let encrypted = encrypt(&key, &plain_text);
+        let encrypted = encrypt(&key, &plain_text).unwrap();
         let decrypted = decrypt(&key, &encrypted).unwrap();
         assert_eq!(decrypted, plain_text);
     }
@@ -385,7 +407,7 @@ mod test {
             .take(MESSAGE_BASE_LENGTH - message.len() - prepend_message.len())
             .collect::<Vec<_>>();
 
-        let pad_message = pad_message_to_base_length_multiple(message);
+        let pad_message = pad_message_to_base_length_multiple(message).unwrap();
 
         // padded message is of correct length
         assert_eq!(pad_message.len(), MESSAGE_BASE_LENGTH);
@@ -402,7 +424,7 @@ mod test {
         // test for large message
         let message = &[100u8; MESSAGE_BASE_LENGTH * 8 - 100];
         let prepend_message = (message.len() as u32).to_le_bytes();
-        let pad_message = pad_message_to_base_length_multiple(message);
+        let pad_message = pad_message_to_base_length_multiple(message).unwrap();
         let pad = std::iter::repeat(0u8)
             .take((8 * MESSAGE_BASE_LENGTH) - message.len() - prepend_message.len())
             .collect::<Vec<_>>();
@@ -426,7 +448,7 @@ mod test {
             .take((9 * MESSAGE_BASE_LENGTH) - message.len() - prepend_message.len())
             .collect::<Vec<_>>();
 
-        let pad_message = pad_message_to_base_length_multiple(message);
+        let pad_message = pad_message_to_base_length_multiple(message).unwrap();
 
         // padded message is of correct length
         assert_eq!(pad_message.len(), 9 * MESSAGE_BASE_LENGTH);
@@ -443,7 +465,7 @@ mod test {
         // test for empty message
         let message: [u8; 0] = [];
         let prepend_message = (message.len() as u32).to_le_bytes();
-        let pad_message = pad_message_to_base_length_multiple(&message);
+        let pad_message = pad_message_to_base_length_multiple(&message).unwrap();
         let pad = [0u8; MESSAGE_BASE_LENGTH - 4];
 
         // padded message is of correct length
@@ -460,32 +482,56 @@ mod test {
         assert_eq!(pad, pad_message[prepend_message.len() + message.len()..]);
     }
 
+    #[test]
+    fn unpadding_failure_modes() {
+        // The padded message is empty
+        let message: [u8; 0] = [];
+        assert!(get_original_message_from_padded_text(&message)
+            .unwrap_err()
+            .to_string()
+            .contains("Padded message is not long enough for length extraction"));
+
+        // We cannot extract the message length
+        let message = [0u8; size_of::<u32>() - 1];
+        assert!(get_original_message_from_padded_text(&message)
+            .unwrap_err()
+            .to_string()
+            .contains("Padded message is not long enough for length extraction"));
+
+        // The padded message is not a multiple of the base length
+        let message = [0u8; 2 * MESSAGE_BASE_LENGTH + 1];
+        assert!(get_original_message_from_padded_text(&message)
+            .unwrap_err()
+            .to_string()
+            .contains("Padded message must be a multiple of the base length"));
+    }
+
     #[test]
     fn get_original_message_from_padded_text_successful() {
         // test for short message
         let message = vec![0u8, 10, 22, 11, 38, 74, 59, 91, 73, 82, 75, 23, 59];
-        let pad_message = pad_message_to_base_length_multiple(message.as_slice());
+        let pad_message = pad_message_to_base_length_multiple(message.as_slice()).unwrap();
 
         let output_message = get_original_message_from_padded_text(pad_message.as_slice()).unwrap();
         assert_eq!(message, output_message);
 
         // test for large message
         let message = vec![100u8; 1024];
-        let pad_message = pad_message_to_base_length_multiple(message.as_slice());
+        let pad_message = pad_message_to_base_length_multiple(message.as_slice()).unwrap();
 
         let output_message = get_original_message_from_padded_text(pad_message.as_slice()).unwrap();
         assert_eq!(message, output_message);
 
         // test for base message of base length
         let message = vec![100u8; 984];
-        let pad_message = pad_message_to_base_length_multiple(message.as_slice());
+        let pad_message = pad_message_to_base_length_multiple(message.as_slice()).unwrap();
 
         let output_message = get_original_message_from_padded_text(pad_message.as_slice()).unwrap();
         assert_eq!(message, output_message);
 
         // test for empty message
         let message: Vec<u8> = vec![];
-        let pad_message = pad_message_to_base_length_multiple(message.as_slice());
+        let pad_message = pad_message_to_base_length_multiple(message.as_slice()).unwrap();
 
         let output_message = get_original_message_from_padded_text(pad_message.as_slice()).unwrap();
         assert_eq!(message, output_message);
@@ -494,7 +540,7 @@ mod test {
     #[test]
     fn padding_fails_if_pad_message_prepend_length_is_bigger_than_plaintext_length() {
         let message = "This is my secret message, keep it secret !".as_bytes();
-        let mut pad_message = pad_message_to_base_length_multiple(message);
+        let mut pad_message = pad_message_to_base_length_multiple(message).unwrap();
 
         // we modify the prepend length, in order to assert that the get original message
         // method will output a different length message
@@ -512,7 +558,7 @@ mod test {
         assert!(get_original_message_from_padded_text(pad_message.as_slice())
             .unwrap_err()
             .to_string()
-            .contains("Original length message is invalid"));
+            .contains("Claimed unpadded message length is too large"));
     }
 
     #[test]
@@ -522,7 +568,7 @@ mod test {
         let pk = CommsPublicKey::default();
         let key = CipherKey(*chacha20::Key::from_slice(pk.as_bytes()));
         let message = "My secret message, keep it secret !".as_bytes().to_vec();
-        let mut encrypted = encrypt(&key, &message);
+        let mut encrypted = encrypt(&key, &message).unwrap();
 
         let n = encrypted.len();
         encrypted[n - 1] += 1;
@@ -535,9 +581,9 @@ mod test {
         let pk = CommsPublicKey::default();
         let key = CipherKey(*chacha20::Key::from_slice(pk.as_bytes()));
         let message = "My secret message, keep it secret !".as_bytes().to_vec();
-        let mut encrypted = encrypt(&key, &message);
+        let mut encrypted = encrypt(&key, &message).unwrap();
 
-        encrypted[size_of::<Nonce>() + LITTLE_ENDIAN_U32_SIZE_REPRESENTATION + 1] += 1;
+        encrypted[size_of::<Nonce>() + size_of::<u32>() + 1] += 1;
 
         assert!(decrypt(&key, &encrypted).unwrap() != message);
     }

comms/dht/src/dht.rs

@@ -598,7 +598,7 @@ mod test {
         let node_identity2 = make_node_identity();
         let ecdh_key = crypt::generate_ecdh_secret(node_identity2.secret_key(), node_identity2.public_key());
         let key_message = crypt::generate_key_message(&ecdh_key);
-        let encrypted_bytes = crypt::encrypt(&key_message, &msg.to_encoded_bytes());
+        let encrypted_bytes = crypt::encrypt(&key_message, &msg.to_encoded_bytes()).unwrap();
         let dht_envelope = make_dht_envelope(
             &node_identity2,
             encrypted_bytes,

comms/dht/src/inbound/decryption.rs

@@ -650,7 +650,7 @@ mod test {
         let key_message = crypt::generate_key_message(&shared_secret);
         let msg_tag = MessageTag::new();
 
-        let message = crypt::encrypt(&key_message, &plain_text_msg);
+        let message = crypt::encrypt(&key_message, &plain_text_msg).unwrap();
         let header = make_dht_header(
             &node_identity,
             &e_public_key,
@@ -711,7 +711,7 @@ mod test {
         let key_message = crypt::generate_key_message(&shared_secret);
         let msg_tag = MessageTag::new();
 
-        let message = crypt::encrypt(&key_message, &plain_text_msg);
+        let message = crypt::encrypt(&key_message, &plain_text_msg).unwrap();
         let header = make_dht_header(
             &node_identity,
             &e_public_key,

comms/dht/src/outbound/broadcast.rs

@@ -500,7 +500,7 @@ where S: Service<DhtOutboundMessage, Response = (), Error = PipelineError>
                 // Generate key message for encryption of message
                 let key_message = crypt::generate_key_message(&shared_ephemeral_secret);
                 // Encrypt the message with the body with key message above
-                let encrypted_body = crypt::encrypt(&key_message, &body);
+                let encrypted_body = crypt::encrypt(&key_message, &body)?;
 
                 // Produce domain separated signature signature
                 let mac_signature = crypt::create_message_domain_separated_hash_parts(

comms/dht/src/outbound/error.rs

@@ -56,6 +56,8 @@ pub enum DhtOutboundError {
     NoMessagesQueued,
     #[error("Cipher error: `{0}`")]
     CipherError(String),
+    #[error("Padding error: `{0}`")]
+    PaddingError(String), // TODO: clean up these errors
 }
 
 impl From<SendFailure> for DhtOutboundError {

comms/dht/src/test_utils/makers.rs

@@ -164,7 +164,7 @@ pub fn make_dht_envelope(
     if flags.is_encrypted() {
         let shared_secret = crypt::generate_ecdh_secret(&e_secret_key, node_identity.public_key());
         let key_message = crypt::generate_key_message(&shared_secret);
-        message = crypt::encrypt(&key_message, &message);
+        message = crypt::encrypt(&key_message, &message).unwrap();
     }
     let header = make_dht_header(
         node_identity,