From 119bacd5fcc8390f9e6f4429d5ea193974498f65 Mon Sep 17 00:00:00 2001 From: Tomasz Tylenda Date: Mon, 7 Jul 2025 14:02:36 +0200 Subject: [PATCH 1/6] S2077 Fix FN on strings built with String.format()/formatted(). --- .../src/main/java/checks/SQLInjection.java | 60 +++++++++++++++++++ .../sonar/java/checks/SQLInjectionCheck.java | 34 ++++++++++- 2 files changed, 91 insertions(+), 3 deletions(-) diff --git a/java-checks-test-sources/default/src/main/java/checks/SQLInjection.java b/java-checks-test-sources/default/src/main/java/checks/SQLInjection.java index 43be49c99b3..3b49e4eec6d 100644 --- a/java-checks-test-sources/default/src/main/java/checks/SQLInjection.java +++ b/java-checks-test-sources/default/src/main/java/checks/SQLInjection.java @@ -7,6 +7,7 @@ import java.sql.SQLException; import java.sql.Statement; import java.sql.PreparedStatement; +import java.util.Locale; import org.hibernate.Session; import javax.persistence.EntityManager; @@ -240,3 +241,62 @@ private void foo() { tmpl.queryForObject(user, String.class); // compliant } } + +class SQLFormat { + private final Statement stmt; + + public SQLFormat(Statement stmt) { + this.stmt = stmt; + } + + public void formatInline(String input) throws SQLException { + this.stmt.execute(String.format("SELECT %s", input)); // Noncompliant + } + + public void formatInlineConst(String input) throws SQLException { + this.stmt.execute(String.format("SELECT %s","1")); + } + + public void formatVar(String input) throws SQLException { + String query = String.format("SELECT %s", input); + this.stmt.execute(query); // Noncompliant + } + + public void formatVarConst(String input) throws SQLException { + String query = String.format("SELECT %s", "1"); + this.stmt.execute(query); + } + + public void formatLocale(Locale locale, String input) throws SQLException { + String query = String.format(locale, "SELECT %s", input); + this.stmt.execute(query); // Noncompliant + } + + public void formatLocaleConst(Locale locale, String input) throws SQLException { + String query = String.format(locale, "SELECT %s", "1"); + this.stmt.execute(query); + } + + public void formatted(Locale locale, String input) throws SQLException { + String query = "SELECT %s".formatted(input); + this.stmt.execute(query); // Noncompliant + } + + public void formattedConst(Locale locale, String input) throws SQLException { + String query = "SELECT %s".formatted("1"); + this.stmt.execute(query); + } + + public void plusAssignment(String input) throws SQLException { + String query = "SELECT"; + query += String.format("WHERE col = %c", input); + this.stmt.execute(query); // Noncompliant + } + + public void plusAssignmentConst() throws SQLException { + // FP, but probably rare and not worth complicating the code to fix it. + String query = "SELECT"; + query += String.format("WHERE col = \"%c\"", "value"); + this.stmt.execute(query); // Noncompliant + } +} diff --git a/java-checks/src/main/java/org/sonar/java/checks/SQLInjectionCheck.java b/java-checks/src/main/java/org/sonar/java/checks/SQLInjectionCheck.java index 8370e854cb3..83d7c65b2b5 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/SQLInjectionCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/SQLInjectionCheck.java @@ -24,6 +24,7 @@ import java.util.stream.Stream; import javax.annotation.Nullable; import org.sonar.check.Rule; +import org.sonar.java.checks.helpers.QuickFixHelper; import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; import org.sonar.plugins.java.api.JavaFileScannerContext; import org.sonar.plugins.java.api.semantic.MethodMatchers; @@ -112,6 +113,14 @@ public class SQLInjectionCheck extends IssuableSubscriptionVisitor { .withAnyParameters() .build()); + private static final String JAVA_LANG_STRING = "java.lang.String"; + + private static final MethodMatchers FORMAT_METHODS = MethodMatchers.create() + .ofTypes(JAVA_LANG_STRING) + .names("format", "formatted") + .withAnyParameters() + .build(); + private static final String MAIN_MESSAGE = "Make sure using a dynamically formatted SQL query is safe here."; @Override @@ -123,12 +132,12 @@ public List nodesToVisit() { public void visitNode(Tree tree) { if (anyMatch(tree)) { Optional sqlStringArg = arguments(tree) - .filter(arg -> arg.symbolType().is("java.lang.String")) + .filter(arg -> arg.symbolType().is(JAVA_LANG_STRING)) .findFirst(); if (sqlStringArg.isPresent()) { ExpressionTree sqlArg = sqlStringArg.get(); - if (isDynamicConcatenation(sqlArg)) { + if (isDynamicString(sqlArg)) { reportIssue(sqlArg, MAIN_MESSAGE); } else if (sqlArg.is(Tree.Kind.IDENTIFIER)) { IdentifierTree identifierTree = (IdentifierTree) sqlArg; @@ -136,7 +145,7 @@ public void visitNode(Tree tree) { ExpressionTree initializerOrExpression = getInitializerOrExpression(symbol.declaration()); List reassignments = getReassignments(symbol.owner().declaration(), symbol.usages()); - if ((initializerOrExpression != null && isDynamicConcatenation(initializerOrExpression)) || + if ((initializerOrExpression != null && isDynamicString(initializerOrExpression)) || reassignments.stream().anyMatch(SQLInjectionCheck::isDynamicPlusAssignment)) { reportIssue(sqlArg, MAIN_MESSAGE, secondaryLocations(initializerOrExpression, reassignments, identifierTree.name()), null); } @@ -197,7 +206,26 @@ private static boolean isDynamicPlusAssignment(ExpressionTree arg) { return arg.is(Tree.Kind.PLUS_ASSIGNMENT) && !((AssignmentExpressionTree) arg).expression().asConstant().isPresent(); } + private static boolean isDynamicString(ExpressionTree arg) { + return isDynamicConcatenation(arg) || isDynamicFormatting(arg); + } + private static boolean isDynamicConcatenation(ExpressionTree arg) { return arg.is(Tree.Kind.PLUS) && !arg.asConstant().isPresent(); } + + private static boolean isDynamicFormatting(Tree tree) { + return tree instanceof MethodInvocationTree mit + && FORMAT_METHODS.matches(mit) + && hasDynamicStringParameters(mit); + } + + private static boolean hasDynamicStringParameters(MethodInvocationTree mit) { + for (ExpressionTree arg: mit.arguments()) { + if (arg.symbolType().is(JAVA_LANG_STRING) && arg.asConstant().isEmpty()) { + return true; + } + } + return false; + } } From 2f1c199fa2f6b553260a6f5558e7f228d14869f9 Mon Sep 17 00:00:00 2001 From: Tomasz Tylenda Date: Mon, 7 Jul 2025 18:09:05 +0200 Subject: [PATCH 2/6] QG --- .../src/main/java/org/sonar/java/checks/SQLInjectionCheck.java | 1 - 1 file changed, 1 deletion(-) diff --git a/java-checks/src/main/java/org/sonar/java/checks/SQLInjectionCheck.java b/java-checks/src/main/java/org/sonar/java/checks/SQLInjectionCheck.java index 83d7c65b2b5..1a30bb31707 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/SQLInjectionCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/SQLInjectionCheck.java @@ -24,7 +24,6 @@ import java.util.stream.Stream; import javax.annotation.Nullable; import org.sonar.check.Rule; -import org.sonar.java.checks.helpers.QuickFixHelper; import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; import org.sonar.plugins.java.api.JavaFileScannerContext; import org.sonar.plugins.java.api.semantic.MethodMatchers; From a8fb0850fa5f116d18816e18184ba81645671a2d Mon Sep 17 00:00:00 2001 From: Tomasz Tylenda Date: Tue, 8 Jul 2025 10:45:00 +0200 Subject: [PATCH 3/6] minor --- .../src/main/java/checks/SQLInjection.java | 17 +++++++++-------- .../sonar/java/checks/SQLInjectionCheck.java | 4 ++-- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/java-checks-test-sources/default/src/main/java/checks/SQLInjection.java b/java-checks-test-sources/default/src/main/java/checks/SQLInjection.java index 3b49e4eec6d..aaf2cf34c79 100644 --- a/java-checks-test-sources/default/src/main/java/checks/SQLInjection.java +++ b/java-checks-test-sources/default/src/main/java/checks/SQLInjection.java @@ -253,8 +253,9 @@ public void formatInline(String input) throws SQLException { this.stmt.execute(String.format("SELECT %s", input)); // Noncompliant } - public void formatInlineConst(String input) throws SQLException { - this.stmt.execute(String.format("SELECT %s","1")); + public void formatInlineConst() throws SQLException { + // Allow strings built with `format` if the arguments are constants. + this.stmt.execute(String.format("SELECT %s", "1")); } public void formatVar(String input) throws SQLException { @@ -262,7 +263,7 @@ public void formatVar(String input) throws SQLException { this.stmt.execute(query); // Noncompliant } - public void formatVarConst(String input) throws SQLException { + public void formatVarConst() throws SQLException { String query = String.format("SELECT %s", "1"); this.stmt.execute(query); } @@ -272,28 +273,28 @@ public void formatLocale(Locale locale, String input) throws SQLException { this.stmt.execute(query); // Noncompliant } - public void formatLocaleConst(Locale locale, String input) throws SQLException { + public void formatLocaleConst(Locale locale) throws SQLException { String query = String.format(locale, "SELECT %s", "1"); this.stmt.execute(query); } - public void formatted(Locale locale, String input) throws SQLException { + public void formatted(String input) throws SQLException { String query = "SELECT %s".formatted(input); this.stmt.execute(query); // Noncompliant } - public void formattedConst(Locale locale, String input) throws SQLException { + public void formattedConst() throws SQLException { String query = "SELECT %s".formatted("1"); this.stmt.execute(query); } - public void plusAssignment(String input) throws SQLException { + public void plusAssignment(String input) throws SQLException { String query = "SELECT"; query += String.format("WHERE col = %c", input); this.stmt.execute(query); // Noncompliant } - public void plusAssignmentConst() throws SQLException { + public void plusAssignmentConst() throws SQLException { // FP, but probably rare and not worth complicating the code to fix it. String query = "SELECT"; query += String.format("WHERE col = \"%c\"", "value"); diff --git a/java-checks/src/main/java/org/sonar/java/checks/SQLInjectionCheck.java b/java-checks/src/main/java/org/sonar/java/checks/SQLInjectionCheck.java index 1a30bb31707..cda5872595d 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/SQLInjectionCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/SQLInjectionCheck.java @@ -206,14 +206,14 @@ private static boolean isDynamicPlusAssignment(ExpressionTree arg) { } private static boolean isDynamicString(ExpressionTree arg) { - return isDynamicConcatenation(arg) || isDynamicFormatting(arg); + return isDynamicConcatenation(arg) || isDynamicFormat(arg); } private static boolean isDynamicConcatenation(ExpressionTree arg) { return arg.is(Tree.Kind.PLUS) && !arg.asConstant().isPresent(); } - private static boolean isDynamicFormatting(Tree tree) { + private static boolean isDynamicFormat(Tree tree) { return tree instanceof MethodInvocationTree mit && FORMAT_METHODS.matches(mit) && hasDynamicStringParameters(mit); From 126fc7d08c76f4a19da80f805591f7fd16c8e527 Mon Sep 17 00:00:00 2001 From: Tomasz Tylenda Date: Wed, 16 Jul 2025 17:56:43 +0200 Subject: [PATCH 4/6] First review --- .../src/main/java/checks/SQLInjection.java | 15 +++++++++++++++ .../org/sonar/java/checks/SQLInjectionCheck.java | 12 +++++++++++- 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/java-checks-test-sources/default/src/main/java/checks/SQLInjection.java b/java-checks-test-sources/default/src/main/java/checks/SQLInjection.java index aaf2cf34c79..ec44577865c 100644 --- a/java-checks-test-sources/default/src/main/java/checks/SQLInjection.java +++ b/java-checks-test-sources/default/src/main/java/checks/SQLInjection.java @@ -263,6 +263,11 @@ public void formatVar(String input) throws SQLException { this.stmt.execute(query); // Noncompliant } + public void formatVarObject(Object input) throws SQLException { + String query = String.format("SELECT %s", input); + this.stmt.execute(query); // Noncompliant + } + public void formatVarConst() throws SQLException { String query = String.format("SELECT %s", "1"); this.stmt.execute(query); @@ -278,6 +283,16 @@ public void formatLocaleConst(Locale locale) throws SQLException { this.stmt.execute(query); } + public void formatPrimitiveVar(int input) throws SQLException { + String query = String.format("SELECT %s", input); + this.stmt.execute(query); + } + + public void formatPrimitiveConst() throws SQLException { + String query = String.format("SELECT %s", 1); + this.stmt.execute(query); + } + public void formatted(String input) throws SQLException { String query = "SELECT %s".formatted(input); this.stmt.execute(query); // Noncompliant diff --git a/java-checks/src/main/java/org/sonar/java/checks/SQLInjectionCheck.java b/java-checks/src/main/java/org/sonar/java/checks/SQLInjectionCheck.java index cda5872595d..eda2fce48cc 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/SQLInjectionCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/SQLInjectionCheck.java @@ -28,6 +28,7 @@ import org.sonar.plugins.java.api.JavaFileScannerContext; import org.sonar.plugins.java.api.semantic.MethodMatchers; import org.sonar.plugins.java.api.semantic.Symbol; +import org.sonar.plugins.java.api.semantic.Type; import org.sonar.plugins.java.api.tree.AssignmentExpressionTree; import org.sonar.plugins.java.api.tree.ExpressionTree; import org.sonar.plugins.java.api.tree.IdentifierTree; @@ -219,11 +220,20 @@ private static boolean isDynamicFormat(Tree tree) { && hasDynamicStringParameters(mit); } + /** + * Checks if parameters to format/formatted are dynamic variables susceptible to SQL injection. + */ private static boolean hasDynamicStringParameters(MethodInvocationTree mit) { + boolean firstArg = true; for (ExpressionTree arg: mit.arguments()) { - if (arg.symbolType().is(JAVA_LANG_STRING) && arg.asConstant().isEmpty()) { + Type type = arg.symbolType(); + // `format` has a variant with Locale as the first argument - we do not need to check that parameter. + boolean isFirstLocaleArgument = firstArg && !type.isUnknown() && type.is("java.util.Locale"); + // Primitives will not lead to SQL injection, so the code is compliant. + if (!isFirstLocaleArgument && !type.isPrimitive() && arg.asConstant().isEmpty()) { return true; } + firstArg = false; } return false; } From 774ad5f14c25b7b06d28383ad7f2855a4601cb7c Mon Sep 17 00:00:00 2001 From: Tomasz Tylenda Date: Thu, 17 Jul 2025 18:08:58 +0200 Subject: [PATCH 5/6] Handle primitive wrappers --- .../default/src/main/java/checks/SQLInjection.java | 10 ++++++++++ .../java/org/sonar/java/checks/SQLInjectionCheck.java | 2 +- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/java-checks-test-sources/default/src/main/java/checks/SQLInjection.java b/java-checks-test-sources/default/src/main/java/checks/SQLInjection.java index ec44577865c..925401b72d3 100644 --- a/java-checks-test-sources/default/src/main/java/checks/SQLInjection.java +++ b/java-checks-test-sources/default/src/main/java/checks/SQLInjection.java @@ -293,6 +293,16 @@ public void formatPrimitiveConst() throws SQLException { this.stmt.execute(query); } + public void formatPrimitiveWrapperVar(Long input) throws SQLException { + String query = String.format("SELECT %s", input); + this.stmt.execute(query); + } + + public void formatPrimitiveWrapperConst() throws SQLException { + String query = String.format("SELECT %s", Long.valueOf(4)); + this.stmt.execute(query); + } + public void formatted(String input) throws SQLException { String query = "SELECT %s".formatted(input); this.stmt.execute(query); // Noncompliant diff --git a/java-checks/src/main/java/org/sonar/java/checks/SQLInjectionCheck.java b/java-checks/src/main/java/org/sonar/java/checks/SQLInjectionCheck.java index eda2fce48cc..6ec28cdf702 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/SQLInjectionCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/SQLInjectionCheck.java @@ -230,7 +230,7 @@ private static boolean hasDynamicStringParameters(MethodInvocationTree mit) { // `format` has a variant with Locale as the first argument - we do not need to check that parameter. boolean isFirstLocaleArgument = firstArg && !type.isUnknown() && type.is("java.util.Locale"); // Primitives will not lead to SQL injection, so the code is compliant. - if (!isFirstLocaleArgument && !type.isPrimitive() && arg.asConstant().isEmpty()) { + if (!isFirstLocaleArgument && !type.isPrimitive() && !type.isPrimitiveWrapper() && arg.asConstant().isEmpty()) { return true; } firstArg = false; From d96fe5fc69c914e8e1ff11ce6943d77076471df0 Mon Sep 17 00:00:00 2001 From: Tomasz Tylenda Date: Thu, 17 Jul 2025 18:31:13 +0200 Subject: [PATCH 6/6] Handle missing semantics --- .../checks/SQLInjectionSample.java | 22 +++++++++++++++++++ .../sonar/java/checks/SQLInjectionCheck.java | 4 ++-- .../java/checks/SQLInjectionCheckTest.java | 9 ++++++++ 3 files changed, 33 insertions(+), 2 deletions(-) create mode 100644 java-checks-test-sources/default/src/main/files/non-compiling/checks/SQLInjectionSample.java diff --git a/java-checks-test-sources/default/src/main/files/non-compiling/checks/SQLInjectionSample.java b/java-checks-test-sources/default/src/main/files/non-compiling/checks/SQLInjectionSample.java new file mode 100644 index 00000000000..2dba1f1a89b --- /dev/null +++ b/java-checks-test-sources/default/src/main/files/non-compiling/checks/SQLInjectionSample.java @@ -0,0 +1,22 @@ +package checks; + +import java.sql.SQLException; +import java.sql.Statement; + +class SQLInjectionSample { + private final Statement stmt; + + public SQLInjectionSample(Statement stmt) { + this.stmt = stmt; + } + + public void formatInline(String input) throws SQLException { + this.stmt.execute(String.format("SELECT %s", input)); // Noncompliant + } + + public void formatLocale(Locale locale, int input, Unknown unknown) throws SQLException { + // Do not generate warnings on unknown types to avoid FPs. + String query = String.format(locale, "SELECT %s %s", input, unknown); + this.stmt.execute(query); + } +} diff --git a/java-checks/src/main/java/org/sonar/java/checks/SQLInjectionCheck.java b/java-checks/src/main/java/org/sonar/java/checks/SQLInjectionCheck.java index 6ec28cdf702..3f5d876647d 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/SQLInjectionCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/SQLInjectionCheck.java @@ -228,9 +228,9 @@ private static boolean hasDynamicStringParameters(MethodInvocationTree mit) { for (ExpressionTree arg: mit.arguments()) { Type type = arg.symbolType(); // `format` has a variant with Locale as the first argument - we do not need to check that parameter. - boolean isFirstLocaleArgument = firstArg && !type.isUnknown() && type.is("java.util.Locale"); + boolean isFirstLocaleArgument = firstArg && type.is("java.util.Locale"); // Primitives will not lead to SQL injection, so the code is compliant. - if (!isFirstLocaleArgument && !type.isPrimitive() && !type.isPrimitiveWrapper() && arg.asConstant().isEmpty()) { + if (!isFirstLocaleArgument && !type.isUnknown() && !type.isPrimitive() && !type.isPrimitiveWrapper() && arg.asConstant().isEmpty()) { return true; } firstArg = false; diff --git a/java-checks/src/test/java/org/sonar/java/checks/SQLInjectionCheckTest.java b/java-checks/src/test/java/org/sonar/java/checks/SQLInjectionCheckTest.java index dd13faf3853..68406b1dd14 100644 --- a/java-checks/src/test/java/org/sonar/java/checks/SQLInjectionCheckTest.java +++ b/java-checks/src/test/java/org/sonar/java/checks/SQLInjectionCheckTest.java @@ -21,6 +21,7 @@ import static org.sonar.java.checks.verifier.TestUtils.mainCodeSourcesPath; import static org.sonar.java.checks.verifier.TestUtils.mainCodeSourcesPathInModule; +import static org.sonar.java.checks.verifier.TestUtils.nonCompilingTestSourcesPath; import static org.sonar.java.test.classpath.TestClasspathUtils.SPRING_32_MODULE; class SQLInjectionCheckTest { @@ -33,6 +34,14 @@ void test() { .verifyIssues(); } + @Test + void test_non_compiling() { + CheckVerifier.newVerifier() + .onFile(nonCompilingTestSourcesPath("checks/SQLInjectionSample.java")) + .withCheck(new SQLInjectionCheck()) + .verifyIssues(); + } + @Test void test_with_spring_3_2() { CheckVerifier.newVerifier()