From c76d3b87ea7ca870ae4796136780fab4b6e22601 Mon Sep 17 00:00:00 2001 From: James Fredley Date: Thu, 3 Sep 2026 17:34:43 -0400 Subject: [PATCH 1/6] Fix SAX parser feature URIs and reject DOCTYPE declarations The SAX XXE hardening used https://xml.org and https://apache.org feature identifiers. Xerces only recognizes the http:// forms, so the empty catch swallowed the misconfiguration. XmlDataBindingSourceCreator and the HTTP test client both now set the registered http:// identifiers, disable XInclude, and reject DOCTYPE declarations. --- .../en/guide/testing/integrationTesting.adoc | 2 +- .../org/grails/io/support/SpringIOUtils.java | 11 ++--- .../io/support/SpringIOUtilsSpec.groovy | 44 +++++++++++++++++++ grails-testing-support-http-client/README.md | 3 +- .../testing/http/client/utils/XmlUtils.groovy | 16 +++---- .../http/client/TestHttpResponseSpec.groovy | 31 +++---------- .../http/client/utils/XmlUtilsSpec.groovy | 34 +++----------- 7 files changed, 71 insertions(+), 70 deletions(-) create mode 100644 grails-gradle/model/src/test/groovy/org/grails/io/support/SpringIOUtilsSpec.groovy diff --git a/grails-doc/src/en/guide/testing/integrationTesting.adoc b/grails-doc/src/en/guide/testing/integrationTesting.adoc index d5fa35c92fe..df9cba510f4 100644 --- a/grails-doc/src/en/guide/testing/integrationTesting.adoc +++ b/grails-doc/src/en/guide/testing/integrationTesting.adoc @@ -500,7 +500,7 @@ Supported named options mirror the `JsonSlurper` settings exposed by `JsonUtils. ===== Custom XML Parsing Response XML parsing uses a secure default `XmlSlurper` configuration. It is namespace-aware, non-validating, -allows inline `DOCTYPE` declarations, and disables external entity expansion plus external DTD loading. +rejects `DOCTYPE` declarations, and disables external entity expansion plus external DTD loading. When a test needs different XML parsing behavior, override it fluently on the response wrapper: diff --git a/grails-gradle/model/src/main/groovy/org/grails/io/support/SpringIOUtils.java b/grails-gradle/model/src/main/groovy/org/grails/io/support/SpringIOUtils.java index cb3da2dc0a8..02a616c05be 100644 --- a/grails-gradle/model/src/main/groovy/org/grails/io/support/SpringIOUtils.java +++ b/grails-gradle/model/src/main/groovy/org/grails/io/support/SpringIOUtils.java @@ -423,19 +423,20 @@ private static SAXParserFactory createParserFactory() throws ParserConfiguration saxParserFactory = FactorySupport.createSaxParserFactory(); saxParserFactory.setNamespaceAware(true); saxParserFactory.setValidating(false); + saxParserFactory.setXIncludeAware(false); try { - saxParserFactory.setFeature("https://apache.org/xml/features/disallow-doctype-decl", false); + saxParserFactory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); } catch (Exception pce) { // ignore, parser doesn't support } try { - saxParserFactory.setFeature("https://xml.org/sax/features/external-general-entities", false); + saxParserFactory.setFeature("http://xml.org/sax/features/external-general-entities", false); } catch (Exception pce) { // ignore, parser doesn't support } try { - saxParserFactory.setFeature("https://xml.org/sax/features/external-parameter-entities", false); + saxParserFactory.setFeature("http://xml.org/sax/features/external-parameter-entities", false); } catch (Exception pce) { // ignore, parser doesn't support } @@ -445,12 +446,12 @@ private static SAXParserFactory createParserFactory() throws ParserConfiguration // ignore, parser doesn't support } try { - saxParserFactory.setFeature("https://apache.org/xml/features/nonvalidating/load-dtd-grammar", false); + saxParserFactory.setFeature("http://apache.org/xml/features/nonvalidating/load-dtd-grammar", false); } catch (Exception e) { // ignore, parser doesn't support } try { - saxParserFactory.setFeature("https://apache.org/xml/features/nonvalidating/load-external-dtd", false); + saxParserFactory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false); } catch (Exception e) { // ignore, parser doesn't support } diff --git a/grails-gradle/model/src/test/groovy/org/grails/io/support/SpringIOUtilsSpec.groovy b/grails-gradle/model/src/test/groovy/org/grails/io/support/SpringIOUtilsSpec.groovy new file mode 100644 index 00000000000..82de74d5d94 --- /dev/null +++ b/grails-gradle/model/src/test/groovy/org/grails/io/support/SpringIOUtilsSpec.groovy @@ -0,0 +1,44 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.grails.io.support + +import org.xml.sax.SAXParseException +import spock.lang.Specification + +class SpringIOUtilsSpec extends Specification { + + void 'createXmlSlurper parses documents without a doctype'() { + when: + def xml = SpringIOUtils.createXmlSlurper().parseText('ok') + + then: + xml.child.text() == 'ok' + } + + void 'createXmlSlurper rejects doctype declarations with external entities'() { + when: + SpringIOUtils.createXmlSlurper().parseText(''' +]> +&ext;''') + + then: + thrown(SAXParseException) + } +} diff --git a/grails-testing-support-http-client/README.md b/grails-testing-support-http-client/README.md index 613c6a5edbc..f4cabb67655 100644 --- a/grails-testing-support-http-client/README.md +++ b/grails-testing-support-http-client/README.md @@ -70,7 +70,7 @@ Supported named options mirror the `JsonSlurper` settings exposed by `JsonUtils. ### Custom XML Parsing Response XML parsing uses a secure default `XmlSlurper` configuration. It is namespace-aware, non-validating, -allows inline `DOCTYPE` declarations, and disables external entity expansion plus external DTD loading. +rejects `DOCTYPE` declarations, and disables external entity expansion plus external DTD loading. When a test needs different XML parsing behavior, override it fluently on the response wrapper: @@ -205,4 +205,3 @@ def payload = XmlUtils.toXml(omitNullAttributes: true, spaceInEmptyElements: fal httpPost('/products', payload, 'application/xml') ``` - diff --git a/grails-testing-support-http-client/src/main/groovy/org/apache/grails/testing/http/client/utils/XmlUtils.groovy b/grails-testing-support-http-client/src/main/groovy/org/apache/grails/testing/http/client/utils/XmlUtils.groovy index 8051154047a..3f2c03b4aa7 100644 --- a/grails-testing-support-http-client/src/main/groovy/org/apache/grails/testing/http/client/utils/XmlUtils.groovy +++ b/grails-testing-support-http-client/src/main/groovy/org/apache/grails/testing/http/client/utils/XmlUtils.groovy @@ -45,12 +45,12 @@ import org.xml.sax.SAXException @CompileStatic class XmlUtils { - private static final String DISALLOW_DOCTYPE_DECL = 'https://apache.org/xml/features/disallow-doctype-decl' - private static final String EXTERNAL_GENERAL_ENTITIES = 'https://xml.org/sax/features/external-general-entities' - private static final String EXTERNAL_PARAMETER_ENTITIES = 'https://xml.org/sax/features/external-parameter-entities' + 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 static final String FEATURE_SECURE_PROCESSING = XMLConstants.FEATURE_SECURE_PROCESSING - private static final String LOAD_DTD_GRAMMAR = 'https://apache.org/xml/features/nonvalidating/load-dtd-grammar' - private static final String LOAD_EXTERNAL_DTD = 'https://apache.org/xml/features/nonvalidating/load-external-dtd' + private static final String LOAD_DTD_GRAMMAR = 'http://apache.org/xml/features/nonvalidating/load-dtd-grammar' + private static final String LOAD_EXTERNAL_DTD = 'http://apache.org/xml/features/nonvalidating/load-external-dtd' private static final Pattern SPACE_AND_EMPTY_ELEMENT_CLOSE = ~/ \/>/ private static final String EMPTY_ELEMENT_CLOSE = '/>' @@ -59,7 +59,7 @@ class XmlUtils { private static final Pattern XML_DECLARATION = ~/^\s*(<\?xml\b.*?\?>)/ private static final Map SECURE_XML_SLURPER_FEATURES = [ - (DISALLOW_DOCTYPE_DECL): false, + (DISALLOW_DOCTYPE_DECL): true, (EXTERNAL_GENERAL_ENTITIES): false, (EXTERNAL_PARAMETER_ENTITIES): false, (FEATURE_SECURE_PROCESSING): true, @@ -118,8 +118,7 @@ class XmlUtils { /** * Creates an {@link XmlSlurper} with secure defaults. *

- * The default parser is namespace aware, non-validating, permits inline DOCTYPE declarations, - * and disables external entity expansion plus external DTD loading. + * The default parser is namespace aware, non-validating, and rejects DOCTYPE declarations. * * @param slurperConfig optional XML parser configuration or custom factory * @return configured {@link XmlSlurper} @@ -224,6 +223,7 @@ class XmlUtils { def saxParserFactory = FactorySupport.createSaxParserFactory().tap { it.namespaceAware = true it.validating = false + it.XIncludeAware = false } SECURE_XML_SLURPER_FEATURES.each { feature, enabled -> diff --git a/grails-testing-support-http-client/src/test/groovy/org/apache/grails/testing/http/client/TestHttpResponseSpec.groovy b/grails-testing-support-http-client/src/test/groovy/org/apache/grails/testing/http/client/TestHttpResponseSpec.groovy index d1b83f9f5c1..71c7fb39729 100644 --- a/grails-testing-support-http-client/src/test/groovy/org/apache/grails/testing/http/client/TestHttpResponseSpec.groovy +++ b/grails-testing-support-http-client/src/test/groovy/org/apache/grails/testing/http/client/TestHttpResponseSpec.groovy @@ -22,7 +22,6 @@ import java.net.http.HttpClient import java.net.http.HttpHeaders import java.net.http.HttpRequest import java.net.http.HttpResponse -import java.nio.file.Files import java.util.regex.Pattern import javax.net.ssl.SSLSession @@ -199,36 +198,18 @@ class TestHttpResponseSpec extends Specification { xmlResponse.xml().item.text() == 'value' } - void 'xml uses a secure default slurper that does not resolve external entities'() { + void 'xml rejects doctype declarations with external entities'() { given: - def secretFile = Files.createTempFile('test-http-response-xml', '.txt') - Files.writeString(secretFile, 'top-secret-token') - def uri = secretFile.toUri().toASCIIString() - def response = mockResponse(200, """ -]> -&ext;""") + def response = mockResponse(200, ''' + ]> +&ext;''') when: response.xml() then: - def e = thrown(SAXParseException) - e.message.contains('External Entity') - - cleanup: - Files.deleteIfExists(secretFile) - } - - void 'xml secure default still allows inline doctype declarations with internal entities'() { - given: - def response = mockResponse(200, ''' -]> -&msg;''') - - expect: - response.xml().text() == 'safe' + thrown(SAXParseException) } void 'withXmlSlurper allows overriding the parser without mutating the original wrapper'() { diff --git a/grails-testing-support-http-client/src/test/groovy/org/apache/grails/testing/http/client/utils/XmlUtilsSpec.groovy b/grails-testing-support-http-client/src/test/groovy/org/apache/grails/testing/http/client/utils/XmlUtilsSpec.groovy index a88acde5228..e0ba0c128a8 100644 --- a/grails-testing-support-http-client/src/test/groovy/org/apache/grails/testing/http/client/utils/XmlUtilsSpec.groovy +++ b/grails-testing-support-http-client/src/test/groovy/org/apache/grails/testing/http/client/utils/XmlUtilsSpec.groovy @@ -19,8 +19,6 @@ package org.apache.grails.testing.http.client.utils import java.nio.charset.StandardCharsets -import java.nio.file.Files - import groovy.xml.XmlSlurper import org.xml.sax.SAXParseException @@ -278,37 +276,15 @@ class XmlUtilsSpec extends Specification { xml == "" } - void 'newXmlSlurper allows inline doctype declarations with internal entities'() { + void 'newXmlSlurper rejects doctype declarations with external entities'() { when: def parsed = XmlUtils.newXmlSlurper().parseText(''' -]> -&msg;''') + + ]> +&ext;''') then: - parsed.text() == 'safe' - } - - void 'newXmlSlurper blocks external entities'() { - given: - def secret = 'xml-utils-secret' - def secretFile = Files.createTempFile('xml-utils-secret', '.txt') - Files.writeString(secretFile, secret) - def uri = secretFile.toUri().toASCIIString() - def xml = """ -]> -&ext;""" - - when: - XmlUtils.newXmlSlurper().parseText(xml) - - then: - def e = thrown(SAXParseException) - e.message.contains('External Entity') - - cleanup: - Files.deleteIfExists(secretFile) + thrown(SAXParseException) } void 'newXmlSlurper supports custom factory overrides'() { From 6fdc50880aa1e4f0bf33131cf343006ec7924ad7 Mon Sep 17 00:00:00 2001 From: James Fredley Date: Thu, 3 Sep 2026 17:53:30 -0400 Subject: [PATCH 2/6] Guard XInclude disable and reject internal DOCTYPE subsets --- .../org/grails/io/support/SpringIOUtils.java | 6 +++++- .../org/grails/io/support/SpringIOUtilsSpec.groovy | 11 +++++++++++ .../testing/http/client/utils/XmlUtils.groovy | 8 +++++++- .../http/client/TestHttpResponseSpec.groovy | 14 ++++++++++++++ .../testing/http/client/utils/XmlUtilsSpec.groovy | 11 +++++++++++ 5 files changed, 48 insertions(+), 2 deletions(-) diff --git a/grails-gradle/model/src/main/groovy/org/grails/io/support/SpringIOUtils.java b/grails-gradle/model/src/main/groovy/org/grails/io/support/SpringIOUtils.java index 02a616c05be..e812c6c9f8f 100644 --- a/grails-gradle/model/src/main/groovy/org/grails/io/support/SpringIOUtils.java +++ b/grails-gradle/model/src/main/groovy/org/grails/io/support/SpringIOUtils.java @@ -423,7 +423,11 @@ private static SAXParserFactory createParserFactory() throws ParserConfiguration saxParserFactory = FactorySupport.createSaxParserFactory(); saxParserFactory.setNamespaceAware(true); saxParserFactory.setValidating(false); - saxParserFactory.setXIncludeAware(false); + try { + saxParserFactory.setXIncludeAware(false); + } catch (UnsupportedOperationException e) { + // ignore, parser doesn't support + } try { saxParserFactory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); diff --git a/grails-gradle/model/src/test/groovy/org/grails/io/support/SpringIOUtilsSpec.groovy b/grails-gradle/model/src/test/groovy/org/grails/io/support/SpringIOUtilsSpec.groovy index 82de74d5d94..a292c1e7b2d 100644 --- a/grails-gradle/model/src/test/groovy/org/grails/io/support/SpringIOUtilsSpec.groovy +++ b/grails-gradle/model/src/test/groovy/org/grails/io/support/SpringIOUtilsSpec.groovy @@ -41,4 +41,15 @@ class SpringIOUtilsSpec extends Specification { then: thrown(SAXParseException) } + + void 'createXmlSlurper rejects doctype declarations with internal entities'() { + when: + SpringIOUtils.createXmlSlurper().parseText(''' +]> +&msg;''') + + then: + thrown(SAXParseException) + } } diff --git a/grails-testing-support-http-client/src/main/groovy/org/apache/grails/testing/http/client/utils/XmlUtils.groovy b/grails-testing-support-http-client/src/main/groovy/org/apache/grails/testing/http/client/utils/XmlUtils.groovy index 3f2c03b4aa7..06570e27863 100644 --- a/grails-testing-support-http-client/src/main/groovy/org/apache/grails/testing/http/client/utils/XmlUtils.groovy +++ b/grails-testing-support-http-client/src/main/groovy/org/apache/grails/testing/http/client/utils/XmlUtils.groovy @@ -223,7 +223,13 @@ class XmlUtils { def saxParserFactory = FactorySupport.createSaxParserFactory().tap { it.namespaceAware = true it.validating = false - it.XIncludeAware = false + } + + try { + saxParserFactory.XIncludeAware = false + } + catch (UnsupportedOperationException ignored) { + // ignore, parser doesn't support } SECURE_XML_SLURPER_FEATURES.each { feature, enabled -> diff --git a/grails-testing-support-http-client/src/test/groovy/org/apache/grails/testing/http/client/TestHttpResponseSpec.groovy b/grails-testing-support-http-client/src/test/groovy/org/apache/grails/testing/http/client/TestHttpResponseSpec.groovy index 71c7fb39729..2a789c9f428 100644 --- a/grails-testing-support-http-client/src/test/groovy/org/apache/grails/testing/http/client/TestHttpResponseSpec.groovy +++ b/grails-testing-support-http-client/src/test/groovy/org/apache/grails/testing/http/client/TestHttpResponseSpec.groovy @@ -212,6 +212,20 @@ class TestHttpResponseSpec extends Specification { thrown(SAXParseException) } + void 'xml rejects doctype declarations with internal entities'() { + given: + def response = mockResponse(200, ''' +]> +&msg;''') + + when: + response.xml() + + then: + thrown(SAXParseException) + } + void 'withXmlSlurper allows overriding the parser without mutating the original wrapper'() { given: def response = mockResponse(200, 'value') diff --git a/grails-testing-support-http-client/src/test/groovy/org/apache/grails/testing/http/client/utils/XmlUtilsSpec.groovy b/grails-testing-support-http-client/src/test/groovy/org/apache/grails/testing/http/client/utils/XmlUtilsSpec.groovy index e0ba0c128a8..e7064d4b98d 100644 --- a/grails-testing-support-http-client/src/test/groovy/org/apache/grails/testing/http/client/utils/XmlUtilsSpec.groovy +++ b/grails-testing-support-http-client/src/test/groovy/org/apache/grails/testing/http/client/utils/XmlUtilsSpec.groovy @@ -287,6 +287,17 @@ class XmlUtilsSpec extends Specification { thrown(SAXParseException) } + void 'newXmlSlurper rejects doctype declarations with internal entities'() { + when: + def parsed = XmlUtils.newXmlSlurper().parseText(''' +]> +&msg;''') + + then: + thrown(SAXParseException) + } + void 'newXmlSlurper supports custom factory overrides'() { given: int factoryCalls = 0 From 840aea9b91b216b54b55ca5336fc22c61bd20fb7 Mon Sep 17 00:00:00 2001 From: James Daugherty Date: Thu, 3 Sep 2026 23:30:30 -0400 Subject: [PATCH 3/6] Share SAX feature identifiers and make DOCTYPE rejection configurable The SAX feature identifiers were declared twice, once in SpringIOUtils and once in the HTTP test client, as bare string literals. That duplication is how the https:// spelling was introduced and went unnoticed: setFeature answers an unrecognised name with SAXNotRecognizedException, and both call sites swallow it, so the hardening silently switched off. Collect the five identifiers in XmlParserFeature in grails-gradle-common, which sits in the grails-gradle build alongside SpringIOUtils and is already exposed to the root build by grails-common. The enum documents that the values are registered identifiers rather than addresses, and carries the reason the http scheme cannot be rewritten. Keep rejecting DOCTYPE declarations by default, and add grails.xml.allowDocTypeDeclaration for applications that must accept them. SpringIOUtils reads it through Metadata, so it is set in application.yml or as a system property. Opting in relaxes only whether a declaration is permitted; external general entities, external parameter entities and external DTDs stay refused either way, so it does not reopen the XXE vector. The setting is needed because this parser factory is shared with readers of trusted classpath descriptors -- TldReader, WebXmlTagLibraryReader and PluginUtils -- and TLDs routinely carry a DOCTYPE. jakarta.servlet.jsp.jstl ships eight, including c-1_0-rt.tld, which the default grails.gsp.tldScanPattern scans, so an application resolving JSP tag libraries from a GSP needs it enabled. Document that on the JSP tag library page and in the upgrade notes. Cover both modules with tests that assert observable behaviour rather than reading feature flags back, so no test holds a second copy of the identifiers that a rewrite could update in step with the production code. XmlParserFeatureSpec additionally asserts every identifier is one a parser actually registers, turning an unrecognised name into a named failure instead of a silent no-op. --- .../gsp/taglibs/usingJSPTagLibraries.adoc | 12 ++ grails-doc/src/en/guide/upgrading.adoc | 19 +++ .../gradle/common/XmlParserFeature.java | 93 ++++++++++++++ .../gradle/common/XmlParserFeatureSpec.groovy | 54 ++++++++ grails-gradle/model/build.gradle | 2 + .../org/grails/io/support/SpringIOUtils.java | 111 +++++++++++------ .../io/support/SpringIOUtilsSpec.groovy | 115 ++++++++++++++++-- .../build.gradle | 1 + .../testing/http/client/utils/XmlUtils.groovy | 21 ++-- 9 files changed, 372 insertions(+), 56 deletions(-) create mode 100644 grails-gradle/common/src/main/groovy/org/apache/grails/gradle/common/XmlParserFeature.java create mode 100644 grails-gradle/common/src/test/groovy/org/apache/grails/gradle/common/XmlParserFeatureSpec.groovy diff --git a/grails-doc/src/en/guide/theWebLayer/gsp/taglibs/usingJSPTagLibraries.adoc b/grails-doc/src/en/guide/theWebLayer/gsp/taglibs/usingJSPTagLibraries.adoc index d532842d03f..2b2e1eb9ab9 100644 --- a/grails-doc/src/en/guide/theWebLayer/gsp/taglibs/usingJSPTagLibraries.adoc +++ b/grails-doc/src/en/guide/theWebLayer/gsp/taglibs/usingJSPTagLibraries.adoc @@ -51,6 +51,18 @@ grails: tldScanPattern: 'classpath*:/META-INF/*.tld,/WEB-INF/tld/*.tld' ---- +Tag library descriptors are parsed with the framework's XML settings, which reject `DOCTYPE` declarations by default. Many descriptors declare one -- `jakarta.servlet.jsp.jstl` ships several, among them `c-1_0-rt.tld`, which the default scan pattern includes -- so scanning them requires permitting declarations: + +[source,yaml] +.grails-app/conf/application.yml +---- +grails: + xml: + allowDocTypeDeclaration: true +---- + +Without it, resolving a JSP tag library fails with a `SAXParseException` reporting that `DOCTYPE is disallowed`. Enabling the setting does not reopen the XXE vector; external entities and external DTDs stay refused. See <> for the full description. + JSTL standard library is no longer added as a dependency by default. In case you are using JSTL, you should also add these dependencies to `build.gradle`: [source,groovy] .build.gradle diff --git a/grails-doc/src/en/guide/upgrading.adoc b/grails-doc/src/en/guide/upgrading.adoc index f3248516212..163905f03b1 100644 --- a/grails-doc/src/en/guide/upgrading.adoc +++ b/grails-doc/src/en/guide/upgrading.adoc @@ -30,3 +30,22 @@ In compatibility mode, a `bindData` call that supplies only `exclude` continues Values of typed `Map` properties are converted to the declared value type. Conversion failures are added to the binding errors and data binding listeners receive the corresponding events. The values `true`, `false`, `'true'`, and `'false'` are accepted for `grails.databinding.denyByDefault`, ignoring case and surrounding whitespace for strings. An unrecognised value logs a warning and enables secure deny-by-default binding. + +=== XML Parsing Defaults + +XML read by Grails is parsed with external entity expansion and external DTD loading disabled. This closes the XXE vector: an entity that points at a file on disk contributes nothing to the parsed document. + +Grails 8 additionally rejects `DOCTYPE` declarations outright. A request body, or any other document parsed through the framework, that carries a `DOCTYPE` is refused with a `SAXParseException` rather than parsed. + +This is stricter than blocking external entities, because it also refuses documents whose `DOCTYPE` is entirely internal and harmless. Applications that must accept such documents can opt back in: + +[source,yaml] +---- +grails: + xml: + allowDocTypeDeclaration: true +---- + +The setting is also accepted as the `grails.xml.allowDocTypeDeclaration` system property. Opting in relaxes only whether a declaration is permitted; external general entities, external parameter entities and external DTDs stay refused either way, so it does not reopen the XXE vector. + +Descriptors read from the classpath are parsed by the same mechanism, and some carry a `DOCTYPE`. JSP tag library descriptors are the common case: `jakarta.servlet.jsp.jstl` ships several, among them `c-1_0-rt.tld`, which the default `grails.gsp.tldScanPattern` scans. An application that uses JSP tag libraries from a GSP needs this setting enabled. diff --git a/grails-gradle/common/src/main/groovy/org/apache/grails/gradle/common/XmlParserFeature.java b/grails-gradle/common/src/main/groovy/org/apache/grails/gradle/common/XmlParserFeature.java new file mode 100644 index 00000000000..a43e730dcbe --- /dev/null +++ b/grails-gradle/common/src/main/groovy/org/apache/grails/gradle/common/XmlParserFeature.java @@ -0,0 +1,93 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.grails.gradle.common; + +/** + * Registered SAX and Xerces parser feature identifiers used to harden XML parsing. + * + *

These values are opaque identifiers, not addresses. Nothing is ever fetched + * from them. A parser matches them by exact string comparison against the prefixes it registers + * internally, {@code http://xml.org/sax/features/} and {@code http://apache.org/xml/features/}. + * + *

Do not rewrite the {@code http} scheme to {@code https}. No parser + * recognizes the {@code https} spelling; {@code setFeature} answers it with + * {@code SAXNotRecognizedException}. Because callers wrap {@code setFeature} in a catch that + * tolerates parsers lacking a feature, an unrecognised name is swallowed and the hardening is + * silently disabled rather than failing loudly. A blanket "prefer https" sweep over the codebase + * therefore turns XML hardening off without leaving a trace, which is exactly what happened + * before these values were collected here. + * + *

Consumers must assert parser behaviour rather than reading these names back, so that + * the guarding tests hold no copy of the identifiers and cannot be rewritten by the same sweep. + * + * @since 8.0.0 + */ +public enum XmlParserFeature { + + /** + * Rejects any document carrying a {@code DOCTYPE} declaration. + * + *

Enabling this is stricter than blocking external entities: it refuses documents whose + * DOCTYPE is entirely internal and harmless. Descriptors read from the classpath — JSP tag + * library definitions, {@code web.xml}, {@code plugin.xml} — routinely carry a DOCTYPE, so a + * parser shared with those callers must leave this disabled. + */ + DISALLOW_DOCTYPE_DECL("http://apache.org/xml/features/disallow-doctype-decl"), + + /** + * Blocks resolution of external general entities, the primary XXE vector. + */ + EXTERNAL_GENERAL_ENTITIES("http://xml.org/sax/features/external-general-entities"), + + /** + * Blocks resolution of external parameter entities. + */ + EXTERNAL_PARAMETER_ENTITIES("http://xml.org/sax/features/external-parameter-entities"), + + /** + * Stops the parser building a grammar from a DTD. + */ + LOAD_DTD_GRAMMAR("http://apache.org/xml/features/nonvalidating/load-dtd-grammar"), + + /** + * Skips external DTD subsets instead of retrieving them. + * + *

This differs from the JAXP {@code XMLConstants.ACCESS_EXTERNAL_DTD} property, which + * raises an error when a document references an external DTD. Skipping is what allows a + * descriptor that names a DTD, such as a JSP 1.2 tag library, to parse without retrieving it. + */ + LOAD_EXTERNAL_DTD("http://apache.org/xml/features/nonvalidating/load-external-dtd"); + + private final String featureName; + + XmlParserFeature(String featureName) { + this.featureName = featureName; + } + + /** + * @return the registered identifier to pass to {@code setFeature} + */ + public String getFeatureName() { + return featureName; + } + + @Override + public String toString() { + return featureName; + } + +} diff --git a/grails-gradle/common/src/test/groovy/org/apache/grails/gradle/common/XmlParserFeatureSpec.groovy b/grails-gradle/common/src/test/groovy/org/apache/grails/gradle/common/XmlParserFeatureSpec.groovy new file mode 100644 index 00000000000..cb46f049fb1 --- /dev/null +++ b/grails-gradle/common/src/test/groovy/org/apache/grails/gradle/common/XmlParserFeatureSpec.groovy @@ -0,0 +1,54 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.grails.gradle.common + +import javax.xml.parsers.SAXParserFactory + +import spock.lang.Specification +import spock.lang.Unroll + +class XmlParserFeatureSpec extends Specification { + + /** + * Every identifier must be one the parser actually registers. + * + *

Callers set these inside a catch that tolerates a parser lacking a feature, so an + * unrecognised identifier disables hardening silently instead of failing. Rewriting the + * {@code http} scheme to {@code https} is the way that happens in practice. This spec derives + * the identifiers from {@link XmlParserFeature#values()} rather than restating them, so the + * same rewrite cannot pass by changing the expectation to match. + */ + @Unroll + void 'feature #feature is recognised by the parser'() { + given: + SAXParserFactory factory = SAXParserFactory.newInstance() + + when: + factory.setFeature(feature.featureName, false) + + then: + noExceptionThrown() + + where: + feature << XmlParserFeature.values() + } + + void 'every feature is distinct'() { + expect: + XmlParserFeature.values()*.featureName.toUnique().size() == XmlParserFeature.values().length + } +} diff --git a/grails-gradle/model/build.gradle b/grails-gradle/model/build.gradle index f4b935f75ba..eb3c9381e38 100644 --- a/grails-gradle/model/build.gradle +++ b/grails-gradle/model/build.gradle @@ -42,6 +42,8 @@ ext { dependencies { implementation platform(project(':grails-gradle-bom')) + implementation project(':grails-gradle-common') // XmlParserFeature + // compile grails-gradle-model with the Groovy version provided by Gradle // Groovy 4+ uses org.apache.groovy coordinates // when used by grails-gradle-plugin diff --git a/grails-gradle/model/src/main/groovy/org/grails/io/support/SpringIOUtils.java b/grails-gradle/model/src/main/groovy/org/grails/io/support/SpringIOUtils.java index e812c6c9f8f..bea3ed946f6 100644 --- a/grails-gradle/model/src/main/groovy/org/grails/io/support/SpringIOUtils.java +++ b/grails-gradle/model/src/main/groovy/org/grails/io/support/SpringIOUtils.java @@ -49,6 +49,9 @@ import org.xml.sax.SAXException; +import grails.util.Metadata; +import org.apache.grails.gradle.common.XmlParserFeature; + /** * Simple utility methods for file and stream copying. * All copy methods use a block size of 4096 bytes, @@ -416,50 +419,86 @@ public static SAXParser newSAXParser() throws ParserConfigurationException, SAXE return factory.newSAXParser(); } - private static SAXParserFactory saxParserFactory = null; + /** + * Configuration key permitting {@code DOCTYPE} declarations in documents parsed by this class. + * + *

Parsers handed out here reject a {@code DOCTYPE} by default. Set + * {@code grails.xml.allowDocTypeDeclaration} to {@code true} in {@code application.yml}, or as + * a system property, to accept one. + * + *

Opting in does not reopen the XXE vector. External general entities, external parameter + * entities and external DTDs stay refused whichever way this is set, so an entity pointing at + * a file on disk still contributes nothing. What opting in changes is only whether a document + * carrying a declaration is refused outright. + * + *

It exists because these parsers also read trusted descriptors from the classpath, and + * some of those carry a {@code DOCTYPE}. JSP tag library descriptors are the common case: + * {@code jakarta.servlet.jsp.jstl} ships several, among them {@code c-1_0-rt.tld}, which the + * default {@code grails.gsp.tldScanPattern} scans. + */ + public static final String ALLOW_DOCTYPE_DECLARATION = "grails.xml.allowDocTypeDeclaration"; + + /** + * Parser features switched off for every parser this class hands out. + * + *

{@link XmlParserFeature#DISALLOW_DOCTYPE_DECL} is handled separately because it is the + * one feature an application may turn off; see {@link #ALLOW_DOCTYPE_DECLARATION}. + */ + private static final XmlParserFeature[] DISABLED_PARSER_FEATURES = { + XmlParserFeature.EXTERNAL_GENERAL_ENTITIES, + XmlParserFeature.EXTERNAL_PARAMETER_ENTITIES, + XmlParserFeature.LOAD_DTD_GRAMMAR, + XmlParserFeature.LOAD_EXTERNAL_DTD + }; + + private static SAXParserFactory strictParserFactory = null; + + private static SAXParserFactory docTypeParserFactory = null; private static SAXParserFactory createParserFactory() throws ParserConfigurationException { - if (saxParserFactory == null) { - saxParserFactory = FactorySupport.createSaxParserFactory(); - saxParserFactory.setNamespaceAware(true); - saxParserFactory.setValidating(false); - try { - saxParserFactory.setXIncludeAware(false); - } catch (UnsupportedOperationException e) { - // ignore, parser doesn't support + if (isDocTypeDeclarationAllowed()) { + if (docTypeParserFactory == null) { + docTypeParserFactory = buildParserFactory(true); } + return docTypeParserFactory; + } + if (strictParserFactory == null) { + strictParserFactory = buildParserFactory(false); + } + return strictParserFactory; + } + private static boolean isDocTypeDeclarationAllowed() { + return Boolean.TRUE.equals( + Metadata.getCurrent().getProperty(ALLOW_DOCTYPE_DECLARATION, Boolean.class, Boolean.FALSE)); + } + + private static SAXParserFactory buildParserFactory(boolean allowDocTypeDeclaration) throws ParserConfigurationException { + SAXParserFactory factory = FactorySupport.createSaxParserFactory(); + factory.setNamespaceAware(true); + factory.setValidating(false); + try { + factory.setXIncludeAware(false); + } catch (UnsupportedOperationException e) { + // ignore, parser doesn't support + } + try { + factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); + } catch (Exception e) { + // ignore, parser doesn't support + } + try { + factory.setFeature(XmlParserFeature.DISALLOW_DOCTYPE_DECL.getFeatureName(), !allowDocTypeDeclaration); + } catch (Exception e) { + // ignore, parser doesn't support + } + for (XmlParserFeature feature : DISABLED_PARSER_FEATURES) { try { - saxParserFactory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); - } catch (Exception pce) { - // ignore, parser doesn't support - } - try { - saxParserFactory.setFeature("http://xml.org/sax/features/external-general-entities", false); - } catch (Exception pce) { - // ignore, parser doesn't support - } - try { - saxParserFactory.setFeature("http://xml.org/sax/features/external-parameter-entities", false); - } catch (Exception pce) { - // ignore, parser doesn't support - } - try { - saxParserFactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); - } catch (Exception e) { - // ignore, parser doesn't support - } - try { - saxParserFactory.setFeature("http://apache.org/xml/features/nonvalidating/load-dtd-grammar", false); - } catch (Exception e) { - // ignore, parser doesn't support - } - try { - saxParserFactory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false); + factory.setFeature(feature.getFeatureName(), false); } catch (Exception e) { // ignore, parser doesn't support } } - return saxParserFactory; + return factory; } } diff --git a/grails-gradle/model/src/test/groovy/org/grails/io/support/SpringIOUtilsSpec.groovy b/grails-gradle/model/src/test/groovy/org/grails/io/support/SpringIOUtilsSpec.groovy index a292c1e7b2d..7c4bed55d06 100644 --- a/grails-gradle/model/src/test/groovy/org/grails/io/support/SpringIOUtilsSpec.groovy +++ b/grails-gradle/model/src/test/groovy/org/grails/io/support/SpringIOUtilsSpec.groovy @@ -18,12 +18,50 @@ */ package org.grails.io.support +import java.nio.file.Files +import java.nio.file.Path + +import grails.util.Metadata + import org.xml.sax.SAXParseException + import spock.lang.Specification +import spock.lang.TempDir +/** + * Asserts the parser hardening applied by {@link SpringIOUtils} through observable parsing + * behaviour rather than by reading feature flags back off the factory. + * + *

This is deliberate. Reading the flags back would require this spec to hold its own copy of + * the feature identifiers, so a search-and-replace over those identifiers would rewrite the + * production code and this spec together and the suite would still pass. Driving real documents + * through the parser keeps the assertions independent of how the hardening is spelled. + */ class SpringIOUtilsSpec extends Specification { - void 'createXmlSlurper parses documents without a doctype'() { + /** Shape of a JSP 1.2 tag library descriptor, as shipped inside jakarta jstl. */ + private static final String TLD = ''' + + jakarta.tags.core + outorg.example.OutTag +''' + + @TempDir + Path tempDir + + void cleanup() { + System.clearProperty(SpringIOUtils.ALLOW_DOCTYPE_DECLARATION) + Metadata.reset() + } + + private static void allowDocTypeDeclarations() { + System.setProperty(SpringIOUtils.ALLOW_DOCTYPE_DECLARATION, 'true') + Metadata.reset() + } + + void 'createXmlSlurper parses a document without a doctype'() { when: def xml = SpringIOUtils.createXmlSlurper().parseText('ok') @@ -31,18 +69,15 @@ class SpringIOUtilsSpec extends Specification { xml.child.text() == 'ok' } - void 'createXmlSlurper rejects doctype declarations with external entities'() { + void 'createXmlSlurper rejects a doctype declaration by default'() { when: - SpringIOUtils.createXmlSlurper().parseText(''' -]> -&ext;''') + SpringIOUtils.createXmlSlurper().parseText(TLD) then: thrown(SAXParseException) } - void 'createXmlSlurper rejects doctype declarations with internal entities'() { + void 'createXmlSlurper rejects an internal doctype subset by default'() { when: SpringIOUtils.createXmlSlurper().parseText(''' @@ -52,4 +87,70 @@ class SpringIOUtilsSpec extends Specification { then: thrown(SAXParseException) } + + void 'the doctype configuration key lets an application parse descriptors that declare one'() { + given: 'an application.yml opting in, as an application would configure it' + Metadata.getInstance(new ByteArrayInputStream('''grails: + xml: + allowDocTypeDeclaration: true +'''.getBytes('UTF-8'))) + + when: + def parsed = SpringIOUtils.createXmlSlurper().parseText(TLD) + + then: 'the descriptor is readable' + parsed.uri.text() == 'jakarta.tags.core' + parsed.tag.name.text() == 'out' + } + + void 'external entities stay blocked when doctype declarations are permitted'() { + given: 'a document whose entity points at a readable file on disk' + allowDocTypeDeclarations() + Path secret = tempDir.resolve('secret.txt') + Files.writeString(secret, 'top-secret-token') + String xml = """ +]> +&ext;""" + + when: + def parsed = SpringIOUtils.createXmlSlurper().parseText(xml) + + then: 'relaxing the doctype rule does not reopen the XXE vector' + !parsed.text().contains('top-secret-token') + } + + void 'external dtds are skipped rather than retrieved when doctype declarations are permitted'() { + given: + allowDocTypeDeclarations() + String xml = """ +ok""" + + expect: + SpringIOUtils.createXmlSlurper().parseText(xml).text() == 'ok' + } + + void 'newSAXParser applies the same hardening as createXmlSlurper'() { + given: + allowDocTypeDeclarations() + Path secret = tempDir.resolve('secret.txt') + Files.writeString(secret, 'top-secret-token') + String xml = """ +]> +&ext;""" + StringBuilder text = new StringBuilder() + + when: + SpringIOUtils.newSAXParser().parse(new ByteArrayInputStream(xml.getBytes('UTF-8')), + new org.xml.sax.helpers.DefaultHandler() { + @Override + void characters(char[] chars, int start, int length) { + text.append(chars, start, length) + } + }) + + then: + !text.toString().contains('top-secret-token') + } } diff --git a/grails-testing-support-http-client/build.gradle b/grails-testing-support-http-client/build.gradle index e4dc84751d3..5041c32836f 100644 --- a/grails-testing-support-http-client/build.gradle +++ b/grails-testing-support-http-client/build.gradle @@ -44,6 +44,7 @@ dependencies { implementation platform(project(':grails-bom')) implementation project(':grails-testing-support-core') + implementation 'org.apache.grails.gradle:grails-gradle-common' // XmlParserFeature implementation 'org.apache.groovy:groovy' implementation 'org.apache.groovy:groovy-json' implementation 'org.apache.groovy:groovy-xml' diff --git a/grails-testing-support-http-client/src/main/groovy/org/apache/grails/testing/http/client/utils/XmlUtils.groovy b/grails-testing-support-http-client/src/main/groovy/org/apache/grails/testing/http/client/utils/XmlUtils.groovy index 06570e27863..fcabc64d861 100644 --- a/grails-testing-support-http-client/src/main/groovy/org/apache/grails/testing/http/client/utils/XmlUtils.groovy +++ b/grails-testing-support-http-client/src/main/groovy/org/apache/grails/testing/http/client/utils/XmlUtils.groovy @@ -37,6 +37,8 @@ import groovy.xml.XmlSlurper import org.xml.sax.SAXException +import org.apache.grails.gradle.common.XmlParserFeature + /** * Utility methods for handling XML. * @@ -45,13 +47,6 @@ import org.xml.sax.SAXException @CompileStatic 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 static final String FEATURE_SECURE_PROCESSING = XMLConstants.FEATURE_SECURE_PROCESSING - private static final String LOAD_DTD_GRAMMAR = 'http://apache.org/xml/features/nonvalidating/load-dtd-grammar' - private static final String LOAD_EXTERNAL_DTD = 'http://apache.org/xml/features/nonvalidating/load-external-dtd' - private static final Pattern SPACE_AND_EMPTY_ELEMENT_CLOSE = ~/ \/>/ private static final String EMPTY_ELEMENT_CLOSE = '/>' @@ -59,12 +54,12 @@ class XmlUtils { private static final Pattern XML_DECLARATION = ~/^\s*(<\?xml\b.*?\?>)/ private static final Map SECURE_XML_SLURPER_FEATURES = [ - (DISALLOW_DOCTYPE_DECL): true, - (EXTERNAL_GENERAL_ENTITIES): false, - (EXTERNAL_PARAMETER_ENTITIES): false, - (FEATURE_SECURE_PROCESSING): true, - (LOAD_DTD_GRAMMAR): false, - (LOAD_EXTERNAL_DTD): false + (XMLConstants.FEATURE_SECURE_PROCESSING): true, + (XmlParserFeature.DISALLOW_DOCTYPE_DECL.featureName): true, + (XmlParserFeature.EXTERNAL_GENERAL_ENTITIES.featureName): false, + (XmlParserFeature.EXTERNAL_PARAMETER_ENTITIES.featureName): false, + (XmlParserFeature.LOAD_DTD_GRAMMAR.featureName): false, + (XmlParserFeature.LOAD_EXTERNAL_DTD.featureName): false ].asImmutable() /** From e10b69c4bee4acc3ea01cb905a1a81f8e4b9a122 Mon Sep 17 00:00:00 2001 From: James Daugherty Date: Thu, 10 Sep 2026 21:36:04 -0400 Subject: [PATCH 4/6] feedback: Parse XML by trust level: strict for request bodies, DOCTYPE-tolerant for classpath descriptors --- THREAT_MODEL.md | 6 +- .../grails/core/plugins/PluginUtils.java | 3 +- .../core/plugins/PluginDiscoverySpec.groovy | 28 ++++ .../gsp/taglibs/usingJSPTagLibraries.adoc | 12 +- grails-doc/src/en/guide/upgrading.adoc | 17 +-- .../src/en/guide/upgrading/upgrading80x.adoc | 15 ++ .../gradle/common/XmlParserFeature.java | 7 +- .../org/grails/io/support/SpringIOUtils.java | 139 +++++++++++------- .../io/support/SpringIOUtilsSpec.groovy | 134 +++++++++++------ .../org/grails/gsp/jsp/TldReader.groovy | 3 +- .../gsp/jsp/WebXmlTagLibraryReader.groovy | 3 +- .../grails/gsp/jsp/JstlDocTypeTldSpec.groovy | 72 +++++++++ .../org/grails/gsp/jsp/TldReaderTests.groovy | 56 +++++++ .../jsp/WebXmlTagLibraryReaderTests.groovy | 20 +++ ...stractGrailsMockHttpServletResponse.groovy | 8 +- .../GrailsMockHttpServletResponseTests.groovy | 33 +++++ .../testing/http/client/utils/XmlUtils.groovy | 12 +- .../http/client/TestHttpResponseSpec.groovy | 10 +- .../http/client/utils/XmlUtilsSpec.groovy | 14 +- .../XmlDataBindingSourceCreatorSpec.groovy | 78 ++++++++++ threat-model.yaml | 13 +- 21 files changed, 537 insertions(+), 146 deletions(-) create mode 100644 grails-gsp/plugin/src/test/groovy/org/grails/gsp/jsp/JstlDocTypeTldSpec.groovy create mode 100644 grails-web-databinding/src/test/groovy/org/grails/web/databinding/bindingsource/XmlDataBindingSourceCreatorSpec.groovy diff --git a/THREAT_MODEL.md b/THREAT_MODEL.md index b3436788d99..58bb2fa620c 100644 --- a/THREAT_MODEL.md +++ b/THREAT_MODEL.md @@ -296,6 +296,7 @@ Each property is stated with its conditions, the symptom of a violation, a sever | P7 | **Compile-time AST transforms (`@Resource`, `@Validateable`, etc.) only act on developer-authored source.** | [CWE-94](https://cwe.mitre.org/data/definitions/94.html) | Build runs on developer-controlled source. | A transform fires on or is influenced by attacker-supplied input. | **Correctness** (security-critical only if reachable from a non-build attacker) | *(inferred)* | | P8 | **Configuration loading does not evaluate `application.groovy` from a path the framework itself chose at runtime - paths come from build-time classpath and operator-supplied environment/system properties.** | [CWE-94](https://cwe.mitre.org/data/definitions/94.html) | Operator has not pointed `grails.config.locations` at attacker-writable storage. | A user request causes evaluation of a Groovy file the operator did not authorize. | **Security-critical (CVE-eligible)** if violated. | *(inferred)* (§14 wave 1) | | P9 | **`maxFileSize` / `maxRequestSize` / `autoGrowCollectionLimit` provide bounded data-binding memory.** | [CWE-770](https://cwe.mitre.org/data/definitions/770.html) | Operator does not raise the limits past application needs. | Memory growth proportional to attacker-controlled input regardless of limit. | **Resource bug** | *(inferred)* (§14 wave 2) | +| P10 | **XML the framework parses is XXE-hardened: external general and parameter entities, external DTDs and DTD grammars are refused, and a request body that declares a `DOCTYPE` is rejected.** | [CWE-611](https://cwe.mitre.org/data/definitions/611.html) | Document is parsed by the framework's XML data binding (`application/xml`, `text/xml`, `application/hal+xml`) or by `XML.parse`; a parser the application constructs itself gets the JDK defaults. | Content of an external entity appears in bound data, or a body carrying a `DOCTYPE` is bound. | **Security-critical (CVE-eligible)** | *(documented: [upgrading.adoc](./grails-doc/src/en/guide/upgrading.adoc) "XML Parsing Defaults")* | ### Resource consumption line @@ -335,6 +336,7 @@ Features that **look like** a security property but are not one. Reports that co - **`grails.serverURL` is for link generation, not an authoritative declaration of the deployment URL for security purposes.** Setting it does not bind the application to that origin; the embedded container still serves whatever the operator binds it to. *(inferred)* - **`GRAILS_ENV=development` is not a security boundary.** Stack traces, verbose error pages, and dev-tool endpoints surfaced in `development` mode are a deployment-configuration symptom, not a framework vulnerability. A report that requires `GRAILS_ENV=development` to reproduce is `OUT-OF-MODEL: non-default-build` (§13), not `VALID`. *(inferred)* (§14 wave 1) - **`grails.config.locations` is a Groovy code-execution path, not a configuration-file path.** Any file the application process can read AND an attacker can write to is equivalent to classpath compromise: the `.groovy` form is evaluated via `ConfigSlurper`. A path that looks like "just config" but lives in attacker-writable storage (e.g. an S3 bucket without write controls, a world-writable `/tmp` derivative, a CI artifact directory) is `BY-DESIGN: property-disclaimed` (§13). *(inferred)* (§14 wave 1) +- **XML hardening covers documents the framework parses, not parsers the application constructs.** Request bodies bound through the framework and descriptors it reads from the classpath are parsed with external entities and external DTDs refused, and request bodies additionally reject a `DOCTYPE` (§8 P10). An `XmlSlurper` or `XmlParser` the application creates itself gets the JDK defaults; XXE through such a parser is application code, not a framework finding. *(inferred)* ### Well-known attack classes against this category of project that the framework does not defend against @@ -343,7 +345,6 @@ One sentence per class. - **Mass assignment.** Binding the request map directly to a domain class without `bindable`/allow-lists. *(inferred)* - **Open redirect.** Using a request parameter as a `redirect(url: params.next)` target. *(documented: [securingAgainstAttacks.adoc](./grails-doc/src/en/guide/security/securingAgainstAttacks.adoc) "XSS - cross-site scripting injection" mentions this in the `successURL` example)* - **Server-Side Request Forgery (SSRF).** No built-in URL-fetch allow-list. *(inferred)* -- **XXE in XML data binding.** XML parsing is delegated to the underlying parser; the framework does not impose a parser configuration. *(inferred)* - **ReDoS in developer-authored URL mappings and constraint regexes.** No complexity ceiling. *(inferred)* - **Zip-bomb / archive expansion** in multipart and `grails-forge` ZIP generation. Bounded only by container size limits. *(inferred)* - **Path traversal** through `MultipartFile.originalFilename` if used as a filesystem path. *(inferred)* @@ -467,7 +468,7 @@ The model is **draft-first**. The questions below are grouped in waves of 3-7 pe 10. **`SimpleDataBinder` mass-assignment.** *Proposed*: binding `new Book(params)` without an allow-list is `VALID-HARDENING` (the framework should warn, not block, but documents should call this out more loudly). Or is it `BY-DESIGN: property-disclaimed`? Choose. 11. **Multipart limits.** *Proposed*: the framework does not impose multipart caps beyond Spring Boot's defaults; this is operator responsibility. Confirm. 12. **`bindable=false` semantics.** *Proposed*: `bindable=false` is enforced for all binding paths (`bindData`, command-object binding, domain-class constructor binding, `properties=`). Confirm coverage - is there any binding path that ignores it? -13. **XML data binding parser configuration.** *Proposed*: the framework does not impose XXE-hardening configuration on the XML parser; XXE in `XmlDataBindingSourceCreator` is the parser's threat model, not the framework's. Confirm. +13. **XML data binding parser configuration.** *Resolved*: the framework imposes XXE hardening on every XML document it parses and rejects a `DOCTYPE` in request bodies (§8 P10). XXE through a parser the application constructs itself remains the application's responsibility (§9 false friend). Confirm the wording of P10 and promote it to *(maintainer)*. ### Wave 3 - misuse, false friends, and §11a curation @@ -512,5 +513,6 @@ This back-map proves §3.1a coverage. Every threat-model-shaped claim already in | [`SECURITY.md`](./SECURITY.md) | Disclosure routes through the ASF Security Team. | §1 reporting cross-reference | | [`AGENTS.md`](./AGENTS.md) | JDK 21, Groovy 4.0.x, Spring Boot 4.0.x, Jakarta EE 10, Spock 2.3. | §5 runtime assumptions | | [`README.md`](./README.md) | The framework is embedded in a user web application; not a service. | §1 description, §2 deployment context | +| [`grails-doc/.../upgrading.adoc`](./grails-doc/src/en/guide/upgrading.adoc) "XML Parsing Defaults" | Framework-parsed XML refuses external entities and external DTDs; a request body declaring a `DOCTYPE` is rejected. | §8 P10, §9 false friend | No claim in the existing documentation is dropped, weakened, or contradicted by this document. Where the existing documentation and this document would conflict, the documentation wins; raise a §14 question rather than silently editing. diff --git a/grails-core/src/main/groovy/org/apache/grails/core/plugins/PluginUtils.java b/grails-core/src/main/groovy/org/apache/grails/core/plugins/PluginUtils.java index 96c7ac51b39..c9cda0a9b28 100644 --- a/grails-core/src/main/groovy/org/apache/grails/core/plugins/PluginUtils.java +++ b/grails-core/src/main/groovy/org/apache/grails/core/plugins/PluginUtils.java @@ -138,7 +138,8 @@ public static List scanPluginDescriptorResources(ClassLoader c try { Enumeration resources = classLoader.getResources(PLUGIN_XML_PATTERN); - SAXParser saxParser = SpringIOUtils.newSAXParser(); + // descriptors on the classpath are trusted input and may declare a DOCTYPE + SAXParser saxParser = SpringIOUtils.newSAXParser(true); while (resources.hasMoreElements()) { URL url = resources.nextElement(); diff --git a/grails-core/src/test/groovy/org/apache/grails/core/plugins/PluginDiscoverySpec.groovy b/grails-core/src/test/groovy/org/apache/grails/core/plugins/PluginDiscoverySpec.groovy index 1ceaaf0ee9b..e33b4b52d75 100644 --- a/grails-core/src/test/groovy/org/apache/grails/core/plugins/PluginDiscoverySpec.groovy +++ b/grails-core/src/test/groovy/org/apache/grails/core/plugins/PluginDiscoverySpec.groovy @@ -82,6 +82,34 @@ class PluginDiscoverySpec extends Specification { tempDir.deleteDir() } + def 'reads a plugin descriptor that declares a doctype'() { + given: 'a grails-plugin.xml whose DOCTYPE names a DTD that does not exist, so retrieval would fail' + def tempDir = File.createTempDir() + def metaInfDir = new File(tempDir, 'META-INF').tap { mkdirs() } + def missingDtd = new File(tempDir, 'missing.dtd').toURI().toASCIIString() + new File(metaInfDir, 'grails-plugin.xml').text = """ + + + com.example.TestGrailsPlugin + com.example.MyDomainClass + +""" + def classLoader = new URLClassLoader([tempDir.toURI().toURL()] as URL[], (ClassLoader) null) + + when: 'plugin descriptor resources are scanned' + def descriptors = PluginUtils.scanPluginDescriptorResources(classLoader) + + then: 'the descriptor is read and the DTD is skipped rather than retrieved' + descriptors.size() == 1 + with(descriptors[0]) { + providedPlugins == ['com.example.TestGrailsPlugin'] + providedClasses == ['com.example.MyDomainClass'] + } + + cleanup: + tempDir.deleteDir() + } + def 'ignores malformed plugin descriptor XML without failing discovery'() { given: 'a classloader that returns a grails-plugin.xml with invalid content' def badXml = 'valid.Class> for the full description. +Descriptors that declare a `DOCTYPE`, such as the JSP 1.2 descriptors in `jakarta.servlet.jsp.jstl`, are read without retrieving the DTD they reference. JSTL standard library is no longer added as a dependency by default. In case you are using JSTL, you should also add these dependencies to `build.gradle`: [source,groovy] diff --git a/grails-doc/src/en/guide/upgrading.adoc b/grails-doc/src/en/guide/upgrading.adoc index 163905f03b1..3a0765d8049 100644 --- a/grails-doc/src/en/guide/upgrading.adoc +++ b/grails-doc/src/en/guide/upgrading.adoc @@ -33,19 +33,10 @@ The values `true`, `false`, `'true'`, and `'false'` are accepted for `grails.dat === XML Parsing Defaults -XML read by Grails is parsed with external entity expansion and external DTD loading disabled. This closes the XXE vector: an entity that points at a file on disk contributes nothing to the parsed document. +XML that Grails parses on an application's behalf is read with external entity expansion, external DTD retrieval and DTD grammar loading disabled. This closes the XXE vector: an entity that points at a file on disk contributes nothing to the parsed document, and a DTD that a document names is skipped rather than fetched. -Grails 8 additionally rejects `DOCTYPE` declarations outright. A request body, or any other document parsed through the framework, that carries a `DOCTYPE` is refused with a `SAXParseException` rather than parsed. +How a `DOCTYPE` declaration is treated depends on where the document comes from. -This is stricter than blocking external entities, because it also refuses documents whose `DOCTYPE` is entirely internal and harmless. Applications that must accept such documents can opt back in: +Request bodies are refused if they declare one. An `application/xml`, `text/xml` or `application/hal+xml` request body, or a string passed to `XML.parse`, that carries a `DOCTYPE` is rejected rather than bound: binding fails with the `invalidRequestBody` error code and the target is left unpopulated. This is stricter than blocking external entities, because a body whose `DOCTYPE` is entirely internal and harmless is refused too, and there is no setting that relaxes it. A client that sends a declaration, which is unusual for an API payload, needs to omit it. -[source,yaml] ----- -grails: - xml: - allowDocTypeDeclaration: true ----- - -The setting is also accepted as the `grails.xml.allowDocTypeDeclaration` system property. Opting in relaxes only whether a declaration is permitted; external general entities, external parameter entities and external DTDs stay refused either way, so it does not reopen the XXE vector. - -Descriptors read from the classpath are parsed by the same mechanism, and some carry a `DOCTYPE`. JSP tag library descriptors are the common case: `jakarta.servlet.jsp.jstl` ships several, among them `c-1_0-rt.tld`, which the default `grails.gsp.tldScanPattern` scans. An application that uses JSP tag libraries from a GSP needs this setting enabled. +Descriptors that Grails reads from the classpath may declare one. JSP tag library descriptors, `web.xml` and `grails-plugin.xml` are trusted input and routinely carry a `DOCTYPE`; `jakarta.servlet.jsp.jstl` ships several such descriptors, among them `c-1_0-rt.tld`, which the default `grails.gsp.tldScanPattern` scans. These are parsed with the declaration permitted and the entity and DTD hardening above still in force, so the DTD a descriptor names is never retrieved. The same applies to `response.xml` in a controller unit test, whose document is the controller's own output. diff --git a/grails-doc/src/en/guide/upgrading/upgrading80x.adoc b/grails-doc/src/en/guide/upgrading/upgrading80x.adoc index 6ae2803bb41..4882dc908ab 100644 --- a/grails-doc/src/en/guide/upgrading/upgrading80x.adoc +++ b/grails-doc/src/en/guide/upgrading/upgrading80x.adoc @@ -3576,3 +3576,18 @@ has used since Grails 3: the context path is compared case-insensitively althoug RFC 9110 section 4.2.3 define paths as case-sensitive, and during a `RequestDispatcher` include the included path is matched. Interceptors and the Spring Security matchers follow dispatch on both points because a matcher that disagrees with dispatch lets a request reach a controller without its interceptors or restrictions. + +==== 59. XML Request Bodies That Declare a DOCTYPE Are Refused + +XML that Grails parses on an application's behalf is now read with external entity expansion, external DTD retrieval and +DTD grammar loading disabled. Earlier releases set these parser features under identifiers the parser did not recognise, +so the parser's own defaults applied instead. + +An `application/xml`, `text/xml` or `application/hal+xml` request body, or a string passed to `XML.parse`, that declares +a `DOCTYPE` is additionally refused rather than bound, whether or not the declaration references anything external. +Binding such a body fails with the `invalidRequestBody` error code. There is no setting that relaxes this; a client that +sends a declaration needs to omit it. + +Descriptors that Grails reads from the classpath, such as JSP tag library descriptors, `web.xml` and `grails-plugin.xml`, +and the `response.xml` accessor in controller unit tests are not affected. They may declare a `DOCTYPE`, though the DTD +it names is never retrieved. See <> for the full description of the XML parsing defaults. diff --git a/grails-gradle/common/src/main/groovy/org/apache/grails/gradle/common/XmlParserFeature.java b/grails-gradle/common/src/main/groovy/org/apache/grails/gradle/common/XmlParserFeature.java index a43e730dcbe..477440aa5d7 100644 --- a/grails-gradle/common/src/main/groovy/org/apache/grails/gradle/common/XmlParserFeature.java +++ b/grails-gradle/common/src/main/groovy/org/apache/grails/gradle/common/XmlParserFeature.java @@ -42,9 +42,10 @@ public enum XmlParserFeature { * Rejects any document carrying a {@code DOCTYPE} declaration. * *

Enabling this is stricter than blocking external entities: it refuses documents whose - * DOCTYPE is entirely internal and harmless. Descriptors read from the classpath — JSP tag - * library definitions, {@code web.xml}, {@code plugin.xml} — routinely carry a DOCTYPE, so a - * parser shared with those callers must leave this disabled. + * DOCTYPE is entirely internal and harmless. It suits untrusted input such as HTTP request + * bodies. Descriptors read from the classpath — JSP tag library definitions, {@code web.xml}, + * {@code grails-plugin.xml} — routinely carry a DOCTYPE, so their readers need a parser that + * leaves this disabled while keeping the entity and DTD features below switched off. */ DISALLOW_DOCTYPE_DECL("http://apache.org/xml/features/disallow-doctype-decl"), diff --git a/grails-gradle/model/src/main/groovy/org/grails/io/support/SpringIOUtils.java b/grails-gradle/model/src/main/groovy/org/grails/io/support/SpringIOUtils.java index bea3ed946f6..20ef4ce3048 100644 --- a/grails-gradle/model/src/main/groovy/org/grails/io/support/SpringIOUtils.java +++ b/grails-gradle/model/src/main/groovy/org/grails/io/support/SpringIOUtils.java @@ -47,9 +47,9 @@ import groovy.xml.FactorySupport; import groovy.xml.XmlSlurper; +import org.slf4j.LoggerFactory; import org.xml.sax.SAXException; -import grails.util.Metadata; import org.apache.grails.gradle.common.XmlParserFeature; /** @@ -410,39 +410,72 @@ public static String copyToString(Reader in) throws IOException { return out.toString(); } + /** + * Creates an {@link XmlSlurper} for untrusted documents such as HTTP request bodies. + * + *

The parser refuses a {@code DOCTYPE} declaration outright, on top of the entity and DTD + * hardening every parser handed out by this class applies. Readers of trusted descriptors that + * declare one use {@link #createXmlSlurper(boolean)} with {@code true} instead. + * + * @return a namespace-aware, non-validating slurper that rejects {@code DOCTYPE} declarations + */ public static XmlSlurper createXmlSlurper() throws ParserConfigurationException, SAXException { - return new XmlSlurper(newSAXParser()); + return createXmlSlurper(false); } - public static SAXParser newSAXParser() throws ParserConfigurationException, SAXException { - SAXParserFactory factory = createParserFactory(); - return factory.newSAXParser(); + /** + * Creates an {@link XmlSlurper}, optionally tolerating a {@code DOCTYPE} declaration. + * + *

Documents parsed through this class fall into two trust levels, and each level gets its + * own parser. Request bodies are untrusted and are refused if they declare a {@code DOCTYPE}; + * that is the {@code false} form and the default. Descriptors read from the application + * classpath, such as JSP tag library descriptors, {@code web.xml} and + * {@code grails-plugin.xml}, are trusted and routinely declare one, so their readers pass + * {@code true}. + * + *

Tolerating the declaration does not reopen the XXE vector. External general entities, + * external parameter entities, DTD grammar loading and external DTD retrieval stay off on both + * parsers, so an entity that points at a file on disk contributes nothing and the DTD a + * document names is skipped rather than fetched. + * + * @param allowDocTypeDeclaration {@code true} to parse documents that declare a {@code DOCTYPE} + * @return a namespace-aware, non-validating slurper with the hardening described above + * @since 8.0.0 + */ + public static XmlSlurper createXmlSlurper(boolean allowDocTypeDeclaration) throws ParserConfigurationException, SAXException { + return new XmlSlurper(newSAXParser(allowDocTypeDeclaration)); } /** - * Configuration key permitting {@code DOCTYPE} declarations in documents parsed by this class. + * Creates a {@link SAXParser} for untrusted documents such as HTTP request bodies. + * + *

The parser refuses a {@code DOCTYPE} declaration outright; see + * {@link #createXmlSlurper(boolean)} for the two trust levels. * - *

Parsers handed out here reject a {@code DOCTYPE} by default. Set - * {@code grails.xml.allowDocTypeDeclaration} to {@code true} in {@code application.yml}, or as - * a system property, to accept one. + * @return a namespace-aware, non-validating parser that rejects {@code DOCTYPE} declarations + */ + public static SAXParser newSAXParser() throws ParserConfigurationException, SAXException { + return newSAXParser(false); + } + + /** + * Creates a {@link SAXParser}, optionally tolerating a {@code DOCTYPE} declaration. * - *

Opting in does not reopen the XXE vector. External general entities, external parameter - * entities and external DTDs stay refused whichever way this is set, so an entity pointing at - * a file on disk still contributes nothing. What opting in changes is only whether a document - * carrying a declaration is refused outright. + *

Applies the same hardening as {@link #createXmlSlurper(boolean)}. * - *

It exists because these parsers also read trusted descriptors from the classpath, and - * some of those carry a {@code DOCTYPE}. JSP tag library descriptors are the common case: - * {@code jakarta.servlet.jsp.jstl} ships several, among them {@code c-1_0-rt.tld}, which the - * default {@code grails.gsp.tldScanPattern} scans. + * @param allowDocTypeDeclaration {@code true} to parse documents that declare a {@code DOCTYPE} + * @return a namespace-aware, non-validating parser + * @since 8.0.0 */ - public static final String ALLOW_DOCTYPE_DECLARATION = "grails.xml.allowDocTypeDeclaration"; + public static SAXParser newSAXParser(boolean allowDocTypeDeclaration) throws ParserConfigurationException, SAXException { + return parserFactory(allowDocTypeDeclaration).newSAXParser(); + } /** * Parser features switched off for every parser this class hands out. * *

{@link XmlParserFeature#DISALLOW_DOCTYPE_DECL} is handled separately because it is the - * one feature an application may turn off; see {@link #ALLOW_DOCTYPE_DECLARATION}. + * one feature that differs between the two parsers; see {@link #createXmlSlurper(boolean)}. */ private static final XmlParserFeature[] DISABLED_PARSER_FEATURES = { XmlParserFeature.EXTERNAL_GENERAL_ENTITIES, @@ -451,26 +484,25 @@ public static SAXParser newSAXParser() throws ParserConfigurationException, SAXE XmlParserFeature.LOAD_EXTERNAL_DTD }; - private static SAXParserFactory strictParserFactory = null; + private static volatile SAXParserFactory strictParserFactory; - private static SAXParserFactory docTypeParserFactory = null; + private static volatile SAXParserFactory docTypeParserFactory; - private static SAXParserFactory createParserFactory() throws ParserConfigurationException { - if (isDocTypeDeclarationAllowed()) { - if (docTypeParserFactory == null) { - docTypeParserFactory = buildParserFactory(true); + private static SAXParserFactory parserFactory(boolean allowDocTypeDeclaration) throws ParserConfigurationException { + if (allowDocTypeDeclaration) { + SAXParserFactory factory = docTypeParserFactory; + if (factory == null) { + factory = buildParserFactory(true); + docTypeParserFactory = factory; } - return docTypeParserFactory; + return factory; } - if (strictParserFactory == null) { - strictParserFactory = buildParserFactory(false); + SAXParserFactory factory = strictParserFactory; + if (factory == null) { + factory = buildParserFactory(false); + strictParserFactory = factory; } - return strictParserFactory; - } - - private static boolean isDocTypeDeclarationAllowed() { - return Boolean.TRUE.equals( - Metadata.getCurrent().getProperty(ALLOW_DOCTYPE_DECLARATION, Boolean.class, Boolean.FALSE)); + return factory; } private static SAXParserFactory buildParserFactory(boolean allowDocTypeDeclaration) throws ParserConfigurationException { @@ -480,25 +512,32 @@ private static SAXParserFactory buildParserFactory(boolean allowDocTypeDeclarati try { factory.setXIncludeAware(false); } catch (UnsupportedOperationException e) { - // ignore, parser doesn't support - } - try { - factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); - } catch (Exception e) { - // ignore, parser doesn't support - } - try { - factory.setFeature(XmlParserFeature.DISALLOW_DOCTYPE_DECL.getFeatureName(), !allowDocTypeDeclaration); - } catch (Exception e) { - // ignore, parser doesn't support + // a parser without XInclude support cannot expand an include either } + setFeature(factory, XMLConstants.FEATURE_SECURE_PROCESSING, true); + setFeature(factory, XmlParserFeature.DISALLOW_DOCTYPE_DECL.getFeatureName(), !allowDocTypeDeclaration); for (XmlParserFeature feature : DISABLED_PARSER_FEATURES) { - try { - factory.setFeature(feature.getFeatureName(), false); - } catch (Exception e) { - // ignore, parser doesn't support - } + setFeature(factory, feature.getFeatureName(), false); } return factory; } + + /** + * Sets a feature, tolerating a parser that lacks it. + * + *

The tolerance keeps this class usable with any SAX provider, but it is also how an + * unrecognised feature identifier once switched the hardening off without a trace. A parser + * that rejects a feature is therefore reported rather than ignored. The logger is looked up + * here rather than held in a static field because this class is used before logging is + * configured during startup. + */ + private static void setFeature(SAXParserFactory factory, String name, boolean value) { + try { + factory.setFeature(name, value); + } catch (ParserConfigurationException | SAXException e) { + LoggerFactory.getLogger(SpringIOUtils.class).warn( + "XML parser factory [{}] does not support feature [{}]: {}", + factory.getClass().getName(), name, e.getMessage()); + } + } } diff --git a/grails-gradle/model/src/test/groovy/org/grails/io/support/SpringIOUtilsSpec.groovy b/grails-gradle/model/src/test/groovy/org/grails/io/support/SpringIOUtilsSpec.groovy index 7c4bed55d06..c3afb02351b 100644 --- a/grails-gradle/model/src/test/groovy/org/grails/io/support/SpringIOUtilsSpec.groovy +++ b/grails-gradle/model/src/test/groovy/org/grails/io/support/SpringIOUtilsSpec.groovy @@ -21,9 +21,8 @@ package org.grails.io.support import java.nio.file.Files import java.nio.file.Path -import grails.util.Metadata - import org.xml.sax.SAXParseException +import org.xml.sax.helpers.DefaultHandler import spock.lang.Specification import spock.lang.TempDir @@ -36,6 +35,11 @@ import spock.lang.TempDir * the feature identifiers, so a search-and-replace over those identifiers would rewrite the * production code and this spec together and the suite would still pass. Driving real documents * through the parser keeps the assertions independent of how the hardening is spelled. + * + *

Two parsers are handed out. The strict one, which every no-argument method returns, is for + * untrusted input and refuses a {@code DOCTYPE}. The tolerant one, requested with {@code true}, is + * for trusted descriptors that declare one; it is the parser that can be driven past the + * declaration, so it is the one the entity and DTD assertions run against. */ class SpringIOUtilsSpec extends Specification { @@ -48,17 +52,29 @@ class SpringIOUtilsSpec extends Specification { outorg.example.OutTag ''' + private static final String SECRET = 'top-secret-token' + @TempDir Path tempDir - void cleanup() { - System.clearProperty(SpringIOUtils.ALLOW_DOCTYPE_DECLARATION) - Metadata.reset() + private String externalEntityDocument() { + Path secret = tempDir.resolve('secret.txt') + Files.writeString(secret, SECRET) + """ +]> +&ext;""" } - private static void allowDocTypeDeclarations() { - System.setProperty(SpringIOUtils.ALLOW_DOCTYPE_DECLARATION, 'true') - Metadata.reset() + private static String parseWithSaxParser(javax.xml.parsers.SAXParser parser, String xml) { + StringBuilder text = new StringBuilder() + parser.parse(new ByteArrayInputStream(xml.getBytes('UTF-8')), new DefaultHandler() { + @Override + void characters(char[] chars, int start, int length) { + text.append(chars, start, length) + } + }) + text.toString() } void 'createXmlSlurper parses a document without a doctype'() { @@ -74,7 +90,8 @@ class SpringIOUtilsSpec extends Specification { SpringIOUtils.createXmlSlurper().parseText(TLD) then: - thrown(SAXParseException) + SAXParseException e = thrown() + e.message.contains('DOCTYPE is disallowed') } void 'createXmlSlurper rejects an internal doctype subset by default'() { @@ -85,72 +102,93 @@ class SpringIOUtilsSpec extends Specification { &msg;''') then: - thrown(SAXParseException) + SAXParseException e = thrown() + e.message.contains('DOCTYPE is disallowed') } - void 'the doctype configuration key lets an application parse descriptors that declare one'() { - given: 'an application.yml opting in, as an application would configure it' - Metadata.getInstance(new ByteArrayInputStream('''grails: - xml: - allowDocTypeDeclaration: true -'''.getBytes('UTF-8'))) + void 'declining doctype tolerance explicitly is the default'() { + when: + SpringIOUtils.createXmlSlurper(false).parseText(TLD) + then: + SAXParseException e = thrown() + e.message.contains('DOCTYPE is disallowed') + } + + void 'newSAXParser rejects a doctype declaration by default'() { + when: + parseWithSaxParser(SpringIOUtils.newSAXParser(), TLD) + + then: + SAXParseException e = thrown() + e.message.contains('DOCTYPE is disallowed') + } + + void 'asking for doctype tolerance parses a descriptor that declares one'() { when: - def parsed = SpringIOUtils.createXmlSlurper().parseText(TLD) + def parsed = SpringIOUtils.createXmlSlurper(true).parseText(TLD) - then: 'the descriptor is readable' + then: parsed.uri.text() == 'jakarta.tags.core' parsed.tag.name.text() == 'out' } - void 'external entities stay blocked when doctype declarations are permitted'() { + void 'the doctype-tolerant slurper does not resolve external general entities'() { given: 'a document whose entity points at a readable file on disk' - allowDocTypeDeclarations() - Path secret = tempDir.resolve('secret.txt') - Files.writeString(secret, 'top-secret-token') + String xml = externalEntityDocument() + + when: + def parsed = SpringIOUtils.createXmlSlurper(true).parseText(xml) + + then: 'tolerating the declaration does not reopen the XXE vector' + !parsed.text().contains(SECRET) + } + + void 'the doctype-tolerant slurper does not resolve external parameter entities'() { + given: 'a parameter entity that would pull a file into the internal subset' + Path secret = tempDir.resolve('secret.dtd') + Files.writeString(secret, "") String xml = """ + +%ext; ]> -&ext;""" +ok""" when: - def parsed = SpringIOUtils.createXmlSlurper().parseText(xml) + def parsed = SpringIOUtils.createXmlSlurper(true).parseText(xml) - then: 'relaxing the doctype rule does not reopen the XXE vector' - !parsed.text().contains('top-secret-token') + then: + parsed.text() == 'ok' } - void 'external dtds are skipped rather than retrieved when doctype declarations are permitted'() { - given: - allowDocTypeDeclarations() + void 'the doctype-tolerant slurper skips an external dtd rather than retrieving it'() { + given: 'a document naming a DTD that does not exist, so retrieval would fail loudly' String xml = """ ok""" expect: - SpringIOUtils.createXmlSlurper().parseText(xml).text() == 'ok' + SpringIOUtils.createXmlSlurper(true).parseText(xml).text() == 'ok' } - void 'newSAXParser applies the same hardening as createXmlSlurper'() { + void 'the doctype-tolerant sax parser applies the same entity hardening'() { given: - allowDocTypeDeclarations() - Path secret = tempDir.resolve('secret.txt') - Files.writeString(secret, 'top-secret-token') - String xml = """ -]> -&ext;""" - StringBuilder text = new StringBuilder() + String xml = externalEntityDocument() when: - SpringIOUtils.newSAXParser().parse(new ByteArrayInputStream(xml.getBytes('UTF-8')), - new org.xml.sax.helpers.DefaultHandler() { - @Override - void characters(char[] chars, int start, int length) { - text.append(chars, start, length) - } - }) + String text = parseWithSaxParser(SpringIOUtils.newSAXParser(true), xml) then: - !text.toString().contains('top-secret-token') + !text.contains(SECRET) + } + + void 'both parsers are namespace aware'() { + given: + String xml = 'ok' + + expect: + SpringIOUtils.createXmlSlurper(allowDocType).parseText(xml).child.text() == 'ok' + + where: + allowDocType << [false, true] } } diff --git a/grails-gsp/grails-web-jsp/src/main/groovy/org/grails/gsp/jsp/TldReader.groovy b/grails-gsp/grails-web-jsp/src/main/groovy/org/grails/gsp/jsp/TldReader.groovy index 1c4bc74569b..a2cc00596df 100644 --- a/grails-gsp/grails-web-jsp/src/main/groovy/org/grails/gsp/jsp/TldReader.groovy +++ b/grails-gsp/grails-web-jsp/src/main/groovy/org/grails/gsp/jsp/TldReader.groovy @@ -46,7 +46,8 @@ class TldReader { @CompileStatic(TypeCheckingMode.SKIP) private init(InputStream inputStream) { - def rootNode = SpringIOUtils.createXmlSlurper().parse(inputStream) + // a descriptor on the classpath is trusted input and may declare a DOCTYPE + def rootNode = SpringIOUtils.createXmlSlurper(true).parse(inputStream) uri = rootNode.uri.text() rootNode.tag.each { tag -> String tagName = tag.name.text() diff --git a/grails-gsp/grails-web-jsp/src/main/groovy/org/grails/gsp/jsp/WebXmlTagLibraryReader.groovy b/grails-gsp/grails-web-jsp/src/main/groovy/org/grails/gsp/jsp/WebXmlTagLibraryReader.groovy index 4ceaf4ca804..bd097b3a3cb 100644 --- a/grails-gsp/grails-web-jsp/src/main/groovy/org/grails/gsp/jsp/WebXmlTagLibraryReader.groovy +++ b/grails-gsp/grails-web-jsp/src/main/groovy/org/grails/gsp/jsp/WebXmlTagLibraryReader.groovy @@ -44,7 +44,8 @@ class WebXmlTagLibraryReader { @CompileStatic(TypeCheckingMode.SKIP) private init(InputStream inputStream) { - def rootNode = SpringIOUtils.createXmlSlurper().parse(inputStream) + // web.xml is trusted input and may declare a DOCTYPE + def rootNode = SpringIOUtils.createXmlSlurper(true).parse(inputStream) rootNode.taglib.each { taglib -> String uri = taglib.'taglib-uri'.text() String location = taglib.'taglib-location'.text() diff --git a/grails-gsp/plugin/src/test/groovy/org/grails/gsp/jsp/JstlDocTypeTldSpec.groovy b/grails-gsp/plugin/src/test/groovy/org/grails/gsp/jsp/JstlDocTypeTldSpec.groovy new file mode 100644 index 00000000000..b92b48b21dd --- /dev/null +++ b/grails-gsp/plugin/src/test/groovy/org/grails/gsp/jsp/JstlDocTypeTldSpec.groovy @@ -0,0 +1,72 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.grails.gsp.jsp + +import grails.core.DefaultGrailsApplication +import org.springframework.core.io.DefaultResourceLoader +import org.springframework.mock.web.MockServletContext +import spock.lang.Specification +import spock.lang.Unroll + +/** + * Scans the JSTL descriptors that declare a JSP 1.2 {@code DOCTYPE}, as shipped in + * {@code jakarta.servlet.jsp.jstl}. The default {@code grails.gsp.tldScanPattern} ends with + * {@code c-1_0-rt.tld}, which is one of them, and the resolver scans every pattern in one pass, so + * a parser that refused the declaration would leave no JSP tag library resolvable in any + * application that adds JSTL. + */ +class JstlDocTypeTldSpec extends Specification { + + private TagLibraryResolverImpl resolverScanning(String... patterns) { + def resolver = new TagLibraryResolverImpl() + resolver.servletContext = new MockServletContext() + resolver.grailsApplication = new DefaultGrailsApplication() + resolver.tldScanPatterns = patterns + resolver.resourceLoader = new DefaultResourceLoader(this.class.classLoader) + resolver + } + + void 'a descriptor declaring a doctype does not stop the scan'() { + given: 'the default scan order: the schema-based c.tld first, then the JSP 1.2 c-1_0-rt.tld' + def resolver = resolverScanning('classpath*:/META-INF/c.tld', 'classpath*:/META-INF/c-1_0-rt.tld') + + expect: 'the descriptor scanned before the declaration still resolves' + resolver.resolveTagLibrary('jakarta.tags.core')?.getTag('out') + + and: 'so does the descriptor that declares it' + resolver.resolveTagLibrary('http://java.sun.com/jstl/core_rt')?.getTag('out') + } + + @Unroll + void 'the JSP 1.2 descriptor for #uri resolves'() { + given: + def resolver = resolverScanning( + 'classpath*:/META-INF/c-1_0*.tld', + 'classpath*:/META-INF/fmt-1_0*.tld', + 'classpath*:/META-INF/sql-1_0*.tld', + 'classpath*:/META-INF/x-1_0*.tld') + + expect: + resolver.resolveTagLibrary(uri) + + where: + uri << ['core', 'core_rt', 'fmt', 'fmt_rt', 'sql', 'sql_rt', 'xml', 'xml_rt'] + .collect { "http://java.sun.com/jstl/$it".toString() } + } +} diff --git a/grails-gsp/plugin/src/test/groovy/org/grails/gsp/jsp/TldReaderTests.groovy b/grails-gsp/plugin/src/test/groovy/org/grails/gsp/jsp/TldReaderTests.groovy index c20e5b09ed8..5220d17e6e7 100644 --- a/grails-gsp/plugin/src/test/groovy/org/grails/gsp/jsp/TldReaderTests.groovy +++ b/grails-gsp/plugin/src/test/groovy/org/grails/gsp/jsp/TldReaderTests.groovy @@ -23,6 +23,7 @@ import org.junit.jupiter.api.Test import org.springframework.core.io.ClassPathResource import static org.junit.jupiter.api.Assertions.assertEquals +import static org.junit.jupiter.api.Assertions.assertFalse class TldReaderTests { @@ -36,4 +37,59 @@ class TldReaderTests { assert tldReader.tags assertEquals tldReader.tags.localeSelect, JspLocaleSelectTag.class.name } + + /** + * JSP 1.2 descriptors declare a DOCTYPE. JSTL ships several, among them {@code c-1_0-rt.tld}, + * which the default {@code grails.gsp.tldScanPattern} scans, so the reader has to accept one. + */ + @Test + void testTldReaderAcceptsDescriptorDeclaringDoctype() { + def tld = ''' + + + 1.0 + 1.2 + c_rt + http://java.sun.com/jstl/core_rt + + out + org.apache.taglibs.standard.tag.rt.core.OutTag + JSP + +''' + + TldReader tldReader = new TldReader(new ByteArrayInputStream(tld.getBytes('ISO-8859-1'))) + + assertEquals 'http://java.sun.com/jstl/core_rt', tldReader.uri + assertEquals 'org.apache.taglibs.standard.tag.rt.core.OutTag', tldReader.tags.out + } + + /** + * Accepting the declaration must not reopen the XXE vector: an entity pointing at a file on + * disk contributes nothing to the descriptor. + */ + @Test + void testTldReaderDoesNotResolveExternalEntities() { + File secret = File.createTempFile('tld-reader-secret', '.txt') + try { + secret.text = 'top-secret-token' + def tld = """ +]> + + &ext; + outorg.example.OutTag +""" + + TldReader tldReader = new TldReader(new ByteArrayInputStream(tld.getBytes('UTF-8'))) + + assertFalse tldReader.uri.contains('top-secret-token') + assertEquals 'org.example.OutTag', tldReader.tags.out + } + finally { + secret.delete() + } + } } diff --git a/grails-gsp/plugin/src/test/groovy/org/grails/gsp/jsp/WebXmlTagLibraryReaderTests.groovy b/grails-gsp/plugin/src/test/groovy/org/grails/gsp/jsp/WebXmlTagLibraryReaderTests.groovy index 08593951969..0dc518dd942 100644 --- a/grails-gsp/plugin/src/test/groovy/org/grails/gsp/jsp/WebXmlTagLibraryReaderTests.groovy +++ b/grails-gsp/plugin/src/test/groovy/org/grails/gsp/jsp/WebXmlTagLibraryReaderTests.groovy @@ -33,6 +33,26 @@ class WebXmlTagLibraryReaderTests extends Specification { webXmlReader.tagLocations['jakarta.tags.core'] == '/WEB-INF/tld/c.tld' } + void 'a web.xml declaring a doctype is read without retrieving the dtd'() { + given: 'a Servlet 2.3 descriptor, whose DOCTYPE names a DTD that must never be fetched' + def webXml = ''' + + + + http://java.sun.com/jstl/core + /WEB-INF/tld/c.tld + + +''' + + when: + WebXmlTagLibraryReader webXmlReader = new WebXmlTagLibraryReader(new ByteArrayInputStream(webXml.getBytes('UTF-8'))) + + then: + webXmlReader.tagLocations == ['http://java.sun.com/jstl/core': '/WEB-INF/tld/c.tld'] + } + def testWebXml = '''\ | |The body is the controller's own output rather than untrusted input, so a + * {@code DOCTYPE} declaration is accepted. External entities and external DTDs are still + * not resolved. * * @return The response XML */ GPathResult getXml() { - SpringIOUtils.createXmlSlurper().parseText(contentAsString) + SpringIOUtils.createXmlSlurper(true).parseText(contentAsString) } /** diff --git a/grails-test-suite-uber/src/test/groovy/org/grails/plugins/testing/GrailsMockHttpServletResponseTests.groovy b/grails-test-suite-uber/src/test/groovy/org/grails/plugins/testing/GrailsMockHttpServletResponseTests.groovy index 43413f88c28..a4d06460ffc 100644 --- a/grails-test-suite-uber/src/test/groovy/org/grails/plugins/testing/GrailsMockHttpServletResponseTests.groovy +++ b/grails-test-suite-uber/src/test/groovy/org/grails/plugins/testing/GrailsMockHttpServletResponseTests.groovy @@ -22,6 +22,7 @@ import org.grails.plugins.testing.GrailsMockHttpServletResponse import org.junit.jupiter.api.Test import static org.junit.jupiter.api.Assertions.assertEquals +import static org.junit.jupiter.api.Assertions.assertFalse /** * Test case for {@link org.grails.plugins.testing.GrailsMockHttpServletResponse}. @@ -42,4 +43,36 @@ class GrailsMockHttpServletResponseTests { testResponse << "\nand another line" assertEquals "Some string or other\nand another line", testResponse.contentAsString } + + /** + * The body is the controller's own output, so a DOCTYPE it renders is accepted. The DTD the + * declaration names is never retrieved. + */ + @Test + void testXmlAcceptsDoctypeInRenderedOutput() { + def testResponse = new GrailsMockHttpServletResponse() + testResponse << ''' +

hello

''' + + assertEquals 'hello', testResponse.xml.body.p.text() + } + + @Test + void testXmlDoesNotResolveExternalEntities() { + File secret = File.createTempFile('mock-response-secret', '.txt') + try { + secret.text = 'top-secret-token' + def testResponse = new GrailsMockHttpServletResponse() + testResponse << """ +]> +&ext;""" + + assertFalse testResponse.xml.text().contains('top-secret-token') + } + finally { + secret.delete() + } + } } diff --git a/grails-testing-support-http-client/src/main/groovy/org/apache/grails/testing/http/client/utils/XmlUtils.groovy b/grails-testing-support-http-client/src/main/groovy/org/apache/grails/testing/http/client/utils/XmlUtils.groovy index fcabc64d861..8fb4bf1d489 100644 --- a/grails-testing-support-http-client/src/main/groovy/org/apache/grails/testing/http/client/utils/XmlUtils.groovy +++ b/grails-testing-support-http-client/src/main/groovy/org/apache/grails/testing/http/client/utils/XmlUtils.groovy @@ -31,6 +31,7 @@ import groovy.transform.CompileStatic import groovy.transform.Immutable import groovy.transform.NamedDelegate import groovy.transform.NamedVariant +import groovy.util.logging.Slf4j import groovy.xml.FactorySupport import groovy.xml.MarkupBuilder import groovy.xml.XmlSlurper @@ -44,6 +45,7 @@ import org.apache.grails.gradle.common.XmlParserFeature * * @since 7.0.10 */ +@Slf4j @CompileStatic class XmlUtils { @@ -113,7 +115,8 @@ class XmlUtils { /** * Creates an {@link XmlSlurper} with secure defaults. *

- * The default parser is namespace aware, non-validating, and rejects DOCTYPE declarations. + * The default parser is namespace aware, non-validating, rejects DOCTYPE declarations, and disables + * external entity expansion plus external DTD loading. * * @param slurperConfig optional XML parser configuration or custom factory * @return configured {@link XmlSlurper} @@ -231,8 +234,11 @@ class XmlUtils { try { saxParserFactory.setFeature(feature, enabled) } - catch (Exception ignored) { - // ignore, parser doesn't support + catch (ParserConfigurationException | SAXException e) { + // tolerated so any SAX provider works, but reported: an unrecognised feature identifier + // once switched this hardening off without a trace + log.warn('XML parser factory [{}] does not support feature [{}]: {}', + saxParserFactory.class.name, feature, e.message) } } diff --git a/grails-testing-support-http-client/src/test/groovy/org/apache/grails/testing/http/client/TestHttpResponseSpec.groovy b/grails-testing-support-http-client/src/test/groovy/org/apache/grails/testing/http/client/TestHttpResponseSpec.groovy index 2a789c9f428..f4a171fa2ca 100644 --- a/grails-testing-support-http-client/src/test/groovy/org/apache/grails/testing/http/client/TestHttpResponseSpec.groovy +++ b/grails-testing-support-http-client/src/test/groovy/org/apache/grails/testing/http/client/TestHttpResponseSpec.groovy @@ -201,15 +201,16 @@ class TestHttpResponseSpec extends Specification { void 'xml rejects doctype declarations with external entities'() { given: def response = mockResponse(200, ''' - ]> + +]> &ext;''') when: response.xml() then: - thrown(SAXParseException) + SAXParseException e = thrown() + e.message.contains('DOCTYPE is disallowed') } void 'xml rejects doctype declarations with internal entities'() { @@ -223,7 +224,8 @@ class TestHttpResponseSpec extends Specification { response.xml() then: - thrown(SAXParseException) + SAXParseException e = thrown() + e.message.contains('DOCTYPE is disallowed') } void 'withXmlSlurper allows overriding the parser without mutating the original wrapper'() { diff --git a/grails-testing-support-http-client/src/test/groovy/org/apache/grails/testing/http/client/utils/XmlUtilsSpec.groovy b/grails-testing-support-http-client/src/test/groovy/org/apache/grails/testing/http/client/utils/XmlUtilsSpec.groovy index e7064d4b98d..9b7948357b8 100644 --- a/grails-testing-support-http-client/src/test/groovy/org/apache/grails/testing/http/client/utils/XmlUtilsSpec.groovy +++ b/grails-testing-support-http-client/src/test/groovy/org/apache/grails/testing/http/client/utils/XmlUtilsSpec.groovy @@ -278,24 +278,26 @@ class XmlUtilsSpec extends Specification { void 'newXmlSlurper rejects doctype declarations with external entities'() { when: - def parsed = XmlUtils.newXmlSlurper().parseText(''' - ]> + XmlUtils.newXmlSlurper().parseText(''' +]> &ext;''') then: - thrown(SAXParseException) + SAXParseException e = thrown() + e.message.contains('DOCTYPE is disallowed') } void 'newXmlSlurper rejects doctype declarations with internal entities'() { when: - def parsed = XmlUtils.newXmlSlurper().parseText(''' ]> &msg;''') then: - thrown(SAXParseException) + SAXParseException e = thrown() + e.message.contains('DOCTYPE is disallowed') } void 'newXmlSlurper supports custom factory overrides'() { diff --git a/grails-web-databinding/src/test/groovy/org/grails/web/databinding/bindingsource/XmlDataBindingSourceCreatorSpec.groovy b/grails-web-databinding/src/test/groovy/org/grails/web/databinding/bindingsource/XmlDataBindingSourceCreatorSpec.groovy new file mode 100644 index 00000000000..d8e70f956f2 --- /dev/null +++ b/grails-web-databinding/src/test/groovy/org/grails/web/databinding/bindingsource/XmlDataBindingSourceCreatorSpec.groovy @@ -0,0 +1,78 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.grails.web.databinding.bindingsource + +import grails.web.mime.MimeType + +import org.grails.databinding.bindingsource.DataBindingSourceCreationException +import org.xml.sax.SAXParseException + +import spock.lang.Specification +import spock.lang.Unroll + +/** + * XML request bodies are untrusted input, so they are parsed with the strict parser: a body that + * declares a {@code DOCTYPE} is refused before anything is bound, whether or not the declaration + * references anything external. + */ +class XmlDataBindingSourceCreatorSpec extends Specification { + + void 'an xml request body binds its elements'() { + given: + def creator = new XmlDataBindingSourceCreator() + + when: + def source = creator.createDataBindingSource(MimeType.XML, Object, + new StringReader('Grails')) + + then: + source.getPropertyValue('title') == 'Grails' + } + + @Unroll + void 'a request body declaring a doctype is refused by #creator.class.simpleName'() { + when: + creator.createDataBindingSource(creator.mimeTypes[0], Object, new StringReader(''' +]> +&title;''')) + + then: + InvalidRequestBodyException e = thrown() + e.cause instanceof SAXParseException + e.cause.message.contains('DOCTYPE is disallowed') + + where: + creator << [new XmlDataBindingSourceCreator(), new HalXmlDataBindingSourceCreator()] + } + + void 'a collection request body declaring a doctype is refused'() { + given: + def creator = new XmlDataBindingSourceCreator() + + when: + creator.createCollectionDataBindingSource(MimeType.XML, Object, + new StringReader('Grails')) + + then: + DataBindingSourceCreationException e = thrown() + e.cause instanceof SAXParseException + e.cause.message.contains('DOCTYPE is disallowed') + } +} diff --git a/threat-model.yaml b/threat-model.yaml index c32479b1559..32b025be9f7 100644 --- a/threat-model.yaml +++ b/threat-model.yaml @@ -182,7 +182,7 @@ entry_points: - surface: Controller.request.XML parameter: body attacker_controllable: true - notes: "XXE hardening is the parser's responsibility (§9 false friend)." + notes: "Parsed with XXE hardening: external entities and external DTDs refused, DOCTYPE rejected (§8 P10)." - surface: bindData parameter: source attacker_controllable: true @@ -304,6 +304,14 @@ properties_provided: severity: resource_bug provenance: inferred open_question: "§14 wave 2" + - id: P10 + description: "XML the framework parses is XXE-hardened: external general and parameter entities, external DTDs and DTD grammars are refused, and a request body declaring a DOCTYPE is rejected." + cwe: CWE-611 + conditions: "Document is parsed by the framework's XML data binding (application/xml, text/xml, application/hal+xml) or by XML.parse; a parser the application constructs itself gets the JDK defaults." + violation_symptom: "Content of an external entity appears in bound data, or a body carrying a DOCTYPE is bound." + severity: security_critical + provenance: documented + source: grails-doc/src/en/guide/upgrading.adoc # §9 - properties the framework does NOT provide. properties_disclaimed: @@ -368,6 +376,9 @@ false_friends: - id: grails_config_locations_as_config looks_like: "Configuration-file path." actually_is: "Groovy code-execution path - .groovy files are evaluated via ConfigSlurper. A file the application can read and an attacker can write is equivalent to classpath compromise." + - id: xml_hardening_scope + looks_like: "XXE protection for all XML parsing in the application." + actually_is: "Hardening of the parsers the framework uses for request bodies and classpath descriptors (§8 P10). An XmlSlurper or XmlParser the application constructs itself gets the JDK defaults." # §11a - recurring false positives that automated triage should suppress or treat as KNOWN-NON-FINDING. known_non_findings: From 890833e9c42078c531a260664248ee9ccdee6a84 Mon Sep 17 00:00:00 2001 From: James Daugherty Date: Fri, 11 Sep 2026 09:57:41 -0400 Subject: [PATCH 5/6] strictly parse plugin config always --- .../grails/core/plugins/PluginUtils.java | 5 ++- .../core/plugins/PluginDiscoverySpec.groovy | 40 ++++++++++--------- ...ailsClassInjectorTransformationSpec.groovy | 39 ++++++++++++++++++ grails-doc/src/en/guide/upgrading.adoc | 4 +- .../src/en/guide/upgrading/upgrading80x.adoc | 9 +++-- .../gradle/common/XmlParserFeature.java | 6 +-- .../org/grails/io/support/SpringIOUtils.java | 5 +-- 7 files changed, 78 insertions(+), 30 deletions(-) diff --git a/grails-core/src/main/groovy/org/apache/grails/core/plugins/PluginUtils.java b/grails-core/src/main/groovy/org/apache/grails/core/plugins/PluginUtils.java index c9cda0a9b28..83d618feab0 100644 --- a/grails-core/src/main/groovy/org/apache/grails/core/plugins/PluginUtils.java +++ b/grails-core/src/main/groovy/org/apache/grails/core/plugins/PluginUtils.java @@ -138,8 +138,9 @@ public static List scanPluginDescriptorResources(ClassLoader c try { Enumeration resources = classLoader.getResources(PLUGIN_XML_PATTERN); - // descriptors on the classpath are trusted input and may declare a DOCTYPE - SAXParser saxParser = SpringIOUtils.newSAXParser(true); + // Grails generates this descriptor and never writes a DOCTYPE, so it is read with the + // strict parser, matching the compile-time transform that generates and rewrites it + SAXParser saxParser = SpringIOUtils.newSAXParser(); while (resources.hasMoreElements()) { URL url = resources.nextElement(); diff --git a/grails-core/src/test/groovy/org/apache/grails/core/plugins/PluginDiscoverySpec.groovy b/grails-core/src/test/groovy/org/apache/grails/core/plugins/PluginDiscoverySpec.groovy index e33b4b52d75..8948786c8b5 100644 --- a/grails-core/src/test/groovy/org/apache/grails/core/plugins/PluginDiscoverySpec.groovy +++ b/grails-core/src/test/groovy/org/apache/grails/core/plugins/PluginDiscoverySpec.groovy @@ -82,32 +82,36 @@ class PluginDiscoverySpec extends Specification { tempDir.deleteDir() } - def 'reads a plugin descriptor that declares a doctype'() { - given: 'a grails-plugin.xml whose DOCTYPE names a DTD that does not exist, so retrieval would fail' - def tempDir = File.createTempDir() - def metaInfDir = new File(tempDir, 'META-INF').tap { mkdirs() } - def missingDtd = new File(tempDir, 'missing.dtd').toURI().toASCIIString() - new File(metaInfDir, 'grails-plugin.xml').text = """ - - - com.example.TestGrailsPlugin - com.example.MyDomainClass + def 'skips a plugin descriptor that declares a doctype'() { + given: 'a descriptor that declares a DOCTYPE, which Grails never generates' + def doctypeDir = File.createTempDir() + def doctypeMetaInf = new File(doctypeDir, 'META-INF').tap { mkdirs() } + new File(doctypeMetaInf, 'grails-plugin.xml').text = """ + + + com.example.DoctypeGrailsPlugin """ - def classLoader = new URLClassLoader([tempDir.toURI().toURL()] as URL[], (ClassLoader) null) + + and: 'a second descriptor on the same classpath that declares none' + def plainDir = File.createTempDir() + def plainMetaInf = new File(plainDir, 'META-INF').tap { mkdirs() } + new File(plainMetaInf, 'grails-plugin.xml').text = """ + com.example.PlainGrailsPlugin + +""" + def classLoader = new URLClassLoader( + [doctypeDir.toURI().toURL(), plainDir.toURI().toURL()] as URL[], (ClassLoader) null) when: 'plugin descriptor resources are scanned' def descriptors = PluginUtils.scanPluginDescriptorResources(classLoader) - then: 'the descriptor is read and the DTD is skipped rather than retrieved' - descriptors.size() == 1 - with(descriptors[0]) { - providedPlugins == ['com.example.TestGrailsPlugin'] - providedClasses == ['com.example.MyDomainClass'] - } + then: 'the declaration is refused and only the descriptor without one is discovered' + descriptors*.providedPlugins == [['com.example.PlainGrailsPlugin']] cleanup: - tempDir.deleteDir() + doctypeDir.deleteDir() + plainDir.deleteDir() } def 'ignores malformed plugin descriptor XML without failing discovery'() { diff --git a/grails-core/src/test/groovy/org/grails/compiler/injection/GlobalGrailsClassInjectorTransformationSpec.groovy b/grails-core/src/test/groovy/org/grails/compiler/injection/GlobalGrailsClassInjectorTransformationSpec.groovy index d3742d8b531..9e72d5f5067 100644 --- a/grails-core/src/test/groovy/org/grails/compiler/injection/GlobalGrailsClassInjectorTransformationSpec.groovy +++ b/grails-core/src/test/groovy/org/grails/compiler/injection/GlobalGrailsClassInjectorTransformationSpec.groovy @@ -720,6 +720,45 @@ class GlobalGrailsClassInjectorTransformationSpec extends Specification { xml.resources.resource*.text() == ['KeptThing'] } + void "plugin xml update recreates a descriptor that declares a doctype"() { + given: + def logCapture = new LogCapture(GlobalGrailsClassInjectorTransformation, Level.WARN) + + and: 'an existing descriptor declaring a doctype, which Grails never generates' + def pluginXml = new File(tempDir, 'doctype-plugin.xml') + pluginXml.text = ''' + + + + ExistingThing + + + ''' + + when: 'the transformation attempts to update the descriptor' + transformation.updatePluginXml(null, null, pluginXml, ['NewThing']) + + then: 'the declaration is refused, so the descriptor is discarded and a warning is logged' + !pluginXml.exists() + logCapture.events.size() == 1 + with(logCapture.events[0]) { + level == Level.WARN + formattedMessage == "Failed to update existing file ${pluginXml.absolutePath}. Recreating it instead..." + } + + and: 'the deferred names are written out when the descriptor is next generated' + transformation.generatePluginXml( + compilePlugin('class DoctypeRecoveredGrailsPlugin {}'), + '1.0', + [] as Set, + pluginXml + ) + new XmlSlurper().parse(pluginXml).resources.resource*.text() == ['NewThing'] + + cleanup: + logCapture.close() + } + void "plugin xml update recreates safely when the existing descriptor is malformed"() { given: def logCapture = new LogCapture(GlobalGrailsClassInjectorTransformation, Level.WARN) diff --git a/grails-doc/src/en/guide/upgrading.adoc b/grails-doc/src/en/guide/upgrading.adoc index 3a0765d8049..5e7edb0cd46 100644 --- a/grails-doc/src/en/guide/upgrading.adoc +++ b/grails-doc/src/en/guide/upgrading.adoc @@ -39,4 +39,6 @@ How a `DOCTYPE` declaration is treated depends on where the document comes from. Request bodies are refused if they declare one. An `application/xml`, `text/xml` or `application/hal+xml` request body, or a string passed to `XML.parse`, that carries a `DOCTYPE` is rejected rather than bound: binding fails with the `invalidRequestBody` error code and the target is left unpopulated. This is stricter than blocking external entities, because a body whose `DOCTYPE` is entirely internal and harmless is refused too, and there is no setting that relaxes it. A client that sends a declaration, which is unusual for an API payload, needs to omit it. -Descriptors that Grails reads from the classpath may declare one. JSP tag library descriptors, `web.xml` and `grails-plugin.xml` are trusted input and routinely carry a `DOCTYPE`; `jakarta.servlet.jsp.jstl` ships several such descriptors, among them `c-1_0-rt.tld`, which the default `grails.gsp.tldScanPattern` scans. These are parsed with the declaration permitted and the entity and DTD hardening above still in force, so the DTD a descriptor names is never retrieved. The same applies to `response.xml` in a controller unit test, whose document is the controller's own output. +Descriptors that Grails reads from the classpath may declare one. JSP tag library descriptors and `web.xml` are trusted input and routinely carry a `DOCTYPE`; `jakarta.servlet.jsp.jstl` ships several such descriptors, among them `c-1_0-rt.tld`, which the default `grails.gsp.tldScanPattern` scans. These are parsed with the declaration permitted and the entity and DTD hardening above still in force, so the DTD a descriptor names is never retrieved. The same applies to `response.xml` in a controller unit test, whose document is the controller's own output. + +The plugin descriptor `grails-plugin.xml` is the exception among the descriptors. Grails generates it during compilation and never writes a `DOCTYPE`, so both the compiler that rewrites it and the plugin discovery that reads it back use the strict parser. A descriptor that has been hand-edited to add a declaration is refused: at compile time it is discarded and regenerated, and at runtime the plugin it describes is skipped. diff --git a/grails-doc/src/en/guide/upgrading/upgrading80x.adoc b/grails-doc/src/en/guide/upgrading/upgrading80x.adoc index 4882dc908ab..4ff9c9c2085 100644 --- a/grails-doc/src/en/guide/upgrading/upgrading80x.adoc +++ b/grails-doc/src/en/guide/upgrading/upgrading80x.adoc @@ -3588,6 +3588,9 @@ a `DOCTYPE` is additionally refused rather than bound, whether or not the declar Binding such a body fails with the `invalidRequestBody` error code. There is no setting that relaxes this; a client that sends a declaration needs to omit it. -Descriptors that Grails reads from the classpath, such as JSP tag library descriptors, `web.xml` and `grails-plugin.xml`, -and the `response.xml` accessor in controller unit tests are not affected. They may declare a `DOCTYPE`, though the DTD -it names is never retrieved. See <> for the full description of the XML parsing defaults. +Descriptors that Grails reads from the classpath, such as JSP tag library descriptors and `web.xml`, and the +`response.xml` accessor in controller unit tests are not affected. They may declare a `DOCTYPE`, though the DTD it names +is never retrieved. The plugin descriptor `grails-plugin.xml` is not among them: Grails generates it and never writes a +declaration, so it is read strictly at compile time and at runtime alike. + +See <> for the full description of the XML parsing defaults. diff --git a/grails-gradle/common/src/main/groovy/org/apache/grails/gradle/common/XmlParserFeature.java b/grails-gradle/common/src/main/groovy/org/apache/grails/gradle/common/XmlParserFeature.java index 477440aa5d7..fb10d909fb0 100644 --- a/grails-gradle/common/src/main/groovy/org/apache/grails/gradle/common/XmlParserFeature.java +++ b/grails-gradle/common/src/main/groovy/org/apache/grails/gradle/common/XmlParserFeature.java @@ -43,9 +43,9 @@ public enum XmlParserFeature { * *

Enabling this is stricter than blocking external entities: it refuses documents whose * DOCTYPE is entirely internal and harmless. It suits untrusted input such as HTTP request - * bodies. Descriptors read from the classpath — JSP tag library definitions, {@code web.xml}, - * {@code grails-plugin.xml} — routinely carry a DOCTYPE, so their readers need a parser that - * leaves this disabled while keeping the entity and DTD features below switched off. + * bodies. Descriptors read from the classpath — JSP tag library definitions and + * {@code web.xml} — routinely carry a DOCTYPE, so their readers need a parser that leaves this + * disabled while keeping the entity and DTD features below switched off. */ DISALLOW_DOCTYPE_DECL("http://apache.org/xml/features/disallow-doctype-decl"), diff --git a/grails-gradle/model/src/main/groovy/org/grails/io/support/SpringIOUtils.java b/grails-gradle/model/src/main/groovy/org/grails/io/support/SpringIOUtils.java index 20ef4ce3048..b107c03d06b 100644 --- a/grails-gradle/model/src/main/groovy/org/grails/io/support/SpringIOUtils.java +++ b/grails-gradle/model/src/main/groovy/org/grails/io/support/SpringIOUtils.java @@ -429,9 +429,8 @@ public static XmlSlurper createXmlSlurper() throws ParserConfigurationException, *

Documents parsed through this class fall into two trust levels, and each level gets its * own parser. Request bodies are untrusted and are refused if they declare a {@code DOCTYPE}; * that is the {@code false} form and the default. Descriptors read from the application - * classpath, such as JSP tag library descriptors, {@code web.xml} and - * {@code grails-plugin.xml}, are trusted and routinely declare one, so their readers pass - * {@code true}. + * classpath, such as JSP tag library descriptors and {@code web.xml}, are trusted and routinely + * declare one, so their readers pass {@code true}. * *

Tolerating the declaration does not reopen the XXE vector. External general entities, * external parameter entities, DTD grammar loading and external DTD retrieval stay off on both From 99a4604dbcbaedcd3c8af61aae43434e632f59e0 Mon Sep 17 00:00:00 2001 From: James Daugherty Date: Fri, 11 Sep 2026 10:03:55 -0400 Subject: [PATCH 6/6] test XML.parse refuses a declared DOCTYPE through every overload --- .../grails/converters/XMLParseSpec.groovy | 73 +++++++++++++++++++ 1 file changed, 73 insertions(+) create mode 100644 grails-converters/src/test/groovy/grails/converters/XMLParseSpec.groovy diff --git a/grails-converters/src/test/groovy/grails/converters/XMLParseSpec.groovy b/grails-converters/src/test/groovy/grails/converters/XMLParseSpec.groovy new file mode 100644 index 00000000000..dc4c533f0d3 --- /dev/null +++ b/grails-converters/src/test/groovy/grails/converters/XMLParseSpec.groovy @@ -0,0 +1,73 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package grails.converters + +import jakarta.servlet.http.HttpServletRequest +import org.springframework.mock.web.MockHttpServletRequest +import org.xml.sax.SAXParseException +import spock.lang.Specification +import spock.lang.Unroll + +import org.grails.web.converters.exceptions.ConverterException + +/** + * Holds {@link XML#parse} to the parsing guarantee the user guide and the threat model state for it: + * a document that declares a {@code DOCTYPE} is refused rather than parsed, whether or not the + * declaration references anything external. + * + *

Refusing the declaration is what closes the entity vectors at this entry point, since an entity + * cannot be declared without one. The parser features themselves are covered by + * {@code SpringIOUtilsSpec}; what is pinned here is that the class the documentation names uses the + * strict parser, through every overload an application reaches, {@code request.XML} included. + */ +class XMLParseSpec extends Specification { + + private static final String DOCUMENT = 'Grails' + + private static final String DOCUMENT_WITH_DOCTYPE = "\n${DOCUMENT}" + + @Unroll + void 'parsing #entryPoint refuses a declared doctype'() { + when: 'a document declaring a doctype is parsed' + invoke(DOCUMENT_WITH_DOCTYPE) + + then: 'it is refused rather than parsed' + ConverterException e = thrown() + e.message == 'Error parsing XML' + e.cause instanceof SAXParseException + e.cause.message.contains('DOCTYPE is disallowed') + + and: 'the same document is read once the declaration is removed' + invoke(DOCUMENT).title.text() == 'Grails' + + where: + entryPoint | invoke + 'a string' | { String xml -> XML.parse(xml) } + 'a stream' | { String xml -> XML.parse(new ByteArrayInputStream(xml.getBytes('UTF-8')), 'UTF-8') } + 'a request' | { String xml -> XML.parse(post(xml)) } + } + + private static HttpServletRequest post(String xml) { + new MockHttpServletRequest('POST', '/books').tap { + characterEncoding = 'UTF-8' + content = xml.getBytes('UTF-8') + } + } +}