Fix S4790 FP: HMACSHA1 is not insecure

[Bobsson]

Description

HMAC-SHA1 is not currently considered insecure, although there are certainly better alternatives. It's not even mentioned in the description for S4790, but it's included on the list of unsecure algorithms in CreatingHashAlgorithmsBase.cs.

Reference for it still being secure (as of 2022): https://crypto.stackexchange.com/questions/101691/is-hmac-sha1-algorithm-secure-in-2022

Repro steps

using HMAC hmac = new HMACSHA1();

Expected behavior

HMACSHA1 shouldn't be being flagged as insecure. If it is going to continue being treated as insecure, then this is a documentation bug and it should be added to the S470 description.

Related information

• SQ Enterprise version 9.9

[gaetan-ferry-sonarsource]

Hi @Bobsson,

First of all, thanks for reaching out. Receiving feedback from our users on our rules, and especially security rules, is always something we look forward to.

I'll try to be as precise and objective as possible here but let me know if there is anything you think I am wrong with or you would like to challenge.

TL;DR

SHA1 is still practically safe to use and will remain so for a long time. However, NIST announced transitioning away from it, and this is probably something you want to work on now. If you want to keep using HMAC with a hash function output size of 160 bits, you can consider using a truncated version of a SHA2 function following NIST recommendations.

I think the rule description is OK when it comes to SHA1 but maybe I missed your point.

Why raise on HMAC-SHA1?

To begin with, the source you link, and especially the paper about collision requirements for HMAC, is perfectly true and valid. The fact is, we don't know of any practical attack on HMAC-SHA1 that would require immediate action on the users' side, and no one expects this will happen anytime soon. So, if you rely on HMAC-SHA1 and have no way to switch away from it, be reassured, you are not under any immediate threat.

However, the fact that attacks exist on SHA1, even if only on the collision side, is already proof that the algorithm design is faulty to some extent. Discovering collision attacks on a hash algorithm usually is a good reason to kickstart a deprecation process.

Actually, NIST published a document in December 2022, a few months after the StackOverflow answer you linked was posted, that posed the milestones for SHA1 deprecation. There is also this news article if you prefer a higher-level view of this topic.

While the final deadline for SHA1 removal is only December 2023, NIST SP.800-131Ar2, published in March 2019 was already conservative about SHA1.

and

HMAC Generation:
Any approved hash function may be used

One could argue that this leaves a grey zone for HMAC with SHA1. I would agree with that. However, overall, it seems clear that moving away from SHA1 is generally a sane decision. It is at least what drove the decision to raise on HMAC-SHA1 in S4790.

S4790 description content

When it comes to the rule description content, it seems to me that it correctly reflects the rule implementation, at least when it comes to SHA1.

Cryptographic hash algorithms such as MD2, MD4, MD5, MD6, HAVAL-128, HMAC-MD5, DSA (which uses SHA-1), RIPEMD, RIPEMD-128, RIPEMD-160, HMACRIPEMD160 and SHA-1 are no longer considered secure

It is possible that I am missing your point here. Let me know if this is the case.

I would agree that, overall, the rule description is not very good and could use some improvement both on the side of provided details and general text quality. We are currently undergoing a rework of our crypto-related rules, of which you will soon be able to see the results. Unfortunately, we did not prioritize hotspots and C# language has not been covered yet. We still have plenty to do there so be sure to stay tuned.

Any replacement?

Generally speaking, using a SHA2 family function to replace SHA1 in the HMAC construction is what should be done. It should not involve a lot of performance overhead or any other big issue apart from protocol compatibility matters.

The only reason I can think of for which you might want to keep SHA1 is the output size. SHA1 has an output size of 160 bits which is much shorter than what SHA224 or SHA256 (let alone SHA512) can provide. If this is an issue for you, you could consider using a truncated version of SHA256. This is perfectly safe if done following NIST recommendations that you can find in NIST SP.800-107r1 chapter 5.1.

This would probably be named something like HMAC-SHA256-160 which, if nothing else, at least has a nice name.

I hope this answers your question and helps.

Let me know if there is anything else we can do.

Cheers!

[Bobsson]

Hi @gaetan-ferry-sonarsource. Thanks for the thorough reply on this. I'll take it back and consider it for a bit.

As far as the documentation issue, this is what I'm seeing:

Algorithm	Code?	Documented?
DSA	Y	Y
HMACMD5	Y	Y
HMACRIPEMD160	Y	Y
HMACSHA1	Y	N
MD2	N	Y
MD4	N	Y
MD5	Y	Y
MD6	N	Y
RIPEMD	N	Y
RIPEMD-128	N	Y
RIPEMD160	Y	Y
SHA1	Y	Y
HAVAL-128	N	Y

As you can see, HMACSHA1 is the only one that exists in the code and not in the documentation. Everything else is in the documentation list, whether or not it's being scanned for.

[gaetan-ferry-sonarsource]

@Bobsson OK, I get it now. There is indeed an inconsistency in how algorithms are documented there. I would not personally expect that HMAC-* be mentioned specifically. HMAC is not a hashing algorithm but a message authentication algorithm. It is a construction upon an underlying, actual hash algorithm and, as such, should not be mentioned here, or at least not in that way.

I just checked our backlog, and it happens we already have a ticket tracking the poor description quality. Considering we are already working on our crypto rule, I think it will be tackled when we work on crypto-related hotspots later this year. Do not expect an update in the upcoming weeks, but be sure we are tracking this in this year's objectives.

Thanks again for your feedback.

Cheers!