Add support for fixing SQL Injection in security hotspot (#384)

Co-authored-by: Carlos Uscanga <[email protected]>

Author: nahsra

core-codemods/src/main/java/io/codemodder/codemods/DefectDojoSqlInjectionCodemod.java

@@ -25,12 +25,14 @@ public final class DefectDojoSqlInjectionCodemod extends JavaParserChanger
     implements FixOnlyCodeChanger {
 
   private final RuleFindings findings;
+  private final JavaParserSQLInjectionRemediatorStrategy remediatorStrategy;
 
   @Inject
   public DefectDojoSqlInjectionCodemod(
       @DefectDojoScan(ruleId = "java.lang.security.audit.sqli.jdbc-sqli.jdbc-sqli")
           RuleFindings findings) {
     this.findings = Objects.requireNonNull(findings);
+    this.remediatorStrategy = JavaParserSQLInjectionRemediatorStrategy.DEFAULT;
   }
 
   @Override
@@ -50,12 +52,11 @@ public DetectorRule detectorRule() {
   public CodemodFileScanningResult visit(
       final CodemodInvocationContext context, final CompilationUnit cu) {
     List<Finding> findingsForThisPath = findings.getForPath(context.path());
-
-    return JavaParserSQLInjectionRemediatorStrategy.DEFAULT.visit(
-        context,
+    return remediatorStrategy.remediateAll(
         cu,
-        findingsForThisPath,
+        context.path().toString(),
         detectorRule(),
+        findingsForThisPath,
         finding -> String.valueOf(finding.getId()),
         Finding::getLine);
   }

core-codemods/src/main/java/io/codemodder/codemods/SonarSQLInjectionCodemod.java

@@ -0,0 +1,53 @@
+package io.codemodder.codemods;
+
+import com.github.javaparser.ast.CompilationUnit;
+import io.codemodder.*;
+import io.codemodder.codemods.util.JavaParserSQLInjectionRemediatorStrategy;
+import io.codemodder.codetf.DetectorRule;
+import io.codemodder.providers.sonar.ProvidedSonarScan;
+import io.codemodder.providers.sonar.RuleHotspot;
+import io.codemodder.providers.sonar.SonarRemediatingJavaParserChanger;
+import io.codemodder.sonar.model.Hotspot;
+import io.codemodder.sonar.model.SonarFinding;
+import java.util.List;
+import java.util.Objects;
+import javax.inject.Inject;
+
+@Codemod(
+    id = "sonar:java/sonar-sql-injection-s2077",
+    reviewGuidance = ReviewGuidance.MERGE_AFTER_REVIEW,
+    importance = Importance.HIGH,
+    executionPriority = CodemodExecutionPriority.HIGH)
+public final class SonarSQLInjectionCodemod extends SonarRemediatingJavaParserChanger {
+
+  private final JavaParserSQLInjectionRemediatorStrategy remediationStrategy;
+  private final RuleHotspot hotspots;
+
+  @Inject
+  public SonarSQLInjectionCodemod(
+      @ProvidedSonarScan(ruleId = "java:S2077") final RuleHotspot hotspots) {
+    this.hotspots = Objects.requireNonNull(hotspots);
+    this.remediationStrategy = JavaParserSQLInjectionRemediatorStrategy.DEFAULT;
+  }
+
+  @Override
+  public DetectorRule detectorRule() {
+    return new DetectorRule(
+        "java:S2077",
+        "Formatting SQL queries is security-sensitive",
+        "https://rules.sonarsource.com/java/RSPEC-2077/");
+  }
+
+  @Override
+  public CodemodFileScanningResult visit(
+      final CodemodInvocationContext context, final CompilationUnit cu) {
+    List<Hotspot> hotspotsForFile = hotspots.getResultsByPath(context.path());
+    return remediationStrategy.remediateAll(
+        cu,
+        context.path().toString(),
+        detectorRule(),
+        hotspotsForFile,
+        SonarFinding::getKey,
+        SonarFinding::getLine);
+  }
+}

core-codemods/src/main/java/io/codemodder/codemods/util/DefaultJavaParserSQLInjectionRemediatorStrategy.java

@@ -4,7 +4,6 @@
 import com.github.javaparser.ast.expr.MethodCallExpr;
 import io.codemodder.CodemodChange;
 import io.codemodder.CodemodFileScanningResult;
-import io.codemodder.CodemodInvocationContext;
 import io.codemodder.codemods.SQLParameterizer;
 import io.codemodder.codemods.SQLParameterizerWithCleanup;
 import io.codemodder.codetf.DetectorRule;
@@ -25,40 +24,40 @@ final class DefaultJavaParserSQLInjectionRemediatorStrategy
   /**
    * Visits the provided CompilationUnit and processes findings for potential SQL injections.
    *
-   * @param context the context of the codemod invocation
    * @param cu the compilation unit to be scanned
-   * @param pathFindings a collection of findings to be processed
-   * @param detectorRule the rule used to detect potential issues
+   * @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 findingLineExtractor a function to extract the line number from a finding
    * @param <T> the type of the findings
    * @return a result object containing the changes and unfixed findings
    */
-  public <T> CodemodFileScanningResult visit(
-      final CodemodInvocationContext context,
+  @Override
+  public <T> CodemodFileScanningResult remediateAll(
       final CompilationUnit cu,
-      final Collection<T> pathFindings,
+      final String path,
       final DetectorRule detectorRule,
+      final Collection<T> findingsForPath,
       final Function<T, String> findingIdExtractor,
       final Function<T, Integer> findingLineExtractor) {
 
     final List<MethodCallExpr> allMethodCalls = cu.findAll(MethodCallExpr.class);
 
-    if (pathFindings.isEmpty()) {
+    if (findingsForPath.isEmpty()) {
       return CodemodFileScanningResult.none();
     }
 
     final List<UnfixedFinding> unfixedFindings = new ArrayList<>();
     final List<CodemodChange> changes = new ArrayList<>();
 
-    for (T finding : pathFindings) {
+    for (T finding : findingsForPath) {
       final String id = findingIdExtractor.apply(finding);
       final Integer line = findingLineExtractor.apply(finding);
 
       if (line == null) {
         final UnfixedFinding unfixableFinding =
-            new UnfixedFinding(
-                id, detectorRule, context.path().toString(), null, "No line number provided");
+            new UnfixedFinding(id, detectorRule, path, null, "No line number provided");
         unfixedFindings.add(unfixableFinding);
         continue;
       }
@@ -72,11 +71,7 @@ public <T> CodemodFileScanningResult visit(
       if (supportedSqlMethodCallsOnThatLine.isEmpty()) {
         final UnfixedFinding unfixableFinding =
             new UnfixedFinding(
-                id,
-                detectorRule,
-                context.path().toString(),
-                line,
-                "No supported SQL methods found on the given line");
+                id, detectorRule, path, line, "No supported SQL methods found on the given line");
         unfixedFindings.add(unfixableFinding);
         continue;
       }
@@ -86,7 +81,7 @@ public <T> CodemodFileScanningResult visit(
             new UnfixedFinding(
                 id,
                 detectorRule,
-                context.path().toString(),
+                path,
                 line,
                 "Multiple supported SQL methods found on the given line");
         unfixedFindings.add(unfixableFinding);
@@ -101,7 +96,7 @@ public <T> CodemodFileScanningResult visit(
             new UnfixedFinding(
                 id,
                 detectorRule,
-                context.path().toString(),
+                path,
                 line,
                 "State changing effects possible or unrecognized code shape");
         unfixedFindings.add(unfixableFinding);

core-codemods/src/main/java/io/codemodder/codemods/util/JavaParserSQLInjectionRemediatorStrategy.java

@@ -2,7 +2,6 @@
 
 import com.github.javaparser.ast.CompilationUnit;
 import io.codemodder.CodemodFileScanningResult;
-import io.codemodder.CodemodInvocationContext;
 import io.codemodder.codetf.DetectorRule;
 import java.util.Collection;
 import java.util.function.Function;
@@ -17,23 +16,22 @@ public interface JavaParserSQLInjectionRemediatorStrategy {
   /**
    * Visits the provided CompilationUnit and processes findings for potential SQL injections.
    *
-   * @param context the context of the codemod invocation
    * @param cu the compilation unit to be scanned
    * @param pathFindings a collection of findings to be processed
-   * @param detectorRule the rule used to detect potential issues
    * @param findingIdExtractor a function to extract the ID from a finding
    * @param findingLineExtractor a function to extract the line number from a finding
    * @param <T> the type of the findings
    * @return a result object containing the changes and unfixed findings
    */
-  <T> CodemodFileScanningResult visit(
-      final CodemodInvocationContext context,
+  <T> CodemodFileScanningResult remediateAll(
       final CompilationUnit cu,
+      final String path,
+      final DetectorRule rule,
       final Collection<T> pathFindings,
-      final DetectorRule detectorRule,
       final Function<T, String> findingIdExtractor,
       final Function<T, Integer> findingLineExtractor);
 
+  /** A default implementation that should be used in all non-test scenarios. */
   JavaParserSQLInjectionRemediatorStrategy DEFAULT =
       new DefaultJavaParserSQLInjectionRemediatorStrategy();
 }

core-codemods/src/main/resources/io/codemodder/codemods/SonarSQLInjectionCodemod/description.md

@@ -0,0 +1,13 @@
+This change refactors SQL statements to be parameterized, rather than built by hand.
+
+Without parameterization, developers must remember to escape inputs using the rules for that database. It's usually buggy, at the least -- and sometimes vulnerable.
+
+Our changes look something like this:
+
+```diff
+- Statement stmt = connection.createStatement();
+- ResultSet rs = stmt.executeQuery("SELECT * FROM users WHERE name = '" + user + "'");
++ PreparedStatement stmt = connection.prepareStatement("SELECT * FROM users WHERE name = ?");
++ stmt.setString(1, user);
++ ResultSet rs = stmt.executeQuery();
+```

core-codemods/src/main/resources/io/codemodder/codemods/SonarSQLInjectionCodemod/report.json

@@ -0,0 +1,9 @@
+{
+  "summary" : "Refactored to use parameterized SQL APIs",
+  "change": "Parameterized SQL usage to prevent any bugs or vulnerabilities",
+  "reviewGuidanceIJustification" : "Although there should be no functional differences, the rewrite here is complex and should be verified by a human.",
+  "references" : [
+    "https://cheatsheetseries.owasp.org/cheatsheets/SQL_Injection_Prevention_Cheat_Sheet.html",
+    "https://cwe.mitre.org/data/definitions/89.html"
+  ]
+}

core-codemods/src/test/java/io/codemodder/codemods/SonarSQLInjectionCodemodTest.java

@@ -0,0 +1,27 @@
+package io.codemodder.codemods;
+
+import io.codemodder.testutils.CodemodTestMixin;
+import io.codemodder.testutils.Metadata;
+import org.junit.jupiter.api.Nested;
+
+final class SonarSQLInjectionCodemodTest {
+
+  @Nested
+  @Metadata(
+      codemodType = SonarSQLInjectionCodemod.class,
+      testResourceDir = "sonar-sql-injection-s2077/unsupported",
+      renameTestFile = "src/main/java/org/owasp/webgoat/container/users/UserService.java",
+      expectingFailedFixesAtLines = {52}, // we don't support this method
+      dependencies = {})
+  class UnsupportedTest implements CodemodTestMixin {}
+
+  @Nested
+  @Metadata(
+      codemodType = SonarSQLInjectionCodemod.class,
+      testResourceDir = "sonar-sql-injection-s2077/supported",
+      renameTestFile =
+          "src/main/java/org/owasp/webgoat/lessons/sqlinjection/advanced/SqlInjectionChallenge.java",
+      expectingFixesAtLines = {69},
+      dependencies = {})
+  class SupportedTest implements CodemodTestMixin {}
+}

core-codemods/src/test/resources/sonar-sql-injection-s2077/supported/SqlInjectionChallenge.java.after

@@ -0,0 +1,106 @@
+/*
+ * This file is part of WebGoat, an Open Web Application Security Project utility. For details, please see http://www.owasp.org/
+ *
+ * Copyright (c) 2002 - 2019 Bruce Mayhew
+ *
+ * This program is free software; you can redistribute it and/or modify it under the terms of the
+ * GNU General Public License as published by the Free Software Foundation; either version 2 of the
+ * License, or (at your option) any later version.
+ *
+ * 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 GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along with this program; if
+ * not, write to the Free Software Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA
+ * 02111-1307, USA.
+ *
+ * Getting Source ==============
+ *
+ * Source for this application is maintained at https://github.com/WebGoat/WebGoat, a repository for free software projects.
+ */
+
+package org.owasp.webgoat.lessons.sqlinjection.advanced;
+
+import java.sql.*;
+import java.sql.PreparedStatement;
+import lombok.extern.slf4j.Slf4j;
+import org.owasp.webgoat.container.LessonDataSource;
+import org.owasp.webgoat.container.assignments.AssignmentEndpoint;
+import org.owasp.webgoat.container.assignments.AssignmentHints;
+import org.owasp.webgoat.container.assignments.AttackResult;
+import org.springframework.util.StringUtils;
+import org.springframework.web.bind.annotation.PutMapping;
+import org.springframework.web.bind.annotation.RequestParam;
+import org.springframework.web.bind.annotation.ResponseBody;
+import org.springframework.web.bind.annotation.RestController;
+
+/**
+ * @author nbaars
+ * @since 4/8/17.
+ */
+@RestController
+@AssignmentHints(
+    value = {"SqlInjectionChallenge1", "SqlInjectionChallenge2", "SqlInjectionChallenge3"})
+@Slf4j
+public class SqlInjectionChallenge extends AssignmentEndpoint {
+
+  private final LessonDataSource dataSource;
+
+  public SqlInjectionChallenge(LessonDataSource dataSource) {
+    this.dataSource = dataSource;
+  }
+
+  @PutMapping("/SqlInjectionAdvanced/challenge")
+  // assignment path is bounded to class so we use different http method :-)
+  @ResponseBody
+  public AttackResult registerNewUser(
+      @RequestParam String username_reg,
+      @RequestParam String email_reg,
+      @RequestParam String password_reg)
+      throws Exception {
+    AttackResult attackResult = checkArguments(username_reg, email_reg, password_reg);
+
+    if (attackResult == null) {
+
+      try (Connection connection = dataSource.getConnection()) {
+        String checkUserQuery =
+            "select userid from sql_challenge_users where userid = ?";
+        PreparedStatement statement = connection.prepareStatement(checkUserQuery);
+        statement.setString(1, username_reg);
+        ResultSet resultSet = statement.execute();
+        if (resultSet.next()) {
+          if (username_reg.contains("tom'")) {
+            attackResult = success(this).feedback("user.exists").build();
+          } else {
+            attackResult = failed(this).feedback("user.exists").feedbackArgs(username_reg).build();
+          }
+        } else {
+          PreparedStatement preparedStatement =
+              connection.prepareStatement("INSERT INTO sql_challenge_users VALUES (?, ?, ?)");
+          preparedStatement.setString(1, username_reg);
+          preparedStatement.setString(2, email_reg);
+          preparedStatement.setString(3, password_reg);
+          preparedStatement.execute();
+          attackResult = success(this).feedback("user.created").feedbackArgs(username_reg).build();
+        }
+
+      } catch (SQLException e) {
+        attackResult = failed(this).output("Something went wrong").build();
+      }
+    }
+    return attackResult;
+  }
+
+  private AttackResult checkArguments(String username_reg, String email_reg, String password_reg) {
+    if (StringUtils.isEmpty(username_reg)
+        || StringUtils.isEmpty(email_reg)
+        || StringUtils.isEmpty(password_reg)) {
+      return failed(this).feedback("input.invalid").build();
+    }
+    if (username_reg.length() > 250 || email_reg.length() > 30 || password_reg.length() > 30) {
+      return failed(this).feedback("input.invalid").build();
+    }
+    return null;
+  }
+}