✨ New Sonar Remediation Codemod for Unsafe Reflection (#390)

Remediates Sonar findings of type java:S2658.

Does not yet cover edge-cases, but I'd like to get some feedback on the
happy path while I work on those edge cases.

/towards #work

---------

Co-authored-by: Arshan Dabirsiaghi <arshan.dabirsiaghi@gmail.com>

Author: gilday

core-codemods/build.gradle.kts

@@ -38,6 +38,7 @@ dependencies {
     testImplementation(project(":framework:codemodder-testutils-llm"))
     testRuntimeOnly(testlibs.junit.jupiter.engine)
     testImplementation(testlibs.jgit)
+    testImplementation(testlibs.assertj)
     testImplementation("org.testcontainers:testcontainers:1.19.0")
 }
 

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

@@ -63,6 +63,7 @@ public static List<Class<? extends CodeChanger>> asList() {
         SwitchLiteralFirstComparisonsCodemod.class,
         SwitchToStandardCharsetsCodemod.class,
         UnverifiedJwtCodemod.class,
+        SonarUnsafeReflectionRemediationCodemod.class,
         UpgradeSSLContextTLSCodemod.class,
         UpgradeSSLEngineTLSCodemod.class,
         UpgradeSSLParametersTLSCodemod.class,

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

@@ -0,0 +1,55 @@
+package io.codemodder.codemods;
+
+import com.github.javaparser.ast.CompilationUnit;
+import io.codemodder.*;
+import io.codemodder.codetf.DetectorRule;
+import io.codemodder.providers.sonar.ProvidedSonarScan;
+import io.codemodder.providers.sonar.RuleIssue;
+import io.codemodder.providers.sonar.SonarRemediatingJavaParserChanger;
+import io.codemodder.remediation.GenericVulnerabilityReporterStrategies;
+import io.codemodder.remediation.reflectioninjection.ReflectionInjectionRemediator;
+import io.codemodder.sonar.model.Issue;
+import java.util.Objects;
+import javax.inject.Inject;
+
+/** Sonar remediation codemod for S2658: Classes should not be loaded dynamically. */
+@Codemod(
+    id = "sonar:java/unsafe-reflection-s2658",
+    reviewGuidance = ReviewGuidance.MERGE_WITHOUT_REVIEW,
+    importance = Importance.HIGH,
+    executionPriority = CodemodExecutionPriority.HIGH)
+public final class SonarUnsafeReflectionRemediationCodemod
+    extends SonarRemediatingJavaParserChanger {
+
+  private final ReflectionInjectionRemediator remediator;
+  private final RuleIssue issues;
+
+  @Inject
+  public SonarUnsafeReflectionRemediationCodemod(
+      @ProvidedSonarScan(ruleId = "java:S2658") final RuleIssue issues) {
+    super(GenericVulnerabilityReporterStrategies.reflectionInjectionStrategy, issues);
+    this.remediator = ReflectionInjectionRemediator.DEFAULT;
+    this.issues = Objects.requireNonNull(issues);
+  }
+
+  @Override
+  public DetectorRule detectorRule() {
+    return new DetectorRule(
+        "java:S2658",
+        "Classes should not be loaded dynamically",
+        "https://rules.sonarsource.com/java/RSPEC-2658/");
+  }
+
+  @Override
+  public CodemodFileScanningResult visit(
+      final CodemodInvocationContext context, final CompilationUnit cu) {
+    return remediator.remediateAll(
+        cu,
+        context.path().toString(),
+        detectorRule(),
+        issues.getResultsByPath(context.path()),
+        Issue::getKey,
+        Issue::getLine,
+        i -> i.getTextRange().getStartOffset());
+  }
+}

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

@@ -0,0 +1,10 @@
+This change fixes Sonar's [Classes should not be loaded dynamically](https://rules.sonarsource.com/java/RSPEC-2658/) issue by replacing the unsafe class load with a hardened alternative.
+
+The hardened alternative blocks the loading of classes that are well-known to be used by attackers to exploit the application.
+
+Our changes look something like this:
+
+```diff
+- Class<MyStrategy> clazz = Class.forName(untrustedInput);
++ Class<MyStrategy> clazz = Reflection.loadAndVerify(untrustedInput);
+```

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

@@ -0,0 +1,8 @@
+{
+  "summary": "Replaced unsafe usages of `Class.forName` with hardened alternative `Reflection.loadAndVerify`",
+  "change": "Replaced unsafe usages of `Class.forName` with hardened alternative `Reflection.loadAndVerify`",
+  "reviewGuidanceJustification": "Reflection.loadAndVerify disallows the loading of classes that are well-known to be dangerous paths to remote code execution. Pathological cases aside, the use of Reflection.loadAndVerify will not disrupt the typical operations of an application.",
+  "references": [
+    "https://rules.sonarsource.com/java/RSPEC-2658/"
+  ]
+}

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

@@ -0,0 +1,21 @@
+package io.codemodder.codemods;
+
+import io.codemodder.DependencyGAV;
+import io.codemodder.testutils.CodemodTestMixin;
+import io.codemodder.testutils.Metadata;
+
+/**
+ * Test for {@link SonarUnsafeReflectionRemediationCodemod}.
+ *
+ * <ul>
+ *   <li>Fully qualified class name {@code java.lang.Class.forName(String)}
+ *   <li>static import {@code forName(String)}
+ * </ul>
+ */
+@Metadata(
+    codemodType = SonarUnsafeReflectionRemediationCodemod.class,
+    testResourceDir = "unsafe-reflection-s2658",
+    renameTestFile = "src/main/java/com/acme/reflection/UnsafeReflection.java",
+    expectingFixesAtLines = {25},
+    dependencies = DependencyGAV.JAVA_SECURITY_TOOLKIT_GAV)
+final class SonarUnsafeReflectionRemediationCodemodTest implements CodemodTestMixin {}

core-codemods/src/test/resources/unsafe-reflection-s2658/Test.java.after

@@ -0,0 +1,48 @@
+package com.acme.reflection;
+
+import io.github.pixee.security.Reflection;
+import jakarta.ws.rs.GET;
+import jakarta.ws.rs.Path;
+import jakarta.ws.rs.QueryParam;
+import java.lang.reflect.Constructor;
+import java.lang.reflect.InvocationTargetException;
+
+/**
+ * Hello world resource that uses unsafe reflection to load a translator by a name controllable by
+ * the end-user.
+ */
+@Path("/unsafe-reflection")
+public class UnsafeReflection {
+
+  @GET
+  public String hello(@QueryParam("translator") final String translationStrategy) {
+    final var translator = loadTranslatorByName(translationStrategy);
+    return translator.translate("Hello, world!");
+  }
+
+  private static TranslatorStrategy loadTranslatorByName(final String translationStrategy) {
+    final Class<?> translatorClazz;
+    try {
+      translatorClazz = Reflection.loadAndVerify("com.acme." + translationStrategy);
+    } catch (ClassNotFoundException e) {
+      throw new IllegalArgumentException("Invalid translator: " + translationStrategy, e);
+    }
+    if (TranslatorStrategy.class.isAssignableFrom(translatorClazz)) {
+      throw new IllegalArgumentException("Invalid translator: " + translationStrategy);
+    }
+    final Constructor<?> translatorCtor;
+    try {
+      translatorCtor = translatorClazz.getConstructor();
+    } catch (NoSuchMethodException e) {
+      throw new IllegalStateException(
+          "Translator " + translationStrategy + " is missing a no-args constructor", e);
+    }
+    final TranslatorStrategy translator;
+    try {
+      translator = (TranslatorStrategy) translatorCtor.newInstance();
+    } catch (InstantiationException | IllegalAccessException | InvocationTargetException e) {
+      throw new IllegalStateException("Failed to initialize translator " + translationStrategy, e);
+    }
+    return translator;
+  }
+}

core-codemods/src/test/resources/unsafe-reflection-s2658/Test.java.before

@@ -0,0 +1,47 @@
+package com.acme.reflection;
+
+import jakarta.ws.rs.GET;
+import jakarta.ws.rs.Path;
+import jakarta.ws.rs.QueryParam;
+import java.lang.reflect.Constructor;
+import java.lang.reflect.InvocationTargetException;
+
+/**
+ * Hello world resource that uses unsafe reflection to load a translator by a name controllable by
+ * the end-user.
+ */
+@Path("/unsafe-reflection")
+public class UnsafeReflection {
+
+  @GET
+  public String hello(@QueryParam("translator") final String translationStrategy) {
+    final var translator = loadTranslatorByName(translationStrategy);
+    return translator.translate("Hello, world!");
+  }
+
+  private static TranslatorStrategy loadTranslatorByName(final String translationStrategy) {
+    final Class<?> translatorClazz;
+    try {
+      translatorClazz = Class.forName("com.acme." + translationStrategy);
+    } catch (ClassNotFoundException e) {
+      throw new IllegalArgumentException("Invalid translator: " + translationStrategy, e);
+    }
+    if (TranslatorStrategy.class.isAssignableFrom(translatorClazz)) {
+      throw new IllegalArgumentException("Invalid translator: " + translationStrategy);
+    }
+    final Constructor<?> translatorCtor;
+    try {
+      translatorCtor = translatorClazz.getConstructor();
+    } catch (NoSuchMethodException e) {
+      throw new IllegalStateException(
+          "Translator " + translationStrategy + " is missing a no-args constructor", e);
+    }
+    final TranslatorStrategy translator;
+    try {
+      translator = (TranslatorStrategy) translatorCtor.newInstance();
+    } catch (InstantiationException | IllegalAccessException | InvocationTargetException e) {
+      throw new IllegalStateException("Failed to initialize translator " + translationStrategy, e);
+    }
+    return translator;
+  }
+}

core-codemods/src/test/resources/unsafe-reflection-s2658/sonar-issues.json

@@ -0,0 +1,76 @@
+{
+  "total": 1,
+  "p": 1,
+  "ps": 100,
+  "paging": {
+    "pageIndex": 1,
+    "pageSize": 100,
+    "total": 1
+  },
+  "effortTotal": 45,
+  "debtTotal": 45,
+  "issues": [
+    {
+      "key": "AZA29hmxtpn0rIhsMyGN",
+      "rule": "java:S2658",
+      "severity": "CRITICAL",
+      "component": "pixee_bad-java-code:src/main/java/com/acme/reflection/UnsafeReflection.java",
+      "project": "pixee_bad-java-code",
+      "line": 25,
+      "hash": "a156d32a33f23ec9cd7fb3dcbbcebf08",
+      "textRange": {
+        "startLine": 25,
+        "endLine": 25,
+        "startOffset": 30,
+        "endOffset": 37
+      },
+      "flows": [],
+      "status": "OPEN",
+      "message": "Remove this use of dynamic class loading.",
+      "effort": "45min",
+      "debt": "45min",
+      "author": "johnathan.gilday@pixee.ai",
+      "tags": [],
+      "creationDate": "2024-06-20T19:50:19+0200",
+      "updateDate": "2024-06-20T20:43:13+0200",
+      "type": "VULNERABILITY",
+      "organization": "pixee",
+      "cleanCodeAttribute": "CONVENTIONAL",
+      "cleanCodeAttributeCategory": "CONSISTENT",
+      "impacts": [
+        {
+          "softwareQuality": "SECURITY",
+          "severity": "HIGH"
+        }
+      ]
+    }
+  ],
+  "components": [
+    {
+      "organization": "pixee",
+      "key": "pixee_bad-java-code",
+      "uuid": "AZAybnMG2i3IMwLpnrFI",
+      "enabled": true,
+      "qualifier": "TRK",
+      "name": "my-bad-code",
+      "longName": "my-bad-code"
+    },
+    {
+      "organization": "pixee",
+      "key": "pixee_bad-java-code:src/main/java/com/acme/reflection/UnsafeReflection.java",
+      "uuid": "AZA2zRFOgsPIvFR8KBcb",
+      "enabled": true,
+      "qualifier": "FIL",
+      "name": "UnsafeReflection.java",
+      "longName": "src/main/java/com/acme/reflection/UnsafeReflection.java",
+      "path": "src/main/java/com/acme/reflection/UnsafeReflection.java"
+    }
+  ],
+  "organizations": [
+    {
+      "key": "pixee",
+      "name": "Pixee"
+    }
+  ],
+  "facets": []
+}

framework/codemodder-base/src/main/java/io/codemodder/RegionNodeMatcher.java

@@ -34,4 +34,19 @@ public interface RegionNodeMatcher {
               && region.start().column() == range.begin.column;
 
   RegionNodeMatcher MATCHES_LINE = (region, range) -> region.start().line() == range.begin.line;
+
+  /**
+   * Return true when the given {@link Region} is inside the given {@link Range}.
+   *
+   * <p>This may be used when the SARIF provider reports a region that is smaller than expected
+   * (e.g. only the method name instead of the whole method expression). In these cases, the codemod
+   * developer should take extra care to make sure the method expression matches expectations,
+   * because this matcher may match multiple nodes erroneously.
+   */
+  RegionNodeMatcher REGION_INSIDE_RANGE =
+      (region, range) ->
+          region.start().line() >= range.begin.line
+              && region.start().column() >= range.begin.column
+              && region.end().line() <= range.end.line
+              && region.end().column() <= range.end.column;
 }