Skip to content
Permalink
Browse files
Merge pull request from GHSA-37xm-4h3m-5w3v
* refactor: Clean up whitespace in existing PgSQLXMLTest

* fix: Fix XXE vulnerability in PgSQLXML by disabling external access and doctypes

Fixes 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.

* fix: Add missing getter and setter for XML_FACTORY_FACTORY to BasicDataSource
  • Loading branch information
sehrope committed Jun 1, 2020
1 parent 97e2e8f commit 14b62aca4764d496813f55a43d050b017e01eb65
Showing 11 changed files with 453 additions and 29 deletions.
@@ -661,6 +661,17 @@ public enum PGProperty {
"false",
"Use SPNEGO in SSPI authentication requests"),

/**
* Factory class to instantiate factories for XML processing.
* The default factory disables external entity processing.
* Legacy behavior with external entity processing can be enabled by specifying a value of LEGACY_INSECURE.
* Or specify a custom class that implements {@code org.postgresql.xml.PGXmlFactoryFactory}.
*/
XML_FACTORY_FACTORY(
"xmlFactoryFactory",
"",
"Factory class to instantiate factories for XML processing"),

;

private final String name;
@@ -10,6 +10,7 @@
import org.postgresql.jdbc.FieldMetadata;
import org.postgresql.jdbc.TimestampUtils;
import org.postgresql.util.LruCache;
import org.postgresql.xml.PGXmlFactoryFactory;

import java.sql.Connection;
import java.sql.ResultSet;
@@ -212,4 +213,12 @@ CachedQuery createQuery(String sql, boolean escapeProcessing, boolean isParamete
* @see PGProperty#READ_ONLY_MODE
*/
boolean hintReadOnly();

/**
* 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;
}
@@ -1526,6 +1526,14 @@ public java.util.logging.Logger getParentLogger() {
return Logger.getLogger("org.postgresql");
}

public String getXmlFactoryFactory() {
return PGProperty.XML_FACTORY_FACTORY.get(properties);
}

public void setXmlFactoryFactory(String xmlFactoryFactory) {
PGProperty.XML_FACTORY_FACTORY.set(properties, xmlFactoryFactory);
}

/*
* Alias methods below, these are to help with ease-of-use with other database tools / frameworks
* which expect normal java bean getters / setters to exist for the property names.
@@ -37,6 +37,9 @@
import org.postgresql.util.PGobject;
import org.postgresql.util.PSQLException;
import org.postgresql.util.PSQLState;
import org.postgresql.xml.DefaultPGXmlFactoryFactory;
import org.postgresql.xml.LegacyInsecurePGXmlFactoryFactory;
import org.postgresql.xml.PGXmlFactoryFactory;

import java.io.IOException;
import java.sql.Array;
@@ -156,6 +159,9 @@ private enum ReadOnlyBehavior {

private final LruCache<FieldMetadata.Key, FieldMetadata> fieldMetadataCache;

private final String xmlFactoryFactoryClass;
private PGXmlFactoryFactory xmlFactoryFactory;

final CachedQuery borrowQuery(String sql) throws SQLException {
return queryExecutor.borrowQuery(sql);
}
@@ -311,6 +317,8 @@ public TimeZone get() {
false);

replicationConnection = PGProperty.REPLICATION.get(info) != null;

xmlFactoryFactoryClass = PGProperty.XML_FACTORY_FACTORY.get(info);
}

private static ReadOnlyBehavior getReadOnlyBehavior(String property) {
@@ -1823,4 +1831,36 @@ public final String getParameterStatus(String parameterName) {
return queryExecutor.getParameterStatus(parameterName);
}

@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;
}
}
@@ -9,10 +9,11 @@
import org.postgresql.util.GT;
import org.postgresql.util.PSQLException;
import org.postgresql.util.PSQLState;
import org.postgresql.xml.DefaultPGXmlFactoryFactory;
import org.postgresql.xml.PGXmlFactoryFactory;

import org.xml.sax.ErrorHandler;
import org.xml.sax.InputSource;
import org.xml.sax.SAXParseException;
import org.xml.sax.XMLReader;

import java.io.ByteArrayInputStream;
import java.io.ByteArrayOutputStream;
@@ -27,7 +28,6 @@
import java.sql.SQLXML;

import javax.xml.parsers.DocumentBuilder;
import javax.xml.parsers.DocumentBuilderFactory;
import javax.xml.stream.XMLInputFactory;
import javax.xml.stream.XMLOutputFactory;
import javax.xml.stream.XMLStreamException;
@@ -77,6 +77,13 @@ private PgSQLXML(BaseConnection conn, String data, boolean initialized) {
this.freed = false;
}

private PGXmlFactoryFactory getXmlFactoryFactory() throws SQLException {
if (conn != null) {
return conn.getXmlFactoryFactory();
}
return DefaultPGXmlFactoryFactory.INSTANCE;
}

@Override
public synchronized void free() {
freed = true;
@@ -132,18 +139,17 @@ public synchronized <T extends Source> T getSource(Class<T> sourceClass) throws

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 (T) new DOMSource(builder.parse(input));
} else if (SAXSource.class.equals(sourceClass)) {
XMLReader reader = getXmlFactoryFactory().createXMLReader();
InputSource is = new InputSource(new StringReader(data));
return (T) new SAXSource(is);
return (T) new SAXSource(reader, is);
} else if (StreamSource.class.equals(sourceClass)) {
return (T) new StreamSource(new StringReader(data));
} else if (StAXSource.class.equals(sourceClass)) {
XMLInputFactory xif = XMLInputFactory.newInstance();
XMLInputFactory xif = getXmlFactoryFactory().newXMLInputFactory();
XMLStreamReader xsr = xif.createXMLStreamReader(new StringReader(data));
return (T) new StAXSource(xsr);
}
@@ -191,8 +197,7 @@ public synchronized <T extends Result> T setResult(Class<T> resultClass) throws
return (T) 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));
@@ -209,7 +214,7 @@ public synchronized <T extends Result> T setResult(Class<T> resultClass) throws
} 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 (T) new StAXResult(xsw);
@@ -272,7 +277,7 @@ private void ensureInitialized() throws SQLException {
// 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();
@@ -298,18 +303,4 @@ private void initialize() throws SQLException {
}
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) {
}
}

}
@@ -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 <a href="https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html">https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html</a>
*/
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) {
}
}
}
@@ -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(""));
}
}

10 comments on commit 14b62ac

@odubaj
Copy link
Contributor

@odubaj odubaj commented on 14b62ac Jul 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, I have a question about the vulnerability, which is fixed via this commit. Is there a possibility, that also version 9.2.1002 (currently in rhel-7) is affected by this problem ? If it is so, is it possible to reproduce this problem in any way ? It may be problematic, as the code structure is quite different and many classes and properties are missing. Thanks for your advice.

@sehrope
Copy link
Member Author

@sehrope sehrope commented on 14b62ac Jul 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that version has the same issue. The package structure is different but the XML handling code is similar. Compare these two methods in the 9.2.1002 class JdbcSQLXML with this PR:

getSource(...) method:

https://github.com/pgjdbc/pgjdbc/blob/REL9_2_1002/org/postgresql/jdbc4/Jdbc4SQLXML.java#L114-L151

getResult(...) method:

https://github.com/pgjdbc/pgjdbc/blob/REL9_2_1002/org/postgresql/jdbc4/Jdbc4SQLXML.java#L177-L214

Neither method sets any of the required security properties on the XML factories so they would be vulnerable to the same issue.

@vlsi
Copy link
Member

@vlsi vlsi commented on 14b62ac Jul 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@odubaj
Copy link
Contributor

@odubaj odubaj commented on 14b62ac Jul 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick replies! Currently we are working on a reproducer for this version. Hopefully, we will not need to backport tons of code, as it can cost us other security issues.

@davecramer
Copy link
Member

@davecramer davecramer commented on 14b62ac Jul 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@odubaj
Copy link
Contributor

@odubaj odubaj commented on 14b62ac Jul 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not make upgrades due to stability reasons, as rhel-7 is considered as a stable system, where we fix only high priority security issues. If we make an upgrade from version 9.* to 42.*, it can damage the functionality of other components.

@davecramer
Copy link
Member

@davecramer davecramer commented on 14b62ac Jul 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@odubaj
Copy link
Contributor

@odubaj odubaj commented on 14b62ac Jul 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with that, but for now, we see the problem is resolvable, so I consider to somehow backport the fix also to this version. We would highly appreciate your willingness to help us.

@odubaj
Copy link
Contributor

@odubaj odubaj commented on 14b62ac Jul 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please advice us, how it is possible to run the internal testsuite? We are aiming to reproduce the problem as a part of internal testsuite. Thanks

@davecramer
Copy link
Member

@davecramer davecramer commented on 14b62ac Jul 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the version you have it would be

ant test

Please sign in to comment.