`ssh-cipher`: Cipher::{encrypt, decrypt} does not persist cipher state.

[lowlevl]

Hi there, first of all thanks for the work on those 3 crates, they're really coming handy when implementing SSH primitives !

About the interfaces for ssh-cipher, it seems like calling Cipher::encrypt or Cipher::decrypt at different times to encrypt/decrypt SSH packet doesn't preserve the inner state of the Cipher and produce invalid data. I may be doing something wrong however, but so far I am only able to decrypt and encrypt the first packet after the key-exchange, the successive packets gets mangled. I test my code against OpenSSH, so I'd guess the client is behaving correctly in this case.

[tarcieri]

The ssh-cipher API is an extraction from ssh-key and largely designed around the concerns of encrypting SSH private key files, so it's possible that its API is somewhat unhelpful for an SSH protocol implementation.

However, it's designed to support AEAD modes which by definition have a one-shot API and can't support incremental decryption at the very least.

I would be curious to know more about the applications for stateful/incremental decryption/encryption and what happens in those cases when an AEAD mode is used instead.

[lowlevl]

That makes sense, I wasn't aware of this historical status, is it something that is planned to be supported at some point ?

However, it's designed to support AEAD modes which by definition have a one-shot API and can't support incremental decryption at the very least.

Those algorithms are the last that I planned to tackle because they require computation of the Tag on the fly, so I haven't tested yet, but I ended up making an enum and directly useing ctr,cbc,des,aes, etc. and it seems to work.

[tarcieri]

We can support it, yes, though I would suggest implementing the AEAD modes first before deciding on an abstraction.

(Really the AEAD modes are the only thing a modern implementation should prefer, and the others are legacy)

[lowlevl]

Thanks for the heads up, I want to do something exhaustive and disable insecure ciphers, MACs, and kexs behind an insecure feature flag, the idea is to be also compatible with older implementations of the standard.

I ended up storing the states in an enum for now for stateful ciphers, I will now try to implement AEAD ciphers.

The implementation is there if you're curious, it's not yet documented, finished, or properly tested and is only server-side for now though. I also made ssh-packet to tackle binary (de)serialization.

[tarcieri]

Nice, that looks quite promising!

So, would it be helpful to have a stateful Cipher object which could be reused for the legacy modes, but is only one-shot for the AEAD modes?

That would be a breaking change but something we could potentially accommodate in the next release.

[lowlevl]

I think so yes, I haven't finished the implementation for the AEAD ciphers because there are some caveats and specifics around those ciphers (since they use implicit MACs) that break my trait design >:(

The main issue with having a single stateful Cipher struct is the need for separate cbc::Decryptor and cbc::Encryptor states for block ciphers, for now I just deduplicated my enum, but I'm not happy with this design, one solution would be using dynamic dispatch and downcasting to the correct state in my case, but that's not something that would be really scalable for ssh-cipher IMHO. Edit: I did this cursed thing for now. And yes the AEAD ciphers should stay stateless because the iv is generated on-the-fly from the sequence number of the packet from what I read.

Also it would be great if we could choose between key0 and key1 for ChaCha20Poly1305, since the SSH packet len is encrypted with key0.

[tarcieri]

Yeah, it would probably involve Cipher creating stateful Encryptor/Decryptor objects which are themselves enums over the various possible modes

[lowlevl]

Maybe a way to have non-breaking changes for Cipher is to add to_decryptor/to_encryptor as well as keeping the stateless methods.