From 777747604ffaea21272e6df8fbf2a460a0312d37 Mon Sep 17 00:00:00 2001 From: dotasek Date: Tue, 19 Nov 2024 16:46:01 -0500 Subject: [PATCH 1/2] Automate testing for XMLUtils factory methods --- checkstyle.xml | 37 ++++++--- checkstyle_suppressions.xml | 2 +- .../checkstyle_suppressions.xml | 4 +- .../org/hl7/fhir/utilities/xml/XMLUtil.java | 1 + .../hl7/fhir/utilities/xml/XMLUtilTests.java | 79 +++++++++++++++++++ .../src/test/resources/xml/evil-resource.xml | 6 ++ .../test/resources/xml/evil-transform.xslt | 6 ++ .../src/test/resources/xml/jackpot.xml | 1 + .../src/test/resources/xml/resource.xml | 5 ++ 9 files changed, 128 insertions(+), 13 deletions(-) create mode 100644 org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/xml/XMLUtilTests.java create mode 100644 org.hl7.fhir.utilities/src/test/resources/xml/evil-resource.xml create mode 100644 org.hl7.fhir.utilities/src/test/resources/xml/evil-transform.xslt create mode 100644 org.hl7.fhir.utilities/src/test/resources/xml/jackpot.xml create mode 100644 org.hl7.fhir.utilities/src/test/resources/xml/resource.xml diff --git a/checkstyle.xml b/checkstyle.xml index 625428252..777ca2e5f 100644 --- a/checkstyle.xml +++ b/checkstyle.xml @@ -11,17 +11,32 @@ - + - - - - - - - - - - + + + + + + + + + + + + + + + + + + + + + + + + + \ 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..c4251154f 100644 --- a/org.hl7.fhir.utilities/checkstyle_suppressions.xml +++ b/org.hl7.fhir.utilities/checkstyle_suppressions.xml @@ -4,5 +4,7 @@ "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..65226f561 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 @@ -522,6 +522,7 @@ public class XMLUtil { final SAXParserFactory spf = SAXParserFactory.newInstance(); spf.setFeature(SAX_FEATURES_EXTERNAL_GENERAL_ENTITIES, false); spf.setFeature(APACHE_XML_FEATURES_DISALLOW_DOCTYPE_DECL, true); + return spf; } 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..0fe47cb87 --- /dev/null +++ b/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/xml/XMLUtilTests.java @@ -0,0 +1,79 @@ +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"); + } +} 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 From d0ed5e359f9502e7b4b051271f90cf647fc216c8 Mon Sep 17 00:00:00 2001 From: dotasek Date: Wed, 20 Nov 2024 11:06:56 -0500 Subject: [PATCH 2/2] Add javadoc and useful messages from checkstyle --- checkstyle.xml | 17 +++++++++ .../checkstyle_suppressions.xml | 7 ++-- .../org/hl7/fhir/utilities/xml/XMLUtil.java | 38 +++++++++++++++++++ .../hl7/fhir/utilities/xml/XMLUtilTests.java | 8 ++++ 4 files changed, 67 insertions(+), 3 deletions(-) diff --git a/checkstyle.xml b/checkstyle.xml index 777ca2e5f..dcc9ca891 100644 --- a/checkstyle.xml +++ b/checkstyle.xml @@ -28,15 +28,32 @@ + + + + + + + + + \ No newline at end of file diff --git a/org.hl7.fhir.utilities/checkstyle_suppressions.xml b/org.hl7.fhir.utilities/checkstyle_suppressions.xml index c4251154f..f17f30fa5 100644 --- a/org.hl7.fhir.utilities/checkstyle_suppressions.xml +++ b/org.hl7.fhir.utilities/checkstyle_suppressions.xml @@ -4,7 +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 65226f561..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,6 +535,18 @@ 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); @@ -526,6 +555,15 @@ public class XMLUtil { 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 index 0fe47cb87..5bec027d3 100644 --- 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 @@ -76,4 +76,12 @@ public class XMLUtilTests { 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"); + } }