Refactored and added new cases to CodeQLStackTraceExposure codemod

andrecsilva · andrecsilva · commit 6b3fb80b3a9d · 2024-11-20T09:36:08.000-03:00
diff --git a/core-codemods/src/main/java/io/codemodder/codemods/DefaultCodemods.java b/core-codemods/src/main/java/io/codemodder/codemods/DefaultCodemods.java
@@ -27,6 +27,7 @@ public static List<Class<? extends CodeChanger>> asList() {
         AddMissingOverrideCodemod.class,
         AvoidImplicitPublicConstructorCodemod.class,
         CodeQLDeserializationOfUserControlledDataCodemod.class,
+        CodeQLErrorMessageExposureCodemod.class,
         CodeQLHttpResponseSplittingCodemod.class,
         CodeQLInputResourceLeakCodemod.class,
         CodeQLInsecureCookieCodemod.class,
@@ -39,7 +40,6 @@ public static List<Class<? extends CodeChanger>> asList() {
         CodeQLRegexInjectionCodemod.class,
         CodeQLSQLInjectionCodemod.class,
         CodeQLSSRFCodemod.class,
-        CodeQLStackTraceExposureCodemod.class,
         CodeQLUnverifiedJwtCodemod.class,
         CodeQLXSSCodemod.class,
         CodeQLXXECodemod.class,
diff --git a/core-codemods/src/main/java/io/codemodder/codemods/codeql/CodeQLErrorMessageExposureCodemod.java b/core-codemods/src/main/java/io/codemodder/codemods/codeql/CodeQLErrorMessageExposureCodemod.java
@@ -0,0 +1,57 @@
+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.errorexposure.ErrorMessageExposureRemediator;
+import io.codemodder.remediation.xxe.XXEIntermediateXMLStreamReaderRemediator;
+
+import javax.inject.Inject;
+import java.util.Optional;
+
+/** A codemod for automatically fixing SQL injection from CodeQL. */
+@Codemod(
+    id = "codeql:java/error-message-exposure",
+    reviewGuidance = ReviewGuidance.MERGE_AFTER_CURSORY_REVIEW,
+    importance = Importance.MEDIUM,
+    executionPriority = CodemodExecutionPriority.HIGH)
+public final class CodeQLErrorMessageExposureCodemod extends CodeQLRemediationCodemod {
+
+  private final Remediator<Result> remediator;
+
+  @Inject
+  public CodeQLErrorMessageExposureCodemod(@ProvidedCodeQLScan(ruleId = "java/error-message-exposure") final RuleSarif sarif) {
+    super(GenericRemediationMetadata.ERROR_MESSAGE_EXPOSURE.reporter(), sarif);
+    this.remediator = new ErrorMessageExposureRemediator<>();
+  }
+
+  @Override
+  public DetectorRule detectorRule() {
+    return new DetectorRule(
+        "error-message-exposure",
+        "Information exposure through an error message",
+        "https://codeql.github.com/codeql-query-help/java/java-error-message-exposure/");
+  }
+
+  @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/core-codemods/src/main/java/io/codemodder/codemods/codeql/CodeQLStackTraceExposureCodemod.java b/core-codemods/src/main/java/io/codemodder/codemods/codeql/CodeQLStackTraceExposureCodemod.java
diff --git a/core-codemods/src/test/java/io/codemodder/codemods/codeql/CodeQLErrorMessageExposureCodemodTest.java b/core-codemods/src/test/java/io/codemodder/codemods/codeql/CodeQLErrorMessageExposureCodemodTest.java
@@ -0,0 +1,10 @@
+package io.codemodder.codemods.codeql;
+
+import io.codemodder.testutils.CodemodTestMixin;
+import io.codemodder.testutils.Metadata;
+
+@Metadata(
+    codemodType = CodeQLErrorMessageExposureCodemod.class,
+    testResourceDir = "error-message-exposure",
+    dependencies = {})
+final class CodeQLErrorMessageExposureCodemodTest implements CodemodTestMixin {}
diff --git a/core-codemods/src/test/java/io/codemodder/codemods/codeql/CodeQLStackTraceExposureCodemodTest.java b/core-codemods/src/test/java/io/codemodder/codemods/codeql/CodeQLStackTraceExposureCodemodTest.java
diff --git a/core-codemods/src/test/resources/error-message-exposure/Test.java.after b/core-codemods/src/test/resources/error-message-exposure/Test.java.after
@@ -13,8 +13,7 @@ public final class Test {
     try {
       throw new Exception();
     } catch (Exception ex) {
-      ex.printStackTrace();
-    }
+}
   }
 
   protected void flowToSendError(HttpServletRequest request, HttpServletResponse response)
@@ -23,7 +22,6 @@ public final class Test {
       throw new Exception();
     } catch (Exception ex) {
       String msg = ex.getMessage();
-      response.sendError(0);
     }
   }
 
@@ -34,7 +32,6 @@ public final class Test {
     } catch (Exception ex) {
       StringWriter sw = new StringWriter();
       ex.printStackTrace(new PrintWriter(sw));
-      response.sendError(0);
     }
   }
 }
diff --git a/core-codemods/src/test/resources/error-message-exposure/Test.java.before b/core-codemods/src/test/resources/error-message-exposure/Test.java.before
diff --git a/core-codemods/src/test/resources/error-message-exposure/out.sarif b/core-codemods/src/test/resources/error-message-exposure/out.sarif
diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/DefaultNodePositionMatcher.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/DefaultNodePositionMatcher.java
@@ -19,7 +19,7 @@ public boolean match(final Node node, int startLine, int endLine) {
   @Override
   public boolean match(final Node node, int startLine, int endLine, int startColumn) {
     return match(node, startLine, endLine)
-        && getRange(node).begin.compareTo(new Position(startLine, startColumn)) <= 0;
+        && getRange(node).begin.compareTo(new Position(startLine, startColumn)) >= 0;
   }
 
   @Override
diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/GenericRemediationMetadata.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/GenericRemediationMetadata.java
@@ -17,7 +17,8 @@ public enum GenericRemediationMetadata {
   WEAK_RANDOM("weak-random"),
   PREDICTABLE_SEED("predictable-seed"),
   ZIP_SLIP("zip-slip"),
-  REGEX_INJECTION("regex-injection");
+  REGEX_INJECTION("regex-injection"),
+  ERROR_MESSAGE_EXPOSURE("error-message-exposure");
 
   private final CodemodReporterStrategy reporter;
 
diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/errorexposure/ErrorMessageExposureFixStrategy.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/errorexposure/ErrorMessageExposureFixStrategy.java
@@ -0,0 +1,70 @@
+package io.codemodder.remediation.errorexposure;
+
+import com.github.javaparser.ast.CompilationUnit;
+import com.github.javaparser.ast.Node;
+import com.github.javaparser.ast.NodeList;
+import com.github.javaparser.ast.expr.Expression;
+import com.github.javaparser.ast.expr.MethodCallExpr;
+import com.github.javaparser.ast.expr.StringLiteralExpr;
+import com.github.javaparser.ast.stmt.ExpressionStmt;
+import com.github.javaparser.ast.stmt.Statement;
+import io.codemodder.CodemodFileScanningResult;
+import io.codemodder.ast.ASTs;
+import io.codemodder.codetf.DetectorRule;
+import io.codemodder.javaparser.ChangesResult;
+import io.codemodder.remediation.*;
+import io.codemodder.remediation.headerinjection.HeaderInjectionFixStrategy;
+import javassist.expr.MethodCall;
+
+import java.lang.reflect.Method;
+import java.util.Collection;
+import java.util.List;
+import java.util.Optional;
+import java.util.Set;
+import java.util.function.Function;
+
+/**
+ * Removes exposure from error messages.
+ */
+public final class ErrorMessageExposureFixStrategy extends MatchAndFixStrategy {
+
+    private static List<String> printErrorMethods = List.of("printStackTrace");
+    private static List<String> printMethods = List.of("println", "print", "sendError");
+
+    /**
+     * Test if the node is an expression that is the argument of a method call
+     * @param node
+     * @return
+     */
+    @Override
+    public boolean match(final Node node) {
+        return Optional.empty()
+                // is an argument of a call e.g. (response.sendError(418,<expression>))
+                .or(() -> Optional.of(node).map(n -> n instanceof Expression? (Expression) n : null).flatMap(ASTs::isArgumentOfMethodCall).filter(mce -> printMethods.contains(mce.getNameAsString())))
+                // is itself a method call that send errors: e.g. err.printStackTrace()
+                .or(() -> Optional.of(node).map(n -> n instanceof Expression? (Expression) n : null).flatMap(e -> e.toMethodCallExpr()).filter(mce -> printErrorMethods.contains(mce.getNameAsString())))
+                .isPresent()
+                ;
+    }
+
+    @Override
+    public SuccessOrReason fix(final CompilationUnit cu, final Node node) {
+        // we know from the match that this is true
+        Expression expr = (Expression) node;
+        // find encompassing statement
+        Optional<Statement> maybeStmt = Optional.<MethodCallExpr>empty()
+                // grab the relevant method call from the two cases
+                .or(() -> Optional.of(node).map(n -> n instanceof Expression? (Expression) n : null).flatMap(ASTs::isArgumentOfMethodCall).filter(mce -> printMethods.contains(mce.getNameAsString())))
+                // is itself a method call that send errors: e.g. err.printStackTrace()
+                .or(() -> Optional.of(node).map(n -> n instanceof Expression? (Expression) n : null).flatMap(e -> e.toMethodCallExpr()).filter(mce -> printErrorMethods.contains(mce.getNameAsString())))
+                // check if the method call is in a statement by itself
+                .flatMap(mce -> mce.getParentNode())
+                .map(p -> p instanceof ExpressionStmt? (ExpressionStmt) p : null);
+        // Remove it
+        if (maybeStmt.isPresent()){
+            maybeStmt.get().remove();
+            return SuccessOrReason.success();
+        }
+        return SuccessOrReason.reason("The call is not a statement");
+    }
+}
diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/errorexposure/ErrorMessageExposureRemediator.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/errorexposure/ErrorMessageExposureRemediator.java
@@ -0,0 +1,55 @@
+package io.codemodder.remediation.errorexposure;
+
+import com.github.javaparser.ast.CompilationUnit;
+import com.github.javaparser.ast.Node;
+import com.github.javaparser.ast.expr.MethodCallExpr;
+import com.github.javaparser.ast.expr.StringLiteralExpr;
+import io.codemodder.CodemodFileScanningResult;
+import io.codemodder.codetf.DetectorRule;
+import io.codemodder.remediation.FixCandidateSearcher;
+import io.codemodder.remediation.Remediator;
+import io.codemodder.remediation.SearcherStrategyRemediator;
+import io.codemodder.remediation.headerinjection.HeaderInjectionFixStrategy;
+
+import java.util.Collection;
+import java.util.Optional;
+import java.util.Set;
+import java.util.function.Function;
+
+/**
+ * Fixes header injection pointed by issues.
+ *
+ * @param <T>
+ */
+public final class ErrorMessageExposureRemediator<T> implements Remediator<T> {
+
+  private final SearcherStrategyRemediator<T> searchStrategyRemediator;
+
+  public ErrorMessageExposureRemediator() {
+    this.searchStrategyRemediator =
+        new SearcherStrategyRemediator.Builder<T>()
+                .withMatchAndFixStrategy(new ErrorMessageExposureFixStrategy())
+            .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/framework/codemodder-base/src/main/resources/generic-remediation-reports/error-message-exposure/description.md b/framework/codemodder-base/src/main/resources/generic-remediation-reports/error-message-exposure/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/framework/codemodder-base/src/main/resources/generic-remediation-reports/error-message-exposure/report.json b/framework/codemodder-base/src/main/resources/generic-remediation-reports/error-message-exposure/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"]
+}

Original file line number	Diff line number	Diff line change
`@@ -13,8 +13,7 @@ public final class Test {`
`13`	`13`	`try {`
`14`	`14`	`throw new Exception();`
`15`	`15`	`} catch (Exception ex) {`
`16`		`- ex.printStackTrace();`
`17`		`- }`
	`16`	`+}`
`18`	`17`	`}`
`19`	`18`
`20`	`19`	`protected void flowToSendError(HttpServletRequest request, HttpServletResponse response)`
`@@ -23,7 +22,6 @@ public final class Test {`
`23`	`22`	`throw new Exception();`
`24`	`23`	`} catch (Exception ex) {`
`25`	`24`	`String msg = ex.getMessage();`
`26`		`- response.sendError(0);`
`27`	`25`	`}`
`28`	`26`	`}`
`29`	`27`
`@@ -34,7 +32,6 @@ public final class Test {`
`34`	`32`	`} catch (Exception ex) {`
`35`	`33`	`StringWriter sw = new StringWriter();`
`36`	`34`	`ex.printStackTrace(new PrintWriter(sw));`
`37`		`- response.sendError(0);`
`38`	`35`	`}`
`39`	`36`	`}`
`40`	`37`	`}`
Original file line number	Diff line number	Diff line change
`@@ -19,7 +19,7 @@ public boolean match(final Node node, int startLine, int endLine) {`
`19`	`19`	`@Override`
`20`	`20`	`public boolean match(final Node node, int startLine, int endLine, int startColumn) {`
`21`	`21`	`return match(node, startLine, endLine)`
`22`		`- && getRange(node).begin.compareTo(new Position(startLine, startColumn)) <= 0;`
	`22`	`+ && getRange(node).begin.compareTo(new Position(startLine, startColumn)) >= 0;`
`23`	`23`	`}`
`24`	`24`
`25`	`25`	`@Override`