Piotr Karwasz created KNOX-3484:
-----------------------------------
Summary: 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
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)
