Java: model org.apache.commons.xml XmlFactories as safe XXE sources - #10
anurag6569201 wants to merge 1 commit into
Conversation
Source PR: github#22269 Source head: 619da40
⛔ Shipwright · BlockedRecommendation: do not merge PR #10 · Tier
Findings (2)
Fireworks usage: 15,268 input · 371 output · 15,639 total tokens · $0.0036 · 8s · 0 fix iteration(s) Open the Shipwright check for full evidence and the audit bundle. Use |
![shipwright-agent[bot]](https://avatars.githubusercontent.com/u/140162738?s=60&v=4){kind=small}
| private class CommonsSecureXmlFactory extends SafeXmlFactorySource, MethodCall { | ||
| CommonsSecureXmlFactory() { | ||
| this.getMethod() | ||
| .getDeclaringType() |
There was a problem hiding this comment.
Shipwright · CRITICAL
CommonsSecureXmlFactory matches all overloads of newInstance/newFactory on the secure factory classes without constraining return type or parameters.
Impact: CommonsSecureXmlFactory matches all overloads of newInstance/newFactory on the secure factory classes without constraining return type or parameters. Overloads accepting a caller-supplied factory class name and ClassLoader (e.g. SecureDocumentBuilderFactory.newInstance(String, ClassLoader), SecureSchemaFactory.newInstance(String, String, ClassLoader)) are treated as safe XXE sources even though they can return an ar…
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| private class CommonsSecureXmlFactory extends SafeXmlFactorySource, MethodCall { | ||
| CommonsSecureXmlFactory() { | ||
| this.getMethod() | ||
| .getDeclaringType() |
There was a problem hiding this comment.
Shipwright · HIGH
The new 'CommonsSecureXmlFactory' class matches any method named 'newInstance', 'newDefaultInstance', 'newNSInstance', 'newDefaultNSInstance', 'newFactory', or 'newDefaultFactory'
Impact: The new 'CommonsSecureXmlFactory' class matches any method named 'newInstance', 'newDefaultInstance', 'newNSInstance', 'newDefaultNSInstance', 'newFactory', or 'newDefaultFactory' declared on the secure factory classes, without constraining the return type. This includes overloads such as 'SecureDocumentBuilderFactory.newInstance(String, ClassLoader)' and 'SecureSchemaFactory.newInstance(String, String, ClassLoader)…
Suggested fix: Fix the review finding before release.
1 participant
Description
We are about to publish the first release of Apache Commons XML, a new Apache Commons component that provides secure-by-default creation of JAXP factories. Its
org.apache.commons.xml.XmlFactoriesentry point returns factories that are hardened against XXE and SSRF regardless of which JAXP implementation is on the classpath (JDK, Android, Xalan, Xerces, Woodstox, Saxon-HE). This differs from the usual approach of setting implementation-specific hardening features on whatever factorynewInstance()returns: when the implementation does not support a given feature, that approach silently fails open, whereas Commons XML detects the implementation and applies a hardening strategy known to work for it, failing closed otherwise.Beyond the technical guarantee, an equally important goal of the library is to reduce the security-report noise around XML parsing of trusted documents, such as configuration files. Maintainers of libraries that parse such files regularly receive reports of alleged XXE vulnerabilities, and each SAST tool expects a slightly different set of hardening features before it stops flagging a JAXP factory call. Centralizing the hardening in one well-audited place gives maintainers a single answer to those reports, and gives SAST tools a single API to recognize.
Why open this PR before the first release?
That last point is exactly why we are opening this PR now rather than after adoption picks up. The library only delivers its value if security tooling recognizes it: otherwise every adoption creates alerts instead of removing them. We saw this with the library's prototype,
copernik-xml-factory: when I tested it in the Apache Log4j project I maintain, CodeQL raised an XXE alert on the very code the library exists to make safe (apache/logging-log4j2#4144 (comment)), and every future adopter would have to triage and dismiss the same false positive or ship a custom model pack.Modeling the library ahead of its 0.1.0 release means early adopters get correct CodeQL results from day one. The API surface modeled here (the six static
newXxxFactory()methods) is final for the first release, so the model is not chasing a moving target and the apache/commons-secure-xml#32 API correction I proposed today does not require changes in the CodeQL code.Concretely: every factory returned by
org.apache.commons.xml.XmlFactoriesis already hardened against XML external entity (XXE) attacks, but the XXE query (java/xxe, CWE-611) does not know this and reports parsers created from these factories as vulnerable.This PR teaches the XXE query that such factories are safely configured:
SafeXmlFactorySourcetosemmle.code.java.security.XmlParsers, representing an expression that evaluates to a JAXP factory that is already hardened against XXE (for example by a helper library). It is wired into the five existing safe-factory flow sources (DocumentBuilderFactory,SAXParserFactory,XMLInputFactory,TransformerFactory,SchemaFactory), dispatching on the static type of the expression.semmle.code.java.frameworks.apache.CommonsXml(which already models Commons Digester for XXE) with aSafeXmlFactorySourcesubclass matching calls to theXmlFactories.newXxxFactory()methods.newXPathFactoryis matched for completeness, but the XXE model has noXPathFactorysafety chain (the XXE sink for XPath is the document being evaluated, not the factory), so it currently has no effect on results.Note
I do not write QL myself. This PR was created with the help of Claude Code (
claude-opus-4-8andclaude-fable-5). I have reviewed every change semantically, that is, which calls are modeled and why they are safe to treat as hardened, but I cannot vouch for QL idiom or style, so feedback on that level is especially welcome.Tests
java/ql/test/query-tests/security/CWE-611/XmlFactoriesTests.javacovering parsers created from each of the hardened factories (direct, chained, and viaXMLReader). All cases are safe, soXXE.expectedis unchanged; the test fails with new#selectrows if any case is still flagged.java/ql/test/stubs/apache-commons-xml-0.1.0/following the existingapache-commons-<name>-<version>naming convention, added to the CWE-611 extractor classpath.codeql test run java/ql/test/query-tests/security/CWE-611passes (1/1).Checklist
java/ql/lib/change-notes/2026-08-02-apache-commons-xml-factories.md, categoryfeaturesinceSafeXmlFactorySourceis a new public API).codeql query format --check-onlypasses).getAQlClass, or regexp matching ontoString.python config/add-overlay-annotations.py --check javapasses).Source merge-base:
5027b3627c363dd266b3e0339fd4102b7b062f8dSource head:
619da4030f84189d0503aae5cca6b338876b9306