From 97b31daed34281e31bfd05e5834a9dc47a7cc348 Mon Sep 17 00:00:00 2001 From: Tony Arcieri Date: Sun, 28 Jul 2024 18:00:43 -0600 Subject: [PATCH] ssh-cipher: length handling improvements (#255) 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 | 30 ++++---------------- 5 files changed, 75 insertions(+), 82 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..454b084 100644 --- a/ssh-key/src/kdf.rs +++ b/ssh-key/src/kdf.rs @@ -86,26 +86,7 @@ impl Kdf { cipher: Cipher, password: impl AsRef<[u8]>, ) -> Result<(Zeroizing>, Vec)> { - let (key_size, iv_size) = match cipher { - // Derive two ChaCha20Poly1305 keys, but only use the first. - // In the typical SSH protocol, the second key is used for length encryption. - // - // From `PROTOCOL.chacha20poly1305`: - // - // > The chacha20-poly1305@openssh.com cipher requires 512 bits of key - // > material as output from the SSH key exchange. This forms two 256 bit - // > keys (K_1 and K_2), used by two separate instances of chacha20. - // > The first 256 bits constitute K_2 and the second 256 bits become - // > K_1. - // > - // > The instance keyed by K_1 is a stream cipher that is used only - // > to encrypt the 4 byte packet length field. The second instance, - // > keyed by K_2, is used in conjunction with poly1305 to build an AEAD - // > (Authenticated Encryption with Associated Data) that is used to encrypt - // > and authenticate the entire packet. - Cipher::ChaCha20Poly1305 => (64, 0), - _ => cipher.key_and_iv_size().ok_or(Error::Decrypted)?, - }; + let (key_size, iv_size) = cipher.key_and_iv_size().ok_or(Error::Decrypted)?; let okm_size = key_size .checked_add(iv_size) @@ -115,12 +96,11 @@ impl Kdf { self.derive(password, &mut okm)?; let mut iv = okm.split_off(key_size); + // For whatever reason `chacha20-poly1305@openssh.com` uses a nonce of all zeros for private + // key encryption, relying on a unique salt used in the password-based encryption key + // derivation to ensure that each encryption key is only used once. if cipher == Cipher::ChaCha20Poly1305 { - // Only use the first ChaCha20 key. - 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.copy_from_slice(&cipher::Nonce::default()); } Ok((okm, iv))