From f0a25dabe3c53660e7d69a39e2ad0b931bffa91b Mon Sep 17 00:00:00 2001 From: dotasek Date: Thu, 17 Oct 2024 15:46:05 -0400 Subject: [PATCH] Move XXE safe SAXParserFactory and XMLReader instantiation to XMLUtil --- .../fhir/dstu2016may/metamodel/XmlParser.java | 11 ++----- .../fhir/dstu3/elementmodel/XmlParser.java | 14 ++------ .../hl7/fhir/r4/elementmodel/XmlParser.java | 12 ++----- .../hl7/fhir/r4b/elementmodel/XmlParser.java | 13 ++------ .../hl7/fhir/r5/elementmodel/XmlParser.java | 13 ++------ .../org/hl7/fhir/utilities/xml/XMLUtil.java | 33 ++++++++++++++++--- 6 files changed, 41 insertions(+), 55 deletions(-) diff --git a/org.hl7.fhir.dstu2016may/src/main/java/org/hl7/fhir/dstu2016may/metamodel/XmlParser.java b/org.hl7.fhir.dstu2016may/src/main/java/org/hl7/fhir/dstu2016may/metamodel/XmlParser.java index e4b37202d..db97ebbc2 100644 --- a/org.hl7.fhir.dstu2016may/src/main/java/org/hl7/fhir/dstu2016may/metamodel/XmlParser.java +++ b/org.hl7.fhir.dstu2016may/src/main/java/org/hl7/fhir/dstu2016may/metamodel/XmlParser.java @@ -36,7 +36,6 @@ import java.util.List; import javax.xml.parsers.DocumentBuilder; import javax.xml.parsers.DocumentBuilderFactory; -import javax.xml.parsers.SAXParser; import javax.xml.parsers.SAXParserFactory; import javax.xml.transform.Transformer; import javax.xml.transform.TransformerFactory; @@ -95,16 +94,10 @@ public class XmlParser extends ParserBase { DocumentBuilder docBuilder = factory.newDocumentBuilder(); doc = docBuilder.newDocument(); DOMResult domResult = new DOMResult(doc); - SAXParserFactory spf = SAXParserFactory.newInstance(); + SAXParserFactory spf = XMLUtil.newXXEProtectedSaxParserFactory(); spf.setNamespaceAware(true); spf.setValidating(false); - SAXParser saxParser = spf.newSAXParser(); - XMLReader xmlReader = saxParser.getXMLReader(); - // xxe protection - spf.setFeature("http://xml.org/sax/features/external-general-entities", false); - spf.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); - xmlReader.setFeature("http://xml.org/sax/features/external-general-entities", false); - xmlReader.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); + XMLReader xmlReader = XMLUtil.getXXEProtectedXMLReader(spf); XmlLocationAnnotator locationAnnotator = new XmlLocationAnnotator(xmlReader, doc); InputSource inputSource = new InputSource(stream); diff --git a/org.hl7.fhir.dstu3/src/main/java/org/hl7/fhir/dstu3/elementmodel/XmlParser.java b/org.hl7.fhir.dstu3/src/main/java/org/hl7/fhir/dstu3/elementmodel/XmlParser.java index 8203d92e3..258735c92 100644 --- a/org.hl7.fhir.dstu3/src/main/java/org/hl7/fhir/dstu3/elementmodel/XmlParser.java +++ b/org.hl7.fhir.dstu3/src/main/java/org/hl7/fhir/dstu3/elementmodel/XmlParser.java @@ -41,7 +41,6 @@ import java.util.List; import javax.xml.parsers.DocumentBuilder; import javax.xml.parsers.DocumentBuilderFactory; -import javax.xml.parsers.SAXParser; import javax.xml.parsers.SAXParserFactory; import javax.xml.transform.Transformer; import javax.xml.transform.TransformerFactory; @@ -114,18 +113,11 @@ public class XmlParser extends ParserBase { DocumentBuilder docBuilder = factory.newDocumentBuilder(); doc = docBuilder.newDocument(); DOMResult domResult = new DOMResult(doc); - SAXParserFactory spf = SAXParserFactory.newInstance(); + SAXParserFactory spf = XMLUtil.newXXEProtectedSaxParserFactory(); spf.setNamespaceAware(true); spf.setValidating(false); - // xxe protection - spf.setFeature("http://xml.org/sax/features/external-general-entities", false); - spf.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); - SAXParser saxParser = spf.newSAXParser(); - XMLReader xmlReader = saxParser.getXMLReader(); - // xxe protection - xmlReader.setFeature("http://xml.org/sax/features/external-general-entities", false); - xmlReader.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); - + XMLReader xmlReader = XMLUtil.getXXEProtectedXMLReader(spf); + XmlLocationAnnotator locationAnnotator = new XmlLocationAnnotator(xmlReader, doc); InputSource inputSource = new InputSource(stream); SAXSource saxSource = new SAXSource(locationAnnotator, inputSource); diff --git a/org.hl7.fhir.r4/src/main/java/org/hl7/fhir/r4/elementmodel/XmlParser.java b/org.hl7.fhir.r4/src/main/java/org/hl7/fhir/r4/elementmodel/XmlParser.java index d5c18197c..7d0268542 100644 --- a/org.hl7.fhir.r4/src/main/java/org/hl7/fhir/r4/elementmodel/XmlParser.java +++ b/org.hl7.fhir.r4/src/main/java/org/hl7/fhir/r4/elementmodel/XmlParser.java @@ -39,7 +39,6 @@ import java.util.List; import javax.xml.parsers.DocumentBuilder; import javax.xml.parsers.DocumentBuilderFactory; -import javax.xml.parsers.SAXParser; import javax.xml.parsers.SAXParserFactory; import javax.xml.transform.Transformer; import javax.xml.transform.TransformerFactory; @@ -113,17 +112,10 @@ public class XmlParser extends ParserBase { DocumentBuilder docBuilder = factory.newDocumentBuilder(); doc = docBuilder.newDocument(); DOMResult domResult = new DOMResult(doc); - SAXParserFactory spf = SAXParserFactory.newInstance(); + SAXParserFactory spf = XMLUtil.newXXEProtectedSaxParserFactory(); spf.setNamespaceAware(true); spf.setValidating(false); - // xxe protection - spf.setFeature("http://xml.org/sax/features/external-general-entities", false); - spf.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); - SAXParser saxParser = spf.newSAXParser(); - XMLReader xmlReader = saxParser.getXMLReader(); - // xxe protection - xmlReader.setFeature("http://xml.org/sax/features/external-general-entities", false); - xmlReader.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); + XMLReader xmlReader = XMLUtil.getXXEProtectedXMLReader(spf); XmlLocationAnnotator locationAnnotator = new XmlLocationAnnotator(xmlReader, doc); InputSource inputSource = new InputSource(stream); diff --git a/org.hl7.fhir.r4b/src/main/java/org/hl7/fhir/r4b/elementmodel/XmlParser.java b/org.hl7.fhir.r4b/src/main/java/org/hl7/fhir/r4b/elementmodel/XmlParser.java index 4b8e948ea..475d8fdc7 100644 --- a/org.hl7.fhir.r4b/src/main/java/org/hl7/fhir/r4b/elementmodel/XmlParser.java +++ b/org.hl7.fhir.r4b/src/main/java/org/hl7/fhir/r4b/elementmodel/XmlParser.java @@ -39,7 +39,6 @@ import java.util.List; import javax.xml.parsers.DocumentBuilder; import javax.xml.parsers.DocumentBuilderFactory; -import javax.xml.parsers.SAXParser; import javax.xml.parsers.SAXParserFactory; import javax.xml.transform.Transformer; import javax.xml.transform.TransformerFactory; @@ -52,7 +51,6 @@ import org.hl7.fhir.exceptions.FHIRFormatError; import org.hl7.fhir.r4b.conformance.ProfileUtilities; import org.hl7.fhir.r4b.context.IWorkerContext; import org.hl7.fhir.r4b.elementmodel.Element.SpecialElement; -import org.hl7.fhir.r4b.elementmodel.ParserBase.NamedElement; import org.hl7.fhir.r4b.formats.FormatUtilities; import org.hl7.fhir.r4b.formats.IParser.OutputStyle; import org.hl7.fhir.r4b.model.DateTimeType; @@ -136,17 +134,10 @@ public class XmlParser extends ParserBase { DocumentBuilder docBuilder = factory.newDocumentBuilder(); doc = docBuilder.newDocument(); DOMResult domResult = new DOMResult(doc); - SAXParserFactory spf = SAXParserFactory.newInstance(); + SAXParserFactory spf = XMLUtil.newXXEProtectedSaxParserFactory(); spf.setNamespaceAware(true); spf.setValidating(false); - // xxe protection - spf.setFeature("http://xml.org/sax/features/external-general-entities", false); - spf.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); - SAXParser saxParser = spf.newSAXParser(); - XMLReader xmlReader = saxParser.getXMLReader(); - // xxe protection - xmlReader.setFeature("http://xml.org/sax/features/external-general-entities", false); - xmlReader.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); + XMLReader xmlReader = XMLUtil.getXXEProtectedXMLReader(spf); XmlLocationAnnotator locationAnnotator = new XmlLocationAnnotator(xmlReader, doc); InputSource inputSource = new InputSource(stream); diff --git a/org.hl7.fhir.r5/src/main/java/org/hl7/fhir/r5/elementmodel/XmlParser.java b/org.hl7.fhir.r5/src/main/java/org/hl7/fhir/r5/elementmodel/XmlParser.java index ac1695120..3d7f43a64 100644 --- a/org.hl7.fhir.r5/src/main/java/org/hl7/fhir/r5/elementmodel/XmlParser.java +++ b/org.hl7.fhir.r5/src/main/java/org/hl7/fhir/r5/elementmodel/XmlParser.java @@ -44,7 +44,6 @@ import java.util.Set; import javax.xml.parsers.DocumentBuilder; import javax.xml.parsers.DocumentBuilderFactory; -import javax.xml.parsers.SAXParser; import javax.xml.parsers.SAXParserFactory; import javax.xml.transform.Transformer; import javax.xml.transform.TransformerFactory; @@ -150,17 +149,11 @@ public class XmlParser extends ParserBase { DocumentBuilder docBuilder = factory.newDocumentBuilder(); doc = docBuilder.newDocument(); DOMResult domResult = new DOMResult(doc); - SAXParserFactory spf = SAXParserFactory.newInstance(); + SAXParserFactory spf = XMLUtil.newXXEProtectedSaxParserFactory(); spf.setNamespaceAware(true); spf.setValidating(false); - // xxe protection - spf.setFeature("http://xml.org/sax/features/external-general-entities", false); - spf.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); - SAXParser saxParser = spf.newSAXParser(); - XMLReader xmlReader = saxParser.getXMLReader(); - // xxe protection - xmlReader.setFeature("http://xml.org/sax/features/external-general-entities", false); - xmlReader.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); + + XMLReader xmlReader = XMLUtil.getXXEProtectedXMLReader(spf); XmlLocationAnnotator locationAnnotator = new XmlLocationAnnotator(xmlReader, doc); InputSource inputSource = new InputSource(stream); 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 fed2a40e0..17e8b8777 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 @@ -42,9 +42,7 @@ import java.util.List; import java.util.Set; import javax.xml.XMLConstants; -import javax.xml.parsers.DocumentBuilder; -import javax.xml.parsers.DocumentBuilderFactory; -import javax.xml.parsers.ParserConfigurationException; +import javax.xml.parsers.*; import javax.xml.transform.Result; import javax.xml.transform.Source; import javax.xml.transform.Transformer; @@ -64,10 +62,15 @@ import org.w3c.dom.NodeList; import org.w3c.dom.ls.DOMImplementationLS; import org.w3c.dom.ls.LSSerializer; import org.xml.sax.SAXException; +import org.xml.sax.SAXNotRecognizedException; +import org.xml.sax.SAXNotSupportedException; +import org.xml.sax.XMLReader; public class XMLUtil { public static final String SPACE_CHAR = "\u00A0"; + public static final String SAX_FEATURES_EXTERNAL_GENERAL_ENTITIES = "http://xml.org/sax/features/external-general-entities"; + public static final String APACHE_XML_FEATURES_DISALLOW_DOCTYPE_DECL = "http://apache.org/xml/features/disallow-doctype-decl"; public static boolean isNMToken(String name) { if (name == null) @@ -510,12 +513,34 @@ public class XMLUtil { public static DocumentBuilderFactory newXXEProtectedDocumentBuilderFactory() throws ParserConfigurationException { final DocumentBuilderFactory documentBuilderFactory = DocumentBuilderFactory.newInstance(); - documentBuilderFactory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); + documentBuilderFactory.setFeature(APACHE_XML_FEATURES_DISALLOW_DOCTYPE_DECL, true); documentBuilderFactory.setXIncludeAware(false); return documentBuilderFactory; } + 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; + } + public static XMLReader getXXEProtectedXMLReader(SAXParserFactory spf) throws ParserConfigurationException, SAXException { + final SAXParser saxParser = spf.newSAXParser(); + final XMLReader xmlReader = saxParser.getXMLReader(); + + final boolean externalGeneralEntitiesFeatureValue = spf.getFeature(SAX_FEATURES_EXTERNAL_GENERAL_ENTITIES); + if (externalGeneralEntitiesFeatureValue) { + throw new IllegalArgumentException("SAXParserFactory has insecure feature setting:" + SAX_FEATURES_EXTERNAL_GENERAL_ENTITIES+ "=" + externalGeneralEntitiesFeatureValue); + } + final boolean disallowDocTypeDeclFeatureValue = spf.getFeature(APACHE_XML_FEATURES_DISALLOW_DOCTYPE_DECL); + if (!disallowDocTypeDeclFeatureValue) { + throw new IllegalArgumentException("SAXParserFactory has insecure feature setting:" + APACHE_XML_FEATURES_DISALLOW_DOCTYPE_DECL + "=" + disallowDocTypeDeclFeatureValue); + } + xmlReader.setFeature(SAX_FEATURES_EXTERNAL_GENERAL_ENTITIES, false); + xmlReader.setFeature(APACHE_XML_FEATURES_DISALLOW_DOCTYPE_DECL, true); + return xmlReader; + } public static void writeDomToFile(Document doc, String filename) throws TransformerException, IOException { TransformerFactory transformerFactory = XMLUtil.newXXEProtectedTransformerFactory(); Transformer transformer = transformerFactory.newTransformer();