search

RustCrypto / stream-ciphers

Collection of stream cipher algorithms
255 stars 49 forks source link

Implement Clone for core structs. #331

Closed kurnevsky closed 12 months ago

kurnevsky commented 12 months ago

I'm trying to update crypto_box in our tox crate. In the 0.8 version SalsaBox used to have Clone implementation but not anymore in 0.9. I guess it was just forgotten so I add Clone here for all new core structs.

tarcieri commented 12 months ago

I think Clone was removed deliberately to prevent duplicating the keystream, which can cause catastrophic failures (full plaintext recovery).

Is there a particular reason you’re cloning a stream cipher?

kurnevsky commented 12 months ago

Is there a particular reason you’re cloning a stream cipher?

Yes, calculating a shared secret is an expensive operation, so we cache these secrets as much as possible.

newpavlov commented 12 months ago

Cloning is an explicit operation, so I think it's fine to implement it for stream ciphers, especially considering that we derive it for block modes and the wrapper structs. Stream cipher crates fall into the hazmat category and, in my opinion, cloning is not too different from seeking, which when used incorrectly may cause similar catastrophic failure.

tarcieri commented 12 months ago

Yes, calculating a shared secret is an expensive operation, so we cache these secrets as much as possible.

But a stream cipher is also initialized with an IV which must never be reused, and cloning it will reuse it.

Why don’t you cache the key instead of the stream cipher instance?

tarcieri commented 12 months ago

Oh, I see this is actually about SalsaBox which is actually closer to an AEAD than a stream cipher.

This is the wrong repo for that. The correct repo is https://github.com/rustcrypto/nacl-compat

kurnevsky commented 12 months ago

Doesn't it use Salsa20 from this repo: https://docs.rs/crypto_box/latest/crypto_box/type.SalsaBox.html ?

tarcieri commented 12 months ago

Yes, but that’s an implementation detail. If you want a Clone impl on SalsaBox it needs to be added/fixed on that type itself

kurnevsky commented 12 months ago

SalsaBox is an alias for CryptoBox which already is marked as Clone. It's not clonable only because it stores Salsa20 internally. Or do you suggest to implement this clone manually without deriving? But anyway this will require some public clone method for Salsa20.

tarcieri commented 12 months ago

But anyway this will require some public clone method for Salsa20.

No it won't. The cipher that SalsaBox/CryptoBox/SecretBox is generic around is just a PhantomData parameter: https://github.com/RustCrypto/nacl-compat/blob/9926d28/crypto_secretbox/src/lib.rs#L148

A handwritten Clone impl has no need for a Clone impl on the underlying stream cipher, and indeed if it did that would be rather bad, because as I already mentioned it would clone the keystream.

kurnevsky commented 12 months ago

Ah, ok, thanks. I read the error wrongly. It seems I need to change the Clone implementation for CryptoBox.