diff --git a/core-codemods/src/main/java/io/codemodder/codemods/DefaultCodemods.java b/core-codemods/src/main/java/io/codemodder/codemods/DefaultCodemods.java index c671672e2..fcd5cfcaf 100644 --- a/core-codemods/src/main/java/io/codemodder/codemods/DefaultCodemods.java +++ b/core-codemods/src/main/java/io/codemodder/codemods/DefaultCodemods.java @@ -56,6 +56,7 @@ public static List> asList() { SemgrepOverlyPermissiveFilePermissionsCodemod.class, SimplifyRestControllerAnnotationsCodemod.class, SubstituteReplaceAllCodemod.class, + SonarXXECodemod.class, SQLParameterizerCodemod.class, SSRFCodemod.class, StackTraceExposureCodemod.class, diff --git a/core-codemods/src/main/java/io/codemodder/codemods/SonarSQLInjectionCodemod.java b/core-codemods/src/main/java/io/codemodder/codemods/SonarSQLInjectionCodemod.java index aba31b659..850102c36 100644 --- a/core-codemods/src/main/java/io/codemodder/codemods/SonarSQLInjectionCodemod.java +++ b/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; } diff --git a/core-codemods/src/main/java/io/codemodder/codemods/SonarXXECodemod.java b/core-codemods/src/main/java/io/codemodder/codemods/SonarXXECodemod.java new file mode 100644 index 000000000..d479765bd --- /dev/null +++ b/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 issuesForFile = issues.getResultsByPath(context.path()); + return remediationStrategy.remediateAll( + cu, + context.path().toString(), + detectorRule(), + issuesForFile, + SonarFinding::getKey, + SonarFinding::getLine, + f -> f.getTextRange().getStartOffset()); + } +} diff --git a/core-codemods/src/main/resources/io/codemodder/codemods/xxe-generic/description.md b/core-codemods/src/main/resources/io/codemodder/codemods/xxe-generic/description.md new file mode 100644 index 000000000..6f4e51779 --- /dev/null +++ b/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 + + ]> + + &xxe; + +``` + +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. diff --git a/core-codemods/src/main/resources/io/codemodder/codemods/xxe-generic/report.json b/core-codemods/src/main/resources/io/codemodder/codemods/xxe-generic/report.json new file mode 100644 index 000000000..b43150d3e --- /dev/null +++ b/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"] +} diff --git a/core-codemods/src/test/java/io/codemodder/codemods/SonarXXECodemodTest.java b/core-codemods/src/test/java/io/codemodder/codemods/SonarXXECodemodTest.java new file mode 100644 index 000000000..a89803d5b --- /dev/null +++ b/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 {} diff --git a/core-codemods/src/test/java/io/codemodder/remediation/xxe/DefaultXXEJavaRemediatorStrategyTest.java b/core-codemods/src/test/java/io/codemodder/remediation/xxe/DefaultXXEJavaRemediatorStrategyTest.java new file mode 100644 index 000000000..4ca6e39c3 --- /dev/null +++ b/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 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 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 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); + } +} diff --git a/core-codemods/src/test/resources/sonar-xxe-s2755/Test.java.after b/core-codemods/src/test/resources/sonar-xxe-s2755/Test.java.after new file mode 100644 index 000000000..2f657dbb6 --- /dev/null +++ b/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(); + } +} diff --git a/core-codemods/src/test/resources/sonar-xxe-s2755/Test.java.before b/core-codemods/src/test/resources/sonar-xxe-s2755/Test.java.before new file mode 100644 index 000000000..f6fba3ae7 --- /dev/null +++ b/core-codemods/src/test/resources/sonar-xxe-s2755/Test.java.before @@ -0,0 +1,77 @@ +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.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(); + 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.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(); + } +} diff --git a/core-codemods/src/test/resources/sonar-xxe-s2755/sonar-issues.json b/core-codemods/src/test/resources/sonar-xxe-s2755/sonar-issues.json new file mode 100644 index 000000000..2138ef5fc --- /dev/null +++ b/core-codemods/src/test/resources/sonar-xxe-s2755/sonar-issues.json @@ -0,0 +1,194 @@ +{ + "total": 11, + "p": 1, + "ps": 100, + "paging": { + "pageIndex": 1, + "pageSize": 100, + "total": 11 + }, + "effortTotal": 115, + "debtTotal": 115, + "issues": [ + { + "key": "AZAzHhmeiBoJ0fAlH956", + "rule": "java:S2755", + "severity": "BLOCKER", + "component": "pixee_bad-java-code:src/main/java/com/acme/XXEVuln.java", + "project": "pixee_bad-java-code", + "line": 53, + "hash": "d79f6dee4e8bcc762ce7d6840b7733ee", + "textRange": { + "startLine": 53, + "endLine": 53, + "startOffset": 48, + "endOffset": 59 + }, + "flows": [], + "status": "OPEN", + "message": "Disable access to external entities in XML parsing.", + "effort": "15min", + "debt": "15min", + "assignee": "nahsra@github", + "author": "arshan.dabirsiaghi@gmail.com", + "tags": [ + "cwe", + "symbolic-execution" + ], + "creationDate": "2024-06-20T02:48:27+0200", + "updateDate": "2024-06-20T02:48:27+0200", + "type": "VULNERABILITY", + "organization": "pixee", + "cleanCodeAttribute": "COMPLETE", + "cleanCodeAttributeCategory": "INTENTIONAL", + "impacts": [ + { + "softwareQuality": "SECURITY", + "severity": "HIGH" + } + ] + }, + { + "key": "AZAzHhmeiBoJ0fAlH957", + "rule": "java:S2755", + "severity": "BLOCKER", + "component": "pixee_bad-java-code:src/main/java/com/acme/XXEVuln.java", + "project": "pixee_bad-java-code", + "line": 62, + "hash": "bca74f470a192baef9933c0a00443593", + "textRange": { + "startLine": 62, + "endLine": 62, + "startOffset": 60, + "endOffset": 71 + }, + "flows": [], + "status": "OPEN", + "message": "Disable access to external entities in XML parsing.", + "effort": "15min", + "debt": "15min", + "assignee": "nahsra@github", + "author": "arshan.dabirsiaghi@gmail.com", + "tags": [ + "cwe", + "symbolic-execution" + ], + "creationDate": "2024-06-20T02:48:27+0200", + "updateDate": "2024-06-20T02:48:27+0200", + "type": "VULNERABILITY", + "organization": "pixee", + "cleanCodeAttribute": "COMPLETE", + "cleanCodeAttributeCategory": "INTENTIONAL", + "impacts": [ + { + "softwareQuality": "SECURITY", + "severity": "HIGH" + } + ] + }, + { + "key": "AZAzHhmeiBoJ0fAlH954", + "rule": "java:S2755", + "severity": "BLOCKER", + "component": "pixee_bad-java-code:src/main/java/com/acme/XXEVuln.java", + "project": "pixee_bad-java-code", + "line": 68, + "hash": "bca74f470a192baef9933c0a00443593", + "textRange": { + "startLine": 68, + "endLine": 68, + "startOffset": 60, + "endOffset": 71 + }, + "flows": [], + "status": "OPEN", + "message": "Disable access to external entities in XML parsing.", + "effort": "15min", + "debt": "15min", + "assignee": "nahsra@github", + "author": "arshan.dabirsiaghi@gmail.com", + "tags": [ + "cwe", + "symbolic-execution" + ], + "creationDate": "2024-06-20T02:48:27+0200", + "updateDate": "2024-06-20T02:48:27+0200", + "type": "VULNERABILITY", + "organization": "pixee", + "cleanCodeAttribute": "COMPLETE", + "cleanCodeAttributeCategory": "INTENTIONAL", + "impacts": [ + { + "softwareQuality": "SECURITY", + "severity": "HIGH" + } + ] + }, + { + "key": "AZAzHhmeiBoJ0fAlH955", + "rule": "java:S2755", + "severity": "BLOCKER", + "component": "pixee_bad-java-code:src/main/java/com/acme/XXEVuln.java", + "project": "pixee_bad-java-code", + "line": 75, + "hash": "9c02741147f1fbd343c736cabf8ceeb6", + "textRange": { + "startLine": 75, + "endLine": 75, + "startOffset": 23, + "endOffset": 38 + }, + "flows": [], + "status": "OPEN", + "message": "Disable access to external entities in XML parsing.", + "effort": "15min", + "debt": "15min", + "assignee": "nahsra@github", + "author": "arshan.dabirsiaghi@gmail.com", + "tags": [ + "cwe", + "symbolic-execution" + ], + "creationDate": "2024-06-20T02:48:27+0200", + "updateDate": "2024-06-20T02:48:27+0200", + "type": "VULNERABILITY", + "organization": "pixee", + "cleanCodeAttribute": "COMPLETE", + "cleanCodeAttributeCategory": "INTENTIONAL", + "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/XXEVuln.java", + "uuid": "AZAycpG1hxTp4FYWZ4N0", + "enabled": true, + "qualifier": "FIL", + "name": "XXEVuln.java", + "longName": "src/main/java/com/acme/XXEVuln.java", + "path": "src/main/java/com/acme/XXEVuln.java" + } + ], + "organizations": [ + { + "key": "pixee", + "name": "Pixee" + } + ], + "facets": [] +} diff --git a/framework/codemodder-base/src/main/java/io/codemodder/CodemodReporterStrategy.java b/framework/codemodder-base/src/main/java/io/codemodder/CodemodReporterStrategy.java index 11f4b3128..bfbedbd05 100644 --- a/framework/codemodder-base/src/main/java/io/codemodder/CodemodReporterStrategy.java +++ b/framework/codemodder-base/src/main/java/io/codemodder/CodemodReporterStrategy.java @@ -11,6 +11,7 @@ import java.util.stream.Collectors; import java.util.stream.StreamSupport; import org.apache.commons.io.IOUtils; +import org.jetbrains.annotations.NotNull; /** A type responsible for reporting codemod changes. */ public interface CodemodReporterStrategy { @@ -73,6 +74,26 @@ static CodemodReporterStrategy fromClasspath(final Class String descriptionResource = "/" + codemodType.getName().replace('.', '/') + "/description.md"; String reportJson = "/" + codemodType.getName().replace('.', '/') + "/report.json"; + return getCodemodReporterStrategy(codemodType, descriptionResource, reportJson); + } + + /** A {@link CodemodReporterStrategy} that reports based on text files in the classpath. */ + static CodemodReporterStrategy fromClasspathDirectory( + final Class codemodType, final String directory) { + Objects.requireNonNull(codemodType); + + // create the expected paths to the files + String descriptionResource = directory + "/description.md"; + String reportJson = directory + "/report.json"; + + return getCodemodReporterStrategy(codemodType, descriptionResource, reportJson); + } + + @NotNull + private static CodemodReporterStrategy getCodemodReporterStrategy( + final Class codemodType, + final String descriptionResource, + final String reportJson) { // load the reporting text ObjectMapper mapper = new ObjectMapper(); final JsonNode parent; @@ -122,6 +143,10 @@ public List getReferences() { }; } + /** + * A reporter strategy that does nothing, useful for tests or CLI situations where reporting isn't + * important. + */ static CodemodReporterStrategy empty() { return new CodemodReporterStrategy() { @Override 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 index 4fb57e3c0..fa659eada 100644 --- a/framework/codemodder-base/src/main/java/io/codemodder/ast/ASTs.java +++ b/framework/codemodder-base/src/main/java/io/codemodder/ast/ASTs.java @@ -1,5 +1,9 @@ package io.codemodder.ast; +import static io.codemodder.javaparser.ASTExpectations.expect; + +import com.github.javaparser.Position; +import com.github.javaparser.ast.CompilationUnit; import com.github.javaparser.ast.Node; import com.github.javaparser.ast.body.BodyDeclaration; import com.github.javaparser.ast.body.ClassOrInterfaceDeclaration; @@ -809,7 +813,7 @@ public static Optional findNonCallableSimpleNameSource( } /** - * Staring from the {@link Node} {@code start}, checks if there exists a local variable + * Starting from the {@link Node} {@code start}, checks if there exists a local variable * declaration whose name is {@code name}. */ public static Optional findEarliestLocalVariableDeclarationOf( @@ -818,7 +822,7 @@ public static Optional findEarliestLocalVariableDeclar } /** - * Staring from the {@link Node} {@code start}, checks if there exists a local variable + * Starting from the {@link Node} {@code start}, checks if there exists a local variable * declaration whose name is {@code name}. */ public static Optional findEarliestLocalDeclarationOf(final SimpleName name) { @@ -826,7 +830,7 @@ public static Optional findEarliestLocalDeclarationOf(final Si } /** - * Staring from the {@link Node} {@code start}, checks if there exists a local variable + * Starting from the {@link Node} {@code start}, checks if there exists a local variable * declaration whose name is {@code name}. */ public static Optional findEarliestLocalDeclarationOf( @@ -869,4 +873,50 @@ public boolean hasNext() { return current.getParentNode().isPresent(); } } + + /** + * This finds all methods that match the given location, with the given name, and is assigned to a + * variable of one of the given types. + */ + public static List findMethodCallsWhichAreAssignedToType( + final CompilationUnit cu, + final int line, + final Integer column, + final String methodName, + final List assignedToTypes) { + List candidateMethods = + cu.findAll(MethodCallExpr.class).stream() + .filter( + m -> + m.getRange() + .isPresent()) // this may be true of nodes we've inserted in a previous + // fix + .filter(m -> m.getRange().get().begin.line == line) + .toList(); + + if (column != null) { + Position reportedPosition = new Position(line, column); + candidateMethods = + candidateMethods.stream() + .filter(m -> m.getRange().get().contains(reportedPosition)) + .toList(); + } + + candidateMethods = + candidateMethods.stream() + .filter(m -> methodName.equals(m.getNameAsString())) + .filter( + m -> { + Optional newFactoryVariableRef = + expect(m).toBeMethodCallExpression().initializingVariable().result(); + if (newFactoryVariableRef.isEmpty()) { + return false; + } + String type = newFactoryVariableRef.get().getTypeAsString(); + return assignedToTypes.contains(type) + || assignedToTypes.stream().anyMatch(type::endsWith); + }) + .toList(); + return candidateMethods; + } } diff --git a/framework/codemodder-base/src/main/java/io/codemodder/javaparser/ASTExpectations.java b/framework/codemodder-base/src/main/java/io/codemodder/javaparser/ASTExpectations.java index 9606faa58..4cf1e963a 100644 --- a/framework/codemodder-base/src/main/java/io/codemodder/javaparser/ASTExpectations.java +++ b/framework/codemodder-base/src/main/java/io/codemodder/javaparser/ASTExpectations.java @@ -305,6 +305,7 @@ public Optional result() { return methodCallExpr; } + /** Return an expectation that asserts the method call has a specific number of arguments. */ public MethodCallExpectation withArgumentsSize(final int expectedSize) { if (methodCallExpr.isEmpty()) { return this; @@ -316,6 +317,7 @@ public MethodCallExpectation withArgumentsSize(final int expectedSize) { return this; } + /** Return an expectation that asserts the method call has a specific name. */ public MethodCallExpectation withName(final String expectedName) { if (methodCallExpr.isEmpty()) { return this; @@ -327,6 +329,7 @@ public MethodCallExpectation withName(final String expectedName) { return this; } + /** Return an expectation that asserts the method call has 1+ arguments. */ public MethodCallExpectation withArguments() { if (methodCallExpr.isEmpty()) { return this; @@ -337,5 +340,21 @@ public MethodCallExpectation withArguments() { } return this; } + + /** Return an expectation that asserts the method call is initializing a variable value. */ + public LocalVariableDeclaratorExpectation initializingVariable() { + if (methodCallExpr.isEmpty()) { + return new LocalVariableDeclaratorExpectation(Optional.empty()); + } + MethodCallExpr call = methodCallExpr.get(); + if (call.getParentNode().isEmpty()) { + return new LocalVariableDeclaratorExpectation(Optional.empty()); + } + Optional initExpr = ASTs.isInitExpr(call); + if (initExpr.isEmpty()) { + return new LocalVariableDeclaratorExpectation(Optional.empty()); + } + return new LocalVariableDeclaratorExpectation(initExpr); + } } } diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/RemediationMessages.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/RemediationMessages.java new file mode 100644 index 000000000..38ad2c5c7 --- /dev/null +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/RemediationMessages.java @@ -0,0 +1,15 @@ +package io.codemodder.remediation; + +/** + * Holds common messages for remediation intended for human consumption as a part of CodeTF + * reporting + */ +public interface RemediationMessages { + + /** No calls at that location */ + String noCallsAtThatLocation = "No calls at that location"; + + /** Multiple calls found at the given location and that may cause confusion */ + String multipleCallsFound = + "Multiple calls found at the given location and that may cause confusion"; +} diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/DefaultXXEJavaRemediatorStrategy.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/DefaultXXEJavaRemediatorStrategy.java new file mode 100644 index 000000000..f09375e0f --- /dev/null +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/DefaultXXEJavaRemediatorStrategy.java @@ -0,0 +1,62 @@ +package io.codemodder.remediation.xxe; + +import com.github.javaparser.ast.CompilationUnit; +import io.codemodder.CodemodChange; +import io.codemodder.CodemodFileScanningResult; +import io.codemodder.codetf.DetectorRule; +import io.codemodder.codetf.FixedFinding; +import io.codemodder.codetf.UnfixedFinding; +import java.util.ArrayList; +import java.util.List; +import java.util.function.Function; + +final class DefaultXXEJavaRemediatorStrategy implements XXEJavaRemediatorStrategy { + + private final List fixers; + + DefaultXXEJavaRemediatorStrategy() { + this.fixers = + List.of( + new DocumentBuilderFactoryAndSAXParserAtCreationFixer(), + new TransformerFactoryAtCreationFixer(), + new XMLReaderAtParseFixer()); + } + + @Override + public CodemodFileScanningResult remediateAll( + final CompilationUnit cu, + final String path, + final DetectorRule detectorRule, + final List issuesForFile, + final Function getKey, + final Function getLine, + final Function getColumn) { + + List unfixedFindings = new ArrayList<>(); + List changes = new ArrayList<>(); + + for (T issue : issuesForFile) { + + String findingId = getKey.apply(issue); + int line = getLine.apply(issue); + Integer column = getColumn.apply(issue); + for (XXEFixer fixer : fixers) { + XXEFixAttempt fixAttempt = fixer.tryFix(issue, line, column, cu); + if (!fixAttempt.isResponsibleFixer()) { + continue; + } + if (fixAttempt.isFixed()) { + CodemodChange change = + CodemodChange.from(line, new FixedFinding(findingId, detectorRule)); + changes.add(change); + } else { + UnfixedFinding unfixedFinding = + new UnfixedFinding(findingId, detectorRule, path, line, fixAttempt.reasonNotFixed()); + unfixedFindings.add(unfixedFinding); + } + } + } + + return CodemodFileScanningResult.from(changes, unfixedFindings); + } +} diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/DocumentBuilderFactoryAndSAXParserAtCreationFixer.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/DocumentBuilderFactoryAndSAXParserAtCreationFixer.java new file mode 100644 index 000000000..0c1b10f19 --- /dev/null +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/DocumentBuilderFactoryAndSAXParserAtCreationFixer.java @@ -0,0 +1,50 @@ +package io.codemodder.remediation.xxe; + +import static io.codemodder.javaparser.ASTExpectations.expect; +import static io.codemodder.remediation.RemediationMessages.multipleCallsFound; +import static io.codemodder.remediation.RemediationMessages.noCallsAtThatLocation; +import static io.codemodder.remediation.xxe.XMLFeatures.addFeatureDisablingStatements; + +import com.github.javaparser.ast.CompilationUnit; +import com.github.javaparser.ast.body.VariableDeclarator; +import com.github.javaparser.ast.expr.MethodCallExpr; +import com.github.javaparser.ast.stmt.Statement; +import io.codemodder.ast.ASTs; +import java.util.List; +import java.util.Optional; + +/** + * Fix XXEs that are reported at the (DocumentBuilderFactory/SAXParserFactory).newInstance() call + * locations. + */ +final class DocumentBuilderFactoryAndSAXParserAtCreationFixer implements XXEFixer { + + @Override + public XXEFixAttempt tryFix( + final T issue, final int line, final Integer column, CompilationUnit cu) { + List candidateMethods = + ASTs.findMethodCallsWhichAreAssignedToType( + cu, line, column, "newInstance", List.of("DocumentBuilderFactory", "SAXParserFactory")); + + if (candidateMethods.isEmpty()) { + return new XXEFixAttempt(false, false, noCallsAtThatLocation); + } else if (candidateMethods.size() > 1) { + return new XXEFixAttempt(false, false, multipleCallsFound); + } + + MethodCallExpr newFactoryInstanceCall = candidateMethods.get(0); + Optional newFactoryVariableRef = + expect(newFactoryInstanceCall).toBeMethodCallExpression().initializingVariable().result(); + VariableDeclarator newFactoryVariable = newFactoryVariableRef.get(); + Optional variableDeclarationStmtRef = + newFactoryVariable.findAncestor(Statement.class); + + if (variableDeclarationStmtRef.isEmpty()) { + return new XXEFixAttempt(true, false, "Not assigned as part of statement"); + } + + Statement statement = variableDeclarationStmtRef.get(); + return addFeatureDisablingStatements( + cu, newFactoryVariable.getNameAsExpression(), statement, false); + } +} diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/TransformerFactoryAtCreationFixer.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/TransformerFactoryAtCreationFixer.java new file mode 100644 index 000000000..46a8bf830 --- /dev/null +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/TransformerFactoryAtCreationFixer.java @@ -0,0 +1,70 @@ +package io.codemodder.remediation.xxe; + +import static io.codemodder.ast.ASTTransforms.addImportIfMissing; +import static io.codemodder.javaparser.ASTExpectations.expect; +import static io.codemodder.remediation.RemediationMessages.*; +import static io.codemodder.remediation.RemediationMessages.multipleCallsFound; + +import com.github.javaparser.ast.CompilationUnit; +import com.github.javaparser.ast.NodeList; +import com.github.javaparser.ast.body.VariableDeclarator; +import com.github.javaparser.ast.expr.FieldAccessExpr; +import com.github.javaparser.ast.expr.MethodCallExpr; +import com.github.javaparser.ast.expr.NameExpr; +import com.github.javaparser.ast.expr.StringLiteralExpr; +import com.github.javaparser.ast.stmt.BlockStmt; +import com.github.javaparser.ast.stmt.ExpressionStmt; +import com.github.javaparser.ast.stmt.Statement; +import io.codemodder.ast.ASTs; +import java.util.List; +import java.util.Optional; + +/** Fix XXEs reported at the TransformerFactory.newInstance() call locations. */ +final class TransformerFactoryAtCreationFixer implements XXEFixer { + + @Override + public XXEFixAttempt tryFix( + final T issue, final int line, final Integer column, CompilationUnit cu) { + List candidateMethods = + ASTs.findMethodCallsWhichAreAssignedToType( + cu, line, column, "newInstance", List.of("TransformerFactory")); + if (candidateMethods.isEmpty()) { + return new XXEFixAttempt(false, false, noCallsAtThatLocation); + } else if (candidateMethods.size() > 1) { + return new XXEFixAttempt(false, false, multipleCallsFound); + } + MethodCallExpr newFactoryInstanceCall = candidateMethods.get(0); + Optional newFactoryVariableRef = + expect(newFactoryInstanceCall).toBeMethodCallExpression().initializingVariable().result(); + VariableDeclarator newFactoryVariable = newFactoryVariableRef.get(); + Optional variableDeclarationStmtRef = + newFactoryVariable.findAncestor(Statement.class); + + if (variableDeclarationStmtRef.isEmpty()) { + return new XXEFixAttempt(true, false, "Not assigned as part of statement"); + } + + Statement statement = variableDeclarationStmtRef.get(); + Optional block = ASTs.findBlockStatementFrom(statement); + if (block.isEmpty()) { + return new XXEFixAttempt(true, false, "No block statement found for newFactory() call"); + } + + BlockStmt blockStmt = block.get(); + MethodCallExpr setAttributeCall = + new MethodCallExpr( + newFactoryVariable.getNameAsExpression(), + "setAttribute", + NodeList.nodeList( + // add field access for javax.xml.XMLConstants.ACCESS_EXTERNAL_DTD + new FieldAccessExpr(new NameExpr("XMLConstants"), "ACCESS_EXTERNAL_DTD"), + new StringLiteralExpr(""))); + + addImportIfMissing(cu, "javax.xml.XMLConstants"); + Statement fixStatement = new ExpressionStmt(setAttributeCall); + NodeList existingStatements = blockStmt.getStatements(); + int index = existingStatements.indexOf(statement); + existingStatements.add(index + 1, fixStatement); + return new XXEFixAttempt(true, true, null); + } +} diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XMLFeatures.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XMLFeatures.java new file mode 100644 index 000000000..8d2aab74d --- /dev/null +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XMLFeatures.java @@ -0,0 +1,67 @@ +package io.codemodder.remediation.xxe; + +import com.github.javaparser.ast.CompilationUnit; +import com.github.javaparser.ast.NodeList; +import com.github.javaparser.ast.expr.BooleanLiteralExpr; +import com.github.javaparser.ast.expr.MethodCallExpr; +import com.github.javaparser.ast.expr.NameExpr; +import com.github.javaparser.ast.expr.StringLiteralExpr; +import com.github.javaparser.ast.stmt.BlockStmt; +import com.github.javaparser.ast.stmt.ExpressionStmt; +import com.github.javaparser.ast.stmt.Statement; +import io.codemodder.ast.ASTs; +import java.util.List; +import java.util.Optional; + +/** Holds shared APIs for working with shared XML feature setting. */ +final class XMLFeatures { + + private XMLFeatures() {} + + /** Creates a call that disables parameter entities. */ + static MethodCallExpr createParameterEntityDisabledCall(final NameExpr nameExpr) { + return new MethodCallExpr( + nameExpr, + "setFeature", + NodeList.nodeList( + new StringLiteralExpr("http://xml.org/sax/features/external-parameter-entities"), + new BooleanLiteralExpr(false))); + } + + /** Creates a call that disables general entities. */ + static MethodCallExpr createGeneralEntityDisablingCall(final NameExpr nameExpr) { + return new MethodCallExpr( + nameExpr, + "setFeature", + NodeList.nodeList( + new StringLiteralExpr("http://xml.org/sax/features/external-general-entities"), + new BooleanLiteralExpr(false))); + } + + static XXEFixAttempt addFeatureDisablingStatements( + final CompilationUnit cu, + final NameExpr variable, + final Statement statementToInjectAround, + final boolean before) { + Optional block = ASTs.findBlockStatementFrom(statementToInjectAround); + if (block.isEmpty()) { + return new XXEFixAttempt(true, false, "No block statement found for newFactory() call"); + } + + BlockStmt blockStmt = block.get(); + MethodCallExpr setFeatureGeneralEntities = createGeneralEntityDisablingCall(variable); + MethodCallExpr setFeatureParameterEntities = createParameterEntityDisabledCall(variable); + List fixStatements = + List.of( + new ExpressionStmt(setFeatureGeneralEntities), + new ExpressionStmt(setFeatureParameterEntities)); + + NodeList existingStatements = blockStmt.getStatements(); + int index = existingStatements.indexOf(statementToInjectAround); + if (!before) { + index++; + } + existingStatements.addAll(index, fixStatements); + return new XXEFixAttempt(true, true, null); + } +} diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XMLReaderAtParseFixer.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XMLReaderAtParseFixer.java new file mode 100644 index 000000000..60d88ced3 --- /dev/null +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XMLReaderAtParseFixer.java @@ -0,0 +1,78 @@ +package io.codemodder.remediation.xxe; + +import static io.codemodder.remediation.RemediationMessages.multipleCallsFound; +import static io.codemodder.remediation.RemediationMessages.noCallsAtThatLocation; + +import com.github.javaparser.Position; +import com.github.javaparser.ast.CompilationUnit; +import com.github.javaparser.ast.Node; +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.ast.nodeTypes.NodeWithType; +import com.github.javaparser.ast.stmt.Statement; +import io.codemodder.ast.ASTs; +import java.util.List; +import java.util.Optional; +import java.util.Set; + +/** Fixes XXEs that are reported at the XMLReader#parse() call. */ +final class XMLReaderAtParseFixer implements XXEFixer { + + @Override + public XXEFixAttempt tryFix( + final T issue, final int line, final Integer column, final CompilationUnit cu) { + List candidateMethods = + cu.findAll(MethodCallExpr.class).stream() + .filter(m -> "parse".equals(m.getNameAsString())) + .filter(m -> m.getScope().isPresent()) + .filter(m -> m.getScope().get().isNameExpr()) + .filter( + m -> { + Optional sourceRef = + ASTs.findNonCallableSimpleNameSource( + m.getScope().get().asNameExpr().getName()); + if (sourceRef.isEmpty()) { + return false; + } + Node source = sourceRef.get(); + if (source instanceof NodeWithType) { + return Set.of("XMLReader", "org.xml.sax.XMLReader") + .contains(((NodeWithType) source).getTypeAsString()); + } + return false; + }) + .filter(m -> m.getRange().isPresent()) + .filter(m -> m.getRange().get().begin.line == line) + .toList(); + + if (column != null) { + Position reportedPosition = new Position(line, column); + candidateMethods = + candidateMethods.stream() + .filter(m -> m.getRange().get().contains(reportedPosition)) + .toList(); + } + + if (candidateMethods.isEmpty()) { + return new XXEFixAttempt(false, false, noCallsAtThatLocation); + } else if (candidateMethods.size() > 1) { + return new XXEFixAttempt(false, false, multipleCallsFound); + } + + MethodCallExpr parseCall = candidateMethods.get(0); + Optional parserRef = parseCall.getScope(); + if (parserRef.isEmpty()) { + return new XXEFixAttempt(false, false, "No scope found for parse() call"); + } else if (!parserRef.get().isNameExpr()) { + return new XXEFixAttempt(false, false, "Scope is not a name expression"); + } + NameExpr parser = parserRef.get().asNameExpr(); + Optional parseStatement = parseCall.findAncestor(Statement.class); + if (parseStatement.isEmpty()) { + return new XXEFixAttempt(true, false, "No statement found for parse() call"); + } + return XMLFeatures.addFeatureDisablingStatements( + cu, parser.asNameExpr(), parseStatement.get(), true); + } +} diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XXEFixAttempt.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XXEFixAttempt.java new file mode 100644 index 000000000..be4b2c75a --- /dev/null +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XXEFixAttempt.java @@ -0,0 +1,14 @@ +package io.codemodder.remediation.xxe; + +/** Represents an attempt to fix an XXE vulnerability. */ +record XXEFixAttempt(boolean isResponsibleFixer, boolean isFixed, String reasonNotFixed) { + + XXEFixAttempt { + if (!isResponsibleFixer && isFixed) { + throw new IllegalStateException("Cannot be fixed by a non-responsible fixer"); + } + if (!isFixed && reasonNotFixed == null) { + throw new IllegalStateException("Reason must be provided if not fixed"); + } + } +} diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XXEFixer.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XXEFixer.java new file mode 100644 index 000000000..e005bcd15 --- /dev/null +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XXEFixer.java @@ -0,0 +1,10 @@ +package io.codemodder.remediation.xxe; + +import com.github.javaparser.ast.CompilationUnit; + +/** Interface for fixing XXEs. */ +interface XXEFixer { + + /** A provider (for a given XML API) attempts to fix the given issue. */ + XXEFixAttempt tryFix(T issue, int line, Integer column, CompilationUnit cu); +} diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XXEJavaRemediatorStrategy.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XXEJavaRemediatorStrategy.java new file mode 100644 index 000000000..f0dde6527 --- /dev/null +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XXEJavaRemediatorStrategy.java @@ -0,0 +1,24 @@ +package io.codemodder.remediation.xxe; + +import com.github.javaparser.ast.CompilationUnit; +import io.codemodder.CodemodFileScanningResult; +import io.codemodder.codetf.DetectorRule; +import java.util.List; +import java.util.function.Function; + +/** Strategy for remediating XXE vulnerabilities using Java's DOM parser. */ +public interface XXEJavaRemediatorStrategy { + + /** A default implementation for callers. */ + XXEJavaRemediatorStrategy DEFAULT = new DefaultXXEJavaRemediatorStrategy(); + + /** Remediate all XXE vulnerabilities in the given compilation unit. */ + CodemodFileScanningResult remediateAll( + CompilationUnit cu, + String string, + DetectorRule detectorRule, + List issuesForFile, + Function getKey, + Function getLine, + Function getColumn); +} diff --git a/framework/codemodder-base/src/test/java/io/codemodder/DefaultCodeDirectoryTest.java b/framework/codemodder-base/src/test/java/io/codemodder/DefaultCodeDirectoryTest.java index 247683736..66e393991 100644 --- a/framework/codemodder-base/src/test/java/io/codemodder/DefaultCodeDirectoryTest.java +++ b/framework/codemodder-base/src/test/java/io/codemodder/DefaultCodeDirectoryTest.java @@ -51,13 +51,13 @@ void it_finds_files(final String givenPath, final String expectedPath) throws IO } else { assertThat(filesWithTrailingPath).isPresent(); Path expected = repoDir.resolve(expectedPath); - assertThat(filesWithTrailingPath.get()).isEqualTo(expected); + assertThat(filesWithTrailingPath).contains(expected); } } private static Stream fileTests() { return Stream.of( - Arguments.of("file1.java", "my/other/test/file1.java"), + Arguments.of("test/file1.java", "my/other/test/file1.java"), Arguments.of("main/file1.java", "src/main/file1.java"), Arguments.of("main//file1.java", "src/main/file1.java"), Arguments.of("main\\file1.java", "src/main/file1.java"), diff --git a/plugins/codemodder-plugin-sonar/src/main/java/io/codemodder/providers/sonar/SonarRemediatingJavaParserChanger.java b/plugins/codemodder-plugin-sonar/src/main/java/io/codemodder/providers/sonar/SonarRemediatingJavaParserChanger.java index 6e1d49d78..eba4768e6 100644 --- a/plugins/codemodder-plugin-sonar/src/main/java/io/codemodder/providers/sonar/SonarRemediatingJavaParserChanger.java +++ b/plugins/codemodder-plugin-sonar/src/main/java/io/codemodder/providers/sonar/SonarRemediatingJavaParserChanger.java @@ -8,12 +8,21 @@ public abstract class SonarRemediatingJavaParserChanger extends JavaParserChanger implements FixOnlyCodeChanger { - protected SonarRemediatingJavaParserChanger(final CodemodReporterStrategy reporter) { + private final boolean shouldRun; + + protected SonarRemediatingJavaParserChanger( + final CodemodReporterStrategy reporter, final RuleFinding findings) { super(reporter); + this.shouldRun = findings.hasResults(); } @Override public String vendorName() { return "Sonar"; } + + @Override + public boolean shouldRun() { + return shouldRun; + } }