Skip to content

Commit

Permalink
Document XML parser usage against security false positives
Browse files Browse the repository at this point in the history
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
  • Loading branch information
bclozel committed Oct 15, 2024
1 parent 166714c commit f204f49
Show file tree
Hide file tree
Showing 6 changed files with 20 additions and 2 deletions.
Original file line number Diff line number Diff line change
@@ -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.
Expand Down Expand Up @@ -88,6 +88,9 @@ public Document loadDocument(InputSource inputSource, EntityResolver entityResol
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);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,9 @@ public SpringPersistenceUnitInfo[] readPersistenceUnitInfos(String[] persistence
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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -603,6 +603,9 @@ private Schema loadSchema(Resource[] resources, String schemaLanguage) throws IO
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();
Expand Down Expand Up @@ -907,6 +910,8 @@ else if (streamSource.getReader() != null) {
}

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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,8 @@ protected Source processSource(Source source) {
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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,8 @@ else if (StreamSource.class == clazz || Source.class == clazz) {

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();
Expand Down
Original file line number Diff line number Diff line change
@@ -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.
Expand Down Expand Up @@ -215,6 +215,9 @@ protected TransformerFactory newTransformerFactory(
}
}
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();
}
}
Expand Down

0 comments on commit f204f49

Please sign in to comment.