[jira] [Commented] (CALCITE-7609) Use Copernik XML Factory for safely parsing XML documents

["Piotr Karwasz (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/CALCITE-7609?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18090269#comment-18090269 ] 

Piotr Karwasz commented on CALCITE-7609:
----------------------------------------

[~zabetak],
Thanks for opening this discussion.

A quick update first: [~ggregory] has created a [commons-xml repository|https://github.com/apache/commons-xml], so development of the library will continue under Commons.

There are real pros and cons to weigh when deciding whether to replace 10-20 lines of code with an external dependency. They are exactly the ones I weighed when writing the library.

XML in Java is unlike other formats in two respects:
* It is *pluggable*: the classpath can influence which implementation is selected. Since Java 9 you can opt out via the {{newDefaultInstance()}} methods, but most of our libraries will keep Java 8 compatibility until its EOL in 2030.
* An implementation ships with the JDK. For a long time it *trailed* the standalone Xerces and Xalan projects, but those are now largely dormant, so the JDK implementations are often the better choice today. The JDK still lags third-party options for {{TransformerFactory}} (Saxon) and {{XMLInputFactory}} (Woodstox).

So there are three ways to approach XML processing:
* use the pluggable factories;
* pin to the JDK-provided factories (by requiring Java 9+ or via reflection);
* use an external library.

h2. Per-project XML helpers

Each project writing its own helpers to build secure factories is the current _status quo_.

h3. Pros
* the hardening code has been around for decades;
* it is only a dozen lines;
* no external dependency.

h3. Cons
* it relies on features that not every pluggable implementation supports;
* hardening may be needed not only on the JAXP factory, but also on the objects it creates. For example:
** configuring {{SAXParserFactory}} is not enough; each {{XMLReader}} it produces must be configured too;
** {{TransformerFactory.newTransformer(Source)}} and {{Transformer.transform(Source, Result)}} can fall back to a default parser when the {{Source}} carries none (e.g. a {{StreamSource}});
* the hardening code usually has no tests;
* the Java 9 "default" factories largely solve the problems above, with the notable exception of {{XMLInputFactory}}, as Tika learned the hard way recently;
* the Java 9 "default" factories on Android differ from those on the JVM and do not support the same features.

h2. Commons XML library

The library aims to be easy to use, so it wraps factories when a hardening step must be deferred.

h3. Pros
* hardening is guaranteed, or the helper throws on an unsupported implementation;
* hardening is tested;
* hardening lives only at factory-creation level: every object the factory produces is safe to use unconfigured.

h3. Cons
* it is an *external* dependency;
* even though we intend to make individual hardening recipes easier to shade separately, the library will be around 40-50 KiB for "a few lines of code".

h2. Preventing regressions

Whichever option we pick, we can keep the raw bootstraps out of the codebase with [forbidden-apis|https://github.com/policeman-tools/forbidden-apis], which Calcite already runs. A small signatures file banning {{DocumentBuilderFactory.newInstance()}}, {{TransformerFactory.newInstance()}}, {{XPathFactory.newInstance()}} and the rest of the {{newInstance}}/{{newFactory}} family would fail the build on any new direct use, leaving the helper (or {{commons-xml}}) as the only sanctioned entry point.

> Use Copernik XML Factory for safely parsing XML documents
> ---------------------------------------------------------
>
>                 Key: CALCITE-7609
>                 URL: https://issues.apache.org/jira/browse/CALCITE-7609
>             Project: Calcite
>          Issue Type: Task
>          Components: core
>            Reporter: Stamatis Zampetakis
>            Priority: Major
>
> There are a few places in the project where we manipulate XML documents (notably inside the \{{XmlFunctions}} class). The default settings for the various XML APIs provide weak security guarantees leading to vulnerabilities like the one we had to address in CALCITE-5263.
> The [copernik-xml-factory|https://github.com/copernik-eu/copernik-xml-factory] library provides various APIs returning factory instances with tighter security guarantees and could be used instead of calls to the raw APIs.
> The goal of this ticket is to investigate if its worth introducing the copernik library to the project for hardening the security around XML parsing.
> If we decide to move forward with this change we should also find a way to prevent the usage of the raw APIs (and always pass through copernik) otherwise sooner or later we will end up in the same situation.



--
This message was sent by Atlassian Jira
(v8.20.10#820010)