diff --git a/its/ruling/src/test/resources/commons-beanutils/java-S9345.json b/its/ruling/src/test/resources/commons-beanutils/java-S9345.json new file mode 100644 index 00000000000..efeec87b4a1 --- /dev/null +++ b/its/ruling/src/test/resources/commons-beanutils/java-S9345.json @@ -0,0 +1,32 @@ +{ +"commons-beanutils:commons-beanutils:src/main/java/org/apache/commons/beanutils2/BeanPropertyValueChangeClosure.java": [ +79 +], +"commons-beanutils:commons-beanutils:src/main/java/org/apache/commons/beanutils2/BeanPropertyValueEqualsPredicate.java": [ +110 +], +"commons-beanutils:commons-beanutils:src/main/java/org/apache/commons/beanutils2/BeanToPropertyValueTransformer.java": [ +71 +], +"commons-beanutils:commons-beanutils:src/main/java/org/apache/commons/beanutils2/FluentPropertyBeanIntrospector.java": [ +78 +], +"commons-beanutils:commons-beanutils:src/main/java/org/apache/commons/beanutils2/MappedPropertyDescriptor.java": [ +44 +], +"commons-beanutils:commons-beanutils:src/main/java/org/apache/commons/beanutils2/MethodUtils.java": [ +1304 +], +"commons-beanutils:commons-beanutils:src/main/java/org/apache/commons/beanutils2/ResultSetDynaClass.java": [ +82 +], +"commons-beanutils:commons-beanutils:src/main/java/org/apache/commons/beanutils2/RowSetDynaClass.java": [ +66 +], +"commons-beanutils:commons-beanutils:src/main/java/org/apache/commons/beanutils2/SuppressPropertiesBeanIntrospector.java": [ +38 +], +"commons-beanutils:commons-beanutils:src/main/java/org/apache/commons/beanutils2/converters/ArrayConverter.java": [ +129 +] +} diff --git a/its/ruling/src/test/resources/eclipse-jetty/java-S9345.json b/its/ruling/src/test/resources/eclipse-jetty/java-S9345.json new file mode 100644 index 00000000000..2ef088f3283 --- /dev/null +++ b/its/ruling/src/test/resources/eclipse-jetty/java-S9345.json @@ -0,0 +1,107 @@ +{ +"org.eclipse.jetty:jetty-project:jetty-http/src/main/java/org/eclipse/jetty/http/HostPortHttpField.java": [ +28 +], +"org.eclipse.jetty:jetty-project:jetty-http/src/main/java/org/eclipse/jetty/http/HttpCookie.java": [ +32 +], +"org.eclipse.jetty:jetty-project:jetty-http/src/main/java/org/eclipse/jetty/http/PrecompressedHttpContent.java": [ +30 +], +"org.eclipse.jetty:jetty-project:jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/UriTemplatePathSpec.java": [ +41 +], +"org.eclipse.jetty:jetty-project:jetty-io/src/main/java/org/eclipse/jetty/io/ArrayByteBufferPool.java": [ +39 +], +"org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/CustomRequestLog.java": [ +273 +], +"org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/Dispatcher.java": [ +41 +], +"org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/EncodingHttpWriter.java": [ +29 +], +"org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannelListeners.java": [ +33 +], +"org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/MultiPartFormInputStream.java": [ +83 +], +"org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/ServletPathMapping.java": [ +38 +], +"org.eclipse.jetty:jetty-project:jetty-util-ajax/src/main/java/org/eclipse/jetty/util/ajax/JSONPojoConvertorFactory.java": [ +29 +], +"org.eclipse.jetty:jetty-project:jetty-util/src/main/java/org/eclipse/jetty/util/BlockingArrayQueue.java": [ +49 +], +"org.eclipse.jetty:jetty-project:jetty-util/src/main/java/org/eclipse/jetty/util/ClassLoadingObjectInputStream.java": [ +32 +], +"org.eclipse.jetty:jetty-project:jetty-util/src/main/java/org/eclipse/jetty/util/CountingCallback.java": [ +41 +], +"org.eclipse.jetty:jetty-project:jetty-util/src/main/java/org/eclipse/jetty/util/HostPort.java": [ +26 +], +"org.eclipse.jetty:jetty-project:jetty-util/src/main/java/org/eclipse/jetty/util/IncludeExcludeSet.java": [ +39 +], +"org.eclipse.jetty:jetty-project:jetty-util/src/main/java/org/eclipse/jetty/util/InetAddressPattern.java": [ +110, +191, +236 +], +"org.eclipse.jetty:jetty-project:jetty-util/src/main/java/org/eclipse/jetty/util/MultiPartOutputStream.java": [ +29 +], +"org.eclipse.jetty:jetty-project:jetty-util/src/main/java/org/eclipse/jetty/util/MultiPartWriter.java": [ +28 +], +"org.eclipse.jetty:jetty-project:jetty-util/src/main/java/org/eclipse/jetty/util/MultiReleaseJarFile.java": [ +38, +154 +], +"org.eclipse.jetty:jetty-project:jetty-util/src/main/java/org/eclipse/jetty/util/PathWatcher.java": [ +70 +], +"org.eclipse.jetty:jetty-project:jetty-util/src/main/java/org/eclipse/jetty/util/QuotedStringTokenizer.java": [ +37 +], +"org.eclipse.jetty:jetty-project:jetty-util/src/main/java/org/eclipse/jetty/util/RolloverFileOutputStream.java": [ +51 +], +"org.eclipse.jetty:jetty-project:jetty-util/src/main/java/org/eclipse/jetty/util/Uptime.java": [ +36 +], +"org.eclipse.jetty:jetty-project:jetty-util/src/main/java/org/eclipse/jetty/util/component/FileDestroyable.java": [ +32 +], +"org.eclipse.jetty:jetty-project:jetty-util/src/main/java/org/eclipse/jetty/util/resource/PathResource.java": [ +53 +], +"org.eclipse.jetty:jetty-project:jetty-util/src/main/java/org/eclipse/jetty/util/resource/ResourceCollection.java": [ +43 +], +"org.eclipse.jetty:jetty-project:jetty-util/src/main/java/org/eclipse/jetty/util/security/CertificateValidator.java": [ +55 +], +"org.eclipse.jetty:jetty-project:jetty-util/src/main/java/org/eclipse/jetty/util/ssl/KeyStoreScanner.java": [ +40 +], +"org.eclipse.jetty:jetty-project:jetty-util/src/main/java/org/eclipse/jetty/util/ssl/X509.java": [ +37 +], +"org.eclipse.jetty:jetty-project:jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java": [ +48 +], +"org.eclipse.jetty:jetty-project:jetty-xml/src/main/java/org/eclipse/jetty/xml/XmlAppendable.java": [ +30 +], +"org.eclipse.jetty:jetty-project:jetty-xml/src/main/java/org/eclipse/jetty/xml/XmlConfiguration.java": [ +87 +] +} diff --git a/its/ruling/src/test/resources/guava/java-S9345.json b/its/ruling/src/test/resources/guava/java-S9345.json new file mode 100644 index 00000000000..68013f90b30 --- /dev/null +++ b/its/ruling/src/test/resources/guava/java-S9345.json @@ -0,0 +1,8 @@ +{ +"com.google.guava:guava:src/com/google/common/base/FinalizableReferenceQueue.java": [ +94 +], +"com.google.guava:guava:src/com/google/common/io/MultiReader.java": [ +33 +] +} diff --git a/its/ruling/src/test/resources/sonar-server/java-S9345.json b/its/ruling/src/test/resources/sonar-server/java-S9345.json new file mode 100644 index 00000000000..bcad8a0b67f --- /dev/null +++ b/its/ruling/src/test/resources/sonar-server/java-S9345.json @@ -0,0 +1,23 @@ +{ +"org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/computation/task/projectanalysis/source/ReportIterator.java": [ +33 +], +"org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/issue/index/IssueIteratorForSingleChunk.java": [ +53 +], +"org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/platform/web/MasterServletFilter.java": [ +42 +], +"org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/plugins/UpdateCenterClient.java": [ +64 +], +"org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/user/SecurityRealmFactory.java": [ +38 +], +"org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/util/ObjectInputStreamIterator.java": [ +31 +], +"org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/util/cache/DiskCache.java": [ +37 +] +} diff --git a/java-checks-test-sources/default/src/main/java/checks/FinalizerAttackCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/FinalizerAttackCheckSample.java new file mode 100644 index 00000000000..bca0864e9a7 --- /dev/null +++ b/java-checks-test-sources/default/src/main/java/checks/FinalizerAttackCheckSample.java @@ -0,0 +1,189 @@ +package checks; + +class FinalizerAttackCheckSample { + + // --- Noncompliant: non-final class with throwing constructor --- + + static class SecurityService { // Secondary {{Non-final class}} + private final String token; + + public SecurityService(String token) throws IllegalArgumentException { // Noncompliant {{Make this class "final" or make this throwing constructor "private".}} + if (token == null) { + throw new IllegalArgumentException("Invalid token"); + } + this.token = token; + } + } + + static class AuthProvider { // Secondary {{Non-final class}} + public AuthProvider(String credentials) throws Exception { // Noncompliant + if (credentials.isEmpty()) { + throw new Exception("Bad credentials"); + } + } + } + + static class ResourceLoader { // Secondary {{Non-final class}} + ResourceLoader(String path) { // Noncompliant + if (path == null) { + throw new NullPointerException(); + } + } + } + + static class MultiConstructorService { // Secondary {{Non-final class}} + MultiConstructorService(int id) throws Exception { // Noncompliant + if (id < 0) { + throw new Exception("Negative id"); + } + } + + MultiConstructorService(String name) { + } + } + + static class ProtectedConstructorService { // Secondary {{Non-final class}} + protected ProtectedConstructorService(String data) throws Exception { // Noncompliant + if (data == null) { + throw new Exception("Null data"); + } + } + } + + static class ThrowsClauseOnly { // Secondary {{Non-final class}} + public ThrowsClauseOnly() throws Exception { // Noncompliant + } + } + + // --- Compliant: final class --- + + static final class SecureService { + public SecureService(String token) throws IllegalArgumentException { + if (token == null) { + throw new IllegalArgumentException("Invalid token"); + } + } + } + + // --- Compliant: all constructors private (factory pattern) --- + + static class FactoryService { + private FactoryService(String data) { + } + + public static FactoryService create(String data) throws Exception { + if (data == null) { + throw new Exception("Null"); + } + return new FactoryService(data); + } + } + + // --- Compliant: no throwing constructor --- + + static class SafeService { + public SafeService(String data) { + // no throw + } + } + + static class NoConstructor { + void doSomething() { + } + } + + // --- Compliant: abstract class --- + + static abstract class AbstractService { + public AbstractService(String data) throws Exception { + if (data == null) { + throw new Exception("Null"); + } + } + } + + // --- Compliant: private throwing constructor, public non-throwing constructor --- + + static class MixedConstructors { + private MixedConstructors(String data) throws Exception { + if (data == null) { + throw new Exception("Null"); + } + } + + public MixedConstructors(int id) { + } + } + + // --- Compliant: enum (implicitly final) --- + + enum Status { + ACTIVE, INACTIVE; + + Status() { + } + } + + // --- Compliant: record (implicitly final) --- + + record Credential(String value) { + Credential { + if (value == null) { + throw new IllegalArgumentException("Null value"); + } + } + } + + // --- Compliant: inner interface (no constructors) --- + + interface Service { + void execute(); + } + + // --- Noncompliant: throw in constructor body without throws clause --- + + static class ConfigLoader { // Secondary {{Non-final class}} + public ConfigLoader(String config) { // Noncompliant + if (config == null) { + throw new IllegalStateException("Missing config"); + } + } + } + + // --- Compliant: throw in a method, not in constructor --- + + static class Processor { + public Processor() { + } + + public void process() { + throw new UnsupportedOperationException(); + } + } + + // --- Noncompliant: nested throw in try block within constructor --- + + static class DatabaseConnection { // Secondary {{Non-final class}} + public DatabaseConnection(String url) { // Noncompliant + try { + if (url == null) { + throw new RuntimeException("Null URL"); + } + } catch (Exception e) { + throw new RuntimeException("Connection failed", e); + } + } + } + + // --- Compliant: all throwing constructors are private --- + + static class PrivateOnlyThrowers { + private PrivateOnlyThrowers(String s) throws Exception { + throw new Exception(); + } + + private PrivateOnlyThrowers(int i) { + throw new IllegalArgumentException(); + } + } +} diff --git a/java-checks/src/main/java/org/sonar/java/checks/FinalizerAttackCheck.java b/java-checks/src/main/java/org/sonar/java/checks/FinalizerAttackCheck.java new file mode 100644 index 00000000000..862189f4d6c --- /dev/null +++ b/java-checks/src/main/java/org/sonar/java/checks/FinalizerAttackCheck.java @@ -0,0 +1,97 @@ +/* + * 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.Collections; +import java.util.List; +import org.sonar.check.Rule; +import org.sonar.java.model.ModifiersUtils; +import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; +import org.sonar.plugins.java.api.JavaFileScannerContext; +import org.sonar.plugins.java.api.tree.BaseTreeVisitor; +import org.sonar.plugins.java.api.tree.BlockTree; +import org.sonar.plugins.java.api.tree.ClassTree; +import org.sonar.plugins.java.api.tree.LambdaExpressionTree; +import org.sonar.plugins.java.api.tree.MethodTree; +import org.sonar.plugins.java.api.tree.Modifier; +import org.sonar.plugins.java.api.tree.ThrowStatementTree; +import org.sonar.plugins.java.api.tree.Tree; +import org.sonar.plugins.java.api.tree.Tree.Kind; + +@Rule(key = "S9345") +public class FinalizerAttackCheck extends IssuableSubscriptionVisitor { + + @Override + public List nodesToVisit() { + return Collections.singletonList(Kind.CLASS); + } + + @Override + public void visitNode(Tree tree) { + ClassTree classTree = (ClassTree) tree; + if (ModifiersUtils.hasModifier(classTree.modifiers(), Modifier.FINAL) || + ModifiersUtils.hasModifier(classTree.modifiers(), Modifier.ABSTRACT)) { + return; + } + List secondaryLocations = Collections.singletonList( + new JavaFileScannerContext.Location("Non-final class", classTree.simpleName())); + for (Tree member : classTree.members()) { + if (member.is(Kind.CONSTRUCTOR) && isVulnerableConstructor((MethodTree) member)) { + MethodTree constructor = (MethodTree) member; + reportIssue(constructor.simpleName(), + "Make this class \"final\" or make this throwing constructor \"private\".", + secondaryLocations, null); + } + } + } + + private static boolean isVulnerableConstructor(MethodTree constructor) { + if (ModifiersUtils.hasModifier(constructor.modifiers(), Modifier.PRIVATE)) { + return false; + } + return !constructor.throwsClauses().isEmpty() || containsThrowStatement(constructor); + } + + private static boolean containsThrowStatement(MethodTree constructor) { + BlockTree block = constructor.block(); + if (block == null) { + return false; + } + ThrowStatementVisitor visitor = new ThrowStatementVisitor(); + block.accept(visitor); + return visitor.hasThrow; + } + + private static class ThrowStatementVisitor extends BaseTreeVisitor { + boolean hasThrow; + + @Override + public void visitThrowStatement(ThrowStatementTree tree) { + hasThrow = true; + } + + @Override + public void visitClass(ClassTree tree) { + // skip nested classes + } + + @Override + public void visitLambdaExpression(LambdaExpressionTree tree) { + // skip lambdas + } + } +} diff --git a/java-checks/src/test/java/org/sonar/java/checks/FinalizerAttackCheckTest.java b/java-checks/src/test/java/org/sonar/java/checks/FinalizerAttackCheckTest.java new file mode 100644 index 00000000000..8ab83cb9003 --- /dev/null +++ b/java-checks/src/test/java/org/sonar/java/checks/FinalizerAttackCheckTest.java @@ -0,0 +1,43 @@ +/* + * 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 FinalizerAttackCheckTest { + + @Test + void test() { + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath("checks/FinalizerAttackCheckSample.java")) + .withCheck(new FinalizerAttackCheck()) + .verifyIssues(); + } + + @Test + void test_without_semantic() { + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath("checks/FinalizerAttackCheckSample.java")) + .withCheck(new FinalizerAttackCheck()) + .withoutSemantic() + .verifyIssues(); + } + +} diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9345.html b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9345.html new file mode 100644 index 00000000000..ff87bbc6281 --- /dev/null +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9345.html @@ -0,0 +1,68 @@ +

Why is this an issue?

+

When a constructor throws an exception, you might expect the object construction to fail completely and no reference to the object to exist. However, +finalization or cleanup mechanisms can be exploited to obtain a reference to a partially-constructed object.

+

Here's how a Finalizer attack works:

+
    +
  1. An attacker creates a malicious derived class that overrides the cleanup/finalization method
    2. +
  2. The attacker attempts to instantiate this derived class
    3. +
  3. If the parent constructor throws an exception during initialization, the object is not fully constructed
    4. +
  4. Despite the exception, the garbage collector will eventually call the finalization method on the partially-constructed object
    5. +
  5. The malicious cleanup method can store a reference to the object being finalized, effectively "resurrecting" the broken object
    6. +
  6. The attacker now has access to an object that bypassed security checks or validation logic in the constructor
    7. +
+

This vulnerability is particularly dangerous for security-sensitive classes where the constructor performs authentication or authorization checks, +input validation, resource allocation with security constraints, or initialization of security-critical fields.

+

How to fix it

+

The simplest solution is to declare the class as final. This prevents attackers from creating malicious subclasses that override the +finalize() method.

+

However, some frameworks such as Spring or JPA/Hibernate require non-final classes. In such cases, use a factory method with a private +constructor to ensure the object is fully validated before any reference is exposed. Since the constructor is private, no malicious subclass +can be created, achieving the same protection as final.

+

Noncompliant code example

+
+public class SecuritySensitiveClass {
+    private final String credentials;
+
+    public SecuritySensitiveClass(String credentials) throws AuthenticationException {
+        if (!isValid(credentials)) {
+            throw new AuthenticationException("Invalid credentials"); // Noncompliant
+        }
+        this.credentials = credentials;
+    }
+
+    private boolean isValid(String credentials) {
+        return credentials != null && credentials.length() > 10;
+    }
+}
+
+

Compliant solution

+
+public final class SecuritySensitiveClass { // Compliant: class is final
+    private final String credentials;
+
+    public SecuritySensitiveClass(String credentials) throws AuthenticationException {
+        if (!isValid(credentials)) {
+            throw new AuthenticationException("Invalid credentials");
+        }
+        this.credentials = credentials;
+    }
+
+    private boolean isValid(String credentials) {
+        return credentials != null && credentials.length() > 10;
+    }
+}
+
+

Resources

+

Documentation

+ +

Standards

+ diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9345.json b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9345.json new file mode 100644 index 00000000000..87c32a9e4cf --- /dev/null +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9345.json @@ -0,0 +1,32 @@ +{ + "title": "Classes with throwing constructors should be protected against Finalizer attacks", + "type": "VULNERABILITY", + "status": "ready", + "remediation": { + "func": "Constant\/Issue", + "constantCost": "5min" + }, + "tags": [ + "cert", + "cwe", + "serialization" + ], + "defaultSeverity": "Critical", + "ruleSpecification": "RSPEC-9345", + "sqKey": "S9345", + "scope": "Main", + "defaultQualityProfiles": [ + "Sonar way" + ], + "quickfix": "unknown", + "code": { + "impacts": { + "SECURITY": "HIGH" + }, + "attribute": "COMPLETE" + }, + "securityStandards": { + "CWE": [586], + "CERT": ["OBJ11-J."] + } +} diff --git a/sonar-java-plugin/src/main/resources/profiles/Sonar_way/S9345 b/sonar-java-plugin/src/main/resources/profiles/Sonar_way/S9345 new file mode 100644 index 00000000000..e69de29bb2d