[PM-2460/24639] Allow empty arrays, and fix string plaintext length hiding (#379)

## 🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-24640
https://bitwarden.atlassian.net/browse/PM-24639

## 📔 Objective

`""` is a valid value to encrypt, and the current vault code in clients
*sometimes* does encrypt `""` (and sometimes just returns null). This
was not anticipated when writing the padding initially. This changes the
padding to allow padding empty byte arrays.

Further, it seems the block padding for strings was done incorrectly and
only hides the first block's plaintext length, but afterwards has a 1:1
correlation to plaintext length:

Before:
```
String Length -> EncString Length
================================
  0 ->  194
  1 ->  194
  2 ->  194
  3 ->  194
  4 ->  194
  5 ->  194
  6 ->  194
  7 ->  194
  8 ->  194
  9 ->  194
 10 ->  194
 11 ->  194
 12 ->  194
 13 ->  194
 14 ->  194
 15 ->  194
 16 ->  194
 17 ->  194
 18 ->  194
 19 ->  194
 20 ->  194
 21 ->  194
 22 ->  194
 23 ->  194
 24 ->  194
 25 ->  194
 26 ->  194
 27 ->  194
 28 ->  194
 29 ->  194
 30 ->  194
 31 ->  194
 32 ->  198
 33 ->  198
 34 ->  198
 35 ->  202
 36 ->  202
 37 ->  202
 38 ->  206
 39 ->  206
 40 ->  206
 41 ->  210
 42 ->  210
 43 ->  210
 44 ->  214
 45 ->  214
 46 ->  214
 47 ->  218
 48 ->  218
 49 ->  218
 50 ->  222
 51 ->  222
 52 ->  222
 53 ->  226
 54 ->  226
 55 ->  226
 56 ->  230
 57 ->  230
 58 ->  230
 59 ->  234
 60 ->  234
 61 ->  234
 62 ->  238
 63 ->  238
 64 ->  238
 65 ->  242
 66 ->  242
 67 ->  242
 68 ->  246
 69 ->  246
 70 ->  246
 71 ->  250
 72 ->  250
 73 ->  250
 74 ->  254
 75 ->  254
 76 ->  254
 77 ->  258
 78 ->  258
 79 ->  258
 80 ->  262
```

After:
```
String Length -> EncString Length
================================
  0 ->  194
  1 ->  194
  2 ->  194
  3 ->  194
  4 ->  194
  5 ->  194
  6 ->  194
  7 ->  194
  8 ->  194
  9 ->  194
 10 ->  194
 11 ->  194
 12 ->  194
 13 ->  194
 14 ->  194
 15 ->  194
 16 ->  194
 17 ->  194
 18 ->  194
 19 ->  194
 20 ->  194
 21 ->  194
 22 ->  194
 23 ->  194
 24 ->  194
 25 ->  194
 26 ->  194
 27 ->  194
 28 ->  194
 29 ->  194
 30 ->  194
 31 ->  194
 32 ->  238
 33 ->  238
 34 ->  238
 35 ->  238
 36 ->  238
 37 ->  238
 38 ->  238
 39 ->  238
 40 ->  238
 41 ->  238
 42 ->  238
 43 ->  238
 44 ->  238
 45 ->  238
 46 ->  238
 47 ->  238
 48 ->  238
 49 ->  238
 50 ->  238
 51 ->  238
 52 ->  238
 53 ->  238
 54 ->  238
 55 ->  238
 56 ->  238
 57 ->  238
 58 ->  238
 59 ->  238
 60 ->  238
 61 ->  238
 62 ->  238
 63 ->  238
 64 ->  282
 65 ->  282
 66 ->  282
 67 ->  282
 68 ->  282
 69 ->  282
 70 ->  282
 71 ->  282
 72 ->  282
 73 ->  282
 74 ->  282
 75 ->  282
 76 ->  282
 77 ->  282
 78 ->  282
 79 ->  282
 80 ->  282
```

Both of these changes don't break compatibility. However, even if they
did, the code is not rolled out yet so it would be OK.

## ⏰ Reminders before review

- Contributor guidelines followed
- All formatters and local linters executed and passed
- Written new unit and / or integration tests where applicable
- Protected functional changes with optionality (feature flags)
- Used internationalization (i18n) for all UI strings
- CI builds passed
- Communicated to DevOps any deployment requirements
- Updated any necessary documentation (Confluence, contributing docs) or
informed the documentation
  team

## 🦮 Reviewer guidelines

<!-- Suggested interactions but feel free to use (or not) as you desire!
-->

- 👍 (`:+1:`) or similar for great changes
- 📝 (`:memo:`) or ℹ️ (`:information_source:`) for notes or general info
- ❓ (`:question:`) for questions
- 🤔 (`:thinking:`) or 💭 (`:thought_balloon:`) for more open inquiry
that's not quite a confirmed
  issue and could potentially benefit from discussion
- 🎨 (`:art:`) for suggestions / improvements
- ❌ (`:x:`) or ⚠️ (`:warning:`) for more significant problems or
concerns needing attention
- 🌱 (`:seedling:`) or ♻️ (`:recycle:`) for future improvements or
indications of technical debt
- ⛏ (`:pick:`) for minor or nitpick changes

Author: quexten

crates/bitwarden-crypto/src/cose.rs

@@ -57,7 +57,9 @@ pub(crate) fn encrypt_xchacha20_poly1305(
 
     if should_pad_content(&content_format) {
         // Pad the data to a block size in order to hide plaintext length
-        crate::keys::utils::pad_bytes(&mut plaintext, XCHACHA20_TEXT_PAD_BLOCK_SIZE);
+        let min_length =
+            XCHACHA20_TEXT_PAD_BLOCK_SIZE * (1 + (plaintext.len() / XCHACHA20_TEXT_PAD_BLOCK_SIZE));
+        crate::keys::utils::pad_bytes(&mut plaintext, min_length)?;
     }
 
     let mut nonce = [0u8; xchacha20::NONCE_SIZE];

crates/bitwarden-crypto/src/enc_string/symmetric.rs

@@ -364,6 +364,44 @@ mod tests {
         KEY_ID_SIZE,
     };
 
+    fn encrypt_with_xchacha20(plaintext: &str) -> EncString {
+        let key_id = [0u8; KEY_ID_SIZE];
+        let enc_key = [0u8; 32];
+        let key = SymmetricCryptoKey::XChaCha20Poly1305Key(crate::XChaCha20Poly1305Key {
+            key_id,
+            enc_key: Box::pin(enc_key.into()),
+        });
+
+        plaintext.encrypt_with_key(&key).expect("encryption works")
+    }
+
+    /// XChaCha20Poly1305 encstrings should be padded in blocks of 32 bytes. This ensures that the
+    /// encstring length does not reveal more than the 32-byte range of lengths that the contained
+    /// string falls into.
+    #[test]
+    fn test_xchacha20_encstring_string_padding_block_sizes() {
+        let cases = [
+            ("", 32),              // empty string, padded to 32
+            (&"a".repeat(31), 32), // largest in first block
+            (&"a".repeat(32), 64), // smallest in second block
+            (&"a".repeat(63), 64), // largest in second block
+            (&"a".repeat(64), 96), // smallest in third block
+        ];
+
+        let ciphertext_lengths: Vec<_> = cases
+            .iter()
+            .map(|(plaintext, _)| encrypt_with_xchacha20(plaintext).to_string().len())
+            .collect();
+
+        // Block 1: 0-31 (same length)
+        assert_eq!(ciphertext_lengths[0], ciphertext_lengths[1]);
+        // Block 2: 32-63 (same length, different from block 1)
+        assert_ne!(ciphertext_lengths[1], ciphertext_lengths[2]);
+        assert_eq!(ciphertext_lengths[2], ciphertext_lengths[3]);
+        // Block 3: 64+ (different from block 2)
+        assert_ne!(ciphertext_lengths[3], ciphertext_lengths[4]);
+    }
+
     #[test]
     fn test_enc_roundtrip_xchacha20() {
         let key_id = [0u8; KEY_ID_SIZE];
@@ -390,6 +428,32 @@ mod tests {
         assert_eq!(decrypted_str, test_string);
     }
 
+    #[test]
+    fn test_enc_roundtrip_xchacha20_empty() {
+        let key_id = [0u8; KEY_ID_SIZE];
+        let enc_key = [0u8; 32];
+        let key = SymmetricCryptoKey::XChaCha20Poly1305Key(crate::XChaCha20Poly1305Key {
+            key_id,
+            enc_key: Box::pin(enc_key.into()),
+        });
+
+        let test_string = "";
+        let cipher = test_string.to_owned().encrypt_with_key(&key).unwrap();
+        let decrypted_str: String = cipher.decrypt_with_key(&key).unwrap();
+        assert_eq!(decrypted_str, test_string);
+    }
+
+    #[test]
+    fn test_enc_string_roundtrip_empty() {
+        let key = SymmetricCryptoKey::Aes256CbcHmacKey(derive_symmetric_key("test"));
+
+        let test_string = "";
+        let cipher = test_string.to_string().encrypt_with_key(&key).unwrap();
+
+        let decrypted_str: String = cipher.decrypt_with_key(&key).unwrap();
+        assert_eq!(decrypted_str, test_string);
+    }
+
     #[test]
     fn test_enc_string_ref_roundtrip() {
         let key = SymmetricCryptoKey::Aes256CbcHmacKey(derive_symmetric_key("test"));

crates/bitwarden-crypto/src/keys/symmetric_crypto_key.rs

@@ -154,7 +154,7 @@ impl SymmetricCryptoKey {
             }
             EncodedSymmetricKey::CoseKey(_) => {
                 let mut encoded_key: Vec<u8> = encoded_key.into();
-                pad_key(&mut encoded_key, Self::AES256_CBC_HMAC_KEY_LEN + 1);
+                pad_key(&mut encoded_key, (Self::AES256_CBC_HMAC_KEY_LEN + 1) as u8); // This is less than 255
                 BitwardenLegacyKeyBytes::from(encoded_key)
             }
         }
@@ -379,8 +379,9 @@ impl std::fmt::Debug for XChaCha20Poly1305Key {
 /// padding is used to make sure that the byte representation uniquely separates the keys by
 /// size of the byte array. The previous key types [SymmetricCryptoKey::Aes256CbcHmacKey] and
 /// [SymmetricCryptoKey::Aes256CbcKey] are 64 and 32 bytes long respectively.
-fn pad_key(key_bytes: &mut Vec<u8>, min_length: usize) {
-    crate::keys::utils::pad_bytes(key_bytes, min_length);
+fn pad_key(key_bytes: &mut Vec<u8>, min_length: u8) {
+    crate::keys::utils::pad_bytes(key_bytes, min_length as usize)
+        .expect("Padding cannot fail since the min_length is < 255")
 }
 
 /// Unpad a key that is padded using the PKCS7-like padding defined by [pad_key].

crates/bitwarden-crypto/src/keys/utils.rs

@@ -17,21 +17,29 @@ pub(super) fn stretch_key(key: &Pin<Box<GenericArray<u8, U32>>>) -> Result<Aes25
 }
 
 /// Pads bytes to a minimum length using PKCS7-like padding.
-/// The last N bytes of the padded bytes all have the value N. Minimum of 1 padding byte.
-/// For example, padded to size 4, the value 0,0 becomes 0,0,2,2.
-pub(crate) fn pad_bytes(bytes: &mut Vec<u8>, min_length: usize) {
+/// The last N bytes of the padded bytes all have the value N. Minimum of 1 padding byte. maximum of
+/// 255 bytes. For example, padded to size 4, the value 0,0 becomes 0,0,2,2.
+/// If the more than 255 bytes of padding is required, this will return an error.
+pub(crate) fn pad_bytes(bytes: &mut Vec<u8>, min_length: usize) -> Result<(), CryptoError> {
     // at least 1 byte of padding is required
     let pad_bytes = min_length.saturating_sub(bytes.len()).max(1);
+    // Since each byte represents the padding size, the maximum padding is 255.
+    // If more padding is required, this will return an error.
+    if pad_bytes > 255 {
+        return Err(CryptoError::InvalidPadding);
+    }
     let padded_length = max(min_length, bytes.len() + 1);
     bytes.resize(padded_length, pad_bytes as u8);
+    Ok(())
 }
 
 /// Unpads bytes that is padded using the PKCS7-like padding defined by [pad_bytes].
 /// The last N bytes of the padded bytes all have the value N. Minimum of 1 padding byte.
 /// For example, padded to size 4, the value 0,0 becomes 0,0,2,2.
 pub(crate) fn unpad_bytes(padded_bytes: &[u8]) -> Result<&[u8], CryptoError> {
     let pad_len = *padded_bytes.last().ok_or(CryptoError::InvalidPadding)? as usize;
-    if pad_len >= padded_bytes.len() {
+    // The padding is at minimum 1 as noted in `pad_bytes`
+    if pad_len == 0 || pad_len > padded_bytes.len() {
         return Err(CryptoError::InvalidPadding);
     }
     Ok(padded_bytes[..(padded_bytes.len() - pad_len)].as_ref())
@@ -68,16 +76,121 @@ mod tests {
         );
     }
 
+    #[test]
+    fn test_pad_bytes_256_error() {
+        let mut bytes = vec![1u8; 0];
+        let result = pad_bytes(&mut bytes, 256);
+        assert!(matches!(result, Err(CryptoError::InvalidPadding)));
+    }
+
     #[test]
     fn test_pad_bytes_roundtrip() {
         let original_bytes = vec![1u8; 10];
         let mut cloned_bytes = original_bytes.clone();
         let mut encoded_bytes = vec![1u8; 12];
         encoded_bytes[10] = 2;
         encoded_bytes[11] = 2;
-        pad_bytes(&mut cloned_bytes, 12);
+        pad_bytes(&mut cloned_bytes, 12).expect("Padding failed");
         assert_eq!(encoded_bytes, cloned_bytes);
         let unpadded_bytes = unpad_bytes(&cloned_bytes).unwrap();
         assert_eq!(original_bytes, unpadded_bytes);
     }
+
+    #[test]
+    fn test_pad_bytes_roundtrip_empty() {
+        let original_bytes = Vec::new();
+        let mut cloned_bytes = original_bytes.clone();
+        pad_bytes(&mut cloned_bytes, 32).expect("Padding failed");
+        let unpadded = unpad_bytes(&cloned_bytes).unwrap();
+        assert_eq!(Vec::<u8>::new(), unpadded);
+    }
+
+    #[test]
+    fn test_unpad_bytes_invalid_empty() {
+        let data: Vec<u8> = vec![];
+        let result = unpad_bytes(&data);
+        assert!(matches!(result, Err(CryptoError::InvalidPadding)));
+    }
+
+    #[test]
+    fn test_unpad_bytes_invalid_too_large() {
+        // Last byte is 5, but only 4 bytes in total
+        let data = vec![1, 2, 3, 5];
+        let result = unpad_bytes(&data);
+        assert!(matches!(result, Err(CryptoError::InvalidPadding)));
+    }
+
+    #[test]
+    fn test_unpad_bytes_invalid_0_padding() {
+        // Padding value of 0 is invalid
+        let data = vec![1, 2, 3, 0];
+        let result = unpad_bytes(&data);
+        assert!(matches!(result, Err(CryptoError::InvalidPadding)));
+    }
+
+    #[test]
+    fn test_pad_and_unpad_bytes_range_0_to_1024() {
+        let cases: Vec<_> = (0..=1024)
+            .flat_map(|data_size| (2..=1024).map(move |padding_size| (data_size, padding_size)))
+            .collect();
+
+        let data_larger_than_padding_cases: Vec<_> = cases
+            .clone()
+            .into_iter()
+            .filter(|(data_size, padding_size)| data_size > padding_size)
+            .collect();
+        for (data_size, padding_size) in data_larger_than_padding_cases {
+            let mut data: Vec<u8> = vec![0x12; data_size];
+            let original = data.clone();
+            pad_bytes(&mut data, padding_size).expect("Padding failed");
+            let unpadded = unpad_bytes(&data).expect("Unpadding failed");
+            assert_eq!(
+                unpadded, original,
+                "Failed at size {} and padding {}",
+                data_size, padding_size
+            );
+        }
+
+        let padding_larger_than_data_cases: Vec<_> = cases
+            .clone()
+            .into_iter()
+            .filter(|(data_size, padding_size)| {
+                data_size <= padding_size && (padding_size - data_size) <= 255
+            })
+            .collect();
+        for (data_size, padding_size) in padding_larger_than_data_cases {
+            println!(
+                "Testing data_size: {}, padding_size: {}",
+                data_size, padding_size
+            );
+            let data_original: Vec<u8> = vec![0x12; data_size];
+            let mut data = data_original.clone();
+
+            pad_bytes(&mut data, padding_size).expect("Padding failed");
+            let unpadded = unpad_bytes(&data).expect("Unpadding failed");
+            assert_eq!(
+                unpadded, data_original,
+                "Failed at size {} and padding {}",
+                data_size, padding_size
+            );
+        }
+
+        let padding_massively_larger_than_data_cases: Vec<_> = cases
+            .into_iter()
+            .filter(|(data_size, padding_size)| {
+                data_size <= padding_size && (padding_size - data_size) > 255
+            })
+            .collect();
+        for (data_size, padding_size) in padding_massively_larger_than_data_cases {
+            let mut data: Vec<u8> = vec![0x12; data_size];
+            let error = pad_bytes(&mut data, padding_size);
+            assert!(
+                matches!(error, Err(CryptoError::InvalidPadding)),
+                "Expected InvalidPadding error at size {} and padding {}, but got {:?}",
+                data_size,
+                padding_size,
+                error
+            );
+        }
+    }
 }