From cf4f9bf1b555755d8be6fd7a3bd401f6bc154fdd Mon Sep 17 00:00:00 2001 From: Aaron Feickert <66188213+AaronFeickert@users.noreply.github.com> Date: Fri, 2 Sep 2022 15:39:15 +0200 Subject: [PATCH] fix(dht): updates to message padding (#4594) Description --- [PR 4362](https://github.com/tari-project/tari/pull/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](https://github.com/tari-project/tari/pull/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 --- comms/dht/src/crypt.rs | 140 ++++++++++++++++++---------- comms/dht/src/dht.rs | 2 +- comms/dht/src/inbound/decryption.rs | 4 +- comms/dht/src/outbound/broadcast.rs | 2 +- comms/dht/src/outbound/error.rs | 2 + comms/dht/src/test_utils/makers.rs | 2 +- 6 files changed, 100 insertions(+), 52 deletions(-) diff --git a/comms/dht/src/crypt.rs b/comms/dht/src/crypt.rs index 4b42361a40..9c0b870cda 100644 --- a/comms/dht/src/crypt.rs +++ b/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 { - 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, 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::() 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::()) % MESSAGE_BASE_LENGTH) == 0 { + 0 + } else { + MESSAGE_BASE_LENGTH - ((message_length + size_of::()) % MESSAGE_BASE_LENGTH) + }; + + // The padded message is the encoded length, message, and zero padding + let mut padded_message = Vec::with_capacity(size_of::() + 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, 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::() { + 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, 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::()]; + encoded_length.copy_from_slice(&padded_message[0..size_of::()]); + 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::()) + .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::(); + 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 { +pub fn encrypt(cipher_key: &CipherKey, plain_text: &[u8]) -> Result, 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::()]; OsRng.fill_bytes(&mut nonce); @@ -179,7 +201,7 @@ pub fn encrypt(cipher_key: &CipherKey, plain_text: &[u8]) -> Vec { 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::>(); - 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::>(); @@ -426,7 +448,7 @@ mod test { .take((9 * MESSAGE_BASE_LENGTH) - message.len() - prepend_message.len()) .collect::>(); - 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::() - 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 = 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::() + LITTLE_ENDIAN_U32_SIZE_REPRESENTATION + 1] += 1; + encrypted[size_of::() + size_of::() + 1] += 1; assert!(decrypt(&key, &encrypted).unwrap() != message); } diff --git a/comms/dht/src/dht.rs b/comms/dht/src/dht.rs index 603506ecdb..c70db16c34 100644 --- a/comms/dht/src/dht.rs +++ b/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, diff --git a/comms/dht/src/inbound/decryption.rs b/comms/dht/src/inbound/decryption.rs index 03b805c361..56c7c05866 100644 --- a/comms/dht/src/inbound/decryption.rs +++ b/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, diff --git a/comms/dht/src/outbound/broadcast.rs b/comms/dht/src/outbound/broadcast.rs index 4632894a3d..71d079029d 100644 --- a/comms/dht/src/outbound/broadcast.rs +++ b/comms/dht/src/outbound/broadcast.rs @@ -500,7 +500,7 @@ where S: Service // 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( diff --git a/comms/dht/src/outbound/error.rs b/comms/dht/src/outbound/error.rs index 4b702e778b..fdd255534a 100644 --- a/comms/dht/src/outbound/error.rs +++ b/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 for DhtOutboundError { diff --git a/comms/dht/src/test_utils/makers.rs b/comms/dht/src/test_utils/makers.rs index cffb0eb34e..7646346b3a 100644 --- a/comms/dht/src/test_utils/makers.rs +++ b/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,