Skip to content
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

XXE in XML Parser #1056

Closed
prodigysml opened this issue Jul 19, 2018 · 9 comments
Closed

XXE in XML Parser #1056

prodigysml opened this issue Jul 19, 2018 · 9 comments
Assignees
Labels
🐞 bug issue is a bug 📦 rio affects the Rio RDF Parser/Writer toolkit security
Milestone

Comments

@prodigysml
Copy link

The Issue

An XML External Entity attack is a type of attack against an application that parses XML input. This attack occurs when XML input containing a reference to an external entity is processed by a weakly configured XML parser. This attack may lead to the disclosure of confidential data, denial of service, server side request forgery, port scanning from the perspective of the machine where the parser is located, and other system impacts.

Where the Issue Occurred

The following code snippets display the usage of XMLReader without disabling entities:

saxParser.parse(inputStreamOrReader);

@barthanssens
Copy link
Contributor

org.eclipse.rdf4j.rio.rdfxml.RDFXMLParser.java sets the XMLParserSettings.SECURE_PROCESSING (line 270), not sure if that covers all the settings mentioned in the OWASP cheat sheet

The TrixParser does not seem to do this

@barthanssens
Copy link
Contributor

IMHO the TrixParser and RDFXMLParser should be reworked to extend a common AbstractRDFParserXML, there seems to be some code duplication (and I assume it would be better to fix this issue in this common parent)

@ruchim
Copy link

ruchim commented Sep 6, 2018

Is there any chance this vulnerability will be patched sometime soon?

@barthanssens
Copy link
Contributor

a quick and dirty patch shouldn't be too hard, (testing can be a bit cumbersome though), but a more elegant solution as mentioned above is going to take a bit longer :-/

@ruchim
Copy link

ruchim commented Sep 7, 2018

I think a patch should work well enough for us! Is it possible to provide an estimate on when you think a patch could be released?

@kshakir
Copy link

kshakir commented Sep 23, 2018

Here are changes that copy-paste the code from @jeenbroekstra link, plus unit tests.

master...kshakir:xxe_xmlparser

I'll try to figure out the CLA stuff, but if anyone else wants to submit the changes please go ahead.

@abrokenjester abrokenjester added the 🐞 bug issue is a bug label Oct 24, 2018
@abrokenjester abrokenjester added this to the 2.4.1 milestone Oct 24, 2018
@abrokenjester abrokenjester added the 📦 rio affects the Rio RDF Parser/Writer toolkit label Oct 24, 2018
@abrokenjester abrokenjester self-assigned this Oct 24, 2018
@abrokenjester
Copy link
Member

A third place we also need to look at is the SparqlXmlResultsReader. I will try and make these settings configurable (with secure defaults) so that parsing files that legit use doctypes and (heaven forbid) external entities or external dtds is still possible.

abrokenjester added a commit that referenced this issue Oct 25, 2018
Signed-off-by: Jeen Broekstra <jeen.broekstra@gmail.com>
abrokenjester added a commit that referenced this issue Oct 29, 2018
Signed-off-by: Jeen Broekstra <jeen.broekstra@gmail.com>
barthanssens added a commit that referenced this issue Oct 29, 2018
@abrokenjester
Copy link
Member

abrokenjester commented Dec 12, 2018

@ruchim @prodigysml we've received a request to loosen some of these restrictions due to the fact that many valid RDF data sets use doctype declarations, and these restrictions suddenly make it impossible to import these files. See #1202 - if you have any views on this could you comment on that issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞 bug issue is a bug 📦 rio affects the Rio RDF Parser/Writer toolkit security
Projects
None yet
Development

No branches or pull requests

5 participants