Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable FEATURE_SECURE_PROCESSING XML setting by default #2384

Merged
merged 6 commits into from Feb 8, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -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");
Expand Down Expand Up @@ -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();
}
}
@@ -1,5 +1,6 @@
package liquibase.parser.core.xml;

import liquibase.GlobalConfiguration;
import liquibase.Scope;
import liquibase.changelog.ChangeLogParameters;
import liquibase.exception.ChangeLogParseException;
Expand All @@ -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;
Expand All @@ -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
Expand All @@ -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();
Expand Down
@@ -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")
}

}

This file was deleted.