From f77127ae6100bcad8ca23589c5da49c8f1c9fc6d Mon Sep 17 00:00:00 2001 From: Stefan Weil Date: Mon, 10 Aug 2026 11:45:16 +0200 Subject: [PATCH 01/11] Prevent XML External Entity injection and DTD expansion (Coverity) Introduce a shared XMLSecurity utility class providing hardened DocumentBuilder, Transformer, Schema, SAXParser and XMLInputFactory instances that reject DOCTYPE declarations and disable external entity resolution to prevent XML External Entity (XXE) and entity expansion attacks. Use the hardened factories in all XML parsing and transformation code paths and null-check the Transformer returned by the factory. CIDs: 486979, 486985, 486986, 486976, 486990, 487007, 454260, 454251, 431632, 431623, 431616, 431601, 431588, 431586, 431585, 431567, 501288, 501287, 501299, 501300, 501305, 501296, 501292, 501285, 415127, 415247, 415087, 501286, 501294, 501303, 503907, 503911 Assisted-by: OpenCode / big-pickle (opencode) Signed-off-by: Stefan Weil --- .../XmlResponseHandler.java | 12 +- .../java/org/kitodo/utils/XMLSecurity.java | 154 ++++++++++++++++++ .../org/kitodo/dataeditor/JaxbXmlUtils.java | 12 +- .../java/org/kitodo/docket/ExportDocket.java | 13 +- .../kitodo/queryurlimport/QueryURLImport.java | 11 +- .../FileStructureValidation.java | 8 +- .../java/org/kitodo/export/ExportMets.java | 3 +- .../kitodo/production/editor/XMLEditor.java | 11 +- .../kitodo/production/helper/XMLUtils.java | 52 +----- .../services/data/ProcessService.java | 3 +- .../services/dataformat/MetsService.java | 9 +- .../thread/ImportEadProcessesThread.java | 3 +- 12 files changed, 212 insertions(+), 79 deletions(-) create mode 100644 Kitodo-API/src/main/java/org/kitodo/utils/XMLSecurity.java diff --git a/Kitodo-API/src/main/java/org/kitodo/api/externaldatamanagement/XmlResponseHandler.java b/Kitodo-API/src/main/java/org/kitodo/api/externaldatamanagement/XmlResponseHandler.java index 50dece0066c..2b34ba41307 100644 --- a/Kitodo-API/src/main/java/org/kitodo/api/externaldatamanagement/XmlResponseHandler.java +++ b/Kitodo-API/src/main/java/org/kitodo/api/externaldatamanagement/XmlResponseHandler.java @@ -13,7 +13,6 @@ import java.io.ByteArrayInputStream; import java.io.IOException; -import java.lang.reflect.UndeclaredThrowableException; import java.nio.charset.Charset; import java.nio.charset.StandardCharsets; import java.util.LinkedList; @@ -34,6 +33,7 @@ import org.kitodo.exceptions.CatalogException; import org.kitodo.exceptions.ConfigException; import org.kitodo.exceptions.NoRecordFoundException; +import org.kitodo.utils.XMLSecurity; import org.w3c.dom.Document; import org.w3c.dom.Element; import org.w3c.dom.NodeList; @@ -42,17 +42,17 @@ public class XmlResponseHandler { - private static final DocumentBuilderFactory documentBuilderFactory = DocumentBuilderFactory.newInstance(); + private static final DocumentBuilderFactory documentBuilderFactory; private static final XMLOutputter xmlOutputter = new XMLOutputter(); private static final XPath xPath = XPathFactory.newInstance().newXPath(); static { - documentBuilderFactory.setNamespaceAware(true); try { - documentBuilderFactory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); - } catch (ParserConfigurationException parserConfigurationException) { - throw new UndeclaredThrowableException(parserConfigurationException); + documentBuilderFactory = XMLSecurity.newDocumentBuilderFactory(); + } catch (ParserConfigurationException e) { + throw new ExceptionInInitializerError(e); } + documentBuilderFactory.setNamespaceAware(true); xmlOutputter.setFormat(Format.getPrettyFormat()); } diff --git a/Kitodo-API/src/main/java/org/kitodo/utils/XMLSecurity.java b/Kitodo-API/src/main/java/org/kitodo/utils/XMLSecurity.java new file mode 100644 index 00000000000..9cc55e5d21d --- /dev/null +++ b/Kitodo-API/src/main/java/org/kitodo/utils/XMLSecurity.java @@ -0,0 +1,154 @@ +/* + * (c) Kitodo. Key to digital objects e. V. + * + * This file is part of the Kitodo project. + * + * It is licensed under GNU General Public License version 3 or later. + * + * For the full copyright and license information, please read the + * GPL3-License.txt file that was distributed with this source code. + */ + +package org.kitodo.utils; + +import java.io.InputStream; + +import javax.xml.XMLConstants; +import javax.xml.parsers.DocumentBuilderFactory; +import javax.xml.parsers.ParserConfigurationException; +import javax.xml.parsers.SAXParserFactory; +import javax.xml.stream.XMLInputFactory; +import javax.xml.transform.TransformerFactory; +import javax.xml.transform.sax.SAXSource; +import javax.xml.validation.SchemaFactory; + +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; +import org.xml.sax.InputSource; +import org.xml.sax.SAXException; +import org.xml.sax.SAXNotRecognizedException; +import org.xml.sax.SAXNotSupportedException; +import org.xml.sax.XMLReader; + +/** + * Provides factory instances that are hardened against XML External Entity + * (XXE) injection and unrestricted document type definitions. + */ +public final class XMLSecurity { + + private static final Logger logger = LogManager.getLogger(XMLSecurity.class); + + private static final String DISALLOW_DOCTYPE_DECL = "http://apache.org/xml/features/disallow-doctype-decl"; + private static final String EXTERNAL_GENERAL_ENTITIES = "http://xml.org/sax/features/external-general-entities"; + private static final String EXTERNAL_PARAMETER_ENTITIES = "http://xml.org/sax/features/external-parameter-entities"; + + private XMLSecurity() { + } + + /** + * Create and return a DocumentBuilderFactory that rejects DOCTYPE declarations and + * external entity resolution. + * + * @return hardened DocumentBuilderFactory + * @throws ParserConfigurationException if a feature cannot be set + */ + public static DocumentBuilderFactory newDocumentBuilderFactory() throws ParserConfigurationException { + DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); + factory.setFeature(DISALLOW_DOCTYPE_DECL, true); + factory.setFeature(EXTERNAL_GENERAL_ENTITIES, false); + factory.setFeature(EXTERNAL_PARAMETER_ENTITIES, false); + factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); + factory.setXIncludeAware(false); + factory.setExpandEntityReferences(false); + return factory; + } + + /** + * Create and return a TransformerFactory that restricts access to external DTDs + * and stylesheets to prevent XML External Entity (XXE) injection. + * + * @return hardened TransformerFactory + */ + public static TransformerFactory newTransformerFactory() { + TransformerFactory factory = TransformerFactory.newInstance(); + try { + factory.setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, ""); + factory.setAttribute(XMLConstants.ACCESS_EXTERNAL_STYLESHEET, ""); + } catch (IllegalArgumentException e) { + logger.warn("Unable to restrict external access on TransformerFactory '{}': {}", + factory.getClass().getName(), e.getMessage()); + } + return factory; + } + + /** + * Create and return a SchemaFactory that rejects external DTD access to prevent + * XML External Entity (XXE) injection during XML validation. + * + * @return hardened SchemaFactory + */ + public static SchemaFactory newSchemaFactory() { + SchemaFactory factory = SchemaFactory.newInstance(XMLConstants.W3C_XML_SCHEMA_NS_URI); + try { + factory.setProperty(XMLConstants.ACCESS_EXTERNAL_DTD, ""); + } catch (IllegalArgumentException | SAXNotRecognizedException | SAXNotSupportedException e) { + logger.warn("Unable to restrict external access on SchemaFactory '{}': {}", + factory.getClass().getName(), e.getMessage()); + } + return factory; + } + + /** + * Create and return an XMLInputFactory with DTD support and external entity + * resolution disabled to prevent XML External Entity (XXE) injection. + * + * @return hardened XMLInputFactory + */ + public static XMLInputFactory newXmlInputFactory() { + XMLInputFactory factory = XMLInputFactory.newInstance(); + factory.setProperty(XMLInputFactory.SUPPORT_DTD, false); + factory.setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false); + return factory; + } + + /** + * Create and return a SAXParserFactory that rejects DOCTYPE declarations and + * external entity resolution to prevent XML External Entity (XXE) injection. + * + * @return hardened SAXParserFactory + */ + public static SAXParserFactory newSaxParserFactory() { + SAXParserFactory factory = SAXParserFactory.newInstance(); + factory.setValidating(false); + factory.setNamespaceAware(true); + try { + factory.setFeature(DISALLOW_DOCTYPE_DECL, true); + factory.setFeature(EXTERNAL_GENERAL_ENTITIES, false); + factory.setFeature(EXTERNAL_PARAMETER_ENTITIES, false); + factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); + } catch (ParserConfigurationException | SAXNotRecognizedException | SAXNotSupportedException e) { + throw new IllegalStateException("Unable to harden SAXParserFactory", e); + } + return factory; + } + + /** + * Create and return a SAXSource that rejects DOCTYPE declarations and external + * entity resolution to prevent XML External Entity (XXE) injection during + * transformation of the given input stream. + * + * @param inputStream input stream containing the XML document to transform + * @return hardened SAXSource + * @throws ParserConfigurationException if a feature cannot be set + * @throws SAXException if the SAX parser cannot be created + */ + public static SAXSource newSecureSource(InputStream inputStream) throws ParserConfigurationException, SAXException { + SAXParserFactory factory = SAXParserFactory.newInstance(); + factory.setFeature(DISALLOW_DOCTYPE_DECL, true); + factory.setFeature(EXTERNAL_GENERAL_ENTITIES, false); + factory.setFeature(EXTERNAL_PARAMETER_ENTITIES, false); + factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); + XMLReader reader = factory.newSAXParser().getXMLReader(); + return new SAXSource(reader, new InputSource(inputStream)); + } +} diff --git a/Kitodo-DataEditor/src/main/java/org/kitodo/dataeditor/JaxbXmlUtils.java b/Kitodo-DataEditor/src/main/java/org/kitodo/dataeditor/JaxbXmlUtils.java index af52fe5ee28..1491c02061d 100644 --- a/Kitodo-DataEditor/src/main/java/org/kitodo/dataeditor/JaxbXmlUtils.java +++ b/Kitodo-DataEditor/src/main/java/org/kitodo/dataeditor/JaxbXmlUtils.java @@ -26,9 +26,10 @@ import javax.xml.datatype.DatatypeConfigurationException; import javax.xml.datatype.DatatypeFactory; import javax.xml.datatype.XMLGregorianCalendar; +import javax.xml.parsers.ParserConfigurationException; +import javax.xml.transform.Source; import javax.xml.transform.Transformer; import javax.xml.transform.TransformerException; -import javax.xml.transform.TransformerFactory; import javax.xml.transform.stream.StreamResult; import javax.xml.transform.stream.StreamSource; @@ -38,6 +39,8 @@ import org.kitodo.dataformat.metskitodo.KitodoType; import org.kitodo.dataformat.metskitodo.MdSecType; import org.kitodo.serviceloader.KitodoServiceLoader; +import org.kitodo.utils.XMLSecurity; +import org.xml.sax.SAXException; /** * Provides methods for handling jaxb generated java objects and xml files. @@ -61,15 +64,16 @@ private JaxbXmlUtils() { */ static String transformXmlByXslt(URI xmlFile, URI xslFile) throws TransformerException, IOException { FileManagementInterface fileManagementModule = new KitodoServiceLoader<>(FileManagementInterface.class).loadModule(); - TransformerFactory factory = TransformerFactory.newInstance(); StreamSource styleSource = new StreamSource(xslFile.getPath()); - Transformer transformer = factory.newTransformer(styleSource); + Transformer transformer = XMLSecurity.newTransformerFactory().newTransformer(styleSource); try (InputStream inputStream = fileManagementModule.read(xmlFile); StringWriter stringWriter = new StringWriter()) { - StreamSource source = new StreamSource(inputStream); + Source source = XMLSecurity.newSecureSource(inputStream); StreamResult result = new StreamResult(stringWriter); transformer.transform(source, result); return stringWriter.toString(); + } catch (ParserConfigurationException | SAXException e) { + throw new IOException(e); } } diff --git a/Kitodo-Docket/src/main/java/org/kitodo/docket/ExportDocket.java b/Kitodo-Docket/src/main/java/org/kitodo/docket/ExportDocket.java index 80ffc2fe5c8..eaa140f279b 100644 --- a/Kitodo-Docket/src/main/java/org/kitodo/docket/ExportDocket.java +++ b/Kitodo-Docket/src/main/java/org/kitodo/docket/ExportDocket.java @@ -17,10 +17,11 @@ import java.io.IOException; import java.io.OutputStream; +import javax.xml.parsers.ParserConfigurationException; import javax.xml.transform.Result; +import javax.xml.transform.Source; import javax.xml.transform.Transformer; import javax.xml.transform.TransformerException; -import javax.xml.transform.TransformerFactory; import javax.xml.transform.sax.SAXResult; import javax.xml.transform.stream.StreamSource; @@ -30,6 +31,8 @@ import org.apache.fop.apps.FopFactoryBuilder; import org.apache.fop.apps.MimeConstants; import org.kitodo.api.docket.DocketData; +import org.kitodo.utils.XMLSecurity; +import org.xml.sax.SAXException; /** * This class provides generating a run note based on the generated xml log. @@ -90,7 +93,6 @@ void startExport(Iterable docketDataList, OutputStream os) throws IO private byte[] generatePdfBytes(ByteArrayOutputStream out) throws IOException { // generate pdf file - StreamSource source = new StreamSource(new ByteArrayInputStream(out.toByteArray())); StreamSource transformSource = new StreamSource(xsltFile); FopFactoryBuilder builder = new FopFactoryBuilder(new File(".").toURI()); builder.setStrictFOValidation(false); @@ -98,14 +100,19 @@ private byte[] generatePdfBytes(ByteArrayOutputStream out) throws IOException { ByteArrayOutputStream outStream = new ByteArrayOutputStream(); // transform xml try { - Transformer xslTransformer = TransformerFactory.newInstance().newTransformer(transformSource); + Transformer xslTransformer = XMLSecurity.newTransformerFactory().newTransformer(transformSource); Fop fop = fopFactory.newFop(MimeConstants.MIME_PDF, outStream); Result res = new SAXResult(fop.getDefaultHandler()); + Source source = XMLSecurity.newSecureSource(new ByteArrayInputStream(out.toByteArray())); xslTransformer.transform(source, res); } catch (FOPException e) { throw new IOException("FOPException occurred", e); } catch (TransformerException e) { throw new IOException("TransformerException occurred", e); + } catch (ParserConfigurationException e) { + throw new IOException("ParserConfigurationException occurred", e); + } catch (SAXException e) { + throw new IOException("SAXException occurred", e); } // write the content to output stream diff --git a/Kitodo-Query-URL-Import/src/main/java/org/kitodo/queryurlimport/QueryURLImport.java b/Kitodo-Query-URL-Import/src/main/java/org/kitodo/queryurlimport/QueryURLImport.java index 912a8e3acdf..add93e7f41d 100644 --- a/Kitodo-Query-URL-Import/src/main/java/org/kitodo/queryurlimport/QueryURLImport.java +++ b/Kitodo-Query-URL-Import/src/main/java/org/kitodo/queryurlimport/QueryURLImport.java @@ -33,13 +33,10 @@ import java.util.Objects; import java.util.stream.Collectors; -import javax.xml.XMLConstants; import javax.xml.parsers.DocumentBuilder; -import javax.xml.parsers.DocumentBuilderFactory; import javax.xml.parsers.ParserConfigurationException; import javax.xml.transform.Transformer; import javax.xml.transform.TransformerException; -import javax.xml.transform.TransformerFactory; import javax.xml.transform.dom.DOMSource; import javax.xml.transform.stream.StreamResult; @@ -76,6 +73,7 @@ import org.kitodo.exceptions.CatalogException; import org.kitodo.exceptions.ConfigException; import org.kitodo.exceptions.NoRecordFoundException; +import org.kitodo.utils.XMLSecurity; import org.w3c.dom.Document; import org.w3c.dom.Node; import org.w3c.dom.NodeList; @@ -428,16 +426,13 @@ private String createSearchFieldString(SearchInterfaceType interfaceType, Linked private Document stringToDocument(String xmlContent) throws ParserConfigurationException, IOException, SAXException { - DocumentBuilderFactory documentBuilderFactory = DocumentBuilderFactory.newInstance(); - documentBuilderFactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); - documentBuilderFactory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); - DocumentBuilder documentBuilder = documentBuilderFactory.newDocumentBuilder(); + DocumentBuilder documentBuilder = XMLSecurity.newDocumentBuilderFactory().newDocumentBuilder(); return documentBuilder.parse(new InputSource(new StringReader(xmlContent))); } private String nodeToString(Node node) throws TransformerException { StringWriter writer = new StringWriter(); - Transformer transformer = TransformerFactory.newInstance().newTransformer(); + Transformer transformer = XMLSecurity.newTransformerFactory().newTransformer(); transformer.transform(new DOMSource(node), new StreamResult(writer)); return writer.toString(); } diff --git a/Kitodo-Validation/src/main/java/org/kitodo/validation/filestructure/FileStructureValidation.java b/Kitodo-Validation/src/main/java/org/kitodo/validation/filestructure/FileStructureValidation.java index 43a00b0a654..3d31ff924b1 100644 --- a/Kitodo-Validation/src/main/java/org/kitodo/validation/filestructure/FileStructureValidation.java +++ b/Kitodo-Validation/src/main/java/org/kitodo/validation/filestructure/FileStructureValidation.java @@ -32,6 +32,7 @@ import org.kitodo.api.validation.State; import org.kitodo.api.validation.ValidationResult; import org.kitodo.api.validation.filestructure.FileStructureValidationInterface; +import org.kitodo.utils.XMLSecurity; import org.xml.sax.SAXException; import org.xml.sax.SAXParseException; @@ -87,13 +88,18 @@ private ValidationResult validateStreamSource(StreamSource source, Validator val private Validator initializeXmlValidator(Collection xsdFilePaths) throws SAXException { FileStructureValidationErrorHandler xmlValidationErrorHandler = new FileStructureValidationErrorHandler(); - SchemaFactory schemaFactory = SchemaFactory.newInstance(XMLConstants.W3C_XML_SCHEMA_NS_URI); + SchemaFactory schemaFactory = XMLSecurity.newSchemaFactory(); Source[] sources = new Source[xsdFilePaths.size()]; for (int i = 0; i < xsdFilePaths.size(); i++) { sources[i] = new StreamSource(new File(xsdFilePaths.toArray(new URI[0])[i])); } Schema schema = schemaFactory.newSchema(sources); Validator xmlValidator = schema.newValidator(); + try { + xmlValidator.setProperty(XMLConstants.ACCESS_EXTERNAL_DTD, ""); + } catch (IllegalArgumentException e) { + logger.warn("Unable to restrict external access on Validator: {}", e.getMessage()); + } xmlValidator.setErrorHandler(xmlValidationErrorHandler); return xmlValidator; } diff --git a/Kitodo/src/main/java/org/kitodo/export/ExportMets.java b/Kitodo/src/main/java/org/kitodo/export/ExportMets.java index 18481e92eeb..c62f2f215cb 100644 --- a/Kitodo/src/main/java/org/kitodo/export/ExportMets.java +++ b/Kitodo/src/main/java/org/kitodo/export/ExportMets.java @@ -53,6 +53,7 @@ import org.kitodo.production.helper.tasks.EmptyTask; import org.kitodo.production.services.ServiceManager; import org.kitodo.production.services.file.FileService; +import org.kitodo.utils.XMLSecurity; import org.w3c.dom.Document; import org.w3c.dom.Element; import org.xml.sax.SAXException; @@ -248,7 +249,7 @@ private void updateInternalLabelsIfNeeded(URI metaFile, byte[] xmlBytes, Process private Map extractLabels(byte[] xmlBytes) { Map labels = new HashMap<>(); try { - DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); + DocumentBuilderFactory factory = XMLSecurity.newDocumentBuilderFactory(); factory.setNamespaceAware(true); Document doc = factory.newDocumentBuilder().parse(new ByteArrayInputStream(xmlBytes)); XPath xpath = XPathFactory.newInstance().newXPath(); diff --git a/Kitodo/src/main/java/org/kitodo/production/editor/XMLEditor.java b/Kitodo/src/main/java/org/kitodo/production/editor/XMLEditor.java index f332ebcb07f..38ffd732c87 100644 --- a/Kitodo/src/main/java/org/kitodo/production/editor/XMLEditor.java +++ b/Kitodo/src/main/java/org/kitodo/production/editor/XMLEditor.java @@ -22,14 +22,11 @@ import java.util.stream.Collectors; import java.util.stream.Stream; -import javax.xml.XMLConstants; import javax.xml.parsers.DocumentBuilder; -import javax.xml.parsers.DocumentBuilderFactory; import javax.xml.parsers.ParserConfigurationException; import javax.xml.transform.Transformer; import javax.xml.transform.TransformerConfigurationException; import javax.xml.transform.TransformerException; -import javax.xml.transform.TransformerFactory; import javax.xml.transform.dom.DOMSource; import javax.xml.transform.stream.StreamResult; @@ -43,6 +40,7 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.kitodo.config.enums.KitodoConfigFile; +import org.kitodo.utils.XMLSecurity; import org.w3c.dom.Document; import org.xml.sax.InputSource; import org.xml.sax.SAXException; @@ -63,9 +61,7 @@ public class XMLEditor implements Serializable { */ public XMLEditor() { try { - DocumentBuilderFactory documentBuilderFactory = DocumentBuilderFactory.newInstance(); - documentBuilderFactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); - documentBuilder = documentBuilderFactory.newDocumentBuilder(); + documentBuilder = XMLSecurity.newDocumentBuilderFactory().newDocumentBuilder(); } catch (ParserConfigurationException e) { logger.error("ERROR: unable to instantiate document builder: {}", e.getMessage()); } @@ -136,8 +132,7 @@ public void saveXMLConfiguration() { logger.info("Saving configuration to file {}", currentConfigurationFile); try { Document document = documentBuilder.parse(new InputSource(new StringReader(this.xmlConfigurationString))); - TransformerFactory transformerFactory = TransformerFactory.newInstance(); - Transformer transformer = transformerFactory.newTransformer(); + Transformer transformer = XMLSecurity.newTransformerFactory().newTransformer(); DOMSource domSource = new DOMSource(document); try (FileOutputStream outputStream = new FileOutputStream(configurationFile.getFile(), false); PrintWriter printWriter = new PrintWriter(outputStream)) { diff --git a/Kitodo/src/main/java/org/kitodo/production/helper/XMLUtils.java b/Kitodo/src/main/java/org/kitodo/production/helper/XMLUtils.java index 18087448f64..16029caf06b 100644 --- a/Kitodo/src/main/java/org/kitodo/production/helper/XMLUtils.java +++ b/Kitodo/src/main/java/org/kitodo/production/helper/XMLUtils.java @@ -23,12 +23,10 @@ import java.util.NoSuchElementException; import java.util.Objects; -import javax.xml.XMLConstants; import javax.xml.parsers.DocumentBuilder; import javax.xml.parsers.DocumentBuilderFactory; import javax.xml.parsers.ParserConfigurationException; import javax.xml.parsers.SAXParser; -import javax.xml.parsers.SAXParserFactory; import javax.xml.stream.XMLInputFactory; import javax.xml.stream.XMLStreamConstants; import javax.xml.stream.XMLStreamException; @@ -36,7 +34,6 @@ import javax.xml.transform.OutputKeys; import javax.xml.transform.Transformer; import javax.xml.transform.TransformerException; -import javax.xml.transform.TransformerFactory; import javax.xml.transform.dom.DOMSource; import javax.xml.transform.stream.StreamResult; import javax.xml.xpath.XPath; @@ -50,6 +47,7 @@ import org.kitodo.api.schemaconverter.MetadataFormat; import org.kitodo.constants.StringConstants; import org.kitodo.data.database.beans.ImportConfiguration; +import org.kitodo.utils.XMLSecurity; import org.w3c.dom.Document; import org.w3c.dom.Element; import org.w3c.dom.Node; @@ -63,10 +61,6 @@ */ public class XMLUtils { - private static final String DISALLOW_DOCTYPE_DECL = "http://apache.org/xml/features/disallow-doctype-decl"; - private static final String EXTERNAL_GENERAL_ENTITIES = "http://xml.org/sax/features/external-general-entities"; - private static final String EXTERNAL_PARAMETER_ENTITIES = "http://xml.org/sax/features/external-parameter-entities"; - /** * Private constructor to hide the implicit public one. */ @@ -91,7 +85,7 @@ private XMLUtils() { public static byte[] documentToByteArray(Document data, Integer indent) throws TransformerException { ByteArrayOutputStream result = new ByteArrayOutputStream(); - Transformer transformer = TransformerFactory.newInstance().newTransformer(); + Transformer transformer = XMLSecurity.newTransformerFactory().newTransformer(); if (Objects.nonNull(indent)) { transformer.setOutputProperty(OutputKeys.INDENT, "yes"); transformer.setOutputProperty("{http://xml.apache.org/xslt}indent-amount", indent.toString()); @@ -143,9 +137,7 @@ public static Element getFirstChildWithTagName(Node data, String tagName) { */ public static Document load(InputStream data) throws SAXException, IOException { try { - DocumentBuilderFactory documentBuilderFactory = DocumentBuilderFactory.newInstance(); - documentBuilderFactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); - return documentBuilderFactory.newDocumentBuilder().parse(data); + return XMLSecurity.newDocumentBuilderFactory().newDocumentBuilder().parse(data); } catch (ParserConfigurationException e) { throw new IOException(e.getMessage(), e); } @@ -164,9 +156,7 @@ public static Document load(InputStream data) throws SAXException, IOException { */ public static Document newDocument() throws IOException { try { - DocumentBuilderFactory documentBuilderFactory = DocumentBuilderFactory.newInstance(); - documentBuilderFactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); - return documentBuilderFactory.newDocumentBuilder().newDocument(); + return XMLSecurity.newDocumentBuilderFactory().newDocumentBuilder().newDocument(); } catch (ParserConfigurationException e) { throw new IOException(e.getMessage(), e); } @@ -184,9 +174,8 @@ public static Document newDocument() throws IOException { */ public static Document parseXMLString(String xmlString) throws IOException, ParserConfigurationException, SAXException { - DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); + DocumentBuilderFactory factory = XMLSecurity.newDocumentBuilderFactory(); factory.setNamespaceAware(true); - disableExternalEntities(factory); DocumentBuilder builder = factory.newDocumentBuilder(); xmlString = removeBom(xmlString); return builder.parse(new InputSource(new ByteArrayInputStream(xmlString.getBytes(StandardCharsets.UTF_8)))); @@ -250,7 +239,7 @@ public static List getElementsByTagNameAndAttributeValue(Document docum */ public static String elementToString(Element element) throws TransformerException { StringWriter stringWriter = new StringWriter(); - Transformer transformer = TransformerFactory.newInstance().newTransformer(); + Transformer transformer = XMLSecurity.newTransformerFactory().newTransformer(); transformer.transform(new DOMSource(element), new StreamResult(stringWriter)); return stringWriter.toString(); } @@ -283,9 +272,7 @@ public static DataRecord createRecordFromXMLElement(String xmlContent, ImportCon */ public static int getNumberOfEADElements(String xmlString, String eadLevel) throws XMLStreamException { int count = 0; - XMLInputFactory factory = XMLInputFactory.newInstance(); - factory.setProperty(XMLInputFactory.SUPPORT_DTD, false); - factory.setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false); + XMLInputFactory factory = XMLSecurity.newXmlInputFactory(); XMLStreamReader reader = factory.createXMLStreamReader(new StringReader(xmlString)); while (reader.hasNext()) { int event = reader.next(); @@ -310,34 +297,11 @@ public static int getNumberOfEADElements(String xmlString, String eadLevel) thro public static void checkIfXmlIsWellFormed(String xmlContent) throws IOException, SAXException { SAXParser saxParser; try { - SAXParserFactory saxParserFactory = SAXParserFactory.newInstance(); - saxParserFactory.setValidating(false); - saxParserFactory.setNamespaceAware(true); - saxParserFactory.setFeature(DISALLOW_DOCTYPE_DECL, true); - - saxParser = saxParserFactory.newSAXParser(); + saxParser = XMLSecurity.newSaxParserFactory().newSAXParser(); } catch (ParserConfigurationException | SAXException e) { throw new RuntimeException(e); } InputSource inputSource = new InputSource(new StringReader(xmlContent)); saxParser.parse(inputSource, new DefaultHandler()); } - - /** - * Disable DOCTYPE declarations and external entity resolution on the given - * factory to prevent XML External Entity (XXE) injection. This mirrors the - * secure configuration already used by {@link #load(InputStream)} and the - * external-catalog response parsers. - * - * @param factory the DocumentBuilderFactory to harden - * @throws ParserConfigurationException if a feature cannot be set - */ - private static void disableExternalEntities(DocumentBuilderFactory factory) throws ParserConfigurationException { - factory.setFeature(DISALLOW_DOCTYPE_DECL, true); - factory.setFeature(EXTERNAL_GENERAL_ENTITIES, false); - factory.setFeature(EXTERNAL_PARAMETER_ENTITIES, false); - factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); - factory.setXIncludeAware(false); - factory.setExpandEntityReferences(false); - } } diff --git a/Kitodo/src/main/java/org/kitodo/production/services/data/ProcessService.java b/Kitodo/src/main/java/org/kitodo/production/services/data/ProcessService.java index e7de3b0ac7f..ff57a344d9f 100644 --- a/Kitodo/src/main/java/org/kitodo/production/services/data/ProcessService.java +++ b/Kitodo/src/main/java/org/kitodo/production/services/data/ProcessService.java @@ -127,6 +127,7 @@ import org.kitodo.production.services.workflow.WorkflowControllerService; import org.kitodo.production.workflow.KitodoNamespaceContext; import org.kitodo.serviceloader.KitodoServiceLoader; +import org.kitodo.utils.XMLSecurity; import org.primefaces.model.SortOrder; import org.primefaces.model.charts.ChartData; import org.primefaces.model.charts.axes.cartesian.linear.CartesianLinearAxes; @@ -1939,7 +1940,7 @@ public static void deleteSymlinksFromUserHomes(Task task) { */ public NodeList getNodeListFromMetadataFile(Process process, String xpath) throws IOException { try (InputStream fileInputStream = ServiceManager.getFileService().readMetadataFile(process)) { - DocumentBuilderFactory builderFactory = DocumentBuilderFactory.newInstance(); + DocumentBuilderFactory builderFactory = XMLSecurity.newDocumentBuilderFactory(); builderFactory.setNamespaceAware(true); DocumentBuilder builder = builderFactory.newDocumentBuilder(); org.w3c.dom.Document xmlDocument = builder.parse(fileInputStream); diff --git a/Kitodo/src/main/java/org/kitodo/production/services/dataformat/MetsService.java b/Kitodo/src/main/java/org/kitodo/production/services/dataformat/MetsService.java index f260d3485b2..364520306df 100644 --- a/Kitodo/src/main/java/org/kitodo/production/services/dataformat/MetsService.java +++ b/Kitodo/src/main/java/org/kitodo/production/services/dataformat/MetsService.java @@ -22,8 +22,8 @@ import javax.xml.transform.Result; import javax.xml.transform.Source; +import javax.xml.transform.Transformer; import javax.xml.transform.TransformerException; -import javax.xml.transform.TransformerFactory; import javax.xml.transform.dom.DOMSource; import javax.xml.transform.stream.StreamResult; @@ -37,6 +37,7 @@ import org.kitodo.production.helper.XMLUtils; import org.kitodo.production.services.ServiceManager; import org.kitodo.serviceloader.KitodoServiceLoader; +import org.kitodo.utils.XMLSecurity; import org.w3c.dom.Document; import org.xml.sax.SAXException; @@ -175,7 +176,11 @@ public Workpiece loadWorkpiece(Document document) throws TransformerException, I ByteArrayOutputStream outputStream = new ByteArrayOutputStream(); Source xmlSource = new DOMSource(document); Result outputTarget = new StreamResult(outputStream); - TransformerFactory.newInstance().newTransformer().transform(xmlSource, outputTarget); + Transformer transformer = XMLSecurity.newTransformerFactory().newTransformer(); + if (Objects.isNull(transformer)) { + throw new IOException("Unable to create transformer"); + } + transformer.transform(xmlSource, outputTarget); InputStream inputStream = new ByteArrayInputStream(outputStream.toByteArray()); return metsXmlElementAccess.read(inputStream); } diff --git a/Kitodo/src/main/java/org/kitodo/production/thread/ImportEadProcessesThread.java b/Kitodo/src/main/java/org/kitodo/production/thread/ImportEadProcessesThread.java index 15ae54b3bcb..33ab2909fe7 100644 --- a/Kitodo/src/main/java/org/kitodo/production/thread/ImportEadProcessesThread.java +++ b/Kitodo/src/main/java/org/kitodo/production/thread/ImportEadProcessesThread.java @@ -69,6 +69,7 @@ import org.kitodo.production.services.ServiceManager; import org.kitodo.production.services.data.ImportService; import org.kitodo.production.services.data.ProcessService; +import org.kitodo.utils.XMLSecurity; import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; import org.springframework.security.core.Authentication; import org.springframework.security.core.context.SecurityContext; @@ -133,7 +134,7 @@ public void run() { boolean stopOnError = ConfigCore.getBooleanParameter(ParameterCore.STOP_EAD_COLLECTION_IMPORT_ON_EXCEPTION); try { int numberOfElements = XMLUtils.getNumberOfEADElements(xmlString, eadLevel); - XMLInputFactory inputFactory = XMLInputFactory.newInstance(); + XMLInputFactory inputFactory = XMLSecurity.newXmlInputFactory(); XMLEventReader eventReader = inputFactory.createXMLEventReader(new StringReader(xmlString)); boolean inProcessElement = false; boolean inParentProcessElement = false; From abd4401070a2df8a7822d37a6ab1590204fbeb5e Mon Sep 17 00:00:00 2001 From: Stefan Weil Date: Mon, 10 Aug 2026 12:37:13 +0200 Subject: [PATCH 02/11] Fix XSLT transformation with hardened SAX source (Coverity) XMLSecurity.newSecureSource() did not enable namespace awareness on the SAX parser, so XSLT transformations that depend on namespaced elements produced output without namespace URIs. This broke the conversion of old Goobi format metadata files in Kitodo-DataEditor (CI failure). CIDs: 501288, 501292, 501305 Assisted-by: OpenCode / big-pickle (opencode) Signed-off-by: Stefan Weil --- Kitodo-API/src/main/java/org/kitodo/utils/XMLSecurity.java | 1 + 1 file changed, 1 insertion(+) diff --git a/Kitodo-API/src/main/java/org/kitodo/utils/XMLSecurity.java b/Kitodo-API/src/main/java/org/kitodo/utils/XMLSecurity.java index 9cc55e5d21d..7f11670afd1 100644 --- a/Kitodo-API/src/main/java/org/kitodo/utils/XMLSecurity.java +++ b/Kitodo-API/src/main/java/org/kitodo/utils/XMLSecurity.java @@ -144,6 +144,7 @@ public static SAXParserFactory newSaxParserFactory() { */ public static SAXSource newSecureSource(InputStream inputStream) throws ParserConfigurationException, SAXException { SAXParserFactory factory = SAXParserFactory.newInstance(); + factory.setNamespaceAware(true); factory.setFeature(DISALLOW_DOCTYPE_DECL, true); factory.setFeature(EXTERNAL_GENERAL_ENTITIES, false); factory.setFeature(EXTERNAL_PARAMETER_ENTITIES, false); From 1ab022c80a5a95d13a46ff41bc4df9eb4d460201 Mon Sep 17 00:00:00 2001 From: Stefan Weil Date: Mon, 14 Sep 2026 11:14:28 +0200 Subject: [PATCH 03/11] Move Validator hardening to XMLSecurity utility Add a newSecureValidator method to the XMLSecurity class to centralize the configuration of hardened XML Validators (restricting external DTD access), mirroring the existing factory helpers. Update FileStructureValidation to use this utility instead of configuring the Validator inline. Assisted-by: OpenCode / qwen3.8-27b-thinking (Alibaba Cloud) Suggested-by: Arved Solth Signed-off-by: Stefan Weil --- .../java/org/kitodo/utils/XMLSecurity.java | 24 +++++++++++++++++-- .../FileStructureValidation.java | 8 +------ 2 files changed, 23 insertions(+), 9 deletions(-) diff --git a/Kitodo-API/src/main/java/org/kitodo/utils/XMLSecurity.java b/Kitodo-API/src/main/java/org/kitodo/utils/XMLSecurity.java index 7f11670afd1..77992e6756d 100644 --- a/Kitodo-API/src/main/java/org/kitodo/utils/XMLSecurity.java +++ b/Kitodo-API/src/main/java/org/kitodo/utils/XMLSecurity.java @@ -20,7 +20,9 @@ import javax.xml.stream.XMLInputFactory; import javax.xml.transform.TransformerFactory; import javax.xml.transform.sax.SAXSource; +import javax.xml.validation.Schema; import javax.xml.validation.SchemaFactory; +import javax.xml.validation.Validator; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -133,8 +135,8 @@ public static SAXParserFactory newSaxParserFactory() { } /** - * Create and return a SAXSource that rejects DOCTYPE declarations and external - * entity resolution to prevent XML External Entity (XXE) injection during + * Create and return a hardened SAXSource that rejects DOCTYPE declarations and + * external entity resolution to prevent XML External Entity (XXE) injection during * transformation of the given input stream. * * @param inputStream input stream containing the XML document to transform @@ -152,4 +154,22 @@ public static SAXSource newSecureSource(InputStream inputStream) throws ParserCo XMLReader reader = factory.newSAXParser().getXMLReader(); return new SAXSource(reader, new InputSource(inputStream)); } + + /** + * Create and return a Validator from the given Schema with external DTD access + * restricted, to prevent XML External Entity (XXE) injection during validation. + * + * @param schema compiled XML schema + * @return hardened Validator + * @throws SAXException if the Validator cannot be created + */ + public static Validator newSecureValidator(Schema schema) throws SAXException { + Validator validator = schema.newValidator(); + try { + validator.setProperty(XMLConstants.ACCESS_EXTERNAL_DTD, ""); + } catch (IllegalArgumentException | SAXNotRecognizedException | SAXNotSupportedException e) { + logger.warn("Unable to restrict external access on Validator: {}", e.getMessage()); + } + return validator; + } } diff --git a/Kitodo-Validation/src/main/java/org/kitodo/validation/filestructure/FileStructureValidation.java b/Kitodo-Validation/src/main/java/org/kitodo/validation/filestructure/FileStructureValidation.java index 3d31ff924b1..c586c6b193e 100644 --- a/Kitodo-Validation/src/main/java/org/kitodo/validation/filestructure/FileStructureValidation.java +++ b/Kitodo-Validation/src/main/java/org/kitodo/validation/filestructure/FileStructureValidation.java @@ -20,7 +20,6 @@ import java.util.List; import java.util.stream.Collectors; -import javax.xml.XMLConstants; import javax.xml.transform.Source; import javax.xml.transform.stream.StreamSource; import javax.xml.validation.Schema; @@ -94,12 +93,7 @@ private Validator initializeXmlValidator(Collection xsdFilePaths) throws SA sources[i] = new StreamSource(new File(xsdFilePaths.toArray(new URI[0])[i])); } Schema schema = schemaFactory.newSchema(sources); - Validator xmlValidator = schema.newValidator(); - try { - xmlValidator.setProperty(XMLConstants.ACCESS_EXTERNAL_DTD, ""); - } catch (IllegalArgumentException e) { - logger.warn("Unable to restrict external access on Validator: {}", e.getMessage()); - } + Validator xmlValidator = XMLSecurity.newSecureValidator(schema); xmlValidator.setErrorHandler(xmlValidationErrorHandler); return xmlValidator; } From f6fd6cbdc2981860270b6778f836985ddf3e07f7 Mon Sep 17 00:00:00 2001 From: Stefan Weil Date: Mon, 14 Sep 2026 11:20:20 +0200 Subject: [PATCH 04/11] Add tests for XMLSecurity hardening Add unit tests for the XMLSecurity utility class ensuring the hardened factories reject DOCTYPE declarations and do not resolve external entities (XXE), mirroring the test approach from #7072. Also covers the new newSecureValidator helper. Assisted-by: OpenCode / qwen3.8-27b-thinking (Alibaba Cloud) Signed-off-by: Stefan Weil --- .../org/kitodo/utils/XMLSecurityTest.java | 185 ++++++++++++++++++ 1 file changed, 185 insertions(+) create mode 100644 Kitodo-API/src/test/java/org/kitodo/utils/XMLSecurityTest.java diff --git a/Kitodo-API/src/test/java/org/kitodo/utils/XMLSecurityTest.java b/Kitodo-API/src/test/java/org/kitodo/utils/XMLSecurityTest.java new file mode 100644 index 00000000000..55fcbeaadda --- /dev/null +++ b/Kitodo-API/src/test/java/org/kitodo/utils/XMLSecurityTest.java @@ -0,0 +1,185 @@ +/* + * (c) Kitodo. Key to digital objects e. V. + * + * This file is part of the Kitodo project. + * + * It is licensed under GNU General Public License version 3 or later. + * + * For the full copyright and license information, please read the + * GPL3-License.txt file that was distributed with this source code. + */ + +package org.kitodo.utils; + +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import java.io.ByteArrayInputStream; +import java.io.File; +import java.io.IOException; +import java.io.InputStream; +import java.io.StringReader; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; + +import javax.xml.parsers.DocumentBuilder; +import javax.xml.parsers.DocumentBuilderFactory; +import javax.xml.parsers.ParserConfigurationException; +import javax.xml.parsers.SAXParserFactory; +import javax.xml.stream.XMLInputFactory; +import javax.xml.stream.XMLStreamException; +import javax.xml.stream.XMLStreamReader; +import javax.xml.transform.Source; +import javax.xml.transform.sax.SAXSource; +import javax.xml.transform.stream.StreamSource; +import javax.xml.validation.Schema; +import javax.xml.validation.Validator; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; +import org.xml.sax.InputSource; +import org.xml.sax.SAXException; +import org.xml.sax.XMLReader; + +public class XMLSecurityTest { + + private static final String CANARY = "XXE-CANARY-SECRET"; + private static final String VALID_XML = "Text"; + + @TempDir + Path tempDir; + + @Test + public void factoriesShouldBeNonNull() throws Exception { + assertNotNull(XMLSecurity.newDocumentBuilderFactory()); + assertNotNull(XMLSecurity.newTransformerFactory()); + assertNotNull(XMLSecurity.newSchemaFactory()); + assertNotNull(XMLSecurity.newXmlInputFactory()); + assertNotNull(XMLSecurity.newSaxParserFactory()); + } + + @Test + public void documentBuilderFactoryShouldNotResolveExternalEntities() throws Exception { + File secret = createTestFile(); + String payload = xxePayload(secret); + DocumentBuilderFactory factory = XMLSecurity.newDocumentBuilderFactory(); + SAXException exception = assertThrows(SAXException.class, + () -> parseWithDocumentBuilder(factory, payload)); + assertFalse(exception.getMessage().contains(CANARY), "secret must not leak into the error"); + } + + @Test + public void documentBuilderFactoryShouldParseValidXml() throws Exception { + DocumentBuilderFactory factory = XMLSecurity.newDocumentBuilderFactory(); + assertDoesNotThrow(() -> parseWithDocumentBuilder(factory, VALID_XML)); + } + + @Test + public void saxParserFactoryShouldNotResolveExternalEntities() throws Exception { + File secret = createTestFile(); + String payload = xxePayload(secret); + SAXParserFactory factory = XMLSecurity.newSaxParserFactory(); + XMLReader reader = factory.newSAXParser().getXMLReader(); + SAXException exception = assertThrows(SAXException.class, + () -> reader.parse(new InputSource(toInputStream(payload)))); + assertFalse(exception.getMessage().contains(CANARY), "secret must not leak into the error"); + } + + @Test + public void secureSourceShouldNotResolveExternalEntities() throws Exception { + File secret = createTestFile(); + String payload = xxePayload(secret); + SAXSource source = XMLSecurity.newSecureSource(toInputStream(payload)); + SAXException exception = assertThrows(SAXException.class, + () -> source.getXMLReader().parse(source.getInputSource())); + assertFalse(exception.getMessage().contains(CANARY), "secret must not leak into the error"); + } + + @Test + public void secureSourceShouldParseValidXml() throws Exception { + SAXSource source = XMLSecurity.newSecureSource(toInputStream(VALID_XML)); + assertDoesNotThrow(() -> source.getXMLReader().parse(source.getInputSource())); + } + + @Test + public void xmlInputFactoryShouldNotResolveExternalEntities() throws Exception { + File secret = createTestFile(); + String payload = xxePayload(secret); + XMLInputFactory factory = XMLSecurity.newXmlInputFactory(); + XMLStreamException exception = assertThrows(XMLStreamException.class, + () -> readWithStax(factory, payload)); + assertFalse(String.valueOf(exception.getMessage()).contains(CANARY), + "secret must not leak into the error"); + } + + @Test + public void secureValidatorShouldValidateXml() throws Exception { + Schema schema = XMLSecurity.newSchemaFactory().newSchema(new StreamSource(new StringReader(XSD))); + Validator validator = XMLSecurity.newSecureValidator(schema); + assertNotNull(validator); + assertDoesNotThrow(() -> validator.validate(new StreamSource(new StringReader( + "hi")))); + assertThrows(SAXException.class, () -> validator.validate(new StreamSource(new StringReader( + "x")))); + } + + @Test + public void secureValidatorShouldNotResolveExternalEntities() throws Exception { + File secret = createTestFile(); + String payload = xxePayload(secret); + Schema schema = XMLSecurity.newSchemaFactory().newSchema(new StreamSource(new StringReader(XSD))); + Validator validator = XMLSecurity.newSecureValidator(schema); + // The document neither matches the schema nor is the external DTD reachable, so + // validation must fail rather than disclosing the file content of the canary. + SAXException exception = assertThrows(SAXException.class, + () -> validator.validate(new StreamSource(new StringReader(payload)))); + assertFalse(exception.getMessage().contains(CANARY), "secret must not leak into the error"); + } + + private void parseWithDocumentBuilder(DocumentBuilderFactory factory, String xml) + throws IOException, SAXException, ParserConfigurationException { + DocumentBuilder builder = factory.newDocumentBuilder(); + builder.parse(new InputSource(new ByteArrayInputStream(xml.getBytes(StandardCharsets.UTF_8)))); + } + + private String readWithStax(XMLInputFactory factory, String xml) throws XMLStreamException { + XMLStreamReader reader = factory.createXMLStreamReader(new StringReader(xml)); + while (reader.hasNext()) { + reader.next(); + } + return ""; + } + + private InputStream toInputStream(String xml) { + return new ByteArrayInputStream(xml.getBytes(StandardCharsets.UTF_8)); + } + + private String xxePayload(File secret) { + return "\n" + + " ]>\n" + + "&xxe;"; + } + + private File createTestFile() throws IOException { + Path secretPath = Files.createTempFile(tempDir, "xxe-canary", ".txt"); + File secret = secretPath.toFile(); + secret.deleteOnExit(); + Files.writeString(secret.toPath(), CANARY); + return secret; + } + + private static final String XSD = + "\n" + + "\n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + ""; +} From d0edb01710309f62fcf7d0ad94426d70be38a84b Mon Sep 17 00:00:00 2001 From: Stefan Weil Date: Mon, 14 Sep 2026 11:36:08 +0200 Subject: [PATCH 05/11] Harden XMLSecurity fail-closed and restrict external schema access Address Copilot review on the XXE hardening: - Fail closed instead of silently returning an unhardened instance when a security property cannot be set (newTransformerFactory, newSchemaFactory, newSecureValidator now throw IllegalStateException). - Restrict external schema resolution to local files only (ACCESS_EXTERNAL_SCHEMA=file) on the SchemaFactory and Validator so xs:import/xs:include cannot fetch remote schemas. 'file' keeps the bundled local schema imports (e.g. mods-3-4.xsd) working. - Add regression test ensuring local xs:import still resolves. - Update class-level and method-level Javadoc to reflect fail-closed behavior and external-schema restriction. - Drop the now-unused Log4j logger. Assisted-by: OpenCode / qwen3.8-27b-thinking (Alibaba Cloud) Signed-off-by: Stefan Weil --- .../java/org/kitodo/utils/XMLSecurity.java | 35 ++++++++++++------- .../org/kitodo/utils/XMLSecurityTest.java | 29 +++++++++++++++ 2 files changed, 51 insertions(+), 13 deletions(-) diff --git a/Kitodo-API/src/main/java/org/kitodo/utils/XMLSecurity.java b/Kitodo-API/src/main/java/org/kitodo/utils/XMLSecurity.java index 77992e6756d..2de6450d8b3 100644 --- a/Kitodo-API/src/main/java/org/kitodo/utils/XMLSecurity.java +++ b/Kitodo-API/src/main/java/org/kitodo/utils/XMLSecurity.java @@ -24,8 +24,6 @@ import javax.xml.validation.SchemaFactory; import javax.xml.validation.Validator; -import org.apache.logging.log4j.LogManager; -import org.apache.logging.log4j.Logger; import org.xml.sax.InputSource; import org.xml.sax.SAXException; import org.xml.sax.SAXNotRecognizedException; @@ -34,12 +32,14 @@ /** * Provides factory instances that are hardened against XML External Entity - * (XXE) injection and unrestricted document type definitions. + * (XXE) injection and unrestricted document type definitions. Every helper + * fails closed: if a required security feature cannot be set it returns an + * error rather than a silently unhardened instance. Schema resolution is + * additionally restricted to local files, so xs:import/xs:include cannot reach + * the network while the bundled local schema imports continue to resolve. */ public final class XMLSecurity { - private static final Logger logger = LogManager.getLogger(XMLSecurity.class); - private static final String DISALLOW_DOCTYPE_DECL = "http://apache.org/xml/features/disallow-doctype-decl"; private static final String EXTERNAL_GENERAL_ENTITIES = "http://xml.org/sax/features/external-general-entities"; private static final String EXTERNAL_PARAMETER_ENTITIES = "http://xml.org/sax/features/external-parameter-entities"; @@ -70,6 +70,7 @@ public static DocumentBuilderFactory newDocumentBuilderFactory() throws ParserCo * and stylesheets to prevent XML External Entity (XXE) injection. * * @return hardened TransformerFactory + * @throws IllegalStateException if the hardening properties cannot be set */ public static TransformerFactory newTransformerFactory() { TransformerFactory factory = TransformerFactory.newInstance(); @@ -77,25 +78,30 @@ public static TransformerFactory newTransformerFactory() { factory.setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, ""); factory.setAttribute(XMLConstants.ACCESS_EXTERNAL_STYLESHEET, ""); } catch (IllegalArgumentException e) { - logger.warn("Unable to restrict external access on TransformerFactory '{}': {}", - factory.getClass().getName(), e.getMessage()); + throw new IllegalStateException( + "Unable to harden TransformerFactory " + factory.getClass().getName(), e); } return factory; } /** - * Create and return a SchemaFactory that rejects external DTD access to prevent - * XML External Entity (XXE) injection during XML validation. + * Create and return a SchemaFactory that rejects external DTD access and restricts + * external schema resolution to local files only, to prevent XML External Entity + * (XXE) injection and remote schema retrieval during XML validation. The bundled + * local schema imports (e.g. mods-3-4.xsd) continue to resolve, while network + * schemas are denied. * * @return hardened SchemaFactory + * @throws IllegalStateException if the hardening properties cannot be set */ public static SchemaFactory newSchemaFactory() { SchemaFactory factory = SchemaFactory.newInstance(XMLConstants.W3C_XML_SCHEMA_NS_URI); try { factory.setProperty(XMLConstants.ACCESS_EXTERNAL_DTD, ""); + factory.setProperty(XMLConstants.ACCESS_EXTERNAL_SCHEMA, "file"); } catch (IllegalArgumentException | SAXNotRecognizedException | SAXNotSupportedException e) { - logger.warn("Unable to restrict external access on SchemaFactory '{}': {}", - factory.getClass().getName(), e.getMessage()); + throw new IllegalStateException( + "Unable to harden SchemaFactory " + factory.getClass().getName(), e); } return factory; } @@ -157,7 +163,9 @@ public static SAXSource newSecureSource(InputStream inputStream) throws ParserCo /** * Create and return a Validator from the given Schema with external DTD access - * restricted, to prevent XML External Entity (XXE) injection during validation. + * restricted and external schema resolution limited to local files, to prevent + * XML External Entity (XXE) injection and remote schema retrieval during + * validation. * * @param schema compiled XML schema * @return hardened Validator @@ -167,8 +175,9 @@ public static Validator newSecureValidator(Schema schema) throws SAXException { Validator validator = schema.newValidator(); try { validator.setProperty(XMLConstants.ACCESS_EXTERNAL_DTD, ""); + validator.setProperty(XMLConstants.ACCESS_EXTERNAL_SCHEMA, "file"); } catch (IllegalArgumentException | SAXNotRecognizedException | SAXNotSupportedException e) { - logger.warn("Unable to restrict external access on Validator: {}", e.getMessage()); + throw new IllegalStateException("Unable to harden Validator", e); } return validator; } diff --git a/Kitodo-API/src/test/java/org/kitodo/utils/XMLSecurityTest.java b/Kitodo-API/src/test/java/org/kitodo/utils/XMLSecurityTest.java index 55fcbeaadda..0dcd5ba1c7f 100644 --- a/Kitodo-API/src/test/java/org/kitodo/utils/XMLSecurityTest.java +++ b/Kitodo-API/src/test/java/org/kitodo/utils/XMLSecurityTest.java @@ -139,6 +139,20 @@ public void secureValidatorShouldNotResolveExternalEntities() throws Exception { assertFalse(exception.getMessage().contains(CANARY), "secret must not leak into the error"); } + @Test + public void schemaFactoryShouldStillAllowLocalSchemaImports() throws Exception { + // The bundled XSDs (e.g. mods-3-4.xsd) use local xs:import. Restricting + // external schema access to the "file" protocol must keep those resolving + // while still denying the network; a blanket deny would break them. + Path dep = tempDir.resolve("dep.xsd"); + Path main = tempDir.resolve("main.xsd"); + Files.writeString(dep, DEP_XSD); + Files.writeString(main, MAIN_XSD); + assertDoesNotThrow(() -> XMLSecurity.newSchemaFactory() + .newSchema(new StreamSource(main.toFile())), + "local xs:import must still resolve under ACCESS_EXTERNAL_SCHEMA=file"); + } + private void parseWithDocumentBuilder(DocumentBuilderFactory factory, String xml) throws IOException, SAXException, ParserConfigurationException { DocumentBuilder builder = factory.newDocumentBuilder(); @@ -182,4 +196,19 @@ private File createTestFile() throws IOException { + " \n" + " \n" + ""; + + private static final String DEP_XSD = + "\n" + + "\n" + + " \n" + + ""; + + private static final String MAIN_XSD = + "\n" + + "\n" + + " \n" + + " \n" + + ""; } From 36a4883be989c25b23e90338bc37da2fdb182934 Mon Sep 17 00:00:00 2001 From: Stefan Weil Date: Mon, 14 Sep 2026 11:51:49 +0200 Subject: [PATCH 06/11] Reject DOCTYPE in hardened Validator and strengthen XXE tests Address Copilot review on the XMLSecurity hardening: - newSecureValidator now also sets disallow-doctype-decl, so the Validator rejects DOCTYPE declarations entirely. This blocks internal entity expansion (billion-laughs), which ACCESS_EXTERNAL_DTD alone does not, and closes the gap where FileStructureValidation fed a raw StreamSource to the validator. - Rework the validator XXE test to use a schema-valid root containing the entity, so it genuinely fails for an insecure validator (it previously passed on the schema mismatch alone). - Add a test asserting internal (non-file) entity expansion is rejected. All XMLSecurityTest and FileStructureValidationTest cases pass. Assisted-by: OpenCode / qwen3.8-27b-thinking (Alibaba Cloud) Signed-off-by: Stefan Weil --- .../java/org/kitodo/utils/XMLSecurity.java | 9 +++---- .../org/kitodo/utils/XMLSecurityTest.java | 24 ++++++++++++++++--- 2 files changed, 26 insertions(+), 7 deletions(-) diff --git a/Kitodo-API/src/main/java/org/kitodo/utils/XMLSecurity.java b/Kitodo-API/src/main/java/org/kitodo/utils/XMLSecurity.java index 2de6450d8b3..bbaf1e0a77d 100644 --- a/Kitodo-API/src/main/java/org/kitodo/utils/XMLSecurity.java +++ b/Kitodo-API/src/main/java/org/kitodo/utils/XMLSecurity.java @@ -162,10 +162,10 @@ public static SAXSource newSecureSource(InputStream inputStream) throws ParserCo } /** - * Create and return a Validator from the given Schema with external DTD access - * restricted and external schema resolution limited to local files, to prevent - * XML External Entity (XXE) injection and remote schema retrieval during - * validation. + * Create and return a Validator from the given Schema that rejects DOCTYPE + * declarations (blocking external and internal entity expansion) and restricts + * external schema resolution to local files, to prevent XML External Entity + * (XXE) injection and remote schema retrieval during validation. * * @param schema compiled XML schema * @return hardened Validator @@ -174,6 +174,7 @@ public static SAXSource newSecureSource(InputStream inputStream) throws ParserCo public static Validator newSecureValidator(Schema schema) throws SAXException { Validator validator = schema.newValidator(); try { + validator.setProperty(DISALLOW_DOCTYPE_DECL, true); validator.setProperty(XMLConstants.ACCESS_EXTERNAL_DTD, ""); validator.setProperty(XMLConstants.ACCESS_EXTERNAL_SCHEMA, "file"); } catch (IllegalArgumentException | SAXNotRecognizedException | SAXNotSupportedException e) { diff --git a/Kitodo-API/src/test/java/org/kitodo/utils/XMLSecurityTest.java b/Kitodo-API/src/test/java/org/kitodo/utils/XMLSecurityTest.java index 0dcd5ba1c7f..e1f6fb9bd27 100644 --- a/Kitodo-API/src/test/java/org/kitodo/utils/XMLSecurityTest.java +++ b/Kitodo-API/src/test/java/org/kitodo/utils/XMLSecurityTest.java @@ -129,11 +129,29 @@ public void secureValidatorShouldValidateXml() throws Exception { @Test public void secureValidatorShouldNotResolveExternalEntities() throws Exception { File secret = createTestFile(); - String payload = xxePayload(secret); + // Schema-valid root: an INSECURE validator (external entities enabled, DOCTYPE + // allowed) would resolve &xxe; to the file content and validate successfully. + // A hardened validator must reject the DOCTYPE instead of completing. + String payload = "\n" + + " ]>\n" + + "&xxe;"; + Schema schema = XMLSecurity.newSchemaFactory().newSchema(new StreamSource(new StringReader(XSD))); + Validator validator = XMLSecurity.newSecureValidator(schema); + SAXException exception = assertThrows(SAXException.class, + () -> validator.validate(new StreamSource(new StringReader(payload)))); + assertFalse(exception.getMessage().contains(CANARY), "secret must not leak into the error"); + } + + @Test + public void secureValidatorShouldRejectInternalEntityExpansion() throws Exception { + // An internally declared entity (no SYSTEM/file) cannot be stopped by + // ACCESS_EXTERNAL_DTD; only rejecting the DOCTYPE declaration prevents its + // expansion. The root is schema-valid so an insecure validator would complete. + String payload = "\n" + + " ]>\n" + + "&bomb;"; Schema schema = XMLSecurity.newSchemaFactory().newSchema(new StreamSource(new StringReader(XSD))); Validator validator = XMLSecurity.newSecureValidator(schema); - // The document neither matches the schema nor is the external DTD reachable, so - // validation must fail rather than disclosing the file content of the canary. SAXException exception = assertThrows(SAXException.class, () -> validator.validate(new StreamSource(new StringReader(payload)))); assertFalse(exception.getMessage().contains(CANARY), "secret must not leak into the error"); From fa00b702525f43be85f0f98618b85e348479dc23 Mon Sep 17 00:00:00 2001 From: Stefan Weil Date: Mon, 14 Sep 2026 12:47:10 +0200 Subject: [PATCH 07/11] Move DOCTYPE rejection to hardened SAXSource for JDK 21 compatibility The JAXP Validator does not universally support the disallow-doctype-decl feature (it throws on JDK 21), so setting it in newSecureValidator broke the build in CI (Temurin 21). DOCTYPE declarations (external and internal entity expansion) are now rejected by feeding the validation input through a hardened SAXSource, whose SAXParserFactory-based reader supports the feature reliably across JDK versions. FileStructureValidation routes all input through the hardened source; the validator tests cover the SAXSource-based rejection. Verified with JDK 21 and 26: XMLSecurityTest and FileStructureValidationTest all pass. Assisted-by: OpenCode / qwen3.8-27b-thinking (Alibaba Cloud) Signed-off-by: Stefan Weil --- .../java/org/kitodo/utils/XMLSecurity.java | 13 +++++---- .../org/kitodo/utils/XMLSecurityTest.java | 16 ++++++----- .../FileStructureValidation.java | 27 +++++++++++++++---- 3 files changed, 39 insertions(+), 17 deletions(-) diff --git a/Kitodo-API/src/main/java/org/kitodo/utils/XMLSecurity.java b/Kitodo-API/src/main/java/org/kitodo/utils/XMLSecurity.java index bbaf1e0a77d..bbc3fe0c7a9 100644 --- a/Kitodo-API/src/main/java/org/kitodo/utils/XMLSecurity.java +++ b/Kitodo-API/src/main/java/org/kitodo/utils/XMLSecurity.java @@ -162,10 +162,14 @@ public static SAXSource newSecureSource(InputStream inputStream) throws ParserCo } /** - * Create and return a Validator from the given Schema that rejects DOCTYPE - * declarations (blocking external and internal entity expansion) and restricts - * external schema resolution to local files, to prevent XML External Entity - * (XXE) injection and remote schema retrieval during validation. + * Create and return a Validator from the given Schema with external DTD access + * blocked and external schema resolution limited to local files, to prevent + * XML External Entity (XXE) injection and remote schema retrieval during + * validation. + * + * DOCTYPE declarations must be rejected by the caller, e.g. by feeding the + * input through newSecureSource(), since the JAXP Validator does not + * universally support disallow-doctype-decl. * * @param schema compiled XML schema * @return hardened Validator @@ -174,7 +178,6 @@ public static SAXSource newSecureSource(InputStream inputStream) throws ParserCo public static Validator newSecureValidator(Schema schema) throws SAXException { Validator validator = schema.newValidator(); try { - validator.setProperty(DISALLOW_DOCTYPE_DECL, true); validator.setProperty(XMLConstants.ACCESS_EXTERNAL_DTD, ""); validator.setProperty(XMLConstants.ACCESS_EXTERNAL_SCHEMA, "file"); } catch (IllegalArgumentException | SAXNotRecognizedException | SAXNotSupportedException e) { diff --git a/Kitodo-API/src/test/java/org/kitodo/utils/XMLSecurityTest.java b/Kitodo-API/src/test/java/org/kitodo/utils/XMLSecurityTest.java index e1f6fb9bd27..51d18dfe535 100644 --- a/Kitodo-API/src/test/java/org/kitodo/utils/XMLSecurityTest.java +++ b/Kitodo-API/src/test/java/org/kitodo/utils/XMLSecurityTest.java @@ -129,31 +129,33 @@ public void secureValidatorShouldValidateXml() throws Exception { @Test public void secureValidatorShouldNotResolveExternalEntities() throws Exception { File secret = createTestFile(); - // Schema-valid root: an INSECURE validator (external entities enabled, DOCTYPE - // allowed) would resolve &xxe; to the file content and validate successfully. - // A hardened validator must reject the DOCTYPE instead of completing. + // Schema-valid root: feeding the input through the hardened SAXSource makes the + // parser reject the DOCTYPE, so &xxe; is never resolved and no file is read. String payload = "\n" + " ]>\n" + "&xxe;"; Schema schema = XMLSecurity.newSchemaFactory().newSchema(new StreamSource(new StringReader(XSD))); Validator validator = XMLSecurity.newSecureValidator(schema); + SAXSource secureSource = XMLSecurity.newSecureSource(toInputStream(payload)); SAXException exception = assertThrows(SAXException.class, - () -> validator.validate(new StreamSource(new StringReader(payload)))); + () -> validator.validate(secureSource)); assertFalse(exception.getMessage().contains(CANARY), "secret must not leak into the error"); } @Test public void secureValidatorShouldRejectInternalEntityExpansion() throws Exception { // An internally declared entity (no SYSTEM/file) cannot be stopped by - // ACCESS_EXTERNAL_DTD; only rejecting the DOCTYPE declaration prevents its - // expansion. The root is schema-valid so an insecure validator would complete. + // ACCESS_EXTERNAL_DTD; only the hardened SAXSource rejecting the DOCTYPE + // declaration prevents its expansion. The root is schema-valid so an + // insecure validator would complete. String payload = "\n" + " ]>\n" + "&bomb;"; Schema schema = XMLSecurity.newSchemaFactory().newSchema(new StreamSource(new StringReader(XSD))); Validator validator = XMLSecurity.newSecureValidator(schema); + SAXSource secureSource = XMLSecurity.newSecureSource(toInputStream(payload)); SAXException exception = assertThrows(SAXException.class, - () -> validator.validate(new StreamSource(new StringReader(payload)))); + () -> validator.validate(secureSource)); assertFalse(exception.getMessage().contains(CANARY), "secret must not leak into the error"); } diff --git a/Kitodo-Validation/src/main/java/org/kitodo/validation/filestructure/FileStructureValidation.java b/Kitodo-Validation/src/main/java/org/kitodo/validation/filestructure/FileStructureValidation.java index c586c6b193e..647719309c4 100644 --- a/Kitodo-Validation/src/main/java/org/kitodo/validation/filestructure/FileStructureValidation.java +++ b/Kitodo-Validation/src/main/java/org/kitodo/validation/filestructure/FileStructureValidation.java @@ -11,16 +11,21 @@ package org.kitodo.validation.filestructure; +import java.io.ByteArrayInputStream; import java.io.File; +import java.io.FileInputStream; import java.io.IOException; -import java.io.StringReader; +import java.io.InputStream; import java.net.URI; +import java.nio.charset.StandardCharsets; import java.util.Collection; import java.util.Collections; import java.util.List; import java.util.stream.Collectors; +import javax.xml.parsers.ParserConfigurationException; import javax.xml.transform.Source; +import javax.xml.transform.sax.SAXSource; import javax.xml.transform.stream.StreamSource; import javax.xml.validation.Schema; import javax.xml.validation.SchemaFactory; @@ -47,23 +52,35 @@ public class FileStructureValidation implements FileStructureValidationInterface public ValidationResult validate(String xmlContent, URI xsdFileUri) throws SAXException, IOException { Collection schemaUris = Collections.singletonList(xsdFileUri); Validator xmlValidator = initializeXmlValidator(schemaUris); - return validateStreamSource(new StreamSource(new StringReader(xmlContent)), xmlValidator, "N/A", schemaUris); + return validateStreamSource(createSecureSource(new ByteArrayInputStream(xmlContent.getBytes(StandardCharsets.UTF_8))), + xmlValidator, "N/A", schemaUris); } @Override public ValidationResult validate(URI xmlFileUri, URI xsdFileUri) throws SAXException, IOException { Collection schemaUris = Collections.singletonList(xsdFileUri); Validator xmlValidator = initializeXmlValidator(schemaUris); - return validateStreamSource(new StreamSource(new File(xmlFileUri)), xmlValidator, xmlFileUri.getPath(), schemaUris); + try (InputStream in = new FileInputStream(new File(xmlFileUri))) { + return validateStreamSource(createSecureSource(in), xmlValidator, xmlFileUri.getPath(), schemaUris); + } } @Override public ValidationResult validate(String xmlContent, Collection xsdFiles) throws IOException, SAXException { Validator xmlValidator = initializeXmlValidator(xsdFiles); - return validateStreamSource(new StreamSource(new StringReader(xmlContent)), xmlValidator, "N/A", xsdFiles); + return validateStreamSource(createSecureSource(new ByteArrayInputStream(xmlContent.getBytes(StandardCharsets.UTF_8))), + xmlValidator, "N/A", xsdFiles); + } + + private SAXSource createSecureSource(InputStream xmlInput) throws SAXException { + try { + return XMLSecurity.newSecureSource(xmlInput); + } catch (ParserConfigurationException e) { + throw new SAXException("Unable to create hardened SAXSource", e); + } } - private ValidationResult validateStreamSource(StreamSource source, Validator validator, String xmlPath, Collection xsdPaths) + private ValidationResult validateStreamSource(Source source, Validator validator, String xmlPath, Collection xsdPaths) throws IOException { try { validator.validate(source); From 8a24df26e9727c908497f8f2e5027bf6af5e1ece Mon Sep 17 00:00:00 2001 From: Stefan Weil Date: Mon, 14 Sep 2026 12:57:42 +0200 Subject: [PATCH 08/11] Fix JavadocParagraph checkstyle violation in XMLSecurity Add

tag after the blank line in the newSecureValidator Javadoc, as required by the checkstyle JavadocParagraph rule. This resolves the checkstyle-check build failure on kitodo-api. Assisted-by: OpenCode / qwen3.8-27b-thinking (Alibaba Cloud) Signed-off-by: Stefan Weil --- Kitodo-API/src/main/java/org/kitodo/utils/XMLSecurity.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Kitodo-API/src/main/java/org/kitodo/utils/XMLSecurity.java b/Kitodo-API/src/main/java/org/kitodo/utils/XMLSecurity.java index bbc3fe0c7a9..52ab31aafb1 100644 --- a/Kitodo-API/src/main/java/org/kitodo/utils/XMLSecurity.java +++ b/Kitodo-API/src/main/java/org/kitodo/utils/XMLSecurity.java @@ -167,7 +167,7 @@ public static SAXSource newSecureSource(InputStream inputStream) throws ParserCo * XML External Entity (XXE) injection and remote schema retrieval during * validation. * - * DOCTYPE declarations must be rejected by the caller, e.g. by feeding the + *

DOCTYPE declarations must be rejected by the caller, e.g. by feeding the * input through newSecureSource(), since the JAXP Validator does not * universally support disallow-doctype-decl. * From 3914c2fc41dcc985c931ce61be83aab9e643f7ab Mon Sep 17 00:00:00 2001 From: Stefan Weil Date: Mon, 14 Sep 2026 13:08:45 +0200 Subject: [PATCH 09/11] Fix Saxon TransformerFactory hardening and reject DOCTYPE in validation input newTransformerFactory() failed closed when a security attribute could not be set, but Saxon (used by Kitodo-Docket) does not support the JAXP accessExternalDTD/accessExternalStylesheet properties and throws on them, which broke the docket export in CI. The attributes are now applied best-effort: each is set independently, and an unsupported attribute is logged as a warning instead of aborting. Xalan (the default JDK factory) supports both, so it remains fully hardened. Validation input is now routed through XMLSecurity.newSecureSource(), whose hardened SAX reader rejects DOCTYPE declarations outright. This covers the gap left by removing disallow-doctype-decl from the Validator (which is not universally supported) and blocks both external-entity and internal-entity expansion during validation. Add FileStructureValidationTest cases asserting that DOCTYPE payloads with external and internal entities are rejected without leaking their content. Verified with JDK 21 and 26: XMLSecurityTest, FileStructureValidationTest and ExportDocketTest all pass; checkstyle clean. Assisted-by: OpenCode / qwen3.8-27b-thinking (Alibaba Cloud) Signed-off-by: Stefan Weil --- .../java/org/kitodo/utils/XMLSecurity.java | 45 +++++++++++++------ .../FileStructureValidationTest.java | 35 ++++++++++++++- 2 files changed, 66 insertions(+), 14 deletions(-) diff --git a/Kitodo-API/src/main/java/org/kitodo/utils/XMLSecurity.java b/Kitodo-API/src/main/java/org/kitodo/utils/XMLSecurity.java index 52ab31aafb1..177a4d76782 100644 --- a/Kitodo-API/src/main/java/org/kitodo/utils/XMLSecurity.java +++ b/Kitodo-API/src/main/java/org/kitodo/utils/XMLSecurity.java @@ -24,6 +24,8 @@ import javax.xml.validation.SchemaFactory; import javax.xml.validation.Validator; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; import org.xml.sax.InputSource; import org.xml.sax.SAXException; import org.xml.sax.SAXNotRecognizedException; @@ -32,14 +34,19 @@ /** * Provides factory instances that are hardened against XML External Entity - * (XXE) injection and unrestricted document type definitions. Every helper - * fails closed: if a required security feature cannot be set it returns an - * error rather than a silently unhardened instance. Schema resolution is - * additionally restricted to local files, so xs:import/xs:include cannot reach - * the network while the bundled local schema imports continue to resolve. + * (XXE) injection and unrestricted document type definitions. Schema + * resolution is additionally restricted to local files, so xs:import/ + * xs:include cannot reach the network while the bundled local schema imports + * continue to resolve. Where a hardening feature cannot be set, the helpers + * fail closed; the TransformerFactory helper is the exception, because not + * all vendor implementations (e.g. Saxon) support the JAXP external-access + * properties there. It applies the hardening best-effort and logs a warning + * per unsupported property. */ public final class XMLSecurity { + private static final Logger logger = LogManager.getLogger(XMLSecurity.class); + private static final String DISALLOW_DOCTYPE_DECL = "http://apache.org/xml/features/disallow-doctype-decl"; private static final String EXTERNAL_GENERAL_ENTITIES = "http://xml.org/sax/features/external-general-entities"; private static final String EXTERNAL_PARAMETER_ENTITIES = "http://xml.org/sax/features/external-parameter-entities"; @@ -66,22 +73,34 @@ public static DocumentBuilderFactory newDocumentBuilderFactory() throws ParserCo } /** - * Create and return a TransformerFactory that restricts access to external DTDs - * and stylesheets to prevent XML External Entity (XXE) injection. + * Create and return a TransformerFactory that restricts access to external + * DTDs and stylesheets to prevent XML External Entity (XXE) injection. + * + * The hardening is applied best-effort: the JAXP accessExternalDTD and + * accessExternalStylesheet properties are supported by the default Xalan + * factory, but not by all vendor implementations (e.g. Saxon). For a + * factory that does not support a property, the property is skipped and a + * warning is logged. Callers using such a factory must ensure the XML + * input is parsed through a hardened source (see newSecureSource()), + * because the transformer itself will not reject DTDs in that case. * * @return hardened TransformerFactory - * @throws IllegalStateException if the hardening properties cannot be set */ public static TransformerFactory newTransformerFactory() { TransformerFactory factory = TransformerFactory.newInstance(); + applyAttribute(factory, XMLConstants.ACCESS_EXTERNAL_DTD, ""); + applyAttribute(factory, XMLConstants.ACCESS_EXTERNAL_STYLESHEET, ""); + return factory; + } + + private static void applyAttribute(TransformerFactory factory, String name, Object value) { try { - factory.setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, ""); - factory.setAttribute(XMLConstants.ACCESS_EXTERNAL_STYLESHEET, ""); + factory.setAttribute(name, value); } catch (IllegalArgumentException e) { - throw new IllegalStateException( - "Unable to harden TransformerFactory " + factory.getClass().getName(), e); + logger.warn("TransformerFactory {} does not support the JAXP property '{}'; " + + "external DTD/stylesheet access cannot be restricted for this vendor. " + + "Input must be parsed via a hardened source.", factory.getClass().getName(), name); } - return factory; } /** diff --git a/Kitodo-Validation/src/test/java/org/kitodo/validation/filestructure/FileStructureValidationTest.java b/Kitodo-Validation/src/test/java/org/kitodo/validation/filestructure/FileStructureValidationTest.java index 600a3b011bb..2b9ab65777f 100644 --- a/Kitodo-Validation/src/test/java/org/kitodo/validation/filestructure/FileStructureValidationTest.java +++ b/Kitodo-Validation/src/test/java/org/kitodo/validation/filestructure/FileStructureValidationTest.java @@ -72,7 +72,40 @@ public void shouldFailToValidateInvalidXmlString() throws IOException, SAXExcept public void shouldFailToValidateMalformedXmlString() throws IOException, SAXException { String xmlContent = Files.readString(Paths.get(MALFORMED_MODS_FILE)); ValidationResult validationResult = xmlValidation.validate(xmlContent, modsSchema); - assertFalse(validationResult.getResultMessages().isEmpty(), "Validation should fail with malformed XML content string"); + assertFalse(validationResult.getResultMessages().isEmpty(), "Validation should fail with malformed XML file content string"); } + @Test + public void shouldRejectDoctypeWithExternalEntity() throws IOException, SAXException { + Path canary = Files.createTempFile("xxe-canary", ".txt"); + Files.writeString(canary, "XXE-CANARY-12345"); + try { + String xmlContent = "\n" + + " ]>\n" + + "&xxe;"; + ValidationResult validationResult = xmlValidation.validate(xmlContent, modsSchema); + assertFalse(validationResult.getResultMessages().isEmpty(), + "Validation should reject a DOCTYPE declaration carrying an external entity"); + for (String message : validationResult.getResultMessages()) { + assertFalse(message.contains("XXE-CANARY-12345"), + "External entity content must not be resolved or leaked"); + } + } finally { + Files.deleteIfExists(canary); + } + } + + @Test + public void shouldRejectDoctypeWithInternalEntity() throws IOException, SAXException { + String xmlContent = "\n" + + " ]>\n" + + "&bomb;"; + ValidationResult validationResult = xmlValidation.validate(xmlContent, modsSchema); + assertFalse(validationResult.getResultMessages().isEmpty(), + "Validation should reject a DOCTYPE declaration carrying an internal entity"); + for (String message : validationResult.getResultMessages()) { + assertFalse(message.contains("XXE-CANARY-12345"), + "Internal entity content must not be resolved or leaked"); + } + } } From 0941e018db7632fa1f71a6dcd36b57209ee239cb Mon Sep 17 00:00:00 2001 From: Stefan Weil Date: Mon, 14 Sep 2026 13:17:13 +0200 Subject: [PATCH 10/11] Make DOCTYPE-rejection tests discriminating in FileStructureValidationTest The DOCTYPE payloads used the MODS schema with a non-matching root element, so validation failed on the schema mismatch regardless of the hardened SAXSource and the tests would also pass on an unhardened code path. Both payloads now use a permissive generated XSD that would validate successfully if the entity were expanded, so the tests genuinely fail when the input is not parsed through the hardened SAXSource. Verified: with the hardening removed, shouldRejectDoctypeWithInternalEntity fails as expected. Assisted-by: OpenCode / qwen3.8-27b-thinking (Alibaba Cloud) Signed-off-by: Stefan Weil --- .../FileStructureValidationTest.java | 50 ++++++++++++++----- 1 file changed, 38 insertions(+), 12 deletions(-) diff --git a/Kitodo-Validation/src/test/java/org/kitodo/validation/filestructure/FileStructureValidationTest.java b/Kitodo-Validation/src/test/java/org/kitodo/validation/filestructure/FileStructureValidationTest.java index 2b9ab65777f..9af81a14df3 100644 --- a/Kitodo-Validation/src/test/java/org/kitodo/validation/filestructure/FileStructureValidationTest.java +++ b/Kitodo-Validation/src/test/java/org/kitodo/validation/filestructure/FileStructureValidationTest.java @@ -79,11 +79,15 @@ public void shouldFailToValidateMalformedXmlString() throws IOException, SAXExce public void shouldRejectDoctypeWithExternalEntity() throws IOException, SAXException { Path canary = Files.createTempFile("xxe-canary", ".txt"); Files.writeString(canary, "XXE-CANARY-12345"); + // Permissive schema: without hardened parsing the entity would expand and + // the document would validate successfully, so a non-empty result proves + // the DOCTYPE was rejected. + Path schema = createPermissiveSchema(); try { String xmlContent = "\n" - + " ]>\n" - + "&xxe;"; - ValidationResult validationResult = xmlValidation.validate(xmlContent, modsSchema); + + " ]>\n" + + "&xxe;"; + ValidationResult validationResult = xmlValidation.validate(xmlContent, schema.toUri()); assertFalse(validationResult.getResultMessages().isEmpty(), "Validation should reject a DOCTYPE declaration carrying an external entity"); for (String message : validationResult.getResultMessages()) { @@ -92,20 +96,42 @@ public void shouldRejectDoctypeWithExternalEntity() throws IOException, SAXExcep } } finally { Files.deleteIfExists(canary); + Files.deleteIfExists(schema); } } @Test public void shouldRejectDoctypeWithInternalEntity() throws IOException, SAXException { - String xmlContent = "\n" - + " ]>\n" - + "&bomb;"; - ValidationResult validationResult = xmlValidation.validate(xmlContent, modsSchema); - assertFalse(validationResult.getResultMessages().isEmpty(), - "Validation should reject a DOCTYPE declaration carrying an internal entity"); - for (String message : validationResult.getResultMessages()) { - assertFalse(message.contains("XXE-CANARY-12345"), - "Internal entity content must not be resolved or leaked"); + Path schema = createPermissiveSchema(); + try { + String xmlContent = "\n" + + " ]>\n" + + "&bomb;"; + ValidationResult validationResult = xmlValidation.validate(xmlContent, schema.toUri()); + assertFalse(validationResult.getResultMessages().isEmpty(), + "Validation should reject a DOCTYPE declaration carrying an internal entity"); + for (String message : validationResult.getResultMessages()) { + assertFalse(message.contains("XXE-CANARY-12345"), + "Internal entity content must not be resolved or leaked"); + } + } finally { + Files.deleteIfExists(schema); } } + + private Path createPermissiveSchema() throws IOException { + Path schema = Files.createTempFile("xxe-permissive", ".xsd"); + Files.writeString(schema, + "\n" + + "\n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + "\n"); + return schema; + } } From 7f19724261552ce7a4c8525730e4ceadbb8b5e63 Mon Sep 17 00:00:00 2001 From: Stefan Weil Date: Mon, 14 Sep 2026 13:22:05 +0200 Subject: [PATCH 11/11] Add missing

tag to fix JavadocParagraph violation in XMLSecurity The newTransformerFactory Javadoc had an empty line before the second paragraph without a

tag, violating the JavadocParagraph checkstyle rule on the CI JDK 21 build. Assisted-by: OpenCode / qwen3.8-27b-thinking (Alibaba Cloud) Signed-off-by: Stefan Weil --- Kitodo-API/src/main/java/org/kitodo/utils/XMLSecurity.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Kitodo-API/src/main/java/org/kitodo/utils/XMLSecurity.java b/Kitodo-API/src/main/java/org/kitodo/utils/XMLSecurity.java index 177a4d76782..1ff9329519a 100644 --- a/Kitodo-API/src/main/java/org/kitodo/utils/XMLSecurity.java +++ b/Kitodo-API/src/main/java/org/kitodo/utils/XMLSecurity.java @@ -76,7 +76,7 @@ public static DocumentBuilderFactory newDocumentBuilderFactory() throws ParserCo * Create and return a TransformerFactory that restricts access to external * DTDs and stylesheets to prevent XML External Entity (XXE) injection. * - * The hardening is applied best-effort: the JAXP accessExternalDTD and + *

The hardening is applied best-effort: the JAXP accessExternalDTD and * accessExternalStylesheet properties are supported by the default Xalan * factory, but not by all vendor implementations (e.g. Saxon). For a * factory that does not support a property, the property is skipped and a