[
https://issues.apache.org/jira/browse/CAMEL-24664?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18113455#comment-18113455
]
Piotr Karwasz commented on CAMEL-24664:
---------------------------------------
Sorry for the long LLM-generated issue description, but I wanted it to be
thorough, since modifying XML hardenings that have been there for 20+ years
might break some users.
I opened COMMONSXML-16 to add some helper methods to Commons Secure XML to
helper the migration.
> Use Apache Commons Secure XML for JAXP factory creation
> -------------------------------------------------------
>
> Key: CAMEL-24664
> URL: https://issues.apache.org/jira/browse/CAMEL-24664
> Project: Camel
> Issue Type: Improvement
> Components: camel-core, camel-validator, camel-xslt
> Reporter: Piotr Karwasz
> Assignee: Andrea Cosentino
> Priority: Minor
>
> Camel hardens JAXP by hand. Around forty call sites each configure their own
> subset of {{setFeature}} / {{setProperty}} calls, and the subsets disagree:
> * {{XmlConverter.createDocumentBuilderFactory()}} omits
> {{external-parameter-entities}} and {{load-external-dtd}};
> {{createSAXParserFactory()}} omits {{disallow-doctype-decl}}.
> * {{XMLConverterHelper}} sets {{disallow-doctype-decl}} twice, the second
> block added by CAMEL-18825 directly beneath an identical one that had been
> there since 2014.
> * {{ProcessorDetailDevConsole}} sets neither {{FEATURE_SECURE_PROCESSING}}
> nor {{load-external-dtd}}.
> * Most sites swallow {{ParserConfigurationException}}, so a parser that
> rejects a feature ends up hardened by accident rather than by contract.
> The hardening accreted over twelve years in at least six waves (CAMEL-6933 in
> 2013, CAMEL-7123 and CAMEL-7131 in 2014, CAMEL-7713 in 2014, CAMEL-10894 in
> 2017, CAMEL-12444 in 2018, CAMEL-18825 in 2022). Each wave patched whichever
> sites its author happened to be looking at. The threat model in
> {{docs/user-manual/modules/ROOT/pages/security-model.adoc}} records the cost
> of that drift: CVE-2015-0263 (XML converter in camel-core), CVE-2015-0264
> (XPath language), CVE-2017-5643 (Validation component) and CVE-2018-8027 (XSD
> validation processor) are all in the XXE / remote-DTD class, and all land in
> the code this issue touches.
> h1. Proposal
> Create every JAXP factory through [Apache Commons Secure
> XML|https://commons.apache.org/proper/commons-secure-xml/]
> ({{org.apache.commons:commons-secure-xml}}, 1.0.0, Java 8+, no transitive
> compile dependencies, Saxon-HE optional/provided, OSGi bundle), and delete
> the hand-rolled feature blocks it subsumes.
> h2. Feature-based hardening versus a resolver floor
> This is the substantive change, and it is not a like-for-like swap.
> || || Today (feature based) || Proposed (resolver floor) ||
> | Mechanism | a set of {{setFeature}} / {{setProperty}} calls per factory | a
> non-removable ignore-all resolver installed under the parser |
> | Coverage | whatever the implementation on the classpath honours | identical
> on the stock JDK, Xerces, Woodstox, Saxon and Android |
> | Default | varies per site, per implementation | external references are
> *not fetched*, everywhere |
> | Failure mode | implementation-specific error, or a silent fetch | the
> reference resolves to *empty content* |
> | Evidence | read the flag back with {{getFeature}} | observe what the parser
> actually fetches |
> Every factory returned by the library carries a floor that refuses the whole
> family of external resource fetches: the external DTD subset, general and
> parameter entities, {{xs:import}} / {{xs:include}} / {{xs:redefine}} and
> {{xsi:schemaLocation}}, {{xsl:import}} / {{xsl:include}} and {{document()}},
> {{XInclude}}, and the URI-fetching XPath functions {{doc()}},
> {{collection()}} and {{unparsed-text()}}.
> *The floor cannot be removed, and a user resolver is layered in front of it
> rather than replacing it.* {{setEntityResolver}}, {{setURIResolver}},
> {{setResourceResolver}} and {{setXMLResolver}} install a _delegate that is
> consulted first_; returning a non-null result opts that specific reference
> in, and anything left unresolved falls through to the floor. So the extension
> point is unchanged in shape, and every resolver Camel already installs
> ({{XsltUriResolver}}, {{DefaultLSResourceResolver}}, a user's own) keeps
> working exactly as before.
> Two consequences worth stating up front:
> * An unresolved reference yields *empty content rather than an error*. A
> parse that used to fail loudly may now continue with nothing in place of the
> resource. The library offers
> {{-Dorg.apache.commons.xml.secure.throwOnUnresolved=true}} to reject instead,
> but it is JVM-global, so Camel cannot set it on a user's behalf.
> * Reading a security feature back off a factory no longer tells you whether
> the parser will fetch, because the blocking moved beneath the flag.
> Assertions must move from factory configuration to parse behaviour.
> h2. Why this is the right direction
> The prevailing philosophy in JAXP is _allowed unless forbidden_, and every
> forbidding knob is optional, implementation-specific and easy to forget. The
> floor inverts it to
> *forbidden unless allowed*, which is the property that would have prevented
> the four CVEs above: a new consumer, converter or data format is secure
> because it did nothing, rather than insecure because its author missed a flag.
> For deployments that need a real external-resource policy rather than an
> all-or-nothing switch, [XML Resolver|https://www.xmlresolver.org/index.html]
> is a good companion: it implements OASIS catalogs and can be installed as the
> delegate in front of the floor, giving a curated allow-list instead of a
> blanket opt-in.
> h1. Scope
> One sub-task per JAXP factory. Listed in increasing order of effort rather
> than in the order of the JAXP API, because the later ones depend on decisions
> taken in the earlier.
> h2. 1. SAX
> The easiest, and the one with no user-facing decision left to take.
> The 4.23 upgrade guide (_camel-core - XmlConverter SAX parser factory_)
> already documents the escape hatch, and it is not a feature flag:
> {quote}Routes that genuinely need to resolve an external DTD or parameter
> entity through this converter must supply their own
> {{SAXParserFactory}}.{quote}
> That statement stays true verbatim under the floor, so no new escape hatch
> and no documentation change is required. The work is:
> * {{SAXParserFactory.newInstance()}} ->
> {{SecureSAXParserFactory.newNSInstance()}} (the {{NS}} variant folds in the
> existing {{setNamespaceAware(true)}}).
> * Delete {{FEATURE_SECURE_PROCESSING}}, {{external-general-entities}},
> {{external-parameter-entities}} and {{nonvalidating/load-external-dtd}},
> together with the {{try}} / {{catch}} scaffolding around them.
> {{disallow-doctype-decl}} is deliberately not set on the SAX path today, and
> does not need to be: the floor makes a DOCTYPE harmless without rejecting the
> document.
> Sites: {{XmlConverter.createSAXParserFactory()}}, {{XmlLineNumberParser}},
> {{XmlPrettyPrinter}}, {{RouteCoverageXmlParser}},
> {{SchematronProcessorFactory}}, {{DefaultBulkApiClient}}.
> h2. 2. DOM
> Harder, because the documented escape hatch _is_ a feature flag.
> The 4.23 upgrade guide (_camel-xpath_, from CAMEL-24475) tells users that
> Camel's {{DocumentBuilderFactory}} "disallows a {{DOCTYPE}} declaration and
> does not resolve external entities", that such a document is "rejected with a
> {{SAXParseException}}", and that the way to parse one anyway is:
> {code:none}
> -Dorg.apache.camel.xmlconverter.documentBuilderFactory.feature:http://apache.org/xml/features/disallow-doctype-decl=false
> {code}
> Under the floor that property still relaxes the DOCTYPE check, but it no
> longer restores external entity resolution, because the fetch is blocked one
> layer below the flag. The escape hatch therefore has to be restated as either:
> * install an {{EntityResolver}} on the {{DocumentBuilder}} that allow-lists
> what the deployment genuinely needs, or
> * supply your own {{DocumentBuilderFactory}}, matching what SAX already says.
> *Interaction with CAMEL-23878.* That issue asks for
> {{createSAXParserFactory}} to be aligned with
> {{createDocumentBuilderFactory}} by adding {{disallow-doctype-decl=true}} and
> {{external-parameter-entities=false}} to the SAX path, and notes that code
> relying on DOCTYPE handling through SAX conversion would break. With a
> resolver floor the cheaper convergence runs the other way: *let both paths
> accept a DOCTYPE and let the floor neutralise its external references*,
> rather than teaching SAX to reject documents it accepts today. That closes
> CAMEL-23878 by making the two paths agree, with no breakage on either side.
> The counter-argument has to be recorded honestly: dropping
> {{disallow-doctype-decl}} from the DOM path relaxes a default that the 4.23
> upgrade guide promised in the same release, and the threat model's committer
> checklist requires an upgrade-guide entry plus PMC review for exactly that.
> It is a decision for the PMC, not an implementation detail. Measured
> behaviour to inform it: with the floor in place and the DOCTYPE permitted, a
> document carrying {{<!ENTITY xxe SYSTEM "file:///...">}} parses and the
> entity expands to *empty* on DOM, SAX and StAX alike; nothing is read.
> Sites: {{XmlConverter.createDocumentBuilderFactory()}},
> {{XMLConverterHelper}}, {{XmlLineNumberParser}}, {{BaseParser}},
> {{RouteCoverageXmlParser}}, {{ProcessorDetailDevConsole}},
> {{FlatpackConverter}}, {{CMSenderOneMessageImpl}},
> {{SaxonXmlSourceHandlerFactoryImpl}} (currently unhardened).
> h2. 3. StAX
> Little work. {{XMLInputFactory.newInstance()}} ->
> {{SecureXMLInputFactory.newFactory()}}.
> The properties currently set can be reclassified rather than simply deleted:
> * {{SUPPORT_DTD}} and {{IS_SUPPORTING_EXTERNAL_ENTITIES}} become
> *optimisations*. The floor would catch these references anyway; switching
> them off stops the parser from walking a DTD it is not allowed to act on.
> Keeping them is cheap and shortens the path.
> * {{IS_REPLACING_ENTITY_REFERENCES}} is *not a security property* at all. It
> selects whether entity references are reported as events or replaced inline,
> and should be documented as a parsing choice rather than left sitting in a
> hardening block.
> {{StaxConverter}} also installs an {{XMLResolver}} that throws {{"Reading
> external entities is disabled"}}. Under the floor it becomes the delegate and
> still throws, which is how the loud failure mode is preserved against the
> library's silent-empty default. Worth keeping for that reason, though note
> {{SUPPORT_DTD=false}} pre-empts it for DOCTYPE-bearing payloads, so it is
> reached less often than it appears.
> {{XMLOutputFactory}} is unaffected; it writes.
> Sites: {{StaxConverter}}, {{XmlStreamDetector}}, {{UblDataFormat}},
> {{StAXJAXBIteratorExpression}}.
> h2. 4. Validation
> Camel already has the concept this needs: the global option
> {{CamelXmlValidatorAccessExternalDTD}} ({{SchemaReader.ACCESS_EXTERNAL_DTD}},
> CAMEL-10894), which today skips the {{ACCESS_EXTERNAL_DTD}} /
> {{ACCESS_EXTERNAL_SCHEMA}} properties on the {{SchemaFactory}} and the
> {{Validator}}. Re-express it as a resolver:
> * {{SchemaFactory.newInstance(lang)}} ->
> {{SecureSchemaFactory.newInstance(lang)}}.
> * When the option is off, install only the resolver the endpoint is
> configured with. The default {{DefaultLSResourceResolver}} already serves
> {{xs:import}} / {{xs:include}} relative to the endpoint's resource URI, and
> continues to do so as the floor's delegate, so multi-file schemas are
> unaffected.
> * When the option is on, chain an allow-all {{LSResourceResolver}} _after_
> the configured one, so a schema that resolves locally still does, and
> anything else is opted in.
> Two refinements worth considering while the code is open:
> * Split the single switch into {{CamelXmlValidatorAccessExternalDTD}} and a
> new {{CamelXmlValidatorAccessExternalSchema}}, mirroring the JAXP distinction
> between {{ACCESS_EXTERNAL_DTD}} and {{ACCESS_EXTERNAL_SCHEMA}}. Today one
> option controls both, which forces a deployment that needs a remote schema to
> also accept remote DTDs.
> * The allow-all resolver must return an {{LSInput}} carrying *identifiers
> only*, with no stream attached, so the parser opens the resource itself.
> Opening it inside the resolver forces the checked {{IOException}} to be
> swallowed (the interface declares none), which converts a real fetch failure
> into silent empty content. This is not theoretical: it is what
> {{ValidatorDtdAccessOnTest}} detects, because it asserts on the
> {{UnknownHostException}} reaching the route.
> {{ValidatingProcessor}} should keep setting {{ACCESS_EXTERNAL_*}} on the
> non-opt-in path: the {{Schema}} may come from a user-injected
> {{SchemaFactory}}, which Camel cannot secure, and that path would otherwise
> lose its only protection.
> Sites: {{SchemaReader}}, {{ValidatingProcessor}}, {{XmlSignatureProcessor}},
> {{JaxbDataFormat}} (see _Follow-ups_).
> h2. 5. TrAX
> The most wiring, because the protection works differently here.
> Commons Secure XML secures a transform by *replacing every parser-less source
> with one it owns*: a {{StreamSource}}, or a {{SAXSource}} whose
> {{getXMLReader()}} returns null, is rewritten to carry a secure {{XMLReader}}
> before the implementation ever sees it. A {{DOMSource}}, or a {{SAXSource}}
> that already carries the caller's own reader, is passed through untouched, on
> the assumption that the caller has taken responsibility for it. The same
> rewrite is applied to sources handed back by a {{URIResolver}}, so an
> opted-in {{xsl:include}} is parsed on the same floor as everything else.
> Camel can and should do the same thing for the sources it constructs itself,
> so that a body converted to a {{Source}} by the type converter arrives at the
> transform already carrying a secure reader rather than relying on the TrAX
> implementation to add one: {{XmlConverter.toSAXSource}} / {{toStreamSource}}
> and {{XsltBuilder}} are the places.
> Notes:
> * {{XsltUriResolver}} already serves {{xsl:include}}, {{xsl:import}} and
> {{document()}} over {{classpath:}}, {{file:}}, {{http:}}, {{ref:}} and
> {{bean:}}, and keeps working as the floor's delegate.
> * A top-level URI the caller passes is fetched as-is by the implementation;
> only follow-up references go through the floor. Camel already resolves the
> top-level stylesheet itself through {{XsltUriResolver}}
> ({{XsltEndpoint.loadResource}}), so the opt-in policy applied to the main
> stylesheet must be applied to its includes too. That is what the existing
> resolver does, and it is the property to protect in review.
> * The guarantees govern what a transform reads, not what it writes.
> {{xsl:result-document}} still writes wherever the stylesheet says, so an
> untrusted stylesheet remains a route-author concern.
> * {{XmlConverter.configureSaxonTransformerFactory}} and
> {{StaxConverter.isWoodstox}} both identify the implementation by
> {{factory.getClass().getName()}}, which becomes the wrapper. Both need
> restructuring: probe the classpath and let {{setAttribute}} reject a
> non-Saxon factory (attributes are delegated, so a real Saxon still gets
> configured), and resolve the StAX implementation once from a plain factory.
> Sites: {{XmlConverter.createTransformerFactory()}}, {{XMLConverterHelper}},
> {{XsltBuilder}}, {{CachedCxfPayload}}, {{SpringWebserviceHelper}},
> {{TikaProducer}}, {{FopProducer}}, {{RouteCoverageMojo}}.
> h1. Threat model
> {{security-model.adoc}} must gain a list of the settings that *invalidate the
> guarantees*, alongside the existing XXE and SSRF sections. A guarantee that
> can be switched off silently is not a guarantee, and these are the switches:
> || Setting || Effect ||
> | {{org.apache.camel.xmlconverter.documentBuilderFactory.feature:<uri>}} with
> {{external-general-entities}}, {{external-parameter-entities}} or
> {{load-external-dtd}} set to {{true}} | re-enables external entity resolution
> on the DOM path |
> | {{org.apache.camel.xmlconverter.documentBuilderFactory.feature:<uri>}} with
> {{disallow-doctype-decl}} set to {{false}} | permits a DOCTYPE; external
> references stay blocked unless a resolver opts them in |
> | {{CamelXmlValidatorAccessExternalDTD}} (and a future
> {{CamelXmlValidatorAccessExternalSchema}}) | allows the validator to fetch
> external DTDs and schemas |
> | a caller-supplied {{DocumentBuilderFactory}}, {{TransformerFactory}} or
> {{SchemaFactory}} ({{XmlConverter.setDocumentBuilderFactory}},
> {{validator:...?schemaFactory=#ref}}, the {{CamelDocumentBuilderFactory}}
> exchange property) | bypasses the floor entirely; the library exposes no way
> to wrap an existing instance |
> | a caller-installed {{EntityResolver}} / {{URIResolver}} /
> {{LSResourceResolver}} / {{XMLResolver}} | opts in whatever it resolves; the
> floor only covers what the resolver declines |
> The last two are the important ones, because they are the extension points
> the framework offers by design. The threat model should state plainly that a
> factory or resolver supplied by the deployment is the deployment's
> responsibility, which is consistent with operators and route authors being
> trusted, but is currently implicit.
> h1. Dependency
> {{org.apache.commons:commons-secure-xml:1.0.0}}, released to Maven Central in
> September 2026. Java 8 target, {{Multi-Release}} jar, OSGi bundle
> {{org.apache.commons.xml.secure}}, Apache 2.0. It has *no transitive compile
> dependencies*; Saxon-HE is {{provided}} and imported optionally. Managed in
> {{parent/pom.xml}} as {{commons-secure-xml-version}}; {{camel-dependencies}}
> picks the property up automatically and neither the BOM nor the catalog needs
> an edit.
> This would be the first third-party compile dependency in
> {{core/camel-xml-jaxp}}. The whole {{core/}} tree currently carries four
> (slf4j-api, jspecify, JAXB, Jackson-YAML), so the addition needs
> justification on the record: it removes roughly 120 lines of security code
> whose divergence is the direct cause of four accepted CVEs, and replaces it
> with a guarantee that does not depend on which parser the deployment happens
> to ship.
> h1. Follow-ups, deliberately out of scope
> * *Saxon-selecting components*: {{camel-xslt-saxon}}, {{camel-xj}},
> {{camel-schematron}} and {{camel-xpath}}'s Saxon path all promise a specific
> implementation class ({{transformerFactoryClass}}, {{saxon=true}},
> {{objectModelUri}}) and then act on {{instanceof}} or a class-name check. A
> wrapper makes those checks fail *silently*, so {{saxonConfiguration}},
> {{saxonExtensionFunctions}}, {{secureProcessing}} and Saxon's
> {{linenumbering}} attribute would stop being applied with no error. These
> stay on their current hardening until Commons Secure XML offers a public
> {{wrap(TransformerFactory)}} / {{unwrap()}} pair, which is worth raising
> upstream as a feature request.
> * *camel-jaxb* compiles multi-file schemas with no {{LSResourceResolver}},
> relying on its {{accessExternalSchemaProtocols}} option. Securing it before
> giving it a resolver would turn "schema fails to load loudly" into "schema
> silently validates nothing", which is worse than the current behaviour. It
> needs the resolver first.
> * *camel-jbang, camel-kamelet-main, tooling*: three copied private
> secure-factory helpers, all missing {{disallow-doctype-decl}} and all
> swallowing exceptions, parsing POMs fetched from remote repositories, POMs
> from arbitrary third-party jars, and MCP-client-supplied strings. The
> least-trusted input in the repository, and the cleanest win once the core
> pattern is settled.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
