-
-
Notifications
You must be signed in to change notification settings - Fork 79
Fix SchemaFactory configuration of XML validator #737
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
66161f8 to
658183c
Compare
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds XXE (XML External Entity) protection to prevent security vulnerabilities when parsing XML BOMs.
- Added security properties to
SchemaFactoryto disable external DTD and schema access - Implemented a comprehensive test that validates XXE protection by attempting an actual network connection
- Updated version from 11.1.0-SNAPSHOT to 11.0.1-SNAPSHOT (patch release)
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/org/cyclonedx/CycloneDxSchema.java | Added XXE protection by restricting external DTD and schema access |
| src/test/java/org/cyclonedx/parsers/XmlParserTest.java | Added test with server socket to verify XXE protection works correctly |
| src/test/resources/security/xxe-protection.xml | Updated port number to 6666 to match the test implementation |
| pom.xml | Changed version from 11.1.0-SNAPSHOT to 11.0.1-SNAPSHOT |
| README.md | Updated version reference to 11.0.1 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| final AtomicBoolean receivedConnection = new AtomicBoolean(false); | ||
|
|
||
| final ExecutorService executor = Executors.newSingleThreadExecutor(); | ||
| try (final ServerSocket serverSocket = new ServerSocket(6666)) { |
Copilot
AI
Nov 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hardcoded port 6666 may conflict with existing services or fail in restricted environments. Consider using port 0 to let the OS assign an available ephemeral port, then retrieve it with serverSocket.getLocalPort() and update the test XML dynamically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to prevent unintended network-block foo, we decided to go with file access the test here: https://github.com/CycloneDX/cyclonedx-javascript-library/blob/1749c7066224c9ee99c686093b6b53db4c80ca73/tests/functional/internals/OpPlug.node.xmlValidator.implementation.spec.js#L74-L90
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modified the fixture to reference a file path instead. The tests now assert on specific failure messages indicating that external resolution was blocked. Whether or not the file actually exists in then no longer relevant.
658183c to
4a0121c
Compare
|
|
||
| public Schema getXmlSchema(InputStream... inputStreams) throws SAXException { | ||
| final SchemaFactory schemaFactory = SchemaFactory.newInstance(XMLConstants.W3C_XML_SCHEMA_NS_URI); | ||
| schemaFactory.setProperty(XMLConstants.ACCESS_EXTERNAL_DTD, ""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this actually prevent XEE? or does it only prevent unintended network access?
what about XMLConstants.FEATURE_SECURE_PROCESSING ?
what about schemaFactory.setExpandEntityReferences(false);?
or similar?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
schemaFactory.setExpandEntityReferences doesn't exist.
The ACCESS_EXTERNAL_* values are a list of protocols which are allowed, setting it to empty makes it disallow all:
- https://docs.oracle.com/javase/8/docs/api/javax/xml/XMLConstants.html#ACCESS_EXTERNAL_SCHEMA
- https://docs.oracle.com/javase/8/docs/api/javax/xml/XMLConstants.html#ACCESS_EXTERNAL_DTD
I verified that this also prevents file:// access.
I will add FEATURE_SECURE_PROCESSING though, thanks for the tip.
Signed-off-by: nscuro <[email protected]>
4a0121c to
af0ec75
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@jkowalleck May I ask for a final look please? |
|
LGTM, but I am no actual Java user. If further review is needed, let me ping some potential reviewers: @DarthHater, @stevespringett, @mr-zepol, @mtsfoni, @hboutemy, @skhokhlov, @glefloch, |
Fixes GHSA-6fhj-vr9j-g45r