Skip to content

Commit

Permalink
Enable xml secure processing
Browse files Browse the repository at this point in the history
  • Loading branch information
trentdm committed Aug 25, 2022
2 parents 46fc9ce + 8e3d236 commit 8b27405
Show file tree
Hide file tree
Showing 5 changed files with 137 additions and 76 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ public class GlobalConfiguration extends AbstractConfigurationContainer {
public static final String GENERATED_CHANGESET_IDS_INCLUDE_DESCRIPTION = "generatedChangeSetIdsContainsDescription";
public static final String INCLUDE_CATALOG_IN_SPECIFICATION = "includeCatalogInSpecification";
public static final String SHOULD_SNAPSHOT_DATA = "shouldSnapshotData";
public static final String SECURE_PARSING = "secureParsing";

public GlobalConfiguration() {
super("liquibase");
Expand Down Expand Up @@ -100,6 +101,10 @@ public GlobalConfiguration() {
getContainer().addProperty(SHOULD_SNAPSHOT_DATA, Boolean.class)
.setDescription("Should Liquibase snapshot data by default?")
.setDefaultValue(false);

getContainer().addProperty(SECURE_PARSING, 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);
}

/**
Expand Down Expand Up @@ -264,4 +269,13 @@ public GlobalConfiguration setGeneratedChangeSetIdsContainDescription(boolean co
getContainer().setValue(GENERATED_CHANGESET_IDS_INCLUDE_DESCRIPTION, containDescription);
return this;
}

public boolean getSecureParsing() {
return getContainer().getValue(SECURE_PARSING, Boolean.class);
}

public GlobalConfiguration setSecureParsing(boolean secureParsing) {
getContainer().setValue(SECURE_PARSING, secureParsing);
return this;
}
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package liquibase.parser.core.xml;

import liquibase.changelog.ChangeLogParameters;
import liquibase.configuration.GlobalConfiguration;
import liquibase.configuration.LiquibaseConfiguration;
import liquibase.exception.ChangeLogParseException;
import liquibase.logging.LogService;
import liquibase.logging.LogType;
Expand Down Expand Up @@ -31,6 +33,13 @@ public XMLChangeLogSAXParser() {
saxParserFactory = SAXParserFactory.newInstance();
saxParserFactory.setValidating(true);
saxParserFactory.setNamespaceAware(true);
if (LiquibaseConfiguration.getInstance().getConfiguration(GlobalConfiguration.class).getSecureParsing()) {
try {
saxParserFactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
} catch (Throwable e) {
LogService.getLog(XMLChangeLogSAXParser.class).warning("Cannot enable FEATURE_SECURE_PROCESSING: " + e.getMessage(), e);
}
}

if (PREFER_INTERNAL_XSD) {
InputStream xsdInputStream = XMLChangeLogSAXParser.class.getResourceAsStream(XSD_FILE);
Expand Down Expand Up @@ -70,6 +79,13 @@ protected ParsedNode parseToNode(String physicalChangeLogLocation, ChangeLogPara
try (
InputStream inputStream = StreamUtil.singleInputStream(physicalChangeLogLocation, resourceAccessor)) {
SAXParser parser = saxParserFactory.newSAXParser();
if (LiquibaseConfiguration.getInstance().getConfiguration(GlobalConfiguration.class).getSecureParsing()) {
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) {
LogService.getLog(XMLChangeLogSAXParser.class).warning("Cannot enable ACCESS_EXTERNAL_SCHEMA: " + e.getMessage(), e);
}
}
trySetSchemaLanguageProperty(parser);

XMLReader xmlReader = parser.getXMLReader();
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
package liquibase.parser.core.xml

import liquibase.Contexts
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.configuration.GlobalConfiguration
import liquibase.configuration.LiquibaseConfiguration
import liquibase.database.Database
import liquibase.exception.ChangeLogParseException
import liquibase.exception.LiquibaseException
import liquibase.sdk.database.MockDatabase
import liquibase.sdk.resource.MockResourceAccessor
import liquibase.test.JUnitResourceAccessor
import spock.lang.Specification

import java.lang.reflect.UndeclaredThrowableException

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(null, null, {
LiquibaseConfiguration.getInstance().getConfiguration(GlobalConfiguration.class).setSecureParsing(false)
new XMLChangeLogSAXParser().parse("com/example/insecure.xml", new ChangeLogParameters(), resourceAccessor)
LiquibaseConfiguration.getInstance().reset()
} as Scope.ScopedRunner)


then:
def e = thrown(UndeclaredThrowableException)
e.cause.message.contains("Error Reading Migration File: invalid.txt")
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import liquibase.sdk.supplier.resource.ResourceSupplier
import liquibase.sql.visitor.AppendSqlVisitor
import liquibase.sql.visitor.ReplaceSqlVisitor
import liquibase.test.JUnitResourceAccessor
import org.junit.BeforeClass
import spock.lang.FailsWith
import spock.lang.Shared
import spock.lang.Specification
Expand All @@ -42,6 +43,7 @@ public class XMLChangeLogSAXParser_RealFile_Test extends Specification {

@Shared resourceSupplier = new ResourceSupplier()

@BeforeClass
def before() {
LiquibaseConfiguration.getInstance().reset();
}
Expand Down

This file was deleted.

0 comments on commit 8b27405

Please sign in to comment.