From ef27751d495525a1af9d76ffc21de66f21c8ff5a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yannick=20Mo=CC=88ller?= Date: Mon, 9 Dec 2024 15:29:32 +0100 Subject: [PATCH 1/3] fix potential XXE vulnerability without disallowing external entities one is able to include webcontent or files directly from the system e.g. file:///etc/hosts or some like this into the uploaded content via declaring an external entity. Details can be found on https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html#java and on https://docs.oracle.com/javase/8/docs/technotes/guides/security/jaxp/jaxp.html#feature-for-secure-processing - fixes: SIRI-1037 --- src/main/java/sirius/kernel/xml/XMLGenerator.java | 2 ++ src/main/java/sirius/kernel/xml/XMLReader.java | 6 +++++- src/main/java/sirius/kernel/xml/XMLStructuredInput.java | 2 ++ 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/main/java/sirius/kernel/xml/XMLGenerator.java b/src/main/java/sirius/kernel/xml/XMLGenerator.java index dcb80198..600b609f 100644 --- a/src/main/java/sirius/kernel/xml/XMLGenerator.java +++ b/src/main/java/sirius/kernel/xml/XMLGenerator.java @@ -16,6 +16,7 @@ import javax.annotation.Nullable; import javax.annotation.ParametersAreNonnullByDefault; +import javax.xml.XMLConstants; import javax.xml.parsers.DocumentBuilder; import javax.xml.parsers.DocumentBuilderFactory; import javax.xml.parsers.ParserConfigurationException; @@ -146,6 +147,7 @@ public static Document createDocument(@Nullable String namespaceURI, String qualifiedName, @Nullable DocumentType docType) throws ParserConfigurationException { DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); + factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); DocumentBuilder builder = factory.newDocumentBuilder(); DOMImplementation impl = builder.getDOMImplementation(); return impl.createDocument(namespaceURI, qualifiedName, docType); diff --git a/src/main/java/sirius/kernel/xml/XMLReader.java b/src/main/java/sirius/kernel/xml/XMLReader.java index 46259bc7..a52e7cd4 100644 --- a/src/main/java/sirius/kernel/xml/XMLReader.java +++ b/src/main/java/sirius/kernel/xml/XMLReader.java @@ -17,6 +17,7 @@ import sirius.kernel.commons.Strings; import sirius.kernel.health.Exceptions; +import javax.xml.XMLConstants; import javax.xml.parsers.DocumentBuilder; import javax.xml.parsers.DocumentBuilderFactory; import javax.xml.parsers.ParserConfigurationException; @@ -63,7 +64,9 @@ public class XMLReader extends DefaultHandler { */ public XMLReader() { try { - documentBuilder = DocumentBuilderFactory.newInstance().newDocumentBuilder(); + DocumentBuilderFactory documentBuilderFactory = DocumentBuilderFactory.newInstance(); + documentBuilderFactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); + documentBuilder = documentBuilderFactory.newDocumentBuilder(); taskContext = TaskContext.get(); } catch (ParserConfigurationException exception) { throw Exceptions.handle(exception); @@ -178,6 +181,7 @@ static class UserInterruptException extends RuntimeException { public void parse(InputStream stream, Function resourceLocator) throws IOException { try (stream) { SAXParserFactory factory = SAXParserFactory.newInstance(); + factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); SAXParser saxParser = factory.newSAXParser(); org.xml.sax.XMLReader reader = saxParser.getXMLReader(); reader.setEntityResolver(new EntityResolver() { diff --git a/src/main/java/sirius/kernel/xml/XMLStructuredInput.java b/src/main/java/sirius/kernel/xml/XMLStructuredInput.java index 5e93e8aa..dda55017 100644 --- a/src/main/java/sirius/kernel/xml/XMLStructuredInput.java +++ b/src/main/java/sirius/kernel/xml/XMLStructuredInput.java @@ -14,6 +14,7 @@ import javax.annotation.Nonnull; import javax.annotation.Nullable; +import javax.xml.XMLConstants; import javax.xml.namespace.NamespaceContext; import javax.xml.parsers.DocumentBuilder; import javax.xml.parsers.DocumentBuilderFactory; @@ -38,6 +39,7 @@ public class XMLStructuredInput implements StructuredInput { public XMLStructuredInput(InputStream inputStream, @Nullable NamespaceContext namespaceContext) throws IOException { try { DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); + factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); if (namespaceContext != null) { factory.setNamespaceAware(true); } From 95199e8b5d1ef9c2857b2aba73e5598c193bf000 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yannick=20Mo=CC=88ller?= Date: Mon, 9 Dec 2024 15:29:56 +0100 Subject: [PATCH 2/3] drive-by: fix some typos --- src/main/java/sirius/kernel/xml/XMLReader.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/main/java/sirius/kernel/xml/XMLReader.java b/src/main/java/sirius/kernel/xml/XMLReader.java index a52e7cd4..f7c29442 100644 --- a/src/main/java/sirius/kernel/xml/XMLReader.java +++ b/src/main/java/sirius/kernel/xml/XMLReader.java @@ -38,11 +38,11 @@ import java.util.function.Function; /** - * A combination of DOM and SAX parser which permits to parse very large XML files while conveniently handling sub tree + * A combination of DOM and SAX parser which permits to parse very large XML files while conveniently handling subtree * using a DOM and xpath api. *

* Used SAX to parse a given XML file. A set of {@link NodeHandler} objects can be given, which get notified if - * a sub-tree below a given tag was parsed. This sub-tree is available as DOM and can conveniently be processed + * a subtree below a given tag was parsed. This subtree is available as DOM and can conveniently be processed * using xpath. */ public class XMLReader extends DefaultHandler { @@ -57,7 +57,7 @@ public class XMLReader extends DefaultHandler { /** * Creates a new XMLReader. *

- * Use {@link #addHandler(String, NodeHandler)} tobind handlers to tags and then call one of the parse + * Use {@link #addHandler(String, NodeHandler)} to bind handlers to tags and then call one of the parse * methods to process the XML file. *

* To interrupt processing use {@link TaskContext#cancel()}. @@ -136,7 +136,7 @@ public void startElement(String uri, String localName, String name, Attributes a * Registers a new handler for a qualified name of a node. *

* Note that this can be either the node name itself or it can be the path to the node separated by - * "/". Therefore <foo><bar> would be matched by bar and by foo/bar, where + * "/". Therefore, <foo><bar> would be matched by bar and by foo/bar, where * the path always has precedence over the single node name. *

* Handlers are invoked after the complete node was read. Namespaces are ignored for now which eases @@ -144,7 +144,7 @@ public void startElement(String uri, String localName, String name, Attributes a * could be easily added by replacing String with QName here. * * @param name the qualified name of the tag which should be parsed and processed - * @param handler the NodeHandler used to process the parsed DOM sub-tree + * @param handler the NodeHandler used to process the parsed DOM subtree */ public void addHandler(String name, NodeHandler handler) { handlers.put(name, handler); @@ -162,7 +162,7 @@ public void parse(InputStream stream) throws IOException { } /** - * Used to handle the an abort via {@link TaskContext} + * Used to handle an abort via {@link TaskContext} */ static class UserInterruptException extends RuntimeException { From 4116001ff640795d83c6fccfcbf04c00d375bc9d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yannick=20Mo=CC=88ller?= Date: Mon, 9 Dec 2024 15:59:56 +0100 Subject: [PATCH 3/3] add test and additional disallow-doctype-decl to disallow it on parsing via disallow declaration of doctypes and also disallowinf xincludes as recommended in https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html#jaxb-unmarshaller - fixes: SIRI-1037 --- .../java/sirius/kernel/xml/XMLReader.java | 2 ++ .../kotlin/sirius/kernel/xml/XmlReaderTest.kt | 22 +++++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/src/main/java/sirius/kernel/xml/XMLReader.java b/src/main/java/sirius/kernel/xml/XMLReader.java index f7c29442..f6627453 100644 --- a/src/main/java/sirius/kernel/xml/XMLReader.java +++ b/src/main/java/sirius/kernel/xml/XMLReader.java @@ -182,6 +182,8 @@ public void parse(InputStream stream, Function resourceLoca try (stream) { SAXParserFactory factory = SAXParserFactory.newInstance(); factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); + factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); + factory.setXIncludeAware(false); SAXParser saxParser = factory.newSAXParser(); org.xml.sax.XMLReader reader = saxParser.getXMLReader(); reader.setEntityResolver(new EntityResolver() { diff --git a/src/test/kotlin/sirius/kernel/xml/XmlReaderTest.kt b/src/test/kotlin/sirius/kernel/xml/XmlReaderTest.kt index 5a5d1bd5..639ae562 100644 --- a/src/test/kotlin/sirius/kernel/xml/XmlReaderTest.kt +++ b/src/test/kotlin/sirius/kernel/xml/XmlReaderTest.kt @@ -9,11 +9,13 @@ package sirius.kernel.xml import org.junit.jupiter.api.Test +import org.junit.jupiter.api.assertThrows import org.junit.jupiter.api.extension.ExtendWith import sirius.kernel.SiriusExtension import sirius.kernel.commons.ValueHolder import sirius.kernel.health.Counter import java.io.ByteArrayInputStream +import java.io.IOException import kotlin.test.assertEquals import kotlin.test.assertFalse import kotlin.test.assertTrue @@ -138,4 +140,24 @@ internal class XmlReaderTest { assertEquals(0, attributes.size) assertEquals("", attribute.get()) } + + @Test + fun `Reading an external entity is not allowed`() { + val readString = ValueHolder.of(null) + val reader = XMLReader() + reader.addHandler("root") { node: StructuredNode -> + readString.set(node.queryString(".")) + } + assertThrows { + reader.parse( + ByteArrayInputStream(//language=xml + """ + + ]> + &xxe; + """.trimIndent().toByteArray() + ) + ) + } + } }