From a9d0dbb249abfa4df8bad3d76afce93a8720e930 Mon Sep 17 00:00:00 2001 From: Tony Arcieri Date: Sun, 28 Jul 2024 17:00:31 -0600 Subject: [PATCH] ssh-cipher: length handling improvements Adds an `Error::Length` variant and returns it whenever the input buffer for an encryption/decryption operation is not properly aligned to the cipher's block size. Also adds a private `Cipher::check_key_and_iv` method to validate keys/IVs are the expected size and return appropriate erros if they are not. --- ssh-cipher/src/decryptor.rs | 55 ++++++++++++++++--------------------- ssh-cipher/src/encryptor.rs | 42 ++++++++++++---------------- ssh-cipher/src/error.rs | 4 +++ ssh-cipher/src/lib.rs | 26 +++++++++++++++++- ssh-key/src/kdf.rs | 2 +- 5 files changed, 71 insertions(+), 58 deletions(-) diff --git a/ssh-cipher/src/decryptor.rs b/ssh-cipher/src/decryptor.rs index 3880832..d21534a 100644 --- a/ssh-cipher/src/decryptor.rs +++ b/ssh-cipher/src/decryptor.rs @@ -45,37 +45,26 @@ enum Inner { impl Decryptor { /// Create a new decryptor object with the given [`Cipher`], key, and IV. pub fn new(cipher: Cipher, key: &[u8], iv: &[u8]) -> Result { + cipher.check_key_and_iv(key, iv)?; + let inner = match cipher { #[cfg(feature = "aes-cbc")] - Cipher::Aes128Cbc => Inner::Aes128Cbc( - cbc::Decryptor::new_from_slices(key, iv).map_err(|_| Error::KeySize)?, - ), + Cipher::Aes128Cbc => cbc::Decryptor::new_from_slices(key, iv).map(Inner::Aes128Cbc), #[cfg(feature = "aes-cbc")] - Cipher::Aes192Cbc => Inner::Aes192Cbc( - cbc::Decryptor::new_from_slices(key, iv).map_err(|_| Error::KeySize)?, - ), + Cipher::Aes192Cbc => cbc::Decryptor::new_from_slices(key, iv).map(Inner::Aes192Cbc), #[cfg(feature = "aes-cbc")] - Cipher::Aes256Cbc => Inner::Aes256Cbc( - cbc::Decryptor::new_from_slices(key, iv).map_err(|_| Error::KeySize)?, - ), + Cipher::Aes256Cbc => cbc::Decryptor::new_from_slices(key, iv).map(Inner::Aes256Cbc), #[cfg(feature = "aes-ctr")] - Cipher::Aes128Ctr => { - Inner::Aes128Ctr(Ctr128BE::new_from_slices(key, iv).map_err(|_| Error::KeySize)?) - } + Cipher::Aes128Ctr => Ctr128BE::new_from_slices(key, iv).map(Inner::Aes128Ctr), #[cfg(feature = "aes-ctr")] - Cipher::Aes192Ctr => { - Inner::Aes192Ctr(Ctr128BE::new_from_slices(key, iv).map_err(|_| Error::KeySize)?) - } + Cipher::Aes192Ctr => Ctr128BE::new_from_slices(key, iv).map(Inner::Aes192Ctr), #[cfg(feature = "aes-ctr")] - Cipher::Aes256Ctr => { - Inner::Aes256Ctr(Ctr128BE::new_from_slices(key, iv).map_err(|_| Error::KeySize)?) - } + Cipher::Aes256Ctr => Ctr128BE::new_from_slices(key, iv).map(Inner::Aes256Ctr), #[cfg(feature = "tdes")] - Cipher::TDesCbc => Inner::TDesCbc( - cbc::Decryptor::new_from_slices(key, iv).map_err(|_| Error::KeySize)?, - ), + Cipher::TDesCbc => cbc::Decryptor::new_from_slices(key, iv).map(Inner::TDesCbc), _ => return Err(cipher.unsupported()), - }; + } + .map_err(|_| Error::Length)?; Ok(Self { inner }) } @@ -100,25 +89,29 @@ impl Decryptor { } } - /// Decrypt the given buffer in place, returning [`Error::Crypto`] on padding failure. + /// Decrypt the given buffer in place. + /// + /// Returns [`Error::Length`] in the event that `buffer` is not a multiple of the cipher's + /// block size. pub fn decrypt(&mut self, buffer: &mut [u8]) -> Result<()> { #[cfg(any(feature = "aes-cbc", feature = "aes-ctr", feature = "tdes"))] match &mut self.inner { #[cfg(feature = "aes-cbc")] - Inner::Aes128Cbc(cipher) => cbc_decrypt(cipher, buffer)?, + Inner::Aes128Cbc(cipher) => cbc_decrypt(cipher, buffer), #[cfg(feature = "aes-cbc")] - Inner::Aes192Cbc(cipher) => cbc_decrypt(cipher, buffer)?, + Inner::Aes192Cbc(cipher) => cbc_decrypt(cipher, buffer), #[cfg(feature = "aes-cbc")] - Inner::Aes256Cbc(cipher) => cbc_decrypt(cipher, buffer)?, + Inner::Aes256Cbc(cipher) => cbc_decrypt(cipher, buffer), #[cfg(feature = "aes-ctr")] - Inner::Aes128Ctr(cipher) => ctr_decrypt(cipher, buffer)?, + Inner::Aes128Ctr(cipher) => ctr_decrypt(cipher, buffer), #[cfg(feature = "aes-ctr")] - Inner::Aes192Ctr(cipher) => ctr_decrypt(cipher, buffer)?, + Inner::Aes192Ctr(cipher) => ctr_decrypt(cipher, buffer), #[cfg(feature = "aes-ctr")] - Inner::Aes256Ctr(cipher) => ctr_decrypt(cipher, buffer)?, + Inner::Aes256Ctr(cipher) => ctr_decrypt(cipher, buffer), #[cfg(feature = "tdes")] - Inner::TDesCbc(cipher) => cbc_decrypt(cipher, buffer)?, + Inner::TDesCbc(cipher) => cbc_decrypt(cipher, buffer), } + .map_err(|_| Error::Length)?; Ok(()) } @@ -134,7 +127,7 @@ where // Ensure input is block-aligned. if !remaining.is_empty() { - return Err(Error::Crypto); + return Err(Error::Length); } decryptor.decrypt_blocks(blocks); diff --git a/ssh-cipher/src/encryptor.rs b/ssh-cipher/src/encryptor.rs index d71f6cb..f69d8cd 100644 --- a/ssh-cipher/src/encryptor.rs +++ b/ssh-cipher/src/encryptor.rs @@ -48,37 +48,26 @@ enum Inner { impl Encryptor { /// Create a new encryptor object with the given [`Cipher`], key, and IV. pub fn new(cipher: Cipher, key: &[u8], iv: &[u8]) -> Result { + cipher.check_key_and_iv(key, iv)?; + let inner = match cipher { #[cfg(feature = "aes-cbc")] - Cipher::Aes128Cbc => Inner::Aes128Cbc( - cbc::Encryptor::new_from_slices(key, iv).map_err(|_| Error::KeySize)?, - ), + Cipher::Aes128Cbc => cbc::Encryptor::new_from_slices(key, iv).map(Inner::Aes128Cbc), #[cfg(feature = "aes-cbc")] - Cipher::Aes192Cbc => Inner::Aes192Cbc( - cbc::Encryptor::new_from_slices(key, iv).map_err(|_| Error::KeySize)?, - ), + Cipher::Aes192Cbc => cbc::Encryptor::new_from_slices(key, iv).map(Inner::Aes192Cbc), #[cfg(feature = "aes-cbc")] - Cipher::Aes256Cbc => Inner::Aes256Cbc( - cbc::Encryptor::new_from_slices(key, iv).map_err(|_| Error::KeySize)?, - ), + Cipher::Aes256Cbc => cbc::Encryptor::new_from_slices(key, iv).map(Inner::Aes256Cbc), #[cfg(feature = "aes-ctr")] - Cipher::Aes128Ctr => { - Inner::Aes128Ctr(Ctr128BE::new_from_slices(key, iv).map_err(|_| Error::KeySize)?) - } + Cipher::Aes128Ctr => Ctr128BE::new_from_slices(key, iv).map(Inner::Aes128Ctr), #[cfg(feature = "aes-ctr")] - Cipher::Aes192Ctr => { - Inner::Aes192Ctr(Ctr128BE::new_from_slices(key, iv).map_err(|_| Error::KeySize)?) - } + Cipher::Aes192Ctr => Ctr128BE::new_from_slices(key, iv).map(Inner::Aes192Ctr), #[cfg(feature = "aes-ctr")] - Cipher::Aes256Ctr => { - Inner::Aes256Ctr(Ctr128BE::new_from_slices(key, iv).map_err(|_| Error::KeySize)?) - } + Cipher::Aes256Ctr => Ctr128BE::new_from_slices(key, iv).map(Inner::Aes256Ctr), #[cfg(feature = "tdes")] - Cipher::TDesCbc => Inner::TDesCbc( - cbc::Encryptor::new_from_slices(key, iv).map_err(|_| Error::KeySize)?, - ), + Cipher::TDesCbc => cbc::Encryptor::new_from_slices(key, iv).map(Inner::TDesCbc), _ => return Err(cipher.unsupported()), - }; + } + .map_err(|_| Error::Length)?; Ok(Self { inner }) } @@ -103,7 +92,10 @@ impl Encryptor { } } - /// Encrypt the given buffer in place, returning [`Error::Crypto`] on padding failure. + /// Encrypt the given buffer in place. + /// + /// Returns [`Error::Length`] in the event that `buffer` is not a multiple of the cipher's + /// block size. pub fn encrypt(&mut self, buffer: &mut [u8]) -> Result<()> { match &mut self.inner { #[cfg(feature = "aes-cbc")] @@ -136,7 +128,7 @@ where // Ensure input is block-aligned. if !remaining.is_empty() { - return Err(Error::Crypto); + return Err(Error::Length); } encryptor.encrypt_blocks(blocks); @@ -153,7 +145,7 @@ where // Ensure input is block-aligned. if !remaining.is_empty() { - return Err(Error::Crypto); + return Err(Error::Length); } encryptor.apply_keystream_blocks(blocks); diff --git a/ssh-cipher/src/error.rs b/ssh-cipher/src/error.rs index c597dea..d9fd830 100644 --- a/ssh-cipher/src/error.rs +++ b/ssh-cipher/src/error.rs @@ -16,6 +16,9 @@ pub enum Error { /// Invalid key size. KeySize, + /// Invalid length (i.e. of an input buffer). + Length, + /// Invalid initialization vector / nonce size. IvSize, @@ -31,6 +34,7 @@ impl fmt::Display for Error { match self { Error::Crypto => write!(f, "cryptographic error"), Error::KeySize => write!(f, "invalid key size"), + Error::Length => write!(f, "invalid input length"), Error::IvSize => write!(f, "invalid initialization vector size"), Error::TagSize => write!(f, "invalid AEAD tag size"), Error::UnsupportedCipher(cipher) => write!(f, "unsupported cipher: {}", cipher), diff --git a/ssh-cipher/src/lib.rs b/ssh-cipher/src/lib.rs index 2437c03..83d1114 100644 --- a/ssh-cipher/src/lib.rs +++ b/ssh-cipher/src/lib.rs @@ -92,7 +92,7 @@ pub type Nonce = [u8; 12]; /// `chacha20-poly1305@openssh.com`. pub type Tag = [u8; 16]; -/// Counter mode with a 32-bit big endian counter. +/// Counter mode with a 128-bit big endian counter. #[cfg(feature = "aes-ctr")] type Ctr128BE = ctr::CtrCore; @@ -221,6 +221,9 @@ impl Cipher { } /// Decrypt the ciphertext in the `buffer` in-place using this cipher. + /// + /// Returns [`Error::Length`] in the event that `buffer` is not a multiple of the cipher's + /// block size. #[cfg_attr( not(any(feature = "aes-cbc", feature = "aes-ctr", feature = "tdes")), allow(unused_variables) @@ -279,6 +282,9 @@ impl Cipher { } /// Encrypt the ciphertext in the `buffer` in-place using this cipher. + /// + /// Returns [`Error::Length`] in the event that `buffer` is not a multiple of the cipher's + /// block size. #[cfg_attr( not(any(feature = "aes-cbc", feature = "aes-ctr", feature = "tdes")), allow(unused_variables) @@ -330,6 +336,24 @@ impl Cipher { Encryptor::new(self, key, iv) } + /// Check that the key and IV are the expected length for this cipher. + #[cfg(any(feature = "aes-cbc", feature = "aes-ctr", feature = "tdes"))] + fn check_key_and_iv(self, key: &[u8], iv: &[u8]) -> Result<()> { + let (key_size, iv_size) = self + .key_and_iv_size() + .ok_or(Error::UnsupportedCipher(self))?; + + if key.len() != key_size { + return Err(Error::KeySize); + } + + if iv.len() != iv_size { + return Err(Error::IvSize); + } + + Ok(()) + } + /// Create an unsupported cipher error. fn unsupported(self) -> Error { Error::UnsupportedCipher(self) diff --git a/ssh-key/src/kdf.rs b/ssh-key/src/kdf.rs index a7ddaa3..cfbc6cc 100644 --- a/ssh-key/src/kdf.rs +++ b/ssh-key/src/kdf.rs @@ -120,7 +120,7 @@ impl Kdf { okm.truncate(32); // Use an all-zero nonce (with a key derived from password + salt providing uniqueness) - iv.extend_from_slice(&cipher::Nonce::default()); + iv = cipher::Nonce::default().to_vec(); } Ok((okm, iv))