From 364702d8b8aecc2a29c1d51bc1da15ad904d2ec0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20C=2E=20Silva?= <12188364+andrecsilva@users.noreply.github.com> Date: Tue, 15 Oct 2024 10:01:31 -0300 Subject: [PATCH] Refactors SQL Remediator and Codemods & HQL Transformation Bugfix (#456) Refactors SQL injection codemods to use the new remediator API and fixes a HQL transformation bug. --- .../DefectDojoSqlInjectionCodemod.java | 11 +- .../codemods/HQLParameterizationCodemod.java | 5 +- .../codemods/SonarSQLInjectionCodemod.java | 12 +- .../semgrep/SemgrepSQLInjectionCodemod.java | 16 +- ...SQLInjectionFormattedSqlStringCodemod.java | 16 +- .../hql-parameterizer/Test.java.after | 8 +- ...aParserSQLInjectionRemediatorStrategy.java | 162 ------------------ ...aParserSQLInjectionRemediatorStrategy.java | 40 ----- .../sqlinjection/SQLInjectionFixComposer.java | 107 +++++++++--- .../sqlinjection/SQLInjectionRemediator.java | 50 ++++++ .../sqlinjection/SQLParameterizer.java | 5 + .../SQLTableInjectionFilterTransform.java | 19 +- 12 files changed, 199 insertions(+), 252 deletions(-) delete mode 100644 framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/DefaultJavaParserSQLInjectionRemediatorStrategy.java delete mode 100644 framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/JavaParserSQLInjectionRemediatorStrategy.java create mode 100644 framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/SQLInjectionRemediator.java diff --git a/core-codemods/src/main/java/io/codemodder/codemods/DefectDojoSqlInjectionCodemod.java b/core-codemods/src/main/java/io/codemodder/codemods/DefectDojoSqlInjectionCodemod.java index ed11b6da9..81f49df00 100644 --- a/core-codemods/src/main/java/io/codemodder/codemods/DefectDojoSqlInjectionCodemod.java +++ b/core-codemods/src/main/java/io/codemodder/codemods/DefectDojoSqlInjectionCodemod.java @@ -7,9 +7,11 @@ import io.codemodder.providers.defectdojo.DefectDojoScan; import io.codemodder.providers.defectdojo.Finding; import io.codemodder.providers.defectdojo.RuleFindings; -import io.codemodder.remediation.sqlinjection.JavaParserSQLInjectionRemediatorStrategy; +import io.codemodder.remediation.Remediator; +import io.codemodder.remediation.sqlinjection.SQLInjectionRemediator; import java.util.List; import java.util.Objects; +import java.util.Optional; import javax.inject.Inject; /** @@ -25,7 +27,7 @@ public final class DefectDojoSqlInjectionCodemod extends JavaParserChanger implements FixOnlyCodeChanger { private final RuleFindings findings; - private final JavaParserSQLInjectionRemediatorStrategy remediatorStrategy; + private final Remediator remediatorStrategy; @Inject public DefectDojoSqlInjectionCodemod( @@ -33,7 +35,7 @@ public DefectDojoSqlInjectionCodemod( RuleFindings findings) { super(CodemodReporterStrategy.fromClasspath(SQLParameterizerCodemod.class)); this.findings = Objects.requireNonNull(findings); - this.remediatorStrategy = JavaParserSQLInjectionRemediatorStrategy.DEFAULT; + this.remediatorStrategy = new SQLInjectionRemediator<>(); } @Override @@ -60,6 +62,7 @@ public CodemodFileScanningResult visit( findingsForThisPath, finding -> String.valueOf(finding.getId()), Finding::getLine, - f -> null); + f -> Optional.empty(), + f -> Optional.empty()); } } diff --git a/core-codemods/src/main/java/io/codemodder/codemods/HQLParameterizationCodemod.java b/core-codemods/src/main/java/io/codemodder/codemods/HQLParameterizationCodemod.java index 6d09c0937..241aa5aba 100644 --- a/core-codemods/src/main/java/io/codemodder/codemods/HQLParameterizationCodemod.java +++ b/core-codemods/src/main/java/io/codemodder/codemods/HQLParameterizationCodemod.java @@ -56,7 +56,7 @@ public CodemodFileScanningResult visit( return CodemodFileScanningResult.withOnlyChanges(changes); } - private static final String queryParameterNamePrefix = ":parameter"; + private static final String queryParameterNamePrefix = "parameter"; private boolean isQueryCreation(final MethodCallExpr methodCallExpr) { final Predicate isQueryCall = @@ -86,7 +86,8 @@ private List fixInjections( final var builder = new StringBuilder(startString); final int lastQuoteIndex = startString.lastIndexOf('\'') + 1; final var prepend = startString.substring(lastQuoteIndex); - builder.replace(lastQuoteIndex - 1, startString.length(), queryParameterNamePrefix + count); + builder.replace( + lastQuoteIndex - 1, startString.length(), ":" + queryParameterNamePrefix + count); start.asStringLiteralExpr().setValue(builder.toString()); // fix end diff --git a/core-codemods/src/main/java/io/codemodder/codemods/SonarSQLInjectionCodemod.java b/core-codemods/src/main/java/io/codemodder/codemods/SonarSQLInjectionCodemod.java index 3ae760eab..c5027a89c 100644 --- a/core-codemods/src/main/java/io/codemodder/codemods/SonarSQLInjectionCodemod.java +++ b/core-codemods/src/main/java/io/codemodder/codemods/SonarSQLInjectionCodemod.java @@ -7,11 +7,14 @@ import io.codemodder.providers.sonar.RuleHotspot; import io.codemodder.providers.sonar.SonarRemediatingJavaParserChanger; import io.codemodder.remediation.GenericRemediationMetadata; -import io.codemodder.remediation.sqlinjection.JavaParserSQLInjectionRemediatorStrategy; +import io.codemodder.remediation.Remediator; +import io.codemodder.remediation.sqlinjection.SQLInjectionRemediator; import io.codemodder.sonar.model.Hotspot; import io.codemodder.sonar.model.SonarFinding; +import io.codemodder.sonar.model.TextRange; import java.util.List; import java.util.Objects; +import java.util.Optional; import javax.inject.Inject; @Codemod( @@ -21,7 +24,7 @@ executionPriority = CodemodExecutionPriority.HIGH) public final class SonarSQLInjectionCodemod extends SonarRemediatingJavaParserChanger { - private final JavaParserSQLInjectionRemediatorStrategy remediationStrategy; + private final Remediator remediationStrategy; private final RuleHotspot hotspots; @Inject @@ -29,7 +32,7 @@ public SonarSQLInjectionCodemod( @ProvidedSonarScan(ruleId = "java:S2077") final RuleHotspot hotspots) { super(GenericRemediationMetadata.SQL_INJECTION.reporter(), hotspots); this.hotspots = Objects.requireNonNull(hotspots); - this.remediationStrategy = JavaParserSQLInjectionRemediatorStrategy.DEFAULT; + this.remediationStrategy = new SQLInjectionRemediator<>(); } @Override @@ -51,6 +54,7 @@ public CodemodFileScanningResult visit( hotspotsForFile, SonarFinding::getKey, i -> i.getTextRange() != null ? i.getTextRange().getStartLine() : i.getLine(), - i -> i.getTextRange() != null ? i.getTextRange().getEndLine() : null); + i -> Optional.ofNullable(i.getTextRange()).map(TextRange::getEndLine), + i -> Optional.ofNullable(i.getTextRange()).map(tr -> tr.getStartOffset() + 1)); } } diff --git a/core-codemods/src/main/java/io/codemodder/codemods/semgrep/SemgrepSQLInjectionCodemod.java b/core-codemods/src/main/java/io/codemodder/codemods/semgrep/SemgrepSQLInjectionCodemod.java index 66c769143..a2a3ae50b 100644 --- a/core-codemods/src/main/java/io/codemodder/codemods/semgrep/SemgrepSQLInjectionCodemod.java +++ b/core-codemods/src/main/java/io/codemodder/codemods/semgrep/SemgrepSQLInjectionCodemod.java @@ -1,5 +1,6 @@ package io.codemodder.codemods.semgrep; +import com.contrastsecurity.sarif.Result; import com.github.javaparser.ast.CompilationUnit; import io.codemodder.Codemod; import io.codemodder.CodemodExecutionPriority; @@ -12,7 +13,9 @@ import io.codemodder.codetf.DetectorRule; import io.codemodder.providers.sarif.semgrep.ProvidedSemgrepScan; import io.codemodder.remediation.GenericRemediationMetadata; -import io.codemodder.remediation.sqlinjection.JavaParserSQLInjectionRemediatorStrategy; +import io.codemodder.remediation.Remediator; +import io.codemodder.remediation.sqlinjection.SQLInjectionRemediator; +import java.util.Optional; import javax.inject.Inject; /** @@ -26,14 +29,14 @@ importance = Importance.HIGH) public final class SemgrepSQLInjectionCodemod extends SemgrepJavaParserChanger { - private final JavaParserSQLInjectionRemediatorStrategy remediator; + private final Remediator remediator; @Inject public SemgrepSQLInjectionCodemod( @ProvidedSemgrepScan(ruleId = "java.lang.security.audit.sqli.jdbc-sqli.jdbc-sqli") final RuleSarif sarif) { super(GenericRemediationMetadata.SQL_INJECTION.reporter(), sarif); - this.remediator = JavaParserSQLInjectionRemediatorStrategy.DEFAULT; + this.remediator = new SQLInjectionRemediator<>(); } @Override @@ -54,6 +57,11 @@ public CodemodFileScanningResult visit( ruleSarif.getResultsByLocationPath(context.path()), SarifFindingKeyUtil::buildFindingId, r -> r.getLocations().get(0).getPhysicalLocation().getRegion().getStartLine(), - r -> r.getLocations().get(0).getPhysicalLocation().getRegion().getEndLine()); + r -> + Optional.ofNullable( + r.getLocations().get(0).getPhysicalLocation().getRegion().getEndLine()), + r -> + Optional.ofNullable( + r.getLocations().get(0).getPhysicalLocation().getRegion().getStartColumn())); } } diff --git a/core-codemods/src/main/java/io/codemodder/codemods/semgrep/SemgrepSQLInjectionFormattedSqlStringCodemod.java b/core-codemods/src/main/java/io/codemodder/codemods/semgrep/SemgrepSQLInjectionFormattedSqlStringCodemod.java index d9b4d7f76..7fca9280b 100644 --- a/core-codemods/src/main/java/io/codemodder/codemods/semgrep/SemgrepSQLInjectionFormattedSqlStringCodemod.java +++ b/core-codemods/src/main/java/io/codemodder/codemods/semgrep/SemgrepSQLInjectionFormattedSqlStringCodemod.java @@ -1,5 +1,6 @@ package io.codemodder.codemods.semgrep; +import com.contrastsecurity.sarif.Result; import com.github.javaparser.ast.CompilationUnit; import io.codemodder.Codemod; import io.codemodder.CodemodExecutionPriority; @@ -12,7 +13,9 @@ import io.codemodder.codetf.DetectorRule; import io.codemodder.providers.sarif.semgrep.ProvidedSemgrepScan; import io.codemodder.remediation.GenericRemediationMetadata; -import io.codemodder.remediation.sqlinjection.JavaParserSQLInjectionRemediatorStrategy; +import io.codemodder.remediation.Remediator; +import io.codemodder.remediation.sqlinjection.SQLInjectionRemediator; +import java.util.Optional; import javax.inject.Inject; /** @@ -26,7 +29,7 @@ importance = Importance.HIGH) public final class SemgrepSQLInjectionFormattedSqlStringCodemod extends SemgrepJavaParserChanger { - private final JavaParserSQLInjectionRemediatorStrategy remediator; + private final Remediator remediator; @Inject public SemgrepSQLInjectionFormattedSqlStringCodemod( @@ -34,7 +37,7 @@ public SemgrepSQLInjectionFormattedSqlStringCodemod( ruleId = "java.lang.security.audit.formatted-sql-string.formatted-sql-string") final RuleSarif sarif) { super(GenericRemediationMetadata.SQL_INJECTION.reporter(), sarif); - this.remediator = JavaParserSQLInjectionRemediatorStrategy.DEFAULT; + this.remediator = new SQLInjectionRemediator<>(); } @Override @@ -55,6 +58,11 @@ public CodemodFileScanningResult visit( ruleSarif.getResultsByLocationPath(context.path()), SarifFindingKeyUtil::buildFindingId, r -> r.getLocations().get(0).getPhysicalLocation().getRegion().getStartLine(), - r -> r.getLocations().get(0).getPhysicalLocation().getRegion().getEndLine()); + r -> + Optional.ofNullable( + r.getLocations().get(0).getPhysicalLocation().getRegion().getEndLine()), + r -> + Optional.ofNullable( + r.getLocations().get(0).getPhysicalLocation().getRegion().getStartColumn())); } } diff --git a/core-codemods/src/test/resources/hql-parameterizer/Test.java.after b/core-codemods/src/test/resources/hql-parameterizer/Test.java.after index 57c234c5b..41619fa59 100644 --- a/core-codemods/src/test/resources/hql-parameterizer/Test.java.after +++ b/core-codemods/src/test/resources/hql-parameterizer/Test.java.after @@ -8,22 +8,22 @@ public final class Test { private Session session; void direct(String tainted) { - Query hqlQuery = session.createQuery("select p from Person p where p.name like :parameter0").setParameter(":parameter0", tainted); + Query hqlQuery = session.createQuery("select p from Person p where p.name like :parameter0").setParameter("parameter0", tainted); } void indirect(String tainted) { String query = "select p from Person p where p.name like :parameter0"; - Query hqlQuery = session.createQuery(query).setParameter(":parameter0", tainted); + Query hqlQuery = session.createQuery(query).setParameter("parameter0", tainted); } void indirectMultiple(String tainted, String tainted2) { String query = "select p from Person p where p.name like :parameter0" + " and p.surname like :parameter1"; - Query hqlQuery = session.createQuery(query).setParameter(":parameter0", tainted).setParameter(":parameter1", tainted2); + Query hqlQuery = session.createQuery(query).setParameter("parameter0", tainted).setParameter("parameter1", tainted2); } void indirectMultipeString(String tainted, String tainted2) { String second = " and p.surname like :parameter1"; String first = "select p from Person p where p.name like :parameter0" + second; - Query hqlQuery = session.createQuery(first).setParameter(":parameter0", tainted).setParameter(":parameter1", tainted2); + Query hqlQuery = session.createQuery(first).setParameter("parameter0", tainted).setParameter("parameter1", tainted2); } } 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 deleted file mode 100644 index c02e6c32b..000000000 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/DefaultJavaParserSQLInjectionRemediatorStrategy.java +++ /dev/null @@ -1,162 +0,0 @@ -package io.codemodder.remediation.sqlinjection; - -import com.github.javaparser.ast.CompilationUnit; -import io.codemodder.CodemodChange; -import io.codemodder.CodemodFileScanningResult; -import io.codemodder.codetf.DetectorRule; -import io.codemodder.codetf.FixedFinding; -import io.codemodder.codetf.UnfixedFinding; -import io.codemodder.remediation.LegacyFixCandidate; -import io.codemodder.remediation.LegacyFixCandidateSearchResults; -import io.codemodder.remediation.LegacyFixCandidateSearcher; -import io.codemodder.remediation.MethodOrConstructor; -import java.util.ArrayList; -import java.util.Collection; -import java.util.List; -import java.util.Map; -import java.util.function.Function; -import java.util.function.Predicate; -import org.javatuples.Pair; - -/** - * Default implementation of the JavaParserSQLInjectionRemediatorStrategy interface. This class - * provides the logic to visit a CompilationUnit and process findings for potential SQL injections. - */ -final class DefaultJavaParserSQLInjectionRemediatorStrategy - implements JavaParserSQLInjectionRemediatorStrategy { - - private final Map, Predicate> - remediationStrategies; - - /** - * Builds a strategy from a matcher-fixer pair. A matcher is a predicate that matches the call, - * ensure it is the right one for attempting a fix. A fixer fixes a predicate that chagnes the AST - * as a side-effect and reports if successful with a boolean. - */ - DefaultJavaParserSQLInjectionRemediatorStrategy( - final Predicate matcher, final Predicate fixer) { - this.remediationStrategies = Map.of(matcher, fixer); - } - - /** Builds a grand strategy as a combination of several strategies. */ - DefaultJavaParserSQLInjectionRemediatorStrategy( - final Map, Predicate> strategies) { - this.remediationStrategies = strategies; - } - - /** Remediate with a chosen strategy. */ - private Pair, List> remediateWithStrategy( - final CompilationUnit cu, - final String path, - final DetectorRule detectorRule, - final Collection findingsForPath, - final Function findingIdExtractor, - final Function findingStartLineExtractor, - final Function findingEndLineExtractor, - final Predicate matcher, - final Predicate fixer) { - - LegacyFixCandidateSearcher searcher = - new LegacyFixCandidateSearcher.Builder().withMatcher(matcher).build(); - - LegacyFixCandidateSearchResults results = - searcher.search( - cu, - path, - detectorRule, - new ArrayList<>(findingsForPath), - findingIdExtractor, - findingStartLineExtractor, - findingEndLineExtractor, - f -> null); - - if (findingsForPath.isEmpty()) { - return Pair.with(List.of(), List.of()); - } - - final List unfixedFindings = new ArrayList<>(); - final List changes = new ArrayList<>(); - - for (LegacyFixCandidate legacyFixCandidate : results.fixCandidates()) { - List issues = legacyFixCandidate.issues(); - Integer line = findingStartLineExtractor.apply(issues.get(0)); - - if (line == null) { - issues.forEach( - issue -> { - final String id = findingIdExtractor.apply(issue); - final UnfixedFinding unfixableFinding = - new UnfixedFinding(id, detectorRule, path, null, "No line number provided"); - unfixedFindings.add(unfixableFinding); - }); - continue; - } - - if (fixer.test(legacyFixCandidate.call())) { - issues.forEach( - issue -> { - final String id = findingIdExtractor.apply(issue); - changes.add(CodemodChange.from(line, new FixedFinding(id, detectorRule))); - }); - } else { - issues.forEach( - issue -> { - final String id = findingIdExtractor.apply(issue); - final UnfixedFinding unfixableFinding = - new UnfixedFinding( - id, - detectorRule, - path, - line, - "State changing effects possible or unrecognized code shape"); - unfixedFindings.add(unfixableFinding); - }); - } - } - return Pair.with(changes, unfixedFindings); - } - - /** - * Visits the provided CompilationUnit and processes findings for potential SQL injections. - * - * @param cu the compilation unit to be scanned - * @param path the path of the file being scanned - * @param detectorRule the detector rule that generated the findings - * @param findingsForPath a collection of findings to be processed - * @param findingIdExtractor a function to extract the ID from a finding - * @param findingStartLineExtractor a function to extract the line number from a finding - * @param the type of the findings - * @return a result object containing the changes and unfixed findings - */ - @Override - public CodemodFileScanningResult remediateAll( - final CompilationUnit cu, - final String path, - final DetectorRule detectorRule, - final Collection findingsForPath, - final Function findingIdExtractor, - final Function findingStartLineExtractor, - final Function findingEndLineExtractor) { - - List allChanges = new ArrayList<>(); - List allUnfixed = new ArrayList<>(); - - for (var matcher : remediationStrategies.keySet()) { - var fixer = remediationStrategies.get(matcher); - var pairResult = - remediateWithStrategy( - cu, - path, - detectorRule, - findingsForPath, - findingIdExtractor, - findingStartLineExtractor, - findingEndLineExtractor, - matcher, - fixer); - allChanges.addAll(pairResult.getValue0()); - allUnfixed.addAll(pairResult.getValue1()); - } - return CodemodFileScanningResult.from(allChanges, allUnfixed); - } -} 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 deleted file mode 100644 index 4ff64b6c6..000000000 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/JavaParserSQLInjectionRemediatorStrategy.java +++ /dev/null @@ -1,40 +0,0 @@ -package io.codemodder.remediation.sqlinjection; - -import com.github.javaparser.ast.CompilationUnit; -import io.codemodder.CodemodFileScanningResult; -import io.codemodder.codetf.DetectorRule; -import java.util.Collection; -import java.util.Map; -import java.util.function.Function; - -/** - * Strategy interface for remediating SQL injection vulnerabilities using JavaParser. - * Implementations of this interface define the method to visit a CompilationUnit and process - * findings for potential SQL injections. - */ -public interface JavaParserSQLInjectionRemediatorStrategy { - - /** - * Visits the provided CompilationUnit and processes findings for potential SQL injections. - * - * @param cu the compilation unit to be scanned - * @param pathFindings a collection of findings to be processed - * @param findingIdExtractor a function to extract the ID from a finding - * @param findingStartLineExtractor a function to extract the line number from a finding - * @param the type of the findings - * @return a result object containing the changes and unfixed findings - */ - CodemodFileScanningResult remediateAll( - final CompilationUnit cu, - final String path, - final DetectorRule rule, - final Collection pathFindings, - final Function findingIdExtractor, - final Function findingStartLineExtractor, - final Function findingEndLineExtractor); - - /** A default implementation that should be used in all non-test scenarios. */ - JavaParserSQLInjectionRemediatorStrategy DEFAULT = - new DefaultJavaParserSQLInjectionRemediatorStrategy( - 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 index 2a7ff650f..1fb45757b 100644 --- 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 @@ -1,49 +1,112 @@ package io.codemodder.remediation.sqlinjection; +import com.github.javaparser.ast.CompilationUnit; +import com.github.javaparser.ast.Node; +import com.github.javaparser.ast.expr.BinaryExpr; +import com.github.javaparser.ast.expr.Expression; import com.github.javaparser.ast.expr.MethodCallExpr; -import io.codemodder.remediation.MethodOrConstructor; +import io.codemodder.Either; +import io.codemodder.ast.ASTs; +import io.codemodder.ast.LocalVariableDeclaration; +import io.codemodder.remediation.RemediationStrategy; +import io.codemodder.remediation.SuccessOrReason; +import java.util.Optional; /** Composes several transformations related to SQL injections. */ -public final class SQLInjectionFixComposer { +public final class SQLInjectionFixComposer implements RemediationStrategy { - private SQLInjectionFixComposer() {} + public SQLInjectionFixComposer() {} /** - * Given a {@link MethodCallExpr} related to executing JDBC API SQL queries (i.e. - * prepareStatement(), executeQuery(), etc.), parameterize data injections or add a validation - * step for structural injections. + * Checks if the given binaryExpr ends up as an argument for a SQL execute method that can be + * fixed. + * + * @param binaryExpr + * @return An Optional containing the execute call if successful */ - public static boolean checkAndFix(final MethodOrConstructor m) { + private Optional flowsIntoExecuteCall(final BinaryExpr binaryExpr) { + // Is argument of a method call + var maybeDirectArgumentOfCall = + ASTs.isArgumentOfMethodCall(binaryExpr).filter(SQLInjectionFixComposer::match); - if (!m.isMethodCall()) { - return false; + // or it is initialization of a variable that flows into an execute call + return maybeDirectArgumentOfCall.or(() -> isIndirectArgumentOfCall(binaryExpr)); + } + + /** + * Test if the expr is initialized into a variable that flows into a call + * + * @param expr + * @return + */ + private Optional isIndirectArgumentOfCall(final Expression expr) { + return ASTs.isInitExpr(expr) + .flatMap(LocalVariableDeclaration::fromVariableDeclarator) + .flatMap( + lvd -> + lvd.findAllReferences() + .flatMap(ne -> ASTs.isArgumentOfMethodCall(ne).stream()) + .filter(SQLInjectionFixComposer::match) + .findFirst()); + } + + /** + * Given a node, checks if it is a {@link MethodCallExpr} related to executing JDBC API SQL + * queries (i.e. prepareStatement(), executeQuery(), etc.), or a {@link BinaryExpr} that flows + * into one, parameterize data injections or add a validation step for structural injections. + */ + public SuccessOrReason fix(final CompilationUnit cu, final Node node) { + + // Is a binary expr or method call expr? + Optional> morb; + if (node instanceof MethodCallExpr m) { + morb = Optional.of(Either.left(m)); + } else if (node instanceof BinaryExpr b) { + morb = Optional.of(Either.right(b)); + } else { + morb = Optional.empty(); + } + if (morb.isEmpty()) { + return SuccessOrReason.reason("Neither a binary expression or method call"); + } + // If binary expr, try to find if it flows into a function as argument + // map the left into optional for type consistency + Optional maybeCall = + morb.flatMap(e -> e.ifLeftOrElseGet(Optional::of, this::flowsIntoExecuteCall)); + if (maybeCall.isEmpty()) { + return SuccessOrReason.reason( + "Could not find any execute call that the binary expr flows into"); } - MethodCallExpr methodCallExpr = m.asMethodCall(); + MethodCallExpr methodCallExpr = maybeCall.get(); // First, check if any data injection fixes apply - var maybeFixed = new SQLParameterizer(methodCallExpr).checkAndFix(); + var maybeFixed = new SQLParameterizer(methodCallExpr, cu).checkAndFix(); if (maybeFixed.isPresent()) { // If yes, execute cleanup steps and check if any table injection remains. SQLParameterizerWithCleanup.cleanup(maybeFixed.get()); SQLTableInjectionFilterTransform.findAndFix(maybeFixed.get()); - return true; + return SuccessOrReason.success(); // If not, try the table injection only } else { - return SQLTableInjectionFilterTransform.findAndFix(methodCallExpr); + return SQLTableInjectionFilterTransform.findAndFix(methodCallExpr) + ? SuccessOrReason.success() + : SuccessOrReason.reason("Could not fix injection"); } } /** - * Check if the {@link MethodCallExpr} is a JDBC API query method that is a target of a SQL - * injection transformation. + * Check if the node is a JDBC API query method that is a target of a SQL injection + * transformation. */ - public static boolean match(final MethodOrConstructor methodOrConstructor) { - if (!methodOrConstructor.isMethodCall()) { - return false; - } - MethodCallExpr methodCallExpr = methodOrConstructor.asMethodCall(); - return SQLParameterizer.isSupportedJdbcMethodCall(methodCallExpr) - || SQLTableInjectionFilterTransform.matchCall(methodCallExpr); + public static boolean match(final Node node) { + var maybeMethodCall = + Optional.of(node) + .map(n -> n instanceof MethodCallExpr ? (MethodCallExpr) n : null) + .filter( + n -> + SQLParameterizer.isSupportedJdbcMethodCall(n) + || SQLTableInjectionFilterTransform.matchCall(n)); + return maybeMethodCall.isPresent(); } } diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/SQLInjectionRemediator.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/SQLInjectionRemediator.java new file mode 100644 index 000000000..88cfecb1e --- /dev/null +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/SQLInjectionRemediator.java @@ -0,0 +1,50 @@ +package io.codemodder.remediation.sqlinjection; + +import com.github.javaparser.ast.CompilationUnit; +import io.codemodder.CodemodFileScanningResult; +import io.codemodder.codetf.DetectorRule; +import io.codemodder.remediation.*; +import java.util.*; +import java.util.function.Function; + +/** + * A Remediator for SQL injection issues. This class provides the logic to visit a CompilationUnit + * and process findings for potential SQL injections. + */ +public final class SQLInjectionRemediator implements Remediator { + + private final SearcherStrategyRemediator searchStrategyRemediator; + + public SQLInjectionRemediator() { + this.searchStrategyRemediator = + new SearcherStrategyRemediator.Builder() + .withSearcherStrategyPair( + new FixCandidateSearcher.Builder() + .withMatcher( + n -> Optional.of(n).filter(SQLInjectionFixComposer::match).isPresent()) + .build(), + new SQLInjectionFixComposer()) + .build(); + } + + @Override + public CodemodFileScanningResult remediateAll( + CompilationUnit cu, + String path, + DetectorRule detectorRule, + Collection findingsForPath, + Function findingIdExtractor, + Function findingStartLineExtractor, + Function> findingEndLineExtractor, + Function> findingColumnExtractor) { + return searchStrategyRemediator.remediateAll( + cu, + path, + detectorRule, + findingsForPath, + findingIdExtractor, + findingStartLineExtractor, + findingEndLineExtractor, + findingColumnExtractor); + } +} diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/SQLParameterizer.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/SQLParameterizer.java index 78f60af10..916b25e86 100644 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/SQLParameterizer.java +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/SQLParameterizer.java @@ -37,6 +37,11 @@ public SQLParameterizer(final MethodCallExpr methodCallExpr) { this.compilationUnit = null; } + public SQLParameterizer(final MethodCallExpr methodCallExpr, final CompilationUnit cu) { + this.executeCall = Objects.requireNonNull(methodCallExpr); + this.compilationUnit = cu; + } + /** * Checks if the {@link MethodCallExpr} is of one of the execute calls of {@link * java.sql.Statement} whose argument is not a {@link String} literal. diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/SQLTableInjectionFilterTransform.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/SQLTableInjectionFilterTransform.java index ee863aa64..aef43fbbb 100644 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/SQLTableInjectionFilterTransform.java +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/SQLTableInjectionFilterTransform.java @@ -1,6 +1,7 @@ package io.codemodder.remediation.sqlinjection; import com.github.javaparser.StaticJavaParser; +import com.github.javaparser.ast.CompilationUnit; import com.github.javaparser.ast.body.ClassOrInterfaceDeclaration; import com.github.javaparser.ast.body.MethodDeclaration; import com.github.javaparser.ast.expr.BinaryExpr; @@ -62,6 +63,10 @@ public static boolean matchCall(final MethodCallExpr call) { } public static boolean fix(final MethodCallExpr call) { + return call.findCompilationUnit().map(cu -> fix(cu, call)).orElse(false); + } + + public static boolean fix(final CompilationUnit cu, final MethodCallExpr call) { final var linearized = new LinearizedStringExpression(call.getArgument(0)); var injections = findTableInjections(linearized); injections = @@ -72,7 +77,7 @@ public static boolean fix(final MethodCallExpr call) { && e.asMethodCallExpr().getNameAsString().equals("validateTableName"))) .toList(); if (!injections.isEmpty()) { - fix(injections, linearized.getResolvedExpressionsMap()); + fix(cu, injections, linearized.getResolvedExpressionsMap()); return true; } return false; @@ -113,7 +118,8 @@ private static List findTableInjections(final LinearizedStringExpres return tableInjections; } - private static void addFilterMethodIfMissing(final ClassOrInterfaceDeclaration classDecl) { + private static void addFilterMethodIfMissing( + final CompilationUnit cu, final ClassOrInterfaceDeclaration classDecl) { final String method = """ String validateTableName(final String tablename){ @@ -135,17 +141,18 @@ String validateTableName(final String tablename){ classDecl.addMember(StaticJavaParser.parseMethodDeclaration(method)); } // Add Pattern import - ASTTransforms.addImportIfMissing( - classDecl.findCompilationUnit().get(), "java.util.regex.Pattern"); + ASTTransforms.addImportIfMissing(cu, "java.util.regex.Pattern"); } private static void fix( - final List injections, final Map resolutionMap) { + final CompilationUnit cu, + final List injections, + final Map resolutionMap) { injections.stream() .map(e -> unresolve(e, resolutionMap)) .forEach(SQLTableInjectionFilterTransform::wrapExpressionWithCall); var classDecl = injections.get(0).findAncestor(ClassOrInterfaceDeclaration.class); - classDecl.ifPresent(SQLTableInjectionFilterTransform::addFilterMethodIfMissing); + classDecl.ifPresent(cd -> addFilterMethodIfMissing(cu, cd)); } private static Expression unresolve(