Adding secure configuration for XML parsers

src/org/mozilla/javascript/Context.java

@@ -347,6 +347,13 @@ public class Context
      */
     public static final int FEATURE_LITTLE_ENDIAN = 19;
 
+    /**
+     * Configure the XMLProcessor to parse XML with security features or not.
+     * Security features include not fetching remote entity references and disabling XIncludes
+     * @since 1.7 Release 12
+     */
+    public static final int FEATURE_ENABLE_XML_SECURE_PARSING = 20;
+
     public static final String languageVersionProperty = "language version";
     public static final String errorReporterProperty   = "error reporter";
 

src/org/mozilla/javascript/ContextFactory.java

@@ -298,6 +298,9 @@ protected boolean hasFeature(Context cx, int featureIndex)
 
           case Context.FEATURE_LITTLE_ENDIAN:
               return false;
+
+          case Context.FEATURE_ENABLE_XML_SECURE_PARSING:
+              return true;
         }
         // It is a bug to call the method with unknown featureIndex
         throw new IllegalArgumentException(String.valueOf(featureIndex));

testsrc/org/mozilla/javascript/tests/CustomTestDBF.java

@@ -0,0 +1,44 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+package org.mozilla.javascript.tests;
+
+import javax.xml.parsers.DocumentBuilder;
+import javax.xml.parsers.DocumentBuilderFactory;
+import javax.xml.parsers.ParserConfigurationException;
+
+public class CustomTestDBF extends DocumentBuilderFactory {
+    public static final String INTENTIONAL_CONFIG_EXCEPTION = "Intentionally thrown";
+
+    @Override
+    public Object getAttribute(String arg0) throws IllegalArgumentException {
+        // TODO Auto-generated method stub
+        return null;
+    }
+
+    @Override
+    public boolean getFeature(String name) throws ParserConfigurationException {
+        // TODO Auto-generated method stub
+        return false;
+    }
+
+    @Override
+    public DocumentBuilder newDocumentBuilder() throws ParserConfigurationException {
+        throw new ParserConfigurationException(INTENTIONAL_CONFIG_EXCEPTION);
+    }
+
+    @Override
+    public void setAttribute(String name, Object value) throws IllegalArgumentException {
+        // TODO Auto-generated method stub
+
+    }
+
+    @Override
+    public void setFeature(String name, boolean value) throws ParserConfigurationException {
+        if("http://apache.org/xml/features/disallow-doctype-decl".equals(name)){
+            org.mozilla.javascript.tests.XMLSecureParserTest.CALLED_BY_XML_PARSER = true;
+        }
+    }
+
+}

testsrc/org/mozilla/javascript/tests/XMLSecureParserTest.java

@@ -0,0 +1,124 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+package org.mozilla.javascript.tests;
+
+import javax.xml.parsers.ParserConfigurationException;
+
+import org.mozilla.javascript.Context;
+import org.mozilla.javascript.ContextFactory;
+import org.mozilla.javascript.Scriptable;
+import org.mozilla.javascript.Wrapper;
+
+import junit.framework.TestCase;
+
+/**
+ * Test for secure xml parsing
+ * @author Chris Smith
+ */
+public class XMLSecureParserTest extends TestCase {
+
+    private static final String XML_PROPERTY = "javax.xml.parsers.DocumentBuilderFactory";
+    private static final String DBF_CLASSNAME = "org.mozilla.javascript.tests.CustomTestDBF";
+
+    public static boolean CALLED_BY_XML_PARSER = false;
+
+    /**
+     * Test first that XML can be run directly with the default XML parser for this JRE. 
+     * Then inject a custom parser to test that the security settings are being applied properly
+     */
+    public void testXmlSecureConfiguration() {
+        CALLED_BY_XML_PARSER = false;
+
+        // run with defaults for this JRE
+        executeXML(ContextFactory.getGlobal().enterContext());
+        assertFalse(CALLED_BY_XML_PARSER);
+
+        // store the original setting for xml, if any
+        String original = System.getProperty(XML_PROPERTY);
+        try{
+            // inject our own xml parser
+            System.setProperty(XML_PROPERTY, DBF_CLASSNAME);
+            // run with our injected parser
+            executeXML(ContextFactory.getGlobal().enterContext());
+        } catch(RuntimeException e){
+            // Our parser immediately throws a ParserConfigurationException on creating a documentbuilder, 
+            // so catch it and make sure it is the correct type of exception with the correct message 
+            //(in case another PCE is thrown for some reason)
+            assertTrue(e.getCause() instanceof ParserConfigurationException);
+            ParserConfigurationException pce = (ParserConfigurationException)e.getCause();
+            assertEquals(org.mozilla.javascript.tests.CustomTestDBF.INTENTIONAL_CONFIG_EXCEPTION, pce.getMessage());
+        } finally{
+            // if we found an xml config in the system properties, replace it
+            if(original != null) {
+                System.setProperty(XML_PROPERTY, original);
+            } else{
+                System.clearProperty(XML_PROPERTY);
+            }
+        }
+        // Our parser will set a flag on this class when configured properly, check that this happened
+        assertTrue(CALLED_BY_XML_PARSER);
+    }    
+
+    /**
+     * Test the same as above, but with the insecure configuration. This means neither the default xml parser
+     * nor the custom xml parser should be configured with the secure features.
+     */
+    public void testXmlInsecureConfiguration() {
+        CALLED_BY_XML_PARSER = false;
+
+        // run with defaults for this JRE
+        executeXML(new InsecureContextFactory().enterContext());
+        assertFalse(CALLED_BY_XML_PARSER);
+
+        // store the original setting for xml, if any
+        String original = System.getProperty(XML_PROPERTY);
+        try{
+            // inject our own xml parser
+            System.setProperty(XML_PROPERTY, DBF_CLASSNAME);
+            // run with our injected parser
+            executeXML(new InsecureContextFactory().enterContext());
+        } catch(RuntimeException e){
+            // Our parser immediately throws a ParserConfigurationException on creating a documentbuilder, 
+            // so catch it and make sure it is the correct type of exception with the correct message 
+            //(in case another PCE is thrown for some reason)
+            assertTrue(e.getCause() instanceof ParserConfigurationException);
+            ParserConfigurationException pce = (ParserConfigurationException)e.getCause();
+            assertEquals(org.mozilla.javascript.tests.CustomTestDBF.INTENTIONAL_CONFIG_EXCEPTION, pce.getMessage());
+        } finally{
+            // if we found an xml config in the system properties, replace it
+            if(original != null) {
+                System.setProperty(XML_PROPERTY, original);
+            } else{
+                System.clearProperty(XML_PROPERTY);
+            }
+            ContextFactory.initGlobal(new ContextFactory());
+        }
+        // Our parser will not set this flag as we skipped all security feature settings
+        assertFalse(CALLED_BY_XML_PARSER);
+    }   
+
+    private void executeXML(Context cx){
+        try {
+            Scriptable scope = cx.initStandardObjects();
+            cx.evaluateString(scope,
+                    "new XML('<a></a>').toXMLString();",
+                    "source", 1, null);
+        } finally {
+            Context.exit();
+        }
+    }
+
+    class InsecureContextFactory extends ContextFactory {
+
+        @Override
+        protected boolean hasFeature(Context cx, int featureIndex) {
+            switch (featureIndex) {
+            case Context.FEATURE_ENABLE_XML_SECURE_PARSING:
+                return false;
+            }
+            return super.hasFeature(cx, featureIndex);
+        }
+    }
+}

xmlimplsrc/org/mozilla/javascript/xmlimpl/XmlProcessor.java

@@ -11,8 +11,11 @@
 import java.util.List;
 import java.util.concurrent.LinkedBlockingDeque;
 
+import javax.xml.XMLConstants;
 import javax.xml.parsers.DocumentBuilder;
+import javax.xml.parsers.DocumentBuilderFactory;
 import javax.xml.parsers.ParserConfigurationException;
+import javax.xml.transform.TransformerConfigurationException;
 
 import org.mozilla.javascript.Context;
 import org.mozilla.javascript.ScriptRuntime;
@@ -45,13 +48,56 @@ class XmlProcessor implements Serializable {
 
     private void readObject(ObjectInputStream stream) throws IOException, ClassNotFoundException {
         stream.defaultReadObject();
-        this.dom = javax.xml.parsers.DocumentBuilderFactory.newInstance();
+        this.dom = DocumentBuilderFactory.newInstance();
         this.dom.setNamespaceAware(true);
         this.dom.setIgnoringComments(false);
+        //create TF and set settings to secure it from XSLT attacks if given a malicious node in toXMLString
         this.xform = javax.xml.transform.TransformerFactory.newInstance();
+        Context ctx = Context.getCurrentContext();
+        if(ctx == null || ctx.hasFeature(Context.FEATURE_ENABLE_XML_SECURE_PARSING)) {
+            configureSecureDBF(this.dom);
+            configureSecureTF(this.xform);
+        }
         int poolSize = Runtime.getRuntime().availableProcessors() * 2;
         this.documentBuilderPool = new LinkedBlockingDeque<DocumentBuilder>(poolSize);
     }
+
+    /*
+     * Secure implementation of a DocumentBuilderFactory to prevent XXE and SSRF attacks 
+     */
+    private void configureSecureDBF(DocumentBuilderFactory dbf){
+        try {
+            //Prevent File attacks in DBF
+            //Disallow all doctypes, removing all ENTITY-type tags as a vector
+            dbf.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
+
+            //Prevent SSRF attacks in DBF
+            //Do not load external dtds, if the underlying DocBuilderFactory is set for a validation mode
+            dbf.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
+
+            //Disallow XIncludeAware as it is an SSRF target using xi:include
+            dbf.setXIncludeAware(false);
+
+        } catch (ParserConfigurationException e) {
+            // Following the other config exception handling
+            throw new RuntimeException("XML parser (DocumentBuilderFactory) cannot be securely configured.", e);
+        }
+    }
+
+    /*
+     * Secure implementation of a TransformerFactory to prevent XXE and SSRF attacks 
+     */
+    private void configureSecureTF(javax.xml.transform.TransformerFactory xform){
+        try {
+            //Disallow all XXEs and SSRF via external calls for DTDs or Stylesheets
+            xform.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
+            xform.setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, "");
+            xform.setAttribute(XMLConstants.ACCESS_EXTERNAL_STYLESHEET, "");
+        } catch (TransformerConfigurationException e) {
+            // Following the other config exception handling
+            throw new RuntimeException("XML parser (TransformerFactory) cannot be securely configured.", e);
+        }
+    }
 
     private static class RhinoSAXErrorHandler implements ErrorHandler, Serializable {
 
@@ -77,10 +123,16 @@ public void warning(SAXParseException e) {
 
     XmlProcessor() {
         setDefault();
-        this.dom = javax.xml.parsers.DocumentBuilderFactory.newInstance();
+        this.dom = DocumentBuilderFactory.newInstance();
         this.dom.setNamespaceAware(true);
         this.dom.setIgnoringComments(false);
+        //create TF and set settings to secure it from XSLT attacks if given a malicious node in toXMLString
         this.xform = javax.xml.transform.TransformerFactory.newInstance();
+        Context ctx = Context.getCurrentContext();
+        if(ctx == null || ctx.hasFeature(Context.FEATURE_ENABLE_XML_SECURE_PARSING)) {
+            configureSecureDBF(this.dom);
+            configureSecureTF(this.xform);
+        }
         int poolSize = Runtime.getRuntime().availableProcessors() * 2;
         this.documentBuilderPool = new LinkedBlockingDeque<DocumentBuilder>(poolSize);
     }