False Negative: Weak Hashing Spec from Object to be used with MessageDigest is not reported

Clean Code Report False-positive / False-negative...
, ,
1

This is similar to False Negative: Weak Algorithm Spec from Object to be used with Cipher is not reported. The difference is that this False Negative is for MessageDigest.

package com.minimals.messagedigest.baseCase;
import java.security.MessageDigest;
import java.security.NoSuchAlgorithmException;
public class MessageDigestBase {
    public static void main(String[] args) {
        MessageDigest digest;
        String algorithmName = "MD5";
        try {
            digest = MessageDigest.getInstance(algorithmName);
            System.out.println(digest.getAlgorithm());
         } catch (NoSuchAlgorithmException e) {
             e.printStackTrace();
         }
    }
}

We scanned the source Java file using the docker. The command we used is:

sonar-scanner \
  -Dsonar.projectKey={PROJECT NAME} \
  -Dsonar.sources=. \
  -Dsonar.host.url=http://localhost:9000 \
  -Dsonar.login={PROJECT KEY}

However, while it is possible to statically compute the value to be passed to MessageDigest.getInstance(), SonarQube does not report it.

Of course, SonarQube reports the issue when MD5 is used directly, such as:

digest = MessageDigest.getInstance("MD5");

in a similar source file.

Which is why I think it is a false negative.

Additional Details
Language: Java
Rule: java:S4790 Using weak hashing algorithms is security-sensitive
Product Details:
SonarQube Community Edition Version - 9.6.1
Sonar Scanner Version - 4.7
Java Version - Java 11.0.14.1 Eclipse Adoptium (64-bit)
Operating System - MacOS Monterey version 12.6

2

Hi,

Can you upgrade to 9.9 and see if this is still replicable?

We improve the analyzers with every release, so it’s possible this has already been addressed.

 
Thx,
Ann

3

Hey there!

Can confirm that this issue persists and is reproducible for version 9.9.

Thanks!

6

Hey there Amit, thank you for pointing this out. Currently our analyzer avoids evaluating non-constant identifiers. This is because evaluating something which is not final might raise a lot of false positives.
To give you an example on top of your example

String algorithmName = "MD5";
try {
    someUknownMethodThatEditsStrings(algorithmName);
    digest = MessageDigest.getInstance(algorithmName);
    System.out.println(digest.getAlgorithm());
}

algorithmName not being final makes it very risky to assume its possible value if there are unknown operations before it gets passed to the MessageDigest, and we always prioritize not raising FPs.

This being said, we might still be able to statically determine if a non-constant variable has gone through steps in the code that might have changed its value. I think it would be interesting to try and track the lifecycle of the variable and determine if it has been changed or not, in order to consider it just like a constant. I created a ticket to dig into it!

2 Likes