Fix Sonar XXE (#387)

Fixes some cases of XXE identified by Sonar.

---------

Co-authored-by: pixeebot[bot] <104101892+pixeebot[bot]@users.noreply.github.com>

Author: nahsra

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

@@ -56,6 +56,7 @@ public static List<Class<? extends CodeChanger>> asList() {
         SemgrepOverlyPermissiveFilePermissionsCodemod.class,
         SimplifyRestControllerAnnotationsCodemod.class,
         SubstituteReplaceAllCodemod.class,
+        SonarXXECodemod.class,
         SQLParameterizerCodemod.class,
         SSRFCodemod.class,
         StackTraceExposureCodemod.class,

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

@@ -26,7 +26,7 @@ public final class SonarSQLInjectionCodemod extends SonarRemediatingJavaParserCh
   @Inject
   public SonarSQLInjectionCodemod(
       @ProvidedSonarScan(ruleId = "java:S2077") final RuleHotspot hotspots) {
-    super(CodemodReporterStrategy.fromClasspath(SQLParameterizerCodemod.class));
+    super(CodemodReporterStrategy.fromClasspath(SQLParameterizerCodemod.class), hotspots);
     this.hotspots = Objects.requireNonNull(hotspots);
     this.remediationStrategy = JavaParserSQLInjectionRemediatorStrategy.DEFAULT;
   }

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

@@ -0,0 +1,56 @@
+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.xxe.XXEJavaRemediatorStrategy;
+import io.codemodder.sonar.model.Issue;
+import io.codemodder.sonar.model.SonarFinding;
+import java.util.List;
+import java.util.Objects;
+import javax.inject.Inject;
+
+@Codemod(
+    id = "sonar:java/xxe-2755",
+    reviewGuidance = ReviewGuidance.MERGE_AFTER_CURSORY_REVIEW,
+    importance = Importance.HIGH,
+    executionPriority = CodemodExecutionPriority.HIGH)
+public final class SonarXXECodemod extends SonarRemediatingJavaParserChanger {
+
+  private final XXEJavaRemediatorStrategy remediationStrategy;
+  private final RuleIssue issues;
+
+  @Inject
+  public SonarXXECodemod(@ProvidedSonarScan(ruleId = "java:S2755") final RuleIssue issues) {
+    super(
+        CodemodReporterStrategy.fromClasspathDirectory(SonarXXECodemod.class, "xxe-generic"),
+        issues);
+    this.issues = Objects.requireNonNull(issues);
+    this.remediationStrategy = XXEJavaRemediatorStrategy.DEFAULT;
+  }
+
+  @Override
+  public DetectorRule detectorRule() {
+    return new DetectorRule(
+        "java:S2755",
+        "XML parsers should not be vulnerable to XXE attacks",
+        "https://rules.sonarsource.com/c/type/Vulnerability/RSPEC-2755/");
+  }
+
+  @Override
+  public CodemodFileScanningResult visit(
+      final CodemodInvocationContext context, final CompilationUnit cu) {
+    List<Issue> issuesForFile = issues.getResultsByPath(context.path());
+    return remediationStrategy.remediateAll(
+        cu,
+        context.path().toString(),
+        detectorRule(),
+        issuesForFile,
+        SonarFinding::getKey,
+        SonarFinding::getLine,
+        f -> f.getTextRange().getStartOffset());
+  }
+}

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

@@ -0,0 +1,13 @@
+This change prevents XML parsing APIs from resolving external entities, which can protect you from arbitrary code execution, sensitive data exfiltration, and probably a bunch more evil things attackers are still discovering.
+
+Without this protection, attackers can cause your parser to retrieve sensitive information with attacks like this:
+
+```xml
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE foo [ <!ENTITY xxe SYSTEM "file:///etc/passwd"> ]>
+<book>
+    <title>&xxe;</title>
+</book>
+```
+
+Yes, it's pretty insane that this is the default behavior. Our change hardens the factories created with the necessary security features to prevent your parser from resolving external entities.

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

@@ -0,0 +1,6 @@
+{
+  "summary" : "Introduced protections against XXE attacks",
+  "change" : "Hardened the XML processor to prevent external entities from being resolved, which can prevent data exfiltration and arbitrary code execution",
+  "reviewGuidanceIJustification" : "We believe this change is safe and effective. The behavior of hardened XML readers will only be different if the XML they process uses external entities, which is exceptionally rare (and, as demonstrated, quite unsafe anyway.)",
+  "references" : ["https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html", "https://owasp.org/www-community/vulnerabilities/XML_External_Entity_(XXE)_Processing", "https://github.com/swisskyrepo/PayloadsAllTheThings/blob/master/XXE%20Injection/README.md"]
+}

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

@@ -0,0 +1,12 @@
+package io.codemodder.codemods;
+
+import io.codemodder.testutils.CodemodTestMixin;
+import io.codemodder.testutils.Metadata;
+
+@Metadata(
+    codemodType = SonarXXECodemod.class,
+    testResourceDir = "sonar-xxe-s2755",
+    renameTestFile = "src/main/java/com/acme/XXEVuln.java",
+    expectingFailedFixesAtLines = {62},
+    dependencies = {})
+final class SonarXXECodemodTest implements CodemodTestMixin {}

core-codemods/src/test/java/io/codemodder/remediation/xxe/DefaultXXEJavaRemediatorStrategyTest.java

@@ -0,0 +1,194 @@
+package io.codemodder.remediation.xxe;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import com.github.javaparser.StaticJavaParser;
+import com.github.javaparser.ast.CompilationUnit;
+import com.github.javaparser.printer.lexicalpreservation.LexicalPreservingPrinter;
+import io.codemodder.CodemodChange;
+import io.codemodder.CodemodFileScanningResult;
+import io.codemodder.codetf.DetectorRule;
+import java.util.List;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+final class DefaultXXEJavaRemediatorStrategyTest {
+
+  private DefaultXXEJavaRemediatorStrategy remediator;
+  private DetectorRule rule;
+
+  @BeforeEach
+  void setup() {
+    this.remediator = new DefaultXXEJavaRemediatorStrategy();
+    this.rule = new DetectorRule("xxe", "XXE", null);
+  }
+
+  private static class Finding {
+    private final String key;
+    private final int line;
+    private final int column;
+
+    Finding(String key, int line, int column) {
+      this.key = key;
+      this.line = line;
+      this.column = column;
+    }
+
+    int getLine() {
+      return line;
+    }
+
+    String getKey() {
+      return key;
+    }
+
+    int getColumn() {
+      return column;
+    }
+  }
+
+  @Test
+  void it_doesnt_fix_unknown_parser() {
+    String vulnerableCode =
+        """
+            public class MyCode {
+              public void foo() {
+                    SomeOtherXMLThing parser = null;
+                    DocumentBuilderFactory dbf = null;
+                    StringReader sr = null;
+                    boolean success;
+                    try
+                    {
+                        parser = XMLReaderFactory.createXMLReader("org.apache.xerces.parsers.SAXParser");
+                        parser.setFeature(VALIDATION, true);
+                        parser.setErrorHandler(new MyErrorHandler());
+                        parser.setProperty(JAXP_SCHEMA_SOURCE, new File(schemaName));
+                        sr = new StringReader(str);
+                        parser.parse(new InputSource(sr));
+                        success = true;
+                    } catch (FileNotFoundException e){
+                        success = false;
+                        logError(e);
+                    }
+              }
+            }
+            """;
+
+    List<Finding> findings = List.of(new Finding("foo", 14, 19));
+    CompilationUnit cu = StaticJavaParser.parse(vulnerableCode);
+    LexicalPreservingPrinter.setup(cu);
+    CodemodFileScanningResult result =
+        remediator.remediateAll(
+            cu, "foo", rule, findings, Finding::getKey, Finding::getLine, Finding::getColumn);
+    assertThat(result.changes()).isEmpty();
+    assertThat(result.unfixedFindings()).isEmpty();
+  }
+
+  @Test
+  void it_fixes_xmlreaders_at_parse_call() {
+    String vulnerableCode =
+        """
+            public class MyCode {
+              public void foo() {
+                    XMLReader parser = null;
+                    DocumentBuilderFactory dbf = null;
+                    StringReader sr = null;
+                    boolean success;
+                    try
+                    {
+                        parser = XMLReaderFactory.createXMLReader("org.apache.xerces.parsers.SAXParser");
+                        parser.setFeature(VALIDATION, true);
+                        parser.setErrorHandler(new MyErrorHandler());
+                        parser.setProperty(JAXP_SCHEMA_SOURCE, new File(schemaName));
+                        sr = new StringReader(str);
+                        parser.parse(new InputSource(sr));
+                        success = true;
+                    } catch (FileNotFoundException e){
+                        success = false;
+                        logError(e);
+                    }
+              }
+            }
+            """;
+
+    List<Finding> findings = List.of(new Finding("foo", 14, 19));
+    CompilationUnit cu = StaticJavaParser.parse(vulnerableCode);
+    LexicalPreservingPrinter.setup(cu);
+    CodemodFileScanningResult result =
+        remediator.remediateAll(
+            cu, "foo", rule, findings, Finding::getKey, Finding::getLine, Finding::getColumn);
+    assertThat(result.unfixedFindings()).isEmpty();
+    assertThat(result.changes()).hasSize(1);
+    CodemodChange change = result.changes().get(0);
+    assertThat(change.lineNumber()).isEqualTo(14);
+
+    String fixedCode =
+        """
+                    public class MyCode {
+                      public void foo() {
+                            XMLReader parser = null;
+                            DocumentBuilderFactory dbf = null;
+                            StringReader sr = null;
+                            boolean success;
+                            try
+                            {
+                                parser = XMLReaderFactory.createXMLReader("org.apache.xerces.parsers.SAXParser");
+                                parser.setFeature(VALIDATION, true);
+                                parser.setErrorHandler(new MyErrorHandler());
+                                parser.setProperty(JAXP_SCHEMA_SOURCE, new File(schemaName));
+                                sr = new StringReader(str);
+                                parser.setFeature("http://xml.org/sax/features/external-general-entities", false);
+                                parser.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
+                                parser.parse(new InputSource(sr));
+                                success = true;
+                            } catch (FileNotFoundException e){
+                                success = false;
+                                logError(e);
+                            }
+                      }
+                    }
+                    """;
+
+    String actualCode = LexicalPreservingPrinter.print(cu);
+    assertThat(actualCode).isEqualToIgnoringCase(fixedCode);
+  }
+
+  @Test
+  void it_fixes_transformers() {
+    String vulnerableCode =
+        """
+            public class MyCode {
+              public void foo() {
+                TransformerFactory factory = TransformerFactory.newInstance();
+                factory.newTransformer().transform(new StreamSource(new StringReader(xml)), new StreamResult(new StringWriter()));
+              }
+            }
+            """;
+    List<Finding> findings = List.of(new Finding("foo", 3, 52));
+    CompilationUnit cu = StaticJavaParser.parse(vulnerableCode);
+    LexicalPreservingPrinter.setup(cu);
+    CodemodFileScanningResult result =
+        remediator.remediateAll(
+            cu, "foo", rule, findings, Finding::getKey, Finding::getLine, Finding::getColumn);
+    assertThat(result.unfixedFindings()).isEmpty();
+    assertThat(result.changes()).hasSize(1);
+    CodemodChange change = result.changes().get(0);
+    assertThat(change.lineNumber()).isEqualTo(3);
+
+    String fixedCode =
+        """
+            import javax.xml.XMLConstants;
+
+            public class MyCode {
+              public void foo() {
+                TransformerFactory factory = TransformerFactory.newInstance();
+                factory.setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, "");
+                factory.newTransformer().transform(new StreamSource(new StringReader(xml)), new StreamResult(new StringWriter()));
+              }
+            }
+            """;
+
+    String actualCode = LexicalPreservingPrinter.print(cu);
+    assertThat(actualCode).isEqualToIgnoringCase(fixedCode);
+  }
+}

core-codemods/src/test/resources/sonar-xxe-s2755/Test.java.after

@@ -0,0 +1,83 @@
+package com.acme;
+
+import org.w3c.dom.Document;
+import org.xml.sax.InputSource;
+import org.xml.sax.SAXException;
+import org.xml.sax.XMLReader;
+import org.xml.sax.helpers.XMLReaderFactory;
+
+import javax.xml.parsers.*;
+import javax.xml.transform.Transformer;
+import javax.xml.transform.TransformerException;
+import javax.xml.transform.TransformerFactory;
+import javax.xml.transform.dom.DOMSource;
+import javax.xml.transform.stream.StreamResult;
+import java.io.IOException;
+import java.io.StringReader;
+import java.io.StringWriter;
+import java.sql.Connection;
+import java.sql.DriverManager;
+import java.sql.SQLException;
+
+/** Holds various XXE vulns for different APIs. */
+public class XXEVuln {
+
+    public static void main(String[] args) throws TransformerException, ParserConfigurationException, IOException, SAXException, SQLException {
+        docToString(null);
+        saxTransformer(args[0]);
+        withDom(args[1]);
+        withDomButDisabled(args[2]);
+        withReaderFactory(null);
+
+        String sql = "select * from users where name= '" + args[0] + "'";
+        Connection conn = DriverManager.getConnection("jdbc:mysql://localhost/test");
+        conn.createStatement().executeQuery(sql);
+    }
+
+    public static String docToString(final Document poDocument) throws TransformerException {
+        if(true) {
+            int a = 1;
+            return "foo";
+        }
+
+        TransformerFactory transformerFactory = TransformerFactory.newInstance();
+        Transformer transformer = transformerFactory.newTransformer();
+        DOMSource domSrc = new DOMSource(poDocument);
+        StringWriter sw = new StringWriter();
+        StreamResult result = new StreamResult(sw);
+        transformer.transform(domSrc, result);
+        return sw.toString();
+    }
+
+    public static void saxTransformer(String xml) throws ParserConfigurationException, SAXException, IOException {
+        SAXParserFactory spf = SAXParserFactory.newInstance();
+        spf.setFeature("http://xml.org/sax/features/external-general-entities", false);
+        spf.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
+        spf.setValidating(true);
+
+        SAXParser saxParser = spf.newSAXParser();
+        XMLReader xmlReader = saxParser.getXMLReader();
+        xmlReader.parse(new InputSource(new StringReader(xml)));
+    }
+
+    public static Document withDom(String xml) throws ParserConfigurationException, IOException, SAXException {
+        DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance();
+        dbf.setFeature("http://xml.org/sax/features/external-general-entities", false);
+        dbf.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
+        DocumentBuilder db = dbf.newDocumentBuilder();
+        return db.parse(new InputSource(new StringReader(xml)));
+    }
+
+    public static Document withDomButDisabled(String xml) throws ParserConfigurationException, IOException, SAXException {
+        DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance();
+        dbf.setFeature("http://xml.org/sax/features/external-general-entities", false);
+        dbf.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
+        dbf.setExpandEntityReferences(true);
+        DocumentBuilder db = dbf.newDocumentBuilder();
+        return db.parse(new InputSource(new StringReader(xml)));
+    }
+
+    public static XMLReader withReaderFactory(XMLReaderFactory factory) throws ParserConfigurationException, IOException, SAXException {
+        return factory.createXMLReader();
+    }
+}