Skip to content
Merged
Original file line number Diff line number Diff line change
Expand Up @@ -102,3 +102,29 @@ java.util.Collections#shuffle(java.util.List) @ Use java.util.Collections#shuffl

@defaultMessage Avoid creating FilePermission objects directly, but use FilePermissionUtils instead
java.io.FilePermission#<init>(java.lang.String,java.lang.String)

@defaultMessage DocumentBuilderFactory should not be used directly. Use XmlUtils#getHardenedDocumentBuilder(java.lang.String[]) instead.
javax.xml.parsers.DocumentBuilderFactory#newInstance()
javax.xml.parsers.DocumentBuilderFactory#newInstance(java.lang.String, java.lang.ClassLoader)
javax.xml.parsers.DocumentBuilderFactory#newDefaultNSInstance()
javax.xml.parsers.DocumentBuilderFactory#newNSInstance()
javax.xml.parsers.DocumentBuilderFactory#newNSInstance(java.lang.String, java.lang.ClassLoader)
Comment on lines +109 to +111
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming we want to block all of these as well


@defaultMessage TransformerFactory should not be used directly. Use XmlUtils#getHardenedXMLTransformer() instead.
javax.xml.transform.TransformerFactory#newInstance()
javax.xml.transform.TransformerFactory#newInstance(java.lang.String, java.lang.ClassLoader)

@defaultMessage SAXParserFactory should not be used directly. Use XmlUtils#getHardenedSaxParser() instead
javax.xml.parsers.SAXParserFactory#newInstance()
javax.xml.parsers.SAXParserFactory#newInstance(java.lang.String, java.lang.ClassLoader)
javax.xml.parsers.SAXParserFactory#newDefaultNSInstance()
javax.xml.parsers.SAXParserFactory#newNSInstance()
javax.xml.parsers.SAXParserFactory#newNSInstance(java.lang.String, java.lang.ClassLoader)

@defaultMessage SchemaValidator should not be used directly. Use XmlUtils#getHardenedSchemaValidator() instead
javax.xml.validation.SchemaFactory#newDefaultInstance()
javax.xml.validation.SchemaFactory#newInstance(java.lang.String)
javax.xml.validation.SchemaFactory#newInstance(java.lang.String, java.lang.String, java.lang.ClassLoader)

@defaultMessage Validator should not be used directly. Use XmlUtils#getHardenedValidator() instead
javax.xml.validation.Schema#newValidator()
1 change: 1 addition & 0 deletions libs/core/src/main/java/module-info.java
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
module org.elasticsearch.base {
requires static jsr305;
requires org.elasticsearch.logging;
requires java.xml;

exports org.elasticsearch.core;
exports org.elasticsearch.jdk;
Expand Down
186 changes: 186 additions & 0 deletions libs/core/src/main/java/org/elasticsearch/core/XmlUtils.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,186 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the "Elastic License
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
* Public License v 1"; you may not use this file except in compliance with, at
* your election, the "Elastic License 2.0", the "GNU Affero General Public
* License v3.0 only", or the "Server Side Public License, v 1".
*/

package org.elasticsearch.core;

import org.elasticsearch.logging.LogManager;
import org.elasticsearch.logging.Logger;
import org.xml.sax.SAXException;
import org.xml.sax.SAXNotRecognizedException;
import org.xml.sax.SAXNotSupportedException;
import org.xml.sax.SAXParseException;

import javax.xml.XMLConstants;
import javax.xml.parsers.DocumentBuilder;
import javax.xml.parsers.DocumentBuilderFactory;
import javax.xml.parsers.ParserConfigurationException;
import javax.xml.parsers.SAXParserFactory;
import javax.xml.transform.Transformer;
import javax.xml.transform.TransformerConfigurationException;
import javax.xml.transform.TransformerException;
import javax.xml.transform.TransformerFactory;
import javax.xml.validation.Schema;
import javax.xml.validation.SchemaFactory;
import javax.xml.validation.Validator;

public class XmlUtils {

private static final Logger LOGGER = LogManager.getLogger(XmlUtils.class);

/**
* Constructs a DocumentBuilder with all the necessary features for it to be secure
*
* @throws ParserConfigurationException if one of the features can't be set on the DocumentBuilderFactory
*/
public static DocumentBuilder getHardenedBuilder(String[] schemaFiles) throws ParserConfigurationException {
final DocumentBuilderFactory dbf = getHardenedBuilderFactory();
dbf.setNamespaceAware(true);
// Ensure that Schema Validation is enabled for the factory
dbf.setValidating(true);
// This is required, otherwise schema validation causes signature invalidation
dbf.setFeature("http://apache.org/xml/features/validation/schema/normalized-value", false);
// Make sure that URL schema namespaces are not resolved/downloaded from URLs we do not control
dbf.setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, "file,jar");
dbf.setAttribute(XMLConstants.ACCESS_EXTERNAL_SCHEMA, "file,jar");
// We ship our own xsd files for schema validation since we do not trust anyone else.
dbf.setAttribute("http://java.sun.com/xml/jaxp/properties/schemaSource", schemaFiles);
DocumentBuilder documentBuilder = dbf.newDocumentBuilder();
documentBuilder.setErrorHandler(new ErrorHandler());
return documentBuilder;
}

/**
* Returns a DocumentBuilderFactory pre-configured to be secure
*/
@SuppressForbidden(reason = "This is the only allowed way to construct a DocumentBuilder")
public static DocumentBuilderFactory getHardenedBuilderFactory() throws ParserConfigurationException {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to provide a DocumentBuilder that doesn't validate schema as some existing usages of this API don't perform schema validation

final DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance();
// Disallow internal and external entity expansion
dbf.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
dbf.setFeature("http://xml.org/sax/features/external-general-entities", false);
dbf.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
dbf.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
dbf.setFeature("http://xml.org/sax/features/validation", true);
dbf.setFeature("http://apache.org/xml/features/nonvalidating/load-dtd-grammar", false);
dbf.setIgnoringComments(true);
dbf.setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, "");
dbf.setAttribute(XMLConstants.ACCESS_EXTERNAL_SCHEMA, "");
dbf.setFeature("http://apache.org/xml/features/honour-all-schemaLocations", true);
// Ensure we do not resolve XIncludes. Defaults to false, but set it explicitly to be future-proof
dbf.setXIncludeAware(false);
// Ensure we do not expand entity reference nodes
dbf.setExpandEntityReferences(false);
// Further limit danger from denial of service attacks
dbf.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
dbf.setAttribute("http://apache.org/xml/features/validation/schema", true);
dbf.setAttribute("http://apache.org/xml/features/validation/schema-full-checking", true);
dbf.setAttribute("http://java.sun.com/xml/jaxp/properties/schemaLanguage", XMLConstants.W3C_XML_SCHEMA_NS_URI);

return dbf;
}

/**
* Constructs a Transformer configured to be secure
*/
@SuppressForbidden(reason = "This is the only allowed way to construct a Transformer")
public static Transformer getHardenedXMLTransformer() throws TransformerConfigurationException {
final TransformerFactory tfactory = TransformerFactory.newInstance();
tfactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
tfactory.setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, "");
tfactory.setAttribute(XMLConstants.ACCESS_EXTERNAL_STYLESHEET, "");
Comment on lines +94 to +96
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I understand, we don't actually need to configure ACCESS_EXTERNAL_DTD or ACCESS_EXTERNAL_STYLESHEET when FEATURE_SECURE_PROCESSING is true; as per JAXP Properties for External Access Restrictions:

Explicitly turning on Feature for Secure Processing (FSP) through the API, for example, factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true), disables all external connections.

Copy link
Contributor

@slobodanadamovic slobodanadamovic Aug 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. This is probably for historical reasons and old JDKs. If no harm, I think we should keep it.

tfactory.setAttribute("indent-number", 2);
Transformer transformer = tfactory.newTransformer();
transformer.setErrorListener(new ErrorListener());
return transformer;
}

/**
* Returns a SchemaFactory configured to be secure
*/
@SuppressForbidden(reason = "This is the only allowed way to construct a SchemaFactory")
public static SchemaFactory getHardenedSchemaFactory() throws SAXNotSupportedException, SAXNotRecognizedException {
SchemaFactory schemaFactory = SchemaFactory.newInstance(XMLConstants.W3C_XML_SCHEMA_NS_URI);
schemaFactory.setProperty(XMLConstants.ACCESS_EXTERNAL_DTD, "");
schemaFactory.setProperty(XMLConstants.ACCESS_EXTERNAL_SCHEMA, "");
return schemaFactory;
}

/**
* Constructs a Validator configured to be secure
*/
@SuppressForbidden(reason = "This is the only allowed way to construct a Validator")
public static Validator getHardenedValidator(Schema schema) throws SAXNotSupportedException, SAXNotRecognizedException {
Validator validator = schema.newValidator();
validator.setProperty(XMLConstants.ACCESS_EXTERNAL_DTD, "");
validator.setProperty(XMLConstants.ACCESS_EXTERNAL_SCHEMA, "");
return validator;
}

@SuppressForbidden(reason = "This is the only allowed way to construct a SAXParser")
public static SAXParserFactory getHardenedSaxParserFactory() throws SAXNotSupportedException, SAXNotRecognizedException,
ParserConfigurationException {
var saxParserFactory = SAXParserFactory.newInstance();

saxParserFactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I understand, this is the only security feature we actually need to set

saxParserFactory.setFeature("http://xml.org/sax/features/external-general-entities", false);
saxParserFactory.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
saxParserFactory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
saxParserFactory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);

return saxParserFactory;
}

private static class ErrorHandler implements org.xml.sax.ErrorHandler {
/**
* Enabling schema validation with `setValidating(true)` in our
* DocumentBuilderFactory requires that we provide our own
* ErrorHandler implementation
*
* @throws SAXException If the document we attempt to parse is not valid according to the specified schema.
*/
@Override
public void warning(SAXParseException e) throws SAXException {
LOGGER.debug("XML Parser error ", e);
throw e;
}

@Override
public void error(SAXParseException e) throws SAXException {
LOGGER.debug("XML Parser error ", e);
throw e;
}

@Override
public void fatalError(SAXParseException e) throws SAXException {
LOGGER.debug("XML Parser error ", e);
throw e;
}
}

private static class ErrorListener implements javax.xml.transform.ErrorListener {

@Override
public void warning(TransformerException e) throws TransformerException {
LOGGER.debug("XML transformation error", e);
throw e;
}

@Override
public void error(TransformerException e) throws TransformerException {
LOGGER.debug("XML transformation error", e);
throw e;
}

@Override
public void fatalError(TransformerException e) throws TransformerException {
LOGGER.debug("XML transformation error", e);
throw e;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import org.elasticsearch.core.Nullable;
import org.elasticsearch.core.SuppressForbidden;
import org.elasticsearch.core.Tuple;
import org.elasticsearch.core.XmlUtils;
import org.elasticsearch.logging.LogManager;
import org.elasticsearch.logging.Logger;
import org.elasticsearch.rest.RestStatus;
Expand All @@ -46,8 +47,6 @@
import java.util.regex.Matcher;
import java.util.regex.Pattern;

import javax.xml.parsers.DocumentBuilderFactory;

import static java.nio.charset.StandardCharsets.UTF_8;
import static org.elasticsearch.test.fixture.HttpHeaderParser.parseRangeHeader;
import static org.junit.Assert.assertEquals;
Expand Down Expand Up @@ -471,7 +470,7 @@ private static Tuple<String, BytesReference> parseRequestBody(final HttpExchange

static List<String> extractPartEtags(BytesReference completeMultipartUploadBody) {
try {
final var document = DocumentBuilderFactory.newInstance().newDocumentBuilder().parse(completeMultipartUploadBody.streamInput());
final var document = XmlUtils.getHardenedBuilderFactory().newDocumentBuilder().parse(completeMultipartUploadBody.streamInput());
final var parts = document.getElementsByTagName("Part");
final var result = new ArrayList<String>(parts.getLength());
for (int partIndex = 0; partIndex < parts.getLength(); partIndex++) {
Expand Down
4 changes: 0 additions & 4 deletions x-pack/plugin/identity-provider/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -80,10 +80,6 @@ tasks.named("forbiddenPatterns").configure {
exclude '**/*.zip'
}

tasks.named('forbiddenApisMain').configure {
signaturesFiles += files('forbidden/xml-signatures.txt')
}

// classes are missing, e.g. com.ibm.icu.lang.UCharacter
tasks.named("thirdPartyAudit").configure {
ignoreMissingClasses(
Expand Down
8 changes: 0 additions & 8 deletions x-pack/plugin/identity-provider/forbidden/xml-signatures.txt

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.util.concurrent.ThreadContext;
import org.elasticsearch.core.Nullable;
import org.elasticsearch.core.XmlUtils;
import org.elasticsearch.xcontent.ObjectPath;
import org.elasticsearch.xcontent.XContentBuilder;
import org.elasticsearch.xcontent.json.JsonXContent;
Expand Down Expand Up @@ -163,7 +164,7 @@ public void testCustomAttributesInIdpInitiatedSso() throws Exception {
final String samlResponse = generateSamlResponseWithAttributes(SP_ENTITY_ID, SP_ACS, null, attributesMap);

// Parse XML directly from samlResponse (it's not base64 encoded at this point)
DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
DocumentBuilderFactory factory = XmlUtils.getHardenedBuilderFactory();
factory.setNamespaceAware(true); // Required for XPath
DocumentBuilder builder = factory.newDocumentBuilder();
Document document = builder.parse(new InputSource(new StringReader(samlResponse)));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ private static void validateNameIdPolicy(AuthnRequest request, SamlServiceProvid
}

private boolean validateSignature(ParsedQueryString queryString, Collection<X509Credential> credentials) {
final String javaSigAlgorithm = SamlFactory.getJavaAlorithmNameFromUri(queryString.sigAlg);
final String javaSigAlgorithm = SamlFactory.getJavaAlgorithmNameFromUri(queryString.sigAlg);
final byte[] contentBytes = queryString.reconstructQueryParameters().getBytes(StandardCharsets.UTF_8);
final byte[] signatureBytes = Base64.getDecoder().decode(queryString.signature);
return credentials.stream().anyMatch(credential -> {
Expand Down
Loading