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

Remove misleading no-op lines incorrectly trying to disable external entities #173

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

chadlwilson
Copy link

As noted in #51 (comment) these lines don't do anything - they use /properties rather than /features so throw an exception: https://xerces.apache.org/xerces-j/features.html

I can't find any reference to such (incorrect) feature keys outside DOM4J.

The correctly-named XXE-secure features are only enabled via SaxReader.createDefault() at

public static SAXReader createDefault() {
SAXReader reader = new SAXReader();
try {
reader.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
reader.setFeature("http://xml.org/sax/features/external-general-entities", false);
reader.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
} catch (SAXException e) {
// nothing to do, incompatible reader
}
return reader;
}
and require "opt-in".

These lines are rather confusing when following the code, and may mislead folks into thinking that XXE is disabled by default by the library - which I don't think they are - if you just use new SAXReader().

An alternative to this PR would be to "correct" the defaults, which would be a breaking change, and make the setFeatures in SAXReader.createDefault() irrelevant, since the same thing would have been done when the reader is created. This would risk complaints such as #51 where loading external DTDs was intentionally disabled by default. That would have the readers more "secure by default".

@ecki
Copy link

ecki commented Oct 29, 2023

Btw whatever will be changed I also think for the safe defaults it should also set FSP on the underlying parser as this will also bring some resource limits into play. For an XMLReader it would be

 SAXParserFactory spf = SAXParserFactory.newInstance();
 spf.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
 SAXParser parser = spf.newSAXParser();
 XMLReader reader = parser.getXMLReader();

as discussed here: https://mail.openjdk.org/pipermail/core-libs-dev/2016-October/044313.html

the parser is already used/created here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

SAXHelper uses invalid parser feature keys for disabling entity expansion
2 participants