Remove Clone impls

[newpavlov]

Cloning of a block mode state can be considered a footgun. If state has to be cloned for some reason, users should prefer passing key/IV instead (see the IvState trait).

Previous discussion: rust-random/rand#1101

Note that we previously had people complaining about Clone: #23

[Eugeny]

Could we at least get an into_inner() to allow reconstructing a copy of the Decryptor manually? It's needed for "peeking" decryption in the SSH protocol

[Eugeny]

Having looked at it more, instead of being a footgun the inability to "branch" off the cipher state is going to basically shoot any SSH stream implementation using RustCrypto right in the head 😓

ssh-cipher can work around it by reinitializing the cipher every time, but that's only because it does not implement streaming encryption. Anybody else would have to essentially reimplement the corresponding Encryptor/Decryptor structs to make them either cloneable or at least allow extracting state.

[tarcieri]

Perhaps an explicit API to get and resume states/positions?

[Eugeny]

I'm open to anything really - an opaque "checkpoint" type is useful if you want to obstruct the user from manipulating the state or for it to work with ciphers that do not have an "any value is valid" state like AES-Cxx, but in the end it does not matter much and could just be hidden behind a hazmat feature

[tarcieri]

Perhaps the SerializableStatehazmat API could work here?

[newpavlov]

Note that you can retrieve current IV using IvState and thus reconstruct the original mode state assuming you have the cipher state (e.g. by using a reference to it). Meanwhile with CTR you can simply seek.

Perhaps the SerializableState hazmat API could work here?

It would be a bad fit since we generally do not implement it for block cipher states.

[Eugeny]

@newpavlov that's what I ended up doing, but that leaks the implementation detail of IV being the "state" into the userspace. And ofc having to reconstruct the cipher on every turn, including cloning the IV

[tarcieri]

We could at least make an abstraction for it in ssh-cipher that works the same across ciphers

[newpavlov]

Could you link the relevant code?

I guess we could add something like this:

/// Call the closure with current mode state and reset the mode/// to the original state before returning from the method.fnpeek<T>(&mutself,f:implFnOnce(&mutSelf) -> T) -> T;

[Eugeny]

Here you go https://github.com/Eugeny/russh/blob/85ad0dd98e97398766d7e14da6a10138ba464622/russh/src/cipher/cbc.rs#L10-L81

[newpavlov]

Your code does not look too terrible to me, the IV stuff is isolated to the linked module. I think it's fine to have some amount of inconvenience while dealing with a potentially dangerous stuff.

Also note that you do not need to clone the cipher every time. You can create cbc::Decryptor<&C> with cbc::Decryptor::inner_iv_init(&self.dec_cipher, &self.dec_iv).

[newpavlov]

BTW if you only need to peek the first block, then you could just decrypt it and XOR the result with IV (retrieved from cbc::Decryptor), i.e. just implement a specialized one block version of CBC.

It also looks like you unnecessarily duplicate the cipher. IMO it would be better to store cipher, dec_iv, enc_iv and reconstruct Decryptor/Encryptor when necessary.

[Eugeny]

Neat, I didn't know Decryptor<&T> was a possibility.

BTW if you only need to peek the first block, then you could just decrypt it and XOR the result with IV (retrieved from cbc::Decryptor), i.e. just implement a specialized one block version of CBC.

I also need to support 3DES-CBC, so it's multiple blocks anyway - besides I precisely wanted to avoid reimplementing crypto primitives

[tarcieri]

I tried to add a ssh_cipher::Decryptor::peek method similar to what @newpavlov had suggested earlier in this thread for the low-level block modes, but while I was able to retrieve CBC IVs with IvState, there's no corresponding way to set them like there is with StreamCipherSeek::(try_)seek:

RustCrypto/SSH@3283b7e#diff-95d2927818f6bc686a4400d3646948a0003eb90dcc87fa54abe148ed6b4db5edR179

Likewise there's not a way to access the inner cipher of cbc::Decryptor, so I can't replace the current cipher instance with one constructed using cbc::Decryptor::inner_iv_init.

I think what would probably be most helpful here is a way to set the IV as functionality analogous to "seeking" in a stream cipher.

[tarcieri]

@Eugeny separately I'd be curious if that kind of API would work for your use cases

[Eugeny]

Yup, I like it

[newpavlov]

there's no corresponding way to set them like there is with StreamCipherSeek::(try_)seek

I meant to implement it as part of the cbc crate.

I think what would probably be most helpful here is a way to set the IV as functionality analogous to "seeking" in a stream cipher.

Yeah, some sort of set_iv method could be a good addition and we could provide a blanket impl for peek on top of it. Ideally, it would be part of the IvState trait, but adding it now would be a breaking change.

But in the case of the provided example, I think that it's better to have cipher, dec_iv, and inc_iv fields as suggested above. Not only it allows to implement peeking, but also eliminates the currently present block cipher duplication.