Merge pull request #2384 from liquibase/enable_xml_secure_processing

Enable FEATURE_SECURE_PROCESSING XML setting by default

liquibase-core/src/main/java/liquibase/GlobalConfiguration.java

@@ -33,6 +33,7 @@ public class GlobalConfiguration implements AutoloadedConfigurations {
     public static final ConfigurationDefinition<Boolean> HEADLESS;
     public static final ConfigurationDefinition<Boolean> STRICT;
     public static final ConfigurationDefinition<Integer> DDL_LOCK_TIMEOUT;
+    public static final ConfigurationDefinition<Boolean> SECURE_PARSING;
 
     static {
         ConfigurationDefinition.Builder builder = new ConfigurationDefinition.Builder("liquibase");
@@ -175,5 +176,10 @@ public class GlobalConfiguration implements AutoloadedConfigurations {
                 .addAliasKey("liquibase.ddl_lock_timeout")
                 .setDescription("The DDL_LOCK_TIMEOUT parameter indicates the number of seconds a DDL command should wait for the locks to become available before throwing the resource busy error message. This applies only to Oracle databases.")
                 .build();
+
+        SECURE_PARSING = builder.define("secureParsing", Boolean.class)
+                .setDescription("If true, remove functionality from file parsers which could be used insecurely. Examples include (but not limited to) disabling remote XML entity support.")
+                .setDefaultValue(true)
+                .build();
     }
 }

liquibase-core/src/main/java/liquibase/parser/core/xml/XMLChangeLogSAXParser.java

@@ -1,5 +1,6 @@
 package liquibase.parser.core.xml;
 
+import liquibase.GlobalConfiguration;
 import liquibase.Scope;
 import liquibase.changelog.ChangeLogParameters;
 import liquibase.exception.ChangeLogParseException;
@@ -9,6 +10,7 @@
 import liquibase.util.FileUtil;
 import org.xml.sax.*;
 
+import javax.xml.XMLConstants;
 import javax.xml.parsers.SAXParser;
 import javax.xml.parsers.SAXParserFactory;
 import java.io.IOException;
@@ -25,6 +27,13 @@ public XMLChangeLogSAXParser() {
         saxParserFactory = SAXParserFactory.newInstance();
         saxParserFactory.setValidating(true);
         saxParserFactory.setNamespaceAware(true);
+        if (GlobalConfiguration.SECURE_PARSING.getCurrentValue()) {
+            try {
+                saxParserFactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
+            } catch (Throwable e) {
+                Scope.getCurrentScope().getLog(getClass()).fine("Cannot enable FEATURE_SECURE_PROCESSING: " + e.getMessage(), e);
+            }
+        }
     }
 
     @Override
@@ -49,6 +58,13 @@ protected SAXParserFactory getSaxParserFactory() {
     protected ParsedNode parseToNode(String physicalChangeLogLocation, ChangeLogParameters changeLogParameters, ResourceAccessor resourceAccessor) throws ChangeLogParseException {
         try (InputStream inputStream = resourceAccessor.openStream(null, physicalChangeLogLocation)) {
             SAXParser parser = saxParserFactory.newSAXParser();
+            if (GlobalConfiguration.SECURE_PARSING.getCurrentValue()) {
+                try {
+                    parser.setProperty(XMLConstants.ACCESS_EXTERNAL_SCHEMA, "http,https"); //need to allow external schemas on http/https to support the liquibase.org xsd files
+                } catch (SAXException e) {
+                    Scope.getCurrentScope().getLog(getClass()).fine("Cannot enable ACCESS_EXTERNAL_SCHEMA: " + e.getMessage(), e);
+                }
+            }
             trySetSchemaLanguageProperty(parser);
 
             XMLReader xmlReader = parser.getXMLReader();

liquibase-core/src/test/groovy/liquibase/parser/core/xml/XMLChangeLogSAXParserTest.groovy

@@ -0,0 +1,103 @@
+package liquibase.parser.core.xml
+
+import liquibase.Contexts
+import liquibase.GlobalConfiguration
+import liquibase.LabelExpression
+import liquibase.RuntimeEnvironment
+import liquibase.Scope
+import liquibase.changelog.ChangeLogIterator
+import liquibase.changelog.ChangeLogParameters
+import liquibase.changelog.ChangeSet
+import liquibase.changelog.DatabaseChangeLog
+import liquibase.changelog.filter.ChangeSetFilterResult
+import liquibase.changelog.visitor.ChangeSetVisitor
+import liquibase.database.Database
+import liquibase.database.core.MockDatabase
+import liquibase.exception.ChangeLogParseException
+import liquibase.exception.LiquibaseException
+import liquibase.sdk.resource.MockResourceAccessor
+import liquibase.test.JUnitResourceAccessor
+import spock.lang.Specification
+
+class XMLChangeLogSAXParserTest extends Specification {
+
+    def INSECURE_XML = """
+<!DOCTYPE databaseChangeLog [
+        <!ENTITY insecure SYSTEM "file://invalid.txt">
+        ]>
+
+<databaseChangeLog xmlns="http://www.liquibase.org/xml/ns/dbchangelog"
+                   xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+                   xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog
+                      http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-4.6.xsd">
+
+    <changeSet id="1" author="example">
+        <output>&insecure;</output>
+    </changeSet>
+
+</databaseChangeLog>
+"""
+
+    def testIgnoreDuplicateChangeSets() throws ChangeLogParseException, Exception {
+        when:
+        def xmlParser = new XMLChangeLogSAXParser()
+        def changeLog = xmlParser.parse("liquibase/parser/core/xml/ignoreDuplicatedChangeLogs/master.changelog.xml",
+                new ChangeLogParameters(), new JUnitResourceAccessor())
+
+        final List<ChangeSet> changeSets = new ArrayList<ChangeSet>()
+
+        new ChangeLogIterator(changeLog).run(new ChangeSetVisitor() {
+            @Override
+            ChangeSetVisitor.Direction getDirection() {
+                return ChangeSetVisitor.Direction.FORWARD
+            }
+
+            @Override
+            void visit(ChangeSet changeSet, DatabaseChangeLog databaseChangeLog, Database database, Set<ChangeSetFilterResult> filterResults) throws LiquibaseException {
+                changeSets.add(changeSet)
+            }
+        }, new RuntimeEnvironment(new MockDatabase(), new Contexts(), new LabelExpression()))
+
+
+        then:
+        changeSets.size() == 8
+        changeSets.get(0).toString() == "liquibase/parser/core/xml/ignoreDuplicatedChangeLogs/included.changelog4.xml::1::testuser"
+        changeSets.get(1).toString() == "liquibase/parser/core/xml/ignoreDuplicatedChangeLogs/included.changelog4.xml::1::testuser"
+        changeSets.get(1).getContexts().getContexts().size() == 1
+        changeSets.get(2).toString() == "liquibase/parser/core/xml/ignoreDuplicatedChangeLogs/included.changelog4.xml::1::testuser"
+        changeSets.get(2).getLabels().getLabels().size() == 1
+        changeSets.get(3).toString() == "liquibase/parser/core/xml/ignoreDuplicatedChangeLogs/included.changelog4.xml::1::testuser"
+        changeSets.get(3).getLabels().getLabels().size() == 2
+        changeSets.get(4).toString() == "liquibase/parser/core/xml/ignoreDuplicatedChangeLogs/included.changelog4.xml::1::testuser"
+        changeSets.get(4).getDbmsSet().size() == 1
+        changeSets.get(5).toString() == "liquibase/parser/core/xml/ignoreDuplicatedChangeLogs/included.changelog1.xml::1::testuser"
+        changeSets.get(6).toString() == "liquibase/parser/core/xml/ignoreDuplicatedChangeLogs/included.changelog3.xml::1::testuser"
+        changeSets.get(7).toString() == "liquibase/parser/core/xml/ignoreDuplicatedChangeLogs/included.changelog2.xml::1::testuser"
+    }
+
+    def "uses liquibase.secureParsing by default"() {
+        when:
+        def resourceAccessor = new MockResourceAccessor(["com/example/insecure.xml": INSECURE_XML])
+
+        new XMLChangeLogSAXParser().parse("com/example/insecure.xml", new ChangeLogParameters(), resourceAccessor)
+
+        then:
+        def e = thrown(ChangeLogParseException)
+        e.message.contains("access is not allowed due to restriction set by the accessExternalDTD property")
+    }
+
+    def "allows liquibase.secureParsing=false to disable secure parsing"() {
+        when:
+        def resourceAccessor = new MockResourceAccessor(["com/example/insecure.xml": INSECURE_XML])
+
+        Scope.child(GlobalConfiguration.SECURE_PARSING.key, "false", { ->
+            new XMLChangeLogSAXParser().parse("com/example/insecure.xml", new ChangeLogParameters(), resourceAccessor)
+        })
+
+
+        then:
+        def e = thrown(ChangeLogParseException)
+        e.message.contains("Error Reading Changelog File: invalid.txt")
+    }
+
+}