From ceb3e74606875254463c1a9276ec2cbfb23454a5 Mon Sep 17 00:00:00 2001 From: andrecs <12188364+andrecsilva@users.noreply.github.com> Date: Wed, 31 Jul 2024 08:39:32 -0300 Subject: [PATCH] Bugfixes and new transformation composing sql fixes --- .../SonarSQLInjectionCodemodTest.java | 9 ++++ .../SQLTestMixed.java.after | 32 +++++++++++++ .../SQLTestMixed.java.before | 24 ++++++++++ .../sonar-hotspots.json | 48 +++++++++++++++++++ .../ast/LinearizedStringExpression.java | 10 +++- ...aParserSQLInjectionRemediatorStrategy.java | 2 +- ...aParserSQLInjectionRemediatorStrategy.java | 6 +-- .../sqlinjection/SQLInjectionFixComposer.java | 26 ++++++++++ .../SQLParameterizerWithCleanup.java | 26 +++++----- 9 files changed, 161 insertions(+), 22 deletions(-) create mode 100644 core-codemods/src/test/resources/sonar-sql-injection-s2077/supportedMixedInjections/SQLTestMixed.java.after create mode 100644 core-codemods/src/test/resources/sonar-sql-injection-s2077/supportedMixedInjections/SQLTestMixed.java.before create mode 100644 core-codemods/src/test/resources/sonar-sql-injection-s2077/supportedMixedInjections/sonar-hotspots.json create mode 100644 framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/SQLInjectionFixComposer.java diff --git a/core-codemods/src/test/java/io/codemodder/codemods/SonarSQLInjectionCodemodTest.java b/core-codemods/src/test/java/io/codemodder/codemods/SonarSQLInjectionCodemodTest.java index ea6ddffe1..1891f8c90 100644 --- a/core-codemods/src/test/java/io/codemodder/codemods/SonarSQLInjectionCodemodTest.java +++ b/core-codemods/src/test/java/io/codemodder/codemods/SonarSQLInjectionCodemodTest.java @@ -33,4 +33,13 @@ class SupportedTest implements CodemodTestMixin {} expectingFixesAtLines = {19, 25, 33, 40}, dependencies = {}) class SupportedTableInjectionTest implements CodemodTestMixin {} + + @Nested + @Metadata( + codemodType = SonarSQLInjectionCodemod.class, + testResourceDir = "sonar-sql-injection-s2077/supportedMixedInjections", + renameTestFile = "core-codemods/src/main/java/io/codemodder/codemods/SQLTestMixed.java", + expectingFixesAtLines = {21}, + dependencies = {}) + class SupportedMixedInjectionTest implements CodemodTestMixin {} } diff --git a/core-codemods/src/test/resources/sonar-sql-injection-s2077/supportedMixedInjections/SQLTestMixed.java.after b/core-codemods/src/test/resources/sonar-sql-injection-s2077/supportedMixedInjections/SQLTestMixed.java.after new file mode 100644 index 000000000..241dcd490 --- /dev/null +++ b/core-codemods/src/test/resources/sonar-sql-injection-s2077/supportedMixedInjections/SQLTestMixed.java.after @@ -0,0 +1,32 @@ +package io.codemodder.codemods; + +import java.sql.Connection; +import java.sql.ResultSet; +import java.sql.SQLException; +import java.sql.Statement; +import java.sql.PreparedStatement; +import java.util.Scanner; +import java.util.regex.Pattern; + +public final class SQLTestMixed { + + private Connection conn; + + public ResultSet simpleIndirect() throws SQLException { + Scanner scanner = new Scanner(System.in); + String input = scanner.nextLine(); + String sql = "SELECT * FROM " + validateTableName(input + "") + " where name=?" ; + PreparedStatement stmt = conn.prepareStatement(sql); + stmt.setString(1, scanner.nextLine()); + return stmt.execute(); + } + + String validateTableName(final String tablename) { + Pattern regex = Pattern.compile("[a-zA-Z0-9_]+(.[a-zA-Z0-9_]+)?"); + if (!regex.matcher(tablename).matches()) { + throw new SecurityException("Supplied table name contains non-alphanumeric characters"); + } + return tablename; + } + +} diff --git a/core-codemods/src/test/resources/sonar-sql-injection-s2077/supportedMixedInjections/SQLTestMixed.java.before b/core-codemods/src/test/resources/sonar-sql-injection-s2077/supportedMixedInjections/SQLTestMixed.java.before new file mode 100644 index 000000000..83521d2b0 --- /dev/null +++ b/core-codemods/src/test/resources/sonar-sql-injection-s2077/supportedMixedInjections/SQLTestMixed.java.before @@ -0,0 +1,24 @@ +package io.codemodder.codemods; + +import java.sql.Connection; +import java.sql.ResultSet; +import java.sql.SQLException; +import java.sql.Statement; +import java.sql.PreparedStatement; +import java.util.Scanner; +import java.util.regex.Pattern; + +public final class SQLTestMixed { + + private Connection conn; + + public ResultSet simpleIndirect() throws SQLException { + Scanner scanner = new Scanner(System.in); + String input = scanner.nextLine(); + String input2 = scanner.nextLine(); + String sql = "SELECT * FROM " + input + " where name='" + input2 + "'" ; + Statement stmt = conn.createStatement(); + return stmt.executeQuery(sql); + } + +} diff --git a/core-codemods/src/test/resources/sonar-sql-injection-s2077/supportedMixedInjections/sonar-hotspots.json b/core-codemods/src/test/resources/sonar-sql-injection-s2077/supportedMixedInjections/sonar-hotspots.json new file mode 100644 index 000000000..fb432b2a8 --- /dev/null +++ b/core-codemods/src/test/resources/sonar-sql-injection-s2077/supportedMixedInjections/sonar-hotspots.json @@ -0,0 +1,48 @@ +{ + "paging": { + "pageIndex": 1, + "pageSize": 100, + "total": 1 + }, + "hotspots": [ + { + "key": "AZEIpASKc7kK4RXktkeh", + "component": "pixee_codemodder-java:core-codemods/src/main/java/io/codemodder/codemods/SQLTestMixed.java", + "project": "pixee_codemodder-java", + "securityCategory": "sql-injection", + "vulnerabilityProbability": "HIGH", + "status": "TO_REVIEW", + "line": 21, + "message": "Make sure using a dynamically formatted SQL query is safe here.", + "creationDate": "2024-07-31T13:53:37+0200", + "updateDate": "2024-07-31T13:53:37+0200", + "textRange": { + "startLine": 21, + "endLine": 21, + "startOffset": 33, + "endOffset": 36 + }, + "flows": [], + "ruleKey": "java:S2077" + } + ], + "components": [ + { + "organization": "pixee", + "key": "pixee_codemodder-java", + "qualifier": "TRK", + "name": "codemodder-java", + "longName": "codemodder-java", + "pullRequest": "434" + }, + { + "organization": "pixee", + "key": "pixee_codemodder-java:core-codemods/src/main/java/io/codemodder/codemods/SQLTestMixed.java", + "qualifier": "FIL", + "name": "SQLTestMixed.java", + "longName": "core-codemods/src/main/java/io/codemodder/codemods/SQLTestMixed.java", + "path": "core-codemods/src/main/java/io/codemodder/codemods/SQLTestMixed.java", + "pullRequest": "434" + } + ] +} diff --git a/framework/codemodder-base/src/main/java/io/codemodder/ast/LinearizedStringExpression.java b/framework/codemodder-base/src/main/java/io/codemodder/ast/LinearizedStringExpression.java index f289569f8..82d6a0d2e 100644 --- a/framework/codemodder-base/src/main/java/io/codemodder/ast/LinearizedStringExpression.java +++ b/framework/codemodder-base/src/main/java/io/codemodder/ast/LinearizedStringExpression.java @@ -6,7 +6,13 @@ import com.github.javaparser.ast.expr.Expression; import com.github.javaparser.ast.expr.NameExpr; import com.github.javaparser.resolution.types.ResolvedType; -import java.util.*; +import java.util.ArrayDeque; +import java.util.Deque; +import java.util.IdentityHashMap; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Optional; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -20,7 +26,7 @@ public final class LinearizedStringExpression { private final Deque linearized; public LinearizedStringExpression(final Expression expression) { - this.resolvedExpressions = new HashMap<>(); + this.resolvedExpressions = new IdentityHashMap<>(); this.root = expression; this.linearized = linearize(expression).collect(Collectors.toCollection(ArrayDeque::new)); } diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/DefaultJavaParserSQLInjectionRemediatorStrategy.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/DefaultJavaParserSQLInjectionRemediatorStrategy.java index 758d29e3f..eeec9542a 100644 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/DefaultJavaParserSQLInjectionRemediatorStrategy.java +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/DefaultJavaParserSQLInjectionRemediatorStrategy.java @@ -25,7 +25,7 @@ final class DefaultJavaParserSQLInjectionRemediatorStrategy implements JavaParserSQLInjectionRemediatorStrategy { - private Map, Predicate> remediationStrategies; + private final Map, Predicate> remediationStrategies; /** * Builds a strategy from a matcher-fixer pair. A matcher is a predicate that matches the call, diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/JavaParserSQLInjectionRemediatorStrategy.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/JavaParserSQLInjectionRemediatorStrategy.java index ebaf20d31..c0b7e45c8 100644 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/JavaParserSQLInjectionRemediatorStrategy.java +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/JavaParserSQLInjectionRemediatorStrategy.java @@ -35,9 +35,5 @@ CodemodFileScanningResult remediateAll( /** A default implementation that should be used in all non-test scenarios. */ JavaParserSQLInjectionRemediatorStrategy DEFAULT = new DefaultJavaParserSQLInjectionRemediatorStrategy( - Map.of( - SQLParameterizer::isSupportedJdbcMethodCall, - SQLParameterizerWithCleanup::checkAndFix, - SQLTableInjectionFilterTransform::matchCall, - SQLTableInjectionFilterTransform::fix)); + Map.of(SQLInjectionFixComposer::match, SQLInjectionFixComposer::checkAndFix)); } diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/SQLInjectionFixComposer.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/SQLInjectionFixComposer.java new file mode 100644 index 000000000..b97a642ba --- /dev/null +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/SQLInjectionFixComposer.java @@ -0,0 +1,26 @@ +package io.codemodder.remediation.sqlinjection; + +import com.github.javaparser.ast.expr.MethodCallExpr; + +public final class SQLInjectionFixComposer { + + private SQLInjectionFixComposer() {} + + public static boolean checkAndFix(final MethodCallExpr methodCallExpr) { + // Check if any data injection fixes apply + var maybeFixed = new SQLParameterizer(methodCallExpr).checkAndFix(); + if (maybeFixed.isPresent()) { + SQLParameterizerWithCleanup.cleanup(maybeFixed.get()); + SQLTableInjectionFilterTransform.findAndFix(maybeFixed.get()); + return true; + // If not, try the table injection only + } else { + return SQLTableInjectionFilterTransform.findAndFix(methodCallExpr); + } + } + + public static boolean match(final MethodCallExpr methodCallExpr) { + return SQLParameterizer.isSupportedJdbcMethodCall(methodCallExpr) + || SQLTableInjectionFilterTransform.matchCall(methodCallExpr); + } +} diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/SQLParameterizerWithCleanup.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/SQLParameterizerWithCleanup.java index 2024eb07d..a359c9e35 100644 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/SQLParameterizerWithCleanup.java +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/SQLParameterizerWithCleanup.java @@ -10,21 +10,19 @@ private SQLParameterizerWithCleanup() {} public static boolean checkAndFix(final MethodCallExpr methodCallExpr) { var maybeFixed = new SQLParameterizer(methodCallExpr).checkAndFix(); - if (maybeFixed.isPresent()) { - // Cleanup - var maybeMethodDecl = methodCallExpr.findAncestor(CallableDeclaration.class); + maybeFixed.ifPresent(call -> cleanup(call)); + return maybeFixed.isPresent(); + } + + public static void cleanup(final MethodCallExpr pstmtCall) { + var maybeMethodDecl = pstmtCall.findAncestor(CallableDeclaration.class); - // Remove concatenation with empty strings e.g "first" + "" -> "first"; - maybeMethodDecl.ifPresent(ASTTransforms::removeEmptyStringConcatenation); - // Remove potential unused variables left after transform - maybeMethodDecl.ifPresent(md -> ASTTransforms.removeUnusedLocalVariables(md)); + // Remove concatenation with empty strings e.g "first" + "" -> "first"; + maybeMethodDecl.ifPresent(ASTTransforms::removeEmptyStringConcatenation); + // Remove potential unused variables left after transform + maybeMethodDecl.ifPresent(md -> ASTTransforms.removeUnusedLocalVariables(md)); - // Merge concatenated literals, e.g. "first" + " and second" -> "first and second" - maybeFixed - .flatMap(mce -> mce.getArguments().getFirst()) - .ifPresent(ASTTransforms::mergeConcatenatedLiterals); - return true; - } - return false; + // Merge concatenated literals, e.g. "first" + " and second" -> "first and second" + pstmtCall.getArguments().getFirst().ifPresent(ASTTransforms::mergeConcatenatedLiterals); } }