Refactor block-cipher packet-length probing to avoid unsafe state duplication

russh/Cargo.toml

@@ -44,7 +44,7 @@ bytes.workspace = true
 cbc = { version = "0.1" }
 cbc_0_2 = { package = "cbc", version = "0.2.0" }
 cipher = "0.5.1" # only pinned due to a cargo-minimal-versions failure in 0.5.0
-ctr = "0.9"
+ctr = "0.9.2"
 ctr_0_10 = { package = "ctr", version = "0.10.0" }
 curve25519-dalek = "=5.0.0-pre.6"
 crypto-bigint = { version = "=0.7.0-rc.28", features = ["alloc"] }

russh/src/cipher/block.rs

@@ -34,10 +34,12 @@
     )
 }
 
-pub struct SshBlockCipher<C: BlockStreamCipher + KeySizeUser + IvSizeUser>(pub PhantomData<C>);
+pub struct SshBlockCipher<C: BlockStreamCipher + PacketLengthProbe + KeySizeUser + IvSizeUser>(
+    pub PhantomData<C>,
+);
 
-impl<C: BlockStreamCipher + KeySizeUser + IvSizeUser + KeyIvInit + Send + 'static> super::Cipher
-    for SshBlockCipher<C>
+impl<C: BlockStreamCipher + PacketLengthProbe + KeySizeUser + IvSizeUser + KeyIvInit + Send + 'static>
+    super::Cipher for SshBlockCipher<C>
 {
     fn key_len(&self) -> usize {
         C::key_size()
@@ -78,7 +80,7 @@
     }
 }
 
-pub struct OpeningKey<C: BlockStreamCipher> {
+pub struct OpeningKey<C: BlockStreamCipher + PacketLengthProbe> {
     pub(crate) cipher: C,
     pub(crate) mac: Box<dyn Mac + Send>,
 }
@@ -88,7 +90,9 @@
     pub(crate) mac: Box<dyn Mac + Send>,
 }
 
-impl<C: BlockStreamCipher + KeySizeUser + IvSizeUser> super::OpeningKey for OpeningKey<C> {
+impl<C: BlockStreamCipher + PacketLengthProbe + KeySizeUser + IvSizeUser> super::OpeningKey
+    for OpeningKey<C>
+{
     fn packet_length_to_read_for_block_length(&self) -> usize {
         16
     }
@@ -108,10 +112,7 @@
             #[allow(clippy::unwrap_used, clippy::indexing_slicing)]
             encrypted_packet_length[..4].try_into().unwrap()
         } else {
-            // Work around uncloneable Aes<>
-            let mut cipher: C = unsafe { std::ptr::read(&self.cipher as *const C) };
-
-            cipher.decrypt_data(&mut first_block);
+            self.cipher.decrypt_packet_length_block(&mut first_block);
 
             // Fine because of self.packet_length_to_read_for_block_length()
             #[allow(clippy::unwrap_used, clippy::indexing_slicing)]
@@ -213,6 +214,10 @@
     fn decrypt_data(&mut self, data: &mut [u8]);
 }
 
+pub(crate) trait PacketLengthProbe {
+    fn decrypt_packet_length_block(&self, first_block: &mut [u8; 16]);
+}
+
 impl<T: StreamCipher> BlockStreamCipher for T {
     fn encrypt_data(&mut self, data: &mut [u8]) {
         self.apply_keystream(data);
@@ -222,3 +227,123 @@
         self.apply_keystream(data);
     }
 }
+
+impl<T: StreamCipher + Clone> PacketLengthProbe for T {
+    fn decrypt_packet_length_block(&self, first_block: &mut [u8; 16]) {
+        let mut cipher = self.clone();
+        cipher.apply_keystream(first_block);
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use aes::cipher::KeyIvInit;
+    use aes::cipher::StreamCipher;
+    use aes::Aes128;
+    use aes::cipher::{IvSizeUser, KeySizeUser};
+    use ctr::Ctr128BE;
+    use digest::typenum::U16;
+    use tokio::io::AsyncWriteExt;
+
+    use super::{BlockStreamCipher, OpeningKey, PacketLengthProbe};
+    use crate::mac::MacAlgorithm;
+    use crate::sshbuffer::SSHBuffer;
+
+    #[test]
+    fn stream_cipher_probe_does_not_advance_cipher_state() {
+        let plaintext = *b"0123456789ABCDEF";
+        let key = fixture_bytes::<16>(7);
+        let iv = fixture_bytes::<16>(3);
+
+        let mut encryptor = Ctr128BE::<Aes128>::new(&key.into(), &iv.into());
+        let mut ciphertext = plaintext;
+        encryptor.apply_keystream(&mut ciphertext);
+
+        let cipher = Ctr128BE::<Aes128>::new(&key.into(), &iv.into());
+        let mut probed_block = ciphertext;
+        cipher.decrypt_packet_length_block(&mut probed_block);
+        assert_eq!(probed_block, plaintext);
+
+        let mut decrypted = ciphertext;
+        let mut cipher_after_probe = cipher;
+        cipher_after_probe.decrypt_data(&mut decrypted);
+        assert_eq!(decrypted, plaintext);
+    }
+
+    #[test]
+    fn decrypt_packet_length_uses_independent_cipher_state() -> std::io::Result<()> {
+        let runtime = tokio::runtime::Builder::new_current_thread()
+            .enable_all()
+            .build()?;
+        let opening = OpeningKey {
+            cipher: OwnedStateCipher::new(),
+            mac: crate::mac::_NONE.make_mac(&[]),
+        };
+        let mut opening = opening;
+        let mut buffer = SSHBuffer::new();
+        let bytes_read = runtime.block_on(async {
+            let (mut writer, mut reader) = tokio::io::duplex(64);
+            writer.write_all(&[0; 17]).await?;
+            drop(writer);
+            crate::cipher::read(&mut reader, &mut buffer, &mut opening).await
+        })
+        .map_err(std::io::Error::other)?;
+
+        assert_eq!(bytes_read, 16);
+        Ok(())
+    }
+
+    struct OwnedStateCipher {
+        packet_length: Box<[u8; 4]>,
+    }
+
+    impl OwnedStateCipher {
+        fn new() -> Self {
+            Self {
+                packet_length: Box::new([0, 0, 0, 13]),
+            }
+        }
+    }
+
+    impl Clone for OwnedStateCipher {
+        fn clone(&self) -> Self {
+            Self {
+                packet_length: Box::new([0, 0, 0, 12]),
+            }
+        }
+    }
+
+    impl KeySizeUser for OwnedStateCipher {
+        type KeySize = U16;
+    }
+
+    impl IvSizeUser for OwnedStateCipher {
+        type IvSize = U16;
+    }
+
+    impl BlockStreamCipher for OwnedStateCipher {
+        fn encrypt_data(&mut self, _data: &mut [u8]) {}
+
+        fn decrypt_data(&mut self, data: &mut [u8]) {
+            if let Some(prefix) = data.get_mut(..4) {
+                prefix.copy_from_slice(&self.packet_length[..]);
+            }
+        }
+    }
+
+    impl PacketLengthProbe for OwnedStateCipher {
+        fn decrypt_packet_length_block(&self, first_block: &mut [u8; 16]) {
+            if let Some(prefix) = first_block.get_mut(..4) {
+                prefix.copy_from_slice(&[0, 0, 0, 12]);
+            }
+        }
+    }
+
+    fn fixture_bytes<const N: usize>(seed: u8) -> [u8; N] {
+        let mut bytes = [0; N];
+        for (i, byte) in bytes.iter_mut().enumerate() {
+            *byte = seed.wrapping_add(i as u8);
+        }
+        bytes
+    }
+}

russh/src/cipher/cbc.rs

@@ -7,7 +7,7 @@
 #[allow(deprecated)]
 use digest::generic_array::GenericArray;
 
-use super::block::BlockStreamCipher;
+use super::block::{BlockStreamCipher, PacketLengthProbe};
 
 // Allow deprecated generic-array 0.14 usage until RustCrypto crates (cipher, cbc, etc.)
 // upgrade to generic-array 1.x. Remove this when dependencies no longer use 0.14.
@@ -50,6 +50,20 @@
     }
 }
 
+impl<C: BlockEncrypt + BlockCipher + BlockDecrypt + Clone> PacketLengthProbe for CbcWrapper<C>
+where
+    C: BlockDecryptMut,
+{
+    fn decrypt_packet_length_block(&self, first_block: &mut [u8; 16]) {
+        let mut decryptor = self.decryptor.clone();
+        for chunk in first_block.chunks_exact_mut(C::block_size()) {
+            let mut block = generic_array_from_slice(chunk);
+            decryptor.decrypt_block_mut(&mut block);
+            chunk.copy_from_slice(&block);
+        }
+    }
+}
+
 impl<C: BlockEncrypt + BlockCipher + BlockDecrypt + Clone> InnerIvInit for CbcWrapper<C>
 where
     C: BlockEncryptMut + BlockCipher,
@@ -62,3 +76,59 @@
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use aes::cipher::KeyIvInit;
+    use aes::Aes128;
+    #[cfg(feature = "des")]
+    use des::TdesEde3;
+
+    use super::{BlockStreamCipher, CbcWrapper, PacketLengthProbe};
+
+    #[test]
+    fn packet_length_probe_does_not_advance_cbc_decryptor_state() {
+        let plaintext = *b"0123456789ABCDEF";
+        let key = fixture_bytes::<16>(11);
+        let iv = fixture_bytes::<16>(5);
+
+        let mut encryptor = CbcWrapper::<Aes128>::new(&key.into(), &iv.into());
+        let mut ciphertext = plaintext;
+        encryptor.encrypt_data(&mut ciphertext);
+
+        let cipher = CbcWrapper::<Aes128>::new(&key.into(), &iv.into());
+        let mut probed_block = ciphertext;
+        cipher.decrypt_packet_length_block(&mut probed_block);
+        assert_eq!(probed_block, plaintext);
+
+        let mut decrypted = ciphertext;
+        let mut cipher_after_probe = cipher;
+        cipher_after_probe.decrypt_data(&mut decrypted);
+        assert_eq!(decrypted, plaintext);
+    }
+
+    #[cfg(feature = "des")]
+    #[test]
+    fn packet_length_probe_respects_3des_block_size() {
+        let plaintext = *b"0123456789ABCDEF";
+        let key = fixture_bytes::<24>(11);
+        let iv = fixture_bytes::<8>(5);
+
+        let mut encryptor = CbcWrapper::<TdesEde3>::new(&key.into(), &iv.into());
+        let mut ciphertext = plaintext;
+        encryptor.encrypt_data(&mut ciphertext);
+
+        let cipher = CbcWrapper::<TdesEde3>::new(&key.into(), &iv.into());
+        let mut probed_block = ciphertext;
+        cipher.decrypt_packet_length_block(&mut probed_block);
+        assert_eq!(probed_block, plaintext);
+    }
+
+    fn fixture_bytes<const N: usize>(seed: u8) -> [u8; N] {
+        let mut bytes = [0; N];
+        for (i, byte) in bytes.iter_mut().enumerate() {
+            *byte = seed.wrapping_add(i as u8);
+        }
+        bytes
+    }
+}