diff --git a/checkstyle.xml b/checkstyle.xml index 625428252..dcc9ca891 100644 --- a/checkstyle.xml +++ b/checkstyle.xml @@ -11,17 +11,49 @@ - + - - - - - - - - - - + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + \ No newline at end of file diff --git a/checkstyle_suppressions.xml b/checkstyle_suppressions.xml index 1c51453aa..1291bd218 100644 --- a/checkstyle_suppressions.xml +++ b/checkstyle_suppressions.xml @@ -4,5 +4,5 @@ "https://checkstyle.org/dtds/suppressions_1_2.dtd"> - + diff --git a/org.hl7.fhir.utilities/checkstyle_suppressions.xml b/org.hl7.fhir.utilities/checkstyle_suppressions.xml index 1291bd218..f17f30fa5 100644 --- a/org.hl7.fhir.utilities/checkstyle_suppressions.xml +++ b/org.hl7.fhir.utilities/checkstyle_suppressions.xml @@ -4,5 +4,8 @@ "https://checkstyle.org/dtds/suppressions_1_2.dtd"> - + + + + diff --git a/org.hl7.fhir.utilities/src/main/java/org/hl7/fhir/utilities/xml/XMLUtil.java b/org.hl7.fhir.utilities/src/main/java/org/hl7/fhir/utilities/xml/XMLUtil.java index 17e8b8777..0cf418cf1 100644 --- a/org.hl7.fhir.utilities/src/main/java/org/hl7/fhir/utilities/xml/XMLUtil.java +++ b/org.hl7.fhir.utilities/src/main/java/org/hl7/fhir/utilities/xml/XMLUtil.java @@ -504,6 +504,14 @@ public class XMLUtil { return e == null ? null : e.getAttribute(aname); } + /** + * This method is used to create a new TransformerFactory instance with external processing features configured + * securely. + *

+ * IMPORTANT This method should be the only place where TransformerFactory is instantiated in this project. + * + * @return A TransformerFactory instance external processing features configured securely. + */ public static TransformerFactory newXXEProtectedTransformerFactory() { final TransformerFactory transformerFactory = TransformerFactory.newInstance(); transformerFactory.setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, ""); @@ -511,6 +519,15 @@ public class XMLUtil { return transformerFactory; } + /** + * This method is used to create a new DocumentBuilderFactory instance with external processing features configured + * securely. + *

+ * IMPORTANT This method should be the only place where DocumentBuilderFactory is instantiated in this project. + * + * @return A DocumentBuilderFactory instance external processing features configured securely. + * @throws ParserConfigurationException If a DocumentBuilder cannot be configured with the requested features. + */ public static DocumentBuilderFactory newXXEProtectedDocumentBuilderFactory() throws ParserConfigurationException { final DocumentBuilderFactory documentBuilderFactory = DocumentBuilderFactory.newInstance(); documentBuilderFactory.setFeature(APACHE_XML_FEATURES_DISALLOW_DOCTYPE_DECL, true); @@ -518,13 +535,35 @@ public class XMLUtil { return documentBuilderFactory; } + /** + * This method is used to create a new SAXParserFactory instance with external processing features configured + * securely. + *

+ * IMPORTANT This method should be the only place where SAXParserFactory is instantiated in this project. + * + * @return A SAXParserFactory instance external processing features configured securely. + * @throws SAXNotSupportedException When the underlying XMLReader recognizes the property name but doesn't support + * the property. + * @throws SAXNotRecognizedException When the underlying XMLReader does not recognize the property name. + * @throws ParserConfigurationException If a SAXParser cannot be configured with the requested features. + */ public static SAXParserFactory newXXEProtectedSaxParserFactory() throws SAXNotSupportedException, SAXNotRecognizedException, ParserConfigurationException { final SAXParserFactory spf = SAXParserFactory.newInstance(); spf.setFeature(SAX_FEATURES_EXTERNAL_GENERAL_ENTITIES, false); spf.setFeature(APACHE_XML_FEATURES_DISALLOW_DOCTYPE_DECL, true); + return spf; } + /** + * This method is used to create a new XMLReader instance from a passed SAXParserFactory with external processing + * features configured securely. + * + * @param spf The SAXParserFactory to create the XMLReader from. + * @return A XMLReader instance external processing features configured securely. + * @throws ParserConfigurationException If a SAXParser cannot be configured with the requested features. + * @throws SAXException If any SAX exceptions occur during the creation of the XMLReader. + */ public static XMLReader getXXEProtectedXMLReader(SAXParserFactory spf) throws ParserConfigurationException, SAXException { final SAXParser saxParser = spf.newSAXParser(); final XMLReader xmlReader = saxParser.getXMLReader(); diff --git a/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/xml/XMLUtilTests.java b/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/xml/XMLUtilTests.java new file mode 100644 index 000000000..5bec027d3 --- /dev/null +++ b/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/xml/XMLUtilTests.java @@ -0,0 +1,87 @@ +package org.hl7.fhir.utilities.xml; + +import net.sf.saxon.trans.XPathException; +import org.hl7.fhir.utilities.filesystem.ManagedFileAccess; +import org.junit.Test; +import org.w3c.dom.Document; +import org.xml.sax.InputSource; +import org.xml.sax.SAXException; +import org.xml.sax.SAXParseException; +import org.xml.sax.XMLReader; + +import javax.xml.parsers.DocumentBuilder; +import javax.xml.parsers.DocumentBuilderFactory; +import javax.xml.parsers.ParserConfigurationException; +import javax.xml.parsers.SAXParserFactory; +import javax.xml.transform.*; +import javax.xml.transform.dom.DOMResult; +import javax.xml.transform.dom.DOMSource; +import javax.xml.transform.sax.SAXSource; +import javax.xml.transform.stream.StreamResult; +import javax.xml.transform.stream.StreamSource; + +import java.io.File; +import java.io.IOException; +import java.io.StringWriter; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Assertions.assertThrows; + +public class XMLUtilTests { + @Test + public void testDocumentBuilderThrowsExceptionForExternalEntity() throws ParserConfigurationException, IOException { + DocumentBuilderFactory factory = XMLUtil.newXXEProtectedDocumentBuilderFactory(); + DocumentBuilder safeBuilder = factory.newDocumentBuilder(); + + File file = ManagedFileAccess.file("src/test/resources/xml/evil-resource.xml"); + + SAXParseException e = assertThrows(SAXParseException.class, ()-> safeBuilder.parse(file)); + assertThat(e.getMessage()).contains("DOCTYPE is disallowed"); + } + + @Test + public void testTransformerFactoryThrowsExceptionForExternalEntity() throws ParserConfigurationException, IOException, SAXException, TransformerException { + DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); + DocumentBuilder safeBuilder = factory.newDocumentBuilder(); + + File file = ManagedFileAccess.file("src/test/resources/xml/resource.xml"); + + Document document = safeBuilder.parse(file); + + StringWriter sw = new StringWriter(); + TransformerFactory tf = XMLUtil.newXXEProtectedTransformerFactory(); + + File templateFile = ManagedFileAccess.file("src/test/resources/xml/evil-transform.xslt"); + Source xsltSource = new StreamSource(templateFile); + Transformer transformer = tf.newTransformer(xsltSource); + transformer.setOutputProperty(OutputKeys.OMIT_XML_DECLARATION, "no"); + transformer.setOutputProperty(OutputKeys.METHOD, "xml"); + transformer.setOutputProperty(OutputKeys.INDENT, "yes"); + transformer.setOutputProperty(OutputKeys.ENCODING, "UTF-8"); + + XPathException e = assertThrows(XPathException.class, () -> transformer.transform(new DOMSource(document), new StreamResult(sw))); + assertThat(e.getMessage()).contains("URIs using protocol file are not permitted"); + } + + @Test + public void testSaxParserFactoryThrowsExceptionForExternalEntity() throws ParserConfigurationException, IOException, SAXException, TransformerException { + + SAXParserFactory spf = XMLUtil.newXXEProtectedSaxParserFactory(); + spf.setNamespaceAware(true); + spf.setValidating(false); + XMLReader xmlReader = spf.newSAXParser().getXMLReader(); + + File templateFile = ManagedFileAccess.file("src/test/resources/xml/evil-resource.xml"); + + SAXParseException e = assertThrows(SAXParseException.class, () -> xmlReader.parse(new StreamSource(templateFile).getSystemId())); + assertThat(e.getMessage()).contains("DOCTYPE is disallowed"); + } + + @Test + public void testXMLReaderThrowsExceptionForExternalEntity() throws ParserConfigurationException, IOException, SAXException, TransformerException { + SAXParserFactory spf = SAXParserFactory.newInstance(); + + IllegalArgumentException e = assertThrows(IllegalArgumentException.class, () -> XMLUtil.getXXEProtectedXMLReader(spf)); + assertThat(e.getMessage()).contains("SAXParserFactory has insecure feature setting"); + } +} diff --git a/org.hl7.fhir.utilities/src/test/resources/xml/evil-resource.xml b/org.hl7.fhir.utilities/src/test/resources/xml/evil-resource.xml new file mode 100644 index 000000000..7606bd1a4 --- /dev/null +++ b/org.hl7.fhir.utilities/src/test/resources/xml/evil-resource.xml @@ -0,0 +1,6 @@ + + ]> + + + &example; + \ No newline at end of file diff --git a/org.hl7.fhir.utilities/src/test/resources/xml/evil-transform.xslt b/org.hl7.fhir.utilities/src/test/resources/xml/evil-transform.xslt new file mode 100644 index 000000000..8ddfba4aa --- /dev/null +++ b/org.hl7.fhir.utilities/src/test/resources/xml/evil-transform.xslt @@ -0,0 +1,6 @@ + + + Evil: + + \ No newline at end of file diff --git a/org.hl7.fhir.utilities/src/test/resources/xml/jackpot.xml b/org.hl7.fhir.utilities/src/test/resources/xml/jackpot.xml new file mode 100644 index 000000000..266caa6e9 --- /dev/null +++ b/org.hl7.fhir.utilities/src/test/resources/xml/jackpot.xml @@ -0,0 +1 @@ +stuff \ No newline at end of file diff --git a/org.hl7.fhir.utilities/src/test/resources/xml/resource.xml b/org.hl7.fhir.utilities/src/test/resources/xml/resource.xml new file mode 100644 index 000000000..e7bd4a4f1 --- /dev/null +++ b/org.hl7.fhir.utilities/src/test/resources/xml/resource.xml @@ -0,0 +1,5 @@ + + + + + \ No newline at end of file