SONARJAVA-7154: Implemented rule S9428 MessageDigest instances should not be stored as fields

its/ruling/src/test/resources/expected/eclipse-jetty/java-S9428.json

@@ -0,0 +1,5 @@
+{
+"eclipse-jetty:jetty-util/src/main/java/org/eclipse/jetty/util/security/Credential.java": [
+190
+]
+}

its/ruling/src/test/resources/expected/guava/java-S9428.json

@@ -0,0 +1,5 @@
+{
+"guava:src/com/google/common/hash/MessageDigestHashFunction.java": [
+121
+]
+}

java-checks-test-sources/default/src/main/java/checks/MessageDigestInstanceFieldCheckSample.java

@@ -0,0 +1,156 @@
+package checks;
+
+import java.security.MessageDigest;
+
+class MessageDigestInstanceFieldCheckSample {
+
+  private MessageDigest requestDigest; // Noncompliant {{Make this MessageDigest a local variable.}}
+//^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+  private static MessageDigest sharedDigest; // Noncompliant {{Make this MessageDigest a local variable.}}
+//^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+  private final MessageDigest configuredDigest; // Noncompliant {{Make this MessageDigest a local variable.}}
+//^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+  private CustomDigest customDigest; // Noncompliant {{Make this MessageDigest a local variable.}}
+//^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+  private String digest = "SHA-256"; // Compliant
+  private ThreadLocal<MessageDigest> threadDigest = new ThreadLocal<>(); // Compliant
+  private Object deferredDigest; // Compliant
+
+  MessageDigestInstanceFieldCheckSample() throws Exception {
+    configuredDigest = MessageDigest.getInstance("SHA-256");
+    MessageDigest constructorDigest = MessageDigest.getInstance("SHA-256"); // Compliant
+  }
+
+  void update(byte[] payload) {
+    requestDigest.update(payload);
+    sharedDigest.reset();
+    this.configuredDigest.digest(payload);
+    customDigest.update(payload);
+  }
+
+  byte[] hash(byte[] payload) throws Exception {
+    MessageDigest digest = MessageDigest.getInstance("SHA-256"); // Compliant
+    digest.update(payload);
+    return digest.digest();
+  }
+
+  void process(byte[][] payloads) throws Exception {
+    PayloadConsumer consumer = payload -> {
+      MessageDigest digest = MessageDigest.getInstance("SHA-256"); // Compliant
+      digest.update(payload);
+    };
+    for (byte[] payload : payloads) {
+      MessageDigest digest = MessageDigest.getInstance("SHA-256"); // Compliant
+      consumer.accept(digest.digest(payload));
+    }
+  }
+
+  void recover() {
+    try {
+      throw new IllegalArgumentException();
+    } catch (IllegalArgumentException exception) {
+      MessageDigest digest = null; // Compliant
+    }
+  }
+
+  class NestedHasher {
+    private MessageDigest digest; // Noncompliant {{Make this MessageDigest a local variable.}}
+//  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+    byte[] hash() {
+      return digest.digest();
+    }
+  }
+
+  static class PrototypeHolder {
+    private final MessageDigest prototype; // Compliant, only used as a prototype for cloning
+    private MessageDigest unused; // Compliant
+
+    PrototypeHolder() throws Exception {
+      this.prototype = MessageDigest.getInstance("SHA-256");
+    }
+
+    MessageDigest newDigest() throws Exception {
+      if (prototype.getDigestLength() > 0) {
+        return (MessageDigest) this.prototype.clone();
+      }
+      return MessageDigest.getInstance(prototype.getAlgorithm());
+    }
+  }
+
+  static class NonPrivatePrototypeHolder {
+    final MessageDigest prototype = null; // Noncompliant
+//  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+    Object newDigest() throws Exception {
+      return prototype.clone();
+    }
+  }
+
+  static class EscapingPrototypeHolder {
+    private MessageDigest returnedDigest; // Noncompliant
+//  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+    private MessageDigest passedDigest; // Noncompliant
+//  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+    private MessageDigest aliasedDigest; // Noncompliant
+//  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+    MessageDigest returned() throws Exception {
+      returnedDigest.clone();
+      return returnedDigest;
+    }
+
+    void passed() throws Exception {
+      passedDigest.clone();
+      consume(passedDigest);
+    }
+
+    void aliased() throws Exception {
+      aliasedDigest.clone();
+      MessageDigest alias = aliasedDigest; // Compliant
+      alias.update(new byte[0]);
+    }
+
+    void consume(MessageDigest digest) {
+    }
+  }
+
+  static class SynchronizedHolder {
+    private static final Object LOCK = new Object();
+    private static MessageDigest lockedDigest; // Noncompliant
+//  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+    private MessageDigest methodDigest; // Noncompliant
+//  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+    byte[] hash(byte[] payload) throws Exception {
+      synchronized (LOCK) {
+        lockedDigest.update(payload);
+        return lockedDigest.digest();
+      }
+    }
+
+    synchronized byte[] hashInMethod(byte[] payload) {
+      methodDigest.update(payload);
+      return methodDigest.digest();
+    }
+  }
+
+  interface PayloadConsumer {
+    void accept(byte[] payload) throws Exception;
+  }
+
+  abstract static class CustomDigest extends MessageDigest {
+    protected CustomDigest(String algorithm) {
+      super(algorithm);
+    }
+  }
+}
+
+interface DigestRegistry {
+  MessageDigest DEFAULT_DIGEST = null; // Noncompliant {{Make this MessageDigest a local variable.}}
+//^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+}
+
+record HashHolder(MessageDigest digest) { // Noncompliant {{Make this MessageDigest a local variable.}}
+//                ^^^^^^^^^^^^^^^^^^^^
+}

java-checks/src/main/java/org/sonar/java/checks/MessageDigestInstanceFieldCheck.java

@@ -0,0 +1,88 @@
+/*
+ * SonarQube Java
+ * Copyright (C) SonarSource Sàrl
+ * mailto:info AT sonarsource DOT com
+ *
+ * You can redistribute and/or modify this program under the terms of
+ * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
+ * See the Sonar Source-Available License for more details.
+ *
+ * You should have received a copy of the Sonar Source-Available License
+ * along with this program; if not, see https://sonarsource.com/license/ssal/
+ */
+package org.sonar.java.checks;
+
+import java.util.List;
+import org.sonar.check.Rule;
+import org.sonar.plugins.java.api.IssuableSubscriptionVisitor;
+import org.sonar.plugins.java.api.semantic.MethodMatchers;
+import org.sonar.plugins.java.api.semantic.Symbol;
+import org.sonar.plugins.java.api.tree.AssignmentExpressionTree;
+import org.sonar.plugins.java.api.tree.IdentifierTree;
+import org.sonar.plugins.java.api.tree.MemberSelectExpressionTree;
+import org.sonar.plugins.java.api.tree.MethodInvocationTree;
+import org.sonar.plugins.java.api.tree.Tree;
+import org.sonar.plugins.java.api.tree.VariableTree;
+
+@Rule(key = "S9428")
+public class MessageDigestInstanceFieldCheck extends IssuableSubscriptionVisitor {
+
+  private static final String MESSAGE_DIGEST = "java.security.MessageDigest";
+  private static final String MESSAGE = "Make this MessageDigest a local variable.";
+
+  private static final MethodMatchers STATELESS_METHODS = MethodMatchers.create()
+    .ofSubTypes(MESSAGE_DIGEST)
+    .names("clone", "getAlgorithm", "getDigestLength", "getProvider", "toString")
+    .addWithoutParametersMatcher()
+    .build();
+
+  @Override
+  public List<Tree.Kind> nodesToVisit() {
+    return List.of(Tree.Kind.VARIABLE);
+  }
+
+  @Override
+  public void visitNode(Tree tree) {
+    VariableTree variable = (VariableTree) tree;
+    Symbol symbol = variable.symbol();
+    if (symbol.type().isSubtypeOf(MESSAGE_DIGEST) && symbol.owner().isTypeSymbol() && !isOnlyUsedAsPrototype(symbol)) {
+      reportIssue(variable, MESSAGE);
+    }
+  }
+
+  /**
+   * A private field is only used as a prototype when, apart from being assigned, it is only the receiver of calls which do not
+   * depend on or alter the digest state, such as {@code clone()} or {@code getAlgorithm()}.
+   */
+  private static boolean isOnlyUsedAsPrototype(Symbol symbol) {
+    return symbol.isPrivate() && symbol.usages().stream()
+      .map(MessageDigestInstanceFieldCheck::fieldExpression)
+      .allMatch(expression -> isAssignedTo(expression) || isReceiverOfStatelessCall(expression));
+  }
+
+  private static Tree fieldExpression(IdentifierTree usage) {
+    Tree parent = usage.parent();
+    if (parent.is(Tree.Kind.MEMBER_SELECT) && ((MemberSelectExpressionTree) parent).identifier() == usage) {
+      return parent;
+    }
+    return usage;
+  }
+
+  private static boolean isAssignedTo(Tree expression) {
+    Tree parent = expression.parent();
+    return parent.is(Tree.Kind.ASSIGNMENT) && ((AssignmentExpressionTree) parent).variable() == expression;
+  }
+
+  private static boolean isReceiverOfStatelessCall(Tree expression) {
+    Tree parent = expression.parent();
+    if (!parent.is(Tree.Kind.MEMBER_SELECT) || ((MemberSelectExpressionTree) parent).expression() != expression) {
+      return false;
+    }
+    Tree invocation = parent.parent();
+    return invocation.is(Tree.Kind.METHOD_INVOCATION) && STATELESS_METHODS.matches((MethodInvocationTree) invocation);
+  }
+}

java-checks/src/test/java/org/sonar/java/checks/MessageDigestInstanceFieldCheckTest.java

@@ -0,0 +1,42 @@
+/*
+ * SonarQube Java
+ * Copyright (C) SonarSource Sàrl
+ * mailto:info AT sonarsource DOT com
+ *
+ * You can redistribute and/or modify this program under the terms of
+ * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
+ * See the Sonar Source-Available License for more details.
+ *
+ * You should have received a copy of the Sonar Source-Available License
+ * along with this program; if not, see https://sonarsource.com/license/ssal/
+ */
+package org.sonar.java.checks;
+
+import org.junit.jupiter.api.Test;
+import org.sonar.java.checks.verifier.CheckVerifier;
+
+import static org.sonar.java.checks.verifier.TestUtils.mainCodeSourcesPath;
+
+class MessageDigestInstanceFieldCheckTest {
+
+  @Test
+  void test() {
+    CheckVerifier.newVerifier()
+      .onFile(mainCodeSourcesPath("checks/MessageDigestInstanceFieldCheckSample.java"))
+      .withCheck(new MessageDigestInstanceFieldCheck())
+      .verifyIssues();
+  }
+
+  @Test
+  void test_without_semantic() {
+    CheckVerifier.newVerifier()
+      .onFile(mainCodeSourcesPath("checks/MessageDigestInstanceFieldCheckSample.java"))
+      .withCheck(new MessageDigestInstanceFieldCheck())
+      .withoutSemantic()
+      .verifyIssues();
+  }
+}

sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9428.html

@@ -0,0 +1,83 @@
+<p>This rule raises an issue when a cryptographic digest/hash object is declared as a class-level variable (either instance-level or shared across all
+instances).</p>
+<p>In Java, this specifically refers to <code>MessageDigest</code> instances.</p>
+<h2>Why is this an issue?</h2>
+<p>Cryptographic hash computation objects are stateful components that calculate hash values by maintaining internal state as data is incrementally
+processed through their methods.</p>
+<p>The problem is that these hash computation objects are not thread-safe. When stored as shared state (such as class-level fields or instance
+variables), the same object becomes accessible to multiple threads or multiple method invocations. If two threads invoke methods on the same hash
+computation object simultaneously, or if the object is reused between method calls without proper synchronization, the internal state becomes
+corrupted.</p>
+<p>This corruption leads to incorrect hash calculations. The resulting hash values may be completely wrong or inconsistent, which defeats the purpose
+of using cryptographic hashing in the first place.</p>
+<p>While it’s technically possible to synchronize access to a shared hash computation object, this approach adds complexity and potential performance
+bottlenecks. The synchronization overhead often negates any perceived performance benefits of reusing the object.</p>
+<p>Creating a new hash computation object for each use is relatively inexpensive. Modern runtime environments optimize object allocation efficiently,
+and the cost of instantiation is negligible compared to the actual hash computation work.</p>
+<p>In Java, this specifically refers to the <code>MessageDigest</code> class from the Java Security API, which provides methods like
+<code>update()</code> and <code>digest()</code> for hash computation.</p>
+<h3>What is the potential impact?</h3>
+<p>When hash calculations produce incorrect results due to unsynchronized access to shared hash computation objects or functions, the consequences can
+be severe:</p>
+<ul>
+  <li><strong>Data integrity failures</strong>: File integrity checks or data verification mechanisms may incorrectly report that data has been
+  corrupted when it hasn’t, or worse, fail to detect actual corruption.</li>
+  <li><strong>Authentication bypasses</strong>: Password hashing or authentication token generation may produce wrong values, potentially allowing
+  unauthorized access or preventing legitimate users from authenticating.</li>
+  <li><strong>Digital signature failures</strong>: Cryptographic signatures may be computed incorrectly, causing signature verification to fail or
+  producing signatures that don’t actually correspond to the signed data.</li>
+  <li><strong>Cache or deduplication errors</strong>: Systems using content hashes for caching or deduplication may incorrectly identify different
+  data as identical or fail to recognize identical data.</li>
+</ul>
+<p>These issues are particularly insidious because they may occur intermittently based on timing, making them difficult to detect during testing but
+capable of causing production failures.</p>
+<h2>How to fix it</h2>
+<p>Create the <code>MessageDigest</code> instance locally within the method that needs it, rather than storing it as a field. This ensures each
+operation has its own instance with isolated state.</p>
+<h3>Code examples</h3>
+<h4>Noncompliant code example</h4>
+<pre data-diff-id="1" data-diff-type="noncompliant">
+import java.security.MessageDigest;
+
+public class HashCalculator {
+    private final MessageDigest digest; // Noncompliant
+
+    public HashCalculator() throws Exception {
+        digest = MessageDigest.getInstance("SHA-256");
+    }
+
+    public byte[] computeHash(byte[] data) {
+        digest.reset();
+        digest.update(data);
+        return digest.digest();
+    }
+}
+</pre>
+<h4>Compliant solution</h4>
+<pre data-diff-id="1" data-diff-type="compliant">
+import java.security.MessageDigest;
+
+public class HashCalculator {
+    public byte[] computeHash(byte[] data) throws Exception {
+        MessageDigest digest = MessageDigest.getInstance("SHA-256");
+        digest.update(data);
+        return digest.digest();
+    }
+}
+</pre>
+<h2>Resources</h2>
+<h3>Documentation</h3>
+<ul>
+  <li>Oracle Java Documentation - <a
+  href="https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/security/MessageDigest.html">MessageDigest (Java SE 17 &amp; JDK
+  17)</a></li>
+  <li>Oracle Java Tutorials - <a href="https://docs.oracle.com/javase/tutorial/security/apisign/index.html">Generating and Verifying
+  Signatures</a></li>
+</ul>
+<h3>Standards</h3>
+<ul>
+  <li>CWE - <a href="https://cwe.mitre.org/data/definitions/362.html">CWE-362: Concurrent Execution using Shared Resource with Improper
+  Synchronization ('Race Condition')</a></li>
+  <li>CWE - <a href="https://cwe.mitre.org/data/definitions/567.html">CWE-567: Unsynchronized Access to Shared Data in a Multithreaded
+  Context</a></li>
+</ul>