diff --git a/src/main/java/org/apache/commons/jxpath/XMLDocumentContainer.java b/src/main/java/org/apache/commons/jxpath/XMLDocumentContainer.java index 04da18a17..d3bc63139 100644 --- a/src/main/java/org/apache/commons/jxpath/XMLDocumentContainer.java +++ b/src/main/java/org/apache/commons/jxpath/XMLDocumentContainer.java @@ -20,6 +20,7 @@ import java.net.URL; import java.util.Objects; +import javax.xml.XMLConstants; import javax.xml.transform.Source; import javax.xml.transform.Transformer; import javax.xml.transform.TransformerFactory; @@ -85,7 +86,11 @@ public Object getValue() { try { if (source != null) { final DOMResult result = new DOMResult(); - final Transformer trans = TransformerFactory.newInstance().newTransformer(); + final TransformerFactory factory = TransformerFactory.newInstance(); + factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); + factory.setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, ""); + factory.setAttribute(XMLConstants.ACCESS_EXTERNAL_STYLESHEET, ""); + final Transformer trans = factory.newTransformer(); trans.transform(source, result); document = result.getNode(); } else { diff --git a/src/main/java/org/apache/commons/jxpath/xml/DOMParser.java b/src/main/java/org/apache/commons/jxpath/xml/DOMParser.java index 26e7c69d7..428e4fbbb 100644 --- a/src/main/java/org/apache/commons/jxpath/xml/DOMParser.java +++ b/src/main/java/org/apache/commons/jxpath/xml/DOMParser.java @@ -39,6 +39,10 @@ public DOMParser() { public Object parseXML(final InputStream stream) { try { final DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); + factory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false); + factory.setFeature("http://xml.org/sax/features/external-general-entities", false); + factory.setFeature("http://xml.org/sax/features/external-parameter-entities", false); + factory.setXIncludeAware(false); factory.setValidating(isValidating()); factory.setNamespaceAware(isNamespaceAware()); factory.setIgnoringElementContentWhitespace(isIgnoringElementContentWhitespace()); diff --git a/src/main/java/org/apache/commons/jxpath/xml/JDOMParser.java b/src/main/java/org/apache/commons/jxpath/xml/JDOMParser.java index a10122cdd..6e8cf0b57 100644 --- a/src/main/java/org/apache/commons/jxpath/xml/JDOMParser.java +++ b/src/main/java/org/apache/commons/jxpath/xml/JDOMParser.java @@ -18,9 +18,11 @@ package org.apache.commons.jxpath.xml; import java.io.InputStream; +import java.io.StringReader; import org.apache.commons.jxpath.JXPathException; import org.jdom.input.SAXBuilder; +import org.xml.sax.InputSource; /** * An implementation of the XMLParser interface that produces a JDOM Document. @@ -41,6 +43,11 @@ public Object parseXML(final InputStream stream) { } try { final SAXBuilder builder = new SAXBuilder(); + builder.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false); + builder.setFeature("http://xml.org/sax/features/external-parameter-entities", false); + // SAXBuilder.setExpandEntities(true) re-enables external general entity + // resolution, so refuse external lookups with a no-op resolver instead. + builder.setEntityResolver((publicId, systemId) -> new InputSource(new StringReader(""))); builder.setExpandEntities(isExpandEntityReferences()); builder.setIgnoringElementContentWhitespace(isIgnoringElementContentWhitespace()); builder.setValidation(isValidating()); diff --git a/src/test/java/org/apache/commons/jxpath/xml/XXETest.java b/src/test/java/org/apache/commons/jxpath/xml/XXETest.java new file mode 100644 index 000000000..5a39a7f7f --- /dev/null +++ b/src/test/java/org/apache/commons/jxpath/xml/XXETest.java @@ -0,0 +1,69 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.commons.jxpath.xml; + +import static org.junit.jupiter.api.Assertions.assertFalse; + +import java.io.IOException; +import java.io.UncheckedIOException; +import java.net.URL; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; + +import org.junit.jupiter.api.io.TempDir; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; +import org.w3c.dom.Document; + +/** + * Tests that the XML parsers do not resolve external entities (XXE). + */ +class XXETest { + + @TempDir + Path dir; + + private URL buildDocument() { + try { + final Path secret = dir.resolve("secret.txt"); + Files.write(secret, "TOP-SECRET".getBytes(StandardCharsets.UTF_8)); + final Path xml = dir.resolve("xxe.xml"); + Files.write(xml, ("\n" + + " ]>\n" + + "&xxe;").getBytes(StandardCharsets.UTF_8)); + return xml.toUri().toURL(); + } catch (final IOException e) { + throw new UncheckedIOException(e); + } + } + + private String rootText(final String model, final Object document) { + if (DocumentContainer.MODEL_JDOM.equals(model)) { + return ((org.jdom.Document) document).getRootElement().getText(); + } + return ((Document) document).getDocumentElement().getTextContent(); + } + + @ParameterizedTest + @ValueSource(strings = {DocumentContainer.MODEL_DOM, DocumentContainer.MODEL_JDOM}) + void externalEntityIsNotResolved(final String model) { + final DocumentContainer container = new DocumentContainer(buildDocument(), model); + assertFalse(rootText(model, container.getValue()).contains("TOP-SECRET")); + } +}