XML parser: refactor hardening to XMLSecurity class, and make sure parser used in JAXBSerialiser on a SAXSource is hardened if not using MOXy (which sets up its own hardened parser, and varies behaviour if called with raw source vs a parsed source)

Author: petergeneric

stdlib/src/main/java/com/peterphi/std/util/DOMUtils.java

@@ -50,12 +50,7 @@ private static DocumentBuilderFactory createDocumentBuilderFactory()
 		{
 			factory.setNamespaceAware(true);
 
-			// Disable DTDs
-			factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", false);
-
-
-			factory.setXIncludeAware(false);
-			factory.setExpandEntityReferences(false);
+			XMLSecurity.harden(factory);
 		}
 		catch (ParserConfigurationException e)
 		{
@@ -66,6 +61,23 @@ private static DocumentBuilderFactory createDocumentBuilderFactory()
 	}
 
 
+	private static TransformerFactory createTransformerFactory()
+	{
+		TransformerFactory factory = TransformerFactory.newInstance();
+
+		try
+		{
+			XMLSecurity.harden(factory);
+		}
+		catch (TransformerConfigurationException e)
+		{
+			throw new RuntimeException("Could not configure XML TransformerFactory!", e);
+		}
+
+		return factory;
+	}
+
+
 	/**
 	 * Create a new (namespace-aware) DocumentBuilder
 	 *
@@ -191,7 +203,7 @@ public static void serialise(Node n, StreamResult result)
 
 		try
 		{
-			Transformer transform = TransformerFactory.newInstance().newTransformer();
+			Transformer transform = createTransformerFactory().newTransformer();
 			transform.transform(new DOMSource(n), result);
 		}
 		catch (TransformerConfigurationException e)
@@ -323,7 +335,7 @@ public static void pretty(final Source input, final StreamResult output)
 		try
 		{
 			// Configure transformer
-			Transformer transformer = TransformerFactory.newInstance().newTransformer();
+			Transformer transformer = createTransformerFactory().newTransformer();
 
 			transformer.setOutputProperty(OutputKeys.ENCODING, "utf-8");
 			transformer.setOutputProperty(OutputKeys.OMIT_XML_DECLARATION, "no");

stdlib/src/main/java/com/peterphi/std/util/JDOMUtils.java

@@ -117,6 +117,18 @@ public static org.jdom2.Element convert(org.w3c.dom.Element node)
 	// Parse
 	//
 
+	private static SAXBuilder createHardenedSAXBuilder()
+	{
+		SAXBuilder builder = new SAXBuilder();
+
+		builder.setFeature(XMLSecurity.DISALLOW_DOCTYPE_DECL, true);
+		builder.setFeature(XMLSecurity.EXTERNAL_GENERAL_ENTITIES, false);
+		builder.setFeature(XMLSecurity.EXTERNAL_PARAMETER_ENTITIES, false);
+		builder.setExpandEntities(false);
+
+		return builder;
+	}
+
 	public static Document parse(String xml)
 	{
 		return parse(new StringReader(xml));
@@ -151,7 +163,7 @@ public static Document parse(InputStream is)
 	{
 		try
 		{
-			return new SAXBuilder().build(is);
+			return createHardenedSAXBuilder().build(is);
 		}
 		catch (IOException e)
 		{
@@ -167,7 +179,7 @@ public static Document parse(Reader reader)
 	{
 		try
 		{
-			return new SAXBuilder().build(reader);
+			return createHardenedSAXBuilder().build(reader);
 		}
 		catch (IOException e)
 		{

stdlib/src/main/java/com/peterphi/std/util/XMLSecurity.java

@@ -0,0 +1,71 @@
+package com.peterphi.std.util;
+
+import org.xml.sax.SAXException;
+
+import javax.xml.XMLConstants;
+import javax.xml.parsers.DocumentBuilderFactory;
+import javax.xml.parsers.ParserConfigurationException;
+import javax.xml.parsers.SAXParserFactory;
+import javax.xml.transform.TransformerConfigurationException;
+import javax.xml.transform.TransformerFactory;
+
+/**
+ * Hardens JAXP XML factories against XXE by disabling DTDs and external entities. Security features fail fast: if a factory
+ * does not honour a feature, configuration throws rather than silently leaving the parser vulnerable.
+ */
+public final class XMLSecurity
+{
+	public static final String DISALLOW_DOCTYPE_DECL = "http://apache.org/xml/features/disallow-doctype-decl";
+	public static final String EXTERNAL_GENERAL_ENTITIES = "http://xml.org/sax/features/external-general-entities";
+	public static final String EXTERNAL_PARAMETER_ENTITIES = "http://xml.org/sax/features/external-parameter-entities";
+	public static final String LOAD_EXTERNAL_DTD = "http://apache.org/xml/features/nonvalidating/load-external-dtd";
+
+	private XMLSecurity()
+	{
+	}
+
+
+	public static void harden(final DocumentBuilderFactory factory) throws ParserConfigurationException
+	{
+		factory.setFeature(DISALLOW_DOCTYPE_DECL, true);
+		factory.setFeature(EXTERNAL_GENERAL_ENTITIES, false);
+		factory.setFeature(EXTERNAL_PARAMETER_ENTITIES, false);
+		factory.setFeature(LOAD_EXTERNAL_DTD, false);
+		factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
+
+		factory.setXIncludeAware(false);
+		factory.setExpandEntityReferences(false);
+	}
+
+
+	public static void harden(final SAXParserFactory factory) throws ParserConfigurationException, SAXException
+	{
+		factory.setFeature(DISALLOW_DOCTYPE_DECL, true);
+		factory.setFeature(EXTERNAL_GENERAL_ENTITIES, false);
+		factory.setFeature(EXTERNAL_PARAMETER_ENTITIES, false);
+		factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
+	}
+
+
+	public static void harden(final TransformerFactory factory) throws TransformerConfigurationException
+	{
+		factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
+
+		// ACCESS_EXTERNAL_* are optional; not all TransformerFactory implementations support them
+		trySetAttribute(factory, XMLConstants.ACCESS_EXTERNAL_DTD, "");
+		trySetAttribute(factory, XMLConstants.ACCESS_EXTERNAL_STYLESHEET, "");
+	}
+
+
+	private static void trySetAttribute(final TransformerFactory factory, final String name, final Object value)
+	{
+		try
+		{
+			factory.setAttribute(name, value);
+		}
+		catch (IllegalArgumentException e)
+		{
+			// Attribute not supported by this implementation
+		}
+	}
+}

stdlib/src/main/java/com/peterphi/std/util/jaxb/JAXBSerialiser.java

@@ -1,23 +1,29 @@
 package com.peterphi.std.util.jaxb;
 
 import com.peterphi.std.util.DOMUtils;
+import com.peterphi.std.util.XMLSecurity;
 import com.peterphi.std.util.jaxb.exception.JAXBRuntimeException;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.w3c.dom.Document;
 import org.w3c.dom.Element;
 import org.w3c.dom.Node;
 import org.xml.sax.InputSource;
+import org.xml.sax.SAXException;
+import org.xml.sax.XMLReader;
 
 import javax.xml.bind.JAXBContext;
 import javax.xml.bind.JAXBElement;
 import javax.xml.bind.JAXBException;
 import javax.xml.bind.Marshaller;
 import javax.xml.bind.PropertyException;
 import javax.xml.bind.Unmarshaller;
+import javax.xml.parsers.ParserConfigurationException;
+import javax.xml.parsers.SAXParserFactory;
 import javax.xml.stream.XMLStreamReader;
 import javax.xml.stream.XMLStreamWriter;
 import javax.xml.transform.Source;
+import javax.xml.transform.sax.SAXSource;
 import javax.xml.validation.Schema;
 import java.io.File;
 import java.io.InputStream;
@@ -37,7 +43,16 @@ public class JAXBSerialiser
 {
 	private static final Logger log = LoggerFactory.getLogger(JAXBSerialiser.class);
 
+	private static SAXParserFactory SAX_PARSER_FACTORY;
+
 	private final JAXBContext context;
+
+	/**
+	 * True if the underlying JAXB provider hardens its own internally-created XML parser against XXE (so we should let it own
+	 * the parse rather than supplying our own hardened parser via a {@link SAXSource}).
+	 */
+	private final boolean isMOXY;
+
 	private Schema schema;
 	private boolean prettyOutput = false;
 
@@ -62,6 +77,8 @@ private JAXBSerialiser(String contextPath)
 		{
 			throw new JAXBRuntimeException("Error creating JAXB Context: " + e.getMessage(), e);
 		}
+
+		this.isMOXY = isMOXY(this.context);
 	}
 
 
@@ -73,6 +90,7 @@ private JAXBSerialiser(String contextPath)
 	private JAXBSerialiser(JAXBContext context)
 	{
 		this.context = context;
+		this.isMOXY = isMOXY(context);
 	}
 
 
@@ -91,6 +109,8 @@ private JAXBSerialiser(Class<?>... classes)
 		{
 			throw new JAXBRuntimeException("Error creating JAXB Context: " + e.getMessage(), e);
 		}
+
+		this.isMOXY = isMOXY(this.context);
 	}
 
 
@@ -282,6 +302,56 @@ private Unmarshaller getUnmarshaller()
 		}
 	}
 
+
+	/**
+	 * Tests if this provider is eclipselink moxy.
+	 * @param context the JAXB context (may be null, defaults to system in this instance)
+	 *
+	 * @return true if the provider is from eclipselink
+	 */
+	private static boolean isMOXY(final JAXBContext context)
+	{
+		return context != null && context.getClass().getName().startsWith("org.eclipse.persistence.");
+	}
+
+
+	private static SAXParserFactory getHardenedSAXParserFactory()
+	{
+		if (SAX_PARSER_FACTORY == null)
+		{
+			try
+			{
+				final SAXParserFactory spf = SAXParserFactory.newInstance();
+
+				XMLSecurity.harden(spf);
+				spf.setNamespaceAware(true);
+
+				SAX_PARSER_FACTORY = spf;
+			}
+			catch (ParserConfigurationException | SAXException e)
+			{
+				throw new JAXBRuntimeException("Unable to set up SAXParserFactory to securely decode XML inputs!", e);
+			}
+		}
+
+		return SAX_PARSER_FACTORY;
+	}
+
+
+	private static SAXSource toHardenedSource(final InputSource inputSource)
+	{
+		try
+		{
+			final XMLReader reader = getHardenedSAXParserFactory().newSAXParser().getXMLReader();
+
+			return new SAXSource(reader, inputSource);
+		}
+		catch (ParserConfigurationException | SAXException e)
+		{
+			throw new JAXBRuntimeException("Error setting up secure XML source for deserialisation", e);
+		}
+	}
+
 	//
 	//
 	// Deserialisers
@@ -428,7 +498,17 @@ public Object deserialise(final File file)
 
 		try
 		{
-			final Object obj = unmarshaller.unmarshal(file);
+			final Object obj;
+			if (isMOXY)
+			{
+				obj = unmarshaller.unmarshal(file);
+			}
+			else
+			{
+				final InputSource inputSource = new InputSource(file.toURI().toASCIIString());
+
+				obj = unmarshaller.unmarshal(toHardenedSource(inputSource));
+			}
 
 			if (obj == null)
 				throw new RuntimeException("Malformed XML from " + file);
@@ -451,7 +531,9 @@ public Object deserialise(final InputSource source)
 
 		try
 		{
-			final Object obj = unmarshaller.unmarshal(source);
+			// MOXy disables DTDs and external entities by default when it owns the parse, so wrapping its input in a parser we build ourselves is unnecessary (MOXy also applies stricter type coercion on a {@link SAXSource} with an external reader)
+			// A StreamSource carries raw, unparsed XML; other Source types are already parsed or carry their own reader.
+			final Object obj = isMOXY ? unmarshaller.unmarshal(source) : unmarshaller.unmarshal(toHardenedSource(source));
 
 			if (obj == null)
 				throw new RuntimeException("Malformed XML! JAXB returned null");
@@ -474,7 +556,24 @@ public Object deserialise(final Source source)
 
 		try
 		{
-			final Object obj = unmarshaller.unmarshal(source);
+			// MOXy disables DTDs and external entities by default when it owns the parse, so wrapping its input in a parser we build ourselves is unnecessary (MOXy also applies stricter type coercion on a {@link SAXSource} with an external reader)
+			// A StreamSource carries raw, unparsed XML; other Source types are already parsed or carry their own reader.
+			final Source effectiveSource;
+			if (!isMOXY && source instanceof javax.xml.transform.stream.StreamSource stream)
+			{
+				final InputSource inputSource = new InputSource();
+				inputSource.setSystemId(stream.getSystemId());
+				inputSource.setByteStream(stream.getInputStream());
+				inputSource.setCharacterStream(stream.getReader());
+
+				effectiveSource = toHardenedSource(inputSource);
+			}
+			else
+			{
+				effectiveSource = source;
+			}
+
+			final Object obj = unmarshaller.unmarshal(effectiveSource);
 
 			if (obj == null)
 				throw new RuntimeException("Malformed XML! JAXB returned null");