Put insecure ciphers and digest dependencies behind a non-default feature

[eric-seppanen]

I find it surprising that russh has dependencies on des and blowfish crates by default. These ciphers have not been trusted for a long time, and haven't been enabled by default in ssh servers for many years (link, link).

If known bad ciphers need to be supported, I think they should be gated behind a feature flag that is off by default, accompanied by a warning that they are insecure.

There are also some insecure digest crates used by russh-keys: md5 was broken and disabled by ssh servers a long time ago (link), and should not be enabled by default. sha1 was broken a bit more recently (link), but also probably long enough ago to remove it from the default features.

[eric-seppanen]

I failed to notice that md5 is only used in pkcs5 decryption. Also, blowfish is a transitive dependency of bcrypt-pbkdf. I'm not sure what the security implications of these uses are (or if they're in common use any more), but their use of long-broken ciphers/digests doesn't inspire confidence.

Also, the bcrypt-pbkdf dependency appears to be unused.

[Eugeny]

Unsafe algorithms are disabled by default, but you are right that we could additionally feature-gate those dependencies

[eric-seppanen]

Are the set of default algorithms documented anywhere?

Is there e.g. a unit test that verifies that unsafe algorithms (e.g. the "none" cipher) are disabled by default?

[Eugeny]

They are listed in Preferred::default() https://github.com/Eugeny/russh/blob/main/russh/src/negotiation.rs#L126
There is no test for it - though feature-gating them would essentially prevent the crate from compiling if any insecure algorithms end up in the list for any reason

[eric-seppanen]

It would be nice to get this into the documentation somehow. Even if I know to look at Preferred::DEFAULT there's no description attached.