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-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') + } + } +} 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..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,6 +138,8 @@ public static List scanPluginDescriptorResources(ClassLoader c try { Enumeration resources = classLoader.getResources(PLUGIN_XML_PATTERN); + // 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()) { 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..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,6 +82,38 @@ class PluginDiscoverySpec extends Specification { tempDir.deleteDir() } + 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 + +""" + + 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 declaration is refused and only the descriptor without one is discovered' + descriptors*.providedPlugins == [['com.example.PlainGrailsPlugin']] + + cleanup: + doctypeDir.deleteDir() + plainDir.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 + + + 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/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-doc/src/en/guide/theWebLayer/gsp/taglibs/usingJSPTagLibraries.adoc b/grails-doc/src/en/guide/theWebLayer/gsp/taglibs/usingJSPTagLibraries.adoc index d532842d03f..f3b01e43cbb 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,8 @@ grails: tldScanPattern: 'classpath*:/META-INF/*.tld,/WEB-INF/tld/*.tld' ---- +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] .build.gradle diff --git a/grails-doc/src/en/guide/upgrading.adoc b/grails-doc/src/en/guide/upgrading.adoc index f3248516212..5e7edb0cd46 100644 --- a/grails-doc/src/en/guide/upgrading.adoc +++ b/grails-doc/src/en/guide/upgrading.adoc @@ -30,3 +30,15 @@ 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 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. + +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 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 63618876cba..df1fa4bec09 100644 --- a/grails-doc/src/en/guide/upgrading/upgrading80x.adoc +++ b/grails-doc/src/en/guide/upgrading/upgrading80x.adoc @@ -3603,3 +3603,21 @@ def index(Integer max) { } } ---- + +==== 60. 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 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 new file mode 100644 index 00000000000..fb10d909fb0 --- /dev/null +++ b/grails-gradle/common/src/main/groovy/org/apache/grails/gradle/common/XmlParserFeature.java @@ -0,0 +1,94 @@ +/* + * 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. It suits untrusted input such as HTTP request + * 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"), + + /** + * 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 cb3da2dc0a8..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 @@ -47,8 +47,11 @@ import groovy.xml.FactorySupport; import groovy.xml.XmlSlurper; +import org.slf4j.LoggerFactory; import org.xml.sax.SAXException; +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, @@ -407,54 +410,133 @@ 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 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 + * 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)); } - private static SAXParserFactory saxParserFactory = null; + /** + * 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. + * + * @return a namespace-aware, non-validating parser that rejects {@code DOCTYPE} declarations + */ + public static SAXParser newSAXParser() throws ParserConfigurationException, SAXException { + return newSAXParser(false); + } - private static SAXParserFactory createParserFactory() throws ParserConfigurationException { - if (saxParserFactory == null) { - saxParserFactory = FactorySupport.createSaxParserFactory(); - saxParserFactory.setNamespaceAware(true); - saxParserFactory.setValidating(false); + /** + * Creates a {@link SAXParser}, optionally tolerating a {@code DOCTYPE} declaration. + * + *

Applies the same hardening as {@link #createXmlSlurper(boolean)}. + * + * @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 SAXParser newSAXParser(boolean allowDocTypeDeclaration) throws ParserConfigurationException, SAXException { + return parserFactory(allowDocTypeDeclaration).newSAXParser(); + } - try { - saxParserFactory.setFeature("https://apache.org/xml/features/disallow-doctype-decl", false); - } catch (Exception pce) { - // ignore, parser doesn't support - } - try { - saxParserFactory.setFeature("https://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); - } 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("https://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); - } catch (Exception e) { - // ignore, parser doesn't support + /** + * 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 that differs between the two parsers; see {@link #createXmlSlurper(boolean)}. + */ + 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 volatile SAXParserFactory strictParserFactory; + + private static volatile SAXParserFactory docTypeParserFactory; + + private static SAXParserFactory parserFactory(boolean allowDocTypeDeclaration) throws ParserConfigurationException { + if (allowDocTypeDeclaration) { + SAXParserFactory factory = docTypeParserFactory; + if (factory == null) { + factory = buildParserFactory(true); + docTypeParserFactory = factory; } + return factory; + } + SAXParserFactory factory = strictParserFactory; + if (factory == null) { + factory = buildParserFactory(false); + strictParserFactory = factory; + } + return factory; + } + + 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) { + // 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) { + 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()); } - return saxParserFactory; } } 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..c3afb02351b --- /dev/null +++ b/grails-gradle/model/src/test/groovy/org/grails/io/support/SpringIOUtilsSpec.groovy @@ -0,0 +1,194 @@ +/* + * 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 java.nio.file.Files +import java.nio.file.Path + +import org.xml.sax.SAXParseException +import org.xml.sax.helpers.DefaultHandler + +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. + * + *

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 { + + /** 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 +''' + + private static final String SECRET = 'top-secret-token' + + @TempDir + Path tempDir + + private String externalEntityDocument() { + Path secret = tempDir.resolve('secret.txt') + Files.writeString(secret, SECRET) + """ +]> +&ext;""" + } + + 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'() { + when: + def xml = SpringIOUtils.createXmlSlurper().parseText('ok') + + then: + xml.child.text() == 'ok' + } + + void 'createXmlSlurper rejects a doctype declaration by default'() { + when: + SpringIOUtils.createXmlSlurper().parseText(TLD) + + then: + SAXParseException e = thrown() + e.message.contains('DOCTYPE is disallowed') + } + + void 'createXmlSlurper rejects an internal doctype subset by default'() { + when: + SpringIOUtils.createXmlSlurper().parseText(''' +]> +&msg;''') + + then: + SAXParseException e = thrown() + e.message.contains('DOCTYPE is disallowed') + } + + 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(true).parseText(TLD) + + then: + parsed.uri.text() == 'jakarta.tags.core' + parsed.tag.name.text() == 'out' + } + + void 'the doctype-tolerant slurper does not resolve external general entities'() { + given: 'a document whose entity points at a readable file on disk' + 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; +]> +ok""" + + when: + def parsed = SpringIOUtils.createXmlSlurper(true).parseText(xml) + + then: + parsed.text() == 'ok' + } + + 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(true).parseText(xml).text() == 'ok' + } + + void 'the doctype-tolerant sax parser applies the same entity hardening'() { + given: + String xml = externalEntityDocument() + + when: + String text = parseWithSaxParser(SpringIOUtils.newSAXParser(true), xml) + + then: + !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/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/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 8051154047a..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,27 +31,24 @@ 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 import org.xml.sax.SAXException +import org.apache.grails.gradle.common.XmlParserFeature + /** * Utility methods for handling XML. * * @since 7.0.10 */ +@Slf4j @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 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 Pattern SPACE_AND_EMPTY_ELEMENT_CLOSE = ~/ \/>/ private static final String EMPTY_ELEMENT_CLOSE = '/>' @@ -59,12 +56,12 @@ class XmlUtils { private static final Pattern XML_DECLARATION = ~/^\s*(<\?xml\b.*?\?>)/ private static final Map SECURE_XML_SLURPER_FEATURES = [ - (DISALLOW_DOCTYPE_DECL): false, - (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() /** @@ -118,8 +115,8 @@ 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, 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} @@ -226,12 +223,22 @@ class XmlUtils { it.validating = false } + try { + saxParserFactory.XIncludeAware = false + } + catch (UnsupportedOperationException ignored) { + // ignore, parser doesn't support + } + SECURE_XML_SLURPER_FEATURES.each { feature, enabled -> 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 d1b83f9f5c1..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 @@ -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,34 @@ 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, """ + def response = mockResponse(200, ''' ]> -&ext;""") +&ext;''') when: response.xml() then: - def e = thrown(SAXParseException) - e.message.contains('External Entity') - - cleanup: - Files.deleteIfExists(secretFile) + SAXParseException e = thrown() + e.message.contains('DOCTYPE is disallowed') } - void 'xml secure default still allows inline doctype declarations with internal entities'() { + void 'xml rejects doctype declarations with internal entities'() { given: def response = mockResponse(200, ''' ]> &msg;''') - expect: - response.xml().text() == 'safe' + when: + response.xml() + + then: + 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 a88acde5228..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 @@ -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,28 @@ 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(''' + XmlUtils.newXmlSlurper().parseText(''' ]> -&msg;''') +&ext;''') then: - parsed.text() == 'safe' + SAXParseException e = thrown() + e.message.contains('DOCTYPE is disallowed') } - 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;""" - + void 'newXmlSlurper rejects doctype declarations with internal entities'() { when: - XmlUtils.newXmlSlurper().parseText(xml) + XmlUtils.newXmlSlurper().parseText(''' +]> +&msg;''') then: - def e = thrown(SAXParseException) - e.message.contains('External Entity') - - cleanup: - Files.deleteIfExists(secretFile) + 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: