diff --git a/core-codemods/src/main/java/io/codemodder/codemods/codeql/CodeQLXXECodemod.java b/core-codemods/src/main/java/io/codemodder/codemods/codeql/CodeQLXXECodemod.java index 7ac1357c3..3e779bbf5 100644 --- a/core-codemods/src/main/java/io/codemodder/codemods/codeql/CodeQLXXECodemod.java +++ b/core-codemods/src/main/java/io/codemodder/codemods/codeql/CodeQLXXECodemod.java @@ -7,7 +7,7 @@ import io.codemodder.providers.sarif.codeql.ProvidedCodeQLScan; import io.codemodder.remediation.GenericRemediationMetadata; import io.codemodder.remediation.Remediator; -import io.codemodder.remediation.xxe.XXEIntermediateXMLStreamReaderRemediator; +import io.codemodder.remediation.xxe.XMLStreamReaderIntermediateRemediator; import java.util.Optional; import javax.inject.Inject; @@ -24,7 +24,7 @@ public final class CodeQLXXECodemod extends CodeQLRemediationCodemod { @Inject public CodeQLXXECodemod(@ProvidedCodeQLScan(ruleId = "java/xxe") final RuleSarif sarif) { super(GenericRemediationMetadata.XXE.reporter(), sarif); - this.remediator = new XXEIntermediateXMLStreamReaderRemediator<>(); + this.remediator = new XMLStreamReaderIntermediateRemediator<>(); } @Override diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/TransformerFactoryAtCreationFixStrategy.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/TransformerFactoryAtCreationFixStrategy.java index b6bd73c3d..fb20610d4 100644 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/TransformerFactoryAtCreationFixStrategy.java +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/TransformerFactoryAtCreationFixStrategy.java @@ -21,7 +21,7 @@ import java.util.Optional; /** - * Fix strategy for XXE vulnerabilities anchored to the TransformerParser newInstance() calls. Finds + * Fix strategy for XXE vulnerabilities anchored to the TransformerParser.newInstance() calls. Finds * the parser's declaration and add statements disabling external entities and features. */ final class TransformerFactoryAtCreationFixStrategy extends MatchAndFixStrategy { @@ -70,7 +70,7 @@ public SuccessOrReason fix(final CompilationUnit cu, final Node node) { } /** - * Matches against TransformerFactory.newInstance() calls + * Matches against XMLInputFactory.newInstance() calls. * * @param node * @return diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XMLInputFactoryAtNewInstanceFixStrategy.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XMLInputFactoryAtNewInstanceFixStrategy.java new file mode 100644 index 000000000..b9a31422c --- /dev/null +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XMLInputFactoryAtNewInstanceFixStrategy.java @@ -0,0 +1,58 @@ +package io.codemodder.remediation.xxe; + +import com.github.javaparser.ast.CompilationUnit; +import com.github.javaparser.ast.Node; +import com.github.javaparser.ast.body.VariableDeclarator; +import com.github.javaparser.ast.expr.Expression; +import com.github.javaparser.ast.expr.MethodCallExpr; +import com.github.javaparser.ast.stmt.Statement; +import io.codemodder.ast.ASTs; +import io.codemodder.remediation.SuccessOrReason; +import java.util.List; +import java.util.Optional; + +final class XMLInputFactoryAtNewInstanceFixStrategy + extends io.codemodder.remediation.MatchAndFixStrategy { + + /** + * Matches (XmlInputFactory | var xif) y = XMLInputFactory.newInstance(), where the node is the + * newDocumentBuilder call. + * + * @param node the node to match + * @return true if this is an assignment to an XMLInputFactory + */ + @Override + public boolean match(final Node node) { + Optional method = + ASTs.isInitializedToType(node, "newInstance", List.of("var", "XMLInputFactory")); + if (method.isEmpty()) { + return false; + } + MethodCallExpr newInstance = method.get(); + Optional scope = newInstance.getScope(); + if (scope.isEmpty()) { + return false; + } + return scope.get().toString().equals("XMLInputFactory"); + } + + @Override + public SuccessOrReason fix(final CompilationUnit cu, final Node node) { + MethodCallExpr newInstance = (MethodCallExpr) node; + Optional initExpr = ASTs.isInitExpr(newInstance); + if (initExpr.isEmpty()) { + return SuccessOrReason.reason("Not an initialization expression"); + } + VariableDeclarator xmlInputFactoryVariable = initExpr.get(); + Optional variableDeclarationStmtRef = + xmlInputFactoryVariable.findAncestor(Statement.class); + + if (variableDeclarationStmtRef.isEmpty()) { + return SuccessOrReason.reason("Not assigned as part of statement"); + } + + Statement stmt = variableDeclarationStmtRef.get(); + return XMLFixBuilder.addXMLInputFactoryDisablingStatement( + xmlInputFactoryVariable.getNameAsExpression(), stmt, false); + } +} diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XXEIntermediateXMLStreamReaderFixStrategy.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XMLStreamReaderIntermediateFixStrategy.java similarity index 93% rename from framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XXEIntermediateXMLStreamReaderFixStrategy.java rename to framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XMLStreamReaderIntermediateFixStrategy.java index 905c6905a..5d672273a 100644 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XXEIntermediateXMLStreamReaderFixStrategy.java +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XMLStreamReaderIntermediateFixStrategy.java @@ -15,7 +15,7 @@ * Fix strategy for XXE vulnerabilities anchored to arguments of a XMLStreamReader calls. Finds the * parser's declaration and add statements disabling external entities and features. */ -final class XXEIntermediateXMLStreamReaderFixStrategy implements RemediationStrategy { +final class XMLStreamReaderIntermediateFixStrategy implements RemediationStrategy { @Override public SuccessOrReason fix(final CompilationUnit cu, final Node node) { @@ -27,7 +27,7 @@ public SuccessOrReason fix(final CompilationUnit cu, final Node node) { if (maybeCall.isEmpty()) { return SuccessOrReason.reason("Not a method call"); } - // get the xmlstreamreader scope variable + // get the XMLStreamReader scope variable MethodCallExpr createXMLStreamReaderCall = maybeCall.get(); Expression xmlStreamReaderScope = createXMLStreamReaderCall.getScope().get(); diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XXEIntermediateXMLStreamReaderRemediator.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XMLStreamReaderIntermediateRemediator.java similarity index 69% rename from framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XXEIntermediateXMLStreamReaderRemediator.java rename to framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XMLStreamReaderIntermediateRemediator.java index d7b06a6c5..cd8c73acc 100644 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XXEIntermediateXMLStreamReaderRemediator.java +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XMLStreamReaderIntermediateRemediator.java @@ -13,11 +13,12 @@ import java.util.Optional; import java.util.function.Function; -public class XXEIntermediateXMLStreamReaderRemediator implements Remediator { +/** Remediator for XXE vulnerabilities anchored to the XMLStreamReader calls. */ +public final class XMLStreamReaderIntermediateRemediator implements Remediator { private final SearcherStrategyRemediator searchStrategyRemediator; - public XXEIntermediateXMLStreamReaderRemediator() { + public XMLStreamReaderIntermediateRemediator() { this.searchStrategyRemediator = new SearcherStrategyRemediator.Builder() .withSearcherStrategyPair( @@ -30,20 +31,20 @@ public XXEIntermediateXMLStreamReaderRemediator() { .filter(Node::hasScope) .isPresent()) .build(), - new XXEIntermediateXMLStreamReaderFixStrategy()) + new XMLStreamReaderIntermediateFixStrategy()) .build(); } @Override public CodemodFileScanningResult remediateAll( - CompilationUnit cu, - String path, - DetectorRule detectorRule, - Collection findingsForPath, - Function findingIdExtractor, - Function findingStartLineExtractor, - Function> findingEndLineExtractor, - Function> findingColumnExtractor) { + final CompilationUnit cu, + final String path, + final DetectorRule detectorRule, + final Collection findingsForPath, + final Function findingIdExtractor, + final Function findingStartLineExtractor, + final Function> findingEndLineExtractor, + final Function> findingColumnExtractor) { return searchStrategyRemediator.remediateAll( cu, path, diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XXERemediator.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XXERemediator.java index fac108846..3c9cada3b 100644 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XXERemediator.java +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XXERemediator.java @@ -29,6 +29,8 @@ public XXERemediator(final NodePositionMatcher matcher) { .withMatchAndFixStrategyAndNodeMatcher( new TransformerFactoryAtCreationFixStrategy(), matcher) .withMatchAndFixStrategyAndNodeMatcher(new XMLReaderAtParseFixStrategy(), matcher) + .withMatchAndFixStrategyAndNodeMatcher( + new XMLInputFactoryAtNewInstanceFixStrategy(), matcher) .build(); } diff --git a/framework/codemodder-base/src/test/java/io/codemodder/remediation/xxe/DefaultXXEIntermediateXMLStreamReaderRemediatorTest.java b/framework/codemodder-base/src/test/java/io/codemodder/remediation/xxe/DefaultXMLStreamReaderIntermediateRemediatorTest.java similarity index 94% rename from framework/codemodder-base/src/test/java/io/codemodder/remediation/xxe/DefaultXXEIntermediateXMLStreamReaderRemediatorTest.java rename to framework/codemodder-base/src/test/java/io/codemodder/remediation/xxe/DefaultXMLStreamReaderIntermediateRemediatorTest.java index a66d4935d..b582e694a 100644 --- a/framework/codemodder-base/src/test/java/io/codemodder/remediation/xxe/DefaultXXEIntermediateXMLStreamReaderRemediatorTest.java +++ b/framework/codemodder-base/src/test/java/io/codemodder/remediation/xxe/DefaultXMLStreamReaderIntermediateRemediatorTest.java @@ -14,14 +14,14 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; -final class DefaultXXEIntermediateXMLStreamReaderRemediatorTest { +final class DefaultXMLStreamReaderIntermediateRemediatorTest { - private XXEIntermediateXMLStreamReaderRemediator remediator; + private XMLStreamReaderIntermediateRemediator remediator; private DetectorRule rule; @BeforeEach void setup() { - remediator = new XXEIntermediateXMLStreamReaderRemediator<>(); + remediator = new XMLStreamReaderIntermediateRemediator<>(); rule = new DetectorRule("xxe", "XXE Fixed At XMLStreamReader", null); } diff --git a/framework/codemodder-base/src/test/java/io/codemodder/remediation/xxe/DefaultXXERemediatorTest.java b/framework/codemodder-base/src/test/java/io/codemodder/remediation/xxe/DefaultXXERemediatorTest.java index 163a4a855..6d46ee273 100644 --- a/framework/codemodder-base/src/test/java/io/codemodder/remediation/xxe/DefaultXXERemediatorTest.java +++ b/framework/codemodder-base/src/test/java/io/codemodder/remediation/xxe/DefaultXXERemediatorTest.java @@ -11,46 +11,153 @@ import io.codemodder.remediation.WithoutScopePositionMatcher; import java.util.List; import java.util.Optional; +import java.util.stream.Stream; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; final class DefaultXXERemediatorTest { private XXERemediator remediator; private DetectorRule rule; + private static Stream fixableSamples() { + return Stream.of( + Arguments.of( + """ + public class MyCode { + public void foo() { + XMLReader parser = null; + DocumentBuilderFactory dbf = null; + StringReader sr = null; + boolean success; + try + { + DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance(); + DocumentBuilder db = dbf.newDocumentBuilder(); + db.parse(new InputSource(sr)); + success = true; + } catch (FileNotFoundException e){ + success = false; + logError(e); + } + } + } + """, + 11, + """ + public class MyCode { + public void foo() { + XMLReader parser = null; + DocumentBuilderFactory dbf = null; + StringReader sr = null; + boolean success; + try + { + 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(); + db.parse(new InputSource(sr)); + success = true; + } catch (FileNotFoundException e){ + success = false; + logError(e); + } + } + } + """), + Arguments.of( + """ + import jakarta.xml.bind.JAXBContext; + import jakarta.xml.bind.JAXBException; + import java.io.StringReader; + import java.time.LocalDateTime; + import java.time.format.DateTimeFormatter; + import java.util.ArrayList; + import java.util.Comparator; + import java.util.HashMap; + import java.util.Map; + import javax.xml.XMLConstants; + import javax.xml.stream.XMLInputFactory; + import javax.xml.stream.XMLStreamException; + import org.owasp.webgoat.container.users.WebGoatUser; + import org.springframework.context.annotation.Scope; + import org.springframework.stereotype.Component; + + public class CommentCreator { + + public Comment comment() { + var jc = JAXBContext.newInstance(Comment.class); + var xif = XMLInputFactory.newInstance(); + + // TODO fix me disabled for now. + if (securityEnabled) { + xif.setProperty(XMLConstants.ACCESS_EXTERNAL_DTD, ""); // Compliant + xif.setProperty(XMLConstants.ACCESS_EXTERNAL_SCHEMA, ""); // compliant + } + + var xsr = xif.createXMLStreamReader(new StringReader(xml)); + + var unmarshaller = jc.createUnmarshaller(); + return (Comment) unmarshaller.unmarshal(xsr); + } + } + """, + 21, + """ + import jakarta.xml.bind.JAXBContext; + import jakarta.xml.bind.JAXBException; + import java.io.StringReader; + import java.time.LocalDateTime; + import java.time.format.DateTimeFormatter; + import java.util.ArrayList; + import java.util.Comparator; + import java.util.HashMap; + import java.util.Map; + import javax.xml.XMLConstants; + import javax.xml.stream.XMLInputFactory; + import javax.xml.stream.XMLStreamException; + import org.owasp.webgoat.container.users.WebGoatUser; + import org.springframework.context.annotation.Scope; + import org.springframework.stereotype.Component; + + public class CommentCreator { + + public Comment comment() { + var jc = JAXBContext.newInstance(Comment.class); + var xif = XMLInputFactory.newInstance(); + xif.setProperty("javax.xml.stream.isSupportingExternalEntities", false); + + // TODO fix me disabled for now. + if (securityEnabled) { + xif.setProperty(XMLConstants.ACCESS_EXTERNAL_DTD, ""); // Compliant + xif.setProperty(XMLConstants.ACCESS_EXTERNAL_SCHEMA, ""); // compliant + } + + var xsr = xif.createXMLStreamReader(new StringReader(xml)); + + var unmarshaller = jc.createUnmarshaller(); + return (Comment) unmarshaller.unmarshal(xsr); + } + } + """)); + } + @BeforeEach void setup() { this.remediator = new XXERemediator<>(new WithoutScopePositionMatcher()); this.rule = new DetectorRule("xxe", "XXE", null); } - @Test - void it_fixes_dbf_at_parse_call() { - String vulnerableCode = - """ - public class MyCode { - public void foo() { - XMLReader parser = null; - DocumentBuilderFactory dbf = null; - StringReader sr = null; - boolean success; - try - { - DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance(); - DocumentBuilder db = dbf.newDocumentBuilder(); - db.parse(new InputSource(sr)); - success = true; - } catch (FileNotFoundException e){ - success = false; - logError(e); - } - } - } - """; + @ParameterizedTest + @MethodSource("fixableSamples") + void it_fixes_dbf_at_parse_call(String beforeCode, int line, String afterCode) { - List findings = List.of(new XXEFinding("foo", 11, 15)); - CompilationUnit cu = StaticJavaParser.parse(vulnerableCode); + List findings = List.of(new XXEFinding("foo", line, null)); + CompilationUnit cu = StaticJavaParser.parse(beforeCode); LexicalPreservingPrinter.setup(cu); CodemodFileScanningResult result = remediator.remediateAll( @@ -65,34 +172,10 @@ public void foo() { assertThat(result.unfixedFindings()).isEmpty(); assertThat(result.changes()).hasSize(1); CodemodChange change = result.changes().get(0); - assertThat(change.lineNumber()).isEqualTo(11); - - String fixedCode = - """ - public class MyCode { - public void foo() { - XMLReader parser = null; - DocumentBuilderFactory dbf = null; - StringReader sr = null; - boolean success; - try - { - 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(); - db.parse(new InputSource(sr)); - success = true; - } catch (FileNotFoundException e){ - success = false; - logError(e); - } - } - } - """; + assertThat(change.lineNumber()).isEqualTo(line); String actualCode = LexicalPreservingPrinter.print(cu); - assertThat(actualCode).isEqualToIgnoringCase(fixedCode); + assertThat(actualCode).isEqualToIgnoringCase(afterCode); } @Test