From 25a39fca9ce1e740c5e962e5e2aaeba659f4a6fc Mon Sep 17 00:00:00 2001 From: Marcel Date: Sun, 17 May 2026 14:48:03 +0200 Subject: [PATCH] security(import): harden DocumentBuilderFactory against XXE in MassImportService Extract XxeSafeXmlParser with all 6 OWASP-recommended features (disallow-doctype-decl, external-general-entities, external-parameter-entities, load-external-dtd, XInclude, expandEntityReferences). Make readOds() package-private; add failing-then-passing regression test and valid-ODS guard test. POI 5.5.0 does not mitigate this: the vulnerable parser is a custom DocumentBuilderFactory call in readOds(), not inside POI's internal ODS reader. The hardening is defence-in-depth, not redundant with POI defaults. Closes #528 Co-Authored-By: Claude Sonnet 4.6 --- .../importing/MassImportService.java | 4 +- .../importing/XxeSafeXmlParser.java | 20 ++++++ .../importing/MassImportServiceTest.java | 67 +++++++++++++++++++ 3 files changed, 89 insertions(+), 2 deletions(-) create mode 100644 backend/src/main/java/org/raddatz/familienarchiv/importing/XxeSafeXmlParser.java diff --git a/backend/src/main/java/org/raddatz/familienarchiv/importing/MassImportService.java b/backend/src/main/java/org/raddatz/familienarchiv/importing/MassImportService.java index ea648870..d6453e19 100644 --- a/backend/src/main/java/org/raddatz/familienarchiv/importing/MassImportService.java +++ b/backend/src/main/java/org/raddatz/familienarchiv/importing/MassImportService.java @@ -168,14 +168,14 @@ public class MassImportService { * Reads an ODS file by parsing its content.xml directly (no extra library needed). * ODS is a ZIP archive; content.xml holds the spreadsheet data as XML. */ - private List> readOds(File file) throws Exception { + List> readOds(File file) throws Exception { List> result = new ArrayList<>(); try (ZipFile zip = new ZipFile(file)) { var entry = zip.getEntry("content.xml"); if (entry == null) throw new RuntimeException("Ungültige ODS-Datei: content.xml fehlt"); - var factory = DocumentBuilderFactory.newInstance(); + var factory = XxeSafeXmlParser.hardenedFactory(); factory.setNamespaceAware(true); var builder = factory.newDocumentBuilder(); var doc = builder.parse(zip.getInputStream(entry)); diff --git a/backend/src/main/java/org/raddatz/familienarchiv/importing/XxeSafeXmlParser.java b/backend/src/main/java/org/raddatz/familienarchiv/importing/XxeSafeXmlParser.java new file mode 100644 index 00000000..949ea054 --- /dev/null +++ b/backend/src/main/java/org/raddatz/familienarchiv/importing/XxeSafeXmlParser.java @@ -0,0 +1,20 @@ +package org.raddatz.familienarchiv.importing; + +import javax.xml.parsers.DocumentBuilderFactory; +import javax.xml.parsers.ParserConfigurationException; + +class XxeSafeXmlParser { + + private XxeSafeXmlParser() {} + + static DocumentBuilderFactory hardenedFactory() throws ParserConfigurationException { + var factory = DocumentBuilderFactory.newInstance(); + factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); + factory.setFeature("http://xml.org/sax/features/external-general-entities", false); + factory.setFeature("http://xml.org/sax/features/external-parameter-entities", false); + factory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false); + factory.setXIncludeAware(false); + factory.setExpandEntityReferences(false); + return factory; + } +} diff --git a/backend/src/test/java/org/raddatz/familienarchiv/importing/MassImportServiceTest.java b/backend/src/test/java/org/raddatz/familienarchiv/importing/MassImportServiceTest.java index 845aaf3b..dfd63607 100644 --- a/backend/src/test/java/org/raddatz/familienarchiv/importing/MassImportServiceTest.java +++ b/backend/src/test/java/org/raddatz/familienarchiv/importing/MassImportServiceTest.java @@ -21,9 +21,12 @@ import software.amazon.awssdk.services.s3.S3Client; import software.amazon.awssdk.services.s3.model.PutObjectRequest; import org.apache.poi.xssf.usermodel.XSSFWorkbook; +import org.xml.sax.SAXParseException; import java.io.File; import java.io.OutputStream; +import java.io.ByteArrayOutputStream; +import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; import java.time.LocalDate; @@ -32,6 +35,8 @@ import java.util.ArrayList; import java.util.List; import java.util.Optional; import java.util.UUID; +import java.util.zip.ZipEntry; +import java.util.zip.ZipOutputStream; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; @@ -520,6 +525,25 @@ class MassImportServiceTest { assertThat(result).isEqualTo("hello"); } + // ─── readOds — XXE security regression ─────────────────────────────────── + + // Security regression — do not remove. Introduced by issue #528. + @Test + void readOds_rejects_xxe_doctype_payload(@TempDir Path tempDir) throws Exception { + File malicious = buildXxeOds(tempDir, "file:///etc/hostname"); + assertThatThrownBy(() -> service.readOds(malicious)) + .isInstanceOf(SAXParseException.class) + .hasMessageContaining("DOCTYPE is disallowed"); + } + + @Test + void readOds_parses_valid_ods_correctly(@TempDir Path tempDir) throws Exception { + File valid = buildValidOds(tempDir, "Mustermann"); + List> rows = service.readOds(valid); + assertThat(rows).isNotEmpty(); + assertThat(rows.get(0)).contains("Mustermann"); + } + // ─── helpers ────────────────────────────────────────────────────────────── /** @@ -553,4 +577,47 @@ class MassImportServiceTest { "" // 13: transcription ); } + + /** Creates a minimal ODS ZIP containing a content.xml with an XXE payload. */ + private File buildXxeOds(Path dir, String entityTarget) throws Exception { + String xml = "" + + "]>" + + "" + + "" + + "" + + "&xxe;" + + "" + + "" + + ""; + return writeOdsZip(dir.resolve("malicious.ods"), xml); + } + + /** Creates a minimal valid ODS ZIP containing a content.xml with the given cell value. */ + private File buildValidOds(Path dir, String cellValue) throws Exception { + String xml = "" + + "" + + "" + + "" + + "" + cellValue + "" + + "" + + "" + + ""; + return writeOdsZip(dir.resolve("valid.ods"), xml); + } + + private File writeOdsZip(Path destination, String contentXml) throws Exception { + try (OutputStream fos = Files.newOutputStream(destination); + ZipOutputStream zip = new ZipOutputStream(fos)) { + zip.putNextEntry(new ZipEntry("content.xml")); + zip.write(contentXml.getBytes(StandardCharsets.UTF_8)); + zip.closeEntry(); + } + return destination.toFile(); + } }