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-test-sources/default/src/main/java/checks/SQLInjection.java b/java-checks-test-sources/default/src/main/java/checks/SQLInjection.java index 43be49c99b3..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 @@ -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,88 @@ 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() 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 { + String query = String.format("SELECT %s", input); + 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); + } + + 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) throws SQLException { + String query = String.format(locale, "SELECT %s", "1"); + 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 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 + } + + public void formattedConst() 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..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 @@ -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; @@ -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,35 @@ 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) || isDynamicFormat(arg); + } + private static boolean isDynamicConcatenation(ExpressionTree arg) { return arg.is(Tree.Kind.PLUS) && !arg.asConstant().isPresent(); } + + private static boolean isDynamicFormat(Tree tree) { + return tree instanceof MethodInvocationTree mit + && FORMAT_METHODS.matches(mit) + && 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()) { + 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.is("java.util.Locale"); + // Primitives will not lead to SQL injection, so the code is compliant. + if (!isFirstLocaleArgument && !type.isUnknown() && !type.isPrimitive() && !type.isPrimitiveWrapper() && arg.asConstant().isEmpty()) { + return true; + } + firstArg = false; + } + return 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()