From 4b80871cb72389d67aa1ab1fb91091c22f1a6100 Mon Sep 17 00:00:00 2001 From: Ondrej Dubaj Date: Fri, 24 Jul 2020 14:08:54 +0200 Subject: [PATCH] Fix for XXE vulnerability by defaulting to disabling external access and doc types. The legacy insecure behavior can be restored via the new connection property xmlFactoryFactory with a value of LEGACY_INSECURE. Alternatively, a custom class name can be specified that implements org.postgresql.xml.PGXmlFactoryFactory and takes a no argument constructor. * refactor: Clean up whitespace in existing PgSQLXMLTest * fix: Fix XXE vulnerability in PgSQLXML by disabling external access and doctypes * fix: Add missing getter and setter for XML_FACTORY_FACTORY to BasicDataSource --- build.xml | 4 + org/postgresql/Driver.java.in | 4 +- org/postgresql/core/BaseConnection.java | 9 ++ org/postgresql/ds/common/BaseDataSource.java | 10 ++ org/postgresql/jdbc4/Jdbc4Connection.java | 44 ++++++ org/postgresql/jdbc4/Jdbc4SQLXML.java | 37 +++-- .../xml/DefaultPGXmlFactoryFactory.java | 140 ++++++++++++++++++ .../xml/EmptyStringEntityResolver.java | 23 +++ .../LegacyInsecurePGXmlFactoryFactory.java | 57 +++++++ org/postgresql/xml/NullErrorHandler.java | 25 ++++ org/postgresql/xml/PGXmlFactoryFactory.java | 30 ++++ 11 files changed, 362 insertions(+), 21 deletions(-) create mode 100644 org/postgresql/xml/DefaultPGXmlFactoryFactory.java create mode 100644 org/postgresql/xml/EmptyStringEntityResolver.java create mode 100644 org/postgresql/xml/LegacyInsecurePGXmlFactoryFactory.java create mode 100644 org/postgresql/xml/NullErrorHandler.java create mode 100644 org/postgresql/xml/PGXmlFactoryFactory.java diff --git a/build.xml b/build.xml index c9c5660..59b81bf 100644 --- a/build.xml +++ b/build.xml @@ -133,6 +133,7 @@ + + + diff --git a/org/postgresql/Driver.java.in b/org/postgresql/Driver.java.in index f6c69c0..669f0b6 100644 --- a/org/postgresql/Driver.java.in +++ b/org/postgresql/Driver.java.in @@ -465,7 +465,9 @@ public class Driver implements java.sql.Driver { "kerberosServerName", Boolean.FALSE, "The Kerberos service name to use when authenticating with GSSAPI. This is equivalent to libpq's PGKRBSRVNAME environment variable." }, { "jaasApplicationName", Boolean.FALSE, - "Specifies the name of the JAAS system or application login configuration." } + "Specifies the name of the JAAS system or application login configuration." }, + { "xmlFactoryFactory", Boolean.FALSE, + "Factory class to instantiate factories for XML processing" } }; /** diff --git a/org/postgresql/core/BaseConnection.java b/org/postgresql/core/BaseConnection.java index 6373d53..69192af 100644 --- a/org/postgresql/core/BaseConnection.java +++ b/org/postgresql/core/BaseConnection.java @@ -10,6 +10,7 @@ package org.postgresql.core; import java.sql.*; import org.postgresql.PGConnection; import org.postgresql.jdbc2.TimestampUtils; +import org.postgresql.xml.PGXmlFactoryFactory; /** * Driver-internal connection interface. Application code should not use @@ -154,4 +155,12 @@ public interface BaseConnection extends PGConnection, Connection * @return True for binary transfer, false for text transfer. */ public boolean binaryTransferSend(int oid); + + /** + * Retrieve the factory to instantiate XML processing factories. + * + * @return The factory to use to instantiate XML processing factories + * @throws SQLException if the class cannot be found or instantiated. + */ + PGXmlFactoryFactory getXmlFactoryFactory() throws SQLException; } diff --git a/org/postgresql/ds/common/BaseDataSource.java b/org/postgresql/ds/common/BaseDataSource.java index bfe54f7..9be3c4c 100644 --- a/org/postgresql/ds/common/BaseDataSource.java +++ b/org/postgresql/ds/common/BaseDataSource.java @@ -16,6 +16,7 @@ import java.io.ObjectInputStream; import java.io.ObjectOutputStream; import java.io.ByteArrayInputStream; import java.io.ByteArrayOutputStream; +import java.util.Properties; /** * Base class for data sources and related classes. @@ -63,6 +64,7 @@ public abstract class BaseDataSource implements Referenceable private int logLevel = 0; private int protocolVersion = 0; private String applicationName; + private String xmlFactoryFactory = ""; /** * Gets a connection to the PostgreSQL database. The database is identified by the @@ -629,4 +631,12 @@ public abstract class BaseDataSource implements Referenceable readBaseObject(ois); } + public String getXmlFactoryFactory() { + return this.xmlFactoryFactory; + } + + public void setXmlFactoryFactory(String xmlFactoryFactory) { + this.xmlFactoryFactory = xmlFactoryFactory; + } + } diff --git a/org/postgresql/jdbc4/Jdbc4Connection.java b/org/postgresql/jdbc4/Jdbc4Connection.java index 209b970..956a545 100644 --- a/org/postgresql/jdbc4/Jdbc4Connection.java +++ b/org/postgresql/jdbc4/Jdbc4Connection.java @@ -10,6 +10,12 @@ package org.postgresql.jdbc4; import java.util.Map; import java.util.Properties; import java.sql.SQLException; +import org.postgresql.util.GT; +import org.postgresql.util.PSQLState; +import org.postgresql.xml.DefaultPGXmlFactoryFactory; +import org.postgresql.xml.LegacyInsecurePGXmlFactoryFactory; +import org.postgresql.xml.PGXmlFactoryFactory; +import org.postgresql.util.PSQLException; import org.postgresql.util.HostSpec; @@ -20,8 +26,13 @@ import org.postgresql.util.HostSpec; */ public class Jdbc4Connection extends AbstractJdbc4Connection implements java.sql.Connection { + + private final String xmlFactoryFactoryClass; + private PGXmlFactoryFactory xmlFactoryFactory; + public Jdbc4Connection(HostSpec[] hostSpecs, String user, String database, Properties info, String url) throws SQLException { super(hostSpecs, user, database, info, url); + xmlFactoryFactoryClass = info.getProperty("xmlFactoryFactory"); } public java.sql.Statement createStatement(int resultSetType, int resultSetConcurrency, int resultSetHoldability) throws SQLException @@ -62,4 +73,37 @@ public class Jdbc4Connection extends AbstractJdbc4Connection implements java.sql setTypeMapImpl(map); } + @Override + public PGXmlFactoryFactory getXmlFactoryFactory() throws SQLException { + if (xmlFactoryFactory == null) { + if (xmlFactoryFactoryClass == null || xmlFactoryFactoryClass.equals("")) { + xmlFactoryFactory = DefaultPGXmlFactoryFactory.INSTANCE; + } else if (xmlFactoryFactoryClass.equals("LEGACY_INSECURE")) { + xmlFactoryFactory = LegacyInsecurePGXmlFactoryFactory.INSTANCE; + } else { + Class clazz; + try { + clazz = Class.forName(xmlFactoryFactoryClass); + } catch (ClassNotFoundException ex) { + throw new PSQLException( + GT.tr("Could not instantiate xmlFactoryFactory: {0}", xmlFactoryFactoryClass), + PSQLState.INVALID_PARAMETER_VALUE, ex); + } + if (!clazz.isAssignableFrom(PGXmlFactoryFactory.class)) { + throw new PSQLException( + GT.tr("Connection property xmlFactoryFactory must implement PGXmlFactoryFactory: {0}", xmlFactoryFactoryClass), + PSQLState.INVALID_PARAMETER_VALUE); + } + try { + xmlFactoryFactory = (PGXmlFactoryFactory) clazz.newInstance(); + } catch (Exception ex) { + throw new PSQLException( + GT.tr("Could not instantiate xmlFactoryFactory: {0}", xmlFactoryFactoryClass), + PSQLState.INVALID_PARAMETER_VALUE, ex); + } + } + } + return xmlFactoryFactory; + } + } diff --git a/org/postgresql/jdbc4/Jdbc4SQLXML.java b/org/postgresql/jdbc4/Jdbc4SQLXML.java index 2302185..7b4ebba 100644 --- a/org/postgresql/jdbc4/Jdbc4SQLXML.java +++ b/org/postgresql/jdbc4/Jdbc4SQLXML.java @@ -4,6 +4,8 @@ import org.postgresql.util.GT; import org.postgresql.util.PSQLState; import org.postgresql.util.PSQLException; import org.postgresql.core.BaseConnection; +import org.postgresql.xml.PGXmlFactoryFactory; +import org.postgresql.xml.DefaultPGXmlFactoryFactory; import java.io.*; import java.sql.SQLXML; @@ -14,7 +16,6 @@ import javax.xml.transform.Result; import javax.xml.transform.dom.DOMSource; import javax.xml.transform.dom.DOMResult; import javax.xml.parsers.DocumentBuilder; -import javax.xml.parsers.DocumentBuilderFactory; import javax.xml.transform.Transformer; import javax.xml.transform.TransformerFactory; import javax.xml.transform.TransformerException; @@ -22,8 +23,7 @@ import javax.xml.transform.TransformerException; import javax.xml.transform.sax.SAXSource; import javax.xml.transform.sax.SAXResult; import org.xml.sax.InputSource; -import org.xml.sax.ErrorHandler; -import org.xml.sax.SAXParseException; +import org.xml.sax.XMLReader; import javax.xml.transform.sax.SAXTransformerFactory; import javax.xml.transform.sax.TransformerHandler; @@ -69,6 +69,13 @@ public class Jdbc4SQLXML implements SQLXML { _freed = false; } + private PGXmlFactoryFactory getXmlFactoryFactory() throws SQLException { + if (_conn != null) { + return _conn.getXmlFactoryFactory(); + } + return DefaultPGXmlFactoryFactory.INSTANCE; + } + public synchronized void free() { _freed = true; @@ -122,16 +129,15 @@ public class Jdbc4SQLXML implements SQLXML { try { if (sourceClass == null || DOMSource.class.equals(sourceClass)) { - DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); - DocumentBuilder builder = factory.newDocumentBuilder(); - builder.setErrorHandler(new NonPrintingErrorHandler()); + DocumentBuilder builder = getXmlFactoryFactory().newDocumentBuilder(); InputSource input = new InputSource(new StringReader(_data)); return new DOMSource(builder.parse(input)); } else if (SAXSource.class.equals(sourceClass)) { + XMLReader reader = getXmlFactoryFactory().createXMLReader(); InputSource is = new InputSource(new StringReader(_data)); - return new SAXSource(is); + return new SAXSource(reader, is); } else if (StreamSource.class.equals(sourceClass)) { @@ -139,7 +145,7 @@ public class Jdbc4SQLXML implements SQLXML { } else if (StAXSource.class.equals(sourceClass)) { - XMLInputFactory xif = XMLInputFactory.newInstance(); + XMLInputFactory xif = getXmlFactoryFactory().newXMLInputFactory(); XMLStreamReader xsr = xif.createXMLStreamReader(new StringReader(_data)); return new StAXSource(xsr); } @@ -185,7 +191,7 @@ public class Jdbc4SQLXML implements SQLXML { return _domResult; } else if (SAXResult.class.equals(resultClass)) { try { - SAXTransformerFactory transformerFactory = (SAXTransformerFactory)SAXTransformerFactory.newInstance(); + SAXTransformerFactory transformerFactory = getXmlFactoryFactory().newSAXTransformerFactory(); TransformerHandler transformerHandler = transformerFactory.newTransformerHandler(); _stringWriter = new StringWriter(); transformerHandler.setResult(new StreamResult(_stringWriter)); @@ -201,7 +207,7 @@ public class Jdbc4SQLXML implements SQLXML { } else if (StAXResult.class.equals(resultClass)) { _stringWriter = new StringWriter(); try { - XMLOutputFactory xof = XMLOutputFactory.newInstance(); + XMLOutputFactory xof = getXmlFactoryFactory().newXMLOutputFactory(); XMLStreamWriter xsw = xof.createXMLStreamWriter(_stringWriter); _active = true; return new StAXResult(xsw); @@ -258,7 +264,7 @@ public class Jdbc4SQLXML implements SQLXML { // and use the identify transform to get it into a // friendlier result format. try { - TransformerFactory factory = TransformerFactory.newInstance(); + TransformerFactory factory = getXmlFactoryFactory().newTransformerFactory(); Transformer transformer = factory.newTransformer(); DOMSource domSource = new DOMSource(_domResult.getNode()); StringWriter stringWriter = new StringWriter(); @@ -284,14 +290,5 @@ public class Jdbc4SQLXML implements SQLXML { _initialized = true; } - // Don't clutter System.err with errors the user can't silence. - // If something bad really happens an exception will be thrown. - static class NonPrintingErrorHandler implements ErrorHandler - { - public void error(SAXParseException e) { } - public void fatalError(SAXParseException e) { } - public void warning(SAXParseException e) { } - } - } diff --git a/org/postgresql/xml/DefaultPGXmlFactoryFactory.java b/org/postgresql/xml/DefaultPGXmlFactoryFactory.java new file mode 100644 index 0000000..7334935 --- /dev/null +++ b/org/postgresql/xml/DefaultPGXmlFactoryFactory.java @@ -0,0 +1,140 @@ +/* + * Copyright (c) 2020, PostgreSQL Global Development Group + * See the LICENSE file in the project root for more information. + */ + +package org.postgresql.xml; + +import org.xml.sax.SAXException; +import org.xml.sax.XMLReader; +import org.xml.sax.helpers.XMLReaderFactory; + +import javax.xml.XMLConstants; +import javax.xml.parsers.DocumentBuilder; +import javax.xml.parsers.DocumentBuilderFactory; +import javax.xml.parsers.ParserConfigurationException; +import javax.xml.stream.XMLInputFactory; +import javax.xml.stream.XMLOutputFactory; +import javax.xml.transform.TransformerFactory; +import javax.xml.transform.sax.SAXTransformerFactory; + +/** + * Default implementation of PGXmlFactoryFactory that configures each factory per OWASP recommendations. + * + * @see https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html + */ +public class DefaultPGXmlFactoryFactory implements PGXmlFactoryFactory { + public static final DefaultPGXmlFactoryFactory INSTANCE = new DefaultPGXmlFactoryFactory(); + + private DefaultPGXmlFactoryFactory() { + } + + private DocumentBuilderFactory getDocumentBuilderFactory() { + DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); + setFactoryProperties(factory); + factory.setXIncludeAware(false); + factory.setExpandEntityReferences(false); + return factory; + } + + @Override + public DocumentBuilder newDocumentBuilder() throws ParserConfigurationException { + DocumentBuilder builder = getDocumentBuilderFactory().newDocumentBuilder(); + builder.setEntityResolver(EmptyStringEntityResolver.INSTANCE); + builder.setErrorHandler(NullErrorHandler.INSTANCE); + return builder; + } + + @Override + public TransformerFactory newTransformerFactory() { + TransformerFactory factory = TransformerFactory.newInstance(); + setFactoryProperties(factory); + return factory; + } + + @Override + public SAXTransformerFactory newSAXTransformerFactory() { + SAXTransformerFactory factory = (SAXTransformerFactory) SAXTransformerFactory.newInstance(); + setFactoryProperties(factory); + return factory; + } + + @Override + public XMLInputFactory newXMLInputFactory() { + XMLInputFactory factory = XMLInputFactory.newInstance(); + setPropertyQuietly(factory, XMLInputFactory.SUPPORT_DTD, false); + setPropertyQuietly(factory, XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false); + return factory; + } + + @Override + public XMLOutputFactory newXMLOutputFactory() { + XMLOutputFactory factory = XMLOutputFactory.newInstance(); + return factory; + } + + @Override + public XMLReader createXMLReader() throws SAXException { + XMLReader factory = XMLReaderFactory.createXMLReader(); + setFeatureQuietly(factory, "http://apache.org/xml/features/disallow-doctype-decl", true); + setFeatureQuietly(factory, "http://apache.org/xml/features/nonvalidating/load-external-dtd", false); + setFeatureQuietly(factory, "http://xml.org/sax/features/external-general-entities", false); + setFeatureQuietly(factory, "http://xml.org/sax/features/external-parameter-entities", false); + factory.setErrorHandler(NullErrorHandler.INSTANCE); + return factory; + } + + private static void setFeatureQuietly(Object factory, String name, boolean value) { + try { + if (factory instanceof DocumentBuilderFactory) { + ((DocumentBuilderFactory) factory).setFeature(name, value); + } else if (factory instanceof TransformerFactory) { + ((TransformerFactory) factory).setFeature(name, value); + } else if (factory instanceof XMLReader) { + ((XMLReader) factory).setFeature(name, value); + } else { + throw new Error("Invalid factory class: " + factory.getClass()); + } + return; + } catch (Exception ignore) { + } + } + + private static void setAttributeQuietly(Object factory, String name, Object value) { + try { + if (factory instanceof DocumentBuilderFactory) { + ((DocumentBuilderFactory) factory).setAttribute(name, value); + } else if (factory instanceof TransformerFactory) { + ((TransformerFactory) factory).setAttribute(name, value); + } else { + throw new Error("Invalid factory class: " + factory.getClass()); + } + } catch (Exception ignore) { + } + } + + private static void setFactoryProperties(Object factory) { + setFeatureQuietly(factory, XMLConstants.FEATURE_SECURE_PROCESSING, true); + setFeatureQuietly(factory, "http://apache.org/xml/features/disallow-doctype-decl", true); + setFeatureQuietly(factory, "http://apache.org/xml/features/nonvalidating/load-external-dtd", false); + setFeatureQuietly(factory, "http://xml.org/sax/features/external-general-entities", false); + setFeatureQuietly(factory, "http://xml.org/sax/features/external-parameter-entities", false); + // Values from XMLConstants inlined for JDK 1.6 compatibility + setAttributeQuietly(factory, "http://javax.xml.XMLConstants/property/accessExternalDTD", ""); + setAttributeQuietly(factory, "http://javax.xml.XMLConstants/property/accessExternalSchema", ""); + setAttributeQuietly(factory, "http://javax.xml.XMLConstants/property/accessExternalStylesheet", ""); + } + + private static void setPropertyQuietly(Object factory, String name, Object value) { + try { + if (factory instanceof XMLReader) { + ((XMLReader) factory).setProperty(name, value); + } else if (factory instanceof XMLInputFactory) { + ((XMLInputFactory) factory).setProperty(name, value); + } else { + throw new Error("Invalid factory class: " + factory.getClass()); + } + } catch (Exception ignore) { + } + } +} \ No newline at end of file diff --git a/org/postgresql/xml/EmptyStringEntityResolver.java b/org/postgresql/xml/EmptyStringEntityResolver.java new file mode 100644 index 0000000..e96a39b --- /dev/null +++ b/org/postgresql/xml/EmptyStringEntityResolver.java @@ -0,0 +1,23 @@ +/* + * Copyright (c) 2020, PostgreSQL Global Development Group + * See the LICENSE file in the project root for more information. + */ + +package org.postgresql.xml; + +import org.xml.sax.EntityResolver; +import org.xml.sax.InputSource; +import org.xml.sax.SAXException; + +import java.io.IOException; +import java.io.StringReader; + +public class EmptyStringEntityResolver implements EntityResolver { + public static final EmptyStringEntityResolver INSTANCE = new EmptyStringEntityResolver(); + + @Override + public InputSource resolveEntity(String publicId, String systemId) + throws SAXException, IOException { + return new InputSource(new StringReader("")); + } +} diff --git a/org/postgresql/xml/LegacyInsecurePGXmlFactoryFactory.java b/org/postgresql/xml/LegacyInsecurePGXmlFactoryFactory.java new file mode 100644 index 0000000..ed7a66b --- /dev/null +++ b/org/postgresql/xml/LegacyInsecurePGXmlFactoryFactory.java @@ -0,0 +1,57 @@ +/* + * Copyright (c) 2020, PostgreSQL Global Development Group + * See the LICENSE file in the project root for more information. + */ + +package org.postgresql.xml; + +import org.xml.sax.SAXException; +import org.xml.sax.XMLReader; +import org.xml.sax.helpers.XMLReaderFactory; + +import javax.xml.parsers.DocumentBuilder; +import javax.xml.parsers.DocumentBuilderFactory; +import javax.xml.parsers.ParserConfigurationException; +import javax.xml.stream.XMLInputFactory; +import javax.xml.stream.XMLOutputFactory; +import javax.xml.transform.TransformerFactory; +import javax.xml.transform.sax.SAXTransformerFactory; + +public class LegacyInsecurePGXmlFactoryFactory implements PGXmlFactoryFactory { + public static final LegacyInsecurePGXmlFactoryFactory INSTANCE = new LegacyInsecurePGXmlFactoryFactory(); + + private LegacyInsecurePGXmlFactoryFactory() { + } + + @Override + public DocumentBuilder newDocumentBuilder() throws ParserConfigurationException { + DocumentBuilder builder = DocumentBuilderFactory.newInstance().newDocumentBuilder(); + builder.setErrorHandler(NullErrorHandler.INSTANCE); + return builder; + } + + @Override + public TransformerFactory newTransformerFactory() { + return TransformerFactory.newInstance(); + } + + @Override + public SAXTransformerFactory newSAXTransformerFactory() { + return (SAXTransformerFactory) SAXTransformerFactory.newInstance(); + } + + @Override + public XMLInputFactory newXMLInputFactory() { + return XMLInputFactory.newInstance(); + } + + @Override + public XMLOutputFactory newXMLOutputFactory() { + return XMLOutputFactory.newInstance(); + } + + @Override + public XMLReader createXMLReader() throws SAXException { + return XMLReaderFactory.createXMLReader(); + } +} \ No newline at end of file diff --git a/org/postgresql/xml/NullErrorHandler.java b/org/postgresql/xml/NullErrorHandler.java new file mode 100644 index 0000000..d12a4fa --- /dev/null +++ b/org/postgresql/xml/NullErrorHandler.java @@ -0,0 +1,25 @@ +/* + * Copyright (c) 2020, PostgreSQL Global Development Group + * See the LICENSE file in the project root for more information. + */ + +package org.postgresql.xml; + +import org.xml.sax.ErrorHandler; +import org.xml.sax.SAXParseException; + +/** + * Error handler that silently suppresses all errors. + */ +public class NullErrorHandler implements ErrorHandler { + public static final NullErrorHandler INSTANCE = new NullErrorHandler(); + + public void error(SAXParseException e) { + } + + public void fatalError(SAXParseException e) { + } + + public void warning(SAXParseException e) { + } +} diff --git a/org/postgresql/xml/PGXmlFactoryFactory.java b/org/postgresql/xml/PGXmlFactoryFactory.java new file mode 100644 index 0000000..4bb98e4 --- /dev/null +++ b/org/postgresql/xml/PGXmlFactoryFactory.java @@ -0,0 +1,30 @@ +/* + * Copyright (c) 2020, PostgreSQL Global Development Group + * See the LICENSE file in the project root for more information. + */ + +package org.postgresql.xml; + +import org.xml.sax.SAXException; +import org.xml.sax.XMLReader; + +import javax.xml.parsers.DocumentBuilder; +import javax.xml.parsers.ParserConfigurationException; +import javax.xml.stream.XMLInputFactory; +import javax.xml.stream.XMLOutputFactory; +import javax.xml.transform.TransformerFactory; +import javax.xml.transform.sax.SAXTransformerFactory; + +public interface PGXmlFactoryFactory { + DocumentBuilder newDocumentBuilder() throws ParserConfigurationException; + + TransformerFactory newTransformerFactory(); + + SAXTransformerFactory newSAXTransformerFactory(); + + XMLInputFactory newXMLInputFactory(); + + XMLOutputFactory newXMLOutputFactory(); + + XMLReader createXMLReader() throws SAXException; +} \ No newline at end of file -- 2.24.1