You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "ppkarwasz (via GitHub)" <gi...@apache.org> on 2026/06/18 08:27:34 UTC
[PR] Harden XML parsing via copernik-xml-factory [commons-configuration]
ppkarwasz opened a new pull request, #640:
URL: https://github.com/apache/commons-configuration/pull/640
## Summary
Route every direct JAXP factory instantiation through the hardened factories from [`eu.copernik:copernik-xml-factory`](https://github.com/copernik-eu/copernik-xml-factory) `0.1.1`. These factories block external DTD and entity fetching and bound internal entity expansion, regardless of the JAXP implementation on the classpath.
Hardening the parsing of a configuration file is admittedly not necessary: configuration files are normally trusted. This limits the side-effects if a user (against advice) decides to parse untrusted configuration files.
## Changes
- Add the `copernik-xml-factory` dependency.
- Route factory creation through `XmlFactories` (`DocumentBuilderFactory`, `SAXParserFactory`, `TransformerFactory`) in `XMLConfiguration`, `XMLDocumentHelper`, `XMLPropertiesConfiguration` and `XMLPropertyListConfiguration`, plus the affected tests.
- Harden the source passed to `XMLDocumentHelper.transform`.
- `XMLConfiguration`'s entity resolver now throws a `SAXException` for unregistered entities instead of returning `null`. Returning `null` would let the parser fall back to fetching the external resource and would override the deny-all resolver the hardening factories install on some implementations. This is done in an anonymous `DefaultEntityResolver` subclass local to `XMLConfiguration`, leaving the public `DefaultEntityResolver` contract (return `null` for unknown entities) unchanged.
## Testing
- `mvn test` for the affected XML test classes: 145 tests, all passing.
- `mvn checkstyle:check`: clean.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Harden XML parsing via copernik-xml-factory [commons-configuration]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #640:
URL: https://github.com/apache/commons-configuration/pull/640#discussion_r3448682732
##########
pom.xml:
##########
@@ -96,6 +96,11 @@
</site>
</distributionManagement>
<dependencies>
+ <dependency>
+ <groupId>eu.copernik</groupId>
Review Comment:
```suggestion
<groupId>org.apache.commons</groupId>
<artifactId>commons-xml</artifactId>
<version>0.1.0-SNAPSHOT</version>
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Harden XML parsing via copernik-xml-factory [commons-configuration]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on PR #640:
URL: https://github.com/apache/commons-configuration/pull/640#issuecomment-5471086395
This was replaced by #647
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Harden XML parsing via copernik-xml-factory [commons-configuration]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on PR #640:
URL: https://github.com/apache/commons-configuration/pull/640#issuecomment-4741152433
> This was theoretically fixed by [JDK-8159139](https://bugs.openjdk.org/browse/JDK-8159139), but still doesn't work.
Apparently OpenJDK never got the fix, probably the fix is only present in Oracle's builds. This mean that on JDK 8 parsing this file will always **fail** if `ACCESS_EXTERNAL_SCHEMA` does not contain `https`. This happens even if an `EntityResolver` is set.
```xml
<configuration
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:noNamespaceSchemaLocation="https://commons.apache.org/testMultiConfiguration.xsd">
...
</configuration>
```
This is a bummer, because on JDK 9+ libraries can choose to call `DocumentBuilderFactory.newDefaultInstance()`, which solves the problem of implementation-specific hardenings altogether. Almost: Android's default instance will be different from the one in the JDK and will not accept the same options.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Harden XML parsing via copernik-xml-factory [commons-configuration]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #640:
URL: https://github.com/apache/commons-configuration/pull/640#issuecomment-4741020962
Shading can cause more problems than it's worth as exemplified with the mess some projects make shading ASM. When downstream users blow up because they can't update ASM to test or support a new Java version since that code is now a hard copy 🤔
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Harden XML parsing via copernik-xml-factory [commons-configuration]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on PR #640:
URL: https://github.com/apache/commons-configuration/pull/640#issuecomment-4741069368
> Shading can cause more problems than it's worth as exemplified with the mess some projects make shading ASM.
I totally agree, but each project should be able to choose its poison.
Unlike ASM, however, XML support is unlikely to change very much in the future, so if a project decides to replace a bunch of cargo-culted hardening options without any tests, with a shaded and relocated class, this still counts as a win.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Harden XML parsing via copernik-xml-factory [commons-configuration]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #640:
URL: https://github.com/apache/commons-configuration/pull/640#issuecomment-4741633033
Here we go:
- https://github.com/apache/commons-xml
- I asked for an COMMONSXML Jira project but it's not there yet. Note that there already is a COMMONSXML Jira project but I can't tell what that's part of, maybe Xerces.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Harden XML parsing via copernik-xml-factory [commons-configuration]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #640:
URL: https://github.com/apache/commons-configuration/pull/640#issuecomment-4740841825
-1: We've already discussed making an new Commons component for this code, let's do that please.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Harden XML parsing via copernik-xml-factory [commons-configuration]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #640:
URL: https://github.com/apache/commons-configuration/pull/640#issuecomment-4741116033
Thanks for the clarifications Piotr. Let's bring the component into Commons and iterate there. It will become more visible and it doesn't need to be perfect or bug free on the first commit. We'll want to create an alpha or milestone or pick-your-name release before a 1.0 anyway. WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Harden XML parsing via copernik-xml-factory [commons-configuration]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on PR #640:
URL: https://github.com/apache/commons-configuration/pull/640#issuecomment-4740354379
Converting this to draft, since some Java-version dependent problems arose.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Harden XML parsing via copernik-xml-factory [commons-configuration]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz closed pull request #640: Harden XML parsing via copernik-xml-factory
URL: https://github.com/apache/commons-configuration/pull/640
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Harden XML parsing via copernik-xml-factory [commons-configuration]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on PR #640:
URL: https://github.com/apache/commons-configuration/pull/640#issuecomment-4741025605
JDK 8 is affected by a bug, which denies access based on `ACCESS_EXTERNAL_SCHEMA` **before** consulting the resolver. This is why tests with `xsi:schemaLocation`/`xsi:noNamespaceSchemaLocation` hints fail.
This was theoretically fixed by [JDK-8159139](https://bugs.openjdk.org/browse/JDK-8159139), but still doesn't work.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Harden XML parsing via copernik-xml-factory [commons-configuration]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #640:
URL: https://github.com/apache/commons-configuration/pull/640#discussion_r3448682732
##########
pom.xml:
##########
@@ -96,6 +96,11 @@
</site>
</distributionManagement>
<dependencies>
+ <dependency>
+ <groupId>eu.copernik</groupId>
Review Comment:
```suggestion
<groupId>org.apache.commons</groupId>
<artifactId>commons-xml</artifactId>
<version>0.1.0-SNAPSHOT</version>
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Harden XML parsing via copernik-xml-factory [commons-configuration]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on PR #640:
URL: https://github.com/apache/commons-configuration/pull/640#issuecomment-4740937637
> -1: We've already discussed making an new Commons component for this code, let's do that please.
Sorry, I was busy iterating on code and I forgot to mention that I created this PR only to test the capabilities (or missing capabilities) of `copernik-xml-factory`, **before** it gets included as a separate Commons component.
For a new component to be useful, it should be adopted by at least a couple of projects. Currently I have tested support for such a library:
- In Log4j (apache/logging-log4j2#4144), where it appears that the library should have separate self-contained public classes, so that code necessary by a single JAXP implementation can be shaded. This does not exclude the presence of an `XmlFactories` common utility,
- In Batik ([BATIK-1395](https://issues.apache.org/jira/browse/BATIK-1395)), where the idea of replacing stock JDK-specific hardenings with an external implementation-independent library didn't find good ground,
- Calcite ([CALCITE-7609](https://issues.apache.org/jira/browse/CALCITE-7609)) is interested, so I will test how well adapted the library is to their code,
- In this PR I am exploring the applicability of the library to Configuration. I already found one bug (copernik-eu/copernik-xml-factory#28) and a problem with JDK 8 and schema validation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Harden XML parsing via copernik-xml-factory [commons-configuration]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on PR #640:
URL: https://github.com/apache/commons-configuration/pull/640#issuecomment-4741163769
> We'll want to create an alpha or milestone or pick-your-name release before a 1.0 anyway. WDYT?
Sounds good to me: please choose a repository name and I'll transfer the code to Commons. Do we need a new JIRA project too?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org