[
https://issues.apache.org/jira/browse/KNOX-3484?focusedWorklogId=1043811&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1043811
]
ASF GitHub Bot logged work on KNOX-3484:
----------------------------------------
Author: ASF GitHub Bot
Created on: 24/Sep/26 21:22
Start Date: 24/Sep/26 21:22
Worklog Time Spent: 10m
Work Description: ppkarwasz opened a new pull request, #1427:
URL: https://github.com/apache/knox/pull/1427
[KNOX-3484](https://issues.apache.org/jira/browse/KNOX-3484) - Use Apache
Commons Secure XML for all JAXP factories
## What changes were proposed in this pull request?
Knox configured its XML parsers by hand: each call site set its own subset
of features and properties, and some relied on the defaults of whichever
implementation (JDK, Woodstox, MOXy, Glassfish JAXB, Commons Digester) served
them. This PR replaces those ad hoc settings with one dependency, [Apache
Commons Secure XML](https://commons.apache.org/proper/commons-secure-xml/)
1.0.0, whose factories are hardened as delivered: external resources (DTDs,
external entities, XInclude, schemas, stylesheets) are ignored, independently
of the JAXP implementation.
This is a hardening and maintenance change: the PMC no longer has to
configure XML parsers.
- **DOM, TrAX, XPath, Validation:** `XmlUtils`,
`UrlRewriteRulesDescriptorAdapter`, `XmlFilterReader`,
`ServiceURLPropertyConfig`, `TopologyValidator` and `TopologyToDescriptor`
obtain their factories from `SecureDocumentBuilderFactory`,
`SecureTransformerFactory`, `SecureXPathFactory` and `SecureSchemaFactory`. The
hand-written feature settings are removed.
- **StAX:** `XmlFilterReader` uses `SecureXMLInputFactory`.
- **JAXB:** `ServiceDefinitionUnmarshaller`, `TopologyMarshaller` (XML
bodies), `ServiceDefinitionsLoader`, `ApplicationDeploymentContributor`,
`TopologyToDescriptor`, `RemoteConfigurationRegistriesParser` and
`ProviderConfigurationParser` unmarshal through two new helpers,
`XmlUtils.unmarshal(Unmarshaller, Class<T>, File)` and
`XmlUtils.unmarshal(Unmarshaller, Class<T>, String, InputStream)`, which
unmarshal from an `XMLEventReader` created by `SecureXMLInputFactory`. This
follows the StAX approach documented for JAXB in apache/commons-secure-xml#103,
and makes the result independent of the JAXB provider serving each context.
- **`TopologyMarshaller`:** selects the XML or JSON reader by exact media
type (`application/xml` or `application/json`, parameters ignored) and passes
MOXy a fixed media type instead of the request's. Other types, including
wildcards, are rejected with HTTP 415.
- **Commons Digester:** `TopologyUtils`, `XmlGatewayDescriptorImporter` and
`XmlUrlRewriteRulesImporter` pass Digester an `XMLReader` created by
`SecureSAXParserFactory`. Since Digester 3.2 resolves any absolute system id by
itself, each site also installs a resolver that returns `null`, leaving
external references to Commons Secure XML.
- **Enforcement:** a new forbidden-apis execution
(`build-tools/forbiddenapis/xml-signatures.txt`, main code only) rejects the
plain JAXP factory methods, the `Unmarshaller.unmarshal` overloads that let the
JAXB provider create its own parser (`File`, `InputStream`, `Reader`, `URL`,
`InputSource`, `Source`) and the Digester constructors that create their own
parser. `TopologyMarshaller` is excluded from this check in
`gateway-service-admin/pom.xml`, because MOXy parses its JSON request bodies
with its own JSON reader. On the current `master` it reports 23 violations in 9
modules, for example:
```
[ERROR] Forbidden method invocation:
javax.xml.parsers.DocumentBuilderFactory#newInstance() [Use
SecureDocumentBuilderFactory from Apache Commons Secure XML]
[ERROR] in org.apache.knox.gateway.util.XmlUtils (XmlUtils.java:47)
[ERROR] Forbidden method invocation:
javax.xml.stream.XMLInputFactory#newFactory() [Use SecureXMLInputFactory from
Apache Commons Secure XML]
[ERROR] in
org.apache.knox.gateway.filter.rewrite.impl.xml.XmlFilterReader
(XmlFilterReader.java:97)
[ERROR] Forbidden method invocation:
org.apache.commons.digester3.binder.DigesterLoader#newDigester() [Pass an
XMLReader created by SecureSAXParserFactory]
[ERROR] in org.apache.knox.gateway.util.TopologyUtils
(TopologyUtils.java:41)
```
**Behavior change:** documents containing a DOCTYPE are now accepted
everywhere. External references are ignored and internal entities are expanded
within the limits of secure processing. Previously `XmlUtils` rejected any
DOCTYPE and the StAX readers disabled DTD support.
Minor fixes in touched code: `XmlUtils.readXml(File)` and
`ProviderConfigurationParser.parseXML(File)` no longer leak their input stream,
and `ServiceDefinitionsLoader` no longer opens a stream it does not read.
## How was this patch tested?
- `mvn install` on the affected modules (`gateway-util-common`,
`gateway-spi`, `gateway-service-admin`, `gateway-provider-rewrite`,
`gateway-provider-rewrite-common`, `gateway-server`,
`gateway-discovery-ambari`, `gateway-service-remoteconfig`,
`gateway-topology-simple`) and their dependencies: all unit tests, checkstyle,
forbidden-apis and dependency analysis pass.
- `GatewayAdminTopologyFuncTest`: 32/32 pass.
- No new tests are added, since the guarantees are the library's and are
tested there. The existing KNOX-1308 and KNOX-3447 tests
(`GatewayAdminTopologyFuncTest#testPutTopologyWithEntityExpansion`,
`ServiceDefinitionUnmarshallerTest#testEntityExpansionIsNotResolved`) asserted
that internal entities are not expanded; they now assert that the content of an
external entity does not leak. Reverting `ServiceDefinitionUnmarshaller` to a
plain `XMLInputFactory` makes both unit tests fail.
## Integration Tests
No integration tests are added or changed: the change does not alter any
Knox feature, and the existing suites cover the affected parsing paths.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Issue Time Tracking
-------------------
Worklog Id: (was: 1043811)
Remaining Estimate: 0h
Time Spent: 10m
> Use Apache Commons Secure XML for all JAXP factories
> ----------------------------------------------------
>
> Key: KNOX-3484
> URL: https://issues.apache.org/jira/browse/KNOX-3484
> Project: Apache Knox
> Issue Type: Improvement
> Reporter: Piotr Karwasz
> Priority: Minor
> Time Spent: 10m
> Remaining Estimate: 0h
>
> Knox configures its XML parsers by hand. Every call site that creates a JAXP
> factory repeats its own subset of features and properties
> ({{{}disallow-doctype-decl{}}}, {{{}FEATURE_SECURE_PROCESSING{}}},
> {{{}SUPPORT_DTD{}}}, {{{}isSupportingExternalEntities{}}},
> {{{}ACCESS_EXTERNAL_*{}}}, ...), and the subsets differ from site to site.
> Some sites set nothing and rely on the defaults of whichever implementation
> (JDK, Woodstox, MOXy, Glassfish JAXB, Commons Digester) happens to serve
> them. Some ignore a failure to apply a setting.
> Keeping this correct is a recurring cost for the PMC: every new parser has to
> remember the full list, several of the feature names are
> implementation-specific, and whether a given setting takes effect depends on
> which implementation serves which call site.
> [Apache Commons Secure
> XML|https://commons.apache.org/proper/commons-secure-xml/] (1.0.0, Java 8+,
> no runtime dependencies, Apache-2.0) moves that work into one dependency. Its
> factories are hardened as delivered: external resources (DTDs, external
> entities, XInclude, schema imports, stylesheets) are ignored by non-removable
> resolvers, independently of the underlying JAXP implementation, and a factory
> that cannot be hardened fails loudly instead of silently. The guarantees are
> documented in its [threat
> model|https://commons.apache.org/proper/commons-secure-xml/threat_model.html]
> and tested against the JDK, Xerces, Woodstox and Saxon.
> This is a hardening and maintenance change: it replaces the ad hoc
> configuration with one consistent dependency, so that the PMC no longer has
> to configure XML parsers.
> h2. Current state
> ||Module||Class||API||Current configuration||
> |gateway-util-common|{{XmlUtils}} ({{{}readXml{}}}, {{{}createDocument{}}},
> {{{}getTransformer{}}})|DOM, TrAX|{{{}FEATURE_SECURE_PROCESSING{}}},
> {{disallow-doctype-decl}}|
> |gateway-service-admin|{{{}ServiceDefinitionUnmarshaller{}}},
> {{TopologyMarshaller}}|StAX + JAXB|{{{}SUPPORT_DTD{}}},
> {{{}isSupportingExternalEntities{}}}, {{isReplacingEntityReferences}}|
> |gateway-provider-rewrite|{{XmlFilterReader}}|StAX, XPath|StAX: same three
> properties; XPath: {{{}FEATURE_SECURE_PROCESSING{}}}, failure ignored|
> |gateway-discovery-ambari|{{ServiceURLPropertyConfig}}|XPath|none|
> |gateway-server|{{TopologyValidator}}|Validation|{{ACCESS_EXTERNAL_*}} on the
> {{Validator}} only|
> |gateway-spi|{{UrlRewriteRulesDescriptorAdapter}}|TrAX|none|
> |gateway-server|{{{}ServiceDefinitionsLoader{}}},
> {{{}ApplicationDeploymentContributor{}}}, {{TopologyToDescriptor}}|JAXB
> {{unmarshal(File/InputStream)}}|none (depends on the JAXB provider)|
> |gateway-service-remoteconfig|{{RemoteConfigurationRegistriesParser}}|JAXB
> {{unmarshal(File)}}|none (depends on the JAXB provider)|
> |gateway-topology-simple|{{ProviderConfigurationParser}}|JAXB
> {{unmarshal(InputStream)}}|none (depends on the JAXB provider)|
> |gateway-server, gateway-provider-rewrite-common|{{{}TopologyUtils{}}},
> {{{}XmlGatewayDescriptorImporter{}}}, {{XmlUrlRewriteRulesImporter}}|Commons
> Digester|none (depends on Digester's {{{}SAXParserFactory{}}})|
> h2. Proposed changes
> Add {{org.apache.commons:commons-secure-xml}} to the root
> {{dependencyManagement}} and to the modules above, then:
> * *DOM and TrAX:* {{XmlUtils}} and {{UrlRewriteRulesDescriptorAdapter}}
> obtain their factories from {{SecureDocumentBuilderFactory}} and
> {{{}SecureTransformerFactory{}}}. The security-motivated feature settings are
> removed; {{disallow-doctype-decl}} may be kept where Knox prefers to reject a
> DOCTYPE outright, as a behavioral choice rather than a security setting.
> * *StAX:* {{{}ServiceDefinitionUnmarshaller{}}}, {{TopologyMarshaller}} and
> {{XmlFilterReader}} obtain their {{XMLInputFactory}} from
> {{{}SecureXMLInputFactory.newFactory(){}}}. {{SUPPORT_DTD=false}} may
> likewise be kept as a behavioral choice: Commons Secure XML accepts settings
> that restrict further.
> * *XPath:* {{XmlFilterReader}} and {{ServiceURLPropertyConfig}} use
> {{{}SecureXPathFactory{}}}.
> * *Validation:* {{TopologyValidator}} uses {{{}SecureSchemaFactory{}}}.
> * *JAXB:* the {{unmarshal(File)}} and {{unmarshal(InputStream)}} calls are
> replaced with {{{}unmarshal(XMLEventReader){}}}, where the reader is created
> by {{{}SecureXMLInputFactory.newFactory().createXMLEventReader(...){}}}. The
> hardening then no longer depends on which JAXB provider (MOXy or the
> Glassfish runtime) serves a given context.
> * *Commons Digester:* the next Digester release depends on Commons Secure
> XML and hardens its parser by default, so upgrading Digester brings the same
> protection and the dependency comes at no extra cost. Until then, the three
> importers can pass a hardened reader explicitly:
> {{{}DigesterLoader.newDigester(SecureSAXParserFactory.newNSInstance().newSAXParser().getXMLReader(),
> rules){}}}.
> h2. Enforcement
> To keep raw factories from coming back, add a
> [forbidden-apis|https://github.com/policeman-tools/forbidden-apis] check with
> a signatures file that rejects the plain JAXP factory methods
> ({{{}DocumentBuilderFactory{}}}, {{{}SAXParserFactory{}}},
> {{{}XMLInputFactory{}}}, {{{}TransformerFactory{}}}, {{{}SchemaFactory{}}},
> {{{}XPathFactory{}}}) and the {{unmarshal(File)}} /
> {{unmarshal(InputStream)}} overloads of {{jakarta.xml.bind.Unmarshaller}} in
> main code.
> No new XXE tests are needed: the guarantees are the library's and are tested
> there. The existing KNOX-1308 and KNOX-3447 tests should keep passing
> unchanged.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
