From cda726366c21790dcb8628f6e32f2cf8864449fb Mon Sep 17 00:00:00 2001 From: Jody Garnett Date: Sun, 18 Sep 2016 16:57:42 +1000 Subject: [PATCH] EntityResolver for DocumentFactory [GEOT-5514] --- .../org/geotools/xml/DocumentFactory.java | 90 +++++++------------ .../org/geotools/xml/XMLHandlerHints.java | 20 +++-- .../java/org/geotools/xml/XMLSAXHandler.java | 86 ++++++++++-------- .../xml/handlers/DocumentHandler.java | 1 + .../org/geotools/xml/DocumentFactoryTest.java | 26 +++--- 5 files changed, 106 insertions(+), 117 deletions(-) diff --git a/modules/library/xml/src/main/java/org/geotools/xml/DocumentFactory.java b/modules/library/xml/src/main/java/org/geotools/xml/DocumentFactory.java index a9b8a57ef44..70747a13c88 100644 --- a/modules/library/xml/src/main/java/org/geotools/xml/DocumentFactory.java +++ b/modules/library/xml/src/main/java/org/geotools/xml/DocumentFactory.java @@ -26,22 +26,31 @@ import javax.xml.parsers.SAXParser; import javax.xml.parsers.SAXParserFactory; +import org.geotools.xml.handlers.DocumentHandler; +import org.geotools.xml.schema.Schema; import org.xml.sax.SAXException; /** * This is the main entry point into the XSI parsing routines. *

* Example Use: - * * 

  *     Object x = DocumentFactory.getInstance(new URI("MyInstanceDocumentURI");
  *
- * *

+ *

+ * A selection of the hints available to configure parsing: + *

+ * * * @author dzwiers, Refractions Research, Inc. http://www.refractions.net - * @author $Author:$ (last modification) - * * * @source $URL$ * http://svn.osgeo.org/geotools/trunk/modules/library/xml/src/main/java/org/geotools/xml @@ -56,6 +65,13 @@ public class DocumentFactory { * the resulting objects is weekend by turning this param to false. */ public static final String VALIDATION_HINT = "DocumentFactory_VALIDATION_HINT"; + + /** + * When this hint is contained and set to Boolean.TRUE, external entities will be disabled. This + * setting is used to alivate XXE attacks, preventing both {@link #VALIDATION_HINT} and + * {@link XMLHandlerHints#ENTITY_RESOLVER} from being effective. + */ + public static final String DISABLE_EXTERNAL_ENTITIES = "DocumentFactory_DISABLE_EXTERNAL_ENTITIES"; /** *

@@ -72,8 +88,8 @@ public class DocumentFactory { * * @see DocumentFactory#getInstance(URI, Map, Level) */ - public static Object getInstance(URI desiredDocument, Map hints) throws SAXException { - return getInstance(desiredDocument, hints, Level.WARNING, false); + public static Object getInstance(URI desiredDocument, Map hints) throws SAXException { + return getInstance(desiredDocument, hints, Level.WARNING); } /** @@ -94,31 +110,10 @@ public static Object getInstance(URI desiredDocument, Map hints) throws SAXExcep * * @see DocumentFactory#getInstance(URI, Map, Level, boolean) */ - public static Object getInstance(URI desiredDocument, Map hints, Level level) + public static Object getInstance(URI desiredDocument, @SuppressWarnings("rawtypes") Map hints, Level level) throws SAXException { - return getInstance(desiredDocument, hints, level, false); - } - - /** - *

- * Parses the instance data provided. This method assumes that the XML document is fully - * described using XML Schemas. Failure to be fully described as Schemas will result in errors, - * as opposed to a vid parse. - *

- * - * @param desiredDocument - * @param hints - * May be null. - * @param level - * @param parseExternalEntities - * - * @return Object - * - * @throws SAXException - */ - public static Object getInstance(URI desiredDocument, Map hints, Level level, - boolean parseExternalEntities) throws SAXException { - SAXParser parser = getParser(parseExternalEntities); + @SuppressWarnings("unchecked") + SAXParser parser = getParser(hints); XMLSAXHandler xmlContentHandler = new XMLSAXHandler(desiredDocument, hints); XMLSAXHandler.setLogLevel(level); @@ -150,30 +145,8 @@ public static Object getInstance(URI desiredDocument, Map hints, Level level, * * @see DocumentFactory#getInstance(InputStream, Map, Level, boolean) */ - public static Object getInstance(InputStream is, Map hints, Level level) throws SAXException { - return getInstance(is, hints, level, false); - } - - /** - *

- * Parses the instance data provided. This method assumes that the XML document is fully - * described using XML Schemas. Failure to be fully described as Schemas will result in errors, - * as opposed to a vid parse. - *

- * - * @param is - * @param hints - * May be null. - * @param level - * @param parseExternalEntities - * - * @return Object - * - * @throws SAXException - */ - public static Object getInstance(InputStream is, Map hints, Level level, - boolean parseExternalEntities) throws SAXException { - SAXParser parser = getParser(parseExternalEntities); + public static Object getInstance(InputStream is, Map hints, Level level) throws SAXException { + SAXParser parser = getParser(hints); XMLSAXHandler xmlContentHandler = new XMLSAXHandler(hints); XMLSAXHandler.setLogLevel(level); @@ -191,19 +164,22 @@ public static Object getInstance(InputStream is, Map hints, Level level, /* * Convenience method to create an instance of a SAXParser if it is null. */ - private static SAXParser getParser(boolean parseExternalEntities) throws SAXException { + private static SAXParser getParser(Map hints) throws SAXException { SAXParserFactory spf = SAXParserFactory.newInstance(); spf.setNamespaceAware(true); spf.setValidating(false); - try { - if(!parseExternalEntities) { + if (hints != null && hints.containsKey(DISABLE_EXTERNAL_ENTITIES) + && Boolean.TRUE.equals(hints.get(DISABLE_EXTERNAL_ENTITIES))) { // The following configuration prevents XML External Entity Injection (XXE) attacks // See for more information: // https://www.owasp.org/index.php/XML_External_Entity_(XXE)_Processing spf.setFeature("http://xml.org/sax/features/external-general-entities", false); spf.setFeature("http://xml.org/sax/features/external-parameter-entities", false); } + // This is an XML Schema driven parser, no DTD required (XMLSaxHandler will reject all dtd references) + spf.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); + spf.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false); SAXParser sp = spf.newSAXParser(); return sp; diff --git a/modules/library/xml/src/main/java/org/geotools/xml/XMLHandlerHints.java b/modules/library/xml/src/main/java/org/geotools/xml/XMLHandlerHints.java index 136c58e2dc4..e40610d8774 100644 --- a/modules/library/xml/src/main/java/org/geotools/xml/XMLHandlerHints.java +++ b/modules/library/xml/src/main/java/org/geotools/xml/XMLHandlerHints.java @@ -30,7 +30,7 @@ * * @source $URL$ */ -public class XMLHandlerHints implements Map { +public class XMLHandlerHints implements Map { /** * Declares the schemas to use for parsing. @@ -44,6 +44,8 @@ public class XMLHandlerHints implements Map { public static final String STREAM_HINT = "org.geotools.xml.gml.STREAM_HINT"; /** Sets the level of compliance that the filter encoder should use */ public static final String FILTER_COMPLIANCE_STRICTNESS = "org.geotools.xml.filter.FILTER_COMPLIANCE_STRICTNESS"; + /** Supplied {@link EntityResolver} for Schema and/or DTD validation */ + public final static String ENTITY_RESOLVER ="org.xml.sax.EntityResolver"; /** * The value so that the parser will encode all Geotools filters with no modifications. */ @@ -119,7 +121,7 @@ public class XMLHandlerHints implements Map { public static final Integer VALUE_FILTER_COMPLIANCE_HIGH = new Integer(2); - private Map map=new HashMap(); + private Map map=new HashMap(); public void clear() { map.clear(); } @@ -132,7 +134,7 @@ public boolean containsValue( Object value ) { return map.containsValue(value); } - public Set entrySet() { + public Set> entrySet() { return map.entrySet(); } @@ -152,16 +154,16 @@ public boolean isEmpty() { return map.isEmpty(); } - public Set keySet() { + public Set keySet() { return map.keySet(); } - public Object put( Object arg0, Object arg1 ) { - return map.put(arg0, arg1); + public Object put( String key, Object value ) { + return map.put(key, value); } - public void putAll( Map arg0 ) { - map.putAll(arg0); + public void putAll( Map other ) { + map.putAll(other); } public Object remove( Object key ) { @@ -172,7 +174,7 @@ public int size() { return map.size(); } - public Collection values() { + public Collection values() { return map.values(); } diff --git a/modules/library/xml/src/main/java/org/geotools/xml/XMLSAXHandler.java b/modules/library/xml/src/main/java/org/geotools/xml/XMLSAXHandler.java index b31e7d6448b..6e243a4174e 100644 --- a/modules/library/xml/src/main/java/org/geotools/xml/XMLSAXHandler.java +++ b/modules/library/xml/src/main/java/org/geotools/xml/XMLSAXHandler.java @@ -20,6 +20,7 @@ import java.io.StringReader; import java.net.URI; import java.net.URISyntaxException; +import java.util.Collections; import java.util.HashMap; import java.util.Iterator; import java.util.Map; @@ -33,6 +34,7 @@ import org.geotools.xml.handlers.ElementHandlerFactory; import org.geotools.xml.handlers.IgnoreHandler; import org.xml.sax.Attributes; +import org.xml.sax.EntityResolver; import org.xml.sax.InputSource; import org.xml.sax.Locator; import org.xml.sax.SAXException; @@ -56,10 +58,13 @@ * periodically check it to see if it should stop parsing. See the FlowHandler * interface. *

+ * + *

+ * This is an XML Schema driven parser and {@link #resolveEntity(String, String)} will + * ignore all dtd references. If an {@link EntityResolver} is provided it will be used. + *

* * @author dzwiers, Refractions Research, Inc. http://www.refractions.net - * @author $Author:$ (last modification) - * * * @source $URL$ * @version $Id$ @@ -82,40 +87,41 @@ public class XMLSAXHandler extends DefaultHandler { */ private StringBuffer characters = new StringBuffer(); + + /** entity resolver */ + EntityResolver entityResolver; + + public void setEntityResolver(EntityResolver entityResolver) { + this.entityResolver = entityResolver; + } + + public EntityResolver getEntityResolver() { + return entityResolver; + } + /** + * Delegate to {@link #entityResolver} if available. + *

+ *

+ * Note this is an XMLSchema based parser, all attempts to resolved DTDs are rejected. + *

* - * TODO summary sentence for resolveEntity ... - * - * @see org.xml.sax.EntityResolver#resolveEntity(java.lang.String, java.lang.String) - * @param pubId - * @param sysId - * @return InputSource - * @throws SAXException + * @param publicId The public identifier, or null if none is available. + * @param systemId The system identifier provided in the XML document. + * @return The new input source, or null to require the default behavior. + * @exception java.io.IOException If there is an error setting up the new input source. + * @exception org.xml.sax.SAXException Any SAX exception, possibly wrapping another exception. */ - public InputSource resolveEntity( String pubId, String sysId ) throws SAXException { + public InputSource resolveEntity( String publicId, String systemId ) throws SAXException, IOException { // avoid dtd files - if(sysId != null && sysId.endsWith("dtd")){ - return new InputSource(new StringReader("")); - } - try{ - if (false) { - /* - * HACK: This dead code exists only in order to make J2SE 1.4 compiler happy. - * This hack is needed because there is a slight API change between J2SE 1.4 - * and 1.5: the 'resolveEntity()' method didn't declared IOException in its - * throw clause in J2SE 1.4. Compare the two following links: - * - * http://java.sun.com/j2se/1.5/docs/api/org/xml/sax/helpers/DefaultHandler.html#resolveEntity(java.lang.String,%20java.lang.String) - * http://java.sun.com/j2se/1.4/docs/api/org/xml/sax/helpers/DefaultHandler.html#resolveEntity(java.lang.String,%20java.lang.String) - */ - throw new IOException(); - } - return super.resolveEntity(pubId,sysId); - }catch(IOException e){ - SAXException se = new SAXException(e.getLocalizedMessage()); - se.initCause(e); - throw se; - } + if (systemId != null && systemId.endsWith("dtd")) { + return new InputSource(new StringReader("")); + } + if (entityResolver != null) { + return entityResolver.resolveEntity(publicId, systemId); + } else { + return super.resolveEntity(publicId, systemId); + } } // hints private Map hints; @@ -147,7 +153,7 @@ public InputSource resolveEntity( String pubId, String sysId ) throws SAXExcepti */ public XMLSAXHandler(URI intendedDocument, Map hints) { instanceDocument = intendedDocument; - this.hints = hints; + init(hints); logger.setLevel(level); } @@ -158,13 +164,21 @@ public XMLSAXHandler(URI intendedDocument, Map hints) { * be provided, as this will allow the parser to resolve relative uri's. *

* - * @param hints DOCUMENT ME! + * @param hints Hints as per {@link {@link XMLHandlerHints} */ public XMLSAXHandler(Map hints) { - this.hints = hints; + init(hints); logger.setLevel(level); } - + protected void init(Map hints){ + if( hints == null ){ + hints = Collections.EMPTY_MAP; + } + this.hints = hints; + if( hints.containsKey(XMLHandlerHints.ENTITY_RESOLVER)){ + setEntityResolver( (EntityResolver) hints.get(XMLHandlerHints.ENTITY_RESOLVER));; + } + } /** * Implementation of endDocument. * diff --git a/modules/library/xml/src/main/java/org/geotools/xml/handlers/DocumentHandler.java b/modules/library/xml/src/main/java/org/geotools/xml/handlers/DocumentHandler.java index 9bd727ce719..d0e9988f982 100644 --- a/modules/library/xml/src/main/java/org/geotools/xml/handlers/DocumentHandler.java +++ b/modules/library/xml/src/main/java/org/geotools/xml/handlers/DocumentHandler.java @@ -39,6 +39,7 @@ * @source $URL$ */ public class DocumentHandler extends XMLElementHandler { + /** Supplied {@link Schema} for parsing and validation */ public final static String DEFAULT_NAMESPACE_HINT_KEY = "org.geotools.xml.handlers.DocumentHandler.DEFAULT_NAMESPACE_HINT_KEY"; private XMLElementHandler xeh = null; private ElementHandlerFactory ehf; diff --git a/modules/library/xml/src/test/java/org/geotools/xml/DocumentFactoryTest.java b/modules/library/xml/src/test/java/org/geotools/xml/DocumentFactoryTest.java index 65f02ca206d..a92b6f0702d 100644 --- a/modules/library/xml/src/test/java/org/geotools/xml/DocumentFactoryTest.java +++ b/modules/library/xml/src/test/java/org/geotools/xml/DocumentFactoryTest.java @@ -54,7 +54,7 @@ public class DocumentFactoryTest { private URI uri; - private Map hints; + private Map hints; @Mock private SAXParserFactory mockSaxParserFactory; @@ -68,7 +68,7 @@ public class DocumentFactoryTest { @Before public void before() throws Exception { uri = new URI("http://geotools.org"); - hints = new HashMap(); + hints = new HashMap(); MockitoAnnotations.initMocks(this); @@ -92,18 +92,10 @@ public Void answer(InvocationOnMock invocationOnMock) throws Throwable { .parse(any(InputStream.class), any(XMLSAXHandler.class)); } - @Test - public void testGetInstanceFromURINoSpecifiedLevelOrParseExternalEntities() - throws Exception { - DocumentFactory.getInstance(uri, hints); - - verify(mockSaxParserFactory).setFeature(EXTERNAL_GENERAL_ENTITIES_FEATURE, false); - verify(mockSaxParserFactory).setFeature(EXTERNAL_PARAMETER_ENTITIES_FEATURE, false); - } - @Test public void testGetInstanceFromURIWithSpecifiedLevelButNoParseExternalEntities() throws Exception { + hints.put(DocumentFactory.DISABLE_EXTERNAL_ENTITIES, Boolean.TRUE); DocumentFactory.getInstance(uri, hints, Level.WARNING); verify(mockSaxParserFactory).setFeature(EXTERNAL_GENERAL_ENTITIES_FEATURE, false); @@ -113,7 +105,8 @@ public void testGetInstanceFromURIWithSpecifiedLevelButNoParseExternalEntities() @Test public void testGetInstanceFromURIWithSpecifiedLevelAndParseExternalEntitiesTrue() throws Exception { - DocumentFactory.getInstance(uri, hints, Level.WARNING, true); + hints.put(DocumentFactory.DISABLE_EXTERNAL_ENTITIES, Boolean.TRUE); + DocumentFactory.getInstance(uri, hints, Level.WARNING); verify(mockSaxParserFactory, never()) .setFeature(EXTERNAL_GENERAL_ENTITIES_FEATURE, false); @@ -124,7 +117,8 @@ public void testGetInstanceFromURIWithSpecifiedLevelAndParseExternalEntitiesTrue @Test public void testGetInstanceFromURIWithSpecifiedLevelAndParseExternalEntitiesFalse() throws Exception { - DocumentFactory.getInstance(uri, hints, Level.WARNING, false); + hints.put(DocumentFactory.DISABLE_EXTERNAL_ENTITIES, Boolean.FALSE); + DocumentFactory.getInstance(uri, hints, Level.WARNING); verify(mockSaxParserFactory).setFeature(EXTERNAL_GENERAL_ENTITIES_FEATURE, false); verify(mockSaxParserFactory).setFeature(EXTERNAL_PARAMETER_ENTITIES_FEATURE, false); @@ -166,7 +160,8 @@ public void testGetInstanceFromInputStreamWithSpecifiedLevelNoParseExternalEntit @Test public void testGetInstanceFromInputStreamWithSpecifiedLevelAndParseExternalEntitiesTrue() throws Exception { - DocumentFactory.getInstance(inputStream, hints, Level.WARNING, true); + hints.put(DocumentFactory.DISABLE_EXTERNAL_ENTITIES, Boolean.TRUE); + DocumentFactory.getInstance(inputStream, hints, Level.WARNING); verify(mockSaxParserFactory, never()) .setFeature(EXTERNAL_GENERAL_ENTITIES_FEATURE, false); @@ -177,7 +172,8 @@ public void testGetInstanceFromInputStreamWithSpecifiedLevelAndParseExternalEnti @Test public void testGetInstanceFromInputStreamWithSpecifiedLevelAndParseExternalEntitiesFalse() throws Exception { - DocumentFactory.getInstance(inputStream, hints, Level.WARNING, false); + hints.put(DocumentFactory.DISABLE_EXTERNAL_ENTITIES, Boolean.FALSE); + DocumentFactory.getInstance(inputStream, hints, Level.WARNING); verify(mockSaxParserFactory).setFeature(EXTERNAL_GENERAL_ENTITIES_FEATURE, false); verify(mockSaxParserFactory).setFeature(EXTERNAL_PARAMETER_ENTITIES_FEATURE, false);