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

[mjc]

Summary

• replace the unsafe bitwise copy in block-cipher packet-length probing with an internal PacketLengthProbe trait
• keep the generic probe path for stream/CTR ciphers
• add a specialized CBC probe that clones only the decryptor state needed for the first block
• add regression tests for the end-to-end read path and for probe state isolation in both CTR and CBC

Minimum fix

The commit 7895b84 fix block cipher packet length state cloning is the smallest self-contained fix.

This PR keeps the same behavior but refactors the probing path so CBC does not need to clone the full wrapper on every packet-length probe.

Why this refactor

OpeningKey::decrypt_packet_length() needs to decrypt the first block without advancing the real cipher state.

Before the fix, it did that with an unsafe bitwise copy of self.cipher, which is unsound for ownership-bearing cipher state.

The direct fix was to clone the cipher state before probing. That is correct, but it adds avoidable overhead on the packet read path.

The refactor makes the probe behavior explicit:

• stream/CTR ciphers probe with an independent cloned state
• CBC probes with an independent cloned decryptor state

That keeps the fix sound while preserving the packet-read performance characteristics much more closely.

Performance

I compared the end-to-end packet-read path three ways with the same Criterion bench and --sample-size 10:

• main
• the clone-based fix
• this internal-trait refactor

The clone-based fix is substantially slower than main, roughly 4x to 5x on these packet-read cases.

This refactor brings performance back in line with main:

• aes256-ctr, payload 11: main 378-385ns, clone fix 1.698-1.719us, this PR 366-377ns
• aes256-ctr, payload 100: main 411-431ns, clone fix 1.730-1.776us, this PR 372-386ns
• aes256-ctr, payload 1000: main 558-576ns, clone fix 1.910-1.966us, this PR 551-579ns
• aes256-cbc, payload 11: main 413-425ns, clone fix 1.724-1.765us, this PR 400-413ns
• aes256-cbc, payload 100: main 429-440ns, clone fix 1.956-2.012us, this PR 402-430ns
• aes256-cbc, payload 1000: main 749-787ns, clone fix 2.293-2.446us, this PR 758-771ns

OpenSSH reference point

This matches the same basic invariant OpenSSH relies on in its packet read path: decrypt enough state to determine the packet length without corrupting the real transport cipher state used for the full packet.

Testing

decrypt_packet_length_uses_independent_cipher_state crashes on main, which is the regression this change fixes.

On this branch:

• cargo test -p russh stream_cipher_probe_does_not_advance_cipher_state -- --nocapture
• cargo test -p russh decrypt_packet_length_uses_independent_cipher_state -- --nocapture
• cargo test -p russh packet_length_probe_does_not_advance_cbc_decryptor_state -- --nocapture
• cargo check -p russh --lib

[Copilot]

Pull request overview

This PR refactors block-cipher packet-length probing to replace unsafe cipher state duplication with an explicit internal probe trait, preserving cipher state isolation during SSH packet reads.

Changes:

• Adds PacketLengthProbe and wires it into block cipher opening keys.
• Implements cloned-state probing for stream/CTR ciphers and CBC decryptors.
• Adds regression tests for probe state isolation and end-to-end read behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
russh/src/cipher/block.rs	Adds the probe trait, updates opening key bounds, replaces unsafe probing, and adds stream/read-path tests.
russh/src/cipher/cbc.rs	Adds CBC-specific packet-length probing and CBC state-isolation test coverage.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[mjc]

as usual this turned out to be more complicated;

either bumping versions is needed or a bunch more stuff to get perf parity.

[mjc]

I pushed e26ab2f, which fixes the minimal-versions failure by raising the direct ctr floor from 0.9 to 0.9.2. I verified this exact one-line change on top of the previous PR head with:

cargo minimal-versions check --all-features --no-dev-deps

I also tested the alternative that avoids changing the dependency floor: wrap CTR stream ciphers so packet-length probing can recreate a fresh cipher from the original key/IV instead of requiring Ctr128BE: Clone. That keeps performance in the same class, but it is much more code than the version bump.

Packet-read Criterion comparison, --sample-size 10:

minimum ctr bump:
  aes256-ctr 11:   [1.6333 us 1.8165 us 2.0178 us]
  aes256-ctr 100:  [1.6697 us 1.6921 us 1.7208 us]
  aes256-ctr 1000: [1.8579 us 1.8767 us 1.8866 us]
  aes256-cbc 11:   [1.6876 us 1.7021 us 1.7174 us]
  aes256-cbc 100:  [1.7645 us 1.7916 us 1.8320 us]
  aes256-cbc 1000: [2.0854 us 2.1094 us 2.1795 us]

wrapper alternative:
  aes256-ctr 11:   [1.6914 us 1.7303 us 1.8321 us]
  aes256-ctr 100:  [1.7693 us 1.8238 us 1.9548 us]
  aes256-ctr 1000: [1.9321 us 1.9950 us 2.1266 us]
  aes256-cbc 11:   [1.6909 us 1.7478 us 1.8404 us]
  aes256-cbc 100:  [1.6716 us 1.6966 us 1.7277 us]
  aes256-cbc 1000: [2.0263 us 2.0781 us 2.1347 us]

If the ctr = "0.9.2" floor bump is not desired, this is the alternative implementation required to keep packet-read performance close without relying on Ctr128BE: Clone under minimal versions:

diff --git a/russh/src/cipher/block.rs b/russh/src/cipher/block.rs
index dc9c02e542649ae30bd9b4df51a792a7ce32cda1..fe2debeb6258f83f9a916489638894731bf01a90 100644
--- a/russh/src/cipher/block.rs
+++ b/russh/src/cipher/block.rs
@@ -228,9 +228,50 @@ impl<T: StreamCipher> BlockStreamCipher for T {
     }
 }
 
-impl<T: StreamCipher + Clone> PacketLengthProbe for T {
+pub(crate) struct ProbeableStreamCipher<C: StreamCipher + KeyIvInit + KeySizeUser + IvSizeUser> {
+    cipher: C,
+    key: GenericArray_0_14<u8, C::KeySize>,
+    iv: GenericArray_0_14<u8, C::IvSize>,
+}
+
+impl<C: StreamCipher + KeyIvInit + KeySizeUser + IvSizeUser> KeySizeUser for ProbeableStreamCipher<C> {
+    type KeySize = C::KeySize;
+}
+
+impl<C: StreamCipher + KeyIvInit + KeySizeUser + IvSizeUser> IvSizeUser for ProbeableStreamCipher<C> {
+    type IvSize = C::IvSize;
+}
+
+impl<C: StreamCipher + KeyIvInit + KeySizeUser + IvSizeUser> KeyIvInit for ProbeableStreamCipher<C> {
+    fn new(
+        key: &GenericArray_0_14<u8, Self::KeySize>,
+        iv: &GenericArray_0_14<u8, Self::IvSize>,
+    ) -> Self {
+        Self {
+            cipher: C::new(key, iv),
+            key: key.clone(),
+            iv: iv.clone(),
+        }
+    }
+}
+
+impl<C: StreamCipher + KeyIvInit + KeySizeUser + IvSizeUser> BlockStreamCipher
+    for ProbeableStreamCipher<C>
+{
+    fn encrypt_data(&mut self, data: &mut [u8]) {
+        self.cipher.apply_keystream(data);
+    }
+
+    fn decrypt_data(&mut self, data: &mut [u8]) {
+        self.cipher.apply_keystream(data);
+    }
+}
+
+impl<C: StreamCipher + KeyIvInit + KeySizeUser + IvSizeUser> PacketLengthProbe
+    for ProbeableStreamCipher<C>
+{
     fn decrypt_packet_length_block(&self, first_block: &mut [u8; 16]) {
-        let mut cipher = self.clone();
+        let mut cipher = C::new(&self.key, &self.iv);
         cipher.apply_keystream(first_block);
     }
 }
@@ -245,7 +286,7 @@ mod tests {
     use digest::typenum::U16;
     use tokio::io::AsyncWriteExt;
 
-    use super::{BlockStreamCipher, OpeningKey, PacketLengthProbe};
+    use super::{BlockStreamCipher, OpeningKey, PacketLengthProbe, ProbeableStreamCipher};
     use crate::mac::MacAlgorithm;
     use crate::sshbuffer::SSHBuffer;
 
@@ -259,7 +300,7 @@ mod tests {
         let mut ciphertext = plaintext;
         encryptor.apply_keystream(&mut ciphertext);
 
-        let cipher = Ctr128BE::<Aes128>::new(&key.into(), &iv.into());
+        let cipher = ProbeableStreamCipher::<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);
diff --git a/russh/src/cipher/mod.rs b/russh/src/cipher/mod.rs
index 1b10055b8b165e70e6f94bab201885266150502b..1fd4e7b7e11ce394ccf9fd0e4853874e830b0e3a 100644
--- a/russh/src/cipher/mod.rs
+++ b/russh/src/cipher/mod.rs
@@ -45,7 +45,7 @@ pub(crate) mod chacha20poly1305;
 pub(crate) mod clear;
 pub(crate) mod gcm;
 
-use block::SshBlockCipher;
+use block::{ProbeableStreamCipher, SshBlockCipher};
 use chacha20poly1305::SshChacha20Poly1305Cipher;
 use clear::Clear;
 use gcm::GcmCipher;
@@ -103,9 +103,12 @@ pub const NONE: Name = Name("none");
 pub(crate) static _CLEAR: Clear = Clear {};
 #[cfg(feature = "des")]
 static _3DES_CBC: SshBlockCipher<CbcWrapper<des::TdesEde3>> = SshBlockCipher(PhantomData);
-static _AES_128_CTR: SshBlockCipher<Ctr128BE<Aes128>> = SshBlockCipher(PhantomData);
-static _AES_192_CTR: SshBlockCipher<Ctr128BE<Aes192>> = SshBlockCipher(PhantomData);
-static _AES_256_CTR: SshBlockCipher<Ctr128BE<Aes256>> = SshBlockCipher(PhantomData);
+static _AES_128_CTR: SshBlockCipher<ProbeableStreamCipher<Ctr128BE<Aes128>>> =
+    SshBlockCipher(PhantomData);
+static _AES_192_CTR: SshBlockCipher<ProbeableStreamCipher<Ctr128BE<Aes192>>> =
+    SshBlockCipher(PhantomData);
+static _AES_256_CTR: SshBlockCipher<ProbeableStreamCipher<Ctr128BE<Aes256>>> =
+    SshBlockCipher(PhantomData);
 static _AES_128_GCM: GcmCipher = GcmCipher(&ALGORITHM_AES_128_GCM);
 static _AES_256_GCM: GcmCipher = GcmCipher(&ALGORITHM_AES_256_GCM);
 static _AES_128_CBC: SshBlockCipher<CbcWrapper<Aes128>> = SshBlockCipher(PhantomData);

[Eugeny]

Thank you! As a baseline, minimum dependency bumps are always acceptable by default as long as it's for a stable version.