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..585d7702993 --- /dev/null +++ b/Kitodo-API/src/main/java/org/kitodo/utils/XMLSecurity.java @@ -0,0 +1,242 @@ +/* + * (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 java.io.Reader; + +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.Schema; +import javax.xml.validation.SchemaFactory; +import javax.xml.validation.Validator; + +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; +import org.jdom2.input.SAXBuilder; +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. 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"; + + 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. + * + *

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 + */ + 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(name, value); + } catch (IllegalArgumentException 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); + } + } + + /** + * 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) { + throw new IllegalStateException( + "Unable to harden SchemaFactory " + factory.getClass().getName(), e); + } + 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 JDOM SAXBuilder that rejects DOCTYPE declarations and does not + * expand entities, to prevent XML External Entity (XXE) injection when parsing with + * the JDOM library. + * + * @return hardened SAXBuilder + */ + public static SAXBuilder newSaxBuilder() { + SAXBuilder builder = new SAXBuilder(); + builder.setExpandEntities(false); + builder.setFeature(DISALLOW_DOCTYPE_DECL, true); + return builder; + } + + /** + * 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 + * @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 { + return new SAXSource(newHardenedXmlReader(), new InputSource(inputStream)); + } + + /** + * 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 character stream. In contrast to the + * {@link #newSecureSource(InputStream)} variant, the XML declaration of the input + * is ignored, so character data does not have to be re-encoded before parsing. + * + * @param reader character 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(Reader reader) throws ParserConfigurationException, SAXException { + return new SAXSource(newHardenedXmlReader(), new InputSource(reader)); + } + + private static XMLReader newHardenedXmlReader() 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); + factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); + return factory.newSAXParser().getXMLReader(); + } + + /** + * 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 + * @throws IllegalStateException if the Validator cannot be created + */ + public static Validator newSecureValidator(Schema schema) { + Validator validator = schema.newValidator(); + try { + validator.setProperty(XMLConstants.ACCESS_EXTERNAL_DTD, ""); + validator.setProperty(XMLConstants.ACCESS_EXTERNAL_SCHEMA, "file"); + } catch (IllegalArgumentException | SAXNotRecognizedException | SAXNotSupportedException e) { + 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 new file mode 100644 index 00000000000..18abf6a9a61 --- /dev/null +++ b/Kitodo-API/src/test/java/org/kitodo/utils/XMLSecurityTest.java @@ -0,0 +1,243 @@ +/* + * (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.sax.SAXSource; +import javax.xml.transform.stream.StreamSource; +import javax.xml.validation.Schema; +import javax.xml.validation.Validator; + +import org.jdom2.JDOMException; +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()); + assertNotNull(XMLSecurity.newSaxBuilder()); + } + + @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 saxBuilderShouldNotResolveExternalEntities() throws Exception { + File secret = createTestFile(); + String payload = xxePayload(secret); + JDOMException exception = assertThrows(JDOMException.class, + () -> XMLSecurity.newSaxBuilder().build(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(); + // 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(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 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(secureSource)); + 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(); + builder.parse(new InputSource(new ByteArrayInputStream(xml.getBytes(StandardCharsets.UTF_8)))); + } + + private void readWithStax(XMLInputFactory factory, String xml) throws XMLStreamException { + XMLStreamReader reader = factory.createXMLStreamReader(new StringReader(xml)); + while (reader.hasNext()) { + reader.next(); + } + } + + 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" + + ""; + + private static final String DEP_XSD = + "\n" + + "\n" + + " \n" + + ""; + + private static final String MAIN_XSD = + "\n" + + "\n" + + " \n" + + " \n" + + ""; +} 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-Docket/src/main/java/org/kitodo/docket/ExportXmlLog.java b/Kitodo-Docket/src/main/java/org/kitodo/docket/ExportXmlLog.java index 2a401000bd6..a4d9f643b1e 100644 --- a/Kitodo-Docket/src/main/java/org/kitodo/docket/ExportXmlLog.java +++ b/Kitodo-Docket/src/main/java/org/kitodo/docket/ExportXmlLog.java @@ -48,6 +48,7 @@ import org.kitodo.api.docket.DocketData; import org.kitodo.api.docket.Property; import org.kitodo.config.KitodoConfig; +import org.kitodo.utils.XMLSecurity; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -413,8 +414,9 @@ private void prepareMetadataElements(List metadataElements, boolean use Namespace[] namespaces, Namespace xmlns) throws IOException, JDOMException { HashMap fields = getMetsFieldsFromConfig(useAnchor); + SAXBuilder builder = XMLSecurity.newSaxBuilder(); try (InputStream in = docketData.metadataFile().toURL().openStream()) { - Document metsDoc = new SAXBuilder().build(in); + Document metsDoc = builder.build(in); prepareMetadataElements(metadataElements, fields, metsDoc, namespaces, xmlns); } } diff --git a/Kitodo-Docket/src/test/java/org/kitodo/docket/ExportXmlLogTest.java b/Kitodo-Docket/src/test/java/org/kitodo/docket/ExportXmlLogTest.java index 696a08cd80f..3c62bda31ab 100644 --- a/Kitodo-Docket/src/test/java/org/kitodo/docket/ExportXmlLogTest.java +++ b/Kitodo-Docket/src/test/java/org/kitodo/docket/ExportXmlLogTest.java @@ -11,17 +11,27 @@ package org.kitodo.docket; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; import java.io.ByteArrayOutputStream; import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; import java.nio.file.Paths; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; import org.kitodo.api.docket.DocketData; public class ExportXmlLogTest extends ExportXmlLog { + private static final String CANARY = "XXE-CANARY-SECRET"; + + @TempDir + Path tempDir; + public ExportXmlLogTest() { super(getDocketData()); } @@ -39,7 +49,29 @@ static DocketData getDocketData() { public void shouldExportXmlLogWithMetadata() throws IOException { try (ByteArrayOutputStream buffer = new ByteArrayOutputStream()) { super.startExport(buffer); - assertTrue(buffer.toString().contains("findMeInOutput"), "Output should contain test string"); + assertTrue(buffer.toString(StandardCharsets.UTF_8).contains("findMeInOutput"), "Output should contain test string"); + } + } + + @Test + public void shouldNotResolveExternalEntitiesInMetadataFile() throws IOException { + Path secret = Files.createTempFile(tempDir, "xxe-canary", ".txt"); + Files.writeString(secret, CANARY); + + String payload = "\n" + + " ]>\n" + + "" + + "&xxe;" + + ""; + Path metsFile = Files.createTempFile(tempDir, "xxe-mets", ".xml"); + Files.writeString(metsFile, payload); + + DocketData data = new DocketData(); + data.setMetadataFile(metsFile.toUri()); + ExportXmlLog exportXmlLog = new ExportXmlLog(data); + try (ByteArrayOutputStream buffer = new ByteArrayOutputStream()) { + exportXmlLog.startExport(buffer); + assertFalse(buffer.toString(StandardCharsets.UTF_8).contains(CANARY), "secret must not leak into the output"); } } } 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..921cca82b96 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 @@ -12,7 +12,10 @@ package org.kitodo.validation.filestructure; import java.io.File; +import java.io.FileInputStream; import java.io.IOException; +import java.io.InputStream; +import java.io.Reader; import java.io.StringReader; import java.net.URI; import java.util.Collection; @@ -20,8 +23,9 @@ import java.util.List; import java.util.stream.Collectors; -import javax.xml.XMLConstants; +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; @@ -32,6 +36,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; @@ -47,23 +52,41 @@ 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 StringReader(xmlContent)), 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 StringReader(xmlContent)), 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 from InputStream", e); + } + } + + private SAXSource createSecureSource(Reader xmlInput) throws SAXException { + try { + return XMLSecurity.newSecureSource(xmlInput); + } catch (ParserConfigurationException e) { + throw new SAXException("Unable to create hardened SAXSource from Reader", 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); @@ -87,13 +110,13 @@ 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(); + Validator xmlValidator = XMLSecurity.newSecureValidator(schema); xmlValidator.setErrorHandler(xmlValidationErrorHandler); return xmlValidator; } 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..35fbf39749a 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 @@ -13,6 +13,7 @@ package org.kitodo.validation.filestructure; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; import org.kitodo.api.validation.ValidationResult; import org.xml.sax.SAXException; @@ -36,6 +37,9 @@ public class FileStructureValidationTest { private static final URI modsSchema = Paths.get(repositoryRoot + MODS_3_4_XSD).toUri(); private final FileStructureValidation xmlValidation = new FileStructureValidation(); + @TempDir + Path tempDir; + @Test public void shouldSucceedToValidateValidXmlFile() throws SAXException, IOException { ValidationResult validationResult = xmlValidation.validate(Paths.get(VALID_MODS_FILE).toUri(), modsSchema); @@ -72,7 +76,67 @@ 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(tempDir, "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(); + String xmlContent = "\n" + + " ]>\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()) { + assertFalse(message.contains("XXE-CANARY-12345"), + "External entity content must not be resolved or leaked"); + } } + @Test + public void shouldRejectDoctypeWithInternalEntity() throws IOException, SAXException { + Path schema = createPermissiveSchema(); + 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"); + } + } + + @Test + public void shouldValidateXmlStringWithNonUtf8EncodingDeclaration() throws IOException, SAXException { + Path schema = createPermissiveSchema(); + String xmlContent = "\n" + + "café"; + ValidationResult validationResult = xmlValidation.validate(xmlContent, schema.toUri()); + assertTrue(validationResult.getResultMessages().isEmpty(), + "Character data must be preserved and must not be re-encoded before parsing"); + } + + private Path createPermissiveSchema() throws IOException { + Path schema = Files.createTempFile(tempDir, "xxe-permissive", ".xsd"); + Files.writeString(schema, + "\n" + + "\n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + "\n"); + return schema; + } } diff --git a/Kitodo-XML-SchemaConverter/src/main/java/org/kitodo/xmlschemaconverter/XMLSchemaConverter.java b/Kitodo-XML-SchemaConverter/src/main/java/org/kitodo/xmlschemaconverter/XMLSchemaConverter.java index ecc382bed82..85dd2c61c68 100644 --- a/Kitodo-XML-SchemaConverter/src/main/java/org/kitodo/xmlschemaconverter/XMLSchemaConverter.java +++ b/Kitodo-XML-SchemaConverter/src/main/java/org/kitodo/xmlschemaconverter/XMLSchemaConverter.java @@ -21,6 +21,7 @@ import java.util.UnknownFormatConversionException; import javax.xml.XMLConstants; +import javax.xml.parsers.ParserConfigurationException; import javax.xml.transform.Transformer; import javax.xml.transform.TransformerException; import javax.xml.transform.TransformerFactory; @@ -35,6 +36,8 @@ import org.kitodo.api.schemaconverter.MetadataFormat; import org.kitodo.api.schemaconverter.SchemaConverterInterface; import org.kitodo.exceptions.ConfigException; +import org.kitodo.utils.XMLSecurity; +import org.xml.sax.SAXException; public class XMLSchemaConverter implements SchemaConverterInterface { private static final FileFormat supportedSourceFileFormat = FileFormat.XML; @@ -100,9 +103,13 @@ private String transformXmlByXslt(String xmlString, File stylesheetFile) { System.setProperty("http.agent", "Chrome"); Transformer transformer = transformerFactory.newTransformer(xsltSource); xmlString = removeBom(xmlString); - transformer.transform(new StreamSource(new StringReader(xmlString)), new StreamResult(writer)); + // The DataRecord may come from an external catalog and is therefore + // untrusted: parse it through a hardened source that rejects DOCTYPE + // declarations and external entities. The stylesheet side intentionally + // keeps its network resolver, because the mapping may fetch remote records. + transformer.transform(XMLSecurity.newSecureSource(new StringReader(xmlString)), new StreamResult(writer)); return writer.toString(); - } catch (TransformerException | IOException e) { + } catch (TransformerException | IOException | ParserConfigurationException | SAXException e) { throw new ConfigException("Error in transforming the response to internal format: " + e.getMessage(), e); } } diff --git a/Kitodo-XML-SchemaConverter/src/test/java/org/kitodo/xmlschemaconverter/XmlSchemaConverterTest.java b/Kitodo-XML-SchemaConverter/src/test/java/org/kitodo/xmlschemaconverter/XmlSchemaConverterTest.java index 337b3253895..c1a0535c7ad 100644 --- a/Kitodo-XML-SchemaConverter/src/test/java/org/kitodo/xmlschemaconverter/XmlSchemaConverterTest.java +++ b/Kitodo-XML-SchemaConverter/src/test/java/org/kitodo/xmlschemaconverter/XmlSchemaConverterTest.java @@ -14,7 +14,9 @@ import static org.hamcrest.CoreMatchers.instanceOf; import static org.hamcrest.MatcherAssert.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; +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; @@ -25,6 +27,7 @@ import java.nio.charset.Charset; import java.nio.charset.StandardCharsets; import java.nio.file.Files; +import java.nio.file.Path; import java.nio.file.Paths; import java.util.LinkedList; import java.util.List; @@ -36,11 +39,13 @@ import org.apache.commons.io.FileUtils; import org.apache.commons.io.IOUtils; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; import org.kitodo.api.schemaconverter.DataRecord; import org.kitodo.api.schemaconverter.FileFormat; import org.kitodo.api.schemaconverter.MetadataFormat; import org.kitodo.api.schemaconverter.MetadataFormatConversion; import org.kitodo.config.KitodoConfig; +import org.kitodo.exceptions.ConfigException; import org.w3c.dom.Document; import org.w3c.dom.Element; import org.w3c.dom.NodeList; @@ -53,6 +58,9 @@ public class XmlSchemaConverterTest { private static final String MODS_TEST_FILE_PATH = "src/test/resources/modsXmlTestRecord.xml"; private static final String MARC_TEST_FILE_PATH = "src/test/resources/marcXmlTestRecord.xml"; + @TempDir + Path tempDir; + @Test public void shouldConvertModsToInternalFormat() throws IOException, ParserConfigurationException, SAXException, URISyntaxException { @@ -154,6 +162,26 @@ public void shouldConvertMarcToInternalFormat() throws IOException, ParserConfig assertEquals("Test-Shelflocator", shelfmarksource, "shelfmarksource after conversion is wrong!"); } + @Test + public void shouldRejectExternalEntitiesInSourceRecord() throws IOException { + String canary = "XXE-CANARY-12345"; + File secret = Files.createTempFile(tempDir, "xxe-canary", ".txt").toFile(); + Files.writeString(secret.toPath(), canary); + + DataRecord testRecord = new DataRecord(); + testRecord.setMetadataFormat(MetadataFormat.MODS); + testRecord.setFileFormat(FileFormat.XML); + testRecord.setOriginalData("\n" + + " ]>\n" + + "&xxe;"); + + ConfigException exception = assertThrows(ConfigException.class, + () -> converter.convert(testRecord, MetadataFormat.KITODO, FileFormat.XML, + List.of(new File("src/test/resources/identity.xsl")))); + assertFalse(String.valueOf(exception.getMessage()).contains(canary), + "External entity content must not be resolved or leaked"); + } + private Document parseInputStreamToDocument(String inputString) throws ParserConfigurationException, IOException, SAXException { try (InputStream inputStream = new ByteArrayInputStream(inputString.getBytes(StandardCharsets.UTF_8))) { diff --git a/Kitodo-XML-SchemaConverter/src/test/resources/identity.xsl b/Kitodo-XML-SchemaConverter/src/test/resources/identity.xsl new file mode 100644 index 00000000000..deb752f5177 --- /dev/null +++ b/Kitodo-XML-SchemaConverter/src/test/resources/identity.xsl @@ -0,0 +1,20 @@ + + + + + + + + + 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/export/XsltHelper.java b/Kitodo/src/main/java/org/kitodo/export/XsltHelper.java index 32f4b978e48..6b852898943 100644 --- a/Kitodo/src/main/java/org/kitodo/export/XsltHelper.java +++ b/Kitodo/src/main/java/org/kitodo/export/XsltHelper.java @@ -12,53 +12,98 @@ package org.kitodo.export; import java.io.ByteArrayOutputStream; +import java.io.FileInputStream; import java.io.IOException; +import java.io.InputStream; import java.net.URI; import java.nio.file.Paths; import java.util.Objects; +import javax.xml.parsers.ParserConfigurationException; import javax.xml.transform.Transformer; import javax.xml.transform.TransformerException; import javax.xml.transform.TransformerFactory; +import javax.xml.transform.sax.SAXSource; import javax.xml.transform.stream.StreamResult; import javax.xml.transform.stream.StreamSource; -import net.sf.saxon.TransformerFactoryImpl; - import org.apache.commons.io.FilenameUtils; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; import org.kitodo.config.ConfigCore; import org.kitodo.config.enums.ParameterCore; import org.kitodo.data.database.beans.Process; +import org.kitodo.utils.XMLSecurity; +import org.xml.sax.SAXException; public class XsltHelper { + private static final Logger logger = LogManager.getLogger(XsltHelper.class); + private XsltHelper() { // private constructor to hide implicit one } /** - * Transforms a xml file by xslt and returns the result as string. + * Transforms an xml file by xslt and returns the result as string. The input + * document is parsed with a hardened SAX parser that rejects DOCTYPE + * declarations and external entity resolution, so the Saxon transformer + * never sees untrusted XML features. * * @param source * The xml file to transform. * @param xslFile * The xsl file. * @return The Result of the transformation as String object. + * @throws IOException if the input cannot be read */ static ByteArrayOutputStream transformXmlByXslt(StreamSource source, URI xslFile) throws TransformerException, IOException { String xsltPath = xslFile.getPath(); StreamSource xsltSource = new StreamSource(xsltPath); - TransformerFactory factory = new TransformerFactoryImpl(); + TransformerFactory factory = XMLSecurity.newTransformerFactory(); Transformer transformer = factory.newTransformer(xsltSource); if (Objects.isNull(transformer)) { throw new IllegalArgumentException("Could not create XSLT transformer. Check " + xsltPath + " for errors."); } + SAXSource secureSource; + InputStream streamToClose = null; + try { + if (Objects.nonNull(source.getReader())) { + secureSource = XMLSecurity.newSecureSource(source.getReader()); + } else if (Objects.nonNull(source.getInputStream())) { + streamToClose = source.getInputStream(); + secureSource = XMLSecurity.newSecureSource(streamToClose); + } else if (Objects.nonNull(source.getSystemId())) { + String systemId = source.getSystemId(); + streamToClose = new FileInputStream(systemId.startsWith("file:") + ? Paths.get(URI.create(systemId)).toFile() + : Paths.get(systemId).toFile()); + secureSource = XMLSecurity.newSecureSource(streamToClose); + } else { + throw new IllegalArgumentException("StreamSource has neither an input stream, a reader, nor a system ID"); + } + } catch (ParserConfigurationException | SAXException e) { + closeQuietly(streamToClose); + throw new IllegalStateException("Unable to create hardened SAX source", e); + } try (ByteArrayOutputStream outputStream = new ByteArrayOutputStream()) { StreamResult streamResult = new StreamResult(outputStream); - transformer.transform(source, streamResult); + transformer.transform(secureSource, streamResult); return outputStream; + } finally { + closeQuietly(streamToClose); + } + } + + private static void closeQuietly(InputStream stream) { + if (Objects.nonNull(stream)) { + try { + stream.close(); + } catch (IOException e) { + logger.debug("Ignoring error while closing XML input stream", e); + } } } 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; diff --git a/Kitodo/src/test/java/org/kitodo/export/XsltHelperTest.java b/Kitodo/src/test/java/org/kitodo/export/XsltHelperTest.java index ea52d525c86..03bb4d5c5eb 100644 --- a/Kitodo/src/test/java/org/kitodo/export/XsltHelperTest.java +++ b/Kitodo/src/test/java/org/kitodo/export/XsltHelperTest.java @@ -12,21 +12,33 @@ package org.kitodo.export; import static org.hamcrest.MatcherAssert.assertThat; +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.ByteArrayOutputStream; import java.io.File; +import java.io.StringReader; import java.net.URI; import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import javax.xml.transform.TransformerException; import javax.xml.transform.stream.StreamSource; import org.apache.commons.io.FileUtils; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; import org.xmlunit.matchers.CompareMatcher; public class XsltHelperTest { private static final String META_XML = "testMetadataFileServiceTest.xml"; + private static final String CANARY = "XXE-CANARY-SECRET"; + + @TempDir + Path tempDir; @Test public void shouldTransformKitodoToMods() throws Exception { @@ -43,4 +55,48 @@ public void shouldTransformKitodoToMods() throws Exception { FileUtils.deleteQuietly(result); } + + @Test + public void shouldTransformFileBackedStreamSource() throws Exception { + Path input = Files.createTempFile(tempDir, "file-backed", ".xml"); + input.toFile().deleteOnExit(); + String content = "fileContent"; + Files.writeString(input, content); + + ByteArrayOutputStream outputStream = XsltHelper.transformXmlByXslt( + new StreamSource(input.toFile()), URI.create("src/test/resources/xslt/identity.xsl")); + + String result = outputStream.toString(StandardCharsets.UTF_8); + assertTrue(result.contains("fileContent"), "Transformation of a file-backed StreamSource should succeed"); + assertTrue(result.contains(""), "Result should contain the transformed document"); + } + + @Test + public void shouldTransformReaderBackedStreamSource() throws Exception { + String content = "readerContent"; + + ByteArrayOutputStream outputStream = XsltHelper.transformXmlByXslt( + new StreamSource(new StringReader(content)), URI.create("src/test/resources/xslt/identity.xsl")); + + String result = outputStream.toString(StandardCharsets.UTF_8); + assertTrue(result.contains("readerContent"), "Transformation of a reader-backed StreamSource should succeed"); + } + + @Test + public void shouldNotResolveExternalEntities() throws Exception { + Path secret = Files.createTempFile(tempDir, "xxe-canary", ".txt"); + secret.toFile().deleteOnExit(); + Files.writeString(secret, CANARY); + + String payload = "\n" + + " ]>\n" + + "&xxe;"; + + TransformerException exception = assertThrows(TransformerException.class, + () -> XsltHelper.transformXmlByXslt( + new StreamSource(new java.io.ByteArrayInputStream(payload.getBytes(StandardCharsets.UTF_8))), + URI.create("src/test/resources/xslt/identity.xsl"))); + + assertFalse(String.valueOf(exception.getMessage()).contains(CANARY), "secret must not leak into the error"); + } } diff --git a/Kitodo/src/test/resources/xslt/identity.xsl b/Kitodo/src/test/resources/xslt/identity.xsl new file mode 100644 index 00000000000..deb752f5177 --- /dev/null +++ b/Kitodo/src/test/resources/xslt/identity.xsl @@ -0,0 +1,20 @@ + + + + + + + + +