Enable FEATURE_SECURE_PROCESSING XML setting by default

[nvoxland]

Description

Sets FEATURE_SECURE_PROCESSING=true in the XML parser. Because XSD files are referenced like http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-4.6.xsd we do need to enable http/https external XSD references still.

Because this feature blocks/changes parser functionality, it will be a breaking change for users expecting the old behavior. Like if the user had a custom XSD with a file: prefix they could no longer use liquibase. Or if they referenced an external DTD entity.

Discussion

Historically I've not tried to set any limitations on what the XML parser can do under the theory that for liquibase usage, the changelog XML file is "trusted data". It's created and managed by internal users, and if they want to do something malicious there are far easier attacks when you have write access to the changelog file than XXE or whatever attacks. Therefore, I left the ability to pull external entities etc. in there because that can be a great way to write reusable XML code. And people who really do want it locked down more can set system properties like javax.xml.accessExternalSchema

That being said, security is important. And supporting all XML functionality does open some additional attack vectors for people who DON'T have changelog access. For example, if the changelog developers had used an external entity for a shared macro and the attacker is able to replace that remote resource they would impact the changelog execution.

So far this PR has no configuration settings exposed to control it. It simply enables security and anyone wanting to modify it would need to subclass and extend XMLChangelogSAXParser to modify the settings they need. We could certainly add settings, but what would they be? Just enabling/disabling FEATURE_SECURE_PROCESSING? Or more granular control over flags (which can vary based on xml vendor and version)

Questions:

1. Should liquibase start dealing with XML feature flags like FEATURE_SECURE_PROCESSING?
2. If so, which should we set by default? Which should we support configuration flags for, and which should we not?

---

Dev Handoff Notes (Internal Use)

Links

• Fixed Issue: Stand alone PR
• Test Results: https://github.com/liquibase/liquibase/pull/2384/checks

Testing

• Steps to Reproduce: See below
• Guidance:
  • Impact: Impacts XML parsing only

Dev Verification

A changelog like this:

<?xml version="1.0" encoding="utf-8"?>

<!DOCTYPE person [
        <!ENTITY ssrf SYSTEM "https://localhost">
        ]>
<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">
        <sql>&ssrf;</sql>
    </changeSet>
</databaseChangeLog>

would previously fail with a "Connection refused" error as it tried to pull the ssrf entity from localhost but you have no server running.

With the fix, you get an Failed to read external document 'localhost', because 'https' access is not allowed due to restriction set by the accessExternalDTD property error unless you have --secure-parsing=false or set liquibase.secureParsing=false

┆Issue is synchronized with this Jira Bug by Unito

[nvoxland]

I don't have a strong feeling on the questions in the discussion section yet.

What feedback does others have?

[suryaaki2]

I don't have a strong feeling on the questions in the discussion section yet.

What feedback does others have?

I would suggest going ahead with this PR and document on reverting to old behavior

[kataggart]

I would like to default to the more secure/restricted approach, with a config where user can opt out of it.

[nvoxland]

The decision was to add a new liquibase.secureParsing configuration which defaults to "true" but can be overridden to "false".

The flag can be used for all file parsing which can limit functionality in order to improve security.

[XDelphiGrl]

The change to enable FEATURE_SECURE_PROCESSING by default during XML parsing is to address a security vulnerability. In addition to translating an existing XML parsing test from JUnit to Spock, @nvoxland added two new integration tests for this feature. One test validates the default works as expected and another to test that the override works as expected.

Breaking Change Alert
This feature blocks/changes parser functionality. It is a breaking change for users with changelogs referencing an external DTD entity or file. The FEATURE_SECURE_PROCESSING can be disabled (at own risk) by using the new global parameter for

New Global Parameter : --secure-parsing=PARAM
See liquibase --help for details on how use JAVA_OPTS, liquibase.properties file or environment variables to change the setting of liquibase.secure-parsing.

---

• Verify update fails for XML changelogs with external DTD references. PASS

<!DOCTYPE person [<!ENTITY ssrf SYSTEM "https://localhost"> ]>

//ERROR MESSAGE
Unexpected error running Liquibase: External Entity: Failed to read external document 'localhost', because 'https' access is not allowed due to restriction set by the accessExternalDTD property.

• Verify update fails for XML changelogs with a DTD reference to a file. PASS

<!DOCTYPE a_file[ <!ENTITY ref SYSTEM "file:///C:/Users/erz/dtd-test.txt" > ]>

// ERROR MESSAGE
External Entity: Failed to read external document 'dtd-test.txt', because 'file' access is not allowed due to restriction set by the accessExternalDTD property

• Verify update with --secure-parsing false no longer blocks deploying changelogs with DTD references. PASS

  • External URL PASS
  • File reference PASS

• Verify update with liquibase.secureParsing:false in the properties file no longer blocks deploying changelogs with DTD references. PASS

  • External URL PASS
  • File reference PASS

• PRO : Verify update with LIQBUIBASE_SECURE_PARSING=false in an environment variable no longer blocks deploying changelogs with DTD references. PASS

  • External URL PASS
  • File reference PASS

---

Test Environment
MariaDB 10.3 (database-agnostic fix, just like to be thorough!)
Liquibase Core: enable_xml_secure_processing/1385/4de44c, Pro: master/546/12ffc3
Passing Functional Tests