ssh-cipher: make Encryptor and Decryptor generic around cipher (#533)

Adds a `BlockCipher` extension trait and defines it for the internally
defined multi-key-size `Aes` type as well as `des::TdesEde3` which is
now re-exported as `ssh_cipher::block_cipher::Tdes`.

The `Decryptor` and `Encryptor` types are now generic around a sealed
`BlockCipher` trait, and have `AesDecryptor`/`AesEncryptor` and
`TdesDecryptor`/`TdesEncryptor` type aliases. They both support CBC
mode, and AES ciphers support CTR too. The API is otherwise the same
except callers need to use one of the typed `*Encryptor`/`*Decryptor`
traits.

The crate now implements both block cipher modes of operation internally
and no longer relies on the `cbc` and `ctr` crates. This makes
dynamically dispatching between the two modes easier while avoiding some
monomorphization bloat, and actually doesn't add significant complexity.

With `Decryptor`/`Encryptor` now well-typed and supporting typed block
sizes, it should be possible to make them impelement the `BlockMode*`
traits and therefore make them work like the other block modes, but that
particular change is deferred for a followup PR.

Author: tarcieri

Cargo.lock

@@ -25,8 +25,6 @@ encoding = { package = "ssh-encoding", version = "0.3.0-rc.9" }
 aead = { version = "0.6.0-rc.10", optional = true, default-features = false }
 aes = { version = "0.9", optional = true, default-features = false }
 aes-gcm = { version = "0.11.0-rc.3", optional = true, default-features = false, features = ["aes"] }
-cbc = { version = "0.2.1", optional = true }
-ctr = { version = "0.10", optional = true, default-features = false }
 ctutils = { version = "0.4", optional = true, default-features = false }
 chacha20 = { version = "0.10", optional = true, default-features = false, features = ["cipher", "legacy"] }
 des = { version = "0.9", optional = true, default-features = false }
@@ -37,9 +35,9 @@ zeroize = { version = "1", optional = true, default-features = false }
 hex-literal = "1"
 
 [features]
-aes = ["dep:aead", "dep:aes", "dep:aes-gcm", "dep:cbc", "dep:ctr"]
+aes = ["dep:aead", "dep:aes", "dep:aes-gcm"]
 chacha20poly1305 = ["dep:aead", "dep:chacha20", "dep:poly1305", "dep:ctutils"]
-tdes = ["dep:des", "dep:cbc"]
+tdes = ["dep:des"]
 zeroize = [
     "dep:zeroize",
     "aes?/zeroize",

ssh-cipher/Cargo.toml

@@ -7,8 +7,77 @@
 mod aes;
 mod decryptor;
 mod encryptor;
+mod state;
 
+#[cfg(feature = "aes")]
+pub use self::aes::Aes;
 pub use self::{decryptor::Decryptor, encryptor::Encryptor};
+#[cfg(feature = "tdes")]
+pub use ::des::TdesEde3 as Tdes;
+
+use self::state::State;
+
+#[cfg(feature = "tdes")]
+use {
+    crate::Cipher,
+    ::cipher::common::{InvalidLength, KeyInit},
+};
+
+/// Seal the `BlockCipher` trait so others cannot implement it.
+pub(crate) mod sealed {
+    use crate::Cipher;
+    use ::cipher::{BlockCipherDecrypt, BlockCipherEncrypt, common::InvalidLength};
+
+    /// Trait for block ciphers supported by this crate.
+    ///
+    /// This trait is deliberately sealed so it cannot be implemented by downstream crates.
+    /// Notably new ciphers added to SSH should be authenticated, and we shouldn't support a
+    /// proliferation of unauthenticated ciphers.
+    pub trait BlockCipher: BlockCipherDecrypt + BlockCipherEncrypt {
+        /// Initialize cipher from a byte slice.
+        ///
+        /// This is defined separate from the [`KeyInit`] trait so it can support variable-sized keys.
+        ///
+        /// # Errors
+        /// Returns [`InvalidLength`] if `slice` is not equal in length to the key size.
+        fn new_from_slice(slice: &[u8]) -> Result<Self, InvalidLength>;
+
+        /// Is this the correct block cipher implementation for the given cipher?
+        fn is_supported(cipher: Cipher) -> bool;
+    }
+}
 
+/// Supported block cipher modes of operation.
+#[derive(Copy, Clone, Debug, Eq, PartialEq)]
+pub(crate) enum BlockMode {
+    /// Cipher block chaining.
+    Cbc,
+
+    /// Counter mode.
+    Ctr,
+}
+
+/// Encryptor for the Advanced Encryption Standard (AES).
+#[cfg(feature = "aes")]
+pub type AesEncryptor = Encryptor<Aes>;
+/// Decryptor for the Advanced Encryption Standard (AES).
 #[cfg(feature = "aes")]
-pub(crate) use self::aes::Aes;
+pub type AesDecryptor = Decryptor<Aes>;
+
+/// Encryptor for 3DES.
+#[cfg(feature = "tdes")]
+pub type TdesEncryptor = Encryptor<Tdes>;
+/// Decryptor for 3DES.
+#[cfg(feature = "tdes")]
+pub type TdesDecryptor = Decryptor<Tdes>;
+
+#[cfg(feature = "tdes")]
+impl sealed::BlockCipher for Tdes {
+    fn new_from_slice(slice: &[u8]) -> Result<Self, InvalidLength> {
+        KeyInit::new_from_slice(slice)
+    }
+
+    fn is_supported(cipher: Cipher) -> bool {
+        cipher == Cipher::TdesCbc
+    }
+}

ssh-cipher/src/block_cipher.rs

@@ -1,15 +1,24 @@
 //! AES block cipher.
 
+use super::sealed::BlockCipher;
+use crate::Cipher;
 use ::aes::{Aes128, Aes192, Aes256, Block};
 use ::cipher::{
     BlockCipherDecClosure, BlockCipherDecrypt, BlockCipherEncClosure, BlockCipherEncrypt,
     BlockSizeUser, InvalidLength, KeyInit, array::sizes::U16,
 };
+use core::fmt;
+use core::fmt::Debug;
 
 /// Advanced Encryption Standard (AES) low-level block cipher.
 ///
-/// Supports 128-bit, 192-bit, and 256-bit key sizes.
-pub(crate) enum Aes {
+/// Supports 128-bit, 192-bit, and 256-bit keys.
+pub struct Aes {
+    inner: Inner,
+}
+
+/// Inner enum over supported key sizes.
+enum Inner {
     Aes128(Aes128),
     Aes192(Aes192),
     Aes256(Aes256),
@@ -23,59 +32,89 @@ impl Aes {
     ///
     /// # Errors
     /// Returns [`InvalidLength`] if the length of `key` is not any of the above.
-    pub(crate) fn new(key: &[u8]) -> Result<Self, InvalidLength> {
+    pub fn new_from_slice(key: &[u8]) -> Result<Self, InvalidLength> {
         if let Ok(cipher) = Aes128::new_from_slice(key) {
-            return Ok(Aes::Aes128(cipher));
+            return Ok(Self {
+                inner: Inner::Aes128(cipher),
+            });
         }
 
         if let Ok(cipher) = Aes192::new_from_slice(key) {
-            return Ok(Aes::Aes192(cipher));
+            return Ok(Self {
+                inner: Inner::Aes192(cipher),
+            });
         }
 
         if let Ok(cipher) = Aes256::new_from_slice(key) {
-            return Ok(Aes::Aes256(cipher));
+            return Ok(Self {
+                inner: Inner::Aes256(cipher),
+            });
         }
 
         Err(InvalidLength)
     }
 }
 
+impl BlockCipher for Aes {
+    fn new_from_slice(slice: &[u8]) -> Result<Self, InvalidLength> {
+        Aes::new_from_slice(slice)
+    }
+
+    fn is_supported(cipher: Cipher) -> bool {
+        matches!(
+            cipher,
+            Cipher::Aes128Cbc
+                | Cipher::Aes192Cbc
+                | Cipher::Aes256Cbc
+                | Cipher::Aes128Ctr
+                | Cipher::Aes192Ctr
+                | Cipher::Aes256Ctr
+        )
+    }
+}
+
 impl BlockCipherDecrypt for Aes {
     fn decrypt_blocks(&self, blocks: &mut [Block]) {
-        match self {
-            Aes::Aes128(aes) => aes.decrypt_blocks(blocks),
-            Aes::Aes192(aes) => aes.decrypt_blocks(blocks),
-            Aes::Aes256(aes) => aes.decrypt_blocks(blocks),
+        match &self.inner {
+            Inner::Aes128(aes) => aes.decrypt_blocks(blocks),
+            Inner::Aes192(aes) => aes.decrypt_blocks(blocks),
+            Inner::Aes256(aes) => aes.decrypt_blocks(blocks),
         }
     }
 
     fn decrypt_with_backend(&self, f: impl BlockCipherDecClosure<BlockSize = Self::BlockSize>) {
-        match self {
-            Aes::Aes128(aes) => aes.decrypt_with_backend(f),
-            Aes::Aes192(aes) => aes.decrypt_with_backend(f),
-            Aes::Aes256(aes) => aes.decrypt_with_backend(f),
+        match &self.inner {
+            Inner::Aes128(aes) => aes.decrypt_with_backend(f),
+            Inner::Aes192(aes) => aes.decrypt_with_backend(f),
+            Inner::Aes256(aes) => aes.decrypt_with_backend(f),
         }
     }
 }
 
 impl BlockCipherEncrypt for Aes {
     fn encrypt_blocks(&self, blocks: &mut [Block]) {
-        match self {
-            Aes::Aes128(aes) => aes.encrypt_blocks(blocks),
-            Aes::Aes192(aes) => aes.encrypt_blocks(blocks),
-            Aes::Aes256(aes) => aes.encrypt_blocks(blocks),
+        match &self.inner {
+            Inner::Aes128(aes) => aes.encrypt_blocks(blocks),
+            Inner::Aes192(aes) => aes.encrypt_blocks(blocks),
+            Inner::Aes256(aes) => aes.encrypt_blocks(blocks),
         }
     }
 
     fn encrypt_with_backend(&self, f: impl BlockCipherEncClosure<BlockSize = Self::BlockSize>) {
-        match self {
-            Aes::Aes128(aes) => aes.encrypt_with_backend(f),
-            Aes::Aes192(aes) => aes.encrypt_with_backend(f),
-            Aes::Aes256(aes) => aes.encrypt_with_backend(f),
+        match &self.inner {
+            Inner::Aes128(aes) => aes.encrypt_with_backend(f),
+            Inner::Aes192(aes) => aes.encrypt_with_backend(f),
+            Inner::Aes256(aes) => aes.encrypt_with_backend(f),
         }
     }
 }
 
 impl BlockSizeUser for Aes {
     type BlockSize = U16;
 }
+
+impl Debug for Aes {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        f.debug_struct("Aes").finish_non_exhaustive()
+    }
+}