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

@@ -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<Self> {
+        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);

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<Self> {
+        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);

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),

ssh-cipher/src/lib.rs

@@ -92,7 +92,7 @@ pub type Nonce = [u8; 12];
 /// `[email protected]`.
 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<Cipher> = ctr::CtrCore<Cipher, ctr::flavors::Ctr128BE>;
 
@@ -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)

ssh-key/src/kdf.rs

@@ -86,26 +86,7 @@ impl Kdf {
         cipher: Cipher,
         password: impl AsRef<[u8]>,
     ) -> Result<(Zeroizing<Vec<u8>>, Vec<u8>)> {
-        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 [email protected] 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 `[email protected]` 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))