Remove obsolete/insecure algorithms

[DemiMarie]

Algorithms like MD5, SHA1, and RC4 have no business in a modern crypto library.

[arcrose]

I'm going to have to respectively disagree. I would agree that no one should be designing new protocols using these primitives. However, I feel that most users probably shouldn't even use something as low level as rust-crypto in the first place, and instead opt for something like a libsodium wrapper. For individuals educated enough about crypto to decide which primitives to use, avoiding the ones you mentioned is a given. There is a use case, however, for those interested in implementing legacy (or just poorly designed) systems in Rust. A great deal of (arguably insecure) software uses these primitives and it's at least worthwhile to have them around for those diminishing cases. This is a library, and not a browser or other similar piece of software, after all.

[mirandadam]

Hello @drbo and @zsck, it did cross my mind to flag these weak algorithms somehow. I have no idea, however, on how to establish a threshold for that kind of flagging (or as @drbo suggested, removing the algorithms altogether). Should AES256 also be flagged/removed because the biclique attack reduced its effective strength from 256 to 254.4 bits? Obviously not, but what about 200 bits? and 180? Where do you draw a line?

Maybe (as @zsck said) developers who do not understand the limitations really should not be designing protocols or systems using these primitives at such low level.

I, however, also believe that crypto in general should have very large security margins and the implementations should strive to be resistant to misuse or bad practices by design. In the case of rust-crypto, a warning to the developer might help in that regard. It might prevent bad design decisions or maybe give a hint to the clueless developer that the subject is full of caveats and he or she needs to study a bit before writing production code. Kudos to @drbo for bringing up the subject of unsafe primitives.

As @zsck said, rust-crypto should not remove the primitives. I can think of at least a few relevant use cases for implementing such algorithms:

• testing the integrity of files which had been hashed with md5 or sha1 in the past (real use case for me)
• Supporting old SHA1 certificates (IIRC) while they are stil valid. Not everyone has migrated already.
• More generally, allow compatibility with legacy systems and hardware. Some latest hardware still does not support sha2 (or sha1 for that matter). My specific usage involves hard drive cloning devices. Others may want to implement decrypting data in legacy formats.

I propose a small paragraph be inserted at the readme.md explaining the caveats and pointing the developer to some kind of updated security reference. What do you think?

[arcrose]

I could get behind the idea of adding some information to the documentation describing the risks/tradeoffs of algorithms. I would suggest that, in the opening paragraph for each primitive's documentation, an explanation or at least a link to some modern literature regarding that primitive's strengths, weaknesses, and recommended use cases be provided. For example, in the case of MD5, SHA1, and other outdated hashes, I would note:

1. That developers of modern systems with no ties to legacy systems use a more reliable alternative and include a recommendation.
2. That it is included for legacy purposes and should only be used to interface with legacy software.
3. A link to some literature describing the weaknesses of the primitive.

This could be extended to cover different modes of block ciphers, achieving a level of granularity suitable to that which one would require when designing their own systems.

[mirandadam]

Hello, @zsck.

I made a suggestion on issue #296 to add that information to a table alongside other implementation metadata. I believe that would be most useful not only to the clueless developer, but also to the seasoned cryptographer. There would be information not only about the algorithms' security status, but about the security of the implementations themselves.

[Spencer-H]

Perhaps it would be reasonable to move them behind an "insecure" feature gate? So that people would explicitly have to point out that they're aware they're insecure to use them?

[arcrose]

I like that idea, @Spencer-H. I might even go a bit further and store legacy and known-insecure algorithms in an insecure module, and encourage people to refer to that module explicitly in their code. That way people end up writing things like

use crypto::insecure;

insecure::md4(...)

The goal being to make it easy to remind users in the code that the algorithm is not safe. Obviously it could be circumvented by useing the algorithm one wants directly, but it could do some good.

[horrorho]

Sorry, rather late to the party. If you want a modern crypto library that's relevant to every day use then you need to support algorithms in common use. Remove them at your peril. In the real world insecure algorithms tend to linger and we still need to work with them.

I'd avoid going down the strongly opinionated route. Let the end-user decide on what is appropriate. They are responsible for educating themselves and for the design choices they make.

If you really do want to inform the user about potential pitfalls, do so in the documentation. However what constitutes an unsafe algorithm? SHA1 was considered unsafe back in 2005, would we have removed it from a crypto library or marked it as unsafe back then? Also isn't HMAC-SHA1 still considered safe? How do we implement this if we remove SHA1? What about NIST curves and potential back doors, are they unsafe too? It's a can of worms that I'm not sure you want to open.

[mirandadam]

Hi @horrorho, this is the suggestion I made here and on issue #296:

1. Document the implementation status of the various ciphers in a table in readme.md (actually @tarcieri's suggestion, see issue #296 on why is that important). It could contain data such as whether the implementation was audited, whether it is constant time, and an whether there are known better alternatives/known security issues, as @zsck suggested in may 25.
2. Generate/update that table automatically from the code documentation itself. Cryptographic primitives that do not have the necessary keywords/markup in the documentation should generate a warning upon trying to regenerate the table or running the tests. Under the DNRY principle, updating this information should be done manually at a single location. A good location would be probably be right next to the code it refers to.
3. Compiler could generate a warning if the application code used a primitive from rust-crypto documented as "unsafe", and the warning should be easy to suppress from within the application code.

[mirandadam]

Now to the core of this issue, maybe we could settle on three arguments: a) Whether the obsolete/flawed/unsafe algorithms should be removed. I am also strongly opinionated that the primitives should not be removed. b) Whether the compiler should issue a warning in case these are used. I have no idea on how to implement that. I used to think it would be a good idea, now not so sure. c) Whether the "unsafe" status should be documented. @tarcieri already volunteered to document the implementation status on issue #296, I think the documentation is a great idea and could also include the security status. No one has volunteered to write code that builds the table automatically from documentation, however.

[horrorho]

Hi, thank you for the feedback. It sounds like we agree more than we disagree.

Implementation details/ auditing should be documented but this could be considered separate from criticisms of the algorithm itself. A flawed implementation of a insecure cipher is not the same as a perfect implementation of an insecure cipher. Well I'm nitpicking/ pushing my own opinion now and documenting security concerns of the algorithm itself is not unreasonable. Either way, you've addressed my main concerns. Thank you and keep up the good work!

[arcrose]

This is practically necro-posting at this point, so I appologize if anyone feels that adding to this discussion is a waste, but I'd like to throw another idea into the ring.

It has occurred to me that, while simply documenting that algorithms are unsafe for modern applications, we have a much more effective means of helping developers to understand the risks they are taking built into Rust: the unsafe keyword. I feel that it may actually be appropriate to define implementations of known insecure algorithms using the unsafe keyword for a couple of reasons:

1. Having unsafe in the code itself means the documentation cannot fail to adequately alert a developer that what they are doing is potentially dangerous.
2. Using the unsafe keyword forces developers to observe the risk they are imposing upon themselves- their code will not compile if they do not acknowledge it.
3. Usage of the unsafe keyword would propagate to all uses of an insecure algorithm, meaning that the developer who uses it and all who follow them will be confronted with the fact that they are using unsafe code and may be compelled to read the documentation to understand why

Of course, there is the caveat that this is a slightly unconventional use of the keyword, but I feel the benefits I mentioned above outweigh the unusualness of this usage.

Here's an example of what I mean.

src/md4.rs:

/// Not really md4, but you get the point.
pub unsafe fn hash(bytes: &[u8]) -> u16 {
    let mut sum = 0u16;
    for x in bytes {
        sum += *x as u16;
    }
    sum
}

src/main.rs:

mod md4;

fn main() {
    let msg = [0, 1, 2, 3, 4, 5];
    let not_a_hash = md4::hash(&msg);
    println!("{}", not_a_hash);
}

Trying to compile this causes the error

src/main.rs:5:26: 5:41 error: call to unsafe function requires unsafe function or block [E0133]
src/main.rs:5         let not_a_hash = md4::hash(&msg);

which is remedied by wrapping lines 5 and 6 of src/main.rs in an unsafe{} block.

[horrorho]

@zsck Whilst interesting my own view is that this would be a violation of the unsafe keyword semantics. Unsafe is for code that doesn't meet certain compile time guarantees, not for expressing the suitability of code for any particular purpose. A rust developer seeing an unsafe error would assume that the code itself is unsafe, not the algorithm.

Over the years do we apply the unsafe keyword to additional algorithms as vulnerabilities are discovered? This would force developers to recode applications that would otherwise remain functional, or would they just continue to use older versions of the library which may contain bugs.

Documentation would be a more appropriate place to voice concerns over the viability of algorithms. Call me heartless, but if the designer of a new crypto protocol doesn't want to read documentation or educate themselves on modern cryptography standards, then they deserve any heartache that may come their way.

Also are the developers of new crypto protocols not in the minority? I suspect, possibly incorrectly, that most users will be implementing protocols based on clearly predefined use cases.

Again just my two cents. I'm no crypto guru so please interpret my opinions in that context and feel free to disregard any points I make.

[tarcieri]

Yeah, unsafe is for memory safety, and changes the semantics of the language (in scary ways that would only make things worse for a crypto context).

However, there is something I think could be useful: cargo features. You could potentially require people to opt into unsafe/legacy ciphers.

[mirandadam]

@zsck brings an interesting point in that we are discussing this issue for some time already. In the name of simplicity, I propose the following actions for solving this issue:

1. Keep the primives. Except for @DemiMarie, the original poster, everyone else seems to think the obsolete primitives should be kept, and the original poster has been silent in this discussion.
2. Add a paragraph in readme.md After: "Rust-Crypto has not been thoroughly audited for correctness, so any use where security is important is not recommended at this time." We could add something along the following lines: "Weaknesses have been steadily found in cryptographic algorithms over the years which belong to the algorithms themselves and are present even if the implementation is found to be flawless. Some of the algorithms implemented in Rust-Crypto have known vulnerabilities of that kind and are kept for compatibility. The developer should do his or her own research and choose appropriately."
3. Then we could either close this issue or, if there are volunteers (preferably with pull requests), implement the other suggestions (document each primitive security status, "feature gate" the unsafe ones, etc.).

[mirandadam]

By the way, I also think the unsafe keyword should not be used for guarding against using obsolete primitives. And, @zsck, posting to an active issue, even if it's old, should probably not be considered necroposting, you are helping with the solution and even displayed some code :-)

[burdges]

I'm with @tarcieri here. cargo features do the job.

Although simply leaving warnings in the docs sounds more important, perhaps with references if the attack is kinda one canonical, as opposed to a slowly increasing trend, folklore, etc.

[mirandadam]

Hi everyone, I submitted pull request #388 to include a warning about known vulnerabilities in readme.md. Suggestion for possible steps from here (volunteers needed):

• You may submit a pull request with a better message. Mine still seems a bit awkward.
• Submit a pull request, preferably in a different file, documenting the security status of each primitive.
• Submit a pull request with a proposal for implementing "feature gating" (using cargo features) the obsolete primitives.
• @DaGenix or whoever else has commit access to this repo can take a look into it.
• Hopefully close this issue.

[burdges]

I'll change my opinion here : There are a bunch of pull requests for ciphers that maybe this crate should not be responsible for maintaining. I'm wondering if those authors are sending pull requests because they used internal features here. If so, then maybe forking off a "rust-crypto-legacy" crate would be good way to make sure that ciphers, hash functions, modes, etc. do not need to be incorporated into this crate to be usable.

[tarcieri]

@burdges I think the simpler solution is for those who are enthusiastic about "legacy" crypto to make their own crates for algorithms they are willing to support, as opposed to some sort of omnibus "broken crypto" library.

In general, I feel a lot better about libraries whose maintainers are willing to draw some sort of line in the sand regarding what cryptography they're willing to support.

[the8472]

Note that the notion of "unsafe" or "insecure" depends on which security properties you need.

E.g. algorithms vulnerable to timing attacks are still useful for offline uses.

Even MD5 is essentially secure if you only need is preimage resistance. And unsuitability for signatures and password hashing stem from entirely different reasons - collisions and performance, respectively.

Under some threat models authenticated encryption schemes could be less secure than unauthenticated ones. E.g. when you need deniability.

So categorically labeling those primitives as unsafe and tossing them out/hiding them behind flags seems wrong, especially when their security guarantees are sufficient for the protocols in which they are used. A more nuanced approach would be more appropriate.

[tarcieri]

So categorically labeling those primitives as unsafe and tossing them out/hiding them behind flags seems wrong

I think if you were to take a straw poll among cryptographers as to whether MD5 is "broken" or "insecure", the answer would overwhelmingly be yes. Almost all useful purposes of hash functions depend on collision resistance. Collision resistance is a basic property of a secure hash function. Claiming it's not broken because it's still preimage resistant despite being easily collidable is grasping at straws.

[the8472]

I think if you were to take a straw poll among cryptographers as to whether MD5 is "broken" or "insecure", the answer would overwhelmingly be yes.

And what if they question instead asked "Is MD5 broken or insecure for all applications"? Would you still claim that this hypothetical poll is a forgone conclusion?

I'm saying that sweeping claims are not helpful when working with cryptography where nuances matter.

If you are allergic to md5, let's pick something more palatable: Argon2. Would you label Argon2i as categorically unsafe because it is susceptible to timing attacks, even though it has uses in areas where timing attacks are not a concern?

[tarcieri]

And what if they question instead asked "Is MD5 broken or insecure for all applications"?

The question is: is MD5 a secure hash function?

The answer is: no, because it's not collision resistant. A secure hash function, by definition, must be collision resistant.

Anything else is conflating the issue, and irrelevant.

[horrorho]

@tarcieri MD5 isn't that broken for all applications. HMAC-MD5 is still in use and doesn't suffer the same levels of weakness, Apple for one still use it. Yes, they would be unlikely to use it if designing a new protocol today, but what compelling evidence is there that they should immediately revoke it's current usage?

As @the8472 also mentioned how do you audit algorithms for selection?

I think you have a two broad camps here. Those pushing for a tight/ highly opinionated library and those pushing for a more relaxed approach. I guess you could list some advantages/ disadvantages for both.

Tight:

• pros: Less code to maintain. Safe algorithms. Reduced resource cost/ cheap.
• cons: Narrower use case coverage/ audience. Algorithm selection/ removal auditing requirements/ conflicts.

Relaxed:

• pros: Wider use case coverage/ audience. Minimal algorithm selection/ removal auditing requirements, users decide.
• cons: More code to maintain/ bloat. Unsafe algorithms. High resource cost/ expensive.

[tarcieri]

I'll just leave this here:

https://www.kb.cert.org/vuls/id/836068

Do not use the MD5 algorithm

Software developers, Certification Authorities, website owners, and users should avoid using the MD5 algorithm in any capacity. As previous research has demonstrated, it should be considered cryptographically broken and unsuitable for further use.

[horrorho]

@tarcieri That note needs to be reviewed in context. At no point do they mention, discuss or provide any solid evidence of an exploitable HMAC-MD5 vulnerability.

The safety of HMAC in regards to the collision weakness of it's underlying hash algorithm has been discussed in more specific terms.

Bruce Schneier on SHA1 although the point also applies to MD5: It pretty much puts a bullet into SHA-1 as a hash function for digital signatures (although it doesn't affect applications such as HMAC where collisions aren't important).

Also: https://www.ietf.org/rfc/rfc2104.txt http://cseweb.ucsd.edu/~mihir/papers/hmac-new.html

Either way as I'm sure you've figured, the point I'm trying to make is that there are differing opinions by experts/ corporations on what is and what isn't acceptable in regards to cryptography. The real question is how opinionated does this library want to be? It's an honest question, hence my thoughts on tight/ relaxed above.

[tarcieri]

@horrorho cryptographic best practice is clear: don't use MD5. If you disagree, please point to one cryptographer or organization specializing in cryptography who holds the same opinion. Bruce Schneier talking about a completely different algorithm which hasn't yet seen a hash collision doesn't count. There is no controversy about MD5 in the in the cryptographic community: it is broken and shouldn't be used.

While there aren't specific attacks against particular constructions using MD5 like HMAC, the evidence against MD5 is massive and damning: it is broken, and shouldn't be used. Just because every last usage of MD5 hasn't been demonstrated insecure is not an argument that it is secure.

The debate in this thread comes down to "well recognized cryptographic best practices" versus "excuses to persist in using insecure cryptography". There is only one excuse for the latter: legacy interop in situations where whatever circumstances prevent upgrading algorithms.

In that regard, as I said earlier I think placing legacy algorithms with known security problems behind a "caveat emptor"-style cargo feature flag is the best approach.

[arcrose]

I for one have to agree with @tarcieri. MD5, SHA1, etc. are considered broken by the cryptography community and an argument that they are still somehow suitable for some applications is irrelevant to this discussion. Surely we can all agree that any newly designed crypto system should not use algorithms like MD5, because stronger alternatives exist and have been standardized, and which can be used in fundamentally the same way.

@horrorho also makes an important point that what we're trying to discern here is basically the stance that this crate should take with regards to supporting broken crypto at all. However, I don't think the question is one of strictness. Instead, we should think about things from users' perspectives.

Continuing along that line of thought, I have a few questions that I think we'd do well to try to answer in order to make it clear what solution we should follow.

Suppose someone is coming to Rust from a language/ecosystem/background without a solid history in crypto fundamentals. If someone thinks "I want to make X in Rust and need a way to hash things like I did before. Where is the MD5 function?" Do we want them to end up going to some other crate and immediately using its implementation of MD5? Do we want rust-crypto to be something of a one-stop-shop for crypto primitives in Rust? We could leverage the fact that people would be likely to land upon rust-crypto to educate them that MD5 should be avoided and teach them about an alternative.

Having said that, I maintain my stance that rust-crypto should provide an implementation of MD5 for legacy purposes and find a way to make it difficult to use (hiding it behind a feature flag for instance) so that we have an opportunity to inform users of more appropriate alternatives.

[quadrupleslap]

=\ Surely everyone knows that MD5 is broken by now, and won't use it unless they have a good reason? I'm not sure if this is correct and idiomatic use of the attribute, but an idea to consider if you haven't already would be to mark "broken" methods as #[deprecated].

[arcrose]

@quadrupleslap Unfortunately not everyone does know this. Even if they did, I think that any library that would support legacy crypto should do its part in ensuring such functionality isn't misused. Try to put the onus on users to know everything they should about what crypto they should and shouldn't use, and you'll see a lot of people making the wrong choices.

Using #[deprecated] feels similar to my older (flawed) idea of using unsafe- it's a case of misusing a language feature. An MD5 function supported by rust-crypto isn't deprecated until the API for it is deprecated by rust-crypto, not by global standards around safe primitives.