From f204f4962d611290a1db4b9b76da93e88ca3759b Mon Sep 17 00:00:00 2001 From: Brian Clozel Date: Tue, 15 Oct 2024 18:59:02 +0200 Subject: [PATCH] Document XML parser usage against security false positives Prior to this commit, our XML parser usage would be already haredened against XXE (XML External Entities) attacks. Still, we recently received several invalid security reports claiming that our setup should be hardened. This commit documents a few usages of XML parsers to add some more context and hopefully prevent future invalid reports. Closes gh-33713 --- .../beans/factory/xml/DefaultDocumentLoader.java | 5 ++++- .../orm/jpa/persistenceunit/PersistenceUnitReader.java | 3 +++ .../java/org/springframework/oxm/jaxb/Jaxb2Marshaller.java | 5 +++++ .../converter/xml/Jaxb2RootElementHttpMessageConverter.java | 2 ++ .../http/converter/xml/SourceHttpMessageConverter.java | 2 ++ .../org/springframework/web/servlet/view/xslt/XsltView.java | 5 ++++- 6 files changed, 20 insertions(+), 2 deletions(-) diff --git a/spring-beans/src/main/java/org/springframework/beans/factory/xml/DefaultDocumentLoader.java b/spring-beans/src/main/java/org/springframework/beans/factory/xml/DefaultDocumentLoader.java index 08b1d16f277..77ec6469f13 100644 --- a/spring-beans/src/main/java/org/springframework/beans/factory/xml/DefaultDocumentLoader.java +++ b/spring-beans/src/main/java/org/springframework/beans/factory/xml/DefaultDocumentLoader.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2018 the original author or authors. + * Copyright 2002-2024 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -88,6 +88,9 @@ public class DefaultDocumentLoader implements DocumentLoader { protected DocumentBuilderFactory createDocumentBuilderFactory(int validationMode, boolean namespaceAware) throws ParserConfigurationException { + // This document loader is used for loading application configuration files. + // As a result, attackers would need complete write access to application configuration + // to leverage XXE attacks. This does not qualify as privilege escalation. DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); factory.setNamespaceAware(namespaceAware); diff --git a/spring-orm/src/main/java/org/springframework/orm/jpa/persistenceunit/PersistenceUnitReader.java b/spring-orm/src/main/java/org/springframework/orm/jpa/persistenceunit/PersistenceUnitReader.java index ddd74f19233..85fd8d7823f 100644 --- a/spring-orm/src/main/java/org/springframework/orm/jpa/persistenceunit/PersistenceUnitReader.java +++ b/spring-orm/src/main/java/org/springframework/orm/jpa/persistenceunit/PersistenceUnitReader.java @@ -157,6 +157,9 @@ final class PersistenceUnitReader { Document buildDocument(ErrorHandler handler, InputStream stream) throws ParserConfigurationException, SAXException, IOException { + // This document loader is used for loading application configuration files. + // As a result, attackers would need complete write access to application configuration + // to leverage XXE attacks. This does not qualify as privilege escalation. DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance(); dbf.setNamespaceAware(true); DocumentBuilder parser = dbf.newDocumentBuilder(); diff --git a/spring-oxm/src/main/java/org/springframework/oxm/jaxb/Jaxb2Marshaller.java b/spring-oxm/src/main/java/org/springframework/oxm/jaxb/Jaxb2Marshaller.java index 2d903c092cd..efddba00456 100644 --- a/spring-oxm/src/main/java/org/springframework/oxm/jaxb/Jaxb2Marshaller.java +++ b/spring-oxm/src/main/java/org/springframework/oxm/jaxb/Jaxb2Marshaller.java @@ -603,6 +603,9 @@ public class Jaxb2Marshaller implements MimeMarshaller, MimeUnmarshaller, Generi Assert.hasLength(schemaLanguage, "No schema language provided"); Source[] schemaSources = new Source[resources.length]; + // This parser is used to read the schema resources provided by the application. + // The parser used for reading the source is protected against XXE attacks. + // See "processSource(Source source)". SAXParserFactory saxParserFactory = this.schemaParserFactory; if (saxParserFactory == null) { saxParserFactory = SAXParserFactory.newInstance(); @@ -907,6 +910,8 @@ public class Jaxb2Marshaller implements MimeMarshaller, MimeUnmarshaller, Generi } try { + // By default, Spring will prevent the processing of external entities. + // This is a mitigation against XXE attacks. if (xmlReader == null) { SAXParserFactory saxParserFactory = this.sourceParserFactory; if (saxParserFactory == null) { diff --git a/spring-web/src/main/java/org/springframework/http/converter/xml/Jaxb2RootElementHttpMessageConverter.java b/spring-web/src/main/java/org/springframework/http/converter/xml/Jaxb2RootElementHttpMessageConverter.java index ee11dec759b..8cc89318184 100644 --- a/spring-web/src/main/java/org/springframework/http/converter/xml/Jaxb2RootElementHttpMessageConverter.java +++ b/spring-web/src/main/java/org/springframework/http/converter/xml/Jaxb2RootElementHttpMessageConverter.java @@ -164,6 +164,8 @@ public class Jaxb2RootElementHttpMessageConverter extends AbstractJaxb2HttpMessa if (source instanceof StreamSource streamSource) { InputSource inputSource = new InputSource(streamSource.getInputStream()); try { + // By default, Spring will prevent the processing of external entities. + // This is a mitigation against XXE attacks. SAXParserFactory saxParserFactory = this.sourceParserFactory; if (saxParserFactory == null) { saxParserFactory = SAXParserFactory.newInstance(); diff --git a/spring-web/src/main/java/org/springframework/http/converter/xml/SourceHttpMessageConverter.java b/spring-web/src/main/java/org/springframework/http/converter/xml/SourceHttpMessageConverter.java index c99d0bb033b..1483ea0b4c6 100644 --- a/spring-web/src/main/java/org/springframework/http/converter/xml/SourceHttpMessageConverter.java +++ b/spring-web/src/main/java/org/springframework/http/converter/xml/SourceHttpMessageConverter.java @@ -177,6 +177,8 @@ public class SourceHttpMessageConverter extends AbstractHttpMe private DOMSource readDOMSource(InputStream body, HttpInputMessage inputMessage) throws IOException { try { + // By default, Spring will prevent the processing of external entities. + // This is a mitigation against XXE attacks. DocumentBuilderFactory builderFactory = this.documentBuilderFactory; if (builderFactory == null) { builderFactory = DocumentBuilderFactory.newInstance(); diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/view/xslt/XsltView.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/view/xslt/XsltView.java index d889ad7873c..b18c4872225 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/view/xslt/XsltView.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/view/xslt/XsltView.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2021 the original author or authors. + * Copyright 2002-2024 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -215,6 +215,9 @@ public class XsltView extends AbstractUrlBasedView { } } else { + // This transformer is used for local XSLT views only. + // As a result, attackers would need complete write access to application configuration + // to leverage XXE attacks. This does not qualify as privilege escalation. return TransformerFactory.newInstance(); } }