From 6083ef56aa4ae7c035ab532b72b8a5f12290d171 Mon Sep 17 00:00:00 2001 From: Joe Doyle Date: Tue, 5 Jan 2021 20:53:07 -0800 Subject: [PATCH 1/5] Add tests to catch & characterize the overflow bug Tests 1, 2, 3, 6, and 7 fail. I suspect they behave differently with the `avx2` backend since it depends on where the boundary of the buffer is. I don't think it's easy to exploit this unless you're trying really hard. That said, I believe it's possible for sufficiently determined user code to bypass the MAX_BLOCKS check and generate looped/overflowed keystream past 256GB. --- chacha20/src/lib.rs | 1 + chacha20/tests/lib.rs | 90 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 91 insertions(+) diff --git a/chacha20/src/lib.rs b/chacha20/src/lib.rs index 78bf72b3..ce48f421 100644 --- a/chacha20/src/lib.rs +++ b/chacha20/src/lib.rs @@ -73,6 +73,7 @@ )] #![cfg_attr(docsrs, feature(doc_cfg))] #![warn(missing_docs, rust_2018_idioms, trivial_casts, unused_qualifications)] +#![feature(untagged_unions)] mod backend; #[cfg(feature = "cipher")] diff --git a/chacha20/tests/lib.rs b/chacha20/tests/lib.rs index 7f100cde..6bd1a736 100644 --- a/chacha20/tests/lib.rs +++ b/chacha20/tests/lib.rs @@ -6,6 +6,96 @@ use chacha20::ChaCha20; cipher::stream_cipher_test!(chacha20_core, ChaCha20, "chacha20"); cipher::stream_cipher_seek_test!(chacha20_seek, ChaCha20); +mod overflow { + use cipher::{NewCipher, StreamCipher, StreamCipherSeek}; + + const OFFSET_256GB: u64 = 256u64<<30; + + #[test] + fn bad_overflow_check1() { + let mut cipher = chacha20::ChaCha20::new(&Default::default(), + &Default::default()); + cipher.try_seek(OFFSET_256GB-1).expect("Couldn't seek to nearly 256GB"); + let mut data = [0u8;1]; + cipher.try_apply_keystream(&mut data) + .expect("Couldn't encrypt the last byte of 256GB"); + assert_eq!(cipher.try_current_pos::().unwrap(), OFFSET_256GB); + let mut data = [0u8;1]; + cipher.try_apply_keystream(&mut data) + .expect_err("Could encrypt past the last byte of 256GB"); + } + + #[test] + fn bad_overflow_check2() { + let mut cipher = chacha20::ChaCha20::new(&Default::default(), + &Default::default()); + cipher.try_seek(OFFSET_256GB-1).expect("Couldn't seek to nearly 256GB"); + let mut data = [0u8;2]; + cipher.try_apply_keystream(&mut data) + .expect_err("Could encrypt over the 256GB boundary"); + } + + #[test] + fn bad_overflow_check3() { + let mut cipher = chacha20::ChaCha20::new(&Default::default(), + &Default::default()); + cipher.try_seek(OFFSET_256GB-1).expect("Couldn't seek to nearly 256GB"); + let mut data = [0u8;1]; + cipher.try_apply_keystream(&mut data) + .expect("Couldn't encrypt the last byte of 256GB"); + assert_eq!(cipher.try_current_pos::().unwrap(), OFFSET_256GB); + let mut data = [0u8;63]; + cipher.try_apply_keystream(&mut data) + .expect_err("Could encrypt past the last byte of 256GB"); + } + + #[test] + fn bad_overflow_check4() { + let mut cipher = chacha20::ChaCha20::new(&Default::default(), + &Default::default()); + cipher.try_seek(OFFSET_256GB-1).expect("Couldn't seek to nearly 256GB"); + let mut data = [0u8;1]; + cipher.try_apply_keystream(&mut data) + .expect("Couldn't encrypt the last byte of 256GB"); + assert_eq!(cipher.try_current_pos::().unwrap(), OFFSET_256GB); + let mut data = [0u8;64]; + cipher.try_apply_keystream(&mut data) + .expect_err("Could encrypt past the last byte of 256GB"); + } + + #[test] + fn bad_overflow_check5() { + let mut cipher = chacha20::ChaCha20::new(&Default::default(), + &Default::default()); + cipher.try_seek(OFFSET_256GB-1).expect("Couldn't seek to nearly 256GB"); + let mut data = [0u8;1]; + cipher.try_apply_keystream(&mut data) + .expect("Couldn't encrypt the last byte of 256GB"); + assert_eq!(cipher.try_current_pos::().unwrap(), OFFSET_256GB); + let mut data = [0u8;65]; + cipher.try_apply_keystream(&mut data) + .expect_err("Could encrypt past the last byte of 256GB"); + } + + #[test] + fn bad_overflow_check6() { + let mut cipher = chacha20::ChaCha20::new(&Default::default(), + &Default::default()); + cipher.try_seek(OFFSET_256GB).expect_err("Could seek to 256GB"); + } + + #[test] + fn bad_overflow_check7() { + let mut cipher = chacha20::ChaCha20::new(&Default::default(), + &Default::default()); + if let Ok(()) = cipher.try_seek(OFFSET_256GB+63) { + let mut data = [0u8;1]; + cipher.try_apply_keystream(&mut data) + .expect_err("Could encrypt the 64th byte past the 256GB boundary"); + } + } +} + #[cfg(feature = "xchacha20")] #[rustfmt::skip] mod xchacha20 { From e18f443ae6c99f1afa637ed6035870499d94a221 Mon Sep 17 00:00:00 2001 From: Joe Doyle Date: Tue, 5 Jan 2021 22:10:07 -0800 Subject: [PATCH 2/5] Fix overflow check --- chacha20/src/chacha.rs | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/chacha20/src/chacha.rs b/chacha20/src/chacha.rs index 327ed163..49ef8cb7 100644 --- a/chacha20/src/chacha.rs +++ b/chacha20/src/chacha.rs @@ -162,8 +162,18 @@ impl StreamCipherSeek for ChaCha { fn try_seek(&mut self, pos: T) -> Result<(), LoopError> { let res = pos.to_block_byte(BLOCK_SIZE as u8)?; + let old_counter = self.counter; + let old_buffer_pos = self.buffer_pos; + self.counter = res.0; self.buffer_pos = res.1; + + if let Err(e) = self.check_data_len(&[0]) { + self.counter = old_counter; + self.buffer_pos = old_buffer_pos; + return Err(e); + } + if self.buffer_pos != 0 { self.generate_block(self.counter); } @@ -174,16 +184,14 @@ impl StreamCipherSeek for ChaCha { impl ChaCha { /// Check data length fn check_data_len(&self, data: &[u8]) -> Result<(), LoopError> { - let leftover_bytes = BUFFER_SIZE - self.buffer_pos as usize; - if data.len() < leftover_bytes { - return Ok(()); - } - let blocks = 1 + (data.len() - leftover_bytes) / BLOCK_SIZE; - let res = self.counter.checked_add(blocks as u64).ok_or(LoopError)?; - if res <= MAX_BLOCKS as u64 { - Ok(()) - } else { + let byte_after_last = self.counter + .checked_mul(BLOCK_SIZE as u64).ok_or(LoopError)? + .checked_add(self.buffer_pos as u64).ok_or(LoopError)? + .checked_add(data.len() as u64).ok_or(LoopError)?; + if byte_after_last > ((MAX_BLOCKS+1)*BLOCK_SIZE) as u64 { Err(LoopError) + } else { + Ok(()) } } From 27c4ad6ce8ea85b17f23e3809b15add6b1168ef3 Mon Sep 17 00:00:00 2001 From: Joe Doyle Date: Tue, 5 Jan 2021 22:26:40 -0800 Subject: [PATCH 3/5] rustfmt --- chacha20/src/chacha.rs | 14 +++--- chacha20/tests/lib.rs | 99 ++++++++++++++++++++++++------------------ 2 files changed, 66 insertions(+), 47 deletions(-) diff --git a/chacha20/src/chacha.rs b/chacha20/src/chacha.rs index 6ff88f46..81453e0b 100644 --- a/chacha20/src/chacha.rs +++ b/chacha20/src/chacha.rs @@ -184,11 +184,15 @@ impl StreamCipherSeek for ChaCha { impl ChaCha { /// Check data length fn check_data_len(&self, data: &[u8]) -> Result<(), LoopError> { - let byte_after_last = self.counter - .checked_mul(BLOCK_SIZE as u64).ok_or(LoopError)? - .checked_add(self.buffer_pos as u64).ok_or(LoopError)? - .checked_add(data.len() as u64).ok_or(LoopError)?; - if byte_after_last > ((MAX_BLOCKS+1)*BLOCK_SIZE) as u64 { + let byte_after_last = self + .counter + .checked_mul(BLOCK_SIZE as u64) + .ok_or(LoopError)? + .checked_add(self.buffer_pos as u64) + .ok_or(LoopError)? + .checked_add(data.len() as u64) + .ok_or(LoopError)?; + if byte_after_last > ((MAX_BLOCKS + 1) * BLOCK_SIZE) as u64 { Err(LoopError) } else { Ok(()) diff --git a/chacha20/tests/lib.rs b/chacha20/tests/lib.rs index a55f9aaf..0dd2328c 100644 --- a/chacha20/tests/lib.rs +++ b/chacha20/tests/lib.rs @@ -9,88 +9,103 @@ cipher::stream_cipher_seek_test!(chacha20_seek, ChaCha20); mod overflow { use cipher::{NewCipher, StreamCipher, StreamCipherSeek}; - const OFFSET_256GB: u64 = 256u64<<30; + const OFFSET_256GB: u64 = 256u64 << 30; #[test] fn bad_overflow_check1() { - let mut cipher = chacha20::ChaCha20::new(&Default::default(), - &Default::default()); - cipher.try_seek(OFFSET_256GB-1).expect("Couldn't seek to nearly 256GB"); - let mut data = [0u8;1]; - cipher.try_apply_keystream(&mut data) + let mut cipher = chacha20::ChaCha20::new(&Default::default(), &Default::default()); + cipher + .try_seek(OFFSET_256GB - 1) + .expect("Couldn't seek to nearly 256GB"); + let mut data = [0u8; 1]; + cipher + .try_apply_keystream(&mut data) .expect("Couldn't encrypt the last byte of 256GB"); assert_eq!(cipher.try_current_pos::().unwrap(), OFFSET_256GB); - let mut data = [0u8;1]; - cipher.try_apply_keystream(&mut data) + let mut data = [0u8; 1]; + cipher + .try_apply_keystream(&mut data) .expect_err("Could encrypt past the last byte of 256GB"); } #[test] fn bad_overflow_check2() { - let mut cipher = chacha20::ChaCha20::new(&Default::default(), - &Default::default()); - cipher.try_seek(OFFSET_256GB-1).expect("Couldn't seek to nearly 256GB"); - let mut data = [0u8;2]; - cipher.try_apply_keystream(&mut data) + let mut cipher = chacha20::ChaCha20::new(&Default::default(), &Default::default()); + cipher + .try_seek(OFFSET_256GB - 1) + .expect("Couldn't seek to nearly 256GB"); + let mut data = [0u8; 2]; + cipher + .try_apply_keystream(&mut data) .expect_err("Could encrypt over the 256GB boundary"); } #[test] fn bad_overflow_check3() { - let mut cipher = chacha20::ChaCha20::new(&Default::default(), - &Default::default()); - cipher.try_seek(OFFSET_256GB-1).expect("Couldn't seek to nearly 256GB"); - let mut data = [0u8;1]; - cipher.try_apply_keystream(&mut data) + let mut cipher = chacha20::ChaCha20::new(&Default::default(), &Default::default()); + cipher + .try_seek(OFFSET_256GB - 1) + .expect("Couldn't seek to nearly 256GB"); + let mut data = [0u8; 1]; + cipher + .try_apply_keystream(&mut data) .expect("Couldn't encrypt the last byte of 256GB"); assert_eq!(cipher.try_current_pos::().unwrap(), OFFSET_256GB); - let mut data = [0u8;63]; - cipher.try_apply_keystream(&mut data) + let mut data = [0u8; 63]; + cipher + .try_apply_keystream(&mut data) .expect_err("Could encrypt past the last byte of 256GB"); } #[test] fn bad_overflow_check4() { - let mut cipher = chacha20::ChaCha20::new(&Default::default(), - &Default::default()); - cipher.try_seek(OFFSET_256GB-1).expect("Couldn't seek to nearly 256GB"); - let mut data = [0u8;1]; - cipher.try_apply_keystream(&mut data) + let mut cipher = chacha20::ChaCha20::new(&Default::default(), &Default::default()); + cipher + .try_seek(OFFSET_256GB - 1) + .expect("Couldn't seek to nearly 256GB"); + let mut data = [0u8; 1]; + cipher + .try_apply_keystream(&mut data) .expect("Couldn't encrypt the last byte of 256GB"); assert_eq!(cipher.try_current_pos::().unwrap(), OFFSET_256GB); - let mut data = [0u8;64]; - cipher.try_apply_keystream(&mut data) + let mut data = [0u8; 64]; + cipher + .try_apply_keystream(&mut data) .expect_err("Could encrypt past the last byte of 256GB"); } #[test] fn bad_overflow_check5() { - let mut cipher = chacha20::ChaCha20::new(&Default::default(), - &Default::default()); - cipher.try_seek(OFFSET_256GB-1).expect("Couldn't seek to nearly 256GB"); - let mut data = [0u8;1]; - cipher.try_apply_keystream(&mut data) + let mut cipher = chacha20::ChaCha20::new(&Default::default(), &Default::default()); + cipher + .try_seek(OFFSET_256GB - 1) + .expect("Couldn't seek to nearly 256GB"); + let mut data = [0u8; 1]; + cipher + .try_apply_keystream(&mut data) .expect("Couldn't encrypt the last byte of 256GB"); assert_eq!(cipher.try_current_pos::().unwrap(), OFFSET_256GB); - let mut data = [0u8;65]; - cipher.try_apply_keystream(&mut data) + let mut data = [0u8; 65]; + cipher + .try_apply_keystream(&mut data) .expect_err("Could encrypt past the last byte of 256GB"); } #[test] fn bad_overflow_check6() { - let mut cipher = chacha20::ChaCha20::new(&Default::default(), - &Default::default()); - cipher.try_seek(OFFSET_256GB).expect_err("Could seek to 256GB"); + let mut cipher = chacha20::ChaCha20::new(&Default::default(), &Default::default()); + cipher + .try_seek(OFFSET_256GB) + .expect_err("Could seek to 256GB"); } #[test] fn bad_overflow_check7() { - let mut cipher = chacha20::ChaCha20::new(&Default::default(), - &Default::default()); - if let Ok(()) = cipher.try_seek(OFFSET_256GB+63) { - let mut data = [0u8;1]; - cipher.try_apply_keystream(&mut data) + let mut cipher = chacha20::ChaCha20::new(&Default::default(), &Default::default()); + if let Ok(()) = cipher.try_seek(OFFSET_256GB + 63) { + let mut data = [0u8; 1]; + cipher + .try_apply_keystream(&mut data) .expect_err("Could encrypt the 64th byte past the 256GB boundary"); } } From 6fe9809f6336e9a919962549a877212ffaf5f0e0 Mon Sep 17 00:00:00 2001 From: Joe Doyle Date: Tue, 5 Jan 2021 22:37:06 -0800 Subject: [PATCH 4/5] Remove the feature flag that nightly needed --- chacha20/src/lib.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/chacha20/src/lib.rs b/chacha20/src/lib.rs index 6dfe3027..e745ada9 100644 --- a/chacha20/src/lib.rs +++ b/chacha20/src/lib.rs @@ -74,7 +74,6 @@ )] #![cfg_attr(docsrs, feature(doc_cfg))] #![warn(missing_docs, rust_2018_idioms, trivial_casts, unused_qualifications)] -#![feature(untagged_unions)] mod backend; #[cfg(feature = "cipher")] From 36b999cb79daef7b5fa07ed2df499f1525a6b8e1 Mon Sep 17 00:00:00 2001 From: Joe Doyle Date: Tue, 5 Jan 2021 22:40:00 -0800 Subject: [PATCH 5/5] 32-bit usize exists, unfortunately --- chacha20/src/chacha.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/chacha20/src/chacha.rs b/chacha20/src/chacha.rs index 81453e0b..6ac501da 100644 --- a/chacha20/src/chacha.rs +++ b/chacha20/src/chacha.rs @@ -192,7 +192,7 @@ impl ChaCha { .ok_or(LoopError)? .checked_add(data.len() as u64) .ok_or(LoopError)?; - if byte_after_last > ((MAX_BLOCKS + 1) * BLOCK_SIZE) as u64 { + if byte_after_last > ((MAX_BLOCKS as u64) + 1) * (BLOCK_SIZE as u64) { Err(LoopError) } else { Ok(())