diff --git a/its/ruling/src/test/resources/expected/eclipse-jetty/java-S9428.json b/its/ruling/src/test/resources/expected/eclipse-jetty/java-S9428.json new file mode 100644 index 00000000000..b33019fcbed --- /dev/null +++ b/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 +] +} diff --git a/its/ruling/src/test/resources/expected/guava/java-S9428.json b/its/ruling/src/test/resources/expected/guava/java-S9428.json new file mode 100644 index 00000000000..2ace408419b --- /dev/null +++ b/its/ruling/src/test/resources/expected/guava/java-S9428.json @@ -0,0 +1,5 @@ +{ +"guava:src/com/google/common/hash/MessageDigestHashFunction.java": [ +121 +] +} diff --git a/java-checks-test-sources/default/src/main/java/checks/MessageDigestInstanceFieldCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/MessageDigestInstanceFieldCheckSample.java new file mode 100644 index 00000000000..934d227efb0 --- /dev/null +++ b/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 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.}} +// ^^^^^^^^^^^^^^^^^^^^ +} diff --git a/java-checks/src/main/java/org/sonar/java/checks/MessageDigestInstanceFieldCheck.java b/java-checks/src/main/java/org/sonar/java/checks/MessageDigestInstanceFieldCheck.java new file mode 100644 index 00000000000..33e118b3c39 --- /dev/null +++ b/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 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); + } +} diff --git a/java-checks/src/test/java/org/sonar/java/checks/MessageDigestInstanceFieldCheckTest.java b/java-checks/src/test/java/org/sonar/java/checks/MessageDigestInstanceFieldCheckTest.java new file mode 100644 index 00000000000..48a73cbeed0 --- /dev/null +++ b/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(); + } +} diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9428.html b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9428.html new file mode 100644 index 00000000000..7856aaa0221 --- /dev/null +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9428.html @@ -0,0 +1,83 @@ +

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).

+

In Java, this specifically refers to MessageDigest instances.

+

Why is this an issue?

+

Cryptographic hash computation objects are stateful components that calculate hash values by maintaining internal state as data is incrementally +processed through their methods.

+

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.

+

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.

+

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.

+

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.

+

In Java, this specifically refers to the MessageDigest class from the Java Security API, which provides methods like +update() and digest() for hash computation.

+

What is the potential impact?

+

When hash calculations produce incorrect results due to unsynchronized access to shared hash computation objects or functions, the consequences can +be severe:

+ +

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.

+

How to fix it

+

Create the MessageDigest 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.

+

Code examples

+

Noncompliant code example

+
+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();
+    }
+}
+
+

Compliant solution

+
+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();
+    }
+}
+
+

Resources

+

Documentation

+ +

Standards

+ diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9428.json b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9428.json new file mode 100644 index 00000000000..9c8021a2753 --- /dev/null +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9428.json @@ -0,0 +1,33 @@ +{ + "title": "MessageDigest instances should not be stored as fields", + "type": "BUG", + "status": "ready", + "remediation": { + "func": "Constant\/Issue", + "constantCost": "5min" + }, + "tags": [ + "concurrency", + "cryptography", + "pitfall" + ], + "defaultSeverity": "Critical", + "ruleSpecification": "RSPEC-9428", + "sqKey": "S9428", + "scope": "All", + "quickfix": "unknown", + "code": { + "impacts": { + "SECURITY": "MEDIUM", + "RELIABILITY": "HIGH", + "MAINTAINABILITY": "MEDIUM" + }, + "attribute": "COMPLETE" + }, + "securityStandards": { + "CWE": [ + 362, + 567 + ] + } +} diff --git a/sonar-java-plugin/src/main/resources/profiles/Sonar_way/S9428 b/sonar-java-plugin/src/main/resources/profiles/Sonar_way/S9428 new file mode 100644 index 00000000000..e69de29bb2d