Skip to content

Commit 49f84e6

Browse files
authored
[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
1 parent ae9b9f1 commit 49f84e6

File tree

4 files changed

+189
-9
lines changed

4 files changed

+189
-9
lines changed

crates/bitwarden-crypto/src/cose.rs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,9 @@ pub(crate) fn encrypt_xchacha20_poly1305(
5757

5858
if should_pad_content(&content_format) {
5959
// Pad the data to a block size in order to hide plaintext length
60-
crate::keys::utils::pad_bytes(&mut plaintext, XCHACHA20_TEXT_PAD_BLOCK_SIZE);
60+
let min_length =
61+
XCHACHA20_TEXT_PAD_BLOCK_SIZE * (1 + (plaintext.len() / XCHACHA20_TEXT_PAD_BLOCK_SIZE));
62+
crate::keys::utils::pad_bytes(&mut plaintext, min_length)?;
6163
}
6264

6365
let mut nonce = [0u8; xchacha20::NONCE_SIZE];

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

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -364,6 +364,44 @@ mod tests {
364364
KEY_ID_SIZE,
365365
};
366366

367+
fn encrypt_with_xchacha20(plaintext: &str) -> EncString {
368+
let key_id = [0u8; KEY_ID_SIZE];
369+
let enc_key = [0u8; 32];
370+
let key = SymmetricCryptoKey::XChaCha20Poly1305Key(crate::XChaCha20Poly1305Key {
371+
key_id,
372+
enc_key: Box::pin(enc_key.into()),
373+
});
374+
375+
plaintext.encrypt_with_key(&key).expect("encryption works")
376+
}
377+
378+
/// XChaCha20Poly1305 encstrings should be padded in blocks of 32 bytes. This ensures that the
379+
/// encstring length does not reveal more than the 32-byte range of lengths that the contained
380+
/// string falls into.
381+
#[test]
382+
fn test_xchacha20_encstring_string_padding_block_sizes() {
383+
let cases = [
384+
("", 32), // empty string, padded to 32
385+
(&"a".repeat(31), 32), // largest in first block
386+
(&"a".repeat(32), 64), // smallest in second block
387+
(&"a".repeat(63), 64), // largest in second block
388+
(&"a".repeat(64), 96), // smallest in third block
389+
];
390+
391+
let ciphertext_lengths: Vec<_> = cases
392+
.iter()
393+
.map(|(plaintext, _)| encrypt_with_xchacha20(plaintext).to_string().len())
394+
.collect();
395+
396+
// Block 1: 0-31 (same length)
397+
assert_eq!(ciphertext_lengths[0], ciphertext_lengths[1]);
398+
// Block 2: 32-63 (same length, different from block 1)
399+
assert_ne!(ciphertext_lengths[1], ciphertext_lengths[2]);
400+
assert_eq!(ciphertext_lengths[2], ciphertext_lengths[3]);
401+
// Block 3: 64+ (different from block 2)
402+
assert_ne!(ciphertext_lengths[3], ciphertext_lengths[4]);
403+
}
404+
367405
#[test]
368406
fn test_enc_roundtrip_xchacha20() {
369407
let key_id = [0u8; KEY_ID_SIZE];
@@ -390,6 +428,32 @@ mod tests {
390428
assert_eq!(decrypted_str, test_string);
391429
}
392430

431+
#[test]
432+
fn test_enc_roundtrip_xchacha20_empty() {
433+
let key_id = [0u8; KEY_ID_SIZE];
434+
let enc_key = [0u8; 32];
435+
let key = SymmetricCryptoKey::XChaCha20Poly1305Key(crate::XChaCha20Poly1305Key {
436+
key_id,
437+
enc_key: Box::pin(enc_key.into()),
438+
});
439+
440+
let test_string = "";
441+
let cipher = test_string.to_owned().encrypt_with_key(&key).unwrap();
442+
let decrypted_str: String = cipher.decrypt_with_key(&key).unwrap();
443+
assert_eq!(decrypted_str, test_string);
444+
}
445+
446+
#[test]
447+
fn test_enc_string_roundtrip_empty() {
448+
let key = SymmetricCryptoKey::Aes256CbcHmacKey(derive_symmetric_key("test"));
449+
450+
let test_string = "";
451+
let cipher = test_string.to_string().encrypt_with_key(&key).unwrap();
452+
453+
let decrypted_str: String = cipher.decrypt_with_key(&key).unwrap();
454+
assert_eq!(decrypted_str, test_string);
455+
}
456+
393457
#[test]
394458
fn test_enc_string_ref_roundtrip() {
395459
let key = SymmetricCryptoKey::Aes256CbcHmacKey(derive_symmetric_key("test"));

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

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -154,7 +154,7 @@ impl SymmetricCryptoKey {
154154
}
155155
EncodedSymmetricKey::CoseKey(_) => {
156156
let mut encoded_key: Vec<u8> = encoded_key.into();
157-
pad_key(&mut encoded_key, Self::AES256_CBC_HMAC_KEY_LEN + 1);
157+
pad_key(&mut encoded_key, (Self::AES256_CBC_HMAC_KEY_LEN + 1) as u8); // This is less than 255
158158
BitwardenLegacyKeyBytes::from(encoded_key)
159159
}
160160
}
@@ -379,8 +379,9 @@ impl std::fmt::Debug for XChaCha20Poly1305Key {
379379
/// padding is used to make sure that the byte representation uniquely separates the keys by
380380
/// size of the byte array. The previous key types [SymmetricCryptoKey::Aes256CbcHmacKey] and
381381
/// [SymmetricCryptoKey::Aes256CbcKey] are 64 and 32 bytes long respectively.
382-
fn pad_key(key_bytes: &mut Vec<u8>, min_length: usize) {
383-
crate::keys::utils::pad_bytes(key_bytes, min_length);
382+
fn pad_key(key_bytes: &mut Vec<u8>, min_length: u8) {
383+
crate::keys::utils::pad_bytes(key_bytes, min_length as usize)
384+
.expect("Padding cannot fail since the min_length is < 255")
384385
}
385386

386387
/// Unpad a key that is padded using the PKCS7-like padding defined by [pad_key].

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

Lines changed: 118 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,21 +17,29 @@ pub(super) fn stretch_key(key: &Pin<Box<GenericArray<u8, U32>>>) -> Result<Aes25
1717
}
1818

1919
/// Pads bytes to a minimum length using PKCS7-like padding.
20-
/// The last N bytes of the padded bytes all have the value N. Minimum of 1 padding byte.
21-
/// For example, padded to size 4, the value 0,0 becomes 0,0,2,2.
22-
pub(crate) fn pad_bytes(bytes: &mut Vec<u8>, min_length: usize) {
20+
/// The last N bytes of the padded bytes all have the value N. Minimum of 1 padding byte. maximum of
21+
/// 255 bytes. For example, padded to size 4, the value 0,0 becomes 0,0,2,2.
22+
/// If the more than 255 bytes of padding is required, this will return an error.
23+
pub(crate) fn pad_bytes(bytes: &mut Vec<u8>, min_length: usize) -> Result<(), CryptoError> {
2324
// at least 1 byte of padding is required
2425
let pad_bytes = min_length.saturating_sub(bytes.len()).max(1);
26+
// Since each byte represents the padding size, the maximum padding is 255.
27+
// If more padding is required, this will return an error.
28+
if pad_bytes > 255 {
29+
return Err(CryptoError::InvalidPadding);
30+
}
2531
let padded_length = max(min_length, bytes.len() + 1);
2632
bytes.resize(padded_length, pad_bytes as u8);
33+
Ok(())
2734
}
2835

2936
/// Unpads bytes that is padded using the PKCS7-like padding defined by [pad_bytes].
3037
/// The last N bytes of the padded bytes all have the value N. Minimum of 1 padding byte.
3138
/// For example, padded to size 4, the value 0,0 becomes 0,0,2,2.
3239
pub(crate) fn unpad_bytes(padded_bytes: &[u8]) -> Result<&[u8], CryptoError> {
3340
let pad_len = *padded_bytes.last().ok_or(CryptoError::InvalidPadding)? as usize;
34-
if pad_len >= padded_bytes.len() {
41+
// The padding is at minimum 1 as noted in `pad_bytes`
42+
if pad_len == 0 || pad_len > padded_bytes.len() {
3543
return Err(CryptoError::InvalidPadding);
3644
}
3745
Ok(padded_bytes[..(padded_bytes.len() - pad_len)].as_ref())
@@ -68,16 +76,121 @@ mod tests {
6876
);
6977
}
7078

79+
#[test]
80+
fn test_pad_bytes_256_error() {
81+
let mut bytes = vec![1u8; 0];
82+
let result = pad_bytes(&mut bytes, 256);
83+
assert!(matches!(result, Err(CryptoError::InvalidPadding)));
84+
}
85+
7186
#[test]
7287
fn test_pad_bytes_roundtrip() {
7388
let original_bytes = vec![1u8; 10];
7489
let mut cloned_bytes = original_bytes.clone();
7590
let mut encoded_bytes = vec![1u8; 12];
7691
encoded_bytes[10] = 2;
7792
encoded_bytes[11] = 2;
78-
pad_bytes(&mut cloned_bytes, 12);
93+
pad_bytes(&mut cloned_bytes, 12).expect("Padding failed");
7994
assert_eq!(encoded_bytes, cloned_bytes);
8095
let unpadded_bytes = unpad_bytes(&cloned_bytes).unwrap();
8196
assert_eq!(original_bytes, unpadded_bytes);
8297
}
98+
99+
#[test]
100+
fn test_pad_bytes_roundtrip_empty() {
101+
let original_bytes = Vec::new();
102+
let mut cloned_bytes = original_bytes.clone();
103+
pad_bytes(&mut cloned_bytes, 32).expect("Padding failed");
104+
let unpadded = unpad_bytes(&cloned_bytes).unwrap();
105+
assert_eq!(Vec::<u8>::new(), unpadded);
106+
}
107+
108+
#[test]
109+
fn test_unpad_bytes_invalid_empty() {
110+
let data: Vec<u8> = vec![];
111+
let result = unpad_bytes(&data);
112+
assert!(matches!(result, Err(CryptoError::InvalidPadding)));
113+
}
114+
115+
#[test]
116+
fn test_unpad_bytes_invalid_too_large() {
117+
// Last byte is 5, but only 4 bytes in total
118+
let data = vec![1, 2, 3, 5];
119+
let result = unpad_bytes(&data);
120+
assert!(matches!(result, Err(CryptoError::InvalidPadding)));
121+
}
122+
123+
#[test]
124+
fn test_unpad_bytes_invalid_0_padding() {
125+
// Padding value of 0 is invalid
126+
let data = vec![1, 2, 3, 0];
127+
let result = unpad_bytes(&data);
128+
assert!(matches!(result, Err(CryptoError::InvalidPadding)));
129+
}
130+
131+
#[test]
132+
fn test_pad_and_unpad_bytes_range_0_to_1024() {
133+
let cases: Vec<_> = (0..=1024)
134+
.flat_map(|data_size| (2..=1024).map(move |padding_size| (data_size, padding_size)))
135+
.collect();
136+
137+
let data_larger_than_padding_cases: Vec<_> = cases
138+
.clone()
139+
.into_iter()
140+
.filter(|(data_size, padding_size)| data_size > padding_size)
141+
.collect();
142+
for (data_size, padding_size) in data_larger_than_padding_cases {
143+
let mut data: Vec<u8> = vec![0x12; data_size];
144+
let original = data.clone();
145+
pad_bytes(&mut data, padding_size).expect("Padding failed");
146+
let unpadded = unpad_bytes(&data).expect("Unpadding failed");
147+
assert_eq!(
148+
unpadded, original,
149+
"Failed at size {} and padding {}",
150+
data_size, padding_size
151+
);
152+
}
153+
154+
let padding_larger_than_data_cases: Vec<_> = cases
155+
.clone()
156+
.into_iter()
157+
.filter(|(data_size, padding_size)| {
158+
data_size <= padding_size && (padding_size - data_size) <= 255
159+
})
160+
.collect();
161+
for (data_size, padding_size) in padding_larger_than_data_cases {
162+
println!(
163+
"Testing data_size: {}, padding_size: {}",
164+
data_size, padding_size
165+
);
166+
let data_original: Vec<u8> = vec![0x12; data_size];
167+
let mut data = data_original.clone();
168+
169+
pad_bytes(&mut data, padding_size).expect("Padding failed");
170+
let unpadded = unpad_bytes(&data).expect("Unpadding failed");
171+
assert_eq!(
172+
unpadded, data_original,
173+
"Failed at size {} and padding {}",
174+
data_size, padding_size
175+
);
176+
}
177+
178+
let padding_massively_larger_than_data_cases: Vec<_> = cases
179+
.into_iter()
180+
.filter(|(data_size, padding_size)| {
181+
data_size <= padding_size && (padding_size - data_size) > 255
182+
})
183+
.collect();
184+
for (data_size, padding_size) in padding_massively_larger_than_data_cases {
185+
let mut data: Vec<u8> = vec![0x12; data_size];
186+
let error = pad_bytes(&mut data, padding_size);
187+
assert!(
188+
matches!(error, Err(CryptoError::InvalidPadding)),
189+
"Expected InvalidPadding error at size {} and padding {}, but got {:?}",
190+
data_size,
191+
padding_size,
192+
error
193+
);
194+
}
195+
}
83196
}

0 commit comments

Comments
 (0)