Initial work

andrecsilva · Dec 10, 2024 · Dec 9, 2024 · Dec 10, 2024 · Dec 17, 2024 · Dec 17, 2024
commit 1b8d8a48c088be5e90d96fd7cce056863fc9d07a
diff --git a/core-codemods/src/main/java/io/codemodder/codemods/DefaultCodemods.java b/core-codemods/src/main/java/io/codemodder/codemods/DefaultCodemods.java
@@ -39,6 +39,7 @@ public static List<Class<? extends CodeChanger>> asList() {
         CodeQLOutputResourceLeakCodemod.class,
         CodeQLPotentiallyUnsafeCryptoAlgorithmCodemod.class,
         CodeQLPredictableSeedCodemod.class,
+        CodeQLRegexDoSCodemod.class,
         CodeQLRegexInjectionCodemod.class,
         CodeQLSQLInjectionCodemod.class,
         CodeQLSSRFCodemod.class,

diff --git a/core-codemods/src/main/java/io/codemodder/codemods/codeql/CodeQLRegexDoSCodemod.java b/core-codemods/src/main/java/io/codemodder/codemods/codeql/CodeQLRegexDoSCodemod.java
@@ -0,0 +1,56 @@
+package io.codemodder.codemods.codeql;
+
+import com.contrastsecurity.sarif.Result;
+import com.github.javaparser.ast.CompilationUnit;
+import io.codemodder.*;
+import io.codemodder.codetf.DetectorRule;
+import io.codemodder.providers.sarif.codeql.ProvidedCodeQLScan;
+import io.codemodder.remediation.GenericRemediationMetadata;
+import io.codemodder.remediation.Remediator;
+import io.codemodder.remediation.regexdos.RegexDoSRemediator;
+import java.util.Optional;
+import javax.inject.Inject;
+
+/** A codemod that mitigates regex dos vulnerabilities * */
+@Codemod(
+    id = "codeql:java/regex-dos",
+    reviewGuidance = ReviewGuidance.MERGE_AFTER_CURSORY_REVIEW,
+    importance = Importance.MEDIUM,
+    executionPriority = CodemodExecutionPriority.HIGH)
+public final class CodeQLRegexDoSCodemod extends CodeQLRemediationCodemod {
+
+  private final Remediator<Result> remediator;
+
+  @Inject
+  public CodeQLRegexDoSCodemod(
+      @ProvidedCodeQLScan(ruleId = "java/polynomial-redos") final RuleSarif sarif) {
+    super(GenericRemediationMetadata.REGEX_DOS.reporter(), sarif);
+    this.remediator = new RegexDoSRemediator<>();
+  }
+
+  @Override
+  public DetectorRule detectorRule() {
+    return new DetectorRule(
+        "polynomial-redos",
+        "Polynomial regular expression used on uncontrolled data",
+        "https://codeql.github.com/codeql-query-help/java/java-polynomial-redos/");
+  }
+
+  @Override
+  public CodemodFileScanningResult visit(
+      final CodemodInvocationContext context, final CompilationUnit cu) {
+    return remediator.remediateAll(
+        cu,
+        context.path().toString(),
+        detectorRule(),
+        ruleSarif.getResultsByLocationPath(context.path()),
+        SarifFindingKeyUtil::buildFindingId,
+        r -> r.getLocations().get(0).getPhysicalLocation().getRegion().getStartLine(),
+        r ->
+            Optional.ofNullable(
+                r.getLocations().get(0).getPhysicalLocation().getRegion().getEndLine()),
+        r ->
+            Optional.ofNullable(
+                r.getLocations().get(0).getPhysicalLocation().getRegion().getStartColumn()));
+  }
+}
diff --git a/framework/codemodder-base/src/main/java/io/codemodder/ast/ASTs.java b/framework/codemodder-base/src/main/java/io/codemodder/ast/ASTs.java
@@ -19,6 +19,7 @@
 import com.github.javaparser.ast.nodeTypes.NodeWithSimpleName;
 import com.github.javaparser.ast.stmt.*;
 import com.github.javaparser.ast.type.TypeParameter;
+import com.github.javaparser.resolution.types.ResolvedType;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Optional;
@@ -883,6 +884,20 @@ public boolean hasNext() {
     }
   }
 
+  /**
+   * Resolves type of a given expression e.
+   *
+   * @param e
+   * @return
+   */
+  public static Optional<ResolvedType> calculateResolvedType(final Expression e) {
+    try {
+      return Optional.of(e.calculateResolvedType());
+    } catch (final RuntimeException exception) {
+      return Optional.empty();
+    }
+  }
+
   /**
    * Checks if a node is a MethodCallExpr that is the initialization of a declaration with one of
    * the types in assignedToTypes.

diff --git a/...k/codemodder-base/src/main/java/io/codemodder/remediation/GenericRemediationMetadata.java b/...k/codemodder-base/src/main/java/io/codemodder/remediation/GenericRemediationMetadata.java
@@ -18,6 +18,7 @@ public enum GenericRemediationMetadata {
   PREDICTABLE_SEED("predictable-seed"),
   ZIP_SLIP("zip-slip"),
   REGEX_INJECTION("regex-injection"),
+  REGEX_DOS("regex-dos"),
   ERROR_MESSAGE_EXPOSURE("error-message-exposure"),
   LOG_INJECTION("log-injection"),
   WEAK_CRYPTO_ALGORITHM("weak-crypto-algorithm");

diff --git a/...codemodder-base/src/main/java/io/codemodder/remediation/regexdos/RegexDoSFixStrategy.java b/...codemodder-base/src/main/java/io/codemodder/remediation/regexdos/RegexDoSFixStrategy.java
@@ -0,0 +1,47 @@
+package io.codemodder.remediation.regexdos;
+
+import com.github.javaparser.ast.CompilationUnit;
+import com.github.javaparser.ast.Node;
+import com.github.javaparser.ast.expr.MethodCallExpr;
+import io.codemodder.ast.ASTs;
+import io.codemodder.remediation.MatchAndFixStrategy;
+import io.codemodder.remediation.SuccessOrReason;
+import java.util.List;
+import java.util.Optional;
+
+/** Adds a timeout function and wraps regex match call with it * */
+final class RegexDoSFixStrategy extends MatchAndFixStrategy {
+
+  /**
+   * Test if the node is a Pattern.matcher*() call
+   *
+   * @param node
+   * @return
+   */
+  @Override
+  public boolean match(final Node node) {
+    return Optional.of(node)
+        .map(n -> n instanceof MethodCallExpr mce ? mce : null)
+        .filter(mce -> mce.hasScope())
+        // Check if the type is Pattern
+        .filter(
+            mce ->
+                ASTs.calculateResolvedType(mce)
+                    .filter(t -> "java.util.regex.Pattern".equals(t.describe()))
+                    .isPresent())
+        .filter(mce -> "matcher".equals(mce.getNameAsString()))
+        .isPresent();
+  }
+
+  private static List<String> matchingMethods =
+      List.of("matches", "find", "replaceAll", "replaceFirst");
+
+  @Override
+  public SuccessOrReason fix(final CompilationUnit cu, final Node node) {
+    // Find all the matcher calls from the matchingMethods list
+    // if any, wrap it with executeWithTimeout with a default 5000 of timeout
+    // Add executeWithTimout method to the encompassing class
+    // Add needed imports (Callable, RuntimeException)
+    return SuccessOrReason.reason("Doesn't match expected code shape");
+  }
+}
diff --git a/.../codemodder-base/src/main/java/io/codemodder/remediation/regexdos/RegexDoSRemediator.java b/.../codemodder-base/src/main/java/io/codemodder/remediation/regexdos/RegexDoSRemediator.java
@@ -0,0 +1,48 @@
+package io.codemodder.remediation.regexdos;
+
+import com.github.javaparser.ast.CompilationUnit;
+import io.codemodder.CodemodFileScanningResult;
+import io.codemodder.codetf.DetectorRule;
+import io.codemodder.remediation.Remediator;
+import io.codemodder.remediation.SearcherStrategyRemediator;
+import java.util.Collection;
+import java.util.Optional;
+import java.util.function.Function;
+
+/**
+ * Fixes header injection pointed by issues.
+ *
+ * @param <T>
+ */
+public final class RegexDoSRemediator<T> implements Remediator<T> {
+
+  private final SearcherStrategyRemediator<T> searchStrategyRemediator;
+
+  public RegexDoSRemediator() {
+    this.searchStrategyRemediator =
+        new SearcherStrategyRemediator.Builder<T>()
+            .withMatchAndFixStrategy(new RegexDoSFixStrategy())
+            .build();
+  }
+
+  @Override
+  public CodemodFileScanningResult remediateAll(
+      CompilationUnit cu,
+      String path,
+      DetectorRule detectorRule,
+      Collection<T> findingsForPath,
+      Function<T, String> findingIdExtractor,
+      Function<T, Integer> findingStartLineExtractor,
+      Function<T, Optional<Integer>> findingEndLineExtractor,
+      Function<T, Optional<Integer>> findingColumnExtractor) {
+    return searchStrategyRemediator.remediateAll(
+        cu,
+        path,
+        detectorRule,
+        findingsForPath,
+        findingIdExtractor,
+        findingStartLineExtractor,
+        findingEndLineExtractor,
+        findingColumnExtractor);
+  }
+}
diff --git a/...ain/resources/generic-remediation-reports/error-message-exposure/description.md b/...ain/resources/generic-remediation-reports/error-message-exposure/description.md
@@ -1,14 +1,20 @@
-This change removes exposure through sending/printing of error and exception data.
+This change adds a timout to regex matching calls from the `java.util.regex` libraries.
 
 Our changes look like this:
 
 ```java
- void function(HttpServletResponse response) {
-    PrintWriter pw = reponse.getWriter();
-    try{
-        ...
-    } catch (Exception e) {
--        pw.println(e.getMessage());
-    }
- }
++public <E> E executeWithTimeout(final Callable<E> action, final int timeout){
++    Future<E> maybeResult = Executors.newSingleThreadExecutor().submit(action);
++    try{
++        return maybeResult.get(timeout, TimeUnit.MILLISECONDS);
++    }catch(Exception e){
++        throw new RuntimeException("Failed to execute within time limit.");
++    }
++}
+...
+String input = "aaaaaaaaaaaaaaaaaaaaa";
+Pattern pat = Pattern.compile("^(a+)+$");
+var matcher = pat.matcher(input);
+- matcher.matches();
++ executeWithTimeout(() -> matcher.matches(), 5000);
 ```
diff --git a/...er-base/src/main/resources/generic-remediation-reports/error-message-exposure/report.json b/...er-base/src/main/resources/generic-remediation-reports/error-message-exposure/report.json
@@ -1,6 +1,6 @@
 {
-  "summary" : "Removed printing/sending of error data",
-  "change" : "Removed printing/sending of error data",
-  "reviewGuidanceIJustification" : "While this change is most likely harmless, it may be the case that the other endpoint is expecting the message and needs adjustment.",
-  "references" : ["https://cwe.mitre.org/data/definitions/209.html", "https://owasp.org/www-community/Improper_Error_Handling", "https://www.securecoding.cert.org/confluence/display/java/ERR01-J.+Do+not+allow+exceptions+to+expose+sensitive+information"]
+  "summary" : "Added a timeout to regular expression matching",
+  "change" : "Added a timeout to regular expression matching",
+  "reviewGuidanceIJustification" : "The expected timeout is highly dependent on the application and should be adjusted to conform to it.",
+  "references" : ["https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS", "https://cwe.mitre.org/data/definitions/400.html", "https://github.com/google/re2j"]
 }
diff --git a/...er-base/src/main/resources/generic-remediation-reports/regex-dos/description.md b/...er-base/src/main/resources/generic-remediation-reports/regex-dos/description.md
@@ -0,0 +1,14 @@
+This change removes exposure through sending/printing of error and exception data.
+
+Our changes look like this:
+
+```java
+ void function(HttpServletResponse response) {
+    PrintWriter pw = reponse.getWriter();
+    try{
+        ...
+    } catch (Exception e) {
+-        pw.println(e.getMessage());
+    }
+ }
+```
diff --git a/...work/codemodder-base/src/main/resources/generic-remediation-reports/regex-dos/report.json b/...work/codemodder-base/src/main/resources/generic-remediation-reports/regex-dos/report.json
@@ -0,0 +1,6 @@
+{
+  "summary" : "Removed printing/sending of error data",
+  "change" : "Removed printing/sending of error data",
+  "reviewGuidanceIJustification" : "While this change is most likely harmless, it may be the case that the other endpoint is expecting the message and needs adjustment.",
+  "references" : ["https://cwe.mitre.org/data/definitions/209.html", "https://owasp.org/www-community/Improper_Error_Handling", "https://www.securecoding.cert.org/confluence/display/java/ERR01-J.+Do+not+allow+exceptions+to+expose+sensitive+information"]
+}