From 08cc6e2dd881548f9729fb2b8db28b9788ddc8d3 Mon Sep 17 00:00:00 2001 From: mherman22 Date: Fri, 29 Mar 2024 20:03:58 +0300 Subject: [PATCH] TRUNK-6041: Proper Restriction of XML External Entity Reference to prevent XXE attacks --- .../openmrs/liquibase/AbstractSnapshotTuner.java | 15 +++++++++++---- .../src/main/java/org/openmrs/liquibase/Main.java | 3 ++- .../liquibase/AbstractSnapshotTunerTest.java | 15 +++++++++++++++ .../org/openmrs/liquibase/CoreDataTunerTest.java | 3 ++- .../test/java/org/openmrs/liquibase/MainTest.java | 3 ++- .../openmrs/liquibase/SchemaOnlyTunerTest.java | 3 ++- 6 files changed, 34 insertions(+), 8 deletions(-) diff --git a/liquibase/src/main/java/org/openmrs/liquibase/AbstractSnapshotTuner.java b/liquibase/src/main/java/org/openmrs/liquibase/AbstractSnapshotTuner.java index 2df44a565a3..775fd4bc503 100644 --- a/liquibase/src/main/java/org/openmrs/liquibase/AbstractSnapshotTuner.java +++ b/liquibase/src/main/java/org/openmrs/liquibase/AbstractSnapshotTuner.java @@ -31,6 +31,9 @@ import org.dom4j.io.XMLWriter; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import org.xml.sax.EntityResolver; +import org.xml.sax.InputSource; +import org.xml.sax.SAXException; /** * This is the base class for applying changes to generated Liquibase snapshots. This class provides @@ -69,7 +72,7 @@ public void addLicenseHeaderToFileIfNeeded(String path) throws IOException { log.info("The file '{}' already contains the OpenMRS license header.", path); } - public void createUpdatedChangeLogFile(String sourcePath, String targetPath) throws DocumentException, IOException { + public void createUpdatedChangeLogFile(String sourcePath, String targetPath) throws DocumentException, IOException, SAXException { deleteFile(targetPath); log.info("Updating generated Liquibase file: '{}'...", sourcePath); Document document = readChangeLogFile(sourcePath); @@ -130,7 +133,7 @@ boolean isLicenseHeaderInFile(String path) throws FileNotFoundException { return false; } - Document readChangeLogFile(String path) throws DocumentException { + static Document readChangeLogFile(String path) throws DocumentException, SAXException { File file = Paths.get(path).toFile(); if (!file.exists()) { log.error("The source file '{}' does not exist. Please generate both Liquibase changelog files and retry. " @@ -139,6 +142,10 @@ Document readChangeLogFile(String path) throws DocumentException { System.exit(0); } SAXReader reader = new SAXReader(); + reader.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); + reader.setFeature("http://xml.org/sax/features/external-general-entities", false); + reader.setFeature("http://xml.org/sax/features/external-parameter-entities", false); + Document document; try { document = reader.read(file); @@ -147,9 +154,9 @@ Document readChangeLogFile(String path) throws DocumentException { log.error("processing the file '{}' raised an exception", path, e); throw e; } - return document; + return document; } - + Document readChangeLogResource(String resourceName) throws DocumentException, IOException { Document document; try (InputStream is = getResourceAsStream(resourceName)) { diff --git a/liquibase/src/main/java/org/openmrs/liquibase/Main.java b/liquibase/src/main/java/org/openmrs/liquibase/Main.java index 34da5a15157..7c1cece1015 100644 --- a/liquibase/src/main/java/org/openmrs/liquibase/Main.java +++ b/liquibase/src/main/java/org/openmrs/liquibase/Main.java @@ -13,6 +13,7 @@ import java.nio.file.Paths; import org.dom4j.DocumentException; +import org.xml.sax.SAXException; public class Main { @@ -37,7 +38,7 @@ public class Main { schemaOnlyTuner = new SchemaOnlyTuner(); } - public static void main(String[] args) throws DocumentException, IOException { + public static void main(String[] args) throws DocumentException, IOException, SAXException { coreDataTuner.addLicenseHeaderToFileIfNeeded(LIQUIBASE_CORE_DATA_SOURCE_PATH); coreDataTuner.createUpdatedChangeLogFile(LIQUIBASE_CORE_DATA_SOURCE_PATH, LIQUIBASE_CORE_DATA_TARGET_PATH); schemaOnlyTuner.addLicenseHeaderToFileIfNeeded(LIQUIBASE_SCHEMA_ONLY_SOURCE_PATH); diff --git a/liquibase/src/test/java/org/openmrs/liquibase/AbstractSnapshotTunerTest.java b/liquibase/src/test/java/org/openmrs/liquibase/AbstractSnapshotTunerTest.java index 1170a377815..687cdb00660 100644 --- a/liquibase/src/test/java/org/openmrs/liquibase/AbstractSnapshotTunerTest.java +++ b/liquibase/src/test/java/org/openmrs/liquibase/AbstractSnapshotTunerTest.java @@ -13,6 +13,7 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.equalToCompressingWhiteSpace; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import java.io.File; @@ -23,6 +24,7 @@ import java.nio.file.Path; import java.nio.file.Paths; +import org.dom4j.DocumentException; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; @@ -121,4 +123,17 @@ private String readFile(File file) throws IOException { return AbstractSnapshotTuner.readInputStream(is); } } + + //TODO: This behaviour should be mocked because directly creating a vulnerable file + // and attempting an XXE attack in a test environment is not recommended. + // It could potentially expose the system to real attacks, even if unintentional. + @Test + public void testReadChangeLogFile_shouldThrowDocumentExceptionIncaseOfAnXxeAttack() throws Exception { + String xmlContent = "]>&ext;"; + Path tempFilePath = Paths.get("test.xml"); + Files.write(tempFilePath, xmlContent.getBytes()); + + assertThrows(DocumentException.class, () -> AbstractSnapshotTuner.readChangeLogFile(String.valueOf(tempFilePath))); + assertTrue(Files.deleteIfExists(tempFilePath)); //ensure the xml file is deleted after the assertion + } } diff --git a/liquibase/src/test/java/org/openmrs/liquibase/CoreDataTunerTest.java b/liquibase/src/test/java/org/openmrs/liquibase/CoreDataTunerTest.java index 74140c67c86..62a1bd16c2f 100644 --- a/liquibase/src/test/java/org/openmrs/liquibase/CoreDataTunerTest.java +++ b/liquibase/src/test/java/org/openmrs/liquibase/CoreDataTunerTest.java @@ -34,6 +34,7 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; +import org.xml.sax.SAXException; public class CoreDataTunerTest { @@ -66,7 +67,7 @@ public void setup() throws DocumentException, IOException { } @Test - public void shouldCreateUpdatedChangeLogFile(@TempDir Path tempDir) throws DocumentException, IOException { + public void shouldCreateUpdatedChangeLogFile(@TempDir Path tempDir) throws DocumentException, IOException, SAXException { // given String sourcePath = PATH_TO_TEST_RESOURCES + File.separator + LIQUIBASE_CORE_DATA_SNAPSHOT_XML; String targetPath = tempDir.resolve("liquibase-core-data-UPDATED-SNAPSHOT.xml").toString(); diff --git a/liquibase/src/test/java/org/openmrs/liquibase/MainTest.java b/liquibase/src/test/java/org/openmrs/liquibase/MainTest.java index 17286590f5f..9c2e1690cb7 100644 --- a/liquibase/src/test/java/org/openmrs/liquibase/MainTest.java +++ b/liquibase/src/test/java/org/openmrs/liquibase/MainTest.java @@ -13,6 +13,7 @@ import org.dom4j.DocumentException; import org.junit.jupiter.api.Test; import org.mockito.Mockito; +import org.xml.sax.SAXException; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.times; @@ -20,7 +21,7 @@ public class MainTest { @Test - public void shouldCreateUpdatedSnapshotFiles() throws DocumentException, IOException { + public void shouldCreateUpdatedSnapshotFiles() throws DocumentException, IOException, SAXException { CoreDataTuner coreDataTuner = Mockito.mock(CoreDataTuner.class); SchemaOnlyTuner schemaOnlyTuner = Mockito.mock(SchemaOnlyTuner.class); diff --git a/liquibase/src/test/java/org/openmrs/liquibase/SchemaOnlyTunerTest.java b/liquibase/src/test/java/org/openmrs/liquibase/SchemaOnlyTunerTest.java index 9d53e6b35b6..b03dbd89b18 100644 --- a/liquibase/src/test/java/org/openmrs/liquibase/SchemaOnlyTunerTest.java +++ b/liquibase/src/test/java/org/openmrs/liquibase/SchemaOnlyTunerTest.java @@ -34,6 +34,7 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; import org.mockito.Mockito; +import org.xml.sax.SAXException; public class SchemaOnlyTunerTest { @@ -60,7 +61,7 @@ public void setup() throws DocumentException, IOException { } @Test - public void shouldCreateUpdatedChangeLogFile(@TempDir Path tempDir) throws DocumentException, IOException { + public void shouldCreateUpdatedChangeLogFile(@TempDir Path tempDir) throws DocumentException, IOException, SAXException { // given String sourcePath = PATH_TO_TEST_RESOURCES + File.separator + LIQUIBASE_SCHEMA_ONLY_SNAPSHOT_XML; String targetPath = tempDir.resolve("liquibase-schema-only-UPDATED-SNAPSHOT.xml").toString();