add log injection remediation

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

@@ -34,6 +34,7 @@ public static List<Class<? extends CodeChanger>> asList() {
         CodeQLJDBCResourceLeakCodemod.class,
         CodeQLJEXLInjectionCodemod.class,
         CodeQLJNDIInjectionCodemod.class,
+        CodeQLLogInjectionCodemod.class,
         CodeQLMavenSecureURLCodemod.class,
         CodeQLOutputResourceLeakCodemod.class,
         CodeQLPredictableSeedCodemod.class,

core-codemods/src/main/java/io/codemodder/codemods/codeql/CodeQLLogInjectionCodemod.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.loginjection.LogInjectionRemediator;
+import java.util.Optional;
+import javax.inject.Inject;
+
+/** A codemod for automatically fixing Log Injection from CodeQL. */
+@Codemod(
+    id = "codeql:java/log-injection",
+    reviewGuidance = ReviewGuidance.MERGE_WITHOUT_REVIEW,
+    importance = Importance.HIGH,
+    executionPriority = CodemodExecutionPriority.HIGH)
+public final class CodeQLLogInjectionCodemod extends CodeQLRemediationCodemod {
+
+  private final Remediator<Result> remediator;
+
+  @Inject
+  public CodeQLLogInjectionCodemod(
+      @ProvidedCodeQLScan(ruleId = "java/log-injection") final RuleSarif sarif) {
+    super(GenericRemediationMetadata.LOG_INJECTION.reporter(), sarif);
+    this.remediator = new LogInjectionRemediator<>();
+  }
+
+  @Override
+  public DetectorRule detectorRule() {
+    return new DetectorRule(
+        "log-injection",
+        "Log Injection",
+        "https://codeql.github.com/codeql-query-help/java/java-log-injection/");
+  }
+
+  @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()));
+  }
+}

framework/codemodder-base/src/main/java/io/codemodder/remediation/GenericRemediationMetadata.java

@@ -18,7 +18,8 @@ public enum GenericRemediationMetadata {
   PREDICTABLE_SEED("predictable-seed"),
   ZIP_SLIP("zip-slip"),
   REGEX_INJECTION("regex-injection"),
-  ERROR_MESSAGE_EXPOSURE("error-message-exposure");
+  ERROR_MESSAGE_EXPOSURE("error-message-exposure"),
+  LOG_INJECTION("log-injection");
 
   private final CodemodReporterStrategy reporter;
 

framework/codemodder-base/src/main/java/io/codemodder/remediation/loginjection/LogInjectionRemediator.java

@@ -0,0 +1,58 @@
+package io.codemodder.remediation.loginjection;
+
+import com.github.javaparser.ast.CompilationUnit;
+import io.codemodder.CodemodFileScanningResult;
+import io.codemodder.codetf.DetectorRule;
+import io.codemodder.remediation.*;
+import java.util.Collection;
+import java.util.Optional;
+import java.util.Set;
+import java.util.function.Function;
+
+/** Remediator for Log Injection vulnerabilities. */
+public final class LogInjectionRemediator<T> implements Remediator<T> {
+
+  private final SearcherStrategyRemediator<T> searchStrategyRemediator;
+
+  public LogInjectionRemediator() {
+    this.searchStrategyRemediator =
+        new SearcherStrategyRemediator.Builder<T>()
+            .withSearcherStrategyPair(
+                new FixCandidateSearcher.Builder<T>()
+                    .withMatcher(
+                        n ->
+                            Optional.of(n)
+                                .map(MethodOrConstructor::new)
+                                .filter(mce -> mce.isMethodCallWithNameIn(loggerNames))
+                                .filter(mce -> mce.asNode().hasScope())
+                                .filter(mce -> !mce.getArguments().isEmpty())
+                                .isPresent())
+                    .build(),
+                new LogStatementFixer())
+            .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>> findingStartColumnExtractor) {
+    return searchStrategyRemediator.remediateAll(
+        cu,
+        path,
+        detectorRule,
+        findingsForPath,
+        findingIdExtractor,
+        findingStartLineExtractor,
+        findingEndLineExtractor,
+        findingStartColumnExtractor);
+  }
+
+  private static final Set<String> loggerNames =
+      Set.of("log", "warn", "error", "info", "debug", "trace", "fatal");
+}

framework/codemodder-base/src/main/java/io/codemodder/remediation/loginjection/LogStatementFixer.java

@@ -0,0 +1,163 @@
+package io.codemodder.remediation.loginjection;
+
+import static io.codemodder.javaparser.JavaParserTransformer.wrap;
+
+import com.github.javaparser.ast.CompilationUnit;
+import com.github.javaparser.ast.Node;
+import com.github.javaparser.ast.NodeList;
+import com.github.javaparser.ast.body.Parameter;
+import com.github.javaparser.ast.body.VariableDeclarator;
+import com.github.javaparser.ast.expr.BinaryExpr;
+import com.github.javaparser.ast.expr.Expression;
+import com.github.javaparser.ast.expr.MethodCallExpr;
+import com.github.javaparser.ast.expr.NameExpr;
+import com.github.javaparser.resolution.types.ResolvedType;
+import io.codemodder.DependencyGAV;
+import io.codemodder.ast.ASTs;
+import io.codemodder.ast.LocalDeclaration;
+import io.codemodder.remediation.RemediationStrategy;
+import io.codemodder.remediation.SuccessOrReason;
+import io.github.pixee.security.Newlines;
+import java.util.List;
+import java.util.Optional;
+
+/**
+ * The shapes of code we want to be able to fix:
+ *
+ * <pre>
+ * log.info("User with id: " + userId + " has been created");
+ * logger.error("User with id: " + userId + " has been created", ex);
+ * log.warn(msg);
+ * </pre>
+ */
+final class LogStatementFixer implements RemediationStrategy {
+
+  @Override
+  public SuccessOrReason fix(final CompilationUnit compilationUnit, final Node node) {
+    MethodCallExpr logCall = (MethodCallExpr) node;
+    NodeList<Expression> arguments = logCall.getArguments();
+    return fixArguments(arguments);
+  }
+
+  private SuccessOrReason fixArguments(final NodeList<Expression> arguments) {
+
+    // first and only is NameExpr (not an exception) (args == 1), (args can be 2 if exception)
+    if ((arguments.size() == 1 && arguments.get(0).isNameExpr())
+        || (arguments.size() == 2
+            && arguments.get(0).isNameExpr()
+            && isException(arguments.get(1)))) {
+      NameExpr argument = arguments.get(0).asNameExpr();
+      wrapWithNewlineSanitizer(argument);
+      return SuccessOrReason.success(List.of(DependencyGAV.OWASP_XSS_JAVA_ENCODER));
+    }
+
+    // first is string literal and second is NameExpr (args can be 3 if not exception)
+    if ((arguments.size() == 2
+            && arguments.get(0).isStringLiteralExpr()
+            && arguments.get(1).isNameExpr())
+        || (arguments.size() == 3
+            && arguments.get(0).isStringLiteralExpr()
+            && arguments.get(1).isNameExpr()
+            && isException(arguments.get(2)))) {
+      NameExpr argument = arguments.get(1).asNameExpr();
+      wrapWithNewlineSanitizer(argument);
+      return SuccessOrReason.success(List.of(DependencyGAV.OWASP_XSS_JAVA_ENCODER));
+    }
+
+    // first is BinaryExpr with NameExpr in it (args == 2) (args can be 3 if last is exception)
+    if ((arguments.size() == 2 && arguments.get(0).isBinaryExpr())
+        || (arguments.size() == 3
+            && arguments.get(0).isBinaryExpr()
+            && isException(arguments.get(2)))) {
+      BinaryExpr binaryExpr = arguments.get(0).asBinaryExpr();
+      Optional<Expression> expressionToWrap = findExpressionToWrap(binaryExpr);
+      if (expressionToWrap.isPresent()) {
+        wrapWithNewlineSanitizer(expressionToWrap.get());
+        return SuccessOrReason.success(List.of(DependencyGAV.OWASP_XSS_JAVA_ENCODER));
+      }
+    }
+
+    return SuccessOrReason.reason("Unfixable log call shape");
+  }
+
+  private boolean isException(final Expression expression) {
+    if (expression.isNameExpr()) {
+      try {
+        ResolvedType type = expression.calculateResolvedType();
+        String typeName = type.describe();
+        return isExceptionTypeName(typeName);
+      } catch (Exception e) {
+        Optional<LocalDeclaration> declarationRef =
+            ASTs.findEarliestLocalDeclarationOf(expression, "ex");
+        if (declarationRef.isPresent()) {
+          LocalDeclaration localDeclaration = declarationRef.get();
+          Node declaration = localDeclaration.getDeclaration();
+          // handle if its a parameter or a local variable
+          if (declaration instanceof Parameter param) {
+            String typeAsString = param.getTypeAsString();
+            return isExceptionTypeName(typeAsString);
+          } else if (declaration instanceof VariableDeclarator var) {
+            String typeAsString = var.getTypeAsString();
+            return isExceptionTypeName(typeAsString);
+          }
+        }
+        Optional<Node> nameSourceNodeRef = ASTs.findNonCallableSimpleNameSource(expression, "e");
+        if (nameSourceNodeRef.isPresent()) {
+          Node declaration = nameSourceNodeRef.get();
+          // handle if its a parameter or a local variable
+          if (declaration instanceof Parameter param) {
+            String typeAsString = param.getTypeAsString();
+            return isExceptionTypeName(typeAsString);
+          } else if (declaration instanceof VariableDeclarator var) {
+            String typeAsString = var.getTypeAsString();
+            return isExceptionTypeName(typeAsString);
+          }
+        }
+      }
+    }
+    return false;
+  }
+
+  private static boolean isExceptionTypeName(final String typeName) {
+    return typeName.endsWith("Exception") || typeName.endsWith("Throwable");
+  }
+
+  /**
+   * We handle 4 expression code shapes. <code>
+   *     print(user.getName());
+   *     print("Hello, " + user.getName());
+   *     print(user.getName() + ", hello!");
+   *     print("Hello, " + user.getName() + ", hello!");
+   * </code>
+   *
+   * <p>Note that we should only handle, for the tougher cases, string literals in combination with
+   * the given expression. Note any other combination of expressions.
+   */
+  private static Optional<Expression> findExpressionToWrap(final Expression argument) {
+
+    if (argument.isNameExpr()) {
+      return Optional.of(argument);
+    } else if (argument.isBinaryExpr()) {
+      BinaryExpr binaryExpr = argument.asBinaryExpr();
+      if (binaryExpr.getLeft().isBinaryExpr() && binaryExpr.getRight().isStringLiteralExpr()) {
+        BinaryExpr leftBinaryExpr = binaryExpr.getLeft().asBinaryExpr();
+        if (leftBinaryExpr.getLeft().isStringLiteralExpr()
+            && !leftBinaryExpr.getRight().isStringLiteralExpr()) {
+          return Optional.of(leftBinaryExpr.getRight());
+        }
+      } else if (binaryExpr.getLeft().isStringLiteralExpr()
+          && binaryExpr.getRight().isStringLiteralExpr()) {
+        return Optional.empty();
+      } else if (binaryExpr.getLeft().isStringLiteralExpr()) {
+        return Optional.of(binaryExpr.getRight());
+      } else if (binaryExpr.getRight().isStringLiteralExpr()) {
+        return Optional.of(binaryExpr.getLeft());
+      }
+    }
+    return Optional.empty();
+  }
+
+  private static void wrapWithNewlineSanitizer(final Expression expression) {
+    wrap(expression).withStaticMethod(Newlines.class.getName(), "stripNewLines", true);
+  }
+}

framework/codemodder-base/src/main/resources/generic-remediation-reports/log-injection/description.md

@@ -0,0 +1,12 @@
+This change ensures that log messages can't contain newline characters, leaving you vulnerable to Log Forging / Log Injection.
+
+If malicious users can get newline characters into a log message, they can inject and forge new log entries that look like they came from the server, and trick log analysis tools, administrators, and more. This leads to vulnerabilities like Log Injection, Log Forging, and more attacks from there.
+
+Our change simply strips out newline characters from log messages, ensuring that they can't be used to forge new log entries.
+```diff
++ import io.github.pixee.security.Newlines;
+  ...
+  String orderId = getUserOrderId();
+- log.info("User order ID: " + orderId);
++ log.info("User order ID: " + Newlines.stripNewlines(orderId));
+```

framework/codemodder-base/src/main/resources/generic-remediation-reports/log-injection/report.json

@@ -0,0 +1,6 @@
+{
+  "summary" : "Introduced protections against Log Injection / Forging attacks",
+  "change" : "Added a call to replace any newlines the value",
+  "reviewGuidanceJustification" : "This strips newlines from the value before it is logged, preventing log injection attacks",
+  "references" : ["https://owasp.org/www-community/attacks/Log_Injection", "https://knowledge-base.secureflag.com/vulnerabilities/inadequate_input_validation/log_injection_vulnerability.html", "https://cwe.mitre.org/data/definitions/117.html"]
+}

framework/codemodder-base/src/test/java/io/codemodder/remediation/jndiinjection/DefaultJNDIInjectionRemediatorTest.java

@@ -53,7 +53,6 @@ void it_doesnt_fix_unfixable(
             i -> Optional.empty());
 
     assertThat(result.changes()).isEmpty();
-    ;
 
     List<UnfixedFinding> unfixedFindings = result.unfixedFindings();
     assertThat(unfixedFindings).hasSize(1);