Issue #12403 - verify XML PublicID behaviors. (#12407)

* Issue #12403 - verify XML PublicID behaviors.

* The XmlConfiguration instance should not
  use an XML parser from the classloader.
  As that XML parser can be broken when
  it comes to XML entity resolution behaviors.
  (We always want local entity resolution,
  never external)
  Switching to SAXParserFactory.newDefaultInstance() ensures this
  behavior that we want/need.
* More unit tests for behavior.
* Document XmlConfiguration override behaviors better.
* Make setFeature methods static
* Allow for configuring the SAXParser at the right time.

documentation/jetty/modules/operations-guide/pages/protocols/index.adoc

@@ -616,7 +616,7 @@ Use the following file as example, copy it as `$JETTY_BASE/webapps/wordpress.xml
 [,xml,options=nowrap]
 ----
 <?xml version="1.0"  encoding="UTF-8"?>
-<!DOCTYPE Configure PUBLIC "-//Mort Bay Consulting//DTD Configure//EN" "https://jetty.org/configure_10_0.dtd">
+<!DOCTYPE Configure PUBLIC "-//Jetty//Configure//EN" "https://jetty.org/configure_10_0.dtd">
 <Configure class="org.eclipse.jetty.server.handler.ContextHandler">
 
   <New id="root" class="java.lang.String">

jetty-core/jetty-xml/src/main/java/org/eclipse/jetty/xml/XmlConfiguration.java

@@ -51,6 +51,9 @@
 import java.util.Set;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
+import javax.xml.XMLConstants;
+import javax.xml.parsers.SAXParser;
+import javax.xml.parsers.SAXParserFactory;
 
 import org.eclipse.jetty.util.ConcurrentPool;
 import org.eclipse.jetty.util.ExceptionUtil;
@@ -69,6 +72,7 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.xml.sax.SAXException;
+import org.xml.sax.XMLReader;
 
 /**
  * <p>Configures objects from XML.</p>
@@ -2068,6 +2072,46 @@ private ConfigurationParser(Pool.Entry<ConfigurationParser> entry)
             addCatalog(URI.create(catalogUrl.toExternalForm()));
         }
 
+        protected SAXParserFactory newSAXParserFactory()
+        {
+            // Use JVM default implementation (not the one found in the classloader from non-JVM sources)
+            SAXParserFactory factory = SAXParserFactory.newDefaultInstance();
+            // Use secure processing factory level defaults (to allow
+            // newly created SAXParsers from this factory to be initialized properly
+            // for external entity handling)
+            setFeature(factory, XMLConstants.FEATURE_SECURE_PROCESSING, true);
+            // Don't allow use of XInclude to reference external entities.
+            factory.setXIncludeAware(false);
+            return factory;
+        }
+
+        @Override
+        protected void configure(SAXParser saxParser)
+        {
+            try
+            {
+                XMLReader xmlReader = getSAXParser().getXMLReader();
+
+                // disable all external entity references with Jetty's Configuration XML.
+
+                // These features configure the XMLEntityManager that the SAXParser
+                // (and Xerces) uses.   These features are applied to the current
+                // entity (the document being parsed) and how the referenced entities
+                // are to be looked up.  The resulting XMLEntityManager is configured
+                // per document being parsed.
+
+                // Configure SAX
+                setFeature(xmlReader, "http://xml.org/sax/features/external-general-entities", false);
+                setFeature(xmlReader, "http://xml.org/sax/features/external-parameter-entities", false);
+                // Configure Xerces
+                setFeature(xmlReader, "http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
+            }
+            catch (SAXException e)
+            {
+                LOG.warn(e.getMessage());
+            }
+        }
+
         @Override
         public void close()
         {

jetty-core/jetty-xml/src/main/java/org/eclipse/jetty/xml/XmlParser.java

@@ -31,6 +31,7 @@
 import javax.xml.catalog.CatalogFeatures;
 import javax.xml.catalog.CatalogManager;
 import javax.xml.catalog.CatalogResolver;
+import javax.xml.parsers.ParserConfigurationException;
 import javax.xml.parsers.SAXParser;
 import javax.xml.parsers.SAXParserFactory;
 
@@ -43,6 +44,8 @@
 import org.xml.sax.EntityResolver;
 import org.xml.sax.InputSource;
 import org.xml.sax.SAXException;
+import org.xml.sax.SAXNotRecognizedException;
+import org.xml.sax.SAXNotSupportedException;
 import org.xml.sax.SAXParseException;
 import org.xml.sax.XMLReader;
 import org.xml.sax.helpers.DefaultHandler;
@@ -66,6 +69,7 @@ public class XmlParser
     private Object _xpaths;
     private String _dtd;
     private List<EntityResolver> _entityResolvers = new ArrayList<>();
+    private SAXParserFactory factory;
 
     /**
      * Construct XmlParser
@@ -109,13 +113,47 @@ protected SAXParserFactory newSAXParserFactory()
         return SAXParserFactory.newInstance();
     }
 
+    protected SAXParser newSAXParser() throws ParserConfigurationException, SAXException
+    {
+        return factory.newSAXParser();
+    }
+
+    protected void configure(SAXParser saxParser)
+    {
+        /* override to configure sax parser at the right time */
+    }
+
+    protected static void setFeature(SAXParserFactory factory, String name, boolean value)
+    {
+        try
+        {
+            factory.setFeature(name, value);
+        }
+        catch (SAXNotSupportedException | SAXNotRecognizedException | ParserConfigurationException e)
+        {
+            LOG.warn("Unable to setFeature({}, {})", name, value, e);
+        }
+    }
+
+    protected static void setFeature(XMLReader xmlReader, String name, boolean value)
+    {
+        try
+        {
+            xmlReader.setFeature(name, value);
+        }
+        catch (SAXNotSupportedException | SAXNotRecognizedException e)
+        {
+            LOG.warn("Unable to setFeature({}, {})", name, value, e);
+        }
+    }
+
     public void setValidating(boolean validating)
     {
         try
         {
-            SAXParserFactory factory = newSAXParserFactory();
+            factory = newSAXParserFactory();
             factory.setValidating(validating);
-            _parser = factory.newSAXParser();
+            _parser = newSAXParser();
 
             try
             {
@@ -148,6 +186,10 @@ public void setValidating(boolean validating)
             LOG.warn("Unable to set validating on XML Parser", e);
             throw new Error(e.toString());
         }
+        finally
+        {
+            configure(_parser);
+        }
     }
 
     public boolean isValidating()

jetty-core/jetty-xml/src/test/java/org/eclipse/jetty/xml/XmlConfigurationTest.java

@@ -1994,29 +1994,36 @@ public void testVirtualCallWithClassAttribute() throws Exception
         assertThat(tc.testObject, instanceOf(ScheduledExecutorService.class));
     }
 
-    public static Stream<Arguments> xmlSystemIdSource()
-    {
-        List<Arguments> ids = new ArrayList<>();
-
-        String[] schemes = {"http", "https"};
-        String[] contexts = {"eclipse.org/jetty", "www.eclipse.org/jetty", "eclipse.dev/jetty", "www.eclipse.dev/jetty", "jetty.org"};
-        String[] paths = {"configure.dtd", // version-less
+    private static List<String> systemIdVariants()
+    {
+        List<String> ids = new ArrayList<>();
+        List<String> schemes = List.of("http", "https");
+        List<String> contexts = List.of("eclipse.org/jetty",
+            "www.eclipse.org/jetty",
+            "eclipse.dev/jetty",
+            "www.eclipse.dev/jetty",
+            "jetty.org");
+        List<String> paths = List.of("configure.dtd", // version-less
                           "configure_9_0.dtd", // 9.0
                           "configure_9_3.dtd", // 9.3
-                          "configure_10_0.dtd"}; // 10.
+                          "configure_10_0.dtd"); // 10.0
 
         for (String scheme: schemes)
         {
             for (String host: contexts)
             {
                 for (String path: paths)
                 {
-                    ids.add(Arguments.of(String.format("%s://%s/%s", scheme, host, path)));
+                    ids.add(String.format("%s://%s/%s", scheme, host, path));
                 }
             }
         }
+        return ids;
+    }
 
-        return ids.stream();
+    public static Stream<Arguments> xmlSystemIdSource()
+    {
+        return systemIdVariants().stream().map(Arguments::of);
     }
 
     /**
@@ -2034,6 +2041,39 @@ public void testSystemIdVariants(String xmlSystemId) throws IOException, SAXExce
         assertNotNull(inputSource, "SystemID: " + xmlSystemId + " does not exist");
     }
 
+    public static Stream<Arguments> xmlPublicIdSource()
+    {
+        List<Arguments> cases = new ArrayList<>();
+
+        // XML Public IDs seen in use with Jetty.
+        List<String> publicIds = List.of("-//Jetty//Configure//EN",
+            "-//Mort Bay Consulting//DTD Configure//EN");
+
+        for (String systemId: systemIdVariants())
+        {
+            for (String publicId: publicIds)
+            {
+                cases.add(Arguments.of(publicId, systemId));
+            }
+        }
+        return cases.stream();
+    }
+
+    /**
+     * Test to ensure that all required XML PUBLIC ID variants are covered in the
+     * {@link XmlConfiguration} internals.
+     */
+    @ParameterizedTest
+    @MethodSource("xmlPublicIdSource")
+    public void testPublicIdVariants(String xmlPublicId, String xmlSystemId) throws IOException, SAXException
+    {
+        // empty raw xml, just to instantiate XmlConfiguration, so we can access the XmlParser / ConfigurationParser.
+        XmlConfiguration xmlConfiguration = asXmlConfiguration("<Configure class=\"org.eclipse.jetty.xml.TestConfiguration\" />");
+        XmlParser configurationProcessor = xmlConfiguration.getXmlParser();
+        InputSource inputSource = configurationProcessor.resolveEntity(xmlPublicId, xmlSystemId);
+        assertNotNull(inputSource, "reference: PUBLIC " + xmlPublicId + " SYSTEM " + xmlSystemId + " does not exist");
+    }
+
     private ByteArrayOutputStream captureLoggingBytes(ThrowableAction action) throws Exception
     {
         Logger slf4jLogger = LoggerFactory.getLogger(XmlConfiguration.class);